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.web09.4937.1612326623786936567 for ; Tue, 02 Feb 2021 20:30:24 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@oracle.com header.s=corp-2020-01-29 header.b=Wushx2mb; 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 1134Svv1118031; Wed, 3 Feb 2021 04:30:19 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=GK9ZuNdBQwPHO0UoNLXvtGTBKtS55hYHSbhzAcDjf2M=; b=Wushx2mbR5qKffMySbV9/gJMV71ZtO8txkGeSxaQ5NncQsbWehNjp9l2+mCFWvnjA7v2 8yGSznnSEw5YzDlMFe3+HXiF01+EHPfwArexPvGGPqodCHwD5tDcKWpnSf8cG9BP1tAp XIKSlA//649DByVferLy3xwbqboDKkqJ3t3p7pYdNLB/WYVMmy/7uj8t3VR03INq9cpN 2UTSngmyf9C+qgMLr/PHwzwPcKxTxKU9dr23EpCE6AjtHk79r4VnIaApgKM98vaSqg9a jCWEZrf3ALieVIOxYTdwHomHuBpjof+ygGMtDfm2DJniFiC9cJvm+IYiBbp+DBuzMVMW wQ== Received: from aserp3020.oracle.com (aserp3020.oracle.com [141.146.126.70]) by aserp2120.oracle.com with ESMTP id 36cydkx01v-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Wed, 03 Feb 2021 04:30:19 +0000 Received: from pps.filterd (aserp3020.oracle.com [127.0.0.1]) by aserp3020.oracle.com (8.16.0.42/8.16.0.42) with SMTP id 1134L0Aa163805; Wed, 3 Feb 2021 04:28:18 GMT Received: from nam02-bl2-obe.outbound.protection.outlook.com (mail-bl2nam02lp2050.outbound.protection.outlook.com [104.47.38.50]) by aserp3020.oracle.com with ESMTP id 36dhc0akjg-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Wed, 03 Feb 2021 04:28:18 +0000 ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=mm2WSJR3i/uc/4Q5pQhC40MD5zn2EV7XqXjY/ZgrIF2z7SWIQ50xvb9+K7AifWBfPh/ZyXHyAy+rwZAc324Vcu6WGqT4DxZpruTKs0Lbc3xsjQw9vO4DZ8mG0E3uUtxyYIjQQwymn0kKMkaw64YMDBXskDEZWVNyhSanq5NYZBAvh/T0ynWSMruCTGGxnammtQnnUzoJRSPrfTZ9zj6GkqOtlsAW+os9SNVpmCHCfU1h72fK/MCjDxXFwY3ht2dfwBlZZgu5CsQ7tyEu6Pb0vhfS/hsRxCHPJwieMwtgvBQUXzX+GgAW3eShbjEN4v+ww6t+uQvc72+Tn1sZ1g27og== 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=GK9ZuNdBQwPHO0UoNLXvtGTBKtS55hYHSbhzAcDjf2M=; b=BhZQkfpKx+ZyYzpDRScdaqpB+7PLo6hi+eaVIAV3M2DF0t68Tps57DZNgCfXoHn1LEfNMpbp95sFLX16WBUAIu5Iy+Wpu021jjyyvaEOT31bF6golVCRsGa+JsHOVpaOyKej2FGgU+7t4J7O/jVBBZu8RblJMfVPCRO+km31WdKXg9lnZbs1JT0v2c577mbA8ddaYyRKQmkQaNpTmVhmjvHR4sjrEAACXzzezyeU/ZJYVIDRi7E+t/PVbUtIWwhuDB1RCjrfo7JGC+q2rf/RCvyJjpwdvuDOe/cLaXi9fg/n0nbrcVoA+JQCx1E1mMdmymNZiaaGrw+ZkjmK/GmwLw== 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=GK9ZuNdBQwPHO0UoNLXvtGTBKtS55hYHSbhzAcDjf2M=; b=Ooa3E8ULOUbX+4Cj3wLdCF/t1pRKcwH9qzrS0MK3ND5YBidSqtVUzBJy4JZEzLcxv1s+dTJfm+qCrfhQO5YNhFJISb2oqbTRTaecmUiXwiUscbST4i2TD6BscLm23EI/5VBrKAv21ADbub27iWaREQGS9PAhW8xWLfyojHR3rwo= Received: from SJ0PR10MB4605.namprd10.prod.outlook.com (2603:10b6:a03:2d9::24) by BYAPR10MB2965.namprd10.prod.outlook.com (2603:10b6:a03:90::21) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.3805.16; Wed, 3 Feb 2021 04:28:16 +0000 Received: from SJ0PR10MB4605.namprd10.prod.outlook.com ([fe80::b1e9:811d:aa8e:f62a]) by SJ0PR10MB4605.namprd10.prod.outlook.com ([fe80::b1e9:811d:aa8e:f62a%6]) with mapi id 15.20.3805.028; Wed, 3 Feb 2021 04:28:16 +0000 Subject: Re: [PATCH v6 4/9] OvmfPkg/CpuHotplugSmm: introduce UnplugCpus() To: Laszlo Ersek , 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-5-ankur.a.arora@oracle.com> <06503fdc-a609-455d-2269-38491e8a9408@redhat.com> From: "Ankur Arora" Message-ID: <9d08c280-6092-41a2-4448-8a51a669c672@oracle.com> Date: Tue, 2 Feb 2021 20:28:10 -0800 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.6.0 In-Reply-To: <06503fdc-a609-455d-2269-38491e8a9408@redhat.com> X-Originating-IP: [148.87.23.11] X-ClientProxiedBy: CO2PR04CA0186.namprd04.prod.outlook.com (2603:10b6:104:5::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.11) by CO2PR04CA0186.namprd04.prod.outlook.com (2603:10b6:104:5::16) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.3825.19 via Frontend Transport; Wed, 3 Feb 2021 04:28:14 +0000 X-MS-PublicTrafficType: Email X-MS-Office365-Filtering-Correlation-Id: a14f5641-6aa0-4801-73bd-08d8c7fc206a X-MS-TrafficTypeDiagnostic: BYAPR10MB2965: 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: Fxo5mECx1sTB5yzogEQD/gDqOSBjlS0tK7dCDgg+bwWqaqaxJi0T1avltCjZR4Fkexcc8TELeKCASVC2uCatwa1UEP/bnwlyQqQDp3u80ozVixYdVGtpAg0OgVUo9t+zQNLkpjsOE8KJrUUCtuQNiOGyHtdKHhRQVo9n9Gye6nepQxzW7c4RiLj9X6/KHm2HsMxT0Wh2uGKKHtTv7mvKbA8fztWuq+B53XB1p8LOqp8hi5Gcp6jehLormJedZL78zridtrxzUlOJPHcimLIzu+Lt4CfPltnJQs8B4ZwNH5P8iEZcgKWoTP1XF8cXmCITnTiCFMZE+dWpZpSdYepSHjklvBtT4qRJsRMZqbZvIaXje1k1XplU/itYbVS2pr1DDtzvB+UwwMG+cZWft/7ZzUnhYM9KtBT2hWLLnb3+xqXKr+ijwhOelA9HA2rqDgOAwI6+NFfYiQf9HxuaXN5zfUo9IJXRygfYYB9EqxSjFiEy5BE02FUEZqxWioxOXd2ZnSpkhZFknFjFLoKtx2F6z4kHYmknkdXsuJMs5hOCDXEwyEvQBwrwV68YBwU9H5ernln3JL+L+0qvncm7Fkf7mnXeYHwn+DSOYBKFTaoDIOOennuSLbWvFDuGvXY9TW+EPSoVdBpOLDWpfmiZzX2Y71b+V5O2J/h080MYUgwNqcAMLK4QlGS0cnnpsddzSfRF 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:(376002)(366004)(39860400002)(136003)(346002)(396003)(8936002)(2906002)(4326008)(53546011)(6486002)(26005)(186003)(16526019)(2616005)(31686004)(956004)(36756003)(86362001)(66556008)(66476007)(8676002)(107886003)(478600001)(6666004)(966005)(54906003)(83380400001)(316002)(31696002)(16576012)(5660300002)(66946007)(45980500001)(43740500002);DIR:OUT;SFP:1101; X-MS-Exchange-AntiSpam-MessageData: =?utf-8?B?cHpnYXZ0dzlWQjZITVJ2eTBCdlpNSU1hWXBNTmlnYVYwZTI3SmtRdFhWSW1S?= =?utf-8?B?R0s0ZlZOWTA3TUZ1RmlrallwR3d2eTM4SmNzanJrZkdxVm9MVGhzaCtwZDNw?= =?utf-8?B?TTVtNkZhVVRnanh4clRQZDcxdG1UZ3ExRVJEMmFYWVdiOEM5K3htQUVpbFVj?= =?utf-8?B?akxXNlZrQVI1K3pwTXdrQkNuUTZhY3dhR1hGdTVxMnBwQlFjOUpXWGRSWlBF?= =?utf-8?B?dXNUWjVRcVFxSTBMRklRLzJIUWZOcC9KQnQ3NW12YzBLY1NSUzVXVXVYdFZY?= =?utf-8?B?aENSek1pY1dERDVtNkFQRFF4YkJ2a0tkZ1lrTXJuNkZYb3AvTERkK1lXWVNr?= =?utf-8?B?Z0dLWkIxbklXWGRmUTdIYmYvckRLbU9SSThHdXpPUE1iS1d3OVRSWFUrTjYr?= =?utf-8?B?YnZFU2RzclRCMWtubitjUlBpQ3gvOUp2NjZnUGpOdkdlK0hlLzZab0IwNEVo?= =?utf-8?B?Z2hhWVM1MlZBN2pUNGhydXJuWTZvUklJckxqR3pwV2hFMjJheHZVS2VXbEVE?= =?utf-8?B?OUplTnA1Q2ppY3pESGh1Sm9tMTllbUxFWWxaYzFKSVg4L1R6a0w4SmsvYkhH?= =?utf-8?B?NVBmMlhTOTNkRFkyY3pSK2J0Y25zKzYvUUxmTGk4aGw5elhQeHYxUnFTdzY0?= =?utf-8?B?T0J1MHVoUk1WZWVuQlgwUlAzWEpIZ0ZSMDJEWW1mUFJCUThPL1lMRlpwR0E1?= =?utf-8?B?NEl2NGZwcURvbENON1JLQkNzcGpDdG1LbUNGNGRyYnBpWGptZkhLWTZjSkpP?= =?utf-8?B?Ty9iNmFLaXAyclF2RC9ZcWY3ZUJXLzdvWHBvMG4xV1JFck1TM0E3NzRYS1Fz?= =?utf-8?B?SDU5QXpNZUFGVVhXaFo2aEcyWlNvK3dwMVdXMFl4MUJvVmdkS3lGeHNNR3kw?= =?utf-8?B?YUtIOUcweWZLSmxVeGtxVm9xOGc5OUplUGF0QzFkcnkrZ0FmVFZaK1kwY0Vw?= =?utf-8?B?Nk44RkxMNHo4MVkwOGQ2RncrT3dvOUxRVmpaaEhoOW9BMXVNYmh6czZFT1RH?= =?utf-8?B?RVZSMWpBaytkemc1UTBNcHVVUEVnVy9JOUR4bnNlcnUxOGJ3eE9ia0hHOGdQ?= =?utf-8?B?cEZXM1B0SkZtOUdXQXlQb2szYkNJd21LcjRTTElXWjZuaHgyRTBqbVhEOEZB?= =?utf-8?B?VWJIUm5oOEc5cElwdVQySFA3Sisya05nRTFkVzArT0RxRXdtTDBIemM2NElV?= =?utf-8?B?YTR3cFVFVnE1Q3psQ3pPVEFxRitnTDVCME9yaUF6Rm9QR0dOMXJUR0E0di84?= =?utf-8?B?T3d0RlJZTWpsdmgvMExud0JySjU5TFNXdXBJOHBlYVBhaFlzNTIyNXRXVVFj?= =?utf-8?B?ek1yLzJzRUNaelQydWphdTlxSTNkSjJjY1N2R0dTTzF0R3FORUhPLzQyNC9W?= =?utf-8?B?WTZVRG51ZkZJeDkvQkh0TkV4d3JPRWRNTmdHZVY2dUp0UlhVdTRuaEdISFhM?= =?utf-8?Q?ZGYYspkD?= X-OriginatorOrg: oracle.com X-MS-Exchange-CrossTenant-Network-Message-Id: a14f5641-6aa0-4801-73bd-08d8c7fc206a X-MS-Exchange-CrossTenant-AuthSource: SJ0PR10MB4605.namprd10.prod.outlook.com X-MS-Exchange-CrossTenant-AuthAs: Internal X-MS-Exchange-CrossTenant-OriginalArrivalTime: 03 Feb 2021 04:28:16.4161 (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: vlSY2Sk4EOQhhxxMg93G7882+1aY3M6Rz0sKAdQFCp+FSVrqD5kjgGp3WSfcpo2pFnPNwWNSUCHnUCIXXdcdNzeplH4BZcDOyBFvBcJsemg= X-MS-Exchange-Transport-CrossTenantHeadersStamped: BYAPR10MB2965 X-Proofpoint-Virus-Version: vendor=nai engine=6200 definitions=9883 signatures=668683 X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 bulkscore=0 adultscore=0 suspectscore=0 spamscore=0 mlxscore=0 malwarescore=0 mlxlogscore=999 phishscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2009150000 definitions=main-2102030022 X-Proofpoint-Virus-Version: vendor=nai engine=6200 definitions=9883 signatures=668683 X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 bulkscore=0 adultscore=0 priorityscore=1501 impostorscore=0 malwarescore=0 clxscore=1015 spamscore=0 lowpriorityscore=0 phishscore=0 mlxlogscore=999 mlxscore=0 suspectscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2009150000 definitions=main-2102030023 Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit On 2021-01-31 7:13 p.m., Laszlo Ersek wrote: > On 01/29/21 01:59, Ankur Arora wrote: >> Introduce UnplugCpus() which maps each APIC ID being unplugged >> onto the hardware ID of the processor and informs PiSmmCpuDxeSmm >> of removal by calling EFI_SMM_CPU_SERVICE_PROTOCOL.RemoveProcessor(). >> >> With this change we handle the first phase of unplug where we collect >> the CPUs that need to be unplugged and mark them for removal in SMM >> data structures. >> >> 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 | 84 ++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 84 insertions(+) >> >> diff --git a/OvmfPkg/CpuHotplugSmm/CpuHotplug.c b/OvmfPkg/CpuHotplugSmm/CpuHotplug.c >> index 05b1f8cb63a6..70d69f6ed65b 100644 >> --- a/OvmfPkg/CpuHotplugSmm/CpuHotplug.c >> +++ b/OvmfPkg/CpuHotplugSmm/CpuHotplug.c >> @@ -188,6 +188,88 @@ RevokeNewSlot: >> } >> >> /** >> + Process to be hot-unplugged CPUs, per QemuCpuhpCollectApicIds(). >> + >> + For each such CPU, report the CPU to PiSmmCpuDxeSmm via >> + EFI_SMM_CPU_SERVICE_PROTOCOL. If the to be hot-unplugged CPU is >> + unknown, skip it silently. >> + >> + @param[in] ToUnplugApicIds The APIC IDs of the CPUs that are about to be >> + hot-unplugged. >> + >> + @param[in] ToUnplugCount The number of filled-in APIC IDs in >> + ToUnplugApicIds. >> + >> + @retval EFI_SUCCESS Known APIC IDs have been removed from SMM data >> + structures. >> + >> + @return Error codes propagated from >> + mMmCpuService->RemoveProcessor(). >> + > > (1) Please drop this empty line (just before the '**/'). > > >> +**/ >> +STATIC >> +EFI_STATUS >> +UnplugCpus ( >> + IN APIC_ID *ToUnplugApicIds, >> + IN UINT32 ToUnplugCount >> + ) >> +{ >> + EFI_STATUS Status; >> + UINT32 ToUnplugIdx; >> + UINTN ProcessorNum; >> + >> + ToUnplugIdx = 0; >> + while (ToUnplugIdx < ToUnplugCount) { >> + APIC_ID RemoveApicId; >> + >> + RemoveApicId = ToUnplugApicIds[ToUnplugIdx]; >> + >> + // >> + // mCpuHotPlugData->ApicId maps ProcessorNum -> ApicId. Use it to find >> + // the ProcessorNum for the APIC ID to be removed. >> + // >> + for (ProcessorNum = 0; >> + ProcessorNum < mCpuHotPlugData->ArrayLength; >> + ProcessorNum++) { >> + if (mCpuHotPlugData->ApicId[ProcessorNum] == RemoveApicId) { >> + break; >> + } >> + } >> + >> + // >> + // Ignore the unplug if APIC ID not found >> + // >> + if (ProcessorNum == mCpuHotPlugData->ArrayLength) { >> + DEBUG ((DEBUG_INFO, "%a: did not find APIC ID " FMT_APIC_ID >> + " to unplug\n", __FUNCTION__, RemoveApicId)); > > (2) Please use DEBUG_VERBOSE here. > > (I agree that we should have *one* DEBUG_INFO message that relates to > the removal of an individual processor; however, I think we should emit > that message when we finally signal QEMU to eject the processor.) Based on our discussion around establishing the correspondence between The ProcessorNum, APIC-ID and CPU selector, I'll change this to DEBUG_VERBOSE and add a new DEBUG_INFO print after successfully putting it in the APICIdMap. > > > (3) Please un-indent ("outdent"?) the second line by two spaces. > > >> + ToUnplugIdx++; >> + continue; >> + } >> + >> + // >> + // Mark ProcessorNum for removal from SMM data structures >> + // >> + Status = mMmCpuService->RemoveProcessor (mMmCpuService, ProcessorNum); >> + > > (4) It would be more idiomatic to remove this empty line (between Status > assignment and check). > > >> + if (EFI_ERROR (Status)) { >> + DEBUG ((DEBUG_ERROR, "%a: RemoveProcessor(" FMT_APIC_ID "): %r\n", >> + __FUNCTION__, RemoveApicId, Status)); >> + goto Fatal; > > (5) Please just "return Status" here, and drop the "Fatal" label. > > >> + } >> + >> + ToUnplugIdx++; >> + } >> + >> + // >> + // We've removed this set of APIC IDs from SMM data structures. >> + // >> + return EFI_SUCCESS; >> + >> +Fatal: >> + return Status; >> +} >> + >> +/** >> CPU Hotplug MMI handler function. >> >> This is a root MMI handler. >> @@ -303,6 +385,8 @@ CpuHotplugMmi ( >> >> if (PluggedCount > 0) { >> Status = ProcessHotAddedCpus (mPluggedApicIds, PluggedCount); >> + } else if (ToUnplugCount > 0) { >> + Status = UnplugCpus (mToUnplugApicIds, ToUnplugCount); >> } >> >> if (EFI_ERROR(Status)) { >> > > (6) Hmm... What's the reason for the exclusivity? > > Why is the following not better: > > if (PluggedCount > 0) { > Status = ProcessHotAddedCpus (mPluggedApicIds, PluggedCount); > if (EFI_ERROR (Status)) { > goto Fatal; > } > } > if (ToUnplugCount > 0) { > Status = UnplugCpus (mToUnplugApicIds, ToUnplugCount); > if (EFI_ERROR (Status)) { > goto Fatal; > } > } > > QemuCpuhpCollectApicIds() intentionally populates both arrays in a > single go. As I suggested earlier: > > https://lists.gnu.org/archive/html/qemu-devel/2020-11/msg06711.html > msgid: > >> [...] please handle plugs first, for which unused slots in >> mCpuHotPlugData.ApicId will be populated, and *then* handle removals >> (in the same invocation of CpuHotplugMmi()). > > Did that turn out as unviable (the "same invocation of CpuHotplugMmi()" > part)? No I had some confusion while looking at the underlying AddProcessor() and RemoveProcessor() logic. Looking at it again, it should work. Will fix. Also acking the rest of your comments here. Thanks Ankur > > > (7) As a side note, addressing point (6) above would allow you to > address my point (13) from the v5 patch#1 thread, too; i.e., nesting the > Status check under (PluggedCount > 0). > > Thanks! > Laszlo >