Re: [edk2-devel] [PATCH v4 4/8] UefiCpuPkg: Implements SmmCpuSyncLib library instance

["Wu, Jiaxin" <jiaxin.wu@intel.com>]

Thanks Ray.

> 
> 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.
> 
> 

Ok, I will update.

> > +/**
> > +  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".
> 
> 

Agree.

> > +
> > +  //
> > +  // 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.

SemAddr is the address of allocated buffer (SemBuffer),     "SemAddr += OneSemSize" will make SemAddr point to the part of allocated buffer. is it possible integer overflow? If so, the allocatepage should fail?


Thanks,
Jiaxin  



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112690): https://edk2.groups.io/g/devel/message/112690
Mute This Topic: https://groups.io/mt/103187894/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-