[edk2,wave,1,3/5] ArmVirtPkg: PlatformIntelBdsLib: install gRootBusesConnectedProtocolGuid

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

Commit Message

Laszlo Ersek March 14, 2016, 12:52 p.m.
The explanation is in the patch titled

  OvmfPkg: introduce gRootBusesConnectedProtocolGuid

At this point, this protocol doesn't do anything yet.

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>

---
 ArmVirtPkg/Library/PlatformIntelBdsLib/PlatformIntelBdsLib.inf |  1 +
 ArmVirtPkg/Library/PlatformIntelBdsLib/IntelBdsPlatform.c      | 11 +++++++++++
 2 files changed, 12 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:33 a.m. | #1
On 14 March 2016 at 13:52, Laszlo Ersek <lersek@redhat.com> wrote:
> The explanation is in the patch titled

>

>   OvmfPkg: introduce gRootBusesConnectedProtocolGuid

>

> At this point, this protocol doesn't do anything yet.

>

> 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>

> ---

>  ArmVirtPkg/Library/PlatformIntelBdsLib/PlatformIntelBdsLib.inf |  1 +

>  ArmVirtPkg/Library/PlatformIntelBdsLib/IntelBdsPlatform.c      | 11 +++++++++++

>  2 files changed, 12 insertions(+)

>

> diff --git a/ArmVirtPkg/Library/PlatformIntelBdsLib/PlatformIntelBdsLib.inf b/ArmVirtPkg/Library/PlatformIntelBdsLib/PlatformIntelBdsLib.inf

> index 79ba7b2afbf7..bc0eab100319 100644

> --- a/ArmVirtPkg/Library/PlatformIntelBdsLib/PlatformIntelBdsLib.inf

> +++ b/ArmVirtPkg/Library/PlatformIntelBdsLib/PlatformIntelBdsLib.inf

> @@ -74,7 +74,8 @@ [Guids]

>

>  [Protocols]

>    gEfiDevicePathProtocolGuid

>    gEfiGraphicsOutputProtocolGuid

>    gEfiLoadedImageProtocolGuid

>    gEfiPciRootBridgeIoProtocolGuid

>    gEfiSimpleFileSystemProtocolGuid

> +  gRootBusesConnectedProtocolGuid

> diff --git a/ArmVirtPkg/Library/PlatformIntelBdsLib/IntelBdsPlatform.c b/ArmVirtPkg/Library/PlatformIntelBdsLib/IntelBdsPlatform.c

> index b242a293a103..c8acfb1e86a6 100644

> --- a/ArmVirtPkg/Library/PlatformIntelBdsLib/IntelBdsPlatform.c

> +++ b/ArmVirtPkg/Library/PlatformIntelBdsLib/IntelBdsPlatform.c

> @@ -21,14 +21,15 @@

>  #include <Library/PlatformBdsLib.h>

>  #include <Library/QemuBootOrderLib.h>

>  #include <Protocol/DevicePath.h>

>  #include <Protocol/GraphicsOutput.h>

>  #include <Protocol/PciIo.h>

>  #include <Protocol/PciRootBridgeIo.h>

>  #include <Guid/EventGroup.h>

> +#include <Protocol/RootBusesConnected.h>


As noted in response to 1/5, if you only ever refer to the guid by its
name 'gEfiPciRootBridgeIoProtocolGuid', I don't think this header is
necessary.

>

>  #include "IntelBdsPlatform.h"

>

>  #define DP_NODE_LEN(Type) { (UINT8)sizeof (Type), (UINT8)(sizeof (Type) >> 8) }

>

>

>  #pragma pack (1)

> @@ -390,22 +391,32 @@ EFIAPI

>  PlatformBdsPolicyBehavior (

>    IN LIST_ENTRY                      *DriverOptionList,

>    IN LIST_ENTRY                      *BootOptionList,

>    IN PROCESS_CAPSULES                ProcessCapsules,

>    IN BASEM_MEMORY_TEST               BaseMemoryTest

>    )

