> + ///
> + /// Indicate CPUs entered SMM after lock door.
> + ///
> + UINTN LockedCpuCount;
1. It's not "LockedCpuCount". It's "ArrivedCpuCountUponLock".
Comments can be:
Before the door is locked, CpuCount stores the arrived CPU count.
After the door is locked, CpuCount is set to -1 indicating the door is
locked. ArrivedCpuCpuntUponLock stores the arrived CPU count then.
> +/**
> + Performs an atomic compare exchange operation to get semaphore.
> + The compare exchange operation must be performed using MP safe
> + mechanisms.
> +
> + @param[in,out] Sem IN: 32-bit unsigned integer
> + OUT: original integer - 1 if Sem is not locked.
> + OUT: original integer if Sem is locked
> (MAX_UINT32).
> +
> + @retval Original integer - 1 if Sem is not locked.
> + Original integer if Sem is locked (MAX_UINT32).
2. Can just say "MAX_UINT32 if Sem is locked".
> + //
> + // Calculate total semaphore size
> + //
> + CacheLineSize = GetSpinLockProperties ();
> + OneSemSize = ALIGN_VALUE (sizeof (SMM_CPU_SYNC_SEMAPHORE),
> CacheLineSize);
3. I prefer:
OneSemSize = GetSpinLockProperties ();
ASSERT (sizeof (SMM_CPU_SYNC_SEMAPHORE) <= OneSemSize);
> +
> + Status = SafeUintnAdd (1, NumberOfCpus, &NumSem);
4. ok:) you are checking if NumberOfCpus + 1 could exceed MAX_UINTN. Fine to me.
> +
> + //
> + // Assign CPU Semaphore pointer
> + //
> + CpuSem = (*Context)->CpuSem;
> + for (CpuIndex = 0; CpuIndex < NumberOfCpus; CpuIndex++) {
> + CpuSem->Run = (SMM_CPU_SYNC_SEMAPHORE *)SemAddr;
> + *CpuSem->Run = 0;
> +
> + CpuSem++;
> + SemAddr += OneSemSize;
5. SafeIntLib was used earlier to make sure no integer overflow.
But "SemAddr += OneSemSize" is simply ignoring the danger of integer overflow.
I agree (NumberOfCpus + 1) * OneSemSize shouldn't cause integer overflow when
code runs to here.
But initial value of SemAddr is not zero. It's still possible the SemAddr +
(NumberOfCpus+1)*OneSemSize causes integer overflow.
I am ok if you don't fix it as I don't believe the integer overflow could
happen in 5 years.
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112638): https://edk2.groups.io/g/devel/message/112638
Mute This Topic: https://groups.io/mt/103187894/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe:
https://edk2.groups.io/g/devel/leave/9847357/21656/1706620634/xyzzy
[arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
