Linux-mm Archive on lore.kernel.org
 help / color / mirror / Atom feed
From: sashiko-bot@kernel.org
To: mpenttil@redhat.com, linux-mm@kvack.org
Cc: dri-devel@lists.freedesktop.org, intel-xe@lists.freedesktop.org,
	"David Hildenbrand" <david@kernel.org>,
	"Jason Gunthorpe" <jgg@nvidia.com>,
	"Leon Romanovsky" <leonro@nvidia.com>,
	"Alistair Popple" <apopple@nvidia.com>,
	"Balbir Singh" <balbirs@nvidia.com>, "Zi Yan" <ziy@nvidia.com>,
	"Matthew Brost" <matthew.brost@intel.com>,
	"Andrew Morton" <akpm@linux-foundation.org>,
	"Lorenzo Stoakes" <ljs@kernel.org>,
	"Liam R. Howlett" <Liam.Howlett@oracle.com>,
	"Vlastimil Babka" <vbabka@suse.cz>,
	"Mike Rapoport" <rppt@kernel.org>,
	"Suren Baghdasaryan" <surenb@google.com>,
	"Michal Hocko" <mhocko@suse.com>, "Lyude Paul" <lyude@redhat.com>,
	"Danilo Krummrich" <dakr@kernel.org>
Subject: Re: [PATCH v16 12/12] drm/nouveau/svm: add support for migrate on fault
Date: Thu, 22 Oct 2026 06:06:46 +0000	[thread overview]
Message-ID: <sashiko-outbox-181060@kernel.org> (raw)
In-Reply-To: <20261021060506.2377545-13-mpenttil@redhat.com>

Thank you for your contribution! Sashiko AI review found 9 potential issue(s) to consider:
- [Critical] drm/nouveau/dmem: freed VRAM page mapped on migrate_vma_pages() abort
- [Critical] drm/nouveau/dmem: copy timeout in nouveau_dmem_migrate_fault() maps uncopied page
- [Critical] drm/nouveau/svm: nouveau_svm_range_invalidate() ignores all driver migrations
- [High] drm/nouveau/dmem: unchecked nouveau_fence_new() in nouveau_dmem_fence_poll() callers
- [High] drm/nouveau/svm: unguarded __nouveau_pfns_map_locked() on empty pfn in nouveau_dmem_migrate_fault()
- [High] drm/nouveau/dmem: unsynchronized access to migrate.chan in nouveau_dmem_migrate_fault()
- [Medium] drm/nouveau: unguarded pfn map and retry state in nouveau_dmem_migrate_fault()
- [Medium] drm/nouveau/svm: __nouveau_pfns_map_locked() drops nvif_object_ioctl() error
- [Low] drm/nouveau/svm: missing kernel-parameters.txt entry for svm_migrate_on_fault
--

commit 44e0adde7e1aeb837684fa123e764779afbd3db4
Author: Mika Penttilä <mpenttil@redhat.com>

drm/nouveau/svm: add support for migrate on fault

This commit adds migrate-on-fault support for Nouveau SVM using
HMM_PFN_REQ_MIGRATE and migrate_hmm_range_setup(), gated by a new
svm_migrate_on_fault module parameter. It introduces
nouveau_dmem_migrate_fault() to fault and collect migratable pages in a
single hmm_range_fault() walk, and polls migration fences via
nouveau_dmem_fence_poll().

