Re: [edk2-devel] [Patch V3] UefiCpuPkg/PiSmmCpuDxeSmm: SmmCpuRendezvous ensure all Aps in Present.

["Yuanhao Xie" <yuanhao.xie@intel.com>]

Hi Ray,

It means that:
The "SmmCpuRendezvous" function has a bug in that it summons all APs into SMM, but there is a time gap before they are set as "present." During this window, if using the "setVariable" operation, it can cause issues because "setVariable" requires all APs to be in SMM and marked as "present." Therefore, this patch adds extra logic to "SmmCpuRendezvous" to check if the APs are already "present" before proceeding.

Thanks,
Yuanhao
-----Original Message-----
From: Ni, Ray <ray.ni@intel.com> 
Sent: Tuesday, December 19, 2023 12:01 PM
To: Xie, Yuanhao <yuanhao.xie@intel.com>; devel@edk2.groups.io
Cc: Li, Zhihao <zhihao.li@intel.com>; Kumar, Rahul R <rahul.r.kumar@intel.com>; Gerd Hoffmann <kraxel@redhat.com>; Wu, Jiaxin <jiaxin.wu@intel.com>
Subject: RE: [Patch V3] UefiCpuPkg/PiSmmCpuDxeSmm: SmmCpuRendezvous ensure all Aps in Present.

Yuanhao, Zhihao,
Can you kindly explain a bit more about this sentence in commit message " SMM read save state requires AP must be present."?

Thanks,
Ray
> -----Original Message-----
> From: Xie, Yuanhao <yuanhao.xie@intel.com>
> Sent: Friday, December 15, 2023 5:30 PM
> To: devel@edk2.groups.io
> Cc: Xie, Yuanhao <yuanhao.xie@intel.com>; Li, Zhihao 
> <zhihao.li@intel.com>; Ni, Ray <ray.ni@intel.com>; Kumar, Rahul R 
> <rahul.r.kumar@intel.com>; Gerd Hoffmann <kraxel@redhat.com>; Wu, 
> Jiaxin <jiaxin.wu@intel.com>
> Subject: [Patch V3] UefiCpuPkg/PiSmmCpuDxeSmm: SmmCpuRendezvous ensure 
> all Aps in Present.
> 
> SMM read save state requires AP must be present.
> This patch aim to avoid the AP not ready for save state check.
> Furthermore, SmmWaitForApArrival has two callers: SmmCpuRendezvous and 
> BSPHandler, the behavior of the two callers can be consistent by add 
> the while loop to check the present of the APs to SmmCpuRendezvous.
> 
> Signed-off-by: Zhihao Li <zhihao.li@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Rahul Kumar <rahul1.kumar@intel.com>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Jiaxin Wu <jiaxin.wu@intel.com>
> ---
>  UefiCpuPkg/PiSmmCpuDxeSmm/CpuService.c     | 27
> +++++++++++++++++++++++++++
>  UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h | 13 +++++++++++++
>  2 files changed, 40 insertions(+)
> 
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuService.c
> b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuService.c
> index c0485b0519..3e98955319 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuService.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuService.c
> @@ -416,8 +416,15 @@ SmmCpuRendezvous (
>    IN BOOLEAN                            BlockingMode
>    )
>  {
> +  UINTN       Index;
> +  UINTN       PresentCount;
> +  UINT32      BlockedCount;
> +  UINT32      DisabledCount;
>    EFI_STATUS  Status;
> 
> +  BlockedCount  = 0;
> +  DisabledCount = 0;
> +
>    //
>    // Return success immediately if all CPUs are already synchronized.
>    //
> @@ -436,6 +443,26 @@ SmmCpuRendezvous (
>      // There are some APs outside SMM, Wait for all avaiable APs to arrive.
>      //
>      SmmWaitForApArrival ();
> +
> +    //
> +    // Make sure all APs have their Present flag set
> +    //
> +    while (TRUE) {
> +      PresentCount = 0;
> +      for (Index = 0; Index < mMaxNumberOfCpus; Index++) {
> +        if (*(mSmmMpSyncData->CpuData[Index].Present)) {
> +          PresentCount++;
> +        }
> +      }
> +
> +      GetSmmDelayedBlockedDisabledCount (NULL, &BlockedCount,
> &DisabledCount);
> +      if (PresentCount == mMaxNumberOfCpus - BlockedCount -
> DisabledCount ) {
> +        break;
> +      }
> +
> +      CpuPause ();
> +    }
> +
>      Status = mSmmMpSyncData->AllApArrivedWithException ?
> EFI_SUCCESS : EFI_TIMEOUT;
>    } else {
>      //
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> index f18345881b..e3538957fb 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> @@ -1552,6 +1552,19 @@ SmmWaitForApArrival (
>    VOID
>    );
> 
> +/**
> +  Returns the Number of SMM Delayed & Blocked & Disabled Thread Count.
> +  @param[in,out] DelayedCount  The Number of SMM Delayed Thread
> Count.
> +  @param[in,out] BlockedCount  The Number of SMM Blocked Thread
> Count.
> +  @param[in,out] DisabledCount The Number of SMM Disabled Thread
> Count.
> +**/
> +VOID
> +GetSmmDelayedBlockedDisabledCount (
> +  IN OUT UINT32  *DelayedCount,
> +  IN OUT UINT32  *BlockedCount,
> +  IN OUT UINT32  *DisabledCount
> +  );
> +
>  /**
>    Write unprotect read-only pages if Cr0.Bits.WP is 1.
> 
> --
> 2.39.1.windows.1



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