On 25 June 2015 at 10:35, Yao, Jiewen <jiewen....@intel.com> 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.
Is there a guaranteed dispatch order for events? If not, this may be problematic, since the silicon driver must receive the end-of-DXE event before AcpiS3Save does, otherwise S3Save () may be called too late > 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. > > 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_Implementing_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
