Re: [Patch] UefiCpuPkg/MpLib: Fix PEI Services Table pointer on AP

["Ni, Ruiyu" <ruiyu.ni@intel.com>]

Felix,
I disagree:) Sorry about that. :)

The commit you mentioned might be made by me (didn't checked).
Because I aimed to avoid calling PEI services from AP. That's a violation of PI spec and not safe by design.

The AP calling standard services concern was raised by Andrew initially.

Thanks,
Ray

> -----Original Message-----
> From: Felix Polyudov [mailto:felixp@ami.com]
> Sent: Friday, November 30, 2018 8:36 AM
> To: edk2-devel@lists.01.org
> Cc: Dong, Eric <eric.dong@intel.com>; Ni, Ruiyu <ruiyu.ni@intel.com>;
> lersek@redhat.com
> Subject: [Patch] UefiCpuPkg/MpLib: Fix PEI Services Table pointer on AP
> 
> According to PI specification PEI Services table pointer is stored right before ITD
> base. Starting from commit c563077a380437c1 BSP and AP have different IDT
> instances.
> PEI Services table pointer was not initialized in the AP IDT instance.
> As a result, any attempt to use functions from PeiServicesTablePointerLib or
> PeiServicesLib on AP caused CPU exception.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Felix Polyudov <felixp@ami.com>
> ---
>  UefiCpuPkg/Library/MpInitLib/MpLib.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> index 7f4d6e6..0e3e362 100644
> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> @@ -1567,6 +1567,7 @@ MpInitLibInitialize (
>    BufferSize  = ApStackSize * MaxLogicalProcessorNumber;
>    BufferSize += MonitorFilterSize * MaxLogicalProcessorNumber;
>    BufferSize += ApResetVectorSize;
> +  BufferSize += sizeof(UINTN);
>    BufferSize  = ALIGN_VALUE (BufferSize, 8);
>    BufferSize += VolatileRegisters.Idtr.Limit + 1;
>    BufferSize += sizeof (CPU_MP_DATA);
> @@ -1587,6 +1588,8 @@ MpInitLibInitialize (
>    //         Backup Buffer
>    //    +--------------------+
>    //           Padding
> +  //    +--------------------+
> +  //    PEI Services Table Pointer
>    //    +--------------------+ <-- ApIdtBase (8-byte boundary)
>    //           AP IDT          All APs share one separate IDT. So AP can get address of
> CPU_MP_DATA from IDT Base.
>    //    +--------------------+ <-- CpuMpData
> @@ -1599,7 +1602,7 @@ MpInitLibInitialize (
>    //
>    MonitorBuffer    = (UINT8 *) (Buffer + ApStackSize *
> MaxLogicalProcessorNumber);
>    BackupBufferAddr = (UINTN) MonitorBuffer + MonitorFilterSize *
> MaxLogicalProcessorNumber;
> -  ApIdtBase        = ALIGN_VALUE (BackupBufferAddr + ApResetVectorSize, 8);
> +  ApIdtBase        = ALIGN_VALUE (BackupBufferAddr + ApResetVectorSize +
> sizeof(UINTN), 8);
>    CpuMpData        = (CPU_MP_DATA *) (ApIdtBase + VolatileRegisters.Idtr.Limit +
> 1);
>    CpuMpData->Buffer           = Buffer;
>    CpuMpData->CpuApStackSize   = ApStackSize;
> @@ -1653,6 +1656,11 @@ MpInitLibInitialize (
>            Buffer + BufferSize);
> 
>    //
> +  // Initialize PEI Services table pointer. Copy the address from BSP.
> +  //
> +  *(UINTN*)(ApIdtBase - sizeof(UINTN)) =
> + *(UINTN*)(VolatileRegisters.Idtr.Base - sizeof (UINTN));
> +
> +  //
>    // Duplicate BSP's IDT to APs.
>    // All APs share one separate IDT. So AP can get the address of CpuMpData by
> using IDTR.BASE + IDTR.LIMIT + 1
>    //
> --
> 2.10.0.windows.1
> 
> 
> 
> Please consider the environment before printing this email.
> 
> The information contained in this message may be confidential and proprietary
> to American Megatrends, Inc.  This communication is intended to be read only
> by the individual or entity to whom it is addressed or by their designee. If the
> reader of this message is not the intended recipient, you are on notice that any
> distribution of this message, in any form, is strictly prohibited.  Please promptly
> notify the sender by reply e-mail or by telephone at 770-246-8600, and then
> delete or destroy all copies of the transmission.