From: "Wu, Hao A" <hao.a.wu@intel.com>
To: "Ni, Ray" <ray.ni@intel.com>,
"edk2-devel@lists.01.org" <edk2-devel@lists.01.org>
Cc: "Wang, Jian J" <jian.j.wang@intel.com>,
"Dong, Eric" <eric.dong@intel.com>
Subject: Re: [PATCH v2 07/12] MdeModulePkg/NvmExpressPei: Consume S3StorageDeviceInitList LockBox
Date: Thu, 31 Jan 2019 05:45:45 +0000 [thread overview]
Message-ID: <B80AF82E9BFB8E4FBD8C89DA810C6A093C892DEB@SHSMSX104.ccr.corp.intel.com> (raw)
In-Reply-To: <734D49CCEBEEF84792F5B80ED585239D5C000040@SHSMSX104.ccr.corp.intel.com>
> -----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
>
>
>
> > -----Original Message-----
> > From: Wu, Hao A <hao.a.wu@intel.com>
> > Sent: Thursday, January 31, 2019 10:49 AM
> > To: edk2-devel@lists.01.org
> > Cc: Wu, Hao A <hao.a.wu@intel.com>; Wang, Jian J <jian.j.wang@intel.com>;
> > Ni, Ray <ray.ni@intel.com>; Dong, Eric <eric.dong@intel.com>
> > Subject: [PATCH v2 07/12] MdeModulePkg/NvmExpressPei: Consume
> > S3StorageDeviceInitList LockBox
> >
> > REF:https://bugzilla.tianocore.org/show_bug.cgi?id=1409
> >
> > For the NvmExpressPei driver, this commit will update the driver to consume
> > the S3StorageDeviceInitList LockBox in S3 phase. The purpose is to perform
> > an on-demand (partial) NVM Express device enumeration/initialization to
> > benefit the S3 resume performance.
> >
> > Cc: Jian J Wang <jian.j.wang@intel.com>
> > Cc: Ray Ni <ray.ni@intel.com>
> > Cc: Eric Dong <eric.dong@intel.com>
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Hao Wu <hao.a.wu@intel.com>
> > ---
> > 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 specified 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 = PeiServicesGetBootMode (&BootMode); if (EFI_ERROR (Status))
> > + {
> > + DEBUG ((DEBUG_ERROR, "%a: Fail to get the current boot mode.\n",
> > __FUNCTION__));
> > + return Status;
> > + }
> > +
> > + //
> > // Locate the NVME host controller PPI
> > //
> > Status = PeiServicesLocatePpi (
> > @@ -279,6 +289,16 @@ NvmExpressPeimEntry (
> > continue;
> > }
> >
> > + if ((BootMode == 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.<BR>
> > +
> > + 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 <Guid/S3StorageDeviceInitList.h>
> > +
> > +#include <Library/LockBoxLib.h>
> > +
> > +/**
> > + 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 specified 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 to
> > + be // initialized in S3.
> > + //
> > + S3InitDevices = NULL;
> > + S3InitDevicesLength = sizeof (DummyData);
> > + Skip = TRUE;
> > + Status = RestoreLockBox (&gS3StorageDeviceInitListGuid, &DummyData,
> > + &S3InitDevicesLength); if (Status != EFI_BUFFER_TOO_SMALL) {
> > + return Skip;
>
> 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.
>
>
> > + } else {
> > + S3InitDevices = AllocatePool (S3InitDevicesLength);
> > + if (S3InitDevices == NULL) {
> > + return Skip;
> > + }
> > +
> > + Status = RestoreLockBox (&gS3StorageDeviceInitListGuid, S3InitDevices,
> > &S3InitDevicesLength);
> > + if (EFI_ERROR (Status)) {
> > + return Skip;
> > + }
> > + }
> > +
> > + if (S3InitDevices == NULL) {
> > + return Skip;
> > + }
> > +
> > + //
> > + // Only need to initialize the controllers that exist in the device list.
> > + //
> > + do {
> > + //
> > + // Fetch the next device path instance.
> > + //
> > + DevicePathInst = GetNextDevicePathInstance (&S3InitDevices,
> > &DevicePathInstLength);
> > + if (DevicePathInst == NULL) {
> > + break;
> > + }
> > +
> > + if ((HcDevicePathLength >= DevicePathInstLength) ||
> > + (HcDevicePathLength <= sizeof (EFI_DEVICE_PATH_PROTOCOL))) {
> > + continue;
> > + }
>
> 2. the "<= sizeof (EFI_DEVICE_PATH_PROTOCOL" check isn't needed in every
> loop, right?
Yes. I think it is redundant.
I will double confirm and refine it in the next series.
>
> 3. Will GetNextDevicePathInstance() allocate pool memory? If yes, maybe we
> need an internal implementation to iterate each instance without allocating
> additional memory.
Good suggestion.
I will update the internal implementation in the next series.
Best Regards,
Hao Wu
>
> > +
> > + //
> > + // Compare the device paths to determine if the device is managed by
> > this
> > + // controller.
> > + //
> > + if (CompareMem (
> > + DevicePathInst,
> > + HcDevicePath,
> > + HcDevicePathLength - sizeof (EFI_DEVICE_PATH_PROTOCOL)
> > + ) == 0) {
> > + Skip = FALSE;
> > + break;
> > + }
> > + } while (S3InitDevices != NULL);
> > +
> > + return Skip;
> > +}
> > --
> > 2.12.0.windows.1
next prev parent reply other threads:[~2019-01-31 5:45 UTC|newest]
Thread overview: 29+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-01-31 2:48 [PATCH v2 00/12] Split the S3 PEI phase HW init codes from Opal driver Hao Wu
2019-01-31 2:48 ` [PATCH v2 01/12] MdeModulePkg: Add definitions for ATA AHCI host controller PPI Hao Wu
2019-01-31 3:25 ` Ni, Ray
2019-01-31 2:48 ` [PATCH v2 02/12] MdeModulePkg: Add definitions for EDKII PEI ATA PassThru PPI Hao Wu
2019-01-31 3:22 ` Ni, Ray
2019-01-31 5:28 ` Wu, Hao A
2019-01-31 2:48 ` [PATCH v2 03/12] MdeModulePkg: Add definitions for Storage Security Command PPI Hao Wu
2019-01-31 3:26 ` Ni, Ray
2019-01-31 2:48 ` [PATCH v2 04/12] MdeModulePkg: Add GUID for LockBox to save storage dev to init in S3 Hao Wu
2019-01-31 3:27 ` Ni, Ray
2019-01-31 5:30 ` Wu, Hao A
2019-01-31 2:48 ` [PATCH v2 05/12] MdeModulePkg/NvmExpressPei: Avoid updating the module-level variable Hao Wu
2019-01-31 3:28 ` Ni, Ray
2019-01-31 2:48 ` [PATCH v2 06/12] MdeModulePkg/NvmExpressPei: Add logic to produce SSC PPI Hao Wu
2019-01-31 3:35 ` Ni, Ray
2019-01-31 5:40 ` Wu, Hao A
2019-01-31 2:48 ` [PATCH v2 07/12] MdeModulePkg/NvmExpressPei: Consume S3StorageDeviceInitList LockBox Hao Wu
2019-01-31 3:45 ` Ni, Ray
2019-01-31 5:45 ` Wu, Hao A [this message]
2019-01-31 2:48 ` [PATCH v2 08/12] MdeModulePkg/AhciPei: Add AHCI mode ATA device support in PEI Hao Wu
2019-01-31 5:49 ` Ni, Ruiyu
2019-01-31 2:48 ` [PATCH v2 09/12] MdeModulePkg/SmmLockBoxLib: Use 'DEBUG_' prefix instead of 'EFI_D_' Hao Wu
2019-01-31 5:49 ` Ni, Ruiyu
2019-01-31 2:48 ` [PATCH v2 10/12] MdeModulePkg/SmmLockBox(PEI): Remove an ASSERT in RestoreLockBox() Hao Wu
2019-01-31 5:50 ` Ni, Ruiyu
2019-01-31 5:53 ` Wu, Hao A
2019-01-31 2:48 ` [PATCH v2 11/12] MdeModulePkg/SmmLockBoxLib: Support LockBox enlarge in UpdateLockBox() Hao Wu
2019-01-31 6:00 ` Ni, Ruiyu
2019-01-31 2:48 ` [PATCH v2 12/12] SecurityPkg/OpalPassword: Remove HW init codes and consume SSC PPI Hao Wu
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=B80AF82E9BFB8E4FBD8C89DA810C6A093C892DEB@SHSMSX104.ccr.corp.intel.com \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox