From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: redhat.com, ip: 209.132.183.28, mailfrom: lersek@redhat.com) Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by groups.io with SMTP; Tue, 23 Jul 2019 02:15:03 -0700 Received: from smtp.corp.redhat.com (int-mx07.intmail.prod.int.phx2.redhat.com [10.5.11.22]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id D309A59455; Tue, 23 Jul 2019 09:15:02 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-117-52.ams2.redhat.com [10.36.117.52]) by smtp.corp.redhat.com (Postfix) with ESMTP id C6D911001B36; Tue, 23 Jul 2019 09:15:01 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH 1/4] UefiCpuPkg/MpInitLib: Enable 5-level paging for AP when BSP's enabled To: devel@edk2.groups.io, ray.ni@intel.com Cc: Eric Dong References: <20190722081547.56100-1-ray.ni@intel.com> <20190722081547.56100-2-ray.ni@intel.com> From: "Laszlo Ersek" Message-ID: Date: Tue, 23 Jul 2019 11:15:00 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <20190722081547.56100-2-ray.ni@intel.com> X-Scanned-By: MIMEDefang 2.84 on 10.5.11.22 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.39]); Tue, 23 Jul 2019 09:15:03 +0000 (UTC) Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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 > Cc: Eric Dong > Cc: Laszlo Ersek > --- > 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.
> + Copyright (c) 2016 - 2019, Intel Corporation. All rights reserved.
> 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.
> +; Copyright (c) 2015 - 2019, Intel Corporation. All rights reserved.
> ; 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.
> +; Copyright (c) 2015 - 2019, Intel Corporation. All rights reserved.
> ; 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 > > ; >