Re: [RFC/POC PATCH 00/12] POC SVM implementation in AMDGPU based on drm_gpusvm

[Matthew Brost <matthew.brost@intel.com>]

On Tue, Mar 24, 2026 at 03:24:43PM +0800, Honglei Huang wrote:
> 
> 
> On 3/23/26 14:31, Matthew Brost wrote:
> > On Thu, Mar 19, 2026 at 10:17:36PM +0800, Honglei Huang wrote:
> > > 
> > > 
> > > On 3/19/26 13:08, Matthew Brost wrote:
> > > > On Wed, Mar 18, 2026 at 04:59:31PM +0800, Honglei Huang wrote:
> > > > > 
> > > > 
> > > > Disclaimer I haven't look at any code in this series yet.
> > > > 
> > > > > 
> > > > > On 3/17/26 19:48, Christian König wrote:
> > > > > > Adding a few XE and drm_gpuvm people on TO.
> > > > > > 
> > > > > > On 3/17/26 12:29, Honglei Huang wrote:
> > > > > > > From: Honglei Huang <honghuan@amd.com>
> > > > > > > 
> > > > > > > This is a POC/draft patch series of SVM feature in amdgpu based on the
> > > > > > > drm_gpusvm framework. The primary purpose of this RFC is to validate
> > > > > > > the framework's applicability, identify implementation challenges,
> > > > > > > and start discussion on framework evolution. This is not a production
> > > > 
> > > > +1. Open to any ideas. Given this was designed originally for Xe we very
> > > > well could have missed other drivers requirements.
> > > Hi Matt,
> > > 
> > > Thank you for the openness. And thank you so much for the incredibly
> > > detailed and patient response. I really appreciate you taking the time to
> > > walk through each point.
> > > 
> > 
> > I'm here to help.
> > 
> > > Actually I am still a learner when it comes to the drm_gpusvm framework and
> > > GPU SVM design in general. Some of my descriptions below may not be entirely
> > > accurate. But I really want to bring drm_gpusvm into amdgpu and make it work
> > > well.
> > 
> > I appreciate another driver jumping in and using this framework—it
> > becomes easier to validate as more users adopt it.
> > 
> > > 
> > > > 
> > > > > > > ready submission.
> > > > > > > 
> > > > > > > This patch series implements basic SVM support with the following features:
> > > > > > > 
> > > > > > >      1. attributes sepatarated from physical page management:
> > > > > > > 
> > > > > > >        - Attribute layer (amdgpu_svm_attr_tree): a driver side interval
> > > > > > >          tree that stores SVM attributes. Managed through the SET_ATTR,
> > > > > > >          and mmu notifier callback.
> > > > 
> > > > Can you explain the mmu notifier callback interaction here? See below in
> > > > Xe the attribute tree is existing VMA tree (gpuvm).
> > > > 
> > > 
> > > Let me try to explain, apologies if the description is not fully
> > > precise.
> > > 
> > > In current implementation, the MMU notifier callback interacts with the attr
> > > tree only in the munmap path remove the corresponding attribute
> > > entries from the attr tree so that stale attributes do not persist for
> > > freed address space.
> > > 
> > 
> > Ah, yes. We reset our attributes upon munmap too. We actually don't this
> > 100% correct quite either and series in flight to fix [1].
> > 
> > [1] https://patchwork.freedesktop.org/series/161815/
> > 
> 
> I studied [1]. This draft has a simliar mechanism to handle attributes when
> munmap. But there are some sligt differences in detail, maybe casued by
> different UMD runtime behaviors.
> 
> 
> > > > > > > 
> > > > > > >        - Physical page layer (drm_gpusvm ranges): managed by the
> > > > > > >          drm_gpusvm framework, representing actual HMM backed DMA
> > > > > > >          mappings and GPU page table entries.
> > > > > > > 
> > > > > > >         This separation is necessary:
> > > > > > >           -  The framework does not support range splitting, so a partial
> > > > > > >              munmap destroys the entire overlapping range, including the
> > > > > > >              still valid parts. If attributes were stored inside drm_gpusvm
> > > > > > >              ranges, they would be lost on unmapping.
> > > > > > >              The separate attr tree preserves userspace set attributes
> > > > > > >              across range operations.
> > > > 
> > > > Yes, in Xe the divide is at the VMA level (set by user space) via VM
> > > > bind (parts of VM may be mappings BOs, parts could be setup for SVM) or
> > > > madvise IOCTLs which reflect user space attributes on current SVM
> > > > mappings or future ones.
> > > > 
> > > > The SVM range tree reflects mappings that have been faulted into the
> > > > device and contain pages. This is an intentional choice.
> > > 
> > > That makes a lot of sense. Thank you for clarifying the design intent. I
> > > think the current adopt the same principle: the drm_gpusvm range tree only
> > > reflect actual faulted in mappings.
> > > 
> > > > 
> > > > > > 
> > > > > > Isn't that actually intended? When parts of the range unmap then that usually means the whole range isn't valid any more.
> > > > 
> > > > 
> > > > Yes, this was an intentional design choice to not support partial unmap,
> > > > and instead rely on the driver to recreate a new range.
> > > > 
> > > > The reasoning is:
> > > > 
> > > > - In practice, this should be rare for well-behaved applications.
> > > > 
> > > > - With THP / large device pages, if a sub-range is unmapped, the entire
> > > > GPU mapping is invalidated anyway due to the page size change. As a
> > > > result, the cost of creating a new range is minimal, since the device
> > > > will likely fault again on the remaining pages.
> > > > 
> > > > So there is no need to over-engineer the common code.
> > > > 
> > > > FWIW, to even test partial unmaps in Xe, I had to do things I doubt
> > > > anyone would ever do:
> > > > 
> > > > ptr = mmap(SZ_2M);
> > > > /* fault in memory to the device */
> > > > munmap(ptr, SZ_1M);
> > > > /* touch memory again on the device */
> > > > 
> > > 
> > > Thank you for this explanation and the concrete example. After further
> > > discussion internally with Christian, we are now aligned with same position
> > > partial unmap. Will remove rebuild on partial unmap logic in the next
> > > version and handle it as only partially backed range.
> > > 
> > > > > 
> > > > > 
> > > > > It is about partial unmap, some subregion in drm_gpusvm_range is still valid
> > > > > but some other subregion is invalid, but under drm_gpusvm, need to destroy
> > > > > the entire range.
> > > > > 
> > > > > e.g.:
> > > > > 
> > > > >             [---------------unmap region in mmu notifier-----------------]
> > > > > [0x1000 ------------ 0x9000]
> > > > > [  valid ][     invalid    ]
> > > > > 
> > > > > see deatil in drm_gpusvm.c:110 line
> > > > > section:Partial Unmapping of Ranges
> > > > > 
> > > > > 
> > > > > > 
> > > > > > > 
> > > > > > >           -  drm_gpusvm range boundaries are determined by fault address
> > > > > > >              and pre setted chunk size, not by userspace attribute boundaries.
> > > > > > >              Ranges  may be rechunked on memory changes. Embedding
> > > > > > >              attributes in framework ranges would scatter attr state
> > > > > > >              across many small ranges and require complex reassemble
> > > > > > >              logic when operate attrbute.
> > > > > > 
> > > > > > Yeah, that makes a lot of sense.
> > > > > > 
> > > > > > > 
> > > > > > >      2) System memory mapping via drm_gpusvm
> > > > > > > 
> > > > > > >         The core mapping path uses drm_gpusvm_range_find_or_insert() to
> > > > > > >         create ranges, drm_gpusvm_range_get_pages() for HMM page fault
> > > > > > >         and DMA mapping, then updates GPU page tables via
> > > > > > >         amdgpu_vm_update_range().
> > > > > > > 
> > > > > > >      3) IOCTL driven mapping (XNACK off / no GPU fault mode)
> > > > > > > 
> > > > > > >         On XNACK off hardware the GPU cannot recover from page faults,
> > > > > > >         so mappings must be established through ioctl. When
> > > > > > >         userspace calls SET_ATTR with ACCESS=ENABLE, the driver
> > > > > > >         walks the attr tree and maps all accessible intervals
> > > > > > >         to the GPU by amdgpu_svm_range_map_attr_ranges().
> > > > 
> > > > Can you expand on XNACK off / GPU no faults? Is this to the share GPU
> > > > between 3D (dma-fences) and faulting clients? We have something similar
> > > > in Xe, but it isn't an explicit IOCTL rather we switch between on demand
> > > > as 3D client submits and then resumes page faults when all dma-fences
> > > > have signaled.
> > > > 
> > > > I see below you mention page tables are modified during quiesce KFD
> > > > queues? I'm not sure that is required - you just need to guarnette
> > > > faulting clients won't trigger page faults when dma-fence is in flight.
> > > > 
> > > > Maybe give me an explaination of exactly what the requirement from AMD
> > > > are here so I have better picture.
> > > 
> > > Thank you for the patience, let me try to explain our situation, though
> > > I may not get every detail right.
> > > 
> > > XNACK off means hardware that does not have GPU page fault capability (or
> > > turned off)
> > > 
> > > So for these GPUs, ALL page table entries must be fully populated before
> > > the GPU can access the memory. This is why we need the ioctl driven
> > > mapping path, when userspace calls SET_ATTR with ACCESS=ENABLE, need
> > > walk the attribute tree and eagerly map all accessible ranges into the
> > > GPU page tables. This is functionally similar to what you describe as
> > > prefetch IOCTLs / VM bind in Xe.
> > > 
> > > Regarding queue quiesce during page table modification: on XNACK off
> > > hardware, because the GPU cannot fault, we must ensure the GPU is
> > > completely stopped before modifying any PTE it might be accessing.
> > > Otherwise the GPU could access a partially updated page table and hang.
> > > The quiesce/resume is the mechanism to guarantee this.
> > > 
> > > I hope that helps clarify the picture.
> > > 
> > 
> > This clarifies a lot. This is what we’d call in Xe “preemption fence”
> > mode for a VM. Anytime memory is moved, we trigger a GPU preemption and
> > resume. We don’t actually support SVM in this case; instead, we use
> > “userptr binds,” which are built on gpusvm for page collection. However,
> > we don’t support migrating memory to the device—though we could.
> > 
> > I’d look at how we converted 'userptr' to be based on GPU SVM [2]. In
> > this case, don’t maintain a range tree, as those—as you suggest—are more
> > of an on-demand fault driver concern. Instead, just embed 'struct
> > drm_gpusvm_pages' in the VMA struct defined by the IOCTLs..
> > 
> > We could extend this to support migrating 'userptr', but we just haven’t
> > done that yet—this may be what you want to do in “XNACK off..
> > 
> > [2] https://patchwork.freedesktop.org/series/146553/
> > 
> 
> Actually we need to swith the xnack mode between on and off, so in xnack off
> mode, the driver operats in "implicit  prefetch mode". This may be due to
> compatibility with older hardware and the need for UMD runtime. We will
> further discuss the handling method under xnack off internally.
> 
> 
> > > 
> > > > 
> > > > > > > 
> > > > > > >      4) Invalidation, GC worker, and restore worker
> > > > > > > 
> > > > > > >         MMU notifier callbacks (amdgpu_svm_range_invalidate) handle
> > > > > > >         three cases based on event type and hardware mode:
> > > > > > >           - unmap event: clear GPU PTEs in the notifier context,
> > > > > > >             unmap DMA pages, mark ranges as unmapped, flush TLB,
> > > > > > >             and enqueue to the GC worker. On XNACK off, also
> > > > > > >             quiesce KFD queues and schedule rebuild of the
> > > > > > >             still valid portions that were destroyed together with
> > > > > > >             the unmapped subregion.
> > > > > > > 
> > > > > > >           - evict on XNACK off:
> > > > > > >             quiesce KFD queues first, then unmap DMA pages and
> > > > > > >             enqueue to the restore worker.
> > > > > > 
> > > > > > Is that done through the DMA fence or by talking directly to the MES/HWS?
> > > > > 
> > > > > Currently KFD queues quiesce/resume API are reused, lookig forward to a
> > > > > better solution.
> > > > > 
> > > > 
> > > > +1
> > > > 
> > > > > Regards,
> > > > > Honglei
> > > > > 
> > > > > > 
> > > > > > Thanks,
> > > > > > Christian.
> > > > > > 
> > > > > > > 
> > > > > > >           - evict on XNACK on:
> > > > > > >             clear GPU PTEs, unmap DMA pages, and flush TLB, but do
> > > > > > >             not schedule any worker. The GPU will fault on next
> > > > > > >             access and the fault handler establishes the mapping.
> > > > > > > 
> > > > > > > Not supported feature:
> > > > > > >      - XNACK on GPU page fault mode
> > > > > > >      - migration and prefetch feature
> > > > > > >      - Multi GPU support
> > > > > > > 
> > > > > > >      XNACK on enablement is ongoing.The GPUs that support XNACK on
> > > > > > >      are currently only accessible to us via remote lab machines, which slows
> > > > > > >      down progress.
> > > > > > > 
> > > > > > > Patch overview:
> > > > > > > 
> > > > > > >      01/12 UAPI definitions: DRM_AMDGPU_GEM_SVM ioctl, SVM flags,
> > > > > > >            SET_ATTR/GET_ATTR operations, attribute types, and related
> > > > > > >            structs in amdgpu_drm.h.
> > > > > > > 
> > > > > > >      02/12 Core data structures: amdgpu_svm wrapping drm_gpusvm with
> > > > > > >            refcount, attr_tree, workqueues, locks, and
> > > > > > >            callbacks (begin/end_restore, flush_tlb).
> > > > > > > 
> > > > > > >      03/12 Attribute data structures: amdgpu_svm_attrs, attr_range
> > > > > > >            (interval tree node), attr_tree, access enum, flag masks,
> > > > > > >            and change trigger enum.
> > > > > > > 
> > > > > > >      04/12 Attribute tree operations: interval tree lookup, insert,
> > > > > > >            remove, and tree create/destroy lifecycle.
> > > > > > > 
> > > > > > >      05/12 Attribute set: validate UAPI attributes, apply to internal
> > > > > > >            attrs, handle hole/existing range with head/tail splitting,
> > > > > > >            compute change triggers, and -EAGAIN retry loop.
> > > > > > >            Implements attr_clear_pages for unmap cleanup and attr_get.
> > > > > > > 
> > > > > > >      06/12 Range data structures: amdgpu_svm_range extending
> > > > > > >            drm_gpusvm_range with gpu_mapped state, pending ops,
> > > > > > >            pte_flags cache, and GC/restore queue linkage.
> > > > > > > 
> > > > > > >      07/12 PTE flags and GPU mapping: simple gpu pte function,
> > > > > > >            GPU page table update with DMA address, range mapping loop:
> > > > > > >            find_or_insert -> get_pages -> validate -> update PTE,
> > > > > > >            and attribute change driven mapping function.
> > > > > > > 
> > > > > > >      08/12 Notifier and invalidation: synchronous GPU PTE clear in
> > > > > > >            notifier context, range removal and overlap cleanup,
> > > > > > >            rebuild after destroy logic, and MMU event dispatcher
> > > > > > > 
> > > > > > >      09/12 Workers: KFD queue quiesce/resume via kgd2kfd APIs, GC
> > > > > > >            worker for unmap processing and rebuild, ordered restore
> > > > > > >            worker for mapping evicted ranges, and flush/sync
> > > > > > >            helpers.
> > > > > > > 
> > > > > > >      10/12 Initialization and fini: kmem_cache for range/attr,
> > > > > > >            drm_gpusvm_init with chunk sizes, XNACK detection, TLB
> > > > > > >            flush helper, and amdgpu_svm init/close/fini lifecycle.
> > > > > > > 
> > > > > > >      11/12 IOCTL and fault handler: PASID based SVM lookup with kref
> > > > > > >            protection, amdgpu_gem_svm_ioctl dispatcher, and
> > > > > > >            amdgpu_svm_handle_fault for GPU page fault recovery.
> > > > > > > 
> > > > > > >      12/12 Build integration: Kconfig option (CONFIG_DRM_AMDGPU_SVM),
> > > > > > >            Makefile rules, ioctl table registration, and amdgpu_vm
> > > > > > >            hooks (init in make_compute, close/fini, fault dispatch).
> > > > > > > 
> > > > > > > Test result:
> > > > > > >      on gfx1100(W7900) and gfx943(MI300x)
> > > > > > >      kfd test: 95%+ passed, same failed cases with offical relase
> > > > > > >      rocr test: all passed
> > > > > > >      hip catch test: 20 cases failed in all 5366 cases, +13 failures vs offical relase
> > > > > > > 
> > > > > > > During implementation we identified several challenges / design questions:
> > > > > > > 
> > > > > > > 1. No range splitting on partial unmap
> > > > > > > 
> > > > > > >      drm_gpusvm explicitly does not support range splitting in drm_gpusvm.c:122.
> > > > > > >      Partial munmap needs to destroy the entire range including the valid interval.
> > > > > > >      GPU fault driven hardware can handle this design by extra gpu fault handle,
> > > > > > >      but AMDGPU needs to support XNACK off hardware, this design requires driver
> > > > > > >      rebuild the valid part in the removed entire range. Whichs bring a very heavy
> > > > > > >      restore work in work queue/GC worker: unmap/destroy -> rebuild(insert and map)
> > > > > > >      this restore work even heavier than kfd_svm. In previous driver work queue
> > > > > > >      only needs to restore or unmap, but in drm_gpusvm driver needs to unmap and restore.
> > > > > > >      which brings about more complex logic, heavier worker queue workload, and
> > > > > > >      synchronization issues.
> > > > 
> > > > Is this common in the workload you are running? I'm also wondering if
> > > > your restore logic / KFDs design is contributing to this actally the
> > > > problem.
> > > > 
> > > 
> > > Honestly, you raise a fair point.
> > > 
> > > We will redesign the logic about the partial munap， which should eliminate
> > > most of this complexity.
> > > 
> > > 
> > 
> > +1, yes test but do optimize for.
> > 
> > > > > > > 
> > > > > > > 2. Fault driven vs ioctl driven mapping
> > > > > > > 
> > > > > > >      drm_gpusvm is designed around GPU page fault handlers. The primary entry
> > > > > > >      point drm_gpusvm_range_find_or_insert() takes a fault_addr.
> > > > > > >      AMDGPU needs to support IOCTL driven mapping cause No XNACK hardware that
> > > > > > >      GPU cannot fault at all
> > > > 
> > > > I think we refer to these as prefetch IOCTLs in Xe. Ideally, user space
> > > > issues these so the device does not fault (e.g., prefetch creates a set
> > > > of SVM ranges based on user input). In Xe, prefetch IOCTLs are simply
> > > > specific VM bind operations.
> > > > 
> > > 
> > > That is a very helpful way to think about it. Yes, our ioctl driven
> > > mapping(xnack off) is essentially equivalent to a prefetch operation. We are
> > > trying to improve it.
> > > 
> > 
> > See above wrt 'userptr'.
> 
> Got it.
> 
> > 
> > > 
> > > > > > > 
> > > > > > >      The ioctl path cannot hold mmap_read_lock across the entire operation
> > > > > > >      because drm_gpusvm_range_find_or_insert() acquires/releases it
> > > > > > >      internally. This creates race windows with MMU notifiers / workers.
> > > > 
> > > > This is a very intentional choice in the locking design: mmap_read_lock
> > > > is held only in very specific parts of GPU SVM, and the driver should
> > > > never need to take this lock.
> > > > 
> > > > Yes, notifiers can race, which is why the GPU fault handler and prefetch
> > > > handler are structured as retry loops when a notifier race is detected.
> > > > In practice, with well-behaved applications, these races should be
> > > > rare—but they do occur, and the driver must handle them.
> > > > 
> > > > __xe_svm_handle_pagefault implements the page fault retry loop. VM bind
> > > > prefetch has similar logic, although it is more spread out given that it
> > > > is part of a deeper software pipeline.
> > > > 
> > > > FWIW, holding locks to avoid races was rejected by Sima because we
> > > > reasoned it is essentially impossible to guarantee the absence of races
> > > > by holding a lock. CPU page fault handlers are also effectively just
> > > > large retry loops.
> > > > 
> > > > So this is one point I believe you will need to fixup driver side.
> > > > 
> > > 
> > > Understood. Thank you for the detailed explanation and for pointing to
> > > __xe_svm_handle_pagefault as a reference. We will restructure both our
> > > fault handler and ioctl path to a betterretry loop pattern with sequence
> > > number race detection.
> > > 
> > 
> > Yes, the typical pattern is:
> > 
> > - Try to migrate once
> > - If you hit a race, give up, evict all memory back to system memory, and bind it
> > 
> > Atomics make this tricky because memory must move, but I’m not sure
> > “XNACK off” applies here. However, GPU SVM provides a timeslice
> > mechanism to ensure the CPU can’t move memory while the GPU needs to
> > execute something.
> 
> Understood.
> 
> > 
> > > > > > > 
> > > > > > > 3. Multi GPU support
> > > > > > > 
> > > > > > > drm_gpusvm binds one drm_device to one instance. In multi GPU systems,
> > > > > > > each GPU gets an independent instance with its own range tree, MMU
> > > > > > > notifiers, notifier_lock, and DMA mappings.
> > > > > > > 
> > > > 
> > > > This is a part I am absolutely open to fixing. Right now, each
> > > > drm_gpusvm_range has a single set of drm_gpusvm_pages. I am open to
> > > > decoupling a GPU SVM instance from a single device, allowing each
> > > > drm_gpusvm_range to have multiple sets of drm_gpusvm_pages (one per
> > > > device).
> > > > 
> > > > This would give drivers the flexibility to use one GPU SVM instance per
> > > > VM/device instance (as in Xe), or to maintain a single GPU SVM per CPU
> > > > MM.
> > > > 
> > > 
> > > That would be wonderful! Looking forward to your patch very much!
> > > 
> > 
> > I can't say I'll code this but we thought about is as options and very
> > open patches which refactor the object model for multiple use cases.
> 
> Understood. I will focus on single GPU first, and once we have a
> solid v1, we'd be happy to explore contributing patches for the
> multi-device object model refactoring.
> 

