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

[Laszlo Ersek <lersek@redhat.com>]

On 10/04/17 03:22, Kinney, Michael D wrote:
> 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.

In addition to the above, I'd like to request a bit more "telling"
subject lines for the patches. Using the OVMF patch as example,

  OvmfPkg: SmmCpuFeaturesLib library.

is not really helpful; I'd consider the following an improvement:

  OvmfPkg/SmmCpuFeaturesLib: consume SMRAM Save State Map Offset PCD

66 characters -- we generally limit subjects to 74.

(Of course I realize the patch might entirely be replaced in the next
version, based on Jiewen's and Mike's feedback -- that's OK with me, I
just wanted to give an example.)

Thanks!
Laszlo