From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-1.mimecast.com (us-smtp-1.mimecast.com [205.139.110.120]) by mx.groups.io with SMTP id smtpd.web10.20.1583181280114869101 for ; Mon, 02 Mar 2020 12:34:40 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=Wh5hwdS8; spf=pass (domain: redhat.com, ip: 205.139.110.120, mailfrom: philmd@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1583181279; 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=z/nGKfThIlm82wCoBRE3QuXxyUFmss+4yCcy1R+8OOA=; b=Wh5hwdS8doPyY0n1iEMWG4NSRZZdTV3/6fWMXOuyc18F5ByeLWJyQG9XIMxyl8pi39PRMP W8FfVCeIonFRgNmrZncazmsdbTX8tSl0q9/KNEUkWNJA8+EKs2ae2EuoYI0EId4gemzhox zeMSl79d59XFbujcSFocrBRr9fVyAbw= Received: from mail-wr1-f71.google.com (mail-wr1-f71.google.com [209.85.221.71]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-110-1BaxRd9DN3ybOArwQSGlZg-1; Mon, 02 Mar 2020 15:34:35 -0500 X-MC-Unique: 1BaxRd9DN3ybOArwQSGlZg-1 Received: by mail-wr1-f71.google.com with SMTP id u18so207578wrn.11 for ; Mon, 02 Mar 2020 12:34:34 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:cc:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=fifIxt56S1WZ/JupizBYsQjZmiWY+zTZx8aFgMMyii4=; b=qIL7k1dMOr2MVSaRCTJVD4V1sfXK8AWKULK+c2q4jcRYJObh/Hb13MZ9yEGyq7STtd KqHmLcXera34ga+zQfcm1y/PQNR3ww5ZcTI0eT3CsXTF4vweFzf1u5GLnHa3vuMr4f9L N4y2pFq8gyvlVpbmROztNWyEtSPsVwZL+34NG6o72bNEs4sVQxQi/+vGNA+POxmrdI3W 3FO3Ce2X81agkhXuwf7cgwUJUACLEHfF/xIcJ2e9ahyX5MRiw/2ERegWJt/d05K3G8wO Az3x0ft9Mmd3A0xSek/JzxZHX6yW2LyFnywtlehmoICGI4Mltgj38tDml6Enlh0Dahk8 AU0Q== X-Gm-Message-State: ANhLgQ15n3q2fVUcNLMgPIVnmi/cAMS3Bfp5fEHDro5PNQK3zO4n2mAV vZmWB4StiyMor/MpJfKsVxshufguvi4eLcwo5LJLtr0VxZ1EeydzWtQze2d1hR2KS6V9pVL0pzg 3PVnXTmZ7qz6Q2A== X-Received: by 2002:adf:e3cd:: with SMTP id k13mr1357290wrm.302.1583181273071; Mon, 02 Mar 2020 12:34:33 -0800 (PST) X-Google-Smtp-Source: ADFU+vuLhfgw58/aKIoo9fSE52Bh/17d6YOOrkceJ+aE3ACF5q02jIJDwBl3G7gMndYlfvnxSH/G+A== X-Received: by 2002:adf:e3cd:: with SMTP id k13mr1357262wrm.302.1583181272772; Mon, 02 Mar 2020 12:34:32 -0800 (PST) Return-Path: Received: from ?IPv6:2a01:e35:2fb0:49e0:3f7b:4b69:b9c:cdc0? ([2a01:e35:2fb0:49e0:3f7b:4b69:b9c:cdc0]) by smtp.gmail.com with ESMTPSA id x21sm225999wmi.30.2020.03.02.12.34.31 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 02 Mar 2020 12:34:32 -0800 (PST) Subject: Re: [PATCH v2 09/16] OvmfPkg/CpuHotplugSmm: add function for collecting CPUs with events To: Laszlo Ersek , edk2-devel-groups-io Cc: Ard Biesheuvel , Igor Mammedov , Jiewen Yao , Jordan Justen , Michael Kinney References: <20200226221156.29589-1-lersek@redhat.com> <20200226221156.29589-10-lersek@redhat.com> From: =?UTF-8?B?UGhpbGlwcGUgTWF0aGlldS1EYXVkw6k=?= Message-ID: Date: Mon, 2 Mar 2020 21:34:30 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.4.1 MIME-Version: 1.0 In-Reply-To: <20200226221156.29589-10-lersek@redhat.com> X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: quoted-printable On 2/26/20 11:11 PM, 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. >=20 > 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. >=20 > 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 > --- >=20 > Notes: > v2: > =20 > - Pick up Ard's Acked-by, which is conditional on approval from Inte= l > reviewers on Cc. (I'd like to save Ard the churn of re-acking > unmodified patches.) >=20 > 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(-) >=20 > 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. > =20 > The macros in this header file map to the minimal subset of the mod= ern > interface that OVMF needs. > **/ > =20 > #ifndef QEMU_CPU_HOTPLUG_H_ > #define QEMU_CPU_HOTPLUG_H_ > =20 > #include > =20 > // > // Each register offset is: > // - relative to the board-dependent IO base address of the register bl= ock, > // - named QEMU_CPUHP_(R|W|RW)_*, according to the possible access mode= s of the > // register, > // - followed by distinguished bitmasks or values in the register. > // > #define QEMU_CPUHP_R_CMD_DATA2 0x0 > =20 > #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 > =20 > #define QEMU_CPUHP_RW_CMD_DATA 0x8 > =20 > #define QEMU_CPUHP_W_CPU_SEL 0x0 > =20 > #define QEMU_CPUHP_W_CMD 0x5 > #define QEMU_CPUHP_CMD_GET_PENDING 0x0 > #define QEMU_CPUHP_CMD_GET_ARCH_ID 0x3 > =20 > #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 > ## > =20 > [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 > =20 > # > # The following information is for reference only and not required by t= he build > # tools. > # > # VALID_ARCHITECTURES =3D IA32 X64 > # > =20 > [Sources] > + ApicId.h > CpuHotplug.c > QemuCpuhp.c > QemuCpuhp.h > =20 > [Packages] > MdePkg/MdePkg.dec > OvmfPkg/OvmfPkg.dec > =20 > [LibraryClasses] > BaseLib > DebugLib > MmServicesTableLib > PcdLib > UefiDriverEntryPoint > =20 > [Protocols] > gEfiMmCpuIoProtocolGuid ## = CONSUMES > =20 > [Pcd] > gUefiOvmfPkgTokenSpaceGuid.PcdQ35SmramAtDefaultSmbase ## = CONSUMES > =20 > [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. > =20 > - 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 ac= cessed > via EFI_MM_CPU_IO_PROTOCOL. If a protocol call fails, these functions= don't > return. > =20 > Copyright (c) 2020, Red Hat, Inc. > =20 > SPDX-License-Identifier: BSD-2-Clause-Patent > **/ > =20 > #ifndef QEMU_CPUHP_H_ > #define QEMU_CPUHP_H_ > =20 > #include // EFI_MM_CPU_IO_PROTOCOL > +#include // EFI_STATUS > + > +#include "ApicId.h" // APIC_ID > =20 > UINT32 > QemuCpuhpReadCommandData2 ( > IN CONST EFI_MM_CPU_IO_PROTOCOL *MmCpuIo > ); > =20 > UINT8 > QemuCpuhpReadCpuStatus ( > IN CONST EFI_MM_CPU_IO_PROTOCOL *MmCpuIo > ); > =20 > UINT32 > QemuCpuhpReadCommandData ( > IN CONST EFI_MM_CPU_IO_PROTOCOL *MmCpuIo > ); > =20 > VOID > QemuCpuhpWriteCpuSelector ( > IN CONST EFI_MM_CPU_IO_PROTOCOL *MmCpuIo, > IN UINT32 Selector > ); > =20 > VOID > QemuCpuhpWriteCommand ( > IN CONST EFI_MM_CPU_IO_PROTOCOL *MmCpuIo, > IN UINT8 Command > ); > =20 > +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. > =20 > - 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 ac= cessed > via EFI_MM_CPU_IO_PROTOCOL. If a protocol call fails, these functions= don't > return. > =20 > Copyright (c) 2020, Red Hat, Inc. > =20 > SPDX-License-Identifier: BSD-2-Clause-Patent > **/ > =20 > #include // ICH9_CPU_HOTPLUG_BASE > #include // QEMU_CPUHP_R_CMD_DATA2 > #include // CpuDeadLoop() > #include // DEBUG() > =20 > #include "QemuCpuhp.h" > =20 > UINT32 > QemuCpuhpReadCommandData2 ( > IN CONST EFI_MM_CPU_IO_PROTOCOL *MmCpuIo > ) > { > UINT32 CommandData2; > @@ -115,22 +115,187 @@ QemuCpuhpWriteCpuSelector ( > =20 > VOID > QemuCpuhpWriteCommand ( > IN CONST EFI_MM_CPU_IO_PROTOCOL *MmCpuIo, > IN UINT8 Command > ) > { > EFI_STATUS Status; > =20 > 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. Maybe nitpicking: "positive (non zero)"? > + > + @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; > +} >=20 Reviewed-by: Philippe Mathieu-Daude