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...@intel.com>
> Cc: Eric Dong <eric.d...@intel.com>
> Cc: Laszlo Ersek <ler...@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/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
