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 <ray...@intel.com>
> Cc: Eric Dong <eric.d...@intel.com>
> Cc: Laszlo Ersek <ler...@redhat.com>
> ---
> 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));
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#48125): https://edk2.groups.io/g/devel/message/48125
Mute This Topic: https://groups.io/mt/34293643/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
