On 06/25/15 18:19, Yao, Jiewen wrote: > Hi Ersek > That is just my thought, and I have no solid time frame at this moment, to be > honest. > If you have time to do the cleanup, please just go ahead. > I appreciate your help on that, and I will be happy to review your code > change.
Okay, I'll add this to my queue. > > I am sorry that I did not realize that OVMF has a its own AcpiS3Save driver. > Maybe I miss some email before. > In most case, we expect people feedback the limitation to us, before > duplicate a generic driver in its own package and update. > Then if we have a way to resolve the limitation, we can update the generic > driver. Right. The simplest way to learn about the reasons why OvmfPkg cloned the AcpiS3Save driver is just to run: git log --reverse OvmfPkg/AcpiS3SaveDxe/ (I expect something similar can be done with the "svn" utility too.) Because we insist on detailed commit messages, that will explain all the changes in chronologically increasing order. > In OVMF, may I know: > 1) Why we need install gEfiLockBoxProtocolGuid in AcpiS3Save driver? This is explained in: https://github.com/tianocore/edk2/commit/d4ba06df The installation of gEfiLockBoxProtocolGuid in OvmfPkg/AcpiS3SaveDxe/ will be made conditional in my upcoming SMM series. > 2) Why we need call SaveS3BootScript() in AcpiS3Save driver, and install > gEfiDxeSmmReadyToLockProtocolGuid? > It looks weird to me. But I guess there must be some reason behind, I am not > aware of. This is explained in: https://github.com/tianocore/edk2/commit/5a217a06 Your question is very good, by the way: this is *exactly* the code that I wrote in order to be compatible with the BDS DXE bug that Ard's patch: [edk2] [PATCH] IntelFrameworkModulePkg/GenericBdsLib: remove AcpiS3->S3Save() call http://thread.gmane.org/gmane.comp.bios.tianocore.devel/16153 removes. So once Ard's patch is committed, and/or your better design is implemented, this is exactly the code that will not be reached in OVMF, and I will have to modify. --*-- In general, if you have questions that relate to specific lines of code, you can run: git blame -- OvmfPkg/AcpiS3SaveDxe/AcpiS3Save.c In the output of that command, each line will be prefixed with the identifier of the commit that added that line. You can then run git show COMMIT_ID which will give you the commit message and the full patch. Thanks Laszlo > > Thank you > Yao Jiewen > > -----Original Message----- > From: Laszlo Ersek [mailto:ler...@redhat.com] > Sent: Thursday, June 25, 2015 11:23 PM > To: Yao, Jiewen > Cc: edk2-devel@lists.sourceforge.net; Fan, Jeff > Subject: Re: [edk2] [RFC PATCH] IntelFrameworkModulePkg: signal end-of-DXE > event upon platform BDS invocation > > Jiewen, > > On 06/25/15 10:35, Yao, Jiewen wrote: >> 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. > > Do you have a time frame in mind? > > Especially in relation to: > > [edk2] [PATCH] IntelFrameworkModulePkg/GenericBdsLib: remove > AcpiS3->S3Save() call > http://thread.gmane.org/gmane.comp.bios.tianocore.devel/16153 > > Because, once that patch goes in, S3 will be broken in OVMF. It will have to > be fixed. I could fix it up according to the current state of the tree, *or* > according to the "better design" above. > >> Then this module eliminates IntelFrameworkPkg dependency and can be moved to >> MdeModulePkg later. > > This idea raises further questions. (Because, "OvmfPkg/AcpiS3SaveDxe" is a > clone of the original driver under IntelFrameworkModulePkg, for good > reasons.) > > - I could try to implement your "better design" above, under > IntelFrameworkModulePkg. Would you like me to do that (and review the > code in turn), or do you prefer to implement it yourself? > > - I could port those changes to OVMF's copy of the driver. > > - I could then fix up OVMF's platform BDS in a "long term" form. > > - However, I will certainly *not* try to move the IntelFrameworkPkg > driver to MdeModulePkg. Such attempts have only bought me misery. > > Summary: > - Who should implement your better design? > - When, in absolute time? > - When, relative to committing the GenericBdsLib patch above? > > Thanks > Laszlo > >> >> Thank you >> Yao Jiewen >> >> -----Original Message----- >> From: Yao, Jiewen [mailto:jiewen....@intel.com] >> Sent: Thursday, June 25, 2015 4:24 PM >> To: edk2-devel@lists.sourceforge.net >> Cc: Fan, Jeff >> Subject: Re: [edk2] [RFC PATCH] IntelFrameworkModulePkg: signal >> end-of-DXE event upon platform BDS invocation >> >> HI Ard >> You are very careful and definitely right. >> >> The code in BdsBoot.c seems be added in more than 10 years ago, when we did >> EDK-I. At that time, we do not have such thorough design review. >> In old days, we just need find a place to call S3Save() then someone added >> it, and leave it till now. >> It still works till now, because S3Save() function only required to be >> entered once. Since most platforms already call S3Save() in platform BDS. It >> has no impact here. See code below: >> ======================= >> // >> // Platform may invoke AcpiS3Save->S3Save() before ExitPmAuth, because we >> need save S3 information there, while BDS ReadyToBoot may invoke it again. >> // So if 2nd S3Save() is triggered later, we need ignore it. >> // >> if (AlreadyEntered) { >> return EFI_SUCCESS; >> } >> AlreadyEntered = TRUE; >> ======================= >> >> Also, if a platform fail to call S3Save() early and let BDS trigger S3Save() >> here, I think system will ASSERT, because SaveLockBox will fail. >> ======================= >> Status = SaveLockBox ( >> &mAcpiS3IdtrProfileGuid, >> (VOID *)(UINTN)Idtr, >> (UINTN)sizeof(IA32_DESCRIPTOR) >> ); >> ASSERT_EFI_ERROR (Status); >> ======================= >> >> As conclusion, I completely agree with you that this code in BdsBoot.c >> should be removed to eliminate confusing. >> >> Thank you >> Yao Jiewen >> >> >> -----Original Message----- >> From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org] >> Sent: Thursday, June 25, 2015 3:51 PM >> To: edk2-devel@lists.sourceforge.net >> Cc: Fan, Jeff >> Subject: Re: [edk2] [RFC PATCH] IntelFrameworkModulePkg: signal >> end-of-DXE event upon platform BDS invocation >> >> On 25 June 2015 at 01:31, Yao, Jiewen <jiewen....@intel.com> wrote: >>> Hi Ard >>> You are right that END_OF_DXE must be signal in Bds. >>> >>> My thought is that this event should be signal in "platform BDS" instead of >>> "generic BDS". >>> >>> The reason is that only platform know when to exit platform manufacture >>> auth state, and platform may have some special step before that. >>> >> >> That is true. >> >>> Signing end of dxe in generic BDS may break existing platform. >>> >>> Would you mind to let me know which platform forget to signal this, and can >>> we fix it in platform BDS? >>> >> >> There are two constraints for the moment where end-of-DXE is signalled: >> - according to the PI spec >> >> """ >> 5.1.2.1 End of DXE Event >> Prior to invoking any UEFI drivers, applications, or connecting consoles, >> the platform should signal the event EFI_END_OF_DXE_EVENT_GUID. >> """ >> >> which means PlatformBdsInit() would be a reasonable candidate location, >> provided that it gets documented in the PlatformBdsLib API that the even >> should be signalled then. >> >> However, there is another constraint, according to >> https://firmware.intel.com/sites/default/files/A_Tour_Beyond_BIOS_Impl >> ementing_S3_resume_with_EDKII.pdf >> >> """ >> PlatformBDS need 1) call AcpiS3->S3Save() to save S3 system information, >> then 2) signal EndOfDxe to give DXE silicon driver last chance to save boot >> script, and finally 3) signal 17 SmmReadyToLock to close boot script and >> save boot script to LockBox. These calls must be in this order, or the boot >> script might not be able to save correctly. >> """ >> >> The problem is (as Laszlo has pointed out) that the generic BDS calls >> AcpiS3->S3Save () in BdsLibBootViaBootOption (), which is problematic >> for two reasons: >> - it is long after PlatformBdsInit(), so signalling end-of-DXE there >> will occur before S3Save >> - BdsLibBootViaBootOption () may never return control to the Platform >> BDS, so there is no way the Platform BDS can ensure that end-of-DXE >> will be signalled after the existing S3Save() >> >> In summary, the S3Save() call needs to be moved if we want to adhere to both >> constraints above. >> >> -- >> Ard. >> >> >>> -----Original Message----- >>> From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org] >>> Sent: Thursday, June 25, 2015 3:48 AM >>> To: edk2-devel@lists.sourceforge.net; ler...@redhat.com; >>> olivier.mar...@arm.com; Justen, Jordan L; Fan, Jeff >>> Subject: [edk2] [RFC PATCH] IntelFrameworkModulePkg: signal >>> end-of-DXE event upon platform BDS invocation >>> >>> According to paragraph 5.1.2.1 of the PI spec vol 2 version 1.4, the >>> EFI_END_OF_DXE_EVENT_GUID event must be signalled prior to invoking any >>> UEFI drivers, applications, or connecting consoles. >>> >>> This implements the event signalling in a new GenericBdsLib function >>> BdsLibSignalEndOfDxeEvent (), and adds the invocation to BdsEntry () right >>> before the point where it hands over to PlatformBdsInit(). >>> >>> In order to retain the mandated ordering between saving the ACPI S3 script >>> and signalling the EFI_END_OF_DXE_EVENT_GUID event, saving of the S3 script >>> is moved to BdsLibSignalEndOfDxeEvent () as well, right before the event is >>> signalled. >>> >>> Contributed-under: TianoCore Contribution Agreement 1.0 >>> Signed-off-by: Ard Biesheuvel <ard.biesheu...@linaro.org> >>> --- >>> >>> I am having trouble wrapping my head around all the interdependencies, but >>> I do think that this may be a feasible approach. >>> >>> IntelFrameworkModulePkg/Include/Library/GenericBdsLib.h | 14 +++ >>> IntelFrameworkModulePkg/Library/GenericBdsLib/BdsBoot.c | 70 >>> ++++++++++-- >>> IntelFrameworkModulePkg/Library/GenericBdsLib/GenericBdsLib.inf | 1 + >>> IntelFrameworkModulePkg/Library/GenericBdsLib/InternalBdsLib.h | 1 + >>> IntelFrameworkModulePkg/Universal/BdsDxe/BdsEntry.c | 3 + >>> 5 files changed, 79 insertions(+), 10 deletions(-) >>> >>> diff --git a/IntelFrameworkModulePkg/Include/Library/GenericBdsLib.h >>> b/IntelFrameworkModulePkg/Include/Library/GenericBdsLib.h >>> index ecd68a003bbb..86cda6615bf7 100644 >>> --- a/IntelFrameworkModulePkg/Include/Library/GenericBdsLib.h >>> +++ b/IntelFrameworkModulePkg/Include/Library/GenericBdsLib.h >>> @@ -1110,5 +1110,19 @@ DisableQuietBoot ( >>> VOID >>> ); >>> >>> + >>> +/** >>> + Signal the event EFI_END_OF_DXE_EVENT_GUID as specified by the PI >>> +spec >>> + vol 2 version 1.4 paragraph 5.1.2.1 >>> + >>> + @retval EFI_SUCCESS The event was signalled successfully >>> + >>> +**/ >>> +EFI_STATUS >>> +EFIAPI >>> +BdsLibSignalEndOfDxeEvent ( >>> + VOID >>> + ); >>> + >>> #endif >>> >>> diff --git a/IntelFrameworkModulePkg/Library/GenericBdsLib/BdsBoot.c >>> b/IntelFrameworkModulePkg/Library/GenericBdsLib/BdsBoot.c >>> index e02a71015edc..d84b0c1a453b 100644 >>> --- a/IntelFrameworkModulePkg/Library/GenericBdsLib/BdsBoot.c >>> +++ b/IntelFrameworkModulePkg/Library/GenericBdsLib/BdsBoot.c >>> @@ -2233,7 +2233,6 @@ BdsLibBootViaBootOption ( >>> EFI_DEVICE_PATH_PROTOCOL *FilePath; >>> EFI_LOADED_IMAGE_PROTOCOL *ImageInfo; >>> EFI_DEVICE_PATH_PROTOCOL *WorkingDevicePath; >>> - EFI_ACPI_S3_SAVE_PROTOCOL *AcpiS3Save; >>> LIST_ENTRY TempBootLists; >>> EFI_BOOT_LOGO_PROTOCOL *BootLogo; >>> >>> @@ -2241,15 +2240,6 @@ BdsLibBootViaBootOption ( >>> *ExitData = NULL; >>> >>> // >>> - // Notes: this code can be remove after the s3 script table >>> - // hook on the event EVT_SIGNAL_READY_TO_BOOT or >>> - // EVT_SIGNAL_LEGACY_BOOT >>> - // >>> - Status = gBS->LocateProtocol (&gEfiAcpiS3SaveProtocolGuid, NULL, >>> (VOID **) &AcpiS3Save); >>> - if (!EFI_ERROR (Status)) { >>> - AcpiS3Save->S3Save (AcpiS3Save, NULL); >>> - } >>> - // >>> // If it's Device Path that starts with a hard drive path, append it >>> with the front part to compose a >>> // full device path >>> // >>> @@ -4363,3 +4353,63 @@ BdsLibUpdateFvFileDevicePath ( >>> } >>> return EFI_NOT_FOUND; >>> } >>> + >>> +/** >>> + An empty function to pass error checking of CreateEventEx (). >>> + >>> + @param Event Event whose notification function is being >>> invoked. >>> + @param Context Pointer to the notification function's >>> context, >>> + which is implementation-dependent. >>> + >>> +**/ >>> +STATIC VOID >>> +EFIAPI >>> +BdsEmptyCallbackFunction ( >>> + IN EFI_EVENT Event, >>> + IN VOID *Context >>> + ) >>> +{ >>> +} >>> + >>> +/** >>> + Signal the event EFI_END_OF_DXE_EVENT_GUID as specified by the PI >>> +spec >>> + vol 2 version 1.4 paragraph 5.1.2.1 >>> + >>> + @retval EFI_SUCCESS The event was signalled successfully >>> + >>> +**/ >>> +EFI_STATUS >>> +EFIAPI >>> +BdsLibSignalEndOfDxeEvent ( >>> + VOID >>> + ) >>> +{ >>> + EFI_ACPI_S3_SAVE_PROTOCOL *AcpiS3Save; >>> + EFI_EVENT EndOfDxeEvent; >>> + EFI_STATUS Status; >>> + >>> + // >>> + // Save the S3 resume script before signalling the end-of-DXE event. >>> + // >>> + Status = gBS->LocateProtocol (&gEfiAcpiS3SaveProtocolGuid, NULL, >>> + (VOID **) &AcpiS3Save); if (!EFI_ERROR (Status)) { >>> + AcpiS3Save->S3Save (AcpiS3Save, NULL); } >>> + >>> + // >>> + // Signal EndOfDxe PI Event >>> + // >>> + Status = gBS->CreateEventEx ( >>> + EVT_NOTIFY_SIGNAL, >>> + TPL_NOTIFY, >>> + BdsEmptyCallbackFunction, >>> + NULL, >>> + &gEfiEndOfDxeEventGroupGuid, >>> + &EndOfDxeEvent >>> + ); >>> + if (!EFI_ERROR (Status)) { >>> + gBS->SignalEvent (EndOfDxeEvent); } >>> + >>> + return Status; >>> +} >>> diff --git >>> a/IntelFrameworkModulePkg/Library/GenericBdsLib/GenericBdsLib.inf >>> b/IntelFrameworkModulePkg/Library/GenericBdsLib/GenericBdsLib.inf >>> index 5381e331ff8c..42ac1185f3ab 100644 >>> --- a/IntelFrameworkModulePkg/Library/GenericBdsLib/GenericBdsLib.inf >>> +++ b/IntelFrameworkModulePkg/Library/GenericBdsLib/GenericBdsLib.inf >>> @@ -104,6 +104,7 @@ [Guids] >>> ## SOMETIMES_CONSUMES ## Variable:L"LegacyDevOrder" >>> gEfiLegacyDevOrderVariableGuid >>> gEdkiiStatusCodeDataTypeVariableGuid ## SOMETIMES_CONSUMES ## >>> GUID >>> + gEfiEndOfDxeEventGroupGuid >>> >>> [Protocols] >>> gEfiSimpleFileSystemProtocolGuid ## SOMETIMES_CONSUMES >>> diff --git >>> a/IntelFrameworkModulePkg/Library/GenericBdsLib/InternalBdsLib.h >>> b/IntelFrameworkModulePkg/Library/GenericBdsLib/InternalBdsLib.h >>> index c32579bfc577..658097ec22ea 100644 >>> --- a/IntelFrameworkModulePkg/Library/GenericBdsLib/InternalBdsLib.h >>> +++ b/IntelFrameworkModulePkg/Library/GenericBdsLib/InternalBdsLib.h >>> @@ -52,6 +52,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER >>> EXPRESS OR IMPLIED. >>> #include <Guid/LastEnumLang.h> >>> #include <Guid/LegacyDevOrder.h> >>> #include <Guid/StatusCodeDataTypeVariable.h> >>> +#include <Guid/EventGroup.h> >>> >>> #include <Library/PrintLib.h> >>> #include <Library/DebugLib.h> >>> diff --git a/IntelFrameworkModulePkg/Universal/BdsDxe/BdsEntry.c >>> b/IntelFrameworkModulePkg/Universal/BdsDxe/BdsEntry.c >>> index ae7ad2153c51..0413d34fce02 100644 >>> --- a/IntelFrameworkModulePkg/Universal/BdsDxe/BdsEntry.c >>> +++ b/IntelFrameworkModulePkg/Universal/BdsDxe/BdsEntry.c >>> @@ -623,6 +623,9 @@ BdsEntry ( >>> InitializeLanguage (TRUE); >>> InitializeFrontPage (TRUE); >>> >>> + Status = BdsLibSignalEndOfDxeEvent (); ASSERT_EFI_ERROR (Status); >>> + >>> // >>> // Do the platform init, can be customized by OEM/IBV >>> // >>> -- >>> 1.9.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 >>> >>> --------------------------------------------------------------------- >>> - >>> -------- 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 >> >> ---------------------------------------------------------------------- >> -------- 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 >> >> ---------------------------------------------------------------------- >> -------- 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 >> >> ---------------------------------------------------------------------- >> -------- 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 >> > ------------------------------------------------------------------------------ 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
