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.web12.5327.1612329645449762892 for ; Tue, 02 Feb 2021 21:20:45 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@oracle.com header.s=corp-2020-01-29 header.b=iRrc3Yne; 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 1135JdjA054616; Wed, 3 Feb 2021 05:20:44 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=KKwBUeDCN/J6XYXNeePLxv7SID499Dlftp3I2MTou5Y=; b=iRrc3YneRw5J/pVqTx9w/nOFPSp1gVb+HpAFZ8mWMYKyzMuKcBjCHgApoIsoJmV9cLsg VUxq0ozXczUjtKJj3y8xpFQOOld/kzlWkZXMlVUcSILoYkeGZn/iMO3h+Bda5V5rEHyH 3T+7ijczcKOvvYgeq6nPUozXH0kxq2WlzoOVdXoayZeXPq/nchrEk/P4RSj4LQgXTeJn NZJiH8jQH38+c3mqt1scCB0U9xpoLtzCuHHlNEj63LI+QKHFxNEeWmbd7gc3OUkDH/R1 XbfUyslatodecA5vwQZm8TSjy+dylg2+8ZqBr5sEwInEUZDegcecciyXeE9rHOSPE4V3 /g== Received: from aserp3030.oracle.com (aserp3030.oracle.com [141.146.126.71]) by userp2120.oracle.com with ESMTP id 36dn4wkyja-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Wed, 03 Feb 2021 05:20:44 +0000 Received: from pps.filterd (aserp3030.oracle.com [127.0.0.1]) by aserp3030.oracle.com (8.16.0.42/8.16.0.42) with SMTP id 1135JWRK154071; Wed, 3 Feb 2021 05:20:43 GMT Received: from nam11-co1-obe.outbound.protection.outlook.com (mail-co1nam11lp2172.outbound.protection.outlook.com [104.47.56.172]) by aserp3030.oracle.com with ESMTP id 36dh1q5kq9-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Wed, 03 Feb 2021 05:20:43 +0000 ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=H074bWoqhWM46nVbBFsVoxRkzeCacXMrr0+gHfAX5njoaiM2iM8TipFINPc2jmx7e2i2Q2Bh4OSEMQFK0onbwPuUKGDNbFzANj62cXcvroC7GjHY1NB6MqCC4WMdzDlBvOS2N5MzPTH3PlIm0s2EqdPj5eiex+M22/CzUhFWvTpTIxwdexFpCihBhm5eNj8MHv3JVGK5HHl/LMaXd2Wi2vKfVUFCDLLMoZnLFvh8rrVu1lt/BlvTXbrttczJJiWxK0po2+/G1cPUGXboU1WfibqGueH88cYz32wOnqKNoVq/GXJrVhecf8zViWO5e/t35GMQVIoV6F4vX6OqwkddAg== 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=KKwBUeDCN/J6XYXNeePLxv7SID499Dlftp3I2MTou5Y=; b=WIb3oNaeIqvpLoaEVF/X79ybR1irZLsde2Zs8XZ2uMVpKP6fUOmjZqQPAOXcVhVKX5z7XY/Z+DFEPuua1oW8GohndpDNMrb0xpIyhdw8Aj97SBno6p08/NNYBpbPxsSrRicvTq74DYRxIMruAdoaGWBxaTXY0s7RdRv0aFcJ5RLO5gFw9SGqp3UvpmS2IBU//vkl+TtGtUTd2KcxDK5oOgH2my+FH9tEnG3yQ022cKOjjvCKjYU8e4aG5HO4f/GeftXRXHu1nY45M4lO9jp/fFnGljMNAdzd+0odkzjqyOKv7GPClHzxtxEGA+lmEa71GkBV49SfVHxSomCgOKDU/A== 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=KKwBUeDCN/J6XYXNeePLxv7SID499Dlftp3I2MTou5Y=; b=HNP0YfFUfd7PyDQHi8BwMdSweeQeG2Acbr3tJu0lYTeom5OQ+qyDFmGS50rT1WmF7Y3bAjZi6nFbQk1LumLdZFCl7Z9HqYQbL9ft9lOSwsNrIlntAiMi4K//RJ9LIyYZ8gSRIS9GqLJBq+H+Z0SPdZqWWWSVw56FGg41DEZjxM8= Received: from SJ0PR10MB4605.namprd10.prod.outlook.com (2603:10b6:a03:2d9::24) by BYAPR10MB3559.namprd10.prod.outlook.com (2603:10b6:a03:11e::14) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.3805.17; Wed, 3 Feb 2021 05:20:41 +0000 Received: from SJ0PR10MB4605.namprd10.prod.outlook.com ([fe80::b1e9:811d:aa8e:f62a]) by SJ0PR10MB4605.namprd10.prod.outlook.com ([fe80::b1e9:811d:aa8e:f62a%6]) with mapi id 15.20.3805.028; Wed, 3 Feb 2021 05:20:35 +0000 Subject: Re: [PATCH v6 6/9] OvmfPkg/SmmCpuFeaturesLib: init CPU ejection state 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-7-ankur.a.arora@oracle.com> <60b9b4ce-b4cc-1399-1ac9-77279762f331@redhat.com> From: "Ankur Arora" Message-ID: Date: Tue, 2 Feb 2021 21:20:28 -0800 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.6.0 In-Reply-To: <60b9b4ce-b4cc-1399-1ac9-77279762f331@redhat.com> X-Originating-IP: [148.87.23.11] X-ClientProxiedBy: MWHPR20CA0033.namprd20.prod.outlook.com (2603:10b6:300:ed::19) To SJ0PR10MB4605.namprd10.prod.outlook.com (2603:10b6:a03:2d9::24) MIME-Version: 1.0 X-MS-Exchange-MessageSentRepresentingType: 1 Received: from [192.168.0.108] (148.87.23.11) by MWHPR20CA0033.namprd20.prod.outlook.com (2603:10b6:300:ed::19) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.3825.17 via Frontend Transport; Wed, 3 Feb 2021 05:20:32 +0000 X-MS-PublicTrafficType: Email X-MS-Office365-Filtering-Correlation-Id: e3eb383b-981a-4b1f-ac96-08d8c8036f22 X-MS-TrafficTypeDiagnostic: BYAPR10MB3559: 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: OIkNW72zidSqgK0klvALSRASFMFT1mrsiCG4s9TrIoSobt8qtELr/OPz7MUHmSc+6rHQ1Dk0zvtuRl9gs5I0ktxeygBKYILeLerHw8esowJdaXvOBt57qrw22USyEY7GWwdqcToSu6EH7J0QAeK43H1M4Meju8gEgo6C9W9QQD4UXM9cNwSkGmmosBWwsAoq3pCrrMgbzFW7M82LYkGxKle0JzBaYKnDCrnzmKPKJ919h6TGj43Cv8uPwgbpFoqiX7hD5xr880Q7j3/iPpCpcbpM5URCVR5Y1XDdDAf/FsFmuk0j9cuZRq/q51ZqyJDLZSCuTuM+SAIENB8oc2u59mge2vcLfIXQ8RdqZH/UqyXQuFc+P9lLGU7wtdEsAGEAMP61TwH+v0FsIQhGd0NjazJZbsZnJJUiX6kAowwy8eblof1hYfSnlBGY5N57YedD5Z1lF5lxdwgR33dTd2q2/+8jqsoUQXwiIWvfUEVnqb45JUPB0fVozH5gs6LybGjZcEjbJlveace1Lr8FmeULgXKb6J0h25PgEejOnWjW0PRahmd2XUGvuGGq2Ne5An13mSyJvtdsa1c4J5gVqD3Nm+B0aM2srVSUisSS+wx77WZvvWNBU/CUpLZX/AlTpYkY/gxfQ0MF4MJXQ6vlgw9iPvZNlxVDAyJGNhMMvEZJE7SF9sMg5/urTJWi7NJTOM0O X-Forefront-Antispam-Report: CIP:255.255.255.255;CTRY:;LANG:en;SCL:1;SRV:;IPV:NLI;SFV:NSPM;H:SJ0PR10MB4605.namprd10.prod.outlook.com;PTR:;CAT:NONE;SFS:(376002)(39860400002)(346002)(366004)(396003)(136003)(31686004)(2906002)(66946007)(6486002)(26005)(956004)(19627235002)(2616005)(53546011)(4326008)(16576012)(83380400001)(30864003)(16526019)(31696002)(8936002)(86362001)(66556008)(5660300002)(966005)(36756003)(54906003)(107886003)(6666004)(478600001)(316002)(8676002)(186003)(66476007)(45980500001)(43740500002);DIR:OUT;SFP:1101; X-MS-Exchange-AntiSpam-MessageData: =?utf-8?B?NkJlSU9kb3FrRUVlQVdlMHVYdHNoNnBIc0UvOWJhdjc3OXduSkgvVnc1NENL?= =?utf-8?B?M0JwY0hQWHJuNm1OSUhCY3BCTkl5R20vL2xRYis0aHhtd2ppb3RtS3VVVFE3?= =?utf-8?B?QW1lNHJoMWJDbGNhb0pIZE5Bb2pEbHhpU2lrZGY1Q2hXeFJJbmthQTNtb2Nw?= =?utf-8?B?OElGWWlHZ2ZZeDE2eXdHaGJJbzFnQldqSEw2a2M4ZWxMeEU2c285ZHJJb29z?= =?utf-8?B?ZThQeUVRSG45Tmp3amxQeGwwT2ZNM0lrdEp1ZWxrM2RhU1R6S1QyQ3ZCY3hx?= =?utf-8?B?ZjVqakhUOUxnN3VyRGwvMkp0Q1hRc1F0TklSNVFRNkg4eE0wVFc2NVQra0pv?= =?utf-8?B?Uzh4eGlyMEFBUUxIc2tQSWFEd3hUSEl5bnlkdE9SQ3JremMwV0NmQlJnZ3Zy?= =?utf-8?B?UmwvTVZzcFREV3dOVHBRK290UVNhbnFyWU95RmJ5YWozVTQzbVhtVUtCQ1ZH?= =?utf-8?B?WEo5aW0zZDY4MHZXdER0RjFhRVRrUG9uSXNGMW85Ykk4U3NPa01LMmxndVpz?= =?utf-8?B?ZGMrVjN3SjNBWmpjSjlyeXNNWFVBbzY4OGhuWTZVNEdBbEJpL0Z6K2I4c0Rn?= =?utf-8?B?cmlYOUcwbTJ0aU5KMVRjQTNNeHAvZ0pBNEdkVWRQRGM3SlRzTTBETG1LUW1I?= =?utf-8?B?MjZnaDlGYUUyL1ZnZXpsWm9MNHJEK2VrYlBCNnRRMnhrSlJjUithQ1RJSVB4?= =?utf-8?B?MlU3QlRGL3g5Mkd3L0s4eElhRDRFNGdzajQ1ZEt4MlNJc2hxUGVJWXQ4VU1j?= =?utf-8?B?S240aEZvaklNT3JmOG9YU1VKUy96VHZyZWRRSEtxWW9iWlVsVkNJSXJSL2ti?= =?utf-8?B?SDQ3blpTMEhPY01xalVrY3czNE5za216SEJPQkprYUNBN3JTd1daZUtIYXpl?= =?utf-8?B?d1BncVdGRlBjUVgxekRZNXo1TklHeUZhRlVvV3hROWY3MFNwRFl1bTZtS2pz?= =?utf-8?B?MStDcTN5OElsS0ROeDJKRVpmQ3g2MnJQUFZsa1ZIRkY0ellnZkIrUVdsR1lY?= =?utf-8?B?Ly9pVFpUME9rUGVabWhFM0dQVkFLYjFiNUUrZHVYS012K1pKejd6T2phUmRY?= =?utf-8?B?K1dTRUJQa0NPMkdMNlRKSlZmdmFXRHlIQlR3M3RUVlRQMjE1alBZRmc1S05o?= =?utf-8?B?Tm8wcTBVeDh4YW5uWTRqL2VGaXVTZTdUQmY2TW1jQkVsc0Y5N1R5UENNUm4x?= =?utf-8?B?QlZxM3BNdDFSY21KK3BTR3Q3bEJWcFpXU2c4eFZjWEFRYitXQTdnWVh0ME9x?= =?utf-8?B?VGVSK1ppNGhYNC9ibmhpUjRMVk83dEdjWi9DQTJMMndnWXVCUG9wM1AzUFBR?= =?utf-8?B?YTZFRCtqVUxkdlV0cWFSRjVSOVVRdUN0QjBhZjU4d2trMTVlcTFERVJLdnNL?= =?utf-8?B?bnBIQTVRUjFZOGpnUlVmMUF3Tjh2cUVtV1pnUDdtamk0VktQcjVaVHBTZTYw?= =?utf-8?Q?gDAjc3Zn?= X-OriginatorOrg: oracle.com X-MS-Exchange-CrossTenant-Network-Message-Id: e3eb383b-981a-4b1f-ac96-08d8c8036f22 X-MS-Exchange-CrossTenant-AuthSource: SJ0PR10MB4605.namprd10.prod.outlook.com X-MS-Exchange-CrossTenant-AuthAs: Internal X-MS-Exchange-CrossTenant-OriginalArrivalTime: 03 Feb 2021 05:20:35.0086 (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: OrE4Xho8Cve58xCq6tOUror/Z8gJod6GvwWT3AkyZDyvRgCSQkUtZnh5eAVfIuz/sDu2rYx0NK944OO+yz3ea8ZK7nz5dEOvDzO6uBCzWKA= X-MS-Exchange-Transport-CrossTenantHeadersStamped: BYAPR10MB3559 X-Proofpoint-Virus-Version: vendor=nai engine=6200 definitions=9883 signatures=668683 X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 adultscore=0 spamscore=0 phishscore=0 suspectscore=0 mlxlogscore=999 bulkscore=0 mlxscore=0 malwarescore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2009150000 definitions=main-2102030030 X-Proofpoint-Virus-Version: vendor=nai engine=6200 definitions=9883 signatures=668683 X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 impostorscore=0 lowpriorityscore=0 spamscore=0 priorityscore=1501 suspectscore=0 phishscore=0 mlxlogscore=999 malwarescore=0 clxscore=1015 bulkscore=0 adultscore=0 mlxscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2009150000 definitions=main-2102030030 Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit On 2021-02-01 5:36 a.m., Laszlo Ersek wrote: > On 01/29/21 01:59, Ankur Arora wrote: >> Init CPU_HOT_EJECT_DATA, which will be used to share CPU ejection state >> between SmmCpuFeaturesLib (via PiSmmCpuDxeSmm) and CpuHotPlugSmm. >> CpuHotplugSmm also sets up the CPU ejection mechanism via >> CPU_HOT_EJECT_DATA->Handler. >> >> Additionally, expose CPU_HOT_EJECT_DATA via PcdCpuHotEjectDataAddress. > > (1) Please mention that the logic is added to > SmmCpuFeaturesSmmRelocationComplete(), and so it will run as part of the > PiSmmCpuDxeSmm entry point function, PiCpuSmmEntry(). > > >> >> 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 >> --- >> .../SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf | 3 + >> .../Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c | 78 ++++++++++++++++++++++ >> 2 files changed, 81 insertions(+) >> >> diff --git a/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf b/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf >> index 97a10afb6e27..32c63722ee62 100644 >> --- a/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf >> +++ b/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf >> @@ -35,4 +35,7 @@ [LibraryClasses] >> UefiBootServicesTableLib >> >> [Pcd] >> + gUefiCpuPkgTokenSpaceGuid.PcdCpuHotPlugSupport >> + gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber >> + gUefiOvmfPkgTokenSpaceGuid.PcdCpuHotEjectDataAddress >> gUefiOvmfPkgTokenSpaceGuid.PcdQ35SmramAtDefaultSmbase >> diff --git a/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c b/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c >> index 7ef7ed98342e..33dd5da92432 100644 >> --- a/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c >> +++ b/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c >> @@ -14,7 +14,9 @@ >> #include >> #include >> #include >> +#include > > (2) The MemoryAllocationLib class is not listed in the [LibraryClasses] > section of "OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf"; so > please list it there as well. > > (Please keep the [LibraryClasses] section in the INF file sorted, while > at it.) > > >> #include >> +#include > > (3) This will change, once you move the header file under > "OvmfPkg/Include/Pcd/"; either way, please keep the #include directives > alphabetically sorted. > > (Before this patch, the #include list is well-sorted.) > > >> #include >> #include >> #include >> @@ -171,6 +173,70 @@ SmmCpuFeaturesHookReturnFromSmm ( >> return OriginalInstructionPointer; >> } >> >> +GLOBAL_REMOVE_IF_UNREFERENCED > > (4a) This is useless unless building with MSVC; I don't really remember > introducing any instance of this macro myself, ever. I suggest dropping > it. Will drop. As you said, it's a NOP for !MSVC. Just as a sidenote, I do see two copies of the mCpuHotEjectData in the PiSmmCpuSmm and CpuHotplugSmm maps (which makes sense, given that both include SmmCpuFeaturesLib): .bss.mCpuHotEjectData 0x0000000000017d60 0x8 /tmp/PiSmmCpuDxeSmm.dll.0k4hl8.ltrans1.ltrans.o .bss.mCpuHotEjectData 0x0000000000005110 0x8 /tmp/CpuHotplugSmm.dll.ixiN9a.ltrans0.ltrans.o I imagine they do get unified in the build process later, but that's the point my understanding stops. > > (4b) On the other hand, STATIC it should be. Yeah, that it should be. > >> +CPU_HOT_EJECT_DATA *mCpuHotEjectData = NULL; >> + >> +/** >> + Initialize CpuHotEjectData if PcdCpuHotPlugSupport is enabled >> + and, if more than 1 CPU is configured. >> + >> + Also sets up the corresponding PcdCpuHotEjectDataAddress. >> +**/ > > (5) typo: s/CpuHotEjectData/mCpuHotEjectData/ > > > (6) As requested elsewhere under v6, there's no need to make this > dependent on PcdCpuHotPlugSupport. > > > (7) "Initialize" is imperative mood, "sets up" is indicative mood. > Either one is fine, just be consistent please. Will fix, and thanks for the very careful reading. Thanks Ankur > > >> +STATIC >> +VOID >> +SmmCpuFeaturesSmmInitHotEject ( > > (8) This is a STATIC function (i.e., it has internal linkage); there's > no need to complicate its name with the "SmmCpuFeatures..." prefix. > > I suggest "InitCpuHotEjectData". > > >> + VOID >> + ) >> +{ >> + UINT32 mMaxNumberOfCpus; > > (9) This is a variable with automatic storage duration, so the "m" > prefix is invalid. > > >> + EFI_STATUS Status; >> + >> + if (!FeaturePcdGet (PcdCpuHotPlugSupport)) { >> + return; >> + } > > (10a) Please drop this, per prior discussion. > > (10b) Please drop the PCD from the INF file too. > > > (11) In the rest of this function, the comment style is incorrect in > several spots. The idiomatic style is: > > // > // Blah. > // > > I.e., normally we'd need leading and trailing empty comment lines. > > *However*, most of those comments don't really explain much beyond > what's emergent from the code anyway, to me anyway, thus, I would simply > suggest dropping those comments. > > >> + >> + // PcdCpuHotPlugSupport => PcdCpuMaxLogicalProcessorNumber >> + mMaxNumberOfCpus = PcdGet32 (PcdCpuMaxLogicalProcessorNumber); >> + >> + // No spare CPUs to hot-eject >> + if (mMaxNumberOfCpus == 1) { >> + return; >> + } >> + >> + mCpuHotEjectData = >> + (CPU_HOT_EJECT_DATA *)AllocatePool (sizeof (*mCpuHotEjectData)); > > (12) The cast is superfluous (it only wastes screen real estate), as > AllocatePool() returns (VOID *). > > (Hopefully this will also let us avoid the somewhat awkward line break.) > > >> + ASSERT (mCpuHotEjectData != NULL); > > (13) Here we need to hang harder than this -- even in a RELEASE build, > in case AllocatePool() fails. The following should work: > > if (mCpuHotEjectData == NULL) { > ASSERT (mCpuHotEjectData != NULL); > CpuDeadLoop (); > } > > I'll have another comment on this, below... > > >> + >> + // >> + // Allocate buffer for pointers to array in CPU_HOT_EJECT_DATA. >> + // >> + >> + // Revision >> + mCpuHotEjectData->Revision = CPU_HOT_EJECT_DATA_REVISION_1; >> + >> + // Array Length of APIC ID >> + mCpuHotEjectData->ArrayLength = mMaxNumberOfCpus; >> + >> + // CpuIndex -> APIC ID map >> + mCpuHotEjectData->ApicIdMap = (UINT64 *)AllocatePool >> + (sizeof (*mCpuHotEjectData->ApicIdMap) * mCpuHotEjectData->ArrayLength); >> + >> + // Hot-eject handler >> + mCpuHotEjectData->Handler = NULL; >> + >> + // Reserved >> + mCpuHotEjectData->Reserved = 0; >> + >> + ASSERT (mCpuHotEjectData->ApicIdMap != NULL); >> + > > (14) I would propose the following: > > (14a) Add SafeIntLib to both the #include directive list, and the > [LibraryClasses] section in the INF file. > > (14b) Use SafeIntLib functions to calculate the cumulative size for both > CPU_HOT_EJECT_DATA, and the ApicIdMap placed right after it, in a local > UINTN variable. So I faintly see why using SafeInt might be a good (edge cases, overflow etc) but given that the SMRAM region (and max CPU count) is much smaller than any overflow case, could you expand on the logic behind using SafeIntLib? Either way, will fix. > > (14c) Use a single AllocatePool() call. This simplifies error handling > -- you'll need just one instance of point (13) above --, plus it might > even reduce SMRAM fragmentation a tiny bit. Makes sense. > > > (15) The following initialization logic, from patch v6 7/9 > ("OvmfPkg/CpuHotplugSmm: add CpuEject()"), belongs in the present patch, > in my opinion: > > // > // For CPU ejection we need to map ProcessorNum -> APIC_ID. By the time > // we need the mapping, however, the Processor's APIC ID has already been > // removed from SMM data structures. So we will maintain a local map > // in mCpuHotEjectData->ApicIdMap. > // > for (Idx = 0; Idx < mCpuHotEjectData->ArrayLength; Idx++) { > mCpuHotEjectData->ApicIdMap[Idx] = CPU_EJECT_INVALID; > } > > If necessary, feel free to trim or reword the comment; I just think the > data structure is not ready for publishing via the PCD until the > "ApicIdMap" elements have been set to INVALID. (IOW, I'm kind of making > a "RAII" argument.) The original reason was that I did not want to expose the CPU_EJECT_* values outside CpuHotplugSmm. That's not really true any more -- given that they are in a common header. Will move that logic here. > > >> + // >> + // Expose address of CPU Hot eject Data structure >> + // > > (this comment is helpful, please keep it) > > >> + Status = PcdSet64S (PcdCpuHotEjectDataAddress, >> + (UINT64)(VOID *)mCpuHotEjectData); > > (16) Incorrect indentation on the second line. > > > (17) The (UINT64) cast could trigger a warning in an IA32 build (casting > between integer and pointer types should keep the width); please replace > (UINT64) with (UINTN). > > >> + ASSERT_EFI_ERROR (Status); > > (18) Given that we don't use the "Status" variable for anything else in > this function, it's more idiomatic for "Status" to directly match the > type returned by PcdSet64S() -- RETURN_STATUS. In such cases, we > generally call the variable "PcdStatus". So the idea is > > RETURN_STATUS PcdStatus; > > PcdStatus = PcdSet64S (...); > ASSERT_RETURN_ERROR (PcdStatus); > > RETURN_STATUS and EFI_STATUS behave identically in practice, but (again) > if we use the status variable *only* for a PcdSet retval, then > RETURN_STATUS is more elegant. > > (RETURN_STATUS is basically a BASE type, while EFI_STATUS exists in > connection with the PI and UEFI specs; IOW, RETURN_STATUS is > semantically more primitive / foundational.) > > The usage of an ASSERT is fine here, BTW; we don't expect this PcdSet > call to ever fail. > > >> +} >> + >> /** >> Hook point in normal execution mode that allows the one CPU that was elected >> as monarch during System Management Mode initialization to perform additional >> @@ -188,6 +254,9 @@ SmmCpuFeaturesSmmRelocationComplete ( >> UINTN MapPagesBase; >> UINTN MapPagesCount; >> >> + >> + SmmCpuFeaturesSmmInitHotEject (); >> + >> if (!MemEncryptSevIsEnabled ()) { >> return; >> } >> @@ -375,6 +444,15 @@ SmmCpuFeaturesRendezvousExit ( >> IN UINTN CpuIndex >> ) >> { >> + // >> + // CPU Hot-eject not enabled. >> + // >> + if (mCpuHotEjectData == NULL || >> + mCpuHotEjectData->Handler == NULL) { >> + return; >> + } >> + >> + mCpuHotEjectData->Handler (CpuIndex); >> } >> >> /** >> > > (19a) Please split the SmmCpuFeaturesRendezvousExit() change to a > separate patch. > > In particular, "init CPU ejection state" in the subject does not cover > the SmmCpuFeaturesRendezvousExit() change at all. > > (19b) In the separate patch's commit message, it would be nice to > mention the *call site* of SmmCpuFeaturesRendezvousExit(), such as "one > of the last actions in SmiRendezvous()". > > > (20) I think we should refine the comment "CPU Hot-eject not enabled". > That comment covers the (mCpuHotEjectData == NULL) case, yes; but it > doesn't cover (mCpuHotEjectData->Handler == NULL). > > The latter condition certainly seems valid, because: > > - some SMIs are likely handled before the SMM driver dispatch reaches > the CpuHotplugSmm driver, and the latter gets a chance to set up the > callback, as a part of erecting the CPU hot-(un)plug support, > > - and even after CpuHotplugSmm is loaded, an unplug request may never > happen. > > However, we should document this particular state, with a dedicated > comment -- perhaps just say, "hot-eject has not been requested yet". Thanks, these are quite helpful. Will fix. Ankur > > Thanks! > Laszlo >