Re: [PATCH 1/2] UefiCpuPkg/PiSmmCpu: Remove hard code when getting physical line size

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

Hi Ray,

On 09/26/19 02:09, Ray Ni wrote:
> The code replaces the hard code with macros defined in
> MdePkg\Include\Register\Intel\CpuId.h.
> 
> No functionality impact.
> 
> 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 | 24 +++++++++++-------------
>  1 file changed, 11 insertions(+), 13 deletions(-)
> 
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
> index e5c4788c13..b8e95bf6ed 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
> @@ -151,30 +151,28 @@ GetSubEntriesNum (
>    @return the maximum support address.
>  **/
>  UINT8
> -CalculateMaximumSupportAddress (
> +GetPhysicalAddressBits (
>    VOID
>    )
>  {
> -  UINT32                                        RegEax;
> -  UINT8                                         PhysicalAddressBits;
> -  VOID                                          *Hob;
> +  CPUID_VIR_PHY_ADDRESS_SIZE_EAX              VirPhyAddressSize;
> +  UINT32                                      MaxExtendedFunctionId;
> +  VOID                                        *Hob;
>  
>    //
>    // Get physical address bits supported.
>    //
>    Hob = GetFirstHob (EFI_HOB_TYPE_CPU);
>    if (Hob != NULL) {
> -    PhysicalAddressBits = ((EFI_HOB_CPU *) Hob)->SizeOfMemorySpace;
> +    return ((EFI_HOB_CPU *) Hob)->SizeOfMemorySpace;
>    } else {
> -    AsmCpuid (0x80000000, &RegEax, NULL, NULL, NULL);
> -    if (RegEax >= 0x80000008) {
> -      AsmCpuid (0x80000008, &RegEax, NULL, NULL, NULL);
> -      PhysicalAddressBits = (UINT8) RegEax;
> -    } else {
> -      PhysicalAddressBits = 36;
> +    AsmCpuid (CPUID_EXTENDED_FUNCTION, &MaxExtendedFunctionId, NULL, NULL, NULL);
> +    if (MaxExtendedFunctionId < CPUID_VIR_PHY_ADDRESS_SIZE) {
> +      return 36;
>      }
> +    AsmCpuid (CPUID_VIR_PHY_ADDRESS_SIZE, &VirPhyAddressSize.Uint32, NULL, NULL, NULL);
> +    return (UINT8) VirPhyAddressSize.Bits.PhysicalAddressBits;
>    }
> -  return PhysicalAddressBits;
>  }

I would prefer if you separated
- the replacement of the magic constants with macros,
- from reorganizing the control flow.

Even if we keep both changes in the same patch, the resultant control
flow is not optimal. Where you return SizeOfMemorySpace, there should be
no "else" branch after -- the rest of the code should be un-indented by
one level, instead.

Thanks
Laszlo

>  
>  /**
> @@ -354,7 +352,7 @@ SmmInitPageTable (
>    mCpuSmmRestrictedMemoryAccess = PcdGetBool (PcdCpuSmmRestrictedMemoryAccess);
>    m1GPageTableSupport           = Is1GPageSupport ();
>    m5LevelPagingNeeded           = Is5LevelPagingNeeded ();
> -  mPhysicalAddressBits          = CalculateMaximumSupportAddress ();
> +  mPhysicalAddressBits          = GetPhysicalAddressBits ();
>    PatchInstructionX86 (gPatch5LevelPagingNeeded, m5LevelPagingNeeded, 1);
>    DEBUG ((DEBUG_INFO, "5LevelPaging Needed             - %d\n", m5LevelPagingNeeded));
>    DEBUG ((DEBUG_INFO, "1GPageTable Support             - %d\n", m1GPageTableSupport));
>