[PATCH v1 1/1] ArmPkg: Dispatch deferred images after EndOfDxe

[PATCH v1 1/1] ArmPkg: Dispatch deferred images after EndOfDxe

[Sami Mujawar]

Third party driver images loaded from Option ROM get queued
for execution after EndOfDxe. These queued images need to be
dispatched from the PlatformBootManagerLib.

Since the queued images were not dispatched, the PCI Option
ROM drivers were not getting loaded on Juno. Therefore,
add call to EfiBootManagerDispatchDeferredImages() for
dispatching deferred images from PlatformBootManagerLib.

Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>
---

The changes can be seen at https://github.com/samimujawar/edk2/tree/527_option_rom_loading_v1

 ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c b/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c
index 0f91692c1a5ee6104bfef8545e4f436e53042178..71b857b5ba884c27ab870f6b75fa3e34d48e6060 100644
--- a/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c
+++ b/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c
@@ -2,7 +2,7 @@
   Implementation for PlatformBootManagerLib library class interfaces.
 
   Copyright (C) 2015-2016, Red Hat, Inc.
-  Copyright (c) 2014, ARM Ltd. All rights reserved.<BR>
+  Copyright (c) 2014 - 2019, ARM Ltd. All rights reserved.<BR>
   Copyright (c) 2004 - 2018, Intel Corporation. All rights reserved.<BR>
   Copyright (c) 2016, Linaro Ltd. All rights reserved.<BR>
 
