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; Thu, 26 Sep 2019 11:01:44 -0700 Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.14]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id B4E5E4FCDA; Thu, 26 Sep 2019 18:01:43 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-49.rdu2.redhat.com [10.10.120.49]) by smtp.corp.redhat.com (Postfix) with ESMTP id A5BA95D9C9; Thu, 26 Sep 2019 18:01:42 +0000 (UTC) Subject: Re: [PATCH 2/2] UefiCpuPkg/PiSmmCpuDxe: Honor the physical address size in CpuInfo HOB To: Ray Ni , devel@edk2.groups.io Cc: Eric Dong References: <20190926000904.187532-1-ray.ni@intel.com> <20190926000904.187532-3-ray.ni@intel.com> From: "Laszlo Ersek" Message-ID: Date: Thu, 26 Sep 2019 20:01:41 +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: <20190926000904.187532-3-ray.ni@intel.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.14 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.30]); Thu, 26 Sep 2019 18:01:43 +0000 (UTC) Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 09/26/19 02:09, Ray Ni wrote: > Today's logic is to only enable 5-level paging when CPU supports it > and the maximum physical address size > 48. > The patch changes to get the maximum physical address size firstly > from CpuInfo HOB then CPUID result. It aligns to the behavior of > existing code that builds the page table. > > Signed-off-by: Ray Ni > Cc: Eric Dong > Cc: Laszlo Ersek > --- > UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c | 39 ++++++++----------------- > 1 file changed, 12 insertions(+), 27 deletions(-) > > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c > index b8e95bf6ed..54c17522ff 100644 > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c > @@ -63,45 +63,25 @@ Is1GPageSupport ( > } > > /** > - The routine returns TRUE when CPU supports it (CPUID[7,0].ECX.BIT[16] is set) and > - the max physical address bits is bigger than 48. Because 4-level paging can support > - to address physical address up to 2^48 - 1, there is no need to enable 5-level paging > - with max physical address bits <= 48. > + The routine returns TRUE when CPU supports 5-level paging. (CPUID[7,0].ECX.BIT[16] is set). > > - @retval TRUE 5-level paging enabling is needed. > - @retval FALSE 5-level paging enabling is not needed. > + @retval TRUE 5-level paging is supported. > + @retval FALSE 5-level paging is not supported. > **/ > BOOLEAN > -Is5LevelPagingNeeded ( > +Is5LevelPagingSupported ( > VOID > ) > { > - CPUID_VIR_PHY_ADDRESS_SIZE_EAX VirPhyAddressSize; > CPUID_STRUCTURED_EXTENDED_FEATURE_FLAGS_ECX ExtFeatureEcx; > - UINT32 MaxExtendedFunctionId; > > - AsmCpuid (CPUID_EXTENDED_FUNCTION, &MaxExtendedFunctionId, NULL, NULL, NULL); > - if (MaxExtendedFunctionId >= CPUID_VIR_PHY_ADDRESS_SIZE) { > - AsmCpuid (CPUID_VIR_PHY_ADDRESS_SIZE, &VirPhyAddressSize.Uint32, NULL, NULL, NULL); > - } else { > - VirPhyAddressSize.Bits.PhysicalAddressBits = 36; > - } > AsmCpuidEx ( > CPUID_STRUCTURED_EXTENDED_FEATURE_FLAGS, > CPUID_STRUCTURED_EXTENDED_FEATURE_FLAGS_SUB_LEAF_INFO, > NULL, NULL, &ExtFeatureEcx.Uint32, NULL > ); > - DEBUG (( > - DEBUG_INFO, "PhysicalAddressBits = %d, 5LPageTable = %d.\n", > - VirPhyAddressSize.Bits.PhysicalAddressBits, ExtFeatureEcx.Bits.FiveLevelPage > - )); > - > - if (VirPhyAddressSize.Bits.PhysicalAddressBits > 4 * 9 + 12) { > - ASSERT (ExtFeatureEcx.Bits.FiveLevelPage == 1); > - return TRUE; > - } else { > - return FALSE; > - } > + > + return (BOOLEAN) (ExtFeatureEcx.Bits.FiveLevelPage == 1); > } > > /** > @@ -351,8 +331,13 @@ SmmInitPageTable ( > > mCpuSmmRestrictedMemoryAccess = PcdGetBool (PcdCpuSmmRestrictedMemoryAccess); > m1GPageTableSupport = Is1GPageSupport (); > - m5LevelPagingNeeded = Is5LevelPagingNeeded (); > mPhysicalAddressBits = GetPhysicalAddressBits (); > + // > + // Enable 5 level paging when CPU supports it and the max physical address bits is bigger than 48. > + // Because 4-level paging can support to address physical address up to 2^48 - 1, there is no need > + // to enable 5-level paging with max physical address bits <= 48. > + // > + m5LevelPagingNeeded = Is5LevelPagingSupported () && (mPhysicalAddressBits > 48); I think we should optimize this a bit: if (mPhysicalAddressBits <= 48), then we shouldn't call Is5LevelPagingSupported() at all. Therefore, I suggest reversing the order of the sub-conditions: m5LevelPagingNeeded = (mPhysicalAddressBits > 48) && Is5LevelPagingSupported (); That saves an AsmCpuidEx() call, at least if the CPU HOB tells us SizeOfMemorySpace. Otherwise, the patch looks OK to me. If you disagree, I'm OK giving R-b for the patch as-is. Thanks Laszlo > PatchInstructionX86 (gPatch5LevelPagingNeeded, m5LevelPagingNeeded, 1); > DEBUG ((DEBUG_INFO, "5LevelPaging Needed - %d\n", m5LevelPagingNeeded)); > DEBUG ((DEBUG_INFO, "1GPageTable Support - %d\n", m1GPageTableSupport)); >