nouveau: fixes for SVM

The patch to add zero page migration to GPU memory inadvertantly included

part of a future change which broke normal page migration to GPU memory by copying too much data and corrupting GPU memory.
Fix this by only copying one page instead of a byte count.

Fixes 9d4296a7d4b3 ("drm/nouveau/nouveau/hmm: fix migrate zero page to GPU")

Signed-off-by Ralph Campbell <[email protected]>

---
  drivers/gpu/drm/nouveau/nouveau_dmem.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

drivers/gpu/drm/nouveau/nouveau_dmem.c +0 -0

index e5c230d9ae24..cc9993837508 100644

Quoted 1 line · Ralph Campbell

The patch to add zero page migration to GPU memory inadvertantly included

inadvertently

Quoted 16 lines

part of a future change which broke normal page migration to GPU memory by copying too much data and corrupting GPU memory.
Fix this by only copying one page instead of a byte count.

Fixes 9d4296a7d4b3 ("drm/nouveau/nouveau/hmm: fix migrate zero page to GPU")

Signed-off-by Ralph Campbell <[email protected]>

---
  drivers/gpu/drm/nouveau/nouveau_dmem.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

drivers/gpu/drm/nouveau/nouveau_dmem.c +0 -0

index e5c230d9ae24..cc9993837508 100644

