Re: [PATCH] UefiCpuPkg/MpInitLib: AP uses memory preceding IDT to store CpuMpData

[Laszlo Ersek <lersek@redhat.com>]

On 06/26/18 19:20, Andrew Fish wrote:
>> On Jun 26, 2018, at 10:06 AM, Laszlo Ersek <lersek@redhat.com> wrote:
>> On 06/25/18 04:54, Ruiyu Ni wrote:
>>> Today's MpInitLib PEI implementation directly calls
>>> PeiServices->GetHobList() from AP which may cause racing issue.
>>>
>>> This patch fixes this issue by storing the CpuMpData to memory
>>> preceding IDT. Pointer to PeiServices pointer is stored there,
>>> so after AP procedure returns, the PeiServices pointer should be
>>> restored.
>>>
>>> Contributed-under: TianoCore Contribution Agreement 1.1
>>> Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com>
>>> Cc: Jeff Fan <vanjeff_919@hotmail.com>
>>> Cc: Eric Dong <eric.dong@intel.com>
>>> Cc: Jiewen Yao <jiewen.yao@intel.com>
>>> Cc: Fish Andrew <afish@apple.com>
>>> ---
>>> UefiCpuPkg/Library/MpInitLib/DxeMpLib.c | 33 ++++++++++++++++++-
>>> UefiCpuPkg/Library/MpInitLib/MpLib.c    |  8 +++++
>>> UefiCpuPkg/Library/MpInitLib/MpLib.h    | 27 +++++++++++++++-
>>> UefiCpuPkg/Library/MpInitLib/PeiMpLib.c | 56 +++++++++++++++++++++++++++++++--
>>> 4 files changed, 119 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
>>> index e7ed21c6cd..26fead2c66 100644
>>> --- a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
>>> +++ b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
>>> @@ -1,7 +1,7 @@
>>> /** @file
>>>   MP initialize support functions for DXE phase.
>>>
>>> -  Copyright (c) 2016, Intel Corporation. All rights reserved.<BR>
>>> +  Copyright (c) 2016 - 2018, Intel Corporation. All rights reserved.<BR>
>>>   This program and the accompanying materials
>>>   are licensed and made available under the terms and conditions of the BSD License
>>>   which accompanies this distribution.  The full text of the license may be found at
>>> @@ -75,6 +75,37 @@ SaveCpuMpData (
>>>   mCpuMpData = CpuMpData;
>>> }
>>>
>>> +/**
>>> +  Push the CpuMpData for AP to use.
>>> +
>>> +  @param[in]  The pointer to CPU MP Data structure will be pushed.
>>> +  @param[out] The pointer to the context which will be passed to PopCpuMpData().
>>> +
>>> +  @return  The pointer value which was stored in where the CPU MP Data is pushed.
>>> +**/
>>> +VOID *
>>> +PushCpuMpData (
>>> +  IN  CPU_MP_DATA    *CpuMpData,
>>> +  OUT VOID           **Context
>>> +  )
>>> +{
>>> +  return NULL;
>>> +}
>>> +
>>> +/**
>>> +  Pop the CpuMpData.
>>> +
>>> +  @param[in] Pointer  The pointer value which was stored in where the CPU MP Data is pushed.
>>> +  @param[in] Context  The context of push/pop operation.
>>> +**/
>>> +VOID
>>> +PopCpuMpData (
>>> +  IN VOID           *Pointer,
>>> +  IN VOID           *Context
>>> +  )
>>> +{
>>> +}
>>> +
>>> /**
>>>   Get available system memory below 1MB by specified size.
>>>
>>> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c
>>> index f2ff40417a..786a7825d5 100644
>>> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
>>> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
>>> @@ -580,6 +580,8 @@ ApWakeupFunction (
>>>   CPU_INFO_IN_HOB            *CpuInfoInHob;
>>>   UINT64                     ApTopOfStack;
>>>   UINTN                      CurrentApicMode;
>>> +  VOID                       *BackupPtr;
>>> +  VOID                       *Context;
>>>
>>>   //
>>>   // AP finished assembly code and begin to execute C code
>>> @@ -659,8 +661,14 @@ ApWakeupFunction (
>>>           EnableDebugAgent ();
>>>           //
>>>           // Invoke AP function here
>>> +          // Use a BSP owned area (PeiServices Pointer storage) to store the CpuMpData.
>>> +          // It's required in PEI phase because CpuMpData cannot be cached in global variable as in DXE phase.
>>> +          // DXE version of Pushxxx andPopxxx is dummy implementation.
>>>           //
>>> +          BackupPtr = PushCpuMpData (CpuMpData, &Context);
>>>           Procedure (Parameter);
>>> +          PopCpuMpData (BackupPtr, Context);
>>> +
>>>           CpuInfoInHob = (CPU_INFO_IN_HOB *) (UINTN) CpuMpData->CpuInfoInHob;
>>>           if (CpuMpData->SwitchBspFlag) {
>>>             //
>>> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.h b/UefiCpuPkg/Library/MpInitLib/MpLib.h
>>> index e7f9a4de0a..270d62ff20 100644
>>> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.h
>>> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.h
>>> @@ -1,7 +1,7 @@
>>> /** @file
>>>   Common header file for MP Initialize Library.
>>>
>>> -  Copyright (c) 2016 - 2017, Intel Corporation. All rights reserved.<BR>
>>> +  Copyright (c) 2016 - 2018, Intel Corporation. All rights reserved.<BR>
>>>   This program and the accompanying materials
>>>   are licensed and made available under the terms and conditions of the BSD License
>>>   which accompanies this distribution.  The full text of the license may be found at
>>> @@ -321,6 +321,31 @@ SaveCpuMpData (
>>>   IN CPU_MP_DATA   *CpuMpData
>>>   );
>>>
>>> +/**
>>> +  Push the CpuMpData for AP to use.
>>> +
>>> +  @param[in]  The pointer to CPU MP Data structure will be pushed.
>>> +  @param[out] The pointer to the context which will be passed to PopCpuMpData().
>>> +
>>> +  @return  The pointer value which was stored in where the CPU MP Data is pushed.
>>> +**/
>>> +VOID *
>>> +PushCpuMpData (
>>> +  IN  CPU_MP_DATA    *CpuMpData,
>>> +  OUT VOID           **Context
>>> +  );
>>> +
>>> +/**
>>> +  Pop the CpuMpData.
>>> +
>>> +  @param[in] Pointer  The pointer value which was stored in where the CPU MP Data is pushed.
>>> +  @param[in] Context  The context of push/pop operation.
>>> +**/
>>> +VOID
>>> +PopCpuMpData (
>>> +  IN VOID           *Pointer,
>>> +  IN VOID           *Context
>>> +  );
>>>
>>> /**
>>>   Get available system memory below 1MB by specified size.
>>> diff --git a/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c b/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c
>>> index 791ae9db6e..5c9c4b3b1e 100644
>>> --- a/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c
>>> +++ b/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c
>>> @@ -27,6 +27,9 @@ EnableDebugAgent (
>>>
>>> /**
>>>   Get pointer to CPU MP Data structure.
>>> +  For BSP, the pointer is retrieved from HOB.
>>> +  For AP, the pointer is retrieved from the location which stores the PeiServices pointer.
>>> +  It's safe because BSP is blocking and has no chance to use PeiServices pointer when AP is executing.
>>>
>>>   @return  The pointer to CPU MP Data structure.
>>> **/
>>> @@ -35,9 +38,17 @@ GetCpuMpData (
>>>   VOID
>>>   )
>>> {
>>> -  CPU_MP_DATA      *CpuMpData;
>>> -
>>> -  CpuMpData = GetCpuMpDataFromGuidedHob ();
>>> +  CPU_MP_DATA                  *CpuMpData;
>>> +  MSR_IA32_APIC_BASE_REGISTER  ApicBaseMsr;
>>> +  IA32_DESCRIPTOR              Idtr;
>>> +
>>> +  ApicBaseMsr.Uint64 = AsmReadMsr64 (MSR_IA32_APIC_BASE);
>>> +  if (ApicBaseMsr.Bits.BSP == 1) {
>>> +    CpuMpData = GetCpuMpDataFromGuidedHob ();
>>> +  } else {
>>> +    AsmReadIdtr (&Idtr);
>>> +    CpuMpData = (CPU_MP_DATA *)(*(UINTN *) (Idtr.Base - sizeof (UINTN)));
>>> +  }
>>>   ASSERT (CpuMpData != NULL);
>>>   return CpuMpData;
>>> }
>>> @@ -64,6 +75,45 @@ SaveCpuMpData (
>>>     );
>>> }
>>>
>>> +/**
>>> +  Push the CpuMpData for AP to use.
>>> +
>>> +  @param[in]  The pointer to CPU MP Data structure will be pushed.
>>> +  @param[out] The pointer to the context which will be passed to PopCpuMpData().
>>> +
>>> +  @return  The pointer value which was stored in where the CPU MP Data is pushed.
>>> +**/
>>> +VOID *
>>> +PushCpuMpData (
>>> +  IN  CPU_MP_DATA    *CpuMpData,
>>> +  OUT VOID           **Context
>>> +  )
>>> +{
>>> +  EFI_PEI_SERVICES  **PeiServices;
>>> +  IA32_DESCRIPTOR   Idtr;
>>> +
>>> +  AsmReadIdtr (&Idtr);
>>> +  *Context = (VOID *) (Idtr.Base - sizeof (UINTN));
>>> +  PeiServices = (EFI_PEI_SERVICES **)(*(UINTN *)(*Context));
>>> +  *(UINTN *)(*Context) = (UINTN)CpuMpData;
>>> +  return PeiServices;
>>> +}
>>> +
>>> +/**
>>> +  Pop the CpuMpData.
>>> +
>>> +  @param[in] Pointer  The pointer value which was stored in where the CPU MP Data is pushed.
>>> +  @param[in] Context  The context of push/pop operation.
>>> +**/
>>> +VOID
>>> +PopCpuMpData (
>>> +  IN VOID           *Pointer,
>>> +  IN VOID           *Context
>>> +  )
>>> +{
>>> +  *(UINTN *)Context = (UINTN)Pointer;
>>> +}
>>> +
>>> /**
>>>   Check if AP wakeup buffer is overlapped with existing allocated buffer.
>>>
>>>

[snip]

>> Ray, can you please explain how this patch is supposed to work? Are
>> you re-purposing an otherwise unused (un-exercised) entry in the
>> interrupt descriptor table, for storing a generic pointer?
>>
>> ... The commit message says, "memory preceding IDT", and the patch
>> says "(Idtr.Base - sizeof (UINTN))". What memory is supposed to be
>> there?
>>
>
> Laszlo,
>
> Some context. A few weeks ago I reported that the APs in this code
> were using PEI Services to get a HOB to store the CPU context
> structure. PEI Services are not defined as MP Safe so this is not a
> good long term direction. Given this is PEI Code and can run XIP
> global variables may not be writable so this code needs some why
> (other than a HOB) to have a pointer.

Right, I remember:

  http://mid.mail-archive.com/31C6D3FB-91D3-491B-8059-4B6DEA11BDF9@apple.com

Specifically, as I understood it, you reported about the following call
stack:

  MpInitLibWhoAmI()                        [UefiCpuPkg/Library/MpInitLib/MpLib.c]
    GetCpuMpData()                         [UefiCpuPkg/Library/MpInitLib/PeiMpLib.c]
      GetCpuMpDataFromGuidedHob()          [UefiCpuPkg/Library/MpInitLib/MpLib.c]
        GetFirstGuidHob()                  [MdePkg/Library/PeiHobLib/HobLib.c]
          GetHobList()                     [MdePkg/Library/PeiHobLib/HobLib.c]
            PeiServicesGetHobList()        [MdePkg/Library/PeiServicesLib/PeiServicesLib.c]
              GetPeiServicesTablePointer() [MdePkg/Library/PeiServicesTablePointerLibIdt/PeiServicesTablePointer.c]
              PeiGetHobList()              [MdeModulePkg/Core/Pei/Hob/Hob.c]

The MpInitLibWhoAmI() function itself is valid to call from AP
procedures, e.g. through the EFI_PEI_MP_SERVICES_PPI.WhoAmI() member
function:

  PeiWhoAmI()         [UefiCpuPkg/CpuMpPei/CpuMpPei.c]
    MpInitLibWhoAmI() [UefiCpuPkg/Library/MpInitLib/MpLib.c]

However, the bottom of the call stack (i.e. the implementation of the
service) is invalid for APs to execute. So yes, I think I understand
that.

> Per the PI Spec Idtr.Base - sizeof(UINTN) is the address of the PEI
> Services Table pointer. We add that so every PEI API did not need to
> pass the PEI Services table pointer. It is what enables things like
> ASSERT() and DEBUG() to work in PEI. Prior to this we had to have
> PEI_ASSERT()/PEI_DEBUG() and pass the PEI Services Table pointer.

Ah, OK, I did miss this information. I've checked the PI spec v1.6 now,
and I do see this, in Volume 1, "5.4 PEI Services Table Retrieval",
"5.4.2 x64".

And that also explains the intent of this patch. Namely,

(1) the patch replaces the PEI services pointer (located just below the
    IDT) with the CpuMpData address, on *each* AP, in the
    ApWakeupFunction() function, saving the previous PEI services
    pointer value to the AP stack,

(2) the AP procedure is entered in this state,

(3) whenever the AP procedure calls EFI_PEI_MP_SERVICES_PPI.WhoAmI(),
   the GetCpuMpData() function reaches back to the PEI services pointer
   location, and grabs the CpuMpData value from it,

(4) once the AP procedure returns, the AP writes back the original PEI
    services pointer value, from the AP stack, to the PEI services
    pointer location.

Here's the problem: the IDT is reused across all processors. In the
MpInitLibInitialize() function, we have

>   //
>   // Save BSP's Control registers to APs
>   //
>   SaveVolatileRegisters (&CpuMpData->CpuData[0].VolatileRegisters);

And in the ApWakeupFunction(), we have, at startup:

>     if (CpuMpData->InitFlag == ApInitConfig) {
>       //
>       // Add CPU number
>       //
>       InterlockedIncrement ((UINT32 *) &CpuMpData->CpuCount);
>       ProcessorNumber = ApIndex;
>       //
>       // This is first time AP wakeup, get BIST information from AP stack
>       //
>       ApTopOfStack  = CpuMpData->Buffer + (ProcessorNumber + 1) * CpuMpData->CpuApStackSize;
>       BistData = *(UINT32 *) ((UINTN) ApTopOfStack - sizeof (UINTN));
>       //
>       // Do some AP initialize sync
>       //
>       ApInitializeSync (CpuMpData);
>       //
>       // Sync BSP's Control registers to APs
>       //
>       RestoreVolatileRegisters (&CpuMpData->CpuData[0].VolatileRegisters, FALSE);

This means that, when multiple APs are fired up in parallel, they *race*
for the PEI services pointer location. There is no isolation between APs
when they execute PushCpuMpData() concurrently.

First, this can corrupt the datum in the PEI services pointer location.

Second, even assuming that PushCpuMpData() and PopCpuMpData() are
atomic, the order in which APs complete the AP procedure is not
deterministic, and it need not be the exact reverse of the entry order.
We may have the following order, for example:

- AP 1 writes CpuMpData, and saves the original PEI services pointer on
  its stack,
- AP 2 writes CpuMpData, and saves the same CpuMpData value (originally
  written by AP 1) on its stack,
- AP 1 completes the AP procedure and restores the original PEI services
  pointer from its stack,
- AP 2 completes the AP procedure, and overwrites the PEI services
  pointer, with the CpuMpData value from its stack, that was originally
  written by AP 1.

Assuming I (remotely) understand what's going on, I'd (vaguely) suggest
three alternatives:

- instead of the idea captured in this patch, we should use an APIC ID
  search similar to the one done initially in "MpFuncs.nasm",

- or else, we should stick with the current idea, but use atomic
  compare-and-swap operations, so that the original PEI services pointer
  value be restored ultimately, at the least,

- or else (possibly the simplest fix), allocate a separate IDT for each
  AP, including the UINTN that precedes the (now AP-specific) IDT. This
  means that the PEI services pointer *location* would be separate for
  each AP, and no contention would occur.

Thanks,
Laszlo