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
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#48058): https://edk2.groups.io/g/devel/message/48058
Mute This Topic: https://groups.io/mt/34293642/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
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));
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#48124): https://edk2.groups.io/g/devel/message/48124
Mute This Topic: https://groups.io/mt/34293642/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
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));
> >
>
>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#48129): https://edk2.groups.io/g/devel/message/48129
Mute This Topic: https://groups.io/mt/34293642/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
© 2016 - 2026 Red Hat, Inc.