On 02/22/2018 06:56 PM, Laszlo Ersek wrote:
On 02/22/18 09:57, Haojian Zhuang wrote:
On 02/20/2018 11:54 PM, Laszlo Ersek wrote:
Hi,
On 02/15/18 16:14, Haojian Zhuang wrote:
Add four boot options in emu variable region. They are "Boot on SD", "Grub", "Android Boot" and "Android Fastboot".
While in both ArmVirtPkg and OvmfPkg we use a similar DATA definition for *formatting* the pristine variable store (just setting the absolutely necessary headers via a hexdump), I think that adding "business content" like this is a bad idea. For one, if the defaults have to be updated ever again, the patches for the DATA definitions will look terrible. It's hard to review and maintain hexdump like these; we should keep such DATA definitions to the absolute minimum.
Such "default" boot options should be set by the platform's PlatformBootManagerLib instance. PlatformBootManagerLib is responsible for generating boot options. In doing that, PlatformBootManagerLib can call helper functions from UefiBootManagerLib, so everything need not be done manually.
For example, EfiBootManagerGetLoadOptions() can be used to retrieve the current boot options. Then, you can iterate over the four boot options that you want to ensure exist -- for each:
- create a EFI_BOOT_MANAGER_LOAD_OPTION object with
EfiBootManagerInitializeLoadOption(),
- check if the option already exists, with EfiBootManagerFindLoadOption(),
- if the option doesn't exist yet, add it with
EfiBootManagerAddLoadOptionVariable(),
- free the EFI_BOOT_MANAGER_LOAD_OPTION object with
EfiBootManagerFreeLoadOption()
Finally, free the originally retrieved set of boot options with EfiBootManagerFreeLoadOptions().
You can find example code in "ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c".
Thanks, Laszlo
Hi Laszlo,
Yes, it's a bit hard to review and maintain. Actually, I implemented a PlatformBootManagerLib in this link (https://github.com/96boards-hikey/OpenPlatformPkg/blob/testing/hikey960_v1.3...).
I format it with hard code since I want to re-use PlatformBootManagerLib in ArmPkg. Because these boot options only exist on HiKey/HiKey960 platforms, and most of them are same of the one in ArmPkg. If I append them into ArmPkg, it's not common enough. At last, I hope to implement a non-volatile region on flash devices in the future. It means that implementing a HiKey specific PlatformBootManagerLib is only a temporary solution.
Sorry, I don't understand how this follows. Again: why is it only a temporary solution to implement a PlatformBootManagerLib instance for HiKey?
The library class says "Platform" in the name. Platforms are supposed to provide it, one way or another.
If you want to minimize code duplication between ArmPkg and HiKey, it should be possible to factor out another library class from ArmPkg's PlatformBootManagerLib instance. It could be a function that returns the list of platform-specific boot options -- as an array of EFI_BOOT_MANAGER_LOAD_OPTION elements -- that should always exist. Then ArmPkg's PlatformBootManagerLib would perform the iteration that I described above.
Platforms different from HiKey would return an empty array, while HiKey could return the four options it needs.
Another benefit is that these four boot options would be restored on every boot, even if the user deleted them. This seems independent of whether the HiKey platform has functional non-volatile storage or not.
It seems that I have two options to go ahead.
- Move those hard code boot options into a FV file, and put the FV file
in edk2-non-osi repository.
- Implement a HiKey related PlatformBootManagerLib.
I think option #2 is far superior. You don't have to duplicate all code from ArmPkg's PlatformBootManagerLib for that, you can extract another utility library class / callback function just for providing the options you always want.
Or perhaps you *don't* want them always, just until NVRAM support arrives to HiKey? And, in that case, it will be alright for the user to delete these options permanently?
Yes, I hope the default boot options could be dropped when NVRAM support arrives on HiKey. User could decide which boot option is really good for him.
... I still think extracting a new lib class for these default boot options would be better. If at some point you'd like to drop the default boot options, it's easy to switch the new callback to returning zero elements. The lib class can remain useful to other development platforms.
OK. I'll try to add the new callback in it.
Best Regards Haojian