Re: [edk2-devel] [PATCH v2] IntelFsp2Pkg: add TempRamExitPpi.h.

["Nate DeSimone" <nathaniel.l.desimone@intel.com>]

Hi All,

The decision to use FSP_TEMP_RAM_EXIT_PPI for both FSP and non-FSP builds is a platform level decision. There are several EDK2 based UEFI implementations that do not use FSP_TEMP_RAM_EXIT_PPI, OvmfPkg, Minnow, and Quark are some of many examples. There is nothing in the UEFI PI spec or the MinPlatform spec mandating that this PPI be implemented. This PPI is however mandated by the FSP spec. As a matter of convenience, if a platform implements the FSP spec, it is easiest to also implement this PPI even on a non-FSP build. This PPI was added to the FSP spec because our prior experience has shown that such a PPI makes it easier to implement platform agnostic SEC phase code.

Furthermore, IntelFsp2Pkg may not have any dependencies on IntelSiliconPkg, it is only allowed to depend on MdePkg since FSP is an industry standard.

Accordingly, I agree with Chasel that two copies of this PPI are currently merited:

1. IntelFsp2Pkg
2. IntelSiliconPkg

I agree with Chasel that depending on ecosystem adoption of FSP we can consider dropping the duplicate from IntelSiliconPkg in the future.

Thanks,
Nate

-----Original Message-----
From: Chiu, Chasel 
Sent: Wednesday, June 19, 2019 8:33 PM
To: Ni, Ray <ray.ni@intel.com>; devel@edk2.groups.io
Cc: Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>; Zeng, Star <star.zeng@intel.com>
Subject: RE: [edk2-devel] [PATCH v2] IntelFsp2Pkg: add TempRamExitPpi.h.


Hi Ray,

Currently we prefer to duplicate header files so we do not have IntelFsp2Pkg dependency for non-FSP build.
We will review for how to support FSP/non-FSP builds better.

Thanks!
Chasel


> -----Original Message-----
> From: Ni, Ray
> Sent: Monday, June 17, 2019 11:27 AM
> To: devel@edk2.groups.io; Chiu, Chasel <chasel.chiu@intel.com>
> Cc: Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>; Zeng, Star 
> <star.zeng@intel.com>
> Subject: RE: [edk2-devel] [PATCH v2] IntelFsp2Pkg: add TempRamExitPpi.h.
> 
> Chasel,
> I found another instance of this PPI in 
> edk2-platforms/Silicon/Intel/KabylakeSiliconPkg/Include/Ppi.
> Will you remove that one after this is checked in?
> 
> 
> Thanks,
> Ray
> 
> > -----Original Message-----
> > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Chiu, 
> > Chasel
> > Sent: Monday, June 17, 2019 10:42 AM
> > To: devel@edk2.groups.io
> > Cc: Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>; Zeng, 
> > Star <star.zeng@intel.com>
> > Subject: [edk2-devel] [PATCH v2] IntelFsp2Pkg: add TempRamExitPpi.h.
> >
> > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1883
> >
> > Add header file for FSP_TEMP_RAM_EXIT_PPI which is defined by FSP 
> > 2.1 spec.
> >
> > Test: Build successfully.
> >
> > Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
> > Cc: Star Zeng <star.zeng@intel.com>
> > Signed-off-by: Chasel Chiu <chasel.chiu@intel.com>
> > ---
> >  IntelFsp2Pkg/Include/Ppi/TempRamExitPpi.h | 52
> > ++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  IntelFsp2Pkg/IntelFsp2Pkg.dec             |  5 +++++
> >  2 files changed, 57 insertions(+)
> >
> > diff --git a/IntelFsp2Pkg/Include/Ppi/TempRamExitPpi.h
> > b/IntelFsp2Pkg/Include/Ppi/TempRamExitPpi.h
> > new file mode 100644
> > index 0000000000..0db54dfa45
> > --- /dev/null
> > +++ b/IntelFsp2Pkg/Include/Ppi/TempRamExitPpi.h
> > @@ -0,0 +1,52 @@
> > +/** @file
> > +  This file defines the Silicon Temp Ram Exit PPI which implements 
> > +the
> > +  required programming steps for disabling temporary memory.
> > +
> > +Copyright (c) 2019, Intel Corporation. All rights reserved.<BR>
> > +SPDX-License-Identifier: BSD-2-Clause-Patent
> > +
> > +**/
> > +
> > +#ifndef _FSP_TEMP_RAM_EXIT_PPI_H_
> > +#define _FSP_TEMP_RAM_EXIT_PPI_H_
> > +
> > +///
> > +/// Global ID for the FSP_TEMP_RAM_EXIT_PPI.
> > +///
> > +#define FSP_TEMP_RAM_EXIT_GUID \
> > +  { \
> > +    0xbc1cfbdb, 0x7e50, 0x42be, { 0xb4, 0x87, 0x22, 0xe0, 0xa9, 
> > +0x0c, 0xb0, 0x52 } \
> > +  }
> > +
> > +//
> > +// Forward declaration for the FSP_TEMP_RAM_EXIT_PPI.
> > +//
> > +typedef struct _FSP_TEMP_RAM_EXIT_PPI FSP_TEMP_RAM_EXIT_PPI;
> > +
> > +/**
> > +  Silicon function for disabling temporary memory.
> > +  @param[in] TempRamExitParamPtr - Pointer to the TempRamExit
> > parameters structure.
> > +                                   This structure is normally 
> > + defined in the
> Integration
> > +                                   Guide. If it is not defined in the Integration Guide,
> > +                                   pass NULL.
> > +  @retval EFI_SUCCESS            - FSP execution environment was initialized
> > successfully.
> > +  @retval EFI_INVALID_PARAMETER  - Input parameters are invalid.
> > +  @retval EFI_UNSUPPORTED        - The FSP calling conditions were not
> met.
> > +  @retval EFI_DEVICE_ERROR       - Temporary memory exit.
> > +**/
> > +typedef
> > +EFI_STATUS
> > +(EFIAPI *FSP_TEMP_RAM_EXIT) (
> > +  IN  VOID    *TempRamExitParamPtr
> > +  );
> > +
> > +///
> > +/// This PPI provides function to disable temporary memory.
> > +///
> > +struct _FSP_TEMP_RAM_EXIT_PPI {
> > +  FSP_TEMP_RAM_EXIT   TempRamExit;
> > +};
> > +
> > +extern EFI_GUID gFspTempRamExitPpiGuid;
> > +
> > +#endif // _FSP_TEMP_RAM_EXIT_PPI_H_
> > diff --git a/IntelFsp2Pkg/IntelFsp2Pkg.dec 
> > b/IntelFsp2Pkg/IntelFsp2Pkg.dec index cc17164742..ad2b7f7fb5 100644
> > --- a/IntelFsp2Pkg/IntelFsp2Pkg.dec
> > +++ b/IntelFsp2Pkg/IntelFsp2Pkg.dec
> > @@ -49,6 +49,11 @@
> >    #
> >    gFspInApiModePpiGuid                  = { 0xa1eeab87, 0xc859, 0x479d,
> {0x89,
> > 0xb5, 0x14, 0x61, 0xf4, 0x06, 0x1a, 0x3e}}
> >
> > +  #
> > +  # PPI to tear down the temporary memory set up by TempRamInit ().
> > +  #
> > +  gFspTempRamExitPpiGuid      = {0xbc1cfbdb, 0x7e50, 0x42be, {0xb4, 0x87,
> > 0x22, 0xe0, 0xa9, 0x0c, 0xb0, 0x52}}
> > +
> >  [Guids]
> >    #
> >    # GUID defined in package
> > --
> > 2.13.3.windows.1
> >
> >
> >