From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by mx.groups.io with SMTP id smtpd.web11.6524.1615890920905380208 for ; Tue, 16 Mar 2021 03:35:21 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=H4RqiXyX; spf=pass (domain: redhat.com, ip: 170.10.133.124, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1615890919; 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=ha6o4sTfYroRWnjeAPnBEHOO2CNSdC8rIuH9+wNHExw=; b=H4RqiXyXbrfVGB+ndK36B5BLadN5IvE6E+NUscdtUiSSICCPUqhZgTvQYdM3IDgRgue80s iF66K2ucRyXR2+Pm6YRtFfntZW3ZD2vhnAOxm8Q8TlRSUJqMWGU5oYygo5pa6GdGYDIBNq fWKzj/VU9kxkapizz/fY2POe5KfaTC0= 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-224-oKzxbzqBP1qsTAGlWieHSg-1; Tue, 16 Mar 2021 06:35:15 -0400 X-MC-Unique: oKzxbzqBP1qsTAGlWieHSg-1 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id F16D983DD20; Tue, 16 Mar 2021 10:35:13 +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 E8AC35D705; Tue, 16 Mar 2021 10:35:11 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH v9 08/10] OvmfPkg/CpuHotplugSmm: add EjectCpu() 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-9-ankur.a.arora@oracle.com> From: "Laszlo Ersek" Message-ID: <18e7cab1-dfe7-6160-cd46-2c900caf7b61@redhat.com> Date: Tue, 16 Mar 2021 11:35:10 +0100 MIME-Version: 1.0 In-Reply-To: <20210312062656.2477515-9-ankur.a.arora@oracle.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.15 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 EjectCpu(), which handles the CPU ejection, and provides a holding > area for said CPUs. It is called via SmmCpuFeaturesRendezvousExit(), > at the tail end of the SMI handling. > > Also UnplugCpus() now stashes QEMU Selectors of CPUs which need to be > ejected in CPU_HOT_EJECT_DATA.QemuSelectorMap. This is used by > EjectCpu() to identify CPUs marked for ejection. > > 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: > > (1) Fixup the coment about UnplugCpus() to reference stashing QEMU > Cpu Selectors instead of APIC IDs. > (2) s/ToUnplugSelector/ToUnplugSelectors/ > (3) Use plural for APIC ID in comment describing retval EFI_ALREADY_STARTED. > (4) Fixup indentation in check against CPU_EJECT_QEMU_SELECTOR_INVALID. > (5) Clarify comment: > - // never match more than one APIC ID and by transitivity, more than one > - // QemuSelector in a single invocation of UnplugCpus(). > + // never match more than one APIC ID -- nor, by transitivity, designate > + // more than one QemuSelector -- in a single invocation of UnplugCpus(). > (6a) Remove unnecessary UINT64 cast for mCpuHotEjectData->QemuSelectorMap[ProcessorNum]. > (6b) Switch from 0x%Lx -> %Lu for QemuSelectorMap[ProcessorNum]. > (6c) Switch from 0x%Lx -> %u for QemuSelector > (7) Switch to "return EFI_ALREADY_STARTED". > (8a) Replace "QemuSelector 0x%Lx" with "QemuSelector %u". > (8b) Replace the mCpuHotEjectData->QemuSelectorMap[ProcessorNum] argument > with just QemuSelector in DEBUG call. > (9) Clarify comment and make the language complementary to that in patch-7 > Explicitly mention release memory fence. > > OvmfPkg/CpuHotplugSmm/CpuHotplugSmm.inf | 2 + > OvmfPkg/CpuHotplugSmm/CpuHotplug.c | 154 ++++++++++++++++++++++++++++++-- > 2 files changed, 148 insertions(+), 8 deletions(-) Reviewed-by: Laszlo Ersek Thanks Laszlo > diff --git a/OvmfPkg/CpuHotplugSmm/CpuHotplugSmm.inf b/OvmfPkg/CpuHotplugSmm/CpuHotplugSmm.inf > index 04322b0d7855..ebcc7e2ac63a 100644 > --- a/OvmfPkg/CpuHotplugSmm/CpuHotplugSmm.inf > +++ b/OvmfPkg/CpuHotplugSmm/CpuHotplugSmm.inf > @@ -40,6 +40,7 @@ [Packages] > [LibraryClasses] > BaseLib > BaseMemoryLib > + CpuLib > DebugLib > LocalApicLib > MmServicesTableLib > @@ -54,6 +55,7 @@ [Protocols] > > [Pcd] > gUefiCpuPkgTokenSpaceGuid.PcdCpuHotPlugDataAddress ## CONSUMES > + gUefiOvmfPkgTokenSpaceGuid.PcdCpuHotEjectDataAddress ## CONSUMES > gUefiOvmfPkgTokenSpaceGuid.PcdQ35SmramAtDefaultSmbase ## CONSUMES > > [FeaturePcd] > diff --git a/OvmfPkg/CpuHotplugSmm/CpuHotplug.c b/OvmfPkg/CpuHotplugSmm/CpuHotplug.c > index 59f000eb7886..2eeb4567a262 100644 > --- a/OvmfPkg/CpuHotplugSmm/CpuHotplug.c > +++ b/OvmfPkg/CpuHotplugSmm/CpuHotplug.c > @@ -10,10 +10,12 @@ > #include // ICH9_APM_CNT > #include // QEMU_CPUHP_CMD_GET_PENDING > #include // CpuDeadLoop() > +#include // CpuSleep() > #include // ASSERT() > #include // gMmst > #include // PcdGetBool() > #include // SafeUintnSub() > +#include // CPU_HOT_EJECT_DATA > #include // EFI_MM_CPU_IO_PROTOCOL > #include // EFI_SMM_CPU_SERVICE_PROTOCOL > #include // EFI_STATUS > @@ -32,11 +34,12 @@ STATIC EFI_MM_CPU_IO_PROTOCOL *mMmCpuIo; > // > STATIC EFI_SMM_CPU_SERVICE_PROTOCOL *mMmCpuService; > // > -// This structure is a communication side-channel between the > +// These structures serve as communication side-channels between the > // EFI_SMM_CPU_SERVICE_PROTOCOL consumer (i.e., this driver) and provider > // (i.e., PiSmmCpuDxeSmm). > // > STATIC CPU_HOT_PLUG_DATA *mCpuHotPlugData; > +STATIC CPU_HOT_EJECT_DATA *mCpuHotEjectData; > // > // SMRAM arrays for fetching the APIC IDs of processors with pending events (of > // known event types), for the time of just one MMI. > @@ -190,18 +193,71 @@ RevokeNewSlot: > } > > /** > + 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 being ejected, wait in a halted loop > + until ejected. > + > + @param[in] ProcessorNum ProcessorNum denotes the CPU exiting SMM, > + and will be used as an index into > + CPU_HOT_EJECT_DATA->QemuSelectorMap. It is > + identical to the processor handle number in > + EFI_SMM_CPU_SERVICE_PROTOCOL. > +**/ > +VOID > +EFIAPI > +EjectCpu ( > + IN UINTN ProcessorNum > + ) > +{ > + UINT64 QemuSelector; > + > + QemuSelector = mCpuHotEjectData->QemuSelectorMap[ProcessorNum]; > + if (QemuSelector == CPU_EJECT_QEMU_SELECTOR_INVALID) { > + return; > + } > + > + // > + // APs being unplugged get here from SmmCpuFeaturesRendezvousExit() > + // after having been cleared to exit the SMI and so have no SMM > + // processing remaining. > + // > + // Keep them penned here until the BSP tells QEMU to eject them. > + // > + for (;;) { > + DisableInterrupts (); > + CpuSleep (); > + } > +} > + > +/** > Process to be hot-unplugged CPUs, per QemuCpuhpCollectApicIds(). > > For each such CPU, report the CPU to PiSmmCpuDxeSmm via > - EFI_SMM_CPU_SERVICE_PROTOCOL. If the to be hot-unplugged CPU is > - unknown, skip it silently. > + EFI_SMM_CPU_SERVICE_PROTOCOL and stash the QEMU Cpu Selectors for later > + ejection. If the to be hot-unplugged CPU is unknown, skip it silently. > + > + Additonally, if we do stash any Cpu Selectors, also install a CPU eject > + handler which would handle the ejection. > > @param[in] ToUnplugApicIds The APIC IDs of the CPUs that are about to be > hot-unplugged. > > + @param[in] ToUnplugSelectors The QEMU Selectors of the CPUs that are about to > + be hot-unplugged. > + > @param[in] ToUnplugCount The number of filled-in APIC IDs in > ToUnplugApicIds. > > + @retval EFI_ALREADY_STARTED For the ProcessorNum that > + EFI_SMM_CPU_SERVICE_PROTOCOL had assigned to > + one of the APIC IDs in ToUnplugApicIds, > + mCpuHotEjectData->QemuSelectorMap already has > + the QemuSelector value stashed. (This should > + never happen.) > + > @retval EFI_SUCCESS Known APIC IDs have been removed from SMM data > structures. > > @@ -212,23 +268,36 @@ STATIC > EFI_STATUS > UnplugCpus ( > IN APIC_ID *ToUnplugApicIds, > + IN UINT32 *ToUnplugSelectors, > IN UINT32 ToUnplugCount > ) > { > EFI_STATUS Status; > UINT32 ToUnplugIdx; > + UINT32 EjectCount; > UINTN ProcessorNum; > > ToUnplugIdx = 0; > + EjectCount = 0; > while (ToUnplugIdx < ToUnplugCount) { > APIC_ID RemoveApicId; > + UINT32 QemuSelector; > > RemoveApicId = ToUnplugApicIds[ToUnplugIdx]; > + QemuSelector = ToUnplugSelectors[ToUnplugIdx]; > > // > - // mCpuHotPlugData->ApicId maps ProcessorNum -> ApicId. Use it to find > - // the ProcessorNum for the APIC ID to be removed. > + // mCpuHotPlugData->ApicId maps ProcessorNum -> ApicId. Use RemoveApicId > + // to find the corresponding ProcessorNum for the CPU to be removed. > // > + // With this we can establish a 3 way mapping: > + // APIC_ID -- ProcessorNum -- QemuSelector > + // > + // We stash the ProcessorNum -> QemuSelector mapping so it can later be > + // used for CPU hot-eject in SmmCpuFeaturesRendezvousExit() context (where > + // we only have ProcessorNum available.) > + // > + > for (ProcessorNum = 0; > ProcessorNum < mCpuHotPlugData->ArrayLength; > ProcessorNum++) { > @@ -257,11 +326,62 @@ UnplugCpus ( > return Status; > } > > + if (mCpuHotEjectData->QemuSelectorMap[ProcessorNum] != > + CPU_EJECT_QEMU_SELECTOR_INVALID) { > + // > + // mCpuHotEjectData->QemuSelectorMap[ProcessorNum] is set to > + // CPU_EJECT_QEMU_SELECTOR_INVALID when mCpuHotEjectData->QemuSelectorMap > + // is allocated, and once the subject processsor is ejected. > + // > + // Additionally, mMmCpuService->RemoveProcessor(ProcessorNum) invalidates > + // mCpuHotPlugData->ApicId[ProcessorNum], so a given ProcessorNum can > + // never match more than one APIC ID -- nor, by transitivity, designate > + // more than one QemuSelector -- in a single invocation of UnplugCpus(). > + // > + DEBUG ((DEBUG_ERROR, "%a: ProcessorNum %Lu maps to QemuSelector %Lu, " > + "cannot also map to %u\n", __FUNCTION__, (UINT64)ProcessorNum, > + mCpuHotEjectData->QemuSelectorMap[ProcessorNum], QemuSelector)); > + > + return EFI_ALREADY_STARTED; > + } > + > + // > + // Stash the QemuSelector so we can do the actual ejection later. > + // > + mCpuHotEjectData->QemuSelectorMap[ProcessorNum] = (UINT64)QemuSelector; > + > + DEBUG ((DEBUG_INFO, "%a: Started hot-unplug on ProcessorNum %Lu, APIC ID " > + FMT_APIC_ID ", QemuSelector %u\n", __FUNCTION__, (UINT64)ProcessorNum, > + RemoveApicId, QemuSelector)); > + > + EjectCount++; > ToUnplugIdx++; > } > > + if (EjectCount != 0) { > + // > + // We have processors to be ejected; install the handler. > + // > + mCpuHotEjectData->Handler = EjectCpu; > + > + // > + // The BSP and APs load mCpuHotEjectData->Handler, and > + // mCpuHotEjectData->QemuSelectorMap[] in SmmCpuFeaturesRendezvousExit() > + // and EjectCpu(). > + // > + // The comment in SmmCpuFeaturesRendezvousExit() details how we use > + // the AllCpusInSync control-dependency to ensure that any loads are > + // ordered-after the stores above. > + // > + // Ensure that the stores above are ordered-before the AllCpusInSync store > + // by using a MemoryFence() with release semantics. > + // > + MemoryFence (); > + } > + > // > - // We've removed this set of APIC IDs from SMM data structures. > + // We've removed this set of APIC IDs from SMM data structures and > + // have installed an ejection handler if needed. > // > return EFI_SUCCESS; > } > @@ -389,7 +509,7 @@ CpuHotplugMmi ( > } > > if (ToUnplugCount > 0) { > - Status = UnplugCpus (mToUnplugApicIds, ToUnplugCount); > + Status = UnplugCpus (mToUnplugApicIds, mToUnplugSelectors, ToUnplugCount); > if (EFI_ERROR (Status)) { > goto Fatal; > } > @@ -460,9 +580,14 @@ CpuHotplugEntry ( > > // > // Our DEPEX on EFI_SMM_CPU_SERVICE_PROTOCOL guarantees that PiSmmCpuDxeSmm > - // has pointed PcdCpuHotPlugDataAddress to CPU_HOT_PLUG_DATA in SMRAM. > + // has pointed: > + // - PcdCpuHotPlugDataAddress to CPU_HOT_PLUG_DATA in SMRAM, > + // - PcdCpuHotEjectDataAddress to CPU_HOT_EJECT_DATA in SMRAM, if the > + // possible CPU count is greater than 1. > // > mCpuHotPlugData = (VOID *)(UINTN)PcdGet64 (PcdCpuHotPlugDataAddress); > + mCpuHotEjectData = (VOID *)(UINTN)PcdGet64 (PcdCpuHotEjectDataAddress); > + > if (mCpuHotPlugData == NULL) { > Status = EFI_NOT_FOUND; > DEBUG ((DEBUG_ERROR, "%a: CPU_HOT_PLUG_DATA: %r\n", __FUNCTION__, Status)); > @@ -474,6 +599,19 @@ CpuHotplugEntry ( > if (mCpuHotPlugData->ArrayLength == 1) { > return EFI_UNSUPPORTED; > } > + > + if (mCpuHotEjectData == NULL) { > + Status = EFI_NOT_FOUND; > + } else if (mCpuHotPlugData->ArrayLength != mCpuHotEjectData->ArrayLength) { > + Status = EFI_INVALID_PARAMETER; > + } else { > + Status = EFI_SUCCESS; > + } > + if (EFI_ERROR (Status)) { > + DEBUG ((DEBUG_ERROR, "%a: CPU_HOT_EJECT_DATA: %r\n", __FUNCTION__, Status)); > + goto Fatal; > + } > + > // > // Allocate the data structures that depend on the possible CPU count. > // >