[RFC PATCH 0/2] MdeModulePkg: reorder Driver#### handling for console drivers

[RFC PATCH 0/2] MdeModulePkg: reorder Driver#### handling for console drivers

[Ard Biesheuvel]

Change the order in which the BDS handles Driver#### entries and invokes
the PlatformBmLib hooks so that we can load GOP drivers via Driver####
entries in time for them to be used as the console.

Cc: Leif Lindholm <leif.lindholm@linaro.org>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Zhichao Gao <zhichao.gao@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Maurice Ma <maurice.ma@intel.com>
Cc: Guo Dong <guo.dong@intel.com>
Cc: Benjamin You <benjamin.you@intel.com>

Ard Biesheuvel (2):
  MdeModulePkg/PlatformBootManagerLib: document change of Driver####
    handling
  MdeModulePkg/BdsDxe: allow consoles on drivers loaded via Driver####

 .../Library/PlatformBootManagerLib/PlatformBm.c |  2 +-
 .../Library/PlatformBootManagerLib/PlatformBm.c |  2 +-
 .../Include/Library/PlatformBootManagerLib.h    |  2 +-
 .../PlatformBootManager.c                       |  2 +-
 MdeModulePkg/Universal/BdsDxe/BdsEntry.c        | 17 +++++++++--------
 .../PlatformBootManagerLib/BdsPlatform.c        |  2 +-
 .../PlatformBootManager.c                       |  2 +-
 7 files changed, 15 insertions(+), 14 deletions(-)

-- 
2.17.1

[RFC PATCH 1/2] MdeModulePkg/PlatformBootManagerLib: document change of Driver#### handling

[Ard Biesheuvel]

A future patch will change the order in which Driver#### options are
processed, removing the possibility to create such options in
PlatformBootManagerBeforeConsole(). Update the code comments duplicated
across the tree to reflect this upcoming change, and replace Driver####
with SysPrep####, which can be used for the same purpose instead.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c                    | 2 +-
 ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c                | 2 +-
 MdeModulePkg/Include/Library/PlatformBootManagerLib.h                 | 2 +-
 MdeModulePkg/Library/PlatformBootManagerLibNull/PlatformBootManager.c | 2 +-
 MdeModulePkg/Universal/BdsDxe/BdsEntry.c                              | 2 +-
 OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c                  | 2 +-
 UefiPayloadPkg/Library/PlatformBootManagerLib/PlatformBootManager.c   | 2 +-
 7 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c b/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c
