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
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#49650): https://edk2.groups.io/g/devel/message/49650
Mute This Topic: https://groups.io/mt/39769104/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
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
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#49943): https://edk2.groups.io/g/devel/message/49943
Mute This Topic: https://groups.io/mt/39769104/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
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
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#49742): https://edk2.groups.io/g/devel/message/49742
Mute This Topic: https://groups.io/mt/39769104/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
© 2016 - 2026 Red Hat, Inc.