From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from ma1-aaemail-dr-lapp03.apple.com (ma1-aaemail-dr-lapp03.apple.com [17.171.2.72]) by mx.groups.io with SMTP id smtpd.web12.42197.1578938257955228230 for ; Mon, 13 Jan 2020 09:57:38 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@apple.com header.s=20180706 header.b=SVzf7+K3; spf=pass (domain: apple.com, ip: 17.171.2.72, mailfrom: afish@apple.com) Received: from pps.filterd (ma1-aaemail-dr-lapp03.apple.com [127.0.0.1]) by ma1-aaemail-dr-lapp03.apple.com (8.16.0.27/8.16.0.27) with SMTP id 00DHc6k3062254; Mon, 13 Jan 2020 09:57:36 -0800 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=apple.com; h=sender : from : message-id : content-type : mime-version : subject : date : in-reply-to : cc : to : references; s=20180706; bh=DiXee/kiMcsjeAc/NcllIp5Hfu21JVKPQFFuC5MQrH0=; b=SVzf7+K34uriT4cMGlrffQPEDSqQQP0TbmNFpKsPfB/JbGuqQCUqiF/nx5oENGibQa66 kIY1jPUlyGOPn3X9ofgvM3p41scxpybX53Z6eaU0UDdHqmTv6L1LpL/IKEEE74cbVE+T 23GtW1iszWV0b0yD6iAsBogW+JUiqKn1pOkko5pxQq+PCokTYCgarf+ewiJmaK5cBgws CqbO7T+CHfTjo4Xfty3zRbRrxb3yyIl0V6w2UODKNicg7qclD+AEsUiKOBxq71sCMdTD Df94N+4WxqCA+82fkQupN6xt5mMhFPoGK+JsMK8vfUCVERp4zWSOKM7yPCM66ykR2fKB CA== Received: from ma1-mtap-s02.corp.apple.com (ma1-mtap-s02.corp.apple.com [17.40.76.6]) by ma1-aaemail-dr-lapp03.apple.com with ESMTP id 2xfe7xw2s5-2 (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128 verify=NO); Mon, 13 Jan 2020 09:57:35 -0800 Received: from nwk-mmpp-sz12.apple.com (nwk-mmpp-sz12.apple.com [17.128.115.204]) by ma1-mtap-s02.corp.apple.com (Oracle Communications Messaging Server 8.0.2.4.20190507 64bit (built May 7 2019)) with ESMTPS id <0Q4200IIW4JVIO50@ma1-mtap-s02.corp.apple.com>; Mon, 13 Jan 2020 09:57:35 -0800 (PST) Received: from process_milters-daemon.nwk-mmpp-sz12.apple.com by nwk-mmpp-sz12.apple.com (Oracle Communications Messaging Server 8.0.2.4.20190507 64bit (built May 7 2019)) id <0Q42000004FVG700@nwk-mmpp-sz12.apple.com>; Mon, 13 Jan 2020 09:57:35 -0800 (PST) X-Va-A: X-Va-T-CD: 650d61ef17a2247d3ba567199bcad85e X-Va-E-CD: a8eebd4dd0e994b0e21db70f44624896 X-Va-R-CD: faabe6adb46224738b5248b7e4027d4b X-Va-CD: 0 X-Va-ID: ac0e5fb1-f3c7-4f57-8e26-f3363b258afb X-V-A: X-V-T-CD: 650d61ef17a2247d3ba567199bcad85e X-V-E-CD: a8eebd4dd0e994b0e21db70f44624896 X-V-R-CD: faabe6adb46224738b5248b7e4027d4b X-V-CD: 0 X-V-ID: 640f7743-7304-4138-8f67-d7ffb609c048 X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:,, definitions=2020-01-13_06:,, signatures=0 Received: from [17.235.60.129] (unknown [17.235.60.129]) by nwk-mmpp-sz12.apple.com (Oracle Communications Messaging Server 8.0.2.4.20190507 64bit (built May 7 2019)) with ESMTPSA id <0Q4200LEW4JXDH20@nwk-mmpp-sz12.apple.com>; Mon, 13 Jan 2020 09:57:35 -0800 (PST) Sender: afish@apple.com From: "Andrew Fish" Message-id: <40949126-F9E5-4B0A-B07D-D9CBF7A1EE3B@apple.com> MIME-version: 1.0 (Mac OS X Mail 13.0 \(3594.4.17\)) Subject: Re: [edk2-devel] [RFC PATCH 2/2] MdeModulePkg/BdsDxe: allow consoles on drivers loaded via Driver#### Date: Mon, 13 Jan 2020 09:57:33 -0800 In-reply-to: Cc: Laszlo Ersek , "Ni, Ray" , Leif Lindholm , "Gao, Zhichao" , "Ma, Maurice" , "Dong, Guo" , "You, Benjamin" To: devel@edk2.groups.io, Ard Biesheuvel References: <20191206143128.19371-1-ard.biesheuvel@linaro.org> <20191206143128.19371-3-ard.biesheuvel@linaro.org> <8d49cca6-b1b9-cddc-0590-4e4a84643e4f@redhat.com> <734D49CCEBEEF84792F5B80ED585239D5C39604F@SHSMSX104.ccr.corp.intel.com> <734D49CCEBEEF84792F5B80ED585239D5C399C9C@SHSMSX104.ccr.corp.intel.com> <734D49CCEBEEF84792F5B80ED585239D5C3E2179@SHSMSX104.ccr.corp.intel.com> <734D49CCEBEEF84792F5B80ED585239D5C3EFAF4@SHSMSX104.ccr.corp.intel.com> X-Mailer: Apple Mail (2.3594.4.17) X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:,, definitions=2020-01-13_06:,, signatures=0 Content-type: multipart/alternative; boundary="Apple-Mail=_E027EF39-C79C-44F5-8B69-591193A21BBC" --Apple-Mail=_E027EF39-C79C-44F5-8B69-591193A21BBC Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=utf-8 Ard, Is the problem GFX console? Would it be possible to have a PCD to assume g= raphics 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 = wrote: >=20 > On Fri, 10 Jan 2020 at 17:23, Laszlo Ersek > wrote: >>=20 >> 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 f= ails to start the GOP driver >>> resulting ConOut not updated. >>>=20 >>> Moving Driver#### processing before BeforeConsole() helps in your case= . But it may impact >>> other platforms: There might be platforms that update Driver#### varia= bles 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#### w= ill not be processed in the first >>> boot. The change impacts these platforms. >>>=20 >>> One backward compatible approach can be: processing Driver#### before = BeforeConsole() and processing the new Driver#### (added by BeforeConsole()= ) after BeforeConsole(). >>>=20 >>> But another problem arises: If BeforeConsole() removes a Driver####, p= latform's expectation is that deleted Driver#### doesn't run. But that driv= er already runs. >>>=20 >>> So actually the question is: when BDS core can consume the Driver#### = and process them? >>> Right now, it=E2=80=99s 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 platf= orm updates Driver#### >>> in other places. It's a new requirement to the platform. >>>=20 >>> With all what I explained in above, I cannot agree with the changes. >>>=20 >>> Another approach is: >>> Platform could connect the GFX in AfterConsole() and update the ConOut= . Then platform could manually call EfiBootManagerConnectConsoleVariable (C= onOut) to re-connect ConOut. >>> It's a bit ugly I agree. >>=20 >> Let me raise three other ideas (alternatives to each other, and to the = above), with varying levels of annoyance. :) >>=20 >=20 > Thanks Laszlo >=20 > 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? >=20 >>=20 >> (1) Keep the following logic (i.e. the subject of this patch): >>=20 >> // >> // Execute Driver Options >> // >> LoadOptions =3D EfiBootManagerGetLoadOptions (&LoadOptionCount, LoadOp= tionTypeDriver); >> ProcessLoadOptions (LoadOptions, LoadOptionCount); >> EfiBootManagerFreeLoadOptions (LoadOptions, LoadOptionCount); >>=20 >> in *both* places, but gate each one with a bit in a new bitmask PCD. >>=20 >> (Note: it's probably not the best for any platform to permit both branc= hes, as driver images would be loaded twice.) >>=20 >>=20 >> (2) EDKII_PLATFORM_BOOT_MANAGER_PROTOCOL has recently been added to edk= 2. It looks like a well-designed (extensible) protocol, for two reasons: >>=20 >> - the protocol structure has a Revision field, >>=20 >> - the only current member function, RefreshAllBootOptions(), is permitt= ed to return EFI_UNSUPPORTED -- and the single call site, in the EfiBootMan= agerRefreshAllBootOption() function, handles that return value well (it doe= s nothing). >>=20 >> The idea would be to bump the Revision field, and add a new member func= tion. Then call this member function (if it exists) in the spot where the c= urrent patch proposes to move the Driver#### dispatch logic to. >>=20 >> This is almost like a new PlatformBootManagerLib interface, except it d= oes not require existent lib instances to be updated. >>=20 >> And, on the EDKII_PLATFORM_BOOT_MANAGER_PROTOCOL implementation side, R= efreshAllBootOptions() would return EFI_UNSUPPORTED. (Because that is irrel= evant to Ard's use case.) >>=20 >> Perhaps add yet another member function that can disable the Driver####= option processing in the current location. >>=20 >>=20 >> (3) Extend the UEFI specification, section "3.1.3 Load Options". >>=20 >> The LOAD_OPTION_CATEGORY bit-field is almost what we want, except it's = specified to be ignored for Driver#### options. So, >>=20 >> - 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_D= RIVER_CATEGORY. >>=20 >> Whichever category bitfield proves acceptable, introduce a new "driver = category bit" in that bitfield, called LOAD_OPTION_DRIVER_CATEGORY_CONSOLE. >>=20 >> Specify that, if any Driver#### option has the LOAD_OPTION_DRIVER_CATEG= ORY_CONSOLE attribute set, then Driver#### options will be processed in two= passes. In both passes, DriverOrder is to be honored, but the first pass w= ill only consider options with the attribute set, and the second pass will = only consider options with the attribute clear. >>=20 >> Implement this in BdsEntry / UefiBootManagerLib. >>=20 >> ... Maybe don't even introduce LOAD_OPTION_DRIVER_CATEGORY*; just a bit= field that is expressly vendor specific? >>=20 >> Thanks >> Laszlo >>=20 >=20 >=20 --Apple-Mail=_E027EF39-C79C-44F5-8B69-591193A21BBC Content-Transfer-Encoding: quoted-printable Content-Type: text/html; charset=utf-8 Ard,

Is the problem GFX console? Would it be pos= sible 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 nvr= am 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, Lasz= lo Ersek <lersek@redhat.com> wrote:<= br style=3D"caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 1= 2px; font-style: normal; font-variant-caps: normal; font-weight: normal; le= tter-spacing: normal; text-align: start; text-indent: 0px; text-transform: = none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0p= x; text-decoration: none;" class=3D"">

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. B= ut the GOP driver is specified in Driver#### and it will be loaded afterBeforeConsole() in today's code. This order makes the Gfx conne= ction fails to start the GOP driver
resulting ConOut not upda= ted.

Moving Driver#### processing before Befor= eConsole() helps in your case. But it may impact
other platfo= rms: There might be platforms that update Driver#### variables in BeforeCon= sole()
and expect these drivers are processed in the same boo= t (not the next boot). I don't have the real
examples. But to= day'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 alrea= dy runs.

So actually the question is: when BDS= core can consume the Driver#### and process them?
Right now,= it=E2=80=99s the time after BeforeConsole(). Just as right now BeforeConso= le() updates ConOut
in your case, BeforeConsole() is the plac= e where platform updates all UEFI defined boot related
variab= les. Processing Driver#### before BeforeConsole() requires platform updates= Driver####
in other places. It's a new requirement to the pl= atform.

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

