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

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

[Ni, Ray]

5-level paging can be enabled on CPU which supports up to 52 physical
address size. But when the feature was enabled, the 48 address size
limit was not removed and the 5-level paging testing didn't access
address >= 2^48. So the issue wasn't detected until recently an
address >= 2^48 is accessed.

Change-Id: Iaedc73be318d4b4122071efc3ba6e967a4b58fc3
Signed-off-by: Ray Ni <ray.ni@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Rahul Kumar <rahul1.kumar@intel.com>
---
 UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c | 26 ++++++++++++++++++--------
 1 file changed, 18 insertions(+), 8 deletions(-)

diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
index fd6583f9d1..89143810b6 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
@@ -1887,11 +1887,13 @@ InitializeMpServiceData (
   IN UINTN       ShadowStackSize
   )
 {
-  UINT32                    Cr3;
-  UINTN                     Index;
-  UINT8                     *GdtTssTables;
-  UINTN                     GdtTableStepSize;
-  CPUID_VERSION_INFO_EDX    RegEdx;
+  UINT32                          Cr3;
+  UINTN                           Index;
+  UINT8                           *GdtTssTables;
+  UINTN                           GdtTableStepSize;
+  CPUID_VERSION_INFO_EDX          RegEdx;
+  UINT32                          MaxExtendedFunction;
+  CPUID_VIR_PHY_ADDRESS_SIZE_EAX  VirPhyAddressSize;
 
   //
   // Determine if this CPU supports machine check
@@ -1918,9 +1920,17 @@ InitializeMpServiceData (
   // Initialize physical address mask
   // NOTE: Physical memory above virtual address limit is not supported !!!
   //
-  AsmCpuid (0x80000008, (UINT32*)&Index, NULL, NULL, NULL);
-  gPhyMask = LShiftU64 (1, (UINT8)Index) - 1;
-  gPhyMask &= (1ull << 48) - EFI_PAGE_SIZE;
+  AsmCpuid (CPUID_EXTENDED_FUNCTION, &MaxExtendedFunction, NULL, NULL, NULL);
+  if (MaxExtendedFunction >= CPUID_VIR_PHY_ADDRESS_SIZE) {
+    AsmCpuid (CPUID_VIR_PHY_ADDRESS_SIZE, &VirPhyAddressSize.Uint32, NULL, NULL, NULL);
+  } else {
+    VirPhyAddressSize.Bits.PhysicalAddressBits = 36;
+  }
+  gPhyMask  = LShiftU64 (1, VirPhyAddressSize.Bits.PhysicalAddressBits) - 1;
+  //
+  // Clear the low 12 bits
+  //
+  gPhyMask &= 0xfffffffffffff000ULL;
 
   //
   // Create page tables
-- 
2.31.1.windows.1

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

[Dong, Eric]

Reviewed-by: Eric Dong <eric.dong@intel.com>

-----Original Message-----
From: Ni, Ray <ray.ni@intel.com> 
Sent: Wednesday, May 12, 2021 12:53 PM
To: devel@edk2.groups.io
Cc: Dong, Eric <eric.dong@intel.com>; Laszlo Ersek <lersek@redhat.com>; Kumar, Rahul1 <rahul1.kumar@intel.com>
Subject: [PATCH] UefiCpuPkg/PiSmmCpu: Remove hardcode 48 address size limitation

5-level paging can be enabled on CPU which supports up to 52 physical
address size. But when the feature was enabled, the 48 address size
limit was not removed and the 5-level paging testing didn't access
address >= 2^48. So the issue wasn't detected until recently an
address >= 2^48 is accessed.

Change-Id: Iaedc73be318d4b4122071efc3ba6e967a4b58fc3
Signed-off-by: Ray Ni <ray.ni@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Rahul Kumar <rahul1.kumar@intel.com>
---
 UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c | 26 ++++++++++++++++++--------
 1 file changed, 18 insertions(+), 8 deletions(-)

diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
index fd6583f9d1..89143810b6 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
@@ -1887,11 +1887,13 @@ InitializeMpServiceData (
   IN UINTN       ShadowStackSize

   )

 {

-  UINT32                    Cr3;

-  UINTN                     Index;

-  UINT8                     *GdtTssTables;

-  UINTN                     GdtTableStepSize;

-  CPUID_VERSION_INFO_EDX    RegEdx;

+  UINT32                          Cr3;

+  UINTN                           Index;

+  UINT8                           *GdtTssTables;

+  UINTN                           GdtTableStepSize;

+  CPUID_VERSION_INFO_EDX          RegEdx;

+  UINT32                          MaxExtendedFunction;

+  CPUID_VIR_PHY_ADDRESS_SIZE_EAX  VirPhyAddressSize;

 

   //

   // Determine if this CPU supports machine check

@@ -1918,9 +1920,17 @@ InitializeMpServiceData (
   // Initialize physical address mask

   // NOTE: Physical memory above virtual address limit is not supported !!!

   //

-  AsmCpuid (0x80000008, (UINT32*)&Index, NULL, NULL, NULL);

-  gPhyMask = LShiftU64 (1, (UINT8)Index) - 1;

-  gPhyMask &= (1ull << 48) - EFI_PAGE_SIZE;

+  AsmCpuid (CPUID_EXTENDED_FUNCTION, &MaxExtendedFunction, NULL, NULL, NULL);

+  if (MaxExtendedFunction >= CPUID_VIR_PHY_ADDRESS_SIZE) {

+    AsmCpuid (CPUID_VIR_PHY_ADDRESS_SIZE, &VirPhyAddressSize.Uint32, NULL, NULL, NULL);

+  } else {

+    VirPhyAddressSize.Bits.PhysicalAddressBits = 36;

+  }

+  gPhyMask  = LShiftU64 (1, VirPhyAddressSize.Bits.PhysicalAddressBits) - 1;

+  //

+  // Clear the low 12 bits

+  //

+  gPhyMask &= 0xfffffffffffff000ULL;

 

   //

   // Create page tables

-- 
2.31.1.windows.1

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

[Laszlo Ersek]

On 05/12/21 06:53, Ni, Ray wrote:
> 5-level paging can be enabled on CPU which supports up to 52 physical
> address size. But when the feature was enabled, the 48 address size
> limit was not removed and the 5-level paging testing didn't access
> address >= 2^48. So the issue wasn't detected until recently an
> address >= 2^48 is accessed.
>
> Change-Id: Iaedc73be318d4b4122071efc3ba6e967a4b58fc3

(1) Please drop the Change-Id from the upstream patch.


> Signed-off-by: Ray Ni <ray.ni@intel.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Rahul Kumar <rahul1.kumar@intel.com>
> ---
>  UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c | 26 ++++++++++++++++++--------
>  1 file changed, 18 insertions(+), 8 deletions(-)
>
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> index fd6583f9d1..89143810b6 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> @@ -1887,11 +1887,13 @@ InitializeMpServiceData (
>    IN UINTN       ShadowStackSize
>    )
>  {
> -  UINT32                    Cr3;
> -  UINTN                     Index;
> -  UINT8                     *GdtTssTables;
> -  UINTN                     GdtTableStepSize;
> -  CPUID_VERSION_INFO_EDX    RegEdx;
> +  UINT32                          Cr3;
> +  UINTN                           Index;
> +  UINT8                           *GdtTssTables;
> +  UINTN                           GdtTableStepSize;
> +  CPUID_VERSION_INFO_EDX          RegEdx;
> +  UINT32                          MaxExtendedFunction;
> +  CPUID_VIR_PHY_ADDRESS_SIZE_EAX  VirPhyAddressSize;
>
>    //
>    // Determine if this CPU supports machine check
> @@ -1918,9 +1920,17 @@ InitializeMpServiceData (
>    // Initialize physical address mask
>    // NOTE: Physical memory above virtual address limit is not supported !!!
>    //
> -  AsmCpuid (0x80000008, (UINT32*)&Index, NULL, NULL, NULL);
> -  gPhyMask = LShiftU64 (1, (UINT8)Index) - 1;
> -  gPhyMask &= (1ull << 48) - EFI_PAGE_SIZE;
> +  AsmCpuid (CPUID_EXTENDED_FUNCTION, &MaxExtendedFunction, NULL, NULL, NULL);
> +  if (MaxExtendedFunction >= CPUID_VIR_PHY_ADDRESS_SIZE) {
> +    AsmCpuid (CPUID_VIR_PHY_ADDRESS_SIZE, &VirPhyAddressSize.Uint32, NULL, NULL, NULL);
> +  } else {
> +    VirPhyAddressSize.Bits.PhysicalAddressBits = 36;
> +  }
> +  gPhyMask  = LShiftU64 (1, VirPhyAddressSize.Bits.PhysicalAddressBits) - 1;
> +  //
> +  // Clear the low 12 bits
> +  //
> +  gPhyMask &= 0xfffffffffffff000ULL;
>
>    //
>    // Create page tables
>

(2) Please introduce the following (new) function to UefiCpuLib:

/**
  Get the physical address width supported by the processor.

  @param[out] ValidAddressMask          Bitmask with valid address bits set to
                                        one; other bits are clear. Optional
                                        parameter.

  @param[out] ValidPageBaseAddressMask  Bitmask with valid page base address
                                        bits set to one; other bits are clear.
                                        Optional parameter.

  @return  The physical address width supported by the processor.
**/
UINT8
EFIAPI
GetPhysicalAddressBits (
  OUT UINT64 *ValidAddressMask         OPTIONAL,
  OUT UINT64 *ValidPageBaseAddressMask OPTIONAL
  )
{
  UINT32                         MaxExtendedFunction;
  CPUID_VIR_PHY_ADDRESS_SIZE_EAX VirPhyAddressSize;
  UINT64                         AddressMask;
  UINT64                         PageBaseAddressMask;

  AsmCpuid (CPUID_EXTENDED_FUNCTION, &MaxExtendedFunction, NULL, NULL, NULL);
  if (MaxExtendedFunction >= CPUID_VIR_PHY_ADDRESS_SIZE) {
    AsmCpuid (
      CPUID_VIR_PHY_ADDRESS_SIZE,
      &VirPhyAddressSize.Uint32,
      NULL,
      NULL,
      NULL
      );
  } else {
    VirPhyAddressSize.Bits.PhysicalAddressBits = 36;
  }

  AddressMask = LShiftU64 (1, VirPhyAddressSize.Bits.PhysicalAddressBits) - 1;
  PageBaseAddressMask = AddressMask & ~(UINT64)EFI_PAGE_MASK;

  if (ValidAddressMask != NULL) {
    *ValidAddressMask = AddressMask;
  }
  if (ValidPageBaseAddressMask != NULL) {
    *ValidPageBaseAddressMask = PageBaseAddressMask;
  }
  return VirPhyAddressSize.Bits.PhysicalAddressBits;
}


(3) In a separate patch, please rewrite the MtrrLibInitializeMtrrMask()
function in "UefiCpuPkg/Library/MtrrLib/MtrrLib.c", to make use of the
new GetPhysicalAddressBits() function.


(4) In a separate patch, please rewrite the Is5LevelPagingNeeded()
function in "UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c", to make use of
the new GetPhysicalAddressBits() function.


(5) Please rework the current patch so that we simply call

  GetPhysicalAddressBits (NULL, &gPhyMask);

in the InitializeMpServiceData() function, in
"UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c".


... Please realize that the current bug exists *precisely* because peole
have always been too lazy to introduce the GetPhysicalAddressBits()
helper function, and they've just gone around duplicating code like
there's no tomorrow. The solution to the problem is *NOT* to introduce
yet another naked CPUID_VIR_PHY_ADDRESS_SIZE call!

Thanks
Laszlo

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

[Ni, Ray]

[-- Attachment #1: Type: text/plain, Size: 954 bytes --]

Laszlo,
Do you think that another API is also needed: GetPhysicalAddressWidth() that returns number 36/52?
physical address width is needed by (besides those that rely on the width for mask calculation):
  UefiCpuPkg\CpuMpPei\CpuPaging.c
  UefiCpuPkg\PiSmmCpuDxeSmm\X64\PageTbl.c
  MdeModulePkg\Core\DxeIplPeim\X64\VirtualMemory.c
  MdeModulePkg\Universal\Acpi\S3SaveStateDxe\AcpiS3ContextSave.c
  MdeModulePkg\Universal\CapsulePei\UefiCapsule.c
  MdePkg\Library\SmmIoLib\SmmIoLib.c
  OvmfPkg\XenPlatformPei\MemDetect.c
  UefiCpuPkg\Universal\Acpi\S3Resume2Pei\S3Resume.c
  UefiPayloadPkg\UefiPayloadEntry\X64\VirtualMemory.c


GetPhysicalAddressMask() can call GetPhysicalAddressWidth().

Since it's a large-scale change but the SMM high MMIO access bug is critical/urgent, I prefer to firstly push this bug fix change and then work on the new APIs.

https://bugzilla.tianocore.org/show_bug.cgi?id=3394 was submitted to capture this.

[-- Attachment #2: Type: text/html, Size: 1062 bytes --]

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

[Laszlo Ersek]

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.

> physical address width is needed by (besides those that rely on the width for mask calculation):
>   UefiCpuPkg\CpuMpPei\CpuPaging.c
>   UefiCpuPkg\PiSmmCpuDxeSmm\X64\PageTbl.c
>   MdeModulePkg\Core\DxeIplPeim\X64\VirtualMemory.c
>   MdeModulePkg\Universal\Acpi\S3SaveStateDxe\AcpiS3ContextSave.c
>   MdeModulePkg\Universal\CapsulePei\UefiCapsule.c
>   MdePkg\Library\SmmIoLib\SmmIoLib.c
>   OvmfPkg\XenPlatformPei\MemDetect.c
>   UefiCpuPkg\Universal\Acpi\S3Resume2Pei\S3Resume.c
>   UefiPayloadPkg\UefiPayloadEntry\X64\VirtualMemory.c

Ah, I couldn't find those because the AsmCpuid() calls in them don't even use the symbolic names for 0x80000008 (CPUID_VIR_PHY_ADDRESS_SIZE) and for the least significant byte of EAX on output (CPUID_VIR_PHY_ADDRESS_SIZE_EAX.Bits.PhysicalAddressBits).

So it's even worse (much worse) than I expected :(

Because of the MdePkg and MdeModulePkg dependencies, we can't even put the helper in UefiCpuPkg; it must go into MdePkg (possibly BaseLib, I'm not sure).

> 
> 
> GetPhysicalAddressMask() can call GetPhysicalAddressWidth().

To me two functions are not really justified, because the address width, and the bit masks are so closely related. But I'm also not too opposed to having two functions.

> 
> Since it's a large-scale change but the SMM high MMIO access bug is critical/urgent, I prefer to firstly push this bug fix change and then work on the new APIs.
> 
> https://bugzilla.tianocore.org/show_bug.cgi?id=3394 was submitted to capture this.

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.

That said, if you are out of time, feel free to go ahead with Eric's R-b.

Thanks
Laszlo



> 
> 
> 
> 
> 
> 

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

[Ni, Ray]

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

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

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

[Laszlo Ersek]

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

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

[Ni, Ray]

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

Re: [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

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

[Michael Brown]

On 18/05/2021 19:42, Laszlo Ersek wrote:
> On 05/18/21 09:51, Ni, Ray wrote:
>> 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".

(Apologies in advance for intruding.)

Ray: I think you may be neglecting one half of the problem.

When making a code change, there are (at least) two requirements:

1. The new version of the code must make sense.  You are definitely 
achieving this: as you say, "make it better, functional better, looks 
better".  This is good.

2. The *change* between the old and new versions of the code (i.e. the 
patch and accompanying commit message) must also make sense.  This is 
the requirement that I think Laszlo would like you to meet.

It's not viable for anyone to meaningfully review code unless both of 
these requirements are met.

Thanks,

Michael