[edk2-devel][PATCH v2 0/2] Add Volatile Keyword to NvmExpress Passthru CQs

[edk2-devel][PATCH v2 0/2] Add Volatile Keyword to NvmExpress Passthru CQs

[Oliver Smith-Denny]

NVMe CQs are hardware queues mapped to EFI memory.
In the NVMExpress Passthru implementations in PEI and DXE,
it has been observed that NVMe CQs are not marked
volatile, meaning the compiler has significant leeway
to optimize accesses to these structures.

This led to an issue where the passthru driver waited
for a timeout period for an NVMe CQ to mark that it was
finished with a transaction, but the compiler had
optimized away the read to the actual HW mapped memory,
so the transaction had completed but the timeout continued.

Marking the CQs as volatile fixes this issue as the reads
happen to the actual HW.

Personal GitHub PR: https://github.com/tianocore/edk2/pull/4320
Github branch: https://github.com/os-d/edk2/tree/osde/volatile_cq_v2

Changes v1 => v2:
=================
- Remove volatile keyword from SQ

Cc: Hao A Wu <hao.a.wu@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Michael Kubacki <mikuback@linux.microsoft.com>
Cc: Sean Brogan <sean.brogan@microsoft.com>


Oliver Smith-Denny (2):
  Add the volatile keyword to NvmExpressDxe's Passthru CQ
  Add volatile keyword to NvmExpressPei's Passthru CQ

 MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c    | 6 +++---
 MdeModulePkg/Bus/Pci/NvmExpressPei/NvmExpressPeiPassThru.c | 6 +++---
 2 files changed, 6 insertions(+), 6 deletions(-)

-- 
2.40.0

[edk2-devel][PATCH v2 1/2] Add the volatile keyword to NvmExpressDxe's Passthru CQ

[Oliver Smith-Denny]

This updates the relevant functions that expect a non-volatile
structure to be passed to them to take casts of the CQ
now that it is volatile.

Cc: Hao A Wu <hao.a.wu@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Michael Kubacki <mikuback@linux.microsoft.com>
Cc: Sean Brogan <sean.brogan@microsoft.com>
Signed-off-by: Oliver Smith-Denny <osde@linux.microsoft.com>
---

 MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c b/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c
