From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from userp2120.oracle.com (userp2120.oracle.com [156.151.31.85]) by mx.groups.io with SMTP id smtpd.web11.2732.1614113510009923539 for ; Tue, 23 Feb 2021 12:51:50 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@oracle.com header.s=corp-2020-01-29 header.b=RAAGj0kG; spf=pass (domain: oracle.com, ip: 156.151.31.85, mailfrom: ankur.a.arora@oracle.com) Received: from pps.filterd (userp2120.oracle.com [127.0.0.1]) by userp2120.oracle.com (8.16.0.42/8.16.0.42) with SMTP id 11NKoSdb095406; Tue, 23 Feb 2021 20:51:46 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=AJcyIBhVL0uHpPhmR6j0Z8nA2fpNclxwzv1SN68oI0w=; b=RAAGj0kGuOZnvLE3GsLJIhF6teb3PyIwTr+I+XkSFO3xVNFsFyJjxUuK9XbHaNazmMam W5GYdZeIHYxDtRvoue39tNbcSl+YdA6XVI3EAtoDI1ZU8mdTC6tCsmc6RgKh3Njhrsxu RjPw2fcFjjmeoRgXpv1oe+EdMuyTL0fFOX1eeF8ho4tDu0vaMsEoFlBKSPv9pOJTj25E w0Fh7RiCxHlYQpcUPQGfAELq7oMoW+cJif23FegmiC+vhTotXhjio/p+BRQ0ZHad0oUB +1y6TPvaT9GUCFVegNA3qyC3XaTEDZ95q1L3ylQLA71P7f2OiVvSOycIjGv2jMykqRwU lg== Received: from userp3020.oracle.com (userp3020.oracle.com [156.151.31.79]) by userp2120.oracle.com with ESMTP id 36ugq3fny5-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Tue, 23 Feb 2021 20:51:46 +0000 Received: from pps.filterd (userp3020.oracle.com [127.0.0.1]) by userp3020.oracle.com (8.16.0.42/8.16.0.42) with SMTP id 11NKoIl4089715; Tue, 23 Feb 2021 20:51:46 GMT Received: from nam10-mw2-obe.outbound.protection.outlook.com (mail-mw2nam10lp2108.outbound.protection.outlook.com [104.47.55.108]) by userp3020.oracle.com with ESMTP id 36uc6s9068-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Tue, 23 Feb 2021 20:51:45 +0000 ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=AboNb5PNAU9MbkZj/iDaleHFPVHWvcCeJdXM+DqLgWtPKoqCI5smccIYKhga6JUt1FG22Xt6EL1pjiE3+iGuErLF5L/I+37dSuBVs5XjOTtewcd2zDT5/JCuOz67MjyfmJT8GpADSzvkRqtOoRGzSjAku+ikzxQa5PufEpno4Is5cC+sHYDE4JVueEhLcLcoMASzA+Qo9b3zstAp1K9qOHBqOh8PzhzR+DaUR0vOZ441aFTSg+hK3/S/uw4F6y201v8lKhPXjdAxgWrgRJVux00xbPxI2gd5mZrPPn65x3MHWsyjgZkVu2E4UiMZcdME89Sx+Tyv+rowRsHmAP/aEw== 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=AJcyIBhVL0uHpPhmR6j0Z8nA2fpNclxwzv1SN68oI0w=; b=XTotmUJJvijuzbhsp+Qw9uOQsF2Y260UWH5eMjZyjZ9oWALppjpPnzRUGT6WizS8rPm1LU0VmUPoVqKlgFFWUIXVt7acuPLGKZi8grCDpQ9RY9aZ3bn2+TLov+0vy/x53hxd38qh/B6vGI5tOqK4KBiYIs2lEp4PA2k1wfJmRjm0N16JdxDaOW0kWY2LZr2OTLQZ2lfDe+HhyO7bc4TZL+GOLwwlP7b53sfoUPSeIVKmmEpz8E6b3PM1cAL8DajIe6D77AMpAQCzKO6cESI8Wmmw1OdyDSfyOiuc81xi96KlzKV1g6Sdi/x2ereC6ExiWSrBI1ArJpl0Z364Vy5CGQ== 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=AJcyIBhVL0uHpPhmR6j0Z8nA2fpNclxwzv1SN68oI0w=; b=Mz/wdsKdxSC9cevIwNhG+E8IqPovLM4gXr/Xirw5IWtdH1IhGK8WEjrm4RLPocU+f1oRybbClbq/cb93MOZjtAINUfuCwlMT34XxxD4jLNX5YJnUGyHlzBtTlLW3cjc27vPbaMPNjqJFUGNiOdDwo6O5sz5xdpYg1XS4sxviG+I= Received: from SJ0PR10MB4605.namprd10.prod.outlook.com (2603:10b6:a03:2d9::24) by BYAPR10MB4085.namprd10.prod.outlook.com (2603:10b6:a03:11f::13) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.3890.19; Tue, 23 Feb 2021 20:51:43 +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; Tue, 23 Feb 2021 20:51:43 +0000 Subject: Re: [edk2-devel] [PATCH v8 08/10] OvmfPkg/CpuHotplugSmm: add EjectCpu() 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-9-ankur.a.arora@oracle.com> <6e37922f-afbe-2b41-4eb9-4b3c62d28aa9@redhat.com> From: "Ankur Arora" Message-ID: Date: Tue, 23 Feb 2021 12:51:39 -0800 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.6.0 In-Reply-To: <6e37922f-afbe-2b41-4eb9-4b3c62d28aa9@redhat.com> X-Originating-IP: [148.87.23.8] X-ClientProxiedBy: MWHPR2201CA0037.namprd22.prod.outlook.com (2603:10b6:301:16::11) 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.8) by MWHPR2201CA0037.namprd22.prod.outlook.com (2603:10b6:301:16::11) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.3890.19 via Frontend Transport; Tue, 23 Feb 2021 20:51:42 +0000 X-MS-PublicTrafficType: Email X-MS-Office365-Filtering-Correlation-Id: 831b8cc2-2345-4b26-2d25-08d8d83cd385 X-MS-TrafficTypeDiagnostic: BYAPR10MB4085: X-MS-Exchange-Transport-Forked: True X-Microsoft-Antispam-PRVS: X-MS-Oob-TLC-OOBClassifiers: OLM:3631; X-MS-Exchange-SenderADCheck: 1 X-Microsoft-Antispam: BCL:0; X-Microsoft-Antispam-Message-Info: mCrFT67PqY36H652opoeZnpfzEkLeFos1uvPZuF0V0oX+c/rnEHMApaHRGz31ml7+mVTs0UHnQAIKJEirqtlmYmYm10BScyhqnAqcLUTQTYtjVJnxY2Rnq0o1pbDSW1QAqDn2pB7l2anEZyzR/1dV2bIT88kjy3iiTPUOHfVLph8YvUjGrcRY/m2X6PojqMIacpuRBL6luL+PvSwM6STGrQRqPDGqWc1ntTBCeHAjjHYoaZB/qBu1wnRZnQi0G/5SwLxTGjgwrUO6izyCS8EHVud88YljwBwZkuaDrkcZnzlReGKI0BWGlpWrJxInAn6iEBcVx1qOoCOqZXQ/2UQodr61gkB5HNpNkL3hQM1rOWXpUayzbWC4ZUDJ0ztH7itVKDayvNR9yMhIBcO+kJ3YhViETDHZ1JyAGSIch1Bs93OW9lx3cy1RNLGBdTATfv8V7VLnErcXG+b8rI5Umbmx0a+YsaSoqkp86bpHvk1AWHOVuSGVnPg+1FCQbwRuh9voTSsf0cDnelVwVG+j3q67RRb9lv09VHBai7n9YBji50uaHBk6cpVYZ31QidacZnpE8lJcUvdbSwPUdSmSAjXx+XLZ62EMkPODjqhinIDiIzH+TPGFvmJns3Z+ZoHnxmqv4BX3fIKCGD0NDvyUMkZ+rN8HKbGU4cI4yVYqI9SOxGx8txjRvFYrYRX6OHgr9KG 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)(39860400002)(136003)(396003)(366004)(376002)(478600001)(6666004)(66946007)(316002)(8676002)(966005)(16576012)(4326008)(54906003)(36756003)(107886003)(31686004)(6486002)(30864003)(2906002)(53546011)(31696002)(86362001)(956004)(2616005)(66556008)(83380400001)(186003)(8936002)(5660300002)(66476007)(26005)(16526019)(43740500002)(45980500001);DIR:OUT;SFP:1101; X-MS-Exchange-AntiSpam-MessageData: =?utf-8?B?S3U4ZlUraTh6aUNUbUdUTkhCODJSbUhSSVZzUHZ1UGlZcVl3ZHh6WXZGa251?= =?utf-8?B?eVcyNzdJb2FUb3drR3pHamxFRXlFWFVsMk84NG1LVEM3eGFpNGdnS3V5Vzhl?= =?utf-8?B?ZEYraHhaTmh2OC9jRElJRHZIZ0d0VjdSYmFuaXFDUEN6QS9qM2FOcXdHRHZO?= =?utf-8?B?NzFEK3hwQXYzQXRpYkFSeEJ1YzE4bWZoYjJITE5DeW4yemJZcXRsZ01yanhZ?= =?utf-8?B?Q0RIcEVTNzk2ZGFMaUFVVkZLbWZocWNKekcxRUpHazlEZ2FzbkkvSmtwMHZW?= =?utf-8?B?QkZ5WGo4UWhLRlZKSE1rTDlGbU9mTkRSQWVmVStlNFJoTVFhUjVRSElDTllI?= =?utf-8?B?WlJqSXlVZ3B6alFYMlFjZkMxbEhYMWlIK3ZSSkRIWkRTRGtGTnJieUxGSjFj?= =?utf-8?B?Zk5wYzM4TnVlcFprT1FXcDc2ZFRpOThIWkZwVFVaYWZra2VEdURUUFFFdW9u?= =?utf-8?B?SVdBVENVNWVmR0VtTUppYUxHeUdEY0JySlhKaEwzMWVqQStqN25nUk1tL2NC?= =?utf-8?B?UVdTZzdKVVVpbmdTbEFKeUNDdGw5eXoyRFRKbEdxOXhBaDI4T20yQ3dvRVhW?= =?utf-8?B?MmRrK3I4SzNiTXc2WFNoc0xrQ3d4eGhuNlhJU21YODRydGdlS2hDWjZiSHUv?= =?utf-8?B?bzQ4dXZXUWJlVENETUppVmVEQnBnZFVOM09QOWhCTHJ5bHpSVjIyZ3NvNVdt?= =?utf-8?B?d3pjdTV3SUpHK1N5dndEeTNRZ0FLRlFKUEs0WlF1Y0EvRXl6ZGhnMTB6ajky?= =?utf-8?B?N1hGZ1JIUTFESFNlTklpNXJ5TEdnZ3hOVkJWWXhjRDQ5SkFSdDRBS056V0hT?= =?utf-8?B?RXE3UjNRNEN5YTVieVV6U1dESjJEYkNlSWJvWHBtMWhDMXJBK1VlYWZ0b1du?= =?utf-8?B?aHpSQlJUMVFpaHhCL01iM0NXK3pWOFJlMjh2aWFTZzRMekYvQlJzTGJWRzJi?= =?utf-8?B?NjNzWUIvY0REeHlsdk9VbXdiaXlpdmUwdll1M0NpMTJJRmp4U0F1R1N1N3B1?= =?utf-8?B?SEU0MHE4WlJPQmxjYXNZd0ZXNWRFV296aFJwSDc3eXkvdTRaRS9zUFhiN2dP?= =?utf-8?B?akNDREJUN0NxL2JycFMvOXIwN1NWcVRvOWt4K09RYUlzNXpmV0ZKQmdCSHpG?= =?utf-8?B?ZFJHRGlJZkNnbVQ2V2xGUnBHcVZDdUVRL3htajRXUFFVWVovUlVQSndpVzBH?= =?utf-8?B?ZWZYYlFyRzJqVXFnb1YzSlhsaENyeEVsZmRkSEJUb1I3MzlLdy8xUlJJSEFJ?= =?utf-8?B?cFBtWk1tNitnZ3FDZEVmeVJmWm9GeUVTc1loUk55SnNqYkpQOFZHbStHQXow?= =?utf-8?B?VyswMElKOWVsZU1ocmluQ1d1QVJCWUFrU2duZ3JuZFpablZNOVVGMGNjNktH?= =?utf-8?B?dWw4ZnBabHNLUTljTTMyZ05jN0VpbklvTzFnSXArSHhRQm9QTzhIN0ZSM2dP?= =?utf-8?B?bDFROEVrRHA4eGFOc1Z4T0ZTczdQbWdpdjR2Y1hUQkJTUGtiZk8vU0U1MXJa?= =?utf-8?B?OHhTcVpvdkdJQllxSkhvMjk2NHRYN1g4eUh2b2ZpUEtrQldFalBjdlVVYkpw?= =?utf-8?B?U1BYaHdzcUxtUzFJZ1ZyQWV1dXcxckNPVlNXVU5ISk1wcjhqenRic3U0bVE5?= =?utf-8?B?cTFVWkVVUUFjeU8wUWZCTVpzQ3lDcWVPM0R3aXZPdnlCZm95dzVJcWUxRUJh?= =?utf-8?B?dHEvUkNwVmJWQlIyeHUyV2ZvNHMxcEtiend5aCt2UFBobzRtSDlJOUxhak1G?= =?utf-8?Q?BA0jnIsgFfB0yYPd9QUAG4imwUTeu/tnuhAxi3N?= X-OriginatorOrg: oracle.com X-MS-Exchange-CrossTenant-Network-Message-Id: 831b8cc2-2345-4b26-2d25-08d8d83cd385 X-MS-Exchange-CrossTenant-AuthSource: SJ0PR10MB4605.namprd10.prod.outlook.com X-MS-Exchange-CrossTenant-AuthAs: Internal X-MS-Exchange-CrossTenant-OriginalArrivalTime: 23 Feb 2021 20:51:43.2637 (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: oggvxsEMsD9EAR8lfcHKuLl8XgbovlDwZjS/9rFPOdW+ni/DvGpkE1NkQkHY1la9vhs5TRy2olw/gQcE0wBP/hvb1LNoggPjqaV1zMMOT5s= X-MS-Exchange-Transport-CrossTenantHeadersStamped: BYAPR10MB4085 X-Proofpoint-Virus-Version: vendor=nai engine=6200 definitions=9904 signatures=668683 X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 suspectscore=0 mlxscore=0 spamscore=0 mlxlogscore=999 adultscore=0 bulkscore=0 malwarescore=0 phishscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2009150000 definitions=main-2102230176 X-Proofpoint-Virus-Version: vendor=nai engine=6200 definitions=9904 signatures=668683 X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 adultscore=0 phishscore=0 malwarescore=0 spamscore=0 mlxscore=0 suspectscore=0 priorityscore=1501 clxscore=1015 impostorscore=0 lowpriorityscore=0 mlxlogscore=999 bulkscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2009150000 definitions=main-2102230176 Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit On 2021-02-23 12:36 p.m., Laszlo Ersek wrote: > superficial comments only; the patch is nearly ready: > > On 02/22/21 08:19, Ankur Arora wrote: >> Add EjectCpu(), which handles the CPU ejection, and provides a holding >> area for said CPUs. It is called via SmmCpuFeaturesRendezvousExit(), >> at the tail end of the SMI handling. >> >> Also UnplugCpus() now stashes QEMU Selectors of CPUs which need to be >> ejected in CPU_HOT_EJECT_DATA.QemuSelectorMap. This is used by >> EjectCpu() to identify CPUs marked for ejection. >> >> 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: >> Address these review comments from v6: >> (1) s/CpuEject/EjectCpu/g >> (2) Ensure that the added include is in sorted order. >> (3) Switch to a cheaper CpuSleep() based loop instead of >> CpuDeadLoop(). Also add the CpuLib LibraryClass. >> (4) Remove the nested else clause >> (5) Use Laszlo's much clearer comment when we try to map multiple >> QemuSelector to the same ProcessorNum. >> (6a) Fix indentation of the debug print in the block in (5). >> (6b,6c,6d) Fix printf types for ProcessorNum, use FMT_APIC_ID for >> APIC_ID and 0x%Lx for QemuSelector[]. >> () As discussed elsewhere add an DEBUG_INFO print logging the >> correspondence between ProcessorNum, APIC_ID, QemuSelector. >> (7a,7b) Use EFI_ALREADY_STARTED instead of EFI_INVALID_PARAMETER and >> document it in the UnplugCpus() comment block. >> () As discussed elsewhere, add the import statement for >> PcdCpuHotEjectDataAddress. >> (9) Use Laszlo's comment in the PcdGet64(PcdCpuHotEjectDataAddress) >> description block. >> (10) Change mCpuHotEjectData init state checks from ASSERT to ones >> consistent with similar checks for mCpuHotPlugData. >> (11-14) Get rid of mCpuHotEjectData init loop: moved to a prior >> patch so it can be done at allocation time. >> (15) s/SmmCpuFeaturesSmiRendezvousExit/SmmCpuFeaturesRendezvousExit/ >> (16,17) Document the ordering requirements of >> mCpuHotEjectData->Handler, and mCpuHotEjectData->QemuSelectorMap. >> >> Not addressed: >> (8) Not removing the EjectCount variable as I'd like to minimize >> stores/loads to CPU_HOT_EJECT_DATA->Handler and so would like to do this >> a single time at the end of the iteration. (It is safe to write multiple >> times to the handler in UnplugCpus() but given the ordering concerns >> around it, it seems cleaner to not access it unnecessarily.) >> >> OvmfPkg/CpuHotplugSmm/CpuHotplugSmm.inf | 2 + >> OvmfPkg/CpuHotplugSmm/CpuHotplug.c | 157 ++++++++++++++++++++++++++++++-- >> 2 files changed, 151 insertions(+), 8 deletions(-) >> >> diff --git a/OvmfPkg/CpuHotplugSmm/CpuHotplugSmm.inf b/OvmfPkg/CpuHotplugSmm/CpuHotplugSmm.inf >> index 04322b0d7855..ebcc7e2ac63a 100644 >> --- a/OvmfPkg/CpuHotplugSmm/CpuHotplugSmm.inf >> +++ b/OvmfPkg/CpuHotplugSmm/CpuHotplugSmm.inf >> @@ -40,6 +40,7 @@ [Packages] >> [LibraryClasses] >> BaseLib >> BaseMemoryLib >> + CpuLib >> DebugLib >> LocalApicLib >> MmServicesTableLib >> @@ -54,6 +55,7 @@ [Protocols] >> >> [Pcd] >> gUefiCpuPkgTokenSpaceGuid.PcdCpuHotPlugDataAddress ## CONSUMES >> + gUefiOvmfPkgTokenSpaceGuid.PcdCpuHotEjectDataAddress ## CONSUMES >> gUefiOvmfPkgTokenSpaceGuid.PcdQ35SmramAtDefaultSmbase ## CONSUMES >> >> [FeaturePcd] >> diff --git a/OvmfPkg/CpuHotplugSmm/CpuHotplug.c b/OvmfPkg/CpuHotplugSmm/CpuHotplug.c >> index f07b5072749a..851e2b28aad9 100644 >> --- a/OvmfPkg/CpuHotplugSmm/CpuHotplug.c >> +++ b/OvmfPkg/CpuHotplugSmm/CpuHotplug.c >> @@ -10,10 +10,12 @@ >> #include // ICH9_APM_CNT >> #include // QEMU_CPUHP_CMD_GET_PENDING >> #include // CpuDeadLoop() >> +#include // CpuSleep() >> #include // ASSERT() >> #include // gMmst >> #include // PcdGetBool() >> #include // SafeUintnSub() >> +#include // CPU_HOT_EJECT_DATA >> #include // EFI_MM_CPU_IO_PROTOCOL >> #include // EFI_SMM_CPU_SERVICE_PROTOCOL >> #include // EFI_STATUS >> @@ -32,11 +34,12 @@ STATIC EFI_MM_CPU_IO_PROTOCOL *mMmCpuIo; >> // >> STATIC EFI_SMM_CPU_SERVICE_PROTOCOL *mMmCpuService; >> // >> -// This structure is a communication side-channel between the >> +// These structures serve as communication side-channels between the >> // EFI_SMM_CPU_SERVICE_PROTOCOL consumer (i.e., this driver) and provider >> // (i.e., PiSmmCpuDxeSmm). >> // >> STATIC CPU_HOT_PLUG_DATA *mCpuHotPlugData; >> +STATIC CPU_HOT_EJECT_DATA *mCpuHotEjectData; >> // >> // SMRAM arrays for fetching the APIC IDs of processors with pending events (of >> // known event types), for the time of just one MMI. >> @@ -188,18 +191,72 @@ 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 being ejected, wait in a halted loop >> + until ejected. >> + >> + @param[in] ProcessorNum ProcessorNum denotes the CPU exiting SMM, >> + and will be used as an index into >> + CPU_HOT_EJECT_DATA->QemuSelectorMap. It is >> + identical to the processor handle number in >> + EFI_SMM_CPU_SERVICE_PROTOCOL. >> +**/ >> +VOID >> +EFIAPI >> +EjectCpu ( >> + IN UINTN ProcessorNum >> + ) >> +{ >> + UINT64 QemuSelector; >> + >> + QemuSelector = mCpuHotEjectData->QemuSelectorMap[ProcessorNum]; >> + if (QemuSelector == CPU_EJECT_QEMU_SELECTOR_INVALID) { >> + return; >> + } >> + >> + // >> + // CPU(s) being unplugged get here from SmmCpuFeaturesRendezvousExit() >> + // after having been cleared to exit the SMI by the BSP and thus have >> + // no SMM processing remaining. >> + // >> + // Given that we cannot allow them to escape to the guest, we pen them >> + // here until the BSP tells QEMU to unplug them. >> + // >> + for (;;) { >> + DisableInterrupts (); >> + CpuSleep (); >> + } >> +} >> + >> +/** >> 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. >> + EFI_SMM_CPU_SERVICE_PROTOCOL and stash the APIC ID for later ejection. >> + If the to be hot-unplugged CPU is unknown, skip it silently. >> + >> + Additonally, if we do stash any APIC IDs, also install a CPU eject handler >> + which would handle the ejection. > > (1) Please update the two APIC ID references above to QEMU CPU selectors > (the commit message has been updated already, correctly). > > >> >> @param[in] ToUnplugApicIds The APIC IDs of the CPUs that are about to be >> hot-unplugged. >> >> + @param[in] ToUnplugSelector The QEMU Selectors of the CPUs that are about to >> + be hot-unplugged. >> + > > (2) Please rename the parameter to "ToUnplugSelectors" (plural), both > here in the comment and in the actual parameter list. > > >> @param[in] ToUnplugCount The number of filled-in APIC IDs in >> ToUnplugApicIds. >> >> + @retval EFI_ALREADY_STARTED For the ProcessorNum that >> + EFI_SMM_CPU_SERVICE_PROTOCOL had assigned to >> + one of the APIC ID in ToUnplugApicIds, >> + mCpuHotEjectData->QemuSelectorMap already has >> + the QemuSelector value stashed. (This should >> + never happen.) >> + > > (3) Unfortunately I made a typing error in my v6 review where I > suggested this language: it should say > > one of the APIC ID*s* in ToUnplugApicIds > > (emphasis only here, in this comment) > > >> @retval EFI_SUCCESS Known APIC IDs have been removed from SMM data >> structures. >> >> @@ -210,23 +267,36 @@ STATIC >> EFI_STATUS >> UnplugCpus ( >> IN APIC_ID *ToUnplugApicIds, >> + IN UINT32 *ToUnplugSelector, >> IN UINT32 ToUnplugCount >> ) >> { >> EFI_STATUS Status; >> UINT32 ToUnplugIdx; >> + UINT32 EjectCount; >> UINTN ProcessorNum; >> >> ToUnplugIdx = 0; >> + EjectCount = 0; >> while (ToUnplugIdx < ToUnplugCount) { >> APIC_ID RemoveApicId; >> + UINT32 QemuSelector; >> >> RemoveApicId = ToUnplugApicIds[ToUnplugIdx]; >> + QemuSelector = ToUnplugSelector[ToUnplugIdx]; >> >> // >> - // mCpuHotPlugData->ApicId maps ProcessorNum -> ApicId. Use it to find >> - // the ProcessorNum for the APIC ID to be removed. >> + // mCpuHotPlugData->ApicId maps ProcessorNum -> ApicId. Use RemoveApicId >> + // to find the corresponding ProcessorNum for the CPU to be removed. >> // >> + // With this we can establish a 3 way mapping: >> + // APIC_ID -- ProcessorNum -- QemuSelector >> + // >> + // We stash the ProcessorNum -> QemuSelector mapping so it can later be >> + // used for CPU hot-eject in SmmCpuFeaturesRendezvousExit() context (where >> + // we only have ProcessorNum available.) >> + // >> + >> for (ProcessorNum = 0; >> ProcessorNum < mCpuHotPlugData->ArrayLength; >> ProcessorNum++) { >> @@ -255,11 +325,64 @@ UnplugCpus ( >> return Status; >> } >> >> + if (mCpuHotEjectData->QemuSelectorMap[ProcessorNum] != >> + CPU_EJECT_QEMU_SELECTOR_INVALID) { > > (4) Invalid indentation; in this (controlling expression) context, > CPU_EJECT_QEMU_SELECTOR_INVALID should line up with "mCpuHotEjectData". > > >> + // >> + // mCpuHotEjectData->QemuSelectorMap[ProcessorNum] is set to >> + // CPU_EJECT_QEMU_SELECTOR_INVALID when mCpuHotEjectData->QemuSelectorMap >> + // is allocated, and once the subject processsor is ejected. >> + // >> + // Additionally, mMmCpuService->RemoveProcessor(ProcessorNum) invalidates >> + // mCpuHotPlugData->ApicId[ProcessorNum], so a given ProcessorNum can >> + // never match more than one APIC ID and by transitivity, more than one >> + // QemuSelector in a single invocation of UnplugCpus(). >> + // > > (5) good comment, but please make it a tiny bit easier to read: please > insert two "em dashes", as follows: > > // never match more than one APIC ID -- and by transitivity, designate > // more than one QemuSelector -- in a single invocation of UnplugCpus(). > > (I've also inserted the verb "designate", because "match" doesn't seem > to apply in that context.) > > >> + DEBUG ((DEBUG_ERROR, "%a: ProcessorNum %Lu maps to QemuSelector 0x%Lx, " >> + "cannot also map to 0x%Lx\n", __FUNCTION__, (UINT64)ProcessorNum, >> + (UINT64)mCpuHotEjectData->QemuSelectorMap[ProcessorNum], QemuSelector)); > > (6a) "mCpuHotEjectData->QemuSelectorMap[ProcessorNum]" has type UINT64, > so the cast is superfluous > > (6b) we log selectors in all other places in decimal, so please use %Lu > here, not 0x%Lx, for logging > "mCpuHotEjectData->QemuSelectorMap[ProcessorNum]" > > (6c) "QemuSelector" has type UINT32 (correctly), so we need to log it > with either "0x%x" or "%u". Given my above point about logging selectors > in decimal everywhere else, please use "%u". > > DEBUG ((DEBUG_ERROR, "%a: ProcessorNum %Lu maps to QemuSelector %Lu, " > "cannot also map to %u\n", __FUNCTION__, (UINT64)ProcessorNum, > mCpuHotEjectData->QemuSelectorMap[ProcessorNum], QemuSelector)); > > >> + >> + Status = EFI_ALREADY_STARTED; >> + return Status; >> + } > > (7) style nit -- this looks better as "return EFI_ALREADY_STARTED". > > >> + >> + // >> + // Stash the QemuSelector so we can do the actual ejection later. >> + // >> + mCpuHotEjectData->QemuSelectorMap[ProcessorNum] = (UINT64)QemuSelector; >> + >> + DEBUG ((DEBUG_INFO, "%a: Started hot-unplug on ProcessorNum %Lu, APIC ID " >> + FMT_APIC_ID ", QemuSelector 0x%Lx\n", __FUNCTION__, (UINT64)ProcessorNum, >> + RemoveApicId, mCpuHotEjectData->QemuSelectorMap[ProcessorNum])); > > (8a) In the format string, please replace "QemuSelector 0x%Lx" with > "QemuSelector %u" -- the main point is the decimal notation > > (8b) As a suggestion, I think we should simplify this DEBUG call by > replacing the "mCpuHotEjectData->QemuSelectorMap[ProcessorNum]" argument > with just "QemuSelector". > > DEBUG ((DEBUG_INFO, "%a: Started hot-unplug on ProcessorNum %Lu, APIC ID " > FMT_APIC_ID ", QemuSelector %u\n", __FUNCTION__, (UINT64)ProcessorNum, > RemoveApicId, QemuSelector)); > > >> + >> + EjectCount++; > > (I'm fine with keeping the "EjectCount" variable.) > > >> ToUnplugIdx++; >> } >> >> + if (EjectCount != 0) { >> + // >> + // We have processors to be ejected; install the handler. >> + // >> + mCpuHotEjectData->Handler = EjectCpu; >> + >> + // >> + // The BSP, CPUs to be ejected dereference mCpuHotEjectData->Handler, and >> + // mCpuHotEjectData->QemuSelectorMap[] in SmmCpuFeaturesRendezvousExit(). >> + // >> + // Assignments to both of these are ordered-before the BSP's SMI exit signal >> + // which happens via a write to SMM_DISPATCHER_MP_SYNC_DATA->AllCpusInSync. >> + // Dereferences of both are ordered-after the synchronization via >> + // "AllCpusInSync". >> + // >> + // So we are guaranteed that the Handler would see the assignments above. >> + // However, add a MemoryFence() here in-lieu of a compiler barrier to >> + // ensure that the compiler doesn't monkey around with the stores. >> + // >> + MemoryFence (); >> + } >> + > > (9) Per previous discussion (under patch v8 #7), please replace the last > paragraph of this comment, with a "ReleaseMemoryFence" reference. Will do. And acking the comments above. Ankur > > The rest looks good.> > Thanks! > Laszlo > >> // >> - // We've removed this set of APIC IDs from SMM data structures. >> + // We've removed this set of APIC IDs from SMM data structures and >> + // have installed an ejection handler if needed. >> // >> return EFI_SUCCESS; >> } >> @@ -387,7 +510,7 @@ CpuHotplugMmi ( >> } >> >> if (ToUnplugCount > 0) { >> - Status = UnplugCpus (mToUnplugApicIds, ToUnplugCount); >> + Status = UnplugCpus (mToUnplugApicIds, mToUnplugSelector, ToUnplugCount); >> if (EFI_ERROR (Status)) { >> goto Fatal; >> } >> @@ -458,9 +581,14 @@ CpuHotplugEntry ( >> >> // >> // Our DEPEX on EFI_SMM_CPU_SERVICE_PROTOCOL guarantees that PiSmmCpuDxeSmm >> - // has pointed PcdCpuHotPlugDataAddress to CPU_HOT_PLUG_DATA in SMRAM. >> + // has pointed: >> + // - PcdCpuHotPlugDataAddress to CPU_HOT_PLUG_DATA in SMRAM, >> + // - PcdCpuHotEjectDataAddress to CPU_HOT_EJECT_DATA in SMRAM, if the >> + // possible CPU count is greater than 1. >> // >> mCpuHotPlugData = (VOID *)(UINTN)PcdGet64 (PcdCpuHotPlugDataAddress); >> + mCpuHotEjectData = (VOID *)(UINTN)PcdGet64 (PcdCpuHotEjectDataAddress); >> + >> if (mCpuHotPlugData == NULL) { >> Status = EFI_NOT_FOUND; >> DEBUG ((DEBUG_ERROR, "%a: CPU_HOT_PLUG_DATA: %r\n", __FUNCTION__, Status)); >> @@ -472,6 +600,19 @@ CpuHotplugEntry ( >> if (mCpuHotPlugData->ArrayLength == 1) { >> return EFI_UNSUPPORTED; >> } >> + >> + if (mCpuHotEjectData == NULL) { >> + Status = EFI_NOT_FOUND; >> + } else if (mCpuHotPlugData->ArrayLength != mCpuHotEjectData->ArrayLength) { >> + Status = EFI_INVALID_PARAMETER; >> + } else { >> + Status = EFI_SUCCESS; >> + } >> + if (EFI_ERROR (Status)) { >> + DEBUG ((DEBUG_ERROR, "%a: CPU_HOT_EJECT_DATA: %r\n", __FUNCTION__, Status)); >> + goto Fatal; >> + } >> + >> // >> // Allocate the data structures that depend on the possible CPU count. >> // >> >