From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wm1-f67.google.com (mail-wm1-f67.google.com [209.85.128.67]) by mx.groups.io with SMTP id smtpd.web11.14268.1583156997313233743 for ; Mon, 02 Mar 2020 05:49:57 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@linaro.org header.s=google header.b=OQqlob1x; spf=pass (domain: linaro.org, ip: 209.85.128.67, mailfrom: ard.biesheuvel@linaro.org) Received: by mail-wm1-f67.google.com with SMTP id u9so4852661wml.3 for ; Mon, 02 Mar 2020 05:49:57 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc:content-transfer-encoding; bh=ZplFzehZMBXNXu37eFwPpGIz8WHuLwyE4MEUw6Oane4=; b=OQqlob1xenNC47tqYIPVivRRBhIzpctdU6TTucxIvnSnFETzdIq0Jv7AZANHgBcNvt NSmzG/rGJ2XPnuVRmGlcvwkY1qOizF63wPrGbRPmbJ61McCP+sZUlbzarRXoGO2xzaQI +kQY3lipwTR0TaQmbwrnv4SR/D9nGH+/CPl/YYRJeQD4TxmdSy7uj7yTdWpAPNLau61Y J13+Xg6595GNhqauM3cr9BXo1wWUuXws8gUjg8Pmn0MBmZ3panzUFQyXrzhrSgM4UkCu W500f+trprXpbakWX3qwubc6el+GkwG7JDMVFqAUNlXR0OmMOUxhOBmxBIfVCiVmDHPo kmTQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc:content-transfer-encoding; bh=ZplFzehZMBXNXu37eFwPpGIz8WHuLwyE4MEUw6Oane4=; b=N+LhuKeZ4UmMVyjMuyXacMtO+YHVLDqRPjK6pib1Ov7ucBDsHuHyjiI3KVtjzyp62J /Cne7exOdtp8ZC5oI3tlJjqxTPZkkKOed3sGIMY9hm9TYk34vf6T3Yb1dGn+z8NAFCLo otiI9P3HC3fodwi1O57Lw9LZnMWruxlwEV8QJSvZHEFzYpzQmoKHQeFMNsKnjjQN9H+B 7QQlhlLdVIBXKC1rJF7ppQq6tQFwYKerVvncYPeBbObPxDrVjXm0zlZnPe6VHsQ0Yu/w Pbb+dVe0v4MxCV+nhYCqOLMqFFqgvrreMxmniZFht5X1XX4zSeyPeNRBZbpThj+5D5qU BHkQ== X-Gm-Message-State: APjAAAW78AwBB+7Iw+C7RKt4yOhGeo5Y+N0plkWeQ+3TOEZbfBh/9KJf efbKZtKk7NVifpV8IcVK4j+HgDJ6/DNVct/srnJCrQ== X-Google-Smtp-Source: APXvYqx4ZCjhSiK/ANe6Q46w07Wksfog/zZRWqaDw0D1mIuh4+H7QXgg6Y8pRKFtI14OCRvzpYd2P1IEYwCRffuKCbs= X-Received: by 2002:a05:600c:24b:: with SMTP id 11mr19662899wmj.1.1583156995756; Mon, 02 Mar 2020 05:49:55 -0800 (PST) MIME-Version: 1.0 References: <20200226221156.29589-1-lersek@redhat.com> <20200226221156.29589-10-lersek@redhat.com> In-Reply-To: <20200226221156.29589-10-lersek@redhat.com> From: "Ard Biesheuvel" Date: Mon, 2 Mar 2020 14:49:44 +0100 Message-ID: Subject: Re: [PATCH v2 09/16] OvmfPkg/CpuHotplugSmm: add function for collecting CPUs with events To: Laszlo Ersek Cc: edk2-devel-groups-io , Igor Mammedov , Jiewen Yao , Jordan Justen , Michael Kinney , =?UTF-8?Q?Philippe_Mathieu=2DDaud=C3=A9?= Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Wed, 26 Feb 2020 at 23:12, Laszlo Ersek wrote: > > Add a function that collects the APIC IDs of CPUs that have just been > hot-plugged, or are about to be hot-unplugged. > > Pending events are only located and never cleared; QEMU's AML needs the > firmware to leave the status bits intact in the hotplug register block. > > Cc: Ard Biesheuvel > Cc: Igor Mammedov > Cc: Jiewen Yao > Cc: Jordan Justen > Cc: Michael Kinney > Cc: Philippe Mathieu-Daud=C3=A9 > Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3D1512 > Signed-off-by: Laszlo Ersek > Acked-by: Ard Biesheuvel Reviewed-by: Ard Biesheuvel > --- > > Notes: > v2: > > - Pick up Ard's Acked-by, which is conditional on approval from Intel > reviewers on Cc. (I'd like to save Ard the churn of re-acking > unmodified patches.) > > OvmfPkg/Include/IndustryStandard/QemuCpuHotplug.h | 2 + > OvmfPkg/CpuHotplugSmm/CpuHotplugSmm.inf | 1 + > OvmfPkg/CpuHotplugSmm/ApicId.h | 23 +++ > OvmfPkg/CpuHotplugSmm/QemuCpuhp.h | 20 ++- > OvmfPkg/CpuHotplugSmm/QemuCpuhp.c | 171 ++++++++++++++++= +++- > 5 files changed, 211 insertions(+), 6 deletions(-) > > diff --git a/OvmfPkg/Include/IndustryStandard/QemuCpuHotplug.h b/OvmfPkg/= Include/IndustryStandard/QemuCpuHotplug.h > index 3d013633501b..a34a6d3fae61 100644 > --- a/OvmfPkg/Include/IndustryStandard/QemuCpuHotplug.h > +++ b/OvmfPkg/Include/IndustryStandard/QemuCpuHotplug.h > @@ -13,32 +13,34 @@ > The new ("modern") hotplug interface appeared in QEMU v2.7.0. > > The macros in this header file map to the minimal subset of the mode= rn > interface that OVMF needs. > **/ > > #ifndef QEMU_CPU_HOTPLUG_H_ > #define QEMU_CPU_HOTPLUG_H_ > > #include > > // > // Each register offset is: > // - relative to the board-dependent IO base address of the register blo= ck, > // - named QEMU_CPUHP_(R|W|RW)_*, according to the possible access modes= of the > // register, > // - followed by distinguished bitmasks or values in the register. > // > #define QEMU_CPUHP_R_CMD_DATA2 0x0 > > #define QEMU_CPUHP_R_CPU_STAT 0x4 > #define QEMU_CPUHP_STAT_ENABLED BIT0 > +#define QEMU_CPUHP_STAT_INSERT BIT1 > +#define QEMU_CPUHP_STAT_REMOVE BIT2 > > #define QEMU_CPUHP_RW_CMD_DATA 0x8 > > #define QEMU_CPUHP_W_CPU_SEL 0x0 > > #define QEMU_CPUHP_W_CMD 0x5 > #define QEMU_CPUHP_CMD_GET_PENDING 0x0 > #define QEMU_CPUHP_CMD_GET_ARCH_ID 0x3 > > #endif // QEMU_CPU_HOTPLUG_H_ > diff --git a/OvmfPkg/CpuHotplugSmm/CpuHotplugSmm.inf b/OvmfPkg/CpuHotplug= Smm/CpuHotplugSmm.inf > index ac4ca4c1f4f2..ab690a9e5e20 100644 > --- a/OvmfPkg/CpuHotplugSmm/CpuHotplugSmm.inf > +++ b/OvmfPkg/CpuHotplugSmm/CpuHotplugSmm.inf > @@ -3,44 +3,45 @@ > # > # Copyright (c) 2020, Red Hat, Inc. > # > # SPDX-License-Identifier: BSD-2-Clause-Patent > ## > > [Defines] > INF_VERSION =3D 1.29 > PI_SPECIFICATION_VERSION =3D 0x00010046 #= PI-1.7.0 > BASE_NAME =3D CpuHotplugSmm > FILE_GUID =3D 84EEA114-C6BE-4445-8F90-51D97863E363 > MODULE_TYPE =3D DXE_SMM_DRIVER > ENTRY_POINT =3D CpuHotplugEntry > > # > # The following information is for reference only and not required by th= e build > # tools. > # > # VALID_ARCHITECTURES =3D IA32 X64 > # > > [Sources] > + ApicId.h > CpuHotplug.c > QemuCpuhp.c > QemuCpuhp.h > > [Packages] > MdePkg/MdePkg.dec > OvmfPkg/OvmfPkg.dec > > [LibraryClasses] > BaseLib > DebugLib > MmServicesTableLib > PcdLib > UefiDriverEntryPoint > > [Protocols] > gEfiMmCpuIoProtocolGuid ## C= ONSUMES > > [Pcd] > gUefiOvmfPkgTokenSpaceGuid.PcdQ35SmramAtDefaultSmbase ## C= ONSUMES > > [FeaturePcd] > diff --git a/OvmfPkg/CpuHotplugSmm/ApicId.h b/OvmfPkg/CpuHotplugSmm/ApicI= d.h > new file mode 100644 > index 000000000000..3c365148ed02 > --- /dev/null > +++ b/OvmfPkg/CpuHotplugSmm/ApicId.h > @@ -0,0 +1,23 @@ > +/** @file > + Type and macro definitions for representing and printing APIC IDs, com= patibly > + with the LocalApicLib and PrintLib classes, respectively. > + > + Copyright (c) 2020, Red Hat, Inc. > + > + SPDX-License-Identifier: BSD-2-Clause-Patent > +**/ > + > +#ifndef APIC_ID_H_ > +#define APIC_ID_H_ > + > +// > +// The type that LocalApicLib represents an APIC ID with. > +// > +typedef UINT32 APIC_ID; > + > +// > +// The PrintLib conversion specification for formatting an APIC_ID. > +// > +#define FMT_APIC_ID "0x%08x" > + > +#endif // APIC_ID_H_ > diff --git a/OvmfPkg/CpuHotplugSmm/QemuCpuhp.h b/OvmfPkg/CpuHotplugSmm/Qe= muCpuhp.h > index 82f88f0b73bb..8adaa0ad91f0 100644 > --- a/OvmfPkg/CpuHotplugSmm/QemuCpuhp.h > +++ b/OvmfPkg/CpuHotplugSmm/QemuCpuhp.h > @@ -1,47 +1,61 @@ > /** @file > - Simple wrapper functions that access QEMU's modern CPU hotplug registe= r > - block. > + Simple wrapper functions and utility functions that access QEMU's mode= rn CPU > + hotplug register block. > > - These functions thinly wrap some of the registers described in > + These functions manipulate some of the registers described in > "docs/specs/acpi_cpu_hotplug.txt" in the QEMU source. IO Ports are acc= essed > via EFI_MM_CPU_IO_PROTOCOL. If a protocol call fails, these functions = don't > return. > > Copyright (c) 2020, Red Hat, Inc. > > SPDX-License-Identifier: BSD-2-Clause-Patent > **/ > > #ifndef QEMU_CPUHP_H_ > #define QEMU_CPUHP_H_ > > #include // EFI_MM_CPU_IO_PROTOCOL > +#include // EFI_STATUS > + > +#include "ApicId.h" // APIC_ID > > UINT32 > QemuCpuhpReadCommandData2 ( > IN CONST EFI_MM_CPU_IO_PROTOCOL *MmCpuIo > ); > > UINT8 > QemuCpuhpReadCpuStatus ( > IN CONST EFI_MM_CPU_IO_PROTOCOL *MmCpuIo > ); > > UINT32 > QemuCpuhpReadCommandData ( > IN CONST EFI_MM_CPU_IO_PROTOCOL *MmCpuIo > ); > > VOID > QemuCpuhpWriteCpuSelector ( > IN CONST EFI_MM_CPU_IO_PROTOCOL *MmCpuIo, > IN UINT32 Selector > ); > > VOID > QemuCpuhpWriteCommand ( > IN CONST EFI_MM_CPU_IO_PROTOCOL *MmCpuIo, > IN UINT8 Command > ); > > +EFI_STATUS > +QemuCpuhpCollectApicIds ( > + IN CONST EFI_MM_CPU_IO_PROTOCOL *MmCpuIo, > + IN UINT32 PossibleCpuCount, > + IN UINT32 ApicIdCount, > + OUT APIC_ID *PluggedApicIds, > + OUT UINT32 *PluggedCount, > + OUT APIC_ID *ToUnplugApicIds, > + OUT UINT32 *ToUnplugCount > + ); > + > #endif // QEMU_CPUHP_H_ > diff --git a/OvmfPkg/CpuHotplugSmm/QemuCpuhp.c b/OvmfPkg/CpuHotplugSmm/Qe= muCpuhp.c > index 31e46f51934a..8d4a6693c8d6 100644 > --- a/OvmfPkg/CpuHotplugSmm/QemuCpuhp.c > +++ b/OvmfPkg/CpuHotplugSmm/QemuCpuhp.c > @@ -1,27 +1,27 @@ > /** @file > - Simple wrapper functions that access QEMU's modern CPU hotplug registe= r > - block. > + Simple wrapper functions and utility functions that access QEMU's mode= rn CPU > + hotplug register block. > > - These functions thinly wrap some of the registers described in > + These functions manipulate some of the registers described in > "docs/specs/acpi_cpu_hotplug.txt" in the QEMU source. IO Ports are acc= essed > via EFI_MM_CPU_IO_PROTOCOL. If a protocol call fails, these functions = don't > return. > > Copyright (c) 2020, Red Hat, Inc. > > SPDX-License-Identifier: BSD-2-Clause-Patent > **/ > > #include // ICH9_CPU_HOTPLUG_BASE > #include // QEMU_CPUHP_R_CMD_DATA2 > #include // CpuDeadLoop() > #include // DEBUG() > > #include "QemuCpuhp.h" > > UINT32 > QemuCpuhpReadCommandData2 ( > IN CONST EFI_MM_CPU_IO_PROTOCOL *MmCpuIo > ) > { > UINT32 CommandData2; > @@ -115,22 +115,187 @@ QemuCpuhpWriteCpuSelector ( > > VOID > QemuCpuhpWriteCommand ( > IN CONST EFI_MM_CPU_IO_PROTOCOL *MmCpuIo, > IN UINT8 Command > ) > { > EFI_STATUS Status; > > Status =3D MmCpuIo->Io.Write ( > MmCpuIo, > MM_IO_UINT8, > ICH9_CPU_HOTPLUG_BASE + QEMU_CPUHP_W_CMD, > 1, > &Command > ); > if (EFI_ERROR (Status)) { > DEBUG ((DEBUG_ERROR, "%a: %r\n", __FUNCTION__, Status)); > ASSERT (FALSE); > CpuDeadLoop (); > } > } > + > +/** > + Collect the APIC IDs of > + - the CPUs that have been hot-plugged, > + - the CPUs that are about to be hot-unplugged. > + > + This function only scans for events -- it does not modify them -- in t= he > + hotplug registers. > + > + 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] 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[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] ToUnplugApicIds The APIC IDs of the CPUs that are about t= o be > + hot-unplugged. > + > + @param[out] ToUnplugCount The number of filled-in APIC IDs in > + ToUnplugApicIds. > + > + @retval EFI_INVALID_PARAMETER PossibleCpuCount is zero, or ApicIdCoun= t is > + zero. > + > + @retval EFI_PROTOCOL_ERROR Invalid bitmap detected in the > + QEMU_CPUHP_R_CPU_STAT register. > + > + @retval EFI_BUFFER_TOO_SMALL There was an attempt to place more than > + ApicIdCount APIC IDs into one of the > + PluggedApicIds and ToUnplugApicIds arra= ys. > + > + @retval EFI_SUCCESS Output parameters have been set success= fully. > +**/ > +EFI_STATUS > +QemuCpuhpCollectApicIds ( > + IN CONST EFI_MM_CPU_IO_PROTOCOL *MmCpuIo, > + IN UINT32 PossibleCpuCount, > + IN UINT32 ApicIdCount, > + OUT APIC_ID *PluggedApicIds, > + OUT UINT32 *PluggedCount, > + OUT APIC_ID *ToUnplugApicIds, > + OUT UINT32 *ToUnplugCount > + ) > +{ > + UINT32 CurrentSelector; > + > + if (PossibleCpuCount =3D=3D 0 || ApicIdCount =3D=3D 0) { > + return EFI_INVALID_PARAMETER; > + } > + > + *PluggedCount =3D 0; > + *ToUnplugCount =3D 0; > + > + CurrentSelector =3D 0; > + do { > + UINT32 PendingSelector; > + UINT8 CpuStatus; > + APIC_ID *ExtendIds; > + UINT32 *ExtendCount; > + APIC_ID NewApicId; > + > + // > + // Write CurrentSelector (which is valid) to the CPU selector regist= er. > + // Consequences: > + // > + // - Other register accesses will be permitted. > + // > + // - The QEMU_CPUHP_CMD_GET_PENDING command will start scanning for = a CPU > + // with pending events at CurrentSelector (inclusive). > + // > + QemuCpuhpWriteCpuSelector (MmCpuIo, CurrentSelector); > + // > + // Write the QEMU_CPUHP_CMD_GET_PENDING command. Consequences > + // (independently of each other): > + // > + // - If there is a CPU with pending events, starting at CurrentSelec= tor > + // (inclusive), the CPU selector will be updated to that CPU. Note= that > + // the scanning in QEMU may wrap around, because we must never cle= ar the > + // event bits. > + // > + // - The QEMU_CPUHP_RW_CMD_DATA register will return the (possibly u= pdated) > + // CPU selector value. > + // > + QemuCpuhpWriteCommand (MmCpuIo, QEMU_CPUHP_CMD_GET_PENDING); > + PendingSelector =3D QemuCpuhpReadCommandData (MmCpuIo); > + if (PendingSelector < CurrentSelector) { > + DEBUG ((DEBUG_VERBOSE, "%a: CurrentSelector=3D%u PendingSelector= =3D%u: " > + "wrap-around\n", __FUNCTION__, CurrentSelector, PendingSelector)= ); > + break; > + } > + CurrentSelector =3D PendingSelector; > + > + // > + // Check the known status / event bits for the currently selected CP= U. > + // > + CpuStatus =3D QemuCpuhpReadCpuStatus (MmCpuIo); > + if ((CpuStatus & QEMU_CPUHP_STAT_INSERT) !=3D 0) { > + // > + // The "insert" event guarantees the "enabled" status; plus it exc= ludes > + // the "remove" event. > + // > + if ((CpuStatus & QEMU_CPUHP_STAT_ENABLED) =3D=3D 0 || > + (CpuStatus & QEMU_CPUHP_STAT_REMOVE) !=3D 0) { > + DEBUG ((DEBUG_ERROR, "%a: CurrentSelector=3D%u CpuStatus=3D0x%x:= " > + "inconsistent CPU status\n", __FUNCTION__, CurrentSelector, > + CpuStatus)); > + return EFI_PROTOCOL_ERROR; > + } > + > + DEBUG ((DEBUG_VERBOSE, "%a: CurrentSelector=3D%u: insert\n", __FUN= CTION__, > + CurrentSelector)); > + > + ExtendIds =3D PluggedApicIds; > + ExtendCount =3D PluggedCount; > + } else if ((CpuStatus & QEMU_CPUHP_STAT_REMOVE) !=3D 0) { > + DEBUG ((DEBUG_VERBOSE, "%a: CurrentSelector=3D%u: remove\n", __FUN= CTION__, > + CurrentSelector)); > + > + ExtendIds =3D ToUnplugApicIds; > + ExtendCount =3D ToUnplugCount; > + } else { > + DEBUG ((DEBUG_VERBOSE, "%a: CurrentSelector=3D%u: no event\n", > + __FUNCTION__, CurrentSelector)); > + break; > + } > + > + // > + // Save the APIC ID of the CPU with the pending event, to the corres= ponding > + // APIC ID array. > + // > + if (*ExtendCount =3D=3D 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 =3D QemuCpuhpReadCommandData (MmCpuIo); > + DEBUG ((DEBUG_VERBOSE, "%a: ApicId=3D" FMT_APIC_ID "\n", __FUNCTION_= _, > + NewApicId)); > + ExtendIds[(*ExtendCount)++] =3D 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= ; > + // otherwise, QEMU_CPUHP_CMD_GET_PENDING would stick to the currentl= y > + // selected CPU. > + // > + CurrentSelector++; > + } while (CurrentSelector < PossibleCpuCount); > + > + DEBUG ((DEBUG_VERBOSE, "%a: PluggedCount=3D%u ToUnplugCount=3D%u\n", > + __FUNCTION__, *PluggedCount, *ToUnplugCount)); > + return EFI_SUCCESS; > +} > -- > 2.19.1.3.g30247aa5d201 > >