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.36268.1614005650391113535 for ; Mon, 22 Feb 2021 06:54:10 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=Nf9nUUDf; 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=1614005649; 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=jNVHIuhZ32fda5Xq1QPLzCWZfgAK81TM6R+4r4EfH0o=; b=Nf9nUUDfkoCliyVHu1ggAkR4F1ZFB3bBoW0wNpOn/lCZWqNFLPZanAADI95kexfAPY6a4c XoTjnR6JNiLX6LanomrtB+/IZ+2WjJhQt+7f0j6sThOm5Rao7FziUQF6QIU9gcJUHuf5V9 JQQhpcTIDoGDKMZLlhWBIg8+Srp3YRs= 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-178-FJzbqQ1gPIaEdVv32l25dA-1; Mon, 22 Feb 2021 09:54:05 -0500 X-MC-Unique: FJzbqQ1gPIaEdVv32l25dA-1 Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 433F8108E1D1; Mon, 22 Feb 2021 14:53:51 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-113-67.ams2.redhat.com [10.36.113.67]) by smtp.corp.redhat.com (Postfix) with ESMTP id 21AAC841D3; Mon, 22 Feb 2021 14:53:48 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH v8 07/10] OvmfPkg/SmmCpuFeaturesLib: call CPU hot-eject handler To: devel@edk2.groups.io, ankur.a.arora@oracle.com Cc: imammedo@redhat.com, boris.ostrovsky@oracle.com, Jordan Justen , Ard Biesheuvel , Aaron Young , Paolo Bonzini References: <20210222071928.1401820-1-ankur.a.arora@oracle.com> <20210222071928.1401820-8-ankur.a.arora@oracle.com> From: "Laszlo Ersek" Message-ID: Date: Mon, 22 Feb 2021 15:53:48 +0100 MIME-Version: 1.0 In-Reply-To: <20210222071928.1401820-8-ankur.a.arora@oracle.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.13 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 Adding Paolo, one comment below: On 02/22/21 08:19, Ankur Arora wrote: > Call the CPU hot-eject handler if one is installed. The condition for > installation is (PcdCpuMaxLogicalProcessorNumber > 1), and there's > a hot-unplug request. > > The handler executes in context of SmmCpuFeaturesRendezvousExit(), > which is called at the tail end of SmiRendezvous() after the BSP has > given the signal to exit via the "AllCpusInSync" loop. > > Cc: Laszlo Ersek > Cc: Jordan Justen > Cc: Ard Biesheuvel > Cc: Igor Mammedov > Cc: Boris Ostrovsky > Cc: Aaron Young > Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3132 > Signed-off-by: Ankur Arora > --- > > Notes: > Address the following review comments from v6, patch-6: > (19a) Move the call to the ejection handler to a separate patch. > (19b) Describe the calling context of SmmCpuFeaturesRendezvousExit(). > (20) Add comment describing the state when the Handler is not armed. > > OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c | 15 +++++++++++++++ > 1 file changed, 15 insertions(+) > > diff --git a/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c b/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c > index adbfc90ad46e..99988285b6a2 100644 > --- a/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c > +++ b/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c > @@ -467,6 +467,21 @@ SmmCpuFeaturesRendezvousExit ( > IN UINTN CpuIndex > ) > { > + // > + // We only call the Handler if CPU hot-eject is enabled > + // (PcdCpuMaxLogicalProcessorNumber > 1), and hot-eject is needed > + // in this SMI exit (otherwise mCpuHotEjectData->Handler is not armed.) > + // > + > + if (mCpuHotEjectData != NULL) { > + CPU_HOT_EJECT_HANDLER Handler; > + > + Handler = mCpuHotEjectData->Handler; This patch looks otherwise OK to me, but: In patch v8 08/10, we have a ReleaseMemoryFence(). (For now, it is only expressed as a MemoryFence() call; we'll make that more precise later.) (1) I think that should be paired with an AcquireMemoryFence() call, just before loading "mCpuHotEjectData->Handler" above -- for now, also expressed as a MemoryFence() call only. BTW the first article in Paolo's series has been published: https://lwn.net/Articles/844224/ so in terms of that, we have something similar to this diagram: thread 1 thread 2 -------------------------------- ------------------------ a.x = 1; smp_wmb(); WRITE_ONCE(message, &a); datum = READ_ONCE(message); smp_rmb(); if (datum != NULL) printk("%x\n", datum->x); In patch 8, UnplugCpus() does the first two lines of the "thread 1" (BSP) actions, and the third line is covered by the final "AllCpusInSync = FALSE" assignment in BSPHandler() [UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c]. Regarding the thread#2 (AP) actions, line#1 is covered by the "AllCpusInSync loop" near the end of SmiRendezvous(). Lines 3+ are covered by our SmmCpuFeaturesRendezvousExit() implementation here. But line#2 (the AcquireMemoryFence()) is missing. ... I'll suspend the review at this point for today; let's see whether we agree on the comments I've made so far. I hope to continue the review tomorrow. Thanks! Laszlo > + > + if (Handler != NULL) { > + Handler (CpuIndex); > + } > + } > } > > /** >