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 E1D82780091 for ; Mon, 29 Jan 2024 21:04:42 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=JVZcycevNEQF7WPA8NCFmqzcwZXvVV/ABgL8UE+mGus=; 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=1706562281; v=1; b=X02vlnYoHIJDjJfr5k3kYo6RqWJpSZhOEdXHJVZ3vBGAGbukhnC31ZDsouYgxGMZG9iPp189 wyNt5sqGbz0AjxTpBOvz9LxFPc0yQIrOMnAlq2yQTHqMALbkHpjoyMe3DTqKFaJr+qeYPjCKNNa TkVRkinxk16E0iXxhve9ptA0= X-Received: by 127.0.0.2 with SMTP id xD2GYY7687511xzRW7xWvp99; Mon, 29 Jan 2024 13:04:41 -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.2287.1706562280580533684 for ; Mon, 29 Jan 2024 13:04:40 -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-63-glLAxaxgN2Wr0Gts8ZfycQ-1; Mon, 29 Jan 2024 16:04:36 -0500 X-MC-Unique: glLAxaxgN2Wr0Gts8ZfycQ-1 X-Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.rdu2.redhat.com [10.11.54.4]) (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 05FF884AE44; Mon, 29 Jan 2024 21:04:36 +0000 (UTC) X-Received: from [10.39.192.97] (unknown [10.39.192.97]) by smtp.corp.redhat.com (Postfix) with ESMTPS id E805B200B3A3; Mon, 29 Jan 2024 21:04:33 +0000 (UTC) Message-ID: <3416850d-8977-5966-97a0-986e1e623e93@redhat.com> Date: Mon, 29 Jan 2024 22:04:32 +0100 MIME-Version: 1.0 Subject: Re: [edk2-devel] [PATCH 2/3] OvmfPkg/ResetVector: add 5-level paging support To: devel@edk2.groups.io, kraxel@redhat.com Cc: Oliver Steffen , Ard Biesheuvel , Michael Roth , Min Xu , Tom Lendacky , Erdem Aktas , Jiewen Yao , Liming Gao References: <20240129120201.344234-1-kraxel@redhat.com> <20240129120201.344234-3-kraxel@redhat.com> From: "Laszlo Ersek" In-Reply-To: <20240129120201.344234-3-kraxel@redhat.com> X-Scanned-By: MIMEDefang 3.4.1 on 10.11.54.4 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: 4N0yE85VUBD8FzleD58zPo5ix7686176AA= 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=X02vlnYo; 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/29/24 13:02, Gerd Hoffmann wrote: > Compile the OVMF ResetVector with 5-level paging support in case > PcdUse5LevelPageTable is TRUE. >=20 > When enabled the ResetVector will check at runtime whenever support for > 5-level paging and gigabyte pages is available. In case both features > are supported it will run OVMF in 5-level paging mode, otherwise > fallback to 4-level paging. >=20 > Signed-off-by: Gerd Hoffmann > --- > OvmfPkg/ResetVector/ResetVector.inf | 1 + > OvmfPkg/ResetVector/Ia32/PageTables64.asm | 72 +++++++++++++++++++++++ > OvmfPkg/ResetVector/ResetVector.nasmb | 1 + > 3 files changed, 74 insertions(+) Gerd, you really need to write more documentation for your code. It's very difficult to follow. I shouldn't have to reverse engineer it :/ More details below. >=20 > diff --git a/OvmfPkg/ResetVector/ResetVector.inf b/OvmfPkg/ResetVector/Re= setVector.inf > index a4154ca90c28..65f71b05a02e 100644 > --- a/OvmfPkg/ResetVector/ResetVector.inf > +++ b/OvmfPkg/ResetVector/ResetVector.inf > @@ -64,3 +64,4 @@ [FixedPcd] > gUefiOvmfPkgTokenSpaceGuid.PcdQemuHashTableSize > gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSnpSecretsBase > gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSnpSecretsSize > + gEfiMdeModulePkgTokenSpaceGuid.PcdUse5LevelPageTable > diff --git a/OvmfPkg/ResetVector/Ia32/PageTables64.asm b/OvmfPkg/ResetVec= tor/Ia32/PageTables64.asm > index 317cad430f29..0f88d1c71798 100644 > --- a/OvmfPkg/ResetVector/Ia32/PageTables64.asm > +++ b/OvmfPkg/ResetVector/Ia32/PageTables64.asm > @@ -81,6 +81,78 @@ clearPageTablesMemoryLoop: > mov dword[ecx * 4 + PT_ADDR (0) - 4], eax > loop clearPageTablesMemoryLoop > =20 > +%if PG_5_LEVEL > + > + ; save edx (cpuid changes it) > + mov edi, edx > + > + ; check for cpuid leaf 0x07 > + mov eax, 0x00 > + cpuid > + cmp eax, 0x07 > + jb Paging4Lvl > + > + ; check for la57 (aka 5-level paging) > + mov eax, 0x07 > + mov ecx, 0x00 > + cpuid > + bt ecx, 16 > + jnc Paging4Lvl > + > + ; check for cpuid leaf 0x80000001 > + mov eax, 0x80000000 > + cpuid > + cmp eax, 0x80000001 > + jb Paging4Lvl > + > + ; check for 1g pages > + mov eax, 0x80000001 > + cpuid > + bt edx, 26 > + jnc Paging4Lvl > + > + ; > + ; use 5-level paging with gigabyte pages > + ; > + debugShowPostCode 0x51 ; 5-level paging > + > + ; restore edx > + mov edx, edi This part is easy to follow, thanks for the many comments. I have not verified each step against the Intel SDM, but the comments provide good coverage, so I'm happy to take this on trust. (1) One nit: the code doesn't say what EDX carries at this point. From the context, it can be determined (it's the "return value" of GetSevCBitMaskAbove31, the most significant dword that goes into the PTEs), but the context is *large*. Please expand on the "save edx" comment. > + > + ; level 5 > + mov dword[PT_ADDR (0)], PT_ADDR (0x1000) + PAGE_PDP_ATTR > + mov dword[PT_ADDR (4)], edx > + > + ; level 4 > + mov dword[PT_ADDR (0x1000)], PT_ADDR (0x2000) + PAGE_PDP_ATTR > + mov dword[PT_ADDR (0x1004)], edx > + > + ; level 3 > + mov dword[PT_ADDR (0x2000)], (0 << 30) + PAGE_2M_PDE_ATTR > + mov dword[PT_ADDR (0x2004)], edx > + mov dword[PT_ADDR (0x2008)], (1 << 30) + PAGE_2M_PDE_ATTR > + mov dword[PT_ADDR (0x200c)], edx > + mov dword[PT_ADDR (0x2010)], (2 << 30) + PAGE_2M_PDE_ATTR > + mov dword[PT_ADDR (0x2014)], edx > + mov dword[PT_ADDR (0x2018)], (3 << 30) + PAGE_2M_PDE_ATTR > + mov dword[PT_ADDR (0x201c)], edx (2) So this is my problem. I've found this hard to decipher. The original (4-level) counterpart is much better commented. So you either consulted the documentation to invent this (but then didn't write up your thought process), or you just "winged it" (which may still be fine, but you need to explain the setup even more). As far as I can tell, we're covering the first 4GB of address space with 4 pages of 1GB size each, and because of that, we need not go deeper than level 3. That one sentence in a comment (which would have cost you 5 seconds) could have saved me 10 minutes of eye-strain, at night. It's *entirely different* if you put the right idea in my mind with natural language and then I verify your code against that idea, from when I have to build the high-level concept from the minute code details without any support. (3) While the macro PAGE_2M_PDE_ATTR might expand to just the right constant here, the "2M" in the name is extremely confusing. Please introduce a new macro for selecting 1GB pages. The replacement text for the macro may be as easy as you want it (you could reuse PAGE_2M_PDE_ATTR there). *Semantically* the 2M reference is a bug here. > + > + ; set la57 bit in cr4 > + mov eax, cr4 > + bts eax, 12 > + mov cr4, eax > + > + ; done > + jmp SetCr3 (4) Functionality question: is the 5-level branch compatible with SEV and/or TDX? Asking because: - EDX is still from GetSevCBitMaskAbove31. I don't know what happens if SEV is enabled and such an EDX is used at level 5. - jumping to the SetCr3 label from here omits the "SevClearPageEncMaskForGhcbPage" and "TdxPostBuildPageTables" pieces of logic. If 5-level paging is not compatible with SEV / TDX, we should document it. Even better, we might want to catch either in the code (on the 5-level branch) and hang hard, early on. If the feature detection code (with the CPUID instructions) *already* guarantees that the 5-level page table setup will never be reached in SEV/TDX guests, that's awesome, but we *absolutely* need to document it. > + > +Paging4Lvl: > + debugShowPostCode 0x41 ; 4-level paging > + > + ; restore edx > + mov edx, edi > + > +%endif > + > ; > ; Top level Page Directory Pointers (1 * 512GB entry) > ; > diff --git a/OvmfPkg/ResetVector/ResetVector.nasmb b/OvmfPkg/ResetVector/= ResetVector.nasmb > index 5832aaa8abf7..16b3eee57671 100644 > --- a/OvmfPkg/ResetVector/ResetVector.nasmb > +++ b/OvmfPkg/ResetVector/ResetVector.nasmb > @@ -49,6 +49,7 @@ > =20 > %define WORK_AREA_GUEST_TYPE (FixedPcdGet32 (PcdOvmfWorkAreaBas= e)) > %define PT_ADDR(Offset) (FixedPcdGet32 (PcdOvmfSecPageTabl= esBase) + (Offset)) > +%define PG_5_LEVEL (FixedPcdGetBool (PcdUse5LevelPage= Table)) > =20 > %define GHCB_PT_ADDR (FixedPcdGet32 (PcdOvmfSecGhcbPage= TableBase)) > %define GHCB_BASE (FixedPcdGet32 (PcdOvmfSecGhcbBase= )) 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 (#114745): https://edk2.groups.io/g/devel/message/114745 Mute This Topic: https://groups.io/mt/104029639/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-