Re: ioccsz and iorcsz check failing

[Max Gurtovoy <mgurtovoy@nvidia.com>]

On 20/12/2023 1:55, Max Gurtovoy wrote:
> 
> 
> On 19/12/2023 21:59, Sagi Grimberg wrote:
>>
>>
>> On 12/19/23 19:24, Max Gurtovoy wrote:
>>>
>>>
>>> On 18/12/2023 18:04, Sagi Grimberg wrote:
>>>>
>>>>>> Is anything else in the identify wrong, or is it just these
>>>>>> fabrics fields?
>>>>>
>>>>> So what I am seeing on the wire are a few fabrics commands (connect,
>>>>> get
>>>>> property) followed by the nvme id ctrl command (opcode 0x6).
>>>>>
>>>>>    nvmet: nvmet_req_init:962
>>>>>    nvmet: nvmet_parse_admin_cmd:1011
>>>>>    nvmet: nvmet_parse_discovery_cmd:359 opcode 6
>>>>>
>>>>> This calls then nvmet_execute_disc_identify, so adding
>>>>>
>>>>> --- a/drivers/nvme/target/discovery.c
>>>>> +++ b/drivers/nvme/target/discovery.c
>>>>> @@ -249,6 +249,7 @@ static void nvmet_execute_disc_identify(struct
>>>>> nvmet_req *req)
>>>>>    {
>>>>>           struct nvmet_ctrl *ctrl = req->sq->ctrl;
>>>>>           struct nvme_id_ctrl *id;
>>>>> +       u32 cmd_capsule_size;
>>>>>           u16 status = 0;
>>>>>
>>>>>           if (!nvmet_check_transfer_len(req, NVME_IDENTIFY_DATA_SIZE))
>>>>> @@ -289,6 +290,17 @@ static void nvmet_execute_disc_identify(struct
>>>>> nvmet_req *req)
>>>>>                   id->sgls |= cpu_to_le32(1 << 2);
>>>>>           if (req->port->inline_data_size)
>>>>>                   id->sgls |= cpu_to_le32(1 << 20);
>>>>> +       /*
>>>>> +        * Max command capsule size is sqe + in-capsule data size.
>>>>> +        * Disable in-capsule data for Metadata capable controllers.
>>>>> +        */
>>>>> +       cmd_capsule_size = sizeof(struct nvme_command);
>>>>> +       if (!ctrl->pi_support)
>>>>> +               cmd_capsule_size += req->port->inline_data_size;
>>>>> +       id->ioccsz = cpu_to_le32(cmd_capsule_size / 16);
>>>>
>>>> Yes, this is the culprit. Nice that it exposed a bug.
>>>>
>>>> There is no in-capsule data for discovery controllers afaict.
>>>
>>> Also, the discovery controllers can't support PI AFAIK.
>>
>> Indeed, we should simply set it to sizeof(struct nvme_command) / 16.
>>
>> It is true that for tcp transport we should add additional 8192 which
>> is mandatory for nvme-tcp. But that can be done in an incremental patch.
> 
> I'm not aware of additional 8k of mandatory size for nvme-tcp discovery 
> controllers.
> what is this needed for ?
> 
> I was re-thinking about this issue again and I think that the original 
> patch that caused the problem that Daniel found is wrong from spec 
> perspective and also breaks compatibility with linux target for no 
> reason. And maybe with other targets as well.
> 
> A discovery controller according to the spec does not implement IO 
> queues or expose namespaces. So why do we even check for IOCCSZ and 
> IORCSZ for these controllers ?
> It is also mentioned explicitly that these 2 fields are reserved for 
> discovery controllers.
> 
> SPEC:
> "
> I/O Queue Command Capsule Supported Size (IOCCSZ): This field defines 
> the maximum I/O command capsule size in 16 byte units. The minimum value 
> that shall be indicated is 4 corresponding to 64 bytes.
> 
> I/O Queue Response Capsule Supported Size (IORCSZ): This field defines 
> the maximum I/O response capsule size in 16 byte units. The minimum 
> value that shall be indicated is 1 corresponding to 16 bytes.
> "
> 
> I suggest the following untested code to fix *only* the host driver side:
> 
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index ee2e4c49892d..89e01e6b5fe4 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -3068,14 +3068,14 @@ static int nvme_check_ctrl_fabric_info(struct 
> nvme_ctrl *ctrl, struct nvme_id_ct
>                  return -EINVAL;
>          }
> 
> -       if (ctrl->ioccsz < 4) {
> +       if (nvme_discovery_ctrl(ctrl) && ctrl->ioccsz < 4) {
>                  dev_err(ctrl->device,
>                          "I/O queue command capsule supported size %d < 
> 4\n",
>                          ctrl->ioccsz);
>                  return -EINVAL;
>          }
> 
> -       if (ctrl->iorcsz < 1) {
> +       if (nvme_discovery_ctrl(ctrl) && ctrl->iorcsz < 1) {
>                  dev_err(ctrl->device,
>                          "I/O queue response capsule supported size %d < 
> 1\n",
>                          ctrl->iorcsz);
> 
> 
> Daniel,
> can you please test the above and if it works I'll send a fix with your 
> Tested-By signature. We must have it merged to next 6.7 rc.
> 
> -Max.
> 

Daniel,
I just saw that Caleb also noticed that it shouldn't be relevant for 
discovery controllers. The mail got lost in the mailbox.

Anyway, I believe the initiator/host code should be updated for 6.7 and 
must ignore these fields as the spec explicitly said it is reserved.
For the target side, I prefer to keep it 0 for reserved fields since 
this is the convention in all the specifications that I'm aware of.

Sagi/Christoph,
WDYT ?

I have a small bug for above code so please use:

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index ee2e4c49892d..b218ac88fcf8 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -3068,14 +3068,14 @@ static int nvme_check_ctrl_fabric_info(struct 
nvme_ctrl *ctrl, struct nvme_id_ct
                 return -EINVAL;
         }

-       if (ctrl->ioccsz < 4) {
+       if (!nvme_discovery_ctrl(ctrl) && ctrl->ioccsz < 4) {
                 dev_err(ctrl->device,
                         "I/O queue command capsule supported size %d < 
4\n",
                         ctrl->ioccsz);
                 return -EINVAL;
         }

-       if (ctrl->iorcsz < 1) {
+       if (!nvme_discovery_ctrl(ctrl) && ctrl->iorcsz < 1) {
                 dev_err(ctrl->device,
                         "I/O queue response capsule supported size %d < 
1\n",
                         ctrl->iorcsz);