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

["Laszlo Ersek" <lersek@redhat.com>]

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