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

[Laszlo Ersek]

Hi Ray,

On 05/20/21 06:28, Ni, Ray wrote:
>>
>> 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.)
>>
> 
> I admit my patch assumes the reviewers should be very familiar to how CPUID "protocol" works.
> In fact, there are two kinds of reviewers at least:
> 1. domain reviewers
> 2. consumers as reviewers
> 
> I didn't consider the second kind of reviewers.
> 

I *am* (somewhat) familiar with how CPUID works; I do know that the max
supported function is supposed to be checked before a particular
(optional) function is exercised. (The max supported function may or may
not be cached in a variable or whatever, but that's beside the point here.)

The problem is that these are two *distinct* issues in the existent
code. One, a violation of the *generic* CPUID "protocol" -- there was no
check for the availability of the particular function. Two, the bad
masking that is *specific* to the function exercised.

Your patch fixes both issues at the same time, which is questionable
practice.

And even if we agree that these two issues are conceptually so close to
each other that they don't deserve separate patches, they absolutely
deserve separate *mentions* in the commit message. It's not that you
need to teach the CPUID protocol to readers in the commit message.
Instead, you need to prepare your knowledgeable readers too, in the
commit message, that you are going to *deal* with the generic CPUID
protocol in this patch, which otherwise mainly intends to fix the bad
masking.

(And then there's the third, again independent, action of replacing
magic numbers with symbolic names.)

So there really are three *valid* approaches to solving *just* this
particular issue that you were working on:

(1) The one-liner mask fix. This is the most surgical (localized) fix,
easy to port to other branches and repositories. It is *strictly* an
improvement, makes nothing worse, so it's perfectly fine to have a
patchlike this.

(2) The mask fix plus the CPUID protocol fix (check max supported
function). Two patches, in any order you like.

(3) The mask fix, the CPUID protocol fix, and the coding style fix.
Three patches, in any order whatsoever that you like.

Implementing (3), but with all three patches squashed into one, is just
barely acceptable, and only if the commit message documents each action
separately.

Open development is all about discipline. You look at the code, you find
the bug, but you realize there are *other* issues with the code too.
That's where *discipline* comes into the picture. You sit down and
figure out what exactly you want to do. You choose one of the approaches
(1), (2) and (3). Maybe your business requirements and schedule dictate
(1). Maybe you have a bit more time and you can implement (3). Either is
fine. Jumbling all three actions together and not preparing reviewers --
even knowledgeable reviewers -- for it is *not* fine.

I can't emphasize discipline enough. That's where you say, "yes, I can
see that we use naked magic numbers here, and that the CPUID protocol is
not observed. But we need an urgent fix, so I'm *not* going to touch
those issues *even if I could do so*, because it messes up the
boundaries between the issues, and makes understanding the changes
difficult". Or else, you can say, "I am definitely going to fix all this
mess, but I'm going to do it carefully, taking one step at a time --
more precisely, taking *my reviewers* through it one step at a time,
because even though I have the complete picture in my mind right now,
they don't, and even I won't, in six months".

I've been harping on this kind of self-discipline for almost a decade
now; it's mind-boggling for me to see that it just never sticks.

The worst you can do to a reviewer is to *surprise them* in the code
part of a patch.

Well, I'm sorry for obsessing about this again.

Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#75413): https://edk2.groups.io/g/devel/message/75413
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]
-=-=-=-=-=-=-=-=-=-=-=-