[PATCH] nouveau/instmem: use iomapping interface for instmem handling (v2)

[PATCH] nouveau/instmem: use iomapping interface for instmem handling (v2)

[Dave Airlie]

From: Dave Airlie <airlied@redhat.com>

This avoids constant need to ioremap when instobjs move at least
on 64-bit systems.

This create the io mapping on first use, because creating it at init
time causes a resource mapping error, because nouveau hasn't kicked
simpledrm off the hardware yet, but ioremap_wc the whole BAR causes
an overlap with BOOTFB/simpledrm. I think the resource system could
do better here, but it's easier to just delay creating the mapping
until first use.

Signed-off-by: Dave Airlie <airlied@redhat.com>
---
 .../nouveau/nvkm/subdev/gsp/rm/r535/fbsr.c    |  1 +
 .../drm/nouveau/nvkm/subdev/instmem/nv50.c    | 38 +++++++++++++++----
 .../drm/nouveau/nvkm/subdev/instmem/priv.h    |  1 +
 3 files changed, 32 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r535/fbsr.c b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r535/fbsr.c
index 700cea5def35..f128330f30d7 100644
--- a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r535/fbsr.c
+++ b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r535/fbsr.c
@@ -297,6 +297,7 @@ r535_fbsr = {
 static void *
 r535_instmem_dtor(struct nvkm_instmem *imem)
 {
+	nv50_instmem_dtor(imem);
 	kfree(imem->func);
 	return imem;
 }
diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/instmem/nv50.c b/drivers/gpu/drm/nouveau/nvkm/subdev/instmem/nv50.c
index 6fbaa1e5876d..7314650f6d23 100644
--- a/drivers/gpu/drm/nouveau/nvkm/subdev/instmem/nv50.c
+++ b/drivers/gpu/drm/nouveau/nvkm/subdev/instmem/nv50.c
@@ -30,10 +30,14 @@
 #include <subdev/gsp.h>
 #include <subdev/mmu.h>
 
+#include <linux/io-mapping.h>
+
 struct nv50_instmem {
 	struct nvkm_instmem base;
 	u64 addr;
 
+	struct io_mapping iomap;
+
 	/* Mappings that can be evicted when BAR2 space has been exhausted. */
 	struct list_head lru;
 };
@@ -124,7 +128,6 @@ nv50_instobj_kmap(struct nv50_instobj *iobj, struct nvkm_vmm *vmm)
 	struct nv50_instobj *eobj;
 	struct nvkm_memory *memory = &iobj->base.memory;
 	struct nvkm_subdev *subdev = &imem->base.subdev;
-	struct nvkm_device *device = subdev->device;
 	struct nvkm_vma *bar = NULL, *ebar;
 	u64 size = nvkm_memory_size(memory);
 	void *emap;
@@ -155,7 +158,7 @@ nv50_instobj_kmap(struct nv50_instobj *iobj, struct nvkm_vmm *vmm)
 		mutex_unlock(&imem->base.mutex);
 		if (!eobj)
 			break;
-		iounmap(emap);
+		io_mapping_unmap(emap);
 		nvkm_vmm_put(vmm, &ebar);
 	}
 
@@ -172,8 +175,7 @@ nv50_instobj_kmap(struct nv50_instobj *iobj, struct nvkm_vmm *vmm)
 
 	/* Make the mapping visible to the host. */
 	iobj->bar = bar;
-	iobj->map = ioremap_wc(device->func->resource_addr(device, NVKM_BAR2_INST) +
-			       (u32)iobj->bar->addr, size);
+	iobj->map = io_mapping_map_wc(&imem->iomap, (u32)iobj->bar->addr, size);
 	if (!iobj->map) {
 		nvkm_warn(subdev, "PRAMIN ioremap failed\n");
 		nvkm_vmm_put(vmm, &iobj->bar);
@@ -188,6 +190,17 @@ nv50_instobj_map(struct nvkm_memory *memory, u64 offset, struct nvkm_vmm *vmm,
 	return nvkm_memory_map(memory, offset, vmm, vma, argv, argc);
 }
 
+static void
+check_io_mapping(struct nv50_instmem *imem)
+{
+	if (imem->iomap.size == 0) {
+		struct nvkm_device *device = imem->base.subdev.device;
+		io_mapping_init_wc(&imem->iomap,
+				   device->func->resource_addr(device, NVKM_BAR2_INST),
+				   device->func->resource_size(device, NVKM_BAR2_INST));
+	}
+}
+
 static void
 nv50_instobj_release(struct nvkm_memory *memory)
 {
@@ -239,7 +252,9 @@ nv50_instobj_acquire(struct nvkm_memory *memory)
 
 	/* Attempt to get a direct CPU mapping of the object. */
 	if ((vmm = nvkm_bar_bar2_vmm(imem->subdev.device))) {
-		if (!iobj->map)
+
+		check_io_mapping(iobj->imem);
+		if (!iobj->map && iobj->imem->iomap.size)
 			nv50_instobj_kmap(iobj, vmm);
 		map = iobj->map;
 	}
@@ -277,6 +292,7 @@ nv50_instobj_boot(struct nvkm_memory *memory, struct nvkm_vmm *vmm)
 		iobj->lru.next = NULL;
 	}
 
+	check_io_mapping(iobj->imem);
 	nv50_instobj_kmap(iobj, vmm);
 	nvkm_instmem_boot(imem);
 	mutex_unlock(&imem->mutex);
@@ -330,7 +346,7 @@ nv50_instobj_dtor(struct nvkm_memory *memory)
 
 	if (map) {
 		struct nvkm_vmm *vmm = nvkm_bar_bar2_vmm(imem->subdev.device);
-		iounmap(map);
+		io_mapping_unmap(map);
 		if (likely(vmm)) /* Can be NULL during BAR destructor. */
 			nvkm_vmm_put(vmm, &bar);
 	}
@@ -406,10 +422,13 @@ nv50_instmem_fini(struct nvkm_instmem *base)
 	nv50_instmem(base)->addr = ~0ULL;
 }
 
-static void *
+void *
 nv50_instmem_dtor(struct nvkm_instmem *base)
 {
-	return nv50_instmem(base);
+	struct nv50_instmem *imem = nv50_instmem(base);
+
+	io_mapping_fini(&imem->iomap);
+	return imem;
 }
 
 static const struct nvkm_instmem_func
@@ -433,8 +452,11 @@ nv50_instmem_new_(const struct nvkm_instmem_func *func,
 
 	if (!(imem = kzalloc_obj(*imem)))
 		return -ENOMEM;
+
+
 	nvkm_instmem_ctor(func, device, type, inst, &imem->base);
 	INIT_LIST_HEAD(&imem->lru);
+
 	*pimem = &imem->base;
 	return 0;
 }
diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/instmem/priv.h b/drivers/gpu/drm/nouveau/nvkm/subdev/instmem/priv.h
index 87bbdd786eaa..b423f01f5fcd 100644
--- a/drivers/gpu/drm/nouveau/nvkm/subdev/instmem/priv.h
+++ b/drivers/gpu/drm/nouveau/nvkm/subdev/instmem/priv.h
@@ -26,6 +26,7 @@ int nv50_instobj_new(struct nvkm_instmem *, u32 size, u32 align, bool zero,
 		     struct nvkm_memory **);
 int nv50_instobj_wrap(struct nvkm_instmem *, struct nvkm_memory *vram,
 		      struct nvkm_memory **bar2);
+void *nv50_instmem_dtor(struct nvkm_instmem *base);
 
 void nvkm_instmem_ctor(const struct nvkm_instmem_func *, struct nvkm_device *,
 		       enum nvkm_subdev_type, int, struct nvkm_instmem *);
-- 
2.54.0

Re: [PATCH] nouveau/instmem: use iomapping interface for instmem handling (v2)

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Unchecked failure in `check_io_mapping` leads to bogus pointer generation in `nv50_instobj_boot`, causing a kernel panic on dereference.
- [Medium] Unconditional call to `io_mapping_fini` during teardown can pass a NULL pointer to `iounmap`, leading to architecture-specific panics or warnings.
- [Medium] Unbounded retries of `ioremap_wc` on hot paths if `check_io_mapping` fails, leading to severe performance degradation.
--

commit 96779776b5649805dcd1602bcce1a5916b4e55a8
Author: Dave Airlie <airlied@redhat.com>

nouveau/instmem: use iomapping interface for instmem handling (v2)