index f37baa626a16..2ff2cb0e8d36 100644
--- a/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c
+++ b/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c
@@ -460,7 +460,7 @@ NvmExpressPassThru (
   EFI_STATUS                     PreviousStatus;
   EFI_PCI_IO_PROTOCOL            *PciIo;
   NVME_SQ                        *Sq;
-  NVME_CQ                        *Cq;
+  volatile NVME_CQ               *Cq;
   UINT16                         QueueId;
   UINT16                         QueueSize;
   UINT32                         Bytes;
@@ -815,14 +815,14 @@ NvmExpressPassThru (
       // Dump every completion entry status for debugging.
       //
       DEBUG_CODE_BEGIN ();
-      NvmeDumpStatus (Cq);
+      NvmeDumpStatus ((NVME_CQ *)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));
+    CopyMem (Packet->NvmeCompletion, (VOID *)Cq, sizeof (EFI_NVM_EXPRESS_COMPLETION));
   } else {
     //
     // Timeout occurs for an NVMe command. Reset the controller to abort the
-- 
2.40.0

[edk2-devel][PATCH v2 2/2] Add volatile keyword to NvmExpressPei's Passthru CQ

[Oliver Smith-Denny]

This applies the volatile keyword and appropriate casts
to the NvmExpressPei's Passthru CQ.

Cc: Hao A Wu <hao.a.wu@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Michael Kubacki <mikuback@linux.microsoft.com>
Cc: Sean Brogan <sean.brogan@microsoft.com>
Signed-off-by: Oliver Smith-Denny <osde@linux.microsoft.com>
---

 MdeModulePkg/Bus/Pci/NvmExpressPei/NvmExpressPeiPassThru.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/MdeModulePkg/Bus/Pci/NvmExpressPei/NvmExpressPeiPassThru.c b/MdeModulePkg/Bus/Pci/NvmExpressPei/NvmExpressPeiPassThru.c
index 5081b53cd5e8..ac9328047fef 100644
--- a/MdeModulePkg/Bus/Pci/NvmExpressPei/NvmExpressPeiPassThru.c
+++ b/MdeModulePkg/Bus/Pci/NvmExpressPei/NvmExpressPeiPassThru.c
@@ -115,7 +115,7 @@ NvmeCreatePrpList (
 **/
 EFI_STATUS
 NvmeCheckCqStatus (
-  IN NVME_CQ  *Cq
+  IN volatile NVME_CQ  *Cq
   )
 {
   if ((Cq->Sct == 0x0) && (Cq->Sc == 0x0)) {
@@ -344,7 +344,7 @@ NvmePassThruExecute (
 {
   EFI_STATUS             Status;
   NVME_SQ                *Sq;
-  NVME_CQ                *Cq;
+  volatile NVME_CQ       *Cq;
   UINT8                  QueueId;
   UINTN                  SqSize;
   UINTN                  CqSize;
@@ -617,7 +617,7 @@ NvmePassThruExecute (
   //
   // Copy the Respose Queue entry for this command to the callers response buffer
   //
-  CopyMem (Packet->NvmeCompletion, Cq, sizeof (EFI_NVM_EXPRESS_COMPLETION));
+  CopyMem (Packet->NvmeCompletion, (VOID *)Cq, sizeof (EFI_NVM_EXPRESS_COMPLETION));
 
   //
   // Check the NVMe cmd execution result
-- 
2.40.0

Re: [edk2-devel][PATCH v2 0/2] Add Volatile Keyword to NvmExpress Passthru CQs

[Wu, Hao A]

For the series:
Reviewed-by: Hao A Wu <hao.a.wu@intel.com>

Will wait a couple of days before merging to see if comments from other reviewers.

Best Regards,
Hao Wu

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Oliver
> Smith-Denny
> Sent: Friday, April 28, 2023 1:36 AM
> To: devel@edk2.groups.io
> Cc: Wu, Hao A <hao.a.wu@intel.com>; Ni, Ray <ray.ni@intel.com>; Wang, Jian
> J <jian.j.wang@intel.com>; Gao, Liming <gaoliming@byosoft.com.cn>; Michael
> Kubacki <mikuback@linux.microsoft.com>; Sean Brogan
> <sean.brogan@microsoft.com>
> Subject: [edk2-devel][PATCH v2 0/2] Add Volatile Keyword to NvmExpress
> Passthru CQs
> 
> NVMe CQs are hardware queues mapped to EFI memory.
> In the NVMExpress Passthru implementations in PEI and DXE,
> it has been observed that NVMe CQs are not marked
> volatile, meaning the compiler has significant leeway
> to optimize accesses to these structures.
> 
> This led to an issue where the passthru driver waited
> for a timeout period for an NVMe CQ to mark that it was
> finished with a transaction, but the compiler had
> optimized away the read to the actual HW mapped memory,
> so the transaction had completed but the timeout continued.
> 
> Marking the CQs as volatile fixes this issue as the reads
> happen to the actual HW.
> 
> Personal GitHub PR: https://github.com/tianocore/edk2/pull/4320
> Github branch: https://github.com/os-d/edk2/tree/osde/volatile_cq_v2
> 
> Changes v1 => v2:
> =================
> - Remove volatile keyword from SQ
> 
> Cc: Hao A Wu <hao.a.wu@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Liming Gao <gaoliming@byosoft.com.cn>
> Cc: Michael Kubacki <mikuback@linux.microsoft.com>
> Cc: Sean Brogan <sean.brogan@microsoft.com>
> 
> 
> Oliver Smith-Denny (2):
>   Add the volatile keyword to NvmExpressDxe's Passthru CQ
>   Add volatile keyword to NvmExpressPei's Passthru CQ
> 
>  MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c    | 6 +++---
>  MdeModulePkg/Bus/Pci/NvmExpressPei/NvmExpressPeiPassThru.c | 6 +++---
>  2 files changed, 6 insertions(+), 6 deletions(-)
> 
> --
> 2.40.0
> 
> 
> 
> -=-=-=-=-=-=
> Groups.io Links: You receive all messages sent to this group.
> View/Reply Online (#103725): https://edk2.groups.io/g/devel/message/103725
> Mute This Topic: https://groups.io/mt/98541927/1768737
> Group Owner: devel+owner@edk2.groups.io
> Unsubscribe: https://edk2.groups.io/g/devel/unsub [hao.a.wu@intel.com]
> -=-=-=-=-=-=
> 

Re: [edk2-devel][PATCH v2 0/2] Add Volatile Keyword to NvmExpress Passthru CQs

[Wu, Hao A]

Pushed via:
PR - https://github.com/tianocore/edk2/pull/4353
Commits:
https://github.com/tianocore/edk2/commit/293b97d0c4624c13a4e934294d2c4b161a09a91b
https://github.com/tianocore/edk2/commit/8dbf868e02c71b407e31f9b41b5266169c702812

Best Regards,
Hao Wu

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Wu, Hao A
> Sent: Thursday, May 4, 2023 10:20 AM
> To: devel@edk2.groups.io; osde@linux.microsoft.com
> Cc: Ni, Ray <ray.ni@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Gao,
> Liming <gaoliming@byosoft.com.cn>; Michael Kubacki
> <mikuback@linux.microsoft.com>; Sean Brogan <sean.brogan@microsoft.com>
> Subject: Re: [edk2-devel][PATCH v2 0/2] Add Volatile Keyword to NvmExpress
> Passthru CQs
> 
> For the series:
> Reviewed-by: Hao A Wu <hao.a.wu@intel.com>
> 
> Will wait a couple of days before merging to see if comments from other
> reviewers.
> 
> Best Regards,
> Hao Wu
> 
> > -----Original Message-----
> > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Oliver
> > Smith-Denny
> > Sent: Friday, April 28, 2023 1:36 AM
> > To: devel@edk2.groups.io
> > Cc: Wu, Hao A <hao.a.wu@intel.com>; Ni, Ray <ray.ni@intel.com>; Wang,
> > Jian J <jian.j.wang@intel.com>; Gao, Liming
> > <gaoliming@byosoft.com.cn>; Michael Kubacki
> > <mikuback@linux.microsoft.com>; Sean Brogan
> > <sean.brogan@microsoft.com>
> > Subject: [edk2-devel][PATCH v2 0/2] Add Volatile Keyword to NvmExpress
> > Passthru CQs
> >
> > NVMe CQs are hardware queues mapped to EFI memory.
> > In the NVMExpress Passthru implementations in PEI and DXE, it has been
> > observed that NVMe CQs are not marked volatile, meaning the compiler
> > has significant leeway to optimize accesses to these structures.
> >
> > This led to an issue where the passthru driver waited for a timeout
> > period for an NVMe CQ to mark that it was finished with a transaction,
> > but the compiler had optimized away the read to the actual HW mapped
> > memory, so the transaction had completed but the timeout continued.
> >
> > Marking the CQs as volatile fixes this issue as the reads happen to
> > the actual HW.
> >
> > Personal GitHub PR: https://github.com/tianocore/edk2/pull/4320
> > Github branch: https://github.com/os-d/edk2/tree/osde/volatile_cq_v2
> >
> > Changes v1 => v2:
> > =================
> > - Remove volatile keyword from SQ
> >
> > Cc: Hao A Wu <hao.a.wu@intel.com>
> > Cc: Ray Ni <ray.ni@intel.com>
> > Cc: Jian J Wang <jian.j.wang@intel.com>
> > Cc: Liming Gao <gaoliming@byosoft.com.cn>
> > Cc: Michael Kubacki <mikuback@linux.microsoft.com>
> > Cc: Sean Brogan <sean.brogan@microsoft.com>
> >
> >
> > Oliver Smith-Denny (2):
> >   Add the volatile keyword to NvmExpressDxe's Passthru CQ
> >   Add volatile keyword to NvmExpressPei's Passthru CQ
> >
> >  MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c    | 6 +++---
> >  MdeModulePkg/Bus/Pci/NvmExpressPei/NvmExpressPeiPassThru.c | 6 +++---
> >  2 files changed, 6 insertions(+), 6 deletions(-)
> >
> > --
> > 2.40.0
> >
> >
> >
> > -=-=-=-=-=-=
> > Groups.io Links: You receive all messages sent to this group.
> > View/Reply Online (#103725):
> > https://edk2.groups.io/g/devel/message/103725
> > Mute This Topic: https://groups.io/mt/98541927/1768737
> > Group Owner: devel+owner@edk2.groups.io
> > Unsubscribe: https://edk2.groups.io/g/devel/unsub [hao.a.wu@intel.com]
> > -=-=-=-=-=-=
> >
> 
> 
> 
> 
> 

Re: [edk2-devel][PATCH v2 0/2] Add Volatile Keyword to NvmExpress Passthru CQs

[Michael Kubacki]

For the series:

Acked-by: Michael Kubacki <michael.kubacki@microsoft.com>

On 4/27/2023 1:36 PM, Oliver Smith-Denny wrote:
> NVMe CQs are hardware queues mapped to EFI memory.
> In the NVMExpress Passthru implementations in PEI and DXE,
> it has been observed that NVMe CQs are not marked
> volatile, meaning the compiler has significant leeway
> to optimize accesses to these structures.
> 
> This led to an issue where the passthru driver waited
> for a timeout period for an NVMe CQ to mark that it was
> finished with a transaction, but the compiler had
> optimized away the read to the actual HW mapped memory,
> so the transaction had completed but the timeout continued.
> 
> Marking the CQs as volatile fixes this issue as the reads
> happen to the actual HW.
> 
> Personal GitHub PR: https://github.com/tianocore/edk2/pull/4320
> Github branch: https://github.com/os-d/edk2/tree/osde/volatile_cq_v2
> 
> Changes v1 => v2:
> =================
> - Remove volatile keyword from SQ
> 
> Cc: Hao A Wu <hao.a.wu@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Liming Gao <gaoliming@byosoft.com.cn>
> Cc: Michael Kubacki <mikuback@linux.microsoft.com>
> Cc: Sean Brogan <sean.brogan@microsoft.com>
> 
> 
> Oliver Smith-Denny (2):
>    Add the volatile keyword to NvmExpressDxe's Passthru CQ
>    Add volatile keyword to NvmExpressPei's Passthru CQ
> 
>   MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c    | 6 +++---
>   MdeModulePkg/Bus/Pci/NvmExpressPei/NvmExpressPeiPassThru.c | 6 +++---
>   2 files changed, 6 insertions(+), 6 deletions(-)
>