Re: [PATCH v2 0/9] Enhanced SMM support to AMD-based x86 systems.

["Kinney, Michael D" <michael.d.kinney@intel.com>]

Leo,

The general design issue here is that a PCD and PCD macro were
added to a UefiCpuPkg include file Include/Register/SmramSaveStateMap.h.
This means any library or module that includes this package include
file will break the build until the PCD is added to the INF file for
that library or module.

The INF file for a module is supposed to express the PCDs that the
library or module sources access.  We do not expect #include
statements from package include files to require adding additional
PCDs.

Updating the code for these values to be detected or configurable
is a good idea.

One option is to update the C code that currently uses the #define
names to use the PCD access function directly, instead of adding
the PcdGetxxx() to the Include/Register/SmramSaveStateMap.h file.

Jiewen's idea to remove the PCD and detect the offset from CPUID
also looks like a reasonable approach.

Thanks,

Mike

> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On
> Behalf Of Yao, Jiewen
> Sent: Tuesday, October 3, 2017 5:50 PM
> To: Leo Duran <leo.duran@amd.com>; edk2-devel@lists.01.org
> Subject: Re: [edk2] [PATCH v2 0/9] Enhanced SMM support to AMD-
> based x86 systems.
> 
> Thanks Leo.
> I agree we should support AMD-based X86, since the most code is
> sharable.
> 
> [1] Comment for PcdCpuSmmSmramSaveStateMapOffset.
> 
> I am a little concerned about the patch 6/9, because it is an
> incompatible change.
> This change will cause all existing intel platform code change,
> to add PCD in INF, such as OvmfPkg (patch 4/9) and QuarkPkg
> (patch 5/9).
> 
> I am thinking an compatible way, to prevent existing intel
> platform code change.
> 
> 1) We can define below according to Intel SDM and AMD SDM.
> 
> #define INTEL_SMRAM_SAVE_STATE_MAP_OFFSET  0xfc00
> #define AMD_SMRAM_SAVE_STATE_MAP_OFFSET  0xfe00
> // This is to provide compatibility.
> #define SMRAM_SAVE_STATE_MAP_OFFSET
> INTEL_SMRAM_SAVE_STATE_MAP_OFFSET
> 
> 2) Because the system has capability to *detect* the CPU, there
> is no need to let user to *configure*.
> I do not suggest we use PCD. IMHO, it is more a system
> attribute, instead of a user configurable data. For example, a
> user cannot configure it to 0xFE00 for Intel CPU, or 0xFC00 for
> AMD CPU.
> 
> 3) I think we can have a CPUID check in the entrypoint, and
> patch the OFFSET at anywhere.
> 
> 
> [2] Comment for PcdCpuSmmPSDOffset.
> I do not mind, since it is driver internal configuration and it
> won't break the compatibility.
> 
> Thank you
> Yao Jiewen
> 
> 
> > -----Original Message-----
> > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On
> Behalf Of Leo
> > Duran
> > Sent: Wednesday, October 4, 2017 6:38 AM
> > To: edk2-devel@lists.01.org
> > Subject: [edk2] [PATCH v2 0/9] Enhanced SMM support to AMD-
> based x86
> > systems.
> >
> > UefiCpuPkg:
> > This patch-set introduces a couple of FixedPCDs to replace
> > Intel-specific macros, for SMM support on AMD-based x86
> systems.
> >
> > 1) PcdCpuSmmSmramSaveStateMapOffset - SMRAM Save State Map
> Offset.
> > 2) PcdCpuSmmPSDOffset - Processor SMM Descriptor Offset in
> SMRAM.
> >
> > OvmfPkg and QuarkSocPkg:
> > The PcdCpuSmmSmramSaveStateMapOffset PCD is declared just to
> resolve
> > the macro replaced by the shared Library/SmmCpuFeaturesLib.h
> file.
> >
> > Changes since v1:
> > Revision to Cc list for UefiCpuPkg.
> >
> > Leo Duran (9):
> >   UefiCpuPkg: UefiCpuPkg.dec
> >   UefiCpuPkg: PiSmmCpuDxeSmm driver.
> >   UefiCpuPkg: SmmCpuFeaturesLib library.
> >   OvmfPkg: SmmCpuFeaturesLib library.
> >   QuarkSocPkg: SmmCpuFeaturesLib library.
> >   UefiCpuPkg: Register/SmramSaveStateMap.h
> >   UefiCpuPkg: PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> >   UefiCpuPkg: PiSmmCpuDxeSmm driver.
> >   UefiCpuPkg: SmmCpuFeaturesLib library.
> >
> >  OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf
> | 5
> > +++++
> >  .../Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf
> | 4
> > ++++
> >  UefiCpuPkg/Include/Register/SmramSaveStateMap.h
> | 4
> > +++-
> >  UefiCpuPkg/Library/SmmCpuFeaturesLib/Ia32/SmiEntry.S
> | 4
> > +++-
> >  UefiCpuPkg/Library/SmmCpuFeaturesLib/Ia32/SmiEntry.asm
> | 4
> > +++-
> >  UefiCpuPkg/Library/SmmCpuFeaturesLib/Ia32/SmiEntry.nasm
> | 4
> > +++-
> >  UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf
> | 5
> > +++++
> >  UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLibStm.inf
> | 6
> > ++++++
> >  UefiCpuPkg/Library/SmmCpuFeaturesLib/X64/SmiEntry.S
> | 4
> > +++-
> >  UefiCpuPkg/Library/SmmCpuFeaturesLib/X64/SmiEntry.asm
> | 4
> > +++-
> >  UefiCpuPkg/Library/SmmCpuFeaturesLib/X64/SmiEntry.nasm
> | 4
> > +++-
> >  UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.S
> | 4
> > +++-
> >  UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.asm
> |
> > 4 +++-
> >  UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.nasm
> |
> > 4 +++-
> >  UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> > | 2 +-
> >  UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf
> > | 4 ++++
> >  UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.S
> |
> > 4 +++-
> >  UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.asm
> |
> > 4 +++-
> >  UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.nasm
> |
> > 4 +++-
> >  UefiCpuPkg/UefiCpuPkg.dec
> | 9
> > +++++++++
> >  20 files changed, 73 insertions(+), 14 deletions(-)
> >
> > --
> > 2.7.4
> >
> > _______________________________________________
> > edk2-devel mailing list
> > edk2-devel@lists.01.org
> > https://lists.01.org/mailman/listinfo/edk2-devel
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel