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

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

[Dave Airlie]

From: Dave Airlie <airlied@redhat.com>

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

Signed-off-by: Dave Airlie <airlied@redhat.com>
---
 .../drm/nouveau/nvkm/subdev/instmem/nv50.c    | 25 ++++++++++++++-----
 1 file changed, 19 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/instmem/nv50.c b/drivers/gpu/drm/nouveau/nvkm/subdev/instmem/nv50.c
index 6fbaa1e5876d..929246bde72d 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, iobj->bar->addr, size);
 	if (!iobj->map) {
 		nvkm_warn(subdev, "PRAMIN ioremap failed\n");
 		nvkm_vmm_put(vmm, &iobj->bar);
@@ -330,7 +332,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);
 	}
@@ -409,7 +411,10 @@ nv50_instmem_fini(struct nvkm_instmem *base)
 static 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 +438,16 @@ nv50_instmem_new_(const struct nvkm_instmem_func *func,
 
 	if (!(imem = kzalloc_obj(*imem)))
 		return -ENOMEM;
+
+	if (!io_mapping_init_wc(&imem->iomap, device->func->resource_addr(device, NVKM_BAR2_INST),
+				device->func->resource_size(device, NVKM_BAR2_INST))) {
+		kfree(imem);
+		return -ENOMEM;
+	}
+
 	nvkm_instmem_ctor(func, device, type, inst, &imem->base);
 	INIT_LIST_HEAD(&imem->lru);
+
 	*pimem = &imem->base;
 	return 0;
 }
-- 
2.54.0

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] Passing `iobj->bar->addr` to `io_mapping_map_wc` without truncating the VMM base address causes an out-of-bounds pointer calculation on 64-bit systems.
- [High] Unconditional io_mapping_init_wc() of the entire NVKM_BAR2_INST breaks driver probe on constrained architectures and completely bypasses the existing graceful BAR0 fallback when BAR2 is missing or unmappable.
- [High] The patch introduces a type confusion vulnerability and potential resource leak in `nv50_instmem_dtor` due to structural assumptions.
--

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

nouveau/instmem: use iomapping interface for instmem handling

This patch transitions the nouveau driver's instmem handling to use the
io_mapping API. Instead of individually mapping and unmapping instobjs
with ioremap_wc(), it maps the entire BAR2 region upfront during
initialization and maps offsets as needed.

> diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/instmem/nv50.c b/drivers/gpu/drm/nouveau/nvkm/subdev/instmem/nv50.c
> index 6fbaa1e5876dc..929246bde72d5 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, iobj->bar->addr, size);

[Severity: Critical]
Is it possible this leads to an out-of-bounds pointer calculation on 64-bit
systems?

The bar2 VMM is initialized with a starting address of 0x0100000000ULL (4GB),
meaning iobj->bar->addr returns an address >= 4GB. Before this patch, the
u32 cast implicitly truncated the 4GB base to extract just the internal offset.

Since io_mapping_map_wc() takes an unsigned long offset, passing the full
64-bit u64 value will preserve the 4GB base, resulting in a pointer that is
out of bounds by at least 4GB.

>  	if (!iobj->map) {
>  		nvkm_warn(subdev, "PRAMIN ioremap failed\n");
>  		nvkm_vmm_put(vmm, &iobj->bar);

[ ... ]

> @@ -409,7 +411,10 @@ nv50_instmem_fini(struct nvkm_instmem *base)
>  static 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;

[Severity: High]
What happens when this destructor is called for derived classes?

In nv50_instmem_new(), if nvkm_gsp_rm() is enabled, the creation is delegated
to r535_instmem_new(&nv50_instmem, ...). This reuses the nv50_instmem
function table (which contains this destructor) but allocates a different
structure.

If the base pointer does not belong to a struct nv50_instmem, won't calling
io_mapping_fini(&imem->iomap) result in type confusion and cause iounmap()
to operate on garbage memory?

>  }

[ ... ]

> @@ -433,8 +438,16 @@ nv50_instmem_new_(const struct nvkm_instmem_func *func,
>  
>  	if (!(imem = kzalloc_obj(*imem)))
>  		return -ENOMEM;
> +
> +	if (!io_mapping_init_wc(&imem->iomap, device->func->resource_addr(device, NVKM_BAR2_INST),
> +				device->func->resource_size(device, NVKM_BAR2_INST))) {
> +		kfree(imem);
> +		return -ENOMEM;
> +	}
> +

[Severity: High]
Will this unconditional initialization break driver probe on systems where the
full BAR2 region cannot be mapped?

Previously, small ioremap_wc() allocations were made on demand, and if they
failed, the driver gracefully fell back to accessing memory safely via the
BAR0 window.

By attempting to map the entire NVKM_BAR2_INST resource upfront and aborting
on failure, it seems the fallback mechanism is bypassed entirely, which could
cause initialization failures on 32-bit architectures or if BAR2 is
exceptionally large.

>  	nvkm_instmem_ctor(func, device, type, inst, &imem->base);
>  	INIT_LIST_HEAD(&imem->lru);

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