From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=134.134.136.65; helo=mga03.intel.com; envelope-from=eric.dong@intel.com; receiver=edk2-devel@lists.01.org Received: from mga03.intel.com (mga03.intel.com [134.134.136.65]) (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 166FB2034A7B8 for ; Wed, 25 Oct 2017 18:09:51 -0700 (PDT) Received: from orsmga005.jf.intel.com ([10.7.209.41]) by orsmga103.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 25 Oct 2017 18:13:37 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.43,433,1503385200"; d="scan'208";a="165059361" Received: from fmsmsx107.amr.corp.intel.com ([10.18.124.205]) by orsmga005.jf.intel.com with ESMTP; 25 Oct 2017 18:13:36 -0700 Received: from shsmsx104.ccr.corp.intel.com (10.239.4.70) by fmsmsx107.amr.corp.intel.com (10.18.124.205) with Microsoft SMTP Server (TLS) id 14.3.319.2; Wed, 25 Oct 2017 18:13:36 -0700 Received: from shsmsx102.ccr.corp.intel.com ([169.254.2.175]) by SHSMSX104.ccr.corp.intel.com ([169.254.5.152]) with mapi id 14.03.0319.002; Thu, 26 Oct 2017 09:13:34 +0800 From: "Dong, Eric" To: Laszlo Ersek , "edk2-devel@lists.01.org" CC: "Ni, Ruiyu" , Paolo Bonzini Thread-Topic: [edk2] [Patch 2/2] UefiCpuPkg/MpInitLib: Enhance waiting for AP initialization logic. Thread-Index: AQHTS8/RPtqizoqgTUaTHjY9+C766aLyRGEAgADUE/D//6gkgIABS5EggAAcHwCAASo9QA== Date: Thu, 26 Oct 2017 01:13:34 +0000 Message-ID: References: <1508743358-3640-1-git-send-email-eric.dong@intel.com> <1508743358-3640-3-git-send-email-eric.dong@intel.com> <4fe39a52-0cd3-de2e-84f2-7363823a1b60@redhat.com> <1329f926-4c33-aaca-108a-7d15ae0503bc@redhat.com> <94c27429-4b7d-6c21-c1e5-e07f3e9a5556@redhat.com> In-Reply-To: <94c27429-4b7d-6c21-c1e5-e07f3e9a5556@redhat.com> Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] MIME-Version: 1.0 Subject: Re: [Patch 2/2] UefiCpuPkg/MpInitLib: Enhance waiting for AP initialization logic. X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 26 Oct 2017 01:09:52 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Laszlo, > -----Original Message----- > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of > Laszlo Ersek > Sent: Wednesday, October 25, 2017 11:08 PM > To: Dong, Eric ; edk2-devel@lists.01.org > Cc: Ni, Ruiyu ; Paolo Bonzini > Subject: Re: [edk2] [Patch 2/2] UefiCpuPkg/MpInitLib: Enhance waiting for > AP initialization logic. >=20 > Hi Eric, >=20 > On 10/25/17 07:42, Dong, Eric wrote: > > Hi Laszlo, > > > > I think I have clear about your raised issues for Ovmf platform. In thi= s case, I > think your platform not suit for this code change. How about I do below > change based on the new code: > > > > - if (CpuMpData->CpuCount =3D=3D 0) { > > TimedWaitForApFinish ( > > CpuMpData, > > PcdGet32 (PcdCpuMaxLogicalProcessorNumber) - 1, > > PcdGet32 (PcdCpuApInitTimeOutInMicroSeconds) > > ); > > - } > > > > After this change, for Ovmf can still set > > PcdCpuApInitTimeOutInMicroSeconds to MAX_UINT32 to keep old > solution. > > For the real platform, it can set a small value to follow the new > > solution. For this new change, it just wait one more > > PcdCpuApInitTimeOutInMicroSeconds timeout, should not have > performance > > impact (This time may also been cost in later while > > (CpuMpData->MpCpuExchangeInfo->NumApsExecuting !=3D 0) loop) or have > > litter performance impact (not cover by the later loop). > The suggested change will work for OVMF, thanks! >=20 > (Just please don't forget to un-indent the TimedWaitForApFinish() call th= at > will become unconditional again.) >=20 > --o-- >=20 > Do you think Brian's concern should be investigated as well (perhaps in a > separate Bugzilla entry)? As Jeff has mentioned, only Ovmf can know the exist Ap numbers before=20 send the Init-sipi-sipi. So we don't know how many bits needed for this bi= tmap. In case we can create the bitmap, we still don't know when all Aps have bee= n found(If the begin map bit value same as finish map bit value, we still ca= n't get=20 the conclusion that all Aps have been found, because maybe other Aps not started yet). Thanks, Eric >=20 > Because, I believe, the scheduling pattern that I described earlier could= be > possible on physical platforms as well, at least in theory: >=20 > >> (2) After at least one AP has started running, the logic expects > >> "NumApsExecuting" to monotonically grow for a while, and then to > >> monotonically decrease, back to zero. For example, if we have 1 BSP > >> and > >> 7 APs, the BSP logic more or less expects the following values in > >> "NumApsExecuting": > >> > >> 1; 2; 3; 4; 5; 6; 7; > >> 6; 5; 4; 3; 2; 1; 0 > >> > >> > >> While this may be a valid expectation for physical processors (which > >> actually run in parallel, in the physical world), in a virtual machine= , it is not > guaranteed. > >> Dependent on hypervisor scheduling artifacts, it is possible that, > >> say, three APs start up *and finish* before the remaining four APs > >> start up *at all*. The INIT-SIPI-SIPI marks all APs for execution / > >> scheduling in the hypervisor, yes, but the actual code execution may > >> commence a lot later. For example, the BSP may witness the following > series of values in "NumApsExecuting": > >> > >> 1; 2; 3; > >> 2; 1; 0; > >> 1; 2; 3; 4; > >> 3; 2; 1; 0 > >> > >> and the BSP could think that there are 3 APs only, when it sees the > >> first 0 value. >=20 > I don't know if such a pattern is "likely", "unlikely", or "extremely unl= ikely" on > physical platforms. I think the pattern is "theoretically possible" at le= ast. >=20 > I suggest that we go ahead with the small patch for the OVMF use case fir= st, > and then open a new BZ if Brian thinks there's a real safety problem with= the > code. >=20 We also notice this theoretical problem when we implement this change, but= =20 We think this is rarely to happen on physical platforms. We think the value= for=20 the change is much bigger than not do this change because of this theoretic= al=20 problem. Thanks, Eric > ... I should note that, with the small patch that's going to fix the OVMF= use > case, the previous behavior will be restored for *all* platforms that con= tinue > using their previous PCD values. >=20 > The cumulative effect of commit 0594ec417c89 and the upcoming patch will > be that a platform may significantly decrease > "PcdCpuApInitTimeOutInMicroSeconds", if it chooses to, and then the > "NumApsExecuting" loop will take the role of the preexisting > TimedWaitForApFinish() call. If a platform doesn't want that, then it sho= uld > keep its "PcdCpuApInitTimeOutInMicroSeconds" as high as before, and then > any implementation details in the "NumApsExecuting" loop will be irreleva= nt. >=20 > Thanks! > Laszlo > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel