On Mon, Jun 30, 2025 at 03:33:43PM +0200, Christoph Hellwig wrote:

Hi Ben,

...

[ 32.857521] iommu_dma_unmap_page+0xc4/0xe8 (P)

Can you resolve this to a source location for me. i.e.

gdb vmlinux

l *(iommu_dma_unmap_page+0xc4)

Also what IOMMU driver is this device using? It looks like it might not support a 4k IOMMU page size.

I think the PRP handling is broken. At the very least, handling the last element is wrong if it appears at the end of the list, so I think we need something like this:

--- --- a/drivers/nvme/host/pci.c +++ b/drivers/nvme/host/pci.c @@ -701,7 +701,7 @@ static void nvme_free_prps(struct request *req) prp_list = iod->descriptors[desc]; do { dma_unmap_page(dma_dev, dma_addr, dma_len, dir); - if (i == NVME_CTRL_PAGE_SIZE >> 3) { + if (i == NVME_CTRL_PAGE_SIZE >> 3 && length > NVME_CTRL_PAGE_SIZE) { prp_list = iod->descriptors[++desc]; i = 0; } --

But even that, the PRP setup doesn't match the teardown. We're calling dma_map_page() on each PRP even if consecutive PRP's came from the same dma mapping segment. So even if it had been coalesced, but if the device doesn't support SGLs, then it would use the prp unmap path.

Ben, can you check if your device supports sgl?

# nvme id-ctrl /dev/nvme0 | grep sgl

On Mon, 30 Jun 2025 at 21:25, Keith Busch kbusch@kernel.org wrote:

On Mon, Jun 30, 2025 at 03:33:43PM +0200, Christoph Hellwig wrote:

...

Hi Ben,

...

[ 32.857521] iommu_dma_unmap_page+0xc4/0xe8 (P)

Can you resolve this to a source location for me. i.e.

gdb vmlinux

l *(iommu_dma_unmap_page+0xc4)

Also what IOMMU driver is this device using? It looks like it might not support a 4k IOMMU page size.

I think the PRP handling is broken. At the very least, handling the last element is wrong if it appears at the end of the list, so I think we need something like this:

---

--- a/drivers/nvme/host/pci.c +++ b/drivers/nvme/host/pci.c @@ -701,7 +701,7 @@ static void nvme_free_prps(struct request *req) prp_list = iod->descriptors[desc]; do { dma_unmap_page(dma_dev, dma_addr, dma_len, dir);

•           if (i == NVME_CTRL_PAGE_SIZE >> 3) {
  

•           if (i == NVME_CTRL_PAGE_SIZE >> 3 && length > NVME_CTRL_PAGE_SIZE) {
                    prp_list = iod->descriptors[++desc];
                    i = 0;
            }
  

--

But even that, the PRP setup doesn't match the teardown. We're calling dma_map_page() on each PRP even if consecutive PRP's came from the same dma mapping segment. So even if it had been coalesced, but if the device doesn't support SGLs, then it would use the prp unmap path.

Even though SGLs are not supported, I gave you the patch a spin and saw the same stack traces.

Ben, can you check if your device supports sgl?

# nvme id-ctrl /dev/nvme0 | grep sgl

sgl is not supported by the drive.. Here was my output:

--- sgls : 0

Ben

On Mon, Jun 30, 2025 at 02:25:23PM -0600, Keith Busch wrote:

I think the PRP handling is broken. At the very least, handling the last element is wrong if it appears at the end of the list, so I think we need something like this:

Yeah.

But even that, the PRP setup doesn't match the teardown. We're calling dma_map_page() on each PRP even if consecutive PRP's came from the same dma mapping segment. So even if it had been coalesced, but if the device doesn't support SGLs, then it would use the prp unmap path.

Yes, that's broken, and I remember fixing it before. A little digging shows that my fixes disappeared between the oct 30 version of Leon's dma-split branch and the latest one somewhere. Below is what should restore it, but at least when forcing my Intel IOMMU down this path it still has issues with VPTEs already set. So maybe Bob should not try it quite yet. I'll try to get to it, but my availability today and tomorrow is a bit limited.

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c index 38be1505dbd9..02bb5cf5db1a 100644 --- a/drivers/nvme/host/pci.c +++ b/drivers/nvme/host/pci.c @@ -678,40 +678,55 @@ static void nvme_free_prps(struct request *req) enum dma_data_direction dir = rq_dma_dir(req); int length = iod->total_len; dma_addr_t dma_addr; - int i, desc; + int prp_len, i, desc; __le64 *prp_list; + dma_addr_t dma_start; u32 dma_len;

dma_addr = le64_to_cpu(iod->cmd.common.dptr.prp1); - dma_len = min_t(u32, length, - NVME_CTRL_PAGE_SIZE - (dma_addr & (NVME_CTRL_PAGE_SIZE - 1))); - length -= dma_len; + prp_len = NVME_CTRL_PAGE_SIZE - (dma_addr & (NVME_CTRL_PAGE_SIZE - 1)); + prp_len = min(length, prp_len); + length -= prp_len; if (!length) { - dma_unmap_page(dma_dev, dma_addr, dma_len, dir); + dma_unmap_page(dma_dev, dma_addr, prp_len, dir); return; }

+ dma_start = dma_addr; + dma_len = prp_len; + dma_addr = le64_to_cpu(iod->cmd.common.dptr.prp2); + if (length <= NVME_CTRL_PAGE_SIZE) { - dma_unmap_page(dma_dev, dma_addr, dma_len, dir); - dma_addr = le64_to_cpu(iod->cmd.common.dptr.prp2); - dma_unmap_page(dma_dev, dma_addr, length, dir); - return; + if (dma_addr != dma_start + dma_len) { + dma_unmap_page(dma_dev, dma_start, dma_len, dir); + dma_start = dma_addr; + dma_len = 0; + } + dma_len += length; + goto done; }

i = 0; desc = 0; prp_list = iod->descriptors[desc]; do { - dma_unmap_page(dma_dev, dma_addr, dma_len, dir); if (i == NVME_CTRL_PAGE_SIZE >> 3) { prp_list = iod->descriptors[++desc]; i = 0; }

dma_addr = le64_to_cpu(prp_list[i++]); - dma_len = min(length, NVME_CTRL_PAGE_SIZE); - length -= dma_len; + if (dma_addr != dma_start + dma_len) { + dma_unmap_page(dma_dev, dma_start, dma_len, dir); + dma_start = dma_addr; + dma_len = 0; + } + prp_len = min(length, NVME_CTRL_PAGE_SIZE); + dma_len += prp_len; + length -= prp_len; } while (length); +done: + dma_unmap_page(dma_dev, dma_start, dma_len, dir); }

static void nvme_free_sgls(struct request *req)

On Tue, Jul 01, 2025 at 05:00:58PM -0400, Keith Busch wrote:

On Tue, Jul 01, 2025 at 03:29:36PM +0200, Christoph Hellwig wrote:

...

Yes, that's broken, and I remember fixing it before. A little digging shows that my fixes disappeared between the oct 30 version of Leon's dma-split branch and the latest one somewhere. Below is what should restore it, but at least when forcing my Intel IOMMU down this path it still has issues with VPTEs already set. So maybe Bob should not try it quite yet. I'll try to get to it, but my availability today and tomorrow is a bit limited.

Let's say we're using ARM64 SMMU configured with 64k granularity like I showed earlier.

Now let's send a read command with 128k transfer, and let's assume the payload is two 64k aligned physical segments, so we have 2 bvecs.

Since nvme's virtual boundary is smaller than the iommu's granule, we won't attempt to coalesce. We instead iommu map each bvec segment individually.

And let's say each segment just so happens to get consecutive IOVA's. The mapping side had done each segment individually, but your proposal here will assume the contiguous dma_addr ranges were done as a single larger mapping. Is that okay?

Not on the DMA API level, and depending on the implementation also not in practice.

I think the idea to reconstruct the dma addresses from PRPs should be considered a failure by now. It works fine for SGLs, but for PRPs we're better off just stashing them away. Bob, can you try something like the patch below? To be fully safe it needs a mempool, and it could use some cleanups, but it does pass testing on my setup here, so I'd love to see if if fixes your issue.

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c index f5cf90ddc3e9..f1242e321a58 100644 --- a/drivers/nvme/host/pci.c +++ b/drivers/nvme/host/pci.c @@ -262,6 +262,11 @@ enum nvme_iod_flags { IOD_SINGLE_SEGMENT = 1U << 2, };

+struct dma_vec { + dma_addr_t addr; + unsigned int len; +}; + /* * The nvme_iod describes the data in an I/O. */ @@ -274,6 +279,8 @@ struct nvme_iod { unsigned int total_len; struct dma_iova_state dma_state; void *descriptors[NVME_MAX_NR_DESCRIPTORS]; + struct dma_vec *dma_vecs; + unsigned int nr_dma_vecs;

dma_addr_t meta_dma; struct sg_table meta_sgt; @@ -676,42 +683,12 @@ static void nvme_free_prps(struct request *req) struct nvme_queue *nvmeq = req->mq_hctx->driver_data; struct device *dma_dev = nvmeq->dev->dev; enum dma_data_direction dir = rq_dma_dir(req); - int length = iod->total_len; - dma_addr_t dma_addr; - int i, desc; - __le64 *prp_list; - u32 dma_len; - - dma_addr = le64_to_cpu(iod->cmd.common.dptr.prp1); - dma_len = min_t(u32, length, - NVME_CTRL_PAGE_SIZE - (dma_addr & (NVME_CTRL_PAGE_SIZE - 1))); - length -= dma_len; - if (!length) { - dma_unmap_page(dma_dev, dma_addr, dma_len, dir); - return; - } - - if (length <= NVME_CTRL_PAGE_SIZE) { - dma_unmap_page(dma_dev, dma_addr, dma_len, dir); - dma_addr = le64_to_cpu(iod->cmd.common.dptr.prp2); - dma_unmap_page(dma_dev, dma_addr, length, dir); - return; - } - - i = 0; - desc = 0; - prp_list = iod->descriptors[desc]; - do { - dma_unmap_page(dma_dev, dma_addr, dma_len, dir); - if (i == NVME_CTRL_PAGE_SIZE >> 3) { - prp_list = iod->descriptors[++desc]; - i = 0; - } + unsigned int i;

- dma_addr = le64_to_cpu(prp_list[i++]); - dma_len = min(length, NVME_CTRL_PAGE_SIZE); - length -= dma_len; - } while (length); + for (i = 0; i < iod->nr_dma_vecs; i++) + dma_unmap_page(dma_dev, iod->dma_vecs[i].addr, + iod->dma_vecs[i].len, dir); + kfree(iod->dma_vecs); }

static void nvme_free_sgls(struct request *req) @@ -770,6 +747,17 @@ static blk_status_t nvme_pci_setup_data_prp(struct request *req, unsigned int prp_len, i; __le64 *prp_list;

+ if (dma_need_unmap(nvmeq->dev->dev)) { + /* XXX: use mempool */ + iod->dma_vecs = kmalloc_array(blk_rq_nr_phys_segments(req), + sizeof(struct dma_vec), GFP_NOIO); + if (!iod->dma_vecs) + return BLK_STS_RESOURCE; + iod->dma_vecs[0].addr = iter->addr; + iod->dma_vecs[0].len = iter->len; + iod->nr_dma_vecs++; + } + /* * PRP1 always points to the start of the DMA transfers. * @@ -793,6 +781,11 @@ static blk_status_t nvme_pci_setup_data_prp(struct request *req, goto bad_sgl; goto done; } + if (dma_need_unmap(nvmeq->dev->dev)) { + iod->dma_vecs[iod->nr_dma_vecs].addr = iter->addr; + iod->dma_vecs[iod->nr_dma_vecs].len = iter->len; + iod->nr_dma_vecs++; + } }

/* @@ -838,6 +831,12 @@ static blk_status_t nvme_pci_setup_data_prp(struct request *req, goto bad_sgl; goto done; } + if (dma_need_unmap(nvmeq->dev->dev)) { + iod->dma_vecs[iod->nr_dma_vecs].addr = + iter->addr; + iod->dma_vecs[iod->nr_dma_vecs].len = iter->len; + iod->nr_dma_vecs++; + } }

/* @@ -1108,6 +1107,7 @@ static blk_status_t nvme_prep_rq(struct request *req) iod->nr_descriptors = 0; iod->total_len = 0; iod->meta_sgt.nents = 0; + iod->nr_dma_vecs = 0;

ret = nvme_setup_cmd(req->q->queuedata, req); if (ret)