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

["Ni, Ray" <ray.ni@intel.com>]

Laszlo,
I agree with your comments.
I will:
1. separate the patch into 2
2. remove the unneeded "else" after getting from HOB.

Thanks,
Ray

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo Ersek
> Sent: Thursday, September 26, 2019 10:54 AM
> To: Ni, Ray <ray.ni@intel.com>; devel@edk2.groups.io
> Cc: Dong, Eric <eric.dong@intel.com>
> Subject: Re: [edk2-devel] [PATCH 1/2] UefiCpuPkg/PiSmmCpu: Remove hard code when getting physical line size
> 
> 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));
> >
> 
> 
>