From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by mx.groups.io with SMTP id smtpd.web09.5236.1621497042679516986 for ; Thu, 20 May 2021 00:50:42 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=VJpPlheI; spf=pass (domain: redhat.com, ip: 170.10.133.124, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1621497041; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=3zhyBYdZ8vp4aamoXSn6ZxgB8Tk9z79hvA0kecsVSWc=; b=VJpPlheII6MLZ3z26Z6xLIkaYxE68Nv8rpHTFp/wdRpN6TXCPVsTbSpLIcoKfkrdW6ahC6 PFKVgqjnzFlwUxYLPiIlfomPERfe/K3xkcvZk0dW0Acd181FZSyq+GUGxxravQPc4L20Wv PPh8iHZRSHn5C1BLgk81xXVjH0SDgZE= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-598-dHTmHGggPhmYUN9em_Y4tg-1; Thu, 20 May 2021 03:50:40 -0400 X-MC-Unique: dHTmHGggPhmYUN9em_Y4tg-1 Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 0B7136D5C0; Thu, 20 May 2021 07:50:39 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-112-217.ams2.redhat.com [10.36.112.217]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 4813B77F10; Thu, 20 May 2021 07:50:38 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH] UefiCpuPkg/PiSmmCpu: Remove hardcode 48 address size limitation To: "Ni, Ray" , "devel@edk2.groups.io" References: <12259.1621037062277250978@groups.io> <877e17dd-8235-1a56-13c1-c61a505d543e@redhat.com> From: "Laszlo Ersek" Message-ID: Date: Thu, 20 May 2021 09:50:36 +0200 MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 2.79 on 10.5.11.13 Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=lersek@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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