| Message ID | 1435141511-9700-1-git-send-email-ard.biesheuvel@linaro.org |
|---|---|
| State | New |
| Headers | show |
On 24 June 2015 at 13:15, Olivier Martin <Olivier.Martin@arm.com> wrote: > I think this patch should move to IntelFrameworkModulePkg/Universal/BdsDxe. It is a PI specification requirement to signal gEfiEndOfDxeEventGroupGuid at the end of the DXE phase. > Yes, that makes sense, I guess. But assuming most of its users are signalling the event at some point, this would result in the event to be signalled twice. Perhaps Jeff can comment here?
On 24 June 2015 at 14:51, Laszlo Ersek <lersek@redhat.com> wrote: > On 06/24/15 13:28, Ard Biesheuvel wrote: >> On 24 June 2015 at 13:15, Olivier Martin <Olivier.Martin@arm.com> wrote: >>> I think this patch should move to IntelFrameworkModulePkg/Universal/BdsDxe. It is a PI specification requirement to signal gEfiEndOfDxeEventGroupGuid at the end of the DXE phase. >>> >> >> Yes, that makes sense, I guess. But assuming most of its users are >> signalling the event at some point, this would result in the event to >> be signalled twice. >> Perhaps Jeff can comment here? > > This topic has been raised several times before (without any solution). > > I agree that signalling the event is a good idea. > So does that mean OVMF does not call it either? I tried grep'ing for instances but I could only find the one in the ARM BDS > I also agree that > > (a) either it should be done in > "IntelFrameworkModulePkg/Universal/BdsDxe", *or* > > (b) since PlatformBdsLib instances *are* linked into that driver, it > should be codified as a rule, perhaps in > > IntelFrameworkModulePkg/Include/Library/PlatformBdsLib.h > > that function FOO of the library instance is *responsible* for signaling > the event. > I'd be interested in understanding how it is called by the platforms people actually work with. For instance, the v2.5 properties table feature clearly relies on it, so implementing and testing that must have occurred on an out-of-tree platform that issues this call *somewhere* > What I would not like to see is platforms signaling the event in wildly > different places. (Unless, of course, that flexibility was an explicit > design choice!) Obviously, I'm thinking that OvmfPkg and ArmVirtPkg > should behave similarly in this context. > Agreed. > So I think we need some guidance here, from the inventors of the event, > and from the implementors of "IntelFrameworkModulePkg/Universal/BdsDxe". > > FWIW, how about this: when the DXE core is done dispatching, it decides > it is time to enter BDS. (See "gEfiBdsArchProtocolGuid" in > "IntelFrameworkModulePkg/Universal/BdsDxe/BdsDxe.inf".) Why doesn't the > DXE core itself signal end-of-dxe? > If it's a PI spec requirement to do so, then I agree it makes sense to put it in DXE core. > It is true that BDS might connect a number of devices, making available > further drivers on them; but those drivers are considered 3rd party (ie. > UEFI_DRIVER) modules, not part of the platform in the strict sense. > By that logic, the DXE phase only ever terminates at ExitBootServices(), which is probably not what was intended.
diff --git a/ArmVirtPkg/Library/PlatformIntelBdsLib/IntelBdsPlatform.c b/ArmVirtPkg/Library/PlatformIntelBdsLib/IntelBdsPlatform.c index 499cce5dcde6..0e60409cdc73 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. + +**/ +VOID +EFIAPI +EmptyCallbackFunction ( + IN EFI_EVENT Event, + IN VOID *Context + ) +{ + return; +} // // BDS Platform Functions @@ -133,6 +151,23 @@ PlatformBdsInit ( VOID ) { + EFI_EVENT EndOfDxeEvent; + EFI_STATUS Status; + + // + // Signal EndOfDxe PI Event + // + Status = gBS->CreateEventEx ( + EVT_NOTIFY_SIGNAL, + TPL_NOTIFY, + EmptyCallbackFunction, + NULL, + &gEfiEndOfDxeEventGroupGuid, + &EndOfDxeEvent + ); + if (!EFI_ERROR (Status)) { + gBS->SignalEvent (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
Currently, the ArmVirtPkg platforms 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. This patch adds signalling of the event to PlatformBdsInit() of the PlatformBdsLib instance for the Intel BDS. Note that the ARM BDS does signal the event, so this looks like a convenient place to put it. Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> --- .../Library/PlatformIntelBdsLib/IntelBdsPlatform.c | 35 ++++++++++++++++++++++ .../PlatformIntelBdsLib/PlatformIntelBdsLib.inf | 1 + 2 files changed, 36 insertions(+)