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: [email protected] <[email protected]> On Behalf Of Oliver > Smith-Denny > Sent: Thursday, April 20, 2023 11:48 PM > To: [email protected]; Ni, Ray <[email protected]> > Cc: Wu, Hao A <[email protected]>; Wang, Jian J <[email protected]>; > Gao, Liming <[email protected]>; Michael Kubacki > <[email protected]>; Sean Brogan <[email protected]> > 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: [email protected] <[email protected]> On Behalf Of Oliver > >> Smith-Denny > >> Sent: Thursday, April 20, 2023 7:41 AM > >> To: [email protected] > >> Cc: Wu, Hao A <[email protected]>; Ni, Ray <[email protected]>; Wang, > >> Jian J <[email protected]>; Gao, Liming > >> <[email protected]>; Michael Kubacki > >> <[email protected]>; Sean Brogan > >> <[email protected]> > >> 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 <[email protected]> > >> > >> Cc: Ray Ni <[email protected]> > >> > >> Cc: Jian J Wang <[email protected]> > >> > >> Cc: Liming Gao <[email protected]> > >> > >> Cc: Michael Kubacki <[email protected]> > >> > >> Cc: Sean Brogan <[email protected]> > >> > >> Signed-off-by: Oliver Smith-Denny <[email protected]> > >> > >> --- > >> > >> 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: [email protected] > >> Unsubscribe: https://edk2.groups.io/g/devel/unsub [[email protected]] > >> -=-=-=-=-=-= > >> > > > > > > > > > > > > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#103617): https://edk2.groups.io/g/devel/message/103617 Mute This Topic: https://groups.io/mt/98378948/21656 Group Owner: [email protected] Unsubscribe: https://edk2.groups.io/g/devel/unsub [[email protected]] -=-=-=-=-=-=-=-=-=-=-=-
