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

[Laszlo Ersek <lersek@redhat.com>]

On 06/27/17 00:36, Jordan Justen wrote:
> 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.

OK.

> 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?

If I remember correctly, I wanted to base SmramAccessGetCapabilities()
directly on hardware configuration. At that point (regardless of whether
the function would be called via PEI_SMM_ACCESS_PPI or
EFI_SMM_ACCESS2_PROTOCOL), the ESMRAMC.TSEG_SZ bits used as source for
the calculation would already be locked, by the SmmAccessPei entry point
function.

I don't have a clear threat model in my mind, but I feel unsafe basing
SmramAccessGetCapabilities() on a dynamic PcdGet() call. Dynamic PCDs
can be changed well into BDS (for example by a UEFI app or driver
launched / loaded from the UEFI shell prompt), without breaking any
rules. I don't think EFI_SMM_ACCESS2_PROTOCOL.GetCapabilities() should
return a different output based on that. I just feel unsafe about adding
a functional change like this to the series.

Most drivers that I can recall from under MdeModulePkg that consume
dynamic PCDs tend to save the values to global variables in their entry
points.

Thanks
Laszlo