|
[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 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). 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 |