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.37804.1612200186056077140 for ; Mon, 01 Feb 2021 09:23:06 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=QF2ne3nT; 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=1612200185; 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=/ks6hBEDhpxu2LuPUXxYlepn7V8Pn8EqQSHACAa+DtU=; b=QF2ne3nTk0Agk7B6wB9ikS82St2oqjYkLZojQbsKfAd0LgNO88b5cEJnal2TAHvcu8b6Ci WdQl49N+abrQG1a3Ttuk02A7cmj3fneN5AAnUoFiaBzdq14ZWtKALpGYGBRXxx1NcOLf5s dQzYW1GpGec6dosAlgnjq9DoaWDFY6s= 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-133-47M_nfb6OYOgCP9jrfJmiw-1; Mon, 01 Feb 2021 12:23:01 -0500 X-MC-Unique: 47M_nfb6OYOgCP9jrfJmiw-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 06C118030A3; Mon, 1 Feb 2021 17:23:00 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-114-253.ams2.redhat.com [10.36.114.253]) by smtp.corp.redhat.com (Postfix) with ESMTP id 25CF239A64; Mon, 1 Feb 2021 17:22:57 +0000 (UTC) Subject: Re: [PATCH v6 8/9] OvmfPkg/CpuHotplugSmm: add worker to do CPU ejection To: Ankur Arora , devel@edk2.groups.io Cc: imammedo@redhat.com, boris.ostrovsky@oracle.com, Jordan Justen , Ard Biesheuvel , Aaron Young References: <20210129005950.467638-1-ankur.a.arora@oracle.com> <20210129005950.467638-9-ankur.a.arora@oracle.com> From: "Laszlo Ersek" Message-ID: <2c25a9d0-38dc-754d-fcef-b372e1211a92@redhat.com> Date: Mon, 1 Feb 2021 18:22:57 +0100 MIME-Version: 1.0 In-Reply-To: <20210129005950.467638-9-ankur.a.arora@oracle.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 01/29/21 01:59, Ankur Arora wrote: > Designate a worker CPU (we use the one executing the root MMI > handler), which will do the actual ejection via QEMU in CpuEject(). > > CpuEject(), on the worker CPU, ejects each marked CPU by first > selecting its APIC ID and then sending the QEMU "eject" command. > QEMU in-turn signals the remote VCPU thread which context-switches > it out of the SMI. > > CpuEject(), on the CPU being ejected, spins around in its holding > area until this final context-switch. This does mean that there is > some CPU state that would ordinarily be restored (in SmiRendezvous() > and in SmiEntry.nasm::CommonHandler), but will not be anymore. > This unrestored state includes FPU state, CET enable, stuffing of > RSB and the final RSM. Since the CPU state is destroyed by QEMU, > this should be okay. > > 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 > --- > OvmfPkg/CpuHotplugSmm/CpuHotplug.c | 73 ++++++++++++++++++++++++++++++++++---- > 1 file changed, 67 insertions(+), 6 deletions(-) (1) s/CpuEject/EjectCpu/g, per previous request (affects commit message and code too). > diff --git a/OvmfPkg/CpuHotplugSmm/CpuHotplug.c b/OvmfPkg/CpuHotplugSmm/CpuHotplug.c > index 526f51faf070..bf91344eef9c 100644 > --- a/OvmfPkg/CpuHotplugSmm/CpuHotplug.c > +++ b/OvmfPkg/CpuHotplugSmm/CpuHotplug.c > @@ -193,9 +193,12 @@ 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 neither a worker, nor being ejected, nothing > + to be done. > If, the executing CPU is being ejected, wait in a CpuDeadLoop() > until ejected. > + If, the executing CPU is a worker CPU, set QEMU CPU status to eject > + for CPUs being ejected. > > @param[in] ProcessorNum Index of executing CPU. > > @@ -217,6 +220,56 @@ CpuEject ( > return; > } > > + if (ApicId == CPU_EJECT_WORKER) { (2) The CPU_EJECT_WORKER approach is needlessly complicated (speculative generality). I wish I understood this idea earlier in the patch set. (2a) In patch #5 (subject "OvmfPkg/CpuHotplugSmm: define CPU_HOT_EJECT_DATA"), the CPU_EJECT_WORKER macro definition should be dropped. (2b) In this patch, the question whether the executing CPU is the BSP or not, should be decided with the same logic that is visible in PlatformSmmBspElection() [OvmfPkg/Library/SmmCpuPlatformHookLibQemu/SmmCpuPlatformHookLibQemu.c]: MSR_IA32_APIC_BASE_REGISTER ApicBaseMsr; BOOLEAN IsBsp; ApicBaseMsr.Uint64 = AsmReadMsr64 (MSR_IA32_APIC_BASE); IsBsp = (BOOLEAN)(ApicBaseMsr.Bits.BSP == 1); (2c) Point (2b) obviates the explicit "mark as worker" logic entirely, in UnplugCpus() below. (2d) The "is worker" language (in comments etc) should be replaced with direct "is BSP" language. > + UINT32 CpuIndex; > + > + for (CpuIndex = 0; CpuIndex < mCpuHotEjectData->ArrayLength; CpuIndex++) { > + UINT64 RemoveApicId; > + > + RemoveApicId = mCpuHotEjectData->ApicIdMap[CpuIndex]; > + > + if ((RemoveApicId != CPU_EJECT_INVALID && > + RemoveApicId != CPU_EJECT_WORKER)) { > + // > + // This to-be-ejected-CPU has already received the BSP's SMI exit > + // signal and, will execute SmmCpuFeaturesSmiRendezvousExit() > + // followed by this callback or is already waiting in the > + // CpuDeadLoop() below. > + // > + // Tell QEMU to context-switch it out. > + // > + QemuCpuhpWriteCpuSelector (mMmCpuIo, (APIC_ID) RemoveApicId); > + QemuCpuhpWriteCpuStatus (mMmCpuIo, QEMU_CPUHP_STAT_EJECTED); (3) While the QEMU CPU selector value *usually* matches the APIC ID, it's not an invariant. APIC IDs have an internal structure, composed of bit-fields, where each bit-field accommodates one hierarchy level in the CPU topology (thread, core, die (maybe), and socket). However, this mapping need not be surjective. QEMU lets you create "pathological" CPU topologies, for example one with: - 3 threads/core, - 5 cores/socket, - (say) 2 sockets. Under that example, the bit-field standing for the "thread number" level would have 2 bits, theoretically permitting *4* threads/core, and the bit-field standing for the "core number" level would have 3 bits, theoretically allowing for *8* cores/socket. Considering the fully populated topology, you'd see the CPU selector range from 0 to (3*5*2-1)=29, inclusive (corresponding to 30 logical processors in total). However, the APIC ID *image* of this CPU selector *domain* would not be "contiguous" -- the APIC ID space, with the above-described structure, would accommodate 4*8*2=64 logical processors. For example, each APIC ID that stood for the nonexistent "thread#3" on a particular core would be left unused (no CPU selector would map to it). All in all, you can't write the APIC ID to the CPU selector register, for ejection. You need to select the CPU whose APIC ID is the APIC ID you want to eject, and then initiate ejection. This requires one of two alternatives: (3a) The first option is to keep the change local to this patch. This alternative is the more CPU-hungry (and uglier) one. The idea is to perform a QEMU_CPUHP_CMD_GET_ARCH_ID loop over all possible CPUs, somewhat similarly to QemuCpuhpCollectApicIds(). At every CPU, knowing the APIC ID, try to find the APIC ID in "ApicIdMap". If there is a match, eject. (3b) The second option is much more elegant (and it's faster too), but it requires a much more intrusive update to the patch set. First, the *element type* of the arrays that QemuCpuhpCollectApicIds() operates on, has to be changed from APIC_ID to a structure type that pairs APIC_ID with the QEMU CPU selector. [*] Second, whenever QemuCpuhpCollectApicIds() outputs an APIC_ID, it should also save the "CurrentSelector" value (in the other field of the output array element structure). Third, the element type of CPU_HOT_EJECT_DATA.ApicIdMap should be replaced with a structure type similar (or identical) to the one described at [*]. The ProcessorNumber lookup in UnplugCpus() would still be based upon the APIC ID, but CPU_HOT_EJECT_DATA should remember both the QEMU selector for that processor, and the APIC ID. Fourth, the actual ejection should use the selector. Fifth, the debug message (below) should continue logging the APIC ID, to mirror the DEBUG_INFO message in ProcessHotAddedCpus(). Would you be willing to implement (3b)? > + > + // > + // Compiler barrier to ensure the next store isn't reordered > + // > + MemoryFence (); > + > + // > + // Clear the eject status for CpuIndex to ensure that an invalid > + // SMI later does not end up trying to eject it or a newly > + // hotplugged CpuIndex does not go into the dead loop. > + // > + mCpuHotEjectData->ApicIdMap[CpuIndex] = CPU_EJECT_INVALID; > + > + DEBUG ((DEBUG_INFO, "%a: Unplugged CPU %u -> " FMT_APIC_ID "\n", > + __FUNCTION__, CpuIndex, RemoveApicId)); (4) The DEBUG_INFO log message is in the right place (and uses the right debug mask), but it is afflicted by the usual warts (indentation, format specifiers etc). Please reapply the comments I made elsewhere. (5a) Please replace "CPU" with "ProcessorNumber" (so that we know it's the protocol-assigned number, not the QEMU selector). (5b) Please replace the arrow " -> " with the string " APIC ID ". Thanks! Laszlo > + } > + } > + > + // > + // Clear our own worker status. > + // > + mCpuHotEjectData->ApicIdMap[ProcessorNum] = CPU_EJECT_INVALID; > + > + // > + // We are done until the next hot-unplug; clear the handler. > + // > + mCpuHotEjectData->Handler = NULL; > + return; > + } > + > // > // CPU(s) being unplugged get here from SmmCpuFeaturesSmiRendezvousExit() > // after having been cleared to exit the SMI by the monarch and thus have > @@ -327,6 +380,19 @@ UnplugCpus ( > } > > if (EjectCount != 0) { > + UINTN Worker; > + > + Status = mMmCpuService->WhoAmI (mMmCpuService, &Worker); > + ASSERT_EFI_ERROR (Status); > + // > + // UnplugCpus() is called via the root MMI handler and thus we are > + // executing in the BSP context. > + // > + // Mark ourselves as the worker CPU. > + // > + ASSERT (mCpuHotEjectData->ApicIdMap[Worker] == CPU_EJECT_INVALID); > + mCpuHotEjectData->ApicIdMap[Worker] = CPU_EJECT_WORKER; > + > // > // We have processors to be ejected; install the handler. > // > @@ -451,11 +517,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); >