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

["Ni, Ray" <ray.ni@intel.com>]

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