On 09/03/15 18:39, Leif Lindholm wrote:
> On Tue, Sep 01, 2015 at 04:26:07PM +0200, Ard Biesheuvel wrote:
>> Like the ArmVirtPkg platforms up until SVN r17713, the ArmPlatformPkg
>> 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 ArmPlatformPkg's
>> PlatformBdsLib implementation for the Intel BDS.
>
> Naïve, and perhaps silly question/comment:
> ArmVirtPkg, and this patch, both call this from PlatformBdsInit(),
> whereas OvmfPkg does it from PlatformBdsPolicyBehavior().
>
> The difference is the following segment in
> IntelFrameworkModulePkg/Universal/BdsDxe/BdsEntry.c
> which gets executed before signalling EndOfDxeEvent in OvmfPkg, but
> not for Arm*:
> ---
> //
> // Set up the device list based on EFI 1.1 variables
> // process Driver#### and Load the driver's in the
> // driver option list
> //
> BdsLibBuildOptionFromVar (&DriverOptionList, L"DriverOrder");
> if (!IsListEmpty (&DriverOptionList)) {
> BdsLibLoadDrivers (&DriverOptionList);
> }
> //
> // Check if we have the boot next option
> //
> mBootNext = BdsLibGetVariableAndSize (
> L"BootNext",
> &gEfiGlobalVariableGuid,
> &BootNextSize
> );
> ---
>
> and the code at the start of Ovmf's PlatformBdsPolicyBehavior():
> ---
> VisitAllInstancesOfProtocol (&gEfiPciRootBridgeIoProtocolGuid,
> ConnectRootBridge, NULL);
>
> //
> // We can't signal End-of-Dxe earlier than this. Namely, End-of-Dxe triggers
> // the preparation of S3 system information. That logic has a hard
> dependency
> // on the presence of the FACS ACPI table. Since our ACPI tables are only
> // installed after PCI enumeration completes, we must not trigger the S3
> save
> // earlier, hence we can't signal End-of-Dxe earlier.
> //
> ---
>
> Does the above not apply for any ArmPlatformPkg platforms (or others
> importing ArmPlatformPkg's PlatformIntelBdsLib)?
> If so, is this also not applicable to virtio-pci in ArmVirtPkg?
I'm glad you asked; I had expected this question while writing the above
comment. :)
First, what ArmVirtPkg does is correct. And, what OvmfPkg does is not
correct, if we choose to be *extremely pedantic* anyway, because
EndOfDxe should be kicked *before* UEFI drivers (ie. third party
drivers, ie. untrusted, non-platform drivers) are dispatched.
However... You can see the justification in the OvmfPkg comment that you
quoted. This is a QEMU peculiarity that we just have to live with. Also,
as mitigating factor, we consider the UEFI drivers that our own selves
build into the firmware "trusted". (A related fact is that we don't
forbid PCD usage in our own UEFI drivers, although the EFI_PCD_PROTOCOL
is not part of the UEFI spec. We kinda consider our own UEFI drivers
part of the platform firmware.)
And, if someone is concerned about "external" UEFI drivers being
dispatched (eg. from some FAT partition) before we get to this part in
OVMF, then just enable Secure Boot, and then those drivers will either
not be dispatched, or (if they are signed appropriately) you should
trust them not to mess with stuff that is otherwise locked down by
EndOfDxe. This is not a perfect argument, but that's what we have to
live with.
Second, to your question why this doesn't apply to ArmVirtPkg even
though we do have virtio-pci there (ie. PCI enumeration that influences
QEMU's ACPI payload). The answer is: because there is no S3, and
therefore no related drivers, in ArmVirtPkg.
If you consider the dependency chain I described in the above comment,
it says
the preparation of S3 system information [...] has a hard dependency
on the presence of the FACS ACPI table
This is not true for ArmVirtPkg -- EndOfDxe doesn't trigger S3 stuff in
ArmVirtPkg, simply because S3 stuff *does not exist* there. So we can
kick EndOfDxe first, then do the PCI enumeration, then download the ACPI
tables (with the updated, PCI resource allocation-dependent contents) last.
Thanks
Laszlo
>
> /
> Leif
>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Ard Biesheuvel <ard.biesheu...@linaro.org>
>> ---
>> ArmPlatformPkg/Library/PlatformIntelBdsLib/IntelBdsPlatform.c | 36
>> ++++++++++++++++++++
>> ArmPlatformPkg/Library/PlatformIntelBdsLib/IntelBdsPlatform.h | 1 +
>> ArmPlatformPkg/Library/PlatformIntelBdsLib/PlatformIntelBdsLib.inf | 1 +
>> 3 files changed, 38 insertions(+)
>>
>> diff --git a/ArmPlatformPkg/Library/PlatformIntelBdsLib/IntelBdsPlatform.c
>> b/ArmPlatformPkg/Library/PlatformIntelBdsLib/IntelBdsPlatform.c
>> index 98d5b277a636..e27e6d66df91 100644
>> --- a/ArmPlatformPkg/Library/PlatformIntelBdsLib/IntelBdsPlatform.c
>> +++ b/ArmPlatformPkg/Library/PlatformIntelBdsLib/IntelBdsPlatform.c
>> @@ -31,6 +31,24 @@ PlatformIntelBdsConstructor (
>> return EFI_SUCCESS;
>> }
>>
>> +/**
>> + 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
>> //
>> @@ -45,6 +63,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);
>> + }
>> }
>>
>> STATIC
>> diff --git a/ArmPlatformPkg/Library/PlatformIntelBdsLib/IntelBdsPlatform.h
>> b/ArmPlatformPkg/Library/PlatformIntelBdsLib/IntelBdsPlatform.h
>> index 7122d58be7d7..da428288fb9f 100644
>> --- a/ArmPlatformPkg/Library/PlatformIntelBdsLib/IntelBdsPlatform.h
>> +++ b/ArmPlatformPkg/Library/PlatformIntelBdsLib/IntelBdsPlatform.h
>> @@ -30,5 +30,6 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER
>> EXPRESS OR IMPLIED.
>> #include <Library/PlatformBdsLib.h>
>>
>> #include <Guid/GlobalVariable.h>
>> +#include <Guid/EventGroup.h>
>>
>> #endif // _INTEL_BDS_PLATFORM_H
>> diff --git
>> a/ArmPlatformPkg/Library/PlatformIntelBdsLib/PlatformIntelBdsLib.inf
>> b/ArmPlatformPkg/Library/PlatformIntelBdsLib/PlatformIntelBdsLib.inf
>> index a18c5ea71f70..39df113288d2 100644
>> --- a/ArmPlatformPkg/Library/PlatformIntelBdsLib/PlatformIntelBdsLib.inf
>> +++ b/ArmPlatformPkg/Library/PlatformIntelBdsLib/PlatformIntelBdsLib.inf
>> @@ -53,6 +53,7 @@ [LibraryClasses]
>>
>> [Guids]
>> gArmGlobalVariableGuid
>> + gEfiEndOfDxeEventGroupGuid
>>
>> [Pcd]
>> gArmPlatformTokenSpaceGuid.PcdDefaultConInPaths
>> --
>> 1.9.1
>>
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
