From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mga12.intel.com (mga12.intel.com [192.55.52.136]) by mx.groups.io with SMTP id smtpd.web11.82722.1673604335520351610 for ; Fri, 13 Jan 2023 02:05:36 -0800 Authentication-Results: mx.groups.io; dkim=fail reason="unable to parse pub key" header.i=@intel.com header.s=intel header.b=BdWEbdV8; spf=pass (domain: intel.com, ip: 192.55.52.136, mailfrom: jiaxin.wu@intel.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1673604335; x=1705140335; h=from:to:cc:subject:date:message-id:references: in-reply-to:content-transfer-encoding:mime-version; bh=lM8HMEKw7XJzKpWkOHNcxMAB+eDvLcXPl05y3XOAjeY=; b=BdWEbdV8VMaQ4hxuQ3OaHRuTBL5xpocwCOWHItfwZUIw3uE2DTP9sFN0 5XL5endIVlN5tyJp89QDwx+wQNORr4iF+emnski+l1dNMZzeT0EZtOX/a CStA21JkbIIAYTvltDeQZc3Njq46WCr6tTZfiGt6RCtqag51y1jpyCxro lOrNMGIGlNYABruxsYfnfoD2pa7sov5dyFVjsyUftaL2hfLG36jL6yLKu 454JDOX8lwfJn/6SOe6qw6GXpaziLrFvvckS5ivNlD+Wle53JMI/hxSpE /dq+5o3MRoTis2VIqldi4P/Rf0s4GTVkzbKahu/tmtUvAwnmK9GRbr5PU Q==; X-IronPort-AV: E=McAfee;i="6500,9779,10588"; a="303658141" X-IronPort-AV: E=Sophos;i="5.97,213,1669104000"; d="scan'208";a="303658141" Received: from orsmga008.jf.intel.com ([10.7.209.65]) by fmsmga106.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 13 Jan 2023 02:05:34 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=McAfee;i="6500,9779,10588"; a="688686799" X-IronPort-AV: E=Sophos;i="5.97,213,1669104000"; d="scan'208";a="688686799" Received: from orsmsx601.amr.corp.intel.com ([10.22.229.14]) by orsmga008.jf.intel.com with ESMTP; 13 Jan 2023 02:05:34 -0800 Received: from orsmsx610.amr.corp.intel.com (10.22.229.23) by ORSMSX601.amr.corp.intel.com (10.22.229.14) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2507.16; Fri, 13 Jan 2023 02:05:34 -0800 Received: from orsmsx610.amr.corp.intel.com (10.22.229.23) 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.16; Fri, 13 Jan 2023 02:05:33 -0800 Received: from orsedg603.ED.cps.intel.com (10.7.248.4) 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.16 via Frontend Transport; Fri, 13 Jan 2023 02:05:33 -0800 Received: from NAM12-MW2-obe.outbound.protection.outlook.com (104.47.66.49) by edgegateway.intel.com (134.134.137.100) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.1.2507.16; Fri, 13 Jan 2023 02:05:33 -0800 ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=ARjhETYdyzYRjr/pzPiBWTBx/BR1XYGkLcbL8zPEEsILrB+VOWHEhmWCZC82+LZaBD2mHgjHnEH0j1Yp+PrqiMa8qZgka/qSxrk8TRD7a5LxM7MIRtUufM24T7+P/JVBXjDh+uuZmbxjKzMC8rmyToBHdwoe0Zf25kNQ4+0ywzeZECWv7wQnWU24Uwe0E1f2y0zenSZVjQgOwBKj346qu5mW1PuHm/R38vMMtzSfdY4Id5nfPbQ9WXdu4k8/rHrznMwZLLbqVtVEvFFWtomwmhhOx5gZ4UObrlgJxZFBLu7rTPaTB3pW4u8znpwjfc7jq6+zY6kMuEDaqVVAaxhDnQ== 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=S6mtQhKp0R27z9QfZh4ItBTxC7h4yf1Ar8SEqsHiYoU=; b=HuTl7fR2SxAqeqzjSiMDFsqSMjDBEQRXWLyrXFwU752ARWQg26o8KEyvJMffqrK33bBVKayRHu+sVsY9IDUANYblDdKPIpRqA9atwHtmZ8Ra9ObHqO42TUZbtym5jSbz2vsHQXi64SPB9yNxpE8mAKAs61PEmoAiEUgL7VONh1ar9GH37ZGhzfhJ1nQPei2QVVG3G026Yo+j0pAw0wA8/CxlNA/z+XLJIDa7ELc6PAm6ijceIUAPEQgRKR1Ym7fRzP3vYLfXHBZ5c0KjyHD8MKKqJVxD3cmSCT+Ps5PJ8H6RUcwsM7DR4DAFKJr9d2cOddnaSU6Beimk4PwFfg0c4A== 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 Received: from MN0PR11MB6158.namprd11.prod.outlook.com (2603:10b6:208:3ca::18) by CO6PR11MB5569.namprd11.prod.outlook.com (2603:10b6:303:139::20) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.5986.18; Fri, 13 Jan 2023 10:05:31 +0000 Received: from MN0PR11MB6158.namprd11.prod.outlook.com ([fe80::e5d9:d16e:172b:aa01]) by MN0PR11MB6158.namprd11.prod.outlook.com ([fe80::e5d9:d16e:172b:aa01%4]) with mapi id 15.20.5986.018; Fri, 13 Jan 2023 10:05:31 +0000 From: "Wu, Jiaxin" To: "Ni, Ray" , "devel@edk2.groups.io" CC: "Dong, Eric" , "Zeng, Star" Subject: Re: [PATCH v2] UefiCpuPkg: Support SMM Relocated SmBase handling Thread-Topic: [PATCH v2] UefiCpuPkg: Support SMM Relocated SmBase handling Thread-Index: AQHZJZelFPLHb92/90qeH4EA7KAwGq6b64XQgAA1V9A= Date: Fri, 13 Jan 2023 10:05:31 +0000 Message-ID: References: <20230111083507.8792-1-jiaxin.wu@intel.com> In-Reply-To: Accept-Language: zh-CN, en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: authentication-results: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=intel.com; x-ms-publictraffictype: Email x-ms-traffictypediagnostic: MN0PR11MB6158:EE_|CO6PR11MB5569:EE_ x-ms-office365-filtering-correlation-id: 87e76e5a-9a41-4099-7992-08daf54db491 x-ms-exchange-senderadcheck: 1 x-ms-exchange-antispam-relay: 0 x-microsoft-antispam: BCL:0; x-microsoft-antispam-message-info: f4m0QNvP853DrsBhPy0TFiySf4pR/uDEjctudv88DHwkul5APY4J2n8F8aeMFEFhwZ+J45qN1rKM8qdmY4+Xk9WPXK9PzpAIpLaCNSrB8RaHI7tscDCIKUNapS4JHYSXFArTEM6GP7ZpI3gwYYi7RWyiJBmeLhz5ZTbjx7KMbBryZz/lD67LHE7oo6lC2HBhvHhjH1ndYx0+AlFMmQzvO/4LDV2gjKvcwYpIAkwHzatfTgva5Y+7EB9pu21x1aLChLu0p7Gl1X+B51LcQ1QyUFTgaEHNP9ucvFWFPuTXyoCtmEJlPgkhP/n4vf5nyPdJ2YrhHaeFq09gzwMMsUFjFWZVQ89fzhDbt+inzMuPJ/DdDql1jj+ghDKQh+o2sARPdv6Z9rwflgToZVvTjzzjge1M0E+PFTrQi34G63Q2R9YQEbsT5OfKJ1plMBPdEZIyXrp5t3TsmzF5Bz4uVnDueFXqw65jAizuIjj/tAVWukLoAhGa6oF51BPu0s/9LbDbxDQrATxaE4oPpbxizM89c7lp5dBUkUYVWscmqhAuuVOBs+XGUal/oWkiaP5nFSug9H2jMn/cigdxakEZGJ5yNQyXJHX53E8Y/ZyKsVjTbjuB/fLhA1I28Bk0lfo2z0Kc/BBoDam1HbJrhBpqYBJWsXjbnjhZhf5PDNJZdqAalN4meEo1kow20KEyhwnhU+68ikH57gB4TJ+gF5atiD5n2A== x-forefront-antispam-report: CIP:255.255.255.255;CTRY:;LANG:en;SCL:1;SRV:;IPV:NLI;SFV:NSPM;H:MN0PR11MB6158.namprd11.prod.outlook.com;PTR:;CAT:NONE;SFS:(13230022)(6029001)(396003)(136003)(366004)(346002)(39860400002)(376002)(451199015)(66946007)(66476007)(7696005)(66556008)(64756008)(8676002)(76116006)(316002)(4326008)(54906003)(38070700005)(110136005)(2906002)(71200400001)(8936002)(26005)(66446008)(41300700001)(5660300002)(83380400001)(52536014)(82960400001)(107886003)(33656002)(478600001)(6506007)(38100700002)(122000001)(55016003)(186003)(86362001)(9686003);DIR:OUT;SFP:1102; x-ms-exchange-antispam-messagedata-chunkcount: 1 x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?GY2arz7ka2MU/qQrvewISchmX5MjfYzK8FPV447tSVX9uKo/raEUZ6BnqtuN?= =?us-ascii?Q?33bU6gigaJj7hvSgobzh/a+m4+/eMehLCo9zU1g1W32RTgJGcCTmXO2o9Cd4?= =?us-ascii?Q?MqeQIFTuLxyz9JvjqeIPn68q+ZDnweBgA9Tk7Q7qymLjv3shpoOuX6hM3nQh?= =?us-ascii?Q?CTcCx/x2C4MG6NQYabywvyzWLC2ijmlL+ABca2GEjHatqRYcXO0NX4EV5lJk?= =?us-ascii?Q?1/T8TEnrppJ3t64TL8fczB70hYeAoJ/nYazWP3LVlE/urzbp0wRpmPWflo7A?= =?us-ascii?Q?Q0DnodFv/NsgvE0Es2YPeqPYfr+lwb2RDM4snF3UtCCaBBvazoWsGKGh+KVM?= =?us-ascii?Q?P+zXgVrB5cFl0spzONen5MgEy8QL2DJRoB+/K11MdTO1U9UDUBTK4ALCvzt7?= =?us-ascii?Q?wcWvy9uGsF58n4/CMQ/oXS7F4T0Ijw5hzTbBPr1y1G6yhJ37It1CH9H4KrUp?= =?us-ascii?Q?sf/jivDG90AQa2YksyYZmH9C8J2Jhxv9C/MTxNs7bxTNIAp+2MjOwzWT257p?= =?us-ascii?Q?ejcSYSH/ki6FNZm+UO7mh1a1dTd+WHlR41AB3EIt+WqdWXWzdqHOy/RzJkEI?= =?us-ascii?Q?1i1o+pNoFyKUFsAmp6LcXwiF/O5196dmB4o9oe7+9xWJ0GT00jc2pa6vRNqK?= =?us-ascii?Q?dfMFRT4NssFDFmS55t+6vp5FKXl7P2ii1MtU/Gm9k/UXzo4CZ6mRAX38cLlV?= =?us-ascii?Q?FYPZM7lwhu9ZH1BQ2oB7Zv9rJXuUur0Idm+qK6165NLzfanLvenrhIe06Avv?= =?us-ascii?Q?eg1Bu+4fz4BbmkUvQtiy1TFEZWxAtw95jZLTkPYmg3Q3lz93JHV7XGctJJay?= =?us-ascii?Q?vinz4ZkfhlE/x0qHLRT6Xv/MVzKp7chMr6UFmAbcPKEMo3WXzgc24ApKq98r?= =?us-ascii?Q?KPP2P71yj74OBDa3GBtGA2zuH5XbS5G9JKlkgYQtQlEnGtLNTyqDMmHsFvCP?= =?us-ascii?Q?4XR8meHG/cEIGeA6uWcFnHayMqyFSGRINZWjqZmqfOcH/85IC9Plb+8yPCvI?= =?us-ascii?Q?zSyYTfIFKMnoeUnWw9yUKCXGHwn388JoMzM9WkiePAxy7/ojXUwSfdplKB/Y?= =?us-ascii?Q?MhgBP2gwqCvOmFa/AHoIgjy3B/GycCz3ofMqD7t0BhTBf3+gTa/3dgZIn7xe?= =?us-ascii?Q?3vmhDwinWokPy1l2f0QfK1LsOk4czzwaYlTPfSXtGh7ZGKphCAaQsz6RSDMo?= =?us-ascii?Q?czMC9aYwJQls/G3JJxmJfe9uWkQSWwM/AXJbryCsDa/Qq6T01QG2WzjXpcWi?= =?us-ascii?Q?/eOU73uGAgX0QgS56lMkX+gq4drG/eJ+3xek5EfMwlHl3Slc44P81zURePXh?= =?us-ascii?Q?76XeV9RxrO6SQZFDrOdsIzV/+TLo7uAppSr7Y3DBf83qh3GEEKYSpA+3AkVX?= =?us-ascii?Q?bmDSW1nbiyA3wpJIVH7zKCrXDuuaRl0yhQP4A9aEcICQmH8svxo0qNKz2eqM?= =?us-ascii?Q?ur0vKWVygE/YAxPOCviAOHwCZWKqsSL62TiVqHAXayD6RslQ72g9nQ9TNzko?= =?us-ascii?Q?voRKgXd+ynu37qPwtSEhDoTaI6/3WNRb1tn6YKV8OlTPOzpGCr2uvGilDdTj?= =?us-ascii?Q?m7SPFF7Yt4uicJUVcbmvoNQAv7hpVOWerYCmcq02?= 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: 87e76e5a-9a41-4099-7992-08daf54db491 X-MS-Exchange-CrossTenant-originalarrivaltime: 13 Jan 2023 10:05:31.6909 (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: hA59KkN5OR7IIMuNhGvicS9/+RfsIjvHJxpzAfg4kDYk6A9krIauVOlLyHU6XUZcS07ReKGwDDOjUvhS4B5k9g== X-MS-Exchange-Transport-CrossTenantHeadersStamped: CO6PR11MB5569 Return-Path: jiaxin.wu@intel.com X-OriginatorOrg: intel.com Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Thanks ray, below are really great feedback. I have checked all and respons= e as below: > The default SMBASE for the x86 processor is 0x30000. When SMI happens, > CPU runs the > SMI handler at SMBASE+0x8000. Also, the SMM save state area is within > SMBASE+0x10000. >=20 > One of the SMM initialization from CPU perspective is to program the new > SMBASE (in TSEG range) > for each CPU thread. When the SMBASE update happens in a PEI module, > the PEI module shall > produce the SMM_BASE_HOB in HOB database which tells the > PiSmmCpuDxeSmm driver which runs > at a later phase about the new SMBASE for each CPU thread. > PiSmmCpuDxeSmm driver installs > the SMI handler at the SMM_BASE_HOB.SmBase[Index]+0x8000 for CPU > thread Index. > When the HOB doesn't exist, PiSmmCpuDxeSmm driver shall program the > new SMBASE itself. >=20 Agree, added! >=20 > 2. Can you write the code as "mSmmRelocationDone =3D (BOOLEAN) > (GetFirstGuidHob (&gSmmBaseHobGuid) !=3D NULL)"? > It removes the assumption that the initial value of mSmmRelocationDon= e is > FALSE. > I understand it's TRUE usually. But it depends on the PE loader. Agree. >=20 > > + if (GetFirstGuidHob (&gSmmBaseHobGuid) !=3D NULL) { > > + mSmmRelocated =3D TRUE; > > + } else { > > + mSmmRelocated =3D FALSE; >=20 > 3. The above code doesn't assume the initial value of global variable. Go= od. > Can you align the variable name between SmmCpuFeaturesLib and the > PiSmmCpuDxeSmm driver? > If the two names are chosen to fix link error, there are two ways to = avoid > the error: > a. Add "static" and both component use mSmmRelocated. > b. One can be renamed as mSmmCpuFeaturesSmmRelocated. No change > to the one in driver. >=20 Agree.=20 > > + } > > + > > + // > > + // Check whether Smm Relocation is done or not. > > + // If not, will do the SmmBases Relocation here!!! > > // > > - SmmRelocateBases (); > > + if (!mSmmRelocated) { > > + // > > + // Restore SMBASE for BSP and all APs > > + // > > + SmmRelocateBases (); > > + } else { > > + mSmmInitialized =3D (BOOLEAN*)AllocateZeroPool (sizeof (BOOLEAN) * > mMaxNumberOfCpus); > 4. I guess mSmmInitialized is already allocated in normal boot phase. Her= e > what we need is only to set all > elements to FALSE. Will keep reviewing following changes and confirm my > guess. > But we still cannot call Allocate in every S3 boot without freeing. I= t may > cause all SMRAM be allocated. >=20 Yes, I checked the detailed, we don't need allocate that. >=20 > > + ASSERT (mSmmInitialized !=3D NULL); > > + > > + mBspApicId =3D GetApicId (); > > + > > + // > > + // Issue SMI IPI (All Excluding Self SMM IPI + BSP SMM IPI) for SM= M init > > + // > > + SendSmiIpi (mBspApicId); > > + SendSmiIpiAllExcludingSelf (); > > + > > + // > > + // Wait for all processors to finish its 1st SMI > > + // > > + for (Index =3D 0; Index < mNumberOfCpus; Index++) { > > + while (mSmmInitialized[Index] =3D=3D FALSE) { > > + } > > + } > 5. I am not sure if the same logic is also needed in normal boot. So, may= be a > local function can be created to > reduce the code duplication? Ok, will define new API to reduce the duplication. >=20 >=20 > > + if (!mSmmRelocated) { > > + IsMonarch =3D mIsBsp; > > + } else if (mBspApicId =3D=3D ApicId) { > > + IsMonarch =3D TRUE; > > + } >=20 > 6. How about removing mIsBsp and always use mBspApicId even when > mSmmRelocated=3D=3DFALSE? > 7. How about renaming IsMonarch to IsBsp? Agree. >=20 > > > > - if (mIsBsp) { > > + if (!mSmmRelocated) { > > + if (mIsBsp) { > > + // > > + // BSP rebase is already done above. > > + // Initialize private data during S3 resume > > + // > > + InitializeMpSyncData (); > 8. Because the same function is already called in driver entrypoint, here= the > call is for s3 path. > Can you check if we need to call it as well in S3 path when SmmReloc= ated is > TRUE? Yes, we need that in S3, will handle it. >=20 > > + if (TileSize > SIZE_8KB) { > > + DEBUG ((DEBUG_ERROR, "The Range of Smbase in SMRAM is not > enough -- Required TileSize =3D 0x%08x, Actual TileSize > > =3D 0x%08x\n", TileSize, SIZE_8KB)); > > + ASSERT (TileSize <=3D SIZE_8KB); >=20 > 9. I suggest we add CpuDeadLoop() here to capture the error in release bu= ild. Agree.