On 25 June 2015 at 14:16, Laszlo Ersek <ler...@redhat.com> wrote: > On 06/25/15 13:00, Ard Biesheuvel wrote: >> Currently, the ArmVirtPkg platforms built with the Intel BDS fail >> to signal the end-of-DXE event 'gEfiEndOfDxeEventGroupGuid' when >> entering the BDS phase, which results in some loss of functionality, >> i.e., variable reclaim in the VariableDxe drivers, and the splitting >> of the memory regions that is part of the recently added UEFI 2.5 >> properties table feature. >> >> As discussed on the edk2-devel mailing list here: >> >> http://thread.gmane.org/gmane.comp.bios.tianocore.devel/16088/focus=16109 >> >> it is up to the platform BDS to signal that event, since there may be >> platform specific ordering constraints with respect to the signalling >> of the event that are difficult to honor at the generic level. >> >> So add the SignalEvent () call to PlatformBdsInit () of ArmVirtPkg's >> PlatformBdsLib implementation for the Intel BDS. >> >> Contributed-under: TianoCore Contribution Agreement 1.0 >> Signed-off-by: Ard Biesheuvel <ard.biesheu...@linaro.org> >> --- >> v2: >> - use STATIC for empty callback function >> - use TPL_CALLBACK priority level >> - close EFI_END_OF_DXE_EVENT_GROUP_GUID event group after signalling it >> - indentation fixes >> >> ArmVirtPkg/Library/PlatformIntelBdsLib/IntelBdsPlatform.c | 36 >> ++++++++++++++++++++ >> ArmVirtPkg/Library/PlatformIntelBdsLib/PlatformIntelBdsLib.inf | 1 + >> 2 files changed, 37 insertions(+) >> >> diff --git a/ArmVirtPkg/Library/PlatformIntelBdsLib/IntelBdsPlatform.c >> b/ArmVirtPkg/Library/PlatformIntelBdsLib/IntelBdsPlatform.c >> index 499cce5dcde6..13830cbe3665 100644 >> --- a/ArmVirtPkg/Library/PlatformIntelBdsLib/IntelBdsPlatform.c >> +++ b/ArmVirtPkg/Library/PlatformIntelBdsLib/IntelBdsPlatform.c >> @@ -24,6 +24,7 @@ >> #include <Protocol/GraphicsOutput.h> >> #include <Protocol/PciIo.h> >> #include <Protocol/PciRootBridgeIo.h> >> +#include <Guid/EventGroup.h> >> >> #include "IntelBdsPlatform.h" >> >> @@ -118,6 +119,23 @@ STATIC PLATFORM_USB_KEYBOARD mUsbKeyboard = { >> } >> }; >> >> +/** >> + 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 >> +EmptyCallbackFunction ( >> + IN EFI_EVENT Event, >> + IN VOID *Context >> + ) >> +{ >> +} >> >> // >> // BDS Platform Functions >> @@ -133,6 +151,24 @@ PlatformBdsInit ( >> VOID >> ) >> { >> + EFI_EVENT EndOfDxeEvent; >> + EFI_STATUS Status; >> + >> + // >> + // Signal EndOfDxe PI Event >> + // >> + Status = gBS->CreateEventEx ( >> + EVT_NOTIFY_SIGNAL, >> + TPL_CALLBACK, >> + EmptyCallbackFunction, >> + NULL, >> + &gEfiEndOfDxeEventGroupGuid, >> + &EndOfDxeEvent >> + ); >> + if (!EFI_ERROR (Status)) { >> + gBS->SignalEvent (EndOfDxeEvent); >> + gBS->CloseEvent (EndOfDxeEvent); >> + } >> } >> >> >> diff --git a/ArmVirtPkg/Library/PlatformIntelBdsLib/PlatformIntelBdsLib.inf >> b/ArmVirtPkg/Library/PlatformIntelBdsLib/PlatformIntelBdsLib.inf >> index d8f892642c2e..d9982167e81d 100644 >> --- a/ArmVirtPkg/Library/PlatformIntelBdsLib/PlatformIntelBdsLib.inf >> +++ b/ArmVirtPkg/Library/PlatformIntelBdsLib/PlatformIntelBdsLib.inf >> @@ -65,6 +65,7 @@ [Guids] >> gEfiFileInfoGuid >> gEfiFileSystemInfoGuid >> gEfiFileSystemVolumeLabelInfoIdGuid >> + gEfiEndOfDxeEventGroupGuid >> >> [Protocols] >> gEfiDevicePathProtocolGuid >> > > Looks good, but you're going to hate me nonetheless. Because, our > exchange has been too quick, and my thoughts about addressing the same > in OvmfPkg had not matured enough until I posted my v1 review. > > Namely, both OvmfPkg and ArmPlatformPkg contain the following snippet at > the very end of their respective PlatformBdsPolicyBehavior() functions: > > BdsLibEnumerateAllBootOption (BootOptionList); > > SetBootOrderFromQemu (BootOptionList); > // > // The BootOrder variable may have changed, reload the in-memory list with > // it. > // > BdsLibBuildOptionFromVar (BootOptionList, L"BootOrder"); > > PlatformBdsEnterFrontPage (GetFrontPageTimeoutFromQemu(), TRUE); > > The pair (BdsLibEnumerateAllBootOption(), SetBootOrderFromQemu()) > massages UEFI non-volatile variables (boot options) quite heavily. The > first creates a bunch of new boot options, while the second prunes quite > a few of those. (This is necessary because a VM's hardware might change > dynamically before every single boot, so everything needs to be > enumerated, and then filtered / reordered according to the fw_cfg boot > order file, every time.) > > Since one of your stated goals is "variable reclaim" before booting the > OS, I propose (for both ArmVirtPkg and my future OvmfPkg patch(es)) that > we signal the event right before the PlatformBdsEnterFrontPage() call -- > ie. as the penultimate step of PlatformBdsPolicyBehavior() --, rather > than in PlatformBdsInit(). Then the reclaim would cover the variable > churn incurred by those two functions I named above. > > What do you think? >
The PI spec says """ 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. """ so I don't think that is going to fly. If the variable churn is a concern, perhaps it is better to trigger another reclaim round in some way? -- Ard. ------------------------------------------------------------------------------ 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
