Linaro-mm-sig

linaro-mm-sig@lists.linaro.org

14 participants
3627 discussions

Re: [PATCH v7 0/4] dma-buf: fix fd leak when copy_to_user() fails after fd_install()

by Christian König

I've gone ahead, added one more rb and pushed the result to drm-misc-fixes. Thanks for the help, Christian. On 8/17/26 07:04, Baineng Shou wrote: > Several drivers call dma_buf_fd() — which internally calls fd_install() > — before copy_to_user() returns the fd number to userspace. If > copy_to_user() fails, the fd is already published in the caller's fd > table but the ioctl returns an error, so userspace never learns the fd > number. Worse, the window between fd_install() and copy_to_user() > allows other threads to observe and manipulate the fd (dup, close, > SCM_RIGHTS), making any "close it on the failure path" fix unsafe. > > The fix is to split the allocation into three steps: reserve an fd with > get_unused_fd_flags() (not yet visible to other threads), do > copy_to_user(), and only then publish the fd with fd_install() via the > new dma_buf_fd_install() helper. On copy_to_user() failure, > put_unused_fd() + dma_buf_put() cleanly unwind with no user-visible > side effects. > > Patch 1 introduces dma_buf_fd_install() in dma-buf.c (wrapping > fd_install() together with the DMA_BUF_TRACE call to preserve export > tracing) and applies the fix to dma-heap. > > Patch 2 applies the same fix to fastrpc, which even had a comment > acknowledging the problem could not be fixed before. > > Patch 3 replaces the bare fd_install() in drm_gem_prime_handle_to_fd() > with dma_buf_fd_install() to restore tracepoint coverage for DRM PRIME > exports (suggested by Christian König). > > Patch 4 adds a selftest to tools/testing/selftests/dmabuf-heaps/ that > reproduces the fd-leak scenario (mprotect flip before the ioctl) and > verifies the fd count is unchanged after a failed ioctl (suggested by > Sumit Semwal). > > v1: https://lore.kernel.org/dri-devel/20260703080922.1838362-1-shoubaineng@gmai… > v2: https://lore.kernel.org/dri-devel/20260710105740.3080070-1-shoubaineng@gmai… > v3: https://lore.kernel.org/dri-devel/20260714114654.3885457-1-shoubaineng@gmai… > v5: https://lore.kernel.org/dri-devel/20260730062645.233148-1-shoubaineng@gmail… > v6: https://lore.kernel.org/dri-devel/20260807101140.1357218-1-shoubaineng@gmai… > > Changes in v7: > - Add Reviewed-by: T.J. Mercier to patch 4 (selftest). > - Add Acked-by: Sumit Semwal to the whole series. > > Changes in v6: > - Rework the selftest (patch 4) per review: extract a count_open_fds() > helper, fix the copy_from_user() comment, fail (not skip) when the > ioctl does not return -1, drop the bogus mprotect-race mention, and > reword the result message. > > Changes in v5: > - Add selftest (patch 4) reproducing the fd-leak scenario (Sumit Semwal) > > Changes in v4: > - Add patch 3: drm/prime: use dma_buf_fd_install() (Christian König) > - Add Acked-by: Christian König to patches 1 and 2 > > Changes in v3: > - Split into two patches (dma-heap + fastrpc separately) > - Add dma_buf_fd_install() to preserve trace_dma_buf_fd tracepoint > - Add fastrpc fix using the new helper (T.J. Mercier) > > Baineng Shou (4): > dma-buf: dma-heap: don't publish fd before copy_to_user() succeeds > misc: fastrpc: don't publish fd before copy_to_user() succeeds > drm/prime: use dma_buf_fd_install() to preserve export tracing > selftests: dmabuf-heaps: add fd-leak-on-EFAULT regression test > > drivers/dma-buf/dma-buf.c | 20 ++++ > drivers/dma-buf/dma-heap.c | 80 ++++++------- > drivers/gpu/drm/drm_prime.c | 2 +- > drivers/misc/fastrpc.c | 16 +-- > include/linux/dma-buf.h | 1 + > .../selftests/dmabuf-heaps/dmabuf-heap.c | 113 +++++++++++++++++- > 6 files changed, 180 insertions(+), 52 deletions(-) >

1 month, 2 weeks

Re: [PATCH 2/2] drm/amdgpu/userq: hold the doorbell xa lock during hang reset

by Christian König

