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.web12.2550.1614112764632287952 for ; Tue, 23 Feb 2021 12:39:24 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=Q5mx6b+G; 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=1614112763; 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=RYXxJEPtq4ze5kS+cLJTaGkCwiXm0SgN9ip3//oBqbQ=; b=Q5mx6b+GGRpBw4CmyUB1D1fns9B1VhpG4Krf9fHttCMpGrznuZQ+mu0A96OkVJdvwJbV0E z95c0kzm3Hk/4xSj4tLNfAUJTjAUGszsYyaz7aL00B/3/wsyqEo2M35y9zgv/nK7a2fDyV ZfCkzWEkR1m66n9wWhf48lbBWT9Uuoc= 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-237-4YboYEBmNieQFLsb1Z_uGQ-1; Tue, 23 Feb 2021 15:39:19 -0500 X-MC-Unique: 4YboYEBmNieQFLsb1Z_uGQ-1 Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 3BF776D515; Tue, 23 Feb 2021 20:36:38 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-113-109.ams2.redhat.com [10.36.113.109]) by smtp.corp.redhat.com (Postfix) with ESMTP id 434B760C04; Tue, 23 Feb 2021 20:36:35 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH v8 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: <20210222071928.1401820-1-ankur.a.arora@oracle.com> <20210222071928.1401820-9-ankur.a.arora@oracle.com> From: "Laszlo Ersek" Message-ID: <6e37922f-afbe-2b41-4eb9-4b3c62d28aa9@redhat.com> Date: Tue, 23 Feb 2021 21:36:35 +0100 MIME-Version: 1.0 In-Reply-To: <20210222071928.1401820-9-ankur.a.arora@oracle.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.12 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 superficial comments only; the patch is nearly ready: On 02/22/21 08:19, 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: > Address these review comments from v6: > (1) s/CpuEject/EjectCpu/g > (2) Ensure that the added include is in sorted order. > (3) Switch to a cheaper CpuSleep() based loop instead of > CpuDeadLoop(). Also add the CpuLib LibraryClass. > (4) Remove the nested else clause > (5) Use Laszlo's much clearer comment when we try to map multiple > QemuSelector to the same ProcessorNum. > (6a) Fix indentation of the debug print in the block in (5). > (6b,6c,6d) Fix printf types for ProcessorNum, use FMT_APIC_ID for > APIC_ID and 0x%Lx for QemuSelector[]. > () As discussed elsewhere add an DEBUG_INFO print logging the > correspondence between ProcessorNum, APIC_ID, QemuSelector. > (7a,7b) Use EFI_ALREADY_STARTED instead of EFI_INVALID_PARAMETER and > document it in the UnplugCpus() comment block. > () As discussed elsewhere, add the import statement for > PcdCpuHotEjectDataAddress. > (9) Use Laszlo's comment in the PcdGet64(PcdCpuHotEjectDataAddress) > description block. > (10) Change mCpuHotEjectData init state checks from ASSERT to ones > consistent with similar checks for mCpuHotPlugData. > (11-14) Get rid of mCpuHotEjectData init loop: moved to a prior > patch so it can be done at allocation time. > (15) s/SmmCpuFeaturesSmiRendezvousExit/SmmCpuFeaturesRendezvousExit/ > (16,17) Document the ordering requirements of > mCpuHotEjectData->Handler, and mCpuHotEjectData->QemuSelectorMap. > > Not addressed: > (8) Not removing the EjectCount variable as I'd like to minimize > stores/loads to CPU_HOT_EJECT_DATA->Handler and so would like to do this > a single time at the end of the iteration. (It is safe to write multiple > times to the handler in UnplugCpus() but given the ordering concerns > around it, it seems cleaner to not access it unnecessarily.) > > OvmfPkg/CpuHotplugSmm/CpuHotplugSmm.inf | 2 + > OvmfPkg/CpuHotplugSmm/CpuHotplug.c | 157 ++++++++++++++++++++++++++++++-- > 2 files changed, 151 insertions(+), 8 deletions(-) > > 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 f07b5072749a..851e2b28aad9 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. > @@ -188,18 +191,72 @@ 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; > + } > + > + // > + // CPU(s) being unplugged get here from SmmCpuFeaturesRendezvousExit() > + // after having been cleared to exit the SMI by the BSP and thus have > + // no SMM processing remaining. > + // > + // Given that we cannot allow them to escape to the guest, we pen them > + // here until the BSP tells QEMU to unplug 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 APIC ID for later ejection. > + If the to be hot-unplugged CPU is unknown, skip it silently. > + > + Additonally, if we do stash any APIC IDs, also install a CPU eject handler > + which would handle the ejection. (1) Please update the two APIC ID references above to QEMU CPU selectors (the commit message has been updated already, correctly). > > @param[in] ToUnplugApicIds The APIC IDs of the CPUs that are about to be > hot-unplugged. > > + @param[in] ToUnplugSelector The QEMU Selectors of the CPUs that are about to > + be hot-unplugged. > + (2) Please rename the parameter to "ToUnplugSelectors" (plural), both here in the comment and in the actual parameter list. > @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 ID in ToUnplugApicIds, > + mCpuHotEjectData->QemuSelectorMap already has > + the QemuSelector value stashed. (This should > + never happen.) > + (3) Unfortunately I made a typing error in my v6 review where I suggested this language: it should say one of the APIC ID*s* in ToUnplugApicIds (emphasis only here, in this comment) > @retval EFI_SUCCESS Known APIC IDs have been removed from SMM data > structures. > > @@ -210,23 +267,36 @@ STATIC > EFI_STATUS > UnplugCpus ( > IN APIC_ID *ToUnplugApicIds, > + IN UINT32 *ToUnplugSelector, > 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 = ToUnplugSelector[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++) { > @@ -255,11 +325,64 @@ UnplugCpus ( > return Status; > } > > + if (mCpuHotEjectData->QemuSelectorMap[ProcessorNum] != > + CPU_EJECT_QEMU_SELECTOR_INVALID) { (4) Invalid indentation; in this (controlling expression) context, CPU_EJECT_QEMU_SELECTOR_INVALID should line up with "mCpuHotEjectData". > + // > + // 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 and by transitivity, more than one > + // QemuSelector in a single invocation of UnplugCpus(). > + // (5) good comment, but please make it a tiny bit easier to read: please insert two "em dashes", as follows: // never match more than one APIC ID -- and by transitivity, designate // more than one QemuSelector -- in a single invocation of UnplugCpus(). (I've also inserted the verb "designate", because "match" doesn't seem to apply in that context.) > + DEBUG ((DEBUG_ERROR, "%a: ProcessorNum %Lu maps to QemuSelector 0x%Lx, " > + "cannot also map to 0x%Lx\n", __FUNCTION__, (UINT64)ProcessorNum, > + (UINT64)mCpuHotEjectData->QemuSelectorMap[ProcessorNum], QemuSelector)); (6a) "mCpuHotEjectData->QemuSelectorMap[ProcessorNum]" has type UINT64, so the cast is superfluous (6b) we log selectors in all other places in decimal, so please use %Lu here, not 0x%Lx, for logging "mCpuHotEjectData->QemuSelectorMap[ProcessorNum]" (6c) "QemuSelector" has type UINT32 (correctly), so we need to log it with either "0x%x" or "%u". Given my above point about logging selectors in decimal everywhere else, please use "%u". DEBUG ((DEBUG_ERROR, "%a: ProcessorNum %Lu maps to QemuSelector %Lu, " "cannot also map to %u\n", __FUNCTION__, (UINT64)ProcessorNum, mCpuHotEjectData->QemuSelectorMap[ProcessorNum], QemuSelector)); > + > + Status = EFI_ALREADY_STARTED; > + return Status; > + } (7) style nit -- this looks better as "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 0x%Lx\n", __FUNCTION__, (UINT64)ProcessorNum, > + RemoveApicId, mCpuHotEjectData->QemuSelectorMap[ProcessorNum])); (8a) In the format string, please replace "QemuSelector 0x%Lx" with "QemuSelector %u" -- the main point is the decimal notation (8b) As a suggestion, I think we should simplify this DEBUG call by replacing the "mCpuHotEjectData->QemuSelectorMap[ProcessorNum]" argument with just "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++; (I'm fine with keeping the "EjectCount" variable.) > ToUnplugIdx++; > } > > + if (EjectCount != 0) { > + // > + // We have processors to be ejected; install the handler. > + // > + mCpuHotEjectData->Handler = EjectCpu; > + > + // > + // The BSP, CPUs to be ejected dereference mCpuHotEjectData->Handler, and > + // mCpuHotEjectData->QemuSelectorMap[] in SmmCpuFeaturesRendezvousExit(). > + // > + // Assignments to both of these are ordered-before the BSP's SMI exit signal > + // which happens via a write to SMM_DISPATCHER_MP_SYNC_DATA->AllCpusInSync. > + // Dereferences of both are ordered-after the synchronization via > + // "AllCpusInSync". > + // > + // So we are guaranteed that the Handler would see the assignments above. > + // However, add a MemoryFence() here in-lieu of a compiler barrier to > + // ensure that the compiler doesn't monkey around with the stores. > + // > + MemoryFence (); > + } > + (9) Per previous discussion (under patch v8 #7), please replace the last paragraph of this comment, with a "ReleaseMemoryFence" reference. The rest looks good. Thanks! Laszlo > // > - // 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; > } > @@ -387,7 +510,7 @@ CpuHotplugMmi ( > } > > if (ToUnplugCount > 0) { > - Status = UnplugCpus (mToUnplugApicIds, ToUnplugCount); > + Status = UnplugCpus (mToUnplugApicIds, mToUnplugSelector, ToUnplugCount); > if (EFI_ERROR (Status)) { > goto Fatal; > } > @@ -458,9 +581,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)); > @@ -472,6 +600,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. > // >