From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from aserp2130.oracle.com (aserp2130.oracle.com [141.146.126.79]) by mx.groups.io with SMTP id smtpd.web10.6891.1614065847348540415 for ; Mon, 22 Feb 2021 23:37:27 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@oracle.com header.s=corp-2020-01-29 header.b=oDwRqlvx; spf=pass (domain: oracle.com, ip: 141.146.126.79, mailfrom: ankur.a.arora@oracle.com) Received: from pps.filterd (aserp2130.oracle.com [127.0.0.1]) by aserp2130.oracle.com (8.16.0.42/8.16.0.42) with SMTP id 11N7bFs3104482; Tue, 23 Feb 2021 07:37:23 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=fSu+Vf800NLoSM4/f74fd4NNWWyYBPIp2CBlai9lsXY=; b=oDwRqlvxcgNB1ypduuAc3OhFSX4BMVfZyMOmVagWYvjoad9ooTTPJ6MfFEF5BFoA08lx yPEQ3ZDYDdVRFXMArCQkyF+/vtUsoH3eQoFGisSdgQ8xIP3fDbXnn4iDZHFIkS7Swr6+ OiroGFKIiJaMnFwHe1iTpmiJvpufZyIIIk3FW1XZ/GNgrD7uuU4wANmAjReeS8CGaONJ QTemWPABzi4mrGKu2meO1Ai0x+XkZ54WyQsJkTnnLb1f2uCj51fJNwdU/7uoyYW1CGAu Ece3uBN4hs/SVq24um5FA9+gxZSNZsC6t7MtAMH+qOuS+/ZoZzyTP8CLX0fAgowuM3ww KQ== Received: from userp3020.oracle.com (userp3020.oracle.com [156.151.31.79]) by aserp2130.oracle.com with ESMTP id 36vr620jdr-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Tue, 23 Feb 2021 07:37:22 +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 11N7a6Md041380; Tue, 23 Feb 2021 07:37:21 GMT Received: from nam12-dm6-obe.outbound.protection.outlook.com (mail-dm6nam12lp2171.outbound.protection.outlook.com [104.47.59.171]) by userp3020.oracle.com with ESMTP id 36uc6rbutf-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Tue, 23 Feb 2021 07:37:21 +0000 ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=iKXIrBtGWD6lXA+Hu4b65PnI/+QPT4sqW76UE0u03enuu3yIO+zj8UJcsEsMum+4YQdC3n8y7N6abi+AuBXFHXYXkA/gMVX37Hs+NdvsK332D5l+Z5dmah9DAWCFLun2X0ywRe3G7QY0OlZK79G4vvXRHRv0cO4s2Eq2kMd/ZBAFkYxrCi257IN4WSzDee6GzBf+ycXCPunWpDHafZxeVOrtSR44tAWRSx5OJ86ubd/XSqiVzxxfaCo5B/9lZlSv1ZsJTiTqFVuF+wnbsUVHlnpKTmrT6XVd32ZLsB1rYQdR2xWjNu6PHPBOFvLX4dMxxTBQSFO/unQ4+wtchY6Pnw== 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=fSu+Vf800NLoSM4/f74fd4NNWWyYBPIp2CBlai9lsXY=; b=GlPQIWQCHTB4uQq2Qe2Gu56wJn3b7K0aGQuRmID0ODb0lsdGBK1SJwVUjWIBvqWzMTtc1aGzIWQbkuQCbYZQU6swr9YsM1IRQ3IfOcMFBR6Hc1+Au69hehPJwLnKl7zwHRfXrhLjziRTs5oEHgZHU7pM07KD267odvCSSAWF3WL9XnYMWgzHtFu87HhBhGzmtE1X79cOjhFJRCI/VLOwQWirYJh5AONDYhy+3e6PSRVLm1yMdcyrF9f0tavm2Socx/gd4PMkhuS1tj0XOQhQNlX2zko+F6clUECXvxFZ2g7gRPPJXcL9hSGSgx9wuzW9NHFcw6o54GUaa3J35jpyqA== 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=fSu+Vf800NLoSM4/f74fd4NNWWyYBPIp2CBlai9lsXY=; b=xRM4gJiiFbSO3Z72VuzsJAU1RbsdAxJozlos7/RWa9RuwfH8weKiay82ZfAZ3uLFjBNVd6USfJYLGXC1XYO3Z5euY8aFIlBnVFRaQHApwgxaGLPW9JD+BAcKtrhzVsrpAf6qNSIRpvhGfkdfOHsyntdG3SPgliulJVUGkRK7VNo= Received: from SJ0PR10MB4605.namprd10.prod.outlook.com (2603:10b6:a03:2d9::24) by BY5PR10MB4259.namprd10.prod.outlook.com (2603:10b6:a03:212::21) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.3868.33; Tue, 23 Feb 2021 07:37:19 +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 07:37:19 +0000 Subject: Re: [edk2-devel] [PATCH v8 06/10] 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: <20210222071928.1401820-1-ankur.a.arora@oracle.com> <20210222071928.1401820-7-ankur.a.arora@oracle.com> <7fcf7a88-9d43-c546-409f-65d2419df7b3@redhat.com> From: "Ankur Arora" Message-ID: Date: Mon, 22 Feb 2021 23:37:17 -0800 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.6.0 In-Reply-To: <7fcf7a88-9d43-c546-409f-65d2419df7b3@redhat.com> X-Originating-IP: [70.36.60.91] X-ClientProxiedBy: MWHPR11CA0037.namprd11.prod.outlook.com (2603:10b6:300:115::23) 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 MWHPR11CA0037.namprd11.prod.outlook.com (2603:10b6:300:115::23) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.3868.27 via Frontend Transport; Tue, 23 Feb 2021 07:37:18 +0000 X-MS-PublicTrafficType: Email X-MS-Office365-Filtering-Correlation-Id: 51fab99f-e4b2-4224-bc11-08d8d7cdd9a8 X-MS-TrafficTypeDiagnostic: BY5PR10MB4259: X-MS-Exchange-Transport-Forked: True X-Microsoft-Antispam-PRVS: X-MS-Oob-TLC-OOBClassifiers: OLM:4502; X-MS-Exchange-SenderADCheck: 1 X-Microsoft-Antispam: BCL:0; X-Microsoft-Antispam-Message-Info: PsbDumUgxqpxxoqc67em7PbMdNUWYlqXlR/EY86DJW4Uk9A8AjpBUX6XBC57Pe0sXzOZaHh7J9eCMD/nuOSkmumtVUbpMWfb3hyM7RgmLjZ1YAmJ2JTkRhQeDZFy8y78O/8YnQ4C8w5k4DJ0l7AdJ+2zH5luSpqVppVfZF7uadSTyjLvM2pWzBaNV1hElZOm1k0bzFpaYkYN3XiXHN93BezZeM2RoP16/MQyBPv6+nkuHJl+dAlb98NFyb8gBAq9oOoV5v4Re2ga4FTx0emhG/EekmcIpPatr2pva4PaVxSepd+r3nNxxCphIUvkNfS7XJg9m9oD3/CO/SKqGiqzfQrwQR4u/1ZJuToRhyGMB2OYFDUy+r4yT7adg/2Az2GokHLP8qnC1XwtjRuL/t3PYp+EOPOmporkhy0v4+07Bo980QFNYJ3vyXrJ2pFsTuu+mlBJyVK0az8BikxSD+q24moFeHeCGABlTC700slY0sZf4mUwm41qFuw03UR+jTWPW1DGty6CZ1tVxliMndBKvJ1QUSE88gdPIXZktQZEOY0XNQy6EUiwC0+ohEUEUZHjNo2LpqrHvhVb7AIEEGnS1+RoP/dUIcvgPKJrHF5gP4ZphUVcLngbbq2ctwFVIszhiSK/lZJJoU0chnWZni4c4b1S0DBruF/5GeY5JlyYLjVKMYn4MrS4qiN+sSboDIAx 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:(396003)(376002)(136003)(366004)(39860400002)(346002)(956004)(83380400001)(6486002)(316002)(16576012)(5660300002)(4326008)(107886003)(2616005)(36756003)(478600001)(86362001)(2906002)(31686004)(8676002)(186003)(8936002)(53546011)(66476007)(16526019)(66946007)(26005)(66556008)(54906003)(966005)(31696002)(45980500001)(43740500002);DIR:OUT;SFP:1101; X-MS-Exchange-AntiSpam-MessageData: =?utf-8?B?eEZhUVBMM0h4R20zd3RIbjJaRmgyUTNna2xaRDVidzVsWERsNDRqR044SHdB?= =?utf-8?B?K3lGV3FMRnF1V1lVSCtscEdhRUMzUEF5MDF6VGFnVjNZd3NaZ0FMS0Y1WURJ?= =?utf-8?B?Yy9IWHd5eWgxQ21hSVFoS3BCcVRYVWNSa0lFMk5lM2MvRlA1dks2K1VZSXUy?= =?utf-8?B?Q2lFTjdtdVNkMGtISndEaW5zWmhmSjJlLzlVQTNpVVpaU2pFZzBlaXRvMnJp?= =?utf-8?B?cHBsN0R3OFloQzYzcms4UzhhQjAvcVFDOG8xQmswcWlwQVhyZ3k2dkVDR3hI?= =?utf-8?B?cG5Qek44T3B3WElHVHhCQ3J1L1pxalp6ekFob0hJQVJZRHRiblZVU3NmbnNB?= =?utf-8?B?WmVuY1BkSUgwRHNQaEFnVGVmbzE2YU1ORUFXTnZlajdRbDk3N3ZVMFdvU3pu?= =?utf-8?B?MDB6WFQyMTcwTmxhd016U0FJUXRJU3o0U1JYc3hyOW4yemlleVQwMmhHcDlz?= =?utf-8?B?eTFOMDVwSDFhRGIrQU91VjlidFk2MCtnYjhnTldMSWNnK0QyYkFKZmluRFZx?= =?utf-8?B?ck5jczhOR3ZsTUV2akhwZXFzQnQrUlFwNmNtbUZCa3Z4eVVubmZlZEpWU01k?= =?utf-8?B?N1pXMFltWXBzNm5DT3owMDlpcFBTSmhmZUxsYXh3MUNQNlRiVzVrN3Q4M3dM?= =?utf-8?B?c0s3ODBLWHNKK2RWZjZDVkpBdzRoMjkyRysrRHVVb2NZZlNkQTRPTHhtTWtt?= =?utf-8?B?cW1TeEZKSnBERHNiZTkySXNSaytaOEhnYTY4R05GUGo4eHRNQ25HODhOd0RR?= =?utf-8?B?RkE3NzBxNkdBSlpLNUVaem1Bblo5a3RlWldLeVJTMWxacG44c2pCSWt6Z29S?= =?utf-8?B?YmduSnJjb1JWay9UbGF0T0ZVc1ZTUERNeGw3MUZpWEV6VWlHeVVpUFNNL21p?= =?utf-8?B?aVFuZ2FPVXU3Rjh0NXBpb25rMGNCWnZQVVd2QzFOYTVWZnJjemljR2ZxRHJB?= =?utf-8?B?enBpbERrTlVEcElLdm8vNjZoTmg5cS9FMzNDLzBjRU9qRjlnUDhuOHZIdXYz?= =?utf-8?B?b1RWZmNRL0FaUXhTUUg3a29ZNGFLODBLYmVwM1dhMVJBMU5vT2xyMWdkRXZD?= =?utf-8?B?aGVKMUYyc0Z2RTBFM3JSR2RQYml2K2xWd1pnSC96SDljTnBmVGVaYXFOSmJs?= =?utf-8?B?czdFMlVjVUZoSXRjSjZ0a3Z0NnMyK2dOU3hJWnd6UUhjVG1GQ0FlOWdHOFp2?= =?utf-8?B?WVJwMFFhbWZSWmdwSEQ0TDI3VHlGYzJSbmhXblc1KzVNWC9qRTdhdEJjWEhv?= =?utf-8?B?NUR4Nyt0UjlBKzdpREJ5bm55M2w3VUN6amU1anpSRWhXcmtBS1B2ZU91Tkh3?= =?utf-8?B?ZStaQ1VHZko5L0VuTmk4d0RZZ2t5T2JIeVpMVVpheE4rRzRPdG1iNkk5TWta?= =?utf-8?B?SkNlaGlEVmlBQnFnYTNoeFArcVpOMnBhYmlDcmQ1cnZuaHIrd3duY1BGcjZs?= =?utf-8?B?RTJrRk9CeFdnekpIeU5rN05mc2lQNER6L2s3a0hkeFczQWpHbkgvRklBQnk0?= =?utf-8?B?WEV5cnA4clgvZDNQaUhjeVdJbjVjZmxUQkRub253SHlDZWxzYm5DVVhhWFZ2?= =?utf-8?B?eHFVK0tET3NQWExrOGhiNW5DM1dqSVdVM2Z6QnljZjRNL1cxQTdkWHpuRFg4?= =?utf-8?B?MzhIZkFLVXZNSzV4SWpMODFBeXExVVUvUWFzQ2hMbnBYVFV3TWQzK0QvaVpV?= =?utf-8?B?YnNUSFZyTnlEcTR2T0ZmZVZvV25uQVpTQ2RjckhQbDBsRW9BV1RwdG0zSGNW?= =?utf-8?Q?jNfORqIL5f5zrtFjR2z2u3kaismbUgzbX23enlf?= X-OriginatorOrg: oracle.com X-MS-Exchange-CrossTenant-Network-Message-Id: 51fab99f-e4b2-4224-bc11-08d8d7cdd9a8 X-MS-Exchange-CrossTenant-AuthSource: SJ0PR10MB4605.namprd10.prod.outlook.com X-MS-Exchange-CrossTenant-AuthAs: Internal X-MS-Exchange-CrossTenant-OriginalArrivalTime: 23 Feb 2021 07:37:19.4798 (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: 0ZkmI8Mr1zob8cFlYdlxzT32Yo162yBjNR4Skzk8ZgphZKk/wmPLmA1MqwIrhfzEt3UHHyerLMG4ABlaM+ACV32lDs8sPS4OGbnAaQWAPi4= X-MS-Exchange-Transport-CrossTenantHeadersStamped: BY5PR10MB4259 X-Proofpoint-Virus-Version: vendor=nai engine=6200 definitions=9903 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-2102230063 X-Proofpoint-Virus-Version: vendor=nai engine=6200 definitions=9903 signatures=668683 X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 suspectscore=0 bulkscore=0 clxscore=1015 mlxlogscore=999 lowpriorityscore=0 phishscore=0 impostorscore=0 adultscore=0 mlxscore=0 priorityscore=1501 malwarescore=0 spamscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2009150000 definitions=main-2102230063 Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit On 2021-02-22 6:19 a.m., Laszlo Ersek wrote: > On 02/22/21 08:19, Ankur Arora wrote: >> Init CPU_HOT_EJECT_DATA, which will be used to share CPU ejection >> state between SmmCpuFeaturesLib (via PiSmmCpuDxeSmm) and CpuHotPlugSmm. >> >> The init happens via SmmCpuFeaturesSmmRelocationComplete(), and so it >> will run as part of the PiSmmCpuDxeSmm entry point function, >> PiCpuSmmEntry(). Once inited, CPU_HOT_EJECT_DATA is exposed via >> PcdCpuHotEjectDataAddress. >> >> The CPU hot-eject handler (CPU_HOT_EJECT_DATA->Handler) is setup when >> there is an ejection request via CpuHotplugSmm. >> >> 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 review comments: >> (1) Detail in commit message about context in which CPU_HOT_EJECT_DATA >> is inited. >> (2) Add in sorted order MemoryAllocationLib in LibraryClasses >> (3) Sort added includes in SmmCpuFeaturesLib.c >> (4a-4b) Fixup linkage directives for mCpuHotEjectData. >> (5) s/CpuHotEjectData/mCpuHotEjectData/ >> (6,10a,10b) Remove dependence on PcdCpuHotPlugSupport >> (7) Make the tense structure consistent in block comment for >> InitCpuHotEject(). >> (8) s/SmmCpuFeaturesSmmInitHotEject/InitCpuHotEject/ >> (9) s/mMaxNumberOfCpus/MaxNumberOfCpus/ >> (11) Remove a bunch of obvious comments. >> (14a,14b,14c) Use SafeUint functions and rework the allocation logic >> so we can just use a single allocation. >> (12) Remove the AllocatePool() cast. >> (13) Use a CpuDeadLoop() in case of failure; albeit via a goto, not >> inline. >> (15) Initialize the mCpuHotEjectData->QemuSelectorMap locally. >> (16) Fix indentation in PcdSet64S. >> (17) Change the cast in PcdSet64S() to UINTN. >> (18) Use RETURN_STATUS instead of EFI_STATUS. >> (19,20) Move the Handler logic in SmmCpuFeaturesRendezvousExit() into >> into a separate patch. >> >> .../SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf | 4 + >> .../Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c | 92 ++++++++++++++++++++++ >> 2 files changed, 96 insertions(+) > > Nice, I've only superficial comments: > >> >> diff --git a/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf b/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf >> index 97a10afb6e27..8a426a4c10fb 100644 >> --- a/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf >> +++ b/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf >> @@ -30,9 +30,13 @@ [LibraryClasses] >> BaseMemoryLib >> DebugLib >> MemEncryptSevLib >> + MemoryAllocationLib >> PcdLib >> + SafeIntLib >> SmmServicesTableLib >> UefiBootServicesTableLib >> >> [Pcd] >> + gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber >> + gUefiOvmfPkgTokenSpaceGuid.PcdCpuHotEjectDataAddress >> gUefiOvmfPkgTokenSpaceGuid.PcdQ35SmramAtDefaultSmbase >> diff --git a/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c b/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c >> index 7ef7ed98342e..adbfc90ad46e 100644 >> --- a/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c >> +++ b/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c >> @@ -11,10 +11,13 @@ >> #include >> #include >> #include >> +#include >> #include >> +#include >> #include >> #include >> #include >> +#include >> #include >> #include >> #include >> @@ -171,6 +174,92 @@ SmmCpuFeaturesHookReturnFromSmm ( >> return OriginalInstructionPointer; >> } >> >> +STATIC CPU_HOT_EJECT_DATA *mCpuHotEjectData = NULL; >> + >> +/** >> + Initialize mCpuHotEjectData if PcdCpuMaxLogicalProcessorNumber > 1. >> + >> + Also setup the corresponding PcdCpuHotEjectDataAddress. >> +**/ >> +STATIC >> +VOID >> +InitCpuHotEjectData ( >> + VOID >> + ) >> +{ >> + UINTN ArrayLen; >> + UINTN BaseLen; >> + UINTN TotalLen; >> + UINT32 Idx; >> + UINT32 MaxNumberOfCpus; >> + RETURN_STATUS PcdStatus; >> + >> + MaxNumberOfCpus = PcdGet32 (PcdCpuMaxLogicalProcessorNumber); >> + > > (1) This doesn't deserve an update in itself, but since I'll ask for > some more below, I might as well name it: please remove the empty line > here. > >> + if (MaxNumberOfCpus == 1) { >> + return; >> + } >> + >> + // >> + // We want the following lay out for CPU_HOT_EJECT_DATA: >> + // UINTN alignment: CPU_HOT_EJECT_DATA >> + // --- padding if needed --- >> + // UINT64 alignment: CPU_HOT_EJECT_DATA->QemuSelectorMap[] >> + // >> + // Accordingly, we allocate: >> + // sizeof(*mCpuHotEjectData) + (MaxNumberOfCpus * >> + // sizeof(mCpuHotEjectData->QemuSelectorMap[0])). >> + // Add sizeof(UINT64) to use as padding if needed. >> + // >> + >> + if (RETURN_ERROR (SafeUintnMult (sizeof (*mCpuHotEjectData), 1, &BaseLen)) || > > (2) This "multiplication" is somewhat awkward. > > First, BaseLen is a UINTN, which always matches the return type of the > sizeof operator, so a direct assignment would be OK. > > But even that would be overkill -- I suggest removing the BaseLen > variable, and just using "sizeof (*mCpuHotEjectData)" in its place. > > >> + RETURN_ERROR (SafeUintnMult ( >> + sizeof (mCpuHotEjectData->QemuSelectorMap[0]), >> + MaxNumberOfCpus, &ArrayLen)) || >> + RETURN_ERROR (SafeUintnAdd (BaseLen, ArrayLen, &TotalLen))|| > > (3) Missing space character before "||" > > >> + RETURN_ERROR (SafeUintnAdd (TotalLen, sizeof (UINT64), &TotalLen))) { > > (4) So, I find the mixture of > > sizeof (UINT64) > > and > > sizeof (mCpuHotEjectData->QemuSelectorMap[0]) > > confusing. > > (And, this remark applies to both the code and the (otherwise very > helpful!) comment above it.) > > We use sizeof (UINT64) *because* that's the type of > "mCpuHotEjectData->QemuSelectorMap[0]" -- we want to ensure natural > alignment for the atomic accesses performed later. > > So, given both kinds of sizeof expression stand for the same concept, we > should use either "sizeof (UINT64)" exclusively, or "sizeof > (mCpuHotEjectData->QemuSelectorMap[0])", exclusively. > > I would vote "sizeof (UINT64)" here, because that could help us > eliminate some of the unpleasant line breaks. > > > (5) Not sure if I should obsess about this, but... an alignment to > UINT64 may require up to (sizeof (UINT64) - 1) padding bytes. (sizeof > (UINT64)) bytes are never needed, as that would imply the allocation is > already correctly aligned -- but then we'd need 0 additional bytes. > > > (6) So how about this formulation instead: > > if (RETURN_ERROR (SafeUintnMult (MaxNumberOfCpus, sizeof (UINT64), &Size)) || > RETURN_ERROR (SafeUintnAdd (Size, sizeof (*mCpuHotEjectData), &Size)) || > RETURN_ERROR (SafeUintnAdd (Size, sizeof (UINT64) - 1, &Size)) { > ... > } > > The longest line (the first line) is 79 characters wide, and we only use > one variable (called "Size"). > > If you don't like this variant, that's OK; but please at least unify the > sizeof expressions (4). I like this forumlation -- though I will get rid of the UINT64 -- I think as you pointed out above the mixture is a little awkward. Also acking comments above. > > >> + DEBUG ((DEBUG_ERROR, "%a: invalid CPU_HOT_EJECT_DATA\n", __FUNCTION__)); >> + goto Fatal; >> + } >> + >> + mCpuHotEjectData = AllocatePool (TotalLen); >> + if (mCpuHotEjectData == NULL) { >> + ASSERT (mCpuHotEjectData != NULL); >> + goto Fatal; >> + } >> + >> + mCpuHotEjectData->Handler = NULL; >> + mCpuHotEjectData->ArrayLength = MaxNumberOfCpus; >> + >> + mCpuHotEjectData->QemuSelectorMap = (void *)mCpuHotEjectData + >> + sizeof (*mCpuHotEjectData); >> + mCpuHotEjectData->QemuSelectorMap = >> + (void *)ALIGN_VALUE ((UINTN)mCpuHotEjectData->QemuSelectorMap, >> + sizeof (UINT64)); > > (7) More idiomatic formulation, for setting the "QemuSelectorMap" field: > > mCpuHotEjectData->QemuSelectorMap = ALIGN_POINTER (mCpuHotEjectData + 1, > sizeof (UINT64)); Thanks. Yeah this is much better. > >> + // >> + // We use mCpuHotEjectData->QemuSelectorMap to map >> + // ProcessorNum -> QemuSelector. Initialize to invalid values. >> + // >> + for (Idx = 0; Idx < mCpuHotEjectData->ArrayLength; Idx++) { >> + mCpuHotEjectData->QemuSelectorMap[Idx] = CPU_EJECT_QEMU_SELECTOR_INVALID; >> + } >> + >> + // >> + // Expose address of CPU Hot eject Data structure >> + // >> + PcdStatus = PcdSet64S (PcdCpuHotEjectDataAddress, >> + (UINTN)(VOID *)mCpuHotEjectData); >> + if (RETURN_ERROR (PcdStatus)) { >> + ASSERT_EFI_ERROR (PcdStatus); > > (8) To be fully clean semantically, this should be > ASSERT_RETURN_ERROR(). Yeah, that makes sense. > ... Sorry about the bike-shedding; the purpose is two-fold: > > - the new code should feel idiomatic, > > - I hope you're going to stick around for further edk2 / OVMF > development, and then these style hints shouldn't be useless in the long > run. Thanks. The project and working with you has decided been a mind expanding experience. Hope to contribute in the future. Ankur > > Thanks! > Laszlo > > >> + goto Fatal; >> + } >> + >> + return; >> + >> +Fatal: >> + CpuDeadLoop (); >> +} >> + >> /** >> 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 +277,9 @@ SmmCpuFeaturesSmmRelocationComplete ( >> UINTN MapPagesBase; >> UINTN MapPagesCount; >> >> + >> + InitCpuHotEjectData (); >> + >> if (!MemEncryptSevIsEnabled ()) { >> return; >> } >> >