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.web10.6059.1615888592007807062 for ; Tue, 16 Mar 2021 02:56:33 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=b6edR9CO; 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=1615888591; 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=uuI21/k5KrGQq7b9w3URSQ/scmg8No1XBBocYN0EQZU=; b=b6edR9COGocBaLvubZ9mVRmjs9xQc4dri6EgzXEintOI365R7GzVEIeSpZGtYWQaD28qrL DprlIiAYZSxdkesfAVuVRKyA5aRQxr7Vvu/OX5SvllNU+mthU/7ToaP5RgtMjDSyMDqG+1 qEGnBGDONRa9nEjRcl5RGBOBFdYE+34= 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-531-o8pFNBehO-uFMUiTdAuN8Q-1; Tue, 16 Mar 2021 05:56:27 -0400 X-MC-Unique: o8pFNBehO-uFMUiTdAuN8Q-1 Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.14]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id ABC24800C78; Tue, 16 Mar 2021 09:56:25 +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 A443E5D9C0; Tue, 16 Mar 2021 09:56:23 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH v9 02/10] OvmfPkg/CpuHotplugSmm: collect hot-unplug events 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-3-ankur.a.arora@oracle.com> From: "Laszlo Ersek" Message-ID: <9a7972f9-70ba-8594-1f2e-f3282a4212c6@redhat.com> Date: Tue, 16 Mar 2021 10:56:22 +0100 MIME-Version: 1.0 In-Reply-To: <20210312062656.2477515-3-ankur.a.arora@oracle.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.14 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: > Process fw_remove events in QemuCpuhpCollectApicIds(), and collect APIC IDs > and QEMU CPU Selectors for CPUs being hot-unplugged. > > In addition, we now ignore CPUs which only have remove set. These > CPUs haven't been processed by OSPM yet. > > This is based on the QEMU hot-unplug protocol documented here: > https://lore.kernel.org/qemu-devel/20201204170939.1815522-3-imammedo@redhat.com/ > > 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) Fix commit message to mention that we collect cpu-selectors as well. > (2,3,6) s/UnplugSelector/UnplugSelectors/ in CpuHotplug.c, QemuCpuhp.c > (4) Fix comment above the declaration of the now renamed mToUnplugSelector. > (5) Fix spacing around "||". > (7) Fix QemuCpuCollectApicIds() comments to line up descriptions for > ToUnplugSelectors and other params. > (8) s/ExtendSel/ExtendSels/. > (9) Add the (ExtendSels => ExtendIds) assert. > (10) Fix the missing CurrentSelector++ bug. > > OvmfPkg/CpuHotplugSmm/QemuCpuhp.h | 1 + > OvmfPkg/Include/IndustryStandard/QemuCpuHotplug.h | 1 + > OvmfPkg/CpuHotplugSmm/CpuHotplug.c | 29 +++++-- > OvmfPkg/CpuHotplugSmm/QemuCpuhp.c | 101 +++++++++++++++------- > 4 files changed, 93 insertions(+), 39 deletions(-) Reviewed-by: Laszlo Ersek Thanks Laszlo > > diff --git a/OvmfPkg/CpuHotplugSmm/QemuCpuhp.h b/OvmfPkg/CpuHotplugSmm/QemuCpuhp.h > index 8adaa0ad91f0..3e2c2192e1c0 100644 > --- a/OvmfPkg/CpuHotplugSmm/QemuCpuhp.h > +++ b/OvmfPkg/CpuHotplugSmm/QemuCpuhp.h > @@ -55,6 +55,7 @@ QemuCpuhpCollectApicIds ( > OUT APIC_ID *PluggedApicIds, > OUT UINT32 *PluggedCount, > OUT APIC_ID *ToUnplugApicIds, > + OUT UINT32 *ToUnplugSelectors, > OUT UINT32 *ToUnplugCount > ); > > diff --git a/OvmfPkg/Include/IndustryStandard/QemuCpuHotplug.h b/OvmfPkg/Include/IndustryStandard/QemuCpuHotplug.h > index a34a6d3fae61..2ec7a107a64d 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_FW_REMOVE BIT4 > > #define QEMU_CPUHP_RW_CMD_DATA 0x8 > > diff --git a/OvmfPkg/CpuHotplugSmm/CpuHotplug.c b/OvmfPkg/CpuHotplugSmm/CpuHotplug.c > index bf68fcd42914..ee1497b93140 100644 > --- a/OvmfPkg/CpuHotplugSmm/CpuHotplug.c > +++ b/OvmfPkg/CpuHotplugSmm/CpuHotplug.c > @@ -45,13 +45,16 @@ STATIC CPU_HOT_PLUG_DATA *mCpuHotPlugData; > // don't want to allocate SMRAM at OS runtime, and potentially fail (or > // fragment the SMRAM map). > // > -// These arrays provide room for ("possible CPU count" minus one) APIC IDs > -// each, as we don't expect every possible CPU to appear, or disappear, in a > -// single MMI. The numbers of used (populated) elements in the arrays are > +// The first array stores APIC IDs for hot-plug events, the second and the > +// third store APIC IDs and QEMU CPU Selectors (both indexed similarly) for > +// hot-unplug events. All of these provide room for "possible CPU count" minus > +// one elements as we don't expect every possible CPU to appear, or disappear, > +// in a single MMI. The numbers of used (populated) elements in the arrays are > // determined on every MMI separately. > // > STATIC APIC_ID *mPluggedApicIds; > STATIC APIC_ID *mToUnplugApicIds; > +STATIC UINT32 *mToUnplugSelectors; > // > // Address of the non-SMRAM reserved memory page that contains the Post-SMM Pen > // for hot-added CPUs. > @@ -289,6 +292,7 @@ CpuHotplugMmi ( > mPluggedApicIds, > &PluggedCount, > mToUnplugApicIds, > + mToUnplugSelectors, > &ToUnplugCount > ); > if (EFI_ERROR (Status)) { > @@ -333,7 +337,9 @@ CpuHotplugEntry ( > ) > { > EFI_STATUS Status; > + UINTN Len; > UINTN Size; > + UINTN SizeSel; > > // > // This module should only be included when SMM support is required. > @@ -387,8 +393,9 @@ CpuHotplugEntry ( > // > // Allocate the data structures that depend on the possible CPU count. > // > - if (RETURN_ERROR (SafeUintnSub (mCpuHotPlugData->ArrayLength, 1, &Size)) || > - RETURN_ERROR (SafeUintnMult (sizeof (APIC_ID), Size, &Size))) { > + if (RETURN_ERROR (SafeUintnSub (mCpuHotPlugData->ArrayLength, 1, &Len)) || > + RETURN_ERROR (SafeUintnMult (sizeof (APIC_ID), Len, &Size)) || > + RETURN_ERROR (SafeUintnMult (sizeof (UINT32), Len, &SizeSel))) { > Status = EFI_ABORTED; > DEBUG ((DEBUG_ERROR, "%a: invalid CPU_HOT_PLUG_DATA\n", __FUNCTION__)); > goto Fatal; > @@ -405,6 +412,12 @@ CpuHotplugEntry ( > DEBUG ((DEBUG_ERROR, "%a: MmAllocatePool(): %r\n", __FUNCTION__, Status)); > goto ReleasePluggedApicIds; > } > + Status = gMmst->MmAllocatePool (EfiRuntimeServicesData, SizeSel, > + (VOID **)&mToUnplugSelectors); > + if (EFI_ERROR (Status)) { > + DEBUG ((DEBUG_ERROR, "%a: MmAllocatePool(): %r\n", __FUNCTION__, Status)); > + goto ReleaseToUnplugApicIds; > + } > > // > // Allocate the Post-SMM Pen for hot-added CPUs. > @@ -412,7 +425,7 @@ CpuHotplugEntry ( > Status = SmbaseAllocatePostSmmPen (&mPostSmmPenAddress, > SystemTable->BootServices); > if (EFI_ERROR (Status)) { > - goto ReleaseToUnplugApicIds; > + goto ReleaseToUnplugSelectors; > } > > // > @@ -472,6 +485,10 @@ ReleasePostSmmPen: > SmbaseReleasePostSmmPen (mPostSmmPenAddress, SystemTable->BootServices); > mPostSmmPenAddress = 0; > > +ReleaseToUnplugSelectors: > + gMmst->MmFreePool (mToUnplugSelectors); > + mToUnplugSelectors = NULL; > + > ReleaseToUnplugApicIds: > gMmst->MmFreePool (mToUnplugApicIds); > mToUnplugApicIds = NULL; > diff --git a/OvmfPkg/CpuHotplugSmm/QemuCpuhp.c b/OvmfPkg/CpuHotplugSmm/QemuCpuhp.c > index 8d4a6693c8d6..8434dd446b96 100644 > --- a/OvmfPkg/CpuHotplugSmm/QemuCpuhp.c > +++ b/OvmfPkg/CpuHotplugSmm/QemuCpuhp.c > @@ -145,27 +145,30 @@ QemuCpuhpWriteCommand ( > > On error, the contents of the output parameters are undefined. > > - @param[in] MmCpuIo The EFI_MM_CPU_IO_PROTOCOL instance for > - accessing IO Ports. > + @param[in] MmCpuIo The EFI_MM_CPU_IO_PROTOCOL instance for > + accessing IO Ports. > > - @param[in] PossibleCpuCount The number of possible CPUs in the system. Must > - be positive. > + @param[in] PossibleCpuCount The number of possible CPUs in the system. Must > + be positive. > > - @param[in] ApicIdCount The number of elements each one of the > - PluggedApicIds and ToUnplugApicIds arrays can > - accommodate. Must be positive. > + @param[in] ApicIdCount The number of elements each one of the > + PluggedApicIds and ToUnplugApicIds arrays can > + accommodate. Must be positive. > > - @param[out] PluggedApicIds The APIC IDs of the CPUs that have been > - hot-plugged. > + @param[out] PluggedApicIds The APIC IDs of the CPUs that have been > + hot-plugged. > > - @param[out] PluggedCount The number of filled-in APIC IDs in > - PluggedApicIds. > + @param[out] PluggedCount The number of filled-in APIC IDs in > + PluggedApicIds. > > - @param[out] ToUnplugApicIds The APIC IDs of the CPUs that are about to be > - hot-unplugged. > + @param[out] ToUnplugApicIds The APIC IDs of the CPUs that are about to be > + hot-unplugged. > > - @param[out] ToUnplugCount The number of filled-in APIC IDs in > - ToUnplugApicIds. > + @param[out] ToUnplugSelectors The QEMU Selectors of the CPUs that are about > + to be hot-unplugged. > + > + @param[out] ToUnplugCount The number of filled-in APIC IDs in > + ToUnplugApicIds. > > @retval EFI_INVALID_PARAMETER PossibleCpuCount is zero, or ApicIdCount is > zero. > @@ -187,6 +190,7 @@ QemuCpuhpCollectApicIds ( > OUT APIC_ID *PluggedApicIds, > OUT UINT32 *PluggedCount, > OUT APIC_ID *ToUnplugApicIds, > + OUT UINT32 *ToUnplugSelectors, > OUT UINT32 *ToUnplugCount > ) > { > @@ -204,6 +208,7 @@ QemuCpuhpCollectApicIds ( > UINT32 PendingSelector; > UINT8 CpuStatus; > APIC_ID *ExtendIds; > + UINT32 *ExtendSels; > UINT32 *ExtendCount; > APIC_ID NewApicId; > > @@ -245,10 +250,10 @@ QemuCpuhpCollectApicIds ( > if ((CpuStatus & QEMU_CPUHP_STAT_INSERT) != 0) { > // > // The "insert" event guarantees the "enabled" status; plus it excludes > - // the "remove" event. > + // the "fw_remove" event. > // > if ((CpuStatus & QEMU_CPUHP_STAT_ENABLED) == 0 || > - (CpuStatus & QEMU_CPUHP_STAT_REMOVE) != 0) { > + (CpuStatus & QEMU_CPUHP_STAT_FW_REMOVE) != 0) { > DEBUG ((DEBUG_ERROR, "%a: CurrentSelector=%u CpuStatus=0x%x: " > "inconsistent CPU status\n", __FUNCTION__, CurrentSelector, > CpuStatus)); > @@ -259,33 +264,63 @@ QemuCpuhpCollectApicIds ( > CurrentSelector)); > > ExtendIds = PluggedApicIds; > + ExtendSels = NULL; > ExtendCount = PluggedCount; > - } else if ((CpuStatus & QEMU_CPUHP_STAT_REMOVE) != 0) { > - DEBUG ((DEBUG_VERBOSE, "%a: CurrentSelector=%u: remove\n", __FUNCTION__, > - CurrentSelector)); > + } else if ((CpuStatus & QEMU_CPUHP_STAT_FW_REMOVE) != 0) { > + // > + // "fw_remove" event guarantees "enabled". > + // > + if ((CpuStatus & QEMU_CPUHP_STAT_ENABLED) == 0) { > + DEBUG ((DEBUG_ERROR, "%a: CurrentSelector=%u CpuStatus=0x%x: " > + "inconsistent CPU status\n", __FUNCTION__, CurrentSelector, > + CpuStatus)); > + return EFI_PROTOCOL_ERROR; > + } > + > + DEBUG ((DEBUG_VERBOSE, "%a: CurrentSelector=%u: fw_remove\n", > + __FUNCTION__, CurrentSelector)); > > ExtendIds = ToUnplugApicIds; > + ExtendSels = ToUnplugSelectors; > ExtendCount = ToUnplugCount; > + } else if ((CpuStatus & QEMU_CPUHP_STAT_REMOVE) != 0) { > + // > + // Let the OSPM deal with the "remove" event. > + // > + DEBUG ((DEBUG_VERBOSE, "%a: CurrentSelector=%u: remove (ignored)\n", > + __FUNCTION__, CurrentSelector)); > + > + ExtendIds = NULL; > + ExtendSels = NULL; > + ExtendCount = NULL; > } else { > DEBUG ((DEBUG_VERBOSE, "%a: CurrentSelector=%u: no event\n", > __FUNCTION__, CurrentSelector)); > break; > } > > - // > - // Save the APIC ID of the CPU with the pending event, to the corresponding > - // APIC ID array. > - // > - if (*ExtendCount == ApicIdCount) { > - DEBUG ((DEBUG_ERROR, "%a: APIC ID array too small\n", __FUNCTION__)); > - return EFI_BUFFER_TOO_SMALL; > - } > - QemuCpuhpWriteCommand (MmCpuIo, QEMU_CPUHP_CMD_GET_ARCH_ID); > - NewApicId = QemuCpuhpReadCommandData (MmCpuIo); > - DEBUG ((DEBUG_VERBOSE, "%a: ApicId=" FMT_APIC_ID "\n", __FUNCTION__, > - NewApicId)); > - ExtendIds[(*ExtendCount)++] = NewApicId; > + ASSERT ((ExtendIds == NULL) == (ExtendCount == NULL)); > + ASSERT ((ExtendSels == NULL) || (ExtendIds != NULL)); > > + if (ExtendIds != NULL) { > + // > + // Save the APIC ID of the CPU with the pending event, to the > + // corresponding APIC ID array. > + // For unplug events, also save the CurrentSelector. > + // > + if (*ExtendCount == ApicIdCount) { > + DEBUG ((DEBUG_ERROR, "%a: APIC ID array too small\n", __FUNCTION__)); > + return EFI_BUFFER_TOO_SMALL; > + } > + QemuCpuhpWriteCommand (MmCpuIo, QEMU_CPUHP_CMD_GET_ARCH_ID); > + NewApicId = QemuCpuhpReadCommandData (MmCpuIo); > + DEBUG ((DEBUG_VERBOSE, "%a: ApicId=" FMT_APIC_ID "\n", __FUNCTION__, > + NewApicId)); > + if (ExtendSels != NULL) { > + ExtendSels[(*ExtendCount)] = CurrentSelector; > + } > + ExtendIds[(*ExtendCount)++] = NewApicId; > + } > // > // We've processed the CPU with (known) pending events, but we must never > // clear events. Therefore we need to advance past this CPU manually; >