On Fri, 10 Jan 2020 at 17:23, Laszlo Ersek <ler...@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 > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#53192): https://edk2.groups.io/g/devel/message/53192 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] -=-=-=-=-=-=-=-=-=-=-=-