Hi, On 8/23/26 08:02, Junrui Luo wrote: > Hi Christian, Alex, > > Sorry for the ping. No need to look at the patch itself. > > I would just be grateful for a quick word on whether the issue it describes > is a real one. If not, I will drop it; if it is, should I send a v2? as far as I can see it is completely nonsense what you try to do here. The fence_drv references an amdgpu_userq object holds are the ones which the queue potentially waits on. When the queue is freed up those references must be dropped, but that shouldn't affect fence_drv->fences in any way possible. What exactly is the leak you are seeing? Regards, Christian. > > Thanks for your time, > Junrui Luo

1 month, 2 weeks

Re: [PATCH 0/2] fpga: add dma-buf based FPGA programming interface

by Christian König

On 8/16/26 17:07, Aravind Thokala wrote: > Some systems need to load large FPGA configuration images. The FPGA > subsystem allows loading images from the filesystem, but this requires > the entire image to be loaded into kernel memory first. For drivers > that need a DMA-capable buffer for programming, the data is then > copied again into DMA memory. This creates needless memory pressure > and delays due to the extra copy. > > This series adds dma-buf support that allows userspace to allocate a > buffer directly from a DMA heap, write the FPGA image into it, and > pass the file descriptor to the kernel via ioctl — skipping the > intermediate kernel buffer entirely. Well when you have a device with limited DMA capabilitiesthen DMA buf heaps doesn't allocate DMA-capable memory for that device either. So the explanation you give above why this interface might be useful is clearly not correct. The DMA subsystem will still do an additional copy when you try to import the DMA-buf allocated from the heap into this device. Either you need to define a heap with specific allocation restrictions (e.g. GFP_DMA32) or you allocate the DMA-buf through your fpga device so that dma_alloc_attrs() knows that a certain device needs to access the pages beforehand. Regards, Christian. > > Userspace flow: > 1. Allocate buffer from /dev/dma_heap/ (e.g., CMA heap) > 2. mmap the buffer and write the FPGA image into it > 3. ioctl(/dev/fpgaX, FPGA_IOCTL_LOAD_DMA_BUF, &dmabuf_fd) > > The dma-buf logic is implemented as a separate layer on top of the > FPGA manager, keeping buffer management separate from the write path. > Individual FPGA drivers opt in by calling fpga_dmabuf_register(). > --- > This work is based on the approach discussed in [1]. > > [1] https://lore.kernel.org/all/20231122053035.3758124-1-nava.kishore.manne@amd… > --- > Aravind Thokala (2): > fpga: Add dma-buf interface for FPGA programming > fpga: versal: add dma-buf programming support > > .../userspace-api/ioctl/ioctl-number.rst | 1 + > MAINTAINERS | 1 + > drivers/fpga/Kconfig | 9 + > drivers/fpga/Makefile | 2 + > drivers/fpga/fpga-dmabuf.c | 198 ++++++++++++++++++ > drivers/fpga/versal-fpga.c | 33 ++- > include/linux/fpga/fpga-dmabuf.h | 22 ++ > include/linux/fpga/fpga-mgr.h | 1 + > include/uapi/linux/fpga.h | 15 ++ > 9 files changed, 281 insertions(+), 1 deletion(-) > create mode 100644 drivers/fpga/fpga-dmabuf.c > create mode 100644 include/linux/fpga/fpga-dmabuf.h > create mode 100644 include/uapi/linux/fpga.h >

1 month, 2 weeks

Re: [PATCH v19 00/40] DEPT(DEPendency Tracker)

by Byungchul Park

