diff mbox

[edk2,wave,1,1/5] OvmfPkg: introduce gRootBusesConnectedProtocolGuid

Message ID 1457959945-29391-2-git-send-email-lersek@redhat.com
State New
Headers show

Commit Message

Laszlo Ersek March 14, 2016, 12:52 p.m. UTC 
QEMU's ACPI table generator can only create meaningful _CRS objects --
apertures -- for the root buses if all of the PCI devices behind those
buses are actively decoding their IO and MMIO resources, at the time of
the firmware fetching the "etc/table-loader" fw_cfg file. This is not a
QEMU error; QEMU follows the definition of BARs (which are meaningless
when decoding is disabled).

Currently we hook up AcpiPlatformDxe to the PCI Bus driver's
gEfiPciEnumerationCompleteProtocolGuid cue. Unfortunately, when the PCI
Bus driver installs this protocol, it's *still* not the right time for
fetching "etc/table-loader": although resources have been allocated and
BARs have been programmed with them, the PCI Bus driver has also cleared
IO and MMIO decoding in the command registers of the devices.

Furthermore, we couldn't reenable IO and MMIO decoding temporarily in our
gEfiPciEnumerationCompleteProtocolGuid callback even if we wanted to,
because at that time the PCI Bus driver has not produced PciIo instances
yet.

Our Platform BDSes are responsible for connecting the root bridges, hence
they know exactly when the PciIo instances become available -- not when
PCI enumeration completes (signaled by the above protocol), but when the
ConnectController() calls return.

This is when our Platform BDSes should explicitly cue in AcpiPlatformDxe.
Then AcpiPlatformDxe can temporarily enable IO and MMIO decoding for all
devices, while it contacts QEMU for the ACPI payload.

This patch introduces the non-standard protocol that we'll use for
unleashing AcpiPlatformDxe from or Platform BDSes.

Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Marcel Apfelbaum <marcel@redhat.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek <lersek@redhat.com>

---
 OvmfPkg/OvmfPkg.dec                           |  1 +
 OvmfPkg/Include/Protocol/RootBusesConnected.h | 33 ++++++++++++++++++++
 2 files changed, 34 insertions(+)

-- 
1.8.3.1


_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Comments

Ard Biesheuvel March 21, 2016, 9:32 a.m. UTC | #1 
On 14 March 2016 at 13:52, Laszlo Ersek <lersek@redhat.com> wrote:
> QEMU's ACPI table generator can only create meaningful _CRS objects --

> apertures -- for the root buses if all of the PCI devices behind those

> buses are actively decoding their IO and MMIO resources, at the time of

> the firmware fetching the "etc/table-loader" fw_cfg file. This is not a

> QEMU error; QEMU follows the definition of BARs (which are meaningless

> when decoding is disabled).

>

> Currently we hook up AcpiPlatformDxe to the PCI Bus driver's

> gEfiPciEnumerationCompleteProtocolGuid cue. Unfortunately, when the PCI

> Bus driver installs this protocol, it's *still* not the right time for

> fetching "etc/table-loader": although resources have been allocated and

> BARs have been programmed with them, the PCI Bus driver has also cleared

> IO and MMIO decoding in the command registers of the devices.

>

> Furthermore, we couldn't reenable IO and MMIO decoding temporarily in our

> gEfiPciEnumerationCompleteProtocolGuid callback even if we wanted to,

> because at that time the PCI Bus driver has not produced PciIo instances

> yet.

>

> Our Platform BDSes are responsible for connecting the root bridges, hence

> they know exactly when the PciIo instances become available -- not when

> PCI enumeration completes (signaled by the above protocol), but when the

> ConnectController() calls return.

>

> This is when our Platform BDSes should explicitly cue in AcpiPlatformDxe.

> Then AcpiPlatformDxe can temporarily enable IO and MMIO decoding for all

> devices, while it contacts QEMU for the ACPI payload.

>

> This patch introduces the non-standard protocol that we'll use for

> unleashing AcpiPlatformDxe from or Platform BDSes.

>

> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>

> Cc: Gerd Hoffmann <kraxel@redhat.com>

> Cc: Jordan Justen <jordan.l.justen@intel.com>

> Cc: Marcel Apfelbaum <marcel@redhat.com>

> Contributed-under: TianoCore Contribution Agreement 1.0

> Signed-off-by: Laszlo Ersek <lersek@redhat.com>

> ---

>  OvmfPkg/OvmfPkg.dec                           |  1 +

>  OvmfPkg/Include/Protocol/RootBusesConnected.h | 33 ++++++++++++++++++++

>  2 files changed, 34 insertions(+)

>

> diff --git a/OvmfPkg/OvmfPkg.dec b/OvmfPkg/OvmfPkg.dec

> index 784542f62368..a7d887ad353c 100644

> --- a/OvmfPkg/OvmfPkg.dec

> +++ b/OvmfPkg/OvmfPkg.dec

> @@ -60,14 +60,15 @@ [Guids]

>    gXenBusRootDeviceGuid           = {0xa732241f, 0x383d, 0x4d9c, {0x8a, 0xe1, 0x8e, 0x09, 0x83, 0x75, 0x89, 0xd7}}

>

>  [Protocols]

>    gVirtioDeviceProtocolGuid       = {0xfa920010, 0x6785, 0x4941, {0xb6, 0xec, 0x49, 0x8c, 0x57, 0x9f, 0x16, 0x0a}}

>    gBlockMmioProtocolGuid          = {0x6b558ce3, 0x69e5, 0x4c67, {0xa6, 0x34, 0xf7, 0xfe, 0x72, 0xad, 0xbe, 0x84}}

>    gXenBusProtocolGuid             = {0x3d3ca290, 0xb9a5, 0x11e3, {0xb7, 0x5d, 0xb8, 0xac, 0x6f, 0x7d, 0x65, 0xe6}}

>    gXenIoProtocolGuid              = {0x6efac84f, 0x0ab0, 0x4747, {0x81, 0xbe, 0x85, 0x55, 0x62, 0x59, 0x04, 0x49}}

> +  gRootBusesConnectedProtocolGuid = {0x24a2d66f, 0xeedd, 0x4086, {0x90, 0x42, 0xf2, 0x6e, 0x47, 0x97, 0xee, 0x69}}

>

>  [PcdsFixedAtBuild]

>    gUefiOvmfPkgTokenSpaceGuid.PcdOvmfPeiMemFvBase|0x0|UINT32|0

>    gUefiOvmfPkgTokenSpaceGuid.PcdOvmfPeiMemFvSize|0x0|UINT32|1

>    gUefiOvmfPkgTokenSpaceGuid.PcdOvmfDxeMemFvBase|0x0|UINT32|0x15

>    gUefiOvmfPkgTokenSpaceGuid.PcdOvmfDxeMemFvSize|0x0|UINT32|0x16

>

> diff --git a/OvmfPkg/Include/Protocol/RootBusesConnected.h b/OvmfPkg/Include/Protocol/RootBusesConnected.h

> new file mode 100644

> index 000000000000..02181cbbcbb8

> --- /dev/null

> +++ b/OvmfPkg/Include/Protocol/RootBusesConnected.h

> @@ -0,0 +1,33 @@

> +/** @file

> +  A non-standard protocol with which BDS indicates that PCI root bridges have

> +  been connected, and PciIo protocol instances have become available.

> +

> +  Note that this differs from the PCI Enumeration Complete Protocol as defined

> +  in the PI 1.1 specification. That protocol is installed by the PCI bus driver

> +  after enumeration and resource allocation have been completed, but before

> +  PciIo protocol instances are created.

> +

> +  Copyright (C) 2016, Red Hat, Inc.

> +

> +  This program and the accompanying materials are licensed and made available

> +  under the terms and conditions of the BSD License which accompanies this

> +  distribution.  The full text of the license may be found at

> +  http://opensource.org/licenses/bsd-license.php

> +

> +  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, WITHOUT

> +  WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.

> +**/

> +

> +#ifndef _ROOT_BUSES_CONNECTED_H_

> +#define _ROOT_BUSES_CONNECTED_H_

> +

> +#define ROOT_BUSES_CONNECTED_PROTOCOL_GUID              \

> +  { 0x24a2d66f,                                         \

> +    0xeedd,                                             \

> +    0x4086,                                             \

> +    { 0x90, 0x42, 0xf2, 0x6e, 0x47, 0x97, 0xee, 0x69 }, \

> +  }

> +

> +extern EFI_GUID gRootBusesConnectedProtocolGuid;

> +

> +#endif


Recently, someone noted that declaring EFI_GUID's like this is not
necessary in EDK2, since AutoGen.h will declare it if the guid is
mentioned in the .inf, and the #define is never referenced either. Do
you have any particular reason to include this .h file regardless?
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Laszlo Ersek March 21, 2016, 9:41 a.m. UTC | #2 
On 03/21/16 10:32, Ard Biesheuvel wrote:
> On 14 March 2016 at 13:52, Laszlo Ersek <lersek@redhat.com> wrote:

>> QEMU's ACPI table generator can only create meaningful _CRS objects --

>> apertures -- for the root buses if all of the PCI devices behind those

>> buses are actively decoding their IO and MMIO resources, at the time of

>> the firmware fetching the "etc/table-loader" fw_cfg file. This is not a

>> QEMU error; QEMU follows the definition of BARs (which are meaningless

>> when decoding is disabled).

>>

>> Currently we hook up AcpiPlatformDxe to the PCI Bus driver's

>> gEfiPciEnumerationCompleteProtocolGuid cue. Unfortunately, when the PCI

>> Bus driver installs this protocol, it's *still* not the right time for

>> fetching "etc/table-loader": although resources have been allocated and

>> BARs have been programmed with them, the PCI Bus driver has also cleared

>> IO and MMIO decoding in the command registers of the devices.

>>

>> Furthermore, we couldn't reenable IO and MMIO decoding temporarily in our

>> gEfiPciEnumerationCompleteProtocolGuid callback even if we wanted to,

>> because at that time the PCI Bus driver has not produced PciIo instances

>> yet.

>>

>> Our Platform BDSes are responsible for connecting the root bridges, hence

>> they know exactly when the PciIo instances become available -- not when

>> PCI enumeration completes (signaled by the above protocol), but when the

>> ConnectController() calls return.

>>

>> This is when our Platform BDSes should explicitly cue in AcpiPlatformDxe.

>> Then AcpiPlatformDxe can temporarily enable IO and MMIO decoding for all

>> devices, while it contacts QEMU for the ACPI payload.

>>

>> This patch introduces the non-standard protocol that we'll use for

>> unleashing AcpiPlatformDxe from or Platform BDSes.

>>

>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>

>> Cc: Gerd Hoffmann <kraxel@redhat.com>

>> Cc: Jordan Justen <jordan.l.justen@intel.com>

>> Cc: Marcel Apfelbaum <marcel@redhat.com>

>> Contributed-under: TianoCore Contribution Agreement 1.0

>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>

>> ---

>>  OvmfPkg/OvmfPkg.dec                           |  1 +

>>  OvmfPkg/Include/Protocol/RootBusesConnected.h | 33 ++++++++++++++++++++

>>  2 files changed, 34 insertions(+)

>>

>> diff --git a/OvmfPkg/OvmfPkg.dec b/OvmfPkg/OvmfPkg.dec

>> index 784542f62368..a7d887ad353c 100644

>> --- a/OvmfPkg/OvmfPkg.dec

>> +++ b/OvmfPkg/OvmfPkg.dec

>> @@ -60,14 +60,15 @@ [Guids]

>>    gXenBusRootDeviceGuid           = {0xa732241f, 0x383d, 0x4d9c, {0x8a, 0xe1, 0x8e, 0x09, 0x83, 0x75, 0x89, 0xd7}}

>>

>>  [Protocols]

