Re: [edk2-devel] [PATCH v3 2/6] UefiCpuPkg: Adds SmmCpuSyncLib library class

["Laszlo Ersek" <lersek@redhat.com>]

On 12/6/23 11:01, Wu, Jiaxin wrote:
> Intel is planning to provide different SMM CPU Sync implementation
> along with some specific registers to improve the SMI performance,
> hence need SmmCpuSyncLib Library for Intel.
> 
> This patch is to:
> 1.Adds SmmCpuSyncLib Library class in UefiCpuPkg.dec.
> 2.Adds SmmCpuSyncLib.h function declaration header file.
> 
> For the new SmmCpuSyncLib, it provides 3 sets of APIs:
> 
> 1. ContextInit/ContextDeinit/ContextReset:
> ContextInit() is called in driver's entrypoint to allocate and
> initialize the SMM CPU Sync context. ContextDeinit() is called in
> driver's unload function to deinitialize SMM CPU Sync context.
> ContextReset() is called before CPU exist SMI, which allows CPU to
> check into the next SMI from this point.
> 
> 2. GetArrivedCpuCount/CheckInCpu/CheckOutCpu/LockDoor:
> When SMI happens, all processors including BSP enter to SMM mode by
> calling CheckInCpu(). The elected BSP calls LockDoor() so that
> CheckInCpu() will return the error code after that. CheckOutCpu() can
> be called in error handling flow for the CPU who calls CheckInCpu()
> earlier. GetArrivedCpuCount() returns the number of checked-in CPUs.
> 
> 3. WaitForAPs/ReleaseOneAp/WaitForBsp/ReleaseBsp
> WaitForAPs() & ReleaseOneAp() are called from BSP to wait the number
> of APs and release one specific AP. WaitForBsp() & ReleaseBsp() are
> called from APs to wait and release BSP. The 4 APIs are used to
> synchronize the running flow among BSP and APs. BSP and AP Sync flow
> can be easy understand as below:
> BSP: ReleaseOneAp  -->  AP: WaitForBsp
> BSP: WaitForAPs    <--  AP: ReleaseBsp
> 
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Zeng Star <star.zeng@intel.com>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Rahul Kumar <rahul1.kumar@intel.com>
> Signed-off-by: Jiaxin Wu <jiaxin.wu@intel.com>
> ---
>  UefiCpuPkg/Include/Library/SmmCpuSyncLib.h | 275 +++++++++++++++++++++++++++++
>  UefiCpuPkg/UefiCpuPkg.dec                  |   3 +
>  2 files changed, 278 insertions(+)
>  create mode 100644 UefiCpuPkg/Include/Library/SmmCpuSyncLib.h
> 
> diff --git a/UefiCpuPkg/Include/Library/SmmCpuSyncLib.h b/UefiCpuPkg/Include/Library/SmmCpuSyncLib.h
> new file mode 100644
> index 0000000000..0f9eb3414a
> --- /dev/null
> +++ b/UefiCpuPkg/Include/Library/SmmCpuSyncLib.h
> @@ -0,0 +1,275 @@
> +/** @file
> +  Library that provides SMM CPU Sync related operations.
> +  The lib provides 3 sets of APIs:
> +  1. ContextInit/ContextDeinit/ContextReset:
> +  ContextInit() is called in driver's entrypoint to allocate and initialize the SMM CPU Sync context.
> +  ContextDeinit() is called in driver's unload function to deinitialize the SMM CPU Sync context.
> +  ContextReset() is called before CPU exist SMI, which allows CPU to check into the next SMI from this point.
> +
> +  2. GetArrivedCpuCount/CheckInCpu/CheckOutCpu/LockDoor:
> +  When SMI happens, all processors including BSP enter to SMM mode by calling CheckInCpu().
> +  The elected BSP calls LockDoor() so that CheckInCpu() will return the error code after that.
> +  CheckOutCpu() can be called in error handling flow for the CPU who calls CheckInCpu() earlier.
> +  GetArrivedCpuCount() returns the number of checked-in CPUs.
> +
> +  3. WaitForAPs/ReleaseOneAp/WaitForBsp/ReleaseBsp
> +  WaitForAPs() & ReleaseOneAp() are called from BSP to wait the number of APs and release one specific AP.
> +  WaitForBsp() & ReleaseBsp() are called from APs to wait and release BSP.
> +  The 4 APIs are used to synchronize the running flow among BSP and APs. BSP and AP Sync flow can be
> +  easy understand as below:
> +  BSP: ReleaseOneAp  -->  AP: WaitForBsp
> +  BSP: WaitForAPs    <--  AP: ReleaseBsp
> +
> +  Copyright (c) 2023, Intel Corporation. All rights reserved.<BR>
> +  SPDX-License-Identifier: BSD-2-Clause-Patent
> +
> +**/

