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

["Laszlo Ersek" <lersek@redhat.com>]

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

>
> 
>