[PATCH v1 0/1] Allow any NSID value for NVMe admin commands

[PATCH v1 0/1] Allow any NSID value for NVMe admin commands

[Wu, Hao A]

Repost the mail to the list.

Best Regards,
Hao Wu


-----Original Message-----
From: Tyler Erickson [mailto:tyler.j.erickson@seagate.com] 
Sent: Tuesday, September 03, 2019 9:55 PM
To: edk2-devel@lists.01.org
Cc: Wu, Hao A; Ni, Ray
Subject: [PATCH v1 0/1] Allow any NSID value for NVMe admin commands

This patch is to allow issuing admin commands to NVMe devices with any value for the NSID.
Use cases for this are for DST commands, format commands, identify commands, and get log page commands.
Changing the NSID in these admin commands changes behavior on what data is returned (controller wide versus individual namespace for logs),
or changes what is formatted (a particular namespace versus all namespaces on the controller), or changes what DST
is testing (controller only, single namespace, or controller and all namespaces).

Changes can be found here: https://github.com/vonericsen/edk2/tree/nvm_passthru_admin_any_NSID

Cc: Hao A Wu <hao.a.wu@intel.com>
Cc: Ray Ni <ray.ni@intel.com>

Tyler Erickson (1):
  MdeModulePkg/NvmExpressDxe: Allow other NSIDs for Admin commands

 MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

-- 
2.17.1

[PATCH v1 1/1] MdeModulePkg/NvmExpressDxe: Allow other NSIDs for Admin commands

[Wu, Hao A]

Repost the mail to the list.

Best Regards,
Hao Wu

-----Original Message-----
From: Tyler Erickson [mailto:tyler.j.erickson@seagate.com] 
Sent: Tuesday, September 03, 2019 9:55 PM
To: edk2-devel@lists.01.org
Cc: Wu, Hao A; Ni, Ray
Subject: [PATCH v1 1/1] MdeModulePkg/NvmExpressDxe: Allow other NSIDs for Admin commands

Cc: Hao A Wu <hao.a.wu@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Signed-off-by: Tyler Erickson <tyler.j.erickson@seagate.com>
---
 MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c b/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c