Thanks. This documentation (in the commit message and the lib class
header file) seems really good (especially with the formatting updates
suggested by Ray).

(1) I think there is one typo: exist <-> exits.

> +
> +#ifndef SMM_CPU_SYNC_LIB_H_
> +#define SMM_CPU_SYNC_LIB_H_
> +
> +#include <Uefi/UefiBaseType.h>
> +
> +//
> +// Opaque structure for SMM CPU Sync context.
> +//
> +typedef struct SMM_CPU_SYNC_CONTEXT SMM_CPU_SYNC_CONTEXT;
> +
> +/**
> +  Create and initialize the SMM CPU Sync context.
> +
> +  SmmCpuSyncContextInit() function is to allocate and initialize the SMM CPU Sync context.
> +
> +  @param[in]  NumberOfCpus          The number of Logical Processors in the system.
> +  @param[out] SmmCpuSyncCtx         Pointer to the new created and initialized SMM CPU Sync context object.
> +                                    NULL will be returned if any error happen during init.
> +
> +  @retval RETURN_SUCCESS            The SMM CPU Sync context was successful created and initialized.
> +  @retval RETURN_INVALID_PARAMETER  SmmCpuSyncCtx is NULL.
> +  @retval RETURN_BUFFER_TOO_SMALL   Overflow happen
> +  @retval RETURN_OUT_OF_RESOURCES   There are not enough resources available to create and initialize SMM CPU Sync context.
> +
> +**/
> +RETURN_STATUS
> +EFIAPI
> +SmmCpuSyncContextInit (
> +  IN   UINTN                 NumberOfCpus,
> +  OUT  SMM_CPU_SYNC_CONTEXT  **SmmCpuSyncCtx
> +  );
> +
> +/**
> +  Deinit an allocated SMM CPU Sync context.
> +
> +  SmmCpuSyncContextDeinit() function is to deinitialize SMM CPU Sync context, the resources allocated in
> +  SmmCpuSyncContextInit() will be freed.
> +
> +  Note: This function only can be called after SmmCpuSyncContextInit() return success.
> +
> +  @param[in,out]  SmmCpuSyncCtx     Pointer to the SMM CPU Sync context object to be deinitialized.
> +
> +  @retval RETURN_SUCCESS            The SMM CPU Sync context was successful deinitialized.
> +  @retval RETURN_INVALID_PARAMETER  SmmCpuSyncCtx is NULL.
> +  @retval RETURN_UNSUPPORTED        Unsupported operation.
> +
> +**/
> +RETURN_STATUS
> +EFIAPI
> +SmmCpuSyncContextDeinit (
> +  IN OUT SMM_CPU_SYNC_CONTEXT  *SmmCpuSyncCtx
> +  );
> +
> +/**
> +  Reset SMM CPU Sync context.
> +
> +  SmmCpuSyncContextReset() function is to reset SMM CPU Sync context to the initialized state.
> +
> +  @param[in,out]  SmmCpuSyncCtx     Pointer to the SMM CPU Sync context object to be reset.
> +
> +  @retval RETURN_SUCCESS            The SMM CPU Sync context was successful reset.
> +  @retval RETURN_INVALID_PARAMETER  SmmCpuSyncCtx is NULL.
> +
> +**/
> +RETURN_STATUS
> +EFIAPI
> +SmmCpuSyncContextReset (
> +  IN OUT SMM_CPU_SYNC_CONTEXT  *SmmCpuSyncCtx
> +  );

(2) The file-top documentation says about this API: "ContextReset() is
called before CPU exist SMI, which allows CPU to check into the next SMI
from this point".

It is not clear *which* CPU is supposed to call ContextReset (and the
function does not take a CPU index). Can you explain this in the
documentation? (Assuming my observation makes sense.)

> +
> +/**
> +  Get current number of arrived CPU in SMI.
> +
> +  For traditional CPU synchronization method, BSP might need to know the current number of arrived CPU in
> +  SMI to make sure all APs in SMI. This API can be for that purpose.
> +
> +  @param[in]      SmmCpuSyncCtx     Pointer to the SMM CPU Sync context object.
> +  @param[in,out]  CpuCount          Current count of arrived CPU in SMI.
> +
> +  @retval RETURN_SUCCESS            Get current number of arrived CPU in SMI successfully.
> +  @retval RETURN_INVALID_PARAMETER  SmmCpuSyncCtx or CpuCount is NULL.
> +  @retval RETURN_UNSUPPORTED        Unsupported operation.
> +
> +**/
> +RETURN_STATUS
> +EFIAPI
> +SmmCpuSyncGetArrivedCpuCount (
> +  IN     SMM_CPU_SYNC_CONTEXT  *SmmCpuSyncCtx,
> +  IN OUT UINTN                 *CpuCount
> +  );

(3) Why is CpuCount IN OUT? I would think just OUT should suffice.


> +
> +/**
> +  Performs an atomic operation to check in CPU.
> +
> +  When SMI happens, all processors including BSP enter to SMM mode by calling SmmCpuSyncCheckInCpu().
> +
> +  @param[in,out]  SmmCpuSyncCtx     Pointer to the SMM CPU Sync context object.
> +  @param[in]      CpuIndex          Check in CPU index.
> +
> +  @retval RETURN_SUCCESS            Check in CPU (CpuIndex) successfully.
> +  @retval RETURN_INVALID_PARAMETER  SmmCpuSyncCtx is NULL.
> +  @retval RETURN_ABORTED            Check in CPU failed due to SmmCpuSyncLockDoor() has been called by one elected CPU.
> +
> +**/
> +RETURN_STATUS
> +EFIAPI
> +SmmCpuSyncCheckInCpu (
> +  IN OUT SMM_CPU_SYNC_CONTEXT  *SmmCpuSyncCtx,
> +  IN     UINTN                 CpuIndex
> +  );

(4) Do we need an error condition for CpuIndex being out of range?

(5) Do we have a special CpuIndex value for the BSP?

> +
> +/**
> +  Performs an atomic operation to check out CPU.
> +
> +  CheckOutCpu() can be called in error handling flow for the CPU who calls CheckInCpu() earlier.
> +
> +  @param[in,out]  SmmCpuSyncCtx     Pointer to the SMM CPU Sync context object.
> +  @param[in]      CpuIndex          Check out CPU index.
> +
> +  @retval RETURN_SUCCESS            Check out CPU (CpuIndex) successfully.
> +  @retval RETURN_INVALID_PARAMETER  SmmCpuSyncCtx is NULL.
> +  @retval RETURN_NOT_READY          The CPU is not checked-in.
> +  @retval RETURN_UNSUPPORTED        Unsupported operation.
> +
> +**/
> +RETURN_STATUS
> +EFIAPI
> +SmmCpuSyncCheckOutCpu (
> +  IN OUT SMM_CPU_SYNC_CONTEXT  *SmmCpuSyncCtx,
> +  IN     UINTN                 CpuIndex
> +  );
> +

