Re: [edk2-devel] [PATCH v4] MdeModulePkg: Add EDK2 Platform Boot Manager Protocol

["Ashish Singhal" <ashishsingha@nvidia.com>]

Hi Sunny,

For the 3 use cases you suggested, please let me know if you think I am wrong.

1. RefreshAllBootOptions can refresh boot options which are auto created by BmEnumerateBootOptions as well as NV boot options in the variable store. Platform implementation of RefreshAllBootOptions can have calls to platform specific other libraries/drivers that create more boot options.
2. In EfiBootManagerRefreshAllBootOption, BmEnumerateBootOptions is the only function that populates boot options and then validates/invalidates them as well as NV boot options. RefreshAllBootOptions can modify static-informational data or configuration data from the boot options created by BmEnumerateBootOptions as well as in NV store.
3. Solution for third use case can be derived by using a PCD which can be defaulted to tell code to call EfiBootManagerRefreshAllBootOption every time but can be overridden by platform to not call it from anywhere except BDS.

Thanks
Ashish

-----Original Message-----
From: Wang, Sunny (HPS SW) <sunnywang@hpe.com> 
Sent: Monday, December 23, 2019 9:38 PM
To: Ni, Ray <ray.ni@intel.com>; devel@edk2.groups.io; Ashish Singhal <ashishsingha@nvidia.com>; Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; Gao, Zhichao <zhichao.gao@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>; 'Andrew Fish (afish@apple.com)' <afish@apple.com>
Cc: Spottswood, Jason <jason.spottswood@hpe.com>; Wang, Sunny (HPS SW) <sunnywang@hpe.com>
Subject: RE: [edk2-devel] [PATCH v4] MdeModulePkg: Add EDK2 Platform Boot Manager Protocol

External email: Use caution opening links or attachments


Thanks for checking this, Ray.

Platform may want to:
 1. Refresh the static boot options (that are not created by BmEnumerateBootOptions) without a reboot.
 2. Update some other static-informational data or configuration data right after calling EfiBootManagerRefreshAllBootOption.
 3. Always skip calling EfiBootManagerRefreshAllBootOption for the cases like BOOT_ASSUMING_NO_CONFIGURATION_CHANGES.

I know these actions can be done by adding code to other places, but using hooks in EfiBootManagerRefreshAllBootOption would be an easier solution for the platform. We won't need to take care of all the EfiBootManagerRefreshAllBootOption callers. Therefore, If we don't have a concern about adding more hooks and want to give the platform more flexibility, we could add two more hooks (1 and 3) in the future to have three hooks as below:
  1. BeginOfRefreshAllBootOptions
  2. RefreshAllBootOptions or RefreshEnumeratedBootOptions
  3. EndOfRefreshAllBootOption

By the way, the current change looks good enough to me. In case Ashish or others are in urgent need of this code change, we can discuss my comments later in a separated email.

Regards,
Sunny Wang

-----Original Message-----
From: Ni, Ray [mailto:ray.ni@intel.com]
Sent: Tuesday, December 24, 2019 10:40 AM
To: Wang, Sunny (HPS SW) <sunnywang@hpe.com>; devel@edk2.groups.io; ashishsingha@nvidia.com; Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; Gao, Zhichao <zhichao.gao@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>; 'Andrew Fish (afish@apple.com)' <afish@apple.com>
Cc: Spottswood, Jason <jason.spottswood@hpe.com>
Subject: RE: [edk2-devel] [PATCH v4] MdeModulePkg: Add EDK2 Platform Boot Manager Protocol



> -----Original Message-----
> From: Wang, Sunny (HPS SW) <sunnywang@hpe.com>
> Sent: Friday, December 20, 2019 7:29 PM
> To: devel@edk2.groups.io; ashishsingha@nvidia.com; Ni, Ray 
> <ray.ni@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A 
> <hao.a.wu@intel.com>; Gao, Zhichao <zhichao.gao@intel.com>; Kinney, 
> Michael D <michael.d.kinney@intel.com>; 'Andrew Fish (afish@apple.com)'
> <afish@apple.com>
> Cc: Spottswood, Jason <jason.spottswood@hpe.com>; Wang, Sunny (HPS
> SW) <sunnywang@hpe.com>
> Subject: RE: [edk2-devel] [PATCH v4] MdeModulePkg: Add EDK2 Platform 
> Boot Manager Protocol
>
> Good point. The way you used is more robust. It can cover a mistake in 
> function's error handling. Thanks for clarifying this, Ashish.
>
> In addition, the other naming suggestion just comes to mind. How about 
> we rename the function to a more generic one (based on location) like 
> AfterEnumerateBootOptions or a more specific one like 
> RefreshEnumeratedBootOptions? In the future, we may add the other hook 
> function in the EfiBootManagerRefreshAllBootOption to deal with the 
> boot options that are not created by BmEnumerateBootOptions. In this 
> case (two hook functions in EfiBootManagerRefreshAllBootOption), the 
> original function name "RefreshAllBootOptions" may cause some confusion.

Sunny,
What else feasibility do you think platform may require in future but this RefreshAllBootOptions cannot support?

Thanks,
Ray


-----------------------------------------------------------------------------------
This email message is for the sole use of the intended recipient(s) and may contain
confidential information.  Any unauthorized review, use, disclosure or distribution
is prohibited.  If you are not the intended recipient, please contact the sender by
reply email and destroy all copies of the original message.
-----------------------------------------------------------------------------------