* [PATCH 0/2] Honor the physical address size in CpuInfo HOB @ 2019-09-26 0:09 Ni, Ray 2019-09-26 0:09 ` [PATCH 1/2] UefiCpuPkg/PiSmmCpu: Remove hard code when getting physical line size Ni, Ray 2019-09-26 0:09 ` [PATCH 2/2] UefiCpuPkg/PiSmmCpuDxe: Honor the physical address size in CpuInfo HOB Ni, Ray 0 siblings, 2 replies; 7+ messages in thread From: Ni, Ray @ 2019-09-26 0:09 UTC (permalink / raw) To: devel Ray Ni (2): UefiCpuPkg/PiSmmCpu: Remove hard code when getting physical line size UefiCpuPkg/PiSmmCpuDxe: Honor the physical address size in CpuInfo HOB UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c | 63 +++++++++---------------- 1 file changed, 23 insertions(+), 40 deletions(-) -- 2.21.0.windows.1 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 1/2] UefiCpuPkg/PiSmmCpu: Remove hard code when getting physical line size 2019-09-26 0:09 [PATCH 0/2] Honor the physical address size in CpuInfo HOB Ni, Ray @ 2019-09-26 0:09 ` Ni, Ray 2019-09-26 17:54 ` Laszlo Ersek 2019-09-26 0:09 ` [PATCH 2/2] UefiCpuPkg/PiSmmCpuDxe: Honor the physical address size in CpuInfo HOB Ni, Ray 1 sibling, 1 reply; 7+ messages in thread From: Ni, Ray @ 2019-09-26 0:09 UTC (permalink / raw) To: devel; +Cc: Eric Dong, Laszlo Ersek 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; } /** @@ -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)); -- 2.21.0.windows.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] UefiCpuPkg/PiSmmCpu: Remove hard code when getting physical line size 2019-09-26 0:09 ` [PATCH 1/2] UefiCpuPkg/PiSmmCpu: Remove hard code when getting physical line size Ni, Ray @ 2019-09-26 17:54 ` Laszlo Ersek 2019-09-26 18:44 ` [edk2-devel] " Ni, Ray 0 siblings, 1 reply; 7+ messages in thread From: Laszlo Ersek @ 2019-09-26 17:54 UTC (permalink / raw) To: Ray Ni, devel; +Cc: Eric Dong 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)); > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [edk2-devel] [PATCH 1/2] UefiCpuPkg/PiSmmCpu: Remove hard code when getting physical line size 2019-09-26 17:54 ` Laszlo Ersek @ 2019-09-26 18:44 ` Ni, Ray 0 siblings, 0 replies; 7+ messages in thread From: Ni, Ray @ 2019-09-26 18:44 UTC (permalink / raw) To: devel@edk2.groups.io, lersek@redhat.com; +Cc: Dong, Eric 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)); > > > > > ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 2/2] UefiCpuPkg/PiSmmCpuDxe: Honor the physical address size in CpuInfo HOB 2019-09-26 0:09 [PATCH 0/2] Honor the physical address size in CpuInfo HOB Ni, Ray 2019-09-26 0:09 ` [PATCH 1/2] UefiCpuPkg/PiSmmCpu: Remove hard code when getting physical line size Ni, Ray @ 2019-09-26 0:09 ` Ni, Ray 2019-09-26 18:01 ` Laszlo Ersek 1 sibling, 1 reply; 7+ messages in thread From: Ni, Ray @ 2019-09-26 0:09 UTC (permalink / raw) To: devel; +Cc: Eric Dong, Laszlo Ersek 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] UefiCpuPkg/PiSmmCpuDxe: Honor the physical address size in CpuInfo HOB 2019-09-26 0:09 ` [PATCH 2/2] UefiCpuPkg/PiSmmCpuDxe: Honor the physical address size in CpuInfo HOB Ni, Ray @ 2019-09-26 18:01 ` Laszlo Ersek 2019-09-26 18:43 ` Ni, Ray 0 siblings, 1 reply; 7+ messages in thread From: Laszlo Ersek @ 2019-09-26 18:01 UTC (permalink / raw) To: Ray Ni, devel; +Cc: Eric Dong 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)); > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] UefiCpuPkg/PiSmmCpuDxe: Honor the physical address size in CpuInfo HOB 2019-09-26 18:01 ` Laszlo Ersek @ 2019-09-26 18:43 ` Ni, Ray 0 siblings, 0 replies; 7+ messages in thread From: Ni, Ray @ 2019-09-26 18:43 UTC (permalink / raw) To: Laszlo Ersek, devel@edk2.groups.io; +Cc: Dong, Eric 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)); > > ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2019-09-26 18:44 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2019-09-26 0:09 [PATCH 0/2] Honor the physical address size in CpuInfo HOB Ni, Ray 2019-09-26 0:09 ` [PATCH 1/2] UefiCpuPkg/PiSmmCpu: Remove hard code when getting physical line size Ni, Ray 2019-09-26 17:54 ` Laszlo Ersek 2019-09-26 18:44 ` [edk2-devel] " Ni, Ray 2019-09-26 0:09 ` [PATCH 2/2] UefiCpuPkg/PiSmmCpuDxe: Honor the physical address size in CpuInfo HOB Ni, Ray 2019-09-26 18:01 ` Laszlo Ersek 2019-09-26 18:43 ` Ni, Ray
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox