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

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

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