From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=209.132.183.28; helo=mx1.redhat.com; envelope-from=lersek@redhat.com; receiver=edk2-devel@lists.01.org Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id B398A21A07A92 for ; Thu, 8 Nov 2018 05:33:43 -0800 (PST) Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 204AB3082A23; Thu, 8 Nov 2018 13:33:43 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-121-147.rdu2.redhat.com [10.10.121.147]) by smtp.corp.redhat.com (Postfix) with ESMTP id CBB5260C60; Thu, 8 Nov 2018 13:33:41 +0000 (UTC) To: Eric Dong , edk2-devel@lists.01.org Cc: Ruiyu Ni References: <20181108025800.12112-1-eric.dong@intel.com> <20181108025800.12112-3-eric.dong@intel.com> From: Laszlo Ersek Message-ID: <2ba7ee0f-b3e9-a2a4-28f5-7317cdf9fa5b@redhat.com> Date: Thu, 8 Nov 2018 14:33:40 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <20181108025800.12112-3-eric.dong@intel.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.12 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.45]); Thu, 08 Nov 2018 13:33:43 +0000 (UTC) Subject: Re: [Patch 2/2] UefiCpuPkg/PiSmmCpuDxeSmm: Separate semaphore container. X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 08 Nov 2018 13:33:44 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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 > Cc: Ruiyu Ni > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Eric Dong > --- > 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 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.) Thanks! Laszlo