Re: [PATCH v4 0/5] Enhanced SMM support for AMD-based x86 systems.

[Marvin H?user <Marvin.Haeuser@outlook.com>]

Hey Jiewen,
Hey Leo,

May I suggest replacing "StandardSignatureIsAuthenticAMD()" and the PCDs introduced by the series with a Fixed "PcdCpuVendor" enum or alike?
The contra of "StandardSignatureIsAuthenticAMD()" is that it's a runtime action. From my point of view, this has no notable advantage as boards use either an Intel or an AMD chipset and hence the EDK2 Firmware Package has compilation-time knowledge of the target platform.
On the other hand, the PCDs introduced by this patch cause the contra that the platform DSC must set the correct vendor-specific (intel, AMD) value.
If the code checked for the CPU Vendor PCD and used the correct values based on that, the values for the other vendors would get optimized away (no unnecessary runtime actions) and the platform package owner does not need to worry about setting PCDs to AMD-specific values except for the CPU Vendor PCD.
Backwards-compatibility could be ensured by defaulting to some reserved value for the PCD and handling detection via StandardSignatureIsAuthenticAMD() if the platform DSC does not change it.

Thanks,
Marvin.

> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> Yao, Jiewen
> Sent: Thursday, October 5, 2017 2:49 AM
> To: Leo Duran <leo.duran@amd.com>; edk2-devel@lists.01.org
> Subject: Re: [edk2] [PATCH v4 0/5] Enhanced SMM support for AMD-based
> x86 systems.
> 
> Hi Leo
> I do not suggest we introduce PcdCpuSmmSmramSaveStateMapOffset.
> 
> This is unnecessary, because it is CPU attribute but not some end user
> configurable data.
> 
> I think we can use CPUID to distinguish AMD from INTEL. Is that technically
> possible?
> 
> I found we already have code at
> 
> C:\home\EdkIIGit\edk2\UefiCpuPkg\Library\BaseXApicX2ApicLib\BaseXApic
> X2ApicLib.c(1206):    if (StandardSignatureIsAuthenticAMD()) {
> 
> C:\home\EdkIIGit\edk2\UefiCpuPkg\Library\BaseXApicLib\BaseXApicLib.c(11
> 11):    if (StandardSignatureIsAuthenticAMD()) {
> 
> 
> 
> Thank you
> Yao Jiewen
> 
> 
> > -----Original Message-----
> > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> > Leo Duran
> > Sent: Thursday, October 5, 2017 3:02 AM
> > To: edk2-devel@lists.01.org
> > Subject: [edk2] [PATCH v4 0/5] Enhanced SMM support for AMD-based x86
> > systems.
> >
> > This patch-set introduces a couple of FixedPCDs to replace
> > Intel-specific macros, and better support AMD-based x86 systems.
> >
> > 1) PcdCpuSmmSmramSaveStateMapOffset - SMRAM Save State Map
> Offset.
> > 2) PcdCpuSmmPSDOffset - Processor SMM Descriptor Offset in SMRAM.
> >
> > Changes since v3:
> > Correction on cover letter.
> >
> > Changes since v2:
> > The intent of this revision is to maintain compatibility with existing
> > packages. To that end, changes to OvmgfPkg and QuarkSocPkg are
> reverted.
> > Moreover, pertinent macros are replaced in the C code, rather than on
> > header files that are shared globally.
> >
> > Changes since v1:
> > Revision to Cc list for UefiCpuPkg.
> >
> > Leo Duran (5):
> >   UefiCpuPkg/UefiCpuPkg.dec: Create FixedPCDs for SMM support
> >   UefiCpuPkg/PiSmmCpuDxeSmm: Consume FixedPCDs to enhance SMM
> support
> >   UefiCpuPkg/PiSmmCpuDxeSmm: Use FixedPCDs to enhance SMM
> support
> >   UefiCpuPkg/SmmCpuFeaturesLib: Consume FixedPCD to enhance SMM
> > support
> >   UefiCpuPkg/SmmCpuFeaturesLib: Use FixedPCD on non-STM library
> >
> >  UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c      |  4
> > +++-
> >  UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf    |  5
> > +++++
> >  UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLibStm.inf |  5
> > +++++
> >  UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/Semaphore.c                    |  4
> > +++-
> >  UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.S                     |  4
> > +++-
> >  UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.asm                   |  4
> > +++-
> >  UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.nasm                  |  4
> > +++-
> >  UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c                    |
> > 10 +++++-----
> >  UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h                    |
> > 2 --
> >  UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf                  |
> > 4 ++++
> >  UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
> > |  4 +++-
> >  UefiCpuPkg/PiSmmCpuDxeSmm/SmramSaveState.c                    |  4
> > +++-
> >  UefiCpuPkg/PiSmmCpuDxeSmm/X64/Semaphore.c                     |
> > 4 +++-
> >  UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.S                      |  4
> > +++-
> >  UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.asm                    |  4
> > +++-
> >  UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.nasm                   |  4
> > +++-
> >  UefiCpuPkg/UefiCpuPkg.dec                                     |  9
> > +++++++++
> >  17 files changed, 61 insertions(+), 18 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