@@ -554,6 +554,11 @@ PlatformBootManagerBeforeConsole (
   EfiEventGroupSignal (&gEfiEndOfDxeEventGroupGuid);
 
   //
+  // Dispatch deferred images after EndOfDxe event.
+  //
+  EfiBootManagerDispatchDeferredImages ();
+
+  //
   // 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.
-- 
'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'

Re: [PATCH v1 1/1] ArmPkg: Dispatch deferred images after EndOfDxe

[Ard Biesheuvel]

On Wed, 1 May 2019 at 16:02, Sami Mujawar <sami.mujawar@arm.com> wrote:
>
> Third party driver images loaded from Option ROM get queued
> for execution after EndOfDxe. These queued images need to be
> dispatched from the PlatformBootManagerLib.
>
> Since the queued images were not dispatched, the PCI Option
> ROM drivers were not getting loaded on Juno. Therefore,
> add call to EfiBootManagerDispatchDeferredImages() for
> dispatching deferred images from PlatformBootManagerLib.
>
> Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>

Hello Sami,

The patch looks correct to me, but I am puzzled why this is broken on
Juno but not on SynQuacer or Overdrive.

It might be that the PCI hierarchy is enumerated before EndOfDxe on
Juno, and after on the other platforms. Any clue?


> ---
>
> The changes can be seen at https://github.com/samimujawar/edk2/tree/527_option_rom_loading_v1
>
>  ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c b/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c
> index 0f91692c1a5ee6104bfef8545e4f436e53042178..71b857b5ba884c27ab870f6b75fa3e34d48e6060 100644
> --- a/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c
> +++ b/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c
> @@ -2,7 +2,7 @@
>    Implementation for PlatformBootManagerLib library class interfaces.
>
>    Copyright (C) 2015-2016, Red Hat, Inc.
> -  Copyright (c) 2014, ARM Ltd. All rights reserved.<BR>
> +  Copyright (c) 2014 - 2019, ARM Ltd. All rights reserved.<BR>
>    Copyright (c) 2004 - 2018, Intel Corporation. All rights reserved.<BR>
>    Copyright (c) 2016, Linaro Ltd. All rights reserved.<BR>
>
> @@ -554,6 +554,11 @@ PlatformBootManagerBeforeConsole (
>    EfiEventGroupSignal (&gEfiEndOfDxeEventGroupGuid);
>
>    //
> +  // Dispatch deferred images after EndOfDxe event.
> +  //
> +  EfiBootManagerDispatchDeferredImages ();
> +
> +  //
>    // 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.
> --
> 'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'
>
>

Re: [PATCH v1 1/1] ArmPkg: Dispatch deferred images after EndOfDxe

[Ard Biesheuvel]

On Wed, 1 May 2019 at 15:02, Sami Mujawar <sami.mujawar@arm.com> wrote:
>
> Third party driver images loaded from Option ROM get queued
> for execution after EndOfDxe. These queued images need to be
> dispatched from the PlatformBootManagerLib.
>
> Since the queued images were not dispatched, the PCI Option
> ROM drivers were not getting loaded on Juno. Therefore,
> add call to EfiBootManagerDispatchDeferredImages() for
> dispatching deferred images from PlatformBootManagerLib.
>
> Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>
> ---
>
> The changes can be seen at https://github.com/samimujawar/edk2/tree/527_option_rom_loading_v1
>
>  ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c b/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c
> index 0f91692c1a5ee6104bfef8545e4f436e53042178..71b857b5ba884c27ab870f6b75fa3e34d48e6060 100644
> --- a/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c
> +++ b/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c
> @@ -2,7 +2,7 @@
>    Implementation for PlatformBootManagerLib library class interfaces.
>
>    Copyright (C) 2015-2016, Red Hat, Inc.
> -  Copyright (c) 2014, ARM Ltd. All rights reserved.<BR>
> +  Copyright (c) 2014 - 2019, ARM Ltd. All rights reserved.<BR>
>    Copyright (c) 2004 - 2018, Intel Corporation. All rights reserved.<BR>
>    Copyright (c) 2016, Linaro Ltd. All rights reserved.<BR>
>
> @@ -554,6 +554,11 @@ PlatformBootManagerBeforeConsole (
>    EfiEventGroupSignal (&gEfiEndOfDxeEventGroupGuid);
>
>    //
> +  // Dispatch deferred images after EndOfDxe event.
> +  //
> +  EfiBootManagerDispatchDeferredImages ();
> +
> +  //
>    // 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.

OK, given that other implementations of PlatformBootManagerLib also
make this call after signalling EndOfDxe, this is obviously the
correct thing to do. I'm still curious why this difference exists, but
it doesn't make sense to block this patch from going in, considering
that I haven't been able to investigate.

So I'll merge this - I just need to figure out how to use the new CI
based workflow :-)

Re: [edk2-devel] [PATCH v1 1/1] ArmPkg: Dispatch deferred images after EndOfDxe

[Laszlo Ersek]

On 12/05/19 21:25, Ard Biesheuvel wrote:
> On Wed, 1 May 2019 at 15:02, Sami Mujawar <sami.mujawar@arm.com> wrote:
>>
>> Third party driver images loaded from Option ROM get queued
>> for execution after EndOfDxe. These queued images need to be
>> dispatched from the PlatformBootManagerLib.
>>
>> Since the queued images were not dispatched, the PCI Option
>> ROM drivers were not getting loaded on Juno. Therefore,
>> add call to EfiBootManagerDispatchDeferredImages() for
>> dispatching deferred images from PlatformBootManagerLib.
>>
>> Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>
>> ---
>>
>> The changes can be seen at https://github.com/samimujawar/edk2/tree/527_option_rom_loading_v1
>>
>>  ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c | 7 ++++++-
>>  1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c b/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c
>> index 0f91692c1a5ee6104bfef8545e4f436e53042178..71b857b5ba884c27ab870f6b75fa3e34d48e6060 100644
>> --- a/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c
>> +++ b/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c
>> @@ -2,7 +2,7 @@
>>    Implementation for PlatformBootManagerLib library class interfaces.
>>
>>    Copyright (C) 2015-2016, Red Hat, Inc.
>> -  Copyright (c) 2014, ARM Ltd. All rights reserved.<BR>
>> +  Copyright (c) 2014 - 2019, ARM Ltd. All rights reserved.<BR>
>>    Copyright (c) 2004 - 2018, Intel Corporation. All rights reserved.<BR>
>>    Copyright (c) 2016, Linaro Ltd. All rights reserved.<BR>
>>
>> @@ -554,6 +554,11 @@ PlatformBootManagerBeforeConsole (
>>    EfiEventGroupSignal (&gEfiEndOfDxeEventGroupGuid);
>>
>>    //
>> +  // Dispatch deferred images after EndOfDxe event.
>> +  //
>> +  EfiBootManagerDispatchDeferredImages ();
>> +
>> +  //
>>    // 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.
>
> OK, given that other implementations of PlatformBootManagerLib also
> make this call after signalling EndOfDxe, this is obviously the
> correct thing to do.

The right sequence of steps is,
- signal EndOfDxe,
- install DxeSmmReadyToLock,
- dispatch deferred images.

The first step gives platform drivers a last chance to do things
- after all services have become available,
- and SMRAM is still open.

The second step locks down SMRAM before 3rd party drivers can mess
around.

(Sorry for the hand-wavy description.)

> I'm still curious why this difference exists,

What difference do you mean?

(I can't see the original patch posting in my list folder, so I could be
missing parts of the discussion thus far.)

One interesting bit that I can see in the code is that EndOfDxe is
signaled before connecting PCI root bridges. That doesn't seem
productive. Until we connect root bridges, PciBusDxe won't bind them,
and won't collect the drivers in the ROM BARs -- they won't be there to
"defer" and "dispatch" later.

Normally, step "connect root bridges" would be *prepended* to the above
three steps. I don't understand where the preexistent disorder comes,
between signaling EndOfDxe, and connecting the root bridges.

... Oh wait. We made a mistake in commit 9cd7d3c5ba44 ("ArmVirtPkg:
signal EndOxDxe event in PlatformBsdInit", 2015-06-25).

In that commit, we signaled EndOfDxe in PlatformBdsInit() -- before
connecting the root bridges at the top of PlatformBdsPolicyBehavior().

(Look at the patch with a huge context, like -U300.)

The BdsEntry() function in BdsDxe would call PlatformBdsInit() before
calling PlatformBdsPolicyBehavior().

So that commit is where the disorder comes from.

The disorder was faithfully preserved through:

- dfc9514794fc ("ArmVirtPkg/PlatformIntelBdsLib: rebase to
                EfiEventGroupSignal", 2016-03-23)
- 8d6203223ab1 ("ArmVirtPkg: duplicate PlatformIntelBdsLib to
                PlatformBootManagerLib", 2016-05-06)
- e3fe3c0ff94d ("ArmVirtPkg/PlatformBootManagerLib: follow
                PlatformBootManagerLib interfaces", 2016-05-06)
- e2a193b733aa ("ArmVirtPkg/PlatformBootManagerLib: init console vars in
                BeforeConsole()", 2016-05-06)

The last commit in the list is where the disorder becomes very visible

Then, the disorder was copied into ArmPkg too, in commit c976f9cb7dbd
("ArmPkg/PlatformBootManagerLib: implement new generic version",
2016-05-12) -- see the PlatformBootManagerBeforeConsole() function.

Later, in commit 34cd940294e5 ("ArmVirPkg/PlatformBds: Dispatch deferred
images after EndOfDxe", 2016-11-10), we added the
EfiBootManagerDispatchDeferredImages() to ArmVirtPkg. However, with the
pre-existent disorder in place (originally from commit 9cd7d3c5ba44, see
above), that call could not be inserted in the 100% correct place.

I mean: during all that time, strictly speaking, it had not been *wrong*
to dispatch the deferred images before connecting PCI root bridges. The
important thing was to delay 3rd party dispatch until after (at least)
EndOfDxe -- that way, platform drivers could "lock down" themselves,
before the platform unleashed 3rd party drivers. So the solution was
correct; it just wasn't *complete* -- because one (maybe the most
important?) source of the deferred images is the PCI oproms.

So that's what we have today in ArmVirtPkg -- and the present patch
seems to be porting 34cd940294e5 to ArmPkg.

The proper solution would be the following (if I may suggest an idea):

(1) fix ArmVirtPkg: hoist the following part:

>   //
>   // 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.
>   //
>   EfiEventGroupSignal (&gRootBridgesConnectedEventGroupGuid);

above this part:

>   //
>   // Signal EndOfDxe PI Event
>   //
>   EfiEventGroupSignal (&gEfiEndOfDxeEventGroupGuid);
>
>   //
>   // Dispatch deferred images after EndOfDxe event.
>   //
>   EfiBootManagerDispatchDeferredImages ();

This will make:

- ArmVirtPkg match the order employed in OvmfPkg,

- EfiBootManagerDispatchDeferredImages() pick up PCI option ROMs,

- run the ACPI platform driver *before* EndOfDxe; but that's just fine.
  (In fact, in OvmfPkg, that's *required*. See the FACS note there.)


(2) Port the same fix to ArmPkg: namely move

>   //
>   // 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);

just above

>   //
>   // Signal EndOfDxe PI Event
>   //
>   EfiEventGroupSignal (&gEfiEndOfDxeEventGroupGuid);


(3) In a separate patch, call EfiBootManagerDispatchDeferredImages()
just after the *new* location of signaling EndOfDxe.


... I think this disorder / bug may have remained dormant so far (ever
since 9cd7d3c5ba44) because in ArmVirt, we have had practically zero use
for option ROMs. With OVMF, there's always physical GPU assignment that
could have triggered the problem quite soon.

So now that you have actual use for oproms on Juno, the disorder was
laid bare. Except... I'm not sure how the *present* patch is supposed to
fix that problem, given that the dispatch of deferred (3rd party) images
would still occur *before* enumerating the PCI option ROMs!

So either the patch (as posted) is functionally wrong, or something is
amiss in my analysis above. :)


> but it doesn't make sense to block this patch from going in,
> considering that I haven't been able to investigate.
>
> So I'll merge this - I just need to figure out how to use the new CI
> based workflow :-)

Short version:

- prepare a local topic branch, on top of master, with the review tags
  in place, such that (per the earlier method) you'd ff-push that topic
  branch as the new edk2 master.

- instead of pushing it to edk2, push it to your personal edk2 repo on
  github.com

- log in to github.com, and initiate a pull request against edk2/master,
  from your personal topic branch

- as soon as the pull request has been filed, locate the "labels" box to
  the right, and apply the "push" label.

Once the CI tests complete, the branch will be merged (fast-forwarded).

Thanks,
Laszlo

>
> 
>

Re: [edk2-devel] [PATCH v1 1/1] ArmPkg: Dispatch deferred images after EndOfDxe

[Laszlo Ersek]

On 12/06/19 00:54, Laszlo Ersek wrote:
> On 12/05/19 21:25, Ard Biesheuvel wrote:
>> On Wed, 1 May 2019 at 15:02, Sami Mujawar <sami.mujawar@arm.com>
>> wrote:

>> I'm still curious why this difference exists,
>
> What difference do you mean?
>
> (I can't see the original patch posting in my list folder, so I could
> be missing parts of the discussion thus far.)

Haha, I missed that Sami's email, which you just replied to, is dated "1
May 2019" -- that's the reason I couldn't find the original posting in
my edk2-devel folder. That message has already been moved into one of my
archive folders :)

But, now I do see it, and I also see that your first question in
response was spot-on:

  https://edk2.groups.io/g/devel/message/39901

(alternative link:

  http://mid.mail-archive.com/CAKv+Gu92gPzGvGZ3M9B52q1TOAvnBjgxpvykbAZPevMULkcF6w@mail.gmail.com
)

Your question there had a small typo -- I think you meant, "It might be
that the PCI hierarchy is enumerated *after* EndOfDxe on Juno, and
*before* on the other platforms."

And yes, that would explain the difference between Juno and {SynQuacer,
Overdrive} very well -- i.e. why Juno was broken and the other platforms
were OK. (Assuming in all cases, the 3rd party dispatch occurred after
EndOfDxe, where it is supposed to.)

Thanks
Laszlo

Re: [edk2-devel] [PATCH v1 1/1] ArmPkg: Dispatch deferred images after EndOfDxe

[Ard Biesheuvel]

On Fri, 6 Dec 2019 at 00:05, Laszlo Ersek <lersek@redhat.com> wrote:
>
> On 12/06/19 00:54, Laszlo Ersek wrote:
> > On 12/05/19 21:25, Ard Biesheuvel wrote:
> >> On Wed, 1 May 2019 at 15:02, Sami Mujawar <sami.mujawar@arm.com>
> >> wrote:
>
> >> I'm still curious why this difference exists,
> >
> > What difference do you mean?
> >
> > (I can't see the original patch posting in my list folder, so I could
> > be missing parts of the discussion thus far.)
>
> Haha, I missed that Sami's email, which you just replied to, is dated "1
> May 2019" -- that's the reason I couldn't find the original posting in
> my edk2-devel folder. That message has already been moved into one of my
> archive folders :)
>
> But, now I do see it, and I also see that your first question in
> response was spot-on:
>
>   https://edk2.groups.io/g/devel/message/39901
>
> (alternative link:
>
>   http://mid.mail-archive.com/CAKv+Gu92gPzGvGZ3M9B52q1TOAvnBjgxpvykbAZPevMULkcF6w@mail.gmail.com
> )
>
> Your question there had a small typo -- I think you meant, "It might be
> that the PCI hierarchy is enumerated *after* EndOfDxe on Juno, and
> *before* on the other platforms."
>
> And yes, that would explain the difference between Juno and {SynQuacer,
> Overdrive} very well -- i.e. why Juno was broken and the other platforms
> were OK. (Assuming in all cases, the 3rd party dispatch occurred after
> EndOfDxe, where it is supposed to.)
>

Hi Laszlo,

Thanks for chiming in.

My question did not have a typo: if Juno enumerates PCI before
EndOfDxe, any option ROMs it encounters will not be dispatched and put
on the deferred list, which never gets processed. If it enumerates
after EndOfDxe, they just get dispatched immediately. That would
explain this behavior, *except* for the fact that these platforms use
the same PCI host bridge and bus drivers, the only difference is the
PciHostBridgeLib used.

The difference is probably explained by the explicit
gBS->ConnectController () connecting the PCI root bridge that takes
place in ArmJunoDxe in an EndOfDxe handler, which seems to be there so
we can set the MAC address on the Marvell Yukon. We should probably
move that to a protocol registration notification callback on the
PciIo protocol, and remove the ConnectController altogether.

Concerning your analysis regarding the order of connecting the PCI
root bridge and signalling EndOfDxe: I agree the OvmfPkg order is
better, since it permits builtin drivers (which are 'trusted') to
attach to devices in the PCIe hierarchy before EndOfDxe is signalled,
which may be useful.

Re: [edk2-devel] [PATCH v1 1/1] ArmPkg: Dispatch deferred images after EndOfDxe

[Laszlo Ersek]

On 12/06/19 11:02, Ard Biesheuvel wrote:
> On Fri, 6 Dec 2019 at 00:05, Laszlo Ersek <lersek@redhat.com> wrote:
>>
>> On 12/06/19 00:54, Laszlo Ersek wrote:
>>> On 12/05/19 21:25, Ard Biesheuvel wrote:
>>>> On Wed, 1 May 2019 at 15:02, Sami Mujawar <sami.mujawar@arm.com>
>>>> wrote:
>>
>>>> I'm still curious why this difference exists,
>>>
>>> What difference do you mean?
>>>
>>> (I can't see the original patch posting in my list folder, so I could
>>> be missing parts of the discussion thus far.)
>>
>> Haha, I missed that Sami's email, which you just replied to, is dated "1
>> May 2019" -- that's the reason I couldn't find the original posting in
>> my edk2-devel folder. That message has already been moved into one of my
>> archive folders :)
>>
>> But, now I do see it, and I also see that your first question in
>> response was spot-on:
>>
>>   https://edk2.groups.io/g/devel/message/39901
>>
>> (alternative link:
>>
>>   http://mid.mail-archive.com/CAKv+Gu92gPzGvGZ3M9B52q1TOAvnBjgxpvykbAZPevMULkcF6w@mail.gmail.com
>> )
>>
>> Your question there had a small typo -- I think you meant, "It might be
>> that the PCI hierarchy is enumerated *after* EndOfDxe on Juno, and
>> *before* on the other platforms."
>>
>> And yes, that would explain the difference between Juno and {SynQuacer,
>> Overdrive} very well -- i.e. why Juno was broken and the other platforms
>> were OK. (Assuming in all cases, the 3rd party dispatch occurred after
>> EndOfDxe, where it is supposed to.)
>>
> 
> Hi Laszlo,
> 
> Thanks for chiming in.
> 
> My question did not have a typo: if Juno enumerates PCI before
> EndOfDxe, any option ROMs it encounters will not be dispatched and put
> on the deferred list, which never gets processed. If it enumerates
> after EndOfDxe, they just get dispatched immediately.

Ugh, indeed!

> That would
> explain this behavior, *except* for the fact that these platforms use
> the same PCI host bridge and bus drivers, the only difference is the
> PciHostBridgeLib used.
> 
> The difference is probably explained by the explicit
> gBS->ConnectController () connecting the PCI root bridge that takes
> place in ArmJunoDxe in an EndOfDxe handler, which seems to be there so
> we can set the MAC address on the Marvell Yukon.

Aha! That does seem to explain why this patch makes a difference.
Because, signaling EndOfDxe leads *internally* to the production of some
PciIo instances, and the deferral of the oprom images found on those. So
there *are* some deferred images to dispatch, in platform BDS, right
after signaling EndOfDxe (even before blanket-enumerating all (other)
root bridges).

So commit 0f9395d7c5cc looks justified, only its message may not tell
the whole story. (I didn't realize oproms could be found and deferred
*inside* signaling EndofDxe, and was looking at the blanket-enumeration
that *followed* the proposed location of
EfiBootManagerDispatchDeferredImages().)

> We should probably
> move that to a protocol registration notification callback on the
> PciIo protocol, and remove the ConnectController altogether.

I think this makes sense. It sounds like a platform tweak, so I agree
that a PciIo notification callback is acceptable (a UEFI driver
following the UEFI driver model doesn't seem to make any sense just for
this one-shot platform tweak).

> Concerning your analysis regarding the order of connecting the PCI
> root bridge and signalling EndOfDxe: I agree the OvmfPkg order is
> better, since it permits builtin drivers (which are 'trusted') to
> attach to devices in the PCIe hierarchy before EndOfDxe is signalled,
> which may be useful.

Thanks -- but, ultimately, I do think my analysis, in the form I meant
it, was flawed. I missed that, if the enumeration occurs after EndOfDxe,
then the PCI oprom images will simply be dispatched at once!

So, I have to change my mind now: I realize & accept that there is no
immediate need to update the order of operations around signaling
EndOfDxe, in the ArmVirtPkg and ArmPks PlatformBootManagerLib instances.

In OvmfPkg, the order is stricter / more intricate , due to a chain of
dependencies around S3 (FACS, boot script opcodes, SMRAM lockbox,
locking down SMRAM etc).

Thank you for the enlightenment! :)
Laszlo

Re: [edk2-devel] [PATCH v1 1/1] ArmPkg: Dispatch deferred images after EndOfDxe

[Ard Biesheuvel]

On Fri, 6 Dec 2019 at 10:33, Laszlo Ersek <lersek@redhat.com> wrote:
>
> On 12/06/19 11:02, Ard Biesheuvel wrote:
> > On Fri, 6 Dec 2019 at 00:05, Laszlo Ersek <lersek@redhat.com> wrote:
> >>
> >> On 12/06/19 00:54, Laszlo Ersek wrote:
> >>> On 12/05/19 21:25, Ard Biesheuvel wrote:
> >>>> On Wed, 1 May 2019 at 15:02, Sami Mujawar <sami.mujawar@arm.com>
> >>>> wrote:
> >>
> >>>> I'm still curious why this difference exists,
> >>>
> >>> What difference do you mean?
> >>>
> >>> (I can't see the original patch posting in my list folder, so I could
> >>> be missing parts of the discussion thus far.)
> >>
> >> Haha, I missed that Sami's email, which you just replied to, is dated "1
> >> May 2019" -- that's the reason I couldn't find the original posting in
> >> my edk2-devel folder. That message has already been moved into one of my
> >> archive folders :)
> >>
> >> But, now I do see it, and I also see that your first question in
> >> response was spot-on:
> >>
> >>   https://edk2.groups.io/g/devel/message/39901
> >>
> >> (alternative link:
> >>
> >>   http://mid.mail-archive.com/CAKv+Gu92gPzGvGZ3M9B52q1TOAvnBjgxpvykbAZPevMULkcF6w@mail.gmail.com
> >> )
> >>
> >> Your question there had a small typo -- I think you meant, "It might be
> >> that the PCI hierarchy is enumerated *after* EndOfDxe on Juno, and
> >> *before* on the other platforms."
> >>
> >> And yes, that would explain the difference between Juno and {SynQuacer,
> >> Overdrive} very well -- i.e. why Juno was broken and the other platforms
> >> were OK. (Assuming in all cases, the 3rd party dispatch occurred after
> >> EndOfDxe, where it is supposed to.)
> >>
> >
> > Hi Laszlo,
> >
> > Thanks for chiming in.
> >
> > My question did not have a typo: if Juno enumerates PCI before
> > EndOfDxe, any option ROMs it encounters will not be dispatched and put
> > on the deferred list, which never gets processed. If it enumerates
> > after EndOfDxe, they just get dispatched immediately.
>
> Ugh, indeed!
>
> > That would
> > explain this behavior, *except* for the fact that these platforms use
> > the same PCI host bridge and bus drivers, the only difference is the
> > PciHostBridgeLib used.
> >
> > The difference is probably explained by the explicit
> > gBS->ConnectController () connecting the PCI root bridge that takes
> > place in ArmJunoDxe in an EndOfDxe handler, which seems to be there so
> > we can set the MAC address on the Marvell Yukon.
>
> Aha! That does seem to explain why this patch makes a difference.
> Because, signaling EndOfDxe leads *internally* to the production of some
> PciIo instances, and the deferral of the oprom images found on those. So
> there *are* some deferred images to dispatch, in platform BDS, right
> after signaling EndOfDxe (even before blanket-enumerating all (other)
> root bridges).
>
> So commit 0f9395d7c5cc looks justified, only its message may not tell
> the whole story. (I didn't realize oproms could be found and deferred
> *inside* signaling EndofDxe, and was looking at the blanket-enumeration
> that *followed* the proposed location of
> EfiBootManagerDispatchDeferredImages().)
>
> > We should probably
> > move that to a protocol registration notification callback on the
> > PciIo protocol, and remove the ConnectController altogether.
>
> I think this makes sense. It sounds like a platform tweak, so I agree
> that a PciIo notification callback is acceptable (a UEFI driver
> following the UEFI driver model doesn't seem to make any sense just for
> this one-shot platform tweak).
>
> > Concerning your analysis regarding the order of connecting the PCI
> > root bridge and signalling EndOfDxe: I agree the OvmfPkg order is
> > better, since it permits builtin drivers (which are 'trusted') to
> > attach to devices in the PCIe hierarchy before EndOfDxe is signalled,
> > which may be useful.
>
> Thanks -- but, ultimately, I do think my analysis, in the form I meant
> it, was flawed. I missed that, if the enumeration occurs after EndOfDxe,
> then the PCI oprom images will simply be dispatched at once!
>
> So, I have to change my mind now: I realize & accept that there is no
> immediate need to update the order of operations around signaling
> EndOfDxe, in the ArmVirtPkg and ArmPks PlatformBootManagerLib instances.
>

Agreed. UEFI drivers shouldn't generalled depend on being able to
connect to the devices before EndOfDxe, and if there is ever a valid
reason for doing so, we can revisit this.

> In OvmfPkg, the order is stricter / more intricate , due to a chain of
> dependencies around S3 (FACS, boot script opcodes, SMRAM lockbox,
> locking down SMRAM etc).
>
> Thank you for the enlightenment! :)

Likewise :-)

Re: [edk2-devel] [PATCH v1 1/1] ArmPkg: Dispatch deferred images after EndOfDxe

[Laszlo Ersek]

On 12/06/19 00:54, Laszlo Ersek wrote:

> The proper solution would be the following (if I may suggest an idea):
> 
> (1) fix ArmVirtPkg: hoist the following part:
> 
>>   //
>>   // 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.
>>   //
>>   EfiEventGroupSignal (&gRootBridgesConnectedEventGroupGuid);
> 
> above this part:
> 
>>   //
>>   // Signal EndOfDxe PI Event
>>   //
>>   EfiEventGroupSignal (&gEfiEndOfDxeEventGroupGuid);
>>
>>   //
>>   // Dispatch deferred images after EndOfDxe event.
>>   //
>>   EfiBootManagerDispatchDeferredImages ();
> 
> This will make:
> 
> - ArmVirtPkg match the order employed in OvmfPkg,
> 
> - EfiBootManagerDispatchDeferredImages() pick up PCI option ROMs,
> 
> - run the ACPI platform driver *before* EndOfDxe; but that's just fine.
>   (In fact, in OvmfPkg, that's *required*. See the FACS note there.)
> 
> 
> (2) Port the same fix to ArmPkg: namely move
> 
>>   //
>>   // 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);
> 
> just above
> 
>>   //
>>   // Signal EndOfDxe PI Event
>>   //
>>   EfiEventGroupSignal (&gEfiEndOfDxeEventGroupGuid);
> 
> 
> (3) In a separate patch, call EfiBootManagerDispatchDeferredImages()
> just after the *new* location of signaling EndOfDxe.

So given that commit 0f9395d7c5cc6ae2beaa2d87008fe158d04a8069 exists
now, I think we should do (1), and then port it whole-sale to ArmPkg.

... Unless of course my analysis is wrong. :)

Thanks
Laszlo