Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Medium] RDMA/irdma: wrong DMA direction in irdma_setup_umode_cq() via ib_umem_get_cq_buf() --
commit 8981c5869a8245f4b87a31412a16b517815f3a39 Author: David Hu <[email protected]> RDMA/irdma: Support UMEM attribute for CQ creation This commit adds support for one-step user-mode CQ buffer creation using UVERBS_ATTR_CREATE_CQ_BUF_UMEM and IRDMA_IB_ATTR_CREATE_CQ_SHADOW_BUF_UMEM, sets up hardware PBLEs directly from the pinned UMEMs, and adds UMEM and PBLE cleanup in irdma_cq_free_rsrc() and irdma_resize_cq(). > diff --git a/drivers/infiniband/hw/irdma/verbs.c > b/drivers/infiniband/hw/irdma/verbs.c > index 9f4905fdb791a..41e56ab8aa6df 100644 > --- a/drivers/infiniband/hw/irdma/verbs.c > +++ b/drivers/infiniband/hw/irdma/verbs.c [ ... ] > @@ -2767,26 +2793,87 @@ static int irdma_setup_umode_cq(struct irdma_device > *iwdev, > if (err_code) > return err_code; > > - spin_lock_irqsave(&ucontext->cq_reg_mem_list_lock, flags); > - iwcq->iwpbl = irdma_get_pbl((unsigned long)req.user_cq_buf, > - &ucontext->cq_reg_mem_list); > - spin_unlock_irqrestore(&ucontext->cq_reg_mem_list_lock, flags); > - if (!iwcq->iwpbl) > - return -EPROTO; > + cqe_64byte_ena = dev->hw_attrs.uk_attrs.feature_flags & > IRDMA_FEATURE_64_BYTE_CQE ? > + true : false; > + cqe_size = cqe_64byte_ena ? 64 : 32; > + > + if (uverbs_attr_is_valid(attrs, UVERBS_ATTR_CREATE_CQ_BUF_UMEM)) { > + struct ib_umem *umem; > + struct irdma_mem_reg_req cq_req = {}; > + u8 shadow_pgcnt = (rf->sc_dev.hw_attrs.uk_attrs.feature_flags & > + IRDMA_FEATURE_CQ_RESIZE) ? 0 : > IRDMA_SHADOW_PGCNT; > + u32 min_size; > + > + cq_req.cq_pages = DIV_ROUND_UP(ukinfo->cq_size * cqe_size, > + IRDMA_HW_PAGE_SIZE); > + min_size = (cq_req.cq_pages + shadow_pgcnt) << > IRDMA_HW_PAGE_SHIFT; > + > + umem = ib_umem_get_cq_buf(&iwdev->ibdev, attrs, min_size, > + IB_ACCESS_LOCAL_WRITE); [Severity: Medium] When IRDMA_FEATURE_CQ_RESIZE is not set in rf->sc_dev.hw_attrs.uk_attrs.feature_flags, the UMEM passed via UVERBS_ATTR_CREATE_CQ_BUF_UMEM backs both the CQ ring and the trailing shadow area page (shadow_pgcnt is IRDMA_SHADOW_PGCNT, and irdma_setup_umode_cq() later sets info->shadow_area_pa = cqmr->shadow from that same UMEM). However, ib_umem_get_cq_buf() in drivers/infiniband/core/umem.c hardcodes DMA_FROM_DEVICE instead of deriving DMA_BIDIRECTIONAL from IB_ACCESS_LOCAL_WRITE: drivers/infiniband/core/umem.c:ib_umem_get_cq_buf() { return ib_umem_get_from_attrs(device, attrs, UVERBS_ATTR_CREATE_CQ_BUF_UMEM, uverbs_create_cq_get_buffer_desc, size, access, DMA_FROM_DEVICE); } Since the CPU writes the CQ head index to the shadow area and hardware reads shadow_area_pa via DMA, mapping this UMEM with DMA_FROM_DEVICE omits IOMMU read permissions (and skips CPU-to-device bounce buffer syncs). Should irdma_setup_umode_cq() use ib_umem_get_attr() with UVERBS_ATTR_CREATE_CQ_BUF_UMEM when IRDMA_FEATURE_CQ_RESIZE is not set (or unconditionally, as noted in the commit message) so the buffer is mapped with DMA_BIDIRECTIONAL? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=5
