From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: intel.com, ip: 192.55.52.88, mailfrom: eric.dong@intel.com) Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by groups.io with SMTP; Tue, 03 Sep 2019 22:37:51 -0700 X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga005.fm.intel.com ([10.253.24.32]) by fmsmga101.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 03 Sep 2019 22:37:51 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.64,465,1559545200"; d="scan'208";a="382378352" Received: from fmsmsx106.amr.corp.intel.com ([10.18.124.204]) by fmsmga005.fm.intel.com with ESMTP; 03 Sep 2019 22:37:51 -0700 Received: from fmsmsx604.amr.corp.intel.com (10.18.126.84) by FMSMSX106.amr.corp.intel.com (10.18.124.204) with Microsoft SMTP Server (TLS) id 14.3.439.0; Tue, 3 Sep 2019 22:37:51 -0700 Received: from fmsmsx604.amr.corp.intel.com (10.18.126.84) by fmsmsx604.amr.corp.intel.com (10.18.126.84) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.1713.5; Tue, 3 Sep 2019 22:37:47 -0700 Received: from shsmsx153.ccr.corp.intel.com (10.239.6.53) by fmsmsx604.amr.corp.intel.com (10.18.126.84) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256) id 15.1.1713.5 via Frontend Transport; Tue, 3 Sep 2019 22:37:47 -0700 Received: from shsmsx102.ccr.corp.intel.com ([169.254.2.113]) by SHSMSX153.ccr.corp.intel.com ([169.254.12.235]) with mapi id 14.03.0439.000; Wed, 4 Sep 2019 13:37:45 +0800 From: "Dong, Eric" To: "Ni, Ray" , "Nikodem, Damian" , "devel@edk2.groups.io" CC: "You, Benjamin" , Laszlo Ersek , "Rusocki, Krzysztof" Subject: Re: [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: Race condition between APHandler's release of Busy spinlock and user-triggered SmmStartupThisAP's Thread-Topic: [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: Race condition between APHandler's release of Busy spinlock and user-triggered SmmStartupThisAP's Thread-Index: AQHVYmhAE7ykA14WjUiuPc1YCiQtsqcZph8AgAFaUWA= Date: Wed, 4 Sep 2019 05:37:45 +0000 Message-ID: References: <20190903145732.18604-1-damian.nikodem@intel.com> <734D49CCEBEEF84792F5B80ED585239D5C2BE9FA@SHSMSX104.ccr.corp.intel.com> In-Reply-To: <734D49CCEBEEF84792F5B80ED585239D5C2BE9FA@SHSMSX104.ccr.corp.intel.com> Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] MIME-Version: 1.0 Return-Path: eric.dong@intel.com Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Agree with Ray, no need to call AcquireSpinLockOrFail anymore. I think fina= l code like change like below: - if (Token =3D=3D NULL) { - AcquireSpinLock (mSmmMpSyncData->CpuData[CpuIndex].Busy); - } else { - if (!AcquireSpinLockOrFail (mSmmMpSyncData->CpuData[CpuIndex].Busy)) { - DEBUG((DEBUG_ERROR, "Can't acquire mSmmMpSyncData->CpuData[%d].Busy\= n", CpuIndex)); - return EFI_NOT_READY; - } + AcquireSpinLock (mSmmMpSyncData->CpuData[CpuIndex].Busy); =20 + if (Token !=3D NULL) { Thanks, Eric > -----Original Message----- > From: Ni, Ray > Sent: Wednesday, September 4, 2019 12:56 AM > To: Nikodem, Damian ; devel@edk2.groups.io > Cc: Dong, Eric ; You, Benjamin > ; Laszlo Ersek ; Rusocki, > Krzysztof > Subject: RE: [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: Race condition between > APHandler's release of Busy spinlock and user-triggered SmmStartupThisAP'= s >=20 > 1. can we directly call AcquireSpinLock()? *OrFail() can be removed IMO. > 2. It's a patch to change the behavior of SmmStartupThisAP(). So that to > reduce the potential bugs in caller's code. Patch title is a bit mis-lead= ing. >=20 > > -----Original Message----- > > From: Nikodem, Damian > > Sent: Tuesday, September 3, 2019 7:58 AM > > To: devel@edk2.groups.io > > Cc: Nikodem, Damian ; Dong, Eric > > ; Ni, Ray ; You, Benjamin > > ; Laszlo Ersek ; Rusocki, > > Krzysztof > > Subject: [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: Race condition between > > APHandler's release of Busy spinlock and user- triggered > > SmmStartupThisAP's > > > > Race condition between APHandler's release of Busy spinlock and > > user-triggered SmmStartupThisAP's acquisition attempt of the Busy spinl= ock > (non-blocking mode). > > > > UserProc is the user's procedure to execute on an AP. > > UserProcCompletion is the user procedure's completion spinlock. > > All other variables are from EDK2. > > > > BSP AP > > > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D > > > APHandler () > > > WaitForSemaphore (Run) > > > > << initial > state >> > > > > AcquireSpinLock (UserProcCompletion) > > SmmStartupThisAp (Procedure) > > AcquireSpinLockOrFail (Busy) > > ReleaseSemaphore (Run) > > > UserProc () > > DoStuff() DoSomeOtherStuff () > > > > AcquireSpinLockOrFail (UserProcCompletion) AcquireSpinLockOrFail > > (UserProcCompletion) > > > > ^^ waiting in a loop for user procedure's > > completion =3D=3D these fail > > ReleaseSpinLock (UserProcCompletion) AcquireSpinLockOrFail > (UserProcCompletion) > > > > ^^ this succeeds > > > > ReleaseSpinLock (UserProcCompletion) > > > > << return control to the caller and > > reenter the flow >>> > > > > AcquireSpinLock (UserProcCompletion) > > SmmStartupThisAp (Procedure) > > AcquireSpinLockOrFail (Busy) > > ^^ this wins the race with AP's > > ReleaseSpinLock and fails; > > > ReleaseSpinLock (Busy) > > return EFI_INVALID_PARAMETER; > > > > To remedy, if AcquireSpinLockOrFail (of the Busy spinlock) fails, > > perform regular AcquireSpinLock -- this eliminates the race condition. > > > > Signed-off-by: Damian Nikodem > > Cc: Eric Dong > > Cc: Ray Ni > > Cc: Benjamin You > > Cc: Laszlo Ersek > > Cc: Krzysztof Rusocki > > > > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c > > b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c > > index d8d2b6f444..206e196a76 100644 > > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c > > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c > > @@ -1239,8 +1239,16 @@ InternalSmmStartupThisAp ( > > AcquireSpinLock (mSmmMpSyncData->CpuData[CpuIndex].Busy); > > } else { > > if (!AcquireSpinLockOrFail (mSmmMpSyncData->CpuData[CpuIndex].Busy= )) > { > > - DEBUG((DEBUG_ERROR, "Can't acquire mSmmMpSyncData- > >CpuData[%d].Busy\n", CpuIndex)); > > - return EFI_NOT_READY; > > + DEBUG ((DEBUG_INFO, "BSP[%d] finds AP[%d] busy at proc 0x%llX > (param 0x%llX), ", > > + mSmmMpSyncData->BspIndex, > > + CpuIndex, > > + *mSmmMpSyncData->CpuData[CpuIndex].Procedure, > > + (VOID*)mSmmMpSyncData->CpuData[CpuIndex].Parameter)); > > + DEBUG ((DEBUG_INFO, "new proc 0x%llX (param 0x%llX). Waiting for > the previous AP procedure to complete...\n", > > + Procedure, > > + ProcArguments)); > > + > > + AcquireSpinLock (mSmmMpSyncData->CpuData[CpuIndex].Busy); > > } > > > > *Token =3D (MM_COMPLETION) CreateToken ();