Patches 1-4 & 7-9: Reviewed-by: Jordan Justen <jordan.l.jus...@intel.com>
For Patches 5 & 6, I would like to ask if the 'end-of-dxe' signal could be added as separate patches. (Inserted before 5/6, I assume.) Would that be possible, or would the S3 interdependencies prevent it? It seems like that is important enough of a platform change that it should stand on its own, if possible. It would also be nice if MdePkg had a PiLib so we could add a EndOfDxe functions, similar to EfiSignalEventReadyToBoot and EfiCreateEventReadyToBootEx. -Jordan On 2015-06-26 18:08:07, 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 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 - > > -- > 1.8.3.1 > ------------------------------------------------------------------------------ Monitor 25 network devices or servers for free with OpManager! OpManager is web-based network management software that monitors network devices and physical & virtual servers, alerts via email & sms for fault. Monitor 25 devices for free with no restriction. Download now http://ad.doubleclick.net/ddm/clk/292181274;119417398;o _______________________________________________ edk2-devel mailing list edk2-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/edk2-devel
