|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [edk2] [PATCH] OvmfPkg: AcpiPlatformDxe: PCI enumeration may be disabled
On 2015-02-14 08:38:37, Laszlo Ersek wrote:
> On 02/12/15 21:53, Jordan Justen wrote:
> > I think gEfiPciEnumerationCompleteProtocolGuid should be installed by
> > MdeModulePkg/Bus/Pci/PciBusDxe, even when PcdPciDisableBusEnumeration
> > is set.
> >
> > Ray's main feedback seemed to be that we need to make sure PciBusDxe
> > only installs the protocol once. (I'll also reply to the other related
> > patch thread.)
>
> First, I now agree that this patch of mine should not go in, hence:
>
> Self-NAK
>
> Second, I tend to disagree that that
> gEfiPciEnumerationCompleteProtocolGuid should be installed even if full
> enumeration was eschewed. This might slightly change the de-facto
> meaning of the protocol (because no resource allocation took place).
De-facto seems the correct term here, since the PI1.2 spec says very
little about the protocol.
My interpretation of the protocol is that it is a signal that the EFI
knows about the basic PCI topology, and has installed most PciIo
instances.
Maybe PcdPciDisableBusEnumeration wasn't the best name. Perhaps
PcdPciBusPreEnumerated would have been better.
At any rate, in the case of Xen, the hypervisor has pre-enumerated the
bus. PciBusDxe uses this and 'enumerates' PCI devices by simply
scanning the pre-enumerated topology.
So, I still think PciBusDxe should install
gEfiPciEnumerationCompleteProtocolGuid, because it still seems like it
acurately reflects the phase of the boot process.
Regarding the ACPI tables issue, would a callback for the ReadyToBoot
event work?
-Jordan
> In general I think we should not try to touch
> MdeModulePkg/Bus/Pci/PciBusDxe for our need here.
>
> So let's think again what we need.
>
> We want to delay the download and installation of ACPI tables on virt
> platforms until PCI enumeration is complete, *except* in some cases:
>
> (1) When OVMF runs on Xen.
>
> In that case PcdPciDisableBusEnumeration is TRUE, and the PCI bus driver
> does not install gEfiPciEnumerationCompleteProtocolGuid (in my opinion:
> correctly, because no resource allocation takes place, which is the
> de-facto meaning of the completion protocol).
>
> This means that the depex in
> "OvmfPkg/AcpiPlatformDxe/AcpiPlatformDxe.inf" is no longer correct:
>
> [Depex]
> gEfiAcpiTableProtocolGuid AND gEfiPciEnumerationCompleteProtocolGuid
>
> (2) When the platform in question lacks PCI.
>
> Right now this means ArmVirtualizationQemu. However, that is about to
> change (I've mostly ported PCI support to ArmVirtualizationQemu;
> virtio-pci, USB keyboard, std-VGA work). Importantly, presence of PCI on
> ARM will become a *dynamic* question, dependent on the QEMU version.
> Current master QEMU does not provide PCI hardware for arm/mach-virt, but
> Alex Graf's patches in Peter Maydell's target-arm.next branch do, and so
> will master soon.
>
> This means that the depex in
> "OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpiPlatformDxe.inf" will also break:
>
> [Depex]
> gEfiAcpiTableProtocolGuid
>
> [Depex.IA32, Depex.X64]
> gEfiPciEnumerationCompleteProtocolGuid
>
> because on ARM we might or might not want to wait for enumeration
> completion dependent on whether the DTB ultimately describes a PCI root
> bridge or not.
>
> So here's what I propose. (Again, the above is *all* motivated by
> OvmfPkg/AcpiPlatformDxe/.) In my PCI-for-ArmVirtualizationQemu patchset,
> I will introduce a new protocol GUID (with NULL structure) that will
> simply say "OvmfPkg's AcpiPlatformDxe should not wait for PCI
> enumeration". Then, *both* INF files under OvmfPkg/AcpiPlatformDxe shall
> receive the following depex:
>
> [Depex]
> gEfiAcpiTableProtocolGuid AND
> (gEfiPciEnumerationCompleteProtocolGuid OR
> gOvmfAcpiPlatformNoPciEnumerationProtocolGuid
> )
>
> Then each particular platform driver shall unblock AcpiPlatformDxe in
> its own way:
>
> - OVMF on QEMU will do nothing special, it'll just go with the usual
> gEfiPciEnumerationCompleteProtocolGuid, installed by PciBusDxe.
>
> - OVMF on Xen will install gOvmfAcpiPlatformNoPciEnumerationProtocolGuid
>
> -- Wei, could you post a patch for this later? I think the protocol
> should be installed in XenBusDxeDriverBindingStart(), when it
> succeeds.
>
> It would be probably prudent to coordinate with Ard, wrt. Ard's series
> that brings Xen to ArmVirtualizationQemu.
>
> - In my PCI-for-ArmVirtualizationQemu series, I'll install
> gOvmfAcpiPlatformNoPciEnumerationProtocolGuid in case PCI is
> unavailable on the particular QEMU version.
>
> My main point is, our *real* target is OvmfPkg/AcpiPlatformDxe here, and
> the conditions whether to delay or not to delay its work until PCI
> enumeration is complete are platform dependent and *dynamic*. We should
> let all platform-specific drivers decide for themselves, and we should
> steer clear of drivers that are central to edk2, like PciBusDxe.
>
> What do you guys think?
>
> Thanks!
> Laszlo
>
> >
> > -Jordan
> >
> > On 2015-02-12 04:16:07, Laszlo Ersek wrote:
> >> SVN r16411 delayed ACPI table installation until PCI enumeration was
> >> complete, because on QEMU the ACPI-related fw_cfg files should only be
> >> downloaded after PCI enumeration.
> >>
> >> However, InitializeXen() in "OvmfPkg/PlatformPei/Xen.c" sets
> >> PcdPciDisableBusEnumeration to TRUE. This causes
> >> PciBusDriverBindingStart() in "MdeModulePkg/Bus/Pci/PciBusDxe/PciBus.c" to
> >> set gFullEnumeration to FALSE, which in turn makes PciEnumerator() in
> >> "MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumerator.c" branch to
> >> PciEnumeratorLight(). The installation of
> >> EFI_PCI_ENUMERATION_COMPLETE_PROTOCOL at the end of PciEnumerator() is not
> >> reached.
> >>
> >> Which means that starting with SVN r16411, AcpiPlatformDxe is never
> >> dispatched on Xen.
> >>
> >> This patch replaces the EFI_PCI_ENUMERATION_COMPLETE_PROTOCOL depex with a
> >> matching protocol registration callback for the PCI enumeration enabled
> >> (ie. QEMU) case. When PCI enumeration is disabled (ie. when running on
> >> Xen), AcpiPlatformDxe doesn't wait for
> >> EFI_PCI_ENUMERATION_COMPLETE_PROTOCOL.
> >>
> >> Contributed-under: TianoCore Contribution Agreement 1.0
> >> Signed-off-by: Laszlo Ersek <lersek@xxxxxxxxxx>
> >> ---
> >> OvmfPkg/AcpiPlatformDxe/AcpiPlatformDxe.inf | 4 +-
> >> OvmfPkg/AcpiPlatformDxe/AcpiPlatform.c | 84
> >> +++++++++++++++++++++++------
> >> 2 files changed, 72 insertions(+), 16 deletions(-)
> >>
> >> diff --git a/OvmfPkg/AcpiPlatformDxe/AcpiPlatformDxe.inf
> >> b/OvmfPkg/AcpiPlatformDxe/AcpiPlatformDxe.inf
> >> index 53292bf..6b2c9d2 100644
> >> --- a/OvmfPkg/AcpiPlatformDxe/AcpiPlatformDxe.inf
> >> +++ b/OvmfPkg/AcpiPlatformDxe/AcpiPlatformDxe.inf
> >> @@ -56,16 +56,18 @@
> >>
> >> [Protocols]
> >> gEfiAcpiTableProtocolGuid # PROTOCOL ALWAYS_CONSUMED
> >> + gEfiPciEnumerationCompleteProtocolGuid # PROTOCOL
> >> SOMETIMES_CONSUMED
> >>
> >> [Guids]
> >> gEfiXenInfoGuid
> >>
> >> [Pcd]
> >> gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiTableStorageFile
> >> + gEfiMdeModulePkgTokenSpaceGuid.PcdPciDisableBusEnumeration
> >> gUefiCpuPkgTokenSpaceGuid.PcdCpuLocalApicBaseAddress
> >> gPcAtChipsetPkgTokenSpaceGuid.Pcd8259LegacyModeEdgeLevel
> >> gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFdBaseAddress
> >>
> >> [Depex]
> >> - gEfiAcpiTableProtocolGuid AND gEfiPciEnumerationCompleteProtocolGuid
> >> + gEfiAcpiTableProtocolGuid
> >>
> >> diff --git a/OvmfPkg/AcpiPlatformDxe/AcpiPlatform.c
> >> b/OvmfPkg/AcpiPlatformDxe/AcpiPlatform.c
> >> index 11f0ca8..9823eba 100644
> >> --- a/OvmfPkg/AcpiPlatformDxe/AcpiPlatform.c
> >> +++ b/OvmfPkg/AcpiPlatformDxe/AcpiPlatform.c
> >> @@ -12,6 +12,7 @@
> >>
> >> **/
> >>
> >> +#include <Protocol/PciEnumerationComplete.h>
> >> #include "AcpiPlatform.h"
> >>
> >> EFI_STATUS
> >> @@ -221,6 +222,47 @@ FindAcpiTablesInFv (
> >> return EFI_SUCCESS;
> >> }
> >>
> >> +STATIC
> >> +EFI_STATUS
> >> +EFIAPI
> >> +InstallTables (
> >> + VOID
> >> + )
> >> +{
> >> + EFI_STATUS Status;
> >> + EFI_ACPI_TABLE_PROTOCOL *AcpiTable;
> >> +
> >> + Status = gBS->LocateProtocol (&gEfiAcpiTableProtocolGuid,
> >> + NULL /* Registration */, (VOID **)&AcpiTable);
> >> + if (EFI_ERROR (Status)) {
> >> + return EFI_ABORTED;
> >> + }
> >> +
> >> + if (XenDetected ()) {
> >> + Status = InstallXenTables (AcpiTable);
> >> + } else {
> >> + Status = InstallAllQemuLinkedTables (AcpiTable);
> >> + }
> >> +
> >> + if (EFI_ERROR (Status)) {
> >> + Status = FindAcpiTablesInFv (AcpiTable);
> >> + }
> >> +
> >> + return Status;
> >> +}
> >> +
> >> +STATIC
> >> +VOID
> >> +EFIAPI
> >> +OnPciEnumerated (
> >> + IN EFI_EVENT Event,
> >> + IN VOID *Context
> >> + )
> >> +{
> >> + InstallTables ();
> >> + gBS->CloseEvent (Event);
> >> +}
> >> +
> >> /**
> >> Entrypoint of Acpi Platform driver.
> >>
> >> @@ -239,31 +281,43 @@ AcpiPlatformEntryPoint (
> >> IN EFI_SYSTEM_TABLE *SystemTable
> >> )
> >> {
> >> - EFI_STATUS Status;
> >> - EFI_ACPI_TABLE_PROTOCOL *AcpiTable;
> >> + EFI_STATUS Status;
> >> + VOID *Interface;
> >> + EFI_EVENT PciEnumerated;
> >> + VOID *Registration;
> >>
> >> //
> >> - // Find the AcpiTable protocol
> >> + // If PCI enumeration has been disabled, or it has already completed,
> >> install
> >> + // the tables at once, and let the entry point's return code reflect
> >> the full
> >> + // functionality.
> >> //
> >> - Status = gBS->LocateProtocol (
> >> - &gEfiAcpiTableProtocolGuid,
> >> - NULL,
> >> - (VOID**)&AcpiTable
> >> - );
> >> - if (EFI_ERROR (Status)) {
> >> - return EFI_ABORTED;
> >> + if (PcdGetBool (PcdPciDisableBusEnumeration)) {
> >> + return InstallTables ();
> >> }
> >>
> >> - if (XenDetected ()) {
> >> - Status = InstallXenTables (AcpiTable);
> >> - } else {
> >> - Status = InstallAllQemuLinkedTables (AcpiTable);
> >> + Status = gBS->LocateProtocol (&gEfiPciEnumerationCompleteProtocolGuid,
> >> + NULL /* Registration */, &Interface);
> >> + if (!EFI_ERROR (Status)) {
> >> + return InstallTables ();
> >> }
> >> + ASSERT (Status == EFI_NOT_FOUND);
> >>
> >> + //
> >> + // Otherwise, delay the installation until PCI enumeration is complete.
> >> The
> >> + // entry point's return status will only reflect the callback setup.
> >> + //
> >> + Status = gBS->CreateEvent (EVT_NOTIFY_SIGNAL, TPL_CALLBACK,
> >> OnPciEnumerated,
> >> + NULL /* Context */, &PciEnumerated);
> >> if (EFI_ERROR (Status)) {
> >> - Status = FindAcpiTablesInFv (AcpiTable);
> >> + return Status;
> >> }
> >>
> >> + Status = gBS->RegisterProtocolNotify (
> >> + &gEfiPciEnumerationCompleteProtocolGuid, PciEnumerated,
> >> + &Registration);
> >> + if (EFI_ERROR (Status)) {
> >> + gBS->CloseEvent (PciEnumerated);
> >> + }
> >> return Status;
> >> }
> >>
> >> --
> >> 1.8.3.1
> >>
> >>
> >> ------------------------------------------------------------------------------
> >> Dive into the World of Parallel Programming. The Go Parallel Website,
> >> sponsored by Intel and developed in partnership with Slashdot Media, is
> >> your
> >> hub for all things parallel software development, from weekly thought
> >> leadership blogs to news, videos, case studies, tutorials and more. Take a
> >> look and join the conversation now. http://goparallel.sourceforge.net/
> >> _______________________________________________
> >> edk2-devel mailing list
> >> edk2-devel@xxxxxxxxxxxxxxxxxxxxx
> >> https://lists.sourceforge.net/lists/listinfo/edk2-devel
>
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |