On 06/27/15 23:53, Jordan Justen wrote: > Patches 1-4 & 7-9: Reviewed-by: Jordan Justen <jordan.l.jus...@intel.com>
Thank you. > 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. Good idea. For OvmfPkg, I'll only have to split patch #6 in two: the first half would only signal EndOfDxe, and the code movement from AcpiS3SaveDxe to PlatformBdsLib would be the separate second part. Considering the order of steps only, it doesn't matter if SaveS3BootScript() is called by S3Ready() as a final step when PlatformBDS signals EndOfDxe, versus PlatformBDS calls SaveS3BootScript() directly, after the EndOfDxe handlers, including S3Ready(), complete. I think I can split up patch #5 similarly: - Signal EndOfDxe first. Three of the AcpiS3Save->S3Save() calls in Vlv2TbltDevicePkg seem to be ineffective anyway. This first patch would just make the one call in InstallReadyToLock() ineffective too. - Remove the AcpiS3Save->S3Save() calls separately. So I think both patches in question can be split up "purely textually", ie. without having to introduce any logical detours. > > It would also be nice if MdePkg had a PiLib so we could add a EndOfDxe > functions, similar to EfiSignalEventReadyToBoot and > EfiCreateEventReadyToBootEx. This is a good idea, and you must know in advance why I hate it nonetheless. Writing this library, with one function in it, probably takes (tens of) minutes. However, getting the naming right, and finding time for Mike when he can review and accept such a patch, would delay this thing for weeks again. Oh and let's not forget the time it would take for everyone to agree on the interface of the library. If you'd like to implement this library, please go ahead, but please do it separately from this work. Once "signal EndOfDxe" is available as a separate library function, I promise I'll rebase OvmfPkg, ArmVirtPkg, and Vlv2TbltDevicePkg on top. Thanks Laszlo > > -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
