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...@intel.com>
> Cc: Eric Dong <eric.d...@intel.com>
> Cc: Laszlo Ersek <ler...@redhat.com>
> Cc: Rahul Kumar <rahul1.ku...@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/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
