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

["Laszlo Ersek" <lersek@redhat.com>]

On 05/18/21 09:51, Ni, Ray wrote:
> 
> 
>> -----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.

My only point was that separate concerns should be implemented in
separate patches, or at least (if they are really difficult, or
overkill, to isolate) that they should be documented.

Please try to think with your reviewers' mindsets in mind, when
preparing a patch (commit message and code both). The question the patch
author has to ask themselves is not only "how do I implement this", but
also "how do I explain this to my reviewers".

I read the subject line and the commit message. Those make me anticipate
some magic constant (related to 48) in the code. But that's not what I
see in the code. I see new macros, new control flow, new variables, new
indentation. The actual purpose of the patch (as documented in the
commit message) is just a tiny fraction of the whole code change, and
the commit message does not prepare the reader for it. *That* is what's
wrong. Improving code wherever you go is great, but all that effort
needs to be structured correctly, or at least justified in natural language.

Patches exist primarily for humans to read, and secondarily for
computers to execute. If we don't believe in that, then edk2 will never
become a true open source, community project. (In my opinion anyway.)

Thanks
Laszlo

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