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

["Laszlo Ersek" <lersek@redhat.com>]

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



> 
> 
> 
> 
> 
>