index e6e788e0f107..d436eacd3494 100644
--- a/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c
+++ b/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c
@@ -532,7 +532,7 @@ PlatformRegisterOptionsAndKeys (
   Possible things that can be done in PlatformBootManagerBeforeConsole:
   > Update console variable: 1. include hot-plug devices;
   >                          2. Clear ConIn and add SOL for AMT
-  > Register new Driver#### or Boot####
+  > Register new SysPrep#### or Boot####
   > Register new Key####: e.g.: F12
   > Signal ReadyToLock event
   > Authentication action: 1. connect Auth devices;
diff --git a/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c b/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c
index 5f6cfe64daca..2df874a0dedd 100644
--- a/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c
+++ b/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c
@@ -674,7 +674,7 @@ PlatformRegisterOptionsAndKeys (
   Possible things that can be done in PlatformBootManagerBeforeConsole:
   > Update console variable: 1. include hot-plug devices;
   >                          2. Clear ConIn and add SOL for AMT
-  > Register new Driver#### or Boot####
+  > Register new SysPrep#### or Boot####
   > Register new Key####: e.g.: F12
   > Signal ReadyToLock event
   > Authentication action: 1. connect Auth devices;
diff --git a/MdeModulePkg/Include/Library/PlatformBootManagerLib.h b/MdeModulePkg/Include/Library/PlatformBootManagerLib.h
index 5d3062e9ddea..0b296b55bcab 100644
--- a/MdeModulePkg/Include/Library/PlatformBootManagerLib.h
+++ b/MdeModulePkg/Include/Library/PlatformBootManagerLib.h
@@ -17,7 +17,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
 
   Such as:
     Update console variable;
-    Register new Driver#### or Boot####;
+    Register new SysPrep#### or Boot####;
     Signal ReadyToLock event.
 **/
 VOID
diff --git a/MdeModulePkg/Library/PlatformBootManagerLibNull/PlatformBootManager.c b/MdeModulePkg/Library/PlatformBootManagerLibNull/PlatformBootManager.c
index 43560bf2aa6c..45cf81c1226b 100644
--- a/MdeModulePkg/Library/PlatformBootManagerLibNull/PlatformBootManager.c
+++ b/MdeModulePkg/Library/PlatformBootManagerLibNull/PlatformBootManager.c
@@ -15,7 +15,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
 
   Such as:
     Update console variable;
-    Register new Driver#### or Boot####;
+    Register new SysPrep#### or Boot####;
     Signal ReadyToLock event.
 **/
 VOID
diff --git a/MdeModulePkg/Universal/BdsDxe/BdsEntry.c b/MdeModulePkg/Universal/BdsDxe/BdsEntry.c
index d387dbe7ac12..1fb04dcbbcda 100644
--- a/MdeModulePkg/Universal/BdsDxe/BdsEntry.c
+++ b/MdeModulePkg/Universal/BdsDxe/BdsEntry.c
@@ -854,7 +854,7 @@ BdsEntry (
   // Do the platform init, can be customized by OEM/IBV
   // Possible things that can be done in PlatformBootManagerBeforeConsole:
   // > Update console variable: 1. include hot-plug devices; 2. Clear ConIn and add SOL for AMT
-  // > Register new Driver#### or Boot####
+  // > Register new SysPrep#### or Boot####
   // > Register new Key####: e.g.: F12
   // > Signal ReadyToLock event
   // > Authentication action: 1. connect Auth devices; 2. Identify auto logon user.
diff --git a/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c b/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c
index 8af9b71f18a3..b7dc9a1fefdc 100644
--- a/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c
+++ b/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c
@@ -339,7 +339,7 @@ SaveS3BootScript (
 
   > Update console variable: 1. include hot-plug devices;
   >                          2. Clear ConIn and add SOL for AMT
-  > Register new Driver#### or Boot####
+  > Register new SysPrep#### or Boot####
   > Register new Key####: e.g.: F12
   > Signal ReadyToLock event
   > Authentication action: 1. connect Auth devices;
diff --git a/UefiPayloadPkg/Library/PlatformBootManagerLib/PlatformBootManager.c b/UefiPayloadPkg/Library/PlatformBootManagerLib/PlatformBootManager.c
index c5c6af0abcb2..2833d2b03019 100644
--- a/UefiPayloadPkg/Library/PlatformBootManagerLib/PlatformBootManager.c
+++ b/UefiPayloadPkg/Library/PlatformBootManagerLib/PlatformBootManager.c
@@ -143,7 +143,7 @@ PlatformRegisterFvBootOption (
 
   Such as:
     Update console variable;
-    Register new Driver#### or Boot####;
+    Register new SysPrep#### or Boot####;
     Signal ReadyToLock event.
 **/
 VOID
-- 
2.17.1

[RFC PATCH 2/2] MdeModulePkg/BdsDxe: allow consoles on drivers loaded via Driver####

[Ard Biesheuvel]

Currently, we dispatch drivers specified via Driver#### entries after
calling PlatformBootManagerBeforeConsole(), which prevents us from
loading drivers that provide consoles via Driver#### entries.

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.

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

Re: [RFC PATCH 2/2] MdeModulePkg/BdsDxe: allow consoles on drivers loaded via Driver####

[Laszlo Ersek]

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.

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

Re: [RFC PATCH 2/2] MdeModulePkg/BdsDxe: allow consoles on drivers loaded via Driver####

[Ard Biesheuvel]

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

Re: [RFC PATCH 2/2] MdeModulePkg/BdsDxe: allow consoles on drivers loaded via Driver####

[Ni, Ray]

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

Ard,
I checked ArmPkg's PlatformBootManagerLib and found it doesn't
call *DispatchDeferredImages() after signaling EndOfDxe.

The deferred image dispatch mechanism assumes the platform
needs to call the *DispatchDeferredImages() after signaling EndOfDxe.

I don't understand why the deferred image can be loaded with your patch.
They are still deferred because the loading time is before EndOfDxe.

Thanks,
Ray

Re: [RFC PATCH 2/2] MdeModulePkg/BdsDxe: allow consoles on drivers loaded via Driver####

[Ard Biesheuvel]

On Mon, 9 Dec 2019 at 03:12, Ni, Ray <ray.ni@intel.com> wrote:
>
> > 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())
> >
>
> Ard,
> I checked ArmPkg's PlatformBootManagerLib and found it doesn't
> call *DispatchDeferredImages() after signaling EndOfDxe.
>

It does. We just added this in 0f9395d7c5cc6ae2beaa2d87008fe158d04a8069

> The deferred image dispatch mechanism assumes the platform
> needs to call the *DispatchDeferredImages() after signaling EndOfDxe.
>

Indeed.

> I don't understand why the deferred image can be loaded with your patch.
> They are still deferred because the loading time is before EndOfDxe.
>

Yes, but because PlatformBootManagerBeforeConsole () does all of this,
the only way to get Driver#### to work for consoles on GOP drivers, we
need to move it before that call.

Re: [RFC PATCH 2/2] MdeModulePkg/BdsDxe: allow consoles on drivers loaded via Driver####

[Ard Biesheuvel]

On Mon, 9 Dec 2019 at 09:42, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>
> On Mon, 9 Dec 2019 at 03:12, Ni, Ray <ray.ni@intel.com> wrote:
> >
> > > 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())
> > >
> >
> > Ard,
> > I checked ArmPkg's PlatformBootManagerLib and found it doesn't
> > call *DispatchDeferredImages() after signaling EndOfDxe.
> >
>
> It does. We just added this in 0f9395d7c5cc6ae2beaa2d87008fe158d04a8069
>
> > The deferred image dispatch mechanism assumes the platform
> > needs to call the *DispatchDeferredImages() after signaling EndOfDxe.
> >
>
> Indeed.
>
> > I don't understand why the deferred image can be loaded with your patch.
> > They are still deferred because the loading time is before EndOfDxe.
> >
>
> Yes, but because PlatformBootManagerBeforeConsole () does all of this,
> the only way to get Driver#### to work for consoles on GOP drivers, we
> need to move it before that call.

Any further comments on this patch?

Re: [edk2-devel] [RFC PATCH 2/2] MdeModulePkg/BdsDxe: allow consoles on drivers loaded via Driver####

[Ni, Ray]

Ard,
I still cannot understand it.

Since Driver#### are processed (process = LoadImage + StartImage) after EndOfDxe, they are not deferred.
It means the Driver#### entrypoints are called directly.
Inside the entrypoints, driverbinding protocols are installed.

After that, EfiBootManagerConnectAllDefaultConsoles () uses driver model logic to start the proper GOP driver.

Thanks,
Ray

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Ard Biesheuvel
> Sent: Wednesday, December 11, 2019 6:40 PM
> To: Ni, Ray <ray.ni@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>; edk2-devel-groups-io <devel@edk2.groups.io>; Leif Lindholm
> <leif.lindholm@linaro.org>; Gao, Zhichao <zhichao.gao@intel.com>; Ma, Maurice <maurice.ma@intel.com>; Dong, Guo
> <guo.dong@intel.com>; You, Benjamin <benjamin.you@intel.com>
> Subject: Re: [edk2-devel] [RFC PATCH 2/2] MdeModulePkg/BdsDxe: allow consoles on drivers loaded via Driver####
> 
> On Mon, 9 Dec 2019 at 09:42, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> >
> > On Mon, 9 Dec 2019 at 03:12, Ni, Ray <ray.ni@intel.com> wrote:
> > >
> > > > 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())
> > > >
> > >
> > > Ard,
> > > I checked ArmPkg's PlatformBootManagerLib and found it doesn't
> > > call *DispatchDeferredImages() after signaling EndOfDxe.
> > >
> >
> > It does. We just added this in 0f9395d7c5cc6ae2beaa2d87008fe158d04a8069
> >
> > > The deferred image dispatch mechanism assumes the platform
> > > needs to call the *DispatchDeferredImages() after signaling EndOfDxe.
> > >
> >
> > Indeed.
> >
> > > I don't understand why the deferred image can be loaded with your patch.
> > > They are still deferred because the loading time is before EndOfDxe.
> > >
> >
> > Yes, but because PlatformBootManagerBeforeConsole () does all of this,
> > the only way to get Driver#### to work for consoles on GOP drivers, we
> > need to move it before that call.
> 
> Any further comments on this patch?
> 
> 

Re: [edk2-devel] [RFC PATCH 2/2] MdeModulePkg/BdsDxe: allow consoles on drivers loaded via Driver####

[Ard Biesheuvel]

On Thu, 12 Dec 2019 at 09:59, Ni, Ray <ray.ni@intel.com> wrote:
>
> Ard,
> I still cannot understand it.
>
> Since Driver#### are processed (process = LoadImage + StartImage) after EndOfDxe, they are not deferred.
> It means the Driver#### entrypoints are called directly.
> Inside the entrypoints, driverbinding protocols are installed.
>
> After that, EfiBootManagerConnectAllDefaultConsoles () uses driver model logic to start the proper GOP driver.
>

It only does that if the GOP console is already in the
ConIn/ConOut/ConErr variables, no?

Today, we have code in the PlatformBdsLib to traverse the PCI
hierarchy to populate those ConXXX variables if we encounter any PCI
graphics cards, but this is done in
PlatformBootManagerBeforeConsole(), so if the Driver#### is loaded
*after* PlatformBootManagerBeforeConsole(), it will not be added to
ConXXX. So we need to process Driver#### before calling
PlatformBootManagerBeforeConsole(), so that the driver is dispatches
right after we call DispatchDeferredImages() but before we do the
traversal and populate the COnXXX variables.

How else do you propose we could make PCI graphics controllers
supported by Driver#### options appear in COnXXX before
EfiBootManagerConnectAllDefaultConsoles() is called?



>
> > -----Original Message-----
> > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Ard Biesheuvel
> > Sent: Wednesday, December 11, 2019 6:40 PM
> > To: Ni, Ray <ray.ni@intel.com>
> > Cc: Laszlo Ersek <lersek@redhat.com>; edk2-devel-groups-io <devel@edk2.groups.io>; Leif Lindholm
> > <leif.lindholm@linaro.org>; Gao, Zhichao <zhichao.gao@intel.com>; Ma, Maurice <maurice.ma@intel.com>; Dong, Guo
> > <guo.dong@intel.com>; You, Benjamin <benjamin.you@intel.com>
> > Subject: Re: [edk2-devel] [RFC PATCH 2/2] MdeModulePkg/BdsDxe: allow consoles on drivers loaded via Driver####
> >
> > On Mon, 9 Dec 2019 at 09:42, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> > >
> > > On Mon, 9 Dec 2019 at 03:12, Ni, Ray <ray.ni@intel.com> wrote:
> > > >
> > > > > 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())
> > > > >
> > > >
> > > > Ard,
> > > > I checked ArmPkg's PlatformBootManagerLib and found it doesn't
> > > > call *DispatchDeferredImages() after signaling EndOfDxe.
> > > >
> > >
> > > It does. We just added this in 0f9395d7c5cc6ae2beaa2d87008fe158d04a8069
> > >
> > > > The deferred image dispatch mechanism assumes the platform
> > > > needs to call the *DispatchDeferredImages() after signaling EndOfDxe.
> > > >
> > >
> > > Indeed.
> > >
> > > > I don't understand why the deferred image can be loaded with your patch.
> > > > They are still deferred because the loading time is before EndOfDxe.
> > > >
> > >
> > > Yes, but because PlatformBootManagerBeforeConsole () does all of this,
> > > the only way to get Driver#### to work for consoles on GOP drivers, we
> > > need to move it before that call.
> >
> > Any further comments on this patch?
> >
> > 
>

Re: [edk2-devel] [RFC PATCH 2/2] MdeModulePkg/BdsDxe: allow consoles on drivers loaded via Driver####

[Ard Biesheuvel]

On Thu, 12 Dec 2019 at 10:05, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>
> On Thu, 12 Dec 2019 at 09:59, Ni, Ray <ray.ni@intel.com> wrote:
> >
> > Ard,
> > I still cannot understand it.
> >
> > Since Driver#### are processed (process = LoadImage + StartImage) after EndOfDxe, they are not deferred.
> > It means the Driver#### entrypoints are called directly.
> > Inside the entrypoints, driverbinding protocols are installed.
> >
> > After that, EfiBootManagerConnectAllDefaultConsoles () uses driver model logic to start the proper GOP driver.
> >
>
> It only does that if the GOP console is already in the
> ConIn/ConOut/ConErr variables, no?
>
> Today, we have code in the PlatformBdsLib to traverse the PCI
> hierarchy to populate those ConXXX variables if we encounter any PCI
> graphics cards, but this is done in
> PlatformBootManagerBeforeConsole(), so if the Driver#### is loaded
> *after* PlatformBootManagerBeforeConsole(), it will not be added to
> ConXXX. So we need to process Driver#### before calling
> PlatformBootManagerBeforeConsole(), so that the driver is dispatches
> right after we call DispatchDeferredImages() but before we do the
> traversal and populate the COnXXX variables.
>
> How else do you propose we could make PCI graphics controllers
> supported by Driver#### options appear in COnXXX before
> EfiBootManagerConnectAllDefaultConsoles() is called?
>

Ping?


>
>
> >
> > > -----Original Message-----
> > > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Ard Biesheuvel
> > > Sent: Wednesday, December 11, 2019 6:40 PM
> > > To: Ni, Ray <ray.ni@intel.com>
> > > Cc: Laszlo Ersek <lersek@redhat.com>; edk2-devel-groups-io <devel@edk2.groups.io>; Leif Lindholm
> > > <leif.lindholm@linaro.org>; Gao, Zhichao <zhichao.gao@intel.com>; Ma, Maurice <maurice.ma@intel.com>; Dong, Guo
> > > <guo.dong@intel.com>; You, Benjamin <benjamin.you@intel.com>
> > > Subject: Re: [edk2-devel] [RFC PATCH 2/2] MdeModulePkg/BdsDxe: allow consoles on drivers loaded via Driver####
> > >
> > > On Mon, 9 Dec 2019 at 09:42, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> > > >
> > > > On Mon, 9 Dec 2019 at 03:12, Ni, Ray <ray.ni@intel.com> wrote:
> > > > >
> > > > > > 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())
> > > > > >
> > > > >
> > > > > Ard,
> > > > > I checked ArmPkg's PlatformBootManagerLib and found it doesn't
> > > > > call *DispatchDeferredImages() after signaling EndOfDxe.
> > > > >
> > > >
> > > > It does. We just added this in 0f9395d7c5cc6ae2beaa2d87008fe158d04a8069
> > > >
> > > > > The deferred image dispatch mechanism assumes the platform
> > > > > needs to call the *DispatchDeferredImages() after signaling EndOfDxe.
> > > > >
> > > >
> > > > Indeed.
> > > >
> > > > > I don't understand why the deferred image can be loaded with your patch.
> > > > > They are still deferred because the loading time is before EndOfDxe.
> > > > >
> > > >
> > > > Yes, but because PlatformBootManagerBeforeConsole () does all of this,
> > > > the only way to get Driver#### to work for consoles on GOP drivers, we
> > > > need to move it before that call.
> > >
> > > Any further comments on this patch?
> > >
> > > 
> >

