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.web11.7028.1615894064834611881 for ; Tue, 16 Mar 2021 04:27:45 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=cDx7DYM2; 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=1615894063; 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=haEAWu1jgI09yt2Tj22WQNzlI8bjlLDIsG+P/m83mro=; b=cDx7DYM2DX6q3YzSjJxj2DllrY24oZbcEqpD7/kcHIknlLoaGtsfamP3LHY6dq1M5AZkCR olJ93err0WrEV2gQc52zvbfd7HbL4PtE7A63SfHch03edsLGD9ZVSwKNYgBWrmQmyZUnAH EFnQP51NnyBR3bP7+lHJCUJ59zatPcQ= 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-599-B-fmoKYMNbikq9R75qA0Mg-1; Tue, 16 Mar 2021 07:27:39 -0400 X-MC-Unique: B-fmoKYMNbikq9R75qA0Mg-1 Received: from smtp.corp.redhat.com (int-mx07.intmail.prod.int.phx2.redhat.com [10.5.11.22]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 69D9B107ACCD; Tue, 16 Mar 2021 11:27:38 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-114-138.ams2.redhat.com [10.36.114.138]) by smtp.corp.redhat.com (Postfix) with ESMTP id 67D0710023B5; Tue, 16 Mar 2021 11:27:36 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH v9 09/10] OvmfPkg/CpuHotplugSmm: do actual CPU hot-eject To: devel@edk2.groups.io, ankur.a.arora@oracle.com Cc: imammedo@redhat.com, boris.ostrovsky@oracle.com, Jordan Justen , Ard Biesheuvel , Aaron Young References: <20210312062656.2477515-1-ankur.a.arora@oracle.com> <20210312062656.2477515-10-ankur.a.arora@oracle.com> From: "Laszlo Ersek" Message-ID: <224d9e07-5bed-3cd6-12de-c94a40ccda87@redhat.com> Date: Tue, 16 Mar 2021 12:27:35 +0100 MIME-Version: 1.0 In-Reply-To: <20210312062656.2477515-10-ankur.a.arora@oracle.com> X-Scanned-By: MIMEDefang 2.84 on 10.5.11.22 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/12/21 07:26, Ankur Arora wrote: > Add logic in EjectCpu() to do the actual the CPU ejection. > > On the BSP, ejection happens by first selecting the CPU via > its QemuSelector and then sending the QEMU "eject" command. > QEMU in-turn signals the remote VCPU thread which context-switches > the CPU out of the SMI handler. > > Meanwhile the CPU being ejected, waits around in its holding > area until it is context-switched out. Note that it is possible > that a slow CPU gets ejected before it reaches the wait loop. > However, this would never happen before it has executed the > "AllCpusInSync" loop in SmiRendezvous(). > It can mean that an ejected CPU does not execute code after > that point but given that the CPU state will be destroyed by > QEMU, the missed cleanup is no great loss. > > 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: > Addresses the following comments from v8: > > (1a,1b) CheckIfBsp(): get rid of ProcessorNum, document retval. > (2) Line up IsBsp and ApicBaseMsr > (3) s/ongoing SMI iteration/ongoing SMI/ > (4) Get rid of the allusions to alignment in the comment in EjectCpu(). > () Also reduce some of the repetitive detail in this comment. > (5) EjectCpu(): reorder logic to cleanly separate the AP and the BSP portions. > (6) Get rid of unnecessary MemoryFence() between QemuCpuhpWrite > and clearing of the eject status. > (7) Change type of QemuSelector to %Lu in DEBUG statement > (8) Get rid of the repetitive comment in SmmCpuFeaturesRendezvousExit(). > The necessary parts of this got moved to patch-7. > > OvmfPkg/Include/IndustryStandard/QemuCpuHotplug.h | 1 + > OvmfPkg/CpuHotplugSmm/CpuHotplug.c | 113 ++++++++++++++++++++-- > 2 files changed, 108 insertions(+), 6 deletions(-) Reviewed-by: Laszlo Ersek Thanks Laszlo > > diff --git a/OvmfPkg/Include/IndustryStandard/QemuCpuHotplug.h b/OvmfPkg/Include/IndustryStandard/QemuCpuHotplug.h > index 2ec7a107a64d..d0e83102c13f 100644 > --- a/OvmfPkg/Include/IndustryStandard/QemuCpuHotplug.h > +++ b/OvmfPkg/Include/IndustryStandard/QemuCpuHotplug.h > @@ -34,6 +34,7 @@ > #define QEMU_CPUHP_STAT_ENABLED BIT0 > #define QEMU_CPUHP_STAT_INSERT BIT1 > #define QEMU_CPUHP_STAT_REMOVE BIT2 > +#define QEMU_CPUHP_STAT_EJECT BIT3 > #define QEMU_CPUHP_STAT_FW_REMOVE BIT4 > > #define QEMU_CPUHP_RW_CMD_DATA 0x8 > diff --git a/OvmfPkg/CpuHotplugSmm/CpuHotplug.c b/OvmfPkg/CpuHotplugSmm/CpuHotplug.c > index 2eeb4567a262..ae3abd525900 100644 > --- a/OvmfPkg/CpuHotplugSmm/CpuHotplug.c > +++ b/OvmfPkg/CpuHotplugSmm/CpuHotplug.c > @@ -18,6 +18,7 @@ > #include // CPU_HOT_EJECT_DATA > #include // EFI_MM_CPU_IO_PROTOCOL > #include // EFI_SMM_CPU_SERVICE_PROTOCOL > +#include // MSR_IA32_APIC_BASE_REGISTER > #include // EFI_STATUS > > #include "ApicId.h" // APIC_ID > @@ -193,12 +194,40 @@ RevokeNewSlot: > } > > /** > + EjectCpu needs to know the BSP at SMI exit at a point when > + some of the EFI_SMM_CPU_SERVICE_PROTOCOL state has been torn > + down. > + Reuse the logic from OvmfPkg::PlatformSmmBspElection() to > + do that. > + > + @retval TRUE If the CPU executing this function is the BSP. > + > + @retval FALSE If the CPU executing this function is an AP. > +**/ > +STATIC > +BOOLEAN > +CheckIfBsp ( > + VOID > + ) > +{ > + MSR_IA32_APIC_BASE_REGISTER ApicBaseMsr; > + BOOLEAN IsBsp; > + > + ApicBaseMsr.Uint64 = AsmReadMsr64 (MSR_IA32_APIC_BASE); > + IsBsp = (BOOLEAN)(ApicBaseMsr.Bits.BSP == 1); > + return IsBsp; > +} > + > +/** > CPU Hot-eject handler, called from SmmCpuFeaturesRendezvousExit() > on each CPU at exit from SMM. > > - If, the executing CPU is not being ejected, nothing to be done. > + If, the executing CPU is neither the BSP, nor being ejected, nothing > + to be done. > If, the executing CPU is being ejected, wait in a halted loop > until ejected. > + If, the executing CPU is the BSP, set QEMU CPU status to eject > + for CPUs being ejected. > > @param[in] ProcessorNum ProcessorNum denotes the CPU exiting SMM, > and will be used as an index into > @@ -214,6 +243,83 @@ EjectCpu ( > { > UINT64 QemuSelector; > > + if (CheckIfBsp ()) { > + UINT32 Idx; > + > + for (Idx = 0; Idx < mCpuHotEjectData->ArrayLength; Idx++) { > + UINT64 QemuSelector; > + > + QemuSelector = mCpuHotEjectData->QemuSelectorMap[Idx]; > + > + if (QemuSelector != CPU_EJECT_QEMU_SELECTOR_INVALID) { > + // > + // This to-be-ejected-CPU has already received the BSP's SMI exit > + // signal and will execute SmmCpuFeaturesRendezvousExit() > + // followed by this callback or is already penned in the > + // CpuSleep() loop below. > + // > + // Tell QEMU to context-switch it out. > + // > + QemuCpuhpWriteCpuSelector (mMmCpuIo, (UINT32) QemuSelector); > + QemuCpuhpWriteCpuStatus (mMmCpuIo, QEMU_CPUHP_STAT_EJECT); > + > + // > + // Now that we've ejected the CPU corresponding to QemuSelectorMap[Idx], > + // clear its eject status to ensure that an invalid future SMI does > + // not end up trying a spurious eject or a newly hotplugged CPU does > + // not get penned in the CpuSleep() loop. > + // > + // Note that the QemuCpuhpWriteCpuStatus() command above is a write to > + // a different address space and uses the EFI_MM_CPU_IO_PROTOCOL. > + // > + // This means that we are guaranteed that the following assignment > + // will not be reordered before the eject. And, so we can safely > + // do this write here. > + // > + mCpuHotEjectData->QemuSelectorMap[Idx] = > + CPU_EJECT_QEMU_SELECTOR_INVALID; > + > + DEBUG ((DEBUG_INFO, "%a: Unplugged ProcessorNum %u, " > + "QemuSelector %Lu\n", __FUNCTION__, Idx, QemuSelector)); > + } > + } > + > + // > + // We are done until the next hot-unplug; clear the handler. > + // > + // mCpuHotEjectData->Handler is a NOP for any CPU not under ejection. > + // So, once we are done with all the ejections, we can safely reset it > + // here since any CPU dereferencing it would only see either the old > + // or the new value (since it is aligned at a natural boundary.) > + // > + mCpuHotEjectData->Handler = NULL; > + return; > + } > + > + // > + // Reached only on APs > + // > + > + // > + // mCpuHotEjectData->QemuSelectorMap[ProcessorNum] is updated > + // on the BSP in the ongoing SMI at two places: > + // > + // - UnplugCpus() where the BSP determines if a CPU is under ejection > + // or not. As a comment in UnplugCpus() at set-up, and in > + // SmmCpuFeaturesRendezvousExit() where it is dereferenced describe, > + // any such updates are guaranteed to be ordered-before the > + // dereference below. > + // > + // - EjectCpu() on the BSP (above) updates QemuSelectorMap[ProcessorNum] > + // for a CPU once it's ejected. > + // > + // The CPU under ejection: might be executing anywhere between the > + // AllCpusInSync loop in SmiRendezvous(), to about to dereference > + // QemuSelectorMap[ProcessorNum]. > + // As described in the comment above where we do the reset, this > + // is not a problem since the ejected CPU never sees the after value. > + // CPUs not-under ejection: never see any changes so they are fine. > + // > QemuSelector = mCpuHotEjectData->QemuSelectorMap[ProcessorNum]; > if (QemuSelector == CPU_EJECT_QEMU_SELECTOR_INVALID) { > return; > @@ -495,11 +601,6 @@ CpuHotplugMmi ( > if (EFI_ERROR (Status)) { > goto Fatal; > } > - if (ToUnplugCount > 0) { > - DEBUG ((DEBUG_ERROR, "%a: hot-unplug is not supported yet\n", > - __FUNCTION__)); > - goto Fatal; > - } > > if (PluggedCount > 0) { > Status = ProcessHotAddedCpus (mPluggedApicIds, PluggedCount); >