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

[Jordan Justen <jordan.l.justen@intel.com>]

On 2017-06-26 12:12:45, Laszlo Ersek wrote:
> On 06/26/17 19:59, Jordan Justen wrote:
> > 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.

I'm assuming you mean that the gEfiPeiMemoryDiscoveredPpiGuid
dependency is arbitrary and quite ugly.

I guess it doesn't seem arbitrary and ugly to me, since on most
firmware you'd need to detect memory before considering anything SMM
related. We should probably still minimize memory usage until Platform
PEI 'installs' it, even though no configuration is required.

> > * 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.

Why can't SmramInternal.c read the size from the PCD directly?

-Jordan