On Thu, Aug 20, 2026 at 06:51:08PM +0100, Matthew Wilcox wrote: > On Thu, Aug 20, 2026 at 07:16:05PM +0200, David Hildenbrand (Arm) wrote: > > > Consider this real deadlock pattern that lockdep cannot detect: > > > > > > context X context Y context Z > > > > > > mutex_lock A > > > folio_lock B > > > folio_lock B <- DEADLOCK > > > mutex_lock A <- DEADLOCK > > > folio_unlock B > > > folio_unlock B > > > mutex_unlock A > > > mutex_unlock A > > > > But that really just boils down to folio lock being implemented as a PG_lock + > > some advanced wait mechanism. And we must do that because of lack of bits in > > struct page. > > > > Willy mentioned in a previous version [1]: "I don't think it makes sense to > > track lock state in the page (nor folio). Partly because there's just so many > > of them, but also because the locking rules don't really apply to individual > > folios so much as they do to the mappings (or anon_vmas) that contain folios." > > > > Given that lockdep is a debug feature, and we will at some point allocate struct > > folio separately, I assume we could just squeeze a "struct lockdep_map" in there > > in such debug configs and the world would not collapse. > > > > Doing that today (one "struct lockdep_map" in each "struct page") wouldn't work > > as mm_zero_struct_page() would not expect such large "struct page". But > > conceptually, for a debug kernel with a special CONFIG_LOCKDEP_PAGE_LOCK, maybe > > that would already be ok and we could just do that (and optimize it as we > > allocate folios separately). > > > > Not that it's ideal, but for a debug feature to at least check PG_lock, probably > > an easier way to achieve it than some completely new infrastructure. > > > > Now, Willy said "locking rules don't really apply to individual folios", I > > wonder if that could just help to also let lockdep check PG_lock with less > > metadata? (didn't fully wrap my head around the implications) > > > > [1] > > https://lore.kernel.org/all/aR3WHf9QZ_dizNun@casper.infradead.org/?utm_sour… > > There are a few things going on that make PG_lock special. Let me try > to explain again, only better this time. > > 1. The current lifetime of a struct page is the lifetime of the system. > But the semantics of its PG_lock bit change each time it is freed and > allocated. Yes, it's a classification issue that is very important. > 2. The position of PG_lock in the locking hierarchy only depend on > what the folio is currently being used for. That is, all folios in > a given xfs inode behave exactly the same from a locking perspective. You are exactly explaining what the classification means. Perfect. > There's no need to build up state about how each PG_lock is used; > they can all share. Arguably all xfs file inodes are the same as Right. That's why DEPT doesn't use a full map in each page but just uses a timestamp in each. For the classification, DEPT uses a few classes for folio, using global maps: 1. folios in mm paths 2. folios in block device buffer (meta data) 3. folios in regular file cache However, yes. I bet you could be a big help when classifying folios more presicely according to its usage. But the current classification is still a good start I think. > each other (directory inodes might be different from file inodes), > so we might want to go further than telling DEPT that "this folio > belongs to this inode" and go to "this folio belongs to this xfs file > inode". Totally agree. > 3. PG_lock can be taken in task context then released in interrupt > context. For full points, we need to mark the exact point at which > we submit the folio for read. Otherwise we can get into the situation > alluded to by f2c817bed58d and better discussed at > https://lore.kernel.org/linux-mm/20200127150024.GN1183@dhcp22.suse.cz/ > where we have the folio locked but haven't yet submitted it for I/O > so it doesn't matter how long we wait, it will never come unlocked. Interesting. The following abstraction might make DEPT work with it. For example: Annotate the point submitting IO as an event for the folio_lock() to be released. That way, the issue above can be detected by DEPT. Again, DEPT can do every thing we need w.r.t. deadlock. Byungchul

1 month, 3 weeks

Re: [PATCH v19 00/40] DEPT(DEPendency Tracker)

by Byungchul Park

On Thu, Aug 20, 2026 at 07:16:05PM +0200, David Hildenbrand (Arm) wrote: > On 7/6/26 08:18, Byungchul Park wrote: > > Hi Linus and folks, > > Hi, Hi, > I think there was plenty of feedback from locking maintainers in the past. One > question and a comment below. > > > > > DEPT(DEPendency Tracker) is a runtime deadlock detection framework that > > sees what lockdep cannot. > > > > I'm thrilled to share that DEPT has moved beyond theory and is now > > catching real deadlocks in the wild: > > > > https://lore.kernel.org/lkml/6383cde5-cf4b-facf-6e07-1378a485657d@I-love.SA… > > https://lore.kernel.org/lkml/1674268856-31807-1-git-send-email-byungchul.pa… > > https://lore.kernel.org/all/b6e00e77-4a8c-4e05-ab79-266bf05fcc2d@igalia.com/ > > > > I've added comprehensive documentation explaining DEPT's design and usage. > > Getting started is as simple as enabling CONFIG_DEPT and watching dmesg. > > > > THE PROBLEM LOCKDEP CANNOT SOLVE > > -------------------------------- > > > > Lockdep has been our trusted deadlock detector for two decades, but it > > has a fundamental blind spot: it tracks lock acquisition order, not the > > actual waits and events that cause deadlocks. This means lockdep misses: > > > > * Deadlocks involving folio locks (not released within the context) > > * Cross-context synchronization like wait_for_completion()/complete() > > * DMA fence waits, RCU waits, and general waitqueue patterns > > * Any synchronization primitive outside the classic lock/unlock model > > > > Consider this real deadlock pattern that lockdep cannot detect: > > > > context X context Y context Z > > > > mutex_lock A > > folio_lock B > > folio_lock B <- DEADLOCK > > mutex_lock A <- DEADLOCK > > folio_unlock B > > folio_unlock B > > mutex_unlock A > > mutex_unlock A > > But that really just boils down to folio lock being implemented as a PG_lock + > some advanced wait mechanism. And we must do that because of lack of bits in > struct page. > > Willy mentioned in a previous version [1]: "I don't think it makes sense to > track lock state in the page (nor folio). Partly because there's just so many > of them, but also because the locking rules don't really apply to individual > folios so much as they do to the mappings (or anon_vmas) that contain folios." Exactly. That's why we use classification e.g. lock class - DEPT also makes use of the concept. DEPT doesn't use a full map in each page but uses a minimum space for a timestamp in each to track when each starts to wait so as to use the recorded timestamp when the event occurs e.g. folio_unlock(). > Given that lockdep is a debug feature, and we will at some point allocate struct > folio separately, I assume we could just squeeze a "struct lockdep_map" in there > in such debug configs and the world would not collapse. That's a good news for lockdep. (And even for DEPT :) > Doing that today (one "struct lockdep_map" in each "struct page") wouldn't work > as mm_zero_struct_page() would not expect such large "struct page". But > conceptually, for a debug kernel with a special CONFIG_LOCKDEP_PAGE_LOCK, maybe > that would already be ok and we could just do that (and optimize it as we > allocate folios separately). Sounds great. > Not that it's ideal, but for a debug feature to at least check PG_lock, probably > an easier way to achieve it than some completely new infrastructure. I understand what you are going to tell. However, it's worth noting that lockdep tracks dependencies basically based on **lock acqusition orders** in the system. To make it track even rwlock and general synchronization mechanism as well, lockdep has no choice but to get more complicated. Focusing on only the dependency checking, the most parts of lockdep are for the tricky things, so the reusable parts are not that big. > Now, Willy said "locking rules don't really apply to individual folios", I > wonder if that could just help to also let lockdep check PG_lock with less > metadata? (didn't fully wrap my head around the implications) That's what DEPT did and what brought external wgen introduced in DEPT. I was considering the exactly same thing :) Again, lockdep that tracks lock acquisition orders can't do that. > [1] > https://lore.kernel.org/all/aR3WHf9QZ_dizNun@casper.infradead.org/?utm_sour… > > > It's your guiding example, that's why I mention it. You do mention other wait > cases here, I don't know anything about them, but for folios it's really just > "we used a single bit so far" AFAIKs. It doesn't matter whether it's implemented using bit or not. folio lock is quite special since it's allowed to be released other than the acquisition context that makes lockdep impossible to track them. > [...] > > > > > Q. Why not build DEPT into lockdep? > > > > A. Lockdep is stable, battle-tested code. I chose separation because > > while DEPT borrows BFS and hashing ideas, the wait/event model > > requires rebuilding from scratch. Lockdep was designed for lock > > acquisition order — retrofitting it would risk its stability. > > Why can't this just be some configurable extension to lockdep > (CONFIG_LOCKDEP_XYZ) until the feature is stable and can unconditionally be > enabled along with it? Answered? > I don't quite buy the "would risk its stability" argument. A lot of stuff we do > "risks stability", every day :) That's awsome anyway :) > Is there another good reason (incompatible with X, dangerous with Y, cinfusing > Z) why this really must be a separate thing? Roughly: 1. Similar or less effort is needed for the new one - retrofitting lockdep is not easy and big changes are required since the reusable parts are not that big. 2. Even though you didn't agree, retrofitting it would risk its stability. > > > > Q. Will DEPT replace lockdep? > > > > A. No. Lockdep validates correct lock usage — that's not going away. > > DEPT supersedes only the dependency-checking logic when mature. > > It's quite unfortunate that we'd end up with another similar-but-different > mechanism, that will just end up confusing people. I meant, at least dependency checking engine should be altered, but you make sense. Worth thinking it more. > But I am not a locking maintainer. I think there was plenty of discussion in the > past, so I might just be raising points that were already discussed in the past, > but I really just read some random pieces of earlier discussions. (ideally > previous discussions would be summarized here) > > Long story short: we are now in v19 and I think there was pushback in the past. > Did the opinion of locking maintainers change, or is there a way forward to > integrate this in a way that would make locking maintainers accept this? One of locking maintainers who I met in an LPC told me that he agrees with the direction of DEPT and supports DEPT, not officially tho. What he and other people are concerning w.r.t DEPT the most is, false positives, which is the most important issue for now. At the same time, I think the most important thing is to make DEPT useful in practice especially with folio locks involved. Actually, I'm planning to share DEPT's true reports periodically to LKML and work with people who believe DEPT can make things better. Any advices will be welcome. Thanks for your opinions. Byungchul > -- > Cheers, > > David

1 month, 3 weeks

[PATCH v2 0/4] Clear shared pages after private-to-shared conversion

by Steven Price

set_memory_decrypted() doesn't (currently) guarantee to preserve or zero memory, but both the GICv3 ITS driver and the system_cc_shared dma-buf heap currently allocate memory with __GFP_ZERO followed by calling set_memory_decrypted(). On an Arm CCA system with MEC this can cause ciphertext to be visible to the guest rather than the expected zeros. Patches 1 and 3 fix this by zeroing after the set_memory_decrypted() call. Patches 2 and 4 fix other related bugs that Sashiko found. Patch 2 fixes the issue that set_memory_decrypted() can be a sleeping call, so moves the allocation out of an atomic context. Patch 4 deals with the situation where set_memory_decrypted() fails and the rollback path could attempt to re-encrypt memory which was never decrypted. I've sorted the patches by area, but there's no (semantic) dependency between them. Changes in v2: * Switched to use BIT(n) rather than 1 << n in the GICv3 change. * Added Jason's R-b. * Patches 2 and 4 are new. v1: https://lore.kernel.org/r/20260820105026.53208-1-steven.price@arm.com Steven Price (4): irqchip/gic-v3-its: Zero shared pages after conversion irqchip/gic-v3-its: Allocate VPE tables from sleepable context dma-buf: heaps: Zero system shared heap pages after conversion dma-buf: heaps: Fix shared system heap allocation rollback drivers/dma-buf/heaps/system_heap.c | 24 +++++++++++++----- drivers/irqchip/irq-gic-v3-its.c | 39 ++++++++++++++++------------- 2 files changed, 40 insertions(+), 23 deletions(-) -- 2.43.0

