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; Wed, 07 Aug 2019 13:18:12 -0700 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 1283A8763B; Wed, 7 Aug 2019 20:18:12 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (unknown [10.36.118.2]) by smtp.corp.redhat.com (Postfix) with ESMTP id 032485D70D; Wed, 7 Aug 2019 20:18:10 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH v4 0/8] Support 5-level paging in DXE long mode To: ray.ni@intel.com References: <20190801095831.274356-1-ray.ni@intel.com> From: "Laszlo Ersek" Cc: devel@edk2.groups.io Message-ID: <43246e53-9f23-9af6-4cc1-c57021fcde02@redhat.com> Date: Wed, 7 Aug 2019 22:18:10 +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: <20190801095831.274356-1-ray.ni@intel.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.15 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.26]); Wed, 07 Aug 2019 20:18:12 +0000 (UTC) Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Hi Ray, On 08/01/19 11:58, Ni, Ray wrote: > v4: > Move all files under UefiCpuPkg/Include/Register/ to MdePkg. > NOTE: Changes like updating BaseLib.h to include Cpuid.h is not > included. Sorry about the delay. I've wanted to regression-test this series after Eric approves it. That happened on 2 Aug 2019, but I've been away until this morning. So I'm coming to the series now. (1) A reminder: please make sure that you get Reviewed-by or Acked-by for the MdePkg and MdeModulePkg patches too, from their respective package maintainers. In particular, patch #5 modifies two packages at once, so the maintainers of both packages will have to approve it. The same applies to patch #7. (2) The series causes a build failure for OVMF: > OvmfPkg/PlatformPei/FeatureControl.c:14:35: fatal error: Register/Msr/Core2Msr.h: No such file or directory > #include That's because patch #7 moves UefiCpuPkg/Include/Register/Msr/Core2Msr.h under MdePkg, without replacement under UefiCpuPkg. Can you please *prepend* the following OvmfPkg patch to the series: > diff --git a/OvmfPkg/PlatformPei/FeatureControl.c b/OvmfPkg/PlatformPei/FeatureControl.c > index 837da2119adb..1a9d75022f48 100644 > --- a/OvmfPkg/PlatformPei/FeatureControl.c > +++ b/OvmfPkg/PlatformPei/FeatureControl.c > @@ -11,7 +11,7 @@ > #include > #include > #include > -#include > +#include > > #include "Platform.h" > > @@ -37,7 +37,7 @@ WriteFeatureControl ( > IN OUT VOID *WorkSpace > ) > { > - AsmWriteMsr64 (MSR_CORE2_FEATURE_CONTROL, mFeatureControlValue); > + AsmWriteMsr64 (MSR_IA32_FEATURE_CONTROL, mFeatureControlValue); > } > > /** This change is a no-op, behaviorally (both macros expand to 0x0000003A), but MSR_IA32_FEATURE_CONTROL matches the original intent in OvmfPkg better -- the code is not Core2 specific --, plus it allows your series to build, without any changes to the v4 patches #1 through #8. (3) With said OvmfPkg/PlatformPei update in place, and your v4 patches applied on top, I couldn't find any regressions. Please post a v5 with the OvmfPkg/PlatformPei patch at the front, and -- if you change *nothing* in the v4 patches --, please add: Regression-tested-by: Laszlo Ersek to the v5 patches #2 through #9. Thanks! Laszlo > v3: > Move UefiCpuPkg/Include/Register/Cpuid.h to MdePkg/Include/Register/Intel/ directory. > Create UefiCpuPkg/Include/Register/Cpuid.h to include MdePkg/Include/Register/Intel/Cpuid.h. > NOTE: > Changes like moving Amd/Cpuid.h to MdePkg is not included. > Changes like updating BaseLib.h to include Cpuid.h is not included. > > v2: > Refined the patch according to reviewers' all comments except: > 0A0h cannot be changed to A0h or build fails. > A big change in this patch is Cpuid.h is moved from UefiCpuPkg to MdePkg. > The move is based on real requirement when certain modules that cannot > depend on UefiCpuPkg but needs to reference structures defined in SDM. > > Ray Ni (8): > UefiCpuPkg/MpInitLib: Enable 5-level paging for AP when BSP's enabled > UefiCpuPkg/CpuDxe: Remove unnecessary macros > UefiCpuPkg/CpuDxe: Support parsing 5-level page table > MdeModulePkg/DxeIpl: Introduce PCD PcdUse5LevelPageTable > MdePkg/Cpuid.h: Move Cpuid.h from UefiCpuPkg to MdePkg > MdeModulePkg/DxeIpl: Create 5-level page table for long mode > UefiCpuPkg|MdePkg: Move Register/ folder to MdePkg/Include/ > UefiCpuPkg: Update code to include register definitions from MdePkg > > MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf | 1 + > .../Core/DxeIplPeim/X64/VirtualMemory.c | 229 +- > MdeModulePkg/MdeModulePkg.dec | 7 + > MdeModulePkg/MdeModulePkg.uni | 8 + > .../Include/Register/Amd/Cpuid.h | 0 > .../Include/Register/Amd/Fam17Msr.h | 0 > .../Include/Register/Amd/Msr.h | 4 +- > .../Register/Intel}/ArchitecturalMsr.h | 8 +- > .../Include/Register/Intel}/Cpuid.h | 6 +- > .../Include/Register/Intel}/LocalApic.h | 6 +- > .../Include/Register/Intel}/Microcode.h | 6 +- > MdePkg/Include/Register/Intel/Msr.h | 44 + > .../Include/Register/Intel}/Msr/AtomMsr.h | 4 +- > .../Register/Intel}/Msr/BroadwellMsr.h | 4 +- > .../Include/Register/Intel}/Msr/Core2Msr.h | 4 +- > .../Include/Register/Intel}/Msr/CoreMsr.h | 4 +- > .../Include/Register/Intel}/Msr/GoldmontMsr.h | 4 +- > .../Register/Intel}/Msr/GoldmontPlusMsr.h | 4 +- > .../Include/Register/Intel}/Msr/HaswellEMsr.h | 4 +- > .../Include/Register/Intel}/Msr/HaswellMsr.h | 4 +- > .../Register/Intel}/Msr/IvyBridgeMsr.h | 4 +- > .../Include/Register/Intel}/Msr/NehalemMsr.h | 4 +- > .../Include/Register/Intel}/Msr/P6Msr.h | 4 +- > .../Include/Register/Intel}/Msr/Pentium4Msr.h | 2 +- > .../Include/Register/Intel}/Msr/PentiumMMsr.h | 2 +- > .../Include/Register/Intel}/Msr/PentiumMsr.h | 2 +- > .../Register/Intel}/Msr/SandyBridgeMsr.h | 2 +- > .../Register/Intel}/Msr/SilvermontMsr.h | 2 +- > .../Include/Register/Intel}/Msr/SkylakeMsr.h | 2 +- > .../Include/Register/Intel}/Msr/Xeon5600Msr.h | 2 +- > .../Include/Register/Intel}/Msr/XeonDMsr.h | 2 +- > .../Include/Register/Intel}/Msr/XeonE7Msr.h | 2 +- > .../Include/Register/Intel}/Msr/XeonPhiMsr.h | 2 +- > .../Register/Intel}/SmramSaveStateMap.h | 6 +- > .../Include/Register/Intel}/StmApi.h | 12 +- > .../Register/Intel}/StmResourceDescriptor.h | 6 +- > .../Include/Register/Intel}/StmStatusCode.h | 6 +- > UefiCpuPkg/Application/Cpuid/Cpuid.c | 2 +- > UefiCpuPkg/CpuDxe/CpuDxe.h | 4 +- > UefiCpuPkg/CpuDxe/CpuPageTable.c | 63 +- > UefiCpuPkg/CpuDxe/CpuPageTable.h | 3 +- > UefiCpuPkg/CpuMpPei/CpuPaging.c | 6 +- > .../Include/Library/RegisterCpuFeaturesLib.h | 2 +- > .../Include/Library/SmmCpuFeaturesLib.h | 2 +- > UefiCpuPkg/Include/Protocol/SmMonitorInit.h | 4 +- > .../Include/Register/ArchitecturalMsr.h | 6565 +---------------- > UefiCpuPkg/Include/Register/Cpuid.h | 3990 +--------- > UefiCpuPkg/Include/Register/LocalApic.h | 175 +- > UefiCpuPkg/Include/Register/Microcode.h | 187 +- > UefiCpuPkg/Include/Register/Msr.h | 36 +- > .../Include/Register/SmramSaveStateMap.h | 179 +- > UefiCpuPkg/Include/Register/StmApi.h | 941 +-- > .../Library/BaseXApicLib/BaseXApicLib.c | 6 +- > .../BaseXApicX2ApicLib/BaseXApicX2ApicLib.c | 6 +- > .../CpuCommonFeaturesLib/CpuCommonFeatures.h | 6 +- > UefiCpuPkg/Library/MpInitLib/MpLib.c | 13 + > UefiCpuPkg/Library/MpInitLib/MpLib.h | 12 +- > UefiCpuPkg/Library/MpInitLib/X64/MpEqu.inc | 3 +- > UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm | 14 +- > UefiCpuPkg/Library/MtrrLib/MtrrLib.c | 4 +- > .../SmmCpuFeaturesLib/SmmCpuFeaturesLib.c | 6 +- > UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmStm.c | 6 +- > UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h | 4 +- > UefiCpuPkg/PiSmmCpuDxeSmm/SmramSaveState.c | 2 - > 64 files changed, 405 insertions(+), 12249 deletions(-) > rename {UefiCpuPkg => MdePkg}/Include/Register/Amd/Cpuid.h (100%) > rename {UefiCpuPkg => MdePkg}/Include/Register/Amd/Fam17Msr.h (100%) > rename {UefiCpuPkg => MdePkg}/Include/Register/Amd/Msr.h (78%) > copy {UefiCpuPkg/Include/Register => MdePkg/Include/Register/Intel}/ArchitecturalMsr.h (96%) > copy {UefiCpuPkg/Include/Register => MdePkg/Include/Register/Intel}/Cpuid.h (96%) > copy {UefiCpuPkg/Include/Register => MdePkg/Include/Register/Intel}/LocalApic.h (95%) > copy {UefiCpuPkg/Include/Register => MdePkg/Include/Register/Intel}/Microcode.h (95%) > create mode 100644 MdePkg/Include/Register/Intel/Msr.h > rename {UefiCpuPkg/Include/Register => MdePkg/Include/Register/Intel}/Msr/AtomMsr.h (96%) > rename {UefiCpuPkg/Include/Register => MdePkg/Include/Register/Intel}/Msr/BroadwellMsr.h (95%) > rename {UefiCpuPkg/Include/Register => MdePkg/Include/Register/Intel}/Msr/Core2Msr.h (96%) > rename {UefiCpuPkg/Include/Register => MdePkg/Include/Register/Intel}/Msr/CoreMsr.h (96%) > rename {UefiCpuPkg/Include/Register => MdePkg/Include/Register/Intel}/Msr/GoldmontMsr.h (96%) > rename {UefiCpuPkg/Include/Register => MdePkg/Include/Register/Intel}/Msr/GoldmontPlusMsr.h (96%) > rename {UefiCpuPkg/Include/Register => MdePkg/Include/Register/Intel}/Msr/HaswellEMsr.h (96%) > rename {UefiCpuPkg/Include/Register => MdePkg/Include/Register/Intel}/Msr/HaswellMsr.h (96%) > rename {UefiCpuPkg/Include/Register => MdePkg/Include/Register/Intel}/Msr/IvyBridgeMsr.h (96%) > rename {UefiCpuPkg/Include/Register => MdePkg/Include/Register/Intel}/Msr/NehalemMsr.h (96%) > rename {UefiCpuPkg/Include/Register => MdePkg/Include/Register/Intel}/Msr/P6Msr.h (95%) > rename {UefiCpuPkg/Include/Register => MdePkg/Include/Register/Intel}/Msr/Pentium4Msr.h (96%) > rename {UefiCpuPkg/Include/Register => MdePkg/Include/Register/Intel}/Msr/PentiumMMsr.h (96%) > rename {UefiCpuPkg/Include/Register => MdePkg/Include/Register/Intel}/Msr/PentiumMsr.h (95%) > rename {UefiCpuPkg/Include/Register => MdePkg/Include/Register/Intel}/Msr/SandyBridgeMsr.h (96%) > rename {UefiCpuPkg/Include/Register => MdePkg/Include/Register/Intel}/Msr/SilvermontMsr.h (96%) > rename {UefiCpuPkg/Include/Register => MdePkg/Include/Register/Intel}/Msr/SkylakeMsr.h (96%) > rename {UefiCpuPkg/Include/Register => MdePkg/Include/Register/Intel}/Msr/Xeon5600Msr.h (95%) > rename {UefiCpuPkg/Include/Register => MdePkg/Include/Register/Intel}/Msr/XeonDMsr.h (96%) > rename {UefiCpuPkg/Include/Register => MdePkg/Include/Register/Intel}/Msr/XeonE7Msr.h (96%) > rename {UefiCpuPkg/Include/Register => MdePkg/Include/Register/Intel}/Msr/XeonPhiMsr.h (96%) > copy {UefiCpuPkg/Include/Register => MdePkg/Include/Register/Intel}/SmramSaveStateMap.h (93%) > copy {UefiCpuPkg/Include/Register => MdePkg/Include/Register/Intel}/StmApi.h (96%) > rename {UefiCpuPkg/Include/Register => MdePkg/Include/Register/Intel}/StmResourceDescriptor.h (92%) > rename {UefiCpuPkg/Include/Register => MdePkg/Include/Register/Intel}/StmStatusCode.h (94%) >