Re: [edk2-devel] [edk2-platforms PATCH v2 1/2] Platform/RaspberryPi: Enable Boot Discovery Policy.

["Gao, Zhichao" <zhichao.gao@intel.com>]

See below comments.

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
> Grzegorz Bernacki
> Sent: Tuesday, July 6, 2021 6:45 PM
> To: devel@edk2.groups.io
> Cc: leif@nuviainc.com; ardb+tianocore@kernel.org; Samer.El-Haj-
> Mahmoud@arm.com; sunny.Wang@arm.com; mw@semihalf.com;
> upstream@semihalf.com; pete@akeo.ie; Wang, Jian J
> <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; Bi, Dandan
> <dandan.bi@intel.com>; Dong, Eric <eric.dong@intel.com>; Grzegorz
> Bernacki <gjb@semihalf.com>; Sunny Wang <sunny.wang@arm.com>
> Subject: [edk2-devel] [edk2-platforms PATCH v2 1/2] Platform/RaspberryPi:
> Enable Boot Discovery Policy.
> 
> This commit modify platform boot to check the value of BootDiscoveryPolicy
> variable and use BootPolicyManager Protocol to connect devices specified by
> the variable.
> 
> Signed-off-by: Grzegorz Bernacki <gjb@semihalf.com>
> Reviewed-by: Sunny Wang <sunny.wang@arm.com>
> ---
>  Platform/RaspberryPi/RPi4/RPi4.dsc                                             |  3 +
>  Platform/RaspberryPi/RPi4/RPi4.fdf                                             |  1 +
> 
> Platform/RaspberryPi/Library/PlatformBootManagerLib/PlatformBootManag
> erLib.inf |  5 ++
>  Platform/RaspberryPi/Library/PlatformBootManagerLib/PlatformBm.c
> | 90 ++++++++++++++++++++
>  4 files changed, 99 insertions(+)
> 
> diff --git a/Platform/RaspberryPi/RPi4/RPi4.dsc
> b/Platform/RaspberryPi/RPi4/RPi4.dsc
> index fd73c4d14b..8b9beac64a 100644
> --- a/Platform/RaspberryPi/RPi4/RPi4.dsc
> +++ b/Platform/RaspberryPi/RPi4/RPi4.dsc
> @@ -555,6 +555,7 @@
> 
> gEfiMdeModulePkgTokenSpaceGuid.PcdConOutColumn|L"Columns"|gRasp
> berryPiTokenSpaceGuid|0x0|80
> 
> gEfiMdeModulePkgTokenSpaceGuid.PcdSetupConOutRow|L"Rows"|gRaspb
> erryPiTokenSpaceGuid|0x0|25
> 
> gEfiMdeModulePkgTokenSpaceGuid.PcdConOutRow|L"Rows"|gRaspberryPi
> TokenSpaceGuid|0x0|25
> +
> +
> gEfiMdeModulePkgTokenSpaceGuid.PcdBootDiscoveryPolicy|L"BootDiscove
> ryP
> + olicy"|gBootDiscoveryPolicyMgrFormsetGuid|0
> 
>  [PcdsDynamicDefault.common]
>    #
> @@ -682,6 +683,7 @@
>    #
>    # Bds
>    #
> +
> MdeModulePkg/Universal/BootManagerPolicyDxe/BootManagerPolicyDxe.i
> nf
>    MdeModulePkg/Universal/DevicePathDxe/DevicePathDxe.inf
>    MdeModulePkg/Universal/DisplayEngineDxe/DisplayEngineDxe.inf
>    MdeModulePkg/Universal/SetupBrowserDxe/SetupBrowserDxe.inf
> @@ -690,6 +692,7 @@
>    Platform/RaspberryPi/Drivers/LogoDxe/LogoDxe.inf
>    MdeModulePkg/Application/UiApp/UiApp.inf {
>      <LibraryClasses>
> +
> +
> NULL|MdeModulePkg/Library/BootDiscoveryPolicyUiLib/BootDiscoveryPolic
> y
> + UiLib.inf
> 
> NULL|MdeModulePkg/Library/DeviceManagerUiLib/DeviceManagerUiLib.inf
>        NULL|MdeModulePkg/Library/BootManagerUiLib/BootManagerUiLib.inf
> 
> NULL|Platform/RaspberryPi/Library/PlatformUiAppLib/PlatformUiAppLib.inf
> diff --git a/Platform/RaspberryPi/RPi4/RPi4.fdf
> b/Platform/RaspberryPi/RPi4/RPi4.fdf
> index 1e13909a57..371197a93e 100644
> --- a/Platform/RaspberryPi/RPi4/RPi4.fdf
> +++ b/Platform/RaspberryPi/RPi4/RPi4.fdf
> @@ -253,6 +253,7 @@ READ_LOCK_STATUS   = TRUE
>    #
>    # Bds
>    #
> +  INF
> +
> MdeModulePkg/Universal/BootManagerPolicyDxe/BootManagerPolicyDxe.i
> nf
>    INF MdeModulePkg/Universal/DevicePathDxe/DevicePathDxe.inf
>    INF MdeModulePkg/Universal/DisplayEngineDxe/DisplayEngineDxe.inf
>    INF MdeModulePkg/Universal/SetupBrowserDxe/SetupBrowserDxe.inf
> diff --git
> a/Platform/RaspberryPi/Library/PlatformBootManagerLib/PlatformBootMan
> agerLib.inf
> b/Platform/RaspberryPi/Library/PlatformBootManagerLib/PlatformBootMan
> agerLib.inf
> index fbf510ab96..4ef2f791ae 100644
> ---
> a/Platform/RaspberryPi/Library/PlatformBootManagerLib/PlatformBootMan
> agerLib.inf
> +++
> b/Platform/RaspberryPi/Library/PlatformBootManagerLib/PlatformBootMa
> +++ nagerLib.inf
> @@ -61,11 +61,13 @@
>    gEfiMdePkgTokenSpaceGuid.PcdDefaultTerminalType
> 
>  [Pcd]
> +  gEfiMdeModulePkgTokenSpaceGuid.PcdBootDiscoveryPolicy
>    gEfiMdePkgTokenSpaceGuid.PcdPlatformBootTimeOut
>    gRaspberryPiTokenSpaceGuid.PcdSdIsArasan
>    gRaspberryPiTokenSpaceGuid.PcdBootPolicy
> 
>  [Guids]
> +  gBootDiscoveryPolicyMgrFormsetGuid
>    gEfiFileInfoGuid
>    gEfiFileSystemInfoGuid
>    gEfiFileSystemVolumeLabelInfoIdGuid
> @@ -73,8 +75,11 @@
>    gEfiTtyTermGuid
>    gUefiShellFileGuid
>    gEfiEventExitBootServicesGuid
> +  gEfiBootManagerPolicyNetworkGuid
> +  gEfiBootManagerPolicyConnectAllGuid
> 
>  [Protocols]
> +  gEfiBootManagerPolicyProtocolGuid
>    gEfiDevicePathProtocolGuid
>    gEfiGraphicsOutputProtocolGuid
>    gEfiLoadedImageProtocolGuid
> diff --git
> a/Platform/RaspberryPi/Library/PlatformBootManagerLib/PlatformBm.c
> b/Platform/RaspberryPi/Library/PlatformBootManagerLib/PlatformBm.c
> index d081fdae63..4bfa906921 100644
> --- a/Platform/RaspberryPi/Library/PlatformBootManagerLib/PlatformBm.c
> +++ b/Platform/RaspberryPi/Library/PlatformBootManagerLib/PlatformBm.c
> @@ -6,6 +6,7 @@
>   *  Copyright (c) 2015-2016, Red Hat, Inc.
>   *  Copyright (c) 2014-2021, ARM Ltd. All rights reserved.
>   *  Copyright (c) 2004-2016, Intel Corporation. All rights reserved.
> + *  Copyright (c) 2021, Semihalf All rights reserved.
>   *
>   *  SPDX-License-Identifier: BSD-2-Clause-Patent
>   *
> @@ -19,10 +20,12 @@
>  #include <Library/UefiBootManagerLib.h>  #include <Library/UefiLib.h>
> #include <Library/PrintLib.h>
> +#include <Protocol/BootManagerPolicy.h>
>  #include <Protocol/DevicePath.h>
>  #include <Protocol/EsrtManagement.h>
>  #include <Protocol/GraphicsOutput.h>
>  #include <Protocol/LoadedImage.h>
> +#include <Guid/BootDiscoveryPolicy.h>
>  #include <Guid/EventGroup.h>
>  #include <Guid/TtyTerm.h>
>  #include <ConfigVars.h>
> @@ -598,6 +601,88 @@ PlatformBootManagerBeforeConsole (
>    FilterAndProcess (&gEfiUsb2HcProtocolGuid, NULL, Connect);  }
> 
> +/**
> +  Connect device specified by BootDiscoverPolicy variable and refresh
> +  Boot order for newly discovered boot device.
> +
> +  @retval  EFI_SUCCESS  Devices connected succesfully or connection
> +                        not required.
> +  @retval  others       Return values from GetVariable(), LocateProtocol()
> +                        and ConnectDeviceClass().
> +--*/
> +STATIC
> +EFI_STATUS
> +BootDiscoveryPolicyHandler (
> +  VOID
> +  )
> +{
> +  EFI_STATUS                       Status;
> +  UINT32                           DiscoveryPolicy;
> +  UINTN                            Size;
> +  EFI_BOOT_MANAGER_POLICY_PROTOCOL *BMPolicy;
> +  EFI_GUID                         *Class;
> +
> +  Size = sizeof (DiscoveryPolicy);
> +  Status = gRT->GetVariable (
> +                  BOOT_DISCOVERY_POLICY_VAR,
> +                  &gBootDiscoveryPolicyMgrFormsetGuid,
> +                  NULL,
> +                  &Size,
> +                  &DiscoveryPolicy
> +                  );
> +  if (Status == EFI_NOT_FOUND) {
> +    Status = PcdSet32S (PcdBootDiscoveryPolicy, PcdGet32
> (PcdBootDiscoveryPolicy));

Why need this Pcd statement?

> +    if (Status == EFI_NOT_FOUND) {
> +      return EFI_SUCCESS;
> +    } else if (EFI_ERROR (Status)) {
> +      return Status;
> +    }
> +  } else if (EFI_ERROR (Status)) {
> +    return Status;
> +  }
> +
> +  if (DiscoveryPolicy == BDP_CONNECT_MINIMAL) {

If the Status is EFI_NOT_FOUND at GetVariable, then the above DiscoveryPolicy is uninitialized.
Maybe you need to use the Pcd value if GetVariable failed.

Thanks,
Zhichao

> +    return EFI_SUCCESS;
> +  }
> +
> +  switch (DiscoveryPolicy) {
> +    case BDP_CONNECT_NET:
> +      Class = &gEfiBootManagerPolicyNetworkGuid;
> +      break;
> +    case BDP_CONNECT_ALL:
> +      Class = &gEfiBootManagerPolicyConnectAllGuid;
> +      break;
> +    default:
> +      DEBUG ((
> +        DEBUG_INFO,
> +        "%a - Unexpected DiscoveryPolicy (0x%x). Run Minimal Discovery
> Policy\n",
> +        __FUNCTION__,
> +        DiscoveryPolicy
> +        ));
> +      return EFI_SUCCESS;
> +  }
> +
> +  Status = gBS->LocateProtocol (
> +                  &gEfiBootManagerPolicyProtocolGuid,
> +                  NULL,
> +                  (VOID **)&BMPolicy
> +                  );
> +  if (EFI_ERROR (Status)) {
> +    DEBUG ((DEBUG_ERROR, "%a - Failed to locate
> gEfiBootManagerPolicyProtocolGuid - %r\n", __FUNCTION__, Status));
> +    return Status;
> +  }
> +
> +  Status = BMPolicy->ConnectDeviceClass (BMPolicy, Class);  if
> + (EFI_ERROR (Status)){
> +    DEBUG ((DEBUG_ERROR, "%a - ConnectDeviceClass returns - %r\n",
> __FUNCTION__, Status));
> +    return Status;
> +  }
> +
> +  EfiBootManagerRefreshAllBootOption();
> +
> +  return EFI_SUCCESS;
> +}
> +
>  /**
>    Do the platform specific action after the console is ready
>    Possible things that can be done in PlatformBootManagerAfterConsole:
> @@ -644,6 +729,11 @@ PlatformBootManagerAfterConsole (
>      DEBUG ((DEBUG_INFO, "Boot Policy is Fast Boot. Skip connecting all
> devices\n"));
>    }
> 
> +  Status = BootDiscoveryPolicyHandler ();  if (EFI_ERROR(Status)) {
> +    DEBUG ((DEBUG_INFO, "Error applying Boot Discovery Policy:%r\n",
> + Status));  }
> +
>    Status = gBS->LocateProtocol (&gEsrtManagementProtocolGuid, NULL,
> (VOID**)&EsrtManagement);
>    if (!EFI_ERROR (Status)) {
>      EsrtManagement->SyncEsrtFmp ();
> --
> 2.25.1
> 
> 
> 
> 
>