Re: [PATCH v2 3/3] UefiCpuPkg/SecCore: Support EFI_PEI_CORE_FV_LOCATION_PPI

["Chiu, Chasel" <chasel.chiu@intel.com>]

> -----Original Message-----
> From: Ni, Ray
> Sent: Thursday, February 14, 2019 4:25 PM
> To: Chiu, Chasel <chasel.chiu@intel.com>; edk2-devel@lists.01.org
> Cc: Dong, Eric <eric.dong@intel.com>; Laszlo Ersek <lersek@redhat.com>
> Subject: RE: [PATCH v2 3/3] UefiCpuPkg/SecCore: Support
> EFI_PEI_CORE_FV_LOCATION_PPI
> 
> 
> 
> > -----Original Message-----
> > From: Chiu, Chasel <chasel.chiu@intel.com>
> > Sent: Wednesday, February 13, 2019 5:47 PM
> > To: edk2-devel@lists.01.org
> > Cc: Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>;
> > Laszlo Ersek <lersek@redhat.com>; Chiu, Chasel <chasel.chiu@intel.com>
> > Subject: [PATCH v2 3/3] UefiCpuPkg/SecCore: Support
> > EFI_PEI_CORE_FV_LOCATION_PPI
> >
> > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1524
> >
> > EFI_PEI_CORE_FV_LOCATION_PPI may be passed by platform when PeiCore
> > not in BFV so SecCore has to search PeiCore either from the FV
> > location provided by EFI_PEI_CORE_FV_LOCATION_PPI or from BFV.
> >
> > Test: Verified on internal platform and booting successfully.
> >
> > Cc: Eric Dong <eric.dong@intel.com>
> > Cc: Ray Ni <ray.ni@intel.com>
> > Cc: Laszlo Ersek <lersek@redhat.com>
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Chasel Chiu <chasel.chiu@intel.com>
> > ---
> >  UefiCpuPkg/SecCore/SecMain.c   | 35
> > +++++++++++++++++++++++++++++------
> >  UefiCpuPkg/SecCore/SecCore.inf |  3 ++-
> >  UefiCpuPkg/SecCore/SecMain.h   |  3 ++-
> >  3 files changed, 33 insertions(+), 8 deletions(-)
> >
> > diff --git a/UefiCpuPkg/SecCore/SecMain.c
> > b/UefiCpuPkg/SecCore/SecMain.c index b24e190617..b99072599d 100644
> > --- a/UefiCpuPkg/SecCore/SecMain.c
> > +++ b/UefiCpuPkg/SecCore/SecMain.c
> > @@ -1,7 +1,7 @@
> >  /** @file
> >    C functions in SEC
> >
> > -  Copyright (c) 2008 - 2018, Intel Corporation. All rights
> > reserved.<BR>
> > +  Copyright (c) 2008 - 2019, Intel Corporation. All rights
> > + reserved.<BR>
> >    This program and the accompanying materials
> >    are licensed and made available under the terms and conditions of
> > the BSD License
> >    which accompanies this distribution.  The full text of the license
> > may be found at @@ -228,26 +228,49 @@ SecStartupPhase2(  {
> >    EFI_SEC_PEI_HAND_OFF        *SecCoreData;
> >    EFI_PEI_PPI_DESCRIPTOR      *PpiList;
> > +  EFI_PEI_PPI_DESCRIPTOR      *PpiListTmp;
> 
> 
> 1. Maybe this local variable is not needed. We can use PpiList[Index].
Agree! I will correct this.

> 
> >    UINT32                      Index;
> >    EFI_PEI_PPI_DESCRIPTOR      *AllSecPpiList;
> >    EFI_PEI_CORE_ENTRY_POINT    PeiCoreEntryPoint;
> >
> > +  PeiCoreEntryPoint = NULL;
> >    SecCoreData   = (EFI_SEC_PEI_HAND_OFF *) Context;
> >    AllSecPpiList = (EFI_PEI_PPI_DESCRIPTOR *) SecCoreData-
> > >PeiTemporaryRamBase;
> > +
> >    //
> >    // Find Pei Core entry point. It will report SEC and Pei Core debug
> > information if remote debug
> >    // is enabled.
> >    //
> > -  FindAndReportEntryPoints ((EFI_FIRMWARE_VOLUME_HEADER *)
> > SecCoreData->BootFirmwareVolumeBase, &PeiCoreEntryPoint);
> > -  if (PeiCoreEntryPoint == NULL)
> > -  {
> > -    CpuDeadLoop ();
> > +  PpiList = SecPlatformMain (SecCoreData);  PpiListTmp = PpiList;
> > + for
> > + (;;) {
> 
> 2. Similar comments as above. Maybe we can just use PpiList[Index] in the loop.
> By the way, original code logic checks PpiList against NULL.
> Do we still need to make sure to deference PpiList after checking against NULL?
Good catch! I will update.

> 
> > +    if (CompareGuid (PpiListTmp->Guid, &gEfiPeiCoreFvLocationPpiGuid)
> > + &&
> > (((EFI_PEI_CORE_FV_LOCATION_PPI *) PpiListTmp->Ppi)-
> > >PeiCoreFvLocation != 0)) {
> > +      FindAndReportEntryPoints ((EFI_FIRMWARE_VOLUME_HEADER *)
> > ((EFI_PEI_CORE_FV_LOCATION_PPI *) PpiListTmp->Ppi)->PeiCoreFvLocation,
> > &PeiCoreEntryPoint);
> > +      if (PeiCoreEntryPoint != NULL) {
> > +        break;
> 
> 3. Is it valid that PeiCore cannot be found in the PeiCoreFvLocation?
> If no, can we just dead-loop here when PeiCoreEntryPoint is NULL?
Yes. I will add dead-loop.

> 
> > +      }
> > +    }
> > +    if ((PpiListTmp->Flags & EFI_PEI_PPI_DESCRIPTOR_TERMINATE_LIST)
> > + ==
> > EFI_PEI_PPI_DESCRIPTOR_TERMINATE_LIST) {
> > +      //
> > +      // Continue until the end of the PPI List.
> > +      //
> > +      break;
> > +    }
> > +    PpiListTmp++;
> > +  }
> > +  //
> > +  // If EFI_PEI_CORE_FV_LOCATION_PPI not found or no PeiCore found by
> > the pointer in provided PPI, try to locate PeiCore from BFV.
> > +  //
> > +  if (PeiCoreEntryPoint == NULL) {
> > +    FindAndReportEntryPoints ((EFI_FIRMWARE_VOLUME_HEADER *)
> > SecCoreData->BootFirmwareVolumeBase, &PeiCoreEntryPoint);
> > +    if (PeiCoreEntryPoint == NULL) {
> > +      CpuDeadLoop ();
> > +    }
> >    }
> >
> >    //
> >    // Perform platform specific initialization before entering PeiCore.
> >    //
> > -  PpiList = SecPlatformMain (SecCoreData);
> >    if (PpiList != NULL) {
> >      //
> >      // Remove the terminal flag from the terminal PPI diff --git
> > a/UefiCpuPkg/SecCore/SecCore.inf b/UefiCpuPkg/SecCore/SecCore.inf
> > index 442f663911..fc1485b5cb 100644
> > --- a/UefiCpuPkg/SecCore/SecCore.inf
> > +++ b/UefiCpuPkg/SecCore/SecCore.inf
> > @@ -7,7 +7,7 @@
> >  #  protected mode, setup flat memory model, enable temporary memory
> > and  #  call into SecStartup().
> >  #
> > -#  Copyright (c) 2006 - 2018, Intel Corporation. All rights
> > reserved.<BR>
> > +#  Copyright (c) 2006 - 2019, Intel Corporation. All rights
> > +reserved.<BR>
> >  #  This program and the accompanying materials  #  are licensed and
> > made available under the terms and conditions of the BSD License  #
> > which accompanies this distribution.  The full text of the license may
> > be found at @@ -73,6 +73,7 @@
> >    ## NOTIFY
> >    ## SOMETIMES_CONSUMES
> >    gPeiSecPerformancePpiGuid
> > +  gEfiPeiCoreFvLocationPpiGuid
> >
> >  [Guids]
> >    ## SOMETIMES_PRODUCES   ## HOB
> > diff --git a/UefiCpuPkg/SecCore/SecMain.h
> > b/UefiCpuPkg/SecCore/SecMain.h index 83244af119..80045d34f4 100644
> > --- a/UefiCpuPkg/SecCore/SecMain.h
> > +++ b/UefiCpuPkg/SecCore/SecMain.h
> > @@ -1,7 +1,7 @@
> >  /** @file
> >    Master header file for SecCore.
> >
> > -  Copyright (c) 2008 - 2018, Intel Corporation. All rights
> > reserved.<BR>
> > +  Copyright (c) 2008 - 2019, Intel Corporation. All rights
> > + reserved.<BR>
> >    This program and the accompanying materials
> >    are licensed and made available under the terms and conditions of
> > the BSD License
> >    which accompanies this distribution.  The full text of the license
> > may be found at @@ -20,6 +20,7 @@  #include
> > <Ppi/SecPlatformInformation2.h> #include <Ppi/TemporaryRamDone.h>
> > #include <Ppi/SecPerformance.h>
> > +#include <Ppi/PeiCoreFvLocation.h>
> >
> >  #include <Guid/FirmwarePerformance.h>
> >
> > --
> > 2.13.3.windows.1