>  {

> +  EFI_STATUS Status;

> +

>    //

>    // Locate the PCI root bridges and make the PCI bus driver connect each,

>    // non-recursively. This will produce a number of child handles with PciIo on

>    // them.

>    //

>    FilterAndProcess (&gEfiPciRootBridgeIoProtocolGuid, NULL, Connect);

>

>    //

> +  // Signal the ACPI platform driver that it can download QEMU ACPI tables.

> +  //

> +  Status = gBS->InstallProtocolInterface (&gImageHandle,

> +                  &gRootBusesConnectedProtocolGuid, EFI_NATIVE_INTERFACE,

> +                  NULL);

> +  ASSERT_EFI_ERROR (Status);

> +

> +  //

>    // Find all display class PCI devices (using the handles from the previous

>    // step), and connect them non-recursively. This should produce a number of

>    // child handles with GOPs on them.

>    //

>    FilterAndProcess (&gEfiPciIoProtocolGuid, IsPciDisplay, Connect);

>

>    //

> --

> 1.8.3.1

>

>

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Laszlo Ersek March 21, 2016, 9:44 a.m. | #2
On 03/21/16 10:33, Ard Biesheuvel wrote:
> On 14 March 2016 at 13:52, Laszlo Ersek <lersek@redhat.com> wrote:

>> The explanation is in the patch titled

>>

>>   OvmfPkg: introduce gRootBusesConnectedProtocolGuid

>>

>> At this point, this protocol doesn't do anything yet.

>>

>> 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>

>> ---

>>  ArmVirtPkg/Library/PlatformIntelBdsLib/PlatformIntelBdsLib.inf |  1 +

>>  ArmVirtPkg/Library/PlatformIntelBdsLib/IntelBdsPlatform.c      | 11 +++++++++++

>>  2 files changed, 12 insertions(+)

>>

>> diff --git a/ArmVirtPkg/Library/PlatformIntelBdsLib/PlatformIntelBdsLib.inf b/ArmVirtPkg/Library/PlatformIntelBdsLib/PlatformIntelBdsLib.inf

>> index 79ba7b2afbf7..bc0eab100319 100644

>> --- a/ArmVirtPkg/Library/PlatformIntelBdsLib/PlatformIntelBdsLib.inf

>> +++ b/ArmVirtPkg/Library/PlatformIntelBdsLib/PlatformIntelBdsLib.inf

>> @@ -74,7 +74,8 @@ [Guids]

>>

>>  [Protocols]

>>    gEfiDevicePathProtocolGuid

>>    gEfiGraphicsOutputProtocolGuid

>>    gEfiLoadedImageProtocolGuid

>>    gEfiPciRootBridgeIoProtocolGuid

>>    gEfiSimpleFileSystemProtocolGuid

>> +  gRootBusesConnectedProtocolGuid

>> diff --git a/ArmVirtPkg/Library/PlatformIntelBdsLib/IntelBdsPlatform.c b/ArmVirtPkg/Library/PlatformIntelBdsLib/IntelBdsPlatform.c

>> index b242a293a103..c8acfb1e86a6 100644

>> --- a/ArmVirtPkg/Library/PlatformIntelBdsLib/IntelBdsPlatform.c

>> +++ b/ArmVirtPkg/Library/PlatformIntelBdsLib/IntelBdsPlatform.c

>> @@ -21,14 +21,15 @@

>>  #include <Library/PlatformBdsLib.h>

>>  #include <Library/QemuBootOrderLib.h>

>>  #include <Protocol/DevicePath.h>

>>  #include <Protocol/GraphicsOutput.h>

>>  #include <Protocol/PciIo.h>

>>  #include <Protocol/PciRootBridgeIo.h>

>>  #include <Guid/EventGroup.h>

>> +#include <Protocol/RootBusesConnected.h>

> 

> As noted in response to 1/5, if you only ever refer to the guid by its

> name 'gEfiPciRootBridgeIoProtocolGuid', I don't think this header is

> necessary.


I think the argument you cited for 1/5 is not correct.

Even if it was, it would break away from existing practice. So I'd like
to see a reference to that argument (and to the agreement that edk2 in
general will adopt that pattern going forward).

Thanks!
Laszlo

> 

>>

>>  #include "IntelBdsPlatform.h"

>>

>>  #define DP_NODE_LEN(Type) { (UINT8)sizeof (Type), (UINT8)(sizeof (Type) >> 8) }

>>

>>

>>  #pragma pack (1)

>> @@ -390,22 +391,32 @@ EFIAPI

>>  PlatformBdsPolicyBehavior (

>>    IN LIST_ENTRY                      *DriverOptionList,

>>    IN LIST_ENTRY                      *BootOptionList,

>>    IN PROCESS_CAPSULES                ProcessCapsules,

>>    IN BASEM_MEMORY_TEST               BaseMemoryTest

>>    )

>>  {

>> +  EFI_STATUS Status;

>> +

>>    //

>>    // Locate the PCI root bridges and make the PCI bus driver connect each,

>>    // non-recursively. This will produce a number of child handles with PciIo on

>>    // them.

>>    //

>>    FilterAndProcess (&gEfiPciRootBridgeIoProtocolGuid, NULL, Connect);

>>

>>    //

>> +  // Signal the ACPI platform driver that it can download QEMU ACPI tables.

>> +  //

>> +  Status = gBS->InstallProtocolInterface (&gImageHandle,

>> +                  &gRootBusesConnectedProtocolGuid, EFI_NATIVE_INTERFACE,

>> +                  NULL);

>> +  ASSERT_EFI_ERROR (Status);

>> +

>> +  //

>>    // Find all display class PCI devices (using the handles from the previous

>>    // step), and connect them non-recursively. This should produce a number of

>>    // child handles with GOPs on them.

>>    //

>>    FilterAndProcess (&gEfiPciIoProtocolGuid, IsPciDisplay, Connect);

>>

>>    //

>> --

>> 1.8.3.1

>>

>>


_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Ard Biesheuvel March 21, 2016, 9:58 a.m. | #3
On 21 March 2016 at 10:44, Laszlo Ersek <lersek@redhat.com> wrote:
> On 03/21/16 10:33, Ard Biesheuvel wrote:

>> On 14 March 2016 at 13:52, Laszlo Ersek <lersek@redhat.com> wrote:

>>> The explanation is in the patch titled

>>>

>>>   OvmfPkg: introduce gRootBusesConnectedProtocolGuid

>>>

>>> At this point, this protocol doesn't do anything yet.

>>>

>>> 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>

>>> ---

>>>  ArmVirtPkg/Library/PlatformIntelBdsLib/PlatformIntelBdsLib.inf |  1 +

>>>  ArmVirtPkg/Library/PlatformIntelBdsLib/IntelBdsPlatform.c      | 11 +++++++++++

>>>  2 files changed, 12 insertions(+)

>>>

>>> diff --git a/ArmVirtPkg/Library/PlatformIntelBdsLib/PlatformIntelBdsLib.inf b/ArmVirtPkg/Library/PlatformIntelBdsLib/PlatformIntelBdsLib.inf

>>> index 79ba7b2afbf7..bc0eab100319 100644

>>> --- a/ArmVirtPkg/Library/PlatformIntelBdsLib/PlatformIntelBdsLib.inf

>>> +++ b/ArmVirtPkg/Library/PlatformIntelBdsLib/PlatformIntelBdsLib.inf

>>> @@ -74,7 +74,8 @@ [Guids]

>>>

>>>  [Protocols]

>>>    gEfiDevicePathProtocolGuid

>>>    gEfiGraphicsOutputProtocolGuid

>>>    gEfiLoadedImageProtocolGuid

>>>    gEfiPciRootBridgeIoProtocolGuid

>>>    gEfiSimpleFileSystemProtocolGuid

>>> +  gRootBusesConnectedProtocolGuid

>>> diff --git a/ArmVirtPkg/Library/PlatformIntelBdsLib/IntelBdsPlatform.c b/ArmVirtPkg/Library/PlatformIntelBdsLib/IntelBdsPlatform.c

>>> index b242a293a103..c8acfb1e86a6 100644

>>> --- a/ArmVirtPkg/Library/PlatformIntelBdsLib/IntelBdsPlatform.c

>>> +++ b/ArmVirtPkg/Library/PlatformIntelBdsLib/IntelBdsPlatform.c

>>> @@ -21,14 +21,15 @@

>>>  #include <Library/PlatformBdsLib.h>

>>>  #include <Library/QemuBootOrderLib.h>

>>>  #include <Protocol/DevicePath.h>

>>>  #include <Protocol/GraphicsOutput.h>

>>>  #include <Protocol/PciIo.h>

>>>  #include <Protocol/PciRootBridgeIo.h>

>>>  #include <Guid/EventGroup.h>

>>> +#include <Protocol/RootBusesConnected.h>

>>

>> As noted in response to 1/5, if you only ever refer to the guid by its

>> name 'gEfiPciRootBridgeIoProtocolGuid', I don't think this header is

>> necessary.

>

> I think the argument you cited for 1/5 is not correct.

>

> Even if it was, it would break away from existing practice. So I'd like

> to see a reference to that argument (and to the agreement that edk2 in

> general will adopt that pattern going forward).

>


I don't care deeply about this, so I am not going to debate this any
further. IIRC it was mentioned to me in a session between some ARM
engineers, myself and Eugene Cohem, whom I think was the person
mentioning this (I could be wrong, though)

In general, including the file allows you to get away with using these
GUIDs without declaring your dependency upon them in the .INF. As to
your point regarding static initializers, I suppose that indeed still
required a header file of the sort you are introducing here. So my
point pertains to the extern symbol declaration only, which is
arguably redundant under EDK2.

But by all means, let's decouple this from the review of this series.

-- 
Ard.
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Laszlo Ersek March 21, 2016, 10:19 a.m. | #4
On 03/21/16 10:58, Ard Biesheuvel wrote:
> On 21 March 2016 at 10:44, Laszlo Ersek <lersek@redhat.com> wrote:

>> On 03/21/16 10:33, Ard Biesheuvel wrote:

>>> On 14 March 2016 at 13:52, Laszlo Ersek <lersek@redhat.com> wrote:

>>>> The explanation is in the patch titled

>>>>

>>>>   OvmfPkg: introduce gRootBusesConnectedProtocolGuid

>>>>

>>>> At this point, this protocol doesn't do anything yet.

>>>>

>>>> 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>

>>>> ---

>>>>  ArmVirtPkg/Library/PlatformIntelBdsLib/PlatformIntelBdsLib.inf |  1 +

>>>>  ArmVirtPkg/Library/PlatformIntelBdsLib/IntelBdsPlatform.c      | 11 +++++++++++

>>>>  2 files changed, 12 insertions(+)

>>>>

>>>> diff --git a/ArmVirtPkg/Library/PlatformIntelBdsLib/PlatformIntelBdsLib.inf b/ArmVirtPkg/Library/PlatformIntelBdsLib/PlatformIntelBdsLib.inf

>>>> index 79ba7b2afbf7..bc0eab100319 100644

>>>> --- a/ArmVirtPkg/Library/PlatformIntelBdsLib/PlatformIntelBdsLib.inf

>>>> +++ b/ArmVirtPkg/Library/PlatformIntelBdsLib/PlatformIntelBdsLib.inf

>>>> @@ -74,7 +74,8 @@ [Guids]

>>>>

>>>>  [Protocols]

>>>>    gEfiDevicePathProtocolGuid

>>>>    gEfiGraphicsOutputProtocolGuid

>>>>    gEfiLoadedImageProtocolGuid

>>>>    gEfiPciRootBridgeIoProtocolGuid

>>>>    gEfiSimpleFileSystemProtocolGuid

>>>> +  gRootBusesConnectedProtocolGuid

>>>> diff --git a/ArmVirtPkg/Library/PlatformIntelBdsLib/IntelBdsPlatform.c b/ArmVirtPkg/Library/PlatformIntelBdsLib/IntelBdsPlatform.c

>>>> index b242a293a103..c8acfb1e86a6 100644

>>>> --- a/ArmVirtPkg/Library/PlatformIntelBdsLib/IntelBdsPlatform.c

>>>> +++ b/ArmVirtPkg/Library/PlatformIntelBdsLib/IntelBdsPlatform.c

>>>> @@ -21,14 +21,15 @@

>>>>  #include <Library/PlatformBdsLib.h>

>>>>  #include <Library/QemuBootOrderLib.h>

>>>>  #include <Protocol/DevicePath.h>

>>>>  #include <Protocol/GraphicsOutput.h>

>>>>  #include <Protocol/PciIo.h>

>>>>  #include <Protocol/PciRootBridgeIo.h>

>>>>  #include <Guid/EventGroup.h>

>>>> +#include <Protocol/RootBusesConnected.h>

>>>

>>> As noted in response to 1/5, if you only ever refer to the guid by its

>>> name 'gEfiPciRootBridgeIoProtocolGuid', I don't think this header is

>>> necessary.

>>

>> I think the argument you cited for 1/5 is not correct.

>>

>> Even if it was, it would break away from existing practice. So I'd like

>> to see a reference to that argument (and to the agreement that edk2 in

>> general will adopt that pattern going forward).

>>

> 

> I don't care deeply about this, so I am not going to debate this any

> further. IIRC it was mentioned to me in a session between some ARM

> engineers, myself and Eugene Cohem, whom I think was the person

> mentioning this (I could be wrong, though)

> 

> In general, including the file allows you to get away with using these

> GUIDs without declaring your dependency upon them in the .INF.


That's not correct. Including the header will give you only the
declaration; it won't give you the definition, and linking the module
will fail. The definition is in "AutoGen.c", and depends on listing the
protocol or the GUID in the INF file.

> As to

> your point regarding static initializers, I suppose that indeed still

> required a header file of the sort you are introducing here. So my

> point pertains to the extern symbol declaration only, which is

> arguably redundant under EDK2.


After checking an AutoGen.h file, I sort of agree.

"Sort of", because I also checked "Source/Python/AutoGen/GenC.py". It
has code like this:

    if Info.AutoGenVersion >= 0x00010005:
        CreateGuidDefinitionCode(Info, AutoGenC, AutoGenH)
        CreateProtocolDefinitionCode(Info, AutoGenC, AutoGenH)

If we say that our INF files are universally >= 0x00010005, then I agree
there's no reason to include the extern declaration in the protocol
header file.

> But by all means, let's decouple this from the review of this series.


Thanks. Two recent Protocol/*.h additions were a48d0a3d72f9 and
78741ce91e12, those also added the extern declarations. I agree the
declarations shouldn't be duplicated, but then we should all start
following this new rule.

Thanks!
Laszlo
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Laszlo Ersek March 22, 2016, 7:10 a.m. | #5
On 03/22/16 06:59, Jordan Justen wrote:
> Why not a guid based event, similar to gIdleLoopEventGuid?


Two reasons:

- specifically, it keeps the current structure of the AcpiPlatformDxe
driver (i.e., we stick to the way the PCI bus driver installs
gEfiPciEnumerationCompleteProtocolGuid, we just use a different GUID and
tie it to a different circumstance)

- (I vaguely think I might have mentioned this earlier) generally, I
prefer protocol installations to events, in this kind of situation. The
protocol stays there permanently, and in general it allows the
"consuming" driver to load & start after the protocol is installed.

The generic reason doesn't apply in this case, but I preferred to stick
with the pattern. Do you have a strong reason to dislike it?

Thanks
Laszlo

> On 2016-03-21 03:19:06, Laszlo Ersek wrote:

>> On 03/21/16 10:58, Ard Biesheuvel wrote:

>>

>>> But by all means, let's decouple this from the review of this series.

>>

>> Thanks. Two recent Protocol/*.h additions were a48d0a3d72f9 and

>> 78741ce91e12, those also added the extern declarations. I agree the

>> declarations shouldn't be duplicated, but then we should all start

>> following this new rule.

>>

> 

> Maybe one of you should post an email with a more noticable subject to

> prompt discussion of this topic more globally on edk2-devel?

> 

> -Jordan

> 


_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Laszlo Ersek March 22, 2016, 10:20 a.m. | #6
Adding Mike, Liming & Jeff

On 03/22/16 08:10, Laszlo Ersek wrote:
> On 03/22/16 06:59, Jordan Justen wrote:

>> Why not a guid based event, similar to gIdleLoopEventGuid?

> 

> Two reasons:

> 

> - specifically, it keeps the current structure of the AcpiPlatformDxe

> driver (i.e., we stick to the way the PCI bus driver installs

> gEfiPciEnumerationCompleteProtocolGuid, we just use a different GUID and

> tie it to a different circumstance)

> 

> - (I vaguely think I might have mentioned this earlier) generally, I

> prefer protocol installations to events, in this kind of situation. The

> protocol stays there permanently, and in general it allows the

> "consuming" driver to load & start after the protocol is installed.

> 

> The generic reason doesn't apply in this case, but I preferred to stick

> with the pattern. Do you have a strong reason to dislike it?


By the way: I seem to recall your earlier idea about abstracting the
signaling of event groups into UefiLib.

Investigating the core UefiLib instance
("MdePkg/Library/UefiLib/UefiLib.c"), I find that this kind of
functionality exists already. See the utility functions
EfiNamedEventListen() and EfiNamedEventSignal() especially -- the latter
takes only an EFI_GUID, so the function signature seems just right.

However, these functions are not implemented with event groups; instead
they too are based on RegisterProtocolNotify() and
InstallProtocolInterface(). I'm unsure about the reason.

I envisage an uphill battle for coming up with a new pair of functions
- for the same UefiLib class & core instance,
- with identical signaling functionality but different implementation
  (= event groups),
- with names clear enough that would prevent the client code programmer
  from mixing up the two implementations (like creating the listener
  with RegisterProtocolNotify() and trying to signal it with
  SignalEvent()).

For our use case specifically, the listener function wouldn't even be
useful. All we'd need is: create an event in the specified event group,
with an empty callback function; signal the event; close the event.

If this functionality is welcome to UefiLib (i.e., to the class and the
two existing instances in the tree), I'm willing to implement it, and to
rebase this series on top. (It would result in several more patches
because the current signaling of EndOfDxe in the OvmfPkg and ArmVirtPkg
PlatformBdsLib instances should be rebased as well.)

However, I'll need buy-in from the UefiLib maintainers (Mike, Liming,
Jeff) *in advance*. I have no capacity to rework this series only to see
the first patches (= for MdePkg and IntelFrameworkPkg) be rejected or
drag on forever.

Again, the entire functionality that would be abstracted into UefiLib, is:

EFI_STATUS
EFIAPI
EfiEventGroupSignal (
  IN CONST EFI_GUID *EventGroupGuid
  )
{
  EFI_STATUS Status;
  EFI_EVENT  Event;

  Status = gBS->CreateEventEx (
                  EVT_NOTIFY_SIGNAL,
                  TPL_CALLBACK,
                  InternalEmptyFunction,
                  NULL,                  // NotifyContext
                  EventGroupGuid,
                  &Event);
  if (EFI_ERROR (Status)) {
    return Status;
  }
  Status = gBS->SignalEvent (Event);
  gBS->CloseEvent (Event);
  return Status;
}

Thanks
Laszlo
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Laszlo Ersek March 22, 2016, 5:39 p.m. | #7
On 03/22/16 06:59, Jordan Justen wrote:
> Why not a guid based event, similar to gIdleLoopEventGuid?


I posted a prereq series for this:

  [edk2] [PATCH 0/6] introduce EfiEventGroupSignal(),
                     rebase ArmVirtPkg and OvmfPkg
  http://thread.gmane.org/gmane.comp.bios.edk2.devel/9579

If it is "warmly received", I will do what you suggest.

Otherwise I would like to stick with the non-standard protocol in this
series. Installing a protocol interface in BDS is trivial (1 or 2
lines); signaling an event group is much more busywork, especially
considering that we're already doing that for End-of-DXE. So, with the
above pre-req series in place, implementing your idea here would improve
things; without the pre-req series it would duplicate more code.

Thanks!
Laszlo

> On 2016-03-21 03:19:06, Laszlo Ersek wrote:

>> On 03/21/16 10:58, Ard Biesheuvel wrote:

>>

>>> But by all means, let's decouple this from the review of this series.

>>

>> Thanks. Two recent Protocol/*.h additions were a48d0a3d72f9 and

>> 78741ce91e12, those also added the extern declarations. I agree the

>> declarations shouldn't be duplicated, but then we should all start

>> following this new rule.

>>

> 

> Maybe one of you should post an email with a more noticable subject to

> prompt discussion of this topic more globally on edk2-devel?

> 

> -Jordan

> _______________________________________________

> edk2-devel mailing list

> edk2-devel@lists.01.org

> https://lists.01.org/mailman/listinfo/edk2-devel

> 


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

Patch

diff --git a/ArmVirtPkg/Library/PlatformIntelBdsLib/PlatformIntelBdsLib.inf b/ArmVirtPkg/Library/PlatformIntelBdsLib/PlatformIntelBdsLib.inf
index 79ba7b2afbf7..bc0eab100319 100644
--- a/ArmVirtPkg/Library/PlatformIntelBdsLib/PlatformIntelBdsLib.inf
+++ b/ArmVirtPkg/Library/PlatformIntelBdsLib/PlatformIntelBdsLib.inf
@@ -74,7 +74,8 @@  [Guids]
 
 [Protocols]
   gEfiDevicePathProtocolGuid
   gEfiGraphicsOutputProtocolGuid
   gEfiLoadedImageProtocolGuid
   gEfiPciRootBridgeIoProtocolGuid
   gEfiSimpleFileSystemProtocolGuid
+  gRootBusesConnectedProtocolGuid
diff --git a/ArmVirtPkg/Library/PlatformIntelBdsLib/IntelBdsPlatform.c b/ArmVirtPkg/Library/PlatformIntelBdsLib/IntelBdsPlatform.c
index b242a293a103..c8acfb1e86a6 100644
--- a/ArmVirtPkg/Library/PlatformIntelBdsLib/IntelBdsPlatform.c
+++ b/ArmVirtPkg/Library/PlatformIntelBdsLib/IntelBdsPlatform.c
@@ -21,14 +21,15 @@ 
 #include <Library/PlatformBdsLib.h>
 #include <Library/QemuBootOrderLib.h>
 #include <Protocol/DevicePath.h>
 #include <Protocol/GraphicsOutput.h>
 #include <Protocol/PciIo.h>
 #include <Protocol/PciRootBridgeIo.h>
 #include <Guid/EventGroup.h>
+#include <Protocol/RootBusesConnected.h>
 
 #include "IntelBdsPlatform.h"
 
 #define DP_NODE_LEN(Type) { (UINT8)sizeof (Type), (UINT8)(sizeof (Type) >> 8) }
 
 
 #pragma pack (1)
@@ -390,22 +391,32 @@  EFIAPI
 PlatformBdsPolicyBehavior (
   IN LIST_ENTRY                      *DriverOptionList,
   IN LIST_ENTRY                      *BootOptionList,
   IN PROCESS_CAPSULES                ProcessCapsules,
   IN BASEM_MEMORY_TEST               BaseMemoryTest
   )
 {
+  EFI_STATUS Status;
+
   //
   // Locate the PCI root bridges and make the PCI bus driver connect each,
   // non-recursively. This will produce a number of child handles with PciIo on
   // them.
   //
   FilterAndProcess (&gEfiPciRootBridgeIoProtocolGuid, NULL, Connect);
 
   //
+  // Signal the ACPI platform driver that it can download QEMU ACPI tables.
+  //
+  Status = gBS->InstallProtocolInterface (&gImageHandle,
+                  &gRootBusesConnectedProtocolGuid, EFI_NATIVE_INTERFACE,
+                  NULL);
+  ASSERT_EFI_ERROR (Status);
+
+  //
   // Find all display class PCI devices (using the handles from the previous
   // step), and connect them non-recursively. This should produce a number of
   // child handles with GOPs on them.
   //
   FilterAndProcess (&gEfiPciIoProtocolGuid, IsPciDisplay, Connect);
 
   //