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 1D827AC0C23 for ; Sat, 20 Jan 2024 06:48:59 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=doQEj+tN4CWjENjgEu82Du37UoGIrBCqI9lU3vGrKUY=; 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; s=20140610; t=1705733338; v=1; b=BQPYplqIBqr2FiYICAUysB2GVuQ0dHKmTXPxGNVOun0Il8z2JAXajpWG8rAfzd8iJyG0Im9J 1WPP0gBOSrw3C3+EQKQAviURlsftQ5/08IVsCczbfRiy/ZR7+l/ifNCkAA0U3hiKqUW8/EXFnQP 1wP+Nesa5r+inVqYiPJPBZgk= X-Received: by 127.0.0.2 with SMTP id bntLYY7687511x5elzZhoRky; Fri, 19 Jan 2024 22:48:58 -0800 X-Received: from mgamail.intel.com (mgamail.intel.com [192.55.52.88]) by mx.groups.io with SMTP id smtpd.web11.16849.1705733337993071968 for ; Fri, 19 Jan 2024 22:48:58 -0800 X-IronPort-AV: E=McAfee;i="6600,9927,10957"; a="432093918" X-IronPort-AV: E=Sophos;i="6.05,207,1701158400"; d="scan'208,217";a="432093918" X-Received: from orviesa005.jf.intel.com ([10.64.159.145]) by fmsmga101.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 19 Jan 2024 22:48:57 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="6.05,207,1701158400"; d="scan'208,217";a="757545" X-Received: from fmsmsx602.amr.corp.intel.com ([10.18.126.82]) by orviesa005.jf.intel.com with ESMTP/TLS/AES256-GCM-SHA384; 19 Jan 2024 22:48:57 -0800 X-Received: from fmsmsx610.amr.corp.intel.com (10.18.126.90) 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; Fri, 19 Jan 2024 22:48:56 -0800 X-Received: from fmsmsx602.amr.corp.intel.com (10.18.126.82) 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; Fri, 19 Jan 2024 22:48:55 -0800 X-Received: from FMSEDG603.ED.cps.intel.com (10.1.192.133) 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 via Frontend Transport; Fri, 19 Jan 2024 22:48:55 -0800 X-Received: from NAM10-DM6-obe.outbound.protection.outlook.com (104.47.58.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.35; Fri, 19 Jan 2024 22:48:55 -0800 ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=b/k/tIjYL3XYGVRjgaAmFf4jGaBqu2PsW9wNDGnPMPQuqJ3nla1lpDYMO77PLg7NZ2LpR1fthAR9dTWXwFaPXeNGrFC7eXHe9ZvDzTFF9lJEj5Z+6REKEj+/4aeh0ew2OhtlzTGxcv0q/PWqC1RLF/LkQDFf0vxqT0svZGViFh+1gSBd5f3eVCOi/YHCnbX1tif4ZwTmVLPuLMYek5B+F65YQdPoUML9vKrUUC1Cg+pMBx/xpLLHTPKUSvNAhlsryzUjwhF39ru79YIEheO9bL2iixyORRXHicP9oSnxu3JgqaSSizrKS8gNVXE0VyipLReYBn95FKZ7TGtHoYFN/Q== 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=sCQq2W4SmhZm5Vq8YK2ymPyFQtFe6+cmkOXvdZtXzrI=; b=dK22Ojy22GOa7zlnVzn8XxSY2nzcYWxQfAUxF1m9kqUPHZjOKIhqFwfH++plhoWEG9vMcjjaVOmbGfe922+QAPFj8PgN1GSpZlJqzwhNU0m4QUgX9mWDRqN2RCLIdFG/xfLz2oL6t5I0Q6lDU0/1bqHHpbF7QqA3OvkzJbq1/cUW7fKKkm52w380jOPTtic9Y/zm4MDNtr4g5ZmRupKgqF0Pbw6tD978u05fNFhhTAMSjuCnLeoarI3KfjGaPRCYthsy55ecKpaKPKKGu1Ji6g+RVCmlhbM7bnBL6wMo2ibBsc2roDpVAtMnGiwyVsKAF+MMJl34PG4NbLlMJD0OtA== 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 BL3PR11MB5682.namprd11.prod.outlook.com (2603:10b6:208:33d::18) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.7202.27; Sat, 20 Jan 2024 06:48:53 +0000 X-Received: from MN6PR11MB8244.namprd11.prod.outlook.com ([fe80::3fea:ca2b:2ef7:e3d4]) by MN6PR11MB8244.namprd11.prod.outlook.com ([fe80::3fea:ca2b:2ef7:e3d4%4]) with mapi id 15.20.7202.024; Sat, 20 Jan 2024 06:48:52 +0000 From: "Ni, Ray" To: "Kinney, Michael D" , Tom Lendacky , "devel@edk2.groups.io" , "Gao, Liming" , "Liu, Zhiguang" , "Dong, Eric" , "Kumar, Rahul R" , Gerd Hoffmann , "Ard Biesheuvel" CC: Michael Roth Subject: Re: [edk2-devel] [PATCH 1/2] UefiCpuPkg/MpInitLib: Use AsmCpuidEx() for CPUID_EXTENDED_TOPOLOGY leaf Thread-Topic: [edk2-devel] [PATCH 1/2] UefiCpuPkg/MpInitLib: Use AsmCpuidEx() for CPUID_EXTENDED_TOPOLOGY leaf Thread-Index: AQHaEQc2QQbjTJ6CsU+Ez2kG5PG30rDe9Z3pgAGvawCAALU6sIAA3q0AgAAH3zqAACLTAIAAUx1Q Date: Sat, 20 Jan 2024 06:48:52 +0000 Message-ID: References: <17952A20A9E21541.12603@groups.io> <26becea4-2ad8-4773-ab3d-5ad98ff48759@amd.com> <179BD02AA4207037.22216@groups.io> In-Reply-To: Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-ms-publictraffictype: Email x-ms-traffictypediagnostic: MN6PR11MB8244:EE_|BL3PR11MB5682:EE_ x-ms-office365-filtering-correlation-id: 17285b42-0fdb-446d-2362-08dc1983dd9f x-ms-exchange-senderadcheck: 1 x-ms-exchange-antispam-relay: 0 x-microsoft-antispam-message-info: LxrvCxI3TIc+PHK9fz7vEUr1k8B/KO2o7HIO5tPXsGIU3ueokcwP86wz+c8laxD5VEh03Xt0fuiJG7UqjDp0bSO/1F2qYRK9Ohz3e78AMsFKM1W+K4WzwnTcH9hJMnSFlbwqI656cRgvHNXEIqUJawKpH5BshpZbgvpF41GFBWUCpCJTmvmW2jfODlLikZ1IBonShxn/qluEiQ5f+BoNBjrakYT0FrJ5NFMOmqThhzvAp87hqDghCTd7vrnzpCxP5GGgQ9qxQ1Dk2rRFCF/ChEe2RC1NnaHY6DbFS0zhXAl7vI3qj8kTZmEBE7QxcW4MgmsfqTQ4cW8lMRAeHA4y7biwvCx7fKQ/wev8/G5TrE5RZiuM/AeUvHC5MbpM2FmBSl9CSeG1tYq3m/lBH5wUGwRNnSYZbCCadvmPWMwqVDP5sGA22fMlaneHp4megkbLnSiW9d5MGw//bumAbAcgSe/ql1y0Z1/SVd2HR0b5ZWqnK3BXvA4vIAjjhQpopo2Zy4jXMgHCzAPdecmGZ8YvzboNOYe4WtmcUGoqyYmDcK81PUi/GgSDTGrYeH0GI2FJrI9VSZDBdEqLXrXhRpTCuAZjsPdm3BTIHmOS8LV4WxX2gS1do5c+T5nws49h8ZJza9BmONVxwU4e6bzDlG41Sg== x-ms-exchange-antispam-messagedata-chunkcount: 1 x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?h0CscAAU4l3FT0qM5Cp2yBir8LxXEF93lQSdladuKZslAE2tAOTsuCWSMf/B?= =?us-ascii?Q?4tY7y5MyhEaGiua47tZd3kw0rAMgUniMyCvX94CgNWFJ2vKPprONY7X7evsj?= =?us-ascii?Q?q3knZedMUWrcSOj0mLB9vVRP/KPP+ou5FfcsPQmb2dchWcR2WH3HYf/Q6cOx?= =?us-ascii?Q?P6+pnO6XlPEMqOTxP6W0a660l06bErbtBGJ0/O7SHH6/uBko3BinvwsszCxy?= =?us-ascii?Q?w6P0GT8B071vnlE3EcqjM7xozQ53l4dh4elhAJFNEU/Yi61AAev65ygBddq+?= =?us-ascii?Q?v1VRACutJwgWtiFbjpZ9td8FDPstBAa7i+uIwKTS8yYXrdU/iWJGanJZ0cLz?= =?us-ascii?Q?YT64KhjAadUdTeeB00U2ipP3sUCDHgs3agZhj3ZpeuBGkqhrVa+KzREN/cId?= =?us-ascii?Q?QW0944sfczM6Eal/JigyueJzWi7sBnU2XbjFk68bcum+MUr+bfmhdoP3ofAb?= =?us-ascii?Q?i9ErvnybQJ+fpBMlg1JoezOltsEFDl1pyv6OwPpBJKz/9Paz4nmLiQj++F1p?= =?us-ascii?Q?XT7JNaOTuC/ydiYf0ogxw2ALJKY4FPAyjAzvfWb6q8iAcPtELztdXosEgtRm?= =?us-ascii?Q?f6d+uKq25nwRZf7Si8PYl4E80551J1KiMkXKNDYefRuZUR8QeJuS6lrt8Duv?= =?us-ascii?Q?lpAPDXHo75sN4FjzlWZcwiOutWxDX4ao5Gd5YUase9mgldOD9ugc7Gt3kVNz?= =?us-ascii?Q?PsHBDW9KX9OHfBm//QeATHEKQ0MbV9O8DoFPzevGRFMAnBcGPto6kuWpumr4?= =?us-ascii?Q?A7GzJ4Ra9/rG3LNBOl1h6Z/OhNbG7X82+iMjeI1MR22sCilQsBz67UodFKdW?= =?us-ascii?Q?XBP5TRTHccY6HVfaYdUH16gBudbZ8K8LyzENM7kyTps8e8NcDws2yBVuqRlG?= =?us-ascii?Q?gmXmrYCAj8y2aSWoywrHD0+BbfVOl3NDYks9vlUrRIayRoqJv9/YgxlVTb6/?= =?us-ascii?Q?lSaAQymo2aYUj7XwlRfj+TwRRPmGRwIsCdH1PRbxb7ywz0gZMwq5/Zzs1isi?= =?us-ascii?Q?wvJ9GuFvVJMXWZC+NsJV6WvZknH/omm2+V381SATab4ABvcmy2m9N20WcsEQ?= =?us-ascii?Q?v3LARNWH+siGimpdA+F3MVcqAzYdFbpvrHakkN/jHP3hW4gfZ0hP6NIk4d9x?= =?us-ascii?Q?CK5vNCps7yx/I6lal33lYdLh9R2nsWtwqMnge07FiX8coer5zXdjEChW5pQw?= =?us-ascii?Q?wJuu82hU6ILzLLHoAisD9K4/6l/eLVme5uHbSoAk8AfkrrwU5WcVCzC191EE?= =?us-ascii?Q?7APSEu/v+BKwdznAjkr0KpN3OvuWSm+eO8bTgOoxhw6tlik1ONC2BdRkf+fA?= =?us-ascii?Q?QjoPQc4TNtgkud4mpQDRapdDg9rg046qr+hgOzSoboUMF48xM7llJSIxgD7P?= =?us-ascii?Q?RCX9CPPP7EX5hk8EtA/konskUWrbrn/+C9EPKFoUBolRQO+XlK5Zu0PJRniK?= =?us-ascii?Q?0Ja9DR4iBQ7jjuwgAP4YtruYAm0niT8aEDB5QGRymQ/N7UEz5PIWmTQg/kcX?= =?us-ascii?Q?XOBTXXwK2O7qb+ltcOh/3vtvhyXgx7eS8AVn/nfkFgUxNi2QgQPUSHFI1qUZ?= =?us-ascii?Q?qjAn/BpRzvr8zpzHJsE=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: 17285b42-0fdb-446d-2362-08dc1983dd9f X-MS-Exchange-CrossTenant-originalarrivaltime: 20 Jan 2024 06:48:52.9033 (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: Kim25S4wlS/JfbyEcuwNZChRIOuEuM7kcreaHjvdsO/YvIeHX1DHAWvutvZcLMN8+cDIgA1qYkEsT59z4DanZw== X-MS-Exchange-Transport-CrossTenantHeadersStamped: BL3PR11MB5682 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: FY3rPQfFdQjnH48f9rgGDvvyx7686176AA= Content-Language: en-US Content-Type: multipart/alternative; boundary="_000_MN6PR11MB8244774B15D9B80C2701DFEC8C772MN6PR11MB8244namp_" X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20140610 header.b=BQPYplqI; 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_MN6PR11MB8244774B15D9B80C2701DFEC8C772MN6PR11MB8244namp_ Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Mike, Tom, How about AsmCpuId() adds ASSERT() to reject those CPUID leaves that do not= have sub-leaf? Another concern I have if AsmCpuid() zeros ECX is callers would call AsmCpu= id(leaf) instead of AsmCpuidEx (leaf, 0). This would cause the caller code a mess. Thanks, Ray From: Kinney, Michael D Sent: Saturday, January 20, 2024 9:49 AM To: Ni, Ray ; Tom Lendacky ; dev= el@edk2.groups.io; Gao, Liming ; Liu, Zhiguang ; Dong, Eric ; Kumar, Rahul R ; Gerd Hoffmann ; Ard Biesheuvel Cc: Michael Roth ; Kinney, Michael D Subject: RE: [edk2-devel] [PATCH 1/2] UefiCpuPkg/MpInitLib: Use AsmCpuidEx(= ) for CPUID_EXTENDED_TOPOLOGY leaf The issue is if AsmCpuid() is called for an Index value that does depend on= ECX. That would be a bug on the caller's part and would not have determin= istic behavior because ECX on input is not deterministic. That is the cond= ition that would be good to catch. Mike From: Ni, Ray > Sent: Friday, January 19, 2024 3:49 PM To: Kinney, Michael D >; Tom Lendacky >; devel@edk2.groups.io; Gao, Liming >; Liu, Zhiguang >; Dong, Eric >; Kumar, Rahul R >; Gerd Hoffmann >; Ard Biesheuvel > Cc: Michael Roth >; Kinne= y, Michael D = > Subject: Re: [edk2-devel] [PATCH 1/2] UefiCpuPkg/MpInitLib: Use AsmCpuidEx(= ) for CPUID_EXTENDED_TOPOLOGY leaf Mike, For a certain Cupid leaf that does not have sub leaf, Cupid instruction doe= s not consume ecx and it always fills ecx with a determined value, defined = by sdm. So, I don't see any hurt to deterministic behavior if not zeroing ecx in As= mCpuid. thanks, ray ________________________________ From: Kinney, Michael D > Sent: Saturday, January 20, 2024 7:16:14 AM To: Ni, Ray >; Tom Lendacky >; devel@edk2.groups.io<= mailto:devel@edk2.groups.io> >; Gao, Liming >; Liu= , Zhiguang >; Dong, E= ric >; Kumar, Rahul R >; Gerd Hoffmann >; Ard Biesheuvel > Cc: Michael Roth >; Kinne= y, Michael D = > Subject: RE: [edk2-devel] [PATCH 1/2] UefiCpuPkg/MpInitLib: Use AsmCpuidEx(= ) for CPUID_EXTENDED_TOPOLOGY leaf Hi Ray, It is about having deterministic behavior if a call if made for a CPUID EAX value that does depend on ECX. If ECX is not zeroed, then it will have a random value that may return different information. The problem statement from Tom is not about zeroing ECX. It is about avoiding code bugs where AsmCpuid() is called for an Index value that is documented to depend on ECX. In this case, we would want an error condition so the developer knows they should use AsmCpuidEx() instead. >From looking at the Intel SDM, there is a small set of Index values that do not look at ECX at all. We could consider adding an ASSERT() condition in AsmCpuid() if Index is a value that depends on ECX. Perhaps in DEBUG_CODE() so it is not always present. Mike > -----Original Message----- > From: Ni, Ray > > Sent: Friday, January 19, 2024 2:01 AM > To: Kinney, Michael D >; Tom Lendacky > >; devel@edk2.gro= ups.io; Gao, Liming > >; Liu, Zhiguang >; Dong, > Eric >; Kumar, Rahul R >; > Gerd Hoffmann >; Ard Biesheuv= el > > > Cc: Michael Roth > > Subject: RE: [edk2-devel] [PATCH 1/2] UefiCpuPkg/MpInitLib: Use > AsmCpuidEx() for CPUID_EXTENDED_TOPOLOGY leaf > > Mike, > I agree with your words after "However". > Zeroing ECX in AsmCpuid() is confusing to future code maintainer: If > CPUID instruction does > not consume "ECX", why is it needed to zero "ECX"? > > Thanks, > Ray > > -----Original Message----- > > From: Kinney, Michael D > > > Sent: Friday, January 19, 2024 7:11 AM > > To: Tom Lendacky >; devel@edk2.groups.io; > > Gao, Liming >; Liu, Z= higuang > >; > > Dong, Eric >; Ni, Ray <= ray.ni@intel.com>; Kumar, > Rahul R > > >; Gerd Hoffman= n >; Ard > > Biesheuvel = > > > Cc: Michael Roth >; K= inney, Michael D > > > > > Subject: RE: [edk2-devel] [PATCH 1/2] UefiCpuPkg/MpInitLib: Use > > AsmCpuidEx() for CPUID_EXTENDED_TOPOLOGY leaf > > > > Hi Tom, > > > > I do not see any harm in zeroing ECX in AsmCpuid(). > > > > If it is not zeroed, then it would have an undefined value. > > > > However, calling AsmCpuid() for any Index that evaluates ECX > > (including a check for 0) should never be done. If ECX is > > evaluated for a given Index, then AsmCpuIdEx() must be used. > > > > Mike > > > > > -----Original Message----- > > > From: Tom Lendacky > > > > Sent: Wednesday, January 17, 2024 1:26 PM > > > To: devel@edk2.groups.io; Kinney, Michae= l D > > > >; Gao,= Liming >; > Liu, > > > Zhiguang >; Don= g, Eric >; > Ni, > > > Ray >; Kumar, Rahul R >; > Gerd > > > Hoffmann >; Ard Biesheuve= l > > > > > > Cc: Michael Roth > > > > Subject: Re: [edk2-devel] [PATCH 1/2] UefiCpuPkg/MpInitLib: Use > > > AsmCpuidEx() for CPUID_EXTENDED_TOPOLOGY leaf > > > > > > On 11/28/23 08:35, Lendacky, Thomas via groups.io wrote: > > > > On 11/6/23 17:15, Tom Lendacky wrote: > > > >> On 11/6/23 16:45, Lendacky, Thomas via groups.io wrote: > > > >>> The CPUID_EXTENDED_TOPOLOGY CPUID leaf takes a subleaf as input > > when > > > >>> returning CPUID information. However, the AsmCpuid() function > does > > > not > > > >>> zero out ECX before the CPUID instruction, so the input leaf is > used > > > as > > > >>> the sub-leaf for the CPUID request and returns erroneous/invalid > > > CPUID > > > >>> data, since the intent of the request was to get data related to > > > sub-leaf > > > >>> 0. Instead, use AsmCpuidEx() for the CPUID_EXTENDED_TOPOLOGY > leaf. > > > >> > > > >> Alternatively, the AsmCpuid() function could be changed to XOR > ECX > > > >> before invoking the CPUID instruction. This would ensure that the > 0 > > > >> sub-leaf is returned for any CPUID leaves that support sub- > leaves. > > > >> Thoughts? > > > >> > > > >> Adding some additional maintainers for their thoughts, too. > > > > > > > > Any thoughts on this approach (as a separate, unrelated patch) to > > > > eliminate future issues that could pop up? > > > > > > > > Seems like zeroing out ECX before calling CPUID would be an > > > appropriate > > > > thing to do, but I'm not sure if that will have any impact on the > > > existing > > > > code base... it shouldn't, but you never know. > > > > > > Just a re-ping for thoughts on this. > > > > > > Thanks, > > > Tom > > > > > > > > > > > Thanks, > > > > Tom > > > > > > > >> > > > >> Thanks, > > > >> Tom -=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 (#114113): https://edk2.groups.io/g/devel/message/114113 Mute This Topic: https://groups.io/mt/102432782/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_MN6PR11MB8244774B15D9B80C2701DFEC8C772MN6PR11MB8244namp_ Content-Type: text/html; charset="us-ascii" Content-Transfer-Encoding: quoted-printable

Mike, Tom,

How about AsmCpuId() adds ASSERT() to reject those C= PUID leaves that do not have sub-leaf?

 

Another concern I have if AsmCpuid() zeros ECX is ca= llers would call AsmCpuid(leaf) instead of AsmCpuidEx (leaf, 0).=

This would cause the caller code a mess.<= /p>

 

Thanks,

Ray

From: Kinney, Michael D <michael.d.kinney@= intel.com>
Sent: Saturday, January 20, 2024 9:49 AM
To: Ni, Ray <ray.ni@intel.com>; Tom Lendacky <thomas.lendac= ky@amd.com>; devel@edk2.groups.io; Gao, Liming <liming.gao@intel.com&= gt;; Liu, Zhiguang <zhiguang.liu@intel.com>; Dong, Eric <eric.dong= @intel.com>; Kumar, Rahul R <rahul.r.kumar@intel.com>; Gerd Hoffmann <kraxel@redhat.com>; Ard Biesheuvel <ardb+tianocore@kern= el.org>
Cc: Michael Roth <michael.roth@amd.com>; Kinney, Michael D <= ;michael.d.kinney@intel.com>
Subject: RE: [edk2-devel] [PATCH 1/2] UefiCpuPkg/MpInitLib: Use AsmC= puidEx() for CPUID_EXTENDED_TOPOLOGY leaf

 

The issue is if AsmCpuid() is called for an Index va= lue that does depend on ECX.  That would be a bug on the caller’= s part and would not have deterministic behavior because ECX on input is no= t deterministic.  That is the condition that would be good to catch.

 

Mike

 

From: Ni, Ray <ray.ni@intel.com>
Sent: Friday, January 19, 2024 3:49 PM
To: Kinney, Michael D <michael.d.kinney@intel.com>; Tom Lendacky <thomas.lendacky@amd.com>; devel@edk2.groups.io; Gao, Limi= ng <liming.gao@intel.com>= ; Liu, Zhiguang <zhiguang.liu@= intel.com>; Dong, Eric <er= ic.dong@intel.com>; Kumar, Rahul R <rahul.r.kuma= r@intel.com>; Gerd Hoffmann <kraxel@redhat.com>; Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Michael Roth <michael= .roth@amd.com>; Kinney, Michael D <michael.d.kinney@intel.com>
Subject: Re: [edk2-devel] [PATCH 1/2] UefiCpuPkg/MpInitLib: Use AsmC= puidEx() for CPUID_EXTENDED_TOPOLOGY leaf

 

Mike,

For a certain Cupid leaf that does not have sub leaf= , Cupid instruction does not consume ecx and it always fills ecx with a det= ermined value, defined by sdm.

So, I don't see any hurt to deterministic behavior i= f not zeroing ecx in AsmCpuid.

 

thanks,

ray

From: Kinney, Michael D <michael.d.kinney@intel.com>
Sent: Saturday, January 20, 2024 7:16:14 AM
To: Ni, Ray <ray.ni@intel.com= >; Tom Lendacky <thoma= s.lendacky@amd.com>; devel@edk2.groups.io <devel@edk2.groups.io>; Gao, Liming= <liming.gao@intel.com>; = Liu, Zhiguang <zhiguang.liu@in= tel.com>; Dong, Eric <eric.dong@intel.com<= /a>>; Kumar, Rahul R <rahu= l.r.kumar@intel.com>; Gerd Hoffmann <kraxel@redhat.com>; Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Michael Roth <michael= .roth@amd.com>; Kinney, Michael D <michael.d.kinney@intel.com>
Subject: RE: [edk2-devel] [PATCH 1/2] UefiCpuPkg/MpInitLib: Use AsmC= puidEx() for CPUID_EXTENDED_TOPOLOGY leaf

 

Hi Ray,

It is about having deterministic behavior if a call if made for
a CPUID EAX value that does depend on ECX.  If ECX is not zeroed,
then it will have a random value that may return different
information.

The problem statement from Tom is not about zeroing ECX.  It is
about avoiding code bugs where AsmCpuid() is called for an Index
value that is documented to depend on ECX.  In this case, we would
want an error condition so the developer knows they should use
AsmCpuidEx() instead.

>From looking at the Intel SDM, there is a small set of Index
values that do not look at ECX at all.  We could consider
adding an ASSERT() condition in AsmCpuid() if Index is
a value that depends on ECX.  Perhaps in DEBUG_CODE() so
it is not always present.

Mike

> -----Original Message-----
> From: Ni, Ray <ray.ni@intel.com= >
> Sent: Friday, January 19, 2024 2:01 AM
> To: Kinney, Michael D <michael.d.kinney@intel.com>; Tom Lendacky
> <thomas.lendacky@amd.com= >; devel@edk2.groups.io; Gao, Liming
> <liming.gao@intel.com&g= t;; Liu, Zhiguang <zhiguang.li= u@intel.com>; Dong,
> Eric <eric.dong@intel.com>; Kumar, Rahul R <rahul.= r.kumar@intel.com>;
> Gerd Hoffmann <kraxel@redhat.c= om>; Ard Biesheuvel
> <ardb+tianocore@kernel= .org>
> Cc: Michael Roth <michael.r= oth@amd.com>
> Subject: RE: [edk2-devel] [PATCH 1/2] UefiCpuPkg/MpInitLib: Use
> AsmCpuidEx() for CPUID_EXTENDED_TOPOLOGY leaf
>
> Mike,
> I agree with your words after "However".
> Zeroing ECX in AsmCpuid() is confusing to future code maintainer: If > CPUID instruction does
> not consume "ECX", why is it needed to zero "ECX"?=
>
> Thanks,
> Ray
> > -----Original Message-----
> > From: Kinney, Michael D <michael.d.kinney@intel.com>
> > Sent: Friday, January 19, 2024 7:11 AM
> > To: Tom Lendacky <t= homas.lendacky@amd.com>; devel@edk2.groups.io;
> > Gao, Liming <liming.ga= o@intel.com>; Liu, Zhiguang
> <zhiguang.liu@intel.com>;
> > Dong, Eric <eric.dong@i= ntel.com>; Ni, Ray <ray.ni@in= tel.com>; Kumar,
> Rahul R
> > <rahul.r.kumar@inte= l.com>; Gerd Hoffmann <kraxe= l@redhat.com>; Ard
> > Biesheuvel <ardb+= tianocore@kernel.org>
> > Cc: Michael Roth <mich= ael.roth@amd.com>; Kinney, Michael D
> > <michael.d.kinne= y@intel.com>
> > Subject: RE: [edk2-devel] [PATCH 1/2] UefiCpuPkg/MpInitLib: Use > > AsmCpuidEx() for CPUID_EXTENDED_TOPOLOGY leaf
> >
> > Hi Tom,
> >
> > I do not see any harm in zeroing ECX in AsmCpuid().
> >
> > If it is not zeroed, then it would have an undefined value.
> >
> > However, calling AsmCpuid() for any Index that evaluates ECX
> > (including a check for 0) should never be done.  If ECX is > > evaluated for a given Index, then AsmCpuIdEx() must be used.
> >
> > Mike
> >
> > > -----Original Message-----
> > > From: Tom Lendacky <thomas.lendacky@amd.com>
> > > Sent: Wednesday, January 17, 2024 1:26 PM
> > > To: devel@edk2.group= s.io; Kinney, Michael D
> > > <michael.d.= kinney@intel.com>; Gao, Liming <liming.gao@intel.com>;
> Liu,
> > > Zhiguang <zhigu= ang.liu@intel.com>; Dong, Eric <eric.dong@intel.com>;
> Ni,
> > > Ray <ray.ni@intel.com= >; Kumar, Rahul R <rah= ul.r.kumar@intel.com>;
> Gerd
> > > Hoffmann <kraxel@red= hat.com>; Ard Biesheuvel
> > <ardb+tianocore@k= ernel.org>
> > > Cc: Michael Roth <michael.roth@amd.com>
> > > Subject: Re: [edk2-devel] [PATCH 1/2] UefiCpuPkg/MpInitLib: = Use
> > > AsmCpuidEx() for CPUID_EXTENDED_TOPOLOGY leaf
> > >
> > > On 11/28/23 08:35, Lendacky, Thomas via groups.io wrote:
> > > > On 11/6/23 17:15, Tom Lendacky wrote:
> > > >> On 11/6/23 16:45, Lendacky, Thomas via groups.io wr= ote:
> > > >>> The CPUID_EXTENDED_TOPOLOGY CPUID leaf takes a = subleaf as input
> > when
> > > >>> returning CPUID information. However, the AsmCp= uid() function
> does
> > > not
> > > >>> zero out ECX before the CPUID instruction, so t= he input leaf is
> used
> > > as
> > > >>> the sub-leaf for the CPUID request and returns = erroneous/invalid
> > > CPUID
> > > >>> data, since the intent of the request was to ge= t data related to
> > > sub-leaf
> > > >>> 0. Instead, use AsmCpuidEx() for the CPUID_EXTE= NDED_TOPOLOGY
> leaf.
> > > >>
> > > >> Alternatively, the AsmCpuid() function could be cha= nged to XOR
> ECX
> > > >> before invoking the CPUID instruction. This would e= nsure that the
> 0
> > > >> sub-leaf is returned for any CPUID leaves that supp= ort sub-
> leaves.
> > > >> Thoughts?
> > > >>
> > > >> Adding some additional maintainers for their though= ts, too.
> > > >
> > > > Any thoughts on this approach (as a separate, unrelated= patch) to
> > > > eliminate future issues that could pop up?
> > > >
> > > > Seems like zeroing out ECX before calling CPUID would b= e an
> > > appropriate
> > > > thing to do, but I'm not sure if that will have any imp= act on the
> > > existing
> > > > code base... it shouldn't, but you never know.
> > >
> > > Just a re-ping for thoughts on this.
> > >
> > > Thanks,
> > > Tom
> > >
> > > >
> > > > Thanks,
> > > > Tom
> > > >
> > > >>
> > > >> Thanks,
> > > >> Tom

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

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

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

_._,_._,_
--_000_MN6PR11MB8244774B15D9B80C2701DFEC8C772MN6PR11MB8244namp_--