From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from linux.microsoft.com (linux.microsoft.com [13.77.154.182]) by mx.groups.io with SMTP id smtpd.web11.11962.1682005701615077971 for ; Thu, 20 Apr 2023 08:48:21 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@linux.microsoft.com header.s=default header.b=RV6hJBEH; spf=pass (domain: linux.microsoft.com, ip: 13.77.154.182, mailfrom: osde@linux.microsoft.com) Received: from [10.137.194.171] (unknown [131.107.1.171]) by linux.microsoft.com (Postfix) with ESMTPSA id 1F17621C2093; Thu, 20 Apr 2023 08:48:21 -0700 (PDT) DKIM-Filter: OpenDKIM Filter v2.11.0 linux.microsoft.com 1F17621C2093 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.microsoft.com; s=default; t=1682005701; bh=Q5S2NN1lWMd1yBV4CmdvkJhzuB2sNEBjLAPFJzCuiEc=; h=Date:Subject:To:Cc:References:From:In-Reply-To:From; b=RV6hJBEHycpVPusNrKwI5oIY6ZhaH3i3XG8Mq3ehKxHQH4IOteNlFLkzG58B/udNF F803MlTFfn4wvDYOmUS23hZem3r0bHqTbFTTafyUXeksgG+YmNthFN7MPBgZmSuyXF bCGEML0ET/32wP5Vowli/JXASfUhuSzx6M8bP8N4= Message-ID: Date: Thu, 20 Apr 2023 08:48:20 -0700 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101 Thunderbird/102.10.0 Subject: Re: [edk2-devel] [PATCH v1 1/2] Add the volatile keyword to NvmExpressDxe's Passthru CQs and SQs. To: devel@edk2.groups.io, ray.ni@intel.com Cc: "Wu, Hao A" , "Wang, Jian J" , "Gao, Liming" , Michael Kubacki , Sean Brogan References: <20230419234108.10243-1-osde@linux.microsoft.com> <20230419234108.10243-2-osde@linux.microsoft.com> From: "Oliver Smith-Denny" In-Reply-To: Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit 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 On Behalf Of Oliver >> Smith-Denny >> Sent: Thursday, April 20, 2023 7:41 AM >> To: devel@edk2.groups.io >> Cc: Wu, Hao A ; Ni, Ray ; Wang, >> Jian J ; Gao, Liming ; >> Michael Kubacki ; Sean Brogan >> >> 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 >> >> Cc: Ray Ni >> >> Cc: Jian J Wang >> >> Cc: Liming Gao >> >> Cc: Michael Kubacki >> >> Cc: Sean Brogan >> >> Signed-off-by: Oliver Smith-Denny >> >> --- >> >> 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] >> -=-=-=-=-=-= >> > > > > >