From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [216.205.24.124]) by mx.groups.io with SMTP id smtpd.web09.856.1621363361982906907 for ; Tue, 18 May 2021 11:42:42 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=ZxHp49+n; spf=pass (domain: redhat.com, ip: 216.205.24.124, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1621363361; 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=e3Dh4iy1nim2jDcxwSUebmDuf6ozko6SdhCI3o0oBZM=; b=ZxHp49+nTNBCx/ioTSH3XjqrEnDJTEBQEFybzsEmSudHxMMgNUrK1XIxsF1gB3hLzj2ug8 YxYOwfnDBnxXA8MoHJ7mmLwrPsV9sz8Dclpf+qHj8eE5dMLY8TYHBMGNDipvbv4lNh8kac TzVxgf/kzr9RXbnS/VHIK3vlJtHCKq0= 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-81-MhKjkzZtN1Kct5E4YcB_sw-1; Tue, 18 May 2021 14:42:38 -0400 X-MC-Unique: MhKjkzZtN1Kct5E4YcB_sw-1 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 22E8E802690; Tue, 18 May 2021 18:42:37 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-112-30.ams2.redhat.com [10.36.112.30]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 6649A5D6AB; Tue, 18 May 2021 18:42:36 +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: Tue, 18 May 2021 20:42:34 +0200 MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 2.79 on 10.5.11.15 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 On 05/18/21 09:51, Ni, Ray wrote: > > >> -----Original Message----- >> From: Laszlo Ersek >> Sent: Sunday, May 16, 2021 9:39 AM >> To: devel@edk2.groups.io; Ni, Ray >> 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 >> >> >> >>> >>> >>> >>> >>> >>> >