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 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 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.

Thanks,

Mike

> -----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
> 
>