From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by mx.groups.io with SMTP id smtpd.web09.7191.1620989760181204187 for ; Fri, 14 May 2021 03:56:00 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=arz0dhUK; spf=pass (domain: redhat.com, ip: 170.10.133.124, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1620989759; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=zYMhK9UIBa30/mY35CuMb0NphZySyucBTYYdtDqK3WM=; b=arz0dhUK9LiQGkIDO/Uw2M8ksvjW/L6VqFZNH9HlbJ/D2wKOCE65UWelaEibLE50vO8EDC C+LqFZ6PZ9zxvbM8dybqRsVb7K33bLulZkDoTHx468SJw4YOm4fiymCbHU2IK+PZwBD8DZ JLsUAjex6fakstO+Eg8r7xRJAK/WdDs= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-402-jcH9xqzPNWWaPa9avEW6Sw-1; Fri, 14 May 2021 06:55:57 -0400 X-MC-Unique: jcH9xqzPNWWaPa9avEW6Sw-1 Received: from smtp.corp.redhat.com (int-mx07.intmail.prod.int.phx2.redhat.com [10.5.11.22]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 89E8F6409D; Fri, 14 May 2021 10:55:56 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-112-190.ams2.redhat.com [10.36.112.190]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 8185D10016FD; Fri, 14 May 2021 10:55:54 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH] UefiCpuPkg/PiSmmCpu: Remove hardcode 48 address size limitation To: devel@edk2.groups.io, ray.ni@intel.com Cc: Eric Dong , Rahul Kumar References: <20210512045310.302-1-ray.ni@intel.com> From: "Laszlo Ersek" Message-ID: Date: Fri, 14 May 2021 12:55:52 +0200 MIME-Version: 1.0 In-Reply-To: <20210512045310.302-1-ray.ni@intel.com> X-Scanned-By: MIMEDefang 2.84 on 10.5.11.22 Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=lersek@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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 > Cc: Eric Dong > Cc: Laszlo Ersek > Cc: Rahul Kumar > --- > 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