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

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

[Gerd Hoffmann]

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.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 OvmfPkg/Library/PlatformInitLib/MemDetect.c |  8 ++++--
 OvmfPkg/Sec/SecMain.c                       | 32 +++++++++++++++++++++
 2 files changed, 37 insertions(+), 3 deletions(-)

diff --git a/OvmfPkg/Library/PlatformInitLib/MemDetect.c b/OvmfPkg/Library/PlatformInitLib/MemDetect.c
index f042517bb64a..cb2ae0f3d79d 100644
--- a/OvmfPkg/Library/PlatformInitLib/MemDetect.c
+++ b/OvmfPkg/Library/PlatformInitLib/MemDetect.c
@@ -1082,11 +1082,13 @@ PlatformQemuInitializeRam (
     MtrrGetAllMtrrs (&MtrrSettings);
 
     //
-    // MTRRs disabled, fixed MTRRs disabled, default type is uncached
+    // Fixed MTRRs disabled, default type is uncached or write back (see SecMtrrSetup())
     //
-    ASSERT ((MtrrSettings.MtrrDefType & BIT11) == 0);
     ASSERT ((MtrrSettings.MtrrDefType & BIT10) == 0);
-    ASSERT ((MtrrSettings.MtrrDefType & 0xFF) == 0);
+    ASSERT (
+      (MtrrSettings.MtrrDefType & 0xFF) == 0x0 ||
+      (MtrrSettings.MtrrDefType & 0xFF) == 0x6
+      );
 
     //
     // flip default type to writeback
diff --git a/OvmfPkg/Sec/SecMain.c b/OvmfPkg/Sec/SecMain.c
index 31da5d0ace51..a672751b046a 100644
--- a/OvmfPkg/Sec/SecMain.c
+++ b/OvmfPkg/Sec/SecMain.c
@@ -30,6 +30,8 @@
 #include <Ppi/MpInitLibDep.h>
 #include <Library/TdxHelperLib.h>
 #include <Library/CcProbeLib.h>
+#include <Register/Intel/ArchitecturalMsr.h>
+#include <Register/Intel/Cpuid.h>
 #include "AmdSev.h"
 
 #define SEC_IDT_ENTRY_COUNT  34
@@ -744,6 +746,31 @@ FindAndReportEntryPoints (
   return;
 }
 
+//
+// Enable MTRR early, set default type to write back.
+// Needed to make sure caching is enabled,
+// without this lzma decompress can be very slow.
+//
+STATIC
+VOID
+SecMtrrSetup (
+  VOID
+  )
+{
+  CPUID_VERSION_INFO_EDX           Edx;
+  MSR_IA32_MTRR_DEF_TYPE_REGISTER  DefType;
+
+  AsmCpuid (CPUID_VERSION_INFO, NULL, NULL, NULL, &Edx.Uint32);
+  if (!Edx.Bits.MTRR) {
+    return;
+  }
+
+  DefType.Uint64    = 0;
+  DefType.Bits.Type = 6; /* write back */
+  DefType.Bits.E    = 1; /* enable */
+  AsmWriteMsr64 (MSR_IA32_MTRR_DEF_TYPE, DefType.Uint64);
+}
+
 VOID
 EFIAPI
 SecCoreStartupWithStack (
@@ -942,6 +969,11 @@ SecCoreStartupWithStack (
   InitializeApicTimer (0, MAX_UINT32, TRUE, 5);
   DisableApicTimerInterrupt ();
 
+  //
+  // Initialize MTRR
+  //
+  SecMtrrSetup ();
+
   //
   // Initialize Debug Agent to support source level debug in SEC/PEI phases before memory ready.
   //
-- 
2.43.0



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#114309): https://edk2.groups.io/g/devel/message/114309
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]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Laszlo Ersek]

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

> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  OvmfPkg/Library/PlatformInitLib/MemDetect.c |  8 ++++--
>  OvmfPkg/Sec/SecMain.c                       | 32 +++++++++++++++++++++
>  2 files changed, 37 insertions(+), 3 deletions(-)

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 still don't understand how that is possible. Assuming we have
noncoherent DMA due to mdev assignment, KVM will honor "Guest
CD/MTRR/PAT". Let's consider each in turn:

- CD (cache disable): set to zero in recent commit e8aa4c6546ad.

- MTRR: we set DefType to WB, in this patch, but not enabled upstream,
at present

- PAT: IIUC, we don't deal with that at all in edk2

- PCD: not set, minimally through edk2 commit 98f378a7be12
("OvmfPkg/ResetVector: enable caching in initial page tables", 2013-09-24)

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.

(1) Is the initial report wrong?

> 
> diff --git a/OvmfPkg/Library/PlatformInitLib/MemDetect.c b/OvmfPkg/Library/PlatformInitLib/MemDetect.c
> index f042517bb64a..cb2ae0f3d79d 100644
> --- a/OvmfPkg/Library/PlatformInitLib/MemDetect.c
> +++ b/OvmfPkg/Library/PlatformInitLib/MemDetect.c
> @@ -1082,11 +1082,13 @@ PlatformQemuInitializeRam (
>      MtrrGetAllMtrrs (&MtrrSettings);
>  
>      //
> -    // MTRRs disabled, fixed MTRRs disabled, default type is uncached
> +    // Fixed MTRRs disabled, default type is uncached or write back (see SecMtrrSetup())
>      //
> -    ASSERT ((MtrrSettings.MtrrDefType & BIT11) == 0);
>      ASSERT ((MtrrSettings.MtrrDefType & BIT10) == 0);
> -    ASSERT ((MtrrSettings.MtrrDefType & 0xFF) == 0);
> +    ASSERT (
> +      (MtrrSettings.MtrrDefType & 0xFF) == 0x0 ||
> +      (MtrrSettings.MtrrDefType & 0xFF) == 0x6
> +      );
>  
>      //
>      // flip default type to writeback

This code comes (originally) from commit 79d274b8b6b1 ("OvmfPkg:
PlatformPei: invert MTRR setup in QemuInitializeRam()", 2015-06-26).

The purpose(s) of that commit include "setting the default to writeback"
-- even in the context quoted above, we see the comment "flip default
type to writeback". A wider context is:

    //
    // flip default type to writeback
    //
    SetMem (&MtrrSettings.Fixed, sizeof MtrrSettings.Fixed, 0x06);
    ZeroMem (&MtrrSettings.Variables, sizeof MtrrSettings.Variables);
    MtrrSettings.MtrrDefType |= BIT11 | BIT10 | 6;
    MtrrSetAllMtrrs (&MtrrSettings);

So:

(2) We already assume (minimally since 2015) that MTRRs are supported by
the processor. This means that (a) the CPUID check in SecMtrrSetup() is
superfluous, and that (b) we need not / should not permit

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

in the ASSERT(), going forward.

(3) The rest of the code and *comments* here -- in
PlatformQemuInitializeRam() -- should be updated such that we only
enable the fixed MTRRs (BIT10) on top of what SEC does earlier (which
is: BIT11 (general enable) and DefType=6).

Something like:

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

  ...

  MtrrSettings.MtrrDefType |= BIT10;

> diff --git a/OvmfPkg/Sec/SecMain.c b/OvmfPkg/Sec/SecMain.c
> index 31da5d0ace51..a672751b046a 100644
> --- a/OvmfPkg/Sec/SecMain.c
> +++ b/OvmfPkg/Sec/SecMain.c
> @@ -30,6 +30,8 @@
>  #include <Ppi/MpInitLibDep.h>
>  #include <Library/TdxHelperLib.h>
>  #include <Library/CcProbeLib.h>
> +#include <Register/Intel/ArchitecturalMsr.h>
> +#include <Register/Intel/Cpuid.h>

(4) with the CPUID gone, the second header should not be needed

>  #include "AmdSev.h"
>  
>  #define SEC_IDT_ENTRY_COUNT  34
> @@ -744,6 +746,31 @@ FindAndReportEntryPoints (
>    return;
>  }
>  
> +//
> +// Enable MTRR early, set default type to write back.
> +// Needed to make sure caching is enabled,
> +// without this lzma decompress can be very slow.
> +//
> +STATIC
> +VOID
> +SecMtrrSetup (
> +  VOID
> +  )
> +{
> +  CPUID_VERSION_INFO_EDX           Edx;
> +  MSR_IA32_MTRR_DEF_TYPE_REGISTER  DefType;
> +
> +  AsmCpuid (CPUID_VERSION_INFO, NULL, NULL, NULL, &Edx.Uint32);
> +  if (!Edx.Bits.MTRR) {
> +    return;
> +  }
> +
> +  DefType.Uint64    = 0;

(5) Can we first read the MSR, instead of explicit zeroing?

> +  DefType.Bits.Type = 6; /* write back */

(6) I've noticed an independent (preexistent!) wart: I dislike the naked
constant "6" for WriteBack.

Turns out we have had MTRR_CACHE_WRITE_BACK in

  UefiCpuPkg/Include/Library/MtrrLib.h

since historical commit e50466da2437 ("Add MTRR library for IA32 & X64
processor architectures.", 2009-05-27).

This means that my original commit 79d274b8b6b1 ("OvmfPkg: PlatformPei:
invert MTRR setup in QemuInitializeRam()", 2015-06-26) could have been
better: that commit already consumed MtrrLib, so it could have used
MTRR_CACHE_WRITE_BACK, in place of "6", from the lib class header.

OVMF SEC however does not include <Library/MtrrLib.h> -- and it
shouldn't. That means it cannot use MTRR_CACHE_WRITE_BACK. That makes me
somewhat unhappy.

Proposal:

- Copy and rename the MTRR_CACHE_* macros from
"UefiCpuPkg/Include/Library/MtrrLib.h" to
"MdePkg/Include/Register/Intel/ArchitecturalMsr.h"

- Redefine MTRR_CACHE_* in MtrrLib.h in terms of the new macros from
"ArchitecturalMsr.h".

(in a separate patch, of course).

This will preserve compatibility, but also expose the (new) macro names
to modules that don't consume MtrrLib -- for example, OVMF SEC.

Probably consult the MtrrLib and ArchitecturalMsr.h maintainers first,
though -- so I would propose three patch in the end:

- this patch, using naked constant "6"

- macro reorg

- adopting the new macro name in OVMF SEC, in place of 6

If the MtrrLib and ArchitecturalMsr.h maintainers disagree with patch
#2, we can simply drop #2 and #3, and just merge #1.


> +  DefType.Bits.E    = 1; /* enable */
> +  AsmWriteMsr64 (MSR_IA32_MTRR_DEF_TYPE, DefType.Uint64);
> +}
> +
>  VOID
>  EFIAPI
>  SecCoreStartupWithStack (
> @@ -942,6 +969,11 @@ SecCoreStartupWithStack (
>    InitializeApicTimer (0, MAX_UINT32, TRUE, 5);
>    DisableApicTimerInterrupt ();
>  
> +  //
> +  // Initialize MTRR
> +  //
> +  SecMtrrSetup ();
> +
>    //
>    // Initialize Debug Agent to support source level debug in SEC/PEI phases before memory ready.
>    //

Ugh, I can't really comment where to place this call. FWIW, the problem
path is:

SecCoreStartupWithStack()
  InitializeDebugAgent()
    SecStartupPhase2()
      FindAndReportEntryPoints()
        FindPeiCoreImageBase()
          DecompressMemFvs()
            ExtractGuidedSectionDecode()

That's where the LZMA decompression happens.

The patch places the call to SecMtrrSetup() "early enough", but is it
the "best" location from all possible, "early enough" locations? I can't
say.

(7) If you have a particular reason for doing it here, please capture
that in the comment.


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

Thanks,
Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#114315): https://edk2.groups.io/g/devel/message/114315
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]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Gerd Hoffmann]

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]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Laszlo Ersek]

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]
-=-=-=-=-=-=-=-=-=-=-=-