(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...@intel.com>
> Cc: Jeff Fan <vanjeff_...@hotmail.com>
> Cc: Eric Dong <eric.d...@intel.com>
> Cc: Jiewen Yao <jiewen....@intel.com>
> Cc: Fish Andrew <af...@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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
