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 <kra...@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/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/leave/9847357/21656/1706620634/xyzzy [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
