Re: [PATCH rdma-next v4 5/6] RDMA/irdma: Support UMEM attribute for CQ creation

[sashiko-bot@kernel.org]

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 <xuehaohu@google.com>

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/20261030181512.2056056-1-dhu@x6u.co?part=5