Re: [PATCH 2/2] UefiCpuPkg/PiSmmCpuDxe: Honor the physical address size in CpuInfo HOB

["Laszlo Ersek" <lersek@redhat.com>]

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.ni@intel.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Laszlo Ersek <lersek@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));
>