Re: [PATCH v3 1/4] UefiPayloadPkg: Add LINUXBOOT payload target

["Guo Dong" <guo.dong@intel.com>]

Hi Ray,

I had updated the patch to update the commit message and fix some coding style.
Please help review V4 patch.

Thanks,
Guo

-----Original Message-----
From: Ni, Ray <ray.ni@intel.com> 
Sent: Tuesday, August 24, 2021 5:35 PM
To: Cheng-Chieh Huang <chengchieh@google.com>; devel@edk2.groups.io
Cc: Schaefer, Daniel <daniel.schaefer@hpe.com>; Trammell Hudson <hudson@trmm.net>; Ma, Maurice <maurice.ma@intel.com>; Dong, Guo <guo.dong@intel.com>; You, Benjamin <benjamin.you@intel.com>
Subject: RE: [PATCH v3 1/4] UefiPayloadPkg: Add LINUXBOOT payload target

Can you describe how LINUXBOOT payload is supported in the commit message?
One line commit message is too simple for such a big feature😊

> -----Original Message-----
> From: Cheng-Chieh Huang <chengchieh@google.com>
> Sent: Monday, August 23, 2021 2:33 PM
> To: devel@edk2.groups.io
> Cc: Cheng-Chieh Huang <chengchieh@google.com>; Schaefer, Daniel 
> <daniel.schaefer@hpe.com>; Trammell Hudson <hudson@trmm.net>; Ma, 
> Maurice <maurice.ma@intel.com>; Dong, Guo <guo.dong@intel.com>; You, 
> Benjamin <benjamin.you@intel.com>; Ni, Ray <ray.ni@intel.com>
> Subject: [PATCH v3 1/4] UefiPayloadPkg: Add LINUXBOOT payload target
> 
> Initial commit to support linuxboot payload.
> 
> Signed-off-by: Cheng-Chieh Huang <chengchieh@google.com>
> Cc: Cheng-Chieh Huang <chengchieh@google.com>
> Cc: Daniel Schaefer <daniel.schaefer@hpe.com>
> Cc: Trammell Hudson <hudson@trmm.net>
> Cc: Maurice Ma <maurice.ma@intel.com>
> Cc: Guo Dong <guo.dong@intel.com>
> Cc: Benjamin You <benjamin.you@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> ---
>  UefiPayloadPkg/UefiPayloadPkg.dsc             |  16 +-
>  .../Library/LbParseLib/LbParseLib.inf         |  39 ++++
>  UefiPayloadPkg/Library/LbParseLib/Linuxboot.h |  47 +++++
>  .../Library/LbParseLib/LbParseLib.c           | 187 ++++++++++++++++++
>  .../PciHostBridgeLib/PciHostBridgeSupport.c   |   6 +-
>  5 files changed, 288 insertions(+), 7 deletions(-)  create mode 
> 100644 UefiPayloadPkg/Library/LbParseLib/LbParseLib.inf
>  create mode 100644 UefiPayloadPkg/Library/LbParseLib/Linuxboot.h
>  create mode 100644 UefiPayloadPkg/Library/LbParseLib/LbParseLib.c
> 
> diff --git a/UefiPayloadPkg/UefiPayloadPkg.dsc 
> b/UefiPayloadPkg/UefiPayloadPkg.dsc
> index bcedf1c746b4..54576ba485b7 100644
> --- a/UefiPayloadPkg/UefiPayloadPkg.dsc
> +++ b/UefiPayloadPkg/UefiPayloadPkg.dsc
> @@ -33,6 +33,7 @@ [Defines]
>    #
>    # SBL:      UEFI payload for Slim Bootloader
>    # COREBOOT: UEFI payload for coreboot
> +  # LINUXBOOT: UEFI payload for linuxboot
>    #
>    DEFINE   BOOTLOADER = SBL
> 
> @@ -93,6 +94,9 @@ [Defines]
> 
>  [BuildOptions]
>    *_*_*_CC_FLAGS                 = -D DISABLE_NEW_DEPRECATED_INTERFACES
> +!if $(BOOTLOADER) == "LINUXBOOT"
> +  *_*_*_CC_FLAGS                 = -D LINUXBOOT_PAYLOAD
> +!endif
>    GCC:*_UNIXGCC_*_CC_FLAGS       = -DMDEPKG_NDEBUG
>    GCC:RELEASE_*_*_CC_FLAGS       = -DMDEPKG_NDEBUG
>    INTEL:RELEASE_*_*_CC_FLAGS     = /D MDEPKG_NDEBUG
> @@ -222,11 +226,13 @@ [LibraryClasses]  !endif
>    
> PlatformSupportLib|UefiPayloadPkg/Library/PlatformSupportLibNull/Platf
> ormSupportLibNull.inf
>  !if $(UNIVERSAL_PAYLOAD) == FALSE
> -  !if $(BOOTLOADER) == "COREBOOT"
> -    BlParseLib|UefiPayloadPkg/Library/CbParseLib/CbParseLib.inf
> -  !else
> -    BlParseLib|UefiPayloadPkg/Library/SblParseLib/SblParseLib.inf
> -  !endif
> + !if $(BOOTLOADER) == "COREBOOT"
> +   BlParseLib|UefiPayloadPkg/Library/CbParseLib/CbParseLib.inf
> + !elseif $(BOOTLOADER) == "LINUXBOOT"
> +   BlParseLib|UefiPayloadPkg/Library/LbParseLib/LbParseLib.inf
> + !else
> +   BlParseLib|UefiPayloadPkg/Library/SblParseLib/SblParseLib.inf
> + !endif
>  !endif
> 
>    
> DebugLib|MdePkg/Library/BaseDebugLibSerialPort/BaseDebugLibSerialPort.
> inf diff --git a/UefiPayloadPkg/Library/LbParseLib/LbParseLib.inf 
> b/UefiPayloadPkg/Library/LbParseLib/LbParseLib.inf
> new file mode 100644
> index 000000000000..d75ba8db8cf3
> --- /dev/null
> +++ b/UefiPayloadPkg/Library/LbParseLib/LbParseLib.inf
> @@ -0,0 +1,39 @@
> +## @file
> +#  Linuxboot Table Parse Library.
> +#
> +#  Copyright (c) 2021, the u-root Authors. All rights reserved.<BR> #  
> +SPDX-License-Identifier: BSD-2-Clause-Patent # ##
> +
> +[Defines]
> +  INF_VERSION                    = 0x00010005
> +  BASE_NAME                      = LbParseLib
> +  FILE_GUID                      = DBA15E1E-4C16-47DF-93C0-AB5888ED14C3
> +  MODULE_TYPE                    = BASE
> +  VERSION_STRING                 = 1.0
> +  LIBRARY_CLASS                  = BlParseLib
> +
> +#
> +# The following information is for reference only and not required by the build tools.
> +#
> +#  VALID_ARCHITECTURES           = IA32 X64
> +#
> +
> +[Sources]
> +  LbParseLib.c
> +
> +[Packages]
> +  MdePkg/MdePkg.dec
> +  MdeModulePkg/MdeModulePkg.dec
> +  UefiPayloadPkg/UefiPayloadPkg.dec
> +
> +[LibraryClasses]
> +  BaseLib
> +  BaseMemoryLib
> +  IoLib
> +  DebugLib
> +  PcdLib
> +
> +[Pcd]
> +  gUefiPayloadPkgTokenSpaceGuid.PcdPayloadFdMemBase
> diff --git a/UefiPayloadPkg/Library/LbParseLib/Linuxboot.h 
> b/UefiPayloadPkg/Library/LbParseLib/Linuxboot.h
> new file mode 100644
> index 000000000000..b3b7e70a7ccb
> --- /dev/null
> +++ b/UefiPayloadPkg/Library/LbParseLib/Linuxboot.h
> @@ -0,0 +1,47 @@
> +/** @file
> +  LinuxBoot PEI module include file.
> +**/
> +#ifndef __LINUXBOOT_PEI_H_INCLUDED__
> +#define __LINUXBOOT_PEI_H_INCLUDED__
> +
> +#if defined(_MSC_VER)
> +#pragma warning(disable : 4200)
> +#endif
> +
> +#pragma pack(1)
> +typedef struct SerialPortConfigStruct {
> +  UINT32 Type;
> +  UINT32 BaseAddr;
> +  UINT32 Baud;
> +  UINT32 RegWidth;
> +  UINT32 InputHertz;
> +  UINT32 UartPciAddr;
> +} SerialPortConfig;
> +
> +typedef struct MemoryMapEntryStruct {
> +  UINT64 Start;
> +  UINT64 End;
> +  UINT32 Type;
> +} MemoryMapEntry;
> +
> +typedef struct UefiPayloadConfigStruct {
> +  UINT64 Version;
> +  UINT64 AcpiBase;
> +  UINT64 AcpiSize;
> +  UINT64 SmbiosBase;
> +  UINT64 SmbiosSize;
> +  SerialPortConfig SerialConfig;
> +  UINT32 NumMemoryMapEntries;
> +  MemoryMapEntry MemoryMapEntries[0]; } UefiPayloadConfig; #pragma 
> +pack()
> +
> +#define UEFI_PAYLOAD_CONFIG_VERSION 1
> +
> +#define LINUXBOOT_MEM_RAM 1
> +#define LINUXBOOT_MEM_DEFAULT 2
> +#define LINUXBOOT_MEM_ACPI 3
> +#define LINUXBOOT_MEM_NVS 4
> +#define LINUXBOOT_MEM_RESERVED 5
> +
> +#endif  // __LINUXBOOT_PEI_H_INCLUDED__
> diff --git a/UefiPayloadPkg/Library/LbParseLib/LbParseLib.c 
> b/UefiPayloadPkg/Library/LbParseLib/LbParseLib.c
> new file mode 100644
> index 000000000000..48d174dfc078
> --- /dev/null
> +++ b/UefiPayloadPkg/Library/LbParseLib/LbParseLib.c
> @@ -0,0 +1,187 @@
> +/** @file
> +  This library will parse the linuxboot table in memory and extract 
> +those required
> +  information.
> +
> +  Copyright (c) 2021, the u-root Authors. All rights reserved.<BR>
> +  SPDX-License-Identifier: BSD-2-Clause-Patent
> +
> +**/
> +
> +
> +#include <IndustryStandard/Acpi.h>
> +#include <IndustryStandard/SmBios.h>
> +#include <Library/BaseLib.h>
> +#include <Library/BaseMemoryLib.h>
> +#include <Library/BlParseLib.h>
> +#include <Library/DebugLib.h>
> +#include <Library/IoLib.h>
> +#include <Library/PcdLib.h>
> +#include <Linuxboot.h>
> +#include <Uefi/UefiBaseType.h>
> +
> +// Retrieve UefiPayloadConfig from Linuxboot's uefiboot
> +UefiPayloadConfig*
> +GetUefiPayLoadConfig()
> +{
> +  UefiPayloadConfig *Config;
> +  Config = (UefiPayloadConfig*)(UINTN)(PcdGet32(PcdPayloadFdMemBase) 
> +- SIZE_64KB);
> +  if (Config->Version != UEFI_PAYLOAD_CONFIG_VERSION) {
> +    DEBUG((DEBUG_ERROR, "Expect payload Config version: %d, but get %d\n",
> +           UEFI_PAYLOAD_CONFIG_VERSION, Config->Version));
> +    CpuDeadLoop ();
> +  }
> +  return Config;
> +}
> +
> +// Align the address and add memory rang to MemInfoCallback VOID 
> +AddMemoryRange (
> +  IN BL_MEM_INFO_CALLBACK MemInfoCallback,
> +  IN UINTN start,
> +  IN UINTN end,
> +  IN int type
> +  ) {
> +  MEMROY_MAP_ENTRY MemoryMap;
> +  UINTN AlignedStart;
> +  UINTN AlignedEnd;
> +  AlignedStart = ALIGN_VALUE(start, SIZE_4KB);
> +  AlignedEnd = ALIGN_VALUE(end, SIZE_4KB);
> +  // Conservative adjustment on Memory map. This should happen when 
> +booting from
> +  // non UEFI bios and it may report a memory region less than 4KB.
> +  if (AlignedStart > start && type != LINUXBOOT_MEM_RAM) {
> +    AlignedStart -= SIZE_4KB;
> +  }
> +  if (AlignedEnd > end + 1 && type == LINUXBOOT_MEM_RAM) {
> +    AlignedEnd -= SIZE_4KB;
> +  }
> +  MemoryMap.Base = AlignedStart;
> +  MemoryMap.Size = AlignedEnd - AlignedStart;
> +  MemoryMap.Type = type;
> +  MemoryMap.Flag = 0;
> +  MemInfoCallback(&MemoryMap, NULL);
> +}
> +
> +/**
> +  Acquire the memory information from the linuxboot table in memory.
> +
> +  @param  MemInfoCallback     The callback routine
> +  @param  Params              Pointer to the callback routine parameter
> +
> +  @retval RETURN_SUCCESS     Successfully find out the memory information.
> +  @retval RETURN_NOT_FOUND   Failed to find the memory information.
> +
> +**/
> +RETURN_STATUS
> +EFIAPI
> +ParseMemoryInfo (
> +  IN BL_MEM_INFO_CALLBACK MemInfoCallback,
> +  IN VOID* Params
> +  ) {
> +  UefiPayloadConfig *Config;
> +  MemoryMapEntry* entry;
> +  int Index;
> +
> +  Config = GetUefiPayLoadConfig();
> +
> +  DEBUG((DEBUG_INFO, "MemoryMap #entries: %d\n", 
> + Config->NumMemoryMapEntries));
> +
> +  entry = &Config->MemoryMapEntries[0];
> +  for (Index = 0; Index < Config->NumMemoryMapEntries; Index++) {
> +    DEBUG((DEBUG_INFO, "Start: 0x%lx End: 0x%lx Type:%d\n", entry->Start,
> +           entry->End, entry->Type));
> +    AddMemoryRange(MemInfoCallback, entry->Start, entry->End, entry->Type);
> +    entry++;
> +  }
> +  return RETURN_SUCCESS;
> +}
> +
> +/**
> +  Acquire acpi table and smbios table from linuxboot
> +
> +  @param  SystemTableInfo          Pointer to the system table info
> +
> +  @retval RETURN_SUCCESS            Successfully find out the tables.
> +  @retval RETURN_NOT_FOUND          Failed to find the tables.
> +
> +**/
> +RETURN_STATUS
> +EFIAPI
> +ParseSystemTable (
> +  OUT SYSTEM_TABLE_INFO* SystemTableInfo
> +  ) {
> +  UefiPayloadConfig *Config;
> +
> +  Config = GetUefiPayLoadConfig();
> +  SystemTableInfo->AcpiTableBase = Config->AcpiBase;  
> + SystemTableInfo->AcpiTableSize = Config->AcpiSize;
> +
> +  SystemTableInfo->SmbiosTableBase = Config->SmbiosBase;  
> + SystemTableInfo->SmbiosTableSize = Config->SmbiosSize;
> +
> +  return RETURN_SUCCESS;
> +}
> +
> +/**
> +  Find the serial port information
> +
> +  @param  SERIAL_PORT_INFO   Pointer to serial port info structure
> +
> +  @retval RETURN_SUCCESS     Successfully find the serial port information.
> +  @retval RETURN_NOT_FOUND   Failed to find the serial port information .
> +
> +**/
> +RETURN_STATUS
> +EFIAPI
> +ParseSerialInfo (
> +  OUT SERIAL_PORT_INFO* SerialPortInfo
> +  ) {
> +  UefiPayloadConfig *Config;
> +  Config = GetUefiPayLoadConfig();
> +
> +  SerialPortInfo->BaseAddr = Config->SerialConfig.BaseAddr;  
> + SerialPortInfo->RegWidth = Config->SerialConfig.RegWidth;  
> + SerialPortInfo->Type = Config->SerialConfig.Type;  
> + SerialPortInfo->Baud = Config->SerialConfig.Baud;  
> + SerialPortInfo->InputHertz = Config->SerialConfig.InputHertz;  
> + SerialPortInfo->UartPciAddr = Config->SerialConfig.UartPciAddr;
> +
> +  return RETURN_SUCCESS;
> +}
> +
> +/**
> +  Find the video frame buffer information
> +
> +  @param  GfxInfo             Pointer to the EFI_PEI_GRAPHICS_INFO_HOB structure
> +
> +  @retval RETURN_SUCCESS     Successfully find the video frame buffer
> +information.
> +  @retval RETURN_NOT_FOUND   Failed to find the video frame buffer information .
> +
> +**/
> +RETURN_STATUS
> +EFIAPI
> +ParseGfxInfo (
> +  OUT EFI_PEI_GRAPHICS_INFO_HOB* GfxInfo
> +  ) {
> +  // Not supported
> +  return RETURN_NOT_FOUND;
> +}
> +
> +/**
> +  Find the video frame buffer device information
> +
> +  @param  GfxDeviceInfo      Pointer to the EFI_PEI_GRAPHICS_DEVICE_INFO_HOB
> +structure
> +
> +  @retval RETURN_SUCCESS     Successfully find the video frame buffer
> +information.
> +  @retval RETURN_NOT_FOUND   Failed to find the video frame buffer information.
> +
> +**/
> +RETURN_STATUS
> +EFIAPI
> +ParseGfxDeviceInfo (
> +  OUT EFI_PEI_GRAPHICS_DEVICE_INFO_HOB* GfxDeviceInfo
> +  ) {
> +  return RETURN_NOT_FOUND;
> +}
> diff --git 
> a/UefiPayloadPkg/Library/PciHostBridgeLib/PciHostBridgeSupport.c
> b/UefiPayloadPkg/Library/PciHostBridgeLib/PciHostBridgeSupport.c
> index b0268f05069c..a4f714f765ea 100644
> --- a/UefiPayloadPkg/Library/PciHostBridgeLib/PciHostBridgeSupport.c
> +++ b/UefiPayloadPkg/Library/PciHostBridgeLib/PciHostBridgeSupport.c
> @@ -40,8 +40,9 @@ AdjustRootBridgeResource (
>    IN  PCI_ROOT_BRIDGE_APERTURE *PMemAbove4G
>  )
>  {
> +#ifndef LINUXBOOT_PAYLOAD
>    UINT64  Mask;
> -
> +#endif
>    //
>    // For now try to downgrade everything into MEM32 since
>    // - coreboot does not assign resource above 4GB @@ -80,7 +81,7 @@ 
> AdjustRootBridgeResource (
>      PMemAbove4G->Base  = MAX_UINT64;
>      PMemAbove4G->Limit = 0;
>    }
> -
> +#ifndef LINUXBOOT_PAYLOAD
>    //
>    // Align IO  resource at 4K  boundary
>    //
> @@ -98,6 +99,7 @@ AdjustRootBridgeResource (
>    if (Mem->Base != MAX_UINT64) {
>      Mem->Base &= ~Mask;
>    }
> +#endif
>  }
> 
>  /**
> --
> 2.33.0.rc2.250.ged5fa647cd-goog