Re: [edk2-devel] [PATCH v1] UefiCpuPkg/Library/MpInitLib: fix SEV-ES AP bootinng failure

["Lendacky, Thomas via groups.io" <thomas.lendacky=amd.com@groups.io>]

On 7/31/24 05:22, Wencheng Yang via groups.io wrote:
> According to SEV-ES Guest-Hypervisor Communication Block Standardization
> section 4.3 SMP Booting, the subsequent reset requires the AP enters
> Reset Hold state either by AP Reset Hold NAE event or
> AP Reset Hold Request MSR Protocol.
> 
> If the AP is not in AP Reset Hold state, it may miss subsequent
> INIT-SIPI, as the INIT-SIPI process depends on GHCB page in kernel,
> which is only mapped in VMGEXIT interception.
> 
> To ensure all APs are in AP Reset Hold state, we add a var in
> AP specific data structure, set the var before the AP going to
> AP Reset Hold state. Subsequent INIT-SIPI should check the var of each
> AP before issuing INIT-SIPI signal.

Were you actually seeing an issue here?

The moving of InterlockedDecrement() was for the same reason as this
patch. I assume this was discovered when the CpuMpData->InitFlag wasn't
set to ApInitConfig, which skips the InterlockedDecrement()? What if the
InterlockedIncrement() was delayed like InterlockedDecrement(), would
that also fix the issue? If so, that would remove the changes to, and
related to, the CPU_AP_DATA struct.

If not, then using this method can remove the changes around the
InterlockedDecrement() in the ApWakeupFunction() and SevEsPlaceApHlt()
and just rely on the EnteredHlt flag/

I just saw this and haven't had a chance to test it, yet. I'm very busy
at the moment but will try to get to it very soon.

