From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail02.groups.io (mail02.groups.io [66.175.222.108]) by spool.mail.gandi.net (Postfix) with ESMTPS id 489A594105A for ; Wed, 6 Dec 2023 03:18:53 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=L+O5vyyjg5+1Vkm+fJcB4Pv1jBy+dz/3Kug15EhyK44=; c=relaxed/simple; d=groups.io; h=ARC-Seal:ARC-Message-Signature:ARC-Authentication-Results:From:To:CC:Subject:Thread-Topic:Thread-Index:Date:Message-ID:References:In-Reply-To:Accept-Language:MIME-Version:Precedence:List-Subscribe:List-Help:Sender:List-Id:Mailing-List:Delivered-To:Reply-To:List-Unsubscribe-Post:List-Unsubscribe:Content-Language:Content-Type:Content-Transfer-Encoding; s=20140610; t=1701832732; v=1; b=wRQuPWCNjiukfuFNBpUMVF7091+NxX/0SshDNFMHXD1pWBu+LIAv6bvcH6HyddQMylr+pKGy diXXh18g64mPH8Un913Fo6cd+3V1XZcWvO01X/sSgD0LF92rBDwdIJUz9roW8Ls9DVMmja+2vPx 72rJ6w6yHUNzMjl4bCpKgxLY= X-Received: by 127.0.0.2 with SMTP id XxLXYY7687511x1wc11pJ88v; Tue, 05 Dec 2023 19:18:52 -0800 X-Received: from mgamail.intel.com (mgamail.intel.com [192.55.52.151]) by mx.groups.io with SMTP id smtpd.web10.23381.1701832728537667984 for ; Tue, 05 Dec 2023 19:18:51 -0800 X-IronPort-AV: E=McAfee;i="6600,9927,10915"; a="374181831" X-IronPort-AV: E=Sophos;i="6.04,254,1695711600"; d="scan'208";a="374181831" X-Received: from fmsmga007.fm.intel.com ([10.253.24.52]) by fmsmga107.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 05 Dec 2023 19:18:43 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=McAfee;i="6600,9927,10915"; a="774846917" X-IronPort-AV: E=Sophos;i="6.04,254,1695711600"; d="scan'208";a="774846917" X-Received: from fmsmsx602.amr.corp.intel.com ([10.18.126.82]) by fmsmga007.fm.intel.com with ESMTP/TLS/AES256-GCM-SHA384; 05 Dec 2023 19:18:43 -0800 X-Received: from fmsmsx611.amr.corp.intel.com (10.18.126.91) by fmsmsx602.amr.corp.intel.com (10.18.126.82) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2507.35; Tue, 5 Dec 2023 19:18:43 -0800 X-Received: from fmsmsx610.amr.corp.intel.com (10.18.126.90) by fmsmsx611.amr.corp.intel.com (10.18.126.91) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2507.35; Tue, 5 Dec 2023 19:18:42 -0800 X-Received: from FMSEDG603.ED.cps.intel.com (10.1.192.133) by fmsmsx610.amr.corp.intel.com (10.18.126.90) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2507.35 via Frontend Transport; Tue, 5 Dec 2023 19:18:42 -0800 X-Received: from NAM10-MW2-obe.outbound.protection.outlook.com (104.47.55.100) by edgegateway.intel.com (192.55.55.68) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.1.2507.34; Tue, 5 Dec 2023 19:18:42 -0800 ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=W4qSkLW3Yk5hm5Sm2H2TdZyiaLR7IRLKu+m4Uxh8bQzvQIR/jnyaUlmAPDoE8jCXTZKbS1H5Ix2aokZozv7KhlertLWtDzrOdRndqKizoNceV3J15nBGfORUcb8wItbaVQsf3ayOA+lCU/88kzM/8tNEy6no3T1d3JbeQ7Jh4u9t8m8Fws2EZqSE0XvcDjkGHK5dBU2J8HhcIOIMMUI7hKrpjDTF4MiodcK68yEwsRl81ou9njPSXRVrwaSoE1dVTdKaNEfhb6O8IT5vBB7pPZ56R4M5zlbloaNn5+Y8xSkVc91feIeIiGsb2gtQYzOSeltpNlBqIh9AXbOjwd0TZA== 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-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=KQuPo06zdhvfpQf9mrfgvC+jNmCZKUWnzWv5fr2A94Q=; b=dA9rVkvaKQxGFfxP8GU33/U686LvUbHTJw0CrtNIHHVb+j27ZjOGnWzCw7qggV/iGavhtDhOdw12z47IlUacC6kphB+Rv/+XbraI8zv+iaUeVFgT3DqCE7GPEN6coryLn+kLQKmpj6nlmWvdLXlTcsoxLv+du2oYjfExV8Rzo15KiBB2XyoX7Y3afd7s1Oiut2szCsmOdZRsRnEjyRDcqRccR6XBgx06AYq2dOPDIa8zYMQGTs/0amb0Gq5WPsl4naKvgIu1N+19pXAdflrGxc0YCWJnSfsd0WgNYrSMFjXhhm/gaKZJ1uso9BKrP5+Wai+ukVevHb5mb/2LFGGgLg== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=intel.com; dmarc=pass action=none header.from=intel.com; dkim=pass header.d=intel.com; arc=none X-Received: from MN6PR11MB8244.namprd11.prod.outlook.com (2603:10b6:208:470::14) by MN0PR11MB6086.namprd11.prod.outlook.com (2603:10b6:208:3ce::11) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.7046.34; Wed, 6 Dec 2023 03:18:34 +0000 X-Received: from MN6PR11MB8244.namprd11.prod.outlook.com ([fe80::b614:1f5e:8b0c:9858]) by MN6PR11MB8244.namprd11.prod.outlook.com ([fe80::b614:1f5e:8b0c:9858%4]) with mapi id 15.20.7046.034; Wed, 6 Dec 2023 03:18:34 +0000 From: "Ni, Ray" To: "Wu, Jiaxin" , "devel@edk2.groups.io" CC: Laszlo Ersek , "Dong, Eric" , "Zeng, Star" , Gerd Hoffmann , "Kumar, Rahul R" Subject: Re: [edk2-devel] [PATCH v2 3/6] UefiCpuPkg: Implements SmmCpuSyncLib library instance Thread-Topic: [PATCH v2 3/6] UefiCpuPkg: Implements SmmCpuSyncLib library instance Thread-Index: AQHaI1bpAeix15aLGkydaIqc1mIFA7CUO1MA Date: Wed, 6 Dec 2023 03:18:33 +0000 Message-ID: References: <20231130063139.7472-1-jiaxin.wu@intel.com> <20231130063139.7472-4-jiaxin.wu@intel.com> In-Reply-To: <20231130063139.7472-4-jiaxin.wu@intel.com> Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-ms-publictraffictype: Email x-ms-traffictypediagnostic: MN6PR11MB8244:EE_|MN0PR11MB6086:EE_ x-ms-office365-filtering-correlation-id: f9ab0f6b-9d02-4ebd-1305-08dbf60a0794 x-ms-exchange-senderadcheck: 1 x-ms-exchange-antispam-relay: 0 x-microsoft-antispam-message-info: Vs54M3LNuEEPwapG09OCDZKCV/eiSAZLP6e2GZ+xAOEoNKREFN1f8b6DkisDMq8W2kMzrisV5mrinZXuWmYhfue0kIIfwmwPVSFQT5NwLyag9QxHQOV2to0oflffiBR11A4ZiRK5UtiLhLeR5faUqpHwOMODyQ6LrgPSxK0g/UYRStwpebzrRdQ/sgylV1bhRXNc2ValOFrDqE2kH23A3NYKnSmUqY9JVDhVbv8OA+HAOAfqJaH4fR8Ta/2N1EyX97nn3c7cHFh1aBs1r/yh17EGHnb3xnfvj05qK+fxOzLlFCRWRFz6YJ3W4fZPlD8qNCg5yEDOpmTtYxCFmmpshti125/trngb1ZXsUZGMNWnamGvD8YLqgrVMRLNrjJdJm0m2FSlvACDNgRqOKDZBI7nyEWL4RgFxcvMRaisw5pwDuy/XLf75ujFA0K/14A7oNyQwnsJmwuaNFPtGTV5FNRPJg4QqPyuqY16NGEpmosaIhyEvXZjo0thohPFvwjLMJCruqwDpBroLryZoM8Wf1CpuCk0HaPgHaT91FG2b4MOirVtw/8uPCL+luo5sMOWj1GtdLR5NBVYDssTSLR0pWwaDktVJX7sOmDTZEw+qj4Z8X3oPolRKqsPX/fainDMs x-ms-exchange-antispam-messagedata-chunkcount: 1 x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?ukQPPSsbAiRC/CpCTqaqbzvfQugnF95CWbw1tC4E/dGdz9eKj2DneqhSeGTZ?= =?us-ascii?Q?wwC/UZPgUpIMdbLtgBA5e4yHuIzyRjm2IcCMBFxATKoy44d4gmnEgA6AYmMk?= =?us-ascii?Q?2DWxHwvDdRRxfWJdMAaybUgkgGwdzVdE3lZAQup0PB6U7up3BD6+YvLLzGgm?= =?us-ascii?Q?w3vlK4y57eg8nm1P6/o77AIMXGqSG8N0ymhHLVVz3LBb06GilDLneozHC76V?= =?us-ascii?Q?YwXisi60x0aJLNTcOwYOtEX9AicP9caUgtrgyw13BlB6FAHZmcpGiGt6ain1?= =?us-ascii?Q?wcJgtg6I/KTtqcOsjjAKs4ydylAFWtG5gPw1XFo9sUxwJT43abrWpNDZDcB/?= =?us-ascii?Q?1GX1O3cfuKJVeUo7s3Rt9CdCNVXu6MV2+BsJf1gBaXLeYs7ELOmuqtd6NSFl?= =?us-ascii?Q?S4M8n+APbt8RF+VoC/qiPHWxzl7s+wjLoxr3qlhO4ydt9OTdoSxV/BvCS4Wl?= =?us-ascii?Q?JodS2gY8Glda7hDHsnbEBHdCjjffZ1JlZ5yVOOvxkLnsNhYWkogSsQJzD13u?= =?us-ascii?Q?xHXmPHPdCmVYTpkmplJdK0932yH0HvwbEWmOEW0pgDvsZAT8nenRCtkuLcPs?= =?us-ascii?Q?VYBprva2nmngXkhwrEWesauyXgcsFxBGzLoDd5bU89+yfRZEE3czcDXOpPfD?= =?us-ascii?Q?gRrKuQjU8wyZnpFs0edq8IjOuhl/cIPjczhnWIt79OxXlFLU82mUfcGtEpde?= =?us-ascii?Q?Pci6Prr5B03SG1DqKe+b7LUeAYb7lImrLzUPhli4lzdFuUiKsocnqzbxUUy2?= =?us-ascii?Q?FuFSLwxpr0kXnp0R/xUHClImYssXDrSlAEyoh1+TncL+ZTOtmBq+L8NUCMst?= =?us-ascii?Q?/3fjm38UvSTm2adSLe+UzamAHEH1pfRUsRcF5Cu8xDYvmKCbMebWcAcPPhoK?= =?us-ascii?Q?ZEJPxs1FJbqNjN8LiX708j29F5xj6Lloe4hlmfr2GFSgJGsEnq9hQy6sHHBB?= =?us-ascii?Q?uNdBkW3d8JqUOEHffnkti08Ex0wYKgDUBMZ9V6zN+PpgT3+cL73D+rGOJu2x?= =?us-ascii?Q?sI6lIdDEK1Ytv6vSkX3okH9oSxkn936+6oZmgXzgxvGSh1YEIMCkE1fxnC1N?= =?us-ascii?Q?Q5EhAi7cKG+N7wfrA57FM0YmsbaVRAg9Etlh1UJfZTZJsKvPbe55Je2+k6fS?= =?us-ascii?Q?IM4vvFPWGR9ghzqIKRCMZTGSJVlL7rKpJnr3B0S0yn7TxtDx2j4LbgQ4Ofk2?= =?us-ascii?Q?yhzpmQuxB/rnqGnNw3MXZpvgjp/Qv9QVK1jM8ncd3ZNv7ELUEqDMg/HWZYDO?= =?us-ascii?Q?2vJS+uR/XyOiGmubSSa/D/nzhl/TvgL0+MjuCnBGnF2THZNgrm6zJMNtZlKo?= =?us-ascii?Q?hwjYBiRwZlmc1u7nGByoMX31i15sScXFQSMLJcImtTJqGt4Ui3TL9cCjYx+o?= =?us-ascii?Q?bjAhCXhBj28qhuZ99/3YP5Hfod9wMTqd9cLwNwNs4otUAUJj+FdF4Q09AZC9?= =?us-ascii?Q?e0521frOpHDCFG7xUrZEbq1e2prJdQ4zFb/OFclFtwzH9WaIGOLlH2BK/jc+?= =?us-ascii?Q?e1HuoEM5AKCNeXgPP/TlqptLXsl50fK9dSjqhZR4FhzLiZXl882hb72y5EAX?= =?us-ascii?Q?6rDVlmKlO+x9UXDnr5k=3D?= MIME-Version: 1.0 X-MS-Exchange-CrossTenant-AuthAs: Internal X-MS-Exchange-CrossTenant-AuthSource: MN6PR11MB8244.namprd11.prod.outlook.com X-MS-Exchange-CrossTenant-Network-Message-Id: f9ab0f6b-9d02-4ebd-1305-08dbf60a0794 X-MS-Exchange-CrossTenant-originalarrivaltime: 06 Dec 2023 03:18:34.0099 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: 46c98d88-e344-4ed4-8496-4ed7712e255d X-MS-Exchange-CrossTenant-mailboxtype: HOSTED X-MS-Exchange-CrossTenant-userprincipalname: UWIssXgWhCv3f2pra4vHNtClQj5ysCiZQLZsYYx0kHgyqlU5ZUwJv4GPjJ/cvXroWcevN8huPk+JA67hlhtnUQ== X-MS-Exchange-Transport-CrossTenantHeadersStamped: MN0PR11MB6086 X-OriginatorOrg: intel.com Precedence: Bulk List-Subscribe: List-Help: Sender: devel@edk2.groups.io List-Id: Mailing-List: list devel@edk2.groups.io; contact devel+owner@edk2.groups.io Reply-To: devel@edk2.groups.io,ray.ni@intel.com List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: X-Gm-Message-State: QA3SNNYW90V3lNcGBnNx6gGFx7686176AA= Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20140610 header.b=wRQuPWCN; dmarc=fail reason="SPF not aligned (relaxed), DKIM not aligned (relaxed)" header.from=intel.com (policy=none); spf=pass (spool.mail.gandi.net: domain of bounce@groups.io designates 66.175.222.108 as permitted sender) smtp.mailfrom=bounce@groups.io > +typedef struct { > + /// > + /// Indicate how many CPU entered SMM. > + /// > + volatile UINT32 *Counter; > +} SMM_CPU_SYNC_SEMAPHORE_GLOBAL; > + > +typedef struct { > + /// > + /// Used for control each CPU continue run or wait for signal > + /// > + volatile UINT32 *Run; > +} SMM_CPU_SYNC_SEMAPHORE_CPU; > + > +struct SMM_CPU_SYNC_CTX { 1. How about "SMM_CPU_SYNC_CONTEXT"? > + /// > + /// All global semaphores' pointer in SMM CPU Sync > + /// > + SMM_CPU_SYNC_SEMAPHORE_GLOBAL *GlobalSem; 2. There is only one GlobalSem. Can you directly use "volatile UINT32 *Coun= ter" instead of "GlobalSem"? > + /// > + /// All semaphores for each processor in SMM CPU Sync > + /// > + SMM_CPU_SYNC_SEMAPHORE_CPU *CpuSem; 3. Can we use "volatile UINT32 **Run" instead of pointing to another struct= ure? Run points to an array where each element is a UINT32 *. Count of array equ= als to NumberOfCpus. > + /// > + /// The number of processors in the system. > + /// This does not indicate the number of processors that entered SMM. > + /// > + UINTN NumberOfCpus; > + /// > + /// Address of global and each CPU semaphores > + /// > + UINTN *SemBlock; 4. How about "SemBuffer"? > + /// > + /// Size of global and each CPU semaphores > + /// > + UINTN SemBlockPages; 5. How about "SemBufferSize"? > +}; > + > +EFIAPI > +SmmCpuSyncContextInit ( > + IN UINTN NumberOfCpus, > + OUT SMM_CPU_SYNC_CTX **SmmCpuSyncCtx > + ) > +{ > + RETURN_STATUS Status; > + > + UINTN CpuSemInCtxSize; > + UINTN CtxSize; > + > + UINTN OneSemSize; > + UINTN GlobalSemSize; > + UINTN OneCpuSemSize; > + UINTN CpuSemSize; > + UINTN TotalSemSize; > + > + UINTN SemAddr; > + UINTN CpuIndex; 6. Can you remove the empty lines among the local variable declarations? > + > + if (SmmCpuSyncCtx =3D=3D NULL) { > + return RETURN_INVALID_PARAMETER; > + } > + > + // > + // Count the CtxSize > + // > + Status =3D SafeUintnMult (NumberOfCpus, sizeof > (SMM_CPU_SYNC_SEMAPHORE_CPU), &CpuSemInCtxSize); 7. Is there a reason to use SafeUintxxx()? I don't believe the multiplicati= on could exceed MAX_UINTN. But using SafeUintxxx() makes code hard to read. > + // > + // Allocate CtxSize buffer for the *SmmCpuSyncCtx > + // > + *SmmCpuSyncCtx =3D NULL; 8. This is not needed. > + Status =3D SafeUintnMult (OneSemSize, sizeof > (SMM_CPU_SYNC_SEMAPHORE_GLOBAL) / sizeof (VOID *), &GlobalSemSize); > + if (EFI_ERROR (Status)) { > + goto ON_ERROR; 9. If you don't use SafeUintxxx(), you don't need "ON_ERROR" label and the = "goto". > +/** > + Performs an atomic operation to check in CPU. > + > + When SMI happens, all processors including BSP enter to SMM mode by > calling SmmCpuSyncCheckInCpu(). > + > + @param[in,out] SmmCpuSyncCtx Pointer to the SMM CPU Sync context > object. > + @param[in] CpuIndex Check in CPU index. > + > + @retval RETURN_SUCCESS Check in CPU (CpuIndex) successfully= . > + @retval RETURN_INVALID_PARAMETER SmmCpuSyncCtx is NULL. > + @retval RETURN_ABORTED Check in CPU failed due to > SmmCpuSyncLockDoor() has been called by one elected CPU. > + > +**/ > +RETURN_STATUS > +EFIAPI > +SmmCpuSyncCheckInCpu ( > + IN OUT SMM_CPU_SYNC_CTX *SmmCpuSyncCtx, > + IN UINTN CpuIndex > + ) > +{ > + SMM_CPU_SYNC_CTX *Ctx; > + > + if (SmmCpuSyncCtx =3D=3D NULL) { > + return RETURN_INVALID_PARAMETER; > + } 10. Can we use "ASSERT" instead of if-check? We can explicitly mention the = behavior in API comments. > + > + Ctx =3D (SMM_CPU_SYNC_CTX *)SmmCpuSyncCtx; 11. Why do we need the type cast? > + > +/** > + Performs an atomic operation lock door for CPU checkin or checkout. > + > + After this function: > + CPU can not check in via SmmCpuSyncCheckInCpu(). > + CPU can not check out via SmmCpuSyncCheckOutCpu(). > + CPU can not get number of arrived CPU in SMI via > SmmCpuSyncGetArrivedCpuCount(). The number of > + arrived CPU in SMI will be returned in CpuCount. 12. I agree that after locking, cpu cannot "checkin". But can cpu "checkout" or "getArrivedcount"? I need to double check. -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#112087): https://edk2.groups.io/g/devel/message/112087 Mute This Topic: https://groups.io/mt/102889293/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/19134562= 12/xyzzy [rebecca@openfw.io] -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-