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 <o...@linux.microsoft.com> > Sent: Thursday, April 27, 2023 5:07 AM > To: devel@edk2.groups.io; Wu, Hao A <hao.a...@intel.com>; Ni, Ray > <ray...@intel.com> > Cc: Wang, Jian J <jian.j.w...@intel.com>; Gao, Liming > <gaolim...@byosoft.com.cn>; Michael Kubacki > <mikub...@linux.microsoft.com>; Sean Brogan <sean.bro...@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...@intel.com> > >> Cc: Wu, Hao A <hao.a...@intel.com>; Wang, Jian J > >> <jian.j.w...@intel.com>; Gao, Liming <gaolim...@byosoft.com.cn>; > >> Michael Kubacki <mikub...@linux.microsoft.com>; Sean Brogan > >> <sean.bro...@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...@intel.com>; Ni, Ray <ray...@intel.com>; > >>>> Wang, Jian J <jian.j.w...@intel.com>; Gao, Liming > >>>> <gaolim...@byosoft.com.cn>; Michael Kubacki > >>>> <mikub...@linux.microsoft.com>; Sean Brogan > >>>> <sean.bro...@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...@intel.com> > >>>> > >>>> Cc: Ray Ni <ray...@intel.com> > >>>> > >>>> Cc: Jian J Wang <jian.j.w...@intel.com> > >>>> > >>>> Cc: Liming Gao <gaolim...@byosoft.com.cn> > >>>> > >>>> Cc: Michael Kubacki <mikub...@linux.microsoft.com> > >>>> > >>>> Cc: Sean Brogan <sean.bro...@microsoft.com> > >>>> > >>>> Signed-off-by: Oliver Smith-Denny <o...@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+ow...@edk2.groups.io > >>>> Unsubscribe: https://edk2.groups.io/g/devel/unsub > >>>> [ray...@intel.com] -=-=-=-=-=-= > >>>> > >>> > >>> > >>> > >>> > >>> > >> > >> > >> > >> > > > > > > > > > > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#103679): https://edk2.groups.io/g/devel/message/103679 Mute This Topic: https://groups.io/mt/98378948/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-