public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
* [PATCH v3] UefiCpuPkg/MpInitLib: Avoid calling PEI services from AP
@ 2018-06-29 10:42 Ruiyu Ni
  2018-06-29 19:25 ` Laszlo Ersek
  0 siblings, 1 reply; 3+ messages in thread
From: Ruiyu Ni @ 2018-06-29 10:42 UTC (permalink / raw)
  To: edk2-devel; +Cc: Jeff Fan, Eric Dong, Jiewen Yao, Fish Andrew, Laszlo Ersek

Today's MpInitLib PEI implementation directly calls
PeiServices->GetHobList() from AP which may cause racing issue.

This patch fixes this issue by duplicating IDT for APs.
Because CpuMpData structure is stored just after IDT, the CpuMPData
address equals to IDTR.BASE + IDTR.LIMIT + 1.

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>
Cc: Laszlo Ersek <lersek@redhat.com>
---
 UefiCpuPkg/Library/MpInitLib/MpLib.c    | 41 +++++++++++++++++++++++++++++----
 UefiCpuPkg/Library/MpInitLib/PeiMpLib.c | 18 +++++++++++----
 2 files changed, 51 insertions(+), 8 deletions(-)

diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c
index eb2765910c..e8ecad870c 100644
--- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
+++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
@@ -1506,6 +1506,7 @@ MpInitLibInitialize (
   UINT32                   MaxLogicalProcessorNumber;
   UINT32                   ApStackSize;
   MP_ASSEMBLY_ADDRESS_MAP  AddressMap;
+  CPU_VOLATILE_REGISTERS   VolatileRegisters;
   UINTN                    BufferSize;
   UINT32                   MonitorFilterSize;
   VOID                     *MpBuffer;
@@ -1516,6 +1517,7 @@ MpInitLibInitialize (
   UINTN                    Index;
   UINTN                    ApResetVectorSize;
   UINTN                    BackupBufferAddr;
+  UINTN                    ApIdtBase;
 
   OldCpuMpData = GetCpuMpDataFromGuidedHob ();
   if (OldCpuMpData == NULL) {
@@ -1530,19 +1532,46 @@ MpInitLibInitialize (
   ApStackSize = PcdGet32(PcdCpuApStackSize);
   ApLoopMode  = GetApLoopMode (&MonitorFilterSize);
 
+  //
+  // Save BSP's Control registers for APs
+  //
+  SaveVolatileRegisters (&VolatileRegisters);
+
   BufferSize  = ApStackSize * MaxLogicalProcessorNumber;
   BufferSize += MonitorFilterSize * MaxLogicalProcessorNumber;
-  BufferSize += sizeof (CPU_MP_DATA);
   BufferSize += ApResetVectorSize;
+  BufferSize += VolatileRegisters.Idtr.Limit + 1;
+  BufferSize += sizeof (CPU_MP_DATA);
   BufferSize += (sizeof (CPU_AP_DATA) + sizeof (CPU_INFO_IN_HOB))* MaxLogicalProcessorNumber;
   MpBuffer    = AllocatePages (EFI_SIZE_TO_PAGES (BufferSize));
   ASSERT (MpBuffer != NULL);
   ZeroMem (MpBuffer, BufferSize);
   Buffer = (UINTN) MpBuffer;
 
+  /*
+    The layout of the Buffer is as below:
+
+      +--------------------+ <-- Buffer
+          AP Stacks (N)
+      +--------------------+ <-- MonitorBuffer
+      AP Monitor Filters (N)
+      +--------------------+ <-- BackupBuffer
+           Backup Buffer
+      +--------------------+ <-- ApIdtBase (8-byte boundary)
+             AP IDT          All APs share one separate IDT. So AP can get address of CPU_MP_DATA from IDT Base.
+      +--------------------+ <-- CpuMpData
+           CPU_MP_DATA
+      +--------------------+ <-- CpuMpData->CpuData
+          CPU_AP_DATA (N)
+      +--------------------+ <-- CpuMpData->CpuInfoHob
+        CPU_INFO_IN_HOB (N)
+      +--------------------+
+
+  */
   MonitorBuffer    = (UINT8 *) (Buffer + ApStackSize * MaxLogicalProcessorNumber);
   BackupBufferAddr = (UINTN) MonitorBuffer + MonitorFilterSize * MaxLogicalProcessorNumber;
-  CpuMpData = (CPU_MP_DATA *) (BackupBufferAddr + ApResetVectorSize);
+  ApIdtBase        = ALIGN_VALUE (BackupBufferAddr + ApResetVectorSize, 8);
+  CpuMpData        = (CPU_MP_DATA *) (ApIdtBase + VolatileRegisters.Idtr.Limit + 1);
   CpuMpData->Buffer           = Buffer;
   CpuMpData->CpuApStackSize   = ApStackSize;
   CpuMpData->BackupBuffer     = BackupBufferAddr;
@@ -1557,10 +1586,14 @@ MpInitLibInitialize (
   CpuMpData->MicrocodePatchAddress    = PcdGet64 (PcdCpuMicrocodePatchAddress);
   CpuMpData->MicrocodePatchRegionSize = PcdGet64 (PcdCpuMicrocodePatchRegionSize);
   InitializeSpinLock(&CpuMpData->MpLock);
+
   //
-  // Save BSP's Control registers to APs
+  // Duplicate BSP's IDT to APs.
+  // All APs share one separate IDT. So AP can get the address of CpuMpData by using IDTR.BASE + IDTR.LIMIT + 1
   //
-  SaveVolatileRegisters (&CpuMpData->CpuData[0].VolatileRegisters);
+  CopyMem ((VOID *)ApIdtBase, (VOID *)VolatileRegisters.Idtr.Base, VolatileRegisters.Idtr.Limit + 1);
+  VolatileRegisters.Idtr.Base = ApIdtBase;
+  CopyMem (&CpuMpData->CpuData[0].VolatileRegisters, &VolatileRegisters, sizeof (VolatileRegisters));
   //
   // Set BSP basic information
   //
diff --git a/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c b/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c
index 791ae9db6e..92f28681e4 100644
--- a/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c
+++ b/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c
@@ -27,6 +27,8 @@ EnableDebugAgent (
 
 /**
   Get pointer to CPU MP Data structure.
+  For BSP, the pointer is retrieved from HOB.
+  For AP, the structure is just after IDT.
 
   @return  The pointer to CPU MP Data structure.
 **/
@@ -35,10 +37,18 @@ GetCpuMpData (
   VOID
   )
 {
-  CPU_MP_DATA      *CpuMpData;
-
-  CpuMpData = GetCpuMpDataFromGuidedHob ();
-  ASSERT (CpuMpData != NULL);
+  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 ();
+    ASSERT (CpuMpData != NULL);
+  } else {
+    AsmReadIdtr (&Idtr);
+    CpuMpData = (CPU_MP_DATA *) (Idtr.Base + Idtr.Limit + 1);
+  }
   return CpuMpData;
 }
 
-- 
2.16.1.windows.1



^ permalink raw reply related	[flat|nested] 3+ messages in thread

* Re: [PATCH v3] UefiCpuPkg/MpInitLib: Avoid calling PEI services from AP
  2018-06-29 10:42 [PATCH v3] UefiCpuPkg/MpInitLib: Avoid calling PEI services from AP Ruiyu Ni
@ 2018-06-29 19:25 ` Laszlo Ersek
  2018-07-02  5:56   ` Ni, Ruiyu
  0 siblings, 1 reply; 3+ messages in thread
From: Laszlo Ersek @ 2018-06-29 19:25 UTC (permalink / raw)
  To: Ruiyu Ni, edk2-devel; +Cc: Jeff Fan, Eric Dong, Jiewen Yao, Fish Andrew

Hi Ray,

I have several comments / questions:

On 06/29/18 12:42, 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 duplicating IDT for APs.
> Because CpuMpData structure is stored just after IDT, the CpuMPData
> address equals to IDTR.BASE + IDTR.LIMIT + 1.
>
> 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>
> Cc: Laszlo Ersek <lersek@redhat.com>
> ---
>  UefiCpuPkg/Library/MpInitLib/MpLib.c    | 41 +++++++++++++++++++++++++++++----
>  UefiCpuPkg/Library/MpInitLib/PeiMpLib.c | 18 +++++++++++----
>  2 files changed, 51 insertions(+), 8 deletions(-)
>
> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> index eb2765910c..e8ecad870c 100644
> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> @@ -1506,6 +1506,7 @@ MpInitLibInitialize (
>    UINT32                   MaxLogicalProcessorNumber;
>    UINT32                   ApStackSize;
>    MP_ASSEMBLY_ADDRESS_MAP  AddressMap;
> +  CPU_VOLATILE_REGISTERS   VolatileRegisters;
>    UINTN                    BufferSize;
>    UINT32                   MonitorFilterSize;
>    VOID                     *MpBuffer;
> @@ -1516,6 +1517,7 @@ MpInitLibInitialize (
>    UINTN                    Index;
>    UINTN                    ApResetVectorSize;
>    UINTN                    BackupBufferAddr;
> +  UINTN                    ApIdtBase;
>
>    OldCpuMpData = GetCpuMpDataFromGuidedHob ();
>    if (OldCpuMpData == NULL) {
> @@ -1530,19 +1532,46 @@ MpInitLibInitialize (
>    ApStackSize = PcdGet32(PcdCpuApStackSize);
>    ApLoopMode  = GetApLoopMode (&MonitorFilterSize);
>
> +  //
> +  // Save BSP's Control registers for APs
> +  //
> +  SaveVolatileRegisters (&VolatileRegisters);
> +
>    BufferSize  = ApStackSize * MaxLogicalProcessorNumber;
>    BufferSize += MonitorFilterSize * MaxLogicalProcessorNumber;
> -  BufferSize += sizeof (CPU_MP_DATA);
>    BufferSize += ApResetVectorSize;

(1) I think that, right after the above line, you are missing:

  BufferSize  = ALIGN_VALUE (BufferSize, 8);

It is not noticed in practice because

  AllocatePages (EFI_SIZE_TO_PAGES (BufferSize))

rounds up the allocation anyway. However, the above ALIGN_VALUE() would
be necessary to remain consistent with the ALIGN_VALUE() that is used
below, in the assignment to "ApIdtBase".

> +  BufferSize += VolatileRegisters.Idtr.Limit + 1;
> +  BufferSize += sizeof (CPU_MP_DATA);
>    BufferSize += (sizeof (CPU_AP_DATA) + sizeof (CPU_INFO_IN_HOB))* MaxLogicalProcessorNumber;
>    MpBuffer    = AllocatePages (EFI_SIZE_TO_PAGES (BufferSize));
>    ASSERT (MpBuffer != NULL);
>    ZeroMem (MpBuffer, BufferSize);
>    Buffer = (UINTN) MpBuffer;
>
> +  /*
> +    The layout of the Buffer is as below:
> +
> +      +--------------------+ <-- Buffer
> +          AP Stacks (N)
> +      +--------------------+ <-- MonitorBuffer
> +      AP Monitor Filters (N)
> +      +--------------------+ <-- BackupBuffer
> +           Backup Buffer
> +      +--------------------+ <-- ApIdtBase (8-byte boundary)
> +             AP IDT          All APs share one separate IDT. So AP can get address of CPU_MP_DATA from IDT Base.
> +      +--------------------+ <-- CpuMpData
> +           CPU_MP_DATA
> +      +--------------------+ <-- CpuMpData->CpuData
> +          CPU_AP_DATA (N)
> +      +--------------------+ <-- CpuMpData->CpuInfoHob
> +        CPU_INFO_IN_HOB (N)
> +      +--------------------+
> +
> +  */

Some remarks for this comment block:

(2) We should likely use the "//" comment style.

(3) The "BackupBuffer" comment should be "CpuMpData->BackupBuffer". We
don't have a "BackupBuffer" local variable, only "BackupBufferAddr".

(4) "CpuMpData->CpuInfoHob" is a typo; it should be
"CpuMpData->CpuInfoInHob".

>    MonitorBuffer    = (UINT8 *) (Buffer + ApStackSize * MaxLogicalProcessorNumber);
>    BackupBufferAddr = (UINTN) MonitorBuffer + MonitorFilterSize * MaxLogicalProcessorNumber;
> -  CpuMpData = (CPU_MP_DATA *) (BackupBufferAddr + ApResetVectorSize);
> +  ApIdtBase        = ALIGN_VALUE (BackupBufferAddr + ApResetVectorSize, 8);
> +  CpuMpData        = (CPU_MP_DATA *) (ApIdtBase + VolatileRegisters.Idtr.Limit + 1);
>    CpuMpData->Buffer           = Buffer;
>    CpuMpData->CpuApStackSize   = ApStackSize;
>    CpuMpData->BackupBuffer     = BackupBufferAddr;
> @@ -1557,10 +1586,14 @@ MpInitLibInitialize (
>    CpuMpData->MicrocodePatchAddress    = PcdGet64 (PcdCpuMicrocodePatchAddress);
>    CpuMpData->MicrocodePatchRegionSize = PcdGet64 (PcdCpuMicrocodePatchRegionSize);
>    InitializeSpinLock(&CpuMpData->MpLock);
> +

(5) At this point, I suggest a self-check, something like:

  ASSERT ((CpuMpData->CpuInfoInHob +
           sizeof (CPU_INFO_IN_HOB) * MaxLogicalProcessorNumber) ==
          Buffer + BufferSize);

>    //
> -  // Save BSP's Control registers to APs
> +  // Duplicate BSP's IDT to APs.
> +  // All APs share one separate IDT. So AP can get the address of CpuMpData by using IDTR.BASE + IDTR.LIMIT + 1
>    //
> -  SaveVolatileRegisters (&CpuMpData->CpuData[0].VolatileRegisters);
> +  CopyMem ((VOID *)ApIdtBase, (VOID *)VolatileRegisters.Idtr.Base, VolatileRegisters.Idtr.Limit + 1);
> +  VolatileRegisters.Idtr.Base = ApIdtBase;
> +  CopyMem (&CpuMpData->CpuData[0].VolatileRegisters, &VolatileRegisters, sizeof (VolatileRegisters));
>    //
>    // Set BSP basic information
>    //

This is my main concern.

The first CopyMem() correctly populates the IDT that is supposed to be
shared by all the APs, from the BSP's IDT.

Then, we modify "VolatileRegisters" (which is never used in the function
beyond the second CopyMem()), so that the IDTR base points to the APs'
shared IDT. OK.

Then, with the second CopyMem(), we put the *modified* VolatileRegisters
into CpuData[0]. This replaces the original SaveVolatileRegisters()
call, and as a result, the CpuData[0] IDTR base will now point to the
APs' shared IDT, and not the BSPs own IDT.

(6) Is this not a problem for the BSP itself?

I see three explicit uses of CpuData[0] in the code, namely:

(6a) The one added above by the patch.

(6b) Lower down in the same MpInitLibInitialize() function:

      CopyMem (
        &CpuMpData->CpuData[Index].VolatileRegisters,
        &CpuMpData->CpuData[0].VolatileRegisters,
        sizeof (CPU_VOLATILE_REGISTERS)
        );

I think this is fine, for all (Index > 0) values. We could as well use
the local "VolatileRegisters" here, as source argument.

(6c) And in ApWakeupFunction():

      //
      // Sync BSP's Control registers to APs
      //
      RestoreVolatileRegisters (&CpuMpData->CpuData[0].VolatileRegisters, FALSE);

Minimally, we should update the comment to:

      //
      // Sync BSP's Control registers to APs. Note that IDTR.BASE is different.
      //

Which means that, from now on, we generally consider CpuData[0] to stand
for the BSP, *except* IDTR.BASE. (I guess we could redefine CpuData[0],
not as "BSP", but as "initializer for APs".)

Thus, are we sure that the BSP never uses CpuData[0].IDTR.BASE for its
own purposes?

Should we perhaps:
- keep CpuData[0] unchanged,
- introduce a new, dedicated field, "CpuMpData->ApIdtBase", and use that
  *explicitly* wherever necessary?


(7) My next question is a corollary of the above -- what about
SwitchBSP()?

- Is SwitchBSP() compatible with this design?
- Will the new BSP start using the old BSP's IDT?
- More importantly, will the old BSP start using the IDT that is shared
  by the APs?

Without these, I think GetCpuMpData() would break. The new BSP would go
to the HOB, which is OK. However, the old BSP (= new AP) would go to the
*original* IDTR, and that's not usable for getting CpuMpData.

(I don't know how CpuData[0] relates to the *current* -- possibly
switched -- BSP.)

Thank you!
Laszlo

> diff --git a/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c b/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c
> index 791ae9db6e..92f28681e4 100644
> --- a/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c
> +++ b/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c
> @@ -27,6 +27,8 @@ EnableDebugAgent (
>
>  /**
>    Get pointer to CPU MP Data structure.
> +  For BSP, the pointer is retrieved from HOB.
> +  For AP, the structure is just after IDT.
>
>    @return  The pointer to CPU MP Data structure.
>  **/
> @@ -35,10 +37,18 @@ GetCpuMpData (
>    VOID
>    )
>  {
> -  CPU_MP_DATA      *CpuMpData;
> -
> -  CpuMpData = GetCpuMpDataFromGuidedHob ();
> -  ASSERT (CpuMpData != NULL);
> +  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 ();
> +    ASSERT (CpuMpData != NULL);
> +  } else {
> +    AsmReadIdtr (&Idtr);
> +    CpuMpData = (CPU_MP_DATA *) (Idtr.Base + Idtr.Limit + 1);
> +  }
>    return CpuMpData;
>  }
>
>



^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH v3] UefiCpuPkg/MpInitLib: Avoid calling PEI services from AP
  2018-06-29 19:25 ` Laszlo Ersek
@ 2018-07-02  5:56   ` Ni, Ruiyu
  0 siblings, 0 replies; 3+ messages in thread
From: Ni, Ruiyu @ 2018-07-02  5:56 UTC (permalink / raw)
  To: Laszlo Ersek, edk2-devel@lists.01.org
  Cc: Jeff Fan, Dong, Eric, Yao, Jiewen, Fish Andrew

On 6/30/2018 3:25 AM, Laszlo Ersek wrote:
> (1) I think that, right after the above line, you are missing:
> 
>    BufferSize  = ALIGN_VALUE (BufferSize, 8);
Yes. That's a bug in the patch. I will fix it in V4.

> 
> It is not noticed in practice because
> 
>    AllocatePages (EFI_SIZE_TO_PAGES (BufferSize))
> 
> rounds up the allocation anyway. However, the above ALIGN_VALUE() would
> be necessary to remain consistent with the ALIGN_VALUE() that is used
> below, in the assignment to "ApIdtBase".
> 
>> +  BufferSize += VolatileRegisters.Idtr.Limit + 1;
>> +  BufferSize += sizeof (CPU_MP_DATA);
>>     BufferSize += (sizeof (CPU_AP_DATA) + sizeof (CPU_INFO_IN_HOB))* MaxLogicalProcessorNumber;
>>     MpBuffer    = AllocatePages (EFI_SIZE_TO_PAGES (BufferSize));
>>     ASSERT (MpBuffer != NULL);
>>     ZeroMem (MpBuffer, BufferSize);
>>     Buffer = (UINTN) MpBuffer;
>>
>> +  /*
>> +    The layout of the Buffer is as below:
>> +
>> +      +--------------------+ <-- Buffer
>> +          AP Stacks (N)
>> +      +--------------------+ <-- MonitorBuffer
>> +      AP Monitor Filters (N)
>> +      +--------------------+ <-- BackupBuffer
>> +           Backup Buffer
>> +      +--------------------+ <-- ApIdtBase (8-byte boundary)
>> +             AP IDT          All APs share one separate IDT. So AP can get address of CPU_MP_DATA from IDT Base.
>> +      +--------------------+ <-- CpuMpData
>> +           CPU_MP_DATA
>> +      +--------------------+ <-- CpuMpData->CpuData
>> +          CPU_AP_DATA (N)
>> +      +--------------------+ <-- CpuMpData->CpuInfoHob
>> +        CPU_INFO_IN_HOB (N)
>> +      +--------------------+
>> +
>> +  */
> Some remarks for this comment block:
> 
> (2) We should likely use the "//" comment style.
OK.
> 
> (3) The "BackupBuffer" comment should be "CpuMpData->BackupBuffer". We
> don't have a "BackupBuffer" local variable, only "BackupBufferAddr".
OK.

> 
> (4) "CpuMpData->CpuInfoHob" is a typo; it should be
> "CpuMpData->CpuInfoInHob".
OK.
> 
>>     MonitorBuffer    = (UINT8 *) (Buffer + ApStackSize * MaxLogicalProcessorNumber);
>>     BackupBufferAddr = (UINTN) MonitorBuffer + MonitorFilterSize * MaxLogicalProcessorNumber;
>> -  CpuMpData = (CPU_MP_DATA *) (BackupBufferAddr + ApResetVectorSize);
>> +  ApIdtBase        = ALIGN_VALUE (BackupBufferAddr + ApResetVectorSize, 8);
>> +  CpuMpData        = (CPU_MP_DATA *) (ApIdtBase + VolatileRegisters.Idtr.Limit + 1);
>>     CpuMpData->Buffer           = Buffer;
>>     CpuMpData->CpuApStackSize   = ApStackSize;
>>     CpuMpData->BackupBuffer     = BackupBufferAddr;
>> @@ -1557,10 +1586,14 @@ MpInitLibInitialize (
>>     CpuMpData->MicrocodePatchAddress    = PcdGet64 (PcdCpuMicrocodePatchAddress);
>>     CpuMpData->MicrocodePatchRegionSize = PcdGet64 (PcdCpuMicrocodePatchRegionSize);
>>     InitializeSpinLock(&CpuMpData->MpLock);
>> +
> (5) At this point, I suggest a self-check, something like:
> 
>    ASSERT ((CpuMpData->CpuInfoInHob +
>             sizeof (CPU_INFO_IN_HOB) * MaxLogicalProcessorNumber) ==
>            Buffer + BufferSize);
> 
OK.
>>     //
>> -  // Save BSP's Control registers to APs
>> +  // Duplicate BSP's IDT to APs.
>> +  // All APs share one separate IDT. So AP can get the address of CpuMpData by using IDTR.BASE + IDTR.LIMIT + 1
>>     //
>> -  SaveVolatileRegisters (&CpuMpData->CpuData[0].VolatileRegisters);
>> +  CopyMem ((VOID *)ApIdtBase, (VOID *)VolatileRegisters.Idtr.Base, VolatileRegisters.Idtr.Limit + 1);
>> +  VolatileRegisters.Idtr.Base = ApIdtBase;
>> +  CopyMem (&CpuMpData->CpuData[0].VolatileRegisters, &VolatileRegisters, sizeof (VolatileRegisters));
>>     //
>>     // Set BSP basic information
>>     //
> This is my main concern.
> 
> The first CopyMem() correctly populates the IDT that is supposed to be
> shared by all the APs, from the BSP's IDT.
> 
> Then, we modify "VolatileRegisters" (which is never used in the function
> beyond the second CopyMem()), so that the IDTR base points to the APs'
> shared IDT. OK.
> 
> Then, with the second CopyMem(), we put the*modified*  VolatileRegisters
> into CpuData[0]. This replaces the original SaveVolatileRegisters()
> call, and as a result, the CpuData[0] IDTR base will now point to the
> APs' shared IDT, and not the BSPs own IDT.
> 
> (6) Is this not a problem for the BSP itself?
> 
> I see three explicit uses of CpuData[0] in the code, namely:
> 
> (6a) The one added above by the patch.
> 
> (6b) Lower down in the same MpInitLibInitialize() function:
> 
>        CopyMem (
>          &CpuMpData->CpuData[Index].VolatileRegisters,
>          &CpuMpData->CpuData[0].VolatileRegisters,
>          sizeof (CPU_VOLATILE_REGISTERS)
>          );
> 
> I think this is fine, for all (Index > 0) values. We could as well use
> the local "VolatileRegisters" here, as source argument.
OK.
> 
> (6c) And in ApWakeupFunction():
> 
>        //
>        // Sync BSP's Control registers to APs
>        //
>        RestoreVolatileRegisters (&CpuMpData->CpuData[0].VolatileRegisters, FALSE);
> 
> Minimally, we should update the comment to:
> 
>        //
>        // Sync BSP's Control registers to APs. Note that IDTR.BASE is different.
>        //
> 
> Which means that, from now on, we generally consider CpuData[0] to stand
> for the BSP,*except*  IDTR.BASE. (I guess we could redefine CpuData[0],
> not as "BSP", but as "initializer for APs
> 
> Thus, are we sure that the BSP never uses CpuData[0].IDTR.BASE for its
> own purposes?
> 
> Should we perhaps:
> - keep CpuData[0] unchanged,
> - introduce a new, dedicated field, "CpuMpData->ApIdtBase", and use that
>    *explicitly*  wherever necessary?
Then we will make the RestoreVolatileRegisters() a bit complex. It needs 
to explicitly restores the IDT.
I will add more comments for CpuData[0].VolatileRegisters.
> 
> 
> (7) My next question is a corollary of the above -- what about
> SwitchBSP()?
> 
> - Is SwitchBSP() compatible with this design?
Yes.
> - Will the new BSP start using the old BSP's IDT?
Yes.
> - More importantly, will the old BSP start using the IDT that is shared
>    by the APs?
Yes.
1). AsmExchangeRole () exchanges the IDT of BSP and AP.
2). After returning from AsmExchangeRole(), old BSP runs in
     ApWakeupFunction(). It saves IDTR value using below call:
SaveVolatileRegisters(&CpuMpData->CpuData[ProcessorNumber].VolatileRegisters);
     It guarantees next time when it's waken up, IDTR is restored using
     below call:
RestoreVolatileRegisters(&CpuMpData->CpuData[ProcessorNumber].VolatileRegisters, 
TRUE);
     ProcessorNumber is 0 after first SwitchBSP() call.
     Please don't worry about your (6c) restore operation. That only
     happens in InitConfig path.

> 
> Without these, I think GetCpuMpData() would break. The new BSP would go
> to the HOB, which is OK. However, the old BSP (= new AP) would go to the
> *original*  IDTR, and that's not usable for getting CpuMpData.
> 
> (I don't know how CpuData[0] relates to the*current*  -- possibly
> switched -- BSP.)
> 
> Thank you!
> Laszlo
> 


-- 
Thanks,
Ray


^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2018-07-02  5:56 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-06-29 10:42 [PATCH v3] UefiCpuPkg/MpInitLib: Avoid calling PEI services from AP Ruiyu Ni
2018-06-29 19:25 ` Laszlo Ersek
2018-07-02  5:56   ` Ni, Ruiyu

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox