From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=134.134.136.24; helo=mga09.intel.com; envelope-from=hao.a.wu@intel.com; receiver=edk2-devel@lists.01.org Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) (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 F35AA21B02822 for ; Thu, 21 Feb 2019 17:54:14 -0800 (PST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga003.fm.intel.com ([10.253.24.29]) by orsmga102.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 21 Feb 2019 17:54:14 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.58,397,1544515200"; d="scan'208";a="135413028" Received: from fmsmsx104.amr.corp.intel.com ([10.18.124.202]) by FMSMGA003.fm.intel.com with ESMTP; 21 Feb 2019 17:54:13 -0800 Received: from fmsmsx121.amr.corp.intel.com (10.18.125.36) by fmsmsx104.amr.corp.intel.com (10.18.124.202) with Microsoft SMTP Server (TLS) id 14.3.408.0; Thu, 21 Feb 2019 17:54:13 -0800 Received: from shsmsx151.ccr.corp.intel.com (10.239.6.50) by fmsmsx121.amr.corp.intel.com (10.18.125.36) with Microsoft SMTP Server (TLS) id 14.3.408.0; Thu, 21 Feb 2019 17:54:13 -0800 Received: from shsmsx104.ccr.corp.intel.com ([169.254.5.102]) by SHSMSX151.ccr.corp.intel.com ([169.254.3.172]) with mapi id 14.03.0415.000; Fri, 22 Feb 2019 09:54:11 +0800 From: "Wu, Hao A" To: "Wu, Hao A" , "edk2-devel@lists.01.org" CC: "Dong, Eric" , "Yao, Jiewen" , "Zeng, Star" , Laszlo Ersek , "Zhang, Chao B" Thread-Topic: [edk2] [PATCH v6 00/13] Split the S3 PEI phase HW init codes from Opal driver Thread-Index: AQHUyXvcRt39pQViakyTwss7mfVJ2KXrD52A Date: Fri, 22 Feb 2019 01:54:11 +0000 Message-ID: References: <20190221002440.2272-1-hao.a.wu@intel.com> In-Reply-To: <20190221002440.2272-1-hao.a.wu@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 v6 00/13] Split the S3 PEI phase HW init codes from Opal driver 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: Fri, 22 Feb 2019 01:54:15 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Thanks all, Series pushed via commits: 94e0dd1afe53d23286968130d836ff0755c17c9b..a3efbc29c45183fe69bcb311c2d974ddc= 4e7c00a Best Regards, Hao Wu > -----Original Message----- > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of > Hao Wu > Sent: Thursday, February 21, 2019 8:24 AM > To: edk2-devel@lists.01.org > Cc: Dong, Eric; Wu, Hao A; Yao, Jiewen; Zeng, Star; Laszlo Ersek; Zhang, = Chao > B > Subject: [edk2] [PATCH v6 00/13] Split the S3 PEI phase HW init codes fro= m > Opal driver >=20 > The series is also available at: > https://github.com/hwu25/edk2/tree/opal_remodel_v6 >=20 > V6 changes: > For patch 6, rename driver internal function NvmeCheckHcDevicePath() to > NvmeIsHcDevicePathValid(). >=20 > For patch 7, add more comments for purpose of selective NVM Express > controller initialization during S3 resume. >=20 > For patch 8: > * Rename driver internal function AhciCheckHcDevicePath() to > AhciIsHcDevicePathValid(); > * Add more comments for purpose of selective port enumeration/ > initialization on a ATA AHCI controller during S3 resume. >=20 > For patch 6~8, since no functional impact, I still keep the 'Reviewed-by' > from Eric. >=20 > For patch 13, as suggested by Ray, simplifies the logic within PPI > notification callback to directly use the PPI instance provided by the > input parameter of callback function. >=20 >=20 > V5 history: > For patch 11, as suggested by Star: > * Refine the UpdateLockBox() API description comment to be more precise; > * Ensure the parameter for macro 'EFI_SIZE_TO_PAGES' is of type 'UINTN'; >=20 > Since the changes are minor, I still keep the origin 'Reviewed-by' from > Ray and the 'Acked-by' tag from Laszlo. >=20 > For patch 12, sync the UpdateLockBox() API description comment with the > change made in patch 11. >=20 > Since the changes are minor, I still keep the origin 'Reviewed-by' from > Laszlo an Ray. >=20 > For patch 13, additional handling logic to prevent NULL pointer > de-reference, just as what has been done in commit > d72d8561fbe03a64e01c2ad57a93777de4b9ae2f. >=20 >=20 > V4 history: >=20 > As suggested by Laszlo, add a new patch to sync the API description > comment for UpdateLockBox() in file > "OvmfPkg/Library/LockBoxLib/LockBoxLib.c" with its counterparts in > MdeModulePkg. >=20 > For patch 13 (compared with patch 12 in V3), minor type refinement in > structure definition 'OPAL_DEVICE_LOCKBOX_DATA'. >=20 > V3 history: >=20 > For patch 2, reuse the definitions within AtaPassThru protocol header fil= e > to reduce code duplication. >=20 > For patch 4, add detailed comments to illustrate the content that will be > stored in the newly introduced LockBox. >=20 > For patch 6, device path validity check refinement within function > NvmeCheckHcDevicePath(). >=20 > For patch 7: > * Remove a redundant check within function NvmeS3SkipThisController(); > * Replace internal implementation of GetNextDevicePathInstance() with > GetDevicePathInstanceSize(), which avoids unnecessary memory allocation= . >=20 > For patch 8: > * Device path validity check refinement within function > AhciCheckHcDevicePath(); > * Remove a redundant check within function AhciS3GetEumeratePorts(); > * Replace internal implementation of GetNextDevicePathInstance() with > GetDevicePathInstanceSize(), which avoids unnecessary memory allocation= . >=20 > For patch 11: > * Remove the ASSERT() for memory allocation failure. Since error handling > codes already exist, no new code is added for this; > * Refine the codes to only allocate new SMM buffer when the required > LockBox size is greater than the size of origin pages allocated; > * Add additional parameter check for 'Offset' & 'Length' to prevent > potential numeric overflow. >=20 >=20 > V2 history: >=20 > For patch 8, the new series removes the codes to produce the Block IO PPI= s > from the AhciPei driver. >=20 > The task to produce the Block IO services is out of the scope of BZ-1409 > (actually covered by BZ-1483). And we will later propose seperate patch(s= ) > to address this. >=20 > V1 history: >=20 > For the below 2 types of storage device: >=20 > 1. NVM Express devices; > 2. ATA hard disk devices working under AHCI mode. >=20 > the OpalPassword driver supports auto-unlocking those devices during S3 > resume. >=20 > Current implementation of the OpalPassword driver is handling the device > initialization (using boot script to restore the host controller PCI > configuration space also counts) in the PEI phase during S3 resume by > itself. >=20 > Meanwhile, the NvmExpressPei driver in MdeModulePkg also handles the > NVME > device initialization during the PEI phase in order to produce the Block > IO PPI. >=20 > Moreover, there is a Bugzilla request (BZ-1483) for adding the Block IO > PPI support for ATA device as well. So there is likely to be an PEI drive= r > for ATA device that will handle the ATA device initialization. >=20 > In order to remove code duplication, the series will split the S3 phase > device initialization related codes out from the OpalPassword driver. And > let the existing NvmExpressPei driver and the new AhciPei driver to handl= e > the task. >=20 > After this remodel, NvmExpressPei and AhciPei drivers will produce a PPI > called Storage Security Command PPI. And the OpalPassword driver will > consume this PPI to perform the device auto-unlock in S3 resume. >=20 >=20 > Patch 1~4: Add the definitions of PPIs and GUIDs; > Patch 5: Refinement for the NvmExpressPei driver; > Patch 6~7: Update the NvmExpressPei driver to produce the PPI needed by > OpalPassword; > Patch 8: Add the Ahci mode ATA device support in the PEI phase, it > will produce the PPI needed by OpalPassword; > Patch 9~10: Refinements for the SmmLockBoxLib; > Patch 11: Support LockBox enlarge for LockBoxLib API UpdateLockBox(); > Patch 12: Remove the hardware initialization codes from the > OpalPassword driver. And consume the SSC PPI to unlock devic= e > in S3 resume. >=20 > Cc: Jian J Wang > Cc: Ray Ni > Cc: Eric Dong > Cc: Star Zeng > Cc: Chao Zhang > Cc: Jiewen Yao > Cc: Laszlo Ersek >=20 > Hao Wu (12): > MdeModulePkg: Add definitions for ATA AHCI host controller PPI > MdeModulePkg: Add definitions for EDKII PEI ATA PassThru PPI > MdeModulePkg: Add definitions for Storage Security Command PPI > MdeModulePkg: Add GUID for LockBox to save storage dev to init in S3 > MdeModulePkg/NvmExpressPei: Avoid updating the module-level variable > MdeModulePkg/NvmExpressPei: Add logic to produce SSC PPI > MdeModulePkg/NvmExpressPei: Consume S3StorageDeviceInitList LockBox > MdeModulePkg/AhciPei: Add AHCI mode ATA device support in PEI > MdeModulePkg/SmmLockBoxLib: Use 'DEBUG_' prefix instead of 'EFI_D_' > MdeModulePkg/SmmLockBox(PEI): Remove an ASSERT in RestoreLockBox() > MdeModulePkg/SmmLockBoxLib: Support LockBox enlarge in > UpdateLockBox() > OvmfPkg/LockBoxLib: Update the comments for API UpdateLockBox() > SecurityPkg/OpalPassword: Remove HW init codes and consume SSC PPI >=20 > MdeModulePkg/MdeModulePkg.dec | 12= + > MdeModulePkg/MdeModulePkg.dsc | 1= + > MdeModulePkg/Bus/Ata/AhciPei/AhciPei.inf | 74= + > MdeModulePkg/Bus/Pci/NvmExpressPei/NvmExpressPei.inf | 18= +- > SecurityPkg/Tcg/Opal/OpalPassword/OpalPasswordDxe.inf | 6= +- > SecurityPkg/Tcg/Opal/OpalPassword/OpalPasswordPei.inf | 12= +- > MdeModulePkg/Bus/Ata/AhciPei/AhciPei.h | 708= +++++++ > MdeModulePkg/Bus/Ata/AhciPei/AhciPeiPassThru.h | 184= ++ > MdeModulePkg/Bus/Ata/AhciPei/AhciPeiStorageSecurity.h | 247= +++ > MdeModulePkg/Bus/Pci/NvmExpressPei/NvmExpressPei.h | 106= +- > MdeModulePkg/Bus/Pci/NvmExpressPei/NvmExpressPeiHci.h | 20= +- > MdeModulePkg/Bus/Pci/NvmExpressPei/NvmExpressPeiStorageSecurity.h > | 247 +++ > MdeModulePkg/Include/Guid/S3StorageDeviceInitList.h | 64= + > MdeModulePkg/Include/Library/LockBoxLib.h | 7= +- > MdeModulePkg/Include/Ppi/AtaAhciController.h | 89= + > MdeModulePkg/Include/Ppi/AtaPassThru.h | 219= +++ > MdeModulePkg/Include/Ppi/StorageSecurityCommand.h | 283= +++ > SecurityPkg/Tcg/Opal/OpalPassword/OpalAhciMode.h | 412= ---- > SecurityPkg/Tcg/Opal/OpalPassword/OpalDriver.h | 4= +- > SecurityPkg/Tcg/Opal/OpalPassword/OpalNvmeMode.h | 327= ---- > SecurityPkg/Tcg/Opal/OpalPassword/OpalNvmeReg.h | 815= -------- > SecurityPkg/Tcg/Opal/OpalPassword/OpalPasswordCommon.h | 45= +- > SecurityPkg/Tcg/Opal/OpalPassword/OpalPasswordPei.h | 97= +- > MdeModulePkg/Bus/Ata/AhciPei/AhciMode.c | 2015 > ++++++++++++++++++++ > MdeModulePkg/Bus/Ata/AhciPei/AhciPei.c | 310= +++ > MdeModulePkg/Bus/Ata/AhciPei/AhciPeiPassThru.c | 521= +++++ > MdeModulePkg/Bus/Ata/AhciPei/AhciPeiS3.c | 139= ++ > MdeModulePkg/Bus/Ata/AhciPei/AhciPeiStorageSecurity.c | 391 > ++++ > MdeModulePkg/Bus/Ata/AhciPei/DevicePath.c | 284= +++ > MdeModulePkg/Bus/Ata/AhciPei/DmaMem.c | 270= +++ > MdeModulePkg/Bus/Pci/NvmExpressPei/DevicePath.c | 284= +++ > MdeModulePkg/Bus/Pci/NvmExpressPei/DmaMem.c | 153= +- > MdeModulePkg/Bus/Pci/NvmExpressPei/NvmExpressPei.c | 172= +- > MdeModulePkg/Bus/Pci/NvmExpressPei/NvmExpressPeiHci.c | 32= +- > MdeModulePkg/Bus/Pci/NvmExpressPei/NvmExpressPeiS3.c | 114 > ++ > MdeModulePkg/Bus/Pci/NvmExpressPei/NvmExpressPeiStorageSecurity.c > | 423 ++++ > MdeModulePkg/Library/LockBoxNullLib/LockBoxNullLib.c | 7= +- > MdeModulePkg/Library/SmmLockBoxLib/SmmLockBoxDxeLib.c | 27 > +- > MdeModulePkg/Library/SmmLockBoxLib/SmmLockBoxPeiLib.c | 32 > +- > MdeModulePkg/Library/SmmLockBoxLib/SmmLockBoxSmmLib.c | > 148 +- > OvmfPkg/Library/LockBoxLib/LockBoxLib.c | 11= +- > SecurityPkg/Tcg/Opal/OpalPassword/OpalAhciMode.c | 1282= -------- > ----- > SecurityPkg/Tcg/Opal/OpalPassword/OpalDriver.c | 414= ++-- > SecurityPkg/Tcg/Opal/OpalPassword/OpalNvmeMode.c | 1823= ------ > ------------ > SecurityPkg/Tcg/Opal/OpalPassword/OpalPasswordPei.c | 702= ++---- > - > MdeModulePkg/Bus/Ata/AhciPei/AhciPei.uni | 21= + > MdeModulePkg/Bus/Ata/AhciPei/AhciPeiExtra.uni | 19= + > 47 files changed, 7680 insertions(+), 5911 deletions(-) > create mode 100644 MdeModulePkg/Bus/Ata/AhciPei/AhciPei.inf > create mode 100644 MdeModulePkg/Bus/Ata/AhciPei/AhciPei.h > create mode 100644 MdeModulePkg/Bus/Ata/AhciPei/AhciPeiPassThru.h > create mode 100644 > MdeModulePkg/Bus/Ata/AhciPei/AhciPeiStorageSecurity.h > create mode 100644 > MdeModulePkg/Bus/Pci/NvmExpressPei/NvmExpressPeiStorageSecurity.h > create mode 100644 > MdeModulePkg/Include/Guid/S3StorageDeviceInitList.h > create mode 100644 MdeModulePkg/Include/Ppi/AtaAhciController.h > create mode 100644 MdeModulePkg/Include/Ppi/AtaPassThru.h > create mode 100644 > MdeModulePkg/Include/Ppi/StorageSecurityCommand.h > delete mode 100644 SecurityPkg/Tcg/Opal/OpalPassword/OpalAhciMode.h > delete mode 100644 > SecurityPkg/Tcg/Opal/OpalPassword/OpalNvmeMode.h > delete mode 100644 SecurityPkg/Tcg/Opal/OpalPassword/OpalNvmeReg.h > create mode 100644 MdeModulePkg/Bus/Ata/AhciPei/AhciMode.c > create mode 100644 MdeModulePkg/Bus/Ata/AhciPei/AhciPei.c > create mode 100644 MdeModulePkg/Bus/Ata/AhciPei/AhciPeiPassThru.c > create mode 100644 MdeModulePkg/Bus/Ata/AhciPei/AhciPeiS3.c > create mode 100644 > MdeModulePkg/Bus/Ata/AhciPei/AhciPeiStorageSecurity.c > create mode 100644 MdeModulePkg/Bus/Ata/AhciPei/DevicePath.c > create mode 100644 MdeModulePkg/Bus/Ata/AhciPei/DmaMem.c > create mode 100644 MdeModulePkg/Bus/Pci/NvmExpressPei/DevicePath.c > create mode 100644 > MdeModulePkg/Bus/Pci/NvmExpressPei/NvmExpressPeiS3.c > create mode 100644 > MdeModulePkg/Bus/Pci/NvmExpressPei/NvmExpressPeiStorageSecurity.c > delete mode 100644 SecurityPkg/Tcg/Opal/OpalPassword/OpalAhciMode.c > delete mode 100644 > SecurityPkg/Tcg/Opal/OpalPassword/OpalNvmeMode.c > create mode 100644 MdeModulePkg/Bus/Ata/AhciPei/AhciPei.uni > create mode 100644 MdeModulePkg/Bus/Ata/AhciPei/AhciPeiExtra.uni >=20 > -- > 2.12.0.windows.1 >=20 > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel