* [PATCH v1 1/1] ArmPkg: Dispatch deferred images after EndOfDxe @ 2019-05-01 14:01 Sami Mujawar 2019-05-02 8:14 ` Ard Biesheuvel 2019-12-05 20:25 ` Ard Biesheuvel 0 siblings, 2 replies; 9+ messages in thread From: Sami Mujawar @ 2019-05-01 14:01 UTC (permalink / raw) To: devel Cc: ard.biesheuvel, leif.lindholm, Matteo.Carlini, Stephanie.Hughes-Fitt, nd 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)' ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v1 1/1] ArmPkg: Dispatch deferred images after EndOfDxe 2019-05-01 14:01 [PATCH v1 1/1] ArmPkg: Dispatch deferred images after EndOfDxe Sami Mujawar @ 2019-05-02 8:14 ` Ard Biesheuvel 2019-12-05 20:25 ` Ard Biesheuvel 1 sibling, 0 replies; 9+ messages in thread From: Ard Biesheuvel @ 2019-05-02 8:14 UTC (permalink / raw) To: Sami Mujawar Cc: edk2-devel-groups-io, Leif Lindholm, Matteo Carlini, Stephanie Hughes-Fitt, nd 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)' > > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v1 1/1] ArmPkg: Dispatch deferred images after EndOfDxe 2019-05-01 14:01 [PATCH v1 1/1] ArmPkg: Dispatch deferred images after EndOfDxe Sami Mujawar 2019-05-02 8:14 ` Ard Biesheuvel @ 2019-12-05 20:25 ` Ard Biesheuvel 2019-12-05 23:54 ` [edk2-devel] " Laszlo Ersek 1 sibling, 1 reply; 9+ messages in thread From: Ard Biesheuvel @ 2019-12-05 20:25 UTC (permalink / raw) To: Sami Mujawar; +Cc: edk2-devel-groups-io, Leif Lindholm, Matteo Carlini, nd 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 :-) ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [edk2-devel] [PATCH v1 1/1] ArmPkg: Dispatch deferred images after EndOfDxe 2019-12-05 20:25 ` Ard Biesheuvel @ 2019-12-05 23:54 ` Laszlo Ersek 2019-12-06 0:04 ` Laszlo Ersek 2019-12-06 0:07 ` Laszlo Ersek 0 siblings, 2 replies; 9+ messages in thread From: Laszlo Ersek @ 2019-12-05 23:54 UTC (permalink / raw) To: devel, ard.biesheuvel, Sami Mujawar; +Cc: Leif Lindholm, Matteo Carlini, nd 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 > > > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [edk2-devel] [PATCH v1 1/1] ArmPkg: Dispatch deferred images after EndOfDxe 2019-12-05 23:54 ` [edk2-devel] " Laszlo Ersek @ 2019-12-06 0:04 ` Laszlo Ersek 2019-12-06 10:02 ` Ard Biesheuvel 2019-12-06 0:07 ` Laszlo Ersek 1 sibling, 1 reply; 9+ messages in thread From: Laszlo Ersek @ 2019-12-06 0:04 UTC (permalink / raw) To: devel, ard.biesheuvel, Sami Mujawar; +Cc: Leif Lindholm, Matteo Carlini, nd 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [edk2-devel] [PATCH v1 1/1] ArmPkg: Dispatch deferred images after EndOfDxe 2019-12-06 0:04 ` Laszlo Ersek @ 2019-12-06 10:02 ` Ard Biesheuvel 2019-12-06 10:33 ` Laszlo Ersek 0 siblings, 1 reply; 9+ messages in thread From: Ard Biesheuvel @ 2019-12-06 10:02 UTC (permalink / raw) To: Laszlo Ersek Cc: edk2-devel-groups-io, Sami Mujawar, Leif Lindholm, Matteo Carlini, nd 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. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [edk2-devel] [PATCH v1 1/1] ArmPkg: Dispatch deferred images after EndOfDxe 2019-12-06 10:02 ` Ard Biesheuvel @ 2019-12-06 10:33 ` Laszlo Ersek 2019-12-06 11:01 ` Ard Biesheuvel 0 siblings, 1 reply; 9+ messages in thread From: Laszlo Ersek @ 2019-12-06 10:33 UTC (permalink / raw) To: Ard Biesheuvel Cc: edk2-devel-groups-io, Sami Mujawar, Leif Lindholm, Matteo Carlini, nd 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [edk2-devel] [PATCH v1 1/1] ArmPkg: Dispatch deferred images after EndOfDxe 2019-12-06 10:33 ` Laszlo Ersek @ 2019-12-06 11:01 ` Ard Biesheuvel 0 siblings, 0 replies; 9+ messages in thread From: Ard Biesheuvel @ 2019-12-06 11:01 UTC (permalink / raw) To: Laszlo Ersek Cc: edk2-devel-groups-io, Sami Mujawar, Leif Lindholm, Matteo Carlini, nd 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 :-) ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [edk2-devel] [PATCH v1 1/1] ArmPkg: Dispatch deferred images after EndOfDxe 2019-12-05 23:54 ` [edk2-devel] " Laszlo Ersek 2019-12-06 0:04 ` Laszlo Ersek @ 2019-12-06 0:07 ` Laszlo Ersek 1 sibling, 0 replies; 9+ messages in thread From: Laszlo Ersek @ 2019-12-06 0:07 UTC (permalink / raw) To: devel, ard.biesheuvel, Sami Mujawar; +Cc: Leif Lindholm, Matteo Carlini, nd 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2019-12-06 11:01 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2019-05-01 14:01 [PATCH v1 1/1] ArmPkg: Dispatch deferred images after EndOfDxe Sami Mujawar 2019-05-02 8:14 ` Ard Biesheuvel 2019-12-05 20:25 ` Ard Biesheuvel 2019-12-05 23:54 ` [edk2-devel] " Laszlo Ersek 2019-12-06 0:04 ` Laszlo Ersek 2019-12-06 10:02 ` Ard Biesheuvel 2019-12-06 10:33 ` Laszlo Ersek 2019-12-06 11:01 ` Ard Biesheuvel 2019-12-06 0:07 ` Laszlo Ersek
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox