Re: [edk2-devel] [PATCH 2/3] OvmfPkg/ResetVector: add 5-level paging support

["Laszlo Ersek" <lersek@redhat.com>]

On 1/29/24 13:02, Gerd Hoffmann wrote:
> Compile the OVMF ResetVector with 5-level paging support in case
> PcdUse5LevelPageTable is TRUE.
> 
> 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.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  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.

> 
> 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 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
>  
> +%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 @@
>  
>  %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))

Thanks
Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
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/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-