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...@intel.com>
> Cc: Eric Dong <eric.d...@intel.com>
> Cc: Laszlo Ersek <ler...@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 <ler...@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/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
