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 <michael.d.kin...@intel.com>; >> devel@edk2.groups.io >> Cc: Dong, Eric <eric.d...@intel.com>; Ni, Ray >> <ray...@intel.com>; Wang, Jian J <jian.j.w...@intel.com> >> 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 <eric.d...@intel.com> >>> Cc: Ray Ni <ray...@intel.com> >>> Cc: Laszlo Ersek <ler...@redhat.com> >>> Signed-off-by: Michael D Kinney >> <michael.d.kin...@intel.com> >>> --- >>> 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.<BR> >>> + Copyright (c) 2016 - 2019, Intel Corporation. All >> rights reserved.<BR> >>> 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 <ler...@redhat.com> >> 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 >> >> > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#39835): https://edk2.groups.io/g/devel/message/39835 Mute This Topic: https://groups.io/mt/31345224/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
