[edk2] ArmVirtPkg: signal EndOxDxe event in PlatformBsdInit

Message ID 1435141511-9700-1-git-send-email-ard.biesheuvel@linaro.org
State New
Headers show

Commit Message

Ard Biesheuvel June 24, 2015, 10:25 a.m.
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(+)

Comments

Ard Biesheuvel June 24, 2015, 11:28 a.m. | #1
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?
Ard Biesheuvel June 24, 2015, 1:37 p.m. | #2
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.

Patch hide | download patch | download mbox

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