From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=192.55.52.43; helo=mga05.intel.com; envelope-from=hao.a.wu@intel.com; receiver=edk2-devel@lists.01.org Received: from mga05.intel.com (mga05.intel.com [192.55.52.43]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 58DD02194D3B8 for ; Wed, 30 Jan 2019 21:45:50 -0800 (PST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga007.jf.intel.com ([10.7.209.58]) by fmsmga105.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 30 Jan 2019 21:45:49 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.56,543,1539673200"; d="scan'208";a="111240443" Received: from fmsmsx104.amr.corp.intel.com ([10.18.124.202]) by orsmga007.jf.intel.com with ESMTP; 30 Jan 2019 21:45:49 -0800 Received: from fmsmsx117.amr.corp.intel.com (10.18.116.17) by fmsmsx104.amr.corp.intel.com (10.18.124.202) with Microsoft SMTP Server (TLS) id 14.3.408.0; Wed, 30 Jan 2019 21:45:48 -0800 Received: from shsmsx106.ccr.corp.intel.com (10.239.4.159) by fmsmsx117.amr.corp.intel.com (10.18.116.17) with Microsoft SMTP Server (TLS) id 14.3.408.0; Wed, 30 Jan 2019 21:45:48 -0800 Received: from shsmsx104.ccr.corp.intel.com ([169.254.5.102]) by SHSMSX106.ccr.corp.intel.com ([169.254.10.174]) with mapi id 14.03.0415.000; Thu, 31 Jan 2019 13:45:46 +0800 From: "Wu, Hao A" To: "Ni, Ray" , "edk2-devel@lists.01.org" CC: "Wang, Jian J" , "Dong, Eric" Thread-Topic: [PATCH v2 07/12] MdeModulePkg/NvmExpressPei: Consume S3StorageDeviceInitList LockBox Thread-Index: AQHUuQ+Qd0N2YIoLD0yu+qqJFnxStaXINo8AgACmatA= Date: Thu, 31 Jan 2019 05:45:45 +0000 Message-ID: References: <20190131024854.4880-1-hao.a.wu@intel.com> <20190131024854.4880-8-hao.a.wu@intel.com> <734D49CCEBEEF84792F5B80ED585239D5C000040@SHSMSX104.ccr.corp.intel.com> In-Reply-To: <734D49CCEBEEF84792F5B80ED585239D5C000040@SHSMSX104.ccr.corp.intel.com> Accept-Language: zh-CN, en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] MIME-Version: 1.0 Subject: Re: [PATCH v2 07/12] MdeModulePkg/NvmExpressPei: Consume S3StorageDeviceInitList LockBox X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 31 Jan 2019 05:45:50 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable > -----Original Message----- > From: Ni, Ray > Sent: Thursday, January 31, 2019 11:45 AM > To: Wu, Hao A; edk2-devel@lists.01.org > Cc: Wang, Jian J; Dong, Eric > Subject: RE: [PATCH v2 07/12] MdeModulePkg/NvmExpressPei: Consume > S3StorageDeviceInitList LockBox >=20 >=20 >=20 > > -----Original Message----- > > From: Wu, Hao A > > Sent: Thursday, January 31, 2019 10:49 AM > > To: edk2-devel@lists.01.org > > Cc: Wu, Hao A ; Wang, Jian J ; > > Ni, Ray ; Dong, Eric > > Subject: [PATCH v2 07/12] MdeModulePkg/NvmExpressPei: Consume > > S3StorageDeviceInitList LockBox > > > > REF:https://bugzilla.tianocore.org/show_bug.cgi?id=3D1409 > > > > For the NvmExpressPei driver, this commit will update the driver to con= sume > > the S3StorageDeviceInitList LockBox in S3 phase. The purpose is to perf= orm > > an on-demand (partial) NVM Express device enumeration/initialization to > > benefit the S3 resume performance. > > > > Cc: Jian J Wang > > Cc: Ray Ni > > Cc: Eric Dong > > Contributed-under: TianoCore Contribution Agreement 1.1 > > Signed-off-by: Hao Wu > > --- > > MdeModulePkg/Bus/Pci/NvmExpressPei/NvmExpressPei.inf | 8 +- > > MdeModulePkg/Bus/Pci/NvmExpressPei/NvmExpressPei.h | 16 +++ > > MdeModulePkg/Bus/Pci/NvmExpressPei/NvmExpressPei.c | 20 ++++ > > MdeModulePkg/Bus/Pci/NvmExpressPei/NvmExpressPeiS3.c | 106 > > ++++++++++++++++++++ > > 4 files changed, 149 insertions(+), 1 deletion(-) > > > > diff --git a/MdeModulePkg/Bus/Pci/NvmExpressPei/NvmExpressPei.inf > > b/MdeModulePkg/Bus/Pci/NvmExpressPei/NvmExpressPei.inf > > index 0666e5892b..22b703e971 100644 > > --- a/MdeModulePkg/Bus/Pci/NvmExpressPei/NvmExpressPei.inf > > +++ b/MdeModulePkg/Bus/Pci/NvmExpressPei/NvmExpressPei.inf > > @@ -40,6 +40,7 @@ > > NvmExpressPeiHci.h > > NvmExpressPeiPassThru.c > > NvmExpressPeiPassThru.h > > + NvmExpressPeiS3.c > > NvmExpressPeiStorageSecurity.c > > NvmExpressPeiStorageSecurity.h > > > > @@ -54,6 +55,7 @@ > > BaseMemoryLib > > IoLib > > TimerLib > > + LockBoxLib > > PeimEntryPoint > > > > [Ppis] > > @@ -64,9 +66,13 @@ > > gEfiPeiVirtualBlockIo2PpiGuid ## SOMETIMES_PRODUCES > > gEdkiiPeiStorageSecurityCommandPpiGuid ## SOMETIMES_PRODUCES > > > > +[Guids] > > + gS3StorageDeviceInitListGuid ## SOMETIMES_CONSUMES= ## > > UNDEFINED > > + > > [Depex] > > gEfiPeiMemoryDiscoveredPpiGuid AND > > - gEdkiiPeiNvmExpressHostControllerPpiGuid > > + gEdkiiPeiNvmExpressHostControllerPpiGuid AND > > + gEfiPeiMasterBootModePpiGuid > > > > [UserExtensions.TianoCore."ExtraFiles"] > > NvmExpressPeiExtra.uni > > diff --git a/MdeModulePkg/Bus/Pci/NvmExpressPei/NvmExpressPei.h > > b/MdeModulePkg/Bus/Pci/NvmExpressPei/NvmExpressPei.h > > index 7047c4f3ff..6f01413e6d 100644 > > --- a/MdeModulePkg/Bus/Pci/NvmExpressPei/NvmExpressPei.h > > +++ b/MdeModulePkg/Bus/Pci/NvmExpressPei/NvmExpressPei.h > > @@ -332,4 +332,20 @@ NvmeBuildDevicePath ( > > OUT EFI_DEVICE_PATH_PROTOCOL **DevicePath > > ); > > > > +/** > > + Determine if a specific NVM Express controller can be skipped for S3= phase. > > + > > + @param[in] HcDevicePath Device path of the controller. > > + @param[in] HcDevicePathLength Length of the device path specifie= d by > > + HcDevicePath. > > + > > + @retval The number of ports that need to be enumerated. > > + > > +**/ > > +BOOLEAN > > +NvmeS3SkipThisController ( > > + IN EFI_DEVICE_PATH_PROTOCOL *HcDevicePath, > > + IN UINTN HcDevicePathLength > > + ); > > + > > #endif > > diff --git a/MdeModulePkg/Bus/Pci/NvmExpressPei/NvmExpressPei.c > > b/MdeModulePkg/Bus/Pci/NvmExpressPei/NvmExpressPei.c > > index 96622e6fd5..43b2dfc3e7 100644 > > --- a/MdeModulePkg/Bus/Pci/NvmExpressPei/NvmExpressPei.c > > +++ b/MdeModulePkg/Bus/Pci/NvmExpressPei/NvmExpressPei.c > > @@ -213,6 +213,7 @@ NvmExpressPeimEntry ( > > ) > > { > > EFI_STATUS Status; > > + EFI_BOOT_MODE BootMode; > > EDKII_NVM_EXPRESS_HOST_CONTROLLER_PPI *NvmeHcPpi; > > UINT8 Controller; > > UINTN MmioBase; > > @@ -224,6 +225,15 @@ NvmExpressPeimEntry ( > > DEBUG ((DEBUG_INFO, "%a: Enters.\n", __FUNCTION__)); > > > > // > > + // Get the current boot mode. > > + // > > + Status =3D PeiServicesGetBootMode (&BootMode); if (EFI_ERROR (Statu= s)) > > + { > > + DEBUG ((DEBUG_ERROR, "%a: Fail to get the current boot mode.\n", > > __FUNCTION__)); > > + return Status; > > + } > > + > > + // > > // Locate the NVME host controller PPI > > // > > Status =3D PeiServicesLocatePpi ( > > @@ -279,6 +289,16 @@ NvmExpressPeimEntry ( > > continue; > > } > > > > + if ((BootMode =3D=3D BOOT_ON_S3_RESUME) && > > + (NvmeS3SkipThisController (DevicePath, DevicePathLength))) { > > + DEBUG (( > > + DEBUG_ERROR, "%a: Controller %d is skipped during S3.\n", > > + __FUNCTION__, Controller > > + )); > > + Controller++; > > + continue; > > + } > > + > > // > > // Memory allocation for controller private data > > // > > diff --git a/MdeModulePkg/Bus/Pci/NvmExpressPei/NvmExpressPeiS3.c > > b/MdeModulePkg/Bus/Pci/NvmExpressPei/NvmExpressPeiS3.c > > new file mode 100644 > > index 0000000000..afcf5f6c0a > > --- /dev/null > > +++ b/MdeModulePkg/Bus/Pci/NvmExpressPei/NvmExpressPeiS3.c > > @@ -0,0 +1,106 @@ > > +/** @file > > + The NvmExpressPei driver is used to manage non-volatile memory > > +subsystem > > + which follows NVM Express specification at PEI phase. > > + > > + Copyright (c) 2019, Intel Corporation. All rights reserved.
> > + > > + This program and the accompanying materials are licensed and made > > + available under the terms and conditions of the BSD License which > > + accompanies this distribution. The full text of the license may be > > + found at http://opensource.org/licenses/bsd-license.php > > + > > + THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" > > BASIS, > > + WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER > > EXPRESS OR IMPLIED. > > + > > +**/ > > + > > +#include "NvmExpressPei.h" > > + > > +#include > > + > > +#include > > + > > +/** > > + Determine if a specific NVM Express controller can be skipped for S3= phase. > > + > > + @param[in] HcDevicePath Device path of the controller. > > + @param[in] HcDevicePathLength Length of the device path specifie= d by > > + HcDevicePath. > > + > > + @retval The number of ports that need to be enumerated. > > + > > +**/ > > +BOOLEAN > > +NvmeS3SkipThisController ( > > + IN EFI_DEVICE_PATH_PROTOCOL *HcDevicePath, > > + IN UINTN HcDevicePathLength > > + ) > > +{ > > + EFI_STATUS Status; > > + UINT8 DummyData; > > + UINTN S3InitDevicesLength; > > + EFI_DEVICE_PATH_PROTOCOL *S3InitDevices; > > + EFI_DEVICE_PATH_PROTOCOL *DevicePathInst; > > + UINTN DevicePathInstLength; > > + BOOLEAN Skip; > > + > > + // > > + // From the LockBox, get the list of device paths for devices need t= o > > + be // initialized in S3. > > + // > > + S3InitDevices =3D NULL; > > + S3InitDevicesLength =3D sizeof (DummyData); > > + Skip =3D TRUE; > > + Status =3D RestoreLockBox (&gS3StorageDeviceInitListGuid, &DummyData= , > > + &S3InitDevicesLength); if (Status !=3D EFI_BUFFER_TOO_SMALL) { > > + return Skip; >=20 > 1. If Status is SUCCESS, the device is skipped. Is that expected behavior= ? Yes. The purpose of this first call of the RestoreLockBox() with a 1-byte-length buffer input is to get the actual length of the content within the LockBox. And also, the size of a valid device path structure is supposed to be bigger than 1 byte. >=20 >=20 > > + } else { > > + S3InitDevices =3D AllocatePool (S3InitDevicesLength); > > + if (S3InitDevices =3D=3D NULL) { > > + return Skip; > > + } > > + > > + Status =3D RestoreLockBox (&gS3StorageDeviceInitListGuid, S3InitDe= vices, > > &S3InitDevicesLength); > > + if (EFI_ERROR (Status)) { > > + return Skip; > > + } > > + } > > + > > + if (S3InitDevices =3D=3D NULL) { > > + return Skip; > > + } > > + > > + // > > + // Only need to initialize the controllers that exist in the device = list. > > + // > > + do { > > + // > > + // Fetch the next device path instance. > > + // > > + DevicePathInst =3D GetNextDevicePathInstance (&S3InitDevices, > > &DevicePathInstLength); > > + if (DevicePathInst =3D=3D NULL) { > > + break; > > + } > > + > > + if ((HcDevicePathLength >=3D DevicePathInstLength) || > > + (HcDevicePathLength <=3D sizeof (EFI_DEVICE_PATH_PROTOCOL))) { > > + continue; > > + } >=20 > 2. the "<=3D sizeof (EFI_DEVICE_PATH_PROTOCOL" check isn't needed in ever= y > loop, right? Yes. I think it is redundant. I will double confirm and refine it in the next series. >=20 > 3. Will GetNextDevicePathInstance() allocate pool memory? If yes, maybe w= e > need an internal implementation to iterate each instance without allocati= ng > additional memory. Good suggestion. I will update the internal implementation in the next series. Best Regards, Hao Wu >=20 > > + > > + // > > + // Compare the device paths to determine if the device is managed = by > > this > > + // controller. > > + // > > + if (CompareMem ( > > + DevicePathInst, > > + HcDevicePath, > > + HcDevicePathLength - sizeof (EFI_DEVICE_PATH_PROTOCOL) > > + ) =3D=3D 0) { > > + Skip =3D FALSE; > > + break; > > + } > > + } while (S3InitDevices !=3D NULL); > > + > > + return Skip; > > +} > > -- > > 2.12.0.windows.1