On 25 June 2015 at 10:08, Ard Biesheuvel <ard.biesheu...@linaro.org> wrote: > On 25 June 2015 at 09:58, Laszlo Ersek <ler...@redhat.com> wrote: >> On 06/25/15 01:31, Yao, Jiewen 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. >>> >>> Signing end of dxe in generic BDS may break existing platform. >> >> Ard, I think we have the justification we needed for going with >> platform-specific End-of-Dxe signalling. Please update your original >> patch with a link to Jiewen's message: >> >> http://thread.gmane.org/gmane.comp.bios.tianocore.devel/16088/focus=16109 >> >> And I'm going to ACK it like that for ArmVirtPkg. >> > > OK, fair enough. > >> ( >> Note, this is actually option #2 from my email >> <http://thread.gmane.org/gmane.comp.bios.tianocore.devel/16088/focus=16095>: >> >>> - Or, OVMF will have to signal End-of-Dxe within the >>> SaveS3BootScript() function (see again the patch), right before >>> installing "gEfiDxeSmmReadyToLockProtocolGuid". Only that would >>> place the new step (2) between existent steps (1) and (3). >>> >>> In parallel, ArmVirtPkg, which doesn't implement S3 or SMM, could >>> signal End-of-Dxe wherever it wanted to. Meaning, your patch is >>> correct (assuming it doesn't break anything in practice), *if* we >>> prefer this option. >> ) >> >> Jiewen, >> >>> Would you mind to let me know which platform forget to signal this, >> >> That's every single platform in the open source edk2 tree that uses >> Intel BDS. >> >>> and can we fix it in platform BDS? >> >> Yes, we can do that; Ard already posted a patch with such an approach >> for ArmVirtPkg, and I intend to do the same for OvmfPkg. Our main doubt >> was whether it was *right* (by design) to do this separately for each >> platform (in the respective PlatformBdsLib instance), as opposed to >> doing it centrally in the BDS driver. >> >> You just lifted that doubt, so we know the way forward. >> > > Indeed. FYI, I noticed that S3Save () does the right thing if it is > invoked more than once. > That means you may be able to get away with calling it both in > PlatformBdsInit() (before end-of-DXE) *and* keep the original call > site in GenericBdsLib intact, and still end up with something that > works. It does rely on an implementation detail of AcpiS3Save though
... which you are probably doing already, I guess ... :-) >>> -----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
