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

["Ni, Ray" <ray.ni@intel.com>]

> -----Original Message-----
> From: Laszlo Ersek <lersek@redhat.com>
> Sent: Tuesday, July 23, 2019 5:15 PM
> To: devel@edk2.groups.io; Ni, Ray <ray.ni@intel.com>
> Cc: Dong, Eric <eric.dong@intel.com>
> Subject: Re: [edk2-devel] [PATCH 1/4] UefiCpuPkg/MpInitLib: Enable 5-level
> paging for AP when BSP's enabled
> 
> 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?

I am not sure about that really.
Since Intel and AMD has magically aligned each bit in control registers for
quite a long period, I don't see big risk here for 5-level paging support on
future AMD processors😊

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

Sure. Will address them in V2.

> 
> > 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"?

Originally I had some idea to make sure "0-9" in the beginning.
Now you ask, as long as build passes I don't see reason to have 0 as prefix.

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

Sure. Will address them in V2.

> 
> (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.
> 
Sure. Will address them in V2.
> Thanks
> Laszlo
> 
> > +
> >      mov        cr4, eax
> >
> >      ;
> >