From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=192.55.52.93; helo=mga11.intel.com; envelope-from=eric.dong@intel.com; receiver=edk2-devel@lists.01.org Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) (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 3D6942096FAD0 for ; Wed, 18 Jul 2018 06:00:21 -0700 (PDT) X-Amp-Result: UNKNOWN X-Amp-Original-Verdict: FILE UNKNOWN X-Amp-File-Uploaded: False Received: from fmsmga004.fm.intel.com ([10.253.24.48]) by fmsmga102.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 18 Jul 2018 06:00:17 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.51,370,1526367600"; d="fd'?log'?scan'208";a="72371993" Received: from fmsmsx103.amr.corp.intel.com ([10.18.124.201]) by fmsmga004.fm.intel.com with ESMTP; 18 Jul 2018 05:59:39 -0700 Received: from fmsmsx116.amr.corp.intel.com (10.18.116.20) by FMSMSX103.amr.corp.intel.com (10.18.124.201) with Microsoft SMTP Server (TLS) id 14.3.319.2; Wed, 18 Jul 2018 05:59:39 -0700 Received: from shsmsx104.ccr.corp.intel.com (10.239.4.70) by fmsmsx116.amr.corp.intel.com (10.18.116.20) with Microsoft SMTP Server (TLS) id 14.3.319.2; Wed, 18 Jul 2018 05:59:10 -0700 Received: from shsmsx102.ccr.corp.intel.com ([169.254.2.124]) by SHSMSX104.ccr.corp.intel.com ([169.254.5.81]) with mapi id 14.03.0319.002; Wed, 18 Jul 2018 20:59:06 +0800 From: "Dong, Eric" To: Laszlo Ersek , "edk2-devel@lists.01.org" CC: "Ni, Ruiyu" Thread-Topic: [edk2] [Patch V2] UefiCpuPkg/MpInitLib: Remove redundant parameter. Thread-Index: AQHUD1ha1xVe4sKjFEqE5j2K5wud9KR2oK+AgB5mWOA= Date: Wed, 18 Jul 2018 12:59:05 +0000 Message-ID: References: <20180629032047.6340-1-eric.dong@intel.com> <2eac3f3f-972f-9844-6567-5503a0403a85@redhat.com> In-Reply-To: <2eac3f3f-972f-9844-6567-5503a0403a85@redhat.com> Accept-Language: en-US X-MS-Has-Attach: yes X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] MIME-Version: 1.0 X-Mailman-Approved-At: Wed, 18 Jul 2018 10:35:03 -0700 X-Content-Filtered-By: Mailman/MimeDel 2.1.27 Subject: Re: [Patch V2] UefiCpuPkg/MpInitLib: Remove redundant parameter. X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.27 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 18 Jul 2018 13:00:22 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Hi Laszlo, I finally succeed to setup the OVMF platform which can verify the boot fail= ure issue. But on my platform, if I use image build with below command (I = assume it is used to enable SMM), the system can't boot to OS (host OS is f= edora 25 and guest OS is Ubuntu 18.04). It hang at OS boot phase after Exit= BootService point (I can see the console log which should been printed at E= xitBootService point, so I think hang should after this point).=20 build -a IA32 -a X64 -p OvmfPkg/OvmfPkgIa32X64.dsc -t VS2015x86 -b NOOPT -= D SMM_REQUIRE -D SECURE_BOOT_ENABLE -D TLS_ENABLE=20 If I use below command to build the image, the system can boot to OS. =20 build -a IA32 -a X64 -p OvmfPkg\OvmfPkgIa32X64.dsc -t VS2015x86 -b NOOPT Does my OVMF environment still has problem? When do the above test, I don't include my two patches. Then I include my p= atches and build the image with SMM enabled, I found I can't reproduce the = issue you met. I can find the "MpInitChangeApLoopCallback done!" message in= the console log. Attached the console log. Can you help to verify the OVMF image build from my side? Also can you att= ach the image from you side to let me try?=20 Thanks, Eric > -----Original Message----- > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of > Laszlo Ersek > Sent: Friday, June 29, 2018 8:15 PM > To: Dong, Eric ; edk2-devel@lists.01.org > Cc: Ni, Ruiyu > Subject: Re: [edk2] [Patch V2] UefiCpuPkg/MpInitLib: Remove redundant > parameter. >=20 > Hi Eric, >=20 > 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) !=3D CpuStateBusy) { > > - CpuMpData->RunningCount ++; > > + CpuMpData->RunningCount --; > > CpuMpData->CpuData[ProcessorNumber].Waiting =3D FALSE; > > > > // > > @@ -1425,7 +1425,7 @@ CheckAllAPs ( > > // > > // If all APs finish, return EFI_SUCCESS. > > // > > - if (CpuMpData->RunningCount =3D=3D CpuMpData->StartCount) { > > + if (CpuMpData->RunningCount =3D=3D 0) { > > return EFI_SUCCESS; > > } > > > > @@ -1442,7 +1442,7 @@ CheckAllAPs ( > > // > > if (CpuMpData->FailedCpuList !=3D NULL) { > > *CpuMpData->FailedCpuList =3D > > - AllocatePool ((CpuMpData->StartCount - CpuMpData->FinishedCou= nt > + 1) * sizeof (UINTN)); > > + AllocatePool ((CpuMpData->RunningCount + 1) * sizeof > > + (UINTN)); > > ASSERT (*CpuMpData->FailedCpuList !=3D NULL); > > } > > ListIndex =3D 0; > > @@ -2121,7 +2121,7 @@ StartupAllAPsWorker ( > > return EFI_NOT_STARTED; > > } > > > > - CpuMpData->StartCount =3D 0; > > + CpuMpData->RunningCount =3D 0; > > for (ProcessorNumber =3D 0; ProcessorNumber < ProcessorCount; > ProcessorNumber++) { > > CpuData =3D &CpuMpData->CpuData[ProcessorNumber]; > > CpuData->Waiting =3D FALSE; > > @@ -2131,7 +2131,7 @@ StartupAllAPsWorker ( > > // Mark this processor as responsible for current calling. > > // > > CpuData->Waiting =3D TRUE; > > - CpuMpData->StartCount++; > > + CpuMpData->RunningCount++; > > } > > } > > } > > @@ -2140,7 +2140,6 @@ StartupAllAPsWorker ( > > CpuMpData->ProcArguments =3D ProcedureArgument; > > CpuMpData->SingleThread =3D SingleThread; > > CpuMpData->FinishedCount =3D 0; > > - CpuMpData->RunningCount =3D 0; > > CpuMpData->FailedCpuList =3D FailedCpuList; > > CpuMpData->ExpectedTime =3D 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; > > >=20 > I got confused by the way you sent out this patch. >=20 > First I thought that you meant it separately (stand-alone). I tried to te= st it, but > it didn't apply. Also my intent was to ask you whether you wanted to send= a > new version of >=20 > [edk2] [Patch 1/2] UefiCpuPkg/MpInitLib: Not use disabled AP. >=20 >=20 > However, upon seeing that this patch wouldn't apply, I'm now thinking tha= t > you would like to preserve >=20 > [edk2] [Patch 1/2] UefiCpuPkg/MpInitLib: Not use disabled AP. >=20 > without any changes, and your intent is to only update >=20 > [edk2] [Patch 2/2] UefiCpuPkg/MpInitLib: Remove redundant parameter. >=20 > to version 2. >=20 >=20 > In such cases, please do not post an independent patch. Instead, pick one= of > the following: >=20 > - Repost the entire series as v2, and mark in the cover letter that > patch #1 is unchanged from the v1 posting. >=20 > - Alternatively, post the patch in response to the *original* v1 thread > (using --in-reply-to=3D with git-send-email), and use the subject >=20 > [edk2] [Patch v2 2/2] UefiCpuPkg/MpInitLib: Remove redundant > parameter. >=20 > Either of these approaches will let reviewers know that you intend the tw= o > patches to go together (and in what order), and that only patch #2 has be= en > updated. >=20 >=20 > So... Assuming this was indeed your intent, I applied the following two p= atches > locally, for testing: >=20 > [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 >=20 > [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) >=20 > 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): >=20 > (1) i440fx machine type, X64 build, without SMM > (2) q35 machine type, IA32 build, with SMM > (3) q35 machine type, IA32X64 build, with SMM >=20 > The guest OS was Fedora in every case. >=20 > The series keeps test case (1) functional. >=20 > 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: >=20 > MpInitChangeApLoopCallback() done! >=20 > and the boot process gets stuck. >=20 > With more context, a before/after log file diff, for case (2): >=20 > > VirtioScsiExitBoot: Context=3D0x7DF3F010 > > SmmInstallProtocolInterface: [EdkiiSmmExitBootServicesProtocol] 0 > > -MpInitChangeApLoopCallback() done! >=20 > With more context, a before/after log file diff, for case (3): >=20 > > VirtioScsiExitBoot: Context=3D0x7DBD4398 > > VirtioRngExitBoot: Context=3D0x7DC47318 > > SmmInstallProtocolInterface: [EdkiiSmmExitBootServicesProtocol] 0 > > -MpInitChangeApLoopCallback() done! >=20 > I guess the patch series breaks something in > MpInitChangeApLoopCallback() when the SMM driver stack is included in the > build. >=20 > ... After repeated testing, the boot succeeds *sometimes* (rarely). In mo= st > cases, the boot fails as described above. >=20 > Thanks, > Laszlo > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel