[edk2-devel] [PATCH] UefiCpuPkg/PiSmmCpu: Remove hardcode 48 address size limitation
Ni, Ray
ray.ni at intel.com
Tue May 18 07:51:11 UTC 2021
> -----Original Message-----
> From: Laszlo Ersek <lersek at redhat.com>
> Sent: Sunday, May 16, 2021 9:39 AM
> To: devel at edk2.groups.io; Ni, Ray <ray.ni at 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
>
>
>
> >
> >
> >
> >
> >
> >
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#75242): https://edk2.groups.io/g/devel/message/75242
Mute This Topic: https://groups.io/mt/82765279/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list