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
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#75056): https://edk2.groups.io/g/devel/message/75056
Mute This Topic: https://groups.io/mt/82765279/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
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
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#75113): https://edk2.groups.io/g/devel/message/75113
Mute This Topic: https://groups.io/mt/82765279/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
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. -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#75134): https://edk2.groups.io/g/devel/message/75134 Mute This Topic: https://groups.io/mt/82765279/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
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 > > > > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#75141): https://edk2.groups.io/g/devel/message/75141 Mute This Topic: https://groups.io/mt/82765279/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
> -----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 > > > > > > > > > > > > > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#75242): https://edk2.groups.io/g/devel/message/75242 Mute This Topic: https://groups.io/mt/82765279/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
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 >> >> >> >>> >>> >>> >>> >>> >>> > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#75284): https://edk2.groups.io/g/devel/message/75284 Mute This Topic: https://groups.io/mt/82765279/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
> > 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. -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#75410): https://edk2.groups.io/g/devel/message/75410 Mute This Topic: https://groups.io/mt/82765279/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
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/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
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 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#75419): https://edk2.groups.io/g/devel/message/75419 Mute This Topic: https://groups.io/mt/82765279/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
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
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#75093): https://edk2.groups.io/g/devel/message/75093
Mute This Topic: https://groups.io/mt/82765279/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
© 2016 - 2024 Red Hat, Inc.