I think roughly what would need to be done is:

- Move struct drm_gpusvm_pages out of struct drm_gpusvm_range.
- Embed either a struct device or a struct drm_device in struct
  drm_gpusvm_pages.
- Drop struct drm_device from struct drm_gpusvm.
- Have the driver’s range structure embed one or more struct
  drm_gpusvm_pages in addition to struct drm_gpusvm_range.
- Refactor a few range-based helpers (drm_gpusvm_range_pages_valid,
  drm_gpusvm_range_get_pages, drm_gpusvm_range_unmap_pages), or simply
  drop them entirely and update drivers to use the drm_gpusvm_pages
  helpers instead.

Then it is up to the drivers whether struct drm_gpusvm maps to a single
device or multiple devices. Either use case seems valid, and giving
drivers the option appears to be the right approach, rather than having
the common drm_gpusvm layer impose its own constraints.

This type of refactor can be done at any time as an independent patch,
so feel free to post it whenever and I can verify on the Xe side that
everything looks good.

Matt

> > 
> > > 
> > > > > > > This may brings huge overhead:
> > > > > > >        - N x MMU notifier registrations for the same address range
> > > > 
> > > > The notifier overhead is a real concern. We recently introduced two-pass
> > > > notifiers [1] to speed up multi-device notifiers. At least in Xe, the
> > > > TLB invalidations—which are the truly expensive part—can be pipelined
> > > > using the two=pass approach. Currently, [1] only implements two-pass
> > > > notifiers for userptr, but Xe’s GPU SVM will be updated to use them
> > > > shortly.
> > > > 
> > > > [1] https://patchwork.freedesktop.org/series/153280/
> > > > 
> > > 
> > > Thank you for the pointer to two-pass notifiers. Will study this
> > > series.
> > > 
> > > > > > >        - N x hmm_range_fault() calls for the same page (KFD: 1x)
> > > > 
> > > > hmm_range_fault is extremely fast compared to the actual migration.
> > > > Running hmm_range_fault on a 2MB region using 4KB pages takes less
> > > > than 1µs. With THP or large device pages [2] (merged last week), it’s
> > > > around 1/20 of a microsecond. So I wouldn’t be too concerned about this.
> > > > 
> > > > [2] https://patchwork.freedesktop.org/series/163141/
> > > > 
> > > 
> > > That is very helpful data. Perhaps worry too much.
> > > 
> > > > > > >        - N x DMA mapping memory
> > > > 
> > > > You will always have N x DMA mapping memory if the pages are in system
> > > > memory as the dma-mapping API is per device.
> > > 
> > > Totally agreed.
> > > 
> > > > 
> > > > > > >        - N x invalidation + restore worker scheduling per CPU unmap event
> > > > > > >        - N x GPU page table flush / TLB invalidation
> > > > 
> > > > I agree you do not want serialize GPU page table flush / TLB
> > > > invalidations. Hence two-pass notifiers [1].
> > > 
> > > Yes, will learn it.
> > > 
> > > > 
> > > > > > >        - Increased mmap_lock hold time, N callbacks serialize under it
> > > > > > > 
> > > > > > > compatibility issues:
> > > > > > >        - Quiesce/resume scope mismatch: to integrate with KFD compute
> > > > > > >          queues, the driver reuses kgd2kfd_quiesce_mm()/resume_mm()
> > > > > > >          which have process level semantics. Under the per GPU
> > > > > > >          drm_gpusvm model, maybe there are some issues on sync. To properly
> > > > > > >          integrate with KFD under the per SVM model, a compatibility or
> > > > > > >          new per VM level queue control APIs maybe need to introduced.
> > > > > > > 
> > > > 
> > > > I thought the idea to get rid of KFD and move over to AMDGPU? I thought
> > > > Christian mentioned this to me at XDC.
> > > > 
> > > 
> > > > > > > Migration challenges:
> > > > > > > 
> > > > > > >      - No global migration decision logic: each per GPU SVM
> > > > > > >        instance maintains its own attribute tree independently. This
> > > > > > >        allows conflicting settings (e.g., GPU0's SVM sets
> > > > > > >        PREFERRED_LOC=GPU0 while GPU1's SVM sets PREFERRED_LOC=GPU1
> > > > > > >        for the same address range) with no detection or resolution.
> > > > > > >        A global attribute coordinator or a shared manager is needed to
> > > > > > >        provide a unified global view for migration decisions
> > > > 
> > > > Yes, this is hole in the Xe API too. We have told UMDs if they setup
> > > > individual VMs with conflict attributes for a single CPU address space
> > > > the behavior is undefined. Our UMD implement madvise is basically loop
> > > > over al GPU VMs setting the same attributes.
> > > 
> > > Will follow the same approach for now, the UMD is responsible for setting
> > > consistent attributes across GPU VMs.
> > > 
> > 
> > +1
> > 
> > > > 
> > > > > > > 
> > > > > > >      - migrate_vma_setup broadcast: one GPU's migration triggers MMU
> > > > > > >        notifier callbacks in ALL N-1 other drm_gpusvm instances,
> > > > > > >        causing N-1 unnecessary restore workers to be scheduled. And
> > > > 
> > > > My feeling is that you shouldn’t reschedule restore workers unless you
> > > > actually have to invalidate page tables (i.e., you have a local SVM
> > > > range within the notifier). So the first migration to an untouched
> > > > region may trigger notifiers, but they won’t do anything because you
> > > > don’t have any valid SVM ranges yet. Subsequent mappings of the migrated
> > > > region won’t trigger a notifier unless the memory is moved again.
> > > > 
> > > 
> > > That is a very good point. We should check whether we actually have
> > > valid SVM ranges before scheduling restore workers. If there is nothing
> > > to invalidate, the notifier callback should be a no-op. We will review
> > > our notifier callback logic to ensure we are not doing unnecessary work
> > > here. Thank you for pointing this out.
> > > 
> > > > > > >        creates races between the initiating migration and the other
> > > > > > >        instance's restore attempts.
> > > > 
> > > > Yes, if multiple devices try to migrate the same CPU pages at the same
> > > > time, that will race. That’s why in Xe we have a module-level
> > > > driver_migrate_lock. The first migration runs in read mode; if it
> > > > detects a race and aborts, it then takes driver_migrate_lock in write
> > > > mode so it becomes the only device allowed to move memory / CPU pages.
> > > > See xe_svm_alloc_vram() for how this is used.
> > > > 
> > > > I’m not sure this approach will work for you, but I just wanted to point
> > > > out that we identified this as a potential issue.
> > > > 
> > > 
> > > Thank you for sharing the driver_migrate_lock approach and pointing to
> > > xe_svm_alloc_vram(). Will explore whether a similar lock pattern can work
> > > for our case.
> > > 
> > > > > > > 
> > > > > > >      - No cross instance migration serialization: each per GPU
> > > > > > >        drm_gpusvm instance has independent locking, so two GPUs'
> > > > > > >        "decide -> migrate -> remap" sequences can interleave. While
> > > > > > >        the kernel page lock prevents truly simultaneous migration of
> > > > > > >        the same physical page, the losing side's retry (evict from
> > > > > > >        other GPU's VRAM -> migrate back) triggers broadcast notifier
> > > > > > >        invalidations and restore workers, compounding the ping pong
> > > > > > >        problem above.
> > > > > > > 
> > > > 
> > > > See the driver_migrate_lock above.
> > > 
> > > Acknowledged, thank you.
> > > > 
> > > > > > >      - No VRAM to VRAM migration: drm_pagemap_migrate_to_devmem()
> > > > > > >        hardcodes MIGRATE_VMA_SELECT_SYSTEM (drm_pagemap.c:328), meaning
> > > > > > >        it only selects system memory pages for migration.
> > > > > > > 
> > > > 
> > > > I think this is fixed? We did find some core MM bugs that blocked VRAM
> > > > to VRAM but those have been worked out.
> > > > 
> > > > The code I'm looking at:
> > > > 
> > > >    517 int drm_pagemap_migrate_to_devmem(struct drm_pagemap_devmem *devmem_allocation,
> > > >    518                                   struct mm_struct *mm,
> > > >    519                                   unsigned long start, unsigned long end,
> > > >    520                                   const struct drm_pagemap_migrate_details *mdetails)
> > > >    521 {
> > > >    522         const struct drm_pagemap_devmem_ops *ops = devmem_allocation->ops;
> > > >    523         struct drm_pagemap *dpagemap = devmem_allocation->dpagemap;
> > > >    524         struct dev_pagemap *pagemap = dpagemap->pagemap;
> > > >    525         struct migrate_vma migrate = {
> > > >    526                 .start          = start,
> > > >    527                 .end            = end,
> > > >    528                 .pgmap_owner    = pagemap->owner,
> > > >    529                 .flags          = MIGRATE_VMA_SELECT_SYSTEM | MIGRATE_VMA_SELECT_DEVICE_COHERENT |
> > > >    530                 MIGRATE_VMA_SELECT_DEVICE_PRIVATE | MIGRATE_VMA_SELECT_COMPOUND,
> > > >    531         };
> > > > 
> > > 
> > > Thank you for checking! I am using v6.18 for this POC, missed the fix, will
> > > rebase to the latest.
> > > 
> > > 
> > > > > > >      - CPU fault reverse migration race: CPU page fault triggers
> > > > > > >        migrate_to_ram while GPU instances are concurrently operating.
> > > > > > >        Per GPU notifier_lock does not protect cross GPU operations.
> > > > 
> > > > No, again retry loop as discussed above.
> > > 
> > > Understood.
> > > 
> > > > 
> > > > > > > 
> > > > > > > We believe a strong, well designed solution at the framework level is
> > > > > > > needed to properly address these problems, and we look forward to
> > > > > > > discussion and suggestions.
> > > > 
> > > > Let's work together to figure out what is missing here.
> > > 
> > > Thank you so much, Matt. Your feedback has been incredibly valuable and
> > > has given us a much clearer picture of the framework's design.
> > > Ireally appreciate the effort you put into building drm_gpusvm as a
> > > shared framework. Will incorporate your suggestions into our next
> > > revision and look forward to continuing the collaboration.
> > > 
> > 
> > No problem. Happy to help.
> 
> Thank you again for all the detailed feedback.
> 
> Regards,
> Honglei
> 
> > 
> > Matt
> > 
> > > Regards,
> > > Honglei
> > > 
> > > 
> > > > 
> > > > Matt
> > > > 
> > > > > > > 
> > > > > > > Honglei Huang (12):
> > > > > > >      drm/amdgpu: add SVM UAPI definitions
> > > > > > >      drm/amdgpu: add SVM data structures and header
> > > > > > >      drm/amdgpu: add SVM attribute data structures
> > > > > > >      drm/amdgpu: implement SVM attribute tree operations
> > > > > > >      drm/amdgpu: implement SVM attribute set
> > > > > > >      drm/amdgpu: add SVM range data structures
> > > > > > >      drm/amdgpu: implement SVM range PTE flags and GPU mapping
> > > > > > >      drm/amdgpu: implement SVM range notifier and invalidation
> > > > > > >      drm/amdgpu: implement SVM range workers
> > > > > > >      drm/amdgpu: implement SVM core initialization and fini
> > > > > > >      drm/amdgpu: implement SVM ioctl and fault handler
> > > > > > >      drm/amdgpu: wire up SVM build system and fault handler
> > > > > > > 
> > > > > > >     drivers/gpu/drm/amd/amdgpu/Kconfig            |   11 +
> > > > > > >     drivers/gpu/drm/amd/amdgpu/Makefile           |   13 +
> > > > > > >     drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c       |    2 +
> > > > > > >     drivers/gpu/drm/amd/amdgpu/amdgpu_svm.c       |  430 ++++++
> > > > > > >     drivers/gpu/drm/amd/amdgpu/amdgpu_svm.h       |  147 ++
> > > > > > >     drivers/gpu/drm/amd/amdgpu/amdgpu_svm_attr.c  |  894 ++++++++++++
> > > > > > >     drivers/gpu/drm/amd/amdgpu/amdgpu_svm_attr.h  |  110 ++
> > > > > > >     drivers/gpu/drm/amd/amdgpu/amdgpu_svm_range.c | 1196 +++++++++++++++++
> > > > > > >     drivers/gpu/drm/amd/amdgpu/amdgpu_svm_range.h |   76 ++
> > > > > > >     drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c        |   40 +-
> > > > > > >     drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h        |    4 +
> > > > > > >     include/uapi/drm/amdgpu_drm.h                 |   39 +
> > > > > > >     12 files changed, 2958 insertions(+), 4 deletions(-)
> > > > > > >     create mode 100644 drivers/gpu/drm/amd/amdgpu/amdgpu_svm.c
> > > > > > >     create mode 100644 drivers/gpu/drm/amd/amdgpu/amdgpu_svm.h
> > > > > > >     create mode 100644 drivers/gpu/drm/amd/amdgpu/amdgpu_svm_attr.c
> > > > > > >     create mode 100644 drivers/gpu/drm/amd/amdgpu/amdgpu_svm_attr.h
> > > > > > >     create mode 100644 drivers/gpu/drm/amd/amdgpu/amdgpu_svm_range.c
> > > > > > >     create mode 100644 drivers/gpu/drm/amd/amdgpu/amdgpu_svm_range.h
> > > > > > > 
> > > > > > > 
> > > > > > > base-commit: 7d0a66e4bb9081d75c82ec4957c50034cb0ea449
> > > > > > 
> > > > > 
> > > 
>