1 month, 3 weeks

Re: [PATCH v2 03/15] accel/qda: Add initial QDA DRM accelerator driver

by Krzysztof Kozlowski

On 20/08/2026 12:07, Dmitry Baryshkov wrote: > On Thu, Aug 20, 2026 at 11:07:45AM +0200, Krzysztof Kozlowski wrote: >>>>>> >>>>>> Device node with this compatible is already populated, so this looks >>>>>> simply wrong or you are adding a duplicated driver. >>>>>> >>>>>> That's a no-go, you are supposed to work with existing drivers and grow >>>>>> them. >>>>> I'll bring the discussion again here, there was a discussion to move the >>>>> driver to accel subsystem if we want to support new features/uAPI >>>>> changes. Please read [1],[2] threads. The intention is to replace >>>>> fastrpc driver with QDA eventually. >>>> >>>> None of them address the problem. You want to grow fastrpc into user of >>>> dmabuf? So you move it from misc to here. >>> >>> It's not as easy and nice, so I think in this case it's better to repeat >> >> I disagree. The existing fastrpc driver is not that complicated. It's >> actually moderate amount of code, much less than Venus was (~7 times less). >> >> It easily can grow to support two interfaces and the only difficulty is >> how to manage these two interfaces simultaneously or exclusively, e.g. >> opening first one disables the second. > > I see the point here. > > Would it be acceptable if we add QDA support only on the new platforms > (e.g. via the SoC-specific compat), provide QDA for those platforms, > and, once it reaches complete API and feature parity, we remove the old > fastrpc driver, migrati old platforms. The problem with this approach is that we have no guarantees that it will reach feature parity in respect of old interface, thus old driver might stay forever. If we agree for duplicated driver, contributors have no incentives to support old approach. Much better is to refine the old driver, gradually adding new features while maintaining old stuff. This is the only way we can force contributors to actively work on minimizing duplicate parts. Best regards, Krzysztof

1 month, 3 weeks

Re: [PATCH 1/2] irqchip/gic-v3-its: Zero shared pages after conversion

by Jason Gunthorpe

On Thu, Aug 20, 2026 at 01:32:05PM +0100, Marc Zyngier wrote: > On Thu, 20 Aug 2026 11:50:24 +0100, > Steven Price <steven.price(a)arm.com> wrote: > > > > its_alloc_pages_node() passes __GFP_ZERO to the page allocator before > > calling set_memory_decrypted(). This assumes that converting a page from > > private to shared preserves its contents. > > > > For Arm CCA with MEC (Memory Encryption Contexts) the key used to access > > the page will change, and so by default the visible data will change. > > The host could ensure that it zeros the page, but rather than relying on > > the host's behaviour it's best if the guest simply zeros after the > > decryption rather than before. Specifically in this case the ITS tables > > are required to be zeroed. > > What are the guarantees that we want to enforce post decryption? My > recollection is that the RME firmware cleans the caches to the PoPA, > making the data immediately visible to the hypervisor. Obviously, this > isn't the case anymore, since the zeroing comes after that, and I > don't see any CMO enforcing this. I thought any CMO stuff was principally about cleaning things as part of the MEC change? Coherency after the memory is made shared should follow the normal cachable memory model rules, just like in a non-CC VM? We don't need further explicit CMOs for that. Post decryption I would expect from all architectures: 1) Neither the guest or host take a fault/error when accessing the memory. ie the host may immediately pass this memory to an O_DIRECT system call and have its kernel read from it. It must not crash the kernel. 2) So long as the memory is mapped cachable it should follow the normal memory model visibility rules. ie it works the same as VM CPU memory prior to CC 3) Rules for actual DMA are the same as prior to CC, the VM is expected to issue its own flushes prior to DMA if the platform requires it. Given the requirements for #1, is there actually any case on any platform where the host doesn't *have* to fill the memory? Is there a platform with MEC that doesn't generate an error on reading with the wrong MEC? Without MEC it surely has to be zero'd in the RMM world, right? I've argued before that set_memory_decrypted() should be defined to return 0'd memory. I think there are real systems that *have* to zero the memory as part of the state change and this API is now forcing an extra zeroing. > I'm concerned that this relies on undocumented behaviours that may > hold today on some undisclosed combinations of HW and hypervisors, but > that are not guaranteed at all. set_memory_decrypted() doesn't really > say anything, and I have the feeling that we may want some hypervisor > specific hook to perform the correct CMO magic. I don't think this is > required right now, but I'm not excluding anything! I would expect any required CMOs to be part of the arch's implementation of set_memory_decrypted()? Jason

1 month, 3 weeks

Re: [PATCH 1/2] irqchip/gic-v3-its: Zero shared pages after conversion

by Steven Price

