[edk2-devel] [PATCH 0/3] OvmfPkg: Add support for 5-level paging

[edk2-devel] [PATCH 0/3] OvmfPkg: Add support for 5-level paging

[Gerd Hoffmann]

Patch #1 has been submitted separately last week.
Intewl raised concerns that removing or renaming
the PCD breaks platforms, so I'm just doing the
minimal fix here.

Patch #2 + #3 update OvmfPkg ResetVector and
PlatformInitLib for 5-level paging support.

Gerd Hoffmann (3):
  MdeModulePkg: fix PcdUse5LevelPageTable assert
  OvmfPkg/ResetVector: add 5-level paging support
  OvmfPkg/PlatformInitLib: add 5-level paging support

 OvmfPkg/ResetVector/ResetVector.inf           |  1 +
 .../Core/DxeIplPeim/X64/VirtualMemory.c       |  4 +-
 OvmfPkg/Library/PlatformInitLib/MemDetect.c   | 57 ++++++++++-----
 OvmfPkg/ResetVector/Ia32/PageTables64.asm     | 72 +++++++++++++++++++
 OvmfPkg/ResetVector/ResetVector.nasmb         |  1 +
 5 files changed, 115 insertions(+), 20 deletions(-)

-- 
2.43.0



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#114687): https://edk2.groups.io/g/devel/message/114687
Mute This Topic: https://groups.io/mt/104029636/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH 1/3] MdeModulePkg: fix PcdUse5LevelPageTable assert

[Gerd Hoffmann]

PcdUse5LevelPageTable documentation says:

  Indicates if 5-Level Paging will be enabled in long mode. 5-Level
  Paging will not be enabled when the PCD is TRUE but CPU doesn't support
  5-Level Paging.

So running in 4-level paging mode with PcdUse5LevelPageTable=TRUE is
possible.  The only invalid combination is 5-level paging being active
with PcdUse5LevelPageTable=FALSE.

Fix the ASSERT accordingly.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c b/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
index 980c2002d4f5..1d240e95966e 100644
--- a/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
+++ b/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
@@ -745,7 +745,9 @@ CreateIdentityMappingPageTables (
     //
     Cr4.UintN         = AsmReadCr4 ();
     Page5LevelSupport = (Cr4.Bits.LA57 != 0);
-    ASSERT (PcdGetBool (PcdUse5LevelPageTable) == Page5LevelSupport);
+    if (Page5LevelSupport) {
+      ASSERT (PcdGetBool (PcdUse5LevelPageTable));
+    }
   } else {
     //
     // If cpu runs in 32bit protected mode PEI, Page table Level in DXE is decided by PCD and feature capability.
-- 
2.43.0



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#114686): https://edk2.groups.io/g/devel/message/114686
Mute This Topic: https://groups.io/mt/104029635/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 1/3] MdeModulePkg: fix PcdUse5LevelPageTable assert

[Laszlo Ersek]

On 1/29/24 13:01, Gerd Hoffmann wrote:
> PcdUse5LevelPageTable documentation says:
> 
>   Indicates if 5-Level Paging will be enabled in long mode. 5-Level
>   Paging will not be enabled when the PCD is TRUE but CPU doesn't support
>   5-Level Paging.
> 
> So running in 4-level paging mode with PcdUse5LevelPageTable=TRUE is
> possible.  The only invalid combination is 5-level paging being active
> with PcdUse5LevelPageTable=FALSE.
> 
> Fix the ASSERT accordingly.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c b/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
> index 980c2002d4f5..1d240e95966e 100644
> --- a/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
> +++ b/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
> @@ -745,7 +745,9 @@ CreateIdentityMappingPageTables (
>      //
>      Cr4.UintN         = AsmReadCr4 ();
>      Page5LevelSupport = (Cr4.Bits.LA57 != 0);
> -    ASSERT (PcdGetBool (PcdUse5LevelPageTable) == Page5LevelSupport);
> +    if (Page5LevelSupport) {
> +      ASSERT (PcdGetBool (PcdUse5LevelPageTable));
> +    }
>    } else {
>      //
>      // If cpu runs in 32bit protected mode PEI, Page table Level in DXE is decided by PCD and feature capability.

FWIW, my observation at [1] was not that the PCD was incorrectly named -- I noted that the *local variable* "Page5LevelSupport" was a misnomer.

[1] http://mid.mail-archive.com/640e533e-5252-32da-c155-a25e3065eb78@redhat.com
    https://edk2.groups.io/g/devel/message/114318

I proposed "Page5LevelEnabled" instead.

But, it's not a show-stopper in any case. (I was a bit surprised to see the PCD rename in v2, but that would also have been an improvement.)

Reviewed-by: Laszlo Ersek <lersek@redhat.com>



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#114743): https://edk2.groups.io/g/devel/message/114743
Mute This Topic: https://groups.io/mt/104029635/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Gerd Hoffmann]

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(+)

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
+
+    ; 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
+
+    ; set la57 bit in cr4
+    mov     eax, cr4
+    bts     eax, 12
+    mov     cr4, eax
+
+    ; done
+    jmp     SetCr3
+
+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))
-- 
2.43.0



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#114689): https://edk2.groups.io/g/devel/message/114689
Mute This Topic: https://groups.io/mt/104029639/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Laszlo Ersek]

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]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Gerd Hoffmann]

  Hi,

