Re: [edk2-devel] [PATCH] UefiCpuPkg/PiSmmCpu: Remove hardcode 48 address size limitation

["Ni, Ray" <ray.ni@intel.com>]

> -----Original Message-----
> From: Laszlo Ersek <lersek@redhat.com>
> Sent: Sunday, May 16, 2021 9:39 AM
> To: devel@edk2.groups.io; Ni, Ray <ray.ni@intel.com>
> Subject: Re: [edk2-devel] [PATCH] UefiCpuPkg/PiSmmCpu: Remove hardcode 48 address size limitation
> 
> On 05/15/21 02:04, Ni, Ray wrote:
> > Laszlo,
> > Do you think that another API is also needed: GetPhysicalAddressWidth() that returns number 36/52?
> 
> No. The GetPhysicalAddressBits() function that I proposed already returns this information. It has three outputs: the number of
> bits (that is, the width), as return value, and the two optional output parameters.
> 
> So if you only need the the bit count, call
> 
>   GetPhysicalAddressBits (NULL, NULL);
> 
> These calculations are so cheap and small that keeping them in a single function makes a lot of sense in my opinion.

I wasn't aware of the return value of the API. with your API, there is no need for another API to retrieve the address size.

> For a critical bugfix, I would prefer not mixing the actual fix with the introduction of the symbolic names. Your patch currently
> fixes three things at the same time: (1) coding style (it replaces magic constants with macros / type names), (2) a bug in
> calculation, (3) a missing CPUID "maximum function" check.
> 
> Maybe writing a separate patch for each of these is unjustified, but I was really unhappy to see that the commit message said
> nothing about (1) and (3), and I had to hunt down (2) between the other changes.
> 
> The minimal fix -- that is, the fix for (2) -- would be just one line:
> 
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> index fd6583f9d172..4592b76fe595 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> @@ -1920,7 +1920,7 @@ InitializeMpServiceData (
>    //
>    AsmCpuid (0x80000008, (UINT32*)&Index, NULL, NULL, NULL);
>    gPhyMask = LShiftU64 (1, (UINT8)Index) - 1;
> -  gPhyMask &= (1ull << 48) - EFI_PAGE_SIZE;
> +  gPhyMask &= 0xfffffffffffff000ULL;
> 
>    //
>    // Create page tables
> 
> 
> I don't like that the patch currently does three things but only documents one.

Thanks for explaining why you don't think it's a good patch. I thought anytime changing a code,
I should try to make it better, functional better, looks better.

I will follow your suggestion next time for bug fixes.

> 
> That said, if you are out of time, feel free to go ahead with Eric's R-b.
Indeed. thanks for the understanding.

> 
> Thanks
> Laszlo
> 
> 
> 
> >
> >
> > 
> >
> >
> >