From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from nwk-aaemail-lapp02.apple.com (nwk-aaemail-lapp02.apple.com [17.151.62.67]) by mx.groups.io with SMTP id smtpd.web10.3602.1573008452121697753 for ; Tue, 05 Nov 2019 18:47:32 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@apple.com header.s=20180706 header.b=wkNrI1VM; spf=pass (domain: apple.com, ip: 17.151.62.67, mailfrom: afish@apple.com) Received: from pps.filterd (nwk-aaemail-lapp02.apple.com [127.0.0.1]) by nwk-aaemail-lapp02.apple.com (8.16.0.27/8.16.0.27) with SMTP id xA62lTrB054898; Tue, 5 Nov 2019 18:47:30 -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=lyyjbKzKUMT4+c5fcyN6Ytp5RsLuRbg6cBnYKqyB5aU=; b=wkNrI1VMk8p8g7SnuL1jpwvsHMdQexkwFcAK9vjEqqde28gm/x9QKPMa2QtsLQsP83H7 GL+p4anY7C7qvNXaKyKsjIJFEyUZFmAGdsmDnTEwKAYAtatpK/CyCnmER19o6Wj+G1iA d4P/RqA0kwiwmOeovHT9bDo4ITUNBz1zp9vyK0mYbiEm2UgMUC7RKvu9JRESpr3V1eTA lDMwnGh08EBtkaIMparsrQIxnE8qDHjEfLM5ZlNRnCIk/NIgYAU8fVNe7Xt20tm3/ce1 f2tUzWk5qvoKp2TLxSshJjnLIJPaECYo1XQQFkMd4D+SOMrdxy5mJPAxCIC0ZQYjI576 ig== Received: from ma1-mtap-s03.corp.apple.com (ma1-mtap-s03.corp.apple.com [17.40.76.7]) by nwk-aaemail-lapp02.apple.com with ESMTP id 2w16cjw3ru-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128 verify=NO); Tue, 05 Nov 2019 18:47:30 -0800 Received: from nwk-mmpp-sz13.apple.com (nwk-mmpp-sz13.apple.com [17.128.115.216]) by ma1-mtap-s03.corp.apple.com (Oracle Communications Messaging Server 8.0.2.4.20190507 64bit (built May 7 2019)) with ESMTPS id <0Q0J00I0T12ZH200@ma1-mtap-s03.corp.apple.com>; Tue, 05 Nov 2019 18:47:24 -0800 (PST) Received: from process_milters-daemon.nwk-mmpp-sz13.apple.com by nwk-mmpp-sz13.apple.com (Oracle Communications Messaging Server 8.0.2.4.20190507 64bit (built May 7 2019)) id <0Q0J00K000STXD00@nwk-mmpp-sz13.apple.com>; Tue, 05 Nov 2019 18:47:23 -0800 (PST) X-Va-A: X-Va-T-CD: dd832fa7197a7045cc604b66a4d528ed X-Va-E-CD: be490e393b9a0fc46442780b7330e634 X-Va-R-CD: 080c83ebb59fdc40182a21f863857514 X-Va-CD: 0 X-Va-ID: 16c4cbb1-0580-4d8c-867f-f50ed020ab5d X-V-A: X-V-T-CD: dd832fa7197a7045cc604b66a4d528ed X-V-E-CD: be490e393b9a0fc46442780b7330e634 X-V-R-CD: 080c83ebb59fdc40182a21f863857514 X-V-CD: 0 X-V-ID: 29500e42-4b8b-4e9a-962b-096c36cd14a9 X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:,, definitions=2019-11-05_09:,, signatures=0 Received: from [17.235.77.209] (unknown [17.235.77.209]) by nwk-mmpp-sz13.apple.com (Oracle Communications Messaging Server 8.0.2.4.20190507 64bit (built May 7 2019)) with ESMTPSA id <0Q0J00CYR12W1HA0@nwk-mmpp-sz13.apple.com>; Tue, 05 Nov 2019 18:47:22 -0800 (PST) Sender: afish@apple.com From: "Andrew Fish" Message-id: <746A8D5E-DC45-4D39-9C4D-97A10BE2E0B0@apple.com> MIME-version: 1.0 (Mac OS X Mail 13.0 \(3594.4.17\)) Subject: Re: [edk2-devel] [PATCH] Support skipping automatic BM enumeration Date: Tue, 05 Nov 2019 20:47:20 -0600 In-reply-to: Cc: Ashish Singhal , Laszlo Ersek , "Ni, Ray" , "Wang, Jian J" , "Wu, Hao A" , "Gao, Zhichao" , Mike Kinney To: devel@edk2.groups.io, jbrasen@nvidia.com References: <1b91c052-f64c-1dca-98ff-a2777afd7f77@redhat.com> <734D49CCEBEEF84792F5B80ED585239D5C34F98A@SHSMSX104.ccr.corp.intel.com> <6766B443-E14A-4F57-984E-5A865FB22CC9@apple.com> <37D801DD-41E8-452E-9F24-ADF52BFDB676@apple.com> <72ce1d71-2a65-a6c0-1dd8-7628429c5a3c@redhat.com> X-Mailer: Apple Mail (2.3594.4.17) X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:,, definitions=2019-11-05_09:,, signatures=0 Content-type: multipart/alternative; boundary="Apple-Mail=_3FFE9A1F-7DCE-422C-A8DA-B6A6A80AF3D1" --Apple-Mail=_3FFE9A1F-7DCE-422C-A8DA-B6A6A80AF3D1 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=us-ascii > On Nov 5, 2019, at 7:34 PM, Jeff Brasen wrote: >=20 >=20 > Wouldn't having a variable that we create and delete on every boot put u= nnecessary stress on the SPI-NOR that the variable store lives on? > What about the alternative approach where we allow the platform code to = modify the attributes of the auto created variable to disable it with hidde= n/!active but still match for detection purposes so that it doesn't delete = and recreate the modified variable each boot? That way all the logic on wha= t to disable can still be in the platform code and all the existing logic i= n the boot manager can stay basically the same? >=20 > What changes every boot that forces the variable to need to get modified= ?=20 >=20 > I would assume the NOR driver is smart enough to not update a variable t= hat is not changing.=20 >=20 > The custom BDS could could only create the variable for this device if i= t does not exist.=20 >=20 > [JB] The current flow with no changes in the boot manager would be as fo= llows >=20 > Scan for instance of the boot option in the variables > It will not be found, so create a new boot option store it to a variable= and update BootOrder > Platform code runs creates the options for the boot option it wants and = writes those to variable store > Delete/disable the boot option in the variable store >=20 > When you reboot it won't find the variable so 1/2/4 will re-occur >=20 > The code that does this (1/2) is EfiBootManagerRefreshAllBootOption in B= mBoot.c >=20 > If you modify the variable to disable it with hidden/not active it would= delete that and create a new one as well as the code wouldn't recognize th= at is the same boot option. >=20 > If however we modify EfiBootManagerFindLoadOption to not compare the att= ributes (at least allow for differences in active and hidden) then the when= it refreshes every thing it would see the match and not delete/create a ne= w variable in the store and thus we wouldn't have changes every boot. >=20 Jeff, Sorry if I'm a little off on the sequence of things as the platform I work= on day to day has a custom BDS and does not use this library..... I though= the patch changed BmEnumerateBootOptions(), so that is going to change how= EfiBootManagerRefreshAllBootOption() works. I'd also point out the patch a= s given is invalid as it changed the behavior of the public library API for= EfiBootManagerRefreshAllBootOption() [1] so for the patch to be valid it w= ould need to change the comments to reflect the new behavior. This is kind = of what Laszlo's technical debt comment was about.=20 I think Laszlo advocated having the BDS platform specific code make sure t= he boot variables are in the correct state. That should happen before the B= oot Manager code runs, and it is not clear to me why the Boot Manager coul= d would need to run if you have a valid EFI nvram variable to boot from.=20 I think the question is how is your use case different than the boot varia= ble that Windows installs? If it works kind of the same way then the answer= is to have the BDS platform specific code write the boot variable.=20 [1] /** The function creates boot options for all possible bootable medias in th= e following order: 1. Removable BlockIo - The boot option only points to the rem= ovable media device, like USB key, DVD, Floppy etc. 2. Fixed BlockIo - The boot option only points to a Fixed= blockIo device, like HardDisk. 3. Non-BlockIo SimpleFileSystem - The boot option points to a device sup= porting SimpleFileSystem Protocol, but not sup= porting BlockIo protocol. 4. LoadFile - The boot option points to the media su= pporting LoadFile protocol. Reference: UEFI Spec chapter 3.3 Boot Option Variables Default Boot Beha= vior The function won't delete the boot option not added by itself. **/ VOID EFIAPI EfiBootManagerRefreshAllBootOption ( VOID ); Thanks, Andrew Fish >=20 > Thanks, >=20 > Andrew Fish >=20 > Thanks, >=20 > Jeff >=20 >=20 > This email message is for the sole use of the intended recipient(s) and = may contain confidential information. Any unauthorized review, use, disclo= sure or distribution is prohibited. If you are not the intended recipient,= please contact the sender by reply email and destroy all copies of the ori= ginal message. >=20 --Apple-Mail=_3FFE9A1F-7DCE-422C-A8DA-B6A6A80AF3D1 Content-Transfer-Encoding: quoted-printable Content-Type: text/html; charset=us-ascii
On Nov 5,= 2019, at 7:34 PM, Jeff Brasen <jbrasen@nvidia.com> wrote:


