Laszlo, I agree with your analysis. I will work on a different solution.
Thanks, Mike > -----Original Message----- > From: devel@edk2.groups.io > [mailto:devel@edk2.groups.io] On Behalf Of Laszlo Ersek > Sent: Tuesday, April 30, 2019 3:02 AM > 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 > > 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/e2d3a25f1a313522 > 1a9c8061e1b8f90245d727eb/SourceLevelDebugPkg/Library/De > bugAgent/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 (#39855): https://edk2.groups.io/g/devel/message/39855 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] -=-=-=-=-=-=-=-=-=-=-=-
