NVME-CLI : Fabrics property-get and 64 bit property (CAP)

NVME-CLI : Fabrics property-get and 64 bit property (CAP)

[Eyal BenDavid]

Hi all,

We use show-regs in NVME-CLI for fabrics (FC) property-get (CAP, CC etc)

The controller capabilities property (CAP) is 64 bit.

The result field of struct nvme_passthru_cmd is of type __u32

Therefore the following code snippet from file nvme-ioctl.c,
function nvme_property() might lose information:

Code:
   /*   __le64 *value  */

    err = nvme_submit_admin_passthru(fd, &cmd);
    if (!err && fctype == nvme_fabrics_type_property_get)
        *value = cpu_to_le64(cmd.result);    // <<<<<<  HERE

When running show-regs command the high dword of CAP is always 0.

My questions:
how can I use nvme-cli to get the whole 8 byte range for CAP property
in property-get command.
Can I get the missing dword by an additional call to offset 4? (using
admin-passtru)
Is it legal for the controller to answer for this request?

Thanks
Eyal

NVME-CLI : Fabrics property-get and 64 bit property (CAP)

[Daniel Verkamp]

On 06/19/2018 09:23 AM, Eyal BenDavid wrote:
> Hi all,
> 
> We use show-regs in NVME-CLI for fabrics (FC) property-get (CAP, CC etc)
> 
> The controller capabilities property (CAP) is 64 bit.
> 
> The result field of struct nvme_passthru_cmd is of type __u32
> 
> Therefore the following code snippet from file nvme-ioctl.c,
> function nvme_property() might lose information:
> 
> Code:
>    /*   __le64 *value  */
> 
>     err = nvme_submit_admin_passthru(fd, &cmd);
>     if (!err && fctype == nvme_fabrics_type_property_get)
>         *value = cpu_to_le64(cmd.result);    // <<<<<<  HERE
> 
> When running show-regs command the high dword of CAP is always 0.
> 
> My questions:
> how can I use nvme-cli to get the whole 8 byte range for CAP property
> in property-get command.
> Can I get the missing dword by an additional call to offset 4? (using
> admin-passtru)
> Is it legal for the controller to answer for this request?

I don't know the answer to the nvme-cli question above, but it seems
like struct nvme_completion is insufficient to represent the Property
Get response, since it only has a 32-bit result field, as you mentioned.

For the question about retrieving CAP via 4-byte accesses, I don't think
this is allowed.  NVMe-oF 1.0 base spec section 3.5.1 (Property
Definitions) says: "The host shall access properties in their native
width with an offset that is at the beginning of the property."

Thanks,
-- Daniel

NVME-CLI : Fabrics property-get and 64 bit property (CAP)

[Eyal BenDavid]

On Tue, Jun 19, 2018@8:54 PM Daniel Verkamp <daniel.verkamp@intel.com> wrote:
>
> On 06/19/2018 09:23 AM, Eyal BenDavid wrote:
> > Hi all,
> >
> > We use show-regs in NVME-CLI for fabrics (FC) property-get (CAP, CC etc)
> >
> > The controller capabilities property (CAP) is 64 bit.
> >
> > The result field of struct nvme_passthru_cmd is of type __u32
> >
> > Therefore the following code snippet from file nvme-ioctl.c,
> > function nvme_property() might lose information:
> >
> > Code:
> >    /*   __le64 *value  */
> >
> >     err = nvme_submit_admin_passthru(fd, &cmd);
> >     if (!err && fctype == nvme_fabrics_type_property_get)
> >         *value = cpu_to_le64(cmd.result);    // <<<<<<  HERE
> >
> > When running show-regs command the high dword of CAP is always 0.
> >
> > My questions:
> > how can I use nvme-cli to get the whole 8 byte range for CAP property
> > in property-get command.
> > Can I get the missing dword by an additional call to offset 4? (using
> > admin-passtru)
> > Is it legal for the controller to answer for this request?
>
> I don't know the answer to the nvme-cli question above, but it seems
> like struct nvme_completion is insufficient to represent the Property
> Get response, since it only has a 32-bit result field, as you mentioned.
>
Thanks.
Are the result field in struct nvme_admin_passthru and struct
nvme_completion the same?
I mean does ioctl copy the result field from completion to
admin_passthru structure?

> For the question about retrieving CAP via 4-byte accesses, I don't think
> this is allowed.  NVMe-oF 1.0 base spec section 3.5.1 (Property
> Definitions) says: "The host shall access properties in their native
> width with an offset that is at the beginning of the property."
>
I missed it. Thanks.

> Thanks,
> -- Daniel

NVME-CLI : Fabrics property-get and 64 bit property (CAP)

[Keith Busch]

On Tue, Jun 19, 2018@10:02:17PM +0300, Eyal BenDavid wrote:
> Are the result field in struct nvme_admin_passthru and struct
> nvme_completion the same?
> I mean does ioctl copy the result field from completion to
> admin_passthru structure?

Yeah, those two are reporting the same thing. bytes 0-3 of the CQE.
The ioctl uapi was developed when the CQE bytes 4-7 were reserved in the
specification (they still are in the base spec), so those bytes were not
reported. If this is something we need to support, I think the only way
is to introduce a version-2 of the ioctl that copies the 8-byte status
for fabrics.

NVME-CLI : Fabrics property-get and 64 bit property (CAP)

[Eyal BenDavid]

On Tue, Jun 19, 2018 at 10:44 PM Keith Busch
<keith.busch@linux.intel.com> wrote:
>
> On Tue, Jun 19, 2018@10:02:17PM +0300, Eyal BenDavid wrote:
> > Are the result field in struct nvme_admin_passthru and struct
> > nvme_completion the same?
> > I mean does ioctl copy the result field from completion to
> > admin_passthru structure?
>
> Yeah, those two are reporting the same thing. bytes 0-3 of the CQE.
> The ioctl uapi was developed when the CQE bytes 4-7 were reserved in the
> specification (they still are in the base spec), so those bytes were not
> reported. If this is something we need to support, I think the only way
> is to introduce a version-2 of the ioctl that copies the 8-byte status
> for fabrics.

Does it mean that a user space program can't get the missing dword by
nvme-cli / ioctl ?
Is there another way to get the information from user space? (e.g.
sysfs / proc / dmesg etc)

Thank you,
Eyal

NVME-CLI : Fabrics property-get and 64 bit property (CAP)

[Keith Busch]

On Tue, Jun 19, 2018@11:00:00PM +0300, Eyal BenDavid wrote:
> Does it mean that a user space program can't get the missing dword by
> nvme-cli / ioctl ?

Right, no way to get this back to user space through the ioctl.

> Is there another way to get the information from user space? (e.g.
> sysfs / proc / dmesg etc)

Yeah, you should be able to see it if you've a recent enough kernel with
event tracers enabled. First enable the tracing:

  echo 1 > /sys/kernel/debug/tracing/events/nvme/enable

Then after running your nvme command, you should see the result with:

  cat /sys/kernel/debug/tracing/trace

NVME-CLI : Fabrics property-get and 64 bit property (CAP)

[Eyal BenDavid]

On Tue, Jun 19, 2018 at 11:22 PM Keith Busch
<keith.busch@linux.intel.com> wrote:

> Yeah, you should be able to see it if you've a recent enough kernel with
> event tracers enabled. First enable the tracing:
>
>   echo 1 > /sys/kernel/debug/tracing/events/nvme/enable
>
> Then after running your nvme command, you should see the result with:
>
>   cat /sys/kernel/debug/tracing/trace

Thanks :-)
Eyal

NVME-CLI : Fabrics property-get and 64 bit property (CAP)

[Eyal BenDavid]

> Yeah, you should be able to see it if you've a recent enough kernel with
> event tracers enabled. First enable the tracing:
>
>   echo 1 > /sys/kernel/debug/tracing/events/nvme/enable
>
> Then after running your nvme command, you should see the result with:
>
>   cat /sys/kernel/debug/tracing/trace

BTW I think there is a bug in function nvme_get_properties() (the
function called for show-regs in the fabric case)
In the case of 64 bit property, the offset loop variable is advanced
correctly only on error.
When the call to nvme_property is successful (and thus err == 0) the
offset variable is advanced by 4 bytes only.

I saw this when looking at the trace:
nvme-25616 [...] nvme_setup_admin_cmd:  cmdid=13, flags=0x0, meta=0x0,
cmd=(0x7f cdw10=00 00 00 00 **04** 00 ...)
show regs tried to get property for location 4.

Here is the code:

    for (off = NVME_REG_CAP; off <= NVME_REG_CMBSZ; off += 4) {
        switch (off) {
        case NVME_REG_CAP:
        case NVME_REG_ASQ:
        case NVME_REG_ACQ:
            is64bit = true;
            break;
        default:
            is64bit = false;
        }
        err = nvme_property(fd, nvme_fabrics_type_property_get,
                off, &value64, is64bit ? 1: 0);
        if (err) {
            if (is64bit)    // <<<<<<<<<<<<<<<<<< Only here
                off += 4;
            continue;
        }
        ret = 0;
        if (is64bit)
            *(uint64_t *)(*pbar + off) = le64_to_cpu(value64);
        else
            *(uint32_t *)(*pbar + off) = le32_to_cpu(value64);
    }

NVME-CLI : Fabrics property-get and 64 bit property (CAP)

[Eyal BenDavid]

Here is a small patch that fixes the offset.

Eyal

------

diff --git i/nvme-ioctl.c w/nvme-ioctl.c
index 63ff8fb..d52dd0a 100644
--- i/nvme-ioctl.c
+++ w/nvme-ioctl.c
@@ -572,8 +572,10 @@ int nvme_get_properties(int fd, void **pbar)
                        continue;
                }
                ret = 0;
-               if (is64bit)
+               if (is64bit) {
                        *(uint64_t *)(*pbar + off) = le64_to_cpu(value64);
+                       off += 4;
+               }
                else
                        *(uint32_t *)(*pbar + off) = le32_to_cpu(value64);
        }