Wouldn't having a variable that we create and delete= on every boot put unnecessary stress on the SPI-NOR that the variable stor= e lives on?
What about the alternative approach where we allo= w the platform code to modify the attributes of the auto created variable t= o disable it with hidden/!active but still match for detection purposes so = that it doesn't delete and recreate the modified variable each boot? That w= ay all the logic on what to disable can still be in the platform code and a= ll the existing logic in the boot manager can stay basically the same?

What changes every boot that forces the variable t= o need to get modified? <= br class=3D"">
I would assume the NOR driver is smart enough = to not update a variable that is not changing. 

The custom BDS could co= uld only create the variable for this device if it does not exist. 

[JB= ] The current flow with no changes in the boot manager would be as follows<= /div>

  1. Scan for instance of the boot option in the= variables
  2. It will not be found, so create a new boot option store it to a = variable and update BootOrder
  3. Platform code runs creates the options for th= e boot option it wants and writes those to variable store
  4. Delete/disable th= e boot option in the variable store

When you reboot= it won't find the variable so 1/2/4 will re-occur

T= he code that does this (1/2) is EfiBootManagerRefreshAllBootOption in = BmBoot.c

If you modify the variable to disable it with = hidden/not active it would delete that and create a new one as well as the = code wouldn't recognize that is the same boot option.

If however we modify EfiBootManagerFindLoadOption to not compar= e the attributes (at least allow for differences in active and hidden) then= the when it refreshes every thing it would see the match and not delete/cr= eate a new variable in the store and thus we wouldn't have changes every bo= ot.


Jeff,

Sorry if I'm a = little off on the sequence of things as the platform I work on day to day h= as a custom BDS and does not use this library..... I though the patch chang= ed BmEnumerateBootOptions(), so that is going to change how EfiBo= otManagerRefreshAllBootOption() works. I'd also point out the patch as give= n is invalid as it changed the behavior of the public library API for EfiBo= otManagerRefreshAllBootOption() [1] so for the patch to be valid it would n= eed to change the comments to reflect the new behavior. This is kind of wha= t Laszlo's technical debt comment was about. 

I think Laszlo advocated having the BDS platform specific code = make sure the boot variables are in the correct state. That should happen b= efore the Boot Manager code runs, and it is  not clear to me why the B= oot Manager could would need to run if you have a valid EFI nvram variable = to boot from. 

I think the questio= n is how is your use case different than the boot variable that Windows ins= talls? If it works kind of the same way then the answer is to have the BDS = platform specific code write the boot variable. 


[1]
/**
  The function creates boot options for all possible bootable m= edias in the following order:
  1. Removable BlockIo   =          - The boot option only points to the remo= vable media
              &nbs= p;                     de= vice, like USB key, DVD, Floppy etc.
  2. Fixed BlockIo &nbs= p;              - The boot option only p= oints to a Fixed blockIo device,
        &nbs= p;                     &n= bsp;     like HardDisk.
  3. Non-BlockIo SimpleFil= eSystem - The boot option points to a device supporting
  &n= bsp;                     =             SimpleFileSystem Protocol, but no= t supporting BlockIo
            &n= bsp;                     =   protocol.
  4. LoadFile         &= nbsp;           - The boot option points to the me= dia supporting
              &= nbsp;                    = LoadFile protocol.
  Reference: UEFI Spec chapter 3.3 Boot = Option Variables Default Boot Behavior

=   The function won't delete the boot option not added by itself.
=
**/
VOID
EFIAPI
EfiBootManagerRefreshAll= BootOption (
  VOID
  );

Thanks,

Andrew Fish


Thanks,

Andrew Fish

Thanks,=

Jeff


=
This email message is for the sole use of the int= ended recipient(s) and may contain confidential information.  Any unau= thorized review, use, disclosure or distribution is prohibited.  If yo= u are not the intended recipient, please contact the sender by reply email = and destroy all copies of the original message.

--Apple-Mail=_3FFE9A1F-7DCE-422C-A8DA-B6A6A80AF3D1--