Re: [edk2-platforms PATCH v2] ArmPkg: Enable boot discovery policy for ARM package.

["Sami Mujawar" <sami.mujawar@arm.com>]

[-- Attachment #1: Type: text/plain, Size: 7725 bytes --]

Hi Grzegorz,

Thank you for this patch. I have a few minor suggestions, otherwise this 
patch looks good to me.

I have tested this patch on FVP and Juno and both platforms work fine.

Reviewed-by: Sami Mujawar <sami.mujawar@arm.com>

Regards,

Sami Mujawar


On 18/08/2021 08:43 PM, Samer El-Haj-Mahmoud wrote:
> + Sami for ArmPkg review
>
>> -----Original Message-----
>> From: Grzegorz Bernacki <gjb@semihalf.com>
>> Sent: Monday, August 16, 2021 6:09 AM
>> To: devel@edk2.groups.io
>> Cc: leif@nuviainc.com; ardb+tianocore@kernel.org; Samer El-Haj-Mahmoud
>> <Samer.El-Haj-Mahmoud@arm.com>; Sunny Wang
>> <Sunny.Wang@arm.com>; mw@semihalf.com; upstream@semihalf.com;
>> Grzegorz Bernacki <gjb@semihalf.com>
>> Subject: [edk2-platforms PATCH v2] ArmPkg: Enable boot discovery policy for
>> ARM package.
>>
>> This commit adds code which check BootDiscoveryPolicy variable and
>> calls Boot Policy Manager Protocol to connect device specified by
>> the variable. To enable that mechanism for platform
>> EfiMdeModulePkgTokenSpaceGuid.PcdBootDiscoveryPolicy PCD must be
>> added to DSC file and BootDiscoveryPolicyUiLib should be added to
>> UiApp libraries.
>>
>> Signed-off-by: Grzegorz Bernacki <gjb@semihalf.com>
>> ---
>>   ArmPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf |  5
>> +
>>   ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c               | 96
>> +++++++++++++++++++-
>>   2 files changed, 100 insertions(+), 1 deletion(-)
>>
>> diff --git
>> a/ArmPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf
>> b/ArmPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf
>> index 353d7a967b..86751b45f8 100644
>> --- a/ArmPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf
>> +++
>> b/ArmPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf
>> @@ -65,11 +65,15 @@
>>
>>   [Pcd]
>>     gEfiMdePkgTokenSpaceGuid.PcdPlatformBootTimeOut
>> +  gEfiMdeModulePkgTokenSpaceGuid.PcdBootDiscoveryPolicy
>>
>>   [Guids]
>> +  gBootDiscoveryPolicyMgrFormsetGuid
>>     gEdkiiNonDiscoverableEhciDeviceGuid
>>     gEdkiiNonDiscoverableUhciDeviceGuid
>>     gEdkiiNonDiscoverableXhciDeviceGuid
>> +  gEfiBootManagerPolicyNetworkGuid
>> +  gEfiBootManagerPolicyConnectAllGuid
>>     gEfiFileInfoGuid
>>     gEfiFileSystemInfoGuid
>>     gEfiFileSystemVolumeLabelInfoIdGuid
>> @@ -79,6 +83,7 @@
>>
>>   [Protocols]
>>     gEdkiiNonDiscoverableDeviceProtocolGuid
>> +  gEfiBootManagerPolicyProtocolGuid
>>     gEfiDevicePathProtocolGuid
>>     gEfiGraphicsOutputProtocolGuid
>>     gEfiLoadedImageProtocolGuid
>> diff --git a/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c
>> b/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c
>> index 5ceb23d822..ef5c323743 100644
>> --- a/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c
>> +++ b/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c
>> @@ -2,9 +2,10 @@
>>     Implementation for PlatformBootManagerLib library class interfaces.
>>
>>     Copyright (C) 2015-2016, Red Hat, Inc.
>> -  Copyright (c) 2014 - 2019, ARM Ltd. All rights reserved.<BR>
>> +  Copyright (c) 2014 - 2021, ARM Ltd. All rights reserved.<BR>
>>     Copyright (c) 2004 - 2018, Intel Corporation. All rights reserved.<BR>
>>     Copyright (c) 2016, Linaro Ltd. All rights reserved.<BR>
>> +  Copyright (c) 2021, Semihalf All rights reserved.<BR>
>>
>>     SPDX-License-Identifier: BSD-2-Clause-Patent
>>
>> @@ -19,6 +20,7 @@
>>   #include <Library/UefiBootManagerLib.h>
>>   #include <Library/UefiLib.h>
>>   #include <Library/UefiRuntimeServicesTableLib.h>
>> +#include <Protocol/BootManagerPolicy.h>
>>   #include <Protocol/DevicePath.h>
>>   #include <Protocol/EsrtManagement.h>
>>   #include <Protocol/GraphicsOutput.h>
>> @@ -27,6 +29,7 @@
>>   #include <Protocol/PciIo.h>
>>   #include <Protocol/PciRootBridgeIo.h>
>>   #include <Protocol/PlatformBootManager.h>
>> +#include <Guid/BootDiscoveryPolicy.h>
>>   #include <Guid/EventGroup.h>
>>   #include <Guid/NonDiscoverableDevice.h>
>>   #include <Guid/TtyTerm.h>
>> @@ -703,6 +706,91 @@ HandleCapsules (
>>
>>   #define VERSION_STRING_PREFIX    L"Tianocore/EDK2 firmware version "
>>
>> +/**
>> +  This functions checks the value of BootDiscoverPolicy variable and
>> +  connect devices of class specified by that variable. Then it refreshes
>> +  Boot order for newly discovered boot device.
>> +
>> +  @retval  EFI_SUCCESS  Devices connected successfully 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));
>> +    if (Status == EFI_NOT_FOUND) {
>> +      return EFI_SUCCESS;
>> +    } else if (EFI_ERROR (Status)) {
>> +      return Status;
>> +    }
>> +    DiscoveryPolicy = PcdGet32 (PcdBootDiscoveryPolicy);
[SAMI] Can 'DiscoveryPolicy' be initialised a few lines above and used 
in the call to PcdSet32S ()?
>> +  } else if (EFI_ERROR (Status)) {
>> +    return Status;
>> +  }
>> +
>> +  if (DiscoveryPolicy == BDP_CONNECT_MINIMAL) {
>> +    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_INFO, "%a - Failed to locate
>> gEfiBootManagerPolicyProtocolGuid."
>> +      "Driver connect will be skipped.\n", __FUNCTION__));
>> +    return Status;
>> +  }
>> +
>> +  Status = BMPolicy->ConnectDeviceClass (BMPolicy, Class);
>> +  if (EFI_ERROR (Status)){
>> +    DEBUG ((DEBUG_ERROR, "%a - ConnectDeviceClass returns - %r\n",
>> __FUNCTION__, Status));
>> +    return Status;
>> +  }
>> +
>> +  EfiBootManagerRefreshAllBootOption();
[SAMI] Please add a space after the function name and the opening 
parenthesis.
>> +
>> +  return EFI_SUCCESS;
>> +}
>> +
>>   /**
>>     Do the platform specific action after the console is ready
>>     Possible things that can be done in PlatformBootManagerAfterConsole:
>> @@ -753,6 +841,12 @@ PlatformBootManagerAfterConsole (
>>       }
>>     }
>>
>> +  //
>> +  // Connect device specified by BootDiscoverPolicy variable and
>> +  // refresh Boot order for newly discovered boot devices
>> +  //
>> +  BootDiscoveryPolicyHandler ();
>> +
>>     //
>>     // On ARM, there is currently no reason to use the phased capsule
>>     // update approach where some capsules are dispatched before EndOfDxe
>> --
>> 2.25.1


[-- Attachment #2: Type: text/html, Size: 9545 bytes --]