Re: [edk2-devel] [RFC PATCH 2/2] MdeModulePkg/BdsDxe: allow consoles on drivers loaded via Driver####

[Ard Biesheuvel]

On Mon, 23 Dec 2019 at 15:09, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>
> On Thu, 12 Dec 2019 at 10:05, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> >
> > On Thu, 12 Dec 2019 at 09:59, Ni, Ray <ray.ni@intel.com> wrote:
> > >
> > > Ard,
> > > I still cannot understand it.
> > >
> > > Since Driver#### are processed (process = LoadImage + StartImage) after EndOfDxe, they are not deferred.
> > > It means the Driver#### entrypoints are called directly.
> > > Inside the entrypoints, driverbinding protocols are installed.
> > >
> > > After that, EfiBootManagerConnectAllDefaultConsoles () uses driver model logic to start the proper GOP driver.
> > >
> >
> > It only does that if the GOP console is already in the
> > ConIn/ConOut/ConErr variables, no?
> >
> > Today, we have code in the PlatformBdsLib to traverse the PCI
> > hierarchy to populate those ConXXX variables if we encounter any PCI
> > graphics cards, but this is done in
> > PlatformBootManagerBeforeConsole(), so if the Driver#### is loaded
> > *after* PlatformBootManagerBeforeConsole(), it will not be added to
> > ConXXX. So we need to process Driver#### before calling
> > PlatformBootManagerBeforeConsole(), so that the driver is dispatches
> > right after we call DispatchDeferredImages() but before we do the
> > traversal and populate the COnXXX variables.
> >
> > How else do you propose we could make PCI graphics controllers
> > supported by Driver#### options appear in COnXXX before
> > EfiBootManagerConnectAllDefaultConsoles() is called?
> >
>
> Ping?
>
>

Ping again?

> >
> >
> > >
> > > > -----Original Message-----
> > > > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Ard Biesheuvel
> > > > Sent: Wednesday, December 11, 2019 6:40 PM
> > > > To: Ni, Ray <ray.ni@intel.com>
> > > > Cc: Laszlo Ersek <lersek@redhat.com>; edk2-devel-groups-io <devel@edk2.groups.io>; Leif Lindholm
> > > > <leif.lindholm@linaro.org>; Gao, Zhichao <zhichao.gao@intel.com>; Ma, Maurice <maurice.ma@intel.com>; Dong, Guo
> > > > <guo.dong@intel.com>; You, Benjamin <benjamin.you@intel.com>
> > > > Subject: Re: [edk2-devel] [RFC PATCH 2/2] MdeModulePkg/BdsDxe: allow consoles on drivers loaded via Driver####
> > > >
> > > > On Mon, 9 Dec 2019 at 09:42, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> > > > >
> > > > > On Mon, 9 Dec 2019 at 03:12, Ni, Ray <ray.ni@intel.com> wrote:
> > > > > >
> > > > > > > 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())
> > > > > > >
> > > > > >
> > > > > > Ard,
> > > > > > I checked ArmPkg's PlatformBootManagerLib and found it doesn't
> > > > > > call *DispatchDeferredImages() after signaling EndOfDxe.
> > > > > >
> > > > >
> > > > > It does. We just added this in 0f9395d7c5cc6ae2beaa2d87008fe158d04a8069
> > > > >
> > > > > > The deferred image dispatch mechanism assumes the platform
> > > > > > needs to call the *DispatchDeferredImages() after signaling EndOfDxe.
> > > > > >
> > > > >
> > > > > Indeed.
> > > > >
> > > > > > I don't understand why the deferred image can be loaded with your patch.
> > > > > > They are still deferred because the loading time is before EndOfDxe.
> > > > > >
> > > > >
> > > > > Yes, but because PlatformBootManagerBeforeConsole () does all of this,
> > > > > the only way to get Driver#### to work for consoles on GOP drivers, we
> > > > > need to move it before that call.
> > > >
> > > > Any further comments on this patch?
> > > >
> > > > 
> > >

Re: [edk2-devel] [RFC PATCH 2/2] MdeModulePkg/BdsDxe: allow consoles on drivers loaded via Driver####

[Ni, Ray]

Ard,
I will give update on this by end of this week. sorry about the delay.

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Ard Biesheuvel
> Sent: Thursday, January 9, 2020 8:54 PM
> To: Ni, Ray <ray.ni@intel.com>
> Cc: devel@edk2.groups.io; Laszlo Ersek <lersek@redhat.com>; Leif Lindholm <leif.lindholm@linaro.org>; Gao, Zhichao
> <zhichao.gao@intel.com>; Ma, Maurice <maurice.ma@intel.com>; Dong, Guo <guo.dong@intel.com>; You, Benjamin
> <benjamin.you@intel.com>
> Subject: Re: [edk2-devel] [RFC PATCH 2/2] MdeModulePkg/BdsDxe: allow consoles on drivers loaded via Driver####
> 
> On Mon, 23 Dec 2019 at 15:09, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> >
> > On Thu, 12 Dec 2019 at 10:05, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> > >
> > > On Thu, 12 Dec 2019 at 09:59, Ni, Ray <ray.ni@intel.com> wrote:
> > > >
> > > > Ard,
> > > > I still cannot understand it.
> > > >
> > > > Since Driver#### are processed (process = LoadImage + StartImage) after EndOfDxe, they are not deferred.
> > > > It means the Driver#### entrypoints are called directly.
> > > > Inside the entrypoints, driverbinding protocols are installed.
> > > >
> > > > After that, EfiBootManagerConnectAllDefaultConsoles () uses driver model logic to start the proper GOP driver.
> > > >
> > >
> > > It only does that if the GOP console is already in the
> > > ConIn/ConOut/ConErr variables, no?
> > >
> > > Today, we have code in the PlatformBdsLib to traverse the PCI
> > > hierarchy to populate those ConXXX variables if we encounter any PCI
> > > graphics cards, but this is done in
> > > PlatformBootManagerBeforeConsole(), so if the Driver#### is loaded
> > > *after* PlatformBootManagerBeforeConsole(), it will not be added to
> > > ConXXX. So we need to process Driver#### before calling
> > > PlatformBootManagerBeforeConsole(), so that the driver is dispatches
> > > right after we call DispatchDeferredImages() but before we do the
> > > traversal and populate the COnXXX variables.
> > >
> > > How else do you propose we could make PCI graphics controllers
> > > supported by Driver#### options appear in COnXXX before
> > > EfiBootManagerConnectAllDefaultConsoles() is called?
> > >
> >
> > Ping?
> >
> >
> 
> Ping again?
> 
> > >
> > >
> > > >
> > > > > -----Original Message-----
> > > > > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Ard Biesheuvel
> > > > > Sent: Wednesday, December 11, 2019 6:40 PM
> > > > > To: Ni, Ray <ray.ni@intel.com>
> > > > > Cc: Laszlo Ersek <lersek@redhat.com>; edk2-devel-groups-io <devel@edk2.groups.io>; Leif Lindholm
> > > > > <leif.lindholm@linaro.org>; Gao, Zhichao <zhichao.gao@intel.com>; Ma, Maurice <maurice.ma@intel.com>; Dong,
> Guo
> > > > > <guo.dong@intel.com>; You, Benjamin <benjamin.you@intel.com>
> > > > > Subject: Re: [edk2-devel] [RFC PATCH 2/2] MdeModulePkg/BdsDxe: allow consoles on drivers loaded via Driver####
> > > > >
> > > > > On Mon, 9 Dec 2019 at 09:42, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> > > > > >
> > > > > > On Mon, 9 Dec 2019 at 03:12, Ni, Ray <ray.ni@intel.com> wrote:
> > > > > > >
> > > > > > > > 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())
> > > > > > > >
> > > > > > >
> > > > > > > Ard,
> > > > > > > I checked ArmPkg's PlatformBootManagerLib and found it doesn't
> > > > > > > call *DispatchDeferredImages() after signaling EndOfDxe.
> > > > > > >
> > > > > >
> > > > > > It does. We just added this in 0f9395d7c5cc6ae2beaa2d87008fe158d04a8069
> > > > > >
> > > > > > > The deferred image dispatch mechanism assumes the platform
> > > > > > > needs to call the *DispatchDeferredImages() after signaling EndOfDxe.
> > > > > > >
> > > > > >
> > > > > > Indeed.
> > > > > >
> > > > > > > I don't understand why the deferred image can be loaded with your patch.
> > > > > > > They are still deferred because the loading time is before EndOfDxe.
> > > > > > >
> > > > > >
> > > > > > Yes, but because PlatformBootManagerBeforeConsole () does all of this,
> > > > > > the only way to get Driver#### to work for consoles on GOP drivers, we
> > > > > > need to move it before that call.
> > > > >
> > > > > Any further comments on this patch?
> > > > >
> > > > >
> > > >
> 
> 

Re: [edk2-devel] [RFC PATCH 2/2] MdeModulePkg/BdsDxe: allow consoles on drivers loaded via Driver####

[Ni, Ray]

Ard,
I understand now that: BeforeConsole() needs to connect Gfx to get the GOP device path
for ConOut updating. But the GOP driver is specified in Driver#### and it will be loaded after
BeforeConsole() in today's code. This order makes the Gfx connection fails to start the GOP driver
resulting ConOut not updated.

Moving Driver#### processing before BeforeConsole() helps in your case. But it may impact
other platforms: There might be platforms that update Driver#### variables in BeforeConsole()
and expect these drivers are processed in the same boot (not the next boot). I don't have the real
examples. But today's flow allows this. With your change, Driver#### will not be processed in the first
boot. The change impacts these platforms.

One backward compatible approach can be: processing Driver#### before BeforeConsole() and processing the new Driver#### (added by BeforeConsole()) after BeforeConsole().

But another problem arises: If BeforeConsole() removes a Driver####, platform's expectation is that deleted Driver#### doesn't run. But that driver already runs.

So actually the question is: when BDS core can consume the Driver#### and process them?
Right now, it’s the time after BeforeConsole(). Just as right now BeforeConsole() updates ConOut
in your case, BeforeConsole() is the place where platform updates all UEFI defined boot related
variables. Processing Driver#### before BeforeConsole() requires platform updates Driver####
in other places. It's a new requirement to the platform.

With all what I explained in above, I cannot agree with the changes.

Another approach is:
Platform could connect the GFX in AfterConsole() and update the ConOut. Then platform could manually call EfiBootManagerConnectConsoleVariable (ConOut) to re-connect ConOut.
It's a bit ugly I agree.

Thanks,
Ray


> -----Original Message-----
> From: Ni, Ray
> Sent: Friday, January 10, 2020 9:20 AM
> To: devel@edk2.groups.io; ard.biesheuvel@linaro.org
> Cc: Laszlo Ersek <lersek@redhat.com>; Leif Lindholm
> <leif.lindholm@linaro.org>; Gao, Zhichao <zhichao.gao@intel.com>; Ma,
> Maurice <maurice.ma@intel.com>; Dong, Guo <guo.dong@intel.com>; You,
> Benjamin <benjamin.you@intel.com>
> Subject: RE: [edk2-devel] [RFC PATCH 2/2] MdeModulePkg/BdsDxe: allow
> consoles on drivers loaded via Driver####
> 
> Ard,
> I will give update on this by end of this week. sorry about the delay.
> 
> > -----Original Message-----
> > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Ard
> Biesheuvel
> > Sent: Thursday, January 9, 2020 8:54 PM
> > To: Ni, Ray <ray.ni@intel.com>
> > Cc: devel@edk2.groups.io; Laszlo Ersek <lersek@redhat.com>; Leif Lindholm
> <leif.lindholm@linaro.org>; Gao, Zhichao
> > <zhichao.gao@intel.com>; Ma, Maurice <maurice.ma@intel.com>; Dong, Guo
> <guo.dong@intel.com>; You, Benjamin
> > <benjamin.you@intel.com>
> > Subject: Re: [edk2-devel] [RFC PATCH 2/2] MdeModulePkg/BdsDxe: allow
> consoles on drivers loaded via Driver####
> >
> > On Mon, 23 Dec 2019 at 15:09, Ard Biesheuvel <ard.biesheuvel@linaro.org>
> wrote:
> > >
> > > On Thu, 12 Dec 2019 at 10:05, Ard Biesheuvel <ard.biesheuvel@linaro.org>
> wrote:
> > > >
> > > > On Thu, 12 Dec 2019 at 09:59, Ni, Ray <ray.ni@intel.com> wrote:
> > > > >
> > > > > Ard,
> > > > > I still cannot understand it.
> > > > >
> > > > > Since Driver#### are processed (process = LoadImage + StartImage) after
> EndOfDxe, they are not deferred.
> > > > > It means the Driver#### entrypoints are called directly.
> > > > > Inside the entrypoints, driverbinding protocols are installed.
> > > > >
> > > > > After that, EfiBootManagerConnectAllDefaultConsoles () uses driver
> model logic to start the proper GOP driver.
> > > > >
> > > >
> > > > It only does that if the GOP console is already in the
> > > > ConIn/ConOut/ConErr variables, no?
> > > >
> > > > Today, we have code in the PlatformBdsLib to traverse the PCI
> > > > hierarchy to populate those ConXXX variables if we encounter any PCI
> > > > graphics cards, but this is done in
> > > > PlatformBootManagerBeforeConsole(), so if the Driver#### is loaded
> > > > *after* PlatformBootManagerBeforeConsole(), it will not be added to
> > > > ConXXX. So we need to process Driver#### before calling
> > > > PlatformBootManagerBeforeConsole(), so that the driver is dispatches
> > > > right after we call DispatchDeferredImages() but before we do the
> > > > traversal and populate the COnXXX variables.
> > > >
> > > > How else do you propose we could make PCI graphics controllers
> > > > supported by Driver#### options appear in COnXXX before
> > > > EfiBootManagerConnectAllDefaultConsoles() is called?
> > > >
> > >
> > > Ping?
> > >
> > >
> >
> > Ping again?
> >
> > > >
> > > >
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
> Ard Biesheuvel
> > > > > > Sent: Wednesday, December 11, 2019 6:40 PM
> > > > > > To: Ni, Ray <ray.ni@intel.com>
> > > > > > Cc: Laszlo Ersek <lersek@redhat.com>; edk2-devel-groups-io
> <devel@edk2.groups.io>; Leif Lindholm
> > > > > > <leif.lindholm@linaro.org>; Gao, Zhichao <zhichao.gao@intel.com>;
> Ma, Maurice <maurice.ma@intel.com>; Dong,
> > Guo
> > > > > > <guo.dong@intel.com>; You, Benjamin <benjamin.you@intel.com>
> > > > > > Subject: Re: [edk2-devel] [RFC PATCH 2/2] MdeModulePkg/BdsDxe:
> allow consoles on drivers loaded via Driver####
> > > > > >
> > > > > > On Mon, 9 Dec 2019 at 09:42, Ard Biesheuvel
> <ard.biesheuvel@linaro.org> wrote:
> > > > > > >
> > > > > > > On Mon, 9 Dec 2019 at 03:12, Ni, Ray <ray.ni@intel.com> wrote:
> > > > > > > >
> > > > > > > > > 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())
> > > > > > > > >
> > > > > > > >
> > > > > > > > Ard,
> > > > > > > > I checked ArmPkg's PlatformBootManagerLib and found it doesn't
> > > > > > > > call *DispatchDeferredImages() after signaling EndOfDxe.
> > > > > > > >
> > > > > > >
> > > > > > > It does. We just added this in
> 0f9395d7c5cc6ae2beaa2d87008fe158d04a8069
> > > > > > >
> > > > > > > > The deferred image dispatch mechanism assumes the platform
> > > > > > > > needs to call the *DispatchDeferredImages() after signaling
> EndOfDxe.
> > > > > > > >
> > > > > > >
> > > > > > > Indeed.
> > > > > > >
> > > > > > > > I don't understand why the deferred image can be loaded with your
> patch.
> > > > > > > > They are still deferred because the loading time is before EndOfDxe.
> > > > > > > >
> > > > > > >
> > > > > > > Yes, but because PlatformBootManagerBeforeConsole () does all of
> this,
> > > > > > > the only way to get Driver#### to work for consoles on GOP drivers,
> we
> > > > > > > need to move it before that call.
> > > > > >
> > > > > > Any further comments on this patch?
> > > > > >
> > > > > >
> > > > >
> >
> > 

Re: [edk2-devel] [RFC PATCH 2/2] MdeModulePkg/BdsDxe: allow consoles on drivers loaded via Driver####

[Laszlo Ersek]

On 01/10/20 15:37, Ni, Ray wrote:
> Ard,
> I understand now that: BeforeConsole() needs to connect Gfx to get the GOP device path
> for ConOut updating. But the GOP driver is specified in Driver#### and it will be loaded after
> BeforeConsole() in today's code. This order makes the Gfx connection fails to start the GOP driver
> resulting ConOut not updated.
> 
> Moving Driver#### processing before BeforeConsole() helps in your case. But it may impact
> other platforms: There might be platforms that update Driver#### variables in BeforeConsole()
> and expect these drivers are processed in the same boot (not the next boot). I don't have the real
> examples. But today's flow allows this. With your change, Driver#### will not be processed in the first
> boot. The change impacts these platforms.
> 
> One backward compatible approach can be: processing Driver#### before BeforeConsole() and processing the new Driver#### (added by BeforeConsole()) after BeforeConsole().
> 
> But another problem arises: If BeforeConsole() removes a Driver####, platform's expectation is that deleted Driver#### doesn't run. But that driver already runs.
> 
> So actually the question is: when BDS core can consume the Driver#### and process them?
> Right now, it’s the time after BeforeConsole(). Just as right now BeforeConsole() updates ConOut
> in your case, BeforeConsole() is the place where platform updates all UEFI defined boot related
> variables. Processing Driver#### before BeforeConsole() requires platform updates Driver####
> in other places. It's a new requirement to the platform.
> 
> With all what I explained in above, I cannot agree with the changes.
> 
> Another approach is:
> Platform could connect the GFX in AfterConsole() and update the ConOut. Then platform could manually call EfiBootManagerConnectConsoleVariable (ConOut) to re-connect ConOut.
> It's a bit ugly I agree.

Let me raise three other ideas (alternatives to each other, and to the above), with varying levels of annoyance. :)


(1) Keep the following logic (i.e. the subject of this patch):

  //
  // Execute Driver Options
  //
  LoadOptions = EfiBootManagerGetLoadOptions (&LoadOptionCount, LoadOptionTypeDriver);
  ProcessLoadOptions (LoadOptions, LoadOptionCount);
  EfiBootManagerFreeLoadOptions (LoadOptions, LoadOptionCount);

in *both* places, but gate each one with a bit in a new bitmask PCD.

