From mboxrd@z Thu Jan  1 00:00:00 1970
Authentication-Results: mx.groups.io;
 dkim=missing; spf=pass (domain: redhat.com, ip: 209.132.183.28, mailfrom: lersek@redhat.com)
Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28])
 by groups.io with SMTP; Thu, 26 Sep 2019 10:54:29 -0700
Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15])
	(using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits))
	(No client certificate requested)
	by mx1.redhat.com (Postfix) with ESMTPS id E181046671;
	Thu, 26 Sep 2019 17:54:28 +0000 (UTC)
Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-49.rdu2.redhat.com [10.10.120.49])
	by smtp.corp.redhat.com (Postfix) with ESMTP id 0C53A5D6B2;
	Thu, 26 Sep 2019 17:54:27 +0000 (UTC)
Subject: Re: [PATCH 1/2] UefiCpuPkg/PiSmmCpu: Remove hard code when getting physical line size
To: Ray Ni <ray.ni@intel.com>, devel@edk2.groups.io
Cc: Eric Dong <eric.dong@intel.com>
References: <20190926000904.187532-1-ray.ni@intel.com>
 <20190926000904.187532-2-ray.ni@intel.com>
From: "Laszlo Ersek" <lersek@redhat.com>
Message-ID: <6230e7c3-f23a-bb53-bebc-aa7fd406d61f@redhat.com>
Date: Thu, 26 Sep 2019 19:54:27 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
 Thunderbird/52.9.1
MIME-Version: 1.0
In-Reply-To: <20190926000904.187532-2-ray.ni@intel.com>
X-Scanned-By: MIMEDefang 2.79 on 10.5.11.15
X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.30]); Thu, 26 Sep 2019 17:54:29 +0000 (UTC)
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit

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));
>