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

["Gerd Hoffmann" <kraxel@redhat.com>]

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.

> (2) We already assume (minimally since 2015) that MTRRs are supported by
> the processor.

No.  The whole MTRR setup is gated by "if (IsMtrrSupported ())".

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

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

take care,
  Gerd



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