Re: [edk2-devel] [plaforms/devel-riscv-v2 PATCHv2 09/14] U500Pkg/Library: Initial version of PlatformBootManagerLib

["Abner Chang" <abner.chang@hpe.com>]

> -----Original Message-----
> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
> Leif Lindholm
> Sent: Thursday, October 3, 2019 6:03 AM
> To: devel@edk2.groups.io; Chen, Gilbert <gilbert.chen@hpe.com>
> Cc: Palmer Dabbelt <palmer@sifive.com>
> Subject: Re: [edk2-devel] [plaforms/devel-riscv-v2 PATCHv2 09/14]
> U500Pkg/Library: Initial version of PlatformBootManagerLib
> 
> On Thu, Sep 19, 2019 at 11:51:26AM +0800, Gilbert Chen wrote:
> > SiFive RISC-V U500 Platform Boot Manager library.
> 
> First of all, let me say that I think before upstreaming to master, you ought to
> look into merging PlatformBootManagerLibs for all Risc-V platforms. Like we
> have for *most* ARM/AARCH64 platforms with
> ArmPkg/Library/PlatformBootManagerLib/.
> 
> (Longer-term we should merge them all together into a single one.)
> 
> > Signed-off-by: Gilbert Chen <gilbert.chen@hpe.com>
> > ---
> >  .../Library/PlatformBootManagerLib/MemoryTest.c    | 682
> +++++++++++++++++++++
> >  .../PlatformBootManagerLib/PlatformBootManager.c   | 274 +++++++++
> >  .../PlatformBootManagerLib/PlatformBootManager.h   | 135 ++++
> >  .../PlatformBootManagerLib.inf                     |  63 ++
> >  .../Library/PlatformBootManagerLib/PlatformData.c  |  49 ++
> >  .../Library/PlatformBootManagerLib/Strings.uni     |  28 +
> >  6 files changed, 1231 insertions(+)
> >  create mode 100644
> >
> Platform/RiscV/SiFive/U500Pkg/Library/PlatformBootManagerLib/MemoryT
> es
> > t.c  create mode 100644
> >
> Platform/RiscV/SiFive/U500Pkg/Library/PlatformBootManagerLib/PlatformB
> > ootManager.c  create mode 100644
> >
> Platform/RiscV/SiFive/U500Pkg/Library/PlatformBootManagerLib/PlatformB
> > ootManager.h  create mode 100644
> >
> Platform/RiscV/SiFive/U500Pkg/Library/PlatformBootManagerLib/PlatformB
> > ootManagerLib.inf  create mode 100644
> >
> Platform/RiscV/SiFive/U500Pkg/Library/PlatformBootManagerLib/PlatformD
> > ata.c  create mode 100644
> > Platform/RiscV/SiFive/U500Pkg/Library/PlatformBootManagerLib/Strings.u
> > ni
> >
> > diff --git
> >
> a/Platform/RiscV/SiFive/U500Pkg/Library/PlatformBootManagerLib/Memory
> T
> > est.c
> >
> b/Platform/RiscV/SiFive/U500Pkg/Library/PlatformBootManagerLib/Memor
> yT
> > est.c
> > new file mode 100644
> > index 00000000..8c6d89e9
> > --- /dev/null
> > +++
> b/Platform/RiscV/SiFive/U500Pkg/Library/PlatformBootManagerLib/Mem
> > +++ oryTest.c
> 
> Why build a MemoryTest into the PlatformBootManagerLib?
Why not to do memory if platform provides memory test protocol?
> 
> > @@ -0,0 +1,682 @@
> > +/** @file
> > +  Perform the RISC-V platform memory test
> > +
> > +Copyright (c) 2019, Hewlett Packard Enterprise Development LP. All
> > +rights reserved.<BR> Copyright (c) 2004 - 2015, Intel Corporation.
> > +All rights reserved.<BR>
> > +
> > +SPDX-License-Identifier: BSD-2-Clause-Patent
> > +
> > +**/
> > +
> > +#include "PlatformBootManager.h"
> > +
> > +EFI_HII_HANDLE gStringPackHandle = NULL;
> > +EFI_GUID       mPlatformBootManagerStringPackGuid = {
> > +  0x154dd51, 0x9079, 0x4a10, { 0x89, 0x5c, 0x9c, 0x7, 0x72, 0x81,
> > +0x57, 0x88 }
> > +  };
> > +// extern UINT8  BdsDxeStrings[];
> 
> No need to keep commented-out code in.
> 
> > +
> > +//
> > +// BDS Platform Functions
> > +//
> > +/**
> > +
> > +  Show progress bar with title above it. It only works in Graphics mode.
> > +
> > +  @param TitleForeground Foreground color for Title.
> > +  @param TitleBackground Background color for Title.
> > +  @param Title           Title above progress bar.
> > +  @param ProgressColor   Progress bar color.
> > +  @param Progress        Progress (0-100)
> > +  @param PreviousValue   The previous value of the progress.
> > +
> > +  @retval  EFI_STATUS       Success update the progress bar
> > +
> > +**/
> > +EFI_STATUS
> > +PlatformBootManagerShowProgress (
> 
> I'm not a super fan of how this file integrates a custom memory test with a
> direct (copy) graphical progress indicator. There are both common memory
> test and common progress indicators - please use those instead. And
> improve them if they are currently insufficient for your needs.

Could you indicate where are those common libraries? Thanks.

> 
> > diff --git
> > a/Platform/RiscV/SiFive/U500Pkg/Library/PlatformBootManagerLib/Platfor
> > mBootManager.c
> > b/Platform/RiscV/SiFive/U500Pkg/Library/PlatformBootManagerLib/Platfor
> > mBootManager.c
> > new file mode 100644
> > index 00000000..9ef85089
> > --- /dev/null
> > +++
> b/Platform/RiscV/SiFive/U500Pkg/Library/PlatformBootManagerLib/Pla
> > +++ tformBootManager.c
> > @@ -0,0 +1,274 @@
> > +/** @file
> > +  This file include all platform action which can be customized
> > +  by IBV/OEM.
> > +
> > +Copyright (c) 2019, Hewlett Packard Enterprise Development LP. All
> > +rights reserved.<BR> Copyright (c) 2015, Intel Corporation. All
> > +rights reserved.<BR>
> > +
> > +SPDX-License-Identifier: BSD-2-Clause-Patent
> > +
> > +**/
> > +
> > +#include "PlatformBootManager.h"
> > +
> > +
> > +EFI_GUID mUefiShellFileGuid = { 0x7C04A583, 0x9E3E, 0x4f1c, {0xAD,
> > +0x65, 0xE0, 0x52, 0x68, 0xD0, 0xB4, 0xD1}};
> > +
> > +/**
> > +  Perform the platform diagnostic, such like test memory. OEM/IBV
> > +also
> > +  can customize this function to support specific platform diagnostic.
> > +
> > +  @param MemoryTestLevel  The memory test intensive level
> > +  @param QuietBoot        Indicate if need to enable the quiet boot
> > +
> > +**/
> > +VOID
> > +PlatformBootManagerDiagnostics (
> > +  IN EXTENDMEM_COVERAGE_LEVEL    MemoryTestLevel,
> > +  IN BOOLEAN                     QuietBoot
> > +  )
> > +{
> > +  EFI_STATUS                     Status;
> > +
> > +  //
> > +  // Here we can decide if we need to show  // the diagnostics screen
> > + // Notes: this quiet boot code should be remove  // from the graphic
> > + lib  //  if (QuietBoot) {
> > +
> > +    //
> > +    // Perform system diagnostic
> > +    //
> > +    Status = PlatformBootManagerMemoryTest (MemoryTestLevel);
> > +    return;
> > +  }
> > +
> > +  //
> > +  // Perform system diagnostic
> > +  //
> > +  Status = PlatformBootManagerMemoryTest (MemoryTestLevel);
> > +}
> > +
> > +/**
> > +  Return the index of the load option in the load option array.
> > +
> > +  The function consider two load options are equal when the
> > +  OptionType, Attributes, Description, FilePath and OptionalData are equal.
> > +
> > +  @param Key    Pointer to the load option to be found.
> > +  @param Array  Pointer to the array of load options to be found.
> > +  @param Count  Number of entries in the Array.
> > +
> > +  @retval -1          Key wasn't found in the Array.
> > +  @retval 0 ~ Count-1 The index of the Key in the Array.
> > +**/
> > +INTN
> > +PlatformFindLoadOption (
> > +  IN CONST EFI_BOOT_MANAGER_LOAD_OPTION *Key,
> > +  IN CONST EFI_BOOT_MANAGER_LOAD_OPTION *Array,
> > +  IN UINTN                              Count
> > +  )
> > +{
> > +  UINTN                             Index;
> > +
> > +  for (Index = 0; Index < Count; Index++) {
> > +    if ((Key->OptionType == Array[Index].OptionType) &&
> > +        (Key->Attributes == Array[Index].Attributes) &&
> > +        (StrCmp (Key->Description, Array[Index].Description) == 0) &&
> > +        (CompareMem (Key->FilePath, Array[Index].FilePath,
> GetDevicePathSize (Key->FilePath)) == 0) &&
> > +        (Key->OptionalDataSize == Array[Index].OptionalDataSize) &&
> > +        (CompareMem (Key->OptionalData, Array[Index].OptionalData, Key-
> >OptionalDataSize) == 0)) {
> > +      return (INTN) Index;
> > +    }
> > +  }
> > +
> > +  return -1;
> > +}
> > +
> > +VOID
> > +PlatformRegisterFvBootOption (
> > +  EFI_GUID                         *FileGuid,
> > +  CHAR16                           *Description,
> > +  UINT32                           Attributes
> > +  )
> > +{
> > +  EFI_STATUS                        Status;
> > +  UINTN                             OptionIndex;
> > +  EFI_BOOT_MANAGER_LOAD_OPTION      NewOption;
> > +  EFI_BOOT_MANAGER_LOAD_OPTION      *BootOptions;
> > +  UINTN                             BootOptionCount;
> > +  MEDIA_FW_VOL_FILEPATH_DEVICE_PATH FileNode;
> > +  EFI_LOADED_IMAGE_PROTOCOL         *LoadedImage;
> > +  EFI_DEVICE_PATH_PROTOCOL          *DevicePath;
> > +
> > +  Status = gBS->HandleProtocol (gImageHandle,
> &gEfiLoadedImageProtocolGuid, (VOID **) &LoadedImage);
> > +  ASSERT_EFI_ERROR (Status);
> > +
> > +  EfiInitializeFwVolDevicepathNode (&FileNode, FileGuid);
> > +  DevicePath = AppendDevicePathNode (
> > +                 DevicePathFromHandle (LoadedImage->DeviceHandle),
> > +                 (EFI_DEVICE_PATH_PROTOCOL *) &FileNode
> > +                 );
> > +
> > +  Status = EfiBootManagerInitializeLoadOption (
> > +             &NewOption,
> > +             LoadOptionNumberUnassigned,
> > +             LoadOptionTypeBoot,
> > +             Attributes,
> > +             Description,
> > +             DevicePath,
> > +             NULL,
> > +             0
> > +             );
> > +  if (!EFI_ERROR (Status)) {
> > +    BootOptions = EfiBootManagerGetLoadOptions (&BootOptionCount,
> LoadOptionTypeBoot);
> > +
> > +    OptionIndex = PlatformFindLoadOption (&NewOption, BootOptions,
> BootOptionCount);
> > +
> > +    if (OptionIndex == -1) {
> > +      Status = EfiBootManagerAddLoadOptionVariable (&NewOption,
> (UINTN) -1);
> > +      ASSERT_EFI_ERROR (Status);
> > +    }
> > +    EfiBootManagerFreeLoadOption (&NewOption);
> > +    EfiBootManagerFreeLoadOptions (BootOptions, BootOptionCount);
> > +  }
> > +}
> > +
> > +/**
> > +  Do the platform specific action before the console is connected.
> > +
> > +  Such as:
> > +    Update console variable;
> > +    Register new Driver#### or Boot####;
> > +    Signal ReadyToLock event.
> > +**/
> > +VOID
> > +EFIAPI
> > +PlatformBootManagerBeforeConsole (
> > +  VOID
> > +  )
> > +{
> > +  UINTN                        Index;
> > +  EFI_STATUS                   Status;
> > +  EFI_INPUT_KEY                Enter;
> > +  EFI_INPUT_KEY                F2;
> > +  EFI_BOOT_MANAGER_LOAD_OPTION BootOption;
> > +
> > +  //
> > +  // Update the console variables.
> > +  //
> > +  for (Index = 0; gPlatformConsole[Index].DevicePath != NULL; Index++) {
> > +    DEBUG ((DEBUG_INFO, "Check gPlatformConsole %d\n", Index));
> > +    if ((gPlatformConsole[Index].ConnectType & CONSOLE_IN) ==
> CONSOLE_IN) {
> > +      Status = EfiBootManagerUpdateConsoleVariable (ConIn,
> gPlatformConsole[Index].DevicePath, NULL);
> > +      DEBUG ((DEBUG_INFO, "CONSOLE_IN variable set %s : %r\n",
> ConvertDevicePathToText (gPlatformConsole[Index].DevicePath, FALSE,
> FALSE), Status));
> 
> Some really long lines here, can we wrap them?
> 
> > +    }
> > +
> > +    if ((gPlatformConsole[Index].ConnectType & CONSOLE_OUT) ==
> CONSOLE_OUT) {
> > +      Status = EfiBootManagerUpdateConsoleVariable (ConOut,
> gPlatformConsole[Index].DevicePath, NULL);
> > +      DEBUG ((DEBUG_INFO, "CONSOLE_OUT variable set %s : %r\n",
> ConvertDevicePathToText (gPlatformConsole[Index].DevicePath, FALSE,
> FALSE), Status));
> > +    }
> > +
> > +    if ((gPlatformConsole[Index].ConnectType & STD_ERROR) ==
> STD_ERROR) {
> > +      Status = EfiBootManagerUpdateConsoleVariable (ErrOut,
> gPlatformConsole[Index].DevicePath, NULL);
> > +      DEBUG ((DEBUG_INFO, "STD_ERROR variable set %r", Status));
> > +    }
> > +  }
> > +
> > +  //
> > +  // Register ENTER as CONTINUE key
> > +  //
> > +  Enter.ScanCode    = SCAN_NULL;
> > +  Enter.UnicodeChar = CHAR_CARRIAGE_RETURN;
> > +  EfiBootManagerRegisterContinueKeyOption (0, &Enter, NULL);
> > +  //
> > +  // Map F2 to Boot Manager Menu
> > +  //
> > +  F2.ScanCode    = SCAN_F2;
> > +  F2.UnicodeChar = CHAR_NULL;
> > +  EfiBootManagerGetBootManagerMenu (&BootOption);
> > +  EfiBootManagerAddKeyOptionVariable (NULL, (UINT16)
> BootOption.OptionNumber, 0, &F2, NULL);
> > +  //
> > +  // Register UEFI Shell
> > +  //
> > +  PlatformRegisterFvBootOption (&mUefiShellFileGuid, L"UEFI Shell",
> LOAD_OPTION_ACTIVE);
> > +}
> > +
> > +/**
> > +  Do the platform specific action after the console is connected.
> > +
> > +  Such as:
> > +    Dynamically switch output mode;
> > +    Signal console ready platform customized event;
> > +    Run diagnostics like memory testing;
> > +    Connect certain devices;
> > +    Dispatch aditional option roms.
> > +**/
> > +VOID
> > +EFIAPI
> > +PlatformBootManagerAfterConsole (
> > +  VOID
> > +  )
> > +{
> > +  EFI_GRAPHICS_OUTPUT_BLT_PIXEL  Black;
> > +  EFI_GRAPHICS_OUTPUT_BLT_PIXEL  White;
> > +
> > +  Black.Blue = Black.Green = Black.Red = Black.Reserved = 0;
> > +  White.Blue = White.Green = White.Red = White.Reserved = 0xFF;
> > +
> > +  EfiBootManagerConnectAll ();
> > +  EfiBootManagerRefreshAllBootOption ();
> > +
> > +  PlatformBootManagerDiagnostics (QUICK, TRUE);
> > +
> > +  PrintXY (10, 10, &White, &Black, L"F2    to enter Boot Manager Menu.
> ");
> > +  PrintXY (10, 30, &White, &Black, L"Enter to boot directly.");
> > +}
> > +
> > +/**
> > +  This function is called each second during the boot manager waits the
> timeout.
> > +
> > +  @param TimeoutRemain  The remaining timeout.
> > +**/
> > +VOID
> > +EFIAPI
> > +PlatformBootManagerWaitCallback (
> > +  UINT16          TimeoutRemain
> > +  )
> > +{
> > +  EFI_GRAPHICS_OUTPUT_BLT_PIXEL Black;
> > +  EFI_GRAPHICS_OUTPUT_BLT_PIXEL White;
> > +  UINT16                        Timeout;
> > +
> > +  Timeout = PcdGet16 (PcdPlatformBootTimeOut);
> > +
> > +  Black.Blue = Black.Green = Black.Red = Black.Reserved = 0;
> > +  White.Blue = White.Green = White.Red = White.Reserved = 0xFF;
> > +
> > +  PlatformBootManagerShowProgress (
> > +    White,
> > +    Black,
> > +    L"Start boot option",
> > +    White,
> > +    (Timeout - TimeoutRemain) * 100 / Timeout,
> > +    0
> > +    );
> > +}
> > +
> > +/**
> > +  The function is called when no boot option could be launched,
> > +  including platform recovery options and options pointing to applications
> > +  built into firmware volumes.
> > +
> > +  If this function returns, BDS attempts to enter an infinite loop.
> > +**/
> > +VOID
> > +EFIAPI
> > +PlatformBootManagerUnableToBoot (
> > +  VOID
> > +  )
> > +{
> > +  return;
> > +}
> > diff --git
> a/Platform/RiscV/SiFive/U500Pkg/Library/PlatformBootManagerLib/Platform
> BootManager.h
> b/Platform/RiscV/SiFive/U500Pkg/Library/PlatformBootManagerLib/Platfor
> mBootManager.h
> > new file mode 100644
> > index 00000000..20d66758
> > --- /dev/null
> > +++
> b/Platform/RiscV/SiFive/U500Pkg/Library/PlatformBootManagerLib/Platfor
> mBootManager.h
> > @@ -0,0 +1,135 @@
> > +/**@file
> > +   Head file for BDS Platform specific code
> > +
> > +Copyright (c) 2016, Hewlett Packard Enterprise Development LP. All rights
> reserved.<BR>
> > +Copyright (c) 2015, Intel Corporation. All rights reserved.<BR>
> > +
> > +SPDX-License-Identifier: BSD-2-Clause-Patent
> > +
> > +**/
> > +
> > +#ifndef _PLATFORM_BOOT_MANAGER_H_
> > +#define _PLATFORM_BOOT_MANAGER_H_
> 
> Please drop leading _.
> 
> > +
> > +#include <PiDxe.h>
> > +#include <IndustryStandard/Bmp.h>
> > +#include <Protocol/GenericMemoryTest.h>
> > +#include <Protocol/LoadedImage.h>
> > +#include <Protocol/UgaDraw.h>
> > +#include <Protocol/GraphicsOutput.h>
> > +#include <Protocol/BootLogo.h>
> > +#include <Protocol/DevicePath.h>
> > +
> > +#include <Library/DebugLib.h>
> > +#include <Library/BaseMemoryLib.h>
> > +#include <Library/UefiBootServicesTableLib.h>
> > +#include <Library/UefiRuntimeServicesTableLib.h>
> > +#include <Library/MemoryAllocationLib.h>
> > +#include <Library/BaseLib.h>
> > +#include <Library/UefiRuntimeServicesTableLib.h>
> > +#include <Library/UefiLib.h>
> > +#include <Library/UefiBootManagerLib.h>
> > +#include <Library/PcdLib.h>
> > +#include <Library/DevicePathLib.h>
> > +#include <Library/HiiLib.h>
> > +#include <Library/PrintLib.h>
> > +#include <Library/DxeServicesLib.h>
> 
> Please remove any includes not needed by this file, and add them to
> the files that actually use them.
> 
> > +
> > +typedef struct {
> > +  EFI_DEVICE_PATH_PROTOCOL  *DevicePath;
> > +  UINTN                     ConnectType;
> > +} PLATFORM_CONSOLE_CONNECT_ENTRY;
> > +
> > +extern PLATFORM_CONSOLE_CONNECT_ENTRY  gPlatformConsole[];
> > +
> > +#define gEndEntire \
> > +  { \
> > +    END_DEVICE_PATH_TYPE,\
> > +    END_ENTIRE_DEVICE_PATH_SUBTYPE,\
> > +    END_DEVICE_PATH_LENGTH,\
> > +    0\
> > +  }
> > +
> > +#define CONSOLE_OUT BIT0
> > +#define CONSOLE_IN  BIT1
> > +#define STD_ERROR   BIT2
> > +
> > +//D3987D4B-971A-435F-8CAF-4967EB627241
> > +#define EFI_SERIAL_DXE_GUID \
> > +  { 0xD3987D4B, 0x971A, 0x435F, { 0x8C, 0xAF, 0x49, 0x67, 0xEB, 0x62, 0x72,
> 0x41 } }
> > +
> > +typedef struct {
> > +  VENDOR_DEVICE_PATH        Guid;
> > +  UART_DEVICE_PATH          Uart;
> > +  VENDOR_DEVICE_PATH        TerminalType;
> > +  EFI_DEVICE_PATH_PROTOCOL  End;
> > +} SERIAL_CONSOLE_DEVICE_PATH;
> > +
> > +/**
> > +  Use SystemTable Conout to stop video based Simple Text Out consoles
> from going
> > +  to the video device. Put up LogoFile on every video device that is a
> console.
> > +
> > +  @param[in]  LogoFile   File name of logo to display on the center of the
> screen.
> > +
> > +  @retval EFI_SUCCESS     ConsoleControl has been flipped to graphics and
> logo displayed.
> > +  @retval EFI_UNSUPPORTED Logo not found
> > +
> > +**/
> > +EFI_STATUS
> > +PlatformBootManagerEnableQuietBoot (
> > +  IN  EFI_GUID  *LogoFile
> > +  );
> > +
> > +/**
> > +  Use SystemTable Conout to turn on video based Simple Text Out
> consoles. The
> > +  Simple Text Out screens will now be synced up with all non video output
> devices
> > +
> > +  @retval EFI_SUCCESS     UGA devices are back in text mode and synced
> up.
> > +
> > +**/
> > +EFI_STATUS
> > +PlatformBootManagerDisableQuietBoot (
> > +  VOID
> > +  );
> > +
> > +/**
> > +  Perform the memory test base on the memory test intensive level,
> > +  and update the memory resource.
> > +
> > +  @param  Level         The memory test intensive level.
> > +
> > +  @retval EFI_STATUS    Success test all the system memory and update
> > +                        the memory resource
> > +
> > +**/
> > +EFI_STATUS
> > +PlatformBootManagerMemoryTest (
> > +  IN EXTENDMEM_COVERAGE_LEVEL Level
> > +  );
> > +
> > +/**
> > +
> > +  Show progress bar with title above it. It only works in Graphics mode.
> > +
> > +
> > +  @param TitleForeground Foreground color for Title.
> > +  @param TitleBackground Background color for Title.
> > +  @param Title           Title above progress bar.
> > +  @param ProgressColor   Progress bar color.
> > +  @param Progress        Progress (0-100)
> > +  @param PreviousValue   The previous value of the progress.
> > +
> > +  @retval  EFI_STATUS       Success update the progress bar
> > +
> > +**/
> > +EFI_STATUS
> > +PlatformBootManagerShowProgress (
> > +  IN EFI_GRAPHICS_OUTPUT_BLT_PIXEL TitleForeground,
> > +  IN EFI_GRAPHICS_OUTPUT_BLT_PIXEL TitleBackground,
> > +  IN CHAR16                        *Title,
> > +  IN EFI_GRAPHICS_OUTPUT_BLT_PIXEL ProgressColor,
> > +  IN UINTN                         Progress,
> > +  IN UINTN                         PreviousValue
> > +  );
> > +
> > +#endif // _PLATFORM_BOOT_MANAGER_H
> > diff --git
> a/Platform/RiscV/SiFive/U500Pkg/Library/PlatformBootManagerLib/Platform
> BootManagerLib.inf
> b/Platform/RiscV/SiFive/U500Pkg/Library/PlatformBootManagerLib/Platfor
> mBootManagerLib.inf
> > new file mode 100644
> > index 00000000..92c31db4
> > --- /dev/null
> > +++
> b/Platform/RiscV/SiFive/U500Pkg/Library/PlatformBootManagerLib/Platfor
> mBootManagerLib.inf
> > @@ -0,0 +1,63 @@
> > +## @file
> > +#  Include all platform action which can be customized by IBV/OEM.
> > +#
> > +#  Copyright (c) 2019, Hewlett Packard Enterprise Development LP. All
> rights reserved.<BR>
> > +#  Copyright (c) 2012 - 2015, Intel Corporation. All rights reserved.<BR>
> > +#
> > +#  SPDX-License-Identifier: BSD-2-Clause-Patent
> > +#
> > +##
> > +
> > +[Defines]
> > +  INF_VERSION                    = 0x00010005
> 
> Please bump specification version.
> 
> > +  BASE_NAME                      = PlatformBootManagerLib
> > +  FILE_GUID                      = 7DDA7916-6139-4D46-A415-30E854AF3BC7
> > +  MODULE_TYPE                    = DXE_DRIVER
> > +  VERSION_STRING                 = 1.0
> > +  LIBRARY_CLASS                  = PlatformBootManagerLib|DXE_DRIVER
> > +
> > +#
> > +# The following information is for reference only and not required by the
> build tools.
> > +#
> > +#  VALID_ARCHITECTURES           = RISCV
> > +#
> > +
> > +[Sources]
> > +  PlatformData.c
> > +  PlatformBootManager.c
> > +  PlatformBootManager.h
> > +  MemoryTest.c
> > +  Strings.uni
> > +
> > +[Packages]
> > +  MdePkg/MdePkg.dec
> > +  MdeModulePkg/MdeModulePkg.dec
> > +  Platform/RiscV/RiscVPlatformPkg.dec
> > +
> > +[LibraryClasses]
> > +  BaseLib
> > +  UefiBootServicesTableLib
> > +  UefiRuntimeServicesTableLib
> > +  UefiLib
> > +  UefiBootManagerLib
> > +  PcdLib
> > +  DxeServicesLib
> > +  MemoryAllocationLib
> > +  DevicePathLib
> > +  HiiLib
> > +  PrintLib
> 
> Please sort Sources, Packages and LibraryClasses alphabetically.
> 
> > +
> > +[Guids]
> > +
> > +[Protocols]
> > +  gEfiGenericMemTestProtocolGuid  ## CONSUMES
> > +  gEfiGraphicsOutputProtocolGuid  ## CONSUMES
> > +  gEfiUgaDrawProtocolGuid         ## CONSUMES
> 
> You're hopefully not really using UGA, so please drop that.
> 
> > +
> > +[Pcd]
> > +  gEfiMdePkgTokenSpaceGuid.PcdPlatformBootTimeOut
> > +  gEfiMdeModulePkgTokenSpaceGuid.PcdConOutRow
> > +  gEfiMdeModulePkgTokenSpaceGuid.PcdConOutColumn
> > +  gEfiMdePkgTokenSpaceGuid.PcdUgaConsumeSupport
> > +  gEfiMdeModulePkgTokenSpaceGuid.PcdConInConnectOnDemand
> > +  gUefiRiscVPlatformPkgTokenSpaceGuid.PcdBootlogoOnlyEnable
> 
> Please sort Pcds alphabetically.
> 
> > diff --git
> a/Platform/RiscV/SiFive/U500Pkg/Library/PlatformBootManagerLib/Platform
> Data.c
> b/Platform/RiscV/SiFive/U500Pkg/Library/PlatformBootManagerLib/Platfor
> mData.c
> > new file mode 100644
> > index 00000000..3208051e
> > --- /dev/null
> > +++
> b/Platform/RiscV/SiFive/U500Pkg/Library/PlatformBootManagerLib/Platfor
> mData.c
> > @@ -0,0 +1,49 @@
> > +/**@file
> > +  Defined the platform specific device path which will be filled to
> > +  ConIn/ConOut variables.
> > +
> > +Copyright (c) 2019, Hewlett Packard Enterprise Development LP. All rights
> reserved.<BR>
> > +Copyright (c) 2015, Intel Corporation. All rights reserved.<BR>
> > +
> > +SPDX-License-Identifier: BSD-2-Clause-Patent
> > +
> > +**/
> > +
> > +#include "PlatformBootManager.h"
> > +
> > +//
> > +// Platform specific serial device path
> > +//
> > +SERIAL_CONSOLE_DEVICE_PATH gSerialConsoleDevicePath0 = {
> > +  {
> > +    { HARDWARE_DEVICE_PATH, HW_VENDOR_DP, { sizeof
> (VENDOR_DEVICE_PATH), 0} },
> > +    EFI_SERIAL_DXE_GUID  // Use the driver's GUID
> > +  },
> > +  {
> > +    { MESSAGING_DEVICE_PATH, MSG_UART_DP, { sizeof
> (UART_DEVICE_PATH), 0} },
> > +    0,                  // Reserved
> > +    115200,             // BaudRate
> > +    8,                  // DataBits
> > +    1,                  // Parity
> > +    1                   // StopBits
> > +  },
> > +  {
> > +    { MESSAGING_DEVICE_PATH, MSG_VENDOR_DP, { sizeof
> (VENDOR_DEVICE_PATH), 0} },
> > +    DEVICE_PATH_MESSAGING_PC_ANSI
> > +  },
> > +  { END_DEVICE_PATH_TYPE, END_ENTIRE_DEVICE_PATH_SUBTYPE,
> { sizeof (EFI_DEVICE_PATH_PROTOCOL), 0 } }
> > +};
> > +
> > +//
> > +// Predefined platform default console device path
> > +//
> > +PLATFORM_CONSOLE_CONNECT_ENTRY   gPlatformConsole[] = {
> > +  {
> > +    (EFI_DEVICE_PATH_PROTOCOL *) &gSerialConsoleDevicePath0,
> > +    CONSOLE_OUT | CONSOLE_IN
> > +  },
> > +  {
> > +    NULL,
> > +    0
> > +  }
> > +};
> > diff --git
> a/Platform/RiscV/SiFive/U500Pkg/Library/PlatformBootManagerLib/Strings.u
> ni
> b/Platform/RiscV/SiFive/U500Pkg/Library/PlatformBootManagerLib/Strings.
> uni
> > new file mode 100644
> > index 00000000..73bf5d51
> > --- /dev/null
> > +++
> b/Platform/RiscV/SiFive/U500Pkg/Library/PlatformBootManagerLib/Strings.
> uni
> > @@ -0,0 +1,28 @@
> > +///** @file
> > +//
> > +//    String definitions for PlatformBootManagerLib.
> > +//
> > +//  Copyright (c) 2019, Hewlett Packard Enterprise Development LP. All
> rights reserved.<BR>
> > +//  Copyright (c) 2004 - 2015, Intel Corporation. All rights reserved.<BR>
> > +//
> > +//  SPDX-License-Identifier: BSD-2-Clause-Patent
> > +//
> > +//**/
> > +
> > +/=#
> > +
> > +#langdef   en-US "English"
> > +#langdef   fr-FR "Français"
> > +
> > +#string STR_PERFORM_MEM_TEST           #language en-US  "Perform
> memory test (ESC to skip)"
> > +                                       #language fr-FR  "Exécute l'examen de mémoire (ESC
> pour sauter)"
> > +#string STR_MEMORY_TEST_PERCENT        #language en-US  "% of the
> system memory tested OK"
> > +                                       #language fr-FR  "% de la mémoire de système
> essayée D'ACCORD"
> > +#string STR_ESC_TO_SKIP_MEM_TEST       #language en-US  "Press ESC
> key to skip memory test"
> > +                                       #language fr-FR  "Appuie sur ESC sauter examen de
> mémoire"
> > +#string STR_MEM_TEST_COMPLETED         #language en-US  " bytes of
> system memory tested OK\r\n"
> > +                                       #language fr-FR  "octets dela mémoire de système
> essayée D'ACCORD\r\n"
> 
> if there's a space before 'bytes' you probably want one before
> 'octets'.
> 
> /
>     Leif
> 
> > +#string STR_SYSTEM_MEM_ERROR           #language en-US  "System
> encounters memory errors"
> > +                                       #language fr-FR  "le Système rencontre les erreurs de
> mémoire"
> > +#string STR_START_BOOT_OPTION          #language en-US  "Start boot
> option"
> > +                                       #language fr-FR  "l'option de botte de Début"
> > --
> > 2.12.0.windows.1
> >
> >
> >
> >
> 
>