Re: SMRAM sizes on large hosts

[Laszlo Ersek <lersek@redhat.com>]

On 05/04/17 00:33, Laszlo Ersek wrote:
> On 05/03/17 15:44, Gerd Hoffmann wrote:
>>   Hi,
>>
>>> I propose the following: add a new fw_cfg file which communicates how
>>> much memory (how many megabytes) the "11b" value in the tseg size
>>> register will configure.
>>
>> I'd prefer to keep fw_cfg out of the picture, and I think we can do it
>> without too much problems.
>>
>> We have a TSEGMB (tseg memory base) register.  qemu doesn't implement
>> it (but I think you can write to it and read back what you've
>> written).
> 
> OVMF already uses this register, for the exact purpose you mention.
> 
> (1) OVMF sets the register in SmmAccessPeiEntryPoint(), which is the
> entry point of the PEI-phase module that produces PEI_SMM_ACCESS_PPI.
> 
> (2) OVMF reads the register back in SmramAccessGetCapabilities(), which
> is built into two modules:
> 
> - the same PEIM as mentioned above, for producing
> PEI_SMM_ACCESS_PPI.GetCapabilities(),
> 
> - a similar-purpose DXE driver, for producing
> EFI_SMM_ACCESS2_PROTOCOL.GetCapabilities().
> 
> Those GetCapabilities() member functions basically return the SMRAM map
> to the caller.
> 
>> We could make qemu update that register in case "11b" is written to
>> the tseg size.  The firmware then can read TSEGMB and figure whenever
>> a large tseg is supported and if so how big it is.
> 
> Do you mean that QEMU would at once calculate the absolute base of TSEG,
> downwards from the top of low RAM, expressed in MB?
> 
> Excerpt from (1):
> 
>>   //
>>   // Set TSEG Memory Base.
>>   //
>>   PciWrite32 (DRAMC_REGISTER_Q35 (MCH_TSEGMB),
>>     (TopOfLowRamMb - FixedPcdGet8 (PcdQ35TsegMbytes)) << MCH_TSEGMB_MB_SHIFT);
>>
>>   //
>>   // Set TSEG size, and disable TSEG visibility outside of SMM. Note that the
>>   // T_EN bit has inverse meaning; when T_EN is set, then TSEG visibility is
>>   // *restricted* to SMM.
>>   //
>>   EsmramcVal &= ~(UINT32)MCH_ESMRAMC_TSEG_MASK;
>>   EsmramcVal |= FixedPcdGet8 (PcdQ35TsegMbytes) == 8 ? MCH_ESMRAMC_TSEG_8MB :
>>                 FixedPcdGet8 (PcdQ35TsegMbytes) == 2 ? MCH_ESMRAMC_TSEG_2MB :
>>                 MCH_ESMRAMC_TSEG_1MB;
>>   EsmramcVal |= MCH_ESMRAMC_T_EN;
>>   PciWrite8 (DRAMC_REGISTER_Q35 (MCH_ESMRAMC), EsmramcVal);
> 
> I guess I could update this to:
> - set MCH_TSEGMB like now, but also remember the written value
> - write 11b to MCH_ESMRAMC, and read back MCH_TSEGMB
> - if the read back value differs from the written one, use that value
> for tseg size going forward, plus write 11b | T_EN to MCH_ESMRAMC
> - otherwise, set MCH_ESMRAMC like now.
> 
> The problem with this is that I need the TSEG size in another module as
> well, namely PlatformPei. The dispatch order between PlatformPei and
> SmmAccessPei is unspecified (they have both TRUE for DEPEX). If
> PlatformPei gets dispatched second, I really wouldn't want to write to
> MCH_ESMRAMC again, just to query MCH_TSEGMB. (I couldn't just read
> MCH_TSEGMB because if PlatformPei were dispatched first, then MCH_TSEGMB
> would still be unset).
> 
> In other words, I wouldn't like to write a register just to receive the
> information; I need the information in two places whose relative
> ordering is unspecified, and one of those already writes the register in
> question, genuinely. I guess I could hack it around with another dynamic
> PCD, but that's kind of ugly.
> 
> If we invented a read-only, side-effect-free PCI config space register
> that gave me this value plain and simple (similarly to how a new fw_cfg
> file would do), that would be a lot cleaner for me. I think this would
> match your earlier alternative where you wrote "Alternatively we could
> add some qemu-specific register". That's my next preference after
> fw_cfg.

If we invent such a new register, it should be in a location that is
either read-only, or zeroed-on-reset, in current QEMU. Otherwise, new
firmware running on old QEMU could be misled by a guest OS that writes
to this register, and then either reboots or enters S3.

... With this in mind, I don't oppose "having to write somewhere to read
back the result", but then let's please make that write access as well
to the same new qemu-specific register, and not to MCH_ESMRAMC.

Thanks!
Laszlo