From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from userp2130.oracle.com (userp2130.oracle.com [156.151.31.86]) by mx.groups.io with SMTP id smtpd.web08.17514.1611688542669049947 for ; Tue, 26 Jan 2021 11:15:44 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@oracle.com header.s=corp-2020-01-29 header.b=xr2EBwk8; spf=pass (domain: oracle.com, ip: 156.151.31.86, mailfrom: ankur.a.arora@oracle.com) Received: from pps.filterd (userp2130.oracle.com [127.0.0.1]) by userp2130.oracle.com (8.16.0.42/8.16.0.42) with SMTP id 10QJ3oSf151135; Tue, 26 Jan 2021 19:15:38 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=37MYQMRkYaXyGAvlt/CBBEte5shoNy4AHi0fbdN0WMk=; b=xr2EBwk8MOR4lbCCiPgDqfo/ZFwQeUrftXkEteTonGvh2w6tLZ+lJnJIr8KTGUUFgIZG 5hl9qye/4OFj3WYQAPCxc96pvXFZ4NfZRP5cYzm+ymVQ4BtAH5H32otJgtj/lRNERYJ7 gf5SgsaJLtItksugRxsx0JLI7YJ6cbtQvVid7BVJUWt3/dOa2Okll8GC8QPN4ccQfcdV p2PEFJcsSlKFeJWq5SN4Q4EoNKDaVU89ZfVgAdlRF0zlnQf8g8fyEc9Hwyg9jjeA/NlX Fc5Jkp3P14MDrOVdN+FkjQePLaE3cDjXGSofPZ/cNbb1StoRhFeuIV44MfNnIGuxLPBj TA== Received: from aserp3020.oracle.com (aserp3020.oracle.com [141.146.126.70]) by userp2130.oracle.com with ESMTP id 368b7qunxg-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Tue, 26 Jan 2021 19:15:38 +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 10QJ0fGc007114; Tue, 26 Jan 2021 19:15:37 GMT Received: from nam04-co1-obe.outbound.protection.outlook.com (mail-co1nam04lp2059.outbound.protection.outlook.com [104.47.45.59]) by aserp3020.oracle.com with ESMTP id 368wpyanak-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Tue, 26 Jan 2021 19:15:37 +0000 ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=I3nSTXPCj2zRRmEHKYb2GYKnRMjxsjUIHZ0ZoBI8ZnB8z0RbcbZ96zuaeBVIjoRKJ8ctGWrj/UX3iRd1wuUd2WJVaIdLymetyynpNnFJe7iDmWehGW1H4fZWaeCtmlEjNOAlNLpVC+qwvkEin4TWQ5Zmfvs8CwlpvcEV/pok1pKCLhB/TfjUmJabpozy+hb/AloDRUd5GKxOj4c2+KYMQQWU6GWlkpLCWa0INYVan5tJbFSK+ExW9ia60zYPgi310ibLZcaoO9jDyBhSENJ+YoCWXPBnZs0vs/gH0Z5P88IrLxtxBzhvfgO3gjjCn1namIWvxoRl2XMpvufcPOtt8Q== 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=37MYQMRkYaXyGAvlt/CBBEte5shoNy4AHi0fbdN0WMk=; b=QkhkaUXZHVT25eSmFG644joYwGPaGxK/bGp9G5QgJLYe4C6MGdpZkc0XpH0fa/8c5YJOGbvw24gDkex56Zr44VoLFagukJUcXwvLxpeFiZdMEbDt34fElqSRqlfZkzjtpYnbYcJyiFNOJmKkjxxkvvN6DxKhKU2xdWEo55tHr4z9d5LjolXAAqQdXeQGlBo4NKgvuUSN9YxPRKD/gXeajaBrJ5CRoH9IRwI4+oia4NNdJVqVs2TqYyXEKyyNb4ubrlZ2sFysF1Ed/Yc2dn5ksZPt6R6AehD+8ZXdJE8tPTzUrAfF4kBWxkwmkE+uIeg4b0uegdy0AfZFE/Qi6KHLcQ== 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=37MYQMRkYaXyGAvlt/CBBEte5shoNy4AHi0fbdN0WMk=; b=lEBIV2oHLCXou79QBSB1wSfC4HnaJAxPynVuOUiheQEi+IWkfzs85a4CZoFEJkeuHrf71RAqfZCmFi77WnzvocTqYBypNPOF94/qE9YqbgiOb4DolnFroSQ4Jjou3PXgT1tnFMzlPQO3tBSHUERcvMAsQEsvQ6ZgYaFL4umH/CI= Received: from CY4PR10MB1718.namprd10.prod.outlook.com (2603:10b6:910:9::17) by CY4PR10MB1351.namprd10.prod.outlook.com (2603:10b6:903:2b::20) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.3784.11; Tue, 26 Jan 2021 19:15:35 +0000 Received: from CY4PR10MB1718.namprd10.prod.outlook.com ([fe80::39dc:6ba7:9397:cb6]) by CY4PR10MB1718.namprd10.prod.outlook.com ([fe80::39dc:6ba7:9397:cb6%6]) with mapi id 15.20.3784.019; Tue, 26 Jan 2021 19:15:35 +0000 Subject: Re: [PATCH v5 1/9] OvmfPkg/CpuHotplugSmm: refactor hotplug logic To: Laszlo Ersek , devel@edk2.groups.io Cc: imammedo@redhat.com, boris.ostrovsky@oracle.com, Jordan Justen , Ard Biesheuvel , Aaron Young References: <20210126064440.299596-1-ankur.a.arora@oracle.com> <20210126064440.299596-2-ankur.a.arora@oracle.com> <3f3fd01d-78a9-315c-85e3-b5788b8d6489@redhat.com> From: "Ankur Arora" Message-ID: <4a1e8dd7-51e4-165c-64c1-d7d6fea6eafc@oracle.com> Date: Tue, 26 Jan 2021 11:15:31 -0800 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.6.0 In-Reply-To: <3f3fd01d-78a9-315c-85e3-b5788b8d6489@redhat.com> X-Originating-IP: [70.36.60.91] X-ClientProxiedBy: CO2PR05CA0101.namprd05.prod.outlook.com (2603:10b6:104:1::27) To CY4PR10MB1718.namprd10.prod.outlook.com (2603:10b6:910:9::17) MIME-Version: 1.0 X-MS-Exchange-MessageSentRepresentingType: 1 Received: from [192.168.0.108] (70.36.60.91) by CO2PR05CA0101.namprd05.prod.outlook.com (2603:10b6:104:1::27) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.3805.5 via Frontend Transport; Tue, 26 Jan 2021 19:15:34 +0000 X-MS-PublicTrafficType: Email X-MS-Office365-Filtering-Correlation-Id: 046c3e40-0a53-4efd-df60-08d8c22ec1ce X-MS-TrafficTypeDiagnostic: CY4PR10MB1351: 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: DeMODSYRfR4y7nzXSzALSRwRK8HfVEoT6pXz/JBoq5EyxKppBNCGoD0EwruLatLZBwrGWbwN3PT/5liDGAN+UoG3ZMHAIMA0K/0tHg868t6XSN7av049Ka2HaaxJBu1rcVIzqyJxSvYQpgLaGApEYL/3lkZ4yZ7ZE++Y+pzIUC+x5jjbB/q8a0gL8C8YaC4BeQF0oQcDg2rqdMSgzdWpFz2LjChNvb68JHT2CEo210zh2bQzvonkhYecVY2kwIdAnFgAuArb4PiYpoVWlTig2RnQb61MolszFS3yU7qm+vNS1uvI07mXgHhbV6Mz+LvopNlaP8wqZo30dnUaKqepMR4UIRFaB+gH9Rm5cXGtLQObBiJsoQoiOgXCslj82LlKVl03M70+OiP5Lv+bbffTmRgYcVs3iK5n72i/9pd4S+Q0o3RqaOsO7tJfqy9OMJhSJM/cSScy8Znf3D9CC5A471uKoU8OJOJW92mWXTAFcoNo4yEodOdWZ4LMwvTO0k3OhUIFc9eJUj+KB1nUvDO/lc5zLFNmWrk+humEUPTNIsVJh03DLdYNSJAbKw4AKbPE5wBFpEBxv9VUq4Ks7+LmdQ3DRfyBO07UmOQzsiTgfs27XaHfhUhFQKc8k5Ld4xnX4UnxrRQgAOKAHtNOjawPgOceiksjNreIpNTHBfdBk4NLbt99f/HSxcy+e8V+7wkD X-Forefront-Antispam-Report: CIP:255.255.255.255;CTRY:;LANG:en;SCL:1;SRV:;IPV:NLI;SFV:NSPM;H:CY4PR10MB1718.namprd10.prod.outlook.com;PTR:;CAT:NONE;SFS:(366004)(39860400002)(396003)(136003)(376002)(346002)(966005)(53546011)(31696002)(8936002)(86362001)(478600001)(6666004)(26005)(30864003)(16576012)(36756003)(8676002)(956004)(54906003)(6486002)(2906002)(4326008)(31686004)(316002)(2616005)(83380400001)(16526019)(66556008)(66476007)(66946007)(186003)(107886003)(5660300002)(43740500002)(45980500001);DIR:OUT;SFP:1101; X-MS-Exchange-AntiSpam-MessageData: =?utf-8?B?U2JLNUhXNjhyQWVMc0VGNlR1ZzNlZS9OZEJBNFFEdWFldklLVnIyTnhOVzJX?= =?utf-8?B?VlIxOUwxYXlVcnRpbVFEQzlNSUtLNmZYa1FCWXNuNXBSOUNOakxkN2tLaWtx?= =?utf-8?B?YndRT1VvNkJ3K2x2RWhyMjBzNUl4NkFvQ0hFOU4rQ3B6MEhSMEp5akF0MWQy?= =?utf-8?B?QkJHV01JRnkvQzBTUXhFYmZzb1EvOU44M3RnT3VOQ3V5bERNNHlIdEZNS3Vm?= =?utf-8?B?RUc5QngyZ01RNnBPUTk5K0ZXZWFkOFNFQWlveGlRcTIyd1RaaDZISzZCNTRx?= =?utf-8?B?aitJeFJsWEI4U0JQaE1lWit0Rk8xdlNiNko1ZXlzZFVCTTZzZTVzNW9pRmgz?= =?utf-8?B?WUd4U3N5L3NKT3dEVWNob0JDby9weFR1OExubm85bnBjNW0wVjViTURUb1FP?= =?utf-8?B?STFZNllJQjVrNHdNR2k2UEIySDlSYmRtU3FxdVp5eDhKRmlMYzBldlhTN0ZI?= =?utf-8?B?TXJhQ0diTkR1bzNDS0tINU01U1J5ejNQRExUQlMybXBybGxJb2FBMy94NDA5?= =?utf-8?B?dlJiMDJMUHkzTkZIWHNtQzJNcUJqdkNHWG0vbzNaSXFuQ3R5Z3RLL3Q1TkE5?= =?utf-8?B?YmRHRXVVOGV6ZGtZYmZsb3lJc2E1b2NoemYrdDBncFlDYTQraTRlOVA3eXpN?= =?utf-8?B?NThYNHloS0ZzWGNWOW1VWmE4MVVXVXJxUFlxZHo4VVJwZ0QxSXAvVWhZcEZN?= =?utf-8?B?M1I4dWswa2I0RXpBcnV0VWkvdkFubVFBU21JY1ZxTzc2QTJFUzV4ZmNXOVJJ?= =?utf-8?B?NHJJOE9VbExiK0NlVGVJeTJNc3hLWTRqQUZLSnNPeENKMTBaUWM5M0NwVEdE?= =?utf-8?B?elBpV1RVMis3cFI4Vkg0cVFlbjR2b043LzVYbDdVUWxEbDhPcEtHZGp1UzBs?= =?utf-8?B?YVNRZlM2bnVic20rRHlKUFR2Q3crRjZ0MHB5UEpWaUpjaUt4bks3SHBCbFZX?= =?utf-8?B?RkFQRzBhdVlKbStTNUkvSmVpZkNFaGttOXA3MDZDVGo0QU5meXlqOEdVc3Ni?= =?utf-8?B?bmVJckFKYzdsQlpoTXRaMi84UmNiNDEzZXZPanBwb2xQMFdvY2V6MVNvRVlU?= =?utf-8?B?M1pXT2ZuYVFoVmVLY05BRWFkWEpwKzhQUEo3c1RLVFovRHNOSzVHOGVhd3FI?= =?utf-8?B?ekZrNnJIdm1remx3M3dFdzdXUDlLSnhwWStPZEFwc3dSaW9wN2dQeHBYalo2?= =?utf-8?B?Q2QwbjRzRzF2c2hVOUF6bW4xMGdPUTI4bUxjbTJlRHd0OEVVNkgycFM0dVlP?= =?utf-8?B?YTJUR3hPZDJiNTBtSXJPMTllNHBZbU44a1ozUGprOGgrVll3M0VzMnB0d3hG?= =?utf-8?B?VDQvdDVNem05OUtXNllhdEJ5enRWZnpkMmNpeXZQcUpVcG4vNjZWT1BmV1JB?= =?utf-8?B?U3VYZWx0a1UzNDYyaEZJOXhScmx1U1ZnRkM2WHp1N1hnT3NuN2pKTlRhUlZa?= =?utf-8?Q?1Y4J6hZM?= X-OriginatorOrg: oracle.com X-MS-Exchange-CrossTenant-Network-Message-Id: 046c3e40-0a53-4efd-df60-08d8c22ec1ce X-MS-Exchange-CrossTenant-AuthSource: CY4PR10MB1718.namprd10.prod.outlook.com X-MS-Exchange-CrossTenant-AuthAs: Internal X-MS-Exchange-CrossTenant-OriginalArrivalTime: 26 Jan 2021 19:15:35.0658 (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: XU4h2iZ7MTpD6IwwMRRRzypDwakDp1gVM37WXG1fEu6kLJ80/CCUe76neRecQzVh8lKEKy1Gj+rti+worgVxAupcm1asPMI5g/yBQdPw/g0= X-MS-Exchange-Transport-CrossTenantHeadersStamped: CY4PR10MB1351 X-Proofpoint-Virus-Version: vendor=nai engine=6200 definitions=9876 signatures=668683 X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 mlxlogscore=999 mlxscore=0 spamscore=0 adultscore=0 bulkscore=0 phishscore=0 suspectscore=0 malwarescore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2009150000 definitions=main-2101260098 X-Proofpoint-Virus-Version: vendor=nai engine=6200 definitions=9876 signatures=668683 X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 suspectscore=0 spamscore=0 phishscore=0 adultscore=0 impostorscore=0 malwarescore=0 lowpriorityscore=0 bulkscore=0 priorityscore=1501 mlxscore=0 clxscore=1015 mlxlogscore=999 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2009150000 definitions=main-2101260098 Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit On 2021-01-26 11:01 a.m., Laszlo Ersek wrote: > On 01/26/21 07:44, Ankur Arora wrote: >> Refactor CpuHotplugMmi() to pull out the CPU hotplug logic into >> PlugCpus(). This is in preparation for supporting CPU hot-unplug. >> >> 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 | 208 ++++++++++++++++++++++--------------- >> 1 file changed, 123 insertions(+), 85 deletions(-) >> >> diff --git a/OvmfPkg/CpuHotplugSmm/CpuHotplug.c b/OvmfPkg/CpuHotplugSmm/CpuHotplug.c >> index cfe698ed2b5e..a5052a501e5a 100644 >> --- a/OvmfPkg/CpuHotplugSmm/CpuHotplug.c >> +++ b/OvmfPkg/CpuHotplugSmm/CpuHotplug.c >> @@ -62,6 +62,124 @@ STATIC UINT32 mPostSmmPenAddress; >> // >> STATIC EFI_HANDLE mDispatchHandle; >> >> +/** >> + CPU Hotplug handler function. >> + >> + @param[in] PluggedApicIds List of APIC IDs to be plugged. >> + >> + @param[in] PluggedCount Count of APIC IDs to be plugged. > > (1) These comments are not optimal. > > The variable names say "Plugged", meaning that the CPUs have already > been plugged, from the QEMU perspective. The purpose of this function is > to go over those CPUs whose APIC IDs have been collected with events > pending, relocate the SMBASE on each, and then expose each such CPU to > EFI_SMM_CPU_SERVICE_PROTOCOL. For a given CPU, I think the comment on > the EFI_SMM_ADD_PROCESSOR prototype > [UefiCpuPkg/Include/Protocol/SmmCpuService.h] best expresses the goal: > "Notify that a new processor has been added to the system ... The SMM > CPU driver should add the processor to the SMM CPU list". > > See also the comment on QemuCpuhpCollectApicIds(): > > """ > Collect the APIC IDs of > - the CPUs that have been hot-plugged, > - the CPUs that are about to be hot-unplugged. > """ > > This closely reflects which agent (firmware vs. QEMU) is driving each > particular operation / direction. > > (1a) So please replace the leading comment with: > > Process those CPUs that have been hot-added, according to > QemuCpuhpCollectApicIds(). > > For each such CPU, relocate the SMBASE, and report the CPU to PiSmmCpuDxeSmm > via EFI_SMM_CPU_SERVICE_PROTOCOL. If a supposedly hot-added CPU is already > known, skip it silently. > > (1b) Similarly, in the parameter comments, "to be plugged" is wrong. I > suggest copying the parameter descriptions from > QemuCpuhpCollectApicIds(): > > @param[out] PluggedApicIds The APIC IDs of the CPUs that have been > hot-plugged. > > @param[out] PluggedCount The number of filled-in APIC IDs in > PluggedApicIds. > > >> + >> + @retval EFI_SUCCESS Some of the requested APIC IDs were hot-plugged. > > (2) This is inexact; on successful return, each one of the collected > CPUs has been relocated and exposed to the SMM CPU driver. (Either in > this particular invocation, or in an earlier invocation, but on success, > there is no entry in PluggedApicIds that is *not known* to the SMM CPU > driver, or whose SMBASE is not relocated.) > > >> + >> + @retval EFI_INTERRUPT_PENDING Fatal error while hot-plugging. > > (3) This error code is very uncommon, and it is mostly/only required > from the function -- CpuHotplugMmi() -- that is registered with > gMmst->MmiHandlerRegister(). The meaning of the error code is, "The MMI > source could not be quiesced", which is a situation that can be > identified at the level of CpuHotplugMmi(), but not at the level of > PlugCpus(). > > Please list the following return values instead: > > @retval EFI_OUT_OF_RESOURCES Out of APIC ID space in "mCpuHotPlugData". > > @return Error codes propagated from SmbaseRelocate() > and mMmCpuService->AddProcessor(). > > (General remark, in addition: please note the difference between > "@retval" and "@return". The latter does not name a particular value; > the set of values is described in natural language instead.) > > >> + >> +**/ >> +STATIC >> +EFI_STATUS >> +EFIAPI > > (4) There is no need to make this function EFIAPI. > > >> +PlugCpus( > > (5) Space character missing between function name and opening > parenthesis. > > Please check every function prototype and function call for this -- one > space char before the opening paren is required, except in the > definition of function-like macros (where the language standard requires > the "(" to be directly adjacent). > > > (6) According to the discussion above, this function should be called > ProcessHotAddedCpus(). > > ... The best hint for this function name is actually the comment that > sits atop the stretch of code you are extracting, namely: > > // > // Process hot-added CPUs. > // > > >> + IN APIC_ID *PluggedApicIds, >> + IN UINT32 PluggedCount >> + ) >> +{ >> + EFI_STATUS Status; >> + UINT32 PluggedIdx; >> + UINT32 NewSlot; >> + >> + // >> + // Process hot-added CPUs. > > (7) This short introductory comment is no longer needed, as it should be > promoted to the name of the function. > > >> + // >> + // The Post-SMM Pen need not be reinstalled multiple times within a single >> + // root MMI handling. Even reinstalling once per root MMI is only prudence; >> + // in theory installing the pen in the driver's entry point function should >> + // suffice. >> + // >> + SmbaseReinstallPostSmmPen (mPostSmmPenAddress); >> + >> + PluggedIdx = 0; >> + NewSlot = 0; >> + while (PluggedIdx < PluggedCount) { >> + APIC_ID NewApicId; >> + UINT32 CheckSlot; >> + UINTN NewProcessorNumberByProtocol; >> + >> + NewApicId = PluggedApicIds[PluggedIdx]; >> + >> + // >> + // Check if the supposedly hot-added CPU is already known to us. >> + // >> + for (CheckSlot = 0; >> + CheckSlot < mCpuHotPlugData->ArrayLength; >> + CheckSlot++) { >> + if (mCpuHotPlugData->ApicId[CheckSlot] == NewApicId) { >> + break; >> + } >> + } >> + if (CheckSlot < mCpuHotPlugData->ArrayLength) { >> + DEBUG ((DEBUG_VERBOSE, "%a: APIC ID " FMT_APIC_ID " was hot-plugged " >> + "before; ignoring it\n", __FUNCTION__, NewApicId)); >> + PluggedIdx++; >> + continue; >> + } >> + >> + // >> + // Find the first empty slot in CPU_HOT_PLUG_DATA. >> + // >> + while (NewSlot < mCpuHotPlugData->ArrayLength && >> + mCpuHotPlugData->ApicId[NewSlot] != MAX_UINT64) { >> + NewSlot++; >> + } >> + if (NewSlot == mCpuHotPlugData->ArrayLength) { >> + DEBUG ((DEBUG_ERROR, "%a: no room for APIC ID " FMT_APIC_ID "\n", >> + __FUNCTION__, NewApicId)); >> + goto Fatal; > > (8) Please replace the "goto" with "return EFI_OUT_OF_RESOURCES". > > >> + } >> + >> + // >> + // Store the APIC ID of the new processor to the slot. >> + // >> + mCpuHotPlugData->ApicId[NewSlot] = NewApicId; >> + >> + // >> + // Relocate the SMBASE of the new CPU. >> + // >> + Status = SmbaseRelocate (NewApicId, mCpuHotPlugData->SmBase[NewSlot], >> + mPostSmmPenAddress); >> + if (EFI_ERROR (Status)) { >> + goto RevokeNewSlot; >> + } >> + >> + // >> + // Add the new CPU with EFI_SMM_CPU_SERVICE_PROTOCOL. >> + // >> + Status = mMmCpuService->AddProcessor (mMmCpuService, NewApicId, >> + &NewProcessorNumberByProtocol); >> + if (EFI_ERROR (Status)) { >> + DEBUG ((DEBUG_ERROR, "%a: AddProcessor(" FMT_APIC_ID "): %r\n", >> + __FUNCTION__, NewApicId, Status)); >> + goto RevokeNewSlot; >> + } >> + >> + DEBUG ((DEBUG_INFO, "%a: hot-added APIC ID " FMT_APIC_ID ", SMBASE 0x%Lx, " >> + "EFI_SMM_CPU_SERVICE_PROTOCOL assigned number %Lu\n", __FUNCTION__, >> + NewApicId, (UINT64)mCpuHotPlugData->SmBase[NewSlot], >> + (UINT64)NewProcessorNumberByProtocol)); >> + >> + NewSlot++; >> + PluggedIdx++; >> + } >> + >> + // >> + // We've handled this hotplug. >> + // > > (9) I suggest: "We've processed this batch of hot-added CPUs.". > > >> + return EFI_SUCCESS; >> + >> +RevokeNewSlot: >> + mCpuHotPlugData->ApicId[NewSlot] = MAX_UINT64; >> + >> +Fatal: > > (10) Please drop this label. > > >> + return EFI_INTERRUPT_PENDING; > > (11) This should be "return Status". > > >> +} >> >> /** >> CPU Hotplug MMI handler function. >> @@ -122,8 +240,6 @@ CpuHotplugMmi ( >> UINT8 ApmControl; >> UINT32 PluggedCount; >> UINT32 ToUnplugCount; >> - UINT32 PluggedIdx; >> - UINT32 NewSlot; >> >> // >> // Assert that we are entering this function due to our root MMI handler >> @@ -179,87 +295,12 @@ CpuHotplugMmi ( >> goto Fatal; >> } >> >> - // >> - // Process hot-added CPUs. >> - // >> - // The Post-SMM Pen need not be reinstalled multiple times within a single >> - // root MMI handling. Even reinstalling once per root MMI is only prudence; >> - // in theory installing the pen in the driver's entry point function should >> - // suffice. >> - // >> - SmbaseReinstallPostSmmPen (mPostSmmPenAddress); >> + if (PluggedCount > 0) { >> + Status = PlugCpus(mPluggedApicIds, PluggedCount); > > (12) Space missing before "(". > > >> + } >> >> - PluggedIdx = 0; >> - NewSlot = 0; >> - while (PluggedIdx < PluggedCount) { >> - APIC_ID NewApicId; >> - UINT32 CheckSlot; >> - UINTN NewProcessorNumberByProtocol; >> - >> - NewApicId = mPluggedApicIds[PluggedIdx]; >> - >> - // >> - // Check if the supposedly hot-added CPU is already known to us. >> - // >> - for (CheckSlot = 0; >> - CheckSlot < mCpuHotPlugData->ArrayLength; >> - CheckSlot++) { >> - if (mCpuHotPlugData->ApicId[CheckSlot] == NewApicId) { >> - break; >> - } >> - } >> - if (CheckSlot < mCpuHotPlugData->ArrayLength) { >> - DEBUG ((DEBUG_VERBOSE, "%a: APIC ID " FMT_APIC_ID " was hot-plugged " >> - "before; ignoring it\n", __FUNCTION__, NewApicId)); >> - PluggedIdx++; >> - continue; >> - } >> - >> - // >> - // Find the first empty slot in CPU_HOT_PLUG_DATA. >> - // >> - while (NewSlot < mCpuHotPlugData->ArrayLength && >> - mCpuHotPlugData->ApicId[NewSlot] != MAX_UINT64) { >> - NewSlot++; >> - } >> - if (NewSlot == mCpuHotPlugData->ArrayLength) { >> - DEBUG ((DEBUG_ERROR, "%a: no room for APIC ID " FMT_APIC_ID "\n", >> - __FUNCTION__, NewApicId)); >> - goto Fatal; >> - } >> - >> - // >> - // Store the APIC ID of the new processor to the slot. >> - // >> - mCpuHotPlugData->ApicId[NewSlot] = NewApicId; >> - >> - // >> - // Relocate the SMBASE of the new CPU. >> - // >> - Status = SmbaseRelocate (NewApicId, mCpuHotPlugData->SmBase[NewSlot], >> - mPostSmmPenAddress); >> - if (EFI_ERROR (Status)) { >> - goto RevokeNewSlot; >> - } >> - >> - // >> - // Add the new CPU with EFI_SMM_CPU_SERVICE_PROTOCOL. >> - // >> - Status = mMmCpuService->AddProcessor (mMmCpuService, NewApicId, >> - &NewProcessorNumberByProtocol); >> - if (EFI_ERROR (Status)) { >> - DEBUG ((DEBUG_ERROR, "%a: AddProcessor(" FMT_APIC_ID "): %r\n", >> - __FUNCTION__, NewApicId, Status)); >> - goto RevokeNewSlot; >> - } >> - >> - DEBUG ((DEBUG_INFO, "%a: hot-added APIC ID " FMT_APIC_ID ", SMBASE 0x%Lx, " >> - "EFI_SMM_CPU_SERVICE_PROTOCOL assigned number %Lu\n", __FUNCTION__, >> - NewApicId, (UINT64)mCpuHotPlugData->SmBase[NewSlot], >> - (UINT64)NewProcessorNumberByProtocol)); >> - >> - NewSlot++; >> - PluggedIdx++; >> + if (EFI_ERROR(Status)) { >> + goto Fatal; >> } > > (13) Without having seen the rest of the patches, I think this error > check should be nested under the same (PluggedCount > 0) condition; in > other words, I think it only makes sense to check Status after we > actually call ProcessHotAddedCpus(). > > >> >> // >> @@ -267,9 +308,6 @@ CpuHotplugMmi ( >> // >> return EFI_SUCCESS; >> >> -RevokeNewSlot: >> - mCpuHotPlugData->ApicId[NewSlot] = MAX_UINT64; >> - >> Fatal: >> ASSERT (FALSE); >> CpuDeadLoop (); >> > > I'll continue the review later this week. Acking the comments above. Meanwhile let me reprocess the series in light of the comments above. Ankur > > Thanks > Laszlo >