Re: [PATCH 2/2] ArmPlatformPkg/PlatformPeim: allow PlatformPeiLib to set the boot mode

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

On Wed, Nov 01, 2017 at 01:11:45PM +0000, Ard Biesheuvel wrote:
> The current interdepencies between the PrePeiCore SEC module, the
> platform PEIM and ArmPlatformLib is a bit awkward: due to the fact
> that ArmPlatformLib is also used by SEC modules, we cannot use PEI
> specific facilities in the implementation of ArmPlatformGetBootMode.
> However, given that we call that library function /after/ invoking
> PlatformPeiLib, there is no way for that library to set the boot mode
> other than resorting to tricks like notification callbacks on arbitrary
> unrelated events.
> 
> ArmPlatformLib should probably be phased out anyway, given its quirky
> nature,

Yes, it should.

> but for now, let's fix this particular issue by deferring the
> call to PlatformPeim() to after the point where we set the boot mode
> by calling ArmPlatformGetBootMode ().
> 
> While we're at it, clean up the code slightly by using PeiServicesLib
> instead of doing double pointer dereferencing.
>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  ArmPlatformPkg/PlatformPei/PlatformPeim.c   | 12 +++++++-----
>  ArmPlatformPkg/PlatformPei/PlatformPeim.inf |  1 +
>  2 files changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/ArmPlatformPkg/PlatformPei/PlatformPeim.c b/ArmPlatformPkg/PlatformPei/PlatformPeim.c
> index e4535250c245..14f301e947a8 100644
> --- a/ArmPlatformPkg/PlatformPei/PlatformPeim.c
> +++ b/ArmPlatformPkg/PlatformPei/PlatformPeim.c
> @@ -83,21 +83,23 @@ InitializePlatformPeim (
>    )
>  {
>    EFI_STATUS                    Status;
> -  UINTN                         BootMode;
> +  EFI_BOOT_MODE                 BootMode;
>  
>    DEBUG ((EFI_D_LOAD | EFI_D_INFO, "Platform PEIM Loaded\n"));
>  
> +  Status = PeiServicesSetBootMode (ArmPlatformGetBootMode ());
> +  ASSERT_EFI_ERROR (Status);
> +
>    PlatformPeim ();
>  
> -  BootMode  = ArmPlatformGetBootMode ();
> -  Status    = (**PeiServices).SetBootMode (PeiServices, (UINT8) BootMode);
> +  Status = PeiServicesGetBootMode (&BootMode);
>    ASSERT_EFI_ERROR (Status);
>  
> -  Status = (**PeiServices).InstallPpi (PeiServices, &mPpiListBootMode);
> +  Status = PeiServicesInstallPpi (&mPpiListBootMode);
>    ASSERT_EFI_ERROR (Status);
>  
>    if (BootMode == BOOT_IN_RECOVERY_MODE) {
> -    Status = (**PeiServices).InstallPpi (PeiServices, &mPpiListRecoveryBootMode);
> +    Status = PeiServicesInstallPpi (&mPpiListRecoveryBootMode);
>      ASSERT_EFI_ERROR (Status);
>    }
>  
> diff --git a/ArmPlatformPkg/PlatformPei/PlatformPeim.inf b/ArmPlatformPkg/PlatformPei/PlatformPeim.inf
> index f466c1412ad3..21701cdc0731 100644
> --- a/ArmPlatformPkg/PlatformPei/PlatformPeim.inf
> +++ b/ArmPlatformPkg/PlatformPei/PlatformPeim.inf
> @@ -43,6 +43,7 @@ [LibraryClasses]
>    HobLib
>    ArmPlatformLib
>    PlatformPeiLib
> +  PeiServicesLib

If you move that one up one line:
Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>

>  
>  [Ppis]
>    gEfiPeiMasterBootModePpiGuid                  # PPI ALWAYS_PRODUCED
> -- 
> 2.11.0
>