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:ler...@redhat.com] > Sent: Thursday, August 29, 2019 8:37 PM > To: Dong, Eric <eric.d...@intel.com>; devel@edk2.groups.io > Cc: Ni, Ray <ray...@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...@intel.com> > > Cc: Laszlo Ersek <ler...@redhat.com> > > Signed-off-by: Eric Dong <eric.d...@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 <ler...@redhat.com> > > Thanks > Laszlo -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#46572): https://edk2.groups.io/g/devel/message/46572 Mute This Topic: https://groups.io/mt/33054478/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-