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);
PatchInstructionX86 (gPatch5LevelPagingNeeded, m5LevelPagingNeeded, 1);
DEBUG ((DEBUG_INFO, "5LevelPaging Needed - %d\n", m5LevelPagingNeeded));
DEBUG ((DEBUG_INFO, "1GPageTable Support - %d\n", m1GPageTableSupport));
--
2.21.0.windows.1
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#48059): https://edk2.groups.io/g/devel/message/48059
Mute This Topic: https://groups.io/mt/34293643/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
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));
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#48125): https://edk2.groups.io/g/devel/message/48125
Mute This Topic: https://groups.io/mt/34293643/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
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));
> >
-=-=-=-=-=-=-=-=-=-=-=-
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/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
© 2016 - 2026 Red Hat, Inc.