From mboxrd@z Thu Jan  1 00:00:00 1970
Authentication-Results: mx.groups.io;
 dkim=pass header.i=@linaro.org header.s=google header.b=m01eVKzk;
 spf=pass (domain: linaro.org, ip: 209.85.221.65, mailfrom: leif.lindholm@linaro.org)
Received: from mail-wr1-f65.google.com (mail-wr1-f65.google.com [209.85.221.65])
 by groups.io with SMTP; Wed, 02 Oct 2019 15:02:54 -0700
Received: by mail-wr1-f65.google.com with SMTP id r3so677190wrj.6
        for <devel@edk2.groups.io>; Wed, 02 Oct 2019 15:02:54 -0700 (PDT)
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed;
        d=linaro.org; s=google;
        h=date:from:to:cc:subject:message-id:references:mime-version
         :content-disposition:content-transfer-encoding:in-reply-to
         :user-agent;
        bh=ruTxaRMtWDAmaZ8HGwjALHD2V1MwsB/tsqZz5CTvqyc=;
        b=m01eVKzkuPJPcL1Lgoi6mBwkx/7XeAS6EVFPN4s78k1UzrITuP0J3XTFFjgBtRo7De
         0F7+LQ0C66Wb669GBmAskOVwbJU30nP9wuMQtQKgOmmYBe5++XRNiHzWVUmb5/DzLLwV
         fLQd0uez0ZK42wZzP8FWwD9NV4WVrAptXz4g4B8EW6JHIb0P12nBlei4B6ocl9Lp4PCt
         +m/uwj6WnKXcMFmlm+8gGROULDqpipedJoztBZQWHJLZGb9vLwNNjZmYwDmukHzttRv3
         n1gOoKxX2QbqOGrcMm1AYWV2UWHH1YmotVtzOalwO5IcQhcecc/LWCAgH5kX7KtCToou
         5FqA==
X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed;
        d=1e100.net; s=20161025;
        h=x-gm-message-state:date:from:to:cc:subject:message-id:references
         :mime-version:content-disposition:content-transfer-encoding
         :in-reply-to:user-agent;
        bh=ruTxaRMtWDAmaZ8HGwjALHD2V1MwsB/tsqZz5CTvqyc=;
        b=VNgKGuTgHSTKvZHL6iGkp7VAdeDUt/znoG+xCre8SgVexo8IWeQt5QrV9bczifCBLv
         vetIQ+uGI1Uf3nl2ylrEJm+kMnJvjtbVqQ4DwfQvZSYu1hpcaEVxUWRtn2fYNqbW3j2C
         SPXml+Kq8X8zRXIopfyCjZnosbASg4WwCkB0SPbTDpwRSrc1f++U3dfd0FG33O1VtKR3
         hcHY4t04qsLh46Z8xehsBcVLgUQ+z1mv8OMQWRtAm92h1Mm4W+PuS495XkjBoYvFmvuc
         p/hfXlMelDhdQ8WAxB8/U2hPqOoifN7AEkSzfafi0BHiU+Rg+pLX9ZFlBypIKRIwzyKu
         O+MQ==
X-Gm-Message-State: APjAAAVkfegFq5lFEJO19/FVImbonJAOmq6+ZNC8nYfObjSkVx08wGMy
	0PYTnnS+nJqo/4QlXmSSQMfIzav758Y=
X-Google-Smtp-Source: APXvYqwzRTvYMu055QOb/+0av8HyXVQ2oPkUttiATbPCVpe3MfBJUaj0dskAUEVzt5UXLpAbuD2kGg==
X-Received: by 2002:a5d:5261:: with SMTP id l1mr4587762wrc.182.1570053771808;
        Wed, 02 Oct 2019 15:02:51 -0700 (PDT)
Return-Path: <leif.lindholm@linaro.org>
Received: from bivouac.eciton.net (bivouac.eciton.net. [2a00:1098:0:86:1000:23:0:2])
        by smtp.gmail.com with ESMTPSA id n22sm358883wmk.19.2019.10.02.15.02.50
        (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);
        Wed, 02 Oct 2019 15:02:50 -0700 (PDT)
Date: Wed, 2 Oct 2019 23:02:48 +0100
From: "Leif Lindholm" <leif.lindholm@linaro.org>
To: devel@edk2.groups.io, 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
Message-ID: <20191002220248.GI25504@bivouac.eciton.net>
References: <20190919035131.4700-1-gilbert.chen@hpe.com>
 <20190919035131.4700-10-gilbert.chen@hpe.com>
MIME-Version: 1.0
In-Reply-To: <20190919035131.4700-10-gilbert.chen@hpe.com>
User-Agent: Mutt/1.10.1 (2018-07-13)
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit

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/MemoryTest.c
>  create mode 100644 Platform/RiscV/SiFive/U500Pkg/Library/PlatformBootManagerLib/PlatformBootManager.c
>  create mode 100644 Platform/RiscV/SiFive/U500Pkg/Library/PlatformBootManagerLib/PlatformBootManager.h
>  create mode 100644 Platform/RiscV/SiFive/U500Pkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf
>  create mode 100644 Platform/RiscV/SiFive/U500Pkg/Library/PlatformBootManagerLib/PlatformData.c
>  create mode 100644 Platform/RiscV/SiFive/U500Pkg/Library/PlatformBootManagerLib/Strings.uni
> 
> diff --git a/Platform/RiscV/SiFive/U500Pkg/Library/PlatformBootManagerLib/MemoryTest.c b/Platform/RiscV/SiFive/U500Pkg/Library/PlatformBootManagerLib/MemoryTest.c
> new file mode 100644
> index 00000000..8c6d89e9
> --- /dev/null
> +++ b/Platform/RiscV/SiFive/U500Pkg/Library/PlatformBootManagerLib/MemoryTest.c

Why build a MemoryTest into the PlatformBootManagerLib?

> @@ -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.

> diff --git a/Platform/RiscV/SiFive/U500Pkg/Library/PlatformBootManagerLib/PlatformBootManager.c b/Platform/RiscV/SiFive/U500Pkg/Library/PlatformBootManagerLib/PlatformBootManager.c
> new file mode 100644
> index 00000000..9ef85089
> --- /dev/null
> +++ b/Platform/RiscV/SiFive/U500Pkg/Library/PlatformBootManagerLib/PlatformBootManager.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/PlatformBootManager.h b/Platform/RiscV/SiFive/U500Pkg/Library/PlatformBootManagerLib/PlatformBootManager.h
> new file mode 100644
> index 00000000..20d66758
> --- /dev/null
> +++ b/Platform/RiscV/SiFive/U500Pkg/Library/PlatformBootManagerLib/PlatformBootManager.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/PlatformBootManagerLib.inf b/Platform/RiscV/SiFive/U500Pkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf
> new file mode 100644
> index 00000000..92c31db4
> --- /dev/null
> +++ b/Platform/RiscV/SiFive/U500Pkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.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/PlatformData.c b/Platform/RiscV/SiFive/U500Pkg/Library/PlatformBootManagerLib/PlatformData.c
> new file mode 100644
> index 00000000..3208051e
> --- /dev/null
> +++ b/Platform/RiscV/SiFive/U500Pkg/Library/PlatformBootManagerLib/PlatformData.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.uni 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
> 
> 
> 
>