From: "Laszlo Ersek" <lersek@redhat.com>
To: devel@edk2.groups.io, kraxel@redhat.com
Cc: Tom Lendacky <thomas.lendacky@amd.com>,
Jiewen Yao <jiewen.yao@intel.com>,
Oliver Steffen <osteffen@redhat.com>,
Erdem Aktas <erdemaktas@google.com>,
Michael Roth <michael.roth@amd.com>,
Ard Biesheuvel <ardb+tianocore@kernel.org>,
Min Xu <min.m.xu@intel.com>
Subject: Re: [edk2-devel] [PATCH 06/10] OvmfPkg/ResetVector: add 5-level paging support
Date: Wed, 28 Feb 2024 06:33:34 +0100 [thread overview]
Message-ID: <8163c5aa-47fd-330c-ba50-c24d676a5903@redhat.com> (raw)
In-Reply-To: <20240222115435.85794-7-kraxel@redhat.com>
On 2/22/24 12:54, Gerd Hoffmann wrote:
> Add macros to check for 5-level paging and gigabyte page support.
> Enable 5-level paging for the non-confidential-computing case.
>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
> OvmfPkg/ResetVector/ResetVector.inf | 1 +
> OvmfPkg/ResetVector/Ia32/PageTables64.asm | 105 ++++++++++++++++++++++
> OvmfPkg/ResetVector/ResetVector.nasmb | 1 +
> 3 files changed, 107 insertions(+)
>
> diff --git a/OvmfPkg/ResetVector/ResetVector.inf b/OvmfPkg/ResetVector/ResetVector.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/ResetVector/Ia32/PageTables64.asm
> index 84a7b4efc019..825589f31193 100644
> --- a/OvmfPkg/ResetVector/Ia32/PageTables64.asm
> +++ b/OvmfPkg/ResetVector/Ia32/PageTables64.asm
> @@ -101,6 +101,97 @@ BITS 32
> loop .pageTableEntriesLoop4Level
> %endmacro
>
> +;
> +; Check whenever 5-level paging can ca used
(1) typo in comment
> +;
> +; Argument: jump label for 4-level paging
> +;
> +%macro Check5LevelPaging 1
> + ; check for cpuid leaf 0x07
> + mov eax, 0x00
> + cpuid
> + cmp eax, 0x07
> + jb %1
> +
> + ; check for la57 (aka 5-level paging)
> + mov eax, 0x07
> + mov ecx, 0x00
> + cpuid
> + bt ecx, 16
> + jnc %1
> +
> + ; check for cpuid leaf 0x80000001
> + mov eax, 0x80000000
> + cpuid
> + cmp eax, 0x80000001
> + jb %1
> +
> + ; check for 1g pages
> + mov eax, 0x80000001
> + cpuid
> + bt edx, 26
> + jnc %1
> +%endmacro
> +
> +;
> +; Create page tables for 5-level paging with gigabyte pages
> +;
> +; Argument: upper 32 bits of the page table entries
> +;
> +; We have 6 pages available for the early page tables,
> +; we use four of them:
> +; PT_ADDR(0) - level 5 directory
> +; PT_ADDR(0x1000) - level 4 directory
> +; PT_ADDR(0x2000) - level 2 directory (0 -> 1GB)
> +; PT_ADDR(0x3000) - level 3 directory
> +;
> +; The level 2 directory for the first gigabyte has the same
> +; physical address in both 4-level and 5-level paging mode,
> +; SevClearPageEncMaskForGhcbPage depends on this.
> +;
> +; The 1 GB -> 4 GB range is mapped using 1G pages in the
> +; level 3 directory.
> +;
> +%macro CreatePageTables5Level 1
> + ; level 5
> + mov dword[PT_ADDR (0)], PT_ADDR (0x1000) + PAGE_PDE_DIRECTORY_ATTR
> + mov dword[PT_ADDR (4)], %1
> +
> + ; level 4
> + mov dword[PT_ADDR (0x1000)], PT_ADDR (0x3000) + PAGE_PDE_DIRECTORY_ATTR
> + mov dword[PT_ADDR (0x1004)], %1
> +
> + ; level 3 (1x -> level 2, 3x 1GB)
> + mov dword[PT_ADDR (0x3000)], PT_ADDR (0x2000) + PAGE_PDE_DIRECTORY_ATTR
> + mov dword[PT_ADDR (0x3004)], %1
> + mov dword[PT_ADDR (0x3008)], (1 << 30) + PAGE_PDE_LARGEPAGE_ATTR
> + mov dword[PT_ADDR (0x300c)], %1
> + mov dword[PT_ADDR (0x3010)], (2 << 30) + PAGE_PDE_LARGEPAGE_ATTR
> + mov dword[PT_ADDR (0x3014)], %1
> + mov dword[PT_ADDR (0x3018)], (3 << 30) + PAGE_PDE_LARGEPAGE_ATTR
> + mov dword[PT_ADDR (0x301c)], %1
> +
> + ;
> + ; level 2 (512 * 2MB entries => 1GB)
> + ;
> + mov ecx, 0x200
> +.pageTableEntriesLoop5Level:
(2) suggest "..@pageTableEntriesLoop5Level" as label name
> + mov eax, ecx
> + dec eax
> + shl eax, 21
> + add eax, PAGE_PDE_LARGEPAGE_ATTR
> + mov dword[ecx * 8 + PT_ADDR (0x2000 - 8)], eax
> + mov dword[(ecx * 8 + PT_ADDR (0x2000 - 8)) + 4], %1
> + loop .pageTableEntriesLoop5Level
> +%endmacro
> +
> +%macro Enable5LevelPaging 0
> + ; set la57 bit in cr4
> + mov eax, cr4
> + bts eax, 12
> + mov cr4, eax
> +%endmacro
> +
> ;
> ; Modified: EAX, EBX, ECX, EDX
> ;
> @@ -125,6 +216,12 @@ SetCr3ForPageTables64:
> ; normal (non-CoCo) workflow
> ;
> ClearOvmfPageTables
> +%if PG_5_LEVEL
> + Check5LevelPaging Paging4Level
> + CreatePageTables5Level 0
> + jmp SetCr3La57
> +Paging4Level:
> +%endif
> CreatePageTables4Level 0
> jmp SetCr3
>
> @@ -151,6 +248,14 @@ TdxBspInit:
> OneTimeCall TdxPostBuildPageTables
> jmp SetCr3
>
> + ;
> + ; common workflow
> + ;
> +%if PG_5_LEVEL
> +SetCr3La57:
> + Enable5LevelPaging
> +%endif
> +
> SetCr3:
> ;
> ; Set CR3 now that the paging structures are available
(3) I don't like SetCr3La57.
It saves minimal code duplication, but makes the control flow much
harder to follow.
(3.1) From this patch, we have one (unconditional) jump to SetCr3La57. I
suggest simply inlining that, like this:
%if PG_5_LEVEL
Check5LevelPaging Paging4Level
CreatePageTables5Level 0
Enable5LevelPaging <-------- inline
jmp SetCr3 <-------- here
Paging4Level:
%endif
This adds one extra line before the unconditional jump, but removes 8
lines at the destination, *plus* it removes a quirky jump / reused code
path.
(3.2) From patch #8 ("OvmfPkg/ResetVector: wire up 5-level paging for
TDX"), we have two jumps to SetCr3La57; one conditional and one
unconditional.
- The unconditional jump can be replaced with inlining (invoke the
Enable5LevelPaging macro, then jump to SetCr3).
- The conditional one can be reworked like this:
%if PG_5_LEVEL
cmp eax, TDX_AP_5_LEVEL
jne CheckForSev
Enable5LevelPaging
jmp SetCr3
CheckForSev:
%endif
This way, the conditional jump is *local*; way easier to follow. The
"CheckForSev" label much resembles the "Paging4Level" label above.
The patch looks fine otherwise.
Thanks!
Laszlo
> diff --git a/OvmfPkg/ResetVector/ResetVector.nasmb b/OvmfPkg/ResetVector/ResetVector.nasmb
> index 366a70fb9992..2bd80149e58b 100644
> --- a/OvmfPkg/ResetVector/ResetVector.nasmb
> +++ b/OvmfPkg/ResetVector/ResetVector.nasmb
> @@ -53,6 +53,7 @@
>
> %define WORK_AREA_GUEST_TYPE (FixedPcdGet32 (PcdOvmfWorkAreaBase))
> %define PT_ADDR(Offset) (FixedPcdGet32 (PcdOvmfSecPageTablesBase) + (Offset))
> +%define PG_5_LEVEL (FixedPcdGetBool (PcdUse5LevelPageTable))
>
> %define GHCB_PT_ADDR (FixedPcdGet32 (PcdOvmfSecGhcbPageTableBase))
> %define GHCB_BASE (FixedPcdGet32 (PcdOvmfSecGhcbBase))
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116085): https://edk2.groups.io/g/devel/message/116085
Mute This Topic: https://groups.io/mt/104506793/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
next prev parent reply other threads:[~2024-02-28 5:33 UTC|newest]
Thread overview: 23+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-02-22 11:54 [edk2-devel] [PATCH 00/10] OvmfPkg/ResetVector: cleanup and add 5-level paging support Gerd Hoffmann
2024-02-22 11:54 ` [edk2-devel] [PATCH 01/10] OvmfPkg/ResetVector: improve page table flag names Gerd Hoffmann
2024-02-22 11:54 ` [edk2-devel] [PATCH 02/10] OvmfPkg/ResetVector: add ClearOvmfPageTables macro Gerd Hoffmann
2024-02-28 4:09 ` Laszlo Ersek
2024-02-28 8:22 ` Gerd Hoffmann
2024-02-29 7:42 ` Laszlo Ersek
2024-02-22 11:54 ` [edk2-devel] [PATCH 03/10] OvmfPkg/ResetVector: add CreatePageTables4Level macro Gerd Hoffmann
2024-02-28 4:14 ` Laszlo Ersek
2024-02-22 11:54 ` [edk2-devel] [PATCH 04/10] OvmfPkg/ResetVector: split TDX BSP workflow Gerd Hoffmann
2024-02-28 4:34 ` Laszlo Ersek
2024-02-22 11:54 ` [edk2-devel] [PATCH 05/10] OvmfPkg/ResetVector: split SEV and non-CoCo workflows Gerd Hoffmann
2024-02-28 4:51 ` Laszlo Ersek
2024-02-22 11:54 ` [edk2-devel] [PATCH 06/10] OvmfPkg/ResetVector: add 5-level paging support Gerd Hoffmann
2024-02-28 5:33 ` Laszlo Ersek [this message]
2024-02-22 11:54 ` [edk2-devel] [PATCH 07/10] OvmfPkg/ResetVector: print post codes for 4/5 level paging Gerd Hoffmann
2024-02-28 5:35 ` Laszlo Ersek
2024-02-22 11:54 ` [edk2-devel] [PATCH 08/10] OvmfPkg/ResetVector: wire up 5-level paging for TDX Gerd Hoffmann
2024-02-28 5:44 ` Laszlo Ersek
2024-02-22 11:54 ` [edk2-devel] [PATCH 09/10] OvmfPkg/ResetVector: leave SEV VC handler installed longer Gerd Hoffmann
2024-02-28 5:52 ` Laszlo Ersek
2024-02-29 15:47 ` Lendacky, Thomas via groups.io
2024-02-22 11:54 ` [edk2-devel] [PATCH 10/10] OvmfPkg/ResetVector: wire up 5-level paging for SEV Gerd Hoffmann
2024-02-28 5:51 ` Laszlo Ersek
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=8163c5aa-47fd-330c-ba50-c24d676a5903@redhat.com \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox