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 <ler...@redhat.com> > Cc: Eric Dong <eric.d...@intel.com> > Cc: Ray Ni <ray...@intel.com> > Cc: Zeng Star <star.z...@intel.com> > Cc: Gerd Hoffmann <kra...@redhat.com> > Cc: Rahul Kumar <rahul1.ku...@intel.com> > Signed-off-by: Jiaxin Wu <jiaxin...@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/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/leave/9847357/21656/1706620634/xyzzy [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