drivers/gpu/drm/nouveau/nouveau_dmem.c static unsigned long nouveau_dmem_migrate_copy_one(struct nouveau_drm *drm, · line 556

Quoted 8 lines

drivers/gpu/drm/nouveau/nouveau_dmem.c +1 -1

@@ -550,7 +550,7 @@ static unsigned long nouveau_dmem_migrate_copy_one(struct nouveau_drm *drm,
                                       DMA_BIDIRECTIONAL);
              if (dma_mapping_error(dev, *dma_addr))
                      goto out_free_page;
-             if (drm->dmem->migrate.copy_func(drm, page_size(spage),
+             if (drm->dmem->migrate.copy_func(drm, 1,
                      NOUVEAU_APER_VRAM, paddr, NOUVEAU_APER_HOST, *dma_addr))
                      goto out_dma_unmap;
      } else {

I Am Not A Nouveau Expert, nor is it really clear to me how page_size(spage) came to contain something other than a page's worth of byte count, but this fix looks accurate to me. It's better for maintenance, too, because the function never intends to migrate "some number of bytes". It intends to migrate exactly one page.

Hope I'm not missing something fundamental, but:

Reviewed-by John Hubbard <[email protected]

thanks,
--
John Hubbard
NVIDIA
_______________________________________________
Nouveau mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/nouveau

Quoted 38 lines · John Hubbard

Quoted 1 line · Ralph Campbell

The patch to add zero page migration to GPU memory inadvertantly included

inadvertently

Quoted 16 lines

part of a future change which broke normal page migration to GPU memory by copying too much data and corrupting GPU memory.
Fix this by only copying one page instead of a byte count.

Fixes 9d4296a7d4b3 ("drm/nouveau/nouveau/hmm: fix migrate zero page to GPU")

Signed-off-by Ralph Campbell <[email protected]>

---
  drivers/gpu/drm/nouveau/nouveau_dmem.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

drivers/gpu/drm/nouveau/nouveau_dmem.c +0 -0

index e5c230d9ae24..cc9993837508 100644

drivers/gpu/drm/nouveau/nouveau_dmem.c static unsigned long nouveau_dmem_migrate_copy_one(struct nouveau_drm *drm, · line 556

Quoted 8 lines

drivers/gpu/drm/nouveau/nouveau_dmem.c +1 -1

@@ -550,7 +550,7 @@ static unsigned long nouveau_dmem_migrate_copy_one(struct nouveau_drm *drm,
                                       DMA_BIDIRECTIONAL);
              if (dma_mapping_error(dev, *dma_addr))
                      goto out_free_page;
-             if (drm->dmem->migrate.copy_func(drm, page_size(spage),
+             if (drm->dmem->migrate.copy_func(drm, 1,
                      NOUVEAU_APER_VRAM, paddr, NOUVEAU_APER_HOST, *dma_addr))
                      goto out_dma_unmap;
      } else {

I Am Not A Nouveau Expert, nor is it really clear to me how page_size(spage) came to contain something other than a page's worth of byte count, but this fix looks accurate to me. It's better for maintenance, too, because the function never intends to migrate "some number of bytes". It intends to migrate exactly one page.

Quoted 1 line

Hope I'm not missing something fundamental, but:

I'm actually a bit confused here too. Because, it *looks* like the function takes a byte count, not a page count, and unless I'm missing something too, it's setup the copy class for a byte count also.

Quoted 13 lines

Reviewed-by John Hubbard <[email protected]

thanks,
--
John Hubbard
NVIDIA
_______________________________________________
Nouveau mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/nouveau

The OpenCL function clEnqueueSVMMigrateMem(), without any flags, will migrate memory in the given address range to device private memory. The source pages might already have been migrated to device private memory. In that case, the source struct page is not checked to see if it is a device private page and incorrectly computes the GPU's physical address of local memory leading to data corruption.
Fix this by checking the source struct page and computing the correct physical address.

Signed-off-by Ralph Campbell <[email protected]>

---
  drivers/gpu/drm/nouveau/nouveau_dmem.c | 8 ++++++++
  1 file changed, 8 insertions(+)

drivers/gpu/drm/nouveau/nouveau_dmem.c +7 -0

index cc9993837508..f6a806ba3caa 100644
@@ -540,6 +540,12 @@ static unsigned long nouveau_dmem_migrate_copy_one(struct nouveau_drm *drm,
  	if (!(src & MIGRATE_PFN_MIGRATE))
  		goto out;
 
+	if (spage && is_device_private_page(spage)) {
+		paddr = nouveau_dmem_page_addr(spage);
+		*dma_addr = DMA_MAPPING_ERROR;
+		goto done;
+	}
+
  	dpage = nouveau_dmem_page_alloc_locked(drm);
  	if (!dpage)
  		goto out;

@@ -560,6 +566,7 @@ static unsigned long nouveau_dmem_migrate_copy_one(struct nouveau_drm *drm,
  			goto out_free_page;
  	}
 
+done:
  	*pfn = NVIF_VMM_PFNMAP_V0_V | NVIF_VMM_PFNMAP_V0_VRAM |
  		((paddr >> PAGE_SHIFT) << NVIF_VMM_PFNMAP_V0_ADDR_SHIFT);
  	if (src & MIGRATE_PFN_WRITE)

Quoted 38 lines · Ralph Campbell

The OpenCL function clEnqueueSVMMigrateMem(), without any flags, will migrate memory in the given address range to device private memory. The source pages might already have been migrated to device private memory. In that case, the source struct page is not checked to see if it is a device private page and incorrectly computes the GPU's physical address of local memory leading to data corruption.
Fix this by checking the source struct page and computing the correct physical address.

Signed-off-by Ralph Campbell <[email protected]>

---
  drivers/gpu/drm/nouveau/nouveau_dmem.c | 8 ++++++++

  1 file changed, 8 insertions(+)

drivers/gpu/drm/nouveau/nouveau_dmem.c +7 -0

index cc9993837508..f6a806ba3caa 100644
@@ -540,6 +540,12 @@ static unsigned long nouveau_dmem_migrate_copy_one(struct nouveau_drm *drm,
       if (!(src & MIGRATE_PFN_MIGRATE))
           goto out;
+    if (spage && is_device_private_page(spage)) {
+        paddr = nouveau_dmem_page_addr(spage);
+        *dma_addr = DMA_MAPPING_ERROR;
+        goto done;
+    }
+
       dpage = nouveau_dmem_page_alloc_locked(drm);
       if (!dpage)
           goto out;

@@ -560,6 +566,7 @@ static unsigned long nouveau_dmem_migrate_copy_one(struct nouveau_drm *drm,
               goto out_free_page;
       }
+done:
       *pfn = NVIF_VMM_PFNMAP_V0_V | NVIF_VMM_PFNMAP_V0_VRAM |
           ((paddr >> PAGE_SHIFT) << NVIF_VMM_PFNMAP_V0_ADDR_SHIFT);
       if (src & MIGRATE_PFN_WRITE)

drivers/gpu/drm/nouveau/nouveau_dmem.c nouveau_dmem_migrate_vma(struct nouveau_drm *drm, · line 625

Quoted 4 lines

drivers/gpu/drm/nouveau/nouveau_dmem.c +1 -0

@@ -615,6 +622,7 @@ nouveau_dmem_migrate_vma(struct nouveau_drm *drm,
       struct migrate_vma args = {
           .vma        = vma,
           .start        = start,
+        .src_owner    = drm->dev,

Hi Ralph,

This .src_owner setting does look like a required fix, but it seems like a completely separate fix from what is listed in this patch's commit description, right? (It feels like a casualty of rearranging the patches.)

thanks,

The OpenCL function clEnqueueSVMMigrateMem(), without any flags, will migrate memory in the given address range to device private memory. The source pages might already have been migrated to device private memory. In that case, the source struct page is not checked to see if it is a device private page and incorrectly computes the GPU's physical address of local memory leading to data corruption.
Fix this by checking the source struct page and computing the correct physical address.

Quoted 8 lines · Ralph Campbell

The OpenCL function clEnqueueSVMMigrateMem(), without any flags, will migrate memory in the given address range to device private memory. The source pages might already have been migrated to device private memory. In that case, the source struct page is not checked to see if it is a device private page and incorrectly computes the GPU's physical address of local memory leading to data corruption.
Fix this by checking the source struct page and computing the correct physical address.

I'm really worried about all this delicate code to fix the mixed ranges. Can't we make it clear at the migrate_vma_* level if we want to migrate from or two device private memory, and then skip all the work for regions of memory that already are in the right place? This might be a little more work initially, but I think it leads to a much better API.

Quoted 10 lines · Christoph Hellwig

Quoted 8 lines · Ralph Campbell

The OpenCL function clEnqueueSVMMigrateMem(), without any flags, will migrate memory in the given address range to device private memory. The source pages might already have been migrated to device private memory. In that case, the source struct page is not checked to see if it is a device private page and incorrectly computes the GPU's physical address of local memory leading to data corruption.
Fix this by checking the source struct page and computing the correct physical address.

Quoted 7 lines

I'm really worried about all this delicate code to fix the mixed ranges.  Can't we make it clear at the migrate_vma_* level if we want to migrate from or two device private memory, and then skip all the work for regions of memory that already are in the right place?  This might be a little more work initially, but I think it leads to a much better API.

The current code does encode the direction with src_owner != NULL meaning device private to system memory and src_owner == NULL meaning system memory to device private memory. This patch would obviously defeat that so perhaps a flag could be added to the struct migrate_vma to indicate the direction but I'm unclear how that makes things less delicate. Can you expand on what you are worried about?

The issue with invalidations might be better addressed by letting the device driver handle device private page TLB invalidations when migrating to system memory and changing migrate_vma_setup() to only invalidate CPU TLB entries for normal pages being migrated to device private memory. If a page isn't migrating, it seems inefficient to invalidate those TLB entries.

Any other suggestions?

Making sure to include linux-mm and Bharata B Rao for IBM's use of migrate_vma*().

Quoted 27 lines · Ralph Campbell

Quoted 10 lines · Christoph Hellwig

Quoted 8 lines · Ralph Campbell

The OpenCL function clEnqueueSVMMigrateMem(), without any flags, will migrate memory in the given address range to device private memory. The source pages might already have been migrated to device private memory. In that case, the source struct page is not checked to see if it is a device private page and incorrectly computes the GPU's physical address of local memory leading to data corruption.
Fix this by checking the source struct page and computing the correct physical address.

Quoted 7 lines

I'm really worried about all this delicate code to fix the mixed ranges.  Can't we make it clear at the migrate_vma_* level if we want to migrate from or two device private memory, and then skip all the work for regions of memory that already are in the right place?  This might be a little more work initially, but I think it leads to a much better API.

The current code does encode the direction with src_owner != NULL meaning device private to system memory and src_owner == NULL meaning system memory to device private memory. This patch would obviously defeat that so perhaps a flag could be added to the struct migrate_vma to indicate the direction but I'm unclear how that makes things less delicate. Can you expand on what you are worried about?

Quoted 8 lines

The issue with invalidations might be better addressed by letting the device driver handle device private page TLB invalidations when migrating to system memory and changing migrate_vma_setup() to only invalidate CPU TLB entries for normal pages being migrated to device private memory. If a page isn't migrating, it seems inefficient to invalidate those TLB entries.

Any other suggestions?

After a night's sleep, I think this might work. What do others think?

1) Add a new MMU_NOTIFY_MIGRATE enum to mmu_notifier_event.

2) Change migrate_vma_collect() to use the new MMU_NOTIFY_MIGRATE event type.

3) Modify nouveau_svmm_invalidate_range_start() to simply return (no invalidations) for MMU_NOTIFY_MIGRATE mmu notifier callbacks.

Quoted 40 lines · Ralph Campbell

Making sure to include linux-mm and Bharata B Rao for IBM's use of migrate_vma*().

Quoted 27 lines · Ralph Campbell

Quoted 10 lines · Christoph Hellwig

Quoted 8 lines · Ralph Campbell

The OpenCL function clEnqueueSVMMigrateMem(), without any flags, will migrate memory in the given address range to device private memory. The source pages might already have been migrated to device private memory. In that case, the source struct page is not checked to see if it is a device private page and incorrectly computes the GPU's physical address of local memory leading to data corruption.
Fix this by checking the source struct page and computing the correct physical address.

Quoted 7 lines

I'm really worried about all this delicate code to fix the mixed ranges.  Can't we make it clear at the migrate_vma_* level if we want to migrate from or two device private memory, and then skip all the work for regions of memory that already are in the right place?  This might be a little more work initially, but I think it leads to a much better API.

The current code does encode the direction with src_owner != NULL meaning device private to system memory and src_owner == NULL meaning system memory to device private memory. This patch would obviously defeat that so perhaps a flag could be added to the struct migrate_vma to indicate the direction but I'm unclear how that makes things less delicate. Can you expand on what you are worried about?

Quoted 8 lines

The issue with invalidations might be better addressed by letting the device driver handle device private page TLB invalidations when migrating to system memory and changing migrate_vma_setup() to only invalidate CPU TLB entries for normal pages being migrated to device private memory. If a page isn't migrating, it seems inefficient to invalidate those TLB entries.

Any other suggestions?

Quoted 8 lines

After a night's sleep, I think this might work. What do others think?

1) Add a new MMU_NOTIFY_MIGRATE enum to mmu_notifier_event.

2) Change migrate_vma_collect() to use the new MMU_NOTIFY_MIGRATE event type.

3) Modify nouveau_svmm_invalidate_range_start() to simply return (no invalidations) for MMU_NOTIFY_MIGRATE mmu notifier callbacks.

Isn't it a bit of an assumption that migrate_vma_collect() is only used by nouveau itself?

What if some other devices' device_private pages are being migrated?

Jason

The functions nvkm_vmm_ctor() and nvkm_mmu_ptp_get() are not called outside of the file defining them so make them static.

Signed-off-by Ralph Campbell <[email protected]>

---
  drivers/gpu/drm/nouveau/nvkm/subdev/mmu/base.c | 2 +-
  drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmm.c  | 2 +-
  drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmm.h  | 3 ---
  3 files changed, 2 insertions(+), 5 deletions(-)

drivers/gpu/drm/nouveau/nvkm/subdev/mmu/base.c +1 -1

index ee11ccaf0563..de91e9a26172 100644
@@ -61,7 +61,7 @@ nvkm_mmu_ptp_put(struct nvkm_mmu *mmu, bool force, struct nvkm_mmu_pt *pt)
  	kfree(pt);
  }
 
-struct nvkm_mmu_pt *
+static struct nvkm_mmu_pt *
  nvkm_mmu_ptp_get(struct nvkm_mmu *mmu, u32 size, bool zero)
  {
  	struct nvkm_mmu_pt *pt;

drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmm.c +1 -1

index 199f94e15c5f..67b00dcef4b8 100644
@@ -1030,7 +1030,7 @@ nvkm_vmm_ctor_managed(struct nvkm_vmm *vmm, u64 addr, u64 size)
  	return 0;
  }
 
-int
+static int
  nvkm_vmm_ctor(const struct nvkm_vmm_func *func, struct nvkm_mmu *mmu,
  	      u32 pd_header, bool managed, u64 addr, u64 size,
  	      struct lock_class_key *key, const char *name,

drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmm.h +0 -0

index d3f8f916d0db..a2b179568970 100644