[PATCH v1 0/3] NvmExpressDxe: Bug fixes within NvmExpressPassThru()

[PATCH v1 0/3] NvmExpressDxe: Bug fixes within NvmExpressPassThru()

[Hao Wu]

The series will address a couple of bugs within the NvmExpressPassThru()
function. Please refer to the log messages of each commit for more
details.

Cc: Liangcheng Tang <liangcheng.tang@intel.com>
Cc: Jiewen Yao <Jiewen.yao@intel.com>
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Star Zeng <star.zeng@intel.com>

Hao Wu (3):
  MdeModulePkg/NvmExpressDxe: Refine data buffer & len check in PassThru
  MdeModulePkg/NvmExpressDxe: Always copy CQ entry to PassThru packet
  MdeModulePkg/NvmExpressDxe: Refine PassThru IO queue creation behavior

 MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c | 84 +++++++++++++-------
 1 file changed, 56 insertions(+), 28 deletions(-)

-- 
2.12.0.windows.1

[PATCH v1 1/3] MdeModulePkg/NvmExpressDxe: Refine data buffer & len check in PassThru

[Hao Wu]

REF:https://bugzilla.tianocore.org/show_bug.cgi?id=1142

According to the the NVM Express spec Revision 1.1, for some commands
(like Get/Set Feature Command, Figure 89 & 90 of the spec), the Memory
Buffer maybe optional although the command opcode indicates there is a
data transfer between host & controller (Get/Set Feature Command, Figure
38 of the spec).

Hence, this commit refine the checks for the 'TransferLength' and
'TransferBuffer' field of the EFI_NVM_EXPRESS_PASS_THRU_COMMAND_PACKET
structure to address this issue.

Cc: Liangcheng Tang <liangcheng.tang@intel.com>
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Hao Wu <hao.a.wu@intel.com>
---
 MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c | 33 +++++++++++---------
 1 file changed, 18 insertions(+), 15 deletions(-)

