public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
From: "Ard Biesheuvel" <ard.biesheuvel@linaro.org>
To: Laszlo Ersek <lersek@redhat.com>
Cc: edk2-devel-groups-io <devel@edk2.groups.io>,
	Leif Lindholm <leif.lindholm@linaro.org>,
	 Zhichao Gao <zhichao.gao@intel.com>, Ray Ni <ray.ni@intel.com>,
	 Maurice Ma <maurice.ma@intel.com>, Guo Dong <guo.dong@intel.com>,
	 Benjamin You <benjamin.you@intel.com>
Subject: Re: [RFC PATCH 2/2] MdeModulePkg/BdsDxe: allow consoles on drivers loaded via Driver####
Date: Fri, 6 Dec 2019 15:50:03 +0000	[thread overview]
Message-ID: <CAKv+Gu_vbyLf_Ykdeg7w275OP7ZHXK-yDKzxa3iOD=ocT0JpYA@mail.gmail.com> (raw)
In-Reply-To: <8d49cca6-b1b9-cddc-0590-4e4a84643e4f@redhat.com>

On Fri, 6 Dec 2019 at 15:44, Laszlo Ersek <lersek@redhat.com> wrote:
>
> On 12/06/19 15:31, Ard Biesheuvel wrote:
> > Currently, we dispatch drivers specified via Driver#### entries after
> > calling PlatformBootManagerBeforeConsole(), which prevents us from
> > loading drivers that provide consoles via Driver#### entries.
>
> I'd put this as:
>
>   ... which prevents PlatformBootManagerBeforeConsole() from adding such
>   consoles to ConIn, ConOut and StdErr that are produced by drivers
>   loaded via Driver#### options.
>
> > This is particularly annoying on AArch64 systems, given that it prevents
> > us from loading AArch64 drivers from the ESP for PCIe graphics cards
> > that shipped with x86 drivers in the option ROM, which is the common
> > case today.
> >
> > So let's reorder the handling of the Driver#### entries with the
> > invocation of PlatformBootManagerBeforeConsole(), which results in
> > Driver#### entries being dispatched in the same way as option ROM
> > images.
>
> Ah, so you are saying the drivers will be added unconditionally to the
> deferred list, with this patch (because, at the new ProcessLoadOptions()
> call site, the platform cannot have signaled EndOfDxe yet).
>
> Then in PlatformBootManagerBeforeConsole(), the platform can signal
> EndOfDxe, call EfiBootManagerDispatchDeferredImages(), connect GOP
> drivers, see what new handles / device paths are created, and *then*
> update ConIn / ConOut / ConErr.
>
> Can you point out where the deferral happens, inside
> ProcessLoadOptions()?
>
> (If there is no deferral, then we can't / shouldn't do this, as drivers
> from the ESP should not be loaded before EndOfDxe, I think.)
>
> Hmmm... it looks like:
>
> BdsEntry()                            [MdeModulePkg/Universal/BdsDxe/BdsEntry.c]
>   ProcessLoadOptions()                [MdeModulePkg/Universal/BdsDxe/BdsEntry.c]
>     EfiBootManagerProcessLoadOption() [MdeModulePkg/Library/UefiBootManagerLib/BmLoadOption.c]
>       CoreLoadImage()                 [MdeModulePkg/Core/Dxe/Image/Image.c] -- via gBS->LoadImage()
>         CoreLoadImageCommon()         [MdeModulePkg/Core/Dxe/Image/Image.c]
>           Security2StubAuthenticate() [MdeModulePkg/Universal/SecurityStubDxe/SecurityStub.c] -- via gSecurity2->FileAuthentication()
>             Defer3rdPartyImageLoad()  [MdeModulePkg/Universal/SecurityStubDxe/Defer3rdPartyImageLoad.c]
>               // checks "mEndOfDxe"
>               QueueImage()            [MdeModulePkg/Universal/SecurityStubDxe/Defer3rdPartyImageLoad.c]
>
> OK.
>

Exactly. This flow is identical to how option ROMs are processed if
they are discovered before EndOfDxe signalling completes (which is why
the Juno platform was broken without the call to
EfiBootManagerDispatchDeferredImages() in
PlatformBootManagerBeforeConsole())

> I think this series makes sense.
>
> I defer to Ray, but from my POV, it should be OK.
>
> With the patch#2 commit message updated:
>
> series
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>
> Thanks
> Laszlo
>
> >
> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > ---
> >  MdeModulePkg/Universal/BdsDxe/BdsEntry.c | 15 ++++++++-------
> >  1 file changed, 8 insertions(+), 7 deletions(-)
> >
> > diff --git a/MdeModulePkg/Universal/BdsDxe/BdsEntry.c b/MdeModulePkg/Universal/BdsDxe/BdsEntry.c
> > index 1fb04dcbbcda..ad4c4c0406f6 100644
> > --- a/MdeModulePkg/Universal/BdsDxe/BdsEntry.c
> > +++ b/MdeModulePkg/Universal/BdsDxe/BdsEntry.c
> > @@ -850,6 +850,14 @@ BdsEntry (
> >      }
> >    }
> >
> > +  //
> > +  // Execute Driver Options. Images will be loaded but dispatch will be
> > +  // deferred for 3rd party images until EndOfDxe is signalled.
> > +  //
> > +  LoadOptions = EfiBootManagerGetLoadOptions (&LoadOptionCount, LoadOptionTypeDriver);
> > +  ProcessLoadOptions (LoadOptions, LoadOptionCount);
> > +  EfiBootManagerFreeLoadOptions (LoadOptions, LoadOptionCount);
> > +
> >    //
> >    // Do the platform init, can be customized by OEM/IBV
> >    // Possible things that can be done in PlatformBootManagerBeforeConsole:
> > @@ -868,13 +876,6 @@ BdsEntry (
> >    //
> >    EfiBootManagerStartHotkeyService (&HotkeyTriggered);
> >
> > -  //
> > -  // Execute Driver Options
> > -  //
> > -  LoadOptions = EfiBootManagerGetLoadOptions (&LoadOptionCount, LoadOptionTypeDriver);
> > -  ProcessLoadOptions (LoadOptions, LoadOptionCount);
> > -  EfiBootManagerFreeLoadOptions (LoadOptions, LoadOptionCount);
> > -
> >    //
> >    // Connect consoles
> >    //
> >
>

  reply	other threads:[~2019-12-06 15:50 UTC|newest]

Thread overview: 21+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-12-06 14:31 [RFC PATCH 0/2] MdeModulePkg: reorder Driver#### handling for console drivers Ard Biesheuvel
2019-12-06 14:31 ` [RFC PATCH 1/2] MdeModulePkg/PlatformBootManagerLib: document change of Driver#### handling Ard Biesheuvel
2019-12-06 14:31 ` [RFC PATCH 2/2] MdeModulePkg/BdsDxe: allow consoles on drivers loaded via Driver#### Ard Biesheuvel
2019-12-06 15:43   ` Laszlo Ersek
2019-12-06 15:50     ` Ard Biesheuvel [this message]
2019-12-09  2:12       ` Ni, Ray
2019-12-09  8:42         ` Ard Biesheuvel
2019-12-11 10:40           ` Ard Biesheuvel
2019-12-12  8:59             ` [edk2-devel] " Ni, Ray
2019-12-12  9:05               ` Ard Biesheuvel
2019-12-23 14:09                 ` Ard Biesheuvel
2020-01-09 12:54                   ` Ard Biesheuvel
2020-01-10  1:19                     ` Ni, Ray
2020-01-10 14:37                       ` Ni, Ray
2020-01-10 16:23                         ` Laszlo Ersek
2020-01-13 17:28                           ` Ard Biesheuvel
2020-01-13 17:57                             ` Andrew Fish
2020-01-14 16:46                               ` Ard Biesheuvel
2020-01-14 17:45                                 ` Andrew Fish
2020-01-15  3:12                                   ` Ni, Ray
2020-01-15  3:14                           ` Ni, Ray

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-list from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAKv+Gu_vbyLf_Ykdeg7w275OP7ZHXK-yDKzxa3iOD=ocT0JpYA@mail.gmail.com' \
    --to=devel@edk2.groups.io \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox