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 41A9D940269 for ; Thu, 7 Dec 2023 00:38:20 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=DO8wC3NP5YR5yEA8A1UwUIfz4mZsaQ8XUUHJlaQOMfQ=; 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=1701909498; v=1; b=kTGg7m2GS94R8VCGH4SRzeJbRN9vB3v+VZm69n+4n8hHfRjT8DE5B7iGhY4e318h6TzVjhxJ v+re1tYID2+WDb7r9hcXCVxX8jhl2PBhR0pvuiX3Dq3KTFBirv6pC08iHL+35N/smjgJ0qZREi4 VBGmcLCYFYKosvhqBb1UQSc8= X-Received: by 127.0.0.2 with SMTP id 8T4FYY7687511xmHU0LJRDjC; Wed, 06 Dec 2023 16:38:18 -0800 X-Received: from mgamail.intel.com (mgamail.intel.com [198.175.65.11]) by mx.groups.io with SMTP id smtpd.web10.72501.1701909498128798193 for ; Wed, 06 Dec 2023 16:38:18 -0800 X-IronPort-AV: E=McAfee;i="6600,9927,10916"; a="1022873" X-IronPort-AV: E=Sophos;i="6.04,256,1695711600"; d="scan'208";a="1022873" X-Received: from fmsmga005.fm.intel.com ([10.253.24.32]) by orvoesa103.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 06 Dec 2023 16:37:58 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=McAfee;i="6600,9927,10916"; a="1103011907" X-IronPort-AV: E=Sophos;i="6.04,256,1695711600"; d="scan'208";a="1103011907" X-Received: from orsmsx602.amr.corp.intel.com ([10.22.229.15]) by fmsmga005.fm.intel.com with ESMTP/TLS/AES256-GCM-SHA384; 06 Dec 2023 16:37:55 -0800 X-Received: from orsmsx612.amr.corp.intel.com (10.22.229.25) 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.35; Wed, 6 Dec 2023 16:37:54 -0800 X-Received: from ORSEDG602.ED.cps.intel.com (10.7.248.7) 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.35 via Frontend Transport; Wed, 6 Dec 2023 16:37:54 -0800 X-Received: from NAM11-BN8-obe.outbound.protection.outlook.com (104.47.58.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.35; Wed, 6 Dec 2023 16:37:52 -0800 ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=Km1zvSbNNo6I0q/LZq2U0Z31DwYlnHzYuA1Oz+j0bA3G8h/wXfgnCzreGFK7Nlw0jbgM6ANjgUb1PwjS594W1hem8H7HCznMxgQbNPGdSvW0snreUdT8+VHkvMekOU6OTUCXhhw/0EhiJD85BmSZJgoqz15nXtl/YkZEnPPXtkovYDMsZ8+h4hp6btvit7p0VTVNhAtaaoZoiQROEfA2nHCkY4uf09dziiQhE/N2RkW+sG6OK7w9QVCoHPu0Gi2YOeNfON+p33ovifBqvMSkvAySHQ3o5e2CTuW6w/yCjcCjAyx15v7Pdv8RZGNYI8FYF7mhNCvW23/2ORrD/tffcA== 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=cy8K9yk225DSSOkmpbN9QYr00WKxhWyXJHggsfzau5w=; b=VZU57GBA1YM7mOEmO/YzJxU2pGs3PqGv/m8DTnmwB6sZuis5FUQsECv2j1mAa8fH4KijI8YOq5XRVOPmQnoRc6r2GTpPnoiXLISp/lVh09knYj2+PpUazKL9u+vwdkjH3Who67EUEM4Ecf1SscZKBHALGAXLzLesztgTdW/brO42lzSeyrPCQlkQf249o8LIoJkwxwxH/6/v1XocWFB8cCwPPqQhxu9AuK5r1OwrACSJaGmtDhsmznrMNMglOKtwC9QXnzCEcfOKBbP5T3ZUFz+aWabtFmR3ltxfA2xqiGheSiMeooyCwbuTXYPrg5o55e3Vf3Q43vs7I2BFmAMDqQ== 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 BN9PR11MB5483.namprd11.prod.outlook.com (2603:10b6:408:104::10) by DS0PR11MB8116.namprd11.prod.outlook.com (2603:10b6:8:12b::11) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.7068.27; Thu, 7 Dec 2023 00:37:50 +0000 X-Received: from BN9PR11MB5483.namprd11.prod.outlook.com ([fe80::6da1:a4b7:4771:14e1]) by BN9PR11MB5483.namprd11.prod.outlook.com ([fe80::6da1:a4b7:4771:14e1%5]) with mapi id 15.20.7068.025; Thu, 7 Dec 2023 00:37:50 +0000 From: "duntan" To: "Ni, Ray" , "devel@edk2.groups.io" CC: "Dong, Eric" , "Kumar, Rahul R" , Gerd Hoffmann Subject: Re: [edk2-devel] [PATCH 6/6] UefiCpuPkg: Avoid assuming only one smmbasehob Thread-Topic: [PATCH 6/6] UefiCpuPkg: Avoid assuming only one smmbasehob Thread-Index: AQHaJz7WWrojerSTqUuFdFtB2LzLs7CcCEsQgADwb3A= Date: Thu, 7 Dec 2023 00:37:50 +0000 Message-ID: References: <20231205054900.926-1-dun.tan@intel.com> <20231205054900.926-7-dun.tan@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: BN9PR11MB5483:EE_|DS0PR11MB8116:EE_ x-ms-office365-filtering-correlation-id: 7d9a15f8-1780-4919-ede1-08dbf6bcbddb x-ms-exchange-senderadcheck: 1 x-ms-exchange-antispam-relay: 0 x-microsoft-antispam-message-info: 2fwbB3S8KtzbBSX/GbdOTpJif5mjA3Xue5jGyQAx5irQ+IJmU+Z5QSc05s1qVEDZdWY+09isgQEtRrbBG+G5turL9ELPdyWsxVRE1N5DLl3CNAixqqBzn/aN4G6UzVe6+AmSfcaWJLFz55gMtE7Ivt5msH1lzZ5SY5wHWFvE/3SPYhR16Jwh/I698cVWo/r9sVrWAKY56W5p737al5Zs0ezWdJnCCptqP2w4+MpahHlPNCEbfzc1HLaCoBqbYJCeXX28X3L4XurvrcYeoX4QWXvltggRYSFeA7YotGGVDKR0YmJu8izBrR5wnVvgkMUVPZFqy3zGeQpd557ZE7UAtH31u6rCm3wRTCfSbhbSDkSwulYhBc0rzTLubsBA1SU6LBWyeL3MORhtxVNaSUz2q28MsG8wY+Ou9wk7fLAxf/hGSIUnh89N3bQhZupaQLqLmHqPFb8rixRGY4DAq84ss98dpFD0YHei/6D+GL41JN1YwyaJ1MjaiOcK3cozv4fjfIGQduGrGEqWKIu7ERDoNE1Y1mN0+i+WnKIzyH+zQB17i7CyYMfoawczx6Tb1CCVu61OZ2zVBCIC6RmBMR5fr47HojvU2MSUKKrv55oeHrqMPo99lTfj0raDFx4weo4i x-ms-exchange-antispam-messagedata-chunkcount: 1 x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?wN7QBaMORN4S/bkDDX1FQVcMqEDwWCY2myGxVhMawqfMhnij1uZzQI80Qwcr?= =?us-ascii?Q?PoaXcdQfgzt/jC4n+Hw6ERT15s20Cag3UbympxKcyZBiAdAOz2X21NsP51rN?= =?us-ascii?Q?ZqNEbwBuRHrd3/71LKPcvTO+jAm1U/J0dtWpAqvgRalMUOn5IYEfgtPpynzH?= =?us-ascii?Q?UdX11cBeKmfu1eohtvOUmu8h9olJrTAaAFW95hal2UaLWMOUrLynV1lFLpdb?= =?us-ascii?Q?zi76ZmYh46TjKjxb7gyejWoVXbbgW4w21jcBTjc0GUy7yfY5kNlj96DJv8Il?= =?us-ascii?Q?1kHeyK9TBXy8DL7hEb/ZNklj08J5nZ9gM5wIY1xAYg3LyCPEKItxxi19cB9M?= =?us-ascii?Q?vW8WM2fk1i4f/JTBtlYZhobAVNemNwJ8nPaL3wBTgMSsgB94wckKsjwiyROB?= =?us-ascii?Q?j09gTNsRH+8pV0RZVYxR+fFxRr76w5hOYkLRfjVrC58mtn9fOMVhAG+h1yq1?= =?us-ascii?Q?0zXbjiq6TFWNrtXVi5zzJUPpSzWXZmU8yX0OkrHaXbioEQGsVe8uc/soK+CJ?= =?us-ascii?Q?+7/ZwpRLUAj4d0EJ5mkEtB2oWwEU4BLBq81ahoSTeV9VASEmaTZRj7pbSu9W?= =?us-ascii?Q?cRwNQjA01+hfaHUOZfPVJIlNxvhDkdgW6HoW/b1MKPguNLc6EX1F/Uy+fora?= =?us-ascii?Q?AtH738xDfUE8JUh4mWhQTR4APe4g7II/+e5NzklR9YaaRJwzNONTc/axBgVc?= =?us-ascii?Q?ESAuQdBDgOA9Zc+L0Zi3yvjiCLGmUoEqoSmS4jcIiOeKTuf0RIPODR+F9jv1?= =?us-ascii?Q?N4m9qPu7KtEE83zpbqFmP+5A9/ve7MFHaQfRHrtqPu4wvFPD6JPfVvdXua88?= =?us-ascii?Q?VErnD8q9y86ZV+QmJ58r9Nc7bwEY2F9VDULen2Vm2IpaKuYAgU//vJoYwHFu?= =?us-ascii?Q?cbGrDwOeraC+mtU3tB1RH3CWgr/0hjha3Vj3kJhy+cFTV2iiUmID6bsERqsf?= =?us-ascii?Q?eZJGI65nTis7g15sPjGzwXsgU8X2ua4J30rZbwJ9FK9O3XU7HViRvl6O76eI?= =?us-ascii?Q?+gzcqeawvfgqwJs+/nz5Dxc9iBjyPxfXRqHbaehO5lVBpWADYA06zVnTj+F0?= =?us-ascii?Q?Tu4CiXAiiYI8jLA4eKUC0mOUuPFhPBixKkKXfso7c6D6/Rbz58kxIocusDGD?= =?us-ascii?Q?8TG00WSJvJNJUD4O1ipioaEqZNWhLXbqMxARa/PM6rqWPxqND8aPCaFzcM01?= =?us-ascii?Q?GI6qU6baV0i1Js2b6ogDp2tUpBQlzP3Jf7JQRGSXRf8Y/7dOT6ljntyUE+2z?= =?us-ascii?Q?FmPJLsvikdwq5xp2uBryY27fuIYIxvNZY1XkwH4aK1/tczkSnBnDCOAqUMd+?= =?us-ascii?Q?XhbyzrqVSplUS7NfwceigDBKiwVK1BpG/4K5uP01xOMxmKvomkmzqRIQaff+?= =?us-ascii?Q?ZKr2Av7kJOqqYbncnLFyfWsjw/jwbRmqX/TGi8LbSJtnrlGVfKCf9XS//KQ2?= =?us-ascii?Q?P5P0OYGHGQQ9CmcaYyL0wFyQBFfpeQPPBQyUKlGuiLqP/tsCAUEPo/NE/LQY?= =?us-ascii?Q?BkPFwPTGi0aD9O5teeWlI9fTjRWQO+tQrh4Yiyzz8rSwaNX3VdYBN3KdBcwg?= =?us-ascii?Q?GCDZBpvsHrbxXwMLQEM=3D?= MIME-Version: 1.0 X-MS-Exchange-CrossTenant-AuthAs: Internal X-MS-Exchange-CrossTenant-AuthSource: BN9PR11MB5483.namprd11.prod.outlook.com X-MS-Exchange-CrossTenant-Network-Message-Id: 7d9a15f8-1780-4919-ede1-08dbf6bcbddb X-MS-Exchange-CrossTenant-originalarrivaltime: 07 Dec 2023 00:37:50.2016 (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: 7+jj8rV12QuXFDg/yjbE6Z1K47EiEjUSx0dCl4K6Ik1qn18Yr5bv2TXCnVD846v1B9vYPG4ndKy8XwdvTdBDGQ== X-MS-Exchange-Transport-CrossTenantHeadersStamped: DS0PR11MB8116 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,dun.tan@intel.com List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: X-Gm-Message-State: N6BtiQVdqYGM15TtfGItlkawx7686176AA= 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=kTGg7m2G; 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 Updated in original message. Thanks, Dun -----Original Message----- From: Ni, Ray =20 Sent: Wednesday, December 6, 2023 6:15 PM To: Tan, Dun ; devel@edk2.groups.io Cc: Dong, Eric ; Kumar, Rahul R ; Gerd Hoffmann Subject: RE: [PATCH 6/6] UefiCpuPkg: Avoid assuming only one smmbasehob > +EFI_STATUS > +GetSmBaseFromSmmBaseHob ( > + IN EFI_HOB_GUID_TYPE *FirstSmmBaseGuidHob, > + IN UINTN MaxNumberOfCpus, > + OUT UINTN **SmBaseBufferPointer > + ) 1. It's a bit strange that caller should locate the first GuidHob. Can you update the existing code as follows: mCpuHotPlugData.SmBase =3D GetSmBase(mMaxNumberOfCpus); if (mCpuHotPlugData= .SmBase !=3D NULL) { mSmmRelocated =3D TRUE; } Dun: Ok, will change code to this. > +{ > + UINTN HobCount; > + EFI_HOB_GUID_TYPE *GuidHob; > + SMM_BASE_HOB_DATA *SmmBaseHobData; > + UINTN NumberOfProcessors; > + SMM_BASE_HOB_DATA **SmBaseHobPointerBuffer; > + UINTN *SmBaseBuffer; > + UINTN Index; > + UINTN SortBuffer; > + UINTN ProcessorIndex; > + UINT64 PrevProcessorIndex; > + > + SmmBaseHobData =3D NULL; > + Index =3D 0; > + ProcessorIndex =3D 0; > + PrevProcessorIndex =3D 0; > + HobCount =3D 0; > + NumberOfProcessors =3D 0; > + GuidHob =3D FirstSmmBaseGuidHob; > + > + while (GuidHob !=3D NULL) { > + HobCount++; > + SmmBaseHobData =3D GET_GUID_HOB_DATA (GuidHob); > + NumberOfProcessors +=3D SmmBaseHobData->NumberOfProcessors; > + GuidHob =3D GetNextGuidHob (&gSmmBaseHobGuid, > GET_NEXT_HOB (GuidHob)); 2. We could break the while-loop when NumberOfProcessors equals to the valu= e we retrieved from MpInfo2Hob. Right? This can speed up the code when there are lots of HOBs after the last SmmBa= seHob instance. Dun: If the code flow break before finding all potential SmmBaseHob instanc= e, there may be more SmmBaseHob instance covering NumberOfProcessors more t= han the expected value. The code is to catch this case. Do you think we sho= uld also catch this? > + } > + > + ASSERT (NumberOfProcessors =3D=3D MaxNumberOfCpus); 3. ASSERT may fail when HotPlug is TRUE? Dun: If HotPlug, I think the SmBase count should be PcdCpuMaxLogicalProcess= orNumber instead of the NumberOfProcessors extracted from MpInfo2Hob? > + > + SmBaseHobPointerBuffer =3D AllocatePool (sizeof (SMM_BASE_HOB_DATA *) > * HobCount); 4. SmBaseHobPointerBuffer -> SmBaseHobs Dun: will change the naming. > + for (Index =3D 0; Index < HobCount; Index++) { > + // > + // Make sure no overlap and no gap in the CPU range covered by=20 > + each > HOB > + // > + ASSERT (SmBaseHobPointerBuffer[Index]->ProcessorIndex =3D=3D > PrevProcessorIndex); 5. similarly, can you move "PrevProcessorIndex" assignment to just above "f= or"? Dun: Will change the code > + > + // > + // Cache each SmBase in order. > + // > + if (sizeof (UINTN) =3D=3D sizeof (UINT64)) { > + CopyMem ( > + SmBaseBuffer + PrevProcessorIndex, > + &SmBaseHobPointerBuffer[Index]->SmBase, > + sizeof (UINT64) * > SmBaseHobPointerBuffer[Index]->NumberOfProcessors > + ); > + } else { > + for (ProcessorIndex =3D 0; ProcessorIndex < > SmBaseHobPointerBuffer[Index]->NumberOfProcessors; ProcessorIndex++) { > + SmBaseBuffer[PrevProcessorIndex + ProcessorIndex] =3D > (UINTN)SmBaseHobPointerBuffer[Index]->SmBase[ProcessorIndex]; > + } > + } 6. I don't like the "if-else" above. Can you just change SmBaseBuffer to UI= NT64 *? Or, you always use for-loop to copy SmBase value for each cpu. Dun: Ok, will always use for-loop to copy SmBase value for each cpu. > + > + PrevProcessorIndex +=3D > SmBaseHobPointerBuffer[Index]->NumberOfProcessors; > + } > + > + FreePool (SmBaseHobPointerBuffer); > + > + *SmBaseBufferPointer =3D SmBaseBuffer; 7. Similarly, how about return SmBaseBuffer? Dun: Ok, will change the code -=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 (#112148): https://edk2.groups.io/g/devel/message/112148 Mute This Topic: https://groups.io/mt/102987142/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-