From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: redhat.com, ip: 209.132.183.28, mailfrom: lersek@redhat.com) Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by groups.io with SMTP; Mon, 03 Jun 2019 08:20:44 -0700 Received: from smtp.corp.redhat.com (int-mx08.intmail.prod.int.phx2.redhat.com [10.5.11.23]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 62EFB821F3; Mon, 3 Jun 2019 15:20:38 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-121-13.rdu2.redhat.com [10.10.121.13]) by smtp.corp.redhat.com (Postfix) with ESMTP id 2289F2E04C; Mon, 3 Jun 2019 15:20:35 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH] UefiCpuPkg/MpInitLib: Decrease NumApsExecuting only for ApInitConfig To: devel@edk2.groups.io, ray.ni@intel.com Cc: Eric Dong , Nandagopal Sathyanarayanan References: <20190531094254.248276-1-ray.ni@intel.com> From: "Laszlo Ersek" Message-ID: <6b2cf741-30aa-fe11-8a0c-66bd17ef2057@redhat.com> Date: Mon, 3 Jun 2019 17:20:35 +0200 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: <20190531094254.248276-1-ray.ni@intel.com> X-Scanned-By: MIMEDefang 2.84 on 10.5.11.23 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.28]); Mon, 03 Jun 2019 15:20:38 +0000 (UTC) Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Hi Ray, On 05/31/19 11:42, Ni, Ray wrote: > The patch fixes the bug that the memory under 1MB is modified by > firmware in S3 boot. > > Root cause is a racing condition in MpInitLib: > 1. BSP: WakeUpByInitSipiSipi is set by NotifyOnS3SmmInitDonePpi() > 2. BSP: WakeUpAP() wakes all APs to run certain procedure. > 2.1. AllocateResetVector() uses <1MB memory for wake up vector. > 2.1. FillExchangeInfoData() resets NumApsExecuting to 0. > 2.2. WaitApWakeup() waits AP to clear WAKEUP_AP_SIGNAL. > 3. AP: ApWakeupFunction() clears WAKEUP_AP_SIGNAL to inform BSP. > 5. BSP: FreeResetVector() restores the <1MB memory > 4. AP: ApWakeupFunction() calls the certain procedure. > 4.1. NumApsExecuting is decreased. > > #4.1 happens after the 1MB memory is restored so the result is > memory below 1MB is changed by #4.1 > It happens only when the AP executes procedure a bit longer. > AP returns back to ApWakeupFunction() from procedure after > BSP restores the <1MB memory. > > Since NumApsExecuting is only used when InitFlag == ApInitConfig > for counting the processor count. > The patch moves the NumApsExecuting decrease to the path when > InitFlag == ApInitConfig. > > Signed-off-by: Ray Ni > Cc: Eric Dong > Cc: Nandagopal Sathyanarayanan > --- > UefiCpuPkg/Library/MpInitLib/MpLib.c | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c > index 3337488892..84f1cb92e3 100644 > --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c > +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c > @@ -1,7 +1,7 @@ > /** @file > CPU MP Initialize Library common functions. > > - Copyright (c) 2016 - 2018, Intel Corporation. All rights reserved.
> + Copyright (c) 2016 - 2019, Intel Corporation. All rights reserved.
> SPDX-License-Identifier: BSD-2-Clause-Patent > > **/ > @@ -619,6 +619,8 @@ ApWakeupFunction ( > RestoreVolatileRegisters (&CpuMpData->CpuData[0].VolatileRegisters, FALSE); > InitializeApData (CpuMpData, ProcessorNumber, BistData, ApTopOfStack); > ApStartupSignalBuffer = CpuMpData->CpuData[ProcessorNumber].StartupApSignal; > + > + InterlockedDecrement ((UINT32 *) &CpuMpData->MpCpuExchangeInfo->NumApsExecuting); > } else { > // > // Execute AP function if AP is ready > @@ -698,7 +700,6 @@ ApWakeupFunction ( > // AP finished executing C code > // > InterlockedIncrement ((UINT32 *) &CpuMpData->FinishedCount); > - InterlockedDecrement ((UINT32 *) &CpuMpData->MpCpuExchangeInfo->NumApsExecuting); > > // > // Place AP is specified loop mode > @@ -805,6 +806,7 @@ FillExchangeInfoData ( > ExchangeInfo->CFunction = (UINTN) ApWakeupFunction; > ExchangeInfo->ApIndex = 0; > ExchangeInfo->NumApsExecuting = 0; > + DEBUG ((DEBUG_ERROR, "NumApsExecuting = %p\n", &ExchangeInfo->NumApsExecuting)); > ExchangeInfo->InitFlag = (UINTN) CpuMpData->InitFlag; > ExchangeInfo->CpuInfo = (CPU_INFO_IN_HOB *) (UINTN) CpuMpData->CpuInfoInHob; > ExchangeInfo->CpuMpData = CpuMpData; > @@ -1598,6 +1600,7 @@ MpInitLibInitialize ( > CpuMpData->Buffer = Buffer; > CpuMpData->CpuApStackSize = ApStackSize; > CpuMpData->BackupBuffer = BackupBufferAddr; > + DEBUG ((DEBUG_ERROR, "BackupBuffer = %p\n", BackupBufferAddr)); > CpuMpData->BackupBufferSize = ApResetVectorSize; > CpuMpData->WakeupBuffer = (UINTN) -1; > CpuMpData->CpuCount = 1; > (1) I think we should not use DEBUG_ERROR for informational or even debug messages. We should use DEBUG_INFO or DEBUG_VERBOSE. (2) %p should be used for logging values of type (VOID*). The address of "ExchangeInfo->NumApsExecuting" is "close enough" (at least the expression produces a pointer value), but "BackupBufferAddr" has type "UINTN". For printing UINTN, the portable way is to cast the value to UINT64, and log it with %Lx. (3) The commit message states "NumApsExecuting is only used when InitFlag == ApInitConfig". This may be the intent, but my reading of the assembly code does not confirm it. It is true that NumApsExecuting is monitored by the BSP in WakeUpAP() only if (InitFlag == ApInitConfig). It is also true that "LongModeStart" in "UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm" increments NumApsExecuting only when (InitFlag == ApInitConfig). However, in "Flat32Start", in "UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm", we increment NumApsExecuting *first*, and check (InitFlag == ApInitConfig) only second. I think the behavior of the IA32 assembly is unintended. I suggest that we fix that first, in a separate patch. And then the correctness of the present patch may be seen more easily. Then we can state: - only CollectProcessorCount() sets InitFlag to ApInitConfig, - the monitoring of NumApsExecuting is restricted to ApInitConfig, - the incrementing of NumApsExecuting is restricted to ApInitConfig (after patch v2 1/2), - and so the decrementing should be similarly restricted (in patch v2 2/2 -- i.e., this patch). Thanks Laszlo