From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=66.187.233.73; helo=mx1.redhat.com; envelope-from=lersek@redhat.com; receiver=edk2-devel@lists.01.org Received: from mx1.redhat.com (mx3-rdu2.redhat.com [66.187.233.73]) (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 9462D203369D5 for ; Fri, 29 Jun 2018 05:14:38 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.rdu2.redhat.com [10.11.54.6]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id BA5BB7263E; Fri, 29 Jun 2018 12:14:37 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-43.rdu2.redhat.com [10.10.120.43]) by smtp.corp.redhat.com (Postfix) with ESMTP id AEC5921565E1; Fri, 29 Jun 2018 12:14:36 +0000 (UTC) To: Eric Dong , edk2-devel@lists.01.org Cc: Ruiyu Ni References: <20180629032047.6340-1-eric.dong@intel.com> From: Laszlo Ersek Message-ID: <2eac3f3f-972f-9844-6567-5503a0403a85@redhat.com> Date: Fri, 29 Jun 2018 14:14:35 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.7.0 MIME-Version: 1.0 In-Reply-To: <20180629032047.6340-1-eric.dong@intel.com> X-Scanned-By: MIMEDefang 2.78 on 10.11.54.6 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.2]); Fri, 29 Jun 2018 12:14:37 +0000 (UTC) X-Greylist: inspected by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.2]); Fri, 29 Jun 2018 12:14:37 +0000 (UTC) for IP:'10.11.54.6' DOMAIN:'int-mx06.intmail.prod.int.rdu2.redhat.com' HELO:'smtp.corp.redhat.com' FROM:'lersek@redhat.com' RCPT:'' Subject: Re: [Patch V2] UefiCpuPkg/MpInitLib: Remove redundant parameter. X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.26 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 29 Jun 2018 12:14:38 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Hi Eric, On 06/29/18 05:20, Eric Dong wrote: > Parameter StartCount duplicates with RunningCount. After this change, > RunningCount means the running AP count. > > V2 changes: Remove volatile for RunningCount. > > Done Test: > 1.PI SCT Test > 2.Boot OS / S3 > > Cc: Ruiyu Ni > Cc: Jeff Fan > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Eric Dong > --- > UefiCpuPkg/Library/MpInitLib/MpLib.c | 11 +++++------ > UefiCpuPkg/Library/MpInitLib/MpLib.h | 3 +-- > 2 files changed, 6 insertions(+), 8 deletions(-) > > diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c > index 3945771764..52c9679099 100644 > --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c > +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c > @@ -1400,7 +1400,7 @@ CheckAllAPs ( > // value of state after setting the it to CpuStateFinished, so BSP can safely make use of its value. > // > if (GetApState(CpuData) != CpuStateBusy) { > - CpuMpData->RunningCount ++; > + CpuMpData->RunningCount --; > CpuMpData->CpuData[ProcessorNumber].Waiting = FALSE; > > // > @@ -1425,7 +1425,7 @@ CheckAllAPs ( > // > // If all APs finish, return EFI_SUCCESS. > // > - if (CpuMpData->RunningCount == CpuMpData->StartCount) { > + if (CpuMpData->RunningCount == 0) { > return EFI_SUCCESS; > } > > @@ -1442,7 +1442,7 @@ CheckAllAPs ( > // > if (CpuMpData->FailedCpuList != NULL) { > *CpuMpData->FailedCpuList = > - AllocatePool ((CpuMpData->StartCount - CpuMpData->FinishedCount + 1) * sizeof (UINTN)); > + AllocatePool ((CpuMpData->RunningCount + 1) * sizeof (UINTN)); > ASSERT (*CpuMpData->FailedCpuList != NULL); > } > ListIndex = 0; > @@ -2121,7 +2121,7 @@ StartupAllAPsWorker ( > return EFI_NOT_STARTED; > } > > - CpuMpData->StartCount = 0; > + CpuMpData->RunningCount = 0; > for (ProcessorNumber = 0; ProcessorNumber < ProcessorCount; ProcessorNumber++) { > CpuData = &CpuMpData->CpuData[ProcessorNumber]; > CpuData->Waiting = FALSE; > @@ -2131,7 +2131,7 @@ StartupAllAPsWorker ( > // Mark this processor as responsible for current calling. > // > CpuData->Waiting = TRUE; > - CpuMpData->StartCount++; > + CpuMpData->RunningCount++; > } > } > } > @@ -2140,7 +2140,6 @@ StartupAllAPsWorker ( > CpuMpData->ProcArguments = ProcedureArgument; > CpuMpData->SingleThread = SingleThread; > CpuMpData->FinishedCount = 0; > - CpuMpData->RunningCount = 0; > CpuMpData->FailedCpuList = FailedCpuList; > CpuMpData->ExpectedTime = CalculateTimeout ( > TimeoutInMicroseconds, > diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.h b/UefiCpuPkg/Library/MpInitLib/MpLib.h > index 90c09fb8fb..ad62acf766 100644 > --- a/UefiCpuPkg/Library/MpInitLib/MpLib.h > +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.h > @@ -210,9 +210,8 @@ struct _CPU_MP_DATA { > UINTN BackupBuffer; > UINTN BackupBufferSize; > > - volatile UINT32 StartCount; > volatile UINT32 FinishedCount; > - volatile UINT32 RunningCount; > + UINT32 RunningCount; > BOOLEAN SingleThread; > EFI_AP_PROCEDURE Procedure; > VOID *ProcArguments; > I got confused by the way you sent out this patch. First I thought that you meant it separately (stand-alone). I tried to test it, but it didn't apply. Also my intent was to ask you whether you wanted to send a new version of [edk2] [Patch 1/2] UefiCpuPkg/MpInitLib: Not use disabled AP. However, upon seeing that this patch wouldn't apply, I'm now thinking that you would like to preserve [edk2] [Patch 1/2] UefiCpuPkg/MpInitLib: Not use disabled AP. without any changes, and your intent is to only update [edk2] [Patch 2/2] UefiCpuPkg/MpInitLib: Remove redundant parameter. to version 2. In such cases, please do not post an independent patch. Instead, pick one of the following: - Repost the entire series as v2, and mark in the cover letter that patch #1 is unchanged from the v1 posting. - Alternatively, post the patch in response to the *original* v1 thread (using --in-reply-to= with git-send-email), and use the subject [edk2] [Patch v2 2/2] UefiCpuPkg/MpInitLib: Remove redundant parameter. Either of these approaches will let reviewers know that you intend the two patches to go together (and in what order), and that only patch #2 has been updated. So... Assuming this was indeed your intent, I applied the following two patches locally, for testing: [edk2] [Patch 1_2] UefiCpuPkg_MpInitLib: Not use disabled AP. Message-Id: <20180628112920.5296-1-eric.dong@intel.com> http://mid.mail-archive.com/20180628112920.5296-1-eric.dong@intel.com [edk2] [Patch V2] UefiCpuPkg_MpInitLib: Remove redundant parameter. Message-Id: <20180629032047.6340-1-eric.dong@intel.com> http://mid.mail-archive.com/20180629032047.6340-1-eric.dong@intel.com (i.e., this patch) I tested the following three scenarios with QEMU/KVM, using normal boot and S3, and 8 virtual CPUs (1 socket, 4 cores, 2 threads -- same topology as my physical laptop CPU): (1) i440fx machine type, X64 build, without SMM (2) q35 machine type, IA32 build, with SMM (3) q35 machine type, IA32X64 build, with SMM The guest OS was Fedora in every case. The series keeps test case (1) functional. The series breaks test cases (2) and (3) however; normal boot for each (S3 is not possible to attempt). The symptom is that, for both cases (2) and (3), the ExitBootServices() handlers are invoked, but the following message is never logged: MpInitChangeApLoopCallback() done! and the boot process gets stuck. With more context, a before/after log file diff, for case (2): > VirtioScsiExitBoot: Context=0x7DF3F010 > SmmInstallProtocolInterface: [EdkiiSmmExitBootServicesProtocol] 0 > -MpInitChangeApLoopCallback() done! With more context, a before/after log file diff, for case (3): > VirtioScsiExitBoot: Context=0x7DBD4398 > VirtioRngExitBoot: Context=0x7DC47318 > SmmInstallProtocolInterface: [EdkiiSmmExitBootServicesProtocol] 0 > -MpInitChangeApLoopCallback() done! I guess the patch series breaks something in MpInitChangeApLoopCallback() when the SMM driver stack is included in the build. ... After repeated testing, the boot succeeds *sometimes* (rarely). In most cases, the boot fails as described above. Thanks, Laszlo