>>    gVirtioDeviceProtocolGuid       = {0xfa920010, 0x6785, 0x4941, {0xb6, 0xec, 0x49, 0x8c, 0x57, 0x9f, 0x16, 0x0a}}

>>    gBlockMmioProtocolGuid          = {0x6b558ce3, 0x69e5, 0x4c67, {0xa6, 0x34, 0xf7, 0xfe, 0x72, 0xad, 0xbe, 0x84}}

>>    gXenBusProtocolGuid             = {0x3d3ca290, 0xb9a5, 0x11e3, {0xb7, 0x5d, 0xb8, 0xac, 0x6f, 0x7d, 0x65, 0xe6}}

>>    gXenIoProtocolGuid              = {0x6efac84f, 0x0ab0, 0x4747, {0x81, 0xbe, 0x85, 0x55, 0x62, 0x59, 0x04, 0x49}}

>> +  gRootBusesConnectedProtocolGuid = {0x24a2d66f, 0xeedd, 0x4086, {0x90, 0x42, 0xf2, 0x6e, 0x47, 0x97, 0xee, 0x69}}

>>

>>  [PcdsFixedAtBuild]

>>    gUefiOvmfPkgTokenSpaceGuid.PcdOvmfPeiMemFvBase|0x0|UINT32|0

>>    gUefiOvmfPkgTokenSpaceGuid.PcdOvmfPeiMemFvSize|0x0|UINT32|1

>>    gUefiOvmfPkgTokenSpaceGuid.PcdOvmfDxeMemFvBase|0x0|UINT32|0x15

>>    gUefiOvmfPkgTokenSpaceGuid.PcdOvmfDxeMemFvSize|0x0|UINT32|0x16

>>

>> diff --git a/OvmfPkg/Include/Protocol/RootBusesConnected.h b/OvmfPkg/Include/Protocol/RootBusesConnected.h

>> new file mode 100644

>> index 000000000000..02181cbbcbb8

>> --- /dev/null

>> +++ b/OvmfPkg/Include/Protocol/RootBusesConnected.h

>> @@ -0,0 +1,33 @@

>> +/** @file

>> +  A non-standard protocol with which BDS indicates that PCI root bridges have

>> +  been connected, and PciIo protocol instances have become available.

>> +

>> +  Note that this differs from the PCI Enumeration Complete Protocol as defined

>> +  in the PI 1.1 specification. That protocol is installed by the PCI bus driver

>> +  after enumeration and resource allocation have been completed, but before

>> +  PciIo protocol instances are created.

>> +

>> +  Copyright (C) 2016, Red Hat, Inc.

>> +

>> +  This program and the accompanying materials are licensed and made available

>> +  under the terms and conditions of the BSD License which accompanies this

>> +  distribution.  The full text of the license may be found at

>> +  http://opensource.org/licenses/bsd-license.php

>> +

>> +  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, WITHOUT

>> +  WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.

>> +**/

>> +

>> +#ifndef _ROOT_BUSES_CONNECTED_H_

>> +#define _ROOT_BUSES_CONNECTED_H_

>> +

>> +#define ROOT_BUSES_CONNECTED_PROTOCOL_GUID              \

>> +  { 0x24a2d66f,                                         \

>> +    0xeedd,                                             \

>> +    0x4086,                                             \

>> +    { 0x90, 0x42, 0xf2, 0x6e, 0x47, 0x97, 0xee, 0x69 }, \

>> +  }

>> +

>> +extern EFI_GUID gRootBusesConnectedProtocolGuid;

>> +

>> +#endif

> 

> Recently, someone noted that declaring EFI_GUID's like this is not

> necessary in EDK2, since AutoGen.h will declare it if the guid is

> mentioned in the .inf, and the #define is never referenced either. Do

> you have any particular reason to include this .h file regardless?

> 


Two reasons:
(a) stick with existing practice
(b) macros like ROOT_BUSES_CONNECTED_PROTOCOL_GUID are useful for
    initializing EFI_GUID objects (including GUID objects that are
    members of structures) that have static storage duration (and are
    optionally constant). So the statement "the #define is never
    referenced" cannot be made in general; it is up to client code.

Thanks
Laszlo
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
diff mbox

Patch

diff --git a/OvmfPkg/OvmfPkg.dec b/OvmfPkg/OvmfPkg.dec
index 784542f62368..a7d887ad353c 100644
--- a/OvmfPkg/OvmfPkg.dec
+++ b/OvmfPkg/OvmfPkg.dec
@@ -60,14 +60,15 @@  [Guids]
   gXenBusRootDeviceGuid           = {0xa732241f, 0x383d, 0x4d9c, {0x8a, 0xe1, 0x8e, 0x09, 0x83, 0x75, 0x89, 0xd7}}
 
 [Protocols]
   gVirtioDeviceProtocolGuid       = {0xfa920010, 0x6785, 0x4941, {0xb6, 0xec, 0x49, 0x8c, 0x57, 0x9f, 0x16, 0x0a}}
   gBlockMmioProtocolGuid          = {0x6b558ce3, 0x69e5, 0x4c67, {0xa6, 0x34, 0xf7, 0xfe, 0x72, 0xad, 0xbe, 0x84}}
   gXenBusProtocolGuid             = {0x3d3ca290, 0xb9a5, 0x11e3, {0xb7, 0x5d, 0xb8, 0xac, 0x6f, 0x7d, 0x65, 0xe6}}
   gXenIoProtocolGuid              = {0x6efac84f, 0x0ab0, 0x4747, {0x81, 0xbe, 0x85, 0x55, 0x62, 0x59, 0x04, 0x49}}
+  gRootBusesConnectedProtocolGuid = {0x24a2d66f, 0xeedd, 0x4086, {0x90, 0x42, 0xf2, 0x6e, 0x47, 0x97, 0xee, 0x69}}
 
 [PcdsFixedAtBuild]
   gUefiOvmfPkgTokenSpaceGuid.PcdOvmfPeiMemFvBase|0x0|UINT32|0
   gUefiOvmfPkgTokenSpaceGuid.PcdOvmfPeiMemFvSize|0x0|UINT32|1
   gUefiOvmfPkgTokenSpaceGuid.PcdOvmfDxeMemFvBase|0x0|UINT32|0x15
   gUefiOvmfPkgTokenSpaceGuid.PcdOvmfDxeMemFvSize|0x0|UINT32|0x16
 
diff --git a/OvmfPkg/Include/Protocol/RootBusesConnected.h b/OvmfPkg/Include/Protocol/RootBusesConnected.h
new file mode 100644
index 000000000000..02181cbbcbb8
--- /dev/null
+++ b/OvmfPkg/Include/Protocol/RootBusesConnected.h
@@ -0,0 +1,33 @@ 
+/** @file
+  A non-standard protocol with which BDS indicates that PCI root bridges have
+  been connected, and PciIo protocol instances have become available.
+
+  Note that this differs from the PCI Enumeration Complete Protocol as defined
+  in the PI 1.1 specification. That protocol is installed by the PCI bus driver
+  after enumeration and resource allocation have been completed, but before
+  PciIo protocol instances are created.
+
+  Copyright (C) 2016, Red Hat, Inc.
+
+  This program and the accompanying materials are licensed and made available
+  under the terms and conditions of the BSD License which accompanies this
+  distribution.  The full text of the license may be found at
+  http://opensource.org/licenses/bsd-license.php
+
+  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, WITHOUT
+  WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
+**/
+
+#ifndef _ROOT_BUSES_CONNECTED_H_
+#define _ROOT_BUSES_CONNECTED_H_
+
+#define ROOT_BUSES_CONNECTED_PROTOCOL_GUID              \
+  { 0x24a2d66f,                                         \
+    0xeedd,                                             \
+    0x4086,                                             \
+    { 0x90, 0x42, 0xf2, 0x6e, 0x47, 0x97, 0xee, 0x69 }, \
+  }
+
+extern EFI_GUID gRootBusesConnectedProtocolGuid;
+
+#endif