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

["Ard Biesheuvel" <ard.biesheuvel@linaro.org>]

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
>