> > +struct SMM_CPU_SYNC_CTX {
>
> 1. How about "SMM_CPU_SYNC_CONTEXT"?
Agree.
>
> > + ///
> > + /// 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"?
I think it's not the big deal to combine into one structure or separate into
different structures.
Separating different attribute field into different structures will benefit the
code readability & maintainability & expansibility. It's easy to understand
which field is for GlobalSem, and which field is for CpuSem.
With separated structures, it will very easy coding the later logic for
semaphore memory allocation/free. Because you know we need 64 bytes alignment
for semaphore, in the later coding, Run[cpuIndex] can't meet requirement for
that since it's UINT32 type. That will bring a lot of inconvenience for coding
and very easy to make mistake. Besides, we only need allcoate/free one
continuous Sembuffer for all semaphores, and easy for consumer point to next
with CpuSem[CpuIndex].Run.
>
> > + ///
> > + /// 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.
>
>
Same as above explained.
>
> > + ///
> > + /// 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"?
>
Agree.
>
>
> > + ///
> > + /// Size of global and each CPU semaphores
> > + ///
> > + UINTN SemBlockPages;
>
> 5. How about "SemBufferSize"?
Agree, you want it to be bytes instead of pages?
>
> > +};
> > +
> > +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?
Agree. I just wanted it to be more readable: no empty lines means it's group
usage for some purpose, that was my original coding style, but I can change it
since you don't like it. :).
>
> > +
> > + 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.
>
This is comments from Laszlo:
Please use SafeIntLib for all calculations, to prevent overflows. This
means primarily (but not exclusively) memory size calculations, for
allocations
>From API perspective, it's possible since the NumberOfCpus defined as UNITN,
>right?
> > + //
> > + // 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".
>
Yes, we can discuss whether need SafeUintxxx(), for me, it's both ok.
SafeUintxxx doesn't do bad things.
>
> > +/**
> > + 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.
Assert can be added by caller for status if wanted. The API only follow the
definition, right? Or add assert before if check?
>
> > +
> > + Ctx = (SMM_CPU_SYNC_CTX *)SmmCpuSyncCtx;
>
> 11. Why do we need the type cast?
Oh, yes, we don't need that, original code defined the ctx as void, I forget
update this.
>
> > +
> > +/**
> > + 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.
>
Existing implementation logic or SMM_CPU_SYNC_CONTEXT definition doesn't
support the checkout or getArrivedcount after lock. I don't want implementation
support such kind of case, so added that. Or can we return unsupported status
if locked directly? then we can remove that words.
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112089): https://edk2.groups.io/g/devel/message/112089
Mute This Topic: https://groups.io/mt/102889293/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
