* [PATCH] UefiCpuPkg/MpInitLib: AP uses memory preceding IDT to store CpuMpData
@ 2018-06-25 2:54 Ruiyu Ni
2018-06-25 16:01 ` Laszlo Ersek
2018-06-26 17:06 ` Laszlo Ersek
0 siblings, 2 replies; 13+ messages in thread
From: Ruiyu Ni @ 2018-06-25 2:54 UTC (permalink / raw)
To: edk2-devel; +Cc: Jeff Fan, Eric Dong, Jiewen Yao, Fish Andrew
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.
--
2.16.1.windows.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH] UefiCpuPkg/MpInitLib: AP uses memory preceding IDT to store CpuMpData
2018-06-25 2:54 [PATCH] UefiCpuPkg/MpInitLib: AP uses memory preceding IDT to store CpuMpData Ruiyu Ni
@ 2018-06-25 16:01 ` Laszlo Ersek
2018-06-25 17:01 ` Laszlo Ersek
2018-06-26 17:06 ` Laszlo Ersek
1 sibling, 1 reply; 13+ messages in thread
From: Laszlo Ersek @ 2018-06-25 16:01 UTC (permalink / raw)
To: Ruiyu Ni; +Cc: edk2-devel, Jiewen Yao, Eric Dong, Fish Andrew
Hello Ray,
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
[snip]
> +/**
> + 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
> + )
> +{
(1) When I applied this patch for regression-testing, "git am"
complained that the above line added two trailing space characters. Can
you please remove them?
(2) Other than that, I tested this patch with OVMF -- the patch breaks
OVMF. The last part of the log is:
> Loading PEIM [CpuMpPei]
> Loading PEIM at 0x000BFE81000 EntryPoint=0x000BFE86DC6 CpuMpPei.efi
> AP Loop Mode is 1
> WakeupBufferStart = 9F000, WakeupBufferSize = 1000
> TimedWaitForApFinish: reached FinishedApLimit=7 in 121 microseconds
> APIC MODE is 1
> MpInitLib: Find 8 processors in system.
> Does not find any stored CPU BIST information from PPI!
> APICID - 0x00000000, BIST - 0x00000000
> APICID - 0x00000001, BIST - 0x00000000
> APICID - 0x00000002, BIST - 0x00000000
> APICID - 0x00000003, BIST - 0x00000000
> APICID - 0x00000004, BIST - 0x00000000
> APICID - 0x00000005, BIST - 0x00000000
> APICID - 0x00000006, BIST - 0x00000000
> APICID - 0x00000007, BIST - 0x00000000
> Install PPI: [EfiSecPlatformInformation2Ppi]
> Install PPI: [EfiPeiMpServicesPpi]
> Notify: PPI Guid: [EfiPeiMpServicesPpi], Peim notify entry point: 8524F8
> PlatformPei: OnMpServicesAvailable
The last line is printed by OvmfPkg/PlatformPei, in
"OvmfPkg/PlatformPei/FeatureControl.c". OVMF uses
EFI_PEI_MP_SERVICES_PPI to write the Feature Control MSR on all CPUs.
https://github.com/tianocore/tianocore.github.io/wiki/Testing-SMM-with-QEMU,-KVM-and-libvirt#enable-nested-virtualization
https://github.com/tianocore/tianocore.github.io/wiki/Testing-SMM-with-QEMU,-KVM-and-libvirt#confirm-simple-multiprocessing-during-boot
Thanks,
Laszlo
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] UefiCpuPkg/MpInitLib: AP uses memory preceding IDT to store CpuMpData
2018-06-25 16:01 ` Laszlo Ersek
@ 2018-06-25 17:01 ` Laszlo Ersek
2018-06-26 7:50 ` Ni, Ruiyu
0 siblings, 1 reply; 13+ messages in thread
From: Laszlo Ersek @ 2018-06-25 17:01 UTC (permalink / raw)
To: Ruiyu Ni; +Cc: edk2-devel, Jiewen Yao, Eric Dong, Fish Andrew
On 06/25/18 18:01, Laszlo Ersek wrote:
> Hello Ray,
>
> 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
>
> [snip]
>
>> +/**
>> + 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
>> + )
>> +{
>
> (1) When I applied this patch for regression-testing, "git am"
> complained that the above line added two trailing space characters. Can
> you please remove them?
>
> (2) Other than that, I tested this patch with OVMF -- the patch breaks
> OVMF. The last part of the log is:
>
>> Loading PEIM [CpuMpPei]
>> Loading PEIM at 0x000BFE81000 EntryPoint=0x000BFE86DC6 CpuMpPei.efi
>> AP Loop Mode is 1
>> WakeupBufferStart = 9F000, WakeupBufferSize = 1000
>> TimedWaitForApFinish: reached FinishedApLimit=7 in 121 microseconds
>> APIC MODE is 1
>> MpInitLib: Find 8 processors in system.
>> Does not find any stored CPU BIST information from PPI!
>> APICID - 0x00000000, BIST - 0x00000000
>> APICID - 0x00000001, BIST - 0x00000000
>> APICID - 0x00000002, BIST - 0x00000000
>> APICID - 0x00000003, BIST - 0x00000000
>> APICID - 0x00000004, BIST - 0x00000000
>> APICID - 0x00000005, BIST - 0x00000000
>> APICID - 0x00000006, BIST - 0x00000000
>> APICID - 0x00000007, BIST - 0x00000000
>> Install PPI: [EfiSecPlatformInformation2Ppi]
>> Install PPI: [EfiPeiMpServicesPpi]
>> Notify: PPI Guid: [EfiPeiMpServicesPpi], Peim notify entry point: 8524F8
>> PlatformPei: OnMpServicesAvailable
Sorry, I failed to mention: after the last line is printed, the boot
process is stuck, and all CPUs appear spinning.
Thanks
Laszlo
> The last line is printed by OvmfPkg/PlatformPei, in
> "OvmfPkg/PlatformPei/FeatureControl.c". OVMF uses
> EFI_PEI_MP_SERVICES_PPI to write the Feature Control MSR on all CPUs.
>
> https://github.com/tianocore/tianocore.github.io/wiki/Testing-SMM-with-QEMU,-KVM-and-libvirt#enable-nested-virtualization
> https://github.com/tianocore/tianocore.github.io/wiki/Testing-SMM-with-QEMU,-KVM-and-libvirt#confirm-simple-multiprocessing-during-boot
>
> Thanks,
> Laszlo
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] UefiCpuPkg/MpInitLib: AP uses memory preceding IDT to store CpuMpData
2018-06-25 17:01 ` Laszlo Ersek
@ 2018-06-26 7:50 ` Ni, Ruiyu
2018-06-26 12:52 ` Laszlo Ersek
0 siblings, 1 reply; 13+ messages in thread
From: Ni, Ruiyu @ 2018-06-26 7:50 UTC (permalink / raw)
To: edk2-devel
On 6/26/2018 1:01 AM, Laszlo Ersek wrote:
> On 06/25/18 18:01, Laszlo Ersek wrote:
>> Hello Ray,
>>
>> 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
>>
>> [snip]
>>
>>> +/**
>>> + 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
>>> + )
>>> +{
>>
>> (1) When I applied this patch for regression-testing, "git am"
>> complained that the above line added two trailing space characters. Can
>> you please remove them?
>>
>> (2) Other than that, I tested this patch with OVMF -- the patch breaks
>> OVMF. The last part of the log is:
>>
>>> Loading PEIM [CpuMpPei]
>>> Loading PEIM at 0x000BFE81000 EntryPoint=0x000BFE86DC6 CpuMpPei.efi
>>> AP Loop Mode is 1
>>> WakeupBufferStart = 9F000, WakeupBufferSize = 1000
>>> TimedWaitForApFinish: reached FinishedApLimit=7 in 121 microseconds
>>> APIC MODE is 1
>>> MpInitLib: Find 8 processors in system.
>>> Does not find any stored CPU BIST information from PPI!
>>> APICID - 0x00000000, BIST - 0x00000000
>>> APICID - 0x00000001, BIST - 0x00000000
>>> APICID - 0x00000002, BIST - 0x00000000
>>> APICID - 0x00000003, BIST - 0x00000000
>>> APICID - 0x00000004, BIST - 0x00000000
>>> APICID - 0x00000005, BIST - 0x00000000
>>> APICID - 0x00000006, BIST - 0x00000000
>>> APICID - 0x00000007, BIST - 0x00000000
>>> Install PPI: [EfiSecPlatformInformation2Ppi]
>>> Install PPI: [EfiPeiMpServicesPpi]
>>> Notify: PPI Guid: [EfiPeiMpServicesPpi], Peim notify entry point: 8524F8
>>> PlatformPei: OnMpServicesAvailable
>
> Sorry, I failed to mention: after the last line is printed, the boot
> process is stuck, and all CPUs appear spinning.
I tried to directly boot OVMF in my Win10. The OnMpServicesAvailable()
is not called because the PPI notification is not installed.
So I tried to modify InstallFeatureControlCallback() in
PlatformPei/FeatureControl.c to unconditionally call
"PeiServicesNotifyPpi (&mMpServicesNotify);".
The new image can boot to Shell and I can find "OnMpServicesAvailable"
debug message in the log.
>
> Thanks
> Laszlo
>
>> The last line is printed by OvmfPkg/PlatformPei, in
>> "OvmfPkg/PlatformPei/FeatureControl.c". OVMF uses
>> EFI_PEI_MP_SERVICES_PPI to write the Feature Control MSR on all CPUs.
>>
>> https://github.com/tianocore/tianocore.github.io/wiki/Testing-SMM-with-QEMU,-KVM-and-libvirt#enable-nested-virtualization
>> https://github.com/tianocore/tianocore.github.io/wiki/Testing-SMM-with-QEMU,-KVM-and-libvirt#confirm-simple-multiprocessing-during-boot
>>
>> Thanks,
>> Laszlo
>> _______________________________________________
>> edk2-devel mailing list
>> edk2-devel@lists.01.org
>> https://lists.01.org/mailman/listinfo/edk2-devel
>>
>
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
>
--
Thanks,
Ray
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] UefiCpuPkg/MpInitLib: AP uses memory preceding IDT to store CpuMpData
2018-06-26 7:50 ` Ni, Ruiyu
@ 2018-06-26 12:52 ` Laszlo Ersek
0 siblings, 0 replies; 13+ messages in thread
From: Laszlo Ersek @ 2018-06-26 12:52 UTC (permalink / raw)
To: Ni, Ruiyu, edk2-devel
Hi Ray,
On 06/26/18 09:50, Ni, Ruiyu wrote:
> On 6/26/2018 1:01 AM, Laszlo Ersek wrote:
>> On 06/25/18 18:01, Laszlo Ersek wrote:
>>> Hello Ray,
>>>
>>> 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
>>>
>>> [snip]
>>>
>>>> +/**
>>>> + 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
>>>> + )
>>>> +{
>>>
>>> (1) When I applied this patch for regression-testing, "git am"
>>> complained that the above line added two trailing space characters. Can
>>> you please remove them?
>>>
>>> (2) Other than that, I tested this patch with OVMF -- the patch breaks
>>> OVMF. The last part of the log is:
>>>
>>>> Loading PEIM [CpuMpPei]
>>>> Loading PEIM at 0x000BFE81000 EntryPoint=0x000BFE86DC6 CpuMpPei.efi
>>>> AP Loop Mode is 1
>>>> WakeupBufferStart = 9F000, WakeupBufferSize = 1000
>>>> TimedWaitForApFinish: reached FinishedApLimit=7 in 121 microseconds
>>>> APIC MODE is 1
>>>> MpInitLib: Find 8 processors in system.
>>>> Does not find any stored CPU BIST information from PPI!
>>>> APICID - 0x00000000, BIST - 0x00000000
>>>> APICID - 0x00000001, BIST - 0x00000000
>>>> APICID - 0x00000002, BIST - 0x00000000
>>>> APICID - 0x00000003, BIST - 0x00000000
>>>> APICID - 0x00000004, BIST - 0x00000000
>>>> APICID - 0x00000005, BIST - 0x00000000
>>>> APICID - 0x00000006, BIST - 0x00000000
>>>> APICID - 0x00000007, BIST - 0x00000000
>>>> Install PPI: [EfiSecPlatformInformation2Ppi]
>>>> Install PPI: [EfiPeiMpServicesPpi]
>>>> Notify: PPI Guid: [EfiPeiMpServicesPpi], Peim notify entry point:
>>>> 8524F8
>>>> PlatformPei: OnMpServicesAvailable
>>
>> Sorry, I failed to mention: after the last line is printed, the boot
>> process is stuck, and all CPUs appear spinning.
>
> I tried to directly boot OVMF in my Win10. The OnMpServicesAvailable()
> is not called because the PPI notification is not installed.
> So I tried to modify InstallFeatureControlCallback() in
> PlatformPei/FeatureControl.c to unconditionally call
> "PeiServicesNotifyPpi (&mMpServicesNotify);".
> The new image can boot to Shell and I can find "OnMpServicesAvailable"
> debug message in the log.
When you run QEMU on Windows, that means that QEMU uses "TCG" (Tiny Code
Generator), which is a Just In Time compiler that runs the guest code
with software emulation. That's not the production use of QEMU.
For using QEMU with hardware virtualization (Intel vmx / AMD svm),
please run QEMU on Linux, with KVM. This is documented in the Wiki
article I referenced earlier -- the article specifically targets
developers with a Windows background. You only need one Linux server
(not desktop) for testing; for the interactive parts, you can use your
usual Windows workstation.
Thanks
Laszlo
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] UefiCpuPkg/MpInitLib: AP uses memory preceding IDT to store CpuMpData
2018-06-25 2:54 [PATCH] UefiCpuPkg/MpInitLib: AP uses memory preceding IDT to store CpuMpData Ruiyu Ni
2018-06-25 16:01 ` Laszlo Ersek
@ 2018-06-26 17:06 ` Laszlo Ersek
2018-06-26 17:20 ` Andrew Fish
2018-06-27 4:50 ` Ni, Ruiyu
1 sibling, 2 replies; 13+ messages in thread
From: Laszlo Ersek @ 2018-06-26 17:06 UTC (permalink / raw)
To: Ruiyu Ni, edk2-devel
Cc: Jiewen Yao, Eric Dong, Fish Andrew, Paolo Bonzini, Bandan Das,
Radim Krčmář
(replying again to the patch email directly, for keeping context --
adding some people to the CC list. Comments below.)
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.
>
>
I captured a KVM trace while the guest was stuck; the following messages
repeat infinitely:
> CPU-8401 [000] 5171.301018: kvm_entry: vcpu 0
> CPU-8401 [000] 5171.301019: kvm_exit: reason DR_ACCESS rip 0xbff0b28d info 17 0
> CPU-8401 [000] 5171.301019: kvm_entry: vcpu 0
> CPU-8401 [000] 5171.301050: kvm_exit: reason EXCEPTION_NMI rip 0xbff03d30 info 0 80000306
> CPU-8401 [000] 5171.301051: kvm_emulate_insn: 0:bff03d30: 60
> CPU-8401 [000] 5171.301051: kvm_inj_exception: #UD (0x0)
The final part of the OVMF log is,
> Loading PEIM at 0x000BFF05000 EntryPoint=0x000BFF0ADC6 CpuMpPei.efi
> AP Loop Mode is 1
> WakeupBufferStart = 9F000, WakeupBufferSize = 1000
> TimedWaitForApFinish: reached FinishedApLimit=7 in 0 microseconds
> APIC MODE is 1
> MpInitLib: Find 8 processors in system.
> Does not find any stored CPU BIST information from PPI!
> APICID - 0x00000000, BIST - 0x00000000
> APICID - 0x00000001, BIST - 0x00000000
> APICID - 0x00000002, BIST - 0x00000000
> APICID - 0x00000003, BIST - 0x00000000
> APICID - 0x00000004, BIST - 0x00000000
> APICID - 0x00000005, BIST - 0x00000000
> APICID - 0x00000006, BIST - 0x00000000
> APICID - 0x00000007, BIST - 0x00000000
> Install PPI: 9E9F374B-8F16-4230-9824-5846EE766A97
> Install PPI: EE16160A-E8BE-47A6-820A-C6900DB0250A
> Notify: PPI Guid: EE16160A-E8BE-47A6-820A-C6900DB0250A, Peim notify entry point: 8524F8
> PlatformPei: OnMpServicesAvailable
Note that the first address in the KVM trace, 0xBFF0B28D, is valid. It
is offset 0x628D bytes from the CpuMpPei.efi load address (0xBFF05000),
and the disassembly for the PEIM is consistent with the "DR_ACCESS"
trap:
> 00000000000061e8 <HasErrorCode>:
> 61e8: 55 push %rbp
> 61e9: 48 89 e5 mov %rsp,%rbp
> 61ec: 6a 00 pushq $0x0
> 61ee: 6a 00 pushq $0x0
> 61f0: 41 57 push %r15
> 61f2: 41 56 push %r14
> 61f4: 41 55 push %r13
> 61f6: 41 54 push %r12
> 61f8: 41 53 push %r11
> 61fa: 41 52 push %r10
> 61fc: 41 51 push %r9
> 61fe: 41 50 push %r8
> 6200: 50 push %rax
> 6201: ff 75 08 pushq 0x8(%rbp)
> 6204: 52 push %rdx
> 6205: 53 push %rbx
> 6206: ff 75 30 pushq 0x30(%rbp)
> 6209: ff 75 00 pushq 0x0(%rbp)
> 620c: 56 push %rsi
> 620d: 57 push %rdi
> 620e: 48 0f b7 45 38 movzwq 0x38(%rbp),%rax
> 6213: 50 push %rax
> 6214: 48 0f b7 45 20 movzwq 0x20(%rbp),%rax
> 6219: 50 push %rax
> 621a: 8c d8 mov %ds,%eax
> 621c: 50 push %rax
> 621d: 8c c0 mov %es,%eax
> 621f: 50 push %rax
> 6220: 8c e0 mov %fs,%eax
> 6222: 50 push %rax
> 6223: 8c e8 mov %gs,%eax
> 6225: 50 push %rax
> 6226: 48 89 4d 08 mov %rcx,0x8(%rbp)
> 622a: ff 75 18 pushq 0x18(%rbp)
> 622d: 48 31 c0 xor %rax,%rax
> 6230: 50 push %rax
> 6231: 50 push %rax
> 6232: 0f 01 0c 24 sidt (%rsp)
> 6236: 48 87 44 24 02 xchg %rax,0x2(%rsp)
> 623b: 48 87 04 24 xchg %rax,(%rsp)
> 623f: 48 87 44 24 08 xchg %rax,0x8(%rsp)
> 6244: 48 31 c0 xor %rax,%rax
> 6247: 50 push %rax
> 6248: 50 push %rax
> 6249: 0f 01 04 24 sgdt (%rsp)
> 624d: 48 87 44 24 02 xchg %rax,0x2(%rsp)
> 6252: 48 87 04 24 xchg %rax,(%rsp)
> 6256: 48 87 44 24 08 xchg %rax,0x8(%rsp)
> 625b: 48 31 c0 xor %rax,%rax
> 625e: 66 0f 00 c8 str %ax
> 6262: 50 push %rax
> 6263: 66 0f 00 c0 sldt %ax
> 6267: 50 push %rax
> 6268: ff 75 28 pushq 0x28(%rbp)
> 626b: 44 0f 20 c0 mov %cr8,%rax
> 626f: 50 push %rax
> 6270: 0f 20 e0 mov %cr4,%rax
> 6273: 48 0d 08 02 00 00 or $0x208,%rax
> 6279: 0f 22 e0 mov %rax,%cr4
> 627c: 50 push %rax
> 627d: 0f 20 d8 mov %cr3,%rax
> 6280: 50 push %rax
> 6281: 0f 20 d0 mov %cr2,%rax
> 6284: 50 push %rax
> 6285: 48 31 c0 xor %rax,%rax
> 6288: 50 push %rax
> 6289: 0f 20 c0 mov %cr0,%rax
> 628c: 50 push %rax
> 628d: 0f 21 f8 mov %db7,%rax <-------- here
> 6290: 50 push %rax
> 6291: 0f 21 f0 mov %db6,%rax
> 6294: 50 push %rax
> 6295: 0f 21 d8 mov %db3,%rax
> 6298: 50 push %rax
> 6299: 0f 21 d0 mov %db2,%rax
> 629c: 50 push %rax
> 629d: 0f 21 c8 mov %db1,%rax
> 62a0: 50 push %rax
> 62a1: 0f 21 c0 mov %db0,%rax
> 62a4: 50 push %rax
> 62a5: 48 81 ec 00 02 00 00 sub $0x200,%rsp
> 62ac: 48 89 e7 mov %rsp,%rdi
> 62af: 0f ae 07 fxsave (%rdi)
> 62b2: fc cld
> 62b3: ff 75 10 pushq 0x10(%rbp)
> 62b6: 48 8b 4d 08 mov 0x8(%rbp),%rcx
> 62ba: 48 89 e2 mov %rsp,%rdx
> 62bd: 48 83 ec 28 sub $0x28,%rsp
> 62c1: e8 61 0c 00 00 callq 6f27 <CommonExceptionHandler>
> 62c2: R_X86_64_PC32 CommonExceptionHandler-0x4
> 62c6: 48 83 c4 28 add $0x28,%rsp
> 62ca: fa cli
> 62cb: 48 83 c4 08 add $0x8,%rsp
> 62cf: 48 89 e6 mov %rsp,%rsi
> 62d2: 0f ae 0e fxrstor (%rsi)
> 62d5: 48 81 c4 00 02 00 00 add $0x200,%rsp
> 62dc: 48 83 c4 30 add $0x30,%rsp
> 62e0: 58 pop %rax
> 62e1: 0f 22 c0 mov %rax,%cr0
> 62e4: 48 83 c4 08 add $0x8,%rsp
> 62e8: 58 pop %rax
> 62e9: 0f 22 d0 mov %rax,%cr2
> 62ec: 58 pop %rax
> 62ed: 0f 22 d8 mov %rax,%cr3
> 62f0: 58 pop %rax
> 62f1: 0f 22 e0 mov %rax,%cr4
> 62f4: 58 pop %rax
> 62f5: 44 0f 22 c0 mov %rax,%cr8
> 62f9: 8f 45 28 popq 0x28(%rbp)
> 62fc: 48 83 c4 30 add $0x30,%rsp
> 6300: 8f 45 18 popq 0x18(%rbp)
> 6303: 58 pop %rax
> 6304: 58 pop %rax
> 6305: 58 pop %rax
> 6306: 8e c0 mov %eax,%es
> 6308: 58 pop %rax
> 6309: 8e d8 mov %eax,%ds
> 630b: 8f 45 20 popq 0x20(%rbp)
> 630e: 8f 45 38 popq 0x38(%rbp)
> 6311: 5f pop %rdi
> 6312: 5e pop %rsi
> 6313: 48 83 c4 08 add $0x8,%rsp
> 6317: 8f 45 30 popq 0x30(%rbp)
> 631a: 5b pop %rbx
> 631b: 5a pop %rdx
> 631c: 59 pop %rcx
> 631d: 58 pop %rax
> 631e: 41 58 pop %r8
> 6320: 41 59 pop %r9
> 6322: 41 5a pop %r10
> 6324: 41 5b pop %r11
> 6326: 41 5c pop %r12
> 6328: 41 5d pop %r13
> 632a: 41 5e pop %r14
> 632c: 41 5f pop %r15
> 632e: 48 89 ec mov %rbp,%rsp
> 6331: 5d pop %rbp
> 6332: 48 83 c4 10 add $0x10,%rsp
> 6336: 48 83 7c 24 e0 00 cmpq $0x0,-0x20(%rsp)
> 633c: 74 14 je 6352 <DoReturn>
> 633e: 48 83 7c 24 d8 01 cmpq $0x1,-0x28(%rsp)
> 6344: 74 04 je 634a <ErrorCode>
> 6346: ff 64 24 e0 jmpq *-0x20(%rsp)
(This function is from
"UefiCpuPkg/Library/CpuExceptionHandlerLib/Ia32/ExceptionHandlerAsm.S"
-- I guess it's already a problem that we are in that file at all?)
However, the opcode 0x60 at address 0xBFF03D30, which triggers the #UD
exception ("invalid opcode"), is *below* the "CpuMpPei.efi" load address
(by 0x12D0 bytes).
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?
Here's a register dump, to see where the IDT is:
> $ virsh qemu-monitor-command ovmf.fedora --hmp 'info registers'
>
> RAX=0000000000000000 RBX=00000000008524f8 RCX=00000000bfeebd30 RDX=ffffffffffffffff
> RSI=00000000bbf1c068 RDI=00000000bfeebd30 RBP=00000000bbf1bee0 RSP=00000000bbf1bea0
> R8 =0000000000000001 R9 =0000000000000000 R10=0000000000000000 R11=00000000000000b0
> R12=00000000bff14b60 R13=0000000000000000 R14=0000000000000000 R15=0000000000000000
> RIP=00000000bff090b3 RFL=00000093 [--S-A-C] CPL=0 II=0 A20=1 SMM=0 HLT=0
> ES =0008 0000000000000000 ffffffff 00c09300 DPL=0 DS [-WA]
> CS =0018 0000000000000000 ffffffff 00a09b00 DPL=0 CS64 [-RA]
> SS =0008 0000000000000000 ffffffff 00c09300 DPL=0 DS [-WA]
> DS =0008 0000000000000000 ffffffff 00c09300 DPL=0 DS [-WA]
> FS =0008 0000000000000000 ffffffff 00c09300 DPL=0 DS [-WA]
> GS =0008 0000000000000000 ffffffff 00c09300 DPL=0 DS [-WA]
> LDT=0000 0000000000000000 0000ffff 00008200 DPL=0 LDT
> TR =0000 0000000000000000 0000ffff 00008b00 DPL=0 TSS64-busy
> GDT= 00000000ffffff80 0000001f
> IDT= 00000000bbf1dd58 0000021f
> CR0=80000033 CR2=0000000000000000 CR3=0000000000800000 CR4=00000668
> DR0=0000000000000000 DR1=0000000000000000 DR2=0000000000000000 DR3=0000000000000000
> DR6=00000000ffff0ff0 DR7=0000000000000400
> EFER=0000000000000500
> FCW=037f FSW=0000 [ST=0] FTW=00 MXCSR=00001f80
> FPR0=0000000000000000 0000 FPR1=0000000000000000 0000
> FPR2=0000000000000000 0000 FPR3=0000000000000000 0000
> FPR4=0000000000000000 0000 FPR5=0000000000000000 0000
> FPR6=0000000000000000 0000 FPR7=0000000000000000 0000
> XMM00=00000000000000000000000000000000 XMM01=00000000000000000000000000000000
> XMM02=00000000000000000000000000000000 XMM03=00000000000000000000000000000000
> XMM04=00000000000000000000000000000000 XMM05=00000000000000000000000000000000
> XMM06=00000000000000000000000000000000 XMM07=00000000000000000000000000000000
> XMM08=00000000000000000000000000000000 XMM09=00000000000000000000000000000000
> XMM10=00000000000000000000000000000000 XMM11=00000000000000000000000000000000
> XMM12=00000000000000000000000000000000 XMM13=00000000000000000000000000000000
> XMM14=00000000000000000000000000000000 XMM15=00000000000000000000000000000000
The IDT base address 0xBBF1DD58 doesn't tell me anything, unfortunately.
Here's a dump of the memory starting at (0xBBF1DD58 - 8):
> $ virsh qemu-monitor-command ovmf.fedora --hmp 'xp /32gx 0xBBF1DD50'
>
> 00000000bbf1dd50: 0x00000000bbf1cac8 0xbff08e000018afb0
> 00000000bbf1dd60: 0x0000000000000000 0xbff08e000018afbf
> 00000000bbf1dd70: 0x0000000000000000 0xbff08e000018afce
> 00000000bbf1dd80: 0x0000000000000000 0xbff08e000018afdd
> 00000000bbf1dd90: 0x0000000000000000 0xbff08e000018afec
> 00000000bbf1dda0: 0x0000000000000000 0xbff08e000018affb
> 00000000bbf1ddb0: 0x0000000000000000 0xbff08e000018b00a
> 00000000bbf1ddc0: 0x0000000000000000 0xbff08e000018b019
> 00000000bbf1ddd0: 0x0000000000000000 0xbff08e000018b028
> 00000000bbf1dde0: 0x0000000000000000 0xbff08e000018b037
> 00000000bbf1ddf0: 0x0000000000000000 0xbff08e000018b046
> 00000000bbf1de00: 0x0000000000000000 0xbff08e000018b055
> 00000000bbf1de10: 0x0000000000000000 0xbff08e000018b064
> 00000000bbf1de20: 0x0000000000000000 0xbff08e000018b073
> 00000000bbf1de30: 0x0000000000000000 0xbff08e000018b082
> 00000000bbf1de40: 0x0000000000000000 0xbff08e000018b091
Thanks
Laszlo
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] UefiCpuPkg/MpInitLib: AP uses memory preceding IDT to store CpuMpData
2018-06-26 17:06 ` Laszlo Ersek
@ 2018-06-26 17:20 ` Andrew Fish
2018-06-26 18:57 ` Laszlo Ersek
2018-06-27 5:06 ` Ni, Ruiyu
2018-06-27 4:50 ` Ni, Ruiyu
1 sibling, 2 replies; 13+ messages in thread
From: Andrew Fish @ 2018-06-26 17:20 UTC (permalink / raw)
To: Laszlo Ersek
Cc: Ruiyu Ni, edk2-devel, Eric Dong, Radim Krčmář,
Bandan Das, Jiewen Yao, Paolo Bonzini
> On Jun 26, 2018, at 10:06 AM, Laszlo Ersek <lersek@redhat.com> wrote:
>
> (replying again to the patch email directly, for keeping context --
> adding some people to the CC list. Comments below.)
>
> 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.
>>
>>
>
> I captured a KVM trace while the guest was stuck; the following messages
> repeat infinitely:
>
>> CPU-8401 [000] 5171.301018: kvm_entry: vcpu 0
>> CPU-8401 [000] 5171.301019: kvm_exit: reason DR_ACCESS rip 0xbff0b28d info 17 0
>> CPU-8401 [000] 5171.301019: kvm_entry: vcpu 0
>> CPU-8401 [000] 5171.301050: kvm_exit: reason EXCEPTION_NMI rip 0xbff03d30 info 0 80000306
>> CPU-8401 [000] 5171.301051: kvm_emulate_insn: 0:bff03d30: 60
>> CPU-8401 [000] 5171.301051: kvm_inj_exception: #UD (0x0)
>
> The final part of the OVMF log is,
>
>> Loading PEIM at 0x000BFF05000 EntryPoint=0x000BFF0ADC6 CpuMpPei.efi
>> AP Loop Mode is 1
>> WakeupBufferStart = 9F000, WakeupBufferSize = 1000
>> TimedWaitForApFinish: reached FinishedApLimit=7 in 0 microseconds
>> APIC MODE is 1
>> MpInitLib: Find 8 processors in system.
>> Does not find any stored CPU BIST information from PPI!
>> APICID - 0x00000000, BIST - 0x00000000
>> APICID - 0x00000001, BIST - 0x00000000
>> APICID - 0x00000002, BIST - 0x00000000
>> APICID - 0x00000003, BIST - 0x00000000
>> APICID - 0x00000004, BIST - 0x00000000
>> APICID - 0x00000005, BIST - 0x00000000
>> APICID - 0x00000006, BIST - 0x00000000
>> APICID - 0x00000007, BIST - 0x00000000
>> Install PPI: 9E9F374B-8F16-4230-9824-5846EE766A97
>> Install PPI: EE16160A-E8BE-47A6-820A-C6900DB0250A
>> Notify: PPI Guid: EE16160A-E8BE-47A6-820A-C6900DB0250A, Peim notify entry point: 8524F8
>> PlatformPei: OnMpServicesAvailable
>
> Note that the first address in the KVM trace, 0xBFF0B28D, is valid. It
> is offset 0x628D bytes from the CpuMpPei.efi load address (0xBFF05000),
> and the disassembly for the PEIM is consistent with the "DR_ACCESS"
> trap:
>
>> 00000000000061e8 <HasErrorCode>:
>> 61e8: 55 push %rbp
>> 61e9: 48 89 e5 mov %rsp,%rbp
>> 61ec: 6a 00 pushq $0x0
>> 61ee: 6a 00 pushq $0x0
>> 61f0: 41 57 push %r15
>> 61f2: 41 56 push %r14
>> 61f4: 41 55 push %r13
>> 61f6: 41 54 push %r12
>> 61f8: 41 53 push %r11
>> 61fa: 41 52 push %r10
>> 61fc: 41 51 push %r9
>> 61fe: 41 50 push %r8
>> 6200: 50 push %rax
>> 6201: ff 75 08 pushq 0x8(%rbp)
>> 6204: 52 push %rdx
>> 6205: 53 push %rbx
>> 6206: ff 75 30 pushq 0x30(%rbp)
>> 6209: ff 75 00 pushq 0x0(%rbp)
>> 620c: 56 push %rsi
>> 620d: 57 push %rdi
>> 620e: 48 0f b7 45 38 movzwq 0x38(%rbp),%rax
>> 6213: 50 push %rax
>> 6214: 48 0f b7 45 20 movzwq 0x20(%rbp),%rax
>> 6219: 50 push %rax
>> 621a: 8c d8 mov %ds,%eax
>> 621c: 50 push %rax
>> 621d: 8c c0 mov %es,%eax
>> 621f: 50 push %rax
>> 6220: 8c e0 mov %fs,%eax
>> 6222: 50 push %rax
>> 6223: 8c e8 mov %gs,%eax
>> 6225: 50 push %rax
>> 6226: 48 89 4d 08 mov %rcx,0x8(%rbp)
>> 622a: ff 75 18 pushq 0x18(%rbp)
>> 622d: 48 31 c0 xor %rax,%rax
>> 6230: 50 push %rax
>> 6231: 50 push %rax
>> 6232: 0f 01 0c 24 sidt (%rsp)
>> 6236: 48 87 44 24 02 xchg %rax,0x2(%rsp)
>> 623b: 48 87 04 24 xchg %rax,(%rsp)
>> 623f: 48 87 44 24 08 xchg %rax,0x8(%rsp)
>> 6244: 48 31 c0 xor %rax,%rax
>> 6247: 50 push %rax
>> 6248: 50 push %rax
>> 6249: 0f 01 04 24 sgdt (%rsp)
>> 624d: 48 87 44 24 02 xchg %rax,0x2(%rsp)
>> 6252: 48 87 04 24 xchg %rax,(%rsp)
>> 6256: 48 87 44 24 08 xchg %rax,0x8(%rsp)
>> 625b: 48 31 c0 xor %rax,%rax
>> 625e: 66 0f 00 c8 str %ax
>> 6262: 50 push %rax
>> 6263: 66 0f 00 c0 sldt %ax
>> 6267: 50 push %rax
>> 6268: ff 75 28 pushq 0x28(%rbp)
>> 626b: 44 0f 20 c0 mov %cr8,%rax
>> 626f: 50 push %rax
>> 6270: 0f 20 e0 mov %cr4,%rax
>> 6273: 48 0d 08 02 00 00 or $0x208,%rax
>> 6279: 0f 22 e0 mov %rax,%cr4
>> 627c: 50 push %rax
>> 627d: 0f 20 d8 mov %cr3,%rax
>> 6280: 50 push %rax
>> 6281: 0f 20 d0 mov %cr2,%rax
>> 6284: 50 push %rax
>> 6285: 48 31 c0 xor %rax,%rax
>> 6288: 50 push %rax
>> 6289: 0f 20 c0 mov %cr0,%rax
>> 628c: 50 push %rax
>> 628d: 0f 21 f8 mov %db7,%rax <-------- here
>> 6290: 50 push %rax
>> 6291: 0f 21 f0 mov %db6,%rax
>> 6294: 50 push %rax
>> 6295: 0f 21 d8 mov %db3,%rax
>> 6298: 50 push %rax
>> 6299: 0f 21 d0 mov %db2,%rax
>> 629c: 50 push %rax
>> 629d: 0f 21 c8 mov %db1,%rax
>> 62a0: 50 push %rax
>> 62a1: 0f 21 c0 mov %db0,%rax
>> 62a4: 50 push %rax
>> 62a5: 48 81 ec 00 02 00 00 sub $0x200,%rsp
>> 62ac: 48 89 e7 mov %rsp,%rdi
>> 62af: 0f ae 07 fxsave (%rdi)
>> 62b2: fc cld
>> 62b3: ff 75 10 pushq 0x10(%rbp)
>> 62b6: 48 8b 4d 08 mov 0x8(%rbp),%rcx
>> 62ba: 48 89 e2 mov %rsp,%rdx
>> 62bd: 48 83 ec 28 sub $0x28,%rsp
>> 62c1: e8 61 0c 00 00 callq 6f27 <CommonExceptionHandler>
>> 62c2: R_X86_64_PC32 CommonExceptionHandler-0x4
>> 62c6: 48 83 c4 28 add $0x28,%rsp
>> 62ca: fa cli
>> 62cb: 48 83 c4 08 add $0x8,%rsp
>> 62cf: 48 89 e6 mov %rsp,%rsi
>> 62d2: 0f ae 0e fxrstor (%rsi)
>> 62d5: 48 81 c4 00 02 00 00 add $0x200,%rsp
>> 62dc: 48 83 c4 30 add $0x30,%rsp
>> 62e0: 58 pop %rax
>> 62e1: 0f 22 c0 mov %rax,%cr0
>> 62e4: 48 83 c4 08 add $0x8,%rsp
>> 62e8: 58 pop %rax
>> 62e9: 0f 22 d0 mov %rax,%cr2
>> 62ec: 58 pop %rax
>> 62ed: 0f 22 d8 mov %rax,%cr3
>> 62f0: 58 pop %rax
>> 62f1: 0f 22 e0 mov %rax,%cr4
>> 62f4: 58 pop %rax
>> 62f5: 44 0f 22 c0 mov %rax,%cr8
>> 62f9: 8f 45 28 popq 0x28(%rbp)
>> 62fc: 48 83 c4 30 add $0x30,%rsp
>> 6300: 8f 45 18 popq 0x18(%rbp)
>> 6303: 58 pop %rax
>> 6304: 58 pop %rax
>> 6305: 58 pop %rax
>> 6306: 8e c0 mov %eax,%es
>> 6308: 58 pop %rax
>> 6309: 8e d8 mov %eax,%ds
>> 630b: 8f 45 20 popq 0x20(%rbp)
>> 630e: 8f 45 38 popq 0x38(%rbp)
>> 6311: 5f pop %rdi
>> 6312: 5e pop %rsi
>> 6313: 48 83 c4 08 add $0x8,%rsp
>> 6317: 8f 45 30 popq 0x30(%rbp)
>> 631a: 5b pop %rbx
>> 631b: 5a pop %rdx
>> 631c: 59 pop %rcx
>> 631d: 58 pop %rax
>> 631e: 41 58 pop %r8
>> 6320: 41 59 pop %r9
>> 6322: 41 5a pop %r10
>> 6324: 41 5b pop %r11
>> 6326: 41 5c pop %r12
>> 6328: 41 5d pop %r13
>> 632a: 41 5e pop %r14
>> 632c: 41 5f pop %r15
>> 632e: 48 89 ec mov %rbp,%rsp
>> 6331: 5d pop %rbp
>> 6332: 48 83 c4 10 add $0x10,%rsp
>> 6336: 48 83 7c 24 e0 00 cmpq $0x0,-0x20(%rsp)
>> 633c: 74 14 je 6352 <DoReturn>
>> 633e: 48 83 7c 24 d8 01 cmpq $0x1,-0x28(%rsp)
>> 6344: 74 04 je 634a <ErrorCode>
>> 6346: ff 64 24 e0 jmpq *-0x20(%rsp)
>
> (This function is from
> "UefiCpuPkg/Library/CpuExceptionHandlerLib/Ia32/ExceptionHandlerAsm.S"
> -- I guess it's already a problem that we are in that file at all?)
>
> However, the opcode 0x60 at address 0xBFF03D30, which triggers the #UD
> exception ("invalid opcode"), is *below* the "CpuMpPei.efi" load address
> (by 0x12D0 bytes).
>
>
> 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.
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.
Ironically I'm in the process of making a change that uses Itdr.Base - (2*sizeof(UINTN)) to store debugging context. Not sure if that is what this code is doing?
Thanks,
Andrew Fish
> Here's a register dump, to see where the IDT is:
>
>> $ virsh qemu-monitor-command ovmf.fedora --hmp 'info registers'
>>
>> RAX=0000000000000000 RBX=00000000008524f8 RCX=00000000bfeebd30 RDX=ffffffffffffffff
>> RSI=00000000bbf1c068 RDI=00000000bfeebd30 RBP=00000000bbf1bee0 RSP=00000000bbf1bea0
>> R8 =0000000000000001 R9 =0000000000000000 R10=0000000000000000 R11=00000000000000b0
>> R12=00000000bff14b60 R13=0000000000000000 R14=0000000000000000 R15=0000000000000000
>> RIP=00000000bff090b3 RFL=00000093 [--S-A-C] CPL=0 II=0 A20=1 SMM=0 HLT=0
>> ES =0008 0000000000000000 ffffffff 00c09300 DPL=0 DS [-WA]
>> CS =0018 0000000000000000 ffffffff 00a09b00 DPL=0 CS64 [-RA]
>> SS =0008 0000000000000000 ffffffff 00c09300 DPL=0 DS [-WA]
>> DS =0008 0000000000000000 ffffffff 00c09300 DPL=0 DS [-WA]
>> FS =0008 0000000000000000 ffffffff 00c09300 DPL=0 DS [-WA]
>> GS =0008 0000000000000000 ffffffff 00c09300 DPL=0 DS [-WA]
>> LDT=0000 0000000000000000 0000ffff 00008200 DPL=0 LDT
>> TR =0000 0000000000000000 0000ffff 00008b00 DPL=0 TSS64-busy
>> GDT= 00000000ffffff80 0000001f
>> IDT= 00000000bbf1dd58 0000021f
>> CR0=80000033 CR2=0000000000000000 CR3=0000000000800000 CR4=00000668
>> DR0=0000000000000000 DR1=0000000000000000 DR2=0000000000000000 DR3=0000000000000000
>> DR6=00000000ffff0ff0 DR7=0000000000000400
>> EFER=0000000000000500
>> FCW=037f FSW=0000 [ST=0] FTW=00 MXCSR=00001f80
>> FPR0=0000000000000000 0000 FPR1=0000000000000000 0000
>> FPR2=0000000000000000 0000 FPR3=0000000000000000 0000
>> FPR4=0000000000000000 0000 FPR5=0000000000000000 0000
>> FPR6=0000000000000000 0000 FPR7=0000000000000000 0000
>> XMM00=00000000000000000000000000000000 XMM01=00000000000000000000000000000000
>> XMM02=00000000000000000000000000000000 XMM03=00000000000000000000000000000000
>> XMM04=00000000000000000000000000000000 XMM05=00000000000000000000000000000000
>> XMM06=00000000000000000000000000000000 XMM07=00000000000000000000000000000000
>> XMM08=00000000000000000000000000000000 XMM09=00000000000000000000000000000000
>> XMM10=00000000000000000000000000000000 XMM11=00000000000000000000000000000000
>> XMM12=00000000000000000000000000000000 XMM13=00000000000000000000000000000000
>> XMM14=00000000000000000000000000000000 XMM15=00000000000000000000000000000000
>
> The IDT base address 0xBBF1DD58 doesn't tell me anything, unfortunately.
> Here's a dump of the memory starting at (0xBBF1DD58 - 8):
>
>> $ virsh qemu-monitor-command ovmf.fedora --hmp 'xp /32gx 0xBBF1DD50'
>>
>> 00000000bbf1dd50: 0x00000000bbf1cac8 0xbff08e000018afb0
>> 00000000bbf1dd60: 0x0000000000000000 0xbff08e000018afbf
>> 00000000bbf1dd70: 0x0000000000000000 0xbff08e000018afce
>> 00000000bbf1dd80: 0x0000000000000000 0xbff08e000018afdd
>> 00000000bbf1dd90: 0x0000000000000000 0xbff08e000018afec
>> 00000000bbf1dda0: 0x0000000000000000 0xbff08e000018affb
>> 00000000bbf1ddb0: 0x0000000000000000 0xbff08e000018b00a
>> 00000000bbf1ddc0: 0x0000000000000000 0xbff08e000018b019
>> 00000000bbf1ddd0: 0x0000000000000000 0xbff08e000018b028
>> 00000000bbf1dde0: 0x0000000000000000 0xbff08e000018b037
>> 00000000bbf1ddf0: 0x0000000000000000 0xbff08e000018b046
>> 00000000bbf1de00: 0x0000000000000000 0xbff08e000018b055
>> 00000000bbf1de10: 0x0000000000000000 0xbff08e000018b064
>> 00000000bbf1de20: 0x0000000000000000 0xbff08e000018b073
>> 00000000bbf1de30: 0x0000000000000000 0xbff08e000018b082
>> 00000000bbf1de40: 0x0000000000000000 0xbff08e000018b091
>
> Thanks
> Laszlo
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org <mailto:edk2-devel@lists.01.org>
> https://lists.01.org/mailman/listinfo/edk2-devel <https://lists.01.org/mailman/listinfo/edk2-devel>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] UefiCpuPkg/MpInitLib: AP uses memory preceding IDT to store CpuMpData
2018-06-26 17:20 ` Andrew Fish
@ 2018-06-26 18:57 ` Laszlo Ersek
2018-06-27 6:00 ` Ni, Ruiyu
2018-06-27 5:06 ` Ni, Ruiyu
1 sibling, 1 reply; 13+ messages in thread
From: Laszlo Ersek @ 2018-06-26 18:57 UTC (permalink / raw)
To: Andrew Fish
Cc: Ruiyu Ni, edk2-devel, Eric Dong, Radim Krčmář,
Bandan Das, Jiewen Yao, Paolo Bonzini
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] UefiCpuPkg/MpInitLib: AP uses memory preceding IDT to store CpuMpData
2018-06-26 17:06 ` Laszlo Ersek
2018-06-26 17:20 ` Andrew Fish
@ 2018-06-27 4:50 ` Ni, Ruiyu
1 sibling, 0 replies; 13+ messages in thread
From: Ni, Ruiyu @ 2018-06-27 4:50 UTC (permalink / raw)
To: Laszlo Ersek, edk2-devel
Cc: Jiewen Yao, Eric Dong, Fish Andrew, Paolo Bonzini, Bandan Das,
Radim Krčmář
On 6/27/2018 1:06 AM, Laszlo Ersek wrote:
> (replying again to the patch email directly, for keeping context --
> adding some people to the CC list. Comments below.)
>
> 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.
>>
>>
>
> I captured a KVM trace while the guest was stuck; the following messages
> repeat infinitely:
>
>> CPU-8401 [000] 5171.301018: kvm_entry: vcpu 0
>> CPU-8401 [000] 5171.301019: kvm_exit: reason DR_ACCESS rip 0xbff0b28d info 17 0
>> CPU-8401 [000] 5171.301019: kvm_entry: vcpu 0
>> CPU-8401 [000] 5171.301050: kvm_exit: reason EXCEPTION_NMI rip 0xbff03d30 info 0 80000306
>> CPU-8401 [000] 5171.301051: kvm_emulate_insn: 0:bff03d30: 60
>> CPU-8401 [000] 5171.301051: kvm_inj_exception: #UD (0x0)
>
> The final part of the OVMF log is,
>
>> Loading PEIM at 0x000BFF05000 EntryPoint=0x000BFF0ADC6 CpuMpPei.efi
>> AP Loop Mode is 1
>> WakeupBufferStart = 9F000, WakeupBufferSize = 1000
>> TimedWaitForApFinish: reached FinishedApLimit=7 in 0 microseconds
>> APIC MODE is 1
>> MpInitLib: Find 8 processors in system.
>> Does not find any stored CPU BIST information from PPI!
>> APICID - 0x00000000, BIST - 0x00000000
>> APICID - 0x00000001, BIST - 0x00000000
>> APICID - 0x00000002, BIST - 0x00000000
>> APICID - 0x00000003, BIST - 0x00000000
>> APICID - 0x00000004, BIST - 0x00000000
>> APICID - 0x00000005, BIST - 0x00000000
>> APICID - 0x00000006, BIST - 0x00000000
>> APICID - 0x00000007, BIST - 0x00000000
>> Install PPI: 9E9F374B-8F16-4230-9824-5846EE766A97
>> Install PPI: EE16160A-E8BE-47A6-820A-C6900DB0250A
>> Notify: PPI Guid: EE16160A-E8BE-47A6-820A-C6900DB0250A, Peim notify entry point: 8524F8
>> PlatformPei: OnMpServicesAvailable
>
> Note that the first address in the KVM trace, 0xBFF0B28D, is valid. It
> is offset 0x628D bytes from the CpuMpPei.efi load address (0xBFF05000),
> and the disassembly for the PEIM is consistent with the "DR_ACCESS"
> trap:
>
>> 00000000000061e8 <HasErrorCode>:
>> 61e8: 55 push %rbp
>> 61e9: 48 89 e5 mov %rsp,%rbp
>> 61ec: 6a 00 pushq $0x0
>> 61ee: 6a 00 pushq $0x0
>> 61f0: 41 57 push %r15
>> 61f2: 41 56 push %r14
>> 61f4: 41 55 push %r13
>> 61f6: 41 54 push %r12
>> 61f8: 41 53 push %r11
>> 61fa: 41 52 push %r10
>> 61fc: 41 51 push %r9
>> 61fe: 41 50 push %r8
>> 6200: 50 push %rax
>> 6201: ff 75 08 pushq 0x8(%rbp)
>> 6204: 52 push %rdx
>> 6205: 53 push %rbx
>> 6206: ff 75 30 pushq 0x30(%rbp)
>> 6209: ff 75 00 pushq 0x0(%rbp)
>> 620c: 56 push %rsi
>> 620d: 57 push %rdi
>> 620e: 48 0f b7 45 38 movzwq 0x38(%rbp),%rax
>> 6213: 50 push %rax
>> 6214: 48 0f b7 45 20 movzwq 0x20(%rbp),%rax
>> 6219: 50 push %rax
>> 621a: 8c d8 mov %ds,%eax
>> 621c: 50 push %rax
>> 621d: 8c c0 mov %es,%eax
>> 621f: 50 push %rax
>> 6220: 8c e0 mov %fs,%eax
>> 6222: 50 push %rax
>> 6223: 8c e8 mov %gs,%eax
>> 6225: 50 push %rax
>> 6226: 48 89 4d 08 mov %rcx,0x8(%rbp)
>> 622a: ff 75 18 pushq 0x18(%rbp)
>> 622d: 48 31 c0 xor %rax,%rax
>> 6230: 50 push %rax
>> 6231: 50 push %rax
>> 6232: 0f 01 0c 24 sidt (%rsp)
>> 6236: 48 87 44 24 02 xchg %rax,0x2(%rsp)
>> 623b: 48 87 04 24 xchg %rax,(%rsp)
>> 623f: 48 87 44 24 08 xchg %rax,0x8(%rsp)
>> 6244: 48 31 c0 xor %rax,%rax
>> 6247: 50 push %rax
>> 6248: 50 push %rax
>> 6249: 0f 01 04 24 sgdt (%rsp)
>> 624d: 48 87 44 24 02 xchg %rax,0x2(%rsp)
>> 6252: 48 87 04 24 xchg %rax,(%rsp)
>> 6256: 48 87 44 24 08 xchg %rax,0x8(%rsp)
>> 625b: 48 31 c0 xor %rax,%rax
>> 625e: 66 0f 00 c8 str %ax
>> 6262: 50 push %rax
>> 6263: 66 0f 00 c0 sldt %ax
>> 6267: 50 push %rax
>> 6268: ff 75 28 pushq 0x28(%rbp)
>> 626b: 44 0f 20 c0 mov %cr8,%rax
>> 626f: 50 push %rax
>> 6270: 0f 20 e0 mov %cr4,%rax
>> 6273: 48 0d 08 02 00 00 or $0x208,%rax
>> 6279: 0f 22 e0 mov %rax,%cr4
>> 627c: 50 push %rax
>> 627d: 0f 20 d8 mov %cr3,%rax
>> 6280: 50 push %rax
>> 6281: 0f 20 d0 mov %cr2,%rax
>> 6284: 50 push %rax
>> 6285: 48 31 c0 xor %rax,%rax
>> 6288: 50 push %rax
>> 6289: 0f 20 c0 mov %cr0,%rax
>> 628c: 50 push %rax
>> 628d: 0f 21 f8 mov %db7,%rax <-------- here
>> 6290: 50 push %rax
>> 6291: 0f 21 f0 mov %db6,%rax
>> 6294: 50 push %rax
>> 6295: 0f 21 d8 mov %db3,%rax
>> 6298: 50 push %rax
>> 6299: 0f 21 d0 mov %db2,%rax
>> 629c: 50 push %rax
>> 629d: 0f 21 c8 mov %db1,%rax
>> 62a0: 50 push %rax
>> 62a1: 0f 21 c0 mov %db0,%rax
>> 62a4: 50 push %rax
>> 62a5: 48 81 ec 00 02 00 00 sub $0x200,%rsp
>> 62ac: 48 89 e7 mov %rsp,%rdi
>> 62af: 0f ae 07 fxsave (%rdi)
>> 62b2: fc cld
>> 62b3: ff 75 10 pushq 0x10(%rbp)
>> 62b6: 48 8b 4d 08 mov 0x8(%rbp),%rcx
>> 62ba: 48 89 e2 mov %rsp,%rdx
>> 62bd: 48 83 ec 28 sub $0x28,%rsp
>> 62c1: e8 61 0c 00 00 callq 6f27 <CommonExceptionHandler>
>> 62c2: R_X86_64_PC32 CommonExceptionHandler-0x4
>> 62c6: 48 83 c4 28 add $0x28,%rsp
>> 62ca: fa cli
>> 62cb: 48 83 c4 08 add $0x8,%rsp
>> 62cf: 48 89 e6 mov %rsp,%rsi
>> 62d2: 0f ae 0e fxrstor (%rsi)
>> 62d5: 48 81 c4 00 02 00 00 add $0x200,%rsp
>> 62dc: 48 83 c4 30 add $0x30,%rsp
>> 62e0: 58 pop %rax
>> 62e1: 0f 22 c0 mov %rax,%cr0
>> 62e4: 48 83 c4 08 add $0x8,%rsp
>> 62e8: 58 pop %rax
>> 62e9: 0f 22 d0 mov %rax,%cr2
>> 62ec: 58 pop %rax
>> 62ed: 0f 22 d8 mov %rax,%cr3
>> 62f0: 58 pop %rax
>> 62f1: 0f 22 e0 mov %rax,%cr4
>> 62f4: 58 pop %rax
>> 62f5: 44 0f 22 c0 mov %rax,%cr8
>> 62f9: 8f 45 28 popq 0x28(%rbp)
>> 62fc: 48 83 c4 30 add $0x30,%rsp
>> 6300: 8f 45 18 popq 0x18(%rbp)
>> 6303: 58 pop %rax
>> 6304: 58 pop %rax
>> 6305: 58 pop %rax
>> 6306: 8e c0 mov %eax,%es
>> 6308: 58 pop %rax
>> 6309: 8e d8 mov %eax,%ds
>> 630b: 8f 45 20 popq 0x20(%rbp)
>> 630e: 8f 45 38 popq 0x38(%rbp)
>> 6311: 5f pop %rdi
>> 6312: 5e pop %rsi
>> 6313: 48 83 c4 08 add $0x8,%rsp
>> 6317: 8f 45 30 popq 0x30(%rbp)
>> 631a: 5b pop %rbx
>> 631b: 5a pop %rdx
>> 631c: 59 pop %rcx
>> 631d: 58 pop %rax
>> 631e: 41 58 pop %r8
>> 6320: 41 59 pop %r9
>> 6322: 41 5a pop %r10
>> 6324: 41 5b pop %r11
>> 6326: 41 5c pop %r12
>> 6328: 41 5d pop %r13
>> 632a: 41 5e pop %r14
>> 632c: 41 5f pop %r15
>> 632e: 48 89 ec mov %rbp,%rsp
>> 6331: 5d pop %rbp
>> 6332: 48 83 c4 10 add $0x10,%rsp
>> 6336: 48 83 7c 24 e0 00 cmpq $0x0,-0x20(%rsp)
>> 633c: 74 14 je 6352 <DoReturn>
>> 633e: 48 83 7c 24 d8 01 cmpq $0x1,-0x28(%rsp)
>> 6344: 74 04 je 634a <ErrorCode>
>> 6346: ff 64 24 e0 jmpq *-0x20(%rsp)
>
> (This function is from
> "UefiCpuPkg/Library/CpuExceptionHandlerLib/Ia32/ExceptionHandlerAsm.S"
> -- I guess it's already a problem that we are in that file at all?)
>
> However, the opcode 0x60 at address 0xBFF03D30, which triggers the #UD
> exception ("invalid opcode"), is *below* the "CpuMpPei.efi" load address
> (by 0x12D0 bytes).
>
>
> 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?
Memory preceding IDT stores the pointer that points to Pei Services
pointer. It's defined by PI Spec so that there is no need to pass
PeiServices pointer to every PPI interfaces.
In OvmfPkg/Sec/SecMain.c, SecCoreStartupWithStack() initializes the IDT.
You could check SEC_IDT_TABLE structure.
>
> ... The commit message says, "memory preceding IDT", and the patch says
> "(Idtr.Base - sizeof (UINTN))". What memory is supposed to be there?
>
> Here's a register dump, to see where the IDT is:
>
>> $ virsh qemu-monitor-command ovmf.fedora --hmp 'info registers'
>>
>> RAX=0000000000000000 RBX=00000000008524f8 RCX=00000000bfeebd30 RDX=ffffffffffffffff
>> RSI=00000000bbf1c068 RDI=00000000bfeebd30 RBP=00000000bbf1bee0 RSP=00000000bbf1bea0
>> R8 =0000000000000001 R9 =0000000000000000 R10=0000000000000000 R11=00000000000000b0
>> R12=00000000bff14b60 R13=0000000000000000 R14=0000000000000000 R15=0000000000000000
>> RIP=00000000bff090b3 RFL=00000093 [--S-A-C] CPL=0 II=0 A20=1 SMM=0 HLT=0
>> ES =0008 0000000000000000 ffffffff 00c09300 DPL=0 DS [-WA]
>> CS =0018 0000000000000000 ffffffff 00a09b00 DPL=0 CS64 [-RA]
>> SS =0008 0000000000000000 ffffffff 00c09300 DPL=0 DS [-WA]
>> DS =0008 0000000000000000 ffffffff 00c09300 DPL=0 DS [-WA]
>> FS =0008 0000000000000000 ffffffff 00c09300 DPL=0 DS [-WA]
>> GS =0008 0000000000000000 ffffffff 00c09300 DPL=0 DS [-WA]
>> LDT=0000 0000000000000000 0000ffff 00008200 DPL=0 LDT
>> TR =0000 0000000000000000 0000ffff 00008b00 DPL=0 TSS64-busy
>> GDT= 00000000ffffff80 0000001f
>> IDT= 00000000bbf1dd58 0000021f
>> CR0=80000033 CR2=0000000000000000 CR3=0000000000800000 CR4=00000668
>> DR0=0000000000000000 DR1=0000000000000000 DR2=0000000000000000 DR3=0000000000000000
>> DR6=00000000ffff0ff0 DR7=0000000000000400
>> EFER=0000000000000500
>> FCW=037f FSW=0000 [ST=0] FTW=00 MXCSR=00001f80
>> FPR0=0000000000000000 0000 FPR1=0000000000000000 0000
>> FPR2=0000000000000000 0000 FPR3=0000000000000000 0000
>> FPR4=0000000000000000 0000 FPR5=0000000000000000 0000
>> FPR6=0000000000000000 0000 FPR7=0000000000000000 0000
>> XMM00=00000000000000000000000000000000 XMM01=00000000000000000000000000000000
>> XMM02=00000000000000000000000000000000 XMM03=00000000000000000000000000000000
>> XMM04=00000000000000000000000000000000 XMM05=00000000000000000000000000000000
>> XMM06=00000000000000000000000000000000 XMM07=00000000000000000000000000000000
>> XMM08=00000000000000000000000000000000 XMM09=00000000000000000000000000000000
>> XMM10=00000000000000000000000000000000 XMM11=00000000000000000000000000000000
>> XMM12=00000000000000000000000000000000 XMM13=00000000000000000000000000000000
>> XMM14=00000000000000000000000000000000 XMM15=00000000000000000000000000000000
>
> The IDT base address 0xBBF1DD58 doesn't tell me anything, unfortunately.
> Here's a dump of the memory starting at (0xBBF1DD58 - 8):
>
>> $ virsh qemu-monitor-command ovmf.fedora --hmp 'xp /32gx 0xBBF1DD50'
>>
>> 00000000bbf1dd50: 0x00000000bbf1cac8 0xbff08e000018afb0
>> 00000000bbf1dd60: 0x0000000000000000 0xbff08e000018afbf
>> 00000000bbf1dd70: 0x0000000000000000 0xbff08e000018afce
>> 00000000bbf1dd80: 0x0000000000000000 0xbff08e000018afdd
>> 00000000bbf1dd90: 0x0000000000000000 0xbff08e000018afec
>> 00000000bbf1dda0: 0x0000000000000000 0xbff08e000018affb
>> 00000000bbf1ddb0: 0x0000000000000000 0xbff08e000018b00a
>> 00000000bbf1ddc0: 0x0000000000000000 0xbff08e000018b019
>> 00000000bbf1ddd0: 0x0000000000000000 0xbff08e000018b028
>> 00000000bbf1dde0: 0x0000000000000000 0xbff08e000018b037
>> 00000000bbf1ddf0: 0x0000000000000000 0xbff08e000018b046
>> 00000000bbf1de00: 0x0000000000000000 0xbff08e000018b055
>> 00000000bbf1de10: 0x0000000000000000 0xbff08e000018b064
>> 00000000bbf1de20: 0x0000000000000000 0xbff08e000018b073
>> 00000000bbf1de30: 0x0000000000000000 0xbff08e000018b082
>> 00000000bbf1de40: 0x0000000000000000 0xbff08e000018b091
>
> Thanks
> Laszlo
>
--
Thanks,
Ray
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] UefiCpuPkg/MpInitLib: AP uses memory preceding IDT to store CpuMpData
2018-06-26 17:20 ` Andrew Fish
2018-06-26 18:57 ` Laszlo Ersek
@ 2018-06-27 5:06 ` Ni, Ruiyu
1 sibling, 0 replies; 13+ messages in thread
From: Ni, Ruiyu @ 2018-06-27 5:06 UTC (permalink / raw)
To: Andrew Fish, Laszlo Ersek
Cc: edk2-devel, Eric Dong, Radim Krčmář, Bandan Das,
Jiewen Yao, Paolo Bonzini
On 6/27/2018 1:20 AM, Andrew Fish wrote:
>
>
>> On Jun 26, 2018, at 10:06 AM, Laszlo Ersek <lersek@redhat.com
>> <mailto:lersek@redhat.com>> wrote:
>>
>> (replying again to the patch email directly, for keeping context --
>> adding some people to the CC list. Comments below.)
>>
>> 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 <mailto:ruiyu.ni@intel.com>>
>>> Cc: Jeff Fan <vanjeff_919@hotmail.com <mailto:vanjeff_919@hotmail.com>>
>>> Cc: Eric Dong <eric.dong@intel.com <mailto:eric.dong@intel.com>>
>>> Cc: Jiewen Yao <jiewen.yao@intel.com <mailto:jiewen.yao@intel.com>>
>>> Cc: Fish Andrew <afish@apple.com <mailto: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.
>>>
>>>
>>
>> I captured a KVM trace while the guest was stuck; the following messages
>> repeat infinitely:
>>
>>> CPU-8401 [000] 5171.301018: kvm_entry: vcpu 0
>>> CPU-8401 [000] 5171.301019: kvm_exit: reason DR_ACCESS
>>> rip 0xbff0b28d info 17 0
>>> CPU-8401 [000] 5171.301019: kvm_entry: vcpu 0
>>> CPU-8401 [000] 5171.301050: kvm_exit: reason
>>> EXCEPTION_NMI rip 0xbff03d30 info 0 80000306
>>> CPU-8401 [000] 5171.301051: kvm_emulate_insn: 0:bff03d30: 60
>>> CPU-8401 [000] 5171.301051: kvm_inj_exception: #UD (0x0)
>>
>> The final part of the OVMF log is,
>>
>>> Loading PEIM at 0x000BFF05000 EntryPoint=0x000BFF0ADC6 CpuMpPei.efi
>>> AP Loop Mode is 1
>>> WakeupBufferStart = 9F000, WakeupBufferSize = 1000
>>> TimedWaitForApFinish: reached FinishedApLimit=7 in 0 microseconds
>>> APIC MODE is 1
>>> MpInitLib: Find 8 processors in system.
>>> Does not find any stored CPU BIST information from PPI!
>>> APICID - 0x00000000, BIST - 0x00000000
>>> APICID - 0x00000001, BIST - 0x00000000
>>> APICID - 0x00000002, BIST - 0x00000000
>>> APICID - 0x00000003, BIST - 0x00000000
>>> APICID - 0x00000004, BIST - 0x00000000
>>> APICID - 0x00000005, BIST - 0x00000000
>>> APICID - 0x00000006, BIST - 0x00000000
>>> APICID - 0x00000007, BIST - 0x00000000
>>> Install PPI: 9E9F374B-8F16-4230-9824-5846EE766A97
>>> Install PPI: EE16160A-E8BE-47A6-820A-C6900DB0250A
>>> Notify: PPI Guid: EE16160A-E8BE-47A6-820A-C6900DB0250A, Peim notify
>>> entry point: 8524F8
>>> PlatformPei: OnMpServicesAvailable
>>
>> Note that the first address in the KVM trace, 0xBFF0B28D, is valid. It
>> is offset 0x628D bytes from the CpuMpPei.efi load address (0xBFF05000),
>> and the disassembly for the PEIM is consistent with the "DR_ACCESS"
>> trap:
>>
>>> 00000000000061e8 <HasErrorCode>:
>>> 61e8:55 push %rbp
>>> 61e9:48 89 e5 mov %rsp,%rbp
>>> 61ec:6a 00 pushq $0x0
>>> 61ee:6a 00 pushq $0x0
>>> 61f0:41 57 push %r15
>>> 61f2:41 56 push %r14
>>> 61f4:41 55 push %r13
>>> 61f6:41 54 push %r12
>>> 61f8:41 53 push %r11
>>> 61fa:41 52 push %r10
>>> 61fc:41 51 push %r9
>>> 61fe:41 50 push %r8
>>> 6200:50 push %rax
>>> 6201:ff 75 08 pushq 0x8(%rbp)
>>> 6204:52 push %rdx
>>> 6205:53 push %rbx
>>> 6206:ff 75 30 pushq 0x30(%rbp)
>>> 6209:ff 75 00 pushq 0x0(%rbp)
>>> 620c:56 push %rsi
>>> 620d:57 push %rdi
>>> 620e:48 0f b7 45 38 movzwq 0x38(%rbp),%rax
>>> 6213:50 push %rax
>>> 6214:48 0f b7 45 20 movzwq 0x20(%rbp),%rax
>>> 6219:50 push %rax
>>> 621a:8c d8 mov %ds,%eax
>>> 621c:50 push %rax
>>> 621d:8c c0 mov %es,%eax
>>> 621f:50 push %rax
>>> 6220:8c e0 mov %fs,%eax
>>> 6222:50 push %rax
>>> 6223:8c e8 mov %gs,%eax
>>> 6225:50 push %rax
>>> 6226:48 89 4d 08 mov %rcx,0x8(%rbp)
>>> 622a:ff 75 18 pushq 0x18(%rbp)
>>> 622d:48 31 c0 xor %rax,%rax
>>> 6230:50 push %rax
>>> 6231:50 push %rax
>>> 6232:0f 01 0c 24 sidt (%rsp)
>>> 6236:48 87 44 24 02 xchg %rax,0x2(%rsp)
>>> 623b:48 87 04 24 xchg %rax,(%rsp)
>>> 623f:48 87 44 24 08 xchg %rax,0x8(%rsp)
>>> 6244:48 31 c0 xor %rax,%rax
>>> 6247:50 push %rax
>>> 6248:50 push %rax
>>> 6249:0f 01 04 24 sgdt (%rsp)
>>> 624d:48 87 44 24 02 xchg %rax,0x2(%rsp)
>>> 6252:48 87 04 24 xchg %rax,(%rsp)
>>> 6256:48 87 44 24 08 xchg %rax,0x8(%rsp)
>>> 625b:48 31 c0 xor %rax,%rax
>>> 625e:66 0f 00 c8 str %ax
>>> 6262:50 push %rax
>>> 6263:66 0f 00 c0 sldt %ax
>>> 6267:50 push %rax
>>> 6268:ff 75 28 pushq 0x28(%rbp)
>>> 626b:44 0f 20 c0 mov %cr8,%rax
>>> 626f:50 push %rax
>>> 6270:0f 20 e0 mov %cr4,%rax
>>> 6273:48 0d 08 02 00 00 or $0x208,%rax
>>> 6279:0f 22 e0 mov %rax,%cr4
>>> 627c:50 push %rax
>>> 627d:0f 20 d8 mov %cr3,%rax
>>> 6280:50 push %rax
>>> 6281:0f 20 d0 mov %cr2,%rax
>>> 6284:50 push %rax
>>> 6285:48 31 c0 xor %rax,%rax
>>> 6288:50 push %rax
>>> 6289:0f 20 c0 mov %cr0,%rax
>>> 628c:50 push %rax
>>> 628d:0f 21 f8 mov %db7,%rax <-------- here
>>> 6290:50 push %rax
>>> 6291:0f 21 f0 mov %db6,%rax
>>> 6294:50 push %rax
>>> 6295:0f 21 d8 mov %db3,%rax
>>> 6298:50 push %rax
>>> 6299:0f 21 d0 mov %db2,%rax
>>> 629c:50 push %rax
>>> 629d:0f 21 c8 mov %db1,%rax
>>> 62a0:50 push %rax
>>> 62a1:0f 21 c0 mov %db0,%rax
>>> 62a4:50 push %rax
>>> 62a5:48 81 ec 00 02 00 00sub $0x200,%rsp
>>> 62ac:48 89 e7 mov %rsp,%rdi
>>> 62af:0f ae 07 fxsave (%rdi)
>>> 62b2:fc cld
>>> 62b3:ff 75 10 pushq 0x10(%rbp)
>>> 62b6:48 8b 4d 08 mov 0x8(%rbp),%rcx
>>> 62ba:48 89 e2 mov %rsp,%rdx
>>> 62bd:48 83 ec 28 sub $0x28,%rsp
>>> 62c1:e8 61 0c 00 00 callq 6f27 <CommonExceptionHandler>
>>> 62c2: R_X86_64_PC32CommonExceptionHandler-0x4
>>> 62c6:48 83 c4 28 add $0x28,%rsp
>>> 62ca:fa cli
>>> 62cb:48 83 c4 08 add $0x8,%rsp
>>> 62cf:48 89 e6 mov %rsp,%rsi
>>> 62d2:0f ae 0e fxrstor (%rsi)
>>> 62d5:48 81 c4 00 02 00 00add $0x200,%rsp
>>> 62dc:48 83 c4 30 add $0x30,%rsp
>>> 62e0:58 pop %rax
>>> 62e1:0f 22 c0 mov %rax,%cr0
>>> 62e4:48 83 c4 08 add $0x8,%rsp
>>> 62e8:58 pop %rax
>>> 62e9:0f 22 d0 mov %rax,%cr2
>>> 62ec:58 pop %rax
>>> 62ed:0f 22 d8 mov %rax,%cr3
>>> 62f0:58 pop %rax
>>> 62f1:0f 22 e0 mov %rax,%cr4
>>> 62f4:58 pop %rax
>>> 62f5:44 0f 22 c0 mov %rax,%cr8
>>> 62f9:8f 45 28 popq 0x28(%rbp)
>>> 62fc:48 83 c4 30 add $0x30,%rsp
>>> 6300:8f 45 18 popq 0x18(%rbp)
>>> 6303:58 pop %rax
>>> 6304:58 pop %rax
>>> 6305:58 pop %rax
>>> 6306:8e c0 mov %eax,%es
>>> 6308:58 pop %rax
>>> 6309:8e d8 mov %eax,%ds
>>> 630b:8f 45 20 popq 0x20(%rbp)
>>> 630e:8f 45 38 popq 0x38(%rbp)
>>> 6311:5f pop %rdi
>>> 6312:5e pop %rsi
>>> 6313:48 83 c4 08 add $0x8,%rsp
>>> 6317:8f 45 30 popq 0x30(%rbp)
>>> 631a:5b pop %rbx
>>> 631b:5a pop %rdx
>>> 631c:59 pop %rcx
>>> 631d:58 pop %rax
>>> 631e:41 58 pop %r8
>>> 6320:41 59 pop %r9
>>> 6322:41 5a pop %r10
>>> 6324:41 5b pop %r11
>>> 6326:41 5c pop %r12
>>> 6328:41 5d pop %r13
>>> 632a:41 5e pop %r14
>>> 632c:41 5f pop %r15
>>> 632e:48 89 ec mov %rbp,%rsp
>>> 6331:5d pop %rbp
>>> 6332:48 83 c4 10 add $0x10,%rsp
>>> 6336:48 83 7c 24 e0 00 cmpq $0x0,-0x20(%rsp)
>>> 633c:74 14 je 6352 <DoReturn>
>>> 633e:48 83 7c 24 d8 01 cmpq $0x1,-0x28(%rsp)
>>> 6344:74 04 je 634a <ErrorCode>
>>> 6346:ff 64 24 e0 jmpq *-0x20(%rsp)
>>
>> (This function is from
>> "UefiCpuPkg/Library/CpuExceptionHandlerLib/Ia32/ExceptionHandlerAsm.S"
>> -- I guess it's already a problem that we are in that file at all?)
>>
>> However, the opcode 0x60 at address 0xBFF03D30, which triggers the #UD
>> exception ("invalid opcode"), is *below* the "CpuMpPei.efi" load address
>> (by 0x12D0 bytes).
>>
>>
>> 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.
>
> 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.
>
> Ironically I'm in the process of making a change that uses Itdr.Base -
> (2*sizeof(UINTN)) to store debugging context. Not sure if that is what
> this code is doing?
Similar. This patch uses the location which stores PeiServices** to
store the CpuMpData pointer because AP doesn't need to use PeiServices.
>
> Thanks,
>
> Andrew Fish
>
>
>> Here's a register dump, to see where the IDT is:
>>
>>> $ virsh qemu-monitor-command ovmf.fedora --hmp 'info registers'
>>>
>>> RAX=0000000000000000 RBX=00000000008524f8 RCX=00000000bfeebd30
>>> RDX=ffffffffffffffff
>>> RSI=00000000bbf1c068 RDI=00000000bfeebd30 RBP=00000000bbf1bee0
>>> RSP=00000000bbf1bea0
>>> R8 =0000000000000001 R9 =0000000000000000 R10=0000000000000000
>>> R11=00000000000000b0
>>> R12=00000000bff14b60 R13=0000000000000000 R14=0000000000000000
>>> R15=0000000000000000
>>> RIP=00000000bff090b3 RFL=00000093 [--S-A-C] CPL=0 II=0 A20=1 SMM=0 HLT=0
>>> ES =0008 0000000000000000 ffffffff 00c09300 DPL=0 DS [-WA]
>>> CS =0018 0000000000000000 ffffffff 00a09b00 DPL=0 CS64 [-RA]
>>> SS =0008 0000000000000000 ffffffff 00c09300 DPL=0 DS [-WA]
>>> DS =0008 0000000000000000 ffffffff 00c09300 DPL=0 DS [-WA]
>>> FS =0008 0000000000000000 ffffffff 00c09300 DPL=0 DS [-WA]
>>> GS =0008 0000000000000000 ffffffff 00c09300 DPL=0 DS [-WA]
>>> LDT=0000 0000000000000000 0000ffff 00008200 DPL=0 LDT
>>> TR =0000 0000000000000000 0000ffff 00008b00 DPL=0 TSS64-busy
>>> GDT= 00000000ffffff80 0000001f
>>> IDT= 00000000bbf1dd58 0000021f
>>> CR0=80000033 CR2=0000000000000000 CR3=0000000000800000 CR4=00000668
>>> DR0=0000000000000000 DR1=0000000000000000 DR2=0000000000000000
>>> DR3=0000000000000000
>>> DR6=00000000ffff0ff0 DR7=0000000000000400
>>> EFER=0000000000000500
>>> FCW=037f FSW=0000 [ST=0] FTW=00 MXCSR=00001f80
>>> FPR0=0000000000000000 0000 FPR1=0000000000000000 0000
>>> FPR2=0000000000000000 0000 FPR3=0000000000000000 0000
>>> FPR4=0000000000000000 0000 FPR5=0000000000000000 0000
>>> FPR6=0000000000000000 0000 FPR7=0000000000000000 0000
>>> XMM00=00000000000000000000000000000000
>>> XMM01=00000000000000000000000000000000
>>> XMM02=00000000000000000000000000000000
>>> XMM03=00000000000000000000000000000000
>>> XMM04=00000000000000000000000000000000
>>> XMM05=00000000000000000000000000000000
>>> XMM06=00000000000000000000000000000000
>>> XMM07=00000000000000000000000000000000
>>> XMM08=00000000000000000000000000000000
>>> XMM09=00000000000000000000000000000000
>>> XMM10=00000000000000000000000000000000
>>> XMM11=00000000000000000000000000000000
>>> XMM12=00000000000000000000000000000000
>>> XMM13=00000000000000000000000000000000
>>> XMM14=00000000000000000000000000000000
>>> XMM15=00000000000000000000000000000000
>>
>> The IDT base address 0xBBF1DD58 doesn't tell me anything, unfortunately.
>> Here's a dump of the memory starting at (0xBBF1DD58 - 8):
>>
>>> $ virsh qemu-monitor-command ovmf.fedora --hmp 'xp /32gx 0xBBF1DD50'
>>>
>>> 00000000bbf1dd50: 0x00000000bbf1cac8 0xbff08e000018afb0
>>> 00000000bbf1dd60: 0x0000000000000000 0xbff08e000018afbf
>>> 00000000bbf1dd70: 0x0000000000000000 0xbff08e000018afce
>>> 00000000bbf1dd80: 0x0000000000000000 0xbff08e000018afdd
>>> 00000000bbf1dd90: 0x0000000000000000 0xbff08e000018afec
>>> 00000000bbf1dda0: 0x0000000000000000 0xbff08e000018affb
>>> 00000000bbf1ddb0: 0x0000000000000000 0xbff08e000018b00a
>>> 00000000bbf1ddc0: 0x0000000000000000 0xbff08e000018b019
>>> 00000000bbf1ddd0: 0x0000000000000000 0xbff08e000018b028
>>> 00000000bbf1dde0: 0x0000000000000000 0xbff08e000018b037
>>> 00000000bbf1ddf0: 0x0000000000000000 0xbff08e000018b046
>>> 00000000bbf1de00: 0x0000000000000000 0xbff08e000018b055
>>> 00000000bbf1de10: 0x0000000000000000 0xbff08e000018b064
>>> 00000000bbf1de20: 0x0000000000000000 0xbff08e000018b073
>>> 00000000bbf1de30: 0x0000000000000000 0xbff08e000018b082
>>> 00000000bbf1de40: 0x0000000000000000 0xbff08e000018b091
>>
>> Thanks
>> Laszlo
>> _______________________________________________
>> edk2-devel mailing list
>> edk2-devel@lists.01.org <mailto:edk2-devel@lists.01.org>
>> https://lists.01.org/mailman/listinfo/edk2-devel
>
--
Thanks,
Ray
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] UefiCpuPkg/MpInitLib: AP uses memory preceding IDT to store CpuMpData
2018-06-26 18:57 ` Laszlo Ersek
@ 2018-06-27 6:00 ` Ni, Ruiyu
2018-06-27 12:00 ` Laszlo Ersek
0 siblings, 1 reply; 13+ messages in thread
From: Ni, Ruiyu @ 2018-06-27 6:00 UTC (permalink / raw)
To: edk2-devel
On 6/27/2018 2:57 AM, Laszlo Ersek wrote:
> 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.
>
Thanks for the analysis. It's a stupid bug that I should be aware of.
That can also explain why I cannot reproduce it. It's random.
> 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",
Don't understand. Can you kindly explain more?
>
> - 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,
I like this idea. Will generate the V2 patch.
>
> - 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.
I think it's the most complicated fix:)
--
Thanks,
Ray
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] UefiCpuPkg/MpInitLib: AP uses memory preceding IDT to store CpuMpData
2018-06-27 6:00 ` Ni, Ruiyu
@ 2018-06-27 12:00 ` Laszlo Ersek
2018-06-29 9:36 ` Ni, Ruiyu
0 siblings, 1 reply; 13+ messages in thread
From: Laszlo Ersek @ 2018-06-27 12:00 UTC (permalink / raw)
To: Ni, Ruiyu; +Cc: edk2-devel
On 06/27/18 08:00, Ni, Ruiyu wrote:
> On 6/27/2018 2:57 AM, Laszlo Ersek wrote:
>> 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.
>>
>
> Thanks for the analysis. It's a stupid bug that I should be aware of.
> That can also explain why I cannot reproduce it. It's random.
>
>> 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",
>
> Don't understand. Can you kindly explain more?
Roughly, I figured that we could set the pre-IDT pointer on the BSP,
*before* starting up the APs, to a mapping table. The mapping table
would consist of two columns, the first containing the APIC ID (of each
CPU), the second containing AP-specific pointer values. Then, after
starting up the APs, each AP could locate its own row (based on APIC ID)
in the table, and the context pointer that belongs to it.
But, this is likely not useful, as we want to expose just the same one
pointer value to all APs together.
>
>>
>> - 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,
>
> I like this idea. Will generate the V2 patch.
>
>>
>> - 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.
>
> I think it's the most complicated fix:)
It might take the most code, but I guess it would be the simplest to
reason about. No data sharing --> no locking necessary, and no races
possible.
Anyway, I saw your v2 (just the subject, and your note that we should
ignore v2 for now). I'll stay tuned for v3.
Meta-question: some of your emails are apparently addressed to the list
only, and not CC'd to people who commented earlier on the thread. Did
you drop CCs deliberately, or is it some kind of mail agent glitch?
Thanks!
Laszlo
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] UefiCpuPkg/MpInitLib: AP uses memory preceding IDT to store CpuMpData
2018-06-27 12:00 ` Laszlo Ersek
@ 2018-06-29 9:36 ` Ni, Ruiyu
0 siblings, 0 replies; 13+ messages in thread
From: Ni, Ruiyu @ 2018-06-29 9:36 UTC (permalink / raw)
To: Laszlo Ersek; +Cc: edk2-devel
On 6/27/2018 8:00 PM, Laszlo Ersek wrote:
> Roughly, I figured that we could set the pre-IDT pointer on the BSP,
> *before* starting up the APs, to a mapping table. The mapping table
> would consist of two columns, the first containing the APIC ID (of each
> CPU), the second containing AP-specific pointer values. Then, after
> starting up the APs, each AP could locate its own row (based on APIC ID)
> in the table, and the context pointer that belongs to it.
>
> But, this is likely not useful, as we want to expose just the same one
> pointer value to all APs together.
>
>>> - 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,
>> I like this idea. Will generate the V2 patch.
I finally give up on this direction.
The basic idea of this approach is there is a master AP and many other
slave APs. The master AP is responsible to save CpuMpData to PeiServices
pointer location, the restores PeiServices pointer back after all slave
APs finish.
When slave AP doesn't hit timeout when executing AP procedure,
everything should be good as long as we take good care of the
synchronous stuff.
But when one slave AP hits timeout, the master AP is treated as hitting
timeout as well from BSP's perspective.
So BSP resets the two APs (including master AP) which leads to no one
restores back the PeiServices pointer.
It's true that in such case BSP can do the restore job. But that causes
the code very unreadable.
>>
>>> - 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.
>> I think it's the most complicated fix:)
> It might take the most code, but I guess it would be the simplest to
> reason about. No data sharing --> no locking necessary, and no races
> possible.
I will go in this direction. Yes it may introduce more code but much
more readable than the above solution. Easy to maintain.
>
> Anyway, I saw your v2 (just the subject, and your note that we should
> ignore v2 for now). I'll stay tuned for v3.
>
> Meta-question: some of your emails are apparently addressed to the list
> only, and not CC'd to people who commented earlier on the thread. Did
> you drop CCs deliberately, or is it some kind of mail agent glitch?
I am using Outlook for internal communication, ThunderBird for this
mailing list.
Sometimes I may click [Reply List] button in that mail client. It will
only send the mail to the mailing list without CC.
I agree [Reply All] is better:)
--
Thanks,
Ray
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2018-06-29 9:36 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-06-25 2:54 [PATCH] UefiCpuPkg/MpInitLib: AP uses memory preceding IDT to store CpuMpData Ruiyu Ni
2018-06-25 16:01 ` Laszlo Ersek
2018-06-25 17:01 ` Laszlo Ersek
2018-06-26 7:50 ` Ni, Ruiyu
2018-06-26 12:52 ` Laszlo Ersek
2018-06-26 17:06 ` Laszlo Ersek
2018-06-26 17:20 ` Andrew Fish
2018-06-26 18:57 ` Laszlo Ersek
2018-06-27 6:00 ` Ni, Ruiyu
2018-06-27 12:00 ` Laszlo Ersek
2018-06-29 9:36 ` Ni, Ruiyu
2018-06-27 5:06 ` Ni, Ruiyu
2018-06-27 4:50 ` Ni, Ruiyu
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox