Re: [edk2-devel] [PATCH 1/1] MdeModulePkg/VirtualMemory: drop 5-level paging assert

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

On 1/24/24 16:49, Gerd Hoffmann wrote:
> The ResetVector decides at runtime (depending in CPU capabilities)

(1) s/depending in/dependent on/

> whenever it uses 5-level paging or not.  Firmware builds with 5-level
> paging enabled (PcdUse5LevelPageTable=TRUE) may run run in 4-level

(2) typo: "run run"

(3) Suggest the following wording improvement:

  Firmware that intends to enable 5-level paging may run in 4-level ...

Justification:

- the expression "firmware builds" suggests a fixed-at-build PCD, but
this PCD is declared as [PcdsFixedAtBuild, PcdsPatchableInModule,
PcdsDynamic, PcdsDynamicEx]. So it may be a runtime decision even to
*request* 5-level paging.

- "intends" rather than "enabled" because the PCD is just an expression
of intent. Per DEC file:

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

Back to the patch:

On 1/24/24 16:49, Gerd Hoffmann wrote:
> paging mode.  The code handles that just fine, by looking at the la57
> bit in cr4 instead of checking PcdUse5LevelPageTable.
>
> The ASSERT is not correct,

I agree, but...

> remove it.

I have a better idea, I believe:

>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c b/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
> index 980c2002d4f5..575e396eeffd 100644
> --- a/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
> +++ b/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
> @@ -745,7 +745,6 @@ CreateIdentityMappingPageTables (
>      //
>      Cr4.UintN         = AsmReadCr4 ();
>      Page5LevelSupport = (Cr4.Bits.LA57 != 0);
> -    ASSERT (PcdGetBool (PcdUse5LevelPageTable) == Page5LevelSupport);

In my opinion, what the code originally wanted to express here was not
*equivalence*, but *implication* (that is, only one direction, not both
directions). The idea is, if we find the LA57 bit set, then the platform
must have requested 5-level paging. As a logical predicate:

  Page5LevelSupport ==> PcdGetBool (PcdUse5LevelPageTable)

In C, we don't have an "implies" operator, but

  A ==> B

is just

  !(A) || (B)

(4) Therefore I suggest the following update (rather than deletion):

    ASSERT (!Page5LevelSupport || PcdGetBool (PcdUse5LevelPageTable));

Alternatively (equivalently):

    if (Page5LevelSupport) {
      ASSERT (PcdGetBool (PcdUse5LevelPageTable));
    }

This variant adds a bit of useless code to RELEASE builds, but in those
builds, the empty body of the "if" should cause the "if" itself to be
optimized away, too. So pick whichever you like.

(If the assertion fails, then the first variant's error message in the
log is more informative, FWIW!)

(5) At the same time, the subject should become "*fix* 5-level paging
assert".

(6) "Page5LevelSupport" is a misnomer to begin with! In my opinion, the
right name would be "Page5LevelEnabled"; after all, it comes from CR4
(it's active!), not from some MSR. Please consider renaming the variable
in a separate patch (ahead of fixing the logic bug).

>    } else {
>      //
>      // If cpu runs in 32bit protected mode PEI, Page table Level in DXE is decided by PCD and feature capability.

Thanks
Laszlo



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