From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by mx.groups.io with SMTP id smtpd.web09.4041.1573010457081462406 for ; Tue, 05 Nov 2019 19:20:57 -0800 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: intel.com, ip: 192.55.52.93, mailfrom: ray.ni@intel.com) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga005.fm.intel.com ([10.253.24.32]) by fmsmga102.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 05 Nov 2019 19:20:56 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.68,272,1569308400"; d="scan'208,217";a="402213023" Received: from fmsmsx103.amr.corp.intel.com ([10.18.124.201]) by fmsmga005.fm.intel.com with ESMTP; 05 Nov 2019 19:20:56 -0800 Received: from fmsmsx115.amr.corp.intel.com (10.18.116.19) by FMSMSX103.amr.corp.intel.com (10.18.124.201) with Microsoft SMTP Server (TLS) id 14.3.439.0; Tue, 5 Nov 2019 19:20:56 -0800 Received: from shsmsx108.ccr.corp.intel.com (10.239.4.97) by fmsmsx115.amr.corp.intel.com (10.18.116.19) with Microsoft SMTP Server (TLS) id 14.3.439.0; Tue, 5 Nov 2019 19:20:55 -0800 Received: from shsmsx104.ccr.corp.intel.com ([169.254.5.127]) by SHSMSX108.ccr.corp.intel.com ([169.254.8.41]) with mapi id 14.03.0439.000; Wed, 6 Nov 2019 11:20:53 +0800 From: "Ni, Ray" To: "afish@apple.com" , "devel@edk2.groups.io" , "jbrasen@nvidia.com" CC: Ashish Singhal , Laszlo Ersek , "Wang, Jian J" , "Wu, Hao A" , "Gao, Zhichao" , "Kinney, Michael D" Subject: Re: [edk2-devel] [PATCH] Support skipping automatic BM enumeration Thread-Topic: [edk2-devel] [PATCH] Support skipping automatic BM enumeration Thread-Index: AQHVjtTBfs56XIdrLUS3rD3UlLLaAqd0BEOAgAfitPD//4ZKgIAAGusAgAABkQCAAARWgIAABc2AgAAJNQCAAD03AIAAdMAAgAAS3ACAABdZAIAAX/iAgAAHo4CAABRiAIAAixow Date: Wed, 6 Nov 2019 03:20:53 +0000 Message-ID: <734D49CCEBEEF84792F5B80ED585239D5C35125E@SHSMSX104.ccr.corp.intel.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> <746A8D5E-DC45-4D39-9C4D-97A10BE2E0B0@apple.com> In-Reply-To: <746A8D5E-DC45-4D39-9C4D-97A10BE2E0B0@apple.com> Accept-Language: en-US, zh-CN X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] MIME-Version: 1.0 Return-Path: ray.ni@intel.com Content-Language: en-US Content-Type: multipart/alternative; boundary="_000_734D49CCEBEEF84792F5B80ED585239D5C35125ESHSMSX104ccrcor_" --_000_734D49CCEBEEF84792F5B80ED585239D5C35125ESHSMSX104ccrcor_ Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Andrew, I agree with your opinion. It's expected that Platform Boot Manager lib calls EfiBootManagerRefreshAl= lBootOption() only in full configuration boot path. The full configuration boot path is chosen when hardware changes happen. S= o it's not expected EfiBootManagerRefresh...() be called in every boot. So you could: 1. Delete the auto-created option pointing to LoadFile instance 2. Create your own one with customized description. From: afish@apple.com Sent: Wednesday, November 6, 2019 10:47 AM To: devel@edk2.groups.io; jbrasen@nvidia.com Cc: Ashish Singhal ; Laszlo Ersek ; Ni, Ray ; Wang, Jian J ; Wu,= Hao A ; Gao, Zhichao ; Kinney, = Michael D Subject: Re: [edk2-devel] [PATCH] Support skipping automatic BM enumeratio= n On Nov 5, 2019, at 7:34 PM, Jeff Brasen > wrote: Wouldn't having a variable that we create and delete on every boot put unn= ecessary stress on the SPI-NOR that the variable store lives on? What about the alternative approach where we allow the platform code to mo= dify the attributes of the auto created variable to disable it with hidden/= !active but still match for detection purposes so that it doesn't delete an= d recreate the modified variable each boot? That way all the logic on what = to disable can still be in the platform code and all the existing logic in = the boot manager can stay basically the same? What changes every boot that forces the variable to need to get modified? I would assume the NOR driver is smart enough to not update a variable tha= t is not changing. The custom BDS could could 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 foll= ows 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 vari= able and update BootOrder 3. Platform code runs creates the options for the boot option it wants = and writes those to variable store 4. Delete/disable the boot option in the variable store When you reboot it won't find the variable so 1/2/4 will re-occur The code that does this (1/2) is EfiBootManagerRefreshAllBootOption in BmB= oot.c If you modify the variable to disable it with hidden/not active it would d= elete 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 compare the attri= butes (at least allow for differences in active and hidden) then the when i= t refreshes every thing it would see the match and not delete/create a new = variable in the store and thus we wouldn't have changes every boot. 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. 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. 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. [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 Thanks, Andrew Fish Thanks, Jeff ________________________________ This email message is for the sole use of the intended recipient(s) and ma= y contain confidential information. Any unauthorized review, use, disclosu= re or distribution is prohibited. If you are not the intended recipient, p= lease contact the sender by reply email and destroy all copies of the origi= nal message. ________________________________ --_000_734D49CCEBEEF84792F5B80ED585239D5C35125ESHSMSX104ccrcor_ Content-Type: text/html; charset="us-ascii" Content-Transfer-Encoding: quoted-printable