Another approac= h is:
Platform could connect the GFX in AfterConsole() and up= date the ConOut. Then platform could manually call EfiBootManagerConnectCon= soleVariable (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 o= f 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 =3D EfiBootManagerGetLoa= dOptions (&LoadOptionCount, LoadOptionTypeDriver);
 = ProcessLoadOptions (LoadOptions, LoadOptionCount);
 EfiB= ootManagerFreeLoadOptions (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 an= y platform to permit both branches, as driver images would be loaded twice.= )


(2) EDKII_PLATFORM_BOOT_MANAG= ER_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 ret= urn EFI_UNSUPPORTED -- and the single call site, in the EfiBootManagerRefre= shAllBootOption() function, handles that return value well (it does nothing= ).

The idea would be to bump the Revision fiel= d, and add a new member function. Then call this member function (if it exi= sts) in the spot where the current patch proposes to move the Driver#### di= spatch logic to.

This is almost like a new Pla= tformBootManagerLib interface, except it does not require existent lib inst= ances to be updated.

And, on the EDKII_PLATFOR= M_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, sec= tion "3.1.3 Load Options".

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

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

Whichever category bitfield proves acceptable, int= roduce a new "driver category bit" in that bitfield, called LOAD_OPTION_DRI= VER_CATEGORY_CONSOLE.

Specify that, if any Dri= ver#### option has the LOAD_OPTION_DRIVER_CATEGORY_CONSOLE attribute set, t= hen Driver#### options will be processed in two passes. In both passes, Dri= verOrder is to be honored, but the first pass will only consider options wi= th the attribute set, and the second pass will only consider options with t= he attribute clear.

Implement this in BdsEntry= / UefiBootManagerLib.

... Maybe don't even in= troduce LOAD_OPTION_DRIVER_CATEGORY*; just a bitfield that is expressly ven= dor specific?

Thanks
Laszlo



--Apple-Mail=_E027EF39-C79C-44F5-8B69-591193A21BBC--