diff --git a/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c b/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c
index 2468871322..bfcd349794 100644
--- a/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c
+++ b/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c
@@ -595,7 +595,8 @@ NvmExpressPassThru (
   //
   if (((Sq->Opc & (BIT0 | BIT1)) != 0) &&
       !((Packet->QueueType == NVME_ADMIN_QUEUE) && ((Sq->Opc == NVME_ADMIN_CRIOCQ_CMD) || (Sq->Opc == NVME_ADMIN_CRIOSQ_CMD)))) {
-    if ((Packet->TransferLength == 0) || (Packet->TransferBuffer == NULL)) {
+    if (((Packet->TransferLength != 0) && (Packet->TransferBuffer == NULL)) ||
+        ((Packet->TransferLength == 0) && (Packet->TransferBuffer != NULL))) {
       return EFI_INVALID_PARAMETER;
     }
 
@@ -605,21 +606,23 @@ NvmExpressPassThru (
       Flag = EfiPciIoOperationBusMasterWrite;
     }
 
-    MapLength = Packet->TransferLength;
-    Status = PciIo->Map (
-                      PciIo,
-                      Flag,
-                      Packet->TransferBuffer,
-                      &MapLength,
-                      &PhyAddr,
-                      &MapData
-                      );
-    if (EFI_ERROR (Status) || (Packet->TransferLength != MapLength)) {
-      return EFI_OUT_OF_RESOURCES;
-    }
+    if ((Packet->TransferLength != 0) && (Packet->TransferBuffer != NULL)) {
+      MapLength = Packet->TransferLength;
+      Status = PciIo->Map (
+                        PciIo,
+                        Flag,
+                        Packet->TransferBuffer,
+                        &MapLength,
+                        &PhyAddr,
+                        &MapData
+                        );
+      if (EFI_ERROR (Status) || (Packet->TransferLength != MapLength)) {
+        return EFI_OUT_OF_RESOURCES;
+      }
 
-    Sq->Prp[0] = PhyAddr;
-    Sq->Prp[1] = 0;
+      Sq->Prp[0] = PhyAddr;
+      Sq->Prp[1] = 0;
+    }
 
     if((Packet->MetadataLength != 0) && (Packet->MetadataBuffer != NULL)) {
       MapLength = Packet->MetadataLength;
-- 
2.12.0.windows.1

[PATCH v1 2/3] MdeModulePkg/NvmExpressDxe: Always copy CQ entry to PassThru packet

[Hao Wu]

REF:https://bugzilla.tianocore.org/show_bug.cgi?id=1259

According to the the NVM Express spec Revision 1.1, for some commands,
command-related information will be stored in the Dword 0 of the
completion queue entry.

One case is for the Get Features Command (Section 5.9.2 of the spec),
Dword 0 of the completion queue entry may contain feature information.

Hence, this commit will always copy the content of completion queue entry
to the PassThru packet regardless of the execution result of the command.

Cc: Liangcheng Tang <liangcheng.tang@intel.com>
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Hao Wu <hao.a.wu@intel.com>
---
 MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c b/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c
index bfcd349794..c52e960771 100644
--- a/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c
+++ b/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c
@@ -781,17 +781,16 @@ NvmExpressPassThru (
     } else {
       Status = EFI_DEVICE_ERROR;
       //
-      // Copy the Respose Queue entry for this command to the callers response buffer
-      //
-      CopyMem(Packet->NvmeCompletion, Cq, sizeof(EFI_NVM_EXPRESS_COMPLETION));
-
-      //
       // Dump every completion entry status for debugging.
       //
       DEBUG_CODE_BEGIN();
         NvmeDumpStatus(Cq);
       DEBUG_CODE_END();
     }
+    //
+    // Copy the Respose Queue entry for this command to the callers response buffer
+    //
+    CopyMem(Packet->NvmeCompletion, Cq, sizeof(EFI_NVM_EXPRESS_COMPLETION));
   } else {
     //
     // Timeout occurs for an NVMe command. Reset the controller to abort the
-- 
2.12.0.windows.1

[PATCH v1 3/3] MdeModulePkg/NvmExpressDxe: Refine PassThru IO queue creation behavior

[Hao Wu]

REF:https://bugzilla.tianocore.org/show_bug.cgi?id=1260

For the PassThru() service of NVM Express Pass Through Protocol, the
current implementation (function NvmExpressPassThru()) will only use the
IO Completion/Submission queues created internally by this driver during
the controller initialization process. Any other IO queues created will
not be consumed.

So the value is little to accept external IO Completion/Submission queue
creation request. This commit will refine the behavior of function
NvmExpressPassThru(), it will only accept driver internal IO queue
creation commands and will return "EFI_UNSUPPORTED" for external ones.

Cc: Jiewen Yao <Jiewen.yao@intel.com>
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Hao Wu <hao.a.wu@intel.com>
---
 MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c | 42 ++++++++++++++++----
 1 file changed, 34 insertions(+), 8 deletions(-)

diff --git a/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c b/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c
index c52e960771..0c550bd52c 100644
--- a/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c
+++ b/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c
@@ -476,6 +476,7 @@ NvmExpressPassThru (
   UINT32                         Data;
   NVME_PASS_THRU_ASYNC_REQ       *AsyncRequest;
   EFI_TPL                        OldTpl;
+  UINT32                         CrIoQid;
 
   //
   // check the data fields in Packet parameter.
@@ -587,14 +588,39 @@ NvmExpressPassThru (
   }
 
   Sq->Prp[0] = (UINT64)(UINTN)Packet->TransferBuffer;
-  //
-  // If the NVMe cmd has data in or out, then mapping the user buffer to the PCI controller specific addresses.
-  // Note here we don't handle data buffer for CreateIOSubmitionQueue and CreateIOCompletionQueue cmds because
-  // these two cmds are special which requires their data buffer must support simultaneous access by both the
-  // processor and a PCI Bus Master. It's caller's responsbility to ensure this.
-  //
-  if (((Sq->Opc & (BIT0 | BIT1)) != 0) &&
-      !((Packet->QueueType == NVME_ADMIN_QUEUE) && ((Sq->Opc == NVME_ADMIN_CRIOCQ_CMD) || (Sq->Opc == NVME_ADMIN_CRIOSQ_CMD)))) {
+  if ((Packet->QueueType == NVME_ADMIN_QUEUE) &&
+      ((Sq->Opc == NVME_ADMIN_CRIOCQ_CMD) || (Sq->Opc == NVME_ADMIN_CRIOSQ_CMD))) {
+    //
+    // Command Dword 10 should be valid for CreateIOCompletionQueue and
+    // CreateIOSubmissionQueue commands.
+    //
+    if (!(Packet->NvmeCmd->Flags & CDW10_VALID)) {
+      return EFI_INVALID_PARAMETER;
+    }
+
+    //
+    // Bits 15:0 of Command Dword 10 is the Queue Identifier (QID) for
+    // CreateIOCompletionQueue and CreateIOSubmissionQueue commands.
+    //
+    CrIoQid = Packet->NvmeCmd->Cdw10 & 0xFFFF;
+
+    //
+    // Currently, we only use the IO Completion/Submission queues created internally
+    // by this driver during controller initialization. Any other IO queues created
+    // will not be consumed here. The value is little to accept external IO queue
+    // creation requests, so here we will return EFI_UNSUPPORTED for external IO
+    // queue creation request.
+    //
+    if ((CrIoQid >= NVME_MAX_QUEUES) ||
+        ((Sq->Opc == NVME_ADMIN_CRIOCQ_CMD) && (Packet->TransferBuffer != Private->CqBufferPciAddr[CrIoQid])) ||
+        ((Sq->Opc == NVME_ADMIN_CRIOSQ_CMD) && (Packet->TransferBuffer != Private->SqBufferPciAddr[CrIoQid]))) {
+      DEBUG ((DEBUG_ERROR, "NvmExpressPassThru: Does not support external IO queues creation request.\n"));
+      return EFI_UNSUPPORTED;
+    }
+  } else if ((Sq->Opc & (BIT0 | BIT1)) != 0) {
+    //
+    // If the NVMe cmd has data in or out, then mapping the user buffer to the PCI controller specific addresses.
+    //
     if (((Packet->TransferLength != 0) && (Packet->TransferBuffer == NULL)) ||
         ((Packet->TransferLength == 0) && (Packet->TransferBuffer != NULL))) {
       return EFI_INVALID_PARAMETER;
-- 
2.12.0.windows.1

Re: [PATCH v1 3/3] MdeModulePkg/NvmExpressDxe: Refine PassThru IO queue creation behavior

[Ni, Ruiyu]

On 10/18/2018 2:42 PM, Hao Wu wrote:
> REF:https://bugzilla.tianocore.org/show_bug.cgi?id=1260
> 
> For the PassThru() service of NVM Express Pass Through Protocol, the
> current implementation (function NvmExpressPassThru()) will only use the
> IO Completion/Submission queues created internally by this driver during
> the controller initialization process. Any other IO queues created will
> not be consumed.
> 
> So the value is little to accept external IO Completion/Submission queue
> creation request. This commit will refine the behavior of function
> NvmExpressPassThru(), it will only accept driver internal IO queue
> creation commands and will return "EFI_UNSUPPORTED" for external ones.
> 
> Cc: Jiewen Yao <Jiewen.yao@intel.com>
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> Cc: Star Zeng <star.zeng@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Hao Wu <hao.a.wu@intel.com>
> ---
>   MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c | 42 ++++++++++++++++----
>   1 file changed, 34 insertions(+), 8 deletions(-)
> 
> diff --git a/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c b/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c
> index c52e960771..0c550bd52c 100644
> --- a/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c
> +++ b/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c
> @@ -476,6 +476,7 @@ NvmExpressPassThru (
>     UINT32                         Data;
>     NVME_PASS_THRU_ASYNC_REQ       *AsyncRequest;
>     EFI_TPL                        OldTpl;
> +  UINT32                         CrIoQid;
>   
>     //
>     // check the data fields in Packet parameter.
> @@ -587,14 +588,39 @@ NvmExpressPassThru (
>     }
>   
>     Sq->Prp[0] = (UINT64)(UINTN)Packet->TransferBuffer;
> -  //
> -  // If the NVMe cmd has data in or out, then mapping the user buffer to the PCI controller specific addresses.
> -  // Note here we don't handle data buffer for CreateIOSubmitionQueue and CreateIOCompletionQueue cmds because
> -  // these two cmds are special which requires their data buffer must support simultaneous access by both the
> -  // processor and a PCI Bus Master. It's caller's responsbility to ensure this.
> -  //
> -  if (((Sq->Opc & (BIT0 | BIT1)) != 0) &&
> -      !((Packet->QueueType == NVME_ADMIN_QUEUE) && ((Sq->Opc == NVME_ADMIN_CRIOCQ_CMD) || (Sq->Opc == NVME_ADMIN_CRIOSQ_CMD)))) {
> +  if ((Packet->QueueType == NVME_ADMIN_QUEUE) &&
> +      ((Sq->Opc == NVME_ADMIN_CRIOCQ_CMD) || (Sq->Opc == NVME_ADMIN_CRIOSQ_CMD))) {
> +    //
> +    // Command Dword 10 should be valid for CreateIOCompletionQueue and
> +    // CreateIOSubmissionQueue commands.
> +    //
> +    if (!(Packet->NvmeCmd->Flags & CDW10_VALID)) {
> +      return EFI_INVALID_PARAMETER;
> +    }
> +
> +    //
> +    // Bits 15:0 of Command Dword 10 is the Queue Identifier (QID) for
> +    // CreateIOCompletionQueue and CreateIOSubmissionQueue commands.
> +    //
> +    CrIoQid = Packet->NvmeCmd->Cdw10 & 0xFFFF;
> +
> +    //
> +    // Currently, we only use the IO Completion/Submission queues created internally
> +    // by this driver during controller initialization. Any other IO queues created
> +    // will not be consumed here. The value is little to accept external IO queue
> +    // creation requests, so here we will return EFI_UNSUPPORTED for external IO
> +    // queue creation request.
> +    //
> +    if ((CrIoQid >= NVME_MAX_QUEUES) ||
> +        ((Sq->Opc == NVME_ADMIN_CRIOCQ_CMD) && (Packet->TransferBuffer != Private->CqBufferPciAddr[CrIoQid])) ||
> +        ((Sq->Opc == NVME_ADMIN_CRIOSQ_CMD) && (Packet->TransferBuffer != Private->SqBufferPciAddr[CrIoQid]))) {
> +      DEBUG ((DEBUG_ERROR, "NvmExpressPassThru: Does not support external IO queues creation request.\n"));
> +      return EFI_UNSUPPORTED;
> +    }

Does the above check is to make sure only accept IO queues creation 
request from NvmeCreateIoCompletionQueue() and 
NvmeCreateIoSubmissionQueue()?

The check is very complex and unnecessary IMO. If we introduce a boolean 
field like CreateQueue in the Private structure and set it to TRUE when 
calling PassThru() from the above two internal functions, the above 
check can be replaced with only one line check:
   if (Private->CreateQueue)

It does introduce a "dirty" flag in Private structure, but the above 
complex check can be avoided.
What's your opinion?

> +  } else if ((Sq->Opc & (BIT0 | BIT1)) != 0) {
> +    //
> +    // If the NVMe cmd has data in or out, then mapping the user buffer to the PCI controller specific addresses.
> +    //
>       if (((Packet->TransferLength != 0) && (Packet->TransferBuffer == NULL)) ||
>           ((Packet->TransferLength == 0) && (Packet->TransferBuffer != NULL))) {
>         return EFI_INVALID_PARAMETER;
> 


-- 
Thanks,
Ray

Re: [PATCH v1 1/3] MdeModulePkg/NvmExpressDxe: Refine data buffer & len check in PassThru

[Ni, Ruiyu]

On 10/18/2018 2:41 PM, Hao Wu wrote:
> REF:https://bugzilla.tianocore.org/show_bug.cgi?id=1142
> 
> According to the the NVM Express spec Revision 1.1, for some commands
> (like Get/Set Feature Command, Figure 89 & 90 of the spec), the Memory
> Buffer maybe optional although the command opcode indicates there is a
> data transfer between host & controller (Get/Set Feature Command, Figure
> 38 of the spec).
> 
> Hence, this commit refine the checks for the 'TransferLength' and
> 'TransferBuffer' field of the EFI_NVM_EXPRESS_PASS_THRU_COMMAND_PACKET
> structure to address this issue.
> 
> Cc: Liangcheng Tang <liangcheng.tang@intel.com>
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> Cc: Star Zeng <star.zeng@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Hao Wu <hao.a.wu@intel.com>
> ---
>   MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c | 33 +++++++++++---------
>   1 file changed, 18 insertions(+), 15 deletions(-)
> 
> diff --git a/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c b/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c
> index 2468871322..bfcd349794 100644
> --- a/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c
> +++ b/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c
> @@ -595,7 +595,8 @@ NvmExpressPassThru (
>     //
>     if (((Sq->Opc & (BIT0 | BIT1)) != 0) &&
>         !((Packet->QueueType == NVME_ADMIN_QUEUE) && ((Sq->Opc == NVME_ADMIN_CRIOCQ_CMD) || (Sq->Opc == NVME_ADMIN_CRIOSQ_CMD)))) {
> -    if ((Packet->TransferLength == 0) || (Packet->TransferBuffer == NULL)) {
> +    if (((Packet->TransferLength != 0) && (Packet->TransferBuffer == NULL)) ||
> +        ((Packet->TransferLength == 0) && (Packet->TransferBuffer != NULL))) {
>         return EFI_INVALID_PARAMETER;
>       }
>   
> @@ -605,21 +606,23 @@ NvmExpressPassThru (
>         Flag = EfiPciIoOperationBusMasterWrite;
>       }
>   
> -    MapLength = Packet->TransferLength;
> -    Status = PciIo->Map (
> -                      PciIo,
> -                      Flag,
> -                      Packet->TransferBuffer,
> -                      &MapLength,
> -                      &PhyAddr,
> -                      &MapData
> -                      );
> -    if (EFI_ERROR (Status) || (Packet->TransferLength != MapLength)) {
> -      return EFI_OUT_OF_RESOURCES;
> -    }
> +    if ((Packet->TransferLength != 0) && (Packet->TransferBuffer != NULL)) {
> +      MapLength = Packet->TransferLength;
> +      Status = PciIo->Map (
> +                        PciIo,
> +                        Flag,
> +                        Packet->TransferBuffer,
> +                        &MapLength,
> +                        &PhyAddr,
> +                        &MapData
> +                        );
> +      if (EFI_ERROR (Status) || (Packet->TransferLength != MapLength)) {
> +        return EFI_OUT_OF_RESOURCES;
> +      }
>   
> -    Sq->Prp[0] = PhyAddr;
> -    Sq->Prp[1] = 0;
> +      Sq->Prp[0] = PhyAddr;
> +      Sq->Prp[1] = 0;
> +    }
>   
>       if((Packet->MetadataLength != 0) && (Packet->MetadataBuffer != NULL)) {
>         MapLength = Packet->MetadataLength;
> 
Reviewed-by: Ruiyu Ni <ruiyu.ni@intel.com>

-- 
Thanks,
Ray

Re: [PATCH v1 2/3] MdeModulePkg/NvmExpressDxe: Always copy CQ entry to PassThru packet

[Ni, Ruiyu]

On 10/18/2018 2:41 PM, Hao Wu wrote:
> REF:https://bugzilla.tianocore.org/show_bug.cgi?id=1259
> 
> According to the the NVM Express spec Revision 1.1, for some commands,
> command-related information will be stored in the Dword 0 of the
> completion queue entry.
> 
> One case is for the Get Features Command (Section 5.9.2 of the spec),
> Dword 0 of the completion queue entry may contain feature information.
> 
> Hence, this commit will always copy the content of completion queue entry
> to the PassThru packet regardless of the execution result of the command.
> 
> Cc: Liangcheng Tang <liangcheng.tang@intel.com>
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> Cc: Star Zeng <star.zeng@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Hao Wu <hao.a.wu@intel.com>
> ---
>   MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c | 9 ++++-----
>   1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c b/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c
> index bfcd349794..c52e960771 100644
> --- a/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c
> +++ b/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c
> @@ -781,17 +781,16 @@ NvmExpressPassThru (
>       } else {
>         Status = EFI_DEVICE_ERROR;
>         //
> -      // Copy the Respose Queue entry for this command to the callers response buffer
> -      //
> -      CopyMem(Packet->NvmeCompletion, Cq, sizeof(EFI_NVM_EXPRESS_COMPLETION));
> -
> -      //
>         // Dump every completion entry status for debugging.
>         //
>         DEBUG_CODE_BEGIN();
>           NvmeDumpStatus(Cq);
>         DEBUG_CODE_END();
>       }
> +    //
> +    // Copy the Respose Queue entry for this command to the callers response buffer
> +    //
> +    CopyMem(Packet->NvmeCompletion, Cq, sizeof(EFI_NVM_EXPRESS_COMPLETION));
>     } else {
>       //
>       // Timeout occurs for an NVMe command. Reset the controller to abort the
> 
Reviewed-by: Ruiyu Ni <ruiyu.ni@intel.com>

-- 
Thanks,
Ray

Re: [PATCH v1 3/3] MdeModulePkg/NvmExpressDxe: Refine PassThru IO queue creation behavior

[Wu, Hao A]

> -----Original Message-----
> From: Ni, Ruiyu
> Sent: Tuesday, October 23, 2018 4:54 PM
> To: Wu, Hao A; edk2-devel@lists.01.org
> Cc: Yao, Jiewen; Zeng, Star
> Subject: Re: [PATCH v1 3/3] MdeModulePkg/NvmExpressDxe: Refine PassThru
> IO queue creation behavior
> 
> On 10/18/2018 2:42 PM, Hao Wu wrote:
> > REF:https://bugzilla.tianocore.org/show_bug.cgi?id=1260
> >
> > For the PassThru() service of NVM Express Pass Through Protocol, the
> > current implementation (function NvmExpressPassThru()) will only use the
> > IO Completion/Submission queues created internally by this driver during
> > the controller initialization process. Any other IO queues created will
> > not be consumed.
> >
> > So the value is little to accept external IO Completion/Submission queue
> > creation request. This commit will refine the behavior of function
> > NvmExpressPassThru(), it will only accept driver internal IO queue
> > creation commands and will return "EFI_UNSUPPORTED" for external ones.
> >
> > Cc: Jiewen Yao <Jiewen.yao@intel.com>
> > Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> > Cc: Star Zeng <star.zeng@intel.com>
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Hao Wu <hao.a.wu@intel.com>
> > ---
> >   MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c | 42
> ++++++++++++++++----
> >   1 file changed, 34 insertions(+), 8 deletions(-)
> >
> > diff --git a/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c
> b/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c
> > index c52e960771..0c550bd52c 100644
> > --- a/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c
> > +++ b/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c
> > @@ -476,6 +476,7 @@ NvmExpressPassThru (
> >     UINT32                         Data;
> >     NVME_PASS_THRU_ASYNC_REQ       *AsyncRequest;
> >     EFI_TPL                        OldTpl;
> > +  UINT32                         CrIoQid;
> >
> >     //
> >     // check the data fields in Packet parameter.
> > @@ -587,14 +588,39 @@ NvmExpressPassThru (
> >     }
> >
> >     Sq->Prp[0] = (UINT64)(UINTN)Packet->TransferBuffer;
> > -  //
> > -  // If the NVMe cmd has data in or out, then mapping the user buffer to
> the PCI controller specific addresses.
> > -  // Note here we don't handle data buffer for CreateIOSubmitionQueue
> and CreateIOCompletionQueue cmds because
> > -  // these two cmds are special which requires their data buffer must
> support simultaneous access by both the
> > -  // processor and a PCI Bus Master. It's caller's responsbility to ensure this.
> > -  //
> > -  if (((Sq->Opc & (BIT0 | BIT1)) != 0) &&
> > -      !((Packet->QueueType == NVME_ADMIN_QUEUE) && ((Sq->Opc ==
> NVME_ADMIN_CRIOCQ_CMD) || (Sq->Opc == NVME_ADMIN_CRIOSQ_CMD))))
> {
> > +  if ((Packet->QueueType == NVME_ADMIN_QUEUE) &&
> > +      ((Sq->Opc == NVME_ADMIN_CRIOCQ_CMD) || (Sq->Opc ==
> NVME_ADMIN_CRIOSQ_CMD))) {
> > +    //
> > +    // Command Dword 10 should be valid for CreateIOCompletionQueue
> and
> > +    // CreateIOSubmissionQueue commands.
> > +    //
> > +    if (!(Packet->NvmeCmd->Flags & CDW10_VALID)) {
> > +      return EFI_INVALID_PARAMETER;
> > +    }
> > +
> > +    //
> > +    // Bits 15:0 of Command Dword 10 is the Queue Identifier (QID) for
> > +    // CreateIOCompletionQueue and CreateIOSubmissionQueue commands.
> > +    //
> > +    CrIoQid = Packet->NvmeCmd->Cdw10 & 0xFFFF;
> > +
> > +    //
> > +    // Currently, we only use the IO Completion/Submission queues created
> internally
> > +    // by this driver during controller initialization. Any other IO queues
> created
> > +    // will not be consumed here. The value is little to accept external IO
> queue
> > +    // creation requests, so here we will return EFI_UNSUPPORTED for
> external IO
> > +    // queue creation request.
> > +    //
> > +    if ((CrIoQid >= NVME_MAX_QUEUES) ||
> > +        ((Sq->Opc == NVME_ADMIN_CRIOCQ_CMD) && (Packet-
> >TransferBuffer != Private->CqBufferPciAddr[CrIoQid])) ||
> > +        ((Sq->Opc == NVME_ADMIN_CRIOSQ_CMD) && (Packet-
> >TransferBuffer != Private->SqBufferPciAddr[CrIoQid]))) {
> > +      DEBUG ((DEBUG_ERROR, "NvmExpressPassThru: Does not support
> external IO queues creation request.\n"));
> > +      return EFI_UNSUPPORTED;
> > +    }
> 
> Does the above check is to make sure only accept IO queues creation
> request from NvmeCreateIoCompletionQueue() and
> NvmeCreateIoSubmissionQueue()?

Yes. Actually, the above codes (including the "Packet->NvmeCmd->Flags") are to
ensure that the IO queues creation request is internal.

> 
> The check is very complex and unnecessary IMO. If we introduce a boolean
> field like CreateQueue in the Private structure and set it to TRUE when
> calling PassThru() from the above two internal functions, the above
> check can be replaced with only one line check:
>    if (Private->CreateQueue)
> 
> It does introduce a "dirty" flag in Private structure, but the above
> complex check can be avoided.
> What's your opinion?

Yes, I think for this way the code is more simple.
I will propose a V2 of the series.

Best Regards,
Hao Wu

> 
> > +  } else if ((Sq->Opc & (BIT0 | BIT1)) != 0) {
> > +    //
> > +    // If the NVMe cmd has data in or out, then mapping the user buffer to
> the PCI controller specific addresses.
> > +    //
> >       if (((Packet->TransferLength != 0) && (Packet->TransferBuffer == NULL))
> ||
> >           ((Packet->TransferLength == 0) && (Packet->TransferBuffer != NULL))) {
> >         return EFI_INVALID_PARAMETER;
> >
> 
> 
> --
> Thanks,
> Ray