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

[Haojian Zhuang <haojian.zhuang@outlook.com>]

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".
>>
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Haojian Zhuang <haojian.zhuang@linaro.org>
>> ---
>>   Platform/Hisilicon/HiKey960/HiKey960.dsc |   3 +
>>   Platform/Hisilicon/HiKey960/HiKey960.fdf | 241 ++++++++++++++++++++++++++++++-
>>   2 files changed, 242 insertions(+), 2 deletions(-)
>>
>> diff --git a/Platform/Hisilicon/HiKey960/HiKey960.dsc b/Platform/Hisilicon/HiKey960/HiKey960.dsc
>> index 98289c0..a6864c1 100644
>> --- a/Platform/Hisilicon/HiKey960/HiKey960.dsc
>> +++ b/Platform/Hisilicon/HiKey960/HiKey960.dsc
>> @@ -78,6 +78,9 @@
>>     gEfiMdeModulePkgTokenSpaceGuid.PcdConOutGopSupport|FALSE
>>   
>>   [PcdsFixedAtBuild.common]
>> +  gEfiMdeModulePkgTokenSpaceGuid.PcdEmuVariableNvStoreReserved|0x1AD88048
>> +  gEfiMdeModulePkgTokenSpaceGuid.PcdVariableStoreSize|0x7FB8
>> +
>>     gEfiMdePkgTokenSpaceGuid.PcdDefaultTerminalType|4
>>   
>>     gEfiMdeModulePkgTokenSpaceGuid.PcdFirmwareVersionString|L"Alpha"
>> diff --git a/Platform/Hisilicon/HiKey960/HiKey960.fdf b/Platform/Hisilicon/HiKey960/HiKey960.fdf
>> index 655032a..af10430 100644
>> --- a/Platform/Hisilicon/HiKey960/HiKey960.fdf
>> +++ b/Platform/Hisilicon/HiKey960/HiKey960.fdf
>> @@ -26,12 +26,12 @@
>>   
>>   [FD.BL33_AP_UEFI]
>>   BaseAddress   = 0x1AC98000|gArmTokenSpaceGuid.PcdFdBaseAddress  # The base address of the Firmware in NOR Flash.
>> -Size          = 0x000F0000|gArmTokenSpaceGuid.PcdFdSize         # The size in bytes of the FLASH Device
>> +Size          = 0x00100000|gArmTokenSpaceGuid.PcdFdSize         # The size in bytes of the FLASH Device
>>   ErasePolarity = 1
>>   
>>   # This one is tricky, it must be: BlockSize * NumBlocks = Size
>>   BlockSize     = 0x00001000
>> -NumBlocks     = 0xF0
>> +NumBlocks     = 0x100
>>   
>>   ################################################################################
>>   #
>> @@ -53,6 +53,243 @@ NumBlocks     = 0xF0
>>   gArmTokenSpaceGuid.PcdFvBaseAddress|gArmTokenSpaceGuid.PcdFvSize
>>   FV = FVMAIN_COMPACT
>>   
>> +0x000F0000|0x00008000
>> +gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase|gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableSize
>> +DATA = {
>> +  ## This is the EFI_FIRMWARE_VOLUME_HEADER
>> +  0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
>> +  0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
>> +  # FileSystemGuid: gEfiSystemNvDataFvGuid         =
>> +  0x8D, 0x2B, 0xF1, 0xFF, 0x96, 0x76, 0x8B, 0x4C,
>> +  0xA9, 0x85, 0x27, 0x47, 0x07, 0x5B, 0x4F, 0x50,
>> +  # FvLength: 0x8000
>> +  0x00, 0x80, 0x0, 0x00, 0x00, 0x00, 0x00, 0x00,
>> +  #Signature "_FVH"       #Attributes
>> +  0x5f, 0x46, 0x56, 0x48, 0xff, 0xfe, 0x04, 0x00,
>> +  #HeaderLength #CheckSum #ExtHeaderOffset #Reserved #Revision
>> +  0x48, 0x00, 0x36, 0x09, 0x00, 0x00, 0x00, 0x02,
>> +  #Blockmap[0]: 8 Blocks * 0x1000 Bytes / Block
>> +  0x08, 0x00, 0x00, 0x00, 0x00, 0x01, 0x00, 0x00,
>> +  #Blockmap[1]: End
>> +  0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
>> +  ## end of EFI_FIRMWARE_VOLUME_HEADER.
>> +
>> +  ### Offset (0x48) ###
>> +  ## This is the VARIABLE_STORE_HEADER gEfiVariableGuid
>> +  0x16, 0x36, 0xcf, 0xdd, 0x75, 0x32, 0x64, 0x41,
>> +  0x98, 0xb6, 0xfe, 0x85, 0x70, 0x7f, 0xfe, 0x7d,
>> +  #Size: 0x8000 (gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableSize) - 0x48 (size of EFI_FIRMWARE_VOLUME_HEADER) = 0x7FB8
>> +  0xB8, 0x7F, 0x00, 0x00,
>> +  #FORMATTED: 0x5A #HEALTHY: 0xFE #Reserved: UINT16 #Reserved1: UINT32
>> +  0x5A, 0xFE, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
>> +
>> +  ### Offset (0x64) ###
>> +  ## This is the VARIABLE_HEADER
>> +  #StartId: VARIABLE_DATA #State: VAR_ADDED #Reserved
>> +  0xAA, 0x55, 0x3F, 0x00,
>> +  #Attributes: EFI_VARIABLE_NON_VOLATILE | EFI_VARIABLE_BOOTSERVICE_ACCESS
>> +  0x03, 0x00, 0x00, 0x00,
>> +  #NameSize:
>> +  0x14, 0x00, 0x00, 0x00,
>> +  #DataSize:
>> +  0x18, 0x00, 0x00, 0x00,
>> +  #VariableGuid: gEfiGlobalVariableGuid
>> +  0x61, 0xDF, 0xE4, 0x8B, 0xCA, 0x93, 0xD2, 0x11,
>> +  0xAA, 0x0D, 0x00, 0xE0, 0x98, 0x03, 0x2B, 0x8C,
>> +  ## End of VARIABLE_HEADER. Offset (0x84)
>> +  #VariableName: BootOrder
>> +  0x42, 0x00, 0x6F, 0x00, 0x6F, 0x00, 0x74, 0x00,
>> +  0x4F, 0x00, 0x72, 0x00, 0x64, 0x00, 0x65, 0x00,
>> +  0x72, 0x00, 0x00, 0x00,
>> +  #Data
>> +  0x00, 0x00, 0x01, 0x00, 0x02, 0x00, 0x03, 0x00,
>> +  0x04, 0x00, 0x05, 0x00, 0x06, 0x00, 0x07, 0x00,
>> +  0x08, 0x00, 0x09, 0x00, 0x0a, 0x00, 0x0b, 0x00,
>> +  ### End of BootOrder.
>> +
>> +  ### Offset (0xB0) ###
>> +  ## This is the VARIABLE_HEADER
>> +  #StartId: VARIABLE_DATA #State: VAR_ADDED #Reserved
>> +  0xAA, 0x55, 0x3F, 0x00,
>> +  #Attributes: EFI_VARIABLE_NON_VOLATILE | EFI_VARIABLE_BOOTSERVICE_ACCESS
>> +  0x03, 0x00, 0x00, 0x00,
>> +  #NameSize:
>> +  0x12, 0x00, 0x00, 0x00,
>> +  #DataSize:
>> +  0x42, 0x00, 0x00, 0x00,
>> +  #VariableGuid: gEfiGlobalVariableGuid
>> +  0x61, 0xDF, 0xE4, 0x8B, 0xCA, 0x93, 0xD2, 0x11,
>> +  0xAA, 0x0D, 0x00, 0xE0, 0x98, 0x03, 0x2B, 0x8C,
>> +  #VariableName: Boot0000
>> +  0x42, 0x00, 0x6F, 0x00, 0x6F, 0x00, 0x74, 0x00,
>> +  0x30, 0x00, 0x30, 0x00, 0x30, 0x00, 0x30, 0x00,
>> +  0x00, 0x00,
>> +  ###  Offset (0xE2), Size(0x32) ###
>> +  #Data:
>> +  #Attributes: LOAD_OPTION_ACTIVE
>> +  0x01, 0x00, 0x00, 0x00,
>> +  #FilePathListLength
>> +  0x26, 0x00,
>> +  #Description: "Boot on SD"
>> +  0x42, 0x00, 0x6F, 0x00, 0x6F, 0x00, 0x74, 0x00,      #Boot
>> +  0x20, 0x00, 0x6F, 0x00, 0x6E, 0x00, 0x20, 0x00,      # on
>> +  0x53, 0x00, 0x44, 0x00, 0x00, 0x00,                  #SD
>> +  #FilePathList (0x26)
>> +  0x01, 0x04, 0x1D, 0x00, 0x5B, 0x90, 0x51, 0x0D,
>> +  0x7E, 0xB7, 0x2A, 0x45, 0xA2, 0xC0, 0xEC, 0xA0,
>> +  0xCC, 0x8D, 0x51, 0x4A, 0x00, 0xF0, 0x37, 0xFF,
>> +  0x00, 0x00, 0x00, 0x00, 0x00, 0x03, 0x1A, 0x05,
>> +  0x00, 0x00, 0x7F, 0xFF, 0x04, 0x00,
>> +  ### Offset (0x124), Size (0x42) ###
>> +  ### End of Boot0000
>> +
>>
> 
> 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.

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.

Which one do you prefer?

Best Regards
Haojian