Andrew,

I agree with your opinion.

It’s expected that Platform Boot Manager lib = calls EfiBootManagerRefreshAllBootOption() only in full configuration boot = path.

The full configuration boot path is chosen when har= dware changes happen. So it’s not expected EfiBootManagerRefresh̷= 0;() be
called in every boot.

So you could:

  1. Delete the auto-created option pointing to LoadFile instance
  2. Create your own one with customized description.

 

 

From: afish@apple.com <afish@apple.com>= ;
Sent: Wednesday, November 6, 2019 10:47 AM
To: devel@edk2.groups.io; jbrasen@nvidia.com
Cc: Ashish Singhal <ashishsingha@nvidia.com>; Laszlo Ersek &l= t;lersek@redhat.com>; Ni, Ray <ray.ni@intel.com>; Wang, Jian J <= ;jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; Gao, Zhic= hao <zhichao.gao@intel.com>; Kinney, Michael D <michael.d.kinney@i= ntel.com>
Subject: Re: [edk2-devel] [PATCH] Support skipping automatic BM enu= meration

 

 



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 unn= ecessary stress on the SPI-NOR that the variable store lives on?
What about the alternative approach where we allow the platform code to mo= dify the attributes of the auto created variable to disable it with hidden/= !active but still match for detection purposes so that it doesn't delete an= d recreate the modified variable each boot? That way all the logic on what to disable can still be in the = platform code and all the existing logic in the boot manager can stay basic= ally the same?

What changes every boot that forces the variable to need to get modified?<= span class=3D"apple-converted-space"> 

I would assume the NOR driver is smart enough to not update a variable tha= t is not changing. 

The custom BDS could could 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 foll= ows

 

  1. Scan for inst= ance of the boot option in the variables
    2. It will not b= e found, so create a new boot option store it to a variable and update Boot= Order
  2. Platform code= runs creates the options for the boot option it wants and writes those to = variable store
  3. Delete/disabl= e the boot option in the variable store

    4.  

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

     

    The code that does this (1/2) is EfiBootManagerRefreshAllBoo= tOption in BmBoot.c

     

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

     

    If however we modify EfiBootManagerFindLoadOption to not com= pare the attributes (at least allow for differences in active and hidden) t= hen the when it refreshes every thing it would see the match and not delete/create a new variable in the store and thus we would= n't have changes every boot.

     

 

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 thi= s library..... I though the patch changed BmEnumerateBootOptions(), so= that is going to change how EfiBootManagerRefreshAllBootOption() works. I'd also point out the patch as given is invalid as it changed the= behavior of the public library API for EfiBootManagerRefreshAllBootOption(= ) [1] so for the patch to be valid it would need to change the comments to = reflect the new behavior. This is kind of what Laszlo's technical debt comment was about. <= /p>

 

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

 

I think the question is how is your use case differ= ent than the boot variable that Windows installs? If it works kind of the s= ame way then the answer is to have the BDS platform specific code write the= boot variable. 

 

 

[1]

/**

  The function creates boot options for all po= ssible bootable medias in the following order:

  1. Removable BlockIo       &n= bsp;    - The boot option only points to the removable media=

              &n= bsp;                     = device, like USB key, DVD, Floppy etc.

  2. Fixed BlockIo        =        - The boot option only points to a Fixed blockI= o device,

              &n= bsp;                     = like HardDisk.

  3. Non-BlockIo SimpleFileSystem - The boot o= ption points to a device supporting

              &n= bsp;                     = SimpleFileSystem Protocol, but not supporting BlockIo

              &n= bsp;                     = protocol.

  4. LoadFile         &nbs= p;           - The boot option points to the media= supporting

              &n= bsp;                     = LoadFile protocol.

  Reference: UEFI Spec chapter 3.3 Boot Option= Variables Default Boot Behavior

 

  The function won't delete the boot option no= t added by itself.

**/

VOID

EFIAPI

EfiBootManagerRefreshAllBootOption (

  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 you are not the intended recipient, please contact t= he sender by reply email and destroy all copies of the original message.

 

--_000_734D49CCEBEEF84792F5B80ED585239D5C35125ESHSMSX104ccrcor_--