Re: [PATCH for-next v2] RDMA/efa: Add EFA query MR support

[Leon Romanovsky <leon@kernel.org>]

On Mon, Dec 11, 2023 at 07:46:40PM +0200, Margolin, Michael wrote:
> 
> On 12/11/2023 1:26 PM, Leon Romanovsky wrote:
> > CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.
> >
> >
> >
> > On Mon, Dec 11, 2023 at 12:35:34PM +0200, Margolin, Michael wrote:
> >> On 12/11/2023 10:10 AM, Leon Romanovsky wrote:
> >>> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.
> >>>
> >>>
> >>>
> >>> On Thu, Dec 07, 2023 at 02:27:48PM +0000, Michael Margolin wrote:
> >>>> Add EFA driver uapi definitions and register a new query MR method that
> >>>> currently returns the physical PCI buses' IDs the device is using to
> >>>> reach the MR. Update admin definitions and efa-abi accordingly.
> >>>>
> >>>> Reviewed-by: Anas Mousa <anasmous@amazon.com>
> >>>> Reviewed-by: Firas Jahjah <firasj@amazon.com>
> >>>> Signed-off-by: Michael Margolin <mrgolin@amazon.com>
> >>>> ---
> >>>>  drivers/infiniband/hw/efa/efa.h               |  5 +-
> >>>>  .../infiniband/hw/efa/efa_admin_cmds_defs.h   | 31 ++++++++
> >>>>  drivers/infiniband/hw/efa/efa_com_cmd.c       |  6 ++
> >>>>  drivers/infiniband/hw/efa/efa_com_cmd.h       |  4 +
> >>>>  drivers/infiniband/hw/efa/efa_main.c          |  5 ++
> >>>>  drivers/infiniband/hw/efa/efa_verbs.c         | 77 +++++++++++++++++++
> >>>>  include/uapi/rdma/efa-abi.h                   | 19 +++++
> >>>>  7 files changed, 146 insertions(+), 1 deletion(-)
> >>> <...>
> >>>
> >>>> +     /*
> >>>> +      * Mask indicating which fields have valid values
> >>>> +      * 0 : recv_pci_bus_id
> >>>> +      * 1 : rdma_read_pci_bus_id
> >>>> +      * 2 : rdma_recv_pci_bus_id
> >>>> +      */
> >>>> +     u8 validity;
> >>> <...>
> >>>
> >>>>  #define EFA_GID_SIZE 16
> >>>> +#define EFA_INVALID_PCI_BUS_ID 0xffff
> >>> Is 0xffff value guaranteed by PCI subsystem to be invalid? Why don't you
> >>> provide "validity" field to userspace instead?
> >> The 0xffff value in only used internally in the driver to indicate an
> >> invalid id and isn't exposed to userspace. For userspace there is a
> >> validity field as you suggested:
> >>
> >> +       return uverbs_copy_to(attrs,
> >> EFA_IB_ATTR_QUERY_MR_RESP_PCI_BUS_ID_VALIDITY,
> >> +                             &pci_bus_id_validity,
> >> sizeof(pci_bus_id_validity));
> > So please rely on your EFA_GET(&cmd_completion.validity, EFA_ADMIN_XXX_PCI_BUS_ID)
> > checks when you fill pci_bus_id_validity and not on 0xffff value which can be
> > valid from PCI perspective.
> >
> > Thanks
> 
> 0xffff can't practically be a valid PCI id in this context, 

As long as PCI subsystem doesn't declare 0xffff as invalid, we can't
assume it too.

Thanks