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.2460.1614138302869098452 for ; Tue, 23 Feb 2021 19:45:03 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@oracle.com header.s=corp-2020-01-29 header.b=C4fPsZph; 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 11O3ZSAL110511; Wed, 24 Feb 2021 03:44:59 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=3KWxMWxT7L9kDAXNMBEJJhVT/nFvl+NeA1ydUVVYT+k=; b=C4fPsZphv7cKHEARhBgIOh0yCug6oPYqE2B4IpYFRAoNIi6OlsH9p4HBu1LCdC++ygXz QCPbiIiLQLf8M4Bny+6t7/lWydX8Lqe0adO/XHxm9duoSJqep6Les4dLFVIiZcQnmAtG yQ9d8G+O3aqpUUi89+gAey1nraRqGMsn1pSN5QwLodOq4bvIpy7GZFg9SjKzrqrawDOk Cwd/qkWaoGxbIdVvi5MC3n0R3mafbIZKiG4AWQAi95rJBWmyluMTbmJrEJ3Cod2Zb+u3 0f2iI5q9QXMkLIK1gHHhfW2jwtiYh6YR5qmg5uGBQjKChT+4PT69t27XD7WSO7VNKX+T Eg== Received: from aserp3020.oracle.com (aserp3020.oracle.com [141.146.126.70]) by userp2120.oracle.com with ESMTP id 36ugq3gcqt-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Wed, 24 Feb 2021 03:44:58 +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 11O3aqSf124150; Wed, 24 Feb 2021 03:44:58 GMT Received: from nam11-dm6-obe.outbound.protection.outlook.com (mail-dm6nam11lp2169.outbound.protection.outlook.com [104.47.57.169]) by aserp3020.oracle.com with ESMTP id 36ucb075es-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Wed, 24 Feb 2021 03:44:57 +0000 ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=LFT+Y1zEjOAZIU7xUMLcThlufId2xPgrLnaBYC9hsQY6zjcHSi7vMGOwc2stKIriG929Xv+QDILULKl3tsGqzcmNfJMR/89bIhxtOfwMbLo3Y5lCib52pZhBZnXOSutQ+3x/2SLCMKBkGr8KtUAO7RQGwINvhhcoTaFLJHPkJg0xyUc/MArXkr8XYv5U+3GdeBKoIvZBL3LerRQgjKSd7Env8rOSJuDs6YFDr4TCWuEE48/0psIsg/Ee/8YDNa/QLTyZ6JI0JXCAuwzrwRgzejx6nbbwZ5i+fcqK8ggkZ/4rlM7uBNPhfltgIi33UF90EEcfnSPS6ABeQMD+KeLR+A== 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=3KWxMWxT7L9kDAXNMBEJJhVT/nFvl+NeA1ydUVVYT+k=; b=RrZMLuQxImnWR4OsvLp0hlvzQivkkdUL8UacmJPLl0b8EeqiONmrSeiuIxWbI+ubGwyh7Fa29Ry07NsPY9KMdyHiVubqhVg6otaUCMP9+6Vt9wVesgWw9SAh9/xtADHcxGbBT8hLIDUsFfwih6rBVSMqIjkuEYlucss+4Glkex4xsgjDklu8BGHXt5wuqIFcipqq4DRCh75F8JCzvIgK6yuo6E7zvX+ebFnwVJZkbu3hr7/rDiToA77vZ9FI5WoNzJ66R/6hPfhrjE6aMECrWGg4EdREtm13S07OMDpsIXjVuKrTnDItayvkNNbW/7Ux7o4VuN0qsD3JH9my/K1Kdg== 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=3KWxMWxT7L9kDAXNMBEJJhVT/nFvl+NeA1ydUVVYT+k=; b=HNctZz5qQh8RrFPNfzAOYx/8RUzUz8AU5LPzE3anIPQCDQf5U1gSt+ME8vKwE/VZf0gy/z8QMhZWz35N6NxmP9uML2jY4Y72GORkeA35Z3/SiueG8Tj+ec5gWy9y6S+A6s6tOIjMMlhToxit8MRjFp1SUAViaeMSD0Js4NEtdKY= Received: from SJ0PR10MB4605.namprd10.prod.outlook.com (2603:10b6:a03:2d9::24) by BY5PR10MB3922.namprd10.prod.outlook.com (2603:10b6:a03:1fd::19) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.3868.32; Wed, 24 Feb 2021 03:44:55 +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; Wed, 24 Feb 2021 03:44:55 +0000 Subject: Re: [edk2-devel] [PATCH v8 09/10] OvmfPkg/CpuHotplugSmm: do actual CPU hot-eject 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-10-ankur.a.arora@oracle.com> <0f373648-0d88-6151-f2f6-aa185041b576@redhat.com> From: "Ankur Arora" Message-ID: Date: Tue, 23 Feb 2021 19:44:53 -0800 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.6.0 In-Reply-To: <0f373648-0d88-6151-f2f6-aa185041b576@redhat.com> X-Originating-IP: [70.36.60.91] X-ClientProxiedBy: MWHPR03CA0012.namprd03.prod.outlook.com (2603:10b6:300:117::22) 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 MWHPR03CA0012.namprd03.prod.outlook.com (2603:10b6:300:117::22) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.3890.19 via Frontend Transport; Wed, 24 Feb 2021 03:44:55 +0000 X-MS-PublicTrafficType: Email X-MS-Office365-Filtering-Correlation-Id: 0a872b34-e423-495c-c964-08d8d8768cfa X-MS-TrafficTypeDiagnostic: BY5PR10MB3922: X-MS-Exchange-Transport-Forked: True X-Microsoft-Antispam-PRVS: X-MS-Oob-TLC-OOBClassifiers: OLM:411; X-MS-Exchange-SenderADCheck: 1 X-Microsoft-Antispam: BCL:0; X-Microsoft-Antispam-Message-Info: o0Xj5sRmmpcSp6/XAAj0WKpjfZnlfKKAOv+jGIiltjGRRrYfB3sKzlFecJjKWUdb4kn51OnBW8ugD//3w0/S99TsbtpvxpooVzlJBwZgwV+dncaagmpgDcDcHTFNKKWuLZ78xRq1bhPMHzySBJ9r9eyvp0xS7kpiJ6inhqa4h+SzhZU2S42KXipO9Da+FiiO8HrKtcT3MP1M2LbVDzWmH+ILs4MDESzgORNXyH4XiLBu9GhHuWEnRBqcxeYBhahbcvLRnMxAbllXCX9x7aew5RljHU5YkS7REW4RQynkk6pnXZQV0WXR1U4HbGiGY2uYUms8Y+/e1lCLW79ZgEQFc4qvZ7UVPCDU8xNPXc6YFkCwxEztKFbXIIrhFmtuQkr1Ec6mv8JCGpQf6ELFTqFfHtL36v3kE099P0LMVikAb0jXJzMm8duxoOtxQ1R9uwPAPG0CJGDyuQmg4ZxUNcKXs7wPI61JqsF0dIxopKwj4l2Tnsq72XIkzwM3TGAUrf3I+58jTtcGe70WOILkI9ZaAGGr5T3HIaB9zAe5oXC38BMQ4pBNhiWXG8nxd6ASgj9Iucdc0/L6d9a/I9cSyPvO8q6CuYur0cg09EsxfZ/nb5r4WCy9XSKhEHtqzP5xxJ8JZbo+VcOlFaHzWNwtt96juJT5dQFk4iB5ZVRNGo8PhSo3iWt3IJXXTr0FTfoluNVe 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:(39860400002)(396003)(346002)(136003)(376002)(366004)(966005)(186003)(16526019)(8936002)(30864003)(53546011)(26005)(2616005)(83380400001)(19627235002)(31686004)(54906003)(66476007)(478600001)(4326008)(66946007)(8676002)(5660300002)(66556008)(2906002)(16576012)(956004)(6486002)(86362001)(316002)(31696002)(107886003)(36756003)(45980500001)(43740500002);DIR:OUT;SFP:1101; X-MS-Exchange-AntiSpam-MessageData: =?utf-8?B?T3BnWXhQTXplQWxSTGVFNkpvdHVEanU1WWpYZ0pKamtPTGNja2JvSUU5eHM5?= =?utf-8?B?WXRGV3ZZbzk4T3AxSEtLWVE5czQyUUk2RnVZc095Q01zWmtKSGRIVHZENW05?= =?utf-8?B?Y1g0OURGQ3BMV3pqekJEcHNMYnI5ZGk2Q0ZJMHVtVGhVT1plcTRFL3prTGtr?= =?utf-8?B?aVBBSEdlWDFESzZ4N3lReGxMWDJjcytYL2tTa1U2bUdvVnpNT3RVa3NwTHhQ?= =?utf-8?B?WVpzcG9oN28rb0I3T05KREdPWHZUS0dWcSsrcEFPbzQvb21GZnBuUG5kTU42?= =?utf-8?B?aDJsdEhtTHVibEs4cnNEdi9hRlAvajZFcGdWRndiNm85VWZ5TzBJNVVKc1hO?= =?utf-8?B?Ky9XZ2ZHdkE1SFFYYXlIV3J1NXU0WTZuL3pJYk5xRHJZQXdOTk1LMFNidmVv?= =?utf-8?B?QWN4N3hGUE5SOUlRbHo5d1hYZzVVTlhoNzU5OGlVWWVWMGhjMVpMMDFsNXNq?= =?utf-8?B?a1JIVnJ1Q0R6UnJFbFN6UU44Z01LOU9uYjdMSTUrWndWalZTS1h5K1JGNFVu?= =?utf-8?B?Myt0VU1HdmhydExKNlZRUzFuODU2ellSdjhNUEpCbzJCa3c2TmdzMzhZcnRH?= =?utf-8?B?d2lwdnhXTVp4dXRwL3c0a1N0UTBGU3hIRDY2eUJHekRwd21OY25RNHUzL1Yr?= =?utf-8?B?dzlXU2ZPK1dqNXF3UVY1V2YrdVpwbjdvZjYweFZiaDdFbXRZWmE0VHdrZlFv?= =?utf-8?B?bUdPNUxZTEdSUFZQdzBOZHZRSWVYSE5vZ1FRS0NHYkFmWlUvU0tBdjFab1BK?= =?utf-8?B?MXJ5S3pBTm96TWxITDJQY2tSS0NGVlVWSEhWdXUxRC93MzEveU5IcVBTU1h4?= =?utf-8?B?OWw0RWpIajVsdjlIMTJrOUxhYXY0czJwd1FGRis5d2RaWUNZZktrcWJQb1p2?= =?utf-8?B?MTZ4V25URlVRNHEvY2VmOTMwUGtTSlRFdzVrUFNsdzd2N1JXblBiS0hoR1Zm?= =?utf-8?B?TlQyVm9ZdHgwVyt0ZjByaFVwZGx5OWh0L0hBaFVab1lVcjU3ZUdSR3F4Vktw?= =?utf-8?B?SXM2VEpYRWRmdi9aakZFYnNjYzNydTVIN3hSTklMVkM0eDN5VkZabmZLa01h?= =?utf-8?B?MTVlZlBqQ2RmYStEMjJsUFhrNk5JMm9GLzY0cHhWTGdpZHFXUWk1RE12QXV3?= =?utf-8?B?clU3U3ZFVmpuRE1CdyswQ01uWTVLSkdxVjQ1QlQyRllHU3VmbGFEbTY4RGMy?= =?utf-8?B?K1Q1dGE4ZSt4OGhBM254VW1hM0hnUXk5QXkrNjF4MjZnNlE5Mm9WVlVSZzBQ?= =?utf-8?B?QlNRRC9VYnk4bVRHa09Mb1lHeXd2b0x6eFFwcGtPWDJzN3o4TmhQZzgzdzIw?= =?utf-8?B?Z3lMaHUveTFKcE5NRzJ4ZjRRclRJeE9FM3FXdkJGTHptSFhwb3I4SnlFRXNw?= =?utf-8?B?anlsZUlmWndtMTJGeE9ZVHVteWEySVZyM29IV1R6UG1HVjh1N3lqOHFPdktn?= =?utf-8?B?OW1nc3JMVWtnT3BkRkYwc2R2WjZ2M3NBVW5xdGpURXRQOThMR29aYlBpdWhD?= =?utf-8?B?T3FHbEgyd2t2SUJrajczWUVxRHNFeWlmdWxkejJjaVlpc04zNGpSSGN3ZmVj?= =?utf-8?B?SEJLVjJTcXlaNmxJenIwb3VUQm1aZFdOYTc1WDI5QlMwWk5HbWk2Y2dFaFly?= =?utf-8?B?NFNlU1lJT24wVnIrMFNZT2tVK2hKSVl2MDQ5R2cvTVFYMDFyUVlIbjFoMWEz?= =?utf-8?B?N2tyU1dpdEZ2a0hscURZcUNzNFE2a1Q0MmVERnM4MmZ1TGpnNzU0N3k1MWQ4?= =?utf-8?Q?gXPYzCjh2a26LMRqGKvi+zCdpcKTRtqmg+0B1No?= X-OriginatorOrg: oracle.com X-MS-Exchange-CrossTenant-Network-Message-Id: 0a872b34-e423-495c-c964-08d8d8768cfa X-MS-Exchange-CrossTenant-AuthSource: SJ0PR10MB4605.namprd10.prod.outlook.com X-MS-Exchange-CrossTenant-AuthAs: Internal X-MS-Exchange-CrossTenant-OriginalArrivalTime: 24 Feb 2021 03:44:55.7653 (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: OoLgiH33BSllkP+T4oLlbX87T3Or6akRIg9DkXLE3YV8Y6Fq+jSSvKNCwElHKo9nhVQLta7f8kFb3F9FxlcS1875h3znwL3qseuLcJhTL2Y= X-MS-Exchange-Transport-CrossTenantHeadersStamped: BY5PR10MB3922 X-Proofpoint-Virus-Version: vendor=nai engine=6200 definitions=9904 signatures=668683 X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 spamscore=0 suspectscore=0 malwarescore=0 mlxlogscore=999 adultscore=0 bulkscore=0 mlxscore=0 phishscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2009150000 definitions=main-2102240027 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-2102240027 Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit On 2021-02-23 1:39 p.m., Laszlo Ersek wrote: > On 02/22/21 08:19, Ankur Arora wrote: >> Add logic in EjectCpu() to do the actual the CPU ejection. >> >> On the BSP, ejection happens by first selecting the CPU via >> its QemuSelector and then sending the QEMU "eject" command. >> QEMU in-turn signals the remote VCPU thread which context-switches >> the CPU out of the SMI handler. >> >> Meanwhile the CPU being ejected, waits around in its holding >> area until it is context-switched out. Note that it is possible >> that a slow CPU gets ejected before it reaches the wait loop. >> However, this would never happen before it has executed the >> "AllCpusInSync" loop in SmiRendezvous(). >> It can mean that an ejected CPU does not execute code after >> that point but given that the CPU state will be destroyed by >> QEMU, the missed cleanup is no great loss. >> >> 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 reviewing comments from v6: >> (1) s/CpuEject/EjectCpu/g >> (2,2a,2c) Get rid of eject-worker and related. >> (2b,2d) Use the PlatformSmmBspElection() logic to find out IsBSP. >> (3,3b) Use CPU_HOT_EJECT_DATA->QemuSelector instead of ApicIdMap to >> do the actual ejection. >> (4,5a,5b) Fix the format etc in the final unplugged log message >> () Also as discussed elsewhere document the ordering requirements for >> mCpuHotEjectData->QemuSelector[] and mCpuHotEjectData->Handler. >> () [from patch 2] Move definition of QEMU_CPUHP_STAT_EJECTED to this >> patch. >> () s/QEMU_CPUHP_STAT_EJECTED/QEMU_CPUHP_STAT_EJECT/ >> >> OvmfPkg/Include/IndustryStandard/QemuCpuHotplug.h | 1 + >> OvmfPkg/CpuHotplugSmm/CpuHotplug.c | 127 +++++++++++++++++++-- >> .../Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c | 31 +++++ >> 3 files changed, 152 insertions(+), 7 deletions(-) >> >> diff --git a/OvmfPkg/Include/IndustryStandard/QemuCpuHotplug.h b/OvmfPkg/Include/IndustryStandard/QemuCpuHotplug.h >> index 2ec7a107a64d..d0e83102c13f 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_EJECT BIT3 >> #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 851e2b28aad9..0484be8fe43c 100644 >> --- a/OvmfPkg/CpuHotplugSmm/CpuHotplug.c >> +++ b/OvmfPkg/CpuHotplugSmm/CpuHotplug.c >> @@ -18,6 +18,7 @@ >> #include // CPU_HOT_EJECT_DATA >> #include // EFI_MM_CPU_IO_PROTOCOL >> #include // EFI_SMM_CPU_SERVICE_PROTOCOL >> +#include // MSR_IA32_APIC_BASE_REGISTER >> #include // EFI_STATUS >> >> #include "ApicId.h" // APIC_ID >> @@ -191,12 +192,39 @@ RevokeNewSlot: >> } >> >> /** >> + EjectCpu needs to know the BSP at SMI exit at a point when >> + some of the EFI_SMM_CPU_SERVICE_PROTOCOL state has been torn >> + down. >> + Reuse the logic from OvmfPkg::PlatformSmmBspElection() to >> + do that. >> + >> + @param[in] ProcessorNum ProcessorNum denotes the processor handle number >> + in EFI_SMM_CPU_SERVICE_PROTOCOL. >> +**/ >> +STATIC >> +BOOLEAN >> +CheckIfBsp ( >> + IN UINTN ProcessorNum >> + ) > > (1a) Please remove the ProcessorNum parameter -- comment and parameter > list alike --; it's useless. In the parameter list, just replace the > line's contents with "VOID". Heh. Yeah, I'm not certain why I added that. > (1b) Additionally, please state: > > @retval TRUE If the CPU executing this function is the BSP. > > @retval FALSE If the CPU executing this function is an AP. > Will add. > > > >> +{ >> + MSR_IA32_APIC_BASE_REGISTER ApicBaseMsr; >> + BOOLEAN IsBsp; > > (2) "IsBsp" should line up with "ApicBaseMsr". > > >> + >> + ApicBaseMsr.Uint64 = AsmReadMsr64 (MSR_IA32_APIC_BASE); >> + IsBsp = (BOOLEAN)(ApicBaseMsr.Bits.BSP == 1); >> + return IsBsp; >> +} >> + >> +/** >> 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 the BSP, nor being ejected, nothing >> + to be done. >> If, the executing CPU is being ejected, wait in a halted loop >> until ejected. >> + If, the executing CPU is the BSP, set QEMU CPU status to eject >> + for CPUs being ejected. >> >> @param[in] ProcessorNum ProcessorNum denotes the CPU exiting SMM, >> and will be used as an index into >> @@ -211,9 +239,99 @@ EjectCpu ( >> ) >> { >> UINT64 QemuSelector; >> + BOOLEAN IsBsp; >> >> + IsBsp = CheckIfBsp (ProcessorNum); >> + >> + // >> + // mCpuHotEjectData->QemuSelectorMap[ProcessorNum] is updated >> + // on the BSP in the ongoing SMI iteration at two places: > > (3) I feel "iteration" doesn't really apply; I'd simply drop that word. > "ongoing SMI" seems sufficient (or maybe "ongoing SMI handling"). Yeah that reads better. > > >> + // >> + // - UnplugCpus() where the BSP determines if a CPU is under ejection >> + // or not. As the comment where mCpuHotEjectData->Handler is set-up >> + // describes any such updates are guaranteed to be ordered-before the >> + // dereference below. >> + // >> + // - EjectCpu() on the BSP updates QemuSelectorMap[ProcessorNum] for >> + // CPUs after they have been hot-ejected. >> + // >> + // The CPU under ejection: might be executing anywhere between the >> + // "AllCpusInSync" exit loop in SmiRendezvous() to about to >> + // dereference QemuSelectorMap[ProcessorNum]. >> + // Given that the BSP ensures that this store only happens after the >> + // CPU has been ejected, this CPU would never see the after value. >> + // (Note that any CPU that is already executing the CpuSleep() loop >> + // below never raced any updates and always saw the before value.) >> + // >> + // CPUs not-under ejection: never see any changes so they are fine. >> + // >> + // Lastly, note that we are also guaranteed that any dereferencing >> + // CPU only sees the before or after value and not an intermediate >> + // value. This is because QemuSelectorMap[ProcessorNum] is aligned at >> + // a natural boundary. >> + // > > (4) Well, about the last paragraph -- when I saw that you used UINT64 in > QemuSelectorMap, to allow for a sentinel value, I immediately thought of > IA32. This module is built for IA32 too, and there I'm not so sure about > the atomicity of UINT64 accesses. > > I didn't raise it at that point because I wasn't sure if we were > actually going to rely on the atomicity. And, I don't think we are. > Again, let me quote Paolo's example from : > > thread 1 thread 2 > -------------------------------- ------------------------ > a.x = 1; > smp_wmb(); > WRITE_ONCE(message, &a); datum = READ_ONCE(message); > smp_rmb(); > if (datum != NULL) > printk("%x\n", datum->x); > > The access to object "x" is not restricted in any particular way. In > thread#1, we have an smp_wmb() which enforces both a compiler barrier > and a store-release fence, and then we have an atomic access to > "message". But the "a.x" assignment need not be atomic. I concur. I think I conflated atomic access to "message" with also needing atomic access for the variables we need to ensure transitivity on. As you say below, this means that a bunch of this logic (and comments) can be simplified. > In our case, "a.x = 1" maps to (a) populating "QemuSelectorMap", and (b) > setting up the "Handler" function pointer, in UnplugCpus(). The > smp_wmb() is the "ReleaseMemoryFence" in UnplugCpus(). The WRITE_ONCE is > the final "AllCpusInSync = FALSE" assignment in BSPHandler() > [UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c]. > > Regarding thread#2, the READ_ONCE is matched by the "AllCpusInSync" > loop. smp_rmb() is the "AcquireMemoryFence" I called for, under PATCH v8 > 07/10, "OvmfPkg/SmmCpuFeaturesLib: call CPU hot-eject handler". And the > "datum" access happens (a) in SmmCpuFeaturesRendezvousExit(), where we > fetch/call Handler, and (b) here, where we read/modify "QemuSelectorMap". > > So this seems to imply we can: > > - drop the natural-alignment padding of "QemuSelectorMap", from PATCH v8 > 06/10, "OvmfPkg/SmmCpuFeaturesLib: init CPU ejection state", > > - drop the last paragraph of the comment above. Will do. > > >> QemuSelector = mCpuHotEjectData->QemuSelectorMap[ProcessorNum]; >> - if (QemuSelector == CPU_EJECT_QEMU_SELECTOR_INVALID) { >> + if (QemuSelector == CPU_EJECT_QEMU_SELECTOR_INVALID && !IsBsp) { >> + return; >> + } >> + >> + if (IsBsp) { > > (5) Can you reorder the high-level flow here? Such as: > > if (CheckIfBsp ()) { > // > // send the eject requests to QEMU here > // > return; > } > > // > // Reached only on APs: > // > QemuSelector = mCpuHotEjectData->QemuSelectorMap[ProcessorNum]; > if (QemuSelector == CPU_EJECT_QEMU_SELECTOR_INVALID) { > return; > } > > // > // AP being hot-ejected; pen it here. > // Yeah, this is clearer. > > >> + UINT32 Idx; >> + >> + for (Idx = 0; Idx < mCpuHotEjectData->ArrayLength; Idx++) { >> + UINT64 QemuSelector; >> + >> + QemuSelector = mCpuHotEjectData->QemuSelectorMap[Idx]; >> + >> + if (QemuSelector != CPU_EJECT_QEMU_SELECTOR_INVALID) { >> + // >> + // This to-be-ejected-CPU has already received the BSP's SMI exit >> + // signal and, will execute SmmCpuFeaturesRendezvousExit() >> + // followed by this callback or is already waiting in the >> + // CpuSleep() loop below. >> + // >> + // Tell QEMU to context-switch it out. >> + // >> + QemuCpuhpWriteCpuSelector (mMmCpuIo, (UINT32) QemuSelector); >> + QemuCpuhpWriteCpuStatus (mMmCpuIo, QEMU_CPUHP_STAT_EJECT); >> + >> + // >> + // We need a compiler barrier here to ensure that the compiler >> + // does not reorder the CpuStatus and QemuSelectorMap[Idx] stores. >> + // >> + // A store fence is not strictly necessary on x86 which has >> + // TSO; however, both of these stores are in different address spaces >> + // so also add a Store Fence here. >> + // >> + MemoryFence (); > > (6) I wonder if this compiler barrier + comment block are helpful. > Paraphrasing your (ex-)colleague Liran, if MMIO and IO Port accessors > didn't contain built-in fences, all hell would break lose. We're using > EFI_MM_CPU_IO_PROTOCOL for IO Port accesses. I think we should be safe > ordering-wise, even without an explicit compiler barrier here. > > To me personally, this particular fence only muddies the picture -- > where we already have an acquire memory fence and a store memory fence > to couple with each other. > > I'd recommend removing this. (If you disagree, I'm willing to listen to > arguments, of course!) You are right that we don't need a memory fence here -- given that there is an implicit fence due to the MMIO. As for the compiler fence, I'm just now re-looking at handlers in EFI_MM_CPU_IO_PROTOCOL and they do seem to include a compiler barrier. So I agree with you that we have all the fences that we need. However, I do think it's a good idea to document both of these here. > >> + >> + // >> + // Clear the eject status for this CPU Idx to ensure that an invalid >> + // SMI later does not end up trying to eject it or a newly >> + // hotplugged CPU Idx does not go into the dead loop. >> + // >> + mCpuHotEjectData->QemuSelectorMap[Idx] = >> + CPU_EJECT_QEMU_SELECTOR_INVALID; >> + >> + DEBUG ((DEBUG_INFO, "%a: Unplugged ProcessorNum %u, " >> + "QemuSelector 0x%Lx\n", __FUNCTION__, Idx, QemuSelector)); > > (7) Please log QemuSelector with "%Lu". Ack. > > >> + } >> + } >> + >> + // >> + // We are done until the next hot-unplug; clear the handler. >> + // >> + // By virtue of the MemoryFence() in the ejection loop above, the >> + // following store is ordered-after all the ejections are done. >> + // (We know that there is at least one CPU hot-eject handler if this >> + // handler was installed.) >> + // >> + // As described in OvmfPkg::SmmCpuFeaturesRendezvousExit() this >> + // means that the only CPUs which might dereference >> + // mCpuHotEjectData->Handler are not under ejection, so we can >> + // safely reset. >> + // >> + mCpuHotEjectData->Handler = NULL; >> return; >> } >> >> @@ -496,11 +614,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); >> diff --git a/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c b/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c >> index 99988285b6a2..ddfef05ee6cf 100644 >> --- a/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c >> +++ b/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c >> @@ -472,6 +472,37 @@ SmmCpuFeaturesRendezvousExit ( >> // (PcdCpuMaxLogicalProcessorNumber > 1), and hot-eject is needed >> // in this SMI exit (otherwise mCpuHotEjectData->Handler is not armed.) >> // >> + // mCpuHotEjectData itself is stable once setup so it can be >> + // dereferenced without needing any synchronization, >> + // but, mCpuHotEjectData->Handler is updated on the BSP in the >> + // ongoing SMI iteration at two places: >> + // >> + // - UnplugCpus() where the BSP determines if a CPU is under ejection >> + // or not. As the comment where mCpuHotEjectData->Handler is set-up >> + // describes any such updates are guaranteed to be ordered-before the >> + // dereference below. >> + // >> + // - EjectCpu() (which is called via the Handler below), on the BSP >> + // updates mCpuHotEjectData->Handler once it is done with all ejections. >> + // >> + // The CPU under ejection: might be executing anywhere between the >> + // "AllCpusInSync" exit loop in SmiRendezvous() to about to >> + // dereference the Handler field. >> + // Given that the BSP ensures that this store only happens after all >> + // CPUs under ejection have been ejected, this CPU would never see >> + // the after value. >> + // (Note that any CPU that is already executing the CpuSleep() loop >> + // below never raced any updates and always saw the before value.) >> + // >> + // CPUs not-under ejection: might see either value of the Handler >> + // which is fine, because the Handler is a NOP for CPUs not-under >> + // ejection. >> + // >> + // Lastly, note that we are also guaranteed that any dereferencing >> + // CPU only sees the before or after value and not an intermediate >> + // value. This is because mCpuHotEjectData->Handler is aligned at a >> + // natural boundary. >> + // >> >> if (mCpuHotEjectData != NULL) { >> CPU_HOT_EJECT_HANDLER Handler; >> > > (8) I can't really put my finger on it, I just feel that repeating > (open-coding) this wall of text here is not really productive. Part of the reason I wanted to document this here was to get your opinion on it and figure out how much of it is useful and how much might be overkill. > > Do you think that, after you add the "acquire memory fence" comment in > patch #7, we could avoid most of the text here? I think we should only > point out (in patch #7) the "release fence" that the logic here pairs with.> > If you really want to present it all from both perspectives, I guess I'm > OK with that, but then I believe we should drop the last paragraph at > least (see point (4)). Rereading it after a gap of a few days and given that most of this is just a repeat, I'm also tending towards overkill. I think a comment talking about acquire/release pairing is useful. Rest of it can probably be met with just a pointer towards the comment in EjectCpus(). Does that make sense? Thanks Ankur > > Thanks > Laszlo >