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

["Dong, Eric" <eric.dong@intel.com>]

Hi Laszlo,

I based on current code logic to adjust the code position. 
I agree it's a good enhancement for the commit message. I will add it when I push the change.  Also I will push the change after the code freeze done.

Thanks,
Eric
> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Thursday, August 29, 2019 8:37 PM
> To: Dong, Eric <eric.dong@intel.com>; devel@edk2.groups.io
> Cc: Ni, Ray <ray.ni@intel.com>
> Subject: Re: [Patch] UefiCpuPkg/SecCore: get AllSecPpiList after
> SecPlatformMain.
> 
> 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