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.
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. In OVMF, may I know: 1) Why we need install gEfiLockBoxProtocolGuid in AcpiS3Save driver? 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. 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