index 8e721379466a..78a3c663ded4 100644
--- a/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c
+++ b/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c
@@ -561,7 +561,7 @@ NvmExpressPassThru (
   Sq  = Private->SqBuffer[QueueId] + Private->SqTdbl[QueueId].Sqt;
   Cq  = Private->CqBuffer[QueueId] + Private->CqHdbl[QueueId].Cqh;
 
-  if (Packet->NvmeCmd->Nsid != NamespaceId) {
+  if (Packet->QueueType != NVME_ADMIN_QUEUE && Packet->NvmeCmd->Nsid != NamespaceId) {
     return EFI_INVALID_PARAMETER;
   }
 
-- 
2.17.1

Re: [PATCH v1 1/1] MdeModulePkg/NvmExpressDxe: Allow other NSIDs for Admin commands

[Wu, Hao A]

> -----Original Message-----
> From: Wu, Hao A
> Sent: Wednesday, September 04, 2019 11:39 AM
> To: devel@edk2.groups.io; Tyler Erickson
> Cc: Wu, Hao A; Ni, Ray
> Subject: [PATCH v1 1/1] MdeModulePkg/NvmExpressDxe: Allow other NSIDs
> for Admin commands
> 
> Repost the mail to the list.
> 
> Best Regards,
> Hao Wu
> 
> -----Original Message-----
> From: Tyler Erickson [mailto:tyler.j.erickson@seagate.com]
> Sent: Tuesday, September 03, 2019 9:55 PM
> To: edk2-devel@lists.01.org
> Cc: Wu, Hao A; Ni, Ray
> Subject: [PATCH v1 1/1] MdeModulePkg/NvmExpressDxe: Allow other NSIDs
> for Admin commands
> 
> Cc: Hao A Wu <hao.a.wu@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Signed-off-by: Tyler Erickson <tyler.j.erickson@seagate.com>
> ---
>  MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c
> b/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c
> index 8e721379466a..78a3c663ded4 100644
> --- a/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c
> +++ b/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c
> @@ -561,7 +561,7 @@ NvmExpressPassThru (
>    Sq  = Private->SqBuffer[QueueId] + Private->SqTdbl[QueueId].Sqt;
>    Cq  = Private->CqBuffer[QueueId] + Private->CqHdbl[QueueId].Cqh;
> 
> -  if (Packet->NvmeCmd->Nsid != NamespaceId) {
> +  if (Packet->QueueType != NVME_ADMIN_QUEUE && Packet->NvmeCmd-
> >Nsid != NamespaceId) {


Hello,

Per my understanding to the codes, I think the:

* Input parameter 'NamespaceId' for the PassThru() service
* The 'Nsid' field of the EFI_NVM_EXPRESS_COMMAND

are of the same meaning.

Do you think setting these 2 values identical when calling the PassThru()
service can resolve the issue you met?

Best Regards,
Hao Wu


>      return EFI_INVALID_PARAMETER;
>    }
> 
> --
> 2.17.1

Re: [PATCH v1 1/1] MdeModulePkg/NvmExpressDxe: Allow other NSIDs for Admin commands

[tyler.j.erickson]

[-- Attachment #1: Type: text/plain, Size: 3334 bytes --]

Hi Hao,

I tried making both the NamespaceID and NSID values the same when calling
the passthru function for these admin commands and it didn't work. I think
that is due to another place in the passthru code filtering the NamespaceId
input values on line 517-520.
The NamespaceId parameter is being checked to make sure it isn't greater
than the number of supported namespaces by the controller and it makes sure
it isn't set to (UINT32)-1 (All F's).
I think this is correct since the NamespaceId input is what is discovered
when calling the EFI_NVM_EXPRESS_PASS_THRU_GET_NEXT_NAMESPACE function.
This is what I used to get the available namespaces available in the system.

In my case I'm sending some commands in my application with the NSID set to
UINT32_MAX in order to request controller wide SMART/Health log data among
other things. There are some commands where setting the NSID to another
value may also be useful. For example, DST can use the NSID in the command
to change between testing only the controller (0), testing all namespaces
(UINT32_MAX), and a specific namespace.

The modification I made was done where the passthru command's NSID was
actually being checked, which seemed like the best place to add this
exception for admin commands.

-Tyler


On Tue, Sep 3, 2019 at 9:39 PM Wu, Hao A <hao.a.wu@intel.com> wrote:

> > -----Original Message-----
> > From: Wu, Hao A
> > Sent: Wednesday, September 04, 2019 11:39 AM
> > To: devel@edk2.groups.io; Tyler Erickson
> > Cc: Wu, Hao A; Ni, Ray
> > Subject: [PATCH v1 1/1] MdeModulePkg/NvmExpressDxe: Allow other NSIDs
> > for Admin commands
> >
> > Repost the mail to the list.
> >
> > Best Regards,
> > Hao Wu
> >
> > -----Original Message-----
> > From: Tyler Erickson [mailto:tyler.j.erickson@seagate.com]
> > Sent: Tuesday, September 03, 2019 9:55 PM
> > To: edk2-devel@lists.01.org
> > Cc: Wu, Hao A; Ni, Ray
> > Subject: [PATCH v1 1/1] MdeModulePkg/NvmExpressDxe: Allow other NSIDs
> > for Admin commands
> >
> > Cc: Hao A Wu <hao.a.wu@intel.com>
> > Cc: Ray Ni <ray.ni@intel.com>
> > Signed-off-by: Tyler Erickson <tyler.j.erickson@seagate.com>
> > ---
> >  MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c
> > b/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c
> > index 8e721379466a..78a3c663ded4 100644
> > --- a/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c
> > +++ b/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c
> > @@ -561,7 +561,7 @@ NvmExpressPassThru (
> >    Sq  = Private->SqBuffer[QueueId] + Private->SqTdbl[QueueId].Sqt;
> >    Cq  = Private->CqBuffer[QueueId] + Private->CqHdbl[QueueId].Cqh;
> >
> > -  if (Packet->NvmeCmd->Nsid != NamespaceId) {
> > +  if (Packet->QueueType != NVME_ADMIN_QUEUE && Packet->NvmeCmd-
> > >Nsid != NamespaceId) {
>
>
> Hello,
>
> Per my understanding to the codes, I think the:
>
> * Input parameter 'NamespaceId' for the PassThru() service
> * The 'Nsid' field of the EFI_NVM_EXPRESS_COMMAND
>
> are of the same meaning.
>
> Do you think setting these 2 values identical when calling the PassThru()
> service can resolve the issue you met?
>
> Best Regards,
> Hao Wu
>
>
> >      return EFI_INVALID_PARAMETER;
> >    }
> >
> > --
> > 2.17.1
>
>

[-- Attachment #2: Type: text/html, Size: 4704 bytes --]

Re: [PATCH v1 1/1] MdeModulePkg/NvmExpressDxe: Allow other NSIDs for Admin commands

[Wu, Hao A]

[-- Attachment #1: Type: text/plain, Size: 4792 bytes --]

Hello Tyler,

Some inline comments below.

Best Regards,
Hao Wu

From: Tyler J Erickson [mailto:tyler.j.erickson@seagate.com]
Sent: Wednesday, September 04, 2019 11:07 PM
To: Wu, Hao A
Cc: devel@edk2.groups.io; Ni, Ray
Subject: Re: [PATCH v1 1/1] MdeModulePkg/NvmExpressDxe: Allow other NSIDs for Admin commands

Hi Hao,

I tried making both the NamespaceID and NSID values the same when calling the passthru function for these admin commands and it didn't work. I think that is due to another place in the passthru code filtering the NamespaceId input values on line 517-520.
The NamespaceId parameter is being checked to make sure it isn't greater than the number of supported namespaces by the controller and it makes sure it isn't set to (UINT32)-1 (All F's).


[Hao]: I think the check on line 517-520 blocks ‘NamespaceId’ with value greater than the number of namespace. But it allows the value to be MAX_UINT32, which means the command takes effect on all namespaces.


I think this is correct since the NamespaceId input is what is discovered when calling the EFI_NVM_EXPRESS_PASS_THRU_GET_NEXT_NAMESPACE function. This is what I used to get the available namespaces available in the system.

In my case I'm sending some commands in my application with the NSID set to UINT32_MAX in order to request controller wide SMART/Health log data among other things. There are some commands where setting the NSID to another value may also be useful. For example, DST can use the NSID in the command to change between testing only the controller (0), testing all namespaces (UINT32_MAX), and a specific namespace.


[Hao]: Yes. Just as you said, commands like DST or Get Log Page will have different target when different Namespace values are provided.
I think for your case, you can:

1.       Set both the 'NamespaceId' parameter and 'Nsid' field to 0 if the target is the controller;

2.       Set both of them MAX_UINT32 if the target is all the namespaces;

3.       Set both of them to a corresponding ID if the target is a specific namespace.

As you can see from the current implementation of NvmExpressPassThru() function, the 'NamespaceId' parameter does not affect the construct of the submission queue item.
The 'NamespaceId' parameter only serves as a valid check for the 'Nsid' field in the EFI_NVM_EXPRESS_COMMAND structure.


The modification I made was done where the passthru command's NSID was actually being checked, which seemed like the best place to add this exception for admin commands.

-Tyler


On Tue, Sep 3, 2019 at 9:39 PM Wu, Hao A <hao.a.wu@intel.com<mailto:hao.a.wu@intel.com>> wrote:
> -----Original Message-----
> From: Wu, Hao A
> Sent: Wednesday, September 04, 2019 11:39 AM
> To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; Tyler Erickson
> Cc: Wu, Hao A; Ni, Ray
> Subject: [PATCH v1 1/1] MdeModulePkg/NvmExpressDxe: Allow other NSIDs
> for Admin commands
>
> Repost the mail to the list.
>
> Best Regards,
> Hao Wu
>
> -----Original Message-----
> From: Tyler Erickson [mailto:tyler.j.erickson@seagate.com<mailto:tyler.j.erickson@seagate.com>]
> Sent: Tuesday, September 03, 2019 9:55 PM
> To: edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
> Cc: Wu, Hao A; Ni, Ray
> Subject: [PATCH v1 1/1] MdeModulePkg/NvmExpressDxe: Allow other NSIDs
> for Admin commands
>
> Cc: Hao A Wu <hao.a.wu@intel.com<mailto:hao.a.wu@intel.com>>
> Cc: Ray Ni <ray.ni@intel.com<mailto:ray.ni@intel.com>>
> Signed-off-by: Tyler Erickson <tyler.j.erickson@seagate.com<mailto:tyler.j.erickson@seagate.com>>
> ---
>  MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c
> b/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c
> index 8e721379466a..78a3c663ded4 100644
> --- a/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c
> +++ b/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c
> @@ -561,7 +561,7 @@ NvmExpressPassThru (
>    Sq  = Private->SqBuffer[QueueId] + Private->SqTdbl[QueueId].Sqt;
>    Cq  = Private->CqBuffer[QueueId] + Private->CqHdbl[QueueId].Cqh;
>
> -  if (Packet->NvmeCmd->Nsid != NamespaceId) {
> +  if (Packet->QueueType != NVME_ADMIN_QUEUE && Packet->NvmeCmd-
> >Nsid != NamespaceId) {


Hello,

Per my understanding to the codes, I think the:

* Input parameter 'NamespaceId' for the PassThru() service
* The 'Nsid' field of the EFI_NVM_EXPRESS_COMMAND

are of the same meaning.

Do you think setting these 2 values identical when calling the PassThru()
service can resolve the issue you met?

Best Regards,
Hao Wu


>      return EFI_INVALID_PARAMETER;
>    }
>
> --
> 2.17.1

[-- Attachment #2: Type: text/html, Size: 36914 bytes --]

Re: [edk2-devel] [PATCH v1 1/1] MdeModulePkg/NvmExpressDxe: Allow other NSIDs for Admin commands

[Tyler J Erickson]

[-- Attachment #1: Type: text/plain, Size: 5576 bytes --]

Hi Hao,

Sorry for the late reply.
I tested and can confirm that if the NSID of the command and the provided
NamespaceID as an input match, the commands do go through on the EDK2
driver. This is not the case with a driver built into a motherboard that I
have here, as it only accepts values for the current NSID and nothing else.
This is ok since I can load the EDK2 driver in place instead.

Since this is unclear in the documentation for the passthru function, can
we make a documentation change instead?

Thanks,
-Tyler


On Wed, Sep 4, 2019 at 8:07 PM Wu, Hao A <hao.a.wu@intel.com> wrote:

> Hello Tyler,
>
>
>
> Some inline comments below.
>
>
>
> Best Regards,
>
> Hao Wu
>
>
>
> *From:* Tyler J Erickson [mailto:tyler.j.erickson@seagate.com]
> *Sent:* Wednesday, September 04, 2019 11:07 PM
> *To:* Wu, Hao A
> *Cc:* devel@edk2.groups.io; Ni, Ray
> *Subject:* Re: [PATCH v1 1/1] MdeModulePkg/NvmExpressDxe: Allow other
> NSIDs for Admin commands
>
>
>
> Hi Hao,
>
>
>
> I tried making both the NamespaceID and NSID values the same when calling
> the passthru function for these admin commands and it didn't work. I
> think that is due to another place in the passthru code filtering the
> NamespaceId input values on line 517-520.
>
> The NamespaceId parameter is being checked to make sure it isn't greater
> than the number of supported namespaces by the controller and it makes sure
> it isn't set to (UINT32)-1 (All F's).
>
>
>
>
>
> [Hao]: I think the check on line 517-520 blocks ‘NamespaceId’ with value
> greater than the number of namespace. But it allows the value to be
> MAX_UINT32, which means the command takes effect on all namespaces.
>
>
>
>
>
> I think this is correct since the NamespaceId input is what is discovered
> when calling the EFI_NVM_EXPRESS_PASS_THRU_GET_NEXT_NAMESPACE function.
> This is what I used to get the available namespaces available in the system.
>
>
>
> In my case I'm sending some commands in my application with the NSID set
> to UINT32_MAX in order to request controller wide SMART/Health log data
> among other things. There are some commands where setting the NSID to
> another value may also be useful. For example, DST can use the NSID in the
> command to change between testing only the controller (0), testing all
> namespaces (UINT32_MAX), and a specific namespace.
>
>
>
>
>
> [Hao]: Yes. Just as you said, commands like DST or Get Log Page will have
> different target when different Namespace values are provided.
>
> I think for your case, you can:
>
> 1.       Set both the 'NamespaceId' parameter and 'Nsid' field to 0 if
> the target is the controller;
>
> 2.       Set both of them MAX_UINT32 if the target is all the namespaces;
>
> 3.       Set both of them to a corresponding ID if the target is a
> specific namespace.
>
>
>
> As you can see from the current implementation of NvmExpressPassThru()
> function, the 'NamespaceId' parameter does not affect the construct of
> the submission queue item.
>
> The 'NamespaceId' parameter only serves as a valid check for the 'Nsid'
> field in the EFI_NVM_EXPRESS_COMMAND structure.
>
>
>
>
>
> The modification I made was done where the passthru command's NSID was
> actually being checked, which seemed like the best place to add this
> exception for admin commands.
>
>
> -Tyler
>
>
>
>
>
> On Tue, Sep 3, 2019 at 9:39 PM Wu, Hao A <hao.a.wu@intel.com> wrote:
>
> > -----Original Message-----
> > From: Wu, Hao A
> > Sent: Wednesday, September 04, 2019 11:39 AM
> > To: devel@edk2.groups.io; Tyler Erickson
> > Cc: Wu, Hao A; Ni, Ray
> > Subject: [PATCH v1 1/1] MdeModulePkg/NvmExpressDxe: Allow other NSIDs
> > for Admin commands
> >
> > Repost the mail to the list.
> >
> > Best Regards,
> > Hao Wu
> >
> > -----Original Message-----
> > From: Tyler Erickson [mailto:tyler.j.erickson@seagate.com]
> > Sent: Tuesday, September 03, 2019 9:55 PM
> > To: edk2-devel@lists.01.org
> > Cc: Wu, Hao A; Ni, Ray
> > Subject: [PATCH v1 1/1] MdeModulePkg/NvmExpressDxe: Allow other NSIDs
> > for Admin commands
> >
> > Cc: Hao A Wu <hao.a.wu@intel.com>
> > Cc: Ray Ni <ray.ni@intel.com>
> > Signed-off-by: Tyler Erickson <tyler.j.erickson@seagate.com>
> > ---
> >  MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c
> > b/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c
> > index 8e721379466a..78a3c663ded4 100644
> > --- a/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c
> > +++ b/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c
> > @@ -561,7 +561,7 @@ NvmExpressPassThru (
> >    Sq  = Private->SqBuffer[QueueId] + Private->SqTdbl[QueueId].Sqt;
> >    Cq  = Private->CqBuffer[QueueId] + Private->CqHdbl[QueueId].Cqh;
> >
> > -  if (Packet->NvmeCmd->Nsid != NamespaceId) {
> > +  if (Packet->QueueType != NVME_ADMIN_QUEUE && Packet->NvmeCmd-
> > >Nsid != NamespaceId) {
>
>
> Hello,
>
> Per my understanding to the codes, I think the:
>
> * Input parameter 'NamespaceId' for the PassThru() service
> * The 'Nsid' field of the EFI_NVM_EXPRESS_COMMAND
>
> are of the same meaning.
>
> Do you think setting these 2 values identical when calling the PassThru()
> service can resolve the issue you met?
>
> Best Regards,
> Hao Wu
>
>
> >      return EFI_INVALID_PARAMETER;
> >    }
> >
> > --
> > 2.17.1
>
> 
>
>

[-- Attachment #2: Type: text/html, Size: 15145 bytes --]

Re: [edk2-devel] [PATCH v1 1/1] MdeModulePkg/NvmExpressDxe: Allow other NSIDs for Admin commands

[Wu, Hao A]

[-- Attachment #1: Type: text/plain, Size: 6049 bytes --]

Hello Tyler,

I agree that the UEFI spec can be refined to explicitly mention the below 2 fields:

* Input parameter 'NamespaceId' for the PassThru() service
* The 'Nsid' field of the EFI_NVM_EXPRESS_COMMAND

should match for the ‘PassThru’ service.

Best Regards,
Hao Wu

From: Tyler J Erickson [mailto:tyler.j.erickson@seagate.com]
Sent: Wednesday, September 18, 2019 12:50 AM
To: Wu, Hao A
Cc: devel@edk2.groups.io; Ni, Ray
Subject: Re: [edk2-devel] [PATCH v1 1/1] MdeModulePkg/NvmExpressDxe: Allow other NSIDs for Admin commands

Hi Hao,

Sorry for the late reply.
I tested and can confirm that if the NSID of the command and the provided NamespaceID as an input match, the commands do go through on the EDK2 driver. This is not the case with a driver built into a motherboard that I have here, as it only accepts values for the current NSID and nothing else. This is ok since I can load the EDK2 driver in place instead.

Since this is unclear in the documentation for the passthru function, can we make a documentation change instead?

Thanks,
-Tyler


On Wed, Sep 4, 2019 at 8:07 PM Wu, Hao A <hao.a.wu@intel.com<mailto:hao.a.wu@intel.com>> wrote:
Hello Tyler,

Some inline comments below.

Best Regards,
Hao Wu

From: Tyler J Erickson [mailto:tyler.j.erickson@seagate.com<mailto:tyler.j.erickson@seagate.com>]
Sent: Wednesday, September 04, 2019 11:07 PM
To: Wu, Hao A
Cc: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; Ni, Ray
Subject: Re: [PATCH v1 1/1] MdeModulePkg/NvmExpressDxe: Allow other NSIDs for Admin commands

Hi Hao,

I tried making both the NamespaceID and NSID values the same when calling the passthru function for these admin commands and it didn't work. I think that is due to another place in the passthru code filtering the NamespaceId input values on line 517-520.
The NamespaceId parameter is being checked to make sure it isn't greater than the number of supported namespaces by the controller and it makes sure it isn't set to (UINT32)-1 (All F's).


[Hao]: I think the check on line 517-520 blocks ‘NamespaceId’ with value greater than the number of namespace. But it allows the value to be MAX_UINT32, which means the command takes effect on all namespaces.


I think this is correct since the NamespaceId input is what is discovered when calling the EFI_NVM_EXPRESS_PASS_THRU_GET_NEXT_NAMESPACE function. This is what I used to get the available namespaces available in the system.

In my case I'm sending some commands in my application with the NSID set to UINT32_MAX in order to request controller wide SMART/Health log data among other things. There are some commands where setting the NSID to another value may also be useful. For example, DST can use the NSID in the command to change between testing only the controller (0), testing all namespaces (UINT32_MAX), and a specific namespace.


[Hao]: Yes. Just as you said, commands like DST or Get Log Page will have different target when different Namespace values are provided.
I think for your case, you can:

1.       Set both the 'NamespaceId' parameter and 'Nsid' field to 0 if the target is the controller;

2.       Set both of them MAX_UINT32 if the target is all the namespaces;

3.       Set both of them to a corresponding ID if the target is a specific namespace.

As you can see from the current implementation of NvmExpressPassThru() function, the 'NamespaceId' parameter does not affect the construct of the submission queue item.
The 'NamespaceId' parameter only serves as a valid check for the 'Nsid' field in the EFI_NVM_EXPRESS_COMMAND structure.


The modification I made was done where the passthru command's NSID was actually being checked, which seemed like the best place to add this exception for admin commands.

-Tyler


On Tue, Sep 3, 2019 at 9:39 PM Wu, Hao A <hao.a.wu@intel.com<mailto:hao.a.wu@intel.com>> wrote:
> -----Original Message-----
> From: Wu, Hao A
> Sent: Wednesday, September 04, 2019 11:39 AM
> To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; Tyler Erickson
> Cc: Wu, Hao A; Ni, Ray
> Subject: [PATCH v1 1/1] MdeModulePkg/NvmExpressDxe: Allow other NSIDs
> for Admin commands
>
> Repost the mail to the list.
>
> Best Regards,
> Hao Wu
>
> -----Original Message-----
> From: Tyler Erickson [mailto:tyler.j.erickson@seagate.com<mailto:tyler.j.erickson@seagate.com>]
> Sent: Tuesday, September 03, 2019 9:55 PM
> To: edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
> Cc: Wu, Hao A; Ni, Ray
> Subject: [PATCH v1 1/1] MdeModulePkg/NvmExpressDxe: Allow other NSIDs
> for Admin commands
>
> Cc: Hao A Wu <hao.a.wu@intel.com<mailto:hao.a.wu@intel.com>>
> Cc: Ray Ni <ray.ni@intel.com<mailto:ray.ni@intel.com>>
> Signed-off-by: Tyler Erickson <tyler.j.erickson@seagate.com<mailto:tyler.j.erickson@seagate.com>>
> ---
>  MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c
> b/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c
> index 8e721379466a..78a3c663ded4 100644
> --- a/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c
> +++ b/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c
> @@ -561,7 +561,7 @@ NvmExpressPassThru (
>    Sq  = Private->SqBuffer[QueueId] + Private->SqTdbl[QueueId].Sqt;
>    Cq  = Private->CqBuffer[QueueId] + Private->CqHdbl[QueueId].Cqh;
>
> -  if (Packet->NvmeCmd->Nsid != NamespaceId) {
> +  if (Packet->QueueType != NVME_ADMIN_QUEUE && Packet->NvmeCmd-
> >Nsid != NamespaceId) {


Hello,

Per my understanding to the codes, I think the:

* Input parameter 'NamespaceId' for the PassThru() service
* The 'Nsid' field of the EFI_NVM_EXPRESS_COMMAND

are of the same meaning.

Do you think setting these 2 values identical when calling the PassThru()
service can resolve the issue you met?

Best Regards,
Hao Wu


>      return EFI_INVALID_PARAMETER;
>    }
>
> --
> 2.17.1


[-- Attachment #2: Type: text/html, Size: 45114 bytes --]