(Note: it's probably not the best for any platform to permit both branches, as driver images would be loaded twice.)


(2) EDKII_PLATFORM_BOOT_MANAGER_PROTOCOL has recently been added to edk2. It looks like a well-designed (extensible) protocol, for two reasons:

- the protocol structure has a Revision field,

- the only current member function, RefreshAllBootOptions(), is permitted to return EFI_UNSUPPORTED -- and the single call site, in the EfiBootManagerRefreshAllBootOption() function, handles that return value well (it does nothing).

The idea would be to bump the Revision field, and add a new member function. Then call this member function (if it exists) in the spot where the current patch proposes to move the Driver#### dispatch logic to.

This is almost like a new PlatformBootManagerLib interface, except it does not require existent lib instances to be updated.

And, on the EDKII_PLATFORM_BOOT_MANAGER_PROTOCOL implementation side, RefreshAllBootOptions() would return EFI_UNSUPPORTED. (Because that is irrelevant to Ard's use case.)

Perhaps add yet another member function that can disable the Driver#### option processing in the current location.


(3) Extend the UEFI specification, section "3.1.3 Load Options".

The LOAD_OPTION_CATEGORY bit-field is almost what we want, except it's specified to be ignored for Driver#### options. So,

- either invent the standardese to let us use LOAD_OPTION_CATEGORY for Driver#### options too, without breaking existing platforms, *or*
- introduce a new (not too wide) distinct bitfield called LOAD_OPTION_DRIVER_CATEGORY.

Whichever category bitfield proves acceptable, introduce a new "driver category bit" in that bitfield, called LOAD_OPTION_DRIVER_CATEGORY_CONSOLE.

Specify that, if any Driver#### option has the LOAD_OPTION_DRIVER_CATEGORY_CONSOLE attribute set, then Driver#### options will be processed in two passes. In both passes, DriverOrder is to be honored, but the first pass will only consider options with the attribute set, and the second pass will only consider options with the attribute clear.

Implement this in BdsEntry / UefiBootManagerLib.

... Maybe don't even introduce LOAD_OPTION_DRIVER_CATEGORY*; just a bitfield that is expressly vendor specific?

Thanks
Laszlo

Re: [edk2-devel] [RFC PATCH 2/2] MdeModulePkg/BdsDxe: allow consoles on drivers loaded via Driver####

[Ard Biesheuvel]

On Fri, 10 Jan 2020 at 17:23, Laszlo Ersek <lersek@redhat.com> wrote:
>
> On 01/10/20 15:37, Ni, Ray wrote:
> > Ard,
> > I understand now that: BeforeConsole() needs to connect Gfx to get the GOP device path
> > for ConOut updating. But the GOP driver is specified in Driver#### and it will be loaded after
> > BeforeConsole() in today's code. This order makes the Gfx connection fails to start the GOP driver
> > resulting ConOut not updated.
> >
> > Moving Driver#### processing before BeforeConsole() helps in your case. But it may impact
> > other platforms: There might be platforms that update Driver#### variables in BeforeConsole()
> > and expect these drivers are processed in the same boot (not the next boot). I don't have the real
> > examples. But today's flow allows this. With your change, Driver#### will not be processed in the first
> > boot. The change impacts these platforms.
> >
> > One backward compatible approach can be: processing Driver#### before BeforeConsole() and processing the new Driver#### (added by BeforeConsole()) after BeforeConsole().
> >
> > But another problem arises: If BeforeConsole() removes a Driver####, platform's expectation is that deleted Driver#### doesn't run. But that driver already runs.
> >
> > So actually the question is: when BDS core can consume the Driver#### and process them?
> > Right now, it’s the time after BeforeConsole(). Just as right now BeforeConsole() updates ConOut
> > in your case, BeforeConsole() is the place where platform updates all UEFI defined boot related
> > variables. Processing Driver#### before BeforeConsole() requires platform updates Driver####
> > in other places. It's a new requirement to the platform.
> >
> > With all what I explained in above, I cannot agree with the changes.
> >
> > Another approach is:
> > Platform could connect the GFX in AfterConsole() and update the ConOut. Then platform could manually call EfiBootManagerConnectConsoleVariable (ConOut) to re-connect ConOut.
> > It's a bit ugly I agree.
>
> Let me raise three other ideas (alternatives to each other, and to the above), with varying levels of annoyance. :)
>

Thanks Laszlo

Ray, given your objection to my approach, could you please consider
the below and give feedback on which approach is suitable to address
the issue I am trying to fix?

>
> (1) Keep the following logic (i.e. the subject of this patch):
>
>   //
>   // Execute Driver Options
>   //
>   LoadOptions = EfiBootManagerGetLoadOptions (&LoadOptionCount, LoadOptionTypeDriver);
>   ProcessLoadOptions (LoadOptions, LoadOptionCount);
>   EfiBootManagerFreeLoadOptions (LoadOptions, LoadOptionCount);
>
> in *both* places, but gate each one with a bit in a new bitmask PCD.
>
> (Note: it's probably not the best for any platform to permit both branches, as driver images would be loaded twice.)
>
>
> (2) EDKII_PLATFORM_BOOT_MANAGER_PROTOCOL has recently been added to edk2. It looks like a well-designed (extensible) protocol, for two reasons:
>
> - the protocol structure has a Revision field,
>
> - the only current member function, RefreshAllBootOptions(), is permitted to return EFI_UNSUPPORTED -- and the single call site, in the EfiBootManagerRefreshAllBootOption() function, handles that return value well (it does nothing).
>
> The idea would be to bump the Revision field, and add a new member function. Then call this member function (if it exists) in the spot where the current patch proposes to move the Driver#### dispatch logic to.
>
> This is almost like a new PlatformBootManagerLib interface, except it does not require existent lib instances to be updated.
>
> And, on the EDKII_PLATFORM_BOOT_MANAGER_PROTOCOL implementation side, RefreshAllBootOptions() would return EFI_UNSUPPORTED. (Because that is irrelevant to Ard's use case.)
>
> Perhaps add yet another member function that can disable the Driver#### option processing in the current location.
>
>
> (3) Extend the UEFI specification, section "3.1.3 Load Options".
>
> The LOAD_OPTION_CATEGORY bit-field is almost what we want, except it's specified to be ignored for Driver#### options. So,
>
> - either invent the standardese to let us use LOAD_OPTION_CATEGORY for Driver#### options too, without breaking existing platforms, *or*
> - introduce a new (not too wide) distinct bitfield called LOAD_OPTION_DRIVER_CATEGORY.
>
> Whichever category bitfield proves acceptable, introduce a new "driver category bit" in that bitfield, called LOAD_OPTION_DRIVER_CATEGORY_CONSOLE.
>
> Specify that, if any Driver#### option has the LOAD_OPTION_DRIVER_CATEGORY_CONSOLE attribute set, then Driver#### options will be processed in two passes. In both passes, DriverOrder is to be honored, but the first pass will only consider options with the attribute set, and the second pass will only consider options with the attribute clear.
>
> Implement this in BdsEntry / UefiBootManagerLib.
>
> ... Maybe don't even introduce LOAD_OPTION_DRIVER_CATEGORY*; just a bitfield that is expressly vendor specific?
>
> Thanks
> Laszlo
>

Re: [edk2-devel] [RFC PATCH 2/2] MdeModulePkg/BdsDxe: allow consoles on drivers loaded via Driver####

[Andrew Fish]

[-- Attachment #1: Type: text/plain, Size: 5697 bytes --]

Ard,

Is the problem GFX console? Would it be possible to have a PCD to assume graphics console, and if non was found on the boot connect those PCI devices and update the NVRAM to cause a console to connect. You might have to do a 2nd connect on the GOP handle after the nvram variable was written to make the ConSpliter see it?

Thanks,

Andrew Fish

> On Jan 13, 2020, at 9:28 AM, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> 
> On Fri, 10 Jan 2020 at 17:23, Laszlo Ersek <lersek@redhat.com <mailto:lersek@redhat.com>> wrote:
>> 
>> On 01/10/20 15:37, Ni, Ray wrote:
>>> Ard,
>>> I understand now that: BeforeConsole() needs to connect Gfx to get the GOP device path
>>> for ConOut updating. But the GOP driver is specified in Driver#### and it will be loaded after
>>> BeforeConsole() in today's code. This order makes the Gfx connection fails to start the GOP driver
>>> resulting ConOut not updated.
>>> 
>>> Moving Driver#### processing before BeforeConsole() helps in your case. But it may impact
>>> other platforms: There might be platforms that update Driver#### variables in BeforeConsole()
>>> and expect these drivers are processed in the same boot (not the next boot). I don't have the real
>>> examples. But today's flow allows this. With your change, Driver#### will not be processed in the first
>>> boot. The change impacts these platforms.
>>> 
>>> One backward compatible approach can be: processing Driver#### before BeforeConsole() and processing the new Driver#### (added by BeforeConsole()) after BeforeConsole().
>>> 
>>> But another problem arises: If BeforeConsole() removes a Driver####, platform's expectation is that deleted Driver#### doesn't run. But that driver already runs.
>>> 
>>> So actually the question is: when BDS core can consume the Driver#### and process them?
>>> Right now, it’s the time after BeforeConsole(). Just as right now BeforeConsole() updates ConOut
>>> in your case, BeforeConsole() is the place where platform updates all UEFI defined boot related
>>> variables. Processing Driver#### before BeforeConsole() requires platform updates Driver####
>>> in other places. It's a new requirement to the platform.
>>> 
>>> With all what I explained in above, I cannot agree with the changes.
>>> 
>>> Another approach is:
>>> Platform could connect the GFX in AfterConsole() and update the ConOut. Then platform could manually call EfiBootManagerConnectConsoleVariable (ConOut) to re-connect ConOut.
>>> It's a bit ugly I agree.
>> 
>> Let me raise three other ideas (alternatives to each other, and to the above), with varying levels of annoyance. :)
>> 
> 
> Thanks Laszlo
> 
> Ray, given your objection to my approach, could you please consider
> the below and give feedback on which approach is suitable to address
> the issue I am trying to fix?
> 
>> 
>> (1) Keep the following logic (i.e. the subject of this patch):
>> 
>>  //
>>  // Execute Driver Options
>>  //
>>  LoadOptions = EfiBootManagerGetLoadOptions (&LoadOptionCount, LoadOptionTypeDriver);
>>  ProcessLoadOptions (LoadOptions, LoadOptionCount);
>>  EfiBootManagerFreeLoadOptions (LoadOptions, LoadOptionCount);
>> 
>> in *both* places, but gate each one with a bit in a new bitmask PCD.
>> 
>> (Note: it's probably not the best for any platform to permit both branches, as driver images would be loaded twice.)
>> 
>> 
>> (2) EDKII_PLATFORM_BOOT_MANAGER_PROTOCOL has recently been added to edk2. It looks like a well-designed (extensible) protocol, for two reasons:
>> 
>> - the protocol structure has a Revision field,
>> 
>> - the only current member function, RefreshAllBootOptions(), is permitted to return EFI_UNSUPPORTED -- and the single call site, in the EfiBootManagerRefreshAllBootOption() function, handles that return value well (it does nothing).
>> 
>> The idea would be to bump the Revision field, and add a new member function. Then call this member function (if it exists) in the spot where the current patch proposes to move the Driver#### dispatch logic to.
>> 
>> This is almost like a new PlatformBootManagerLib interface, except it does not require existent lib instances to be updated.
>> 
>> And, on the EDKII_PLATFORM_BOOT_MANAGER_PROTOCOL implementation side, RefreshAllBootOptions() would return EFI_UNSUPPORTED. (Because that is irrelevant to Ard's use case.)
>> 
>> Perhaps add yet another member function that can disable the Driver#### option processing in the current location.
>> 
>> 
>> (3) Extend the UEFI specification, section "3.1.3 Load Options".
>> 
>> The LOAD_OPTION_CATEGORY bit-field is almost what we want, except it's specified to be ignored for Driver#### options. So,
>> 
>> - either invent the standardese to let us use LOAD_OPTION_CATEGORY for Driver#### options too, without breaking existing platforms, *or*
>> - introduce a new (not too wide) distinct bitfield called LOAD_OPTION_DRIVER_CATEGORY.
>> 
>> Whichever category bitfield proves acceptable, introduce a new "driver category bit" in that bitfield, called LOAD_OPTION_DRIVER_CATEGORY_CONSOLE.
>> 
>> Specify that, if any Driver#### option has the LOAD_OPTION_DRIVER_CATEGORY_CONSOLE attribute set, then Driver#### options will be processed in two passes. In both passes, DriverOrder is to be honored, but the first pass will only consider options with the attribute set, and the second pass will only consider options with the attribute clear.
>> 
>> Implement this in BdsEntry / UefiBootManagerLib.
>> 
>> ... Maybe don't even introduce LOAD_OPTION_DRIVER_CATEGORY*; just a bitfield that is expressly vendor specific?
>> 
>> Thanks
>> Laszlo
>> 
> 
> 


[-- Attachment #2: Type: text/html, Size: 13798 bytes --]

Re: [edk2-devel] [RFC PATCH 2/2] MdeModulePkg/BdsDxe: allow consoles on drivers loaded via Driver####

[Ard Biesheuvel]

On Mon, 13 Jan 2020 at 18:57, Andrew Fish via Groups.Io
<afish=apple.com@groups.io> wrote:
>
> Ard,
>
> Is the problem GFX console? Would it be possible to have a PCD to assume graphics console, and if non was found on the boot connect those PCI devices and update the NVRAM to cause a console to connect. You might have to do a 2nd connect on the GOP handle after the nvram variable was written to make the ConSpliter see it?
>

I'm not sure I follow. Do you mean update the console variable if it
doesn't contain the GOP produced by the Driver#### option and reboot?



>
> On Jan 13, 2020, at 9:28 AM, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>
> On Fri, 10 Jan 2020 at 17:23, Laszlo Ersek <lersek@redhat.com> wrote:
>
>
> On 01/10/20 15:37, Ni, Ray wrote:
>
> Ard,
> I understand now that: BeforeConsole() needs to connect Gfx to get the GOP device path
> for ConOut updating. But the GOP driver is specified in Driver#### and it will be loaded after
> BeforeConsole() in today's code. This order makes the Gfx connection fails to start the GOP driver
> resulting ConOut not updated.
>
> Moving Driver#### processing before BeforeConsole() helps in your case. But it may impact
> other platforms: There might be platforms that update Driver#### variables in BeforeConsole()
> and expect these drivers are processed in the same boot (not the next boot). I don't have the real
> examples. But today's flow allows this. With your change, Driver#### will not be processed in the first
> boot. The change impacts these platforms.
>
> One backward compatible approach can be: processing Driver#### before BeforeConsole() and processing the new Driver#### (added by BeforeConsole()) after BeforeConsole().
>
> But another problem arises: If BeforeConsole() removes a Driver####, platform's expectation is that deleted Driver#### doesn't run. But that driver already runs.
>
> So actually the question is: when BDS core can consume the Driver#### and process them?
> Right now, it’s the time after BeforeConsole(). Just as right now BeforeConsole() updates ConOut
> in your case, BeforeConsole() is the place where platform updates all UEFI defined boot related
> variables. Processing Driver#### before BeforeConsole() requires platform updates Driver####
> in other places. It's a new requirement to the platform.
>
> With all what I explained in above, I cannot agree with the changes.
>
> Another approach is:
> Platform could connect the GFX in AfterConsole() and update the ConOut. Then platform could manually call EfiBootManagerConnectConsoleVariable (ConOut) to re-connect ConOut.
> It's a bit ugly I agree.
>
>
> Let me raise three other ideas (alternatives to each other, and to the above), with varying levels of annoyance. :)
>
>
> Thanks Laszlo
>
> Ray, given your objection to my approach, could you please consider
> the below and give feedback on which approach is suitable to address
> the issue I am trying to fix?
>
>
> (1) Keep the following logic (i.e. the subject of this patch):
>
>  //
>  // Execute Driver Options
>  //
>  LoadOptions = EfiBootManagerGetLoadOptions (&LoadOptionCount, LoadOptionTypeDriver);
>  ProcessLoadOptions (LoadOptions, LoadOptionCount);
>  EfiBootManagerFreeLoadOptions (LoadOptions, LoadOptionCount);
>
> in *both* places, but gate each one with a bit in a new bitmask PCD.
>
> (Note: it's probably not the best for any platform to permit both branches, as driver images would be loaded twice.)
>
>
> (2) EDKII_PLATFORM_BOOT_MANAGER_PROTOCOL has recently been added to edk2. It looks like a well-designed (extensible) protocol, for two reasons:
>
> - the protocol structure has a Revision field,
>
> - the only current member function, RefreshAllBootOptions(), is permitted to return EFI_UNSUPPORTED -- and the single call site, in the EfiBootManagerRefreshAllBootOption() function, handles that return value well (it does nothing).
>
> The idea would be to bump the Revision field, and add a new member function. Then call this member function (if it exists) in the spot where the current patch proposes to move the Driver#### dispatch logic to.
>
> This is almost like a new PlatformBootManagerLib interface, except it does not require existent lib instances to be updated.
>
> And, on the EDKII_PLATFORM_BOOT_MANAGER_PROTOCOL implementation side, RefreshAllBootOptions() would return EFI_UNSUPPORTED. (Because that is irrelevant to Ard's use case.)
>
> Perhaps add yet another member function that can disable the Driver#### option processing in the current location.
>
>
> (3) Extend the UEFI specification, section "3.1.3 Load Options".
>
> The LOAD_OPTION_CATEGORY bit-field is almost what we want, except it's specified to be ignored for Driver#### options. So,
>
> - either invent the standardese to let us use LOAD_OPTION_CATEGORY for Driver#### options too, without breaking existing platforms, *or*
> - introduce a new (not too wide) distinct bitfield called LOAD_OPTION_DRIVER_CATEGORY.
>
> Whichever category bitfield proves acceptable, introduce a new "driver category bit" in that bitfield, called LOAD_OPTION_DRIVER_CATEGORY_CONSOLE.
>
> Specify that, if any Driver#### option has the LOAD_OPTION_DRIVER_CATEGORY_CONSOLE attribute set, then Driver#### options will be processed in two passes. In both passes, DriverOrder is to be honored, but the first pass will only consider options with the attribute set, and the second pass will only consider options with the attribute clear.
>
> Implement this in BdsEntry / UefiBootManagerLib.
>
> ... Maybe don't even introduce LOAD_OPTION_DRIVER_CATEGORY*; just a bitfield that is expressly vendor specific?
>
> Thanks
> Laszlo
>
>
>
> 

Re: [edk2-devel] [RFC PATCH 2/2] MdeModulePkg/BdsDxe: allow consoles on drivers loaded via Driver####

[Andrew Fish]

> On Jan 14, 2020, at 8:46 AM, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> 
> On Mon, 13 Jan 2020 at 18:57, Andrew Fish via Groups.Io
> <afish=apple.com@groups.io> wrote:
>> 
>> Ard,
>> 
>> Is the problem GFX console? Would it be possible to have a PCD to assume graphics console, and if non was found on the boot connect those PCI devices and update the NVRAM to cause a console to connect. You might have to do a 2nd connect on the GOP handle after the nvram variable was written to make the ConSpliter see it?
>> 
> 
> I'm not sure I follow. Do you mean update the console variable if it
> doesn't contain the GOP produced by the Driver#### option and reboot?
> 

Ard,

I was thinking this specific case was no active GOP driver in the system due to the emulation of the ROM. I was saying there could be a platform policy to require GOP, and there could be a  platform hook point to check for no GOP and take action. This is not really a Driver#### path, but a fallback path for no GOP. So that is kind of cheating :). 

Also I was thinking the ConSpliter would activate the new GOP if you do a 2nd gBS->ConnectController on the Graphics PCI or GOP handle after updating the console nvram variable. 

Sorry I'm not really familiar with the modern TianoCore BDS, as we have a custom BDS on Macs, so I'm talking more hypothetically about how BDS could work.

Thanks,

Andrew Fish

> 
> 
>> 
>> On Jan 13, 2020, at 9:28 AM, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>> 
>> On Fri, 10 Jan 2020 at 17:23, Laszlo Ersek <lersek@redhat.com> wrote:
>> 
>> 
>> On 01/10/20 15:37, Ni, Ray wrote:
>> 
>> Ard,
>> I understand now that: BeforeConsole() needs to connect Gfx to get the GOP device path
>> for ConOut updating. But the GOP driver is specified in Driver#### and it will be loaded after
>> BeforeConsole() in today's code. This order makes the Gfx connection fails to start the GOP driver
>> resulting ConOut not updated.
>> 
>> Moving Driver#### processing before BeforeConsole() helps in your case. But it may impact
>> other platforms: There might be platforms that update Driver#### variables in BeforeConsole()
>> and expect these drivers are processed in the same boot (not the next boot). I don't have the real
>> examples. But today's flow allows this. With your change, Driver#### will not be processed in the first
>> boot. The change impacts these platforms.
>> 
>> One backward compatible approach can be: processing Driver#### before BeforeConsole() and processing the new Driver#### (added by BeforeConsole()) after BeforeConsole().
>> 
>> But another problem arises: If BeforeConsole() removes a Driver####, platform's expectation is that deleted Driver#### doesn't run. But that driver already runs.
>> 
>> So actually the question is: when BDS core can consume the Driver#### and process them?
>> Right now, it’s the time after BeforeConsole(). Just as right now BeforeConsole() updates ConOut
>> in your case, BeforeConsole() is the place where platform updates all UEFI defined boot related
>> variables. Processing Driver#### before BeforeConsole() requires platform updates Driver####
>> in other places. It's a new requirement to the platform.
>> 
>> With all what I explained in above, I cannot agree with the changes.
>> 
>> Another approach is:
>> Platform could connect the GFX in AfterConsole() and update the ConOut. Then platform could manually call EfiBootManagerConnectConsoleVariable (ConOut) to re-connect ConOut.
>> It's a bit ugly I agree.
>> 
>> 
>> Let me raise three other ideas (alternatives to each other, and to the above), with varying levels of annoyance. :)
>> 
>> 
>> Thanks Laszlo
>> 
>> Ray, given your objection to my approach, could you please consider
>> the below and give feedback on which approach is suitable to address
>> the issue I am trying to fix?
>> 
>> 
>> (1) Keep the following logic (i.e. the subject of this patch):
>> 
>> //
>> // Execute Driver Options
>> //
>> LoadOptions = EfiBootManagerGetLoadOptions (&LoadOptionCount, LoadOptionTypeDriver);
>> ProcessLoadOptions (LoadOptions, LoadOptionCount);
>> EfiBootManagerFreeLoadOptions (LoadOptions, LoadOptionCount);
>> 
>> in *both* places, but gate each one with a bit in a new bitmask PCD.
>> 
>> (Note: it's probably not the best for any platform to permit both branches, as driver images would be loaded twice.)
>> 
>> 
>> (2) EDKII_PLATFORM_BOOT_MANAGER_PROTOCOL has recently been added to edk2. It looks like a well-designed (extensible) protocol, for two reasons:
>> 
>> - the protocol structure has a Revision field,
>> 
>> - the only current member function, RefreshAllBootOptions(), is permitted to return EFI_UNSUPPORTED -- and the single call site, in the EfiBootManagerRefreshAllBootOption() function, handles that return value well (it does nothing).
>> 
>> The idea would be to bump the Revision field, and add a new member function. Then call this member function (if it exists) in the spot where the current patch proposes to move the Driver#### dispatch logic to.
>> 
>> This is almost like a new PlatformBootManagerLib interface, except it does not require existent lib instances to be updated.
>> 
>> And, on the EDKII_PLATFORM_BOOT_MANAGER_PROTOCOL implementation side, RefreshAllBootOptions() would return EFI_UNSUPPORTED. (Because that is irrelevant to Ard's use case.)
>> 
>> Perhaps add yet another member function that can disable the Driver#### option processing in the current location.
>> 
>> 
>> (3) Extend the UEFI specification, section "3.1.3 Load Options".
>> 
>> The LOAD_OPTION_CATEGORY bit-field is almost what we want, except it's specified to be ignored for Driver#### options. So,
>> 
>> - either invent the standardese to let us use LOAD_OPTION_CATEGORY for Driver#### options too, without breaking existing platforms, *or*
>> - introduce a new (not too wide) distinct bitfield called LOAD_OPTION_DRIVER_CATEGORY.
>> 
>> Whichever category bitfield proves acceptable, introduce a new "driver category bit" in that bitfield, called LOAD_OPTION_DRIVER_CATEGORY_CONSOLE.
>> 
>> Specify that, if any Driver#### option has the LOAD_OPTION_DRIVER_CATEGORY_CONSOLE attribute set, then Driver#### options will be processed in two passes. In both passes, DriverOrder is to be honored, but the first pass will only consider options with the attribute set, and the second pass will only consider options with the attribute clear.
>> 
>> Implement this in BdsEntry / UefiBootManagerLib.
>> 
>> ... Maybe don't even introduce LOAD_OPTION_DRIVER_CATEGORY*; just a bitfield that is expressly vendor specific?
>> 
>> Thanks
>> Laszlo
>> 
>> 
>> 
>> 
> 
> 
> 

Re: [edk2-devel] [RFC PATCH 2/2] MdeModulePkg/BdsDxe: allow consoles on drivers loaded via Driver####

[Ni, Ray]

> -----Original Message-----
> From: afish@apple.com <afish@apple.com>
> Sent: Wednesday, January 15, 2020 1:45 AM
> To: devel@edk2.groups.io; Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Laszlo Ersek <lersek@redhat.com>; Ni, Ray <ray.ni@intel.com>; Leif
> Lindholm <leif.lindholm@linaro.org>; Gao, Zhichao <zhichao.gao@intel.com>;
> Ma, Maurice <maurice.ma@intel.com>; Dong, Guo <guo.dong@intel.com>;
> You, Benjamin <benjamin.you@intel.com>
> Subject: Re: [edk2-devel] [RFC PATCH 2/2] MdeModulePkg/BdsDxe: allow
> consoles on drivers loaded via Driver####
> 
> 
> 
> > On Jan 14, 2020, at 8:46 AM, Ard Biesheuvel <ard.biesheuvel@linaro.org>
> wrote:
> >
> > On Mon, 13 Jan 2020 at 18:57, Andrew Fish via Groups.Io
> > <afish=apple.com@groups.io> wrote:
> >>
> >> Ard,
> >>
> >> Is the problem GFX console? Would it be possible to have a PCD to
> assume graphics console, and if non was found on the boot connect those
> PCI devices and update the NVRAM to cause a console to connect. You might
> have to do a 2nd connect on the GOP handle after the nvram variable was
> written to make the ConSpliter see it?
> >>
> >
> > I'm not sure I follow. Do you mean update the console variable if it
> > doesn't contain the GOP produced by the Driver#### option and reboot?
> >
> 
> Ard,
> 
> I was thinking this specific case was no active GOP driver in the system due to
> the emulation of the ROM. I was saying there could be a platform policy to
> require GOP, and there could be a  platform hook point to check for no GOP
> and take action. This is not really a Driver#### path, but a fallback path for no
> GOP. So that is kind of cheating :).
> 
> Also I was thinking the ConSpliter would activate the new GOP if you do a
> 2nd gBS->ConnectController on the Graphics PCI or GOP handle after
> updating the console nvram variable.
> 
> Sorry I'm not really familiar with the modern TianoCore BDS, as we have a
> custom BDS on Macs, so I'm talking more hypothetically about how BDS could
> work.

Andrew,
TianoCore BDS in this part is very simple. The BDS core expects a platform
hook (PlatformBootManagerBeforeConsole()) to initialize every BDS related UEFI
variables like ConIn, ConOut, Driver####, Boot####, DriverOrder, BootOrder, etc.
Then BDS core consumes these variables and acts accordingly.
I think the "platform hook" you suggested is similar to what I suggested earlier,
except that you think that putting GOP in Driver#### is unnecessary.

Ard,
Assuming you've put GOP driver path to Driver####,
you could put below code in the beginning of PlatformBootManagerAfterConsole():
1. Find the GFX controller handle
2. EfiBootManagerConnectVideoController (GFXHandle)

Thanks,
Ray

Re: [edk2-devel] [RFC PATCH 2/2] MdeModulePkg/BdsDxe: allow consoles on drivers loaded via Driver####

[Ni, Ray]

Laszlo,
I tend to make it a platform local implementation first.
Your proposals impact a broader scope which we could
consider later if it's hard for a platform to handle the issue.

Thanks,
Ray

> -----Original Message-----
> From: Laszlo Ersek <lersek@redhat.com>
> Sent: Saturday, January 11, 2020 12:24 AM
> To: Ni, Ray <ray.ni@intel.com>; 'devel@edk2.groups.io'
> <devel@edk2.groups.io>; 'ard.biesheuvel@linaro.org'
> <ard.biesheuvel@linaro.org>
> Cc: 'Leif Lindholm' <leif.lindholm@linaro.org>; Gao, Zhichao
> <zhichao.gao@intel.com>; Ma, Maurice <maurice.ma@intel.com>; Dong,
> Guo <guo.dong@intel.com>; You, Benjamin <benjamin.you@intel.com>
> Subject: Re: [edk2-devel] [RFC PATCH 2/2] MdeModulePkg/BdsDxe: allow
> consoles on drivers loaded via Driver####
> 
> On 01/10/20 15:37, Ni, Ray wrote:
> > Ard,
> > I understand now that: BeforeConsole() needs to connect Gfx to get the
> GOP device path
> > for ConOut updating. But the GOP driver is specified in Driver#### and it
> will be loaded after
> > BeforeConsole() in today's code. This order makes the Gfx connection fails
> to start the GOP driver
> > resulting ConOut not updated.
> >
> > Moving Driver#### processing before BeforeConsole() helps in your case.
> But it may impact
> > other platforms: There might be platforms that update Driver####
> variables in BeforeConsole()
> > and expect these drivers are processed in the same boot (not the next
> boot). I don't have the real
> > examples. But today's flow allows this. With your change, Driver#### will
> not be processed in the first
> > boot. The change impacts these platforms.
> >
> > One backward compatible approach can be: processing Driver#### before
> BeforeConsole() and processing the new Driver#### (added by
> BeforeConsole()) after BeforeConsole().
> >
> > But another problem arises: If BeforeConsole() removes a Driver####,
> platform's expectation is that deleted Driver#### doesn't run. But that driver
> already runs.
> >
> > So actually the question is: when BDS core can consume the Driver####
> and process them?
> > Right now, it’s the time after BeforeConsole(). Just as right now
> BeforeConsole() updates ConOut
> > in your case, BeforeConsole() is the place where platform updates all UEFI
> defined boot related
> > variables. Processing Driver#### before BeforeConsole() requires platform
> updates Driver####
> > in other places. It's a new requirement to the platform.
> >
> > With all what I explained in above, I cannot agree with the changes.
> >
> > Another approach is:
> > Platform could connect the GFX in AfterConsole() and update the ConOut.
> Then platform could manually call EfiBootManagerConnectConsoleVariable
> (ConOut) to re-connect ConOut.
> > It's a bit ugly I agree.
> 
> Let me raise three other ideas (alternatives to each other, and to the above),
> with varying levels of annoyance. :)
> 
> 
> (1) Keep the following logic (i.e. the subject of this patch):
> 
>   //
>   // Execute Driver Options
>   //
>   LoadOptions = EfiBootManagerGetLoadOptions (&LoadOptionCount,
> LoadOptionTypeDriver);
>   ProcessLoadOptions (LoadOptions, LoadOptionCount);
>   EfiBootManagerFreeLoadOptions (LoadOptions, LoadOptionCount);
> 
> in *both* places, but gate each one with a bit in a new bitmask PCD.
> 
> (Note: it's probably not the best for any platform to permit both branches, as
> driver images would be loaded twice.)
> 
> 
> (2) EDKII_PLATFORM_BOOT_MANAGER_PROTOCOL has recently been
> added to edk2. It looks like a well-designed (extensible) protocol, for two
> reasons:
> 
> - the protocol structure has a Revision field,
> 
> - the only current member function, RefreshAllBootOptions(), is permitted to
> return EFI_UNSUPPORTED -- and the single call site, in the
> EfiBootManagerRefreshAllBootOption() function, handles that return value
> well (it does nothing).
> 
> The idea would be to bump the Revision field, and add a new member
> function. Then call this member function (if it exists) in the spot where the
> current patch proposes to move the Driver#### dispatch logic to.
> 
> This is almost like a new PlatformBootManagerLib interface, except it does
> not require existent lib instances to be updated.
> 
> And, on the EDKII_PLATFORM_BOOT_MANAGER_PROTOCOL
> implementation side, RefreshAllBootOptions() would return
> EFI_UNSUPPORTED. (Because that is irrelevant to Ard's use case.)
> 
> Perhaps add yet another member function that can disable the Driver####
> option processing in the current location.
> 
> 
> (3) Extend the UEFI specification, section "3.1.3 Load Options".
> 
> The LOAD_OPTION_CATEGORY bit-field is almost what we want, except it's
> specified to be ignored for Driver#### options. So,
> 
> - either invent the standardese to let us use LOAD_OPTION_CATEGORY for
> Driver#### options too, without breaking existing platforms, *or*
> - introduce a new (not too wide) distinct bitfield called
> LOAD_OPTION_DRIVER_CATEGORY.
> 
> Whichever category bitfield proves acceptable, introduce a new "driver
> category bit" in that bitfield, called
> LOAD_OPTION_DRIVER_CATEGORY_CONSOLE.
> 
> Specify that, if any Driver#### option has the
> LOAD_OPTION_DRIVER_CATEGORY_CONSOLE attribute set, then
> Driver#### options will be processed in two passes. In both passes,
> DriverOrder is to be honored, but the first pass will only consider options
> with the attribute set, and the second pass will only consider options with
> the attribute clear.
> 
> Implement this in BdsEntry / UefiBootManagerLib.
> 
> ... Maybe don't even introduce LOAD_OPTION_DRIVER_CATEGORY*; just a
> bitfield that is expressly vendor specific?
> 
> Thanks
> Laszlo