> -----Original Message----- > From: Laszlo Ersek <ler...@redhat.com> > Sent: Sunday, May 16, 2021 9:39 AM > To: devel@edk2.groups.io; Ni, Ray <ray...@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/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
