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 DD4DE740032 for ; Wed, 6 Dec 2023 04:23:27 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=wE6/tue1gBT0sxXwaF4Ne+EUz8YLc7VYYY57KA/2tYc=; 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=1701836606; v=1; b=l6SaYAzt0+36j9dq2A+JdIi1WeB6nqhjG0wSPRgKCbCXSUue/LQjhjFe1+rT66UX5Y+Rodso Ta85fWvL9Q4mEZCvfbDUsxCowwOMKbq4p3QEFjQbg7i8VS3dTEwq+0Al70P321G0sQel/enjWBX znn01Zu92vl0j39pJp9c0OCk= X-Received: by 127.0.0.2 with SMTP id qKgAYY7687511xTvB5ZaHQQt; Tue, 05 Dec 2023 20:23:26 -0800 X-Received: from mgamail.intel.com (mgamail.intel.com [192.198.163.7]) by mx.groups.io with SMTP id smtpd.web11.24209.1701836605670768537 for ; Tue, 05 Dec 2023 20:23:25 -0800 X-IronPort-AV: E=McAfee;i="6600,9927,10915"; a="15553941" X-IronPort-AV: E=Sophos;i="6.04,254,1695711600"; d="scan'208";a="15553941" X-Received: from orviesa001.jf.intel.com ([10.64.159.141]) by fmvoesa101.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 05 Dec 2023 20:23:25 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="6.04,254,1695711600"; d="scan'208";a="19201754" X-Received: from orsmsx603.amr.corp.intel.com ([10.22.229.16]) by orviesa001.jf.intel.com with ESMTP/TLS/AES256-GCM-SHA384; 05 Dec 2023 20:23:25 -0800 X-Received: from orsmsx610.amr.corp.intel.com (10.22.229.23) by ORSMSX603.amr.corp.intel.com (10.22.229.16) 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 20:23:18 -0800 X-Received: from ORSEDG601.ED.cps.intel.com (10.7.248.6) by orsmsx610.amr.corp.intel.com (10.22.229.23) 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 20:23:18 -0800 X-Received: from NAM04-MW2-obe.outbound.protection.outlook.com (104.47.73.169) by edgegateway.intel.com (134.134.137.102) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.1.2507.35; Tue, 5 Dec 2023 20:23:18 -0800 ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=U2hiGKer6OlbET5afRRS2Jwbwy/JlS7Jb70nzOeGAiMbVbx0kj85xnfS7Po1SF88v2Jua2s65BxEhPvpudMlzeIVwkfq7QepbhL3PuYPjtYQbvfA2GIlwwgXHw6qh/VY0Gf3DH4KxxGPxlN8l1k66ZgcJ/ark3Rz8hyApj69egl93XAXj3L6QH1ojrqUbOGvjTIWXY+hZpo/WwbETczVA194t0IuBnACc5eC6ow2wrIdOGgVtLHk0KXWBnEklw6ywhqBovpHUrGV2/IKlCco+R+KvFxdCnb5X4Xz045zCuBkY7z3SR8SqDQ1Wd1fgZQoNqWjtBeBMqSxp8iIhgrsMQ== 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=4YCI7TYqcW4mLnznklyN0UL4wwfAcBRlMpd/6ySYEd8=; b=YK6yVXLvcURDnAtOnLkDAQEPBEn3WlMHTXyaUWsLINVmwBLzEayD08pQ4Q64LOiE/xjk0YCt4v53ktnNkqdeJjRC5wDWK+DZMYGmA8nqLPVvGaqgN1rk1jDbBAx/LkLwVn0reaP8lTU6HLj+/ebCQoaU3PwZMiUhKCvryo4xF34bhDH/5uzOuLJmPyDB22zzb6FWWQPBn7gVoft/LYIR+ntBtGeNnoZcW9Xa7UvtX+sr9DVLl/C40BqF1pzjSHEAPgYPa8gOTEx3XOC6lPXQZKd9G3ey+HvxT2X+aSDVO1+EWOU1AzEOOifz01Wa/IhYds6Szrs2tQGVdyj4PZ3/Gg== 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 MN0PR11MB6158.namprd11.prod.outlook.com (2603:10b6:208:3ca::18) by PH0PR11MB7660.namprd11.prod.outlook.com (2603:10b6:510:26f::5) 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 04:23:14 +0000 X-Received: from MN0PR11MB6158.namprd11.prod.outlook.com ([fe80::864d:d91a:4674:e0f7]) by MN0PR11MB6158.namprd11.prod.outlook.com ([fe80::864d:d91a:4674:e0f7%7]) with mapi id 15.20.7046.034; Wed, 6 Dec 2023 04:23:13 +0000 From: "Wu, Jiaxin" To: "Ni, Ray" , "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: AQHaI1bpAeix15aLGkydaIqc1mIFA7CUO1MAgAdpExA= Date: Wed, 6 Dec 2023 04:23:12 +0000 Message-ID: References: <20231130063139.7472-1-jiaxin.wu@intel.com> <20231130063139.7472-4-jiaxin.wu@intel.com> In-Reply-To: Accept-Language: zh-CN, en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-ms-publictraffictype: Email x-ms-traffictypediagnostic: MN0PR11MB6158:EE_|PH0PR11MB7660:EE_ x-ms-office365-filtering-correlation-id: a41b33c7-a816-4fd6-0446-08dbf6130f4f x-ms-exchange-senderadcheck: 1 x-ms-exchange-antispam-relay: 0 x-microsoft-antispam-message-info: FqCezjnHDGukmbSrbZyCvZKHyGKGa9hpN8P1q8T6GfzR9CxBbtFejYyQuf2C4bMwqtDRBrCjru089vckoSDhjpJv4eq0tS63t012QcEmwE7R7HD2Rrcc+ZuO0qylT6QMaHELYe3u0wdzyJ1hdTy2d8KDhy9jT92/7Gy05WdD47ACh9j89UwAxQfw4CqO2WvjgqOKihvWQA/NMK0GLFdumnLRMeqEKNm3u1YgO7f2bMZGdTztbs0PU6xxq+MCPlE/kcwUQNT+SZKjj1OKMiTEgGSkKko0RsroUlRD9YNdv3fjpfAVgbmkFYZqKwsKIL/oG0aBXK7nZnQy4RlSSHr32TqxLZUX38noCh6wrJxAQlIpcXqAb1I/i4/ePfMsWT2MFPlOwfwZg6gIV+we41pExr5Pn6TAm3YUebOc2sL1dko4KwvQW6JmlZGS/YhBAyK9Dlk3V+B6QffRtPXGNse44s7b3t9BqtOLicvKqUrFrGC/f8E20ZwP/0V7Me5wxCbkAdcRlAavan/49Gp/fHb3bXkNQWRGXZB6WM7r3KjzxJMxyObQ/XKrRG0x3xZxCkAN9qKhjXhsUgL96eprsX19/aNXgFzgsLZ//wTxmrrcOmJNOz7eBTbCBuF9AeIUSjQr x-ms-exchange-antispam-messagedata-chunkcount: 1 x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?aDn8hv2DNGV6qbbdghZkyZ98BPBxizP39qpVb8nNXZx9asW78qFzc+VP5nlC?= =?us-ascii?Q?R2VgwhK/O2B49mu0/DngQun80nvRwAKWie1sMqXjdjgJ9OpJT5F3v+Ls9Wm1?= =?us-ascii?Q?8ugu1HeD03viLZCPi3hgdouY6tf6eTDaftTTogQEuF+3HUkTOqmEwGnhehJL?= =?us-ascii?Q?1h4S06MO8IcSRpcnkn9D5aWK/LRiJmJIQIAwCDsRG7c8VFaDw+gVXDQd5bTz?= =?us-ascii?Q?NH1+YU7A8U1Zh0YaumQ4Ux6aeZkJAwfvlHT57HODHKc7ysfwEwVF5UAkcMCM?= =?us-ascii?Q?tO4hJ+Rebdk7U/hrbVniQgEVwJZnzL0/CkWheWreJhjGTItJGJa602vvitQJ?= =?us-ascii?Q?TX/FQtaoffoPnsHCpc+k/AW5wa+ss8sQVXwfYbxLrB2E0ozn41N5bZg/Ho5X?= =?us-ascii?Q?7ubn9XjW0jp8KSanRg51Bke4qZhE03zyGcvnPDQaUiJvP2zEHjpUohO8KYPT?= =?us-ascii?Q?wnSVBIxrsr2IdVI5Q5qwBHoWdP3d3kN1MuDn87QbIOkzPsGg3O3tBtL5mUsV?= =?us-ascii?Q?PHU3vr8z0olEIkvXzMBnS+VL0NzWlkC28QZhjLGCXzFziI8KpZIqVwGGbiH8?= =?us-ascii?Q?dvgToUDRIKve+TkWhcrs6wTaLWm9NYKqTMhjEWz8nch3M+PArZxipNo33Ww+?= =?us-ascii?Q?V/moIIpPyZOnQCdGi0i7MRxteFsF9vjHfFC3dK1XHmuUzjQzFcvxWCb4IyEb?= =?us-ascii?Q?/B2+7Zmt2jZP//WGglGQt+FQzzIGf+vnbB4n1iHd370tJRdF9YtKCtfVRixi?= =?us-ascii?Q?9KaXnGFX9CRW5ycpkJ8f7p+yUU7fTVvwrH8ucWX332z/sE3/48UF/WMni2vy?= =?us-ascii?Q?lrNKsN4gCkIUtKD6ZSgJjJHdlmTCSZW7WZ4uUGfjCMd/rVyealWlhg+OLdKI?= =?us-ascii?Q?CzQunqmBdNMQPpR7Kwcqzp2CvN1Y023WLCAITkGvgPXmV83YX2rs7Zb5O6I6?= =?us-ascii?Q?AUfrGB8I0cjHqqPSlZx3/sIjAX0urFx13KwtoMFPFXqcsad/LVHFxp5xucRI?= =?us-ascii?Q?nX9741eXYvdLsME+dUNFSvWRO8We8gVawCcazoUlsiq0AKXzBuZSb7RM4FVX?= =?us-ascii?Q?AJNXxl6I78Qfdt1aOAFcBxHWhCNP5a4eLSSRNtEUEZnX71LfYmV3qypiJGas?= =?us-ascii?Q?h0ZaJfNS2mKu+Fw478sBcPFeufwqH0J4ARvExZi28FwcUWeJGRTQgYPMOAaC?= =?us-ascii?Q?ZegfPQ3pqwCu/u3MO1Q45QO2G4+qywKgfBRcPJ3FUpBCB+dljRkeFzbI9eyw?= =?us-ascii?Q?KKJpipuNGcudkcdFQDNkwlP8G7bc1si01+0VOxX/G8RL/i7R8UQXxjdnYCel?= =?us-ascii?Q?1d9DOtouvLBcM83wUteDvNP1HQMWTyM5KrtMSXFhkCKN8GsyX4pH+/thopXp?= =?us-ascii?Q?ocjGhtn+dh4/oXh5eDIWiNQNeLuAzPFcXB7b9lwof5vs/C2Ic20dlEqZh+vm?= =?us-ascii?Q?+q7/HIbkITXY5blkS9hRoCZlJpsAqMtQPJ4KyLs87+Mr8XVQYkckgjzgwc9l?= =?us-ascii?Q?BDC3LkrnzCogLSyDVSe5Uz+VsWFHHwqcbZMR1xLzwxDBBRgp4tKkF4XAFV+3?= =?us-ascii?Q?IT+7oc1DkvoyX48SpwH1kdRCqVVGyQDdfsnY5LeA?= MIME-Version: 1.0 X-MS-Exchange-CrossTenant-AuthAs: Internal X-MS-Exchange-CrossTenant-AuthSource: MN0PR11MB6158.namprd11.prod.outlook.com X-MS-Exchange-CrossTenant-Network-Message-Id: a41b33c7-a816-4fd6-0446-08dbf6130f4f X-MS-Exchange-CrossTenant-originalarrivaltime: 06 Dec 2023 04:23:12.4252 (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: Prv9zvI8E9iBJkaJ7A0Uny/jDjCWBpybEUK20sUjsEtnx6TlTpCEK/j5I0qLA+M8qTdSR4dgYNwQ2uudbJi3ag== X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH0PR11MB7660 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,jiaxin.wu@intel.com List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: X-Gm-Message-State: kEtUlWGHUkvV9Dp7hRArHvJ3x7686176AA= 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=l6SaYAzt; 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; arc=reject ("signature check failed: fail, {[1] = sig:microsoft.com:reject}") > > +struct SMM_CPU_SYNC_CTX { >=20 > 1. How about "SMM_CPU_SYNC_CONTEXT"? Agree. >=20 > > + /// > > + /// All global semaphores' pointer in SMM CPU Sync > > + /// > > + SMM_CPU_SYNC_SEMAPHORE_GLOBAL *GlobalSem; >=20 > 2. There is only one GlobalSem. Can you directly use "volatile UINT32 > *Counter" instead of "GlobalSem"? I think it's not the big deal to combine into one structure or separate int= o different structures. Separating different attribute field into different structures will benefit= the code readability & maintainability & expansibility. It's easy to under= stand which field is for GlobalSem, and which field is for CpuSem. With separated structures, it will very easy coding the later logic for sem= aphore memory allocation/free. Because you know we need 64 bytes alignment = for semaphore, in the later coding, Run[cpuIndex] can't meet requirement fo= r that since it's UINT32 type. That will bring a lot of inconvenience for c= oding and very easy to make mistake. Besides, we only need allcoate/free on= e continuous Sembuffer for all semaphores, and easy for consumer point to n= ext with CpuSem[CpuIndex].Run. >=20 > > + /// > > + /// All semaphores for each processor in SMM CPU Sync > > + /// > > + SMM_CPU_SYNC_SEMAPHORE_CPU *CpuSem; >=20 > 3. Can we use "volatile UINT32 **Run" instead of pointing to another > structure? > Run points to an array where each element is a UINT32 *. Count of array > equals to NumberOfCpus. >=20 >=20 Same as above explained. >=20 > > + /// > > + /// 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"? >=20 Agree.=20 >=20 >=20 > > + /// > > + /// Size of global and each CPU semaphores > > + /// > > + UINTN SemBlockPages; >=20 > 5. How about "SemBufferSize"? Agree, you want it to be bytes instead of pages? >=20 > > +}; > > + > > +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; >=20 > 6. Can you remove the empty lines among the local variable declarations? Agree. I just wanted it to be more readable: no empty lines means it's grou= p usage for some purpose, that was my original coding style, but I can chan= ge it since you don't like it. :). >=20 > > + > > + if (SmmCpuSyncCtx =3D=3D NULL) { > > + return RETURN_INVALID_PARAMETER; > > + } > > + > > + // > > + // Count the CtxSize > > + // > > + Status =3D SafeUintnMult (NumberOfCpus, sizeof > > (SMM_CPU_SYNC_SEMAPHORE_CPU), &CpuSemInCtxSize); >=20 > 7. Is there a reason to use SafeUintxxx()? I don't believe the multiplica= tion > could exceed MAX_UINTN. > But using SafeUintxxx() makes code hard to read. >=20 This is comments from Laszlo: Please use SafeIntLib for all calculations, to prevent overflows. This means primarily (but not exclusively) memory size calculations, for allocations >From API perspective, it's possible since the NumberOfCpus defined as UNITN= , right? > > + // > > + // Allocate CtxSize buffer for the *SmmCpuSyncCtx > > + // > > + *SmmCpuSyncCtx =3D NULL; > 8. This is not needed. >=20 > > + Status =3D SafeUintnMult (OneSemSize, sizeof > > (SMM_CPU_SYNC_SEMAPHORE_GLOBAL) / sizeof (VOID *), > &GlobalSemSize); > > + if (EFI_ERROR (Status)) { > > + goto ON_ERROR; >=20 > 9. If you don't use SafeUintxxx(), you don't need "ON_ERROR" label and th= e > "goto". >=20 Yes, we can discuss whether need SafeUintxxx(), for me, it's both ok. SafeU= intxxx doesn't do bad things. >=20 > > +/** > > + 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) successful= ly. > > + @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; > > + } >=20 > 10. Can we use "ASSERT" instead of if-check? We can explicitly mention th= e > behavior in API comments. Assert can be added by caller for status if wanted. The API only follow the= definition, right? Or add assert before if check? >=20 > > + > > + Ctx =3D (SMM_CPU_SYNC_CTX *)SmmCpuSyncCtx; >=20 > 11. Why do we need the type cast? Oh, yes, we don't need that, original code defined the ctx as void, I forge= t update this. >=20 > > + > > +/** > > + 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. > Existing implementation logic or SMM_CPU_SYNC_CONTEXT definition doesn't su= pport the checkout or getArrivedcount after lock. I don't want implementati= on support such kind of case, so added that. Or can we return unsupported s= tatus if locked directly? then we can remove that words. -=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 (#112089): https://edk2.groups.io/g/devel/message/112089 Mute This Topic: https://groups.io/mt/102889293/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io] -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-