Re: [edk2-devel] [PATCH 1/4] UefiCpuPkg/MpInitLib: Enable 5-level paging for AP when BSP's enabled

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

Later (after more feedback has been collected), I would like to
regression-test this series; for now, just some superficial comments:

On 07/22/19 10:15, Ni, Ray wrote:
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2008
> 
> MpInitLib is the library that's responsible to wake up APs to provide
> MP PPI and Protocol services.
> 
> The patch synchronizes BSP's CR4.LA57 to each AP's CR4.LA57.
> Without this change, AP may enter to GP fault when BSP's 5-level page
> table is set to AP during AP wakes up.
> 
> Signed-off-by: Ray Ni <ray.ni@intel.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> ---
>  UefiCpuPkg/Library/MpInitLib/MpLib.c          | 11 +++++++++++
>  UefiCpuPkg/Library/MpInitLib/MpLib.h          |  6 +++++-
>  UefiCpuPkg/Library/MpInitLib/X64/MpEqu.inc    |  3 ++-
>  UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm | 14 +++++++++++++-
>  4 files changed, 31 insertions(+), 3 deletions(-)
> 
> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> index 6f51bc4ebf..e4691315e9 100644
> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> @@ -790,6 +790,7 @@ FillExchangeInfoData (
>    volatile MP_CPU_EXCHANGE_INFO    *ExchangeInfo;
>    UINTN                            Size;
>    IA32_SEGMENT_DESCRIPTOR          *Selector;
> +  IA32_CR4                         Cr4;
>  
>    ExchangeInfo                  = CpuMpData->MpCpuExchangeInfo;
>    ExchangeInfo->Lock            = 0;
> @@ -814,6 +815,16 @@ FillExchangeInfoData (
>  
>    ExchangeInfo->InitializeFloatingPointUnitsAddress = (UINTN)InitializeFloatingPointUnits;
>  
> +  //
> +  // We can check either CPUID(7).ECX[bit16] or check CR4.LA57[bit12]
> +  //  to determin whether 5-Level Paging is enabled.
> +  // Using latter way is simpler because it also eliminates the needs to
> +  //  check whether platform wants to enable it.
> +  //
> +  Cr4.UintN = AsmReadCr4 ();

(1) Are the above checks (CPUID and CR4) interchangeable on AMD
processors too?

> +  ExchangeInfo->Enable5LevelPaging = (BOOLEAN) (Cr4.Bits.LA57 == 1);
> +  DEBUG ((DEBUG_INFO, "CpuMp: 5-Level Paging = %d\n", ExchangeInfo->Enable5LevelPaging));
> +
>    //
>    // Get the BSP's data of GDT and IDT
>    //

(2) Quite unimportant comment, but I might as well make it:

- In library code, it's best to refer to actual module names with
gEfiCallerBaseName. "CpuMp" isn't ideal in this log message.

- "ExchangeInfo->Enable5LevelPaging" is a UINTN; we shouldn't log it
with %d. The portable logging for UINTN is to cast it to UINT64, and
print it with %Lu. In this particular case, we know it's either 0 or 1,
so we can print it with %d too, but then we should cast it to INT32.

> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.h b/UefiCpuPkg/Library/MpInitLib/MpLib.h
> index f89037c59e..fa7d6b32e9 100644
> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.h
> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.h
> @@ -1,7 +1,7 @@
>  /** @file
>    Common header file for MP Initialize Library.
>  
> -  Copyright (c) 2016 - 2018, Intel Corporation. All rights reserved.<BR>
> +  Copyright (c) 2016 - 2019, Intel Corporation. All rights reserved.<BR>
>    SPDX-License-Identifier: BSD-2-Clause-Patent
>  
>  **/
> @@ -185,6 +185,10 @@ typedef struct {
>    UINT16                ModeTransitionSegment;
>    UINT32                ModeHighMemory;
>    UINT16                ModeHighSegment;
> +  //
> +  // Enable5LevelPaging indicates whether 5-level paging is enabled in long mode.
> +  //
> +  UINTN                 Enable5LevelPaging;
>  } MP_CPU_EXCHANGE_INFO;
>  
>  #pragma pack()
> diff --git a/UefiCpuPkg/Library/MpInitLib/X64/MpEqu.inc b/UefiCpuPkg/Library/MpInitLib/X64/MpEqu.inc
> index 467f54a860..58ef369342 100644
> --- a/UefiCpuPkg/Library/MpInitLib/X64/MpEqu.inc
> +++ b/UefiCpuPkg/Library/MpInitLib/X64/MpEqu.inc
> @@ -1,5 +1,5 @@
>  ;------------------------------------------------------------------------------ ;
> -; Copyright (c) 2015 - 2017, Intel Corporation. All rights reserved.<BR>
> +; Copyright (c) 2015 - 2019, Intel Corporation. All rights reserved.<BR>
>  ; SPDX-License-Identifier: BSD-2-Clause-Patent
>  ;
>  ; Module Name:
> @@ -40,3 +40,4 @@ ModeTransitionMemoryLocation        equ  LockLocation + 94h
>  ModeTransitionSegmentLocation       equ  LockLocation + 98h
>  ModeHighMemoryLocation              equ  LockLocation + 9Ah
>  ModeHighSegmentLocation             equ  LockLocation + 9Eh
> +Enable5LevelPagingLocation          equ  LockLocation + 0A0h

(3) Any particular reason for "0A0h" rather than just "A0h"?

> diff --git a/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm b/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
> index cea90f3d4d..b563c2ed3e 100644
> --- a/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
> +++ b/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
> @@ -1,5 +1,5 @@
>  ;------------------------------------------------------------------------------ ;
> -; Copyright (c) 2015 - 2018, Intel Corporation. All rights reserved.<BR>
> +; Copyright (c) 2015 - 2019, Intel Corporation. All rights reserved.<BR>
>  ; SPDX-License-Identifier: BSD-2-Clause-Patent
>  ;
>  ; Module Name:
> @@ -100,6 +100,18 @@ SkipEnableExecuteDisableBit:
>      ;
>      mov        eax, cr4
>      bts        eax, 5
> +
> +    mov        esi, Enable5LevelPagingLocation
> +    cmp        byte [ebx + esi], 0

(4) If we use a byte comparison here, why don't we make the field itself
a UINT8 (or even BOOLEAN)? The MP_CPU_EXCHANGE_INFO structure is packed.

(If we still want to use a whole UINTN for this purpose, then I think
the zero comparison should cover the whole field.)

> +    jz         SkipEnable5Paging
> +
> +    ;
> +    ; Enable 5 Level Paging
> +    ;
> +    bts        eax, 12                     ; Set LA57=1.
> +
> +SkipEnable5Paging:

(5) Not too important, but we might as well be consistent with the
naming elsewhere, and call this "SkipEnable5LevelPaging". Up to you.

Thanks
Laszlo

> +
>      mov        cr4, eax
>  
>      ;
>