From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [63.128.21.124]) by mx.groups.io with SMTP id smtpd.web08.37379.1615215937038217086 for ; Mon, 08 Mar 2021 07:05:37 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=FC8Nwmrr; spf=pass (domain: redhat.com, ip: 63.128.21.124, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1615215936; 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=Q6Q4Uc/h3z9Z1ZowYbe8Xfzn/4zlgn+Aho9raW4367k=; b=FC8NwmrrHr12Dli8GZP4YHRkdFNNyfv2Icr79S4twHFup9nF3WZFH9lmj0bY7bx9FHs8QO 5vLxNVl88wPzqaXvuVv3wXXpx4dna9ZmCDLLWgZprcm2XofAp7qDH8gsGeWHbjlLXSS3e8 72C0dgYkhsRuCPPbsUD5Iotk6jh1XTI= 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-558-FxqsoOc-PEeSzzR4JM25YA-1; Mon, 08 Mar 2021 10:05:32 -0500 X-MC-Unique: FxqsoOc-PEeSzzR4JM25YA-1 Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 07D5584B9CE; Mon, 8 Mar 2021 15:05:30 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-114-174.ams2.redhat.com [10.36.114.174]) by smtp.corp.redhat.com (Postfix) with ESMTP id D6E9850C0B; Mon, 8 Mar 2021 15:05:28 +0000 (UTC) Subject: Re: [PATCH] UefiCpuPkg/PiSmmCpu: Don't allocate Token for SmmStartupThisAp To: Ray Ni , devel@edk2.groups.io Cc: Eric Dong , Rahul Kumar References: <20210308021647.528-1-ray.ni@intel.com> From: "Laszlo Ersek" Message-ID: Date: Mon, 8 Mar 2021 16:05:27 +0100 MIME-Version: 1.0 In-Reply-To: <20210308021647.528-1-ray.ni@intel.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.11 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 03/08/21 03:16, Ray Ni wrote: > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3199 > > When Token points to mSmmStartupThisApToken, this routine is called > from SmmStartupThisAp() in non-blocking mode due to > PcdCpuSmmBlockStartupThisAp == FALSE. > > In this case, caller wants to startup AP procedure in non-blocking > mode and cannot get the completion status from the Token because there > is no way to return the Token to caller from SmmStartupThisAp(). > Caller needs to use its specific way to query the completion status. > > There is no need to allocate a token for such case so the 2 overheads > can be avoided: > 1. Call AllocateTokenBuffer() when there is no free token. > 2. Get a free token from the token buffer. > > Signed-off-by: Ray Ni > Cc: Eric Dong > Cc: Laszlo Ersek > Cc: Rahul Kumar > --- > UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c | 27 ++++++++++++++++++++------- > 1 file changed, 20 insertions(+), 7 deletions(-) > > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c > index 6227b2428a..efb89832ca 100644 > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c > @@ -1,7 +1,7 @@ > /** @file > SMM MP service implementation > > -Copyright (c) 2009 - 2020, Intel Corporation. All rights reserved.
> +Copyright (c) 2009 - 2021, Intel Corporation. All rights reserved.
> Copyright (c) 2017, AMD Incorporated. All rights reserved.
> > SPDX-License-Identifier: BSD-2-Clause-Patent > @@ -22,6 +22,7 @@ UINTN mSemaphoreSize; > SPIN_LOCK *mPFLock = NULL; > SMM_CPU_SYNC_MODE mCpuSmmSyncMode; > BOOLEAN mMachineCheckSupported = FALSE; > +MM_COMPLETION mSmmStartupThisApToken; > > extern UINTN mSmmShadowStackSize; > > @@ -1240,9 +1241,23 @@ InternalSmmStartupThisAp ( > mSmmMpSyncData->CpuData[CpuIndex].Procedure = Procedure; > mSmmMpSyncData->CpuData[CpuIndex].Parameter = ProcArguments; > if (Token != NULL) { > - ProcToken= GetFreeToken (1); > - mSmmMpSyncData->CpuData[CpuIndex].Token = ProcToken; > - *Token = (MM_COMPLETION)ProcToken->SpinLock; > + if (Token != &mSmmStartupThisApToken) { > + // > + // When Token points to mSmmStartupThisApToken, this routine is called > + // from SmmStartupThisAp() in non-blocking mode (PcdCpuSmmBlockStartupThisAp == FALSE). > + // > + // In this case, caller wants to startup AP procedure in non-blocking > + // mode and cannot get the completion status from the Token because there > + // is no way to return the Token to caller from SmmStartupThisAp(). > + // Caller needs to use its implementation specific way to query the completion status. > + // > + // There is no need to allocate a token for such case so the overhead of SMRAM and > + // the allocation operation can be avoided. > + // > + ProcToken= GetFreeToken (1); (1) please fix the whitespace error here (it comes from the pre-patch code, but it's just one character, so we can fix it in this patch) > + mSmmMpSyncData->CpuData[CpuIndex].Token = ProcToken; (2) It seems like this patch introduces a new code path. InternalSmmStartupThisAp() will continue thinking that this invocation is non-blocking: if (Token == NULL) { AcquireSpinLock (mSmmMpSyncData->CpuData[CpuIndex].Busy); ReleaseSpinLock (mSmmMpSyncData->CpuData[CpuIndex].Busy); } but APHandler() will think that the invocation was blocking, and it will not call ReleaseToken() any longer: if (mSmmMpSyncData->CpuData[CpuIndex].Token != NULL) { ReleaseToken (CpuIndex); } This behavior seems OK to me. However, please modify the commit message: I think we should list the 3rd step we are omitting (after AllocateTokenBuffer / GetFreeToken) -- namely, ReleaseToken(). With these two superficial updates: Reviewed-by: Laszlo Ersek Thanks Laszlo > + *Token = (MM_COMPLETION)ProcToken->SpinLock; > + } > } > mSmmMpSyncData->CpuData[CpuIndex].Status = CpuStatus; > if (mSmmMpSyncData->CpuData[CpuIndex].Status != NULL) { > @@ -1474,8 +1489,6 @@ SmmStartupThisAp ( > IN OUT VOID *ProcArguments OPTIONAL > ) > { > - MM_COMPLETION Token; > - > gSmmCpuPrivate->ApWrapperFunc[CpuIndex].Procedure = Procedure; > gSmmCpuPrivate->ApWrapperFunc[CpuIndex].ProcedureArgument = ProcArguments; > > @@ -1486,7 +1499,7 @@ SmmStartupThisAp ( > ProcedureWrapper, > CpuIndex, > &gSmmCpuPrivate->ApWrapperFunc[CpuIndex], > - FeaturePcdGet (PcdCpuSmmBlockStartupThisAp) ? NULL : &Token, > + FeaturePcdGet (PcdCpuSmmBlockStartupThisAp) ? NULL : &mSmmStartupThisApToken, > 0, > NULL > ); >