Re: [PATCH edk2-platforms v4 27/34] Silicon/SynQuacerMemoryInitPeiLib: ignore capsules when clearing NVRAM

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

On Fri, Nov 10, 2017 at 02:21:20PM +0000, Ard Biesheuvel wrote:
> In preparation of adding support for setting a DIP switch to clear the
> EFI variable store, update the early capsule handling logic to take the
> boot mode into account.
> 
> This is necessary for two reasons:
> - we override the boot mode when a capsule is detected,
> - the capsule detection itself involves reading a EFI variable, which we
>   shouldn't be doing if the varstore may be in a bad state.
> 
> So factor out the initial capsule check (to keep the code understandable)
> and only perform it if we are not booting in 'clear NVRAM' mode.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

Minor suggestion below, fold in if you like it. Regardless:
Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>

> ---
>  Silicon/Socionext/SynQuacer/Library/SynQuacerMemoryInitPeiLib/SynQuacerMemoryInitPeiLib.c | 68 +++++++++++++-------
>  1 file changed, 45 insertions(+), 23 deletions(-)
> 
> diff --git a/Silicon/Socionext/SynQuacer/Library/SynQuacerMemoryInitPeiLib/SynQuacerMemoryInitPeiLib.c b/Silicon/Socionext/SynQuacer/Library/SynQuacerMemoryInitPeiLib/SynQuacerMemoryInitPeiLib.c
> index b44c58d61062..63c441872da7 100644
> --- a/Silicon/Socionext/SynQuacer/Library/SynQuacerMemoryInitPeiLib/SynQuacerMemoryInitPeiLib.c
> +++ b/Silicon/Socionext/SynQuacer/Library/SynQuacerMemoryInitPeiLib/SynQuacerMemoryInitPeiLib.c
> @@ -170,6 +170,44 @@ DeclareDram (
>    return EFI_SUCCESS;
>  }
>  
> +STATIC
> +BOOLEAN
> +CheckCapsule (
> +  IN  EFI_PEI_SERVICES              **PeiServices,
> +  IN  PEI_CAPSULE_PPI               *Capsule,
> +  IN  EFI_PHYSICAL_ADDRESS          UefiMemoryBase,
> +  OUT VOID                          **CapsuleBuffer,
> +  OUT UINTN                         *CapsuleBufferLength
> +  )
> +{
> +  EFI_STATUS        Status;
> +
> +  Status = Capsule->CheckCapsuleUpdate (PeiServices);
> +  if (!EFI_ERROR (Status)) {
> +
> +    //
> +    // Coalesce the capsule into unused memory. CreateState() below will copy
> +    // it to a properly allocated buffer.
> +    //
> +    *CapsuleBuffer = (VOID *)PcdGet64 (PcdSystemMemoryBase);
> +    *CapsuleBufferLength = UefiMemoryBase - PcdGet64 (PcdSystemMemoryBase);
> +
> +    PeiServicesSetBootMode (BOOT_ON_FLASH_UPDATE);
> +
> +    Status = Capsule->Coalesce (PeiServices, CapsuleBuffer,
> +                        CapsuleBufferLength);
> +    if (!EFI_ERROR (Status)) {
> +      DEBUG ((DEBUG_INFO, "%a: Coalesced capsule @ %p (0x%lx)\n",
> +        __FUNCTION__, *CapsuleBuffer, *CapsuleBufferLength));
> +      return TRUE;
> +    } else {
> +      DEBUG ((DEBUG_WARN, "%a: failed to coalesce() capsule (Status == %r)\n",
> +        __FUNCTION__, Status));
> +    }
> +  }
> +  return FALSE;
> +}
> +
>  EFI_STATUS
>  EFIAPI
>  MemoryPeim (
> @@ -184,6 +222,7 @@ MemoryPeim (
>    VOID                          *CapsuleBuffer;
>    UINTN                         CapsuleBufferLength;
>    BOOLEAN                       HaveCapsule;
> +  EFI_BOOT_MODE                 BootMode;
>  
>    Status = DeclareDram (&VirtualMemoryTable);
>    ASSERT_EFI_ERROR (Status);
> @@ -199,31 +238,14 @@ MemoryPeim (
>    ASSERT_EFI_ERROR (Status);
>  
>    //
> -  // Check for persistent capsules
> +  // Check for persistent capsules, unless we are booting with default
> +  // settings.
>    //
>    HaveCapsule = FALSE;
> -  Status = Capsule->CheckCapsuleUpdate (PeiServices);
> -  if (!EFI_ERROR (Status)) {
> -
> -    //
> -    // Coalesce the capsule into unused memory. CreateState() below will copy
> -    // it to a properly allocated buffer.
> -    //
> -    CapsuleBuffer = (VOID *)PcdGet64 (PcdSystemMemoryBase);
> -    CapsuleBufferLength = UefiMemoryBase - PcdGet64 (PcdSystemMemoryBase);
> -
> -    PeiServicesSetBootMode (BOOT_ON_FLASH_UPDATE);
> -
> -    Status = Capsule->Coalesce (PeiServices, &CapsuleBuffer,
> -                           &CapsuleBufferLength);
> -    if (!EFI_ERROR (Status)) {
> -      DEBUG ((DEBUG_INFO, "%a: Coalesced capsule @ %p (0x%lx)\n",
> -        __FUNCTION__, CapsuleBuffer, CapsuleBufferLength));
> -      HaveCapsule = TRUE;
> -    } else {
> -      DEBUG ((DEBUG_WARN, "%a: failed to coalesce() capsule (Status == %r)\n",
> -        __FUNCTION__, Status));
> -    }
> +  Status = PeiServicesGetBootMode (&BootMode);
> +  if (!EFI_ERROR (Status) && BootMode != BOOT_WITH_DEFAULT_SETTINGS) {
> +    HaveCapsule = CheckCapsule (PeiServices, Capsule, UefiMemoryBase,
> +                    &CapsuleBuffer, &CapsuleBufferLength);

You could do
     } else {
       HaveCapsule = FALSE;
here instead of unconditionally before the test.

/
    Leif

>    }
>  
>    Status = ArmConfigureMmu (VirtualMemoryTable, NULL, NULL);
> -- 
> 2.11.0
>