(6) Looks good, my only question is again if we need a status code for
CpuIndex being out of range.

> +/**
> +  Performs an atomic operation lock door for CPU checkin or checkout.
> +
> +  After this function, CPU can not check in via SmmCpuSyncCheckInCpu().
> +
> +  The CPU specified by CpuIndex is elected to lock door.
> +
> +  @param[in,out]  SmmCpuSyncCtx     Pointer to the SMM CPU Sync context object.
> +  @param[in]      CpuIndex          Indicate which CPU to lock door.
> +  @param[in,out]  CpuCount          Number of arrived CPU in SMI after look door.
> +
> +  @retval RETURN_SUCCESS            Lock door for CPU successfully.
> +  @retval RETURN_INVALID_PARAMETER  SmmCpuSyncCtx or CpuCount is NULL.
> +
> +**/
> +RETURN_STATUS
> +EFIAPI
> +SmmCpuSyncLockDoor (
> +  IN OUT SMM_CPU_SYNC_CONTEXT  *SmmCpuSyncCtx,
> +  IN     UINTN                 CpuIndex,
> +  IN OUT UINTN                 *CpuCount
> +  );

This is where it's getting tricky :)

(7) error condition for CpuIndex being out of range?

(8) why is CpuCount IN OUT and not just OUT? (Other than that, I can see
how outputting CpuCout at once can be useful.)

(9) do we need error conditions for:

(9.1) CpuIndex being "wrong" (i.e., not the CPU that's "supposed" to
lock the door)?

(9.2) CpuIndex not having checked in already, before trying to lock the
door?

Now, I can imagine that problem (9.1) is undetectable, i.e., it causes
undefined behavior. That's fine, but then we should mention that.

> +
> +/**
> +  Used by the BSP to wait for APs.
> +
> +  The number of APs need to be waited is specified by NumberOfAPs. The BSP is specified by BspIndex.
> +
> +  Note: This function is blocking mode, and it will return only after the number of APs released by
> +  calling SmmCpuSyncReleaseBsp():
> +  BSP: WaitForAPs    <--  AP: ReleaseBsp
> +
> +  @param[in,out]  SmmCpuSyncCtx     Pointer to the SMM CPU Sync context object.
> +  @param[in]      NumberOfAPs       Number of APs need to be waited by BSP.
> +  @param[in]      BspIndex          The BSP Index to wait for APs.
> +
> +  @retval RETURN_SUCCESS            BSP to wait for APs successfully.
> +  @retval RETURN_INVALID_PARAMETER  SmmCpuSyncCtx is NULL or NumberOfAPs > total number of processors in system.
> +
> +**/
> +RETURN_STATUS
> +EFIAPI
> +SmmCpuSyncWaitForAPs (
> +  IN OUT SMM_CPU_SYNC_CONTEXT  *SmmCpuSyncCtx,
> +  IN     UINTN                 NumberOfAPs,
> +  IN     UINTN                 BspIndex
> +  );

The "NumberOfAPs > total number of processors in system" check is nice!

(10) Again, do we need a similar error condition for BspIndex being out
of range?

(11) Do we need to document / enforce explicitly (status code) that the
BSP and the APs must have checked in, and/or the door must have been
locked? Again -- if we can't detect / enforce these conditions, that's
fine, but then we should mention the expected call environment. The
file-top description does not seem very explicit about it.

> +
> +/**
> +  Used by the BSP to release one AP.
> +
> +  The AP is specified by CpuIndex. The BSP is specified by BspIndex.
> +
> +  @param[in,out]  SmmCpuSyncCtx     Pointer to the SMM CPU Sync context object.
> +  @param[in]      CpuIndex          Indicate which AP need to be released.
> +  @param[in]      BspIndex          The BSP Index to release AP.
> +
> +  @retval RETURN_SUCCESS            BSP to release one AP successfully.
> +  @retval RETURN_INVALID_PARAMETER  SmmCpuSyncCtx is NULL or CpuIndex is same as BspIndex.
> +
> +**/
> +RETURN_STATUS
> +EFIAPI
> +SmmCpuSyncReleaseOneAp   (
> +  IN OUT SMM_CPU_SYNC_CONTEXT  *SmmCpuSyncCtx,
> +  IN     UINTN                 CpuIndex,
> +  IN     UINTN                 BspIndex
> +  );

