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

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

Laszlo,
I agree to reverse the order of the two conditions. thanks for the suggestions.

Thanks,
Ray

> -----Original Message-----
> From: Laszlo Ersek <lersek@redhat.com>
> Sent: Thursday, September 26, 2019 11:02 AM
> To: Ni, Ray <ray.ni@intel.com>; devel@edk2.groups.io
> Cc: Dong, Eric <eric.dong@intel.com>
> Subject: Re: [PATCH 2/2] UefiCpuPkg/PiSmmCpuDxe: Honor the physical address size in CpuInfo HOB
> 
> 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));
> >