On 20/08/2026 13:32, Marc Zyngier wrote: > On Thu, 20 Aug 2026 11:50:24 +0100, > Steven Price <steven.price(a)arm.com> wrote: >> >> its_alloc_pages_node() passes __GFP_ZERO to the page allocator before >> calling set_memory_decrypted(). This assumes that converting a page from >> private to shared preserves its contents. >> >> For Arm CCA with MEC (Memory Encryption Contexts) the key used to access >> the page will change, and so by default the visible data will change. >> The host could ensure that it zeros the page, but rather than relying on >> the host's behaviour it's best if the guest simply zeros after the >> decryption rather than before. Specifically in this case the ITS tables >> are required to be zeroed. > > What are the guarantees that we want to enforce post decryption? My > recollection is that the RME firmware cleans the caches to the PoPA, > making the data immediately visible to the hypervisor. Obviously, this > isn't the case anymore, since the zeroing comes after that, and I > don't see any CMO enforcing this. The firmware should be ensuring that things are cleaned sufficiently that the original data is inaccessible - that's required as part of the wiping when converting from private. However the wipe doesn't have to be writing zeros, indeed the RMM spec suggests that two "possible implementations" are: * The RMM (or other platform firmware) writing either random data or zeroes to the memory location * The MEC of the memory location being changed My assumption (I have to admit I haven't checked) is that the GIC code is doing sufficient CMO to ensure that the zeros that are being written after the conversion are visible to the hypervisor - but that's no different to the non-CCA case. > I'm concerned that this relies on undocumented behaviours that may > hold today on some undisclosed combinations of HW and hypervisors, but > that are not guaranteed at all. set_memory_decrypted() doesn't really > say anything, and I have the feeling that we may want some hypervisor > specific hook to perform the correct CMO magic. I don't think this is > required right now, but I'm not excluding anything! This is reducing how much Linux relies on undocumented behaviour - at the moment Linux is relying on either the zeros it has written still being visible or the firmware/hypervisor writing zeros after any private->shared transition. This patch makes the guest do it rather than relying on anything else. You have a point that a spec clarification about CMOs might be worth having - the RMM spec doesn't make clear what is required of the firmware. Clearly for security it should be doing something to ensure that the old data from the realm doesn't become visible. >> >> Mask out __GFP_ZERO from the allocation request, and do the zeroing as a >> separate step after decryption. >> >> Fixes: b08e2f42e86b ("irqchip/gic-v3-its: Share ITS tables with a non-trusted hypervisor") >> Signed-off-by: Steven Price <steven.price(a)arm.com> >> --- >> drivers/irqchip/irq-gic-v3-its.c | 7 ++++++- >> 1 file changed, 6 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c >> index 6f5811aae59c..c954bbe9f4db 100644 >> --- a/drivers/irqchip/irq-gic-v3-its.c >> +++ b/drivers/irqchip/irq-gic-v3-its.c >> @@ -213,10 +213,12 @@ static gfp_t gfp_flags_quirk; >> static struct page *its_alloc_pages_node(int node, gfp_t gfp, >> unsigned int order) >> { >> + bool want_zero = gfp & __GFP_ZERO; >> struct page *page; >> int ret = 0; >> >> - page = alloc_pages_node(node, gfp | gfp_flags_quirk, order); >> + page = alloc_pages_node(node, (gfp & ~__GFP_ZERO) | gfp_flags_quirk, >> + order); >> >> if (!page) >> return NULL; >> @@ -231,6 +233,9 @@ static struct page *its_alloc_pages_node(int node, gfp_t gfp, >> if (ret) >> return NULL; >> >> + if (want_zero) >> + clear_pages(page_address(page), 1 << order); >> + > > nit: please use BIT(order), which matches the type required for > clear_pages(). Sure, this was matching the use in set_memory_decrypted(), but I can update that too. > But I'd really like some discussion about the CMO side of things. I'm not sure what more to say about CMO - if you want changes in the commit message(s) then please suggest something. AFAICT this patch doesn't change anything about cache maintenance. You're welcome to raise spec clarifications if you want to. Thanks, Steve PS. I'll take a look at the Sashiko comments - but they are both pre-existing issues not issues with this series.

1 month, 3 weeks

Re: [PATCH v2 03/15] accel/qda: Add initial QDA DRM accelerator driver

by Krzysztof Kozlowski

On 20/08/2026 10:52, Dmitry Baryshkov wrote: > On Wed, Aug 19, 2026 at 04:18:51PM +0200, Krzysztof Kozlowski wrote: >> On 19/08/2026 15:17, Ekansh Gupta wrote: >>> On 19-08-2026 00:40, Krzysztof Kozlowski wrote: >>>> On 17/08/2026 06:47, Ekansh Gupta wrote: >>>>> Add the skeleton of the Qualcomm DSP Accelerator (QDA) driver, a DRM >>>>> accel driver for the Hexagon DSPs found on Qualcomm SoCs. >>>>> >>>>> This patch registers a DRM accel device, exposing a /dev/accel/accelN >>>>> character device node, and binds it to the RPMsg channel used to reach >>>>> the DSP. Buffer management, IOMMU context banks and the FastRPC >>>>> protocol are added by later patches in this series. >>>>> >>>>> qda_drv.c / qda_drv.h define the drm_driver ops table, the per-file >>>>> private state (qda_file_priv) and the main device structure (qda_dev), >>>>> which embeds drm_device so that it can be recovered with container_of(). >>>>> >>>>> qda_rpmsg.c binds to the "qcom,fastrpc" compatible via >>>>> module_rpmsg_driver(), reads the DSP domain name from the "label" >>>>> device-tree property, and registers the DRM device. >>>>> >>>>> Assisted-by: Claude:claude-sonnet-5 >>>>> Signed-off-by: Ekansh Gupta <ekansh.gupta(a)oss.qualcomm.com> >>>>> --- >>>>> Changes in v2: >>>>> - Use module_rpmsg_driver() and drop the qda_rpmsg_register()/ >>>>> _unregister() wrappers, module_init()/module_exit() and >>>>> qda_rpmsg.h entirely (Dmitry Baryshkov) >>>>> - Read the "label" property directly into qdev->dsp_name (Dmitry Baryshkov) >>>>> - Drop the probe/remove/init log messages (Dmitry Baryshkov) >>>>> - Return the result of qda_register_device() directly (Dmitry Baryshkov) >>>>> - Clarify the Kconfig help text (Dmitry Baryshkov) >>>>> --- >>>>> drivers/accel/Kconfig | 1 + >>>>> drivers/accel/Makefile | 1 + >>>>> drivers/accel/qda/Kconfig | 30 ++++++++++++++++ >>>>> drivers/accel/qda/Makefile | 10 ++++++ >>>>> drivers/accel/qda/qda_drv.c | 71 ++++++++++++++++++++++++++++++++++++++ >>>>> drivers/accel/qda/qda_drv.h | 61 +++++++++++++++++++++++++++++++++ >>>>> drivers/accel/qda/qda_rpmsg.c | 79 +++++++++++++++++++++++++++++++++++++++++++ >>>>> 7 files changed, 253 insertions(+) >>>>> >>>>> diff --git a/drivers/accel/Kconfig b/drivers/accel/Kconfig >>>>> index bdf48ccafcf2..74ac0f71bc9d 100644 >>>>> --- a/drivers/accel/Kconfig >>>>> +++ b/drivers/accel/Kconfig >>>>> @@ -29,6 +29,7 @@ source "drivers/accel/ethosu/Kconfig" >>>>> source "drivers/accel/habanalabs/Kconfig" >>>>> source "drivers/accel/ivpu/Kconfig" >>>>> source "drivers/accel/qaic/Kconfig" >>>>> +source "drivers/accel/qda/Kconfig" >>>>> source "drivers/accel/rocket/Kconfig" >>>>> >>>>> endif >>>>> diff --git a/drivers/accel/Makefile b/drivers/accel/Makefile >>>>> index 1d3a7251b950..58c08dd5f389 100644 >>>>> --- a/drivers/accel/Makefile >>>>> +++ b/drivers/accel/Makefile >>>>> @@ -5,4 +5,5 @@ obj-$(CONFIG_DRM_ACCEL_ARM_ETHOSU) += ethosu/ >>>>> obj-$(CONFIG_DRM_ACCEL_HABANALABS) += habanalabs/ >>>>> obj-$(CONFIG_DRM_ACCEL_IVPU) += ivpu/ >>>>> obj-$(CONFIG_DRM_ACCEL_QAIC) += qaic/ >>>>> +obj-$(CONFIG_DRM_ACCEL_QDA) += qda/ >>>>> obj-$(CONFIG_DRM_ACCEL_ROCKET) += rocket/ >>>>> \ No newline at end of file >>>> >>>> You have trivial patch errors. >>> newline problem was already there, wasn't introduced as part of this >>> patch series, so I wasn't sure to fix it here. I can fix this in v3.> >>>> ... >>>> >>>>> +} >>>>> + >>>>> +static const struct of_device_id qda_rpmsg_id_table[] = { >>>>> + { .compatible = "qcom,fastrpc" }, >>>>> + {}, >>>> >>>> Device node with this compatible is already populated, so this looks >>>> simply wrong or you are adding a duplicated driver. >>>> >>>> That's a no-go, you are supposed to work with existing drivers and grow >>>> them. >>> I'll bring the discussion again here, there was a discussion to move the >>> driver to accel subsystem if we want to support new features/uAPI >>> changes. Please read [1],[2] threads. The intention is to replace >>> fastrpc driver with QDA eventually. >> >> None of them address the problem. You want to grow fastrpc into user of >> dmabuf? So you move it from misc to here. > > It's not as easy and nice, so I think in this case it's better to repeat I disagree. The existing fastrpc driver is not that complicated. It's actually moderate amount of code, much less than Venus was (~7 times less). It easily can grow to support two interfaces and the only difficulty is how to manage these two interfaces simultaneously or exclusively, e.g. opening first one disables the second. > what we did for the venus/iris migration or what happened already > several times in the kernel history (for example the AIC7xxx SCSI host > drivers were migrated by introducing the second driver and then removing > the first one after the grace period). The backwards compatibility is a > separate topic, but it will be addressed before the driver can be Backwards compatibility should be one of the first things explained in cover letter in one of the first paragraphs. And if you make it backwards compatible, then just remove old driver, because there is no point to keep it there. Again, this should be one of the first things explained in cover letter. Best regards, Krzysztof

1 month, 3 weeks

← Newer
1
...
8
9
10
11
12
13
14
...
363
Older →

Jump to page:

2026

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

Linaro-mm-sig