On 10/30/19 10:52, Ray Ni wrote:
> Today's logic sets X2ApicEnable flag in each AP's initialization
> path when InitFlag == ApInitConfig.
> Since all CPUs update the same global data, a spin-lock is used
> to avoid modifications from multiple CPUs happen at the same time.
> The spin-lock causes two problems:
> 1. Potential performance downgrade.
> 2. Undefined behavior when improper timer lib is used.
> For example we saw certain platforms used AcpiTimerLib from
> PcAtChipsetPkg and that library depends on retrieving PeiServices
> from idtr. But in fact AP's (idtr - 4) doesn't point to
> PeiServices.
>
> The patch simplifies the code to let BSP set the X2ApicEnable flag so
> the spin-lock acquisition from AP is not needed any more.
>
> Signed-off-by: Ray Ni <ray...@intel.com>
> Cc: Eric Dong <eric.d...@intel.com>
> Cc: Laszlo Ersek <ler...@redhat.com>
> ---
> UefiCpuPkg/Library/MpInitLib/MpLib.c | 25 ++++++++++++++++---------
> 1 file changed, 16 insertions(+), 9 deletions(-)
>
> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> index 622b70ca3c..8f62a8d965 100644
> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> @@ -458,6 +458,7 @@ CollectProcessorCount (
> )
> {
> UINTN Index;
> + CPU_INFO_IN_HOB *CpuInfoInHob;
>
> //
> // Send 1st broadcast IPI to APs to wakeup APs
> @@ -474,12 +475,27 @@ CollectProcessorCount (
> CpuPause ();
> }
>
> +
> + //
> + // Enable x2APIC mode if
> + // 1. Number of CPU is greater than 255; or
> + // 2. There are any logical processors reporting an Initial APIC ID of
> 255 or greater.
> + //
> if (CpuMpData->CpuCount > 255) {
> //
> // If there are more than 255 processor found, force to enable X2APIC
> //
> CpuMpData->X2ApicEnable = TRUE;
> + } else {
> + CpuInfoInHob = (CPU_INFO_IN_HOB *) (UINTN) CpuMpData->CpuInfoInHob;
> + for (Index = 0; Index < CpuMpData->CpuCount; Index++) {
> + if (CpuInfoInHob[Index].InitialApicId >= 0xFF) {
> + CpuMpData->X2ApicEnable = TRUE;
> + break;
> + }
> + }
> }
> +
> if (CpuMpData->X2ApicEnable) {
> DEBUG ((DEBUG_INFO, "Force x2APIC mode!\n"));
> //
> @@ -541,15 +557,6 @@ InitializeApData (
>
> CpuMpData->CpuData[ProcessorNumber].Waiting = FALSE;
> CpuMpData->CpuData[ProcessorNumber].CpuHealthy = (BistData == 0) ? TRUE :
> FALSE;
> - if (CpuInfoInHob[ProcessorNumber].InitialApicId >= 0xFF) {
> - //
> - // Set x2APIC mode if there are any logical processor reporting
> - // an Initial APIC ID of 255 or greater.
> - //
> - AcquireSpinLock(&CpuMpData->MpLock);
> - CpuMpData->X2ApicEnable = TRUE;
> - ReleaseSpinLock(&CpuMpData->MpLock);
> - }
>
> InitializeSpinLock(&CpuMpData->CpuData[ProcessorNumber].ApLock);
> SetApState (&CpuMpData->CpuData[ProcessorNumber], CpuStateIdle);
>
I would have suggested "InterlockedCompareExchange8" as an alternative,
but (a) maybe there's really no reason for updating that field from APs,
(b) there doesn't seem to be a single byte version of that function.
So, the patch looks plausible. Got no time to test it now, alas.
Acked-by: Laszlo Ersek <ler...@redhat.com>
Thanks
Laszlo
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#49741): https://edk2.groups.io/g/devel/message/49741
Mute This Topic: https://groups.io/mt/39769103/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
