Re: [PATCH 1/5] OvmfPkg: introduce Q35TsegSizeLib (class header and sole lib instance)

[Laszlo Ersek <lersek@redhat.com>]

On 06/19/17 19:30, Jordan Justen wrote:
> On 2017-06-08 10:13:29, Laszlo Ersek wrote:
>> OvmfPkg contains three modules that work with the TSEG (SMRAM) size:
>> PlatformPei (PEIM), SmmAccessPei (PEIM), and SmmAccess2Dxe (DXE_DRIVER).
>> These modules open-code the interpretation of the ESMRAMC register's
>> TSEG_SZ bit-field. That is OK as long as we stick with the Q35 hardware
>> spec and nothing more, but it makes it difficult to benefit from an
>> upcoming QEMU feature, namely extended TSEG sizes.
>>
>> Introduce the Q35TsegSizeLib class, and its sole lib instance, for
>> extracting / centralizing TSEG size querying and interpretation. This
>> library instance is self contained and does not consume dynamic PCDs (for
>> example, it doesn't consume PcdOvmfHostBridgePciDevId), because such PCDs
>> tend to be set in PlatformPei, but the dispatch order between PlatformPei
>> and SmmAccessPei is unspecified (both have TRUE for DEPEX).
> 
> I think that on 'real' platforms there would be an enforced ordering
> of Platform PEI before the PEI SMM drivers. I'm not sure what the
> mechanism is, and whether it is applicable to OVMF.
> 
> Jeff, Mike, Jiewen, Do you know? Do the chipset SMM drivers depend on
> gEfiPeiMemoryDiscoveredPpiGuid that is triggered by Platform PEI?

In OVMF's Ia32X64 build report file (with SMM enabled), the only PEIM
with a DEPEX on gEfiPeiMemoryDiscoveredPpiGuid is CpuMpPei.

The final DEPEX of SmmAccessPei is ((TRUE) AND (gEfiPeiPcdPpiGuid)); the
latter is inherited from the PcdLib instance that SmmAccessPei uses. (I
think.)

> Does
> gEfiPeiMemoryDiscoveredPpiGuid also install on S3 resume?

Yes. That PPI GUID is produced as a consequence of our call to
PublishSystemMemory(), which we (must) do on the S3 resume path as well
(using a pre-reserved area); see PublishPeiMemory() in "MemDetect.c".

And, we actively use CpuMpPei to set the Feature Control MSR on all
processors, on both normal boot and S3 resume. (See "FeatureControl.c".)

> 
> Laszlo, It sounded like you'd be open to using a PCD, but the PEI
> driver ordering issue prevented it. Is that right?

* If we can turn the following two variables into dynamic PCDs:

- mPreferredEsmramcTsegSzMask (UINT8)
- mExtendedTsegMbytes (UINT16)

such that PlatformPei sets them "early enough" on both normal boot and
S3 resume, incorporating the logic of the Q35TsegSizeGetPreferences()
function, then the library instance itself can drop
Q35TsegSizeGetPreferences(), and the rest of the library functions can
be rebased to these PCDs.

* The library interfaces should stay as they are; they provide precisely
what the client code needs.


Even if the PCDs were technically doable, I think I'd slightly prefer
the current approach. The PCI config space accesses at module startup
are minimal, and they affect only three modules. OTOH, introducing two
PCDs feels, in this particular case, a bit like "polluting" OvmfPkg.dec.
(Well, "PcdPreferredEsmramcTsegSzMask" would *replace*
"PcdQ35TsegMbytes", so it would mean the addition of one PCD.)

Anyway, this is just a slight preference. What I feel much more strongly
about are the library APIs.

Thanks,
Laszlo