Re: [edk2-devel] [Patch 2/4] UefiCpuPkg/MpInitLib: Avoid MSR_IA32_APIC_BASE for single core

["Michael D Kinney" <michael.d.kinney@intel.com>]

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.kinney@intel.com>;
> devel@edk2.groups.io
> Cc: Dong, Eric <eric.dong@intel.com>; Ni, Ray
> <ray.ni@intel.com>; Wang, Jian J
> <jian.j.wang@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.kinney@intel.com>;
> >> devel@edk2.groups.io
> >> Cc: Dong, Eric <eric.dong@intel.com>; Ni, Ray
> >> <ray.ni@intel.com>; Wang, Jian J
> <jian.j.wang@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.dong@intel.com>
> >>> Cc: Ray Ni <ray.ni@intel.com>
> >>> Cc: Laszlo Ersek <lersek@redhat.com>
> >>> Signed-off-by: Michael D Kinney
> >> <michael.d.kinney@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 <lersek@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
> >>
> >>
> >
> 
> 
>