From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from aserp2120.oracle.com (aserp2120.oracle.com [141.146.126.78]) by mx.groups.io with SMTP id smtpd.web12.1072.1614031392355714158 for ; Mon, 22 Feb 2021 14:03:12 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@oracle.com header.s=corp-2020-01-29 header.b=tsVv/AKQ; spf=pass (domain: oracle.com, ip: 141.146.126.78, mailfrom: ankur.a.arora@oracle.com) Received: from pps.filterd (aserp2120.oracle.com [127.0.0.1]) by aserp2120.oracle.com (8.16.0.42/8.16.0.42) with SMTP id 11MM1QDY080195; Mon, 22 Feb 2021 22:03:09 GMT DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=oracle.com; h=subject : to : cc : references : from : message-id : date : in-reply-to : content-type : content-transfer-encoding : mime-version; s=corp-2020-01-29; bh=48DznTr258+OJSgFmIGV8OIB+zgabwwdC5j45uawoIY=; b=tsVv/AKQY5og1Q/QGV83yJGQ7csgHpnUFzSaituR6NQJrJWwKlJW6HnTGroXlgC49qRs pXoitdnRxDIWb0exf1URIABzV1eiY1/cEzZOT9Fgec91PyDZAnuwbTLC9FbfDRT7r+ZA SCYyNlYf1RaWo4GdDdBR/+oDi8UkalAxKbWLFKfzxPqSW5Px1F2zqgGeQrZijyX6y5Xe 7YAbmYCpDmhAB5hWLMd8BO51+mEN2H4fjXZL4YDx1sFGnTYsastQQGL2+zQjEvBzl8YG 9q5Z7IjPtrYqYnz2ytkwQZbmPw00RvhC4yol1AMzk5LJlX8FamRLQHvUOrvwaaTRo2t1 Vw== Received: from aserp3030.oracle.com (aserp3030.oracle.com [141.146.126.71]) by aserp2120.oracle.com with ESMTP id 36ttcm59k4-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Mon, 22 Feb 2021 22:03:09 +0000 Received: from pps.filterd (aserp3030.oracle.com [127.0.0.1]) by aserp3030.oracle.com (8.16.0.42/8.16.0.42) with SMTP id 11MM0LQg053111; Mon, 22 Feb 2021 22:03:08 GMT Received: from nam11-dm6-obe.outbound.protection.outlook.com (mail-dm6nam11lp2169.outbound.protection.outlook.com [104.47.57.169]) by aserp3030.oracle.com with ESMTP id 36v9m3t5w3-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Mon, 22 Feb 2021 22:03:08 +0000 ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=Hc66XtNXpmBkBt/zHXtQqIzx1nFVQmnsMTmGHK+F1qGt1cbCg7d4kEczwqlIMSemob/RMwBoWaxLrd8EvSkH6PQOJOHgxVj/phQ2bYPeXcTptOxwIHJim50rcrQJ8j/LOk4EU6CEluo5NKUJk5pJd6OPu2mpFc1VFiOvZcZX9g0q6SHrce+MQlPVw6uYEcABJOgsaVQNNRr/n+kXyGuAXxrxOjJJcMsJp73plpl4fMoI8XjSXqf9RtzNoXmAqZrmF2bKUkApj+Fv55RZ32UeWe+/C35cJ8T/FDVr+wIODaApgT+Dc8PE2rhUC/7GTB1KyQnmxXFp8NfhYTuGLjHIqw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=48DznTr258+OJSgFmIGV8OIB+zgabwwdC5j45uawoIY=; b=AI/iGqoXylSAeuQaByMrtipBfY+CMVkXGXDVz/lMQnTOUCqVwDPLNx0kwmPr+jP7g3D4s3EyzIL3eLvbt9qgPDEkRQHIJEmIIV8/Ungc01hvXVkcAdzTE52xexYy9YPG4A7WVUYQVNJhjVDkmxPGAT6JfXsEwfJRc+1smSCPC/pInw2j1LHROwC0AgDhc3EpLVJ1I4NpKZoklV/7mDyR9QBeXXMjVuqHzUytlPIXj2gJHical34boFUiRWh08CE+RuYHA4kDFBoKgjQzt7K5i5siNwsT33svvkzVdyXSyeiIBddVA9JFsWpUu8cZT2+oO3Lo2t9AWO+8+wBj2pK64g== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=oracle.com; dmarc=pass action=none header.from=oracle.com; dkim=pass header.d=oracle.com; arc=none DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=oracle.onmicrosoft.com; s=selector2-oracle-onmicrosoft-com; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=48DznTr258+OJSgFmIGV8OIB+zgabwwdC5j45uawoIY=; b=GxVg2WqO4MEv3cdkH4CRA6rwBWxlC7leh4V6TkzLelEibNbh0k6S3M/zHeP1hbBL44/rgNchzo2Mnu+m71LAU+/m2jAGkpjGbdB1HBOLmk0wQb3Bd9PyNAVLgLQ/LmnU/ETzGXvQ1T1ROhWS57WdZYRXx8AH0nr9wYrPaIA6QHM= Received: from SJ0PR10MB4605.namprd10.prod.outlook.com (2603:10b6:a03:2d9::24) by SJ0PR10MB4608.namprd10.prod.outlook.com (2603:10b6:a03:2d4::9) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.3868.27; Mon, 22 Feb 2021 22:03:05 +0000 Received: from SJ0PR10MB4605.namprd10.prod.outlook.com ([fe80::a021:790:7ce6:6f16]) by SJ0PR10MB4605.namprd10.prod.outlook.com ([fe80::a021:790:7ce6:6f16%6]) with mapi id 15.20.3868.033; Mon, 22 Feb 2021 22:03:05 +0000 Subject: Re: [edk2-devel] [PATCH v8 02/10] OvmfPkg/CpuHotplugSmm: collect hot-unplug events To: Laszlo Ersek , devel@edk2.groups.io 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-3-ankur.a.arora@oracle.com> <90b4d42f-20fd-b04c-cdf1-774fa19c1052@redhat.com> From: "Ankur Arora" Message-ID: <1fc423a8-1230-bcc9-2544-bdec8cd682bf@oracle.com> Date: Mon, 22 Feb 2021 14:03:02 -0800 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.6.0 In-Reply-To: <90b4d42f-20fd-b04c-cdf1-774fa19c1052@redhat.com> X-Originating-IP: [148.87.23.13] X-ClientProxiedBy: MW4PR04CA0281.namprd04.prod.outlook.com (2603:10b6:303:89::16) To SJ0PR10MB4605.namprd10.prod.outlook.com (2603:10b6:a03:2d9::24) MIME-Version: 1.0 X-MS-Exchange-MessageSentRepresentingType: 1 Received: from [192.168.0.108] (148.87.23.13) by MW4PR04CA0281.namprd04.prod.outlook.com (2603:10b6:303:89::16) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.3868.27 via Frontend Transport; Mon, 22 Feb 2021 22:03:04 +0000 X-MS-PublicTrafficType: Email X-MS-Office365-Filtering-Correlation-Id: fda6f574-c0a8-41ff-b805-08d8d77da1a6 X-MS-TrafficTypeDiagnostic: SJ0PR10MB4608: X-MS-Exchange-Transport-Forked: True X-Microsoft-Antispam-PRVS: X-MS-Oob-TLC-OOBClassifiers: OLM:10000; X-MS-Exchange-SenderADCheck: 1 X-Microsoft-Antispam: BCL:0; X-Microsoft-Antispam-Message-Info: jZiB1+r9eWWwd/f2whmeLTiKM8wWnIocjkX7n2hgskvu27OQ/Ency/x6BLWv0Yil0BD5tfJjZDO77vvlzYu9f2PFDXxRHiQjzya58qFQbOE5HYiqTBdeXIUS1SDmoFQlWSO1ctkYKQGZ2U2MvtEkWRZLThvyjWIJP6fr5EtVS6LaDoKR2MOj3yEwTaFoLV2Fdn/yxz89nFBtyqf8CrdX6Z5G6InOxbpivo1bZsFvA8PdUw/Rh9zqSTleqv55i3vjv9/Ln34qLwro6GyU/gfyC+jCS3+MEC+dpSwuZa09xk2vBWpzvwngS7PPIVsxNWYD4eZlCU342fdWgn8KRCe5N7+5GYXfCO8ofx/FQlZ7FWE8OdihkFJFY/ZX2sxtEbgGs/BBzPOM/SNEtQR1Kv7vd+ANqN5W0Twd8nZBGI+mwE15BueyiZXkNPcdYryCCB717G6OzAon591FwizSNJrbVXtjB/i/7FFBN5ZbbAK9YkFOsGOza5dhMxtedRwEubagE1roygzeeZayc5txsZe2+xn2fXtvP6iK/Ca3xKP6rG4mX40FIPiuVbSjW+/qO30WkVj2jRIBv3raKuWooLVEYm3h8au2XhmgtQGuB82uXG5DdFpPTCPRUanGBQpluF6VDEVN2bjyyHfQ0wffQFcOw/tmQhCjavoQsTCoUH9SOeHuTRiWX2BivUnSnxa+Z1Ri X-Forefront-Antispam-Report: CIP:255.255.255.255;CTRY:;LANG:en;SCL:1;SRV:;IPV:NLI;SFV:NSPM;H:SJ0PR10MB4605.namprd10.prod.outlook.com;PTR:;CAT:NONE;SFS:(136003)(376002)(396003)(366004)(346002)(39860400002)(30864003)(66476007)(66556008)(4326008)(83380400001)(5660300002)(6486002)(31686004)(53546011)(478600001)(16576012)(186003)(16526019)(54906003)(31696002)(8676002)(966005)(2906002)(107886003)(36756003)(86362001)(8936002)(956004)(66946007)(316002)(2616005)(26005)(45980500001)(43740500002);DIR:OUT;SFP:1101; X-MS-Exchange-AntiSpam-MessageData: =?utf-8?B?WndyM2VCZTJSNjkycm5JS0NHNkhIb1phZHNuYTlhYzhScThzcmpPUTFwa0xO?= =?utf-8?B?aVg0ZU8xeDJVU1dlN3lrb0pjd3J3RFFXSFZiVnk0TTNvYUVvUGxUMmtlcFk0?= =?utf-8?B?Zk03VDhHMUo5WWVCL0grNTVvVStyUnYvbThPK05xVC9YT0FRQjlKVm5CcWtu?= =?utf-8?B?cVJQNWxpVDJjUmp1L1lxb1dlcDhEVStRaGF1ZW42UUZ2Y3hUOU10VFIzK3Y4?= =?utf-8?B?bC9MYU1mSmJLYStyMVkzTjVveURWZDd6Mk1iVjc0SmN2VVRETjRHQldlM2ND?= =?utf-8?B?cEI0d2crcjhGQlUrTE8vdUJnellaa3hZdEZTNStHR3hrc2M1NWJadHh5dnpP?= =?utf-8?B?YStQZUkrOHMwbEdZOWZwWjRqK014SWRPRjBOQ1BSSjlVRHZuc2djcUEzTEFw?= =?utf-8?B?TFhMbjAyaVdLVjNxV1lMNFpEdEdWTlhxZ2V2MzVueXhrTzFVU2Z4TW0zYmVB?= =?utf-8?B?aVNjb3k5ZG44UXlraTZzOThiUyt4YWNaWHBTTk9ISUhCYlFxUzBiSlBISTBi?= =?utf-8?B?ZW42RmxzblRTcXdjaDlseERKWkFKYUN2QVZUZGJNSzF2YmhZd3NsQVRnWUha?= =?utf-8?B?Q3AzLytaYzZsbEFMRDZqK1BETG1TRFZrKzg5NUJJS3BmblZXR3hxc2g3Mmcx?= =?utf-8?B?aUxyZ3BXUUVkMWJ2ZWRJWnBaWFV4bGtVSE9pVjgvQlI5bUFJZCtkTVQ4UnF2?= =?utf-8?B?V3VJdVRUMGdFQkp0UVFqUEU4ZWJRcTVOM0hJMlNyUWt0SjJpMVZFSkgrMUZM?= =?utf-8?B?K0hGL1Y1TTNoOTdaaEo4Ty91Q1dMUHlGUVo0TVNnaDU3UmdySE5LRXh3aU1P?= =?utf-8?B?bXc5b2hHeWNzaXU5cDVVTDJrRjdILzg3dXZXYU05b0hFZEIxbEg0QmQ2TnFi?= =?utf-8?B?c1FtTlcxOTlSOFNERUFVbXNyRFZWNG95dy9wcEdyc3d1K09YNDd2Y0NQRXYr?= =?utf-8?B?K1ZhSnpkRHViUzYyRS9GVUZVYWdzT2VHVUV5N3hlZTE5NEhGZnVlV3B0QmFx?= =?utf-8?B?a1RKdUowdGFVMzRUTDVYV3ZObjl5YWRPdTY0S0oyODhwd2k2dzlFbURwaElv?= =?utf-8?B?M1JYbDZTWDFaYUYzY1F5RjBKcjNTdnpQYzFtNUhKKzR3R3hleEV3ekdyNVlw?= =?utf-8?B?dUFhYm9rc2tsakliUGtadEp0eFh6RW1pLzRVcEg2TzFJZnF3UHd4RUk0RElS?= =?utf-8?B?cnZ0M1hNdmtVT3VrZ0ZRaGF3bi8rYUZHaURDMkNHSEtJelRocnIzQUo3bU9J?= =?utf-8?B?RVhad2JiNVFtdzBXVmF5cnQ4SUtCWGtZLzZZTlR6dWtXdVVOcGQ1ZGVkb2tC?= =?utf-8?B?a2IwNDc1OUlBbnlaMjFDMUpoTnQzbGl0a1VlQzdFZnJxWnBCL2p6ZVFIek5B?= =?utf-8?B?QzlrbkswSCtWSVFKRXpWNjlyRGh5cmZqOG83WitCL2FLTzBvOVlHS2wzSHJD?= =?utf-8?B?TkgzdVhnZnZPMEdQczI1aVBDL1VENTFucmdSd2M3QTZpM3EvbWlkNGxhRmVy?= =?utf-8?B?dG4rbGM3SGsrMUJpR0lxVXdtZlo5dzVTUmcrcnFHVHZodWNJS0czRHhlYlc1?= =?utf-8?B?Yjc2OVFRTXpWUGgyRGZuU3RXV04yRTRwT1JobWdLa2tGMnlnOUlhMy8vTEVF?= =?utf-8?B?dXdTdktVYjh5ZmhNa3N4TmQzaSsxcjE5WWplZnZwOEhyRHFWSkNnQ0orVVpU?= =?utf-8?B?NHVuSWNHRlZtR09YS2Z0MlhBa0tOK0phc1NyRGFLdjV3TmJWakFSNXV6eHdB?= =?utf-8?Q?jSv6js4xi2Q8osBI2c880iAvq7St+oPDx8to1is?= X-OriginatorOrg: oracle.com X-MS-Exchange-CrossTenant-Network-Message-Id: fda6f574-c0a8-41ff-b805-08d8d77da1a6 X-MS-Exchange-CrossTenant-AuthSource: SJ0PR10MB4605.namprd10.prod.outlook.com X-MS-Exchange-CrossTenant-AuthAs: Internal X-MS-Exchange-CrossTenant-OriginalArrivalTime: 22 Feb 2021 22:03:05.7888 (UTC) X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted X-MS-Exchange-CrossTenant-Id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b X-MS-Exchange-CrossTenant-MailboxType: HOSTED X-MS-Exchange-CrossTenant-UserPrincipalName: vb3ThneSKB+yOLgnD4lmM+mwqePi6MaW3Twi54uZpBMvAqe0URKErGcOqhrSw4TG7qWYo9xoXzLHUgA6FH6IL7GtTbtwYImO2ZFEcxo9ipg= X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ0PR10MB4608 X-Proofpoint-Virus-Version: vendor=nai engine=6200 definitions=9903 signatures=668683 X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 malwarescore=0 adultscore=0 suspectscore=0 mlxlogscore=999 mlxscore=0 spamscore=0 bulkscore=0 phishscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2009150000 definitions=main-2102220190 X-Proofpoint-Virus-Version: vendor=nai engine=6200 definitions=9903 signatures=668683 X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 mlxlogscore=999 adultscore=0 lowpriorityscore=0 spamscore=0 mlxscore=0 bulkscore=0 clxscore=1015 priorityscore=1501 malwarescore=0 impostorscore=0 suspectscore=0 phishscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2009150000 definitions=main-2102220190 Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit On 2021-02-22 4:27 a.m., Laszlo Ersek wrote: > On 02/22/21 08:19, Ankur Arora wrote: >> Process fw_remove events in QemuCpuhpCollectApicIds() and collect >> corresponding APIC IDs for CPUs that are being hot-unplugged. > > (1) We also collect selectors for those; please mention the fact here. > > >> >> 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 review comments from v6: >> (1,4) Move (and also rename) QEMU_CPUHP_STAT_EJECTED to patch 8, >> where we actually use it. >> (2) Downgrade debug mask from DEBUG_INFO to DEBUG_VERBOSE. >> (3a,3b,3c) Keep the CurrentSelector increment operation at >> the tail of the loop. >> () As discussed elsewhere we also need to get the CpuSelector while >> collecting ApicIds in QemuCpuhpCollectApicIds(). This patch adds a >> separate parameter for the CpuSelector values, because that works >> better alongside the hotplug ExtendIds logic. >> >> OvmfPkg/CpuHotplugSmm/QemuCpuhp.h | 1 + >> OvmfPkg/Include/IndustryStandard/QemuCpuHotplug.h | 1 + >> OvmfPkg/CpuHotplugSmm/CpuHotplug.c | 21 +++++- >> OvmfPkg/CpuHotplugSmm/QemuCpuhp.c | 84 ++++++++++++++++------- >> 4 files changed, 79 insertions(+), 28 deletions(-) >> >> diff --git a/OvmfPkg/CpuHotplugSmm/QemuCpuhp.h b/OvmfPkg/CpuHotplugSmm/QemuCpuhp.h >> index 8adaa0ad91f0..1e23b150910e 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 *ToUnplugSelector, >> OUT UINT32 *ToUnplugCount >> ); >> > > (2) For consistency with the parameter names "PluggedApicIds" (plural) > and "ToUnplugApicIds" (plural), please rename this parameter to > "ToUnplugSelectors". > > >> 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..3192bfea1f15 100644 >> --- a/OvmfPkg/CpuHotplugSmm/CpuHotplug.c >> +++ b/OvmfPkg/CpuHotplugSmm/CpuHotplug.c >> @@ -52,6 +52,7 @@ STATIC CPU_HOT_PLUG_DATA *mCpuHotPlugData; >> // >> STATIC APIC_ID *mPluggedApicIds; >> STATIC APIC_ID *mToUnplugApicIds; >> +STATIC UINT32 *mToUnplugSelector; > > (3) For consistency with the other two global variable identifiers, > please call this "mToUnplugSelector" (plural), too. > > (4) The comment above these global variables only talks about APIC IDs; > please update the comment with a reference / explanation of the selectors. > > >> // >> // Address of the non-SMRAM reserved memory page that contains the Post-SMM Pen >> // for hot-added CPUs. >> @@ -289,6 +290,7 @@ CpuHotplugMmi ( >> mPluggedApicIds, >> &PluggedCount, >> mToUnplugApicIds, >> + mToUnplugSelector, >> &ToUnplugCount >> ); >> if (EFI_ERROR (Status)) { >> @@ -333,7 +335,9 @@ CpuHotplugEntry ( >> ) >> { >> EFI_STATUS Status; >> + UINTN Len; >> UINTN Size; >> + UINTN SizeSel; >> >> // >> // This module should only be included when SMM support is required. >> @@ -387,8 +391,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))|| > > (5) Missing space character before "||". > > >> + 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 +410,12 @@ CpuHotplugEntry ( >> DEBUG ((DEBUG_ERROR, "%a: MmAllocatePool(): %r\n", __FUNCTION__, Status)); >> goto ReleasePluggedApicIds; >> } >> + Status = gMmst->MmAllocatePool (EfiRuntimeServicesData, SizeSel, >> + (VOID **)&mToUnplugSelector); >> + 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 +423,7 @@ CpuHotplugEntry ( >> Status = SmbaseAllocatePostSmmPen (&mPostSmmPenAddress, >> SystemTable->BootServices); >> if (EFI_ERROR (Status)) { >> - goto ReleaseToUnplugApicIds; >> + goto ReleaseToUnplugSelector; >> } >> >> // >> @@ -472,6 +483,10 @@ ReleasePostSmmPen: >> SmbaseReleasePostSmmPen (mPostSmmPenAddress, SystemTable->BootServices); >> mPostSmmPenAddress = 0; >> >> +ReleaseToUnplugSelector: > > (6) Please call this label "ReleaseToUnplugSelectors" (plural). > > >> + gMmst->MmFreePool (mToUnplugSelector); >> + mToUnplugSelector = NULL; >> + >> ReleaseToUnplugApicIds: >> gMmst->MmFreePool (mToUnplugApicIds); >> mToUnplugApicIds = NULL; >> diff --git a/OvmfPkg/CpuHotplugSmm/QemuCpuhp.c b/OvmfPkg/CpuHotplugSmm/QemuCpuhp.c >> index 8d4a6693c8d6..36372a5e6193 100644 >> --- a/OvmfPkg/CpuHotplugSmm/QemuCpuhp.c >> +++ b/OvmfPkg/CpuHotplugSmm/QemuCpuhp.c >> @@ -164,6 +164,9 @@ QemuCpuhpWriteCommand ( >> @param[out] ToUnplugApicIds The APIC IDs of the CPUs that are about to be >> hot-unplugged. >> >> + @param[out] ToUnplugSelector 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. >> > Acking the comments above. > (7) Please (a) call the parameter "ToUnplugSelectors" (plural), and (b) > make sure that there are two space characters between the variable name > "column" and the documentation "column". (All in all, please move the > RHS column to the right by two spaces.) That would make the RHS of ToUnplugSelectors not line up with the other two out params. (Even though this mail does not seem to show that, they do line up in the code.) Is that okay? > >> @@ -187,6 +190,7 @@ QemuCpuhpCollectApicIds ( >> OUT APIC_ID *PluggedApicIds, >> OUT UINT32 *PluggedCount, >> OUT APIC_ID *ToUnplugApicIds, >> + OUT UINT32 *ToUnplugSelector, >> OUT UINT32 *ToUnplugCount >> ) >> { >> @@ -204,6 +208,7 @@ QemuCpuhpCollectApicIds ( >> UINT32 PendingSelector; >> UINT8 CpuStatus; >> APIC_ID *ExtendIds; >> + UINT32 *ExtendSel; > > (8) Please use plural -- "ExtendSels" --, consistently with "ExtendIds". > > >> 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,40 +264,69 @@ QemuCpuhpCollectApicIds ( >> CurrentSelector)); >> >> ExtendIds = PluggedApicIds; >> + ExtendSel = 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; >> + ExtendSel = ToUnplugSelector; >> 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; >> + ExtendSel = 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)); > > (9) Please follow this ASSERT with another ASSERT(), on a separate line: > > ASSERT (ExtendSels == NULL || ExtendIds != NULL); > > Explanation: > > - I'd like us to capture: if ExtendSels is not NULL, then ExtendIds is > also not NULL. > > - Furthermore, express the A-->B implication using its equivalent > formulation (!A)||B. This is a good check. Will add. > > >> + 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 (ExtendSel != NULL) { >> + ExtendSel[(*ExtendCount)] = CurrentSelector; >> + } >> + ExtendIds[(*ExtendCount)++] = NewApicId; > > OK thus far... > >> >> - // >> - // 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 currently >> - // selected CPU. >> - // >> - CurrentSelector++; >> + // >> + // 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 currently >> + // selected CPU. >> + // >> + CurrentSelector++; >> + } > > (10) ... but this is a logic bug: the comment and the CurrentSelector > increment must not depend on (ExtendIds != NULL). > > As written, if we ever see a QEMU_CPUHP_STAT_REMOVE event (which we're > supposed to ignore), we'll never move past that CPU, but will be stuck > in an infinite loop. The QEMU_CPUHP_CMD_GET_PENDING command will keep > returning the CPU with the QEMU_CPUHP_STAT_REMOVE event pending. > > If this was unclear from my previous feedback, then I apologize, I > should have been clearer. No, my bad. Don't quite know how I missed that -- that was the whole reason why I had a CurrentSelector++ before the continue statement in v6. Will fix. Thanks Ankur > > Thanks, > Laszlo > >> } while (CurrentSelector < PossibleCpuCount); >> >> DEBUG ((DEBUG_VERBOSE, "%a: PluggedCount=%u ToUnplugCount=%u\n", >> >