[edk2-devel][PATCH v1 0/2] MdeModulePkg: Add Volatile Keyword to NvmExpress Passthru CQs and SQs

[edk2-devel][PATCH v1 0/2] MdeModulePkg: Add Volatile Keyword to NvmExpress Passthru CQs and SQs

[Oliver Smith-Denny]

NVMe CQs and SQs are hardware queues mapped to EFI memory.
In the NVMExpress Passthru implementations in PEI and DXE,
it has been observed that NVMe CQs and SQs 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. There may be similiar issues with
NVMe SQs, so this patch also marks them as volatile to
reflect they are HW resources.

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

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 CQs and SQs.
  Add volatile keyword to NvmExpressPei's Passthru CQs and SQs.

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

-- 
2.39.2

[edk2-devel][PATCH v1 1/2] Add the volatile keyword to NvmExpressDxe's Passthru CQs and SQs.

[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 and SQ,
now that they are 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 | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c b/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c
index f37baa626a16..1a7e39500ac0 100644
--- a/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c
+++ b/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c
@@ -459,8 +459,8 @@ NvmExpressPassThru (
   EFI_STATUS                     Status;
   EFI_STATUS                     PreviousStatus;
   EFI_PCI_IO_PROTOCOL            *PciIo;
-  NVME_SQ                        *Sq;
-  NVME_CQ                        *Cq;
+  volatile NVME_SQ               *Sq;
+  volatile NVME_CQ               *Cq;
   UINT16                         QueueId;
   UINT16                         QueueSize;
   UINT32                         Bytes;
@@ -581,7 +581,7 @@ NvmExpressPassThru (
     return EFI_INVALID_PARAMETER;
   }
 
-  ZeroMem (Sq, sizeof (NVME_SQ));
+  ZeroMem ((VOID *)Sq, sizeof (NVME_SQ));
   Sq->Opc  = (UINT8)Packet->NvmeCmd->Cdw0.Opcode;
   Sq->Fuse = (UINT8)Packet->NvmeCmd->Cdw0.FusedOperation;
   Sq->Cid  = Private->Cid[QueueId]++;
@@ -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.39.2

Re: [edk2-devel][PATCH v1 1/2] Add the volatile keyword to NvmExpressDxe's Passthru CQs and SQs.

[Ni, Ray]

If it's to copy from hw to sw memory, why do we need volatile?

Thanks,
Ray

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Oliver
> Smith-Denny
> Sent: Thursday, April 20, 2023 7:41 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 v1 1/2] Add the volatile keyword to
> NvmExpressDxe's Passthru CQs and SQs.
> 
> This updates the relevant functions that expect a non-volatile
> 
> structure to be passed to them to take casts of the CQ and SQ,
> 
> now that they are 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 | 10
> +++++-----
> 
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> 
> 
> diff --git a/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c
> b/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c
> 
> index f37baa626a16..1a7e39500ac0 100644
> 
> --- a/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c
> 
> +++ b/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c
> 
> @@ -459,8 +459,8 @@ NvmExpressPassThru (
> 
>    EFI_STATUS                     Status;
> 
>    EFI_STATUS                     PreviousStatus;
> 
>    EFI_PCI_IO_PROTOCOL            *PciIo;
> 
> -  NVME_SQ                        *Sq;
> 
> -  NVME_CQ                        *Cq;
> 
> +  volatile NVME_SQ               *Sq;
> 
> +  volatile NVME_CQ               *Cq;
> 
>    UINT16                         QueueId;
> 
>    UINT16                         QueueSize;
> 
>    UINT32                         Bytes;
> 
> @@ -581,7 +581,7 @@ NvmExpressPassThru (
> 
>      return EFI_INVALID_PARAMETER;
> 
>    }
> 
> 
> 
> -  ZeroMem (Sq, sizeof (NVME_SQ));
> 
> +  ZeroMem ((VOID *)Sq, sizeof (NVME_SQ));
> 
>    Sq->Opc  = (UINT8)Packet->NvmeCmd->Cdw0.Opcode;
> 
>    Sq->Fuse = (UINT8)Packet->NvmeCmd->Cdw0.FusedOperation;
> 
>    Sq->Cid  = Private->Cid[QueueId]++;
> 
> @@ -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.39.2
> 
> 
> 
> 
> 
> -=-=-=-=-=-=
> Groups.io Links: You receive all messages sent to this group.
> View/Reply Online (#103263):
> https://edk2.groups.io/g/devel/message/103263
> Mute This Topic: https://groups.io/mt/98378948/1712937
> Group Owner: devel+owner@edk2.groups.io
> Unsubscribe: https://edk2.groups.io/g/devel/unsub [ray.ni@intel.com]
> -=-=-=-=-=-=
> 

Re: [edk2-devel] [PATCH v1 1/2] Add the volatile keyword to NvmExpressDxe's Passthru CQs and SQs.

[Oliver Smith-Denny]

Hi Ray,

This is not a pure copy from HW to SW memory, we are also
polling the CQ to see if a transaction has completed:

   //
   // Wait for completion queue to get filled in.
   //
   Status = EFI_TIMEOUT;
   while (EFI_ERROR (gBS->CheckEvent (TimerEvent))) {
     if (Cq->Pt != Private->Pt[QueueId]) {
       Status = EFI_SUCCESS;
       break;
     }
   }


What we have seen happen is that without the volatile
keyword, the compiler can move the Cq->Pt read outside
of the loop and only do register compares inside the loop,
i.e. we end up going the full timeout even if the CQ reports
it is finished.

Here is the issue that was filed on the project Mu side:
https://github.com/microsoft/mu_basecore/issues/324.

Thanks,
Oliver

On 4/19/2023 5:48 PM, Ni, Ray wrote:
> If it's to copy from hw to sw memory, why do we need volatile?
> 
> Thanks,
> Ray
> 
>> -----Original Message-----
>> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Oliver
>> Smith-Denny
>> Sent: Thursday, April 20, 2023 7:41 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 v1 1/2] Add the volatile keyword to
>> NvmExpressDxe's Passthru CQs and SQs.
>>
>> This updates the relevant functions that expect a non-volatile
>>
>> structure to be passed to them to take casts of the CQ and SQ,
>>
>> now that they are 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 | 10
>> +++++-----
>>
>>   1 file changed, 5 insertions(+), 5 deletions(-)
>>
>>
>>
>> diff --git a/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c
>> b/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c
>>
>> index f37baa626a16..1a7e39500ac0 100644
>>
>> --- a/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c
>>
>> +++ b/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c
>>
>> @@ -459,8 +459,8 @@ NvmExpressPassThru (
>>
>>     EFI_STATUS                     Status;
>>
>>     EFI_STATUS                     PreviousStatus;
>>
>>     EFI_PCI_IO_PROTOCOL            *PciIo;
>>
>> -  NVME_SQ                        *Sq;
>>
>> -  NVME_CQ                        *Cq;
>>
>> +  volatile NVME_SQ               *Sq;
>>
>> +  volatile NVME_CQ               *Cq;
>>
>>     UINT16                         QueueId;
>>
>>     UINT16                         QueueSize;
>>
>>     UINT32                         Bytes;
>>
>> @@ -581,7 +581,7 @@ NvmExpressPassThru (
>>
>>       return EFI_INVALID_PARAMETER;
>>
>>     }
>>
>>
>>
>> -  ZeroMem (Sq, sizeof (NVME_SQ));
>>
>> +  ZeroMem ((VOID *)Sq, sizeof (NVME_SQ));
>>
>>     Sq->Opc  = (UINT8)Packet->NvmeCmd->Cdw0.Opcode;
>>
>>     Sq->Fuse = (UINT8)Packet->NvmeCmd->Cdw0.FusedOperation;
>>
>>     Sq->Cid  = Private->Cid[QueueId]++;
>>
>> @@ -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.39.2
>>
>>
>>
>>
>>
>> -=-=-=-=-=-=
>> Groups.io Links: You receive all messages sent to this group.
>> View/Reply Online (#103263):
>> https://edk2.groups.io/g/devel/message/103263
>> Mute This Topic: https://groups.io/mt/98378948/1712937
>> Group Owner: devel+owner@edk2.groups.io
>> Unsubscribe: https://edk2.groups.io/g/devel/unsub [ray.ni@intel.com]
>> -=-=-=-=-=-=
>>
> 
> 
> 
> 
> 

Re: [edk2-devel] [PATCH v1 1/2] Add the volatile keyword to NvmExpressDxe's Passthru CQs and SQs.

[Wu, Hao A]

Thanks Oliver,

For the Submission Queue pointer "Sq", I think it is being used to format the command that will be sent to the NVME controller.
NvmExpressPassThru() does not read back its content for checking after the command gets submitted.
My opinion is that it might be not necessary to add volatile attribute for it.

For the Completion Queue pointer "Cq", I am not sure which of the following is better:
a) Introduce a volatile pointer to "Cq->Pt", or
b) Mark "Cq" as volatile
Would like to get your feedback on this. Thanks. 

Best Regards,
Hao Wu

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Oliver
> Smith-Denny
> Sent: Thursday, April 20, 2023 11:48 PM
> To: devel@edk2.groups.io; Ni, Ray <ray.ni@intel.com>
> Cc: Wu, Hao A <hao.a.wu@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 v1 1/2] Add the volatile keyword to
> NvmExpressDxe's Passthru CQs and SQs.
> 
> Hi Ray,
> 
> This is not a pure copy from HW to SW memory, we are also polling the CQ to
> see if a transaction has completed:
> 
>    //
>    // Wait for completion queue to get filled in.
>    //
>    Status = EFI_TIMEOUT;
>    while (EFI_ERROR (gBS->CheckEvent (TimerEvent))) {
>      if (Cq->Pt != Private->Pt[QueueId]) {
>        Status = EFI_SUCCESS;
>        break;
>      }
>    }
> 
> 
> What we have seen happen is that without the volatile keyword, the compiler
> can move the Cq->Pt read outside of the loop and only do register compares
> inside the loop, i.e. we end up going the full timeout even if the CQ reports it is
> finished.
> 
> Here is the issue that was filed on the project Mu side:
> https://github.com/microsoft/mu_basecore/issues/324.
> 
> Thanks,
> Oliver
> 
> On 4/19/2023 5:48 PM, Ni, Ray wrote:
> > If it's to copy from hw to sw memory, why do we need volatile?
> >
> > Thanks,
> > Ray
> >
> >> -----Original Message-----
> >> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Oliver
> >> Smith-Denny
> >> Sent: Thursday, April 20, 2023 7:41 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 v1 1/2] Add the volatile keyword to
> >> NvmExpressDxe's Passthru CQs and SQs.
> >>
> >> This updates the relevant functions that expect a non-volatile
> >>
> >> structure to be passed to them to take casts of the CQ and SQ,
> >>
> >> now that they are 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 | 10
> >> +++++-----
> >>
> >>   1 file changed, 5 insertions(+), 5 deletions(-)
> >>
> >>
> >>
> >> diff --git a/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c
> >> b/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c
> >>
> >> index f37baa626a16..1a7e39500ac0 100644
> >>
> >> --- a/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c
> >>
> >> +++ b/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c
> >>
> >> @@ -459,8 +459,8 @@ NvmExpressPassThru (
> >>
> >>     EFI_STATUS                     Status;
> >>
> >>     EFI_STATUS                     PreviousStatus;
> >>
> >>     EFI_PCI_IO_PROTOCOL            *PciIo;
> >>
> >> -  NVME_SQ                        *Sq;
> >>
> >> -  NVME_CQ                        *Cq;
> >>
> >> +  volatile NVME_SQ               *Sq;
> >>
> >> +  volatile NVME_CQ               *Cq;
> >>
> >>     UINT16                         QueueId;
> >>
> >>     UINT16                         QueueSize;
> >>
> >>     UINT32                         Bytes;
> >>
> >> @@ -581,7 +581,7 @@ NvmExpressPassThru (
> >>
> >>       return EFI_INVALID_PARAMETER;
> >>
> >>     }
> >>
> >>
> >>
> >> -  ZeroMem (Sq, sizeof (NVME_SQ));
> >>
> >> +  ZeroMem ((VOID *)Sq, sizeof (NVME_SQ));
> >>
> >>     Sq->Opc  = (UINT8)Packet->NvmeCmd->Cdw0.Opcode;
> >>
> >>     Sq->Fuse = (UINT8)Packet->NvmeCmd->Cdw0.FusedOperation;
> >>
> >>     Sq->Cid  = Private->Cid[QueueId]++;
> >>
> >> @@ -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.39.2
> >>
> >>
> >>
> >>
> >>
> >> -=-=-=-=-=-=
> >> Groups.io Links: You receive all messages sent to this group.
> >> View/Reply Online (#103263):
> >> https://edk2.groups.io/g/devel/message/103263
> >> Mute This Topic: https://groups.io/mt/98378948/1712937
> >> Group Owner: devel+owner@edk2.groups.io
> >> Unsubscribe: https://edk2.groups.io/g/devel/unsub [ray.ni@intel.com]
> >> -=-=-=-=-=-=
> >>
> >
> >
> >
> >
> >
> 
> 
> 
> 

Re: [edk2-devel] [PATCH v1 1/2] Add the volatile keyword to NvmExpressDxe's Passthru CQs and SQs.

[Oliver Smith-Denny]

Hi Hao,

Thanks for the review! For the Sq, I agree, currently some metadata is
read from the queue, but it is not fields that are going to change
(such as SGL usage). The thought process there was in case we interact
with the HW queue differently in the future. I will drop the Sq change
in v2 of this patch.

For the Cq, I think the safer option is to mark the whole structure
as volatile, because there are other bits that we read out of there
that the HW updates, for example the status code here:


   //
   // Check the NVMe cmd execution result
   //
   if (Status != EFI_TIMEOUT) {
     if ((Cq->Sct == 0) && (Cq->Sc == 0)) {
       Status = EFI_SUCCESS;


Without the structure marked as volatile, I believe the compiler
could optimize the code such that it only reads these metadata
fields at the beginning of this function, potentially before they
are set by the HW.

I do not believe there is much of a performance downside to marking
the structure vs individual fields. I am curious to get your feedback
here, as well. My goal would be to have a robust solution here so
we don't play whack a mole as different compilers make different
choices, but obviously without too much overhead :)

Thanks,
Oliver


On 4/25/2023 11:32 PM, Wu, Hao A wrote:
> Thanks Oliver,
> 
> For the Submission Queue pointer "Sq", I think it is being used to format the command that will be sent to the NVME controller.
> NvmExpressPassThru() does not read back its content for checking after the command gets submitted.
> My opinion is that it might be not necessary to add volatile attribute for it.
> 
> For the Completion Queue pointer "Cq", I am not sure which of the following is better:
> a) Introduce a volatile pointer to "Cq->Pt", or
> b) Mark "Cq" as volatile
> Would like to get your feedback on this. Thanks.
> 
> Best Regards,
> Hao Wu
> 
>> -----Original Message-----
>> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Oliver
>> Smith-Denny
>> Sent: Thursday, April 20, 2023 11:48 PM
>> To: devel@edk2.groups.io; Ni, Ray <ray.ni@intel.com>
>> Cc: Wu, Hao A <hao.a.wu@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 v1 1/2] Add the volatile keyword to
>> NvmExpressDxe's Passthru CQs and SQs.
>>
>> Hi Ray,
>>
>> This is not a pure copy from HW to SW memory, we are also polling the CQ to
>> see if a transaction has completed:
>>
>>     //
>>     // Wait for completion queue to get filled in.
>>     //
>>     Status = EFI_TIMEOUT;
>>     while (EFI_ERROR (gBS->CheckEvent (TimerEvent))) {
>>       if (Cq->Pt != Private->Pt[QueueId]) {
>>         Status = EFI_SUCCESS;
>>         break;
>>       }
>>     }
>>
>>
>> What we have seen happen is that without the volatile keyword, the compiler
>> can move the Cq->Pt read outside of the loop and only do register compares
>> inside the loop, i.e. we end up going the full timeout even if the CQ reports it is
>> finished.
>>
>> Here is the issue that was filed on the project Mu side:
>> https://github.com/microsoft/mu_basecore/issues/324.
>>
>> Thanks,
>> Oliver
>>
>> On 4/19/2023 5:48 PM, Ni, Ray wrote:
>>> If it's to copy from hw to sw memory, why do we need volatile?
>>>
>>> Thanks,
>>> Ray
>>>
>>>> -----Original Message-----
>>>> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Oliver
>>>> Smith-Denny
>>>> Sent: Thursday, April 20, 2023 7:41 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 v1 1/2] Add the volatile keyword to
>>>> NvmExpressDxe's Passthru CQs and SQs.
>>>>
>>>> This updates the relevant functions that expect a non-volatile
>>>>
>>>> structure to be passed to them to take casts of the CQ and SQ,
>>>>
>>>> now that they are 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 | 10
>>>> +++++-----
>>>>
>>>>    1 file changed, 5 insertions(+), 5 deletions(-)
>>>>
>>>>
>>>>
>>>> diff --git a/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c
>>>> b/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c
>>>>
>>>> index f37baa626a16..1a7e39500ac0 100644
>>>>
>>>> --- a/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c
>>>>
>>>> +++ b/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c
>>>>
>>>> @@ -459,8 +459,8 @@ NvmExpressPassThru (
>>>>
>>>>      EFI_STATUS                     Status;
>>>>
>>>>      EFI_STATUS                     PreviousStatus;
>>>>
>>>>      EFI_PCI_IO_PROTOCOL            *PciIo;
>>>>
>>>> -  NVME_SQ                        *Sq;
>>>>
>>>> -  NVME_CQ                        *Cq;
>>>>
>>>> +  volatile NVME_SQ               *Sq;
>>>>
>>>> +  volatile NVME_CQ               *Cq;
>>>>
>>>>      UINT16                         QueueId;
>>>>
>>>>      UINT16                         QueueSize;
>>>>
>>>>      UINT32                         Bytes;
>>>>
>>>> @@ -581,7 +581,7 @@ NvmExpressPassThru (
>>>>
>>>>        return EFI_INVALID_PARAMETER;
>>>>
>>>>      }
>>>>
>>>>
>>>>
>>>> -  ZeroMem (Sq, sizeof (NVME_SQ));
>>>>
>>>> +  ZeroMem ((VOID *)Sq, sizeof (NVME_SQ));
>>>>
>>>>      Sq->Opc  = (UINT8)Packet->NvmeCmd->Cdw0.Opcode;
>>>>
>>>>      Sq->Fuse = (UINT8)Packet->NvmeCmd->Cdw0.FusedOperation;
>>>>
>>>>      Sq->Cid  = Private->Cid[QueueId]++;
>>>>
>>>> @@ -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.39.2
>>>>
>>>>
>>>>
>>>>
>>>>
>>>> -=-=-=-=-=-=
>>>> Groups.io Links: You receive all messages sent to this group.
>>>> View/Reply Online (#103263):
>>>> https://edk2.groups.io/g/devel/message/103263
>>>> Mute This Topic: https://groups.io/mt/98378948/1712937
>>>> Group Owner: devel+owner@edk2.groups.io
>>>> Unsubscribe: https://edk2.groups.io/g/devel/unsub [ray.ni@intel.com]
>>>> -=-=-=-=-=-=
>>>>
>>>
>>>
>>>
>>>
>>>
>>
>>
>>
>>
> 
> 
> 
> 
> 
> 

Re: [edk2-devel] [PATCH v1 1/2] Add the volatile keyword to NvmExpressDxe's Passthru CQs and SQs.

[Wu, Hao A]

Thanks Oliver,

Agree with your points. Let's keep "Sq" as is and mark "Cq" as volatile.

Best Regards,
Hao Wu

> -----Original Message-----
> From: Oliver Smith-Denny <osde@linux.microsoft.com>
> Sent: Thursday, April 27, 2023 5:07 AM
> To: devel@edk2.groups.io; Wu, Hao A <hao.a.wu@intel.com>; Ni, Ray
> <ray.ni@intel.com>
> Cc: 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 v1 1/2] Add the volatile keyword to
> NvmExpressDxe's Passthru CQs and SQs.
> 
> Hi Hao,
> 
> Thanks for the review! For the Sq, I agree, currently some metadata is read
> from the queue, but it is not fields that are going to change (such as SGL usage).
> The thought process there was in case we interact with the HW queue
> differently in the future. I will drop the Sq change in v2 of this patch.
> 
> For the Cq, I think the safer option is to mark the whole structure as volatile,
> because there are other bits that we read out of there that the HW updates, for
> example the status code here:
> 
> 
>    //
>    // Check the NVMe cmd execution result
>    //
>    if (Status != EFI_TIMEOUT) {
>      if ((Cq->Sct == 0) && (Cq->Sc == 0)) {
>        Status = EFI_SUCCESS;
> 
> 
> Without the structure marked as volatile, I believe the compiler could optimize
> the code such that it only reads these metadata fields at the beginning of this
> function, potentially before they are set by the HW.
> 
> I do not believe there is much of a performance downside to marking the
> structure vs individual fields. I am curious to get your feedback here, as well.
> My goal would be to have a robust solution here so we don't play whack a mole
> as different compilers make different choices, but obviously without too much
> overhead :)
> 
> Thanks,
> Oliver
> 
> 
> On 4/25/2023 11:32 PM, Wu, Hao A wrote:
> > Thanks Oliver,
> >
> > For the Submission Queue pointer "Sq", I think it is being used to format the
> command that will be sent to the NVME controller.
> > NvmExpressPassThru() does not read back its content for checking after the
> command gets submitted.
> > My opinion is that it might be not necessary to add volatile attribute for it.
> >
> > For the Completion Queue pointer "Cq", I am not sure which of the following
> is better:
> > a) Introduce a volatile pointer to "Cq->Pt", or
> > b) Mark "Cq" as volatile
> > Would like to get your feedback on this. Thanks.
> >
> > Best Regards,
> > Hao Wu
> >
> >> -----Original Message-----
> >> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Oliver
> >> Smith-Denny
> >> Sent: Thursday, April 20, 2023 11:48 PM
> >> To: devel@edk2.groups.io; Ni, Ray <ray.ni@intel.com>
> >> Cc: Wu, Hao A <hao.a.wu@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 v1 1/2] Add the volatile keyword to
> >> NvmExpressDxe's Passthru CQs and SQs.
> >>
> >> Hi Ray,
> >>
> >> This is not a pure copy from HW to SW memory, we are also polling the
> >> CQ to see if a transaction has completed:
> >>
> >>     //
> >>     // Wait for completion queue to get filled in.
> >>     //
> >>     Status = EFI_TIMEOUT;
> >>     while (EFI_ERROR (gBS->CheckEvent (TimerEvent))) {
> >>       if (Cq->Pt != Private->Pt[QueueId]) {
> >>         Status = EFI_SUCCESS;
> >>         break;
> >>       }
> >>     }
> >>
> >>
> >> What we have seen happen is that without the volatile keyword, the
> >> compiler can move the Cq->Pt read outside of the loop and only do
> >> register compares inside the loop, i.e. we end up going the full
> >> timeout even if the CQ reports it is finished.
> >>
> >> Here is the issue that was filed on the project Mu side:
> >> https://github.com/microsoft/mu_basecore/issues/324.
> >>
> >> Thanks,
> >> Oliver
> >>
> >> On 4/19/2023 5:48 PM, Ni, Ray wrote:
> >>> If it's to copy from hw to sw memory, why do we need volatile?
> >>>
> >>> Thanks,
> >>> Ray
> >>>
> >>>> -----Original Message-----
> >>>> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
> >>>> Oliver Smith-Denny
> >>>> Sent: Thursday, April 20, 2023 7:41 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 v1 1/2] Add the volatile keyword to
> >>>> NvmExpressDxe's Passthru CQs and SQs.
> >>>>
> >>>> This updates the relevant functions that expect a non-volatile
> >>>>
> >>>> structure to be passed to them to take casts of the CQ and SQ,
> >>>>
> >>>> now that they are 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 | 10
> >>>> +++++-----
> >>>>
> >>>>    1 file changed, 5 insertions(+), 5 deletions(-)
> >>>>
> >>>>
> >>>>
> >>>> diff --git
> >>>> a/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c
> >>>> b/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c
> >>>>
> >>>> index f37baa626a16..1a7e39500ac0 100644
> >>>>
> >>>> --- a/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c
> >>>>
> >>>> +++ b/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c
> >>>>
> >>>> @@ -459,8 +459,8 @@ NvmExpressPassThru (
> >>>>
> >>>>      EFI_STATUS                     Status;
> >>>>
> >>>>      EFI_STATUS                     PreviousStatus;
> >>>>
> >>>>      EFI_PCI_IO_PROTOCOL            *PciIo;
> >>>>
> >>>> -  NVME_SQ                        *Sq;
> >>>>
> >>>> -  NVME_CQ                        *Cq;
> >>>>
> >>>> +  volatile NVME_SQ               *Sq;
> >>>>
> >>>> +  volatile NVME_CQ               *Cq;
> >>>>
> >>>>      UINT16                         QueueId;
> >>>>
> >>>>      UINT16                         QueueSize;
> >>>>
> >>>>      UINT32                         Bytes;
> >>>>
> >>>> @@ -581,7 +581,7 @@ NvmExpressPassThru (
> >>>>
> >>>>        return EFI_INVALID_PARAMETER;
> >>>>
> >>>>      }
> >>>>
> >>>>
> >>>>
> >>>> -  ZeroMem (Sq, sizeof (NVME_SQ));
> >>>>
> >>>> +  ZeroMem ((VOID *)Sq, sizeof (NVME_SQ));
> >>>>
> >>>>      Sq->Opc  = (UINT8)Packet->NvmeCmd->Cdw0.Opcode;
> >>>>
> >>>>      Sq->Fuse = (UINT8)Packet->NvmeCmd->Cdw0.FusedOperation;
> >>>>
> >>>>      Sq->Cid  = Private->Cid[QueueId]++;
> >>>>
> >>>> @@ -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.39.2
> >>>>
> >>>>
> >>>>
> >>>>
> >>>>
> >>>> -=-=-=-=-=-=
> >>>> Groups.io Links: You receive all messages sent to this group.
> >>>> View/Reply Online (#103263):
> >>>> https://edk2.groups.io/g/devel/message/103263
> >>>> Mute This Topic: https://groups.io/mt/98378948/1712937
> >>>> Group Owner: devel+owner@edk2.groups.io
> >>>> Unsubscribe: https://edk2.groups.io/g/devel/unsub
> >>>> [ray.ni@intel.com] -=-=-=-=-=-=
> >>>>
> >>>
> >>>
> >>>
> >>>
> >>>
> >>
> >>
> >>
> >>
> >
> >
> >
> > 
> >
> >

[edk2-devel][PATCH v1 2/2] Add volatile keyword to NvmExpressPei's Passthru CQs and SQs.

[Oliver Smith-Denny]

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

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 | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/MdeModulePkg/Bus/Pci/NvmExpressPei/NvmExpressPeiPassThru.c b/MdeModulePkg/Bus/Pci/NvmExpressPei/NvmExpressPeiPassThru.c
index 5081b53cd5e8..70b0b9d5965c 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)) {
@@ -343,8 +343,8 @@ NvmePassThruExecute (
   )
 {
   EFI_STATUS             Status;
-  NVME_SQ                *Sq;
-  NVME_CQ                *Cq;
+  volatile NVME_SQ       *Sq;
+  volatile NVME_CQ       *Cq;
   UINT8                  QueueId;
   UINTN                  SqSize;
   UINTN                  CqSize;
@@ -414,7 +414,7 @@ NvmePassThruExecute (
     return EFI_INVALID_PARAMETER;
   }
 
-  ZeroMem (Sq, sizeof (NVME_SQ));
+  ZeroMem ((VOID *)Sq, sizeof (NVME_SQ));
   Sq->Opc  = (UINT8)Packet->NvmeCmd->Cdw0.Opcode;
   Sq->Fuse = (UINT8)Packet->NvmeCmd->Cdw0.FusedOperation;
   Sq->Cid  = Private->Cid[QueueId]++;
@@ -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.39.2