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...@intel.com>; devel@edk2.groups.io
> Cc: Dong, Eric <eric.d...@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...@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 (#48129): https://edk2.groups.io/g/devel/message/48129
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]
-=-=-=-=-=-=-=-=-=-=-=-
