> -----Original Message-----
> From: Laszlo Ersek <ler...@redhat.com>
> Sent: Tuesday, July 23, 2019 5:15 PM
> To: devel@edk2.groups.io; Ni, Ray <ray...@intel.com>
> Cc: Dong, Eric <eric.d...@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...@intel.com>
> > Cc: Eric Dong <eric.d...@intel.com>
> > Cc: Laszlo Ersek <ler...@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
> >
> > ;
> >
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#44287): https://edk2.groups.io/g/devel/message/44287
Mute This Topic: https://groups.io/mt/32556532/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
