On Thu, Sep 03, 2015 at 06:58:11PM +0200, Laszlo Ersek wrote:
> > 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.
OK.
> 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.)
Sure, that makes sense.
> 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.
OK.
Thanks for the detailed reply Laszlo!
Given that - Reviewed-by: Leif Lindholm <leif.lindh...@linaro.org>
> >> 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
