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

[Laszlo Ersek <lersek@redhat.com>]

On 06/26/17 19:59, Jordan Justen wrote:
> On 2017-06-19 12:39:51, Laszlo Ersek wrote:
>>
>> 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.
> 
> The library is the part that seems odd to me. A library just to deal
> with Q35 tseg seems a bit specific.

Maybe so, but that's only because edk2 requires all libraries (even
one-off utility ones) that are used by at least two modules to have
full-blown library classes. (NULL class library resolution is different,
but those in turn require constructors.) This makes the overhead for
small utility libraries worse.

Specifically, the first four patches in the series can be considered a
refactoring / improvement per se, because they centralize the
TSEG_SZ<->megabytes mapping to one module.

Extracting the mapping is also useful because the last patch can then
add the feature to all affected modules at once. If we proceed
piecemeal, then there's going to be a stage where the PEI phase modules
know how to handle extended TSEG, but the DXE phase doesn't. Not tragic,
but not optimal for bisection.

> I looked at this series a number of times. My thought is that:
> 
> * PcdQ35TsegMbytes should become dynamic
> 
> * No PCD for tseg mask
> 
> * PlatformPei adds knowledge of qemu ext tseg to set PcdQ35TsegMbytes
> 
> * SmmAccessPei adds dep on gEfiPeiMemoryDiscoveredPpiGuid to make sure
>   PcdQ35TsegMbytes is set.

Arbitrary and quite ugly, but I guess I can live with it.

> * SmmAccessPei adds some duplicated code to know about qemu ext tseg
>   for mask. (But, I think a reasonably small amount of dup code.)
> 
> * SmramInternal.c just reads the size from the PCD

This last bullet is a functional change for SmramAccessGetCapabilities()
that I wanted to avoid -- I sought to preserve the logic of working off
of the currently set (and possibly locked) contents of ESMRAMC.TSEG_SZ.

I guess the idea is still doable if SmramInternal.c introduces a global
variable for caching the PCD at module startup -- setting the variable
from the PCD would be the responsibility of each separate module, both
PEIM and DXE driver --, and then SmramAccessGetCapabilities() could use
the global variable if it read "11" binary from the TSEG_SZ bit-field
register.

This would be in effect the same as the current patch series, except the
bit-field<->MB mappings would remain scattered over the tree.

Please confirm if you are OK with the above and I'll spin a v2.

Thanks
Laszlo

> 
> Now, as usual, I expect you'll point out some trivially obvious issue
> that I missed. :)
> 
> -Jordan
>