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 1178E941B81 for ; Thu, 25 Jan 2024 19:44:29 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=68wLHtDJA52/n/Y03z94CiOUV7w1E8f5h0iaLVo5NEg=; 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=1706211868; v=1; b=eO70PzSsQAm5MudgZNyqRchEnov0RZlHx/E1bJh8kiqgmPgNtDgxyk8RPd2KSSMLkKER4Mgw gpx5ExhaUGOCZWAVG1ZC9L6GMZDi6se6Ds0qVpir+QD3KodMkydkg8e0FqRlwEauJ5/vtsHlj98 p/oWOQJWrwd+J9n36+/fYHFc= X-Received: by 127.0.0.2 with SMTP id 9TCjYY7687511x7hV1ImSISX; Thu, 25 Jan 2024 11:44:28 -0800 X-Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by mx.groups.io with SMTP id smtpd.web10.28272.1706211868031089567 for ; Thu, 25 Jan 2024 11:44:28 -0800 X-Received: from mimecast-mx02.redhat.com (mx-ext.redhat.com [66.187.233.73]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-593-MVEysZSnO2Kow8KF3WxR3g-1; Thu, 25 Jan 2024 14:44:21 -0500 X-MC-Unique: MVEysZSnO2Kow8KF3WxR3g-1 X-Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.rdu2.redhat.com [10.11.54.3]) (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 A90A71C0515F; Thu, 25 Jan 2024 19:44:20 +0000 (UTC) X-Received: from [10.39.195.100] (unknown [10.39.195.100]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 27F4F1121306; Thu, 25 Jan 2024 19:44:19 +0000 (UTC) Message-ID: <48277711-76dd-116b-9e9f-c2830c8474cd@redhat.com> Date: Thu, 25 Jan 2024 20:44:18 +0100 MIME-Version: 1.0 Subject: Re: [edk2-devel] [PATCH 1/1] OvmfPkg/Sec: Setup MTRR early in the boot process. To: Gerd Hoffmann Cc: devel@edk2.groups.io, Michael Roth , Tom Lendacky , Jiewen Yao , Ard Biesheuvel , Oliver Steffen , Min Xu , Erdem Aktas References: <20240124151542.2091782-1-kraxel@redhat.com> <6d89f7aa-938c-2a96-3eb8-53169dcfd3db@redhat.com> <34qfpf44zr4e5fgxbajked3dbwuc2k6yl6bherm4zazziuyyef@33k4wclwrh44> From: "Laszlo Ersek" In-Reply-To: <34qfpf44zr4e5fgxbajked3dbwuc2k6yl6bherm4zazziuyyef@33k4wclwrh44> X-Scanned-By: MIMEDefang 3.4.1 on 10.11.54.3 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: yd5aUdnM4icji1q0EA8ekB4Dx7686176AA= 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=eO70PzSs; 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/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 uncompres= s >>> 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_SHIF= T; > } > >> 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 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.N= W. When this quirk is disabled, KVM does n= ot 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 (=3D 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) =3D=3D 0x0 and we should still do ASSERT ((MtrrSettings.MtrrDefType & BIT11) !=3D 0); ASSERT ((MtrrSettings.MtrrDefType & BIT10) =3D=3D 0); ASSERT ((MtrrSettings.MtrrDefType & 0xFF) =3D=3D 6); ... MtrrSettings.MtrrDefType |=3D 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=3Doff), > 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 -=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 (#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/19134562= 12/xyzzy [rebecca@openfw.io] -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-