Re: [Patch] UefiCpuPkg/SecCore: get AllSecPpiList after SecPlatformMain.

["Laszlo Ersek" <lersek@redhat.com>]

Hi Eric,

On 08/28/19 08:50, Eric Dong wrote:
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2136
> 
> SecPlatformMain is a platform hook function which let platform does
> some update. Some platform may adjust SecCoreData->PeiTemporaryRamBase
> which caused former saved AllSecPpiList variable invalid.
> 
> This patch update the logic to get AllSecPpiList after SecPlatformMain.
> 
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Signed-off-by: Eric Dong <eric.dong@intel.com>
> ---
>  UefiCpuPkg/SecCore/SecMain.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/UefiCpuPkg/SecCore/SecMain.c b/UefiCpuPkg/SecCore/SecMain.c
> index f914446257..66c952b897 100644
> --- a/UefiCpuPkg/SecCore/SecMain.c
> +++ b/UefiCpuPkg/SecCore/SecMain.c
> @@ -228,7 +228,6 @@ SecStartupPhase2(
>  
>    PeiCoreEntryPoint = NULL;
>    SecCoreData   = (EFI_SEC_PEI_HAND_OFF *) Context;
> -  AllSecPpiList = (EFI_PEI_PPI_DESCRIPTOR *) SecCoreData->PeiTemporaryRamBase;
>  
>    //
>    // Perform platform specific initialization before entering PeiCore.
> @@ -282,6 +281,8 @@ SecStartupPhase2(
>    }
>  
>    if (PpiList != NULL) {
> +    AllSecPpiList = (EFI_PEI_PPI_DESCRIPTOR *) SecCoreData->PeiTemporaryRamBase;
> +
>      //
>      // Remove the terminal flag from the terminal PPI
>      //
> 

Based on the SecPlatformMain() documentation in
"UefiCpuPkg/Include/Library/PlatformSecLib.h", it seems valid for a
platform to change "SecCoreData->PeiTemporaryRamBase".

Therefore, in SecStartupPhase2(), it appears justified to assign
"AllSecPpiList" only after calling SecPlatformMain().


This patch does something else too: it makes the assignment to
"AllSecPpiList" conditional. I agree that is justified, as well. Namely:

[*] If SecPlatformMain() returns no platform-specific PPI list, then
there is nothing to merge, so we don't need "AllSecPpiList" at all.

I think however that this change should be documented explicitly.


When pushing, please add the sentence [*] to the commit message. With that:

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Thanks
Laszlo