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

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

On 12 June 2018 at 14:25, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> 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.

It has been substantially modified.

> Anyway:
> Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
>

Thanks.

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