> > +    ;
> > +    ; use 5-level paging with gigabyte pages
> > +    ;

> > +    ; 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.

Correct.

> That one sentence in a comment (which would have cost you 5 seconds)

Well, there is a comment saying it's 5-level paging with gigabyte pages
(quoted above).  I'll add more details.

> (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.

I think rename "2M" to "LARGEPAGE" would be better here.  The page
directory bit essentially says "stop page table walk here, there are no
page tables/page directories below this".  When set in a 4-level page
directory you'll get a 2M page, when set in a 3-level page directory
you'll get a 1G page.

> > +    ; 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.

Not fully sure.  The 4-level paging code applies edx on all levels, so I
did the same for 5-level paging.

Tom?

> - jumping to the SetCr3 label from here omits the
> "SevClearPageEncMaskForGhcbPage" and "TdxPostBuildPageTables" pieces of
> logic.

Good catch.  That is a bug.

take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#114767): https://edk2.groups.io/g/devel/message/114767
Mute This Topic: https://groups.io/mt/104029639/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Laszlo Ersek]

On 1/30/24 10:14, Gerd Hoffmann wrote:

>> (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.
> 
> I think rename "2M" to "LARGEPAGE" would be better here.  The page
> directory bit essentially says "stop page table walk here, there are no
> page tables/page directories below this".  When set in a 4-level page
> directory you'll get a 2M page, when set in a 3-level page directory
> you'll get a 1G page.

*Great* idea; please do that! (Please also include this great
explanation near the changed macro, in the source code!)

Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#114804): https://edk2.groups.io/g/devel/message/114804
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]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH 3/3] OvmfPkg/PlatformInitLib: add 5-level paging support

[Gerd Hoffmann]

Adjust physical address space logic for la57 mode (5-level paging).
With a larger logical address space we can identity-map a larger
physical address space.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 OvmfPkg/Library/PlatformInitLib/MemDetect.c | 57 ++++++++++++++-------
 1 file changed, 38 insertions(+), 19 deletions(-)

diff --git a/OvmfPkg/Library/PlatformInitLib/MemDetect.c b/OvmfPkg/Library/PlatformInitLib/MemDetect.c
index f042517bb64a..b882d9b82fa8 100644
--- a/OvmfPkg/Library/PlatformInitLib/MemDetect.c
+++ b/OvmfPkg/Library/PlatformInitLib/MemDetect.c
@@ -628,11 +628,12 @@ PlatformAddressWidthFromCpuid (
   IN     BOOLEAN                QemuQuirk
   )
 {
-  UINT32   RegEax, RegEbx, RegEcx, RegEdx, Max;
-  UINT8    PhysBits;
-  CHAR8    Signature[13];
-  BOOLEAN  Valid         = FALSE;
-  BOOLEAN  Page1GSupport = FALSE;
+  UINT32    RegEax, RegEbx, RegEcx, RegEdx, Max;
+  UINT8     PhysBits;
+  CHAR8     Signature[13];
+  IA32_CR4  Cr4;
+  BOOLEAN   Valid         = FALSE;
+  BOOLEAN   Page1GSupport = FALSE;
 
   ZeroMem (Signature, sizeof (Signature));
 
@@ -670,30 +671,48 @@ PlatformAddressWidthFromCpuid (
     }
   }
 
