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 F34382034A7B9 for ; Tue, 24 Oct 2017 03:12:05 -0700 (PDT) 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 E21976106; Tue, 24 Oct 2017 10:15:48 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com E21976106 Authentication-Results: ext-mx01.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx01.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=lersek@redhat.com Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-52.rdu2.redhat.com [10.10.120.52]) by smtp.corp.redhat.com (Postfix) with ESMTP id C292460F8B; Tue, 24 Oct 2017 10:15:47 +0000 (UTC) To: Eric Dong , edk2-devel@lists.01.org Cc: Ruiyu Ni , Paolo Bonzini References: <1508743358-3640-1-git-send-email-eric.dong@intel.com> <1508743358-3640-3-git-send-email-eric.dong@intel.com> From: Laszlo Ersek Message-ID: <4fe39a52-0cd3-de2e-84f2-7363823a1b60@redhat.com> Date: Tue, 24 Oct 2017 12:15:46 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.4.0 MIME-Version: 1.0 In-Reply-To: <1508743358-3640-3-git-send-email-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.25]); Tue, 24 Oct 2017 10:15:49 +0000 (UTC) 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: Tue, 24 Oct 2017 10:12:06 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit CC Paolo On 10/23/17 09:22, Eric Dong wrote: > Current logic always waiting for a specific value to collect all APs > count. This logic may caused some platforms cost too much time to > wait for time out. > This patch add new logic to collect APs count. It adds new variable > NumApsExecuting to detect whether all APs have finished initialization. > Each AP let NumApsExecuting++ when begin to initialize itself and let > NumApsExecuting-- when it finish the initialization. BSP base on whether > NumApsExecuting == 0 to finished the collect AP process. > > Cc: Ruiyu Ni > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Eric Dong > --- > UefiCpuPkg/Library/MpInitLib/Ia32/MpEqu.inc | 1 + > UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm | 6 ++++++ > UefiCpuPkg/Library/MpInitLib/MpLib.c | 20 ++++++++++++++------ > UefiCpuPkg/Library/MpInitLib/MpLib.h | 1 + > UefiCpuPkg/Library/MpInitLib/X64/MpEqu.inc | 3 ++- > UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm | 6 ++++++ > 6 files changed, 30 insertions(+), 7 deletions(-) I was out of office yesterday, and did not get a chance to comment on this patch. In a virtualization guest, I see the following problem with the patch: VCPUs (virtual CPUs) may not be scheduled by the hypervisor similarly to how physical CPUs behave on a physical board. It is possible that a VCPU starts up and even finishes its initialization routine before another VCPU starts running at all. Therefore the locked NumApsExecuting counter may hit zero, even multiple times, before all APs have finished initializing. In OVMF, we query QEMU about the exact number of virtual processors, in PlatformPei. So OVMF configures the logical processor count in advance that MpInitLib has to wait for. Correspondingly, we also set the timeout to "infinity". Please see the MaxCpuCountInitialization() function in following commit: https://github.com/tianocore/edk2/commit/45a70db3c3a59 In the past, we used to have AP initialization problems in OVMF due to the VCPU scheduling artifacts I mention above. After commit 45a70db3c3a59, things have been stable; it would be nice to keep that working. Please note that simply testing this patch on my end is not sufficient. The AP init problems we used to face were sporadic and also specific to the virtualization host systems (i.e., dependent on the physical hardware and the host kernel). Furthermore: > diff --git a/UefiCpuPkg/Library/MpInitLib/Ia32/MpEqu.inc b/UefiCpuPkg/Library/MpInitLib/Ia32/MpEqu.inc > index 976af1f..bdfe0d3 100644 > --- a/UefiCpuPkg/Library/MpInitLib/Ia32/MpEqu.inc > +++ b/UefiCpuPkg/Library/MpInitLib/Ia32/MpEqu.inc > @@ -40,4 +40,5 @@ EnableExecuteDisableLocation equ LockLocation + 30h > Cr3Location equ LockLocation + 34h > InitFlagLocation equ LockLocation + 38h > CpuInfoLocation equ LockLocation + 3Ch > +NumApsExecutingLocation equ LockLocation + 40h > > diff --git a/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm b/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm > index 1b9c6a6..2b6c27d 100644 > --- a/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm > +++ b/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm > @@ -86,6 +86,12 @@ Flat32Start: ; protected mode entry point > > mov esi, ebx > > + ; Increment the number of APs executing here as early as possible > + ; This is decremented in C code when AP is finished executing > + mov edi, esi > + add edi, NumApsExecutingLocation > + lock inc dword [edi] > + > mov edi, esi > add edi, EnableExecuteDisableLocation > cmp byte [edi], 0 > diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c > index db923c9..48f930b 100644 > --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c > +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c > @@ -662,6 +662,7 @@ ApWakeupFunction ( > // AP finished executing C code > // > InterlockedIncrement ((UINT32 *) &CpuMpData->FinishedCount); > + InterlockedDecrement ((UINT32 *) &CpuMpData->MpCpuExchangeInfo->NumApsExecuting); > > // > // Place AP is specified loop mode > @@ -765,6 +766,7 @@ FillExchangeInfoData ( > > ExchangeInfo->CFunction = (UINTN) ApWakeupFunction; > ExchangeInfo->ApIndex = 0; > + ExchangeInfo->NumApsExecuting = 0; > ExchangeInfo->InitFlag = (UINTN) CpuMpData->InitFlag; > ExchangeInfo->CpuInfo = (CPU_INFO_IN_HOB *) (UINTN) CpuMpData->CpuInfoInHob; > ExchangeInfo->CpuMpData = CpuMpData; > @@ -934,13 +936,19 @@ WakeUpAP ( > } > if (CpuMpData->InitFlag == ApInitConfig) { > // > - // Wait for all potential APs waken up in one specified period > + // Wait for one potential AP waken up in one specified period > // > - TimedWaitForApFinish ( > - CpuMpData, > - PcdGet32 (PcdCpuMaxLogicalProcessorNumber) - 1, > - PcdGet32 (PcdCpuApInitTimeOutInMicroSeconds) > - ); > + if (CpuMpData->CpuCount == 0) { > + TimedWaitForApFinish ( > + CpuMpData, > + PcdGet32 (PcdCpuMaxLogicalProcessorNumber) - 1, > + PcdGet32 (PcdCpuApInitTimeOutInMicroSeconds) > + ); > + } I don't understand this change. The new comment says, Wait for *one* potential AP waken up in one specified period However, the second parameter of TimedWaitForApFinish(), namely "FinishedApLimit", gets the same value as before: PcdGet32 (PcdCpuMaxLogicalProcessorNumber) - 1 It means that all of the (possible) APs are waited-for, just the same as before. In other words, I think the patch does not correctly implement what the commit message says -- and for QEMU / OVMF, that's actually a good thing at the moment, because a correct implementation of the description would likely break on QEMU. Thanks Laszlo > + > + while (CpuMpData->MpCpuExchangeInfo->NumApsExecuting != 0) { > + CpuPause(); > + } > } else { > // > // Wait all APs waken up if this is not the 1st broadcast of SIPI > diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.h b/UefiCpuPkg/Library/MpInitLib/MpLib.h > index e41d2db..d13d5c0 100644 > --- a/UefiCpuPkg/Library/MpInitLib/MpLib.h > +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.h > @@ -176,6 +176,7 @@ typedef struct { > UINTN Cr3; > UINTN InitFlag; > CPU_INFO_IN_HOB *CpuInfo; > + UINTN NumApsExecuting; > CPU_MP_DATA *CpuMpData; > UINTN InitializeFloatingPointUnitsAddress; > } MP_CPU_EXCHANGE_INFO; > diff --git a/UefiCpuPkg/Library/MpInitLib/X64/MpEqu.inc b/UefiCpuPkg/Library/MpInitLib/X64/MpEqu.inc > index 114f4e0..d255ca5 100644 > --- a/UefiCpuPkg/Library/MpInitLib/X64/MpEqu.inc > +++ b/UefiCpuPkg/Library/MpInitLib/X64/MpEqu.inc > @@ -40,5 +40,6 @@ EnableExecuteDisableLocation equ LockLocation + 5Ch > Cr3Location equ LockLocation + 64h > InitFlagLocation equ LockLocation + 6Ch > CpuInfoLocation equ LockLocation + 74h > -InitializeFloatingPointUnitsAddress equ LockLocation + 84h > +NumApsExecutingLocation equ LockLocation + 7Ch > +InitializeFloatingPointUnitsAddress equ LockLocation + 8Ch > > diff --git a/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm b/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm > index 4ada649..21d2786 100644 > --- a/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm > +++ b/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm > @@ -124,6 +124,12 @@ LongModeStart: > cmp qword [edi], 1 ; ApInitConfig > jnz GetApicId > > + ; Increment the number of APs executing here as early as possible > + ; This is decremented in C code when AP is finished executing > + mov edi, esi > + add edi, NumApsExecutingLocation > + lock inc dword [edi] > + > ; AP init > mov edi, esi > add edi, LockLocation >