Re: [PATCH rdma-next v2 4/4] RDMA/bnxt_re: Direct Verbs: Support CQ and QP verbs

Sriharsha Basavapatna
2025-11-19T16:14:27Z

[-- Attachment #1: Type: text/plain, Size: 5459 bytes --]

On Mon, Nov 17, 2025 at 11:03 PM Jason Gunthorpe <jgg@ziepe.ca> wrote:
>
> On Tue, Nov 04, 2025 at 12:53:20PM +0530, Sriharsha Basavapatna wrote:
> > The following Direct Verb (DV) methods have been implemented in
> > this patch.
> >
> > CQ Direct Verbs:
> > ----------------
> > - BNXT_RE_METHOD_DV_CREATE_CQ:
> > Create a CQ of requested size (cqe). The application must have
> > already registered this memory with the driver using DV_UMEM_REG.
> > The CQ umem-handle and umem-offset are passed to the driver. The
> > driver now maps/pins the CQ user memory and registers it with the
> > hardware. The driver returns a CQ-handle to the application.
> >
> > - BNXT_RE_METHOD_DV_DESTROY_CQ:
> > Destroy the DV_CQ specified by the CQ-handle; unmap the user memory.
> >
> > QP Direct Verbs:
> > ----------------
> > - BNXT_RE_METHOD_DV_CREATE_QP:
> > Create a QP using specified params (struct bnxt_re_dv_create_qp_req).
> > The application must have already registered SQ/RQ memory with the
> > driver using DV_UMEM_REG. The SQ/RQ umem-handle and umem-offset are
> > passed to the driver. The driver now maps/pins the SQ/RQ user memory
> > and registers it with the hardware. The driver returns a QP-handle to
> > the application.
> >
> > - BNXT_RE_METHOD_DV_DESTROY_QP:
> > Destroy the DV_QP specified by the QP-handle; unmap SQ/RQ user memory.
> >
> > - BNXT_RE_METHOD_DV_MODIFY_QP:
> > Modify QP attributes for the DV_QP specified by the QP-handle;
> > wrapper functions have been implemented to resolve dmac/smac using
> > rdma_resolve_ip().
> >
> > - BNXT_RE_METHOD_DV_QUERY_QP:
> > Return QP attributes for the DV_QP specified by the QP-handle.
>
> I think this is generally not how things should work..
>
> If you want to create enhanced QP/CQ objects then you should use the
> existing create QP/CQ ioctls and enhance them with a driver specific
> uverb spec, not special ioctls like this.
>
> Certainly new stuff should be broadly avoiding the 'struct
> bnxt_re_dv_create_qp_req' kind of design pattern in favour of the new
> ioctl system's finer grained specs.
>
We chose the new ioctl approach since it provides a simple direct path
from the bnxt_re library to the driver, without going through
ibv_cmd_*() and also since there's really no need to have the kernel
IB objects (ib_cq/ib_qp) for the DV use case. But we will discuss this
internally and consider redesigning this based on driver specific
udata, like you are suggesting.

> If you want to just create some kind of object by raw FW call then you
> need something like mlx5's devx to push raw FW calls (eg create QP)
> and wire them up securely.
>
Currently we are not looking to create objects by raw FW calls, but we
understand your point that a new ioctl could be used when there's a
requirement for raw FW interface.

> > Some applications might want to allocate memory for all resources of a
> > given type (CQ/QP) in one big chunk and then register that entire memory
> > once using DV_UMEM_REG. At the time of creating each individual
> > resource, the application would pass a specific offset/length in the
> > umem registered memory.
> >
> > - The DV_UMEM_REG handler (previous patch) only creates a dv_umem object
> > and saves user memory parameters, but doesn't really map/pin this
> > memory.
>
> That doesn't sound sensible, what is the point of that?
>
> The umem in mlx5 is all about pinning the memory and sending to the
> firwmare immediately. If you are not doing that, then you shouldn't
> havea "umem" type object at all.
>
> There is no reason, indeed it is a pretty bad idea, to have the kernel
> hold onto some userspace pointer and NOT pin it within the same system
> call.
>
This design was chosen since some of the applications written by our
customers are already using umem APIs for mlx devices and they wanted
to use similar code flow with our devices. Our adapter requires
pinning only when resources are created. So we choose deferred
pinning. We will address your concern by terminating the umem APIs in
the library and not going to the driver. Later, at the time of
resource creation (cq/qp) we will pass the address directly like you
suggested in your next comment below.

> > - The mapping would be done at the time of creating individual objects.
> > - This actual mapping of specific umem offsets is implemented by the
> > function bnxt_re_dv_umem_get(). This function validates the
> > umem-offset and size parameters passed during CQ/QP creation. If the
> > request is valid, it maps the specified offset/length within the umem
> > registered memory.
>
> We don't support "remote" pinning. This code is wrong:
>
> + dv_umem->addr = obj->addr + umem_offset;
This is not remote pinning. This is the local queue memory allocated
by the application.

> [..]
> + umem = ib_umem_get(&rdev->ibdev, (unsigned long)dv_umem->addr,
> + dv_umem->size, dv_umem->access);
>
> Since "addr" is only valid for pinning within the single system call that
> specifies it. You cannot pass it over to another, later, systemcall.
>
> I have no idea why you would want to do a design like this, if you
> want deferred pinning then pass the addr/etc directly later.
>
Responded above, in the previous comment on umem objects.
Thanks,
-Harsha
> Jason

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 5505 bytes --]