+  Cr4.UintN = AsmReadCr4 ();
+
   DEBUG ((
     DEBUG_INFO,
-    "%a: Signature: '%a', PhysBits: %d, QemuQuirk: %a, Valid: %a\n",
+    "%a: Signature: '%a', PhysBits: %d, QemuQuirk: %a, la57: %a, Valid: %a\n",
     __func__,
     Signature,
     PhysBits,
     QemuQuirk ? "On" : "Off",
+    Cr4.Bits.LA57 ? "On" : "Off",
     Valid ? "Yes" : "No"
     ));
 
   if (Valid) {
-    if (PhysBits > 46) {
-      /*
-       * Avoid 5-level paging altogether for now, which limits
-       * PhysBits to 48.  Also avoid using address bit 48, due to sign
-       * extension we can't identity-map these addresses (and lots of
-       * places in edk2 assume we have everything identity-mapped).
-       * So the actual limit is 47.
-       *
-       * Also some older linux kernels apparently have problems handling
-       * phys-bits > 46 correctly, so use that as limit.
-       */
-      DEBUG ((DEBUG_INFO, "%a: limit PhysBits to 46 (avoid 5-level paging)\n", __func__));
-      PhysBits = 46;
+    /*
+     * Due to the sign extension we can use only the lower half of the
+     * virtual address space to identity-map physical address space,
+     * which gives us a 47 bit wide address space with 4 paging levels
+     * and a 56 bit wide address space with 5 paging levels.
+     */
+    if (Cr4.Bits.LA57) {
+      if (PhysBits > 48) {
+        /*
+         * Some Intel CPUs support 5-level paging, have more than 48
+         * phys-bits but support only 4-level EPT, which effectively
+         * limits guest phys-bits to 48.  Until we have some way to
+         * communicate that limitation from hypervisor to guest limit
+         * phys-bits to 48 unconditionally.
+         */
+        DEBUG ((DEBUG_INFO, "%a: limit PhysBits to 48 (5-level paging)\n", __func__));
+        PhysBits = 48;
+      }
+    } else {
+      if (PhysBits > 46) {
+        /*
+         * Some older linux kernels apparently have problems handling
+         * phys-bits > 46 correctly, so use that instead of 47 as
+         * limit.
+         */
+        DEBUG ((DEBUG_INFO, "%a: limit PhysBits to 46 (4-level paging)\n", __func__));
+        PhysBits = 46;
+      }
     }
 
     if (!Page1GSupport && (PhysBits > 40)) {
-- 
2.43.0



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#114688): https://edk2.groups.io/g/devel/message/114688
Mute This Topic: https://groups.io/mt/104029638/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Laszlo Ersek]

On 1/29/24 13:02, Gerd Hoffmann wrote:
> Adjust physical address space logic for la57 mode (5-level paging).
> With a larger logical address space we can identity-map a larger
> physical address space.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  OvmfPkg/Library/PlatformInitLib/MemDetect.c | 57 ++++++++++++++-------
>  1 file changed, 38 insertions(+), 19 deletions(-)
> 
> diff --git a/OvmfPkg/Library/PlatformInitLib/MemDetect.c b/OvmfPkg/Library/PlatformInitLib/MemDetect.c
> index f042517bb64a..b882d9b82fa8 100644
> --- a/OvmfPkg/Library/PlatformInitLib/MemDetect.c
> +++ b/OvmfPkg/Library/PlatformInitLib/MemDetect.c
> @@ -628,11 +628,12 @@ PlatformAddressWidthFromCpuid (
>    IN     BOOLEAN                QemuQuirk
>    )
>  {
> -  UINT32   RegEax, RegEbx, RegEcx, RegEdx, Max;
> -  UINT8    PhysBits;
> -  CHAR8    Signature[13];
> -  BOOLEAN  Valid         = FALSE;
> -  BOOLEAN  Page1GSupport = FALSE;
> +  UINT32    RegEax, RegEbx, RegEcx, RegEdx, Max;
> +  UINT8     PhysBits;
> +  CHAR8     Signature[13];
> +  IA32_CR4  Cr4;
> +  BOOLEAN   Valid         = FALSE;
> +  BOOLEAN   Page1GSupport = FALSE;

Initialization of locals breaks the edk2 coding conventions.

... Well, I now realize this is preexistent code, only shown as
"different" due to visual realignment. Ignore.

>  
>    ZeroMem (Signature, sizeof (Signature));
>  
> @@ -670,30 +671,48 @@ PlatformAddressWidthFromCpuid (
>      }
>    }
>  
> +  Cr4.UintN = AsmReadCr4 ();
> +
>    DEBUG ((
>      DEBUG_INFO,
> -    "%a: Signature: '%a', PhysBits: %d, QemuQuirk: %a, Valid: %a\n",
> +    "%a: Signature: '%a', PhysBits: %d, QemuQuirk: %a, la57: %a, Valid: %a\n",
>      __func__,
>      Signature,
>      PhysBits,
>      QemuQuirk ? "On" : "Off",
> +    Cr4.Bits.LA57 ? "On" : "Off",
>      Valid ? "Yes" : "No"
>      ));
>  
>    if (Valid) {
> -    if (PhysBits > 46) {
> -      /*
> -       * Avoid 5-level paging altogether for now, which limits
> -       * PhysBits to 48.  Also avoid using address bit 48, due to sign
> -       * extension we can't identity-map these addresses (and lots of
> -       * places in edk2 assume we have everything identity-mapped).
> -       * So the actual limit is 47.
> -       *
> -       * Also some older linux kernels apparently have problems handling
> -       * phys-bits > 46 correctly, so use that as limit.
> -       */
> -      DEBUG ((DEBUG_INFO, "%a: limit PhysBits to 46 (avoid 5-level paging)\n", __func__));
> -      PhysBits = 46;
> +    /*
> +     * Due to the sign extension we can use only the lower half of the
> +     * virtual address space to identity-map physical address space,
> +     * which gives us a 47 bit wide address space with 4 paging levels
> +     * and a 56 bit wide address space with 5 paging levels.
> +     */
> +    if (Cr4.Bits.LA57) {
> +      if (PhysBits > 48) {
> +        /*
> +         * Some Intel CPUs support 5-level paging, have more than 48
> +         * phys-bits but support only 4-level EPT, which effectively
> +         * limits guest phys-bits to 48.  Until we have some way to
> +         * communicate that limitation from hypervisor to guest limit

(1) Very nice comment, but please insert a comma "," between the words
"guest" and "limit".

> +         * phys-bits to 48 unconditionally.
> +         */
> +        DEBUG ((DEBUG_INFO, "%a: limit PhysBits to 48 (5-level paging)\n", __func__));
> +        PhysBits = 48;
> +      }
> +    } else {
> +      if (PhysBits > 46) {
> +        /*
> +         * Some older linux kernels apparently have problems handling
> +         * phys-bits > 46 correctly, so use that instead of 47 as
> +         * limit.
> +         */
> +        DEBUG ((DEBUG_INFO, "%a: limit PhysBits to 46 (4-level paging)\n", __func__));
> +        PhysBits = 46;
> +      }
>      }
>  
>      if (!Page1GSupport && (PhysBits > 40)) {

Reviewed-by: Laszlo Ersek <lersek@redhat.com>



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#114747): https://edk2.groups.io/g/devel/message/114747
Mute This Topic: https://groups.io/mt/104029638/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-