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 34DBBAC0750 for ; Mon, 13 Nov 2023 03:15:57 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=7htW6Ka5pdTVqsvF3AoQRGijD+9N0zdQmi+GBP/KakY=; 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:msip_labels: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; s=20140610; t=1699845355; v=1; b=GXfCzEOOdK3z+CsRyROfHivNlli15/aXcQDJitCXNZycuu1gZSm+taG4KwK4Z4YOcKAYsZv9 3jp6hUl0/Ia+1+G7p7XTGIXTMbqklJ8vtWld/HoILYAr/EiChB7BhXQfDQiUa/sf1rz5/ZZgktk Fa7F8QQjualhncx6ACoEpG0U= X-Received: by 127.0.0.2 with SMTP id 3DBTYY7687511xWgDitdbhSk; Sun, 12 Nov 2023 19:15:55 -0800 X-Received: from mgamail.intel.com (mgamail.intel.com [192.55.52.120]) by mx.groups.io with SMTP id smtpd.web11.29954.1699845355208881443 for ; Sun, 12 Nov 2023 19:15:55 -0800 X-IronPort-AV: E=McAfee;i="6600,9927,10892"; a="389238604" X-IronPort-AV: E=Sophos;i="6.03,298,1694761200"; d="scan'208,217";a="389238604" X-Received: from orsmga008.jf.intel.com ([10.7.209.65]) by fmsmga104.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 12 Nov 2023 19:15:54 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=McAfee;i="6600,9927,10892"; a="793330331" X-IronPort-AV: E=Sophos;i="6.03,298,1694761200"; d="scan'208,217";a="793330331" X-Received: from orsmsx603.amr.corp.intel.com ([10.22.229.16]) by orsmga008.jf.intel.com with ESMTP/TLS/AES256-GCM-SHA384; 12 Nov 2023 19:15:54 -0800 X-Received: from orsmsx612.amr.corp.intel.com (10.22.229.25) 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.34; Sun, 12 Nov 2023 19:15:52 -0800 X-Received: from orsmsx602.amr.corp.intel.com (10.22.229.15) by ORSMSX612.amr.corp.intel.com (10.22.229.25) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2507.34; Sun, 12 Nov 2023 19:15:51 -0800 X-Received: from ORSEDG602.ED.cps.intel.com (10.7.248.7) by orsmsx602.amr.corp.intel.com (10.22.229.15) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2507.34 via Frontend Transport; Sun, 12 Nov 2023 19:15:51 -0800 X-Received: from NAM11-CO1-obe.outbound.protection.outlook.com (104.47.56.168) by edgegateway.intel.com (134.134.137.103) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.1.2507.34; Sun, 12 Nov 2023 19:15:51 -0800 ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=DHZ2rkm1cjiy+/S46RYNIxGchveFZTHGDmmnn+kaR0at0uhGCC5Fj+hivoJRbP6Z0JbhfkjScUPEOyx827IUA7dDtKnUHVPnbj6wDdRrDGTG8531gFEGPg5cpBzWc8i/F1zLQtaoYEK47q5ljPRERKQLM+aKbezbkadKHDuLk5lOKX798hE0b7RrASBkIth0VCehCXTthYdA/+43NyDrHfC5VBi5gr3a0Pin6R0n0HnAmhRKmLl75rD5nbnNloy321kWri63QbvzBF34CErjzyIrBQ5eKkCj9LGURacp+60ixpt6zZpr5ZLJ6R3suGuq6dgjHEgb/d2Q3wYRheo1bg== 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=pqHD1Hc4dmJvloro0VQLVHoH/3n1uaIp+7ACcP/36/8=; b=i8joEaM4bXR5pec4ydwHP9LAKYHUzLFq582tQGJzeg8g8+6E39PIljFUPLeZI30Mp60Pk5R8bvC+TgN1HwqfPWALljC0HboN0sw4haS/nt8PL+ke06kFTxIpENz/RHT+OoUAsLAc++w6fITKv1kw05fa4ioolHqp24euMyFdoedFkduNYlNlEv8zrF6Ww8bFn9aQ7ItfDUoRHY6aI2xTZPDocr/OTs5JCrAHMiPMqiCHLZtOunQYOcWyRVnIlCZDpguKBpSmJta1VfGoU4dq7v7k3U0MbjOJKsSDVmoss1/+OxGI2ueiViDVEmQ4vC3KkQ9mtiKjvmhUHsWD6uPzPg== 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 SA1PR11MB7110.namprd11.prod.outlook.com (2603:10b6:806:2b3::19) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.6954.28; Mon, 13 Nov 2023 03:15:49 +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.6977.029; Mon, 13 Nov 2023 03:15:49 +0000 From: "Ni, Ray" To: Laszlo Ersek , "devel@edk2.groups.io" , "Wu, Jiaxin" CC: "Dong, Eric" , "Zeng, Star" , Gerd Hoffmann , "Kumar, Rahul R" Subject: Re: [edk2-devel] [PATCH v1 3/7] UefiCpuPkg: Adds SmmCpuSyncLib library class Thread-Topic: [edk2-devel] [PATCH v1 3/7] UefiCpuPkg: Adds SmmCpuSyncLib library class Thread-Index: AQHaDmq40hK9Pt7F5Eei/quHpg/ZGrBurM4AgAjem8U= Date: Mon, 13 Nov 2023 03:15:49 +0000 Message-ID: References: <20231103153012.3704-1-jiaxin.wu@intel.com> <20231103153012.3704-4-jiaxin.wu@intel.com> In-Reply-To: Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: msip_labels: x-ms-publictraffictype: Email x-ms-traffictypediagnostic: MN6PR11MB8244:EE_|SA1PR11MB7110:EE_ x-ms-office365-filtering-correlation-id: aad765b7-61e9-4412-31ce-08dbe3f6d638 x-ms-exchange-senderadcheck: 1 x-ms-exchange-antispam-relay: 0 x-microsoft-antispam-message-info: JouFQHJPXSN4+GOUfw7xVNxRSNG+QjIK1bOMFex2Z67NhjFrnXR7eDOZtVreJTVxkE3Y7gO6CuLObocm95Dv8NFfARxPwsmvrNgZXKx2gu0hM9ioJvDQGtkV3pkb4tNxhnQ6ZAP+tISKn1ttq+YVbDWu5n0YOBGWPo7b0KCBDbHslz5onOnY8UjpdBd6TX9nrZbWjTlbcIA9qCdfztJqPS0P9d8suywlpBh6Gbx1BhzPiMWzEXq90jhUjE7hDFOPvVS6mdgAds3YeWGhZ9z1rTUKA9+219vjCxL17wTi69B5daQcDvdfgg/U2FuJKsP8gkwBoU+Yg6320/A5A6oY+SymTEArALqjT3iZ8dmJGFKqUmuupf71czv3pGmA/MUMEfnTeFfBpI62YX26dThJhwKYR/hwOoq91YdoIb0eipRVs7dj65RkklqFVxKYtaaEnZjYIReJ99VEOba9GLFekcqh2ZF97VSXWwzXopeGaew5PNsHgurBWGbYckSvi2oHmxP3BVNU213ybB0XvThdoBTS30cKC+uUZi4RB8hUOXWEckK8AVsnJ8yqs9zWmspZmuejN1l8cPVqHpruCp+ZHK7KPAWTLUJrSnOQLcL+JsBm8Wp5Hg7+n8VnQL779Kgi x-ms-exchange-antispam-messagedata-chunkcount: 1 x-ms-exchange-antispam-messagedata-0: =?iso-8859-1?Q?cA0Ypr2+AfLvoqAU4BCW0YlwBnBWViTiomhMXPwSE6ZsagSwVcA/8OIOW8?= =?iso-8859-1?Q?psYpq2Y86Wo/aR6EDusN91TltEFyY7fCVEBF9P7WPbGoNZTu10FWzsEngk?= =?iso-8859-1?Q?y8lWNp4WnMTGYEBkEsvYkQbl4g+xzTzzZAagCRQYLugB57/6fNwI+Mhyf8?= =?iso-8859-1?Q?0KaiOeHccAPHNXroGkyNWWgNgeh2/Yxk2I892AXkdXfPhBVtFxHrLjUW+n?= =?iso-8859-1?Q?ZW/vLYCCZmKET91D3E04pXnkb4RSoZDN9CXDIaR7kh8J4hQfvO2HGcQUSY?= =?iso-8859-1?Q?MNcBG2HlWOV6DlcrSgP7F1PSBHD+LXS3OpJ6YipVgic6ZXAyqY4YbM2Zsz?= =?iso-8859-1?Q?KosQ2o59BXV5hGaB2GVygFNzVcYu2d/YWA3VV5qnV4kd8Y/10cRuGKWPg+?= =?iso-8859-1?Q?UwSdBw0+dmwPuiEQWa3Iv0YR6gfiL/ke32gtj0yfE4YXz+12ykw8hgZlvU?= =?iso-8859-1?Q?lvlmz5xZod59NOv2eF3djoOgl98qSXoX9m0DQTd/97cVTWCNwCE5OM7udQ?= =?iso-8859-1?Q?Mko+0PImQ99Um9/hYYwBU3V189TvD9eMs9QEeSQ/jRaRTf/h1RjAiXKXr8?= =?iso-8859-1?Q?MHMqY1AUamHRDGZ0raKsG7D6dZ2Am5nxhq2ytlpMWkkIvTh2TIQn0IdpFE?= =?iso-8859-1?Q?J821LTEvBxJXThyoxB2NInl2X0AXJd/7EmbsxtaN342rl6HfEydsm9MAJY?= =?iso-8859-1?Q?UqQOBxjBQSOM/YQkxyRrDpN4YbqK2pSOa/ZRAHa8oDfh4OKolRGmjlnvx3?= =?iso-8859-1?Q?QKO5HdFpAOrBKZ11WkRnSUM5OUXNn7ey18l1/bobjgUkuJG0qVsBmbaULV?= =?iso-8859-1?Q?QOCrkybVD3k+MJKwYvr9OhjDlBnMemjZjb8Mn12nU3jlAAIP4DS7UqInEb?= =?iso-8859-1?Q?b3mKt+IY47c+xofcQFRfmO60ewyLSz2mgAODWW0wM3/vmd2MKpCs2b7RTT?= =?iso-8859-1?Q?vXUUEnlgi1ZsHAk8fwyXpCAmbAGnEgO1PQtcZKA67vqPZoD3jVoKbJkUDm?= =?iso-8859-1?Q?OqrXlw1Aru6aw6xdh8yAeAJDsNr8ts6UGL4QtsMVsqLgEzGCRVheHnY8A5?= =?iso-8859-1?Q?fhAS1B/maT8nIxG7nHk2exFYrrg16u9zapYRONkfzIR24orQTqh8QTQnLd?= =?iso-8859-1?Q?NSJTyquiLpd82sRx/26G0DxyNhyWSMrsnmwzExHl4OymtfLO7Gxjb0/v8n?= =?iso-8859-1?Q?2l4Ssj/VzpylgZOhJ5GAcD8D9/0SKBpcZ/mkUEMANpcpitd/3PJfjc14Ru?= =?iso-8859-1?Q?wImmI1p2xTvlMmEnM79cZt6NyjyCTaI2N5Nmiwk77lsY2oKtRlafrFAZ/g?= =?iso-8859-1?Q?I8IqddH9zq2RUkDCkZHCrG3H+4nDovdmR7GI0bLl7mcFICJnE9JsJlKx4t?= =?iso-8859-1?Q?F+3rov4ns6/JLlVqvz9K6And4hFWgApEWKUyGmA4WtjH0mQoNSp9CCR9L4?= =?iso-8859-1?Q?5HVd8VrDTbxWVr/Ttfw7XevaxDT24LlXVUYiAFMAYGM9lmVYynLx8UfggV?= =?iso-8859-1?Q?0Y06yHghaFzZzG1auiynSdPqQ1XdKLEsgJNGdwED1rZP/HSKhfkUhd6rh6?= =?iso-8859-1?Q?uFE7uGXUa4DqU/Ar4r+zVo1RIOH+kHBoQAXNDjENJdlwR/WbauAl1wI3Rg?= =?iso-8859-1?Q?FkiBcjP+qS8VA=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: aad765b7-61e9-4412-31ce-08dbe3f6d638 X-MS-Exchange-CrossTenant-originalarrivaltime: 13 Nov 2023 03:15:49.8520 (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: gt1voMomOEO2AEbRL/8Ve9FybHqoFa2ljdJ46JyEwDsp/QuTgdA1AmC5qk1xxcV1BaPG8TKuWjAKbxVawIvEfQ== X-MS-Exchange-Transport-CrossTenantHeadersStamped: SA1PR11MB7110 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: qwoWYlH2v4vBbGhzVwout8hDx7686176AA= Content-Language: en-US Content-Type: multipart/alternative; boundary="_000_MN6PR11MB82440135FD79C1B5D52B2E978CB3AMN6PR11MB8244namp_" X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20140610 header.b=GXfCzEOO; arc=reject ("signature check failed: fail, {[1] = sig:microsoft.com:reject}"); 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 --_000_MN6PR11MB82440135FD79C1B5D52B2E978CB3AMN6PR11MB8244namp_ Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable I provided 4 comments in below, starting with "[Ray". Sorry for the poor new Outlook that I am not able to put ">" in the origina= l email. Thanks, Ray ________________________________ (1) I agree that the internals of the context should be hidden, but (VOID*) is not the right way. Instead, please use an incomplete structure type. That is, in the library header class file, do the following: typedef struct SMM_CPU_SYNC_CTX SMM_CPU_SYNC_CTX; and then make all these functions return and take SMM_CPU_SYNC_CTX * The library users will still not know what is inside SMM_CPU_SYNC_CTX, just like with (VOID*), but the compiler will be able to perform much stronger type checking than with (VOID*). And then in the library *instance(s)*, you can complete the incomplete type= : struct SMM_CPU_SYNC_CTX { ... }; [Ray.1] Good suggestion. I still remember you taught me this technique when= I wrote the FrameBufferBltLib. (3) By definition, in a function that resets the internals of an object, you cannot specify "IN" for that function. It must be OUT. [Ray.2] I have a different opinion about IN/OUT. I think we should use "IN= OUT". Please add a large comment to the top of the library class header that explains the operation / concepts of the context. What operations and behaviors are defined for this data type? [Ray.3] Good suggestions. The lib provides 3 sets of APIs: 1. Init/Deinit Init() is called in driver's entrypoint for allocating the storage and init= ialize the content of sync object. Deinit() is called in driver's unload function for undoing what has been do= ne in Init(). 2. CheckInCpu/CheckOutCpu/LockDoor/GetArrivedCpuCount When SMI happens, all processors including BSP enter to SMM mode by calling= CheckInCpu(). The elected BSP calls LockDoor() so that CheckInCpu() after that returns fa= ilure code. CheckOutCpu() is called in error handling flow for the CPU who calls CheckI= nCpu() earlier. GetArrivedCpuCount() returns the number of checked-in CPUs. 3. WaitForAllAPs/ReleaseOneAp/WaitForBsp/ReleaseBsp First 2 APIs are called from BSP. Latter 2 APIs are called from APs. The 4 APIs are used to synchronize the running flow among BSP and APs. > + > + @return CPU arrival count number. (14) why is it necessary for outputting the arrived number when locking? [Ray.4] As I explained above, when BSP locks the door, it might need to kno= w how many CPUs are in the SMM "room". Maybe today the number is not cared by BSP. -=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 (#111118): https://edk2.groups.io/g/devel/message/111118 Mute This Topic: https://groups.io/mt/102366300/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- --_000_MN6PR11MB82440135FD79C1B5D52B2E978CB3AMN6PR11MB8244namp_ Content-Type: text/html; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable
I provided 4 comments in below, starting with "[Ray".

Sorry for the poor new Outlook that I am not able to put ">" i= n the original email.

Thanks,
Ray


(1) I agree that the internals of the context should be hidden, but
(VOID*) is not the right way. Instead, please use an incomplete
structure type.

That is, in the library header class file, do the following:

  typedef struct SMM_CPU_SYNC_CTX SMM_CPU_SYNC_CTX;

and then make all these functions return and take

  SMM_CPU_SYNC_CTX *

The library users will still not know what is inside SMM_CPU_SYNC_CTX,
just like with (VOID*), but the compiler will be able to perform much
stronger type checking than with (VOID*).

And then in the library *instance(s)*, you can complete the incomplete type= :

  struct SMM_CPU_SYNC_CTX {
    ...
  };

[Ray.1] Good suggestion. I still reme= mber you taught me this technique when I
wrote the FrameBufferBltLib.

(3) By definition, in a function that resets the internals of an object, you cannot specify "IN" for that function. It must be OUT.

[Ray.2] I have a different opinion about IN/OUT.  I think we should us= e "IN OUT".


Please add a large comment to the top of the library class header that
explains the operation / concepts of the context. What operations and
behaviors are defined for this data type?

[Ray.3] Good= suggestions.
The lib prov= ides 3 sets of APIs:
1. Init/Dein= it
Init() is called in driver's entrypoi= nt for allocating the storage and initialize the content of sync object.
Deinit() is = called in driver's unload function for undoing what has been done in Init()= .

2. CheckInCp= u/CheckOutCpu/LockDoor/GetArrivedCpuCount
When SMI hap= pens, all processors including BSP enter to SMM mode by calling CheckInCpu(= ).
The elected = BSP calls LockDoor() so that CheckInCpu() after that returns failure code.<= /span>
CheckOutCpu() is called in error hand= ling flow for the CPU who calls CheckInCpu() earlier.
GetArrivedCp= uCount() returns the number of checked-in CPUs.

3. WaitForAl= lAPs/ReleaseOneAp/WaitForBsp/ReleaseBsp
First 2 APIs are called from BSP.
Latter 2 APIs are called from APs.
The 4 APIs are used to synchronize th= e running flow among BSP and APs.

> +
> +  @return     CPU arrival count number.

(14) why is it necessary for outputting the arrived number when locking?
[Ray.4] As I= explained above, when BSP locks the door, it might need to know how many C= PUs are in the SMM "room".
Maybe today the number is not cared by BSP.

_._,_._,_
Groups.io Links:

=20 You receive all messages sent to this group. =20 =20

View/Reply Online (#111118) | =20 | Mute= This Topic | New Topic
Your Subscriptio= n | Contact Group Owner | Unsubscribe [rebecca@openfw.io]

_._,_._,_
--_000_MN6PR11MB82440135FD79C1B5D52B2E978CB3AMN6PR11MB8244namp_--