* [PATCH 0/2] Set X2ApicEnable flag from BSP @ 2019-10-30 9:52 Ni, Ray 2019-10-30 9:52 ` [PATCH 1/2] UefiCpuPkg/MpInitLib: " Ni, Ray 2019-10-30 9:52 ` [PATCH 2/2] UefiCpuPkg/MpInitLib: Remove global variable X2ApicEnable Ni, Ray 0 siblings, 2 replies; 7+ messages in thread From: Ni, Ray @ 2019-10-30 9:52 UTC (permalink / raw) To: devel Ray Ni (2): UefiCpuPkg/MpInitLib: Set X2ApicEnable flag from BSP UefiCpuPkg/MpInitLib: Remove global variable X2ApicEnable UefiCpuPkg/Library/MpInitLib/MpLib.c | 37 ++++++++++++++++------------ UefiCpuPkg/Library/MpInitLib/MpLib.h | 1 - 2 files changed, 21 insertions(+), 17 deletions(-) -- 2.21.0.windows.1 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 1/2] UefiCpuPkg/MpInitLib: Set X2ApicEnable flag from BSP 2019-10-30 9:52 [PATCH 0/2] Set X2ApicEnable flag from BSP Ni, Ray @ 2019-10-30 9:52 ` Ni, Ray 2019-10-31 9:11 ` Laszlo Ersek 2019-11-05 1:47 ` [edk2-devel] " Dong, Eric 2019-10-30 9:52 ` [PATCH 2/2] UefiCpuPkg/MpInitLib: Remove global variable X2ApicEnable Ni, Ray 1 sibling, 2 replies; 7+ messages in thread From: Ni, Ray @ 2019-10-30 9:52 UTC (permalink / raw) To: devel; +Cc: Eric Dong, Laszlo Ersek 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.ni@intel.com> Cc: Eric Dong <eric.dong@intel.com> Cc: Laszlo Ersek <lersek@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); -- 2.21.0.windows.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] UefiCpuPkg/MpInitLib: Set X2ApicEnable flag from BSP 2019-10-30 9:52 ` [PATCH 1/2] UefiCpuPkg/MpInitLib: " Ni, Ray @ 2019-10-31 9:11 ` Laszlo Ersek 2019-11-05 1:47 ` [edk2-devel] " Dong, Eric 1 sibling, 0 replies; 7+ messages in thread From: Laszlo Ersek @ 2019-10-31 9:11 UTC (permalink / raw) To: Ray Ni, devel; +Cc: Eric Dong 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.ni@intel.com> > Cc: Eric Dong <eric.dong@intel.com> > Cc: Laszlo Ersek <lersek@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 <lersek@redhat.com> Thanks Laszlo ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [edk2-devel] [PATCH 1/2] UefiCpuPkg/MpInitLib: Set X2ApicEnable flag from BSP 2019-10-30 9:52 ` [PATCH 1/2] UefiCpuPkg/MpInitLib: " Ni, Ray 2019-10-31 9:11 ` Laszlo Ersek @ 2019-11-05 1:47 ` Dong, Eric 1 sibling, 0 replies; 7+ messages in thread From: Dong, Eric @ 2019-11-05 1:47 UTC (permalink / raw) To: devel@edk2.groups.io, Ni, Ray; +Cc: Laszlo Ersek Reviewed-by: Eric Dong <eric.dong@intel.com> -----Original Message----- From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of Ni, Ray Sent: Wednesday, October 30, 2019 5:53 PM To: devel@edk2.groups.io Cc: Dong, Eric <eric.dong@intel.com>; Laszlo Ersek <lersek@redhat.com> Subject: [edk2-devel] [PATCH 1/2] UefiCpuPkg/MpInitLib: Set X2ApicEnable flag from BSP 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.ni@intel.com> Cc: Eric Dong <eric.dong@intel.com> Cc: Laszlo Ersek <lersek@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); -- 2.21.0.windows.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH 2/2] UefiCpuPkg/MpInitLib: Remove global variable X2ApicEnable 2019-10-30 9:52 [PATCH 0/2] Set X2ApicEnable flag from BSP Ni, Ray 2019-10-30 9:52 ` [PATCH 1/2] UefiCpuPkg/MpInitLib: " Ni, Ray @ 2019-10-30 9:52 ` Ni, Ray 2019-10-31 9:20 ` Laszlo Ersek 2019-11-05 1:47 ` [edk2-devel] " Dong, Eric 1 sibling, 2 replies; 7+ messages in thread From: Ni, Ray @ 2019-10-30 9:52 UTC (permalink / raw) To: devel; +Cc: Eric Dong, Laszlo Ersek MpInitLib sets X2ApicEnable in two places. 1. CollectProcessorCount() This function is called when MpInitLibInitialize() hasn't been called before. It sets X2ApicEnable and later in the same function it configures all CPUs to operate in X2 APIC mode. 2. MpInitLibInitialize() The X2ApicEnable setting happens when this function is called in second time. But after that setting, no code consumes that flag. With the above analysis and with the purpose of simplifying the code, the X2ApicEnable in #1 is changed to local variable and the #2 can be changed to remove the setting of X2ApicEnable. Signed-off-by: Ray Ni <ray.ni@intel.com> Cc: Eric Dong <eric.dong@intel.com> Cc: Laszlo Ersek <lersek@redhat.com> --- UefiCpuPkg/Library/MpInitLib/MpLib.c | 14 ++++++-------- UefiCpuPkg/Library/MpInitLib/MpLib.h | 1 - 2 files changed, 6 insertions(+), 9 deletions(-) diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c index 8f62a8d965..49be5d5385 100644 --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c @@ -459,12 +459,12 @@ CollectProcessorCount ( { UINTN Index; CPU_INFO_IN_HOB *CpuInfoInHob; + BOOLEAN X2Apic; // // Send 1st broadcast IPI to APs to wakeup APs // - CpuMpData->InitFlag = ApInitConfig; - CpuMpData->X2ApicEnable = FALSE; + CpuMpData->InitFlag = ApInitConfig; WakeUpAP (CpuMpData, TRUE, 0, NULL, NULL, TRUE); CpuMpData->InitFlag = ApInitDone; ASSERT (CpuMpData->CpuCount <= PcdGet32 (PcdCpuMaxLogicalProcessorNumber)); @@ -481,22 +481,23 @@ CollectProcessorCount ( // 1. Number of CPU is greater than 255; or // 2. There are any logical processors reporting an Initial APIC ID of 255 or greater. // + X2Apic = FALSE; if (CpuMpData->CpuCount > 255) { // // If there are more than 255 processor found, force to enable X2APIC // - CpuMpData->X2ApicEnable = TRUE; + X2Apic = 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; + X2Apic = TRUE; break; } } } - if (CpuMpData->X2ApicEnable) { + if (X2Apic) { DEBUG ((DEBUG_INFO, "Force x2APIC mode!\n")); // // Wakeup all APs to enable x2APIC mode @@ -1780,9 +1781,6 @@ MpInitLibInitialize ( CpuInfoInHob = (CPU_INFO_IN_HOB *) (UINTN) CpuMpData->CpuInfoInHob; for (Index = 0; Index < CpuMpData->CpuCount; Index++) { InitializeSpinLock(&CpuMpData->CpuData[Index].ApLock); - if (CpuInfoInHob[Index].InitialApicId >= 255 || Index > 254) { - CpuMpData->X2ApicEnable = TRUE; - } CpuMpData->CpuData[Index].CpuHealthy = (CpuInfoInHob[Index].Health == 0)? TRUE:FALSE; CpuMpData->CpuData[Index].ApFunction = 0; CopyMem (&CpuMpData->CpuData[Index].VolatileRegisters, &VolatileRegisters, sizeof (CPU_VOLATILE_REGISTERS)); diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.h b/UefiCpuPkg/Library/MpInitLib/MpLib.h index 107872b367..8fa07b12c5 100644 --- a/UefiCpuPkg/Library/MpInitLib/MpLib.h +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.h @@ -227,7 +227,6 @@ struct _CPU_MP_DATA { UINTN **FailedCpuList; AP_INIT_STATE InitFlag; - BOOLEAN X2ApicEnable; BOOLEAN SwitchBspFlag; UINTN NewBspNumber; CPU_EXCHANGE_ROLE_INFO BSPInfo; -- 2.21.0.windows.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] UefiCpuPkg/MpInitLib: Remove global variable X2ApicEnable 2019-10-30 9:52 ` [PATCH 2/2] UefiCpuPkg/MpInitLib: Remove global variable X2ApicEnable Ni, Ray @ 2019-10-31 9:20 ` Laszlo Ersek 2019-11-05 1:47 ` [edk2-devel] " Dong, Eric 1 sibling, 0 replies; 7+ messages in thread From: Laszlo Ersek @ 2019-10-31 9:20 UTC (permalink / raw) To: Ray Ni, devel; +Cc: Eric Dong On 10/30/19 10:52, Ray Ni wrote: > MpInitLib sets X2ApicEnable in two places. > 1. CollectProcessorCount() > This function is called when MpInitLibInitialize() hasn't been > called before. > It sets X2ApicEnable and later in the same function it configures > all CPUs to operate in X2 APIC mode. > 2. MpInitLibInitialize() > The X2ApicEnable setting happens when this function is called in > second time. But after that setting, no code consumes that flag. > > With the above analysis and with the purpose of simplifying the code, > the X2ApicEnable in #1 is changed to local variable and the #2 can be > changed to remove the setting of X2ApicEnable. > > Signed-off-by: Ray Ni <ray.ni@intel.com> > Cc: Eric Dong <eric.dong@intel.com> > Cc: Laszlo Ersek <lersek@redhat.com> > --- > UefiCpuPkg/Library/MpInitLib/MpLib.c | 14 ++++++-------- > UefiCpuPkg/Library/MpInitLib/MpLib.h | 1 - > 2 files changed, 6 insertions(+), 9 deletions(-) > > diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c > index 8f62a8d965..49be5d5385 100644 > --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c > +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c > @@ -459,12 +459,12 @@ CollectProcessorCount ( > { > UINTN Index; > CPU_INFO_IN_HOB *CpuInfoInHob; > + BOOLEAN X2Apic; > > // > // Send 1st broadcast IPI to APs to wakeup APs > // > - CpuMpData->InitFlag = ApInitConfig; > - CpuMpData->X2ApicEnable = FALSE; > + CpuMpData->InitFlag = ApInitConfig; > WakeUpAP (CpuMpData, TRUE, 0, NULL, NULL, TRUE); > CpuMpData->InitFlag = ApInitDone; > ASSERT (CpuMpData->CpuCount <= PcdGet32 (PcdCpuMaxLogicalProcessorNumber)); > @@ -481,22 +481,23 @@ CollectProcessorCount ( > // 1. Number of CPU is greater than 255; or > // 2. There are any logical processors reporting an Initial APIC ID of 255 or greater. > // > + X2Apic = FALSE; > if (CpuMpData->CpuCount > 255) { > // > // If there are more than 255 processor found, force to enable X2APIC > // > - CpuMpData->X2ApicEnable = TRUE; > + X2Apic = 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; > + X2Apic = TRUE; > break; > } > } > } > > - if (CpuMpData->X2ApicEnable) { > + if (X2Apic) { > DEBUG ((DEBUG_INFO, "Force x2APIC mode!\n")); > // > // Wakeup all APs to enable x2APIC mode > @@ -1780,9 +1781,6 @@ MpInitLibInitialize ( > CpuInfoInHob = (CPU_INFO_IN_HOB *) (UINTN) CpuMpData->CpuInfoInHob; > for (Index = 0; Index < CpuMpData->CpuCount; Index++) { > InitializeSpinLock(&CpuMpData->CpuData[Index].ApLock); > - if (CpuInfoInHob[Index].InitialApicId >= 255 || Index > 254) { > - CpuMpData->X2ApicEnable = TRUE; > - } > CpuMpData->CpuData[Index].CpuHealthy = (CpuInfoInHob[Index].Health == 0)? TRUE:FALSE; > CpuMpData->CpuData[Index].ApFunction = 0; > CopyMem (&CpuMpData->CpuData[Index].VolatileRegisters, &VolatileRegisters, sizeof (CPU_VOLATILE_REGISTERS)); > diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.h b/UefiCpuPkg/Library/MpInitLib/MpLib.h > index 107872b367..8fa07b12c5 100644 > --- a/UefiCpuPkg/Library/MpInitLib/MpLib.h > +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.h > @@ -227,7 +227,6 @@ struct _CPU_MP_DATA { > UINTN **FailedCpuList; > > AP_INIT_STATE InitFlag; > - BOOLEAN X2ApicEnable; > BOOLEAN SwitchBspFlag; > UINTN NewBspNumber; > CPU_EXCHANGE_ROLE_INFO BSPInfo; > Assuming the previous patch does the right thing in the series (and I think that's plausible), this patch looks correct as well. For a second I got concerned as to whether the field removal from CPU_MP_DATA would require updates to the offset constants in the assembly code. However, assembly code seems to know offsets into MP_CPU_EXCHANGE_INFO only, and doesn't care about CPU_MP_DATA. So the patch is OK I think. Reviewed-by: Laszlo Ersek <lersek@redhat.com> Thanks Laszlo ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [edk2-devel] [PATCH 2/2] UefiCpuPkg/MpInitLib: Remove global variable X2ApicEnable 2019-10-30 9:52 ` [PATCH 2/2] UefiCpuPkg/MpInitLib: Remove global variable X2ApicEnable Ni, Ray 2019-10-31 9:20 ` Laszlo Ersek @ 2019-11-05 1:47 ` Dong, Eric 1 sibling, 0 replies; 7+ messages in thread From: Dong, Eric @ 2019-11-05 1:47 UTC (permalink / raw) To: devel@edk2.groups.io, Ni, Ray; +Cc: Laszlo Ersek Reviewed-by: Eric Dong <eric.dong@intel.com> -----Original Message----- From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of Ni, Ray Sent: Wednesday, October 30, 2019 5:53 PM To: devel@edk2.groups.io Cc: Dong, Eric <eric.dong@intel.com>; Laszlo Ersek <lersek@redhat.com> Subject: [edk2-devel] [PATCH 2/2] UefiCpuPkg/MpInitLib: Remove global variable X2ApicEnable MpInitLib sets X2ApicEnable in two places. 1. CollectProcessorCount() This function is called when MpInitLibInitialize() hasn't been called before. It sets X2ApicEnable and later in the same function it configures all CPUs to operate in X2 APIC mode. 2. MpInitLibInitialize() The X2ApicEnable setting happens when this function is called in second time. But after that setting, no code consumes that flag. With the above analysis and with the purpose of simplifying the code, the X2ApicEnable in #1 is changed to local variable and the #2 can be changed to remove the setting of X2ApicEnable. Signed-off-by: Ray Ni <ray.ni@intel.com> Cc: Eric Dong <eric.dong@intel.com> Cc: Laszlo Ersek <lersek@redhat.com> --- UefiCpuPkg/Library/MpInitLib/MpLib.c | 14 ++++++-------- UefiCpuPkg/Library/MpInitLib/MpLib.h | 1 - 2 files changed, 6 insertions(+), 9 deletions(-) diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c index 8f62a8d965..49be5d5385 100644 --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c @@ -459,12 +459,12 @@ CollectProcessorCount ( { UINTN Index; CPU_INFO_IN_HOB *CpuInfoInHob; + BOOLEAN X2Apic; // // Send 1st broadcast IPI to APs to wakeup APs // - CpuMpData->InitFlag = ApInitConfig; - CpuMpData->X2ApicEnable = FALSE; + CpuMpData->InitFlag = ApInitConfig; WakeUpAP (CpuMpData, TRUE, 0, NULL, NULL, TRUE); CpuMpData->InitFlag = ApInitDone; ASSERT (CpuMpData->CpuCount <= PcdGet32 (PcdCpuMaxLogicalProcessorNumber)); @@ -481,22 +481,23 @@ CollectProcessorCount ( // 1. Number of CPU is greater than 255; or // 2. There are any logical processors reporting an Initial APIC ID of 255 or greater. // + X2Apic = FALSE; if (CpuMpData->CpuCount > 255) { // // If there are more than 255 processor found, force to enable X2APIC // - CpuMpData->X2ApicEnable = TRUE; + X2Apic = 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; + X2Apic = TRUE; break; } } } - if (CpuMpData->X2ApicEnable) { + if (X2Apic) { DEBUG ((DEBUG_INFO, "Force x2APIC mode!\n")); // // Wakeup all APs to enable x2APIC mode @@ -1780,9 +1781,6 @@ MpInitLibInitialize ( CpuInfoInHob = (CPU_INFO_IN_HOB *) (UINTN) CpuMpData->CpuInfoInHob; for (Index = 0; Index < CpuMpData->CpuCount; Index++) { InitializeSpinLock(&CpuMpData->CpuData[Index].ApLock); - if (CpuInfoInHob[Index].InitialApicId >= 255 || Index > 254) { - CpuMpData->X2ApicEnable = TRUE; - } CpuMpData->CpuData[Index].CpuHealthy = (CpuInfoInHob[Index].Health == 0)? TRUE:FALSE; CpuMpData->CpuData[Index].ApFunction = 0; CopyMem (&CpuMpData->CpuData[Index].VolatileRegisters, &VolatileRegisters, sizeof (CPU_VOLATILE_REGISTERS)); diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.h b/UefiCpuPkg/Library/MpInitLib/MpLib.h index 107872b367..8fa07b12c5 100644 --- a/UefiCpuPkg/Library/MpInitLib/MpLib.h +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.h @@ -227,7 +227,6 @@ struct _CPU_MP_DATA { UINTN **FailedCpuList; AP_INIT_STATE InitFlag; - BOOLEAN X2ApicEnable; BOOLEAN SwitchBspFlag; UINTN NewBspNumber; CPU_EXCHANGE_ROLE_INFO BSPInfo; -- 2.21.0.windows.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
end of thread, other threads:[~2019-11-05 1:48 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2019-10-30 9:52 [PATCH 0/2] Set X2ApicEnable flag from BSP Ni, Ray 2019-10-30 9:52 ` [PATCH 1/2] UefiCpuPkg/MpInitLib: " Ni, Ray 2019-10-31 9:11 ` Laszlo Ersek 2019-11-05 1:47 ` [edk2-devel] " Dong, Eric 2019-10-30 9:52 ` [PATCH 2/2] UefiCpuPkg/MpInitLib: Remove global variable X2ApicEnable Ni, Ray 2019-10-31 9:20 ` Laszlo Ersek 2019-11-05 1:47 ` [edk2-devel] " Dong, Eric
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox