[PATCH v2] RDMA/irdma: always route IB_WR_REG_MR through the allocated PBL

[PATCH v2] RDMA/irdma: always route IB_WR_REG_MR through the allocated PBL

[Stefan Metzmacher]

irdma_alloc_mr() always allocates a PBL via irdma_get_pble(), but
irdma_post_send()'s IB_WR_REG_MR handling only pointed the FAST_REGISTER
WQE at that PBL (chunk_size != 0, first_pm_pbl_index) when
iwmr->npages exceeded IRDMA_MIN_PAGES_PER_FMR (1) - leaving chunk_size
at 0 for a single-page (npages == 1) registration, telling hardware to
use direct/PBL-less addressing for a registration that was never
allocated that way.

This reproduces as data corruption / an IOMMU IO_PAGE_FAULT when a
peer later performs an RDMA WRITE/READ against such a single-page
FRWR-registered MR, even though ib_map_mr_sg() itself reports success.
100% reproducible on iWARP, intermittent on RoCEv2, with fs/smb/smbdirect's
own single-page memory registrations on an Intel E810-CQDA2 (irdma).

Intel's current out-of-tree irdma driver (2.6.96) never gates this on
npages - it always sets chunk_size/first_pm_pbl_index based on the
actual PBL level (PBLE_LEVEL_1 vs PBLE_LEVEL_2), matching what
irdma_alloc_mr() actually allocated. Mirror that here instead of the
npages-based heuristic.

Also drop the reg_addr_pa assignment. irdma_sc_mr_fast_register()
packs reg_addr_pa into the FAST_REGISTER WQE's PBLADDR field, which
hardware only consults when chunk_size == 0 (direct/PBL-less
addressing); with chunk_size now always nonzero for this opcode, that
field is never read for IB_WR_REG_MR and the assignment is dead code -
Intel's driver never sets it for this path either (only the separate,
non-FRWR irdma_sc_mr_reg_non_shared() path actually branches on
chunk_size between reg_addr_pa and first_pm_pbl_index). Removing it is
not required for correctness: *palloc->level1.addr is always a
page-aligned address (low 12 bits zero), so it could never have
collided with the first_pm_pbl_index bits sharing those same bit
positions in the WQE word even if some path still read it.

Fixes: b48c24c2d710 ("RDMA/irdma: Implement device supported verb APIs")
Cc: Tatyana Nikolova <tatyana.e.nikolova@intel.com>
Cc: Jason Gunthorpe <jgg@ziepe.ca>
Cc: Leon Romanovsky <leon@kernel.org>
Cc: Mustafa Ismail <mustafa.ismail@intel.com>
Cc: Shiraz Saleem <shiraz.saleem@intel.com>
Cc: linux-rdma@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Assisted-by: Claude:claude-sonnet-5-5
Signed-off-by: Stefan Metzmacher <metze@samba.org>

--

change in v2:
- rebased on linux-rdma/for-next
---
 drivers/infiniband/hw/irdma/verbs.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/infiniband/hw/irdma/verbs.c b/drivers/infiniband/hw/irdma/verbs.c
index 71d7ae7baa47..e3cd38e0b3dd 100644
--- a/drivers/infiniband/hw/irdma/verbs.c
+++ b/drivers/infiniband/hw/irdma/verbs.c
@@ -4286,11 +4286,14 @@ static int irdma_post_send(struct ib_qp *ibqp,
 			stag_info.wr_id = ib_wr->wr_id;
 			stag_info.va = iwmr->ibmr.iova;
 			stag_info.total_len = iwmr->ibmr.length;
-			stag_info.reg_addr_pa = *palloc->level1.addr;
-			stag_info.first_pm_pbl_index = palloc->level1.idx;
-			stag_info.local_fence = true;
-			if (iwmr->npages > IRDMA_MIN_PAGES_PER_FMR)
+			if (palloc->level == PBLE_LEVEL_2) {
+				stag_info.chunk_size = 3;
+				stag_info.first_pm_pbl_index = palloc->level2.root.idx;
+			} else {
 				stag_info.chunk_size = 1;
+				stag_info.first_pm_pbl_index = palloc->level1.idx;
+			}
+			stag_info.local_fence = true;
 			err = irdma_sc_mr_fast_register(&iwqp->sc_qp,
 							&stag_info);
 			break;
-- 
2.43.0

Re: [PATCH v2] RDMA/irdma: always route IB_WR_REG_MR through the allocated PBL

[sashiko-bot]

> irdma_alloc_mr() always allocates a PBL via irdma_get_pble(), but
> irdma_post_send()'s IB_WR_REG_MR handling only pointed the FAST_REGISTER
> WQE at that PBL (chunk_size != 0, first_pm_pbl_index) when
> iwmr->npages exceeded IRDMA_MIN_PAGES_PER_FMR (1) - leaving chunk_size
> at 0 for a single-page (npages == 1) registration, telling hardware to
> use direct/PBL-less addressing for a registration that was never
> allocated that way.
> 
> This reproduces as data corruption / an IOMMU IO_PAGE_FAULT when a
> peer later performs an RDMA WRITE/READ against such a single-page
> FRWR-registered MR, even though ib_map_mr_sg() itself reports success.
> 100% reproducible on iWARP, intermittent on RoCEv2, with fs/smb/smbdirect's
> own single-page memory registrations on an Intel E810-CQDA2 (irdma).
> 
> Intel's current out-of-tree irdma driver (2.6.96) never gates this on
> [ ... ]
> Cc: Shiraz Saleem <shiraz.saleem@intel.com>
> Cc: linux-rdma@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Assisted-by: Claude:claude-sonnet-5-5
> Signed-off-by: Stefan Metzmacher <metze@samba.org>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261012161135.494544-1-metze@samba.org?part=1

Re: [PATCH v2] RDMA/irdma: always route IB_WR_REG_MR through the allocated PBL

[Jacob Moroni]

Hi,

Nice find. I was curious why I have not hit this and I think it's because
the problem might be elsewhere. Or rather, there may be two
problems. Looking at ctrl.c:

    set_64bit_val(wqe, 8,
        FIELD_PREP(IRDMAQPSQ_FIRSTPMPBLIDXHI, temp) |
        FIELD_PREP(IRDMAQPSQ_PBLADDR >> IRDMA_HW_PAGE_SHIFT,
info->reg_addr_pa));

I bet that second FIELD_PREP is supposed to be shifting
`info->reg_addr_pa` and not `IRDMAQPSQ_PBLADDR`.

That incorrectly chops off the upper 12 bits of the DMA addr that the
HW uses when in direct mode for FRWRs. I think it just so happens
to work as long as your DMA addresses/IOVAs are all less than 52 bits
and the lower 12 bits are 0.

If that was fixed, then I think the direct mode would work fine
as long as the MR was originally allocated as PBLE_LEVEL_1.

However, if it was allocated with PBLE_LEVEL_2 (as it would be
for smbdirect which uses max_num_sg == 2048), then it ends up
using a completely incorrect value for `reg_addr_pa` because
`*palloc->level1.addr` ends up being the leaf PBL's HMC index
rather than the page's DMA address like in the level 1 case.

So, I think if you did:

-  FIELD_PREP(IRDMAQPSQ_PBLADDR >> IRDMA_HW_PAGE_SHIFT, info->reg_addr_pa));
+ FIELD_PREP(IRDMAQPSQ_PBLADDR, info->reg_addr_pa >> IRDMA_HW_PAGE_SHIFT));

and made the post_send like:

    if (iwmr->npages <= IRDMA_MIN_PAGES_PER_FMR) {
        stag_info.reg_addr_pa = (palloc->level == PBLE_LEVEL_1) ?
            *palloc->level1.addr : *palloc->level2.leaf[0].addr;
    } else if (palloc->level == PBLE_LEVEL_2) {
        stag_info.chunk_size = 3;
        stag_info.first_pm_pbl_index = palloc->level2.root.idx;
    } else {
        stag_info.chunk_size = 1;
        stag_info.first_pm_pbl_index = palloc->level1.idx;
    }

...then it may all just work, and we don't have to give up the direct
mode optimization.

I'll give it a try later.

Thanks,
Jake