Re: [Patch 2/2] UefiCpuPkg/PiSmmCpuDxeSmm: Separate semaphore container.

[Laszlo Ersek <lersek@redhat.com>]

On 11/08/18 14:33, Laszlo Ersek wrote:
> On 11/08/18 03:58, Eric Dong wrote:
>> In current implementation, core level semaphore use same container
>> with package level semaphore. This design will let the core level
>> semaphore not works as expected in below case:
>> 1. Feature A has CPU_FEATURE_CORE_BEFORE dependence with Feature B.
>> 2. Feature C has CPU_FEATURE_PACKAGE_AFTER dependence with Feature B.
>> in this case an core level semaphore will be add between A and B, and
>> an package level semaphore will be add between B and C.
>>
>> For a CPU has one package, two cores and 4 threads. Execute like below:
>>
>>   Thread 1          Thread 2    .....     Thread 4
>> ReleaseSemaph(1,2)  -|
>> WaitForSemaph(1(2)) -|<-----------------------These two are Core Semaph
>>                   ReleaseSemaph(1,2) -|
>>                   WaitForSemaph(2)   -| <---  Core Semaph
>>
>> ReleaseSemaph (1,2,3,4) -|
>> WaitForSemaph (1(4))    -| <----------------  Package Semaph
>>
>>                                       ReleaseSemaph(3,4)
>>                                       WaitForSemaph(4(2)) <- Core Semaph
>>
>> In above case, for thread 4, when it executes a core semaphore, i will
>> found WaitForSemaph(4(2)) is met because Thread 1 has execute a package
>> semaphore and ReleaseSemaph(4) for it before. This is not an expect
>> behavior. Thread 4 should wait for thread 3 to do this.
>>
>> Fix this issue by separate the semaphore container for core level and
>> package level.
>>
>> Cc: Laszlo Ersek <lersek@redhat.com>
>> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Eric Dong <eric.dong@intel.com>
>> ---
>>  UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c | 21 ++++++++++++++-------
>>  1 file changed, 14 insertions(+), 7 deletions(-)
>>
>> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
>> index a45e2dd3d7..65461485a4 100644
>> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
>> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
>> @@ -41,9 +41,10 @@ typedef struct {
>>  // Flags used when program the register.
>>  //
>>  typedef struct {
>> -  volatile UINTN           ConsoleLogLock;       // Spinlock used to control console.
>> -  volatile UINTN           MemoryMappedLock;     // Spinlock used to program mmio
>> -  volatile UINT32          *SemaphoreCount;      // Semaphore used to program semaphore.
>> +  volatile UINTN           ConsoleLogLock;          // Spinlock used to control console.
>> +  volatile UINTN           MemoryMappedLock;        // Spinlock used to program mmio
>> +  volatile UINT32          *CoreSemaphoreCount;     // Semaphore used to program semaphore.
>> +  volatile UINT32          *PackageSemaphoreCount;  // Semaphore used to program semaphore.
>>  } PROGRAM_CPU_REGISTER_FLAGS;
>>  
>>  //
>> @@ -348,11 +349,12 @@ ProgramProcessorRegister (
>>        ASSERT (
>>          (ApLocation != NULL) &&
>>          (CpuStatus->ValidCoreCountPerPackage != 0) &&
>> -        (CpuFlags->SemaphoreCount) != NULL
>> +        (CpuFlags->CoreSemaphoreCount != NULL) &&
>> +        (CpuFlags->PackageSemaphoreCount != NULL)
>>          );
>> -      SemaphorePtr = CpuFlags->SemaphoreCount;
>>        switch (RegisterTableEntry->Value) {
>>        case CoreDepType:
>> +        SemaphorePtr = CpuFlags->CoreSemaphoreCount;
>>          //
>>          // Get Offset info for the first thread in the core which current thread belongs to.
>>          //
>> @@ -373,6 +375,7 @@ ProgramProcessorRegister (
>>          break;
>>  
>>        case PackageDepType:
>> +        SemaphorePtr = CpuFlags->PackageSemaphoreCount;
>>          ValidCoreCountPerPackage = (UINT32 *)(UINTN)CpuStatus->ValidCoreCountPerPackage;
>>          //
>>          // Get Offset info for the first thread in the package which current thread belongs to.
>> @@ -1037,10 +1040,14 @@ GetAcpiCpuData (
>>      ASSERT (mAcpiCpuData.ApLocation != 0);
>>    }
>>    if (CpuStatus->PackageCount != 0) {
>> -    mCpuFlags.SemaphoreCount = AllocateZeroPool (
>> +    mCpuFlags.CoreSemaphoreCount = AllocateZeroPool (
>>                                   sizeof (UINT32) * CpuStatus->PackageCount *
>>                                   CpuStatus->MaxCoreCount * CpuStatus->MaxThreadCount);
>> -    ASSERT (mCpuFlags.SemaphoreCount != NULL);
>> +    ASSERT (mCpuFlags.CoreSemaphoreCount != NULL);
>> +    mCpuFlags.PackageSemaphoreCount = AllocateZeroPool (
>> +                                 sizeof (UINT32) * CpuStatus->PackageCount *
>> +                                 CpuStatus->MaxCoreCount * CpuStatus->MaxThreadCount);
>> +    ASSERT (mCpuFlags.PackageSemaphoreCount != NULL);
>>    }
>>    InitializeSpinLock((SPIN_LOCK*) &mCpuFlags.MemoryMappedLock);
>>    InitializeSpinLock((SPIN_LOCK*) &mCpuFlags.ConsoleLogLock);
>>
> 
> The patch looks OK, superficially speaking.
> 
> Also this looks like a bugfix to a new feature already committed in this
> development cycle, so I think it may go in during the hard feature freeze.
> 
> I have some requests (no need to repost):
> 
> (1) Please make sure there is a TianoCore BZ for this issue.
> 
> (2) Please reference said BZ in the commit message.
> 
> (For example, commit c60d36b4d1, for
> <https://bugzilla.tianocore.org/show_bug.cgi?id=1307> missed the
> reference to BZ#1307.)
> 
> (3) Before pushing, please fix up the indentation of the
> AllocateZeroPool() arguments (both calls).
> 
> (4) Can you please file the bugzilla now about unifying the
> implementation between RegisterCpuFeaturesLib and PiSmmCpuDxeSmm?
> 
> (That's for the next development cycle, but we should report the BZ for
> it at this point, I believe.)

Oh, sorry, I thought I had added my ACK, but it turns out I forgot about
it. So, with the above points,

Acked-by: Laszlo Ersek <lersek@redhat.com>

Thanks
Laszlo