From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from aserp2120.oracle.com (aserp2120.oracle.com [141.146.126.78]) by mx.groups.io with SMTP id smtpd.web08.48485.1612246576331243517 for ; Mon, 01 Feb 2021 22:16:16 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@oracle.com header.s=corp-2020-01-29 header.b=KUhynSFE; spf=pass (domain: oracle.com, ip: 141.146.126.78, mailfrom: ankur.a.arora@oracle.com) Received: from pps.filterd (aserp2120.oracle.com [127.0.0.1]) by aserp2120.oracle.com (8.16.0.42/8.16.0.42) with SMTP id 1126Evcj039031; Tue, 2 Feb 2021 06:16:12 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=nCH7IgCfm85aQetSnzjoYPTp5AGB9Zoh8HU0GrB7WHw=; b=KUhynSFEfSnq4v0qRGw06DiFlfzyM0riCNo6xEv4HmDFJqprIDV9t+HN2LyWK+wcKwiK +MtFgYVPXHtu/u95YDgELBxBhFHNEafQAyRvAKeLnq7HZba0P1dSKubS35f755fkp1do um1sRButlFpslZzZvl7OnED3OZgLz7v03GlvP2N2s35fxRvWWbcyM0T3YBHu3RKBbRob l4fcPsivMj9NmesHGHql4u0JgjFWu51RFdSYTzz98s4R3T1jT1ONzcvwDtINJjCp1J2w dT06pSjqFxE8HT9ws8TtC8VxzQ2QBrB3PE14KgUrFVdHb2cYdxK61qPwTaT2rEwgJBx7 dQ== Received: from userp3020.oracle.com (userp3020.oracle.com [156.151.31.79]) by aserp2120.oracle.com with ESMTP id 36cydks00q-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Tue, 02 Feb 2021 06:16:12 +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 1126GA12037262; Tue, 2 Feb 2021 06:16:11 GMT Received: from nam02-bl2-obe.outbound.protection.outlook.com (mail-bl2nam02lp2051.outbound.protection.outlook.com [104.47.38.51]) by userp3020.oracle.com with ESMTP id 36dh7qwvum-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Tue, 02 Feb 2021 06:16:11 +0000 ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=XNEhBBR8mBLOzQ22k42NmY9JgWWu5/tFwr2DJqgKHrQlLMtsWjFoeF+783riw5OyNDD3QuDA4zinf2c8GRxZxL8fyu7l9roc1ugYKXi+BQe1UeZtoXsNoEwHfWpZMEaPsFAeDnVgUVry6JaBeX2qB0O+G/u25+dWreX+UFBM6er7D/kjD30TWnVvbLAOa4AmB34Ch63cja1RGTFdEr7h+EkMKLADDRi+UGUDSpvxm4ay9E987Ed1FOgJexEqsnF9MO7pSPuDKQQz31YZ6KF75tR7u9OWsKO/yUUDXjT++t29mfRMCY87JeLq5ObuWrXXw4MMtpBPmT//ql4R5JZ9DQ== 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=nCH7IgCfm85aQetSnzjoYPTp5AGB9Zoh8HU0GrB7WHw=; b=E6Lwij1jGYm+sZC2qrEgKqz2qrYKR1Zli9/klT1gmJxPg27iPnQQ78IYhvJE13Nhipg+FYkFciHLyd3ETatpiKh28MLOI8NnPAhCDnptfQZsQwN+oq4/a26QDXvHs2P0iz9brI9lZVlbpnwvibZQWxXRGtRreUBlHgYBaqMrvptNukh4eWIhunTiM4hEYkB689VXkPvyXhr5XlvUMLWemXzB1HnH84YhulP+hMcT8tjEtrrMrOUe0MUOqtvLI1yTHmI/IMjuEYVVMxImweAo50lUGcxMndhe/FyfFMnQQi4LnwXYrc8+x764VTQWT3PNFNnPM8XrkEL/gM93k33HuA== 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=nCH7IgCfm85aQetSnzjoYPTp5AGB9Zoh8HU0GrB7WHw=; b=g5tHVE5Dc4Cnl5+Jm0/U5AUqICs89JoDsMev62SHp9ynGJ2nVSnTp0FX2PffdZqcRTZdOJ6I7EBJajjF87fIsVetLoDw52FYhd6yY9ni4+RTMx4yZ66MWN/z+H/aIfysIja+ogJ9Nn6G8CmpTA+BxuxdwRcZs6kU+v63RR3h/ws= Received: from SJ0PR10MB4605.namprd10.prod.outlook.com (2603:10b6:a03:2d9::24) by BYAPR10MB3032.namprd10.prod.outlook.com (2603:10b6:a03:82::15) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.3805.17; Tue, 2 Feb 2021 06:15:58 +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; Tue, 2 Feb 2021 06:15:57 +0000 Subject: Re: [PATCH v6 5/9] OvmfPkg/CpuHotplugSmm: define CPU_HOT_EJECT_DATA 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-6-ankur.a.arora@oracle.com> From: "Ankur Arora" Message-ID: Date: Mon, 1 Feb 2021 22:15:55 -0800 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.6.0 In-Reply-To: X-Originating-IP: [70.36.60.91] X-ClientProxiedBy: MWHPR20CA0024.namprd20.prod.outlook.com (2603:10b6:300:13d::34) 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 MWHPR20CA0024.namprd20.prod.outlook.com (2603:10b6:300:13d::34) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.3805.17 via Frontend Transport; Tue, 2 Feb 2021 06:15:57 +0000 X-MS-PublicTrafficType: Email X-MS-Office365-Filtering-Correlation-Id: 632a4b6f-ae3e-4e68-1375-08d8c7420152 X-MS-TrafficTypeDiagnostic: BYAPR10MB3032: 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: kgiJu4q9r2IiR++4eb9lPt9O3yvPMGCwprSaQAUBb/Exla5NIHOu0mqeoSSzkcRc/fKy3oztlFASTFF2fIvOt2kfufb9OKK8rrlwwKsf2J4PDs9kHp5ySUjjULFN0kExGgFTVwnyZqbCnnoTYj07ZI/2js2f7rXIIh3l2yHq0Ugw8MEFveJjbVyexpcOXlzNbbBjb9CHUlTrSssysClTYwkb/stsjHbwgLyjssvuwGx1GX3YC9e1wkGN0nkSts7FoUn2Rh3SZAui2DVSFWR3ilTBcPaJjyE5paGOcUY/IhuLbQbV+1IqevGkVdbbJ0Hf4fXQKY/v1DQLNPnJNpsxWGIEyepHbpk+iV3RW7AlgG861bCQEN6dxVUO59ezdfeHodUpJpVAXYcJYKanFxzC1L9HyX7XJP795wQyEnZkHMVGbWf0aNdnTm9b48F/be/CB4H3flQf8Jeli4rLoFBn/1AzhMRrFqzccr6Ch3r3jpvCI3CQmtw14+ldmnRJ2uiF9X4TLv+q4GCfGvDSr6jwiiX6rI1zho7FSNqCgEi27HpcbJJFZ4+L8I/zmD3iOOI+aOjQ01Vf0RtZZTvfVVVuxzjUC5r2jwTGRlRDDV3I50Ezbj2iUrzDRqSKT/3Y1nh8Y8n7PG4ayGUBqJoGArAeD1OhCZu/MqT/zcQSwf1KTMSCyQ7bIjoaCcho1Ix7TxSy 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:(136003)(396003)(376002)(346002)(366004)(39860400002)(8676002)(66556008)(66946007)(31696002)(66476007)(31686004)(478600001)(5660300002)(36756003)(54906003)(316002)(30864003)(16576012)(966005)(956004)(2616005)(107886003)(16526019)(2906002)(6486002)(83380400001)(186003)(4326008)(8936002)(86362001)(53546011)(26005)(43740500002)(45980500001);DIR:OUT;SFP:1101; X-MS-Exchange-AntiSpam-MessageData: =?utf-8?B?c2hvbGlUNGQrQ2tNMW5WWndBZE1FYSt6dHVLSjRhS3UvUkV6VUFjOHovYjUx?= =?utf-8?B?aFZaRnVrcmkwYzJnWGp2NmdxLzB0OHEyNWZFK2pKYUpOek1RZEt2V2ZVRVJo?= =?utf-8?B?aXQ4VWk3dm4zeWZxUzR0RFJ3R0w4aGUwMk9kVnZNbDRsR3RQUldNU0hQMUY0?= =?utf-8?B?MFZQWmlOSXF1VVNERHFKUHNZclkzTUZXQVZIeTZxSWdnV0oramk5ODlCZmd6?= =?utf-8?B?RjZLcWpUY0ErLzBydEZFQTNsUjBQZVhpMWJUbUk1SEF5cDJ5aTJ1c3E1eUN2?= =?utf-8?B?WUJ3Z2tYU1JpTjkzc1drMkFORzd1R3VwUXpleHhYQ1FOWWs0TUVBN1pXL2NJ?= =?utf-8?B?aVNRVGtMZzFSaUFBNk9TYTZ0aVJIdDdmZmR6ZmcxclZvVUR3ejQ3by9RL1dy?= =?utf-8?B?VnU3VEZBU2RKZ0dNZEhwZHAxL1U5TW1NUC84MTZUSWJ1dXMvcWNiY3JFT1M5?= =?utf-8?B?QUNiRzVxemp2RUhudmFTVDh3Wlg0UWh4aEt4WGFaNjlSMmt5blNTdGkwREFj?= =?utf-8?B?dHB4VUV2VlZlbzBCNlM5MmsrMWl5MnZFTmZ4eGVKWXVySThMMFpRSHVJWVJP?= =?utf-8?B?Q2MxOWI5MDY3eXRXbzVEeUU5VGpXQ1prZG05aGszMDVMTmg4K2grTDNtMkdF?= =?utf-8?B?dWZ5OHNzUTZaVVJzOFJkYU9BaWo0dFBVZjZJVWV2SDJ0ZzNISGxiWHdXMDBy?= =?utf-8?B?Vm9ZQW14dk82aFRYblp6ZUgvdExPOENqSFJvWVQwRElHaFFZbTltRG9zcXFE?= =?utf-8?B?SFVvZW9aWk9rUlpnaGRZM1F3cmVYQjdUeGxRSUQ1c2JhSnEvYXhkb2NRcVhk?= =?utf-8?B?L09UdXhOREs4S1hNeUtOalVSTHl4TW1OMEVjMnJPMC9RbExGRVB4YXp1T3NI?= =?utf-8?B?MG1FdkVOdG5JSHRySGlsVUhMRytvU0xHMHd4Z3MrRlhvazVvTzRpSUhFK25Q?= =?utf-8?B?cHh6ZVlDVHZ5b2NnSFFiWE02UWxXNG1oTG1OQVNsalBETW9IRHJWUXI1UktY?= =?utf-8?B?MVJUZWFUbFpaa1hwUXpjUWptYURhbjZRVEZBYzRxTVJUTlFJeGhMZFp5UlhG?= =?utf-8?B?UVRVUGRJdmVDclFoWE85RWRNRFpCSXNyUTBmclRrbGs4VVNmL2ZCcmZYWlNH?= =?utf-8?B?K2djb252ZHlyUmhZRjI5S3hIZmM0MnRlWHdTbjBrdjVPS3MyNzVOQU1KMEg5?= =?utf-8?B?U1RNdTdnN2tjM0hab2JRVEt3RDNZVC9sMkp6bXBhTjBnTmNWL1UwaXBIOVRL?= =?utf-8?B?dDArN1NBOEFYdXhaMmtnM2dpb2lQMVZmeGtUV0tEWkVSQitaTEJmdDJwYklZ?= =?utf-8?B?WUE1QjBrN0xmWWR0bzRXOUNMcTNybzUrMUhkTWRtbHVGSzhPejJIZ1Q1NCs4?= =?utf-8?B?SXJ5dUg1SFUzaExlTGI1d0E1bDFPcXhPekxRVDdKUEtQV2VDSzY4T2NrN3dr?= =?utf-8?Q?JkcTsql+?= X-OriginatorOrg: oracle.com X-MS-Exchange-CrossTenant-Network-Message-Id: 632a4b6f-ae3e-4e68-1375-08d8c7420152 X-MS-Exchange-CrossTenant-AuthSource: SJ0PR10MB4605.namprd10.prod.outlook.com X-MS-Exchange-CrossTenant-AuthAs: Internal X-MS-Exchange-CrossTenant-OriginalArrivalTime: 02 Feb 2021 06:15:57.8453 (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: 0YK3aXWvGVYeGw55gkLbruSa21pIWbYCWAhHKUkqbtQKYlO+vk74npJh5B5FPa9ckUtl98pPiou+zAkZFJVSKwcEOoTLXfTWE1qYidZpZyE= X-MS-Exchange-Transport-CrossTenantHeadersStamped: BYAPR10MB3032 X-Proofpoint-Virus-Version: vendor=nai engine=6200 definitions=9882 signatures=668683 X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 malwarescore=0 mlxscore=0 suspectscore=0 spamscore=0 mlxlogscore=999 phishscore=0 adultscore=0 bulkscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2009150000 definitions=main-2102020044 X-Proofpoint-Virus-Version: vendor=nai engine=6200 definitions=9882 signatures=668683 X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 bulkscore=0 adultscore=0 priorityscore=1501 impostorscore=0 malwarescore=0 clxscore=1015 spamscore=0 lowpriorityscore=0 phishscore=0 mlxlogscore=999 mlxscore=0 suspectscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2009150000 definitions=main-2102020044 Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit On 2021-01-31 8:53 p.m., Laszlo Ersek wrote: > On 01/29/21 01:59, Ankur Arora wrote: >> Define CPU_HOT_EJECT_DATA and add PCD PcdCpuHotEjectDataAddress, which >> will be used to share CPU ejection state between OvmfPkg/CpuHotPlugSmm >> and PiSmmCpuDxeSmm. >> >> 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/OvmfPkg.dec | 10 +++++++++ >> OvmfPkg/CpuHotplugSmm/CpuHotplugSmm.inf | 1 + >> OvmfPkg/Include/Library/CpuHotEjectData.h | 35 +++++++++++++++++++++++++++++++ >> 3 files changed, 46 insertions(+) >> create mode 100644 OvmfPkg/Include/Library/CpuHotEjectData.h >> >> diff --git a/OvmfPkg/OvmfPkg.dec b/OvmfPkg/OvmfPkg.dec >> index 4348bb45c64a..1a2debb821d7 100644 >> --- a/OvmfPkg/OvmfPkg.dec >> +++ b/OvmfPkg/OvmfPkg.dec >> @@ -106,6 +106,10 @@ [LibraryClasses] >> # >> XenPlatformLib|Include/Library/XenPlatformLib.h >> >> + ## @libraryclass Share CPU hot-eject state >> + # >> + CpuHotEjectData|Include/Library/CpuHotEjectData.h >> + >> [Guids] >> gUefiOvmfPkgTokenSpaceGuid = {0x93bb96af, 0xb9f2, 0x4eb8, {0x94, 0x62, 0xe0, 0xba, 0x74, 0x56, 0x42, 0x36}} >> gEfiXenInfoGuid = {0xd3b46f3b, 0xd441, 0x1244, {0x9a, 0x12, 0x0, 0x12, 0x27, 0x3f, 0xc1, 0x4d}} > > (1) Please drop this hunk -- the [LibraryClasses] section should not be > modified, as we're not introducing a new library class. > > >> @@ -352,6 +356,12 @@ [PcdsDynamic, PcdsDynamicEx] >> # This PCD is only accessed if PcdSmmSmramRequire is TRUE (see below). >> gUefiOvmfPkgTokenSpaceGuid.PcdQ35SmramAtDefaultSmbase|FALSE|BOOLEAN|0x34 >> >> + ## This PCD adds a communication channel between PiSmmCpuDxeSmm and >> + # CpuHotplugSmm. > > (2) I suggest: > > ## This PCD adds a communication channel between OVMF's SmmCpuFeaturesLib > # instance in PiSmmCpuDxeSmm, and CpuHotplugSmm. > > >> + # >> + # Only accessed if PcdCpuHotPlugSupport is TRUE > > (3) This statement is technically true, but I suggest dropping it. In my > opinion, it is not useful (it's a superfluous statement). Here's why: > > - We set the "PcdCpuHotPlugSupport" feature flag to TRUE in the OVMF DSC > files exactly when the SMM_REQUIRE feature test macro is set on the > "build" command line. > > - The whole SMM infrastructure is included in the firmware binary > exactly when SMM_REQUIRE is set. > > In other words, PcdCpuHotPlugSupport is *equivalent* with > SmmCpuFeaturesLib, PiSmmCpuDxeSmm, and CpuHotplugSmm being included in > the firmware binary. > > Given that the first comment already declares the PCD as an info channel > between SmmCpuFeaturesLib (as built into PiSmmCpuDxeSmm) and > CpuHotplugSmm, the second comment adds nothing. That makes sense. > > >> + gUefiOvmfPkgTokenSpaceGuid.PcdCpuHotEjectDataAddress|0|UINT64|0x46 >> + >> [PcdsFeatureFlag] >> gUefiOvmfPkgTokenSpaceGuid.PcdQemuBootOrderPciTranslation|TRUE|BOOLEAN|0x1c >> gUefiOvmfPkgTokenSpaceGuid.PcdQemuBootOrderMmioTranslation|FALSE|BOOLEAN|0x1d >> diff --git a/OvmfPkg/CpuHotplugSmm/CpuHotplugSmm.inf b/OvmfPkg/CpuHotplugSmm/CpuHotplugSmm.inf >> index 04322b0d7855..e08b572ef169 100644 >> --- a/OvmfPkg/CpuHotplugSmm/CpuHotplugSmm.inf >> +++ b/OvmfPkg/CpuHotplugSmm/CpuHotplugSmm.inf >> @@ -54,6 +54,7 @@ [Protocols] >> >> [Pcd] >> gUefiCpuPkgTokenSpaceGuid.PcdCpuHotPlugDataAddress ## CONSUMES >> + gUefiOvmfPkgTokenSpaceGuid.PcdCpuHotEjectDataAddress ## CONSUMES >> gUefiOvmfPkgTokenSpaceGuid.PcdQ35SmramAtDefaultSmbase ## CONSUMES >> >> [FeaturePcd] > > (4) Please move this hunk to patch#7 (subject: "OvmfPkg/CpuHotplugSmm: > add CpuEject()"). That's where CpuHotplugSmm first needs the new PCD. > > >> diff --git a/OvmfPkg/Include/Library/CpuHotEjectData.h b/OvmfPkg/Include/Library/CpuHotEjectData.h >> new file mode 100644 >> index 000000000000..b6fb629a1283 >> --- /dev/null >> +++ b/OvmfPkg/Include/Library/CpuHotEjectData.h >> @@ -0,0 +1,35 @@ >> +/** @file >> + Definition for a CPU hot-eject state sharing structure. >> + > > (5a) I suggest the following language: > > Definition of the CPU_HOT_EJECT_DATA structure, which shares CPU hot-eject > state between OVMF's SmmCpuFeaturesLib instance in PiSmmCpuDxeSmm, and > CpuHotplugSmm. > > CPU_HOT_EJECT_DATA is allocated in SMRAM, and pointed-to by > PcdCpuHotEjectDataAddress. > > (5b) Please append at least one more sentence to state the condition > when the PCD is *not* NULL. > > > (6) This new header file should be located at: > > OvmfPkg/Include/Pcd/CpuHotEjectData.h Your explanation below makes sense. OvmfPkg/Include/Library did not quite seem like the right place but I wasn't sure what would be better. Will fix. > > please. > > The (more or less) general rule is this: > > - if you have a macro definition or a structure type that is accessible > through a Pcd, a Protocol, a Guid -- HOB, VenHw() devpath node etc --, > a Library, a Register, etc, > > - and the Pcd, Protocol, Guid, Library etc in question is declared in > "WhateverPkg/WhateverPkg.dec", > > - then the header file defining the structure or macro should be placed > in the following directory (according to the access type): > > WhateverPkg/Include/Pcd/ > WhateverPkg/Include/Protocol/ > WhateverPkg/Include/Guid/ > WhateverPkg/Include/Library/ > WhateverPkg/Include/Register/ > > Admittedly, while this rule is universally honored in edk2 in the > Protocol, Guid, and Library cases, the Register case is somewhat less > frequently followed, and the Pcd case is almost nonexistent. For > example, "UefiCpuPkg/Include/CpuHotPlugData.h" itself does not follow > the rule (no "Pcd" subdir). However, there are examples that do follow > the rule: > > CryptoPkg/Include/Pcd/PcdCryptoServiceFamilyEnable.h > RedfishPkg/Include/Pcd/RestExServiceDevicePath.h > > >> + Copyright (C) 2021, Oracle Corporation. >> + >> + SPDX-License-Identifier: BSD-2-Clause-Patent >> +**/ >> + >> +#ifndef _CPU_HOT_EJECT_DATA_H_ >> +#define _CPU_HOT_EJECT_DATA_H_ > > (7) Please use the following guard macro: > > CPU_HOT_EJECT_DATA_H_ > > (i.e., please drop the leading underscore). > > Although the leading underscore is widely used in edk2, in include guard > macros, it's a bad practice (it creates identifiers that are reserved by > the C standard), so we should not introduce more of it. > > >> + >> +typedef >> +VOID >> +(EFIAPI *CPU_HOT_EJECT_FN)( > > (8) Please replace _FN with _HANDLER or _FUNCTION. > > In edk2, we tend to avoid abbreviations. (Yes, the practice has not > entirely been consistent, and sometimes it's actually *annoying* that > our type names are too long. But that's what we got.) > > ... _HANDLER would be better, as you call the related field "Handler" in > the structure. > > > (9) Missing space character before the last parenthesis on the line. > > > (10) Please add a leading comment block on this function prototype. > (Well, yes, I realize it is technically a *pointer* type, but still.) > > This is not just a formality; I'd really like the "ProcessorNum" > parameter to be described, for example its relationship with the > "ProcessorNumber" parameter of EFI_SMM_CPU_SERVICE_PROTOCOL member > functions, and/or the "CPU_HOT_PLUG_DATA.ApicId" array. > > >> + IN UINTN ProcessorNum >> + ); >> + >> +#define CPU_EJECT_INVALID (MAX_UINT64) >> +#define CPU_EJECT_WORKER (MAX_UINT64-1) > > (11a) If these are meant as special values for the "ApicIdMap" array, > then I'd suggest something like: > > CPU_EJECT_APIC_ID_INVALID > CPU_EJECT_APIC_ID_WORKER > > (11b) Can you add a single-sentence comment to each macro? (Observe the > comment style while at it, please.) > > >> + >> +#define CPU_HOT_EJECT_DATA_REVISION_1 0x00000001 >> + >> +typedef struct { >> + UINT32 Revision; // Used for version identification of >> + // this structure > > (12) Please drop both the "CPU_HOT_EJECT_DATA_REVISION_1" macro and the > "Revision" field. > > The "CPU_HOT_PLUG_DATA" structure, from > "UefiCpuPkg/Include/CpuHotPlugData.h", is different. That structure is > versioned because it establishes a communication channel between a core > module (PiSmmCpuDxeSmm) and a platform module (such as > OvmfPkg/CpuHotplugSmm); what's more, those modules could even be built > separately, and be available in binary-only form. > > (Side note: we ignore "CPU_HOT_PLUG_DATA.Revision" in > "OvmfPkg/CpuHotplugSmm" because the OVMF platforms exist in the exact > same repository as PiSmmCpuDxeSmm, so we can keep them in sync. This is > BTW one reason why I absolutely want OVMF to live in the core edk2 > repository. Anyway, digression ends.) > > However, the same versioning idea (or requirement) does not hold for the > present use case. The new communication channel is between: > > - OVMF's SmmCpuFeaturesLib instance in PiSmmCpuDxeSmm, > - and CpuHotplugSmm. > > Both of those are OVMF platform modules, and we never build one without > building the other. (Put differently, we never build PiSmmCpuDxeSmm and > CpuHotplugSmm separately, for any particular OVMF binary.) > > Thus, the "Revision" field is useless. Agreed. I was unsure about adding this field -- but wasn't sure if there might be situations when CpuHotplugSmm and PiSmmCpuDxeSmm might come separately or not. Thanks for clarifying it. > > >> + UINT32 ArrayLength; // Entries in the ApicIdMap array >> + >> + UINT64 *ApicIdMap; // Pointer to CpuIndex->ApicId map for >> + // pending hot-ejects > > (13a) "CpuIndex" is yet another name here; if you mean > "ProcessorNum[ber]" -- see point (10) above --, then please use that > word. I did. Will fix. > > (13b) Also, the "->" arrow is a bit confusing (is "CpuIndex" a > pointer???), so please either use " -> " (spaces on both sides) or write > "ProcessorNumber-to-ApicId". > > >> + CPU_HOT_EJECT_FN Handler; // Handler to do the CPU ejection >> + >> + UINT64 Reserved; > > (14) Please drop the "Reserved" field as well, with the justification > given in (12). > > >> +} CPU_HOT_EJECT_DATA; >> + >> +#endif /* _CPU_HOT_EJECT_DATA_H_ */ >> > > (15) Comment style is wrong; should be //. > > (I admit that you may find many examples for the wrong comment style, > near such "#endif" directives, under OvmfPkg/Include; sorry about that.) > > > (16) Please drop the leading underscore here too. > Will fix and acking all the other comments that I did not explicitly address as well. Thanks Ankur > > I plan to continue the review either today, or sometime later this week. > > Thanks! > Laszlo >