> 
> Cc: Yuanhao Xie <yuanhao.xie@intel.com>
> Cc: Brijesh Singh <brijesh.singh@amd.com>
> Cc: Min Xu <min.m.xu@intel.com>
> Cc: Michael Kubacki <michael.kubacki@microsoft.com>
> 
> Signed-off-by: Wencheng Yang <east.moutain.yang@gmail.com>
> ---
>  UefiCpuPkg/Library/MpInitLib/AmdSev.c    | 11 ++++++-
>  UefiCpuPkg/Library/MpInitLib/MpHandOff.h |  1 +
>  UefiCpuPkg/Library/MpInitLib/MpLib.c     | 39 ++++++++++++++++++++++--
>  UefiCpuPkg/Library/MpInitLib/MpLib.h     |  4 ++-
>  UefiCpuPkg/Library/MpInitLib/PeiMpLib.c  |  2 ++
>  5 files changed, 52 insertions(+), 5 deletions(-)
> 
> diff --git a/UefiCpuPkg/Library/MpInitLib/AmdSev.c b/UefiCpuPkg/Library/MpInitLib/AmdSev.c
> index d34f9513e0..e5d5ecb181 100644
> --- a/UefiCpuPkg/Library/MpInitLib/AmdSev.c
> +++ b/UefiCpuPkg/Library/MpInitLib/AmdSev.c
> @@ -194,16 +194,19 @@ SetSevEsJumpTable (
>  **/
>  VOID
>  SevEsPlaceApHlt (
> -  CPU_MP_DATA  *CpuMpData
> +  CPU_MP_DATA  *CpuMpData,
> +  UINT32 ProcessorNumber

The alignment is off here (and in other places below). You'll need to
run this through uncrustify so that it doesn't fail CI.

>    )
>  {
>    MSR_SEV_ES_GHCB_REGISTER  Msr;
>    GHCB                      *Ghcb;
>    UINT64                    Status;
>    BOOLEAN                   DoDecrement;
> +  BOOLEAN                   EnterHltLoop;

Maybe a better name is EnteredHlt.

>    BOOLEAN                   InterruptState;
>  
>    DoDecrement = (BOOLEAN)(CpuMpData->InitFlag == ApInitConfig);
> +  EnterHltLoop = FALSE;
>  
>    while (TRUE) {
>      Msr.GhcbPhysicalAddress = AsmReadMsr64 (MSR_SEV_ES_GHCB);
> @@ -221,7 +224,13 @@ SevEsPlaceApHlt (
>        InterlockedDecrement ((UINT32 *)&CpuMpData->MpCpuExchangeInfo->NumApsExecuting);
>      }
>  
> +    if (!EnterHltLoop) {
> +      EnterHltLoop = TRUE;
> +      CpuMpData->CpuData[ProcessorNumber].SevEsApEnterHltLoopAfterWakeup = 1;
> +    }
> +
>      Status = CcExitVmgExit (Ghcb, SVM_EXIT_AP_RESET_HOLD, 0, 0);
> +

Remove the added blank line.

>      if ((Status == 0) && (Ghcb->SaveArea.SwExitInfo2 != 0)) {
>        CcExitVmgDone (Ghcb, InterruptState);
>        break;
> diff --git a/UefiCpuPkg/Library/MpInitLib/MpHandOff.h b/UefiCpuPkg/Library/MpInitLib/MpHandOff.h
> index ae93b7e3d7..50e290f5b3 100644
> --- a/UefiCpuPkg/Library/MpInitLib/MpHandOff.h
> +++ b/UefiCpuPkg/Library/MpInitLib/MpHandOff.h
> @@ -37,6 +37,7 @@ extern EFI_GUID  mMpHandOffConfigGuid;
>  typedef struct {
>    UINT32    ApicId;
>    UINT32    Health;
> +  UINT32    SevEsApEnterHltLoopAfterWakeup;

SevEsEnteredHlt should be enough, right?

>    UINT64    StartupSignalAddress;
>    UINT64    StartupProcedureAddress;
>  } PROCESSOR_HAND_OFF;
> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> index 1951922912..59f0d87f9d 100644
> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> @@ -660,13 +660,14 @@ InitializeApData (
>  **/
>  VOID
>  PlaceAPInHltLoop (
> -  IN CPU_MP_DATA  *CpuMpData
> +  IN CPU_MP_DATA  *CpuMpData,
> +  IN UINTN ProcessorNumber
>    )
>  {
>    while (TRUE) {
>      DisableInterrupts ();
>      if (CpuMpData->UseSevEsAPMethod) {
> -      SevEsPlaceApHlt (CpuMpData);
> +      SevEsPlaceApHlt (CpuMpData, ProcessorNumber);
>      } else {
>        CpuSleep ();
>      }
> @@ -762,6 +763,9 @@ ApWakeupFunction (
>    while (TRUE) {
>      if (CpuMpData->InitFlag == ApInitConfig) {
>        ProcessorNumber = ApIndex;
> +      if (CpuMpData->UseSevEsAPMethod) {
> +        CpuMpData->CpuData[ProcessorNumber].SevEsApEnterHltLoopAfterWakeup = 0;
> +      }
>        //
>        // This is first time AP wakeup, get BIST information from AP stack
>        //
> @@ -782,6 +786,9 @@ ApWakeupFunction (
>        // Execute AP function if AP is ready
>        //
>        GetProcessorNumber (CpuMpData, &ProcessorNumber);
> +      if (CpuMpData->UseSevEsAPMethod) {
> +        CpuMpData->CpuData[ProcessorNumber].SevEsApEnterHltLoopAfterWakeup = 0;
> +      }
>        //
>        // Clear AP start-up signal when AP waken up
>        //
> @@ -903,7 +910,7 @@ ApWakeupFunction (
>      // Place AP is specified loop mode
>      //
>      if (CpuMpData->ApLoopMode == ApInHltLoop) {
> -      PlaceAPInHltLoop (CpuMpData);
> +      PlaceAPInHltLoop (CpuMpData, ProcessorNumber);
>        //
>        // Never run here
>        //
> @@ -993,6 +1000,20 @@ GetApResetVectorSize (
>      *SizeAbove1Mb = AddressMap->RendezvousFunnelSize - AddressMap->ModeTransitionOffset;
>    }
>  }
> +/**
> +  Wait for SEV-ES AP enter in HLT-LOOP.^M
> +
> +  @param[in] SevEsApInHltLoop  Pointer to SevEsApInHltLoop^M
> +**/
> +VOID
> +WaitSevEsApEnterHltLoopAfterWakeup (
> +  IN volatile UINT32  *SevEsApEnterHltLoop
> +  )
> +{
> +  while (*(UINT32*)SevEsApEnterHltLoop == 0) {
> +    CpuPause ();
> +  }
> +}
>  
>  /**
>    This function will fill the exchange info structure.
> @@ -1324,6 +1345,17 @@ WakeUpAP (
>            //
>            SendStartupIpiAllExcludingSelf ((UINT32)ExchangeInfo->BufferStart);
>          } else {
> +          //
> +          // Subsequent INIT-SIPI-SIPI
> +          //
> +          if (CpuMpData->SevEsIsEnabled && (CpuMpData->InitFlag != ApInitConfig)) {
> +            for (Index = 0; Index < CpuMpData->CpuCount; Index++) {
> +              CpuData = &CpuMpData->CpuData[Index];
> +              if (Index != CpuMpData->BspNumber) {
> +                WaitSevEsApEnterHltLoopAfterWakeup(&CpuData->SevEsApEnterHltLoopAfterWakeup);
> +              }
> +            }
> +          }
>            SendInitSipiSipiAllExcludingSelf ((UINT32)ExchangeInfo->BufferStart);
>          }
>        }
> @@ -2270,6 +2302,7 @@ MpInitLibInitialize (
>          InitializeSpinLock (&CpuMpData->CpuData[Index].ApLock);
>          CpuMpData->CpuData[Index].CpuHealthy = (MpHandOff->Info[HobIndex].Health == 0) ? TRUE : FALSE;
>          CpuMpData->CpuData[Index].ApFunction = 0;
> +        CpuMpData->CpuData[Index].SevEsApEnterHltLoopAfterWakeup = MpHandOff->Info[HobIndex].SevEsApEnterHltLoopAfterWakeup;
>          CpuInfoInHob[Index].InitialApicId    = MpHandOff->Info[HobIndex].ApicId;
>          CpuInfoInHob[Index].ApTopOfStack     = CpuMpData->Buffer + (Index + 1) * CpuMpData->CpuApStackSize;
>          CpuInfoInHob[Index].ApicId           = MpHandOff->Info[HobIndex].ApicId;
> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.h b/UefiCpuPkg/Library/MpInitLib/MpLib.h
> index 88b31fecca..c33a7bf658 100644
> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.h
> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.h
> @@ -146,6 +146,7 @@ typedef enum {
>  typedef struct {
>    SPIN_LOCK                 ApLock;
>    volatile UINT32           *StartupApSignal;
> +  volatile UINT32           SevEsApEnterHltLoopAfterWakeup;

Similar to above, SevEsEnteredHlt.

Thanks,
Tom

>    volatile UINTN            ApFunction;
>    volatile UINTN            ApFunctionArgument;
>    BOOLEAN                   CpuHealthy;
> @@ -862,7 +863,8 @@ SetSevEsJumpTable (
>  **/
>  VOID
>  SevEsPlaceApHlt (
> -  CPU_MP_DATA  *CpuMpData
> +  CPU_MP_DATA  *CpuMpData,
> +  UINT32 ProcessorNumber
>    );
>  
>  /**
> diff --git a/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c b/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c
> index 16a858d542..b393ce00be 100644
> --- a/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c
> +++ b/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c
> @@ -164,6 +164,8 @@ SaveCpuMpData (
>      if (CpuMpData->ApLoopMode != ApInHltLoop) {
>        MpHandOff->Info[Index-HobBase].StartupSignalAddress    = (UINT64)(UINTN)CpuMpData->CpuData[Index].StartupApSignal;
>        MpHandOff->Info[Index-HobBase].StartupProcedureAddress = (UINT64)(UINTN)&CpuMpData->CpuData[Index].ApFunction;
> +    } else {
> +      MpHandOff->Info[Index-HobBase].SevEsApEnterHltLoopAfterWakeup = CpuMpData->CpuData[Index].SevEsApEnterHltLoopAfterWakeup;
>      }
>    }
>  


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#120179): https://edk2.groups.io/g/devel/message/120179
Mute This Topic: https://groups.io/mt/107630732/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-