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.39704.1612207299372004776 for ; Mon, 01 Feb 2021 11:21:39 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@oracle.com header.s=corp-2020-01-29 header.b=RjoK7s26; 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 111JIxqE169292; Mon, 1 Feb 2021 19:21:35 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=3F1I+2wbrgQ8w9wsfJ1lP7Q/bLYgv+Tr29WLS7CmRzM=; b=RjoK7s26NYlRn3ASQ0oYfaGl4QIF11Pk2oL9TjxpjM1L7mImXDihwhCZbofJR50MhWek kpYbvw1Y/MgqIKI245+DORsVbfBUXybrfqLdFKyNjrMfwKpu0vtNiiRzlFbGFGAiymGa 4tw/BXyUXvXeQyaXEdPhJNtJZ8FtpquDl8S6gFpx5zt0hHKX3RRJ7KF8leuB/t4d6bMQ EGh4PIbTToRSuS4tZ+bE1WWz8gUOjMvJY5W9iwAxShS7MHHOKBVHEpfzgU8369+H5Mqk Soh05Wqr53JvLTNTKnUbMVMhj8w6EJmU1IlD9i1yADSPsB3vtNVqd/fTszOvyVe2ny99 Nw== Received: from userp3030.oracle.com (userp3030.oracle.com [156.151.31.80]) by aserp2120.oracle.com with ESMTP id 36cydkq1ja-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Mon, 01 Feb 2021 19:21:35 +0000 Received: from pps.filterd (userp3030.oracle.com [127.0.0.1]) by userp3030.oracle.com (8.16.0.42/8.16.0.42) with SMTP id 111JKUZu165028; Mon, 1 Feb 2021 19:21:34 GMT Received: from nam04-dm6-obe.outbound.protection.outlook.com (mail-dm6nam08lp2043.outbound.protection.outlook.com [104.47.73.43]) by userp3030.oracle.com with ESMTP id 36dhcvf1jw-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Mon, 01 Feb 2021 19:21:34 +0000 ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=D+FdUI44pnhiFn1GN+bd9+BVuGEnAgqI1rnvqH6GTqAKNTBuC+T8nmVR7w/BLcfGpOQfaMp9mY/bX1AdR8sPKfrZNxRvaLbxC7d4aTvC8wBoXafKAr7oDjQSyPcA9SKgpFVDChe2wRxHnkR8+lxG4TE3vIH5dCVOLR08rErBgmSlqPdVzJ8PmV4lu7BhtsUmv+quEgKggPirQ/0QS4Qhoeh2PZwwcVW3bxWYMHew2sxt8p7d0SMMA0N0aCXOiniLp2yIs1LU9zyv/deCHFRCEFHuFkjzUX1uTUSRDFcrglJzzZbHYlvZ4uQmrGgRuMFwn8jmulOziCQo1wMEtGSFeg== 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=3F1I+2wbrgQ8w9wsfJ1lP7Q/bLYgv+Tr29WLS7CmRzM=; b=fGMJp5E9jJ1zi/s/9NmhjKB79l7inqHUTaIcZy1XK1nJ8LgO0VYvTe/3L07conaIZwW+BvYyGu2YJjkmImyZ09AyVUxh1rFlcFO83d/0ixHQR4RTYrMkRp7/p3BHf+thpbQnzVfI3YPLNYiXQae6Dr25umpJeqGFT6biwCqR7Bj/3qIXhbl2EZDzKw2qBoYZSat2dY/1cc0Ey6NEk3fi/eH7UHYei7wwYfU7zPM95cKZ1QGmfgm8vKF4NlEi7pj8dDYPWWXoDMQidVzuazDATxxbTXXZlkSVRPssX/ycOrb74N7b7ukVZt7oGCJo6YoAYCS+7vL2sCTE0YzHzSIWtA== 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=3F1I+2wbrgQ8w9wsfJ1lP7Q/bLYgv+Tr29WLS7CmRzM=; b=Bj7j2zkUnQiVVT4B/2f10EQ5sH+F4WGzeo1URt4Z41PC05LIbu4PkHoObzCBtzDu+AOGyd9F5DkBTAnUAjZcMZXtyGsJSZabdX5Ozy8mSZPv9z260SBmkcf4HNcCu+CAFqr5EYryscJUqzja0RRq/vdlCIimrn/GJbcZYDr0wiA= Received: from SJ0PR10MB4605.namprd10.prod.outlook.com (2603:10b6:a03:2d9::24) by SJ0PR10MB4575.namprd10.prod.outlook.com (2603:10b6:a03:2da::15) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.3805.19; Mon, 1 Feb 2021 19:21:31 +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.025; Mon, 1 Feb 2021 19:21:31 +0000 Subject: Re: [PATCH v6 8/9] OvmfPkg/CpuHotplugSmm: add worker to do CPU ejection 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-9-ankur.a.arora@oracle.com> <2c25a9d0-38dc-754d-fcef-b372e1211a92@redhat.com> From: "Ankur Arora" Message-ID: Date: Mon, 1 Feb 2021 11:21:28 -0800 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.6.0 In-Reply-To: <2c25a9d0-38dc-754d-fcef-b372e1211a92@redhat.com> X-Originating-IP: [70.36.60.91] X-ClientProxiedBy: MW4PR04CA0025.namprd04.prod.outlook.com (2603:10b6:303:69::30) 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] (70.36.60.91) by MW4PR04CA0025.namprd04.prod.outlook.com (2603:10b6:303:69::30) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.3805.17 via Frontend Transport; Mon, 1 Feb 2021 19:21:30 +0000 X-MS-PublicTrafficType: Email X-MS-Office365-Filtering-Correlation-Id: 89fc3b88-b4bf-4cf2-f4da-08d8c6e694a8 X-MS-TrafficTypeDiagnostic: SJ0PR10MB4575: X-MS-Exchange-Transport-Forked: True X-Microsoft-Antispam-PRVS: X-MS-Oob-TLC-OOBClassifiers: OLM:6790; X-MS-Exchange-SenderADCheck: 1 X-Microsoft-Antispam: BCL:0; X-Microsoft-Antispam-Message-Info: YYcacy1avLF8P7C2sY2b63CbhtyX3Jawdrqwb3XJaIfjBFtbT4pofZ0SS190ELdVKoflk5QoX0NfeZ5PYyabLaEXuZzxByg3SLRxnd72L4157vjX3Xn0uTVB94q/EjMzlKUSwfzS35vPnzogVHU3JNIl+XiNYiKZyb3VcPmYHu41VQr066G8iMWcmYnFTCPPmjHWBXDL488st83p/mT/FrPo6q1Y8Ddn84LMcQ6kF5IUNm4DxVNQQIdSXGV2gTU50jwRx/hXAZ+OeWRL2nV1Y/o+BYHp9jXi/LscOth5FxGanFX58sTvE/DIMUHWPgmgruiqRzbVeP8f4EdmrJ/UGYBYKQ3fJNo9Jr6Y8FXTqxhp9VJgY3rHCxzBFtpQYzjpUVJw0NFK5C/BscyT1aV31FpHFRNJwpNs7cMim4T0/NvUw6TYaCsz8wUqGj+mgBXxyWLp85X+0h4j1Taw7mswH/l09UCvbhK+Gv2Gi1PeoEDEp9mupfcfkX2Xwe32MJrrTUB+ptrUego+1GzIE9ZxK+VMQA6O3PbnjxtR6fvnIMqZ6m2Iwmjwv8YTCniMASAh+7df5CQ93C9PLOFvF5EYJkge/+kASxzOPmpQe0LlJGBEOi+xqDNZ/3mqTN0eCkhlP1fDyj0SPcfd0TNrxQL+vS8kg8iQWadiq3ElixHatkvy+c+Sa0Uj1VmjPTJXOMTs 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:(346002)(136003)(366004)(376002)(39860400002)(396003)(8676002)(6486002)(107886003)(4326008)(36756003)(54906003)(2906002)(31686004)(8936002)(66476007)(66946007)(478600001)(5660300002)(26005)(16526019)(186003)(86362001)(19627235002)(2616005)(83380400001)(16576012)(31696002)(316002)(956004)(66556008)(53546011)(966005)(43740500002)(45980500001);DIR:OUT;SFP:1101; X-MS-Exchange-AntiSpam-MessageData: =?utf-8?B?WDkvUTlaQ3NzZU14KzVRZ2ZWSHUvN3Q3UUxKVnJJbFpxQWx6SExzc083SCtD?= =?utf-8?B?cVFZd3c0VUNyWVZ1Ym9id2w0YTV0Y2sraks0UWIzZVRTd2x6cUNOQklrcXJT?= =?utf-8?B?bzZXQ0tmK1B1MEN0dnhiS3JpcGxvemVuaUdIQU14ZEg5enNIeWJDSi9vVTRo?= =?utf-8?B?WDhMOWlaVWRWRTYyU1kvdWxCRDB6NGVzYnpJbjNnalZLUGZkc0MxWVp0TEpi?= =?utf-8?B?alE0YjJXSTVXa1l1UE5MSjNlbXplYzIrMHBtVG9majBkZTFpSkhoeFdZemlh?= =?utf-8?B?U0U0Rm5qTjFHLzk5Y1pBaVdaZkNpUVkxNDZGSml1V2lSbndTVit6TGFqZ0VV?= =?utf-8?B?WHdvMGRiOXpmMzNKK2YrWUpLekwvYnhMNm1MWldwMGFkRjI2NS9oU01nRXdw?= =?utf-8?B?aktYTytZSlFYQ1BTZ3hndlFzeU9OOHc3cU5SQlR3V2hoY1ZxekptaWt2SjhT?= =?utf-8?B?RlEvVWtNU0ZKS0lCZm9aeWFvd3g5NFl5RFUyRmQ3WG01THI2Z3hlQVhObFBt?= =?utf-8?B?MW13NkUvc2x2RHdjL3FSaHFRTVlEMzRPUm5oanZvc1lUTENhQ0Y3NHl0eTEy?= =?utf-8?B?UEVHRlNBVzA4eWh5UndWdnYyeUFaUVR4S3g0RHJSZS84REswN3R1S3dvTEhE?= =?utf-8?B?M0VmSThkNHVacCtZemJEQlRYN2JtZ0Y2eEQ4VlFjZFV3WkdsS2hyem1xeFRo?= =?utf-8?B?K003bk02aXRJL3o5ZjRIaE50TmpueFl4bGRKQkQ5ZnQ3VE9WTnpwMVlFR0Rl?= =?utf-8?B?QUdYQ2xBRUx0TytXWkRKWFdnMjF3ZHRZTDBlcEthcjIzREoxQkF3b01EWGZO?= =?utf-8?B?UmJVeTdhVmo3cUUvYUZmOXdRTkJkOHVFYml1SldEUVA5bFhKM2krdVU5aytq?= =?utf-8?B?RFlrRjlTTS9HUjZQejgvQWR2WURpbWxndlZwaXMzVXRoL25UVGVpS0VWMkVa?= =?utf-8?B?ZVF4ZEoxdTFEVGJZQ2hrNEg4NUUxTUF5a1piRWZEVmxjbkdrUTFHckVKaFlo?= =?utf-8?B?NW56SU1PTWxmUktnQzhocHBCUVZocVdodnJFL29QV3RucTRScEhQRUErNi9n?= =?utf-8?B?MG4rZlgrNFpHMVE2YS9BanVFQlgxUm1JTmVSY2lXd1FMUWYxbkh5czhNbWFo?= =?utf-8?B?T2RQSjBoR2FsNkU4Mitwa2hzamF4dXRLcVRSUkRjanJzb3ArNUpzT1pGQ296?= =?utf-8?B?RTVXbkQxQmZzSXBERWRJazkyQmlEaS9vN0RsK1ZZcUhkMEkwZDdhOTJFeEhK?= =?utf-8?B?dnZyc1MxdnFyYXBLUVBsNG1kWkFPK1YwSGwzNkQvelpyRkFjbjNCS3lxM2hQ?= =?utf-8?B?OUxCZ1A5aGtZWHh5ZC9pVS96MUtJWWdVTUROOWRGNFRvNlZMSitZb0JZNDlL?= =?utf-8?B?VDJTd2FEN1JkTFBRVi9yQWQzZ1NrNEpMTmRlb3dhN0FvMEkyWW85VGRIeEtP?= =?utf-8?Q?cRo3ztiE?= X-OriginatorOrg: oracle.com X-MS-Exchange-CrossTenant-Network-Message-Id: 89fc3b88-b4bf-4cf2-f4da-08d8c6e694a8 X-MS-Exchange-CrossTenant-AuthSource: SJ0PR10MB4605.namprd10.prod.outlook.com X-MS-Exchange-CrossTenant-AuthAs: Internal X-MS-Exchange-CrossTenant-OriginalArrivalTime: 01 Feb 2021 19:21:31.7896 (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: nPLBAX3p85eUHXqm2CVg7kTMUu5Fzg6pDEuimrZWBCHZb672gZRDlfq840ZmiFI2ODUskl1isWRUj52gO7q41PdU2MOYdQVTMgpRUxpIcTg= X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ0PR10MB4575 X-Proofpoint-Virus-Version: vendor=nai engine=6200 definitions=9882 signatures=668683 X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 mlxscore=0 mlxlogscore=999 phishscore=0 spamscore=0 suspectscore=0 malwarescore=0 adultscore=0 bulkscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2009150000 definitions=main-2102010101 X-Proofpoint-Virus-Version: vendor=nai engine=6200 definitions=9882 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-2102010101 Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit On 2021-02-01 9:22 a.m., Laszlo Ersek wrote: > On 01/29/21 01:59, Ankur Arora wrote: >> Designate a worker CPU (we use the one executing the root MMI >> handler), which will do the actual ejection via QEMU in CpuEject(). >> >> CpuEject(), on the worker CPU, ejects each marked CPU by first >> selecting its APIC ID and then sending the QEMU "eject" command. >> QEMU in-turn signals the remote VCPU thread which context-switches >> it out of the SMI. >> >> CpuEject(), on the CPU being ejected, spins around in its holding >> area until this final context-switch. This does mean that there is >> some CPU state that would ordinarily be restored (in SmiRendezvous() >> and in SmiEntry.nasm::CommonHandler), but will not be anymore. >> This unrestored state includes FPU state, CET enable, stuffing of >> RSB and the final RSM. Since the CPU state is destroyed by QEMU, >> this should be okay. >> >> 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 | 73 ++++++++++++++++++++++++++++++++++---- >> 1 file changed, 67 insertions(+), 6 deletions(-) > > (1) s/CpuEject/EjectCpu/g, per previous request (affects commit message > and code too). > > >> diff --git a/OvmfPkg/CpuHotplugSmm/CpuHotplug.c b/OvmfPkg/CpuHotplugSmm/CpuHotplug.c >> index 526f51faf070..bf91344eef9c 100644 >> --- a/OvmfPkg/CpuHotplugSmm/CpuHotplug.c >> +++ b/OvmfPkg/CpuHotplugSmm/CpuHotplug.c >> @@ -193,9 +193,12 @@ RevokeNewSlot: >> CPU Hot-eject handler, called from SmmCpuFeaturesRendezvousExit(), >> on each CPU at exit from SMM. >> >> - If, the executing CPU is not being ejected, nothing to be done. >> + If, the executing CPU is neither a worker, nor being ejected, nothing >> + to be done. >> If, the executing CPU is being ejected, wait in a CpuDeadLoop() >> until ejected. >> + If, the executing CPU is a worker CPU, set QEMU CPU status to eject >> + for CPUs being ejected. >> >> @param[in] ProcessorNum Index of executing CPU. >> >> @@ -217,6 +220,56 @@ CpuEject ( >> return; >> } >> >> + if (ApicId == CPU_EJECT_WORKER) { > > (2) The CPU_EJECT_WORKER approach is needlessly complicated (speculative > generality). I wish I understood this idea earlier in the patch set. > > (2a) In patch #5 (subject "OvmfPkg/CpuHotplugSmm: define > CPU_HOT_EJECT_DATA"), the CPU_EJECT_WORKER macro definition should be > dropped. > > (2b) In this patch, the question whether the executing CPU is the BSP or > not, should be decided with the same logic that is visible in > PlatformSmmBspElection() > [OvmfPkg/Library/SmmCpuPlatformHookLibQemu/SmmCpuPlatformHookLibQemu.c]: > > MSR_IA32_APIC_BASE_REGISTER ApicBaseMsr; > BOOLEAN IsBsp; > > ApicBaseMsr.Uint64 = AsmReadMsr64 (MSR_IA32_APIC_BASE); > IsBsp = (BOOLEAN)(ApicBaseMsr.Bits.BSP == 1); > > (2c) Point (2b) obviates the explicit "mark as worker" logic entirely, > in UnplugCpus() below. > > (2d) The "is worker" language (in comments etc) should be replaced with > direct "is BSP" language. > > >> + UINT32 CpuIndex; >> + >> + for (CpuIndex = 0; CpuIndex < mCpuHotEjectData->ArrayLength; CpuIndex++) { >> + UINT64 RemoveApicId; >> + >> + RemoveApicId = mCpuHotEjectData->ApicIdMap[CpuIndex]; >> + >> + if ((RemoveApicId != CPU_EJECT_INVALID && >> + RemoveApicId != CPU_EJECT_WORKER)) { >> + // >> + // This to-be-ejected-CPU has already received the BSP's SMI exit >> + // signal and, will execute SmmCpuFeaturesSmiRendezvousExit() >> + // followed by this callback or is already waiting in the >> + // CpuDeadLoop() below. >> + // >> + // Tell QEMU to context-switch it out. >> + // >> + QemuCpuhpWriteCpuSelector (mMmCpuIo, (APIC_ID) RemoveApicId); >> + QemuCpuhpWriteCpuStatus (mMmCpuIo, QEMU_CPUHP_STAT_EJECTED); > > (3) While the QEMU CPU selector value *usually* matches the APIC ID, > it's not an invariant. APIC IDs have an internal structure, composed of > bit-fields, where each bit-field accommodates one hierarchy level in the > CPU topology (thread, core, die (maybe), and socket). > > However, this mapping need not be surjective. QEMU lets you create > "pathological" CPU topologies, for example one with: > - 3 threads/core, > - 5 cores/socket, > - (say) 2 sockets. > > Under that example, the bit-field standing for the "thread number" level > would have 2 bits, theoretically permitting *4* threads/core, and the > bit-field standing for the "core number" level would have 3 bits, > theoretically allowing for *8* cores/socket. > > Considering the fully populated topology, you'd see the CPU selector > range from 0 to (3*5*2-1)=29, inclusive (corresponding to 30 logical > processors in total). However, the APIC ID *image* of this CPU selector > *domain* would not be "contiguous" -- the APIC ID space, with the > above-described structure, would accommodate 4*8*2=64 logical > processors. For example, each APIC ID that stood for the nonexistent > "thread#3" on a particular core would be left unused (no CPU selector > would map to it). > > All in all, you can't write the APIC ID to the CPU selector register, > for ejection. You need to select the CPU whose APIC ID is the APIC ID > you want to eject, and then initiate ejection. Yeah, this is a clear bug. Should have seen it earlier. Thanks for pointing it out. > > This requires one of two alternatives: > > > (3a) The first option is to keep the change local to this patch. > > This alternative is the more CPU-hungry (and uglier) one. > > The idea is to perform a QEMU_CPUHP_CMD_GET_ARCH_ID loop over all > possible CPUs, somewhat similarly to QemuCpuhpCollectApicIds(). At every > CPU, knowing the APIC ID, try to find the APIC ID in "ApicIdMap". If > there is a match, eject. > > > (3b) The second option is much more elegant (and it's faster too), but > it requires a much more intrusive update to the patch set. > > First, the *element type* of the arrays that QemuCpuhpCollectApicIds() > operates on, has to be changed from APIC_ID to a structure type that > pairs APIC_ID with the QEMU CPU selector. [*] > > Second, whenever QemuCpuhpCollectApicIds() outputs an APIC_ID, it should > also save the "CurrentSelector" value (in the other field of the output > array element structure). > > Third, the element type of CPU_HOT_EJECT_DATA.ApicIdMap should be > replaced with a structure type similar (or identical) to the one > described at [*]. The ProcessorNumber lookup in UnplugCpus() would still > be based upon the APIC ID, but CPU_HOT_EJECT_DATA should remember both > the QEMU selector for that processor, and the APIC ID. > > Fourth, the actual ejection should use the selector. > > Fifth, the debug message (below) should continue logging the APIC ID, to > mirror the DEBUG_INFO message in ProcessHotAddedCpus(). > > > Would you be willing to implement (3b)? 3b is clearly the better solution. However, is there enough value in the print message containing APIC ID, that CPU_HOT_EJECT_DATA.ApicIdMap carry both the cpu-selector and APIC ID? As you say, the ejection itself just needs the ProcessorNum -> QEMU cpu-selector mapping. Ankur > > >> + >> + // >> + // Compiler barrier to ensure the next store isn't reordered >> + // >> + MemoryFence (); >> + >> + // >> + // Clear the eject status for CpuIndex to ensure that an invalid >> + // SMI later does not end up trying to eject it or a newly >> + // hotplugged CpuIndex does not go into the dead loop. >> + // >> + mCpuHotEjectData->ApicIdMap[CpuIndex] = CPU_EJECT_INVALID; >> + >> + DEBUG ((DEBUG_INFO, "%a: Unplugged CPU %u -> " FMT_APIC_ID "\n", >> + __FUNCTION__, CpuIndex, RemoveApicId)); > > (4) The DEBUG_INFO log message is in the right place (and uses the right > debug mask), but it is afflicted by the usual warts (indentation, format > specifiers etc). Please reapply the comments I made elsewhere. > > > (5a) Please replace "CPU" with "ProcessorNumber" (so that we know it's > the protocol-assigned number, not the QEMU selector). > > (5b) Please replace the arrow " -> " with the string " APIC ID ". > > > Thanks! > Laszlo > >> + } >> + } >> + >> + // >> + // Clear our own worker status. >> + // >> + mCpuHotEjectData->ApicIdMap[ProcessorNum] = CPU_EJECT_INVALID; >> + >> + // >> + // We are done until the next hot-unplug; clear the handler. >> + // >> + mCpuHotEjectData->Handler = NULL; >> + return; >> + } >> + >> // >> // CPU(s) being unplugged get here from SmmCpuFeaturesSmiRendezvousExit() >> // after having been cleared to exit the SMI by the monarch and thus have >> @@ -327,6 +380,19 @@ UnplugCpus ( >> } >> >> if (EjectCount != 0) { >> + UINTN Worker; >> + >> + Status = mMmCpuService->WhoAmI (mMmCpuService, &Worker); >> + ASSERT_EFI_ERROR (Status); >> + // >> + // UnplugCpus() is called via the root MMI handler and thus we are >> + // executing in the BSP context. >> + // >> + // Mark ourselves as the worker CPU. >> + // >> + ASSERT (mCpuHotEjectData->ApicIdMap[Worker] == CPU_EJECT_INVALID); >> + mCpuHotEjectData->ApicIdMap[Worker] = CPU_EJECT_WORKER; >> + >> // >> // We have processors to be ejected; install the handler. >> // >> @@ -451,11 +517,6 @@ CpuHotplugMmi ( >> if (EFI_ERROR (Status)) { >> goto Fatal; >> } >> - if (ToUnplugCount > 0) { >> - DEBUG ((DEBUG_ERROR, "%a: hot-unplug is not supported yet\n", >> - __FUNCTION__)); >> - goto Fatal; >> - } >> >> if (PluggedCount > 0) { >> Status = ProcessHotAddedCpus (mPluggedApicIds, PluggedCount); >> >