Re: [edk2-devel] [PATCH v2 3/6] UefiCpuPkg: Implements SmmCpuSyncLib library instance

["Ni, Ray" <ray.ni@intel.com>]

> +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/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-