Re: [PATCH v3 3/4] ArmPkg/PlatformBootManagerLib: call ProcessCapsules() only once

[Leif Lindholm <leif.lindholm@linaro.org>]

On Tue, Jun 12, 2018 at 01:23:28PM +0200, Ard Biesheuvel wrote:
> ARM platforms have no restriction on when a system firmware update
> capsule can be applied, and so it is not necessary to call
> ProcessCapsules() twice. So let's drop the first invocation that
> occurs before EndOfDxe, and rewrite the second call so that all
> capsule updates will be applied when the console is up and able to
> provide progress feedback.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

I thought I acked this one last time around? Perhaps you wanted it
again after the discussion. Anyway:
Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>

> ---
>  ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c               | 87 ++++++++++++++------
>  ArmPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf |  1 +
>  2 files changed, 61 insertions(+), 27 deletions(-)
> 
> diff --git a/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c b/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c
> index 3456a71fbb9c..7c21cce5960b 100644
> --- a/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c
> +++ b/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c
> @@ -24,6 +24,7 @@
>  #include <Library/PcdLib.h>
>  #include <Library/UefiBootManagerLib.h>
>  #include <Library/UefiLib.h>
> +#include <Library/UefiRuntimeServicesTableLib.h>
>  #include <Protocol/DevicePath.h>
>  #include <Protocol/EsrtManagement.h>
>  #include <Protocol/GraphicsOutput.h>
> @@ -553,21 +554,6 @@ PlatformBootManagerBeforeConsole (
>    VOID
>    )
>  {
> -  EFI_STATUS                    Status;
> -  ESRT_MANAGEMENT_PROTOCOL      *EsrtManagement;
> -
> -  if (GetBootModeHob() == BOOT_ON_FLASH_UPDATE) {
> -    DEBUG ((DEBUG_INFO, "ProcessCapsules Before EndOfDxe ......\n"));
> -    Status = ProcessCapsules ();
> -    DEBUG ((DEBUG_INFO, "ProcessCapsules returned %r\n", Status));
> -  } else {
> -    Status = gBS->LocateProtocol (&gEsrtManagementProtocolGuid, NULL,
> -                    (VOID **)&EsrtManagement);
> -    if (!EFI_ERROR (Status)) {
> -      EsrtManagement->SyncEsrtFmp ();
> -    }
> -  }
> -
>    //
>    // Signal EndOfDxe PI Event
>    //
> @@ -618,6 +604,57 @@ PlatformBootManagerBeforeConsole (
>    PlatformRegisterOptionsAndKeys ();
>  }
>  
> +STATIC
> +VOID
> +HandleCapsules (
> +  VOID
> +  )
> +{
> +  ESRT_MANAGEMENT_PROTOCOL    *EsrtManagement;
> +  EFI_PEI_HOB_POINTERS        HobPointer;
> +  EFI_CAPSULE_HEADER          *CapsuleHeader;
> +  BOOLEAN                     NeedReset;
> +  EFI_STATUS                  Status;
> +
> +  DEBUG ((DEBUG_INFO, "%a: processing capsules ...\n", __FUNCTION__));
> +
> +  Status = gBS->LocateProtocol (&gEsrtManagementProtocolGuid, NULL,
> +                  (VOID **)&EsrtManagement);
> +  if (!EFI_ERROR (Status)) {
> +    EsrtManagement->SyncEsrtFmp ();
> +  }
> +
> +  //
> +  // Find all capsule images from hob
> +  //
> +  HobPointer.Raw = GetHobList ();
> +  NeedReset = FALSE;
> +  while ((HobPointer.Raw = GetNextHob (EFI_HOB_TYPE_UEFI_CAPSULE,
> +                             HobPointer.Raw)) != NULL) {
> +    CapsuleHeader = (VOID *)(UINTN)HobPointer.Capsule->BaseAddress;
> +
> +    Status = ProcessCapsuleImage (CapsuleHeader);
> +    if (EFI_ERROR (Status)) {
> +      DEBUG ((DEBUG_ERROR, "%a: failed to process capsule %p - %r\n",
> +        __FUNCTION__, CapsuleHeader, Status));
> +      return;
> +    }
> +    if ((CapsuleHeader->Flags & CAPSULE_FLAGS_INITIATE_RESET) != 0) {
> +      NeedReset = TRUE;
> +    }
> +    HobPointer.Raw = GET_NEXT_HOB (HobPointer);
> +  }
> +
> +  if (NeedReset) {
> +      DEBUG ((DEBUG_WARN, "%a: capsule update successful, resetting ...\n",
> +        __FUNCTION__));
> +
> +      gRT->ResetSystem(EfiResetCold, EFI_SUCCESS, 0, NULL);
> +      CpuDeadLoop();
> +  }
> +}
> +
> +
>  #define VERSION_STRING_PREFIX    L"Tianocore/EDK2 firmware version "
>  
>  /**
> @@ -637,7 +674,6 @@ PlatformBootManagerAfterConsole (
>    VOID
>    )
>  {
> -  ESRT_MANAGEMENT_PROTOCOL      *EsrtManagement;
>    EFI_STATUS                    Status;
>    EFI_GRAPHICS_OUTPUT_PROTOCOL  *GraphicsOutput;
>    UINTN                         FirmwareVerLength;
> @@ -675,17 +711,14 @@ PlatformBootManagerAfterConsole (
>    //
>    EfiBootManagerConnectAll ();
>  
> -  Status = gBS->LocateProtocol (&gEsrtManagementProtocolGuid, NULL,
> -                  (VOID **)&EsrtManagement);
> -  if (!EFI_ERROR (Status)) {
> -    EsrtManagement->SyncEsrtFmp ();
> -  }
> -
> -  if (GetBootModeHob() == BOOT_ON_FLASH_UPDATE) {
> -    DEBUG((DEBUG_INFO, "ProcessCapsules After EndOfDxe ......\n"));
> -    Status = ProcessCapsules ();
> -    DEBUG((DEBUG_INFO, "ProcessCapsules returned %r\n", Status));
> -  }
> +  //
> +  // On ARM, there is currently no reason to use the phased capsule
> +  // update approach where some capsules are dispatched before EndOfDxe
> +  // and some are dispatched after. So just handle all capsules here,
> +  // when the console is up and we can actually give the user some
> +  // feedback about what is going on.
> +  //
> +  HandleCapsules ();
>  
>    //
>    // Enumerate all possible boot options.
> diff --git a/ArmPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf b/ArmPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf
> index e8cbb10dabdd..28d606d5c329 100644
> --- a/ArmPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf
> +++ b/ArmPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf
> @@ -55,6 +55,7 @@ [LibraryClasses]
>    UefiBootManagerLib
>    UefiBootServicesTableLib
>    UefiLib
> +  UefiRuntimeServicesTableLib
>  
>  [FeaturePcd]
>    gEfiMdePkgTokenSpaceGuid.PcdUgaConsumeSupport
> -- 
> 2.17.1
>