On 2/15/24 10:31, Gerd Hoffmann wrote:
> Loop over all MP_HAND_OFF HOBs instead of expecting a single HOB
> covering all CPUs in the system.
>
> Add a new HaveMpHandOff variable to track whenever MP_HAND_OFF HOBs are
> present, using the MpHandOff pointer for that does not work because the
> variable will be NULL after looping over all HOBs.
>
> Signed-off-by: Gerd Hoffmann <kra...@redhat.com>
> ---
> UefiCpuPkg/Library/MpInitLib/MpLib.c | 54 +++++++++++++++++++---------
> 1 file changed, 37 insertions(+), 17 deletions(-)
>
> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> index 35f47d3b1289..1547b7e74a1a 100644
> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> @@ -2023,6 +2023,7 @@ MpInitLibInitialize (
> )
> {
> MP_HAND_OFF *MpHandOff;
> + BOOLEAN HaveMpHandOff;
> CPU_INFO_IN_HOB *CpuInfoInHob;
> UINT32 MaxLogicalProcessorNumber;
> UINT32 ApStackSize;
> @@ -2035,17 +2036,29 @@ MpInitLibInitialize (
> CPU_MP_DATA *CpuMpData;
> UINT8 ApLoopMode;
> UINT8 *MonitorBuffer;
> - UINTN Index;
> + UINTN Index, HobIndex;
> UINTN ApResetVectorSizeBelow1Mb;
> UINTN ApResetVectorSizeAbove1Mb;
> UINTN BackupBufferAddr;
> UINTN ApIdtBase;
>
> - MpHandOff = GetMpHandOffHob (0);
> - if (MpHandOff == NULL) {
> + MaxLogicalProcessorNumber = 0;
> + HaveMpHandOff = FALSE;
> +
> + while ((MpHandOff = GetMpHandOffHob (MaxLogicalProcessorNumber)) != 0) {
> + DEBUG ((
> + DEBUG_INFO,
> + "%a: ProcessorIndex=%u CpuCount=%u\n",
> + __func__,
> + MpHandOff->ProcessorIndex,
> + MpHandOff->CpuCount
> + ));
> + MaxLogicalProcessorNumber += MpHandOff->CpuCount;
> + HaveMpHandOff = TRUE;
> + }
> +
> + if (!HaveMpHandOff) {
> MaxLogicalProcessorNumber = PcdGet32 (PcdCpuMaxLogicalProcessorNumber);
> - } else {
> - MaxLogicalProcessorNumber = MpHandOff->CpuCount;
> }
>
> ASSERT (MaxLogicalProcessorNumber != 0);
How about:
- just looping over the GUID HOBs, summing their CpuCount fields,
- replacing the HaveMpHandOff variable with the sum being nonzero, after
the loop
(One extra assertion, on top, could be: while iterating over the GUID
HOB list, also perform a maximum search for (MpHandOff->ProcessorIndex +
MpHandOff->CpuCount), and after the loop, check that the maximum found
like this equals the sum of CpuCount fields. But this is really an extra.)
In other words -- again, I don't see any need for going through the HOBs
in a particular order, just for summing their CpuCount fields.
> @@ -2189,7 +2202,7 @@ MpInitLibInitialize (
> //
> ProgramVirtualWireMode ();
>
> - if (MpHandOff == NULL) {
> + if (!HaveMpHandOff) {
> if (MaxLogicalProcessorNumber > 1) {
> //
> // Wakeup all APs and calculate the processor count in system
> @@ -2205,19 +2218,26 @@ MpInitLibInitialize (
> AmdSevUpdateCpuMpData (CpuMpData);
> }
Hm, okay, considering this code, I do agree we need the "HaveMpHandOff"
variable. Because, at this point, MaxLogicalProcessorNumber will be
nonzero, regardless of how we calculated it it.
>
> - CpuMpData->CpuCount = MpHandOff->CpuCount;
> + CpuMpData->CpuCount = MaxLogicalProcessorNumber;
> CpuMpData->BspNumber = GetBspNumber ();
> CpuInfoInHob = (CPU_INFO_IN_HOB *)(UINTN)CpuMpData->CpuInfoInHob;
> - for (Index = 0; Index < CpuMpData->CpuCount; Index++) {
This original code here is a bit confused. Namely, after briefly
auditing MpInitLibInitialize(), it seems like "Index" should always have
been UINT32; making it UINTN is a confusing and unneeded generality.
(But, if you disagree with my claim, please say so.)
And the new variable HobIndex should be UINT32 too.
> - InitializeSpinLock (&CpuMpData->CpuData[Index].ApLock);
> - CpuMpData->CpuData[Index].CpuHealthy = (MpHandOff->Info[Index].Health
> == 0) ? TRUE : FALSE;
Comment on the pre-patch code:
Predicate ? TRUE : FALSE
is poor style.
> - CpuMpData->CpuData[Index].ApFunction = 0;
> - CpuInfoInHob[Index].InitialApicId = MpHandOff->Info[Index].ApicId;
> - CpuInfoInHob[Index].ApTopOfStack = CpuMpData->Buffer + (Index + 1)
> * CpuMpData->CpuApStackSize;
> - CpuInfoInHob[Index].ApicId = MpHandOff->Info[Index].ApicId;
> - CpuInfoInHob[Index].Health = MpHandOff->Info[Index].Health;
> + for (MpHandOff = GetMpHandOffHob (0);
> + MpHandOff != NULL;
> + MpHandOff = GetMpHandOffHob (MpHandOff->ProcessorIndex +
> MpHandOff->CpuCount))
> + {
> + for (HobIndex = 0; HobIndex < MpHandOff->CpuCount; HobIndex++) {
> + Index = MpHandOff->ProcessorIndex + HobIndex;
> + InitializeSpinLock (&CpuMpData->CpuData[Index].ApLock);
> + CpuMpData->CpuData[Index].CpuHealthy =
> (MpHandOff->Info[HobIndex].Health == 0) ? TRUE : FALSE;
> + CpuMpData->CpuData[Index].ApFunction = 0;
> + CpuInfoInHob[Index].InitialApicId =
> MpHandOff->Info[HobIndex].ApicId;
> + CpuInfoInHob[Index].ApTopOfStack = CpuMpData->Buffer + (Index +
> 1) * CpuMpData->CpuApStackSize;
> + CpuInfoInHob[Index].ApicId =
> MpHandOff->Info[HobIndex].ApicId;
> + CpuInfoInHob[Index].Health =
> MpHandOff->Info[HobIndex].Health;
> + }
> }
Same observation as before: we don't need to *dicate* the HOB order for
these nested loops, we can (and should) just process the HOBs in
whatever order they appear. That makes the outer loop linear, rather
than quadratic.
>
> + MpHandOff = GetMpHandOffHob (0);
> DEBUG ((DEBUG_INFO, "MpHandOff->WaitLoopExecutionMode: %04d, sizeof
> (VOID *): %04d\n", MpHandOff->WaitLoopExecutionMode, sizeof (VOID *)));
> if (MpHandOff->WaitLoopExecutionMode == sizeof (VOID *)) {
> ASSERT (CpuMpData->ApLoopMode != ApInHltLoop);
This indicates that a refactoring -- "normalization" -- of the
MP_HAND_OFF structure is necessary.
Namely, each MP_HAND_OFF HOB contains a WaitLoopExecutionMode field, but
their values are expected to be identical. And here you have to use one
of the HOBs (doesn't matter which one) for getting that value.
The WaitLoopExecutionMode field should be a singleton, regardless of how
many MP_HAND_OFF HOBs exist. It could be a separate GUID HOB, or perhaps
(for simplicity?) a dynamic UINT32 PCD.
... In fact, the same seems to apply to the StartupSignalValue field.
That field is only ever set to MP_HAND_OFF_SIGNAL, and that setting only
depends on "CpuMpData->ApLoopMode". Therefore both WaitLoopExecutionMode
and StartupSignalValue are singletons, and should not be duplicated;
they should be "normalized out" to dynamic PCDs or a separate (unique)
GUID HOB.
> @@ -2284,7 +2304,7 @@ MpInitLibInitialize (
> // Wakeup APs to do some AP initialize sync (Microcode & MTRR)
> //
> if (CpuMpData->CpuCount > 1) {
> - if (MpHandOff != NULL) {
> + if (HaveMpHandOff) {
> //
> // Only needs to use this flag for DXE phase to update the wake up
> // buffer. Wakeup buffer allocated in PEI phase is no longer valid
> @@ -2301,7 +2321,7 @@ MpInitLibInitialize (
> CpuPause ();
> }
>
> - if (MpHandOff != NULL) {
> + if (HaveMpHandOff) {
> CpuMpData->InitFlag = ApInitDone;
> }
>
Laszlo
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#115594): https://edk2.groups.io/g/devel/message/115594
Mute This Topic: https://groups.io/mt/104369843/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
