* [Patch] UefiCpuPkg/MpLib: Fix PEI Services Table pointer on AP
@ 2018-11-30 0:35 Felix Polyudov
2018-11-30 3:42 ` Ni, Ruiyu
0 siblings, 1 reply; 6+ messages in thread
From: Felix Polyudov @ 2018-11-30 0:35 UTC (permalink / raw)
To: edk2-devel; +Cc: eric.dong, ruiyu.ni, lersek
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.
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [Patch] UefiCpuPkg/MpLib: Fix PEI Services Table pointer on AP
2018-11-30 0:35 [Patch] UefiCpuPkg/MpLib: Fix PEI Services Table pointer on AP Felix Polyudov
@ 2018-11-30 3:42 ` Ni, Ruiyu
2018-11-30 14:33 ` Felix Polyudov
0 siblings, 1 reply; 6+ messages in thread
From: Ni, Ruiyu @ 2018-11-30 3:42 UTC (permalink / raw)
To: Felix Polyudov, edk2-devel@lists.01.org
Cc: Dong, Eric, lersek@redhat.com,
'Andrew Fish (afish@apple.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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Patch] UefiCpuPkg/MpLib: Fix PEI Services Table pointer on AP
2018-11-30 3:42 ` Ni, Ruiyu
@ 2018-11-30 14:33 ` Felix Polyudov
2018-11-30 20:37 ` Andrew Fish
0 siblings, 1 reply; 6+ messages in thread
From: Felix Polyudov @ 2018-11-30 14:33 UTC (permalink / raw)
To: 'Ni, Ruiyu', edk2-devel@lists.01.org
Cc: lersek@redhat.com, Dong, Eric
Ray,
I agree with the premise that calling PEI services from AP should generally be avoided.
However, the PEI services can be used on AP under certain special circumstances.
A couple of examples:
1. For debugging purposes. The MpInitLib contains 12 DEBUG calls and 19 ASSERT calls. Depending on the DebugLib instance used in the project, these calls may lead to PEI services invocation.
2. MpInitLib provides ability to call AP in a serialized manner (only one AP is running, other APs and BSP are waiting), when it is safe to call PEI services.
Additionally, I think even if PEI services should not be used on AP, there is still a reason to keep PEI services table pointer initialized.
On one hand, given the complexity of modern firmware projects comprised of modules coming from multiple vendors, making sure PEI services are not used on AP can be a challenge.
For example, in my case the call was coming from the chipset reference code.
On the other hand, with the current implementation, when somebody does try to use PEI services on AP the behavior is unpredictable.
Depending on the content of the uninitialized PEI service table pointer, the system may either crash with one of the handful of different exceptions,
or it may start executing code from a random location. It's very difficult to debug such issues. One can spend weeks chasing a problem like this.
-----Original Message-----
From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Ni, Ruiyu
Sent: Thursday, November 29, 2018 10:43 PM
To: Felix Polyudov; edk2-devel@lists.01.org
Cc: lersek@redhat.com; Dong, Eric
Subject: Re: [edk2] [Patch] UefiCpuPkg/MpLib: Fix PEI Services Table pointer on AP
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.
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Patch] UefiCpuPkg/MpLib: Fix PEI Services Table pointer on AP
2018-11-30 14:33 ` Felix Polyudov
@ 2018-11-30 20:37 ` Andrew Fish
2018-12-06 0:07 ` Felix Polyudov
0 siblings, 1 reply; 6+ messages in thread
From: Andrew Fish @ 2018-11-30 20:37 UTC (permalink / raw)
To: Felix Poludov
Cc: Ni, Ruiyu, edk2-devel@lists.01.org, lersek@redhat.com, Dong, Eric
Felix,
I agree the obfuscation may not really be helpful. I've got a couple of thoughts.
1) I agree that I don't think the DebugLib is inherently safe on APs. I wonder if we could make a library class that was DebugLibMpSafe and have the same API as DebugLib. That way the library class matches how the module was coded.
2) If we want to enforce the rules we should add code to the PEI or DXE Core to CpuBreakpoint() etc. if an AP calls a core service.
Adding detection code is possible, but it is not trivial. For example if you have remembered the BSP and if the WhoAmI returns something different you need to check to see if some one changed the BSP.
I guess for PEI the other option could be to have a fake PEI Services Table that stubs out all the functions with CpuBreakpoint() or some such?
Thanks,
Andrew Fish
> On Nov 30, 2018, at 6:33 AM, Felix Polyudov <Felixp@ami.com> wrote:
>
> Ray,
>
> I agree with the premise that calling PEI services from AP should generally be avoided.
> However, the PEI services can be used on AP under certain special circumstances.
> A couple of examples:
> 1. For debugging purposes. The MpInitLib contains 12 DEBUG calls and 19 ASSERT calls. Depending on the DebugLib instance used in the project, these calls may lead to PEI services invocation.
> 2. MpInitLib provides ability to call AP in a serialized manner (only one AP is running, other APs and BSP are waiting), when it is safe to call PEI services.
>
> Additionally, I think even if PEI services should not be used on AP, there is still a reason to keep PEI services table pointer initialized.
> On one hand, given the complexity of modern firmware projects comprised of modules coming from multiple vendors, making sure PEI services are not used on AP can be a challenge.
> For example, in my case the call was coming from the chipset reference code.
> On the other hand, with the current implementation, when somebody does try to use PEI services on AP the behavior is unpredictable.
> Depending on the content of the uninitialized PEI service table pointer, the system may either crash with one of the handful of different exceptions,
> or it may start executing code from a random location. It's very difficult to debug such issues. One can spend weeks chasing a problem like this.
>
>
> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org <mailto:edk2-devel-bounces@lists.01.org>] On Behalf Of Ni, Ruiyu
> Sent: Thursday, November 29, 2018 10:43 PM
> To: Felix Polyudov; edk2-devel@lists.01.org <mailto:edk2-devel@lists.01.org>
> Cc: lersek@redhat.com <mailto:lersek@redhat.com>; Dong, Eric
> Subject: Re: [edk2] [Patch] UefiCpuPkg/MpLib: Fix PEI Services Table pointer on AP
>
> 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.
> _______________________________________________
> 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>
>
> 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.
> _______________________________________________
> 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] 6+ messages in thread
* Re: [Patch] UefiCpuPkg/MpLib: Fix PEI Services Table pointer on AP
2018-11-30 20:37 ` Andrew Fish
@ 2018-12-06 0:07 ` Felix Polyudov
2018-12-06 18:12 ` Brian J. Johnson
0 siblings, 1 reply; 6+ messages in thread
From: Felix Polyudov @ 2018-12-06 0:07 UTC (permalink / raw)
To: 'afish@apple.com'
Cc: Ni, Ruiyu, edk2-devel@lists.01.org, lersek@redhat.com, Dong, Eric
Andrew,
I think there are two aspects here:
1) What's the final solution we would like to see?
I think DebugLibMpSafe and a fake PEI Services Table that you are proposing are good ideas.
Ideally, it should still be possible to use the real PEI Services table by overriding a library class.
Please correct me if I'm wrong, but I believe it is legal to call PEI Service from AP if all other CPUs (including BSP) are not-running (halted or sitting in a spin-loop).
2) Should we do something short-term?
In my opinion yes, because commit c563077a380437c1 breaks perhaps questionable but working production code.
Additionally, as I've mentioned the library itself is broken because it uses DebugLib, which is likely to use PEI Services.
The patch I've submitted certainly does not provide a complete solution, but it provides a workaround for the immediate issue.
So I think it makes sense to apply the patch.
Thanks
Felix
From: afish@apple.com [mailto:afish@apple.com]
Sent: Friday, November 30, 2018 3:37 PM
To: Felix Polyudov
Cc: Ni, Ruiyu; edk2-devel@lists.01.org; lersek@redhat.com; Dong, Eric
Subject: Re: [edk2] [Patch] UefiCpuPkg/MpLib: Fix PEI Services Table pointer on AP
Felix,
I agree the obfuscation may not really be helpful. I've got a couple of thoughts.
1) I agree that I don't think the DebugLib is inherently safe on APs. I wonder if we could make a library class that was DebugLibMpSafe and have the same API as DebugLib. That way the library class matches how the module was coded.
2) If we want to enforce the rules we should add code to the PEI or DXE Core to CpuBreakpoint() etc. if an AP calls a core service.
Adding detection code is possible, but it is not trivial. For example if you have remembered the BSP and if the WhoAmI returns something different you need to check to see if some one changed the BSP.
I guess for PEI the other option could be to have a fake PEI Services Table that stubs out all the functions with CpuBreakpoint() or some such?
Thanks,
Andrew Fish
On Nov 30, 2018, at 6:33 AM, Felix Polyudov <Felixp@ami.com<mailto:Felixp@ami.com>> wrote:
Ray,
I agree with the premise that calling PEI services from AP should generally be avoided.
However, the PEI services can be used on AP under certain special circumstances.
A couple of examples:
1. For debugging purposes. The MpInitLib contains 12 DEBUG calls and 19 ASSERT calls. Depending on the DebugLib instance used in the project, these calls may lead to PEI services invocation.
2. MpInitLib provides ability to call AP in a serialized manner (only one AP is running, other APs and BSP are waiting), when it is safe to call PEI services.
Additionally, I think even if PEI services should not be used on AP, there is still a reason to keep PEI services table pointer initialized.
On one hand, given the complexity of modern firmware projects comprised of modules coming from multiple vendors, making sure PEI services are not used on AP can be a challenge.
For example, in my case the call was coming from the chipset reference code.
On the other hand, with the current implementation, when somebody does try to use PEI services on AP the behavior is unpredictable.
Depending on the content of the uninitialized PEI service table pointer, the system may either crash with one of the handful of different exceptions,
or it may start executing code from a random location. It's very difficult to debug such issues. One can spend weeks chasing a problem like this.
-----Original Message-----
From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Ni, Ruiyu
Sent: Thursday, November 29, 2018 10:43 PM
To: Felix Polyudov; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
Cc: lersek@redhat.com<mailto:lersek@redhat.com>; Dong, Eric
Subject: Re: [edk2] [Patch] UefiCpuPkg/MpLib: Fix PEI Services Table pointer on AP
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<mailto:edk2-devel@lists.01.org>
Cc: Dong, Eric <eric.dong@intel.com<mailto:eric.dong@intel.com>>; Ni, Ruiyu <ruiyu.ni@intel.com<mailto:ruiyu.ni@intel.com>>;
lersek@redhat.com<mailto: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.
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
https://lists.01.org/mailman/listinfo/edk2-devel
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.
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
https://lists.01.org/mailman/listinfo/edk2-devel
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.
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [Patch] UefiCpuPkg/MpLib: Fix PEI Services Table pointer on AP
2018-12-06 0:07 ` Felix Polyudov
@ 2018-12-06 18:12 ` Brian J. Johnson
0 siblings, 0 replies; 6+ messages in thread
From: Brian J. Johnson @ 2018-12-06 18:12 UTC (permalink / raw)
To: Felix Polyudov, 'afish@apple.com'
Cc: Ni, Ruiyu, edk2-devel@lists.01.org, lersek@redhat.com, Dong, Eric
On 12/5/18 6:07 PM, Felix Polyudov wrote:
> Andrew,
>
> I think there are two aspects here:
>
> 1) What's the final solution we would like to see?
> I think DebugLibMpSafe and a fake PEI Services Table that you are proposing are good ideas.
> Ideally, it should still be possible to use the real PEI Services table by overriding a library class.
> Please correct me if I'm wrong, but I believe it is legal to call PEI Service from AP if all other CPUs (including BSP) are not-running (halted or sitting in a spin-loop).
>
It's highly platform-dependent. For instance, on many Intel server
processors, RAM is not available for most of PEI (the memory init code
runs in a PEIM.) The CPUs are configured to use their internal caches
as small amounts of temporary RAM, but they are not shared. So the PEI
environment set up on the BSP is not available to the APs. No PPIs,
HOBs, dynamic PCDs, PEI services, etc, etc. The platform code is
responsible for transferring anything the APs need, generally via
chipset registers. Slow, ugly, and extremely platform-dependent.
A basic self-contained DebugLib without dependencies is certainly handy.
But again, its implementation is platform-dependent. For instance, if
all APs try to write output to a single UART at the same time, all
you'll see is a mess. If a platform has some way for them to
synchronize, or a separate debug channel per CPU, you could fix that.
FWIW, BaseDebugLibSerialPort has few or no dependencies if your
SerialPortLib and DebugPrintErrorLevelLib don't. Nor does
BaseDebugLibNull. :)
Brian Johnson
> 2) Should we do something short-term?
> In my opinion yes, because commit c563077a380437c1 breaks perhaps questionable but working production code.
> Additionally, as I've mentioned the library itself is broken because it uses DebugLib, which is likely to use PEI Services.
> The patch I've submitted certainly does not provide a complete solution, but it provides a workaround for the immediate issue.
> So I think it makes sense to apply the patch.
>
> Thanks
> Felix
>
> From: afish@apple.com [mailto:afish@apple.com]
> Sent: Friday, November 30, 2018 3:37 PM
> To: Felix Polyudov
> Cc: Ni, Ruiyu; edk2-devel@lists.01.org; lersek@redhat.com; Dong, Eric
> Subject: Re: [edk2] [Patch] UefiCpuPkg/MpLib: Fix PEI Services Table pointer on AP
>
> Felix,
>
> I agree the obfuscation may not really be helpful. I've got a couple of thoughts.
>
> 1) I agree that I don't think the DebugLib is inherently safe on APs. I wonder if we could make a library class that was DebugLibMpSafe and have the same API as DebugLib. That way the library class matches how the module was coded.
> 2) If we want to enforce the rules we should add code to the PEI or DXE Core to CpuBreakpoint() etc. if an AP calls a core service.
>
> Adding detection code is possible, but it is not trivial. For example if you have remembered the BSP and if the WhoAmI returns something different you need to check to see if some one changed the BSP.
>
> I guess for PEI the other option could be to have a fake PEI Services Table that stubs out all the functions with CpuBreakpoint() or some such?
>
> Thanks,
>
> Andrew Fish
>
>
>
> On Nov 30, 2018, at 6:33 AM, Felix Polyudov <Felixp@ami.com<mailto:Felixp@ami.com>> wrote:
>
> Ray,
>
> I agree with the premise that calling PEI services from AP should generally be avoided.
> However, the PEI services can be used on AP under certain special circumstances.
> A couple of examples:
> 1. For debugging purposes. The MpInitLib contains 12 DEBUG calls and 19 ASSERT calls. Depending on the DebugLib instance used in the project, these calls may lead to PEI services invocation.
> 2. MpInitLib provides ability to call AP in a serialized manner (only one AP is running, other APs and BSP are waiting), when it is safe to call PEI services.
>
> Additionally, I think even if PEI services should not be used on AP, there is still a reason to keep PEI services table pointer initialized.
> On one hand, given the complexity of modern firmware projects comprised of modules coming from multiple vendors, making sure PEI services are not used on AP can be a challenge.
> For example, in my case the call was coming from the chipset reference code.
> On the other hand, with the current implementation, when somebody does try to use PEI services on AP the behavior is unpredictable.
> Depending on the content of the uninitialized PEI service table pointer, the system may either crash with one of the handful of different exceptions,
> or it may start executing code from a random location. It's very difficult to debug such issues. One can spend weeks chasing a problem like this.
>
>
> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Ni, Ruiyu
> Sent: Thursday, November 29, 2018 10:43 PM
> To: Felix Polyudov; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
> Cc: lersek@redhat.com<mailto:lersek@redhat.com>; Dong, Eric
> Subject: Re: [edk2] [Patch] UefiCpuPkg/MpLib: Fix PEI Services Table pointer on AP
>
> 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<mailto:edk2-devel@lists.01.org>
> Cc: Dong, Eric <eric.dong@intel.com<mailto:eric.dong@intel.com>>; Ni, Ruiyu <ruiyu.ni@intel.com<mailto:ruiyu.ni@intel.com>>;
> lersek@redhat.com<mailto: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.
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
> https://lists.01.org/mailman/listinfo/edk2-devel
>
> 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.
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
> https://lists.01.org/mailman/listinfo/edk2-devel
>
>
> 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.
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
>
--
Brian J. Johnson
Enterprise X86 Lab
Hewlett Packard Enterprise
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-12-06 18:13 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-11-30 0:35 [Patch] UefiCpuPkg/MpLib: Fix PEI Services Table pointer on AP Felix Polyudov
2018-11-30 3:42 ` Ni, Ruiyu
2018-11-30 14:33 ` Felix Polyudov
2018-11-30 20:37 ` Andrew Fish
2018-12-06 0:07 ` Felix Polyudov
2018-12-06 18:12 ` Brian J. Johnson
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox