Re: [PATCH 2/2] UefiCpuPkg/MpInitLib: resend the Start-up IPI when an AP misses it #VSTOR-146639

[Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>]

Ack
Overall LGTM, see some minor nits below

On 9/21/26 6:44 PM, Denis V. Lunev wrote:
> From: Denis V. Lunev <den@openvz.org>
> 
> WaitApWakeup spins until the AP clears its wakeup signal, with no
> timeout and no way out. If the AP never consumes the Start-up IPI, the
> BSP spins forever and the boot is dead.
> 
> A hypervisor can lose that IPI. Under KVM, INIT is latched while a
> vCPU is in SMM but the Start-up IPI is discarded outright, so an AP
> that takes an SMI just before the BSP broadcasts INIT-SIPI-SIPI is
> left in wait-for-SIPI with nothing left to start it. The window is the
> few instructions between an AP releasing the BSP at the end of the SMM
> rendezvous and its own RSM, which a scheduled vCPU can be preempted
> inside of. KVM holds that delivering the SIPI is software's job, so
> the firmware has to cope.
> 
> Resend rather than spin. Address the AP individually so that APs
> already running the procedure are not disturbed, and pass the reset
> vector only on the paths that sent a Start-up IPI in the first place,
> so the SEV-SNP and hand-off paths keep waiting as before. A Start-up
> IPI is ignored unless the target is in wait-for-SIPI, so resending
> costs nothing when the AP is merely slow.
> 
> The failure this fixes looks like a guest that never boots: firmware
> starts the boot manager, paints nothing further, and one CPU spins
> while the other APs sit idle.
> 
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> Cc: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
> ---
>  UefiCpuPkg/Library/MpInitLib/MpLib.c | 85 +++++++++++++++++++++++++---
>  UefiCpuPkg/Library/MpInitLib/MpLib.h |  2 +
>  2 files changed, 80 insertions(+), 7 deletions(-)
> 
> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> index 426d998b9a..87e8f824b6 100644
> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> @@ -942,16 +942,63 @@ DxeApEntryPoint (
>    ApWakeupFunction (CpuMpData, ProcessorNumber);
>  }
>  
> +/**
> +  Calculate timeout value and return the current performance counter value.
> +
> +  @param[in]  TimeoutInMicroseconds  The timeout value in microseconds.
> +  @param[out] CurrentTime            Returns the current value of the performance counter.
> +
> +  @return Expected timestamp counter for timeout, or 0 if TimeoutInMicroseconds is 0.
> +**/
> +UINT64
> +CalculateTimeout (
> +  IN  UINTN   TimeoutInMicroseconds,
> +  OUT UINT64  *CurrentTime
> +  );
> +
> +/**
> +  Checks whether timeout expires.
> +
> +  @param[in, out]  PreviousTime   On input, the requested time. On output, the elapsed time.
> +  @param[in]       TotalTime      The total amount of elapsed time.
> +  @param[in]       Timeout        The timeout ticks, 0 means infinity.
> +
> +  @retval TRUE   A timeout occurred.
> +  @retval FALSE  No timeout occurred.
> +**/
> +BOOLEAN
> +CheckTimeout (
> +  IN OUT UINT64  *PreviousTime,
> +  IN     UINT64  *TotalTime,
> +  IN     UINT64  Timeout
> +  );
> +

These functions are only used here in this translation unit, yet they
aren't declared static.  Should we wither declare them static and move
their definitions above (without the need for forward declarations), or
just declare them in MpLib.h?

>  /**
>    Wait for AP wakeup and write AP start-up signal till AP is waken up.
>  
>    @param[in] ApStartupSignalBuffer  Pointer to AP wakeup signal
> +  @param[in] ApicId                 Local APIC ID of the AP being woken
> +  @param[in] StartupRoutine         AP reset vector, or 0 if the AP was not
> +                                    woken by INIT-SIPI-SIPI and no Start-up
> +                                    IPI may be resent
>  **/
>  VOID
>  WaitApWakeup (
> -  IN volatile UINT32  *ApStartupSignalBuffer
> +  IN volatile UINT32  *ApStartupSignalBuffer,
> +  IN UINT32           ApicId,
> +  IN UINT32           StartupRoutine
>    )
>  {
> +  UINT64  CurrentTime;
> +  UINT64  TotalTime;
> +  UINT64  Timeout;
> +
> +  Timeout = CalculateTimeout (
> +              (StartupRoutine != 0) ? AP_SIPI_RESEND_TIMEOUT_US : 0,
> +              &CurrentTime
> +              );
> +  TotalTime = 0;
> +
>    //
>    // If AP is waken up, StartupApSignal should be cleared.
>    // Otherwise, write StartupApSignal again till AP waken up.
> @@ -962,6 +1009,12 @@ WaitApWakeup (
>             WAKEUP_AP_SIGNAL
>             ) != 0)
>    {
> +    if (CheckTimeout (&CurrentTime, &TotalTime, Timeout)) {
> +      SendStartupIpi (ApicId, StartupRoutine);
> +      Timeout   = CalculateTimeout (AP_SIPI_RESEND_TIMEOUT_US, &CurrentTime);
> +      TotalTime = 0;
> +    }

This re-send of IPI should happen rarely.  Wouldn't we wanna know when
it does happen?  How about adding some debug info when IPI is re-issued?

Andrey

> +
>      CpuPause ();
>    }
>  }
> @@ -1256,8 +1309,10 @@ WakeUpAP (
>    CPU_AP_DATA                    *CpuData;
>    BOOLEAN                        ResetVectorRequired;
>    CPU_INFO_IN_HOB                *CpuInfoInHob;
> +  UINT32                         SipiVector;
>  
>    CpuMpData->FinishedCount = 0;
> +  SipiVector               = 0;
>    ResetVectorRequired      = FALSE;
>  
>    if (CpuMpData->WakeUpByInitSipiSipi ||
> @@ -1320,13 +1375,14 @@ WakeUpAP (
>        if (CanUseSevSnpCreateAP (CpuMpData)) {
>          SevSnpCreateAP (CpuMpData, -1);
>        } else {
> +        SipiVector = (UINT32)ExchangeInfo->BufferStart;
>          if ((CpuMpData->InitFlag == ApInitConfig) && FixedPcdGetBool (PcdFirstTimeWakeUpAPsBySipi)) {
>            //
>            // SIPI can be used for the first time wake up after reset to reduce boot time.
>            //
> -          SendStartupIpiAllExcludingSelf ((UINT32)ExchangeInfo->BufferStart);
> +          SendStartupIpiAllExcludingSelf (SipiVector);
>          } else {
> -          SendInitSipiSipiAllExcludingSelf ((UINT32)ExchangeInfo->BufferStart);
> +          SendInitSipiSipiAllExcludingSelf (SipiVector);
>          }
>        }
>      }
> @@ -1398,10 +1454,15 @@ WakeUpAP (
>        //
>        // Wait all APs waken up if this is not the 1st broadcast of SIPI
>        //
> +      CpuInfoInHob = (CPU_INFO_IN_HOB *)(UINTN)CpuMpData->CpuInfoInHob;
>        for (Index = 0; Index < CpuMpData->CpuCount; Index++) {
>          CpuData = &CpuMpData->CpuData[Index];
>          if (Index != CpuMpData->BspNumber) {
> -          WaitApWakeup (CpuData->StartupApSignal);
> +          WaitApWakeup (
> +            CpuData->StartupApSignal,
> +            CpuInfoInHob[Index].ApicId,
> +            SipiVector
> +            );
>          }
>        }
>      }
> @@ -1430,9 +1491,10 @@ WakeUpAP (
>        if (CanUseSevSnpCreateAP (CpuMpData)) {
>          SevSnpCreateAP (CpuMpData, (INTN)ProcessorNumber);
>        } else {
> +        SipiVector = (UINT32)ExchangeInfo->BufferStart;
>          SendInitSipiSipi (
>            CpuInfoInHob[ProcessorNumber].ApicId,
> -          (UINT32)ExchangeInfo->BufferStart
> +          SipiVector
>            );
>        }
>      }
> @@ -1440,7 +1502,12 @@ WakeUpAP (
>      //
>      // Wait specified AP waken up
>      //
> -    WaitApWakeup (CpuData->StartupApSignal);
> +    CpuInfoInHob = (CPU_INFO_IN_HOB *)(UINTN)CpuMpData->CpuInfoInHob;
> +    WaitApWakeup (
> +      CpuData->StartupApSignal,
> +      CpuInfoInHob[ProcessorNumber].ApicId,
> +      SipiVector
> +      );
>    }
>  
>    if (ResetVectorRequired) {
> @@ -2017,7 +2084,11 @@ SwitchApContext (
>    {
>      for (Index = 0; Index < MpHandOff->CpuCount; Index++) {
>        if (MpHandOff->ProcessorIndex + Index != BspNumber) {
> -        WaitApWakeup ((UINT32 *)(UINTN)(MpHandOff->Info[Index].StartupSignalAddress));
> +        WaitApWakeup (
> +          (UINT32 *)(UINTN)(MpHandOff->Info[Index].StartupSignalAddress),
> +          0,
> +          0
> +          );
>        }
>      }
>    }
> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.h b/UefiCpuPkg/Library/MpInitLib/MpLib.h
> index 5d10516cd8..94bda7f1ea 100644
> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.h
> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.h
> @@ -44,6 +44,8 @@
>  #include "MpHandOff.h"
>  
>  #define WAKEUP_AP_SIGNAL  SIGNATURE_32 ('S', 'T', 'A', 'P')
> +
> +#define AP_SIPI_RESEND_TIMEOUT_US  10000
>  //
>  // To trigger the start-up signal, BSP writes the specified
>  // StartupSignalValue to the StartupSignalAddress of each processor.