All, On 06/27/15 03:08, Laszlo Ersek wrote: > In > <http://thread.gmane.org/gmane.comp.bios.tianocore.devel/16088/focus=16146>, > Jiewen proposed: > >> And in the future, I believe we could remove EFI_ACPI_S3_SAVE_PROTOCOL >> protocol completely. (This is Framework 0.9 defined API, not PI >> defined API) >> >> The better design might be: >> 1) AcpiS3Save does not install EFI_ACPI_S3_SAVE_PROTOCOL. >> 2) AcpiS3Save registers EndOfDxe callback function. >> 3) AcpiS3Save does S3Save() in EndOfDxe callback function. >> So everything could be self-contained. There is no need to rely on >> platform BDS. >> >> Then this module eliminates IntelFrameworkPkg dependency and can be >> moved to MdeModulePkg later. > > This series implements the idea, and "on the side", it changes OVMF to > kick the EndOfDxe event group (which is where the entire discussion has > come from -- Ard started it because ArmVirtPkg lacked EndOfDxe). > > The series is organized carefully. It is bisectable, plus it has > another, "orthogonal" dimension (see below). The structure is: > > - Patches #1 and #2 make IntelFrameworkModulePkg/AcpiS3SaveDxe act upon > EndOfDxe *in addition to* the usual S3Save() protocol member call. > > - Patches #3 and #4 do the same to OvmfPkg's clone of the driver. > (Before anyone asks: yes, that clone needs to remain separate.) > > - Patch #5 modifies Vlv2TbltDevicePkg. That package is the only one > in the edk2 tree that uses IntelFrameworkModulePkg/AcpiS3SaveDxe. > > Because I'm implementing Jiewen's idea in > IntelFrameworkModulePkg/AcpiS3SaveDxe, I must migrate the driver's > clients, and Vlv2TbltDevicePkg is one client. (The only one.) > > So patch #5 flips Vlv2TbltDevicePkg from calling the S3Save() protocol > member to kicking the EndOfDxe event group. Unfortunately, I couldn't > even *compile* this package, let alone test it. Thus, I need help with > this patch. > > Importantly, because Vlv2TbltDevicePkg has its own clone of > GenericBdsLib, Ard's patch (patch #7) below does *not* affect it. > Therefore the Vlv2TbltDevicePkg changes are completely isolated to > patch #5. > > - Patch #6 switches over OvmfPkg from depending on the S3Save() protocol > member call (inside IntelFrameworkModulePkg/GenericBdsLib) to kicking > EndOfDxe explicitly. > > - Patch #7 is the one from Ard. It eliminates the S3Save() protocol > member call in IntelFrameworkModulePkg/GenericBdsLib. > > - After patch #5, nothing in the edk2 tree depends on the > EFI_ACPI_S3_SAVE_PROTOCOL interface exported by > IntelFrameworkModulePkg/AcpiS3SaveDxe, so that interface is removed in > patch #8. > > - After patches #6 and #7, nothing in the edk2 tree depends on the > EFI_ACPI_S3_SAVE_PROTOCOL interface exported by OvmfPkg/AcpiS3SaveDxe. > Therefore that interface is removed in patch #9. > > Now here comes the "orthogonal" dimension: the sub-series consisting of > patches #3, #4, #6, #7 and #9 implements Jiewen's idea for OvmfPkg > *only*. It does affect central code in one patch (in Ard's patch #7), > but that central code is actually not used by anything except OvmfPkg, > because Vlv2TbltDevicePkg has its own clone of GenericBdsLib. > > So, if patches #1, #2, #5 and #8 look problematic, then I'm completely > fine dropping them. Then IntelFrameworkModulePkg/AcpiS3SaveDxe and > Vlv2TbltDevicePkg won't be touched at all, and Jiewen's idea (and the > kicking of EndOfDxe) will only be implemented for OvmfPkg.
I have not received any kind of feedback for the Vlv2TbltDevicePkg patches (in nine days). Since I would not commit Vlv2TbltDevicePkg patches without maintainer R-b tags, I'm going to drop all patches from the v2 series that affect Vlv2TbltDevicePkg. (This is technically feasible: please see my earlier description above.) The change will be limited to OvmfPkg and IntelFrameworkModulePkg/Library/GenericBdsLib exclusively. (And even the latter does not affect Vlv2TbltDevicePkg.) If you disagree (ie. you'd like me to keep the Vlv2TbltDevicePkg patches, modified or unmodified), then please speak up soon. Personally I'm completely fine dropping those patches, hence my plan for v2. Thanks Laszlo > > I regression tested OVMF with these changes: S3 continues to work fine > with Fedora and Windows Server 2012 R2, and they also boot okay (and > reject S3 suspend attempts) when S3 is disabled in QEMU. > > Public branch: > https://github.com/lersek/edk2/commits/drop_efi_acpi_s3_save_proto > > Cc: Yao Jiewen <jiewen....@intel.com> > Cc: Jeff Fan <jeff....@intel.com> > Cc: Jordan Justen <jordan.l.jus...@intel.com> > Cc: Ard Biesheuvel <ard.biesheu...@linaro.org> > Cc: David Wei <david....@intel.com> > Cc: Tim He <tim...@intel.com> > > Thanks > Laszlo > > Ard Biesheuvel (1): > IntelFrameworkModulePkg/GenericBdsLib: remove AcpiS3->S3Save() call > > Laszlo Ersek (8): > IntelFrameworkModulePkg: AcpiS3SaveDxe: prepare for End-of-Dxe > callback > IntelFrameworkModulePkg: AcpiS3SaveDxe: call S3Ready() at End-of-Dxe > OvmfPkg: AcpiS3SaveDxe: prepare for End-of-Dxe callback > OvmfPkg: AcpiS3SaveDxe: call S3Ready() at End-of-Dxe > Vlv2TbltDevicePkg: replace AcpiS3Save->S3Save() with End-of-Dxe signal > OvmfPkg: reorganize (but don't reorder) S3 save state sequence > IntelFrameworkModulePkg: AcpiS3SaveDxe: drop EFI_ACPI_S3_SAVE_PROTOCOL > OvmfPkg: AcpiS3SaveDxe: drop EFI_ACPI_S3_SAVE_PROTOCOL > > 21 files changed, 228 insertions(+), 283 deletions(-) > IntelFrameworkModulePkg/Library/GenericBdsLib/BdsBoot.c > | 10 -- > IntelFrameworkModulePkg/Library/GenericBdsLib/GenericBdsLib.inf > | 1 - > IntelFrameworkModulePkg/Library/GenericBdsLib/InternalBdsLib.h > | 1 - > IntelFrameworkModulePkg/Universal/Acpi/AcpiS3SaveDxe/AcpiS3Save.c > | 103 ++++++------- > IntelFrameworkModulePkg/Universal/Acpi/AcpiS3SaveDxe/AcpiS3Save.h > | 28 +--- > IntelFrameworkModulePkg/Universal/Acpi/AcpiS3SaveDxe/AcpiS3SaveDxe.inf > | 5 +- > OvmfPkg/AcpiS3SaveDxe/AcpiS3Save.c > | 151 ++++++-------------- > OvmfPkg/AcpiS3SaveDxe/AcpiS3Save.h > | 28 +--- > OvmfPkg/AcpiS3SaveDxe/AcpiS3SaveDxe.inf > | 9 +- > OvmfPkg/Library/PlatformBdsLib/BdsPlatform.c > | 86 +++++++++++ > OvmfPkg/Library/PlatformBdsLib/BdsPlatform.h > | 4 + > OvmfPkg/Library/PlatformBdsLib/PlatformBdsLib.inf > | 4 + > Vlv2TbltDevicePkg/AcpiPlatform/AcpiPlatform.c > | 11 -- > Vlv2TbltDevicePkg/AcpiPlatform/AcpiPlatform.inf > | 1 - > Vlv2TbltDevicePkg/Library/PlatformBdsLib/BdsPlatform.c > | 53 +++++-- > Vlv2TbltDevicePkg/Library/PlatformBdsLib/BdsPlatform.h > | 1 - > Vlv2TbltDevicePkg/Library/PlatformBdsLib/PlatformBdsLib.inf > | 2 +- > > Vlv2TbltDevicePkg/Override/IntelFrameworkModulePkg/Library/GenericBdsLib/BdsBoot.c > | 10 -- > > Vlv2TbltDevicePkg/Override/IntelFrameworkModulePkg/Library/GenericBdsLib/GenericBdsLib.inf > | 1 - > > Vlv2TbltDevicePkg/Override/IntelFrameworkModulePkg/Library/GenericBdsLib/InternalBdsLib.h > | 1 - > Vlv2TbltDevicePkg/PlatformPkg.dec > | 1 - > ------------------------------------------------------------------------------ Don't Limit Your Business. Reach for the Cloud. GigeNET's Cloud Solutions provide you with the tools and support that you need to offload your IT needs and focus on growing your business. Configured For All Businesses. Start Your Cloud Today. https://www.gigenetcloud.com/ _______________________________________________ edk2-devel mailing list edk2-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/edk2-devel
