From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: redhat.com, ip: 209.132.183.28, mailfrom: lersek@redhat.com) Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by groups.io with SMTP; Tue, 30 Apr 2019 03:02:17 -0700 Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id CEBA130832E3; Tue, 30 Apr 2019 10:02:16 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-121-42.rdu2.redhat.com [10.10.121.42]) by smtp.corp.redhat.com (Postfix) with ESMTP id 41EF77013C; Tue, 30 Apr 2019 10:02:15 +0000 (UTC) Subject: Re: [edk2-devel] [Patch 2/4] UefiCpuPkg/MpInitLib: Avoid MSR_IA32_APIC_BASE for single core To: "Kinney, Michael D" , "devel@edk2.groups.io" Cc: "Dong, Eric" , "Ni, Ray" , "Wang, Jian J" References: <20190425175334.5944-1-michael.d.kinney@intel.com> <20190425175334.5944-3-michael.d.kinney@intel.com> <8c03f148-8b61-2477-7708-0e55f32b581b@redhat.com> From: "Laszlo Ersek" Message-ID: Date: Tue, 30 Apr 2019 12:02:14 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 2.79 on 10.5.11.12 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.44]); Tue, 30 Apr 2019 10:02:16 +0000 (UTC) Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 04/29/19 19:11, Kinney, Michael D wrote: > Laszlo, > > I was attempting to follow the equivalent detection logic > that is used in the SourceLevelDebugPkg. > > https://github.com/tianocore/edk2/blob/e2d3a25f1a3135221a9c8061e1b8f90245d727eb/SourceLevelDebugPkg/Library/DebugAgent/DebugAgentCommon/DebugMp.c#L140 > > Yes. CPUID can be used to determine availability of > MSR_IA32_APIC_BASE. That would be safer if the maximum > number of CPUs can start with a value of 1 and change to > a higher value later. But based on your analysis, > it looks like the max number of CPUs is known when this > function runs which is always after memory is discovered. The proposed patch is safe wrt. the PCD production-consumption sequence, yes. However, the patch is unsafe due to APs calling a PPI member function (namely, the PCD_PPI.Get32 function). To my understanding, APs may not call PPIs. > The MSR access is in the function GetCpuMpData(), which is > called from all the MP service functions. I was trying > to avoid an extra CPUID check on all those paths. > > I prefer the current patch if it is safe. Please let me > know if you think extra comments are required in the code > or the commit message. I think the patch is unsafe, because it is possible for both of the following conditions to hold, at the same time: - the function may be entered by an AP - the PCD may be dynamic That would lead to an AP calling (GetPcdPpiPointer ())->Get32 (TokenNumber) which I believe is forbidden. If the patch called FixedPcdGet32(), then the patch would be safe. Unfortunately, in that case, the patch wouldn't compile for OvmfPkg. If we can use a new PCD for this, such that UefiCpuPkg.dec does not permit platforms to pick "dynamic" for that PCD -- i.e. it would have to be Feature, Fixed, or Patch --, and then the patch is updated to call the appropriate flavor-specific PCD macro (FeaturePcdGet, PatchPcdGet*, FixedPcdGet*), then the patch will be fine. The point is that the "getting the PCD" action never enter a library function that is not explicitly MP-safe, nor enter a PPI member function. Thanks Laszlo >> -----Original Message----- >> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] >> On Behalf Of Laszlo Ersek >> Sent: Friday, April 26, 2019 12:25 PM >> To: Kinney, Michael D ; >> devel@edk2.groups.io >> Cc: Dong, Eric ; Ni, Ray >> ; Wang, Jian J >> Subject: Re: [edk2-devel] [Patch 2/4] >> UefiCpuPkg/MpInitLib: Avoid MSR_IA32_APIC_BASE for >> single core >> >> (+Jian) >> >> Hi Mike, >> >> thank you for the CC. >> >> On 04/25/19 19:53, Michael D Kinney wrote: >>> Avoid access to MSR_IA32_APIC_BASE that may not be >> supported >>> on single core CPUs. If >> PcdCpuMaxLogicalProcessorNumber is 1, >>> then there is only one CPU that must be the BSP. >>> >>> Cc: Eric Dong >>> Cc: Ray Ni >>> Cc: Laszlo Ersek >>> Signed-off-by: Michael D Kinney >> >>> --- >>> UefiCpuPkg/Library/MpInitLib/PeiMpLib.c | 15 >> ++++++++++++++- >>> 1 file changed, 14 insertions(+), 1 deletion(-) >>> >>> diff --git a/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c >> b/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c >>> index 35dff91fd2..5488049c08 100644 >>> --- a/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c >>> +++ b/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c >>> @@ -1,7 +1,7 @@ >>> /** @file >>> MP initialize support functions for PEI phase. >>> >>> - Copyright (c) 2016 - 2018, Intel Corporation. All >> rights reserved.
>>> + Copyright (c) 2016 - 2019, Intel Corporation. All >> rights reserved.
>>> SPDX-License-Identifier: BSD-2-Clause-Patent >>> >>> **/ >>> @@ -101,6 +101,19 @@ GetCpuMpData ( >>> MSR_IA32_APIC_BASE_REGISTER ApicBaseMsr; >>> IA32_DESCRIPTOR Idtr; >>> >>> + // >>> + // If there is only 1 CPU, then it must be the BSP. >> This avoids an access to >>> + // MSR_IA32_APIC_BASE that may not be supported on >> single core CPUs. >>> + // >>> + if (PcdGet32 (PcdCpuMaxLogicalProcessorNumber) == >> 1) { >>> + CpuMpData = GetCpuMpDataFromGuidedHob (); >>> + ASSERT (CpuMpData != NULL); >>> + return CpuMpData; >>> + } >>> + >>> + // >>> + // Otherwise use MSR_IA32_APIC_BASE to determine if >> the CPU is BSP or AP. >>> + // >>> ApicBaseMsr.Uint64 = AsmReadMsr64 >> (MSR_IA32_APIC_BASE); >>> if (ApicBaseMsr.Bits.BSP == 1) { >>> CpuMpData = GetCpuMpDataFromGuidedHob (); >>> >> >> This patch leads me down on two paths: >> >> (1) Specifically regarding the code change. I think this >> patch is unsafe >> on platforms that dynamically set the PCD to a value >> larger than 1. >> (Including OVMF.) >> >> If the value is larger than 1, then the system has >> at least one AP, >> and the AP may enter the function. In addition, >> because the PCD is >> dynamic, the PcdGet32() call will invoke the PCD PPI >> (if I >> understand correctly), which is not allowed for an >> AP. >> >> Is it not possible to determine the availability of >> MSR_IA32_APIC_BASE from CPUID, or a different MSR? >> >> >> (2) More generally, this patch made me review >> OvmfPkg/PlatformPei: >> >> (a) OvmfPkg/PlatformPei sets the PCD in >> MaxCpuCountInitialization(), >> >> (b) later, OvmfPkg/PlatformPei publishes the >> permanent PEI RAM in >> PublishPeiMemory() >> >> (c) which in turn leads to the installation of >> gEfiPeiMemoryDiscoveredPpiGuid intto the PPI >> database, by the >> PEI Core >> >> (d) CpuMpPei can now be dispatched, because it has a >> depex on the >> "memory discovered" PPI >> >> (e) PeiMpInitLib, which is linked into CpuMpPei, can >> consume the PCD >> safely. >> >> I relied on this behavior in the following OVMF >> commit: >> >> commit 45a70db3c3a59b64e0f517870415963fbfacf507 >> Author: Laszlo Ersek >> Date: Thu Nov 24 15:18:44 2016 +0100 >> >> OvmfPkg/PlatformPei: take VCPU count from QEMU >> and configure MpInitLib >> >> These settings will allow CpuMpPei and CpuDxe to >> wait for the initial AP >> check-ins exactly as long as necessary. >> >> It is safe to set >> PcdCpuMaxLogicalProcessorNumber and >> PcdCpuApInitTimeOutInMicroSeconds in >> OvmfPkg/PlatformPei. >> OvmfPkg/PlatformPei installs the permanent PEI >> RAM, producing >> gEfiPeiMemoryDiscoveredPpiGuid, and >> UefiCpuPkg/CpuMpPei has a depex on >> gEfiPeiMemoryDiscoveredPpiGuid. >> >> [...] >> >> Except... in commit 0a0d5296e448 >> ("UefiCpuPkg/CpuMpPei: support >> stack guard feature", 2018-09-10), the DEPEX >> mentioned in step (d) >> was deleted. >> >> So now I got a bit nervous, because how are then the >> setting and >> reading of the PCD serialized between >> OvmfPkg/PlatformPei, and >> PeiMpInitLib in CpuMpPei? >> >> Luckily however, I think we're safe: >> >> - CpuMpPei itself doesn't consume the PCD, >> >> - MpInitLib consumes the PCD in several functions, >> but all clients >> of MpInitLib must call MpInitLibInitialize() >> first, before using >> other library APIs >> >> - CpuMpPei calls MpInitLibInitialize() in the >> InitializeCpuMpWorker() function >> >> - the InitializeCpuMpWorker() function is only >> called from the >> MemoryDiscoveredPpiNotifyCallback(). >> >> So the PCD set/get order remains safe and >> deterministic. Even though >> CpuMpPei can now be dispatched before permanent >> memory is >> discovered, MpInitLibInitialize() -- and so the >> reading of the PCD >> -- is still delayed until after permanent PEI RAM is >> available. >> That's a relief. >> >> In fact, it looks like commit 0a0d5296e448 >> ("UefiCpuPkg/CpuMpPei: >> support stack guard feature", 2018-09-10) delayed >> the entire >> original entry point of CpuMpPei into the "memory >> discovered" >> callback. That appears OK to me, it's just that the >> patch (~1000 >> lines) could have been split at least in two: one >> patch could have >> implemented the "PPI DEPEX --> PPI notify" >> transformation, without >> other changes in behavior, and the second patch >> could have extended >> the (now delayed) startup logic with >> InitializeMpExceptionStackSwitchHandlers() & >> friends. >> >> Thanks >> Laszlo >> >> >