Re: [edk2-devel] [PATCH 1/1] OvmfPkg/Sec: Setup MTRR early in the boot process.

["Laszlo Ersek" <lersek@redhat.com>]

On 1/25/24 07:52, Gerd Hoffmann wrote:
> On Wed, Jan 24, 2024 at 05:14:10PM +0100, Laszlo Ersek wrote:
>> On 1/24/24 16:15, Gerd Hoffmann wrote:
>>> Specifically before running lzma uncompress of the main firmware volume.
>>> This is needed to make sure caching is enabled, otherwise the uncompress
>>> can be extremely slow.
>>>
>>> Adapt the ASSERTs in PlatformInitLib to the changes.
>>>
>>> Background:  In some virtual machine configurations with assigned
>>> devices kvm uses EPT memory types to apply guest MTRR settings.
>>> In case MTRRs are disabled kvm will use the uncachable memory type
>>> for all mappings.
>>
>> I suggest mentioning mdev and noncoherent DMA here (the linux code you
>> quoted elsewhere would be welcome too).
>
> Ok, I guess it makes sense to quote it completely in the
> commit message then ...
>
> static u8 vmx_get_mt_mask(struct kvm_vcpu *vcpu, gfn_t gfn, bool is_mmio)
> {
> 	/* We wanted to honor guest CD/MTRR/PAT, but doing so could result in
> 	 * memory aliases with conflicting memory types and sometimes MCEs.
> 	 * We have to be careful as to what are honored and when.
> 	 *
> 	 * For MMIO, guest CD/MTRR are ignored.  The EPT memory type is set to
> 	 * UC.  The effective memory type is UC or WC depending on guest PAT.
> 	 * This was historically the source of MCEs and we want to be
> 	 * conservative.
> 	 *
> 	 * When there is no need to deal with noncoherent DMA (e.g., no VT-d
> 	 * or VT-d has snoop control), guest CD/MTRR/PAT are all ignored.  The
> 	 * EPT memory type is set to WB.  The effective memory type is forced
> 	 * WB.
> 	 *
> 	 * Otherwise, we trust guest.  Guest CD/MTRR/PAT are all honored.  The
> 	 * EPT memory type is used to emulate guest CD/MTRR.
> 	 */
>
> 	if (is_mmio)
> 		return MTRR_TYPE_UNCACHABLE << VMX_EPT_MT_EPTE_SHIFT;
>
> 	if (!kvm_arch_has_noncoherent_dma(vcpu->kvm))
> 		return (MTRR_TYPE_WRBACK << VMX_EPT_MT_EPTE_SHIFT) | VMX_EPT_IPAT_BIT;
>
> 	if (kvm_read_cr0_bits(vcpu, X86_CR0_CD)) {
> 		if (kvm_check_has_quirk(vcpu->kvm, KVM_X86_QUIRK_CD_NW_CLEARED))
> 			return MTRR_TYPE_WRBACK << VMX_EPT_MT_EPTE_SHIFT;
> 		else
> 			return (MTRR_TYPE_UNCACHABLE << VMX_EPT_MT_EPTE_SHIFT) |
> 				VMX_EPT_IPAT_BIT;
> 	}
>
> 	return kvm_mtrr_get_guest_memory_type(vcpu, gfn) << VMX_EPT_MT_EPTE_SHIFT;
> }
>
>> The original report <https://edk2.groups.io/g/devel/message/113517>
>> claims that commit e8aa4c6546ad ("UefiCpuPkg/ResetVector: Cache
>> Disable should not be set by default in CR0", 2023-08-30) triggered
>> the symptom.
>
>> I don't understand how *clearing* CD in CR0, could make guest memory
>> *less* cacheable. I understand the point about MTRR, but exactly the
>> MTRR's currently upstream state should have masked any changes to CD.
>
> ... because it also explains that question:  I think with CD set we
> take the KVM_X86_QUIRK_CD_NW_CLEARED code path and run with memory in
> write-back mode.

Ah. Technically, this is a good answer -- it's simply what the host
does.

However, that just points at some inconsistency in the kernel-side code.
The comment that applies is the last paragraph: "Otherwise, we trust
guest.  Guest CD/MTRR/PAT are all honored.  [...]". The comment does not
spell out "*except* when the KVM_X86_QUIRK_CD_NW_CLEARED quirk is
enabled, which *is* enabled, by default".

(Compare "Documentation/virt/kvm/api.rst":

 KVM_X86_QUIRK_CD_NW_CLEARED        By default, KVM clears CR0.CD and CR0.NW.
                                    When this quirk is disabled, KVM does not
                                    change the value of CR0.CD and CR0.NW.
)

All in all, this has the weird effect that guest-side CD behaves in the
inverse of the expected (intuitive) behavior. With CD set in the guest,
the host considers the quirk (= default on), and so the decision is
write-back. With CD clear in the guest, the host ignores the quirk (!),
and relies on the guest-side MTRR -- which happens to dictate
"uncached".

This seems broken. A strict guest side global setting combined with the
quirk leads to looser behavior than a looser guest-side setting (where
the quirk is not considered at all).

Anyway, thank you for explaining, and please do include the whole
snippet in the commit message. The behavior is arbitrary and only the
code snippet explains the symptom.

>
>> (2) We already assume (minimally since 2015) that MTRRs are supported
>> by the processor.
>
> No.  The whole MTRR setup is gated by "if (IsMtrrSupported ())".

(Please don't snip too much context out of my emails; now I need to go
back to my earlier message, for the context.)

... You are correct, I missed the IsMtrrSupported() call in 79d274b8b6b1
("OvmfPkg: PlatformPei: invert MTRR setup in QemuInitializeRam()",
2015-06-26). Sorry!

So yes, I figure the CPUID check in SecMtrrSetup() is necessary indeed.

However, within the IsMtrrSupported() block in QemuInitializeRam(), we
should still not permit

  (MtrrSettings.MtrrDefType & 0xFF) == 0x0

and we should still do

  ASSERT ((MtrrSettings.MtrrDefType & BIT11) != 0);
  ASSERT ((MtrrSettings.MtrrDefType & BIT10) == 0);
  ASSERT ((MtrrSettings.MtrrDefType & 0xFF) == 6);
  ...
  MtrrSettings.MtrrDefType |= BIT10;

That's because, if we reach this block, then SecMtrrSetup() will have
enabled MTRR in general (BIT11) and set the deftype to 6.

Do I understand right?

> Also note that qemu allows to turn off MTRRs (-cpu host,mtrr=off),
> which btw can be used as workaround for this bug.  With MTRR support
> disabled kvm takes yet another code path ...

OK.

>
>>>    InitializeApicTimer (0, MAX_UINT32, TRUE, 5);
>>>    DisableApicTimerInterrupt ();
>>>
>>> +  //
>>> +  // Initialize MTRR
>>> +  //
>>> +  SecMtrrSetup ();
>
>> (7) If you have a particular reason for doing it here, please capture
>> that in the comment.
>
> Placing it near other hardware init (timers) looked somewhat logical
> to me.  Any other place in that function should work equally well
> though.
>
>> (8) Just for symmetry -- I'm noticing there are two
>> SecCoreStartupWithStack() functions; the other being in
>> "OvmfPkg/IntelTdx/Sec/SecMain.c".
>>
>> Also, Min's original QEMU command line included TDVF references.
>>
>> Does that mean this patch should be reflected to the TDX platform's
>> modules?
>
> Probably, I'll have a look.

Thanks
Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#114459): https://edk2.groups.io/g/devel/message/114459
Mute This Topic: https://groups.io/mt/103933443/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-