Re: [PATCH for-next] RDMA/efa: Support CQ receive entries with source GID

[Gal Pressman <gal.pressman@linux.dev>]

On 14/08/2022 18:04, Margolin, Michael wrote:
> On 8/14/2022 2:38 PM, Gal Pressman wrote:
>> On 09/08/2022 18:16, Michael Margolin wrote:
>>> Add a parameter for create CQ admin command to set source address on
>>> receive completion descriptors. Report capability for this feature
>>> through query device verb.
>>>
>>> Reviewed-by: Firas Jahjah <firasj@amazon.com>
>>> Reviewed-by: Yossi Leybovich <sleybo@amazon.com>
>>> Signed-off-by: Daniel Kranzdorf <dkkranzd@amazon.com>
>>> Signed-off-by: Michael Margolin <mrgolin@amazon.com>
>>> ---
>>>  drivers/infiniband/hw/efa/efa_admin_cmds_defs.h | 6 +++++-
>>>  drivers/infiniband/hw/efa/efa_com_cmd.c         | 5 ++++-
>>>  drivers/infiniband/hw/efa/efa_com_cmd.h         | 1 +
>>>  drivers/infiniband/hw/efa/efa_verbs.c           | 4 +++-
>>>  include/uapi/rdma/efa-abi.h                     | 4 +++-
>>>  5 files changed, 16 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/infiniband/hw/efa/efa_admin_cmds_defs.h b/drivers/infiniband/hw/efa/efa_admin_cmds_defs.h
>>> index 0b0b93b529f3..d4b9226088bd 100644
>>> --- a/drivers/infiniband/hw/efa/efa_admin_cmds_defs.h
>>> +++ b/drivers/infiniband/hw/efa/efa_admin_cmds_defs.h
>>> @@ -444,7 +444,10 @@ struct efa_admin_create_cq_cmd {
>>>       /*
>>>        * 4:0 : cq_entry_size_words - size of CQ entry in
>>>        *    32-bit words, valid values: 4, 8.
>>> -      * 7:5 : reserved7 - MBZ
>>> +      * 5 : set_src_addr - If set, source address will be
>>> +      *    filled on RX completions from unknown senders.
>>> +      *    Requires 8 words CQ entry size.
>>> +      * 7:6 : reserved7 - MBZ
>>>        */
>>>       u8 cq_caps_2;
>>>
>>> @@ -980,6 +983,7 @@ struct efa_admin_host_info {
>>>  #define EFA_ADMIN_CREATE_CQ_CMD_INTERRUPT_MODE_ENABLED_MASK BIT(5)
>>>  #define EFA_ADMIN_CREATE_CQ_CMD_VIRT_MASK                   BIT(6)
>>>  #define EFA_ADMIN_CREATE_CQ_CMD_CQ_ENTRY_SIZE_WORDS_MASK    GENMASK(4, 0)
>>> +#define EFA_ADMIN_CREATE_CQ_CMD_SET_SRC_ADDR_MASK           BIT(5)
>>>
>>>  /* create_cq_resp */
>>>  #define EFA_ADMIN_CREATE_CQ_RESP_DB_VALID_MASK              BIT(0)
>>> diff --git a/drivers/infiniband/hw/efa/efa_com_cmd.c b/drivers/infiniband/hw/efa/efa_com_cmd.c
>>> index fb405da4e1db..8f8885e002ba 100644
>>> --- a/drivers/infiniband/hw/efa/efa_com_cmd.c
>>> +++ b/drivers/infiniband/hw/efa/efa_com_cmd.c
>>> @@ -168,7 +168,10 @@ int efa_com_create_cq(struct efa_com_dev *edev,
>>>                       EFA_ADMIN_CREATE_CQ_CMD_INTERRUPT_MODE_ENABLED, 1);
>>>               create_cmd.eqn = params->eqn;
>>>       }
>>> -
>>> +     if (params->set_src_addr) {
>>> +             EFA_SET(&create_cmd.cq_caps_2,
>>> +                     EFA_ADMIN_CREATE_CQ_CMD_SET_SRC_ADDR, 1);
>>> +     }
>> Don't you need to validate the CQE size requested by the user somewhere?
>> I assume you must use 32 bytes completions for this.
> This is a good point. Requested CQE size is validated against command
> feature bits in the device FW. In a case of user requesting for a wrong
> or unsupported configuration efa_com_cmd_exec() will return an
> appropriate error code. This is to avoid driver dependency on CQE
> structure or size.
>
>
> Michael
>

The driver usually terminates bad inputs before they get to the device
to prevent unnecessary noise.
As of today, command failures are unexpected so we have a ibdev_err to
indicate that something went very wrong, I don't think you want to allow
a non-root user to flood your device (and dmesg) with errors.