Re: [PATCH 1/2] Platform/Hisilicon/HiKey960: add boot options

[Haojian Zhuang <haojian.zhuang@linaro.org>]

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.4/Platforms/Hisilicon/Library/PlatformBootManagerLib/PlatformBm.c).
>>
>> 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.
>> 1. Move those hard code boot options into a FV file, and put the FV file
>> in edk2-non-osi repository.
>>
>> 2. 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