> +typedef struct {
> + ///
> + /// Indicate how many CPU entered SMM.
> + ///
> + volatile UINT32 *Counter;
> +} SMM_CPU_SYNC_SEMAPHORE_GLOBAL;
> +
> +typedef struct {
> + ///
> + /// Used for control each CPU continue run or wait for signal
> + ///
> + volatile UINT32 *Run;
> +} SMM_CPU_SYNC_SEMAPHORE_CPU;
> +
> +struct SMM_CPU_SYNC_CTX {
1. How about "SMM_CPU_SYNC_CONTEXT"?
> + ///
> + /// All global semaphores' pointer in SMM CPU Sync
> + ///
> + SMM_CPU_SYNC_SEMAPHORE_GLOBAL *GlobalSem;
2. There is only one GlobalSem. Can you directly use "volatile UINT32 *Counter"
instead of "GlobalSem"?
> + ///
> + /// All semaphores for each processor in SMM CPU Sync
> + ///
> + SMM_CPU_SYNC_SEMAPHORE_CPU *CpuSem;
3. Can we use "volatile UINT32 **Run" instead of pointing to another structure?
Run points to an array where each element is a UINT32 *. Count of array equals
to NumberOfCpus.
> + ///
> + /// The number of processors in the system.
> + /// This does not indicate the number of processors that entered SMM.
> + ///
> + UINTN NumberOfCpus;
> + ///
> + /// Address of global and each CPU semaphores
> + ///
> + UINTN *SemBlock;
4. How about "SemBuffer"?
> + ///
> + /// Size of global and each CPU semaphores
> + ///
> + UINTN SemBlockPages;
5. How about "SemBufferSize"?
> +};
> +
> +EFIAPI
> +SmmCpuSyncContextInit (
> + IN UINTN NumberOfCpus,
> + OUT SMM_CPU_SYNC_CTX **SmmCpuSyncCtx
> + )
> +{
> + RETURN_STATUS Status;
> +
> + UINTN CpuSemInCtxSize;
> + UINTN CtxSize;
> +
> + UINTN OneSemSize;
> + UINTN GlobalSemSize;
> + UINTN OneCpuSemSize;
> + UINTN CpuSemSize;
> + UINTN TotalSemSize;
> +
> + UINTN SemAddr;
> + UINTN CpuIndex;
6. Can you remove the empty lines among the local variable declarations?
> +
> + if (SmmCpuSyncCtx == NULL) {
> + return RETURN_INVALID_PARAMETER;
> + }
> +
> + //
> + // Count the CtxSize
> + //
> + Status = SafeUintnMult (NumberOfCpus, sizeof
> (SMM_CPU_SYNC_SEMAPHORE_CPU), &CpuSemInCtxSize);
7. Is there a reason to use SafeUintxxx()? I don't believe the multiplication
could exceed MAX_UINTN.
But using SafeUintxxx() makes code hard to read.
> + //
> + // Allocate CtxSize buffer for the *SmmCpuSyncCtx
> + //
> + *SmmCpuSyncCtx = NULL;
8. This is not needed.
> + Status = SafeUintnMult (OneSemSize, sizeof
> (SMM_CPU_SYNC_SEMAPHORE_GLOBAL) / sizeof (VOID *), &GlobalSemSize);
> + if (EFI_ERROR (Status)) {
> + goto ON_ERROR;
9. If you don't use SafeUintxxx(), you don't need "ON_ERROR" label and the
"goto".
> +/**
> + 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_CTX *SmmCpuSyncCtx,
> + IN UINTN CpuIndex
> + )
> +{
> + SMM_CPU_SYNC_CTX *Ctx;
> +
> + if (SmmCpuSyncCtx == NULL) {
> + return RETURN_INVALID_PARAMETER;
> + }
10. Can we use "ASSERT" instead of if-check? We can explicitly mention the
behavior in API comments.
> +
> + Ctx = (SMM_CPU_SYNC_CTX *)SmmCpuSyncCtx;
11. Why do we need the type cast?
> +
> +/**
> + Performs an atomic operation lock door for CPU checkin or checkout.
> +
> + After this function:
> + CPU can not check in via SmmCpuSyncCheckInCpu().
> + CPU can not check out via SmmCpuSyncCheckOutCpu().
> + CPU can not get number of arrived CPU in SMI via
> SmmCpuSyncGetArrivedCpuCount(). The number of
> + arrived CPU in SMI will be returned in CpuCount.
12. I agree that after locking, cpu cannot "checkin".
But can cpu "checkout" or "getArrivedcount"? I need to double check.
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112087): https://edk2.groups.io/g/devel/message/112087
Mute This Topic: https://groups.io/mt/102889293/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe:
https://edk2.groups.io/g/devel/leave/9847357/21656/1706620634/xyzzy
[arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
