On 25 June 2015 at 12:32, Laszlo Ersek <ler...@redhat.com> wrote:
> comments below
>
> On 06/25/15 10:19, 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>
>> ---
>> .../Library/PlatformIntelBdsLib/IntelBdsPlatform.c | 34
>> ++++++++++++++++++++++
>> .../PlatformIntelBdsLib/PlatformIntelBdsLib.inf | 1 +
>> 2 files changed, 35 insertions(+)
>>
>> diff --git a/ArmVirtPkg/Library/PlatformIntelBdsLib/IntelBdsPlatform.c
>> b/ArmVirtPkg/Library/PlatformIntelBdsLib/IntelBdsPlatform.c
>> index 499cce5dcde6..983a92e192b7 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,22 @@ 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
>> + )
>> +{
>> +}
>
> I'd prefer STATIC.
>
OK.
> In addition: although I can't tell off the top of my head how other
> callbacks behave for this event exactly, I'd prefer if you actually
> closed down the event in the callback. (That's a valid thing to do; it
> unregisters the callback and releases all related resources.)
>
I copy-pasted the whole thing, which kind of answers your question :-)
I was surprised to see that you need to subscribe to an event in order
to be able to signal it, but that's something I can live with.
>>
>> //
>> // BDS Platform Functions
>> @@ -133,6 +150,23 @@ PlatformBdsInit (
>> VOID
>> )
>> {
>> + EFI_EVENT EndOfDxeEvent;
>> + EFI_STATUS Status;
>> +
>> + //
>> + // Signal EndOfDxe PI Event
>> + //
>> + Status = gBS->CreateEventEx (
>> + EVT_NOTIFY_SIGNAL,
>> + TPL_NOTIFY,
>
> TPL_CALLBACK is sufficient, and more "friendly". (Unless, of course, I'm
> contradicting the revelant specs.) I usually like to go with the "least
> urgent" task priority level that serves my needs.
>
I will use MdePkg/Library/UefiLib/UefiNotTiano.c as an example, and
indeed use TPL_CALLBACK, and close the event (but not from the
callback)
>> + EmptyCallbackFunction,
>> + NULL,
>> + &gEfiEndOfDxeEventGroupGuid,
>> + &EndOfDxeEvent
>> + );
>
> The arguments and the closing paren should be indented so they line up
> with the "e" in Cr[e]ateEventEx.
>
OK
>> + 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
>>
>
> I apologize if I should have / could have pointed out all this earlier.
> Kinda chaotic around here. :(
>
No worries, same here ...
--
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
