UefiCpuPkg/SecCore: get AllSecPpiList after SecPlatformMain.

[edk2-devel] [Patch] UefiCpuPkg/SecCore: get AllSecPpiList after SecPlatformMain.

Posted by Dong, Eric 4 years, 8 months ago

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
     //
-- 
2.21.0.windows.1


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#46529): https://edk2.groups.io/g/devel/message/46529
Mute This Topic: https://groups.io/mt/33054478/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

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

Posted by Laszlo Ersek 4 years, 7 months ago

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

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#46567): https://edk2.groups.io/g/devel/message/46567
Mute This Topic: https://groups.io/mt/33054478/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

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

Posted by Dong, Eric 4 years, 7 months ago

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

-=-=-=-=-=-=-=-=-=-=-=-
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/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-