This patch modifies the nouveau driver to use the iomapping interface for
instmem handling. It delays creating the IO mapping until the first use to
avoid resource mapping conflicts with simpledrm during initialization.

> diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/instmem/nv50.c b/drivers/gpu/drm/nouveau/nvkm/subdev/instmem/nv50.c
> index 6fbaa1e5876dc..7314650f6d230 100644
> --- a/drivers/gpu/drm/nouveau/nvkm/subdev/instmem/nv50.c
> +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/instmem/nv50.c
[ ... ]
> @@ -172,8 +175,7 @@ nv50_instobj_kmap(struct nv50_instobj *iobj, struct nvkm_vmm *vmm)
>  
>  	/* Make the mapping visible to the host. */
>  	iobj->bar = bar;
> -	iobj->map = ioremap_wc(device->func->resource_addr(device, NVKM_BAR2_INST) +
> -			       (u32)iobj->bar->addr, size);
> +	iobj->map = io_mapping_map_wc(&imem->iomap, (u32)iobj->bar->addr, size);
>  	if (!iobj->map) {

[Severity: High]
If check_io_mapping() fails to initialize the mapping, imem->iomap.iomem will 
be NULL. io_mapping_map_wc() computes mapping->iomem + offset, which means it
will return NULL + (u32)iobj->bar->addr.

If the offset is greater than 0, this results in a non-NULL but invalid 
pointer. The check if (!iobj->map) will fail to catch it. Can this lead 
to a kernel panic when the bogus pointer is later dereferenced?

>  		nvkm_warn(subdev, "PRAMIN ioremap failed\n");
>  		nvkm_vmm_put(vmm, &iobj->bar);
> @@ -188,6 +190,17 @@ nv50_instobj_map(struct nvkm_memory *memory, u64 offset, struct nvkm_vmm *vmm,
>  	return nvkm_memory_map(memory, offset, vmm, vma, argv, argc);
>  }
>  
> +static void
> +check_io_mapping(struct nv50_instmem *imem)
> +{
> +	if (imem->iomap.size == 0) {
> +		struct nvkm_device *device = imem->base.subdev.device;
> +		io_mapping_init_wc(&imem->iomap,
> +				   device->func->resource_addr(device, NVKM_BAR2_INST),
> +				   device->func->resource_size(device, NVKM_BAR2_INST));

[Severity: Medium]
If io_mapping_init_wc() fails, it returns NULL and leaves imem->iomap.size 
as 0. 

Since check_io_mapping() doesn't record the failure or return an error code, 
will the driver endlessly retry this expensive ioremap_wc() operation on 
subsequent calls in hot paths (like nv50_instobj_acquire()) because 
imem->iomap.size is still 0?

> +	}
> +}
> +
[ ... ]
> @@ -277,6 +292,7 @@ nv50_instobj_boot(struct nvkm_memory *memory, struct nvkm_vmm *vmm)
>  		iobj->lru.next = NULL;
>  	}
>  
> +	check_io_mapping(iobj->imem);
>  	nv50_instobj_kmap(iobj, vmm);

[Severity: High]
Since check_io_mapping() does not return an error status, we unconditionally
call nv50_instobj_kmap() here even if the lazy initialization failed. 
Should this verify that the mapping initialization actually succeeded before
proceeding, to prevent generating the bogus pointer mentioned above?

>  	nvkm_instmem_boot(imem);
>  	mutex_unlock(&imem->mutex);
[ ... ]
> @@ -406,10 +422,13 @@ nv50_instmem_fini(struct nvkm_instmem *base)
>  	nv50_instmem(base)->addr = ~0ULL;
>  }
>  
> -static void *
> +void *
>  nv50_instmem_dtor(struct nvkm_instmem *base)
>  {
> -	return nv50_instmem(base);
> +	struct nv50_instmem *imem = nv50_instmem(base);
> +
> +	io_mapping_fini(&imem->iomap);

[Severity: Medium]
Is it safe to call this unconditionally? If the IO mapping was never 
initialized (e.g., if probe failed early, or check_io_mapping() was never 
called), imem->iomap.iomem will be NULL. 

Calling io_mapping_fini() will invoke iounmap(NULL), which can trigger 
warnings or panics on some architectures.

> +	return imem;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260706022812.834677-1-airlied@gmail.com?part=1