(12) Same comments as elsewhere:

- it's good that we check CpuIndex versus BspIndex, but do we also need
to range-check each?

- document that both affected CPUs need to have checked in, with the
door potentially locked?


> +
> +/**
> +  Used by the AP to wait BSP.
> +
> +  The AP is specified by CpuIndex. The BSP is specified by BspIndex.
> +
> +  Note: This function is blocking mode, and it will return only after the AP released by
> +  calling SmmCpuSyncReleaseOneAp():
> +  BSP: ReleaseOneAp  -->  AP: WaitForBsp
> +
> +  @param[in,out]  SmmCpuSyncCtx    Pointer to the SMM CPU Sync context object.
> +  @param[in]      CpuIndex         Indicate which AP wait BSP.
> +  @param[in]      BspIndex         The BSP Index to be waited.
> +
> +  @retval RETURN_SUCCESS            AP to wait BSP successfully.
> +  @retval RETURN_INVALID_PARAMETER  SmmCpuSyncCtx is NULL or CpuIndex is same as BspIndex.
> +
> +**/
> +RETURN_STATUS
> +EFIAPI
> +SmmCpuSyncWaitForBsp (
> +  IN OUT SMM_CPU_SYNC_CONTEXT  *SmmCpuSyncCtx,
> +  IN     UINTN                 CpuIndex,
> +  IN     UINTN                 BspIndex
> +  );
> +

(13) Same questions as under (12).

> +/**
> +  Used by the AP to release BSP.
> +
> +  The AP is specified by CpuIndex. The BSP is specified by BspIndex.
> +
> +  @param[in,out]  SmmCpuSyncCtx     Pointer to the SMM CPU Sync context object.
> +  @param[in]      CpuIndex          Indicate which AP release BSP.
> +  @param[in]      BspIndex          The BSP Index to be released.
> +
> +  @retval RETURN_SUCCESS            AP to release BSP successfully.
> +  @retval RETURN_INVALID_PARAMETER  SmmCpuSyncCtx is NULL or CpuIndex is same as BspIndex.
> +
> +**/
> +RETURN_STATUS
> +EFIAPI
> +SmmCpuSyncReleaseBsp (
> +  IN OUT SMM_CPU_SYNC_CONTEXT  *SmmCpuSyncCtx,
> +  IN     UINTN                 CpuIndex,
> +  IN     UINTN                 BspIndex
> +  );

(14) Same questions as under (12).

> +
> +#endif
> diff --git a/UefiCpuPkg/UefiCpuPkg.dec b/UefiCpuPkg/UefiCpuPkg.dec
> index 0b5431dbf7..20ab079219 100644
> --- a/UefiCpuPkg/UefiCpuPkg.dec
> +++ b/UefiCpuPkg/UefiCpuPkg.dec
> @@ -62,10 +62,13 @@
>    CpuPageTableLib|Include/Library/CpuPageTableLib.h
>  
>    ## @libraryclass   Provides functions for manipulating smram savestate registers.
>    MmSaveStateLib|Include/Library/MmSaveStateLib.h
>  
> +  ## @libraryclass   Provides functions for SMM CPU Sync Operation.
> +  SmmCpuSyncLib|Include/Library/SmmCpuSyncLib.h
> +
>  [LibraryClasses.RISCV64]
>    ##  @libraryclass  Provides functions to manage MMU features on RISCV64 CPUs.
>    ##
>    RiscVMmuLib|Include/Library/BaseRiscVMmuLib.h
>  

These interfaces look real nice, my comments/questions are all docs-related.

Thanks!
Laszlo



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