From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail02.groups.io (mail02.groups.io [66.175.222.108]) by spool.mail.gandi.net (Postfix) with ESMTPS id A130DAC18AF for ; Wed, 24 Jan 2024 16:14:19 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=HBWxc4GTLMP+oat6BnmnZXfGOJAAtOzuCdDWH/I7cho=; c=relaxed/simple; d=groups.io; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From:In-Reply-To:Precedence:List-Subscribe:List-Help:Sender:List-Id:Mailing-List:Delivered-To:Reply-To:List-Unsubscribe-Post:List-Unsubscribe:Content-Language:Content-Type:Content-Transfer-Encoding; s=20140610; t=1706112858; v=1; b=q+Drv6HLWkcxxN/7GXJie/OFM94XTP/nRkzCXYHrFpDPNOcxLdExSalGouwW1YSLe/MceECu eWegTMViE0ECvG/7odtqAkPhU7Fuvg1WsvqZYR2Ge8EkYTuSpA27x8zAFtknKEfoKapQ1Wi3K4A YLVo+njchYnQqIJ+lQL6by14= X-Received: by 127.0.0.2 with SMTP id Y8wOYY7687511xQZIqB8Xcpw; Wed, 24 Jan 2024 08:14:18 -0800 X-Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by mx.groups.io with SMTP id smtpd.web11.26910.1706112857736266663 for ; Wed, 24 Jan 2024 08:14:17 -0800 X-Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-550-_DvFO4F3M4atDnq3tBLUdw-1; Wed, 24 Jan 2024 11:14:13 -0500 X-MC-Unique: _DvFO4F3M4atDnq3tBLUdw-1 X-Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.rdu2.redhat.com [10.11.54.2]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id E7779837228; Wed, 24 Jan 2024 16:14:12 +0000 (UTC) X-Received: from [10.39.195.127] (unknown [10.39.195.127]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 7B74540C1430; Wed, 24 Jan 2024 16:14:11 +0000 (UTC) Message-ID: <6d89f7aa-938c-2a96-3eb8-53169dcfd3db@redhat.com> Date: Wed, 24 Jan 2024 17:14:10 +0100 MIME-Version: 1.0 Subject: Re: [edk2-devel] [PATCH 1/1] OvmfPkg/Sec: Setup MTRR early in the boot process. To: Gerd Hoffmann , devel@edk2.groups.io Cc: Michael Roth , Tom Lendacky , Jiewen Yao , Ard Biesheuvel , Oliver Steffen , Min Xu , Erdem Aktas References: <20240124151542.2091782-1-kraxel@redhat.com> From: "Laszlo Ersek" In-Reply-To: <20240124151542.2091782-1-kraxel@redhat.com> X-Scanned-By: MIMEDefang 3.4.1 on 10.11.54.2 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Precedence: Bulk List-Subscribe: List-Help: Sender: devel@edk2.groups.io List-Id: Mailing-List: list devel@edk2.groups.io; contact devel+owner@edk2.groups.io Reply-To: devel@edk2.groups.io,lersek@redhat.com List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: X-Gm-Message-State: CNLMvB57bS45e9rpRhlOgBfvx7686176AA= Content-Language: en-US Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20140610 header.b=q+Drv6HL; dmarc=fail reason="SPF not aligned (relaxed), DKIM not aligned (relaxed)" header.from=redhat.com (policy=none); spf=pass (spool.mail.gandi.net: domain of bounce@groups.io designates 66.175.222.108 as permitted sender) smtp.mailfrom=bounce@groups.io 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. >=20 > Adapt the ASSERTs in PlatformInitLib to the changes. >=20 > 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). >=20 > Signed-off-by: Gerd Hoffmann > --- > OvmfPkg/Library/PlatformInitLib/MemDetect.c | 8 ++++-- > OvmfPkg/Sec/SecMain.c | 32 +++++++++++++++++++++ > 2 files changed, 37 insertions(+), 3 deletions(-) The original report 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? >=20 > diff --git a/OvmfPkg/Library/PlatformInitLib/MemDetect.c b/OvmfPkg/Librar= y/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); > =20 > // > - // 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) =3D=3D 0); > ASSERT ((MtrrSettings.MtrrDefType & BIT10) =3D=3D 0); > - ASSERT ((MtrrSettings.MtrrDefType & 0xFF) =3D=3D 0); > + ASSERT ( > + (MtrrSettings.MtrrDefType & 0xFF) =3D=3D 0x0 || > + (MtrrSettings.MtrrDefType & 0xFF) =3D=3D 0x6 > + ); > =20 > // > // 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 |=3D 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) =3D=3D 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=3D6). Something like: ASSERT ((MtrrSettings.MtrrDefType & BIT11) !=3D 0); ASSERT ((MtrrSettings.MtrrDefType & BIT10) =3D=3D 0); ASSERT ((MtrrSettings.MtrrDefType & 0xFF) =3D=3D 6); ... MtrrSettings.MtrrDefType |=3D 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 > #include > #include > +#include > +#include (4) with the CPUID gone, the second header should not be needed > #include "AmdSev.h" > =20 > #define SEC_IDT_ENTRY_COUNT 34 > @@ -744,6 +746,31 @@ FindAndReportEntryPoints ( > return; > } > =20 > +// > +// 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 =3D 0; (5) Can we first read the MSR, instead of explicit zeroing? > + DefType.Bits.Type =3D 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 -- 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 =3D 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 (); > =20 > + // > + // Initialize MTRR > + // > + SecMtrrSetup (); > + > // > // Initialize Debug Agent to support source level debug in SEC/PEI pha= ses 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 -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D- 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/19134562= 12/xyzzy [rebecca@openfw.io] -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-