> diff --git a/drivers/gpu/drm/nouveau/nouveau_dmem.c b/drivers/gpu/drm/nouveau/nouveau_dmem.c
> index e74d7bb975a86..998c63e2ae78b 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_dmem.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_dmem.c
[ ... ]
> @@ -150,6 +155,32 @@ static void nouveau_dmem_fence_done(struct nouveau_fence **fence)
>  		 * the hmem object.
>  		 */
>  	}
> +
> +	/* 0 on completion, -EBUSY on timeout, or other errno; propagated. */
> +	return ret;
> +}
> +
> +/*
> + * Poll the copy fence instead of the lazy (interrupt-driven) wait since
> + * completion IRQ is randomly missed, hitting the timeout.
> + *
> + * Returns 0 on completion, -EBUSY on timeout. Consumes the fence reference.
> + */
> +static int nouveau_dmem_fence_poll(struct nouveau_fence **fence)
> +{
> +	unsigned long deadline = jiffies + 30 * HZ;
> +	int ret = 0;
> +
> +	while (!nouveau_fence_done(*fence)) {

[Severity: High]
What happens in nouveau_dmem_fence_poll() in
drivers/gpu/drm/nouveau/nouveau_dmem.c when nouveau_fence_new() fails in its
callers?

In drivers/gpu/drm/nouveau/nouveau_fence.c, nouveau_fence_new() can fail in
nouveau_fence_create() (without writing *pfence) or in nouveau_fence_emit()
(which calls nouveau_fence_unref(pfence) and sets *pfence = NULL).

Both callers of nouveau_dmem_fence_poll() ignore the return value of
nouveau_fence_new():

1. In nouveau_dmem_migrate_to_ram() in
drivers/gpu/drm/nouveau/nouveau_dmem.c, fence is declared uninitialized on
the stack:

	struct nouveau_fence *fence;
	...
	nouveau_fence_new(&fence, dmem->migrate.chan);
	migrate_vma_pages(&args);
	fret = nouveau_dmem_fence_poll(&fence);

2. In nouveau_dmem_migrate_fault() in
drivers/gpu/drm/nouveau/nouveau_dmem.c, fence is initialized to NULL:

	if (migrated)
		nouveau_fence_new(&fence, drm->dmem->migrate.chan);
	...
	if (migrated)
		fret = nouveau_dmem_fence_poll(&fence);

Since nouveau_fence_done() in drivers/gpu/drm/nouveau/nouveau_fence.c
immediately dereferences fence via nouveau_fctx(fence) and
dma_fence_is_signaled(&fence->base), won't a failure in nouveau_fence_new()
cause an uninitialized or NULL pointer dereference in
nouveau_dmem_fence_poll()?

> +		if (time_after(jiffies, deadline)) {
> +			ret = -EBUSY;
> +			break;
> +		}
> +		schedule_timeout_uninterruptible(msecs_to_jiffies(1));
> +	}
> +	nouveau_fence_unref(fence);
> +
> +	return ret;
>  }
[ ... ]
> @@ -266,9 +299,12 @@ static vm_fault_t nouveau_dmem_migrate_to_ram(struct vm_fault *vmf)
>  
>  	nouveau_fence_new(&fence, dmem->migrate.chan);
>  	migrate_vma_pages(&args);
> -	nouveau_dmem_fence_done(&fence);
> +	fret = nouveau_dmem_fence_poll(&fence);
>  	dma_unmap_page(drm->dev->dev, dma_info.dma_addr, dma_info.size,
>  				DMA_BIDIRECTIONAL);
> +	if (fret)
> +		/* Copy failed; the pte now points at possibly stale sysram data. */
> +		ret = VM_FAULT_SIGBUS;
>  done:
>  	migrate_vma_finalize(&args);
>  err:
[ ... ]
> @@ -832,6 +884,215 @@ static void nouveau_dmem_migrate_chunk(struct nouveau_drm *drm,
[ ... ]
> +int
> +nouveau_dmem_migrate_fault(struct nouveau_drm *drm, struct nouveau_svmm *svmm,
> +			   struct mmu_interval_notifier *notifier,
> +			   unsigned long start, unsigned long end,
> +			   unsigned long hmm_flags,
> +			   u64 *out_pfn0, unsigned long *out_addr,
> +			   unsigned int *out_shift)
> +{
[ ... ]
> +	/* hmm_range_fault() shares the source pfn array with migrate_vma. */
> +	range.hmm_pfns = migrate.src;
> +
> +	do {
> +		mmap_read_lock(mm);
> +		range.notifier_seq = mmu_interval_read_begin(notifier);
> +
> +		ret = hmm_range_fault(&range);
> +		/*
> +		 * Snapshot the pfn before migrate_hmm_range_setup() overwrites the
> +		 * shared array; used to map in place if nothing migrates.
> +		 */
> +		if (!ret)
> +			hmm_pfn0 = range.hmm_pfns[0];
> +		migrate_hmm_range_setup(&range);

[Severity: Medium]
When the retry loop in nouveau_dmem_migrate_fault() in
drivers/gpu/drm/nouveau/nouveau_dmem.c repeats after ret == -EBUSY (either
from hmm_range_fault() or from mmu_interval_read_retry()), does migrate.src
(aliased by range.hmm_pfns) retain stale migrate_pfn values from the previous
iteration?

migrate_hmm_range_setup() in mm/migrate_device.c overwrites range.hmm_pfns[i]
with migrate_pfn(...) flags, and neither migrate_vma_pages() nor
migrate_vma_finalize() clears migrate.src[]. Unlike migrate_vma_setup() in
mm/migrate_device.c, which zeroes args->src before calling hmm_range_fault(),
nouveau_dmem_migrate_fault() re-enters hmm_range_fault(&range) without
clearing migrate.src. If the next walk hits an early return path that does
not overwrite *hmm_pfn (for example, softleaf_is_migration() in
hmm_vma_handle_pte()), won't range.hmm_pfns still hold stale migrate_pfn data
from the prior attempt?

> +		if (ret) {
> +			migrate_vma_pages(&migrate);
> +			migrate_vma_finalize(&migrate);
> +			goto unlock;
> +		}
> +
> +		n = nouveau_dmem_migrate_alloc_and_copy(drm, svmm, &migrate,
> +							dma_info, pfns,
> +							&nr_dma, &order);
> +
> +		/* Only a migrated page has an in-flight CE copy to unblock below. */
> +		for (i = 0, migrated = false; i < npages; i++) {
> +			if (migrate.dst[i]) {
> +				migrated = true;
> +				break;
> +			}
> +		}
> +
> +		/*
> +		 * Track a copy fence only when a copy was enqueued; waiting on an
> +		 * empty fence would block until the fault's CTXSW_TIMEOUT.
> +		 */
> +		if (migrated)
> +			nouveau_fence_new(&fence, drm->dmem->migrate.chan);

[Severity: High]
How is access to drm->dmem->migrate.chan synchronized in
nouveau_dmem_migrate_fault() in drivers/gpu/drm/nouveau/nouveau_dmem.c?

Here, nouveau_dmem_migrate_alloc_and_copy() (which calls
nvc0b5_migrate_copy() and nvc0b5_migrate_clear() to push multi-word DMA
commands via PUSH_WAIT(push, ...) on drm->dmem->migrate.chan->chan.push) and
nouveau_fence_new(&fence, drm->dmem->migrate.chan) run while holding only
mmap_read_lock(mm).

Because drm->dmem->migrate.chan aliases drm->ttm.chan (set up in
nouveau_dmem_migrate_init()), can this race with concurrent CPU faults in
nouveau_dmem_migrate_to_ram() (which pushes copy commands under svmm->mutex
and calls nouveau_fence_new() outside svmm->mutex) or concurrent TTM buffer
moves in nouveau_bo_move_m2mf() in drivers/gpu/drm/nouveau/nouveau_bo.c
(which serializes using cli->mutex), corrupting the shared channel pushbuffer
or racing on ++fctx->sequence?

> +		migrate_vma_pages(&migrate);
> +
> +		/*
> +		 * The fault-in path must replay the faulting channel before waiting on
> +		 * the copy fence: with the fault still unreplayed the copy does not
> +		 * make progress. GPU has no mapping so it might refault, until it
> +		 * gets the migrated or mirrored mapping.
> +		 */
> +		if (drm->svm && migrated)
> +			nouveau_svm_fault_replay(drm->svm);
> +
> +		if (migrated)
> +			fret = nouveau_dmem_fence_poll(&fence);
> +
> +		mutex_lock(&svmm->mutex);
> +		if (fret) {
> +			/* Copy did not complete; don't map a half-copied page. */
> +			ret = -EIO;

[Severity: Critical]
If nouveau_dmem_fence_poll(&fence) times out and returns -EBUSY here in
nouveau_dmem_migrate_fault() (or above in nouveau_dmem_migrate_to_ram()) in
drivers/gpu/drm/nouveau/nouveau_dmem.c, doesn't the page migration still get
committed with uncopied data while DMA may still be active?

Because migrate_vma_pages(&migrate) was already called before
nouveau_dmem_fence_poll(&fence), the folio metadata migration has already
succeeded and MIGRATE_PFN_MIGRATE remains set in migrate.src[0]. When fret is
non-zero:

1. dma_unmap_page() is called immediately on the source page (or destination
page in nouveau_dmem_migrate_to_ram()) even though the timed-out copy engine
DMA may still be in flight.

2. migrate_vma_finalize(&migrate) sees MIGRATE_PFN_MIGRATE still set in
migrate.src[0], so it installs a device-private swap PTE for the half-copied
VRAM page into the CPU page tables and frees the valid source system RAM
folio (or in nouveau_dmem_migrate_to_ram(), installs a present CPU PTE to
the stale system RAM page and frees the source VRAM folio).

3. In the GPU fault path, nouveau_dmem_migrate_fault() returns -EIO to
nouveau_range_fault_and_migrate(), and nouveau_svm_fault() in
drivers/gpu/drm/nouveau/nouveau_svm.c then executes:

	ret = nouveau_range_fault_and_migrate(svmm, svm->drm,
					      args, hmm_flags,
					      &notifier);
	if (ret)
		ret = nouveau_range_fault(svmm, svm->drm, args,
					  __struct_size(args),
					  hmm_flags, &notifier);

Won't nouveau_range_fault() walk the CPU page table, find the device-private
swap PTE just installed by migrate_vma_finalize(), and map that half-copied
VRAM page into the GPU anyway?

> +		} else if (mmu_interval_read_retry(notifier, range.notifier_seq)) {
> +			ret = -EBUSY;
> +		} else {
> +			unsigned long map_addr = migrate.start;
> +			unsigned int page_shift = PAGE_SHIFT + order;
> +
> +			/*
> +			 * If nothing migrated might still be able to mirror.
> +			 * In that case use the snapshot hmm_pfn.
> +			 * npages == 1 always today.
> +			 */
> +			if (npages == 1 && !(pfns[0] & NVIF_VMM_PFNMAP_V0_V))
> +				pfns[0] = nouveau_dmem_hmm_pfn_to_map(hmm_pfn0,
> +						start, &map_addr, &page_shift);
> +
> +			__nouveau_pfns_map_locked(svmm, map_addr, pfns, n,
> +						  page_shift);

[Severity: Critical]
Can nouveau_dmem_migrate_fault() in drivers/gpu/drm/nouveau/nouveau_dmem.c
map a VRAM page into the GPU that migrate_vma_pages() rejected and that
migrate_vma_finalize() is about to free?

Before migrate_vma_pages(&migrate) is called,
nouveau_dmem_migrate_alloc_and_copy() -> nouveau_dmem_migrate_copy_one()
allocates a destination VRAM page dpage for any entry with
MIGRATE_PFN_MIGRATE set and populates pfns[0]:

	*pfn = NVIF_VMM_PFNMAP_V0_V | NVIF_VMM_PFNMAP_V0_VRAM |
		((paddr >> PAGE_SHIFT) << NVIF_VMM_PFNMAP_V0_ADDR_SHIFT);

Next, migrate_vma_pages(&migrate) calls __migrate_device_pages() in
mm/migrate_device.c. If migration of the folio is rejected or fails — which
happens for all present file-backed or shared/shmem pages (collected by
hmm_vma_handle_migrate_prepare(), then rejected by __migrate_device_pages()
at !folio_test_anon(folio)), or when folio_free_swap() or
folio_migrate_mapping() fails with -EAGAIN due to extra references —
__migrate_device_pages() aborts the migration by clearing MIGRATE_PFN_MIGRATE
in migrate.src[0].

However, pfns[0] still has NVIF_VMM_PFNMAP_V0_V set from
nouveau_dmem_migrate_copy_one(). Because nouveau_dmem_migrate_fault() checks
!(pfns[0] & NVIF_VMM_PFNMAP_V0_V) without checking whether
migrate.src[0] & MIGRATE_PFN_MIGRATE is still set after migrate_vma_pages():

1. The in-place mirror fallback is skipped.
2. __nouveau_pfns_map_locked() maps the aborted VRAM page dpage into the GPU
VMM.
3. Immediately below, migrate_vma_finalize(&migrate) sees
!(migrate.src[0] & MIGRATE_PFN_MIGRATE), restores the CPU PTE to the original
system RAM folio, and calls folio_put(dst) on dpage, freeing it back to
dmem->free_pages via nouveau_dmem_folio_free() while the GPU retains a live
mapping to it.

Should nouveau_dmem_migrate_fault() check migrate.src[i] & MIGRATE_PFN_MIGRATE
after migrate_vma_pages(&migrate) and clear pfns[i] when migration did not
succeed?

> +
> +			*out_pfn0 = pfns[0];
> +			*out_addr = map_addr;
> +			*out_shift = page_shift;
> +		}
> +		mutex_unlock(&svmm->mutex);
> +
> +		if (fret)
> +			NV_ERROR(drm,
> +				 "svm: migrate copy did not complete (%d) for %lx-%lx\n",
> +				 fret, start, end);
> +
> +		while (nr_dma--) {
> +			dma_unmap_page(drm->dev->dev, dma_info[nr_dma].dma_addr,
> +				       dma_info[nr_dma].size, DMA_BIDIRECTIONAL);
> +		}
> +		migrate_vma_finalize(&migrate);
> +unlock:
> +		mmap_read_unlock(mm);
> +	} while (ret == -EBUSY && !time_after(jiffies, timeout));
> +
> +	/* Nothing migrated or mappable in place, fallback to nouveau_range_fault(). */
> +	if (!ret && !(pfns[0] & NVIF_VMM_PFNMAP_V0_V))
> +		ret = -ENOENT;

[Severity: High]
Why is __nouveau_pfns_map_locked() called before checking whether
pfns[0] & NVIF_VMM_PFNMAP_V0_V is set in nouveau_dmem_migrate_fault() in
drivers/gpu/drm/nouveau/nouveau_dmem.c?

hmm_range_fault(&range) can return 0 while hmm_pfn0 does not have
HMM_PFN_VALID set — for example:
- when faulting on a PMD-mapped THP or PTE-mapped large folio where
folio_trylock() or folio splitting fails in
hmm_vma_handle_migrate_prepare_pmd() or hmm_vma_walk_pmd() in mm/hmm.c
(setting HMM_PFN_ERROR and returning 0),
- when faulting on the huge zero folio in
hmm_vma_handle_migrate_prepare_pmd(), or
- on a read fault to the shared zero page where
hmm_vma_handle_migrate_prepare() sets *hmm_pfn = HMM_PFN_MIGRATE (without
HMM_PFN_VALID) and VRAM allocation in nouveau_dmem_migrate_copy_one() fails.

In those cases, nothing is migrated (pfns[0] is NVIF_VMM_PFNMAP_V0_NONE) and
nouveau_dmem_hmm_pfn_to_map(hmm_pfn0, ...) also returns
NVIF_VMM_PFNMAP_V0_NONE with page_shift = PAGE_SHIFT. Calling
__nouveau_pfns_map_locked() with pfns[0] == NVIF_VMM_PFNMAP_V0_NONE invokes
nvkm_vmm_pfn_map() in drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmm.c with
map == false at 4KB granularity. If the faulting address was already mapped
in the GPU VMM (for instance, mapped read-only prior to a write fault),
nvkm_vmm_pfn_map() splits the GPU VMA and unmaps the 4KB range; and if the
existing GPU VMM mapping was a 2MB page (vma->refd at shift 21 with no 4KB
page table allocated), walking down to shift 12 with ref == false in
nvkm_vmm_iter() leaves it.pt[0] == NULL and dereferences it in
nvkm_vmm_unref_ptes().

Should __nouveau_pfns_map_locked() only be called when
pfns[0] & NVIF_VMM_PFNMAP_V0_V is set?

> +
> +	nouveau_pfns_free(pfns);
> +out_free_dma:
> +	kfree(dma_info);
> +out_free_dst:
> +	kfree(migrate.dst);
> +out_free_src:
> +	kfree(migrate.src);
> +	return ret;
> +}
[ ... ]
> diff --git a/drivers/gpu/drm/nouveau/nouveau_svm.c b/drivers/gpu/drm/nouveau/nouveau_svm.c
> index 58735446d7832..83ea480abcf58 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_svm.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_svm.c
[ ... ]
> @@ -78,6 +79,13 @@ struct nouveau_svm {
>  #define SVM_DBG(s,f,a...) NV_DEBUG((s)->drm, "svm: "f"\n", ##a)
>  #define SVM_ERR(s,f,a...) NV_WARN((s)->drm, "svm: "f"\n", ##a)
>  
> +/* When set, migrate the faulting range to VRAM on GPU fault (default off). */
> +static bool nouveau_svm_migrate_on_fault;
> +module_param_named(svm_migrate_on_fault, nouveau_svm_migrate_on_fault, bool,
> +		   0644);
> +MODULE_PARM_DESC(svm_migrate_on_fault,
> +		 "migrate system memory to VRAM on GPU fault (default off)");

[Severity: Low]
This adds the nouveau.svm_migrate_on_fault module/boot parameter in
drivers/gpu/drm/nouveau/nouveau_svm.c. Should this parameter also be
documented in Documentation/admin-guide/kernel-parameters.txt?

[ ... ]
> @@ -514,6 +523,14 @@ static bool nouveau_svm_range_invalidate(struct mmu_interval_notifier *mni,
>  	    range->owner == sn->svmm->vmm->cli->drm->dev)
>  		return true;
>  
> +	/*
> +	 * Ignore our own migrate-on-fault invalidation; otherwise its
> +	 * MMU_NOTIFY_MIGRATE bumps the seq and read_retry() spins to timeout.
> +	 */
> +	if (range->event == MMU_NOTIFY_MIGRATE &&
> +	    range->owner == sn->svmm->vmm->cli->drm->dev)
> +		return true;

[Severity: Critical]
Doesn't ignoring all MMU_NOTIFY_MIGRATE events with
range->owner == sn->svmm->vmm->cli->drm->dev in
nouveau_svm_range_invalidate() in drivers/gpu/drm/nouveau/nouveau_svm.c also
ignore concurrent migrations triggered by other paths on the same device,
allowing freed VRAM or system RAM pages to be mapped into the GPU VMM?

drm->dev is also passed as pgmap_owner in nouveau_dmem_migrate_to_ram() (on
CPU page faults migrating device-private VRAM pages back to system RAM) and
in nouveau_dmem_migrate_vma() (on nouveau_svmm_bind() migrating system RAM to
VRAM).

Consider two races:

1. A page is currently in device-private VRAM. The GPU faults on it (either
via nouveau_range_fault() when svm_migrate_on_fault is off or on
FAULT_ACCESS_PREFETCH, or via nouveau_dmem_migrate_fault() which only sets
MIGRATE_VMA_SELECT_SYSTEM so hmm_vma_handle_migrate_prepare() skips the
device-private page without locking or referencing it). HMM records the VRAM
page into range.hmm_pfns[0] / hmm_pfn0. Concurrently under shared
mmap_read_lock(mm), a CPU fault on the same address runs
nouveau_dmem_migrate_to_ram(). Its migrate_vma_setup() emits
MMU_NOTIFY_MIGRATE with owner == drm->dev, which
nouveau_svm_range_invalidate() now ignores without taking svmm->mutex or
calling mmu_interval_set_seq(). nouveau_dmem_migrate_to_ram() invalidates the
GPU VMM under svmm->mutex, copies the page to RAM, and frees the VRAM folio
in migrate_vma_finalize(). The GPU fault worker then takes svmm->mutex, sees
mmu_interval_read_retry() == false because invalidate_seq was never bumped,
and maps the freed VRAM page into the GPU VMM.

2. In nouveau_range_fault(), after hmm_range_fault_unlocked_timeout() reads a
system RAM PFN and drops mmap_read_lock(mm), a concurrent
nouveau_dmem_migrate_vma() or nouveau_dmem_migrate_fault() unmaps the
single-mapped PTE in hmm_vma_handle_migrate_prepare() while emitting only
MMU_NOTIFY_MIGRATE with owner == drm->dev, migrates the page to VRAM, and
frees the system RAM folio. Because nouveau_svm_range_invalidate() ignored
that MMU_NOTIFY_MIGRATE event, nouveau_range_fault() sees
mmu_interval_read_retry() == false and maps the freed system RAM page into
the GPU VMM.

[ ... ]
> @@ -922,21 +965,29 @@ nouveau_pfns_free(u64 *pfns)
>  }
>  
>  void
> -nouveau_pfns_map(struct nouveau_svmm *svmm, struct mm_struct *mm,
> -		 unsigned long addr, u64 *pfns, unsigned long npages,
> -		 unsigned int page_shift)
> +__nouveau_pfns_map_locked(struct nouveau_svmm *svmm, unsigned long addr,
> +			  u64 *pfns, unsigned long npages,
> +			  unsigned int page_shift)
>  {
>  	struct nouveau_pfnmap_args *args = nouveau_pfns_to_args(pfns);
>  
> +	lockdep_assert_held(&svmm->mutex);
> +
>  	args->p.addr = addr;
>  	args->p.size = npages << page_shift;
>  	args->p.page = page_shift;
>  
> -	mutex_lock(&svmm->mutex);
> -
>  	nvif_object_ioctl(&svmm->vmm->vmm.object, args,
>  			  struct_size(args, p.phys, npages), NULL);
> +}

[Severity: Medium]
Should __nouveau_pfns_map_locked() in drivers/gpu/drm/nouveau/nouveau_svm.c
propagate the return value of nvif_object_ioctl() to
nouveau_dmem_migrate_fault()?

In nouveau_range_fault() and nouveau_atomic_range_fault(), the return value
of nvif_object_ioctl() is returned to nouveau_svm_fault() so that failed
mappings trigger nouveau_svm_fault_cancel_fault().

In contrast, __nouveau_pfns_map_locked() discards the return value of
nvif_object_ioctl(), and nouveau_dmem_migrate_fault() only checks
!(pfns[0] & NVIF_VMM_PFNMAP_V0_V). If nvkm_vmm_pfn_map() in
drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmm.c returns early with -EINVAL (for
example, when hmm_pfn_to_map_order() returns an order such as a 1GiB PUD or
64KiB hugetlb page that the GPU VMM page table descriptor does not support)
or -ENOENT (when the address is not in a managed GPU VMA), nvkm_vmm_pfn_map()
returns before the loop that clears NVKM_VMM_PFN_V in pfns[0]. Won't
nouveau_dmem_migrate_fault() then falsely return 0, causing
nouveau_svm_fault() to replay the unhandled GPU fault in an infinite fault
loop instead of cancelling it?

>  
> +void
> +nouveau_pfns_map(struct nouveau_svmm *svmm, struct mm_struct *mm,
> +		 unsigned long addr, u64 *pfns, unsigned long npages,
> +		 unsigned int page_shift)
> +{
> +	mutex_lock(&svmm->mutex);
> +	__nouveau_pfns_map_locked(svmm, addr, pfns, npages, page_shift);
>  	mutex_unlock(&svmm->mutex);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261021060506.2377545-1-mpenttil@redhat.com?part=12


      reply	other threads:[~2026-10-22  6:07 UTC|newest]

Thread overview: 28+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-10-21  6:04 [PATCH v16 00/12] migrate on fault for device pages mpenttil
2026-10-21  6:04 ` [PATCH v16 01/12] mm/Kconfig: changes for " mpenttil
2026-10-21  6:04 ` [PATCH v16 02/12] mm: add helper to convert HMM pfn to migrate pfn mpenttil
2026-10-21  6:04 ` [PATCH v16 03/12] mm/hmm: preparations for HMM to participate in migration mpenttil
2026-10-21  7:17   ` Balbir Singh
2026-10-21  8:11     ` Mika Penttilä
2026-10-22  6:06   ` sashiko-bot
2026-10-21  6:04 ` [PATCH v16 04/12] mm/hmm: do the plumbing " mpenttil
2026-10-22  6:06   ` sashiko-bot
2026-10-21  6:04 ` [PATCH v16 05/12] mm/hmm: migrate collection in HMM pagewalk - pte level mpenttil
2026-10-21  6:56   ` Balbir Singh
2026-10-21  7:09     ` Mika Penttilä
2026-10-22  6:06   ` sashiko-bot
2026-10-21  6:05 ` [PATCH v16 06/12] mm/hmm: migrate collection in HMM pagewalk - pmd level mpenttil
2026-10-22  6:06   ` sashiko-bot
2026-10-21  6:05 ` [PATCH v16 07/12] mm/hmm: add lazy MMU mode support for migration in HMM pagewalk mpenttil
2026-10-21  6:05 ` [PATCH v16 08/12] mm/hmm: implement rollback for device page " mpenttil
2026-10-22  6:06   ` sashiko-bot
2026-10-21  6:05 ` [PATCH v16 09/12] mm: enable device page migration from " mpenttil
2026-10-21  6:19   ` Balbir Singh
2026-10-21  6:30     ` Mika Penttilä
2026-10-22  6:06   ` sashiko-bot
2026-10-21  6:05 ` [PATCH v16 10/12] lib/test_hmm: add a new testcase for the migrate on fault mpenttil
2026-10-22  6:06   ` sashiko-bot
2026-10-21  6:05 ` [PATCH v16 11/12] Documentation/mm/hmm: document migration through hmm_range_fault() mpenttil
2026-10-22  6:06   ` sashiko-bot
2026-10-21  6:05 ` [PATCH v16 12/12] drm/nouveau/svm: add support for migrate on fault mpenttil
2026-10-22  6:06   ` sashiko-bot [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=sashiko-outbox-181060@kernel.org \
    --to=sashiko-bot@kernel.org \
    --cc=Liam.Howlett@oracle.com \
    --cc=akpm@linux-foundation.org \
    --cc=apopple@nvidia.com \
    --cc=balbirs@nvidia.com \
    --cc=dakr@kernel.org \
    --cc=david@kernel.org \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=intel-xe@lists.freedesktop.org \
    --cc=jgg@nvidia.com \
    --cc=leonro@nvidia.com \
    --cc=linux-mm@kvack.org \
    --cc=ljs@kernel.org \
    --cc=lyude@redhat.com \
    --cc=matthew.brost@intel.com \
    --cc=mhocko@suse.com \
    --cc=mpenttil@redhat.com \
    --cc=rppt@kernel.org \
    --cc=sashiko-reviews@lists.linux.dev \
    --cc=surenb@google.com \
    --cc=vbabka@suse.cz \
    --cc=ziy@nvidia.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox