Re: ioccsz and iorcsz check failing

[Max Gurtovoy <mgurtovoy@nvidia.com>]

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.