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 <ler...@redhat.com> > Sent: Saturday, January 11, 2020 12:24 AM > To: Ni, Ray <ray...@intel.com>; 'devel@edk2.groups.io' > <devel@edk2.groups.io>; 'ard.biesheu...@linaro.org' > <ard.biesheu...@linaro.org> > Cc: 'Leif Lindholm' <leif.lindh...@linaro.org>; Gao, Zhichao > <zhichao....@intel.com>; Ma, Maurice <maurice...@intel.com>; Dong, > Guo <guo.d...@intel.com>; You, Benjamin <benjamin....@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 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#53257): https://edk2.groups.io/g/devel/message/53257 Mute This Topic: https://groups.io/mt/67470372/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
