From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [216.205.24.124]) by mx.groups.io with SMTP id smtpd.web12.2621.1611609461481633842 for ; Mon, 25 Jan 2021 13:17:41 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=QX2kYR99; spf=pass (domain: redhat.com, ip: 216.205.24.124, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1611609460; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=68mstXF/1KkdRE23blLjtSc0fTDznnedR7OBh75FN3I=; b=QX2kYR99aApRS/BLiM+n5n0NPSsdFcAotug0hwPrGvEc8STwoiJWq18+NqtZ/4pyb8EEL7 6hnptCdbJqbGE2iYd1lWwuhmSfLeEDJ9qt0bnEz0R50Jj5Q+8waLTskm8ox8ZHEgFGQjYm jJF6Huejzh6sPaJ8f4zJNwZQYLNqeFk= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-213-bgL9j8FZMsGkDiN2xywrWQ-1; Mon, 25 Jan 2021 16:17:36 -0500 X-MC-Unique: bgL9j8FZMsGkDiN2xywrWQ-1 Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.14]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id C218659; Mon, 25 Jan 2021 21:17:34 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-112-67.ams2.redhat.com [10.36.112.67]) by smtp.corp.redhat.com (Postfix) with ESMTP id 13A845D9DB; Mon, 25 Jan 2021 21:17:32 +0000 (UTC) Subject: Re: [edk2-devel] [Patch 1/1] UefiCpuPkg/Library/MpInitLib: Fix AP VolatileRegisters race condition To: "Zeng, Star" , "Ni, Ray" , "Kinney, Michael D" , "devel@edk2.groups.io" Cc: "Dong, Eric" , "Kumar, Rahul1" References: <20210122171020.589-1-michael.d.kinney@intel.com> From: "Laszlo Ersek" Message-ID: Date: Mon, 25 Jan 2021 22:17:32 +0100 MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 2.79 on 10.5.11.14 Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=lersek@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 01/25/21 12:04, Zeng, Star wrote: > BTW: > > Do you think it worth or not to add the check like below in Edk2\UefiCpuPkg\Library\MpInitLib\PeiMpLib.c GetCpuMpData()? If the assert was there, it would facilitate the debug? > > CpuMpData = (CPU_MP_DATA *) (Idtr.Base + Idtr.Limit + 1); > + ASSERT (CpuMpData != NULL); Could be worthwhile, but in a separate patch, IMO. Thanks Laszlo >> -----Original Message----- >> From: Ni, Ray >> Sent: Monday, January 25, 2021 4:53 PM >> To: Zeng, Star ; Kinney, Michael D >> ; devel@edk2.groups.io >> Cc: Dong, Eric ; Laszlo Ersek ; >> Kumar, Rahul1 >> Subject: RE: [edk2-devel] [Patch 1/1] UefiCpuPkg/Library/MpInitLib: Fix AP >> VolatileRegisters race condition >> >> Star, >> You are right. There is no sequence requirement between FinishedCount++ >> and NumApsExecuting--. >> >> In fact, I have submitted another bugzilla >> https://bugzilla.tianocore.org/show_bug.cgi?id=3179. >> >> With that Bugzilla, the wait on (FinishedCount == CpuCount - 1) will be >> removed for the 1st wake up case. >> >> Thanks, >> Ray >> >> >>> -----Original Message----- >>> From: Zeng, Star >>> Sent: Monday, January 25, 2021 4:43 PM >>> To: Kinney, Michael D ; >>> devel@edk2.groups.io >>> Cc: Dong, Eric ; Ni, Ray ; >>> Laszlo Ersek ; Kumar, Rahul1 >>> ; Zeng, Star >>> Subject: RE: [edk2-devel] [Patch 1/1] UefiCpuPkg/Library/MpInitLib: >>> Fix AP VolatileRegisters race condition >>> >>> Mike, >>> >>> Oh, see it. >>> There is no sequence dependence between FinishedCount increment and >>> NumApsExecuting decrement, right? >>> >>> InterlockedIncrement ((UINT32 *) &CpuMpData->FinishedCount); >>> >>> InterlockedDecrement ((UINT32 *) &CpuMpData- >>> MpCpuExchangeInfo- >>>> NumApsExecuting); >>> >>> >>> Thanks, >>> Star >>> >>>> -----Original Message----- >>>> From: Kinney, Michael D >>>> Sent: Monday, January 25, 2021 1:16 PM >>>> To: Zeng, Star ; devel@edk2.groups.io; Kinney, >>>> Michael D >>>> Cc: Dong, Eric ; Ni, Ray ; >>>> Laszlo Ersek ; Kumar, Rahul1 >>>> >>>> Subject: RE: [edk2-devel] [Patch 1/1] UefiCpuPkg/Library/MpInitLib: >>>> Fix AP VolatileRegisters race condition >>>> >>>> Hi Star, >>>> >>>> That line is only active when (CpuMpData->SevEsIsEnabled) is TRUE. >>>> >>>> The race condition addressed by this BZ is for systems with >>>> SecEsIsEnabled FALSE. >>>> >>>> From comments in this file the SecEsIsEnabled cases have already >>>> been handled. >>>> >>>> Mike >>>> >>>>> -----Original Message----- >>>>> From: Zeng, Star >>>>> Sent: Sunday, January 24, 2021 7:15 PM >>>>> To: devel@edk2.groups.io; Kinney, Michael D >>>>> >>>>> Cc: Dong, Eric ; Ni, Ray ; >>>>> Laszlo Ersek ; Kumar, Rahul1 >>>>> ; Zeng, Star >>>>> Subject: RE: [edk2-devel] [Patch 1/1] UefiCpuPkg/Library/MpInitLib: >>>>> Fix AP VolatileRegisters race condition >>>>> >>>>> Does >>>>> >>>> >>> >> https://github.com/tianocore/edk2/blob/master/UefiCpuPkg/Library/MpIni >>>>> tLib/MpLib.c#L909 (also decrements >>>>> NumApsExecuting) also need be handled? >>>>> >>>>> Thanks, >>>>> Star >>>>> >>>>>> -----Original Message----- >>>>>> From: devel@edk2.groups.io On Behalf Of >>>>>> Michael D Kinney >>>>>> Sent: Saturday, January 23, 2021 1:10 AM >>>>>> To: devel@edk2.groups.io >>>>>> Cc: Dong, Eric ; Ni, Ray >>>>>> ; Laszlo Ersek ; Kumar, >>>>>> Rahul1 >>>>>> Subject: [edk2-devel] [Patch 1/1] UefiCpuPkg/Library/MpInitLib: >>>>>> Fix AP VolatileRegisters race condition >>>>>> >>>>>> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3182 >>>>>> >>>>>> Fix the order of operations in ApWakeupFunction() when >>>>>> PcdCpuApLoopMode is set to HLT mode that uses INIT-SIPI-SIPI to >>> wake >>>>>> APs. In this mode, volatile state is restored and saved each >>>>>> time a INIT-SIPI-SIPI is sent to an AP to request a function to >>>>>> be executed on the AP. When the function is completed the >>>>>> volatile state of the AP is saved. However, the counters >>>>>> NumApsExecuting and FinishedCount are updated before the >>>>>> volatile state is saved. This allows for a race condition >>>>>> window for the BSP that is waiting on these counters to request >>>>>> a new INIT-SIPI-SIPI before all the APs have completely saved >>>>>> their volatile state. The fix is to save the AP volatile state >>>>>> before updating the NumApsExecuting and FinishedCount counters. >>>>>> >>>>>> Cc: Eric Dong >>>>>> Cc: Ray Ni >>>>>> Cc: Laszlo Ersek >>>>>> Cc: Rahul Kumar >>>>>> Signed-off-by: Michael D Kinney >>>>>> --- >>>>>> UefiCpuPkg/Library/MpInitLib/MpLib.c | 31 >>>>>> ++++++++++++++++------------ >>>>>> 1 file changed, 18 insertions(+), 13 deletions(-) >>>>>> >>>>>> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c >>>>>> b/UefiCpuPkg/Library/MpInitLib/MpLib.c >>>>>> index 681fa79b4cff..8b1f7f84bad6 100644 >>>>>> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c >>>>>> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c >>>>>> @@ -769,15 +769,6 @@ ApWakeupFunction ( >>>>>> RestoreVolatileRegisters >>>>>> (&CpuMpData->CpuData[0].VolatileRegisters, >>>>>> FALSE); >>>>>> InitializeApData (CpuMpData, ProcessorNumber, BistData, >>>>>> ApTopOfStack); >>>>>> ApStartupSignalBuffer = CpuMpData- >>>>>>> CpuData[ProcessorNumber].StartupApSignal; >>>>>> - >>>>>> - // >>>>>> - // Delay decrementing the APs executing count when SEV-ES is >>>> enabled >>>>>> - // to allow the APs to issue an AP_RESET_HOLD before the BSP >>>> possibly >>>>>> - // performs another INIT-SIPI-SIPI sequence. >>>>>> - // >>>>>> - if (!CpuMpData->SevEsIsEnabled) { >>>>>> - InterlockedDecrement ((UINT32 *) &CpuMpData- >>>>>>> MpCpuExchangeInfo->NumApsExecuting); >>>>>> - } >>>>>> } else { >>>>>> // >>>>>> // Execute AP function if AP is ready @@ -866,19 +857,33 >>>>>> @@ ApWakeupFunction ( >>>>>> } >>>>>> } >>>>>> >>>>>> + if (CpuMpData->ApLoopMode == ApInHltLoop) { >>>>>> + // >>>>>> + // Save AP volatile registers >>>>>> + // >>>>>> + SaveVolatileRegisters (&CpuMpData- >>>>>>> CpuData[ProcessorNumber].VolatileRegisters); >>>>>> + } >>>>>> + >>>>>> // >>>>>> // AP finished executing C code >>>>>> // >>>>>> InterlockedIncrement ((UINT32 *) >>>>>> &CpuMpData->FinishedCount); >>>>>> >>>>>> + if (CpuMpData->InitFlag == ApInitConfig) { >>>>>> + // >>>>>> + // Delay decrementing the APs executing count when SEV-ES >>>>>> + is >>>> enabled >>>>>> + // to allow the APs to issue an AP_RESET_HOLD before the >>>>>> + BSP >>>> possibly >>>>>> + // performs another INIT-SIPI-SIPI sequence. >>>>>> + // >>>>>> + if (!CpuMpData->SevEsIsEnabled) { >>>>>> + InterlockedDecrement ((UINT32 *) &CpuMpData- >>>>>>> MpCpuExchangeInfo->NumApsExecuting); >>>>>> + } >>>>>> + } >>>>>> + >>>>>> // >>>>>> // Place AP is specified loop mode >>>>>> // >>>>>> if (CpuMpData->ApLoopMode == ApInHltLoop) { >>>>>> - // >>>>>> - // Save AP volatile registers >>>>>> - // >>>>>> - SaveVolatileRegisters (&CpuMpData- >>>>>>> CpuData[ProcessorNumber].VolatileRegisters); >>>>>> // >>>>>> // Place AP in HLT-loop >>>>>> // >>>>>> -- >>>>>> 2.29.2.windows.2 >>>>>> >>>>>> >>>>>> >>>>>> >>>>>> >