Laszlo,
I agree to reverse the order of the two conditions. thanks for the suggestions.
Thanks,
Ray
> -----Original Message-----
> From: Laszlo Ersek <ler...@redhat.com>
> Sent: Thursday, September 26, 2019 11:02 AM
> To: Ni, Ray <ray...@intel.com>; devel@edk2.groups.io
> Cc: Dong, Eric <eric.d...@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...@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 (#48128): https://edk2.groups.io/g/devel/message/48128
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]
-=-=-=-=-=-=-=-=-=-=-=-
