From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mga05.intel.com (mga05.intel.com [192.55.52.43]) by mx.groups.io with SMTP id smtpd.web10.81040.1673596349590493049 for ; Thu, 12 Jan 2023 23:52:29 -0800 Authentication-Results: mx.groups.io; dkim=fail reason="unable to parse pub key" header.i=@intel.com header.s=intel header.b=HSD6yqGi; spf=pass (domain: intel.com, ip: 192.55.52.43, mailfrom: ray.ni@intel.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1673596349; x=1705132349; h=from:to:cc:subject:date:message-id:references: in-reply-to:content-transfer-encoding:mime-version; bh=S1C/7xZtbe6w52bCcD5EMp2S1AwbOdaG8WZqxb0GYR8=; b=HSD6yqGibBsV1xyydldXdXM2+xxOXKa7Zm8vQIfWUiue8OjIsGcHo31u IaD+Ucm1rPZPRZm1IAaD2TFLrzY66rw6ltWhU4ZOXZagtuet555tG0lQq pX27h3rA1QeA12vMz7pMs8QN3KAHgmepsqTyKPPNuwwyK9NkOO4CSNu/Y doW+O7izntPD/4UUXXZ3Fzy7Gt9Qy0HU7JFyRho6A4R2V810deo0ALQQo 99goMqAgXREVZa68l3V6OFT7gYH2SlsQ3sVFJDW9RYod7us5haNJyQ63J Xa+0PK0kP+X0mddm+JpX/kB4e5c9eQW60nFs7zCeM73U3YWeY4VgnI6b0 A==; X-IronPort-AV: E=McAfee;i="6500,9779,10588"; a="410175218" X-IronPort-AV: E=Sophos;i="5.97,213,1669104000"; d="scan'208";a="410175218" Received: from orsmga008.jf.intel.com ([10.7.209.65]) by fmsmga105.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 12 Jan 2023 23:52:29 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=McAfee;i="6500,9779,10588"; a="688655586" X-IronPort-AV: E=Sophos;i="5.97,213,1669104000"; d="scan'208";a="688655586" Received: from fmsmsx602.amr.corp.intel.com ([10.18.126.82]) by orsmga008.jf.intel.com with ESMTP; 12 Jan 2023 23:52:28 -0800 Received: from fmsmsx612.amr.corp.intel.com (10.18.126.92) 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.16; Thu, 12 Jan 2023 23:52:28 -0800 Received: from fmsedg601.ED.cps.intel.com (10.1.192.135) by fmsmsx612.amr.corp.intel.com (10.18.126.92) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2507.16 via Frontend Transport; Thu, 12 Jan 2023 23:52:28 -0800 Received: from NAM11-DM6-obe.outbound.protection.outlook.com (104.47.57.174) by edgegateway.intel.com (192.55.55.70) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.1.2507.16; Thu, 12 Jan 2023 23:52:27 -0800 ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=Q2Urn1t7zoiLLJ986H0QquxnGf+gzM4VSONotxIAEQx4uSSBRj4pQoA1Mz2CfOSbdELHyniRz7y5uHO3KqIdBPktF/56pcvMT4OP6NECYiPLCSosXefRWiH74Q4CtMYsE5HPkYu50CSHid8fudr8O78wg1EGRlPC2WwjFMvdTFIYbwggnJ1knalhRPe89M2UfxtOIUqtEolhtpBEm6spl1o2m0brcjeF09cr+gsLlN5B928tk/hXgRWrdbBc3O2RB6NkItVCLpD7tnjxXwA8gn61A8Eahp1gh3343/a9ybOsKdB4kmldAtK4X7FWWAG8zHhyc2nrIT1aYJP3Y62FYg== 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=x63RsZc650zDMZTx81ne+hFZb9nX8NslY9PEohmUv3g=; b=EIDIJMWGLMz36KpJlmiiQX6v0CFbxJMKqjvM0GbZrTU9LLQ/GxRJK68C2jbG1/4rpnv+o/1duyFcSkSDY+/yzFdjUaV4v2GC88we982PkpFfx8+MSWCILw5XOxx9Qa9ZmDcwims7hRxN+vZOruF8bSfSBPbPyCJhPOZniHG7lWHdnqZf+KTS7Uixg2MJ1e35Ui6rCUUmv5avEJxyMKq5vG4quk56SvC+4Yse2czaC86j/TmThtH8rXNuf0gQOFUjGUPbwW/V49jeCqDBY7miEwnQ4s551+p6rI1xvU2+t1wbf59eF72iW7MheZB+tXcex1oz8+vOJHYQWKtb34KFxg== 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 MN6PR11MB8244.namprd11.prod.outlook.com (2603:10b6:208:470::14) by SJ0PR11MB5182.namprd11.prod.outlook.com (2603:10b6:a03:2ae::13) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.6002.13; Fri, 13 Jan 2023 07:52:20 +0000 Received: from MN6PR11MB8244.namprd11.prod.outlook.com ([fe80::f890:e4ec:e2d8:5831]) by MN6PR11MB8244.namprd11.prod.outlook.com ([fe80::f890:e4ec:e2d8:5831%3]) with mapi id 15.20.5986.018; Fri, 13 Jan 2023 07:52:20 +0000 From: "Ni, Ray" To: "Wu, Jiaxin" , "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/90qeH4EA7KAwGq6b64XQ Date: Fri, 13 Jan 2023 07:52:20 +0000 Message-ID: References: <20230111083507.8792-1-jiaxin.wu@intel.com> In-Reply-To: <20230111083507.8792-1-jiaxin.wu@intel.com> Accept-Language: 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: MN6PR11MB8244:EE_|SJ0PR11MB5182:EE_ x-ms-office365-filtering-correlation-id: d62c98ab-ee90-469d-7baa-08daf53b199f x-ms-exchange-senderadcheck: 1 x-ms-exchange-antispam-relay: 0 x-microsoft-antispam: BCL:0; x-microsoft-antispam-message-info: SEgR65rmtRtirX/oA5ApRp6buq5gYqtVus8ntE5tnhuIBJJzZw5US6P4gVR29byKpKMqrkFwiTXXq91juU41bG1SXK8nGz9h0hnRt5VjU+eW8H+fH2SwircmpkBlel7YgAPbljsTnl++X54uCbZF7V/QnEl6ZVaMd4ESQOmB/VFFYOjvIMwVUbFmyL9hxvTySL9QPMX1E/bBUDLnX2XfZWyyAnlA4dCxcwmvLHAgsEd2i+e9wO5OgC4Lao81jRkTrDn6wlgmGpgSq5yn4Dl1eiVAwy68srz4SH5Ix/SEULt1l6/0YAOGp17TjM9wzWgmaHT9FpRDo0Z0fGf7p0CEPIfhk3ittnrTqoXgfgrCGHKTYqK+b5IB2dP/MfHr0G7D18mzMcmn0J+qfxWUi/yalYWr8xkNU/f6SixwGsCL9ZuF52amGAYvSZzl65RRjaEp4FUlmBT7rWtNnOQGidx/ELIAs7PqlI9ZV2LUOOQ8+DT+mYcUxSbiEIUDPP0feVtM9LDOWxCFn1DSM0APKSstXQJ8JS68E6BNo9benB4ufejjjV5xcKsn+CrMoyohpOtI7ZrKLOCO/0dXm91zz9v/H8Ec+ydkyAtlhbTOpk1BjQGg5idXM2bHGRhZfL7/FPLKVARv97HgKEm8goLtICcTmKORcxvU8U+dGZzu54/DXQhv0HacxTHlVvNW+rgmU9tYZjlTUoWF0QrQfm79m/mM7g== x-forefront-antispam-report: CIP:255.255.255.255;CTRY:;LANG:en;SCL:1;SRV:;IPV:NLI;SFV:NSPM;H:MN6PR11MB8244.namprd11.prod.outlook.com;PTR:;CAT:NONE;SFS:(13230022)(6029001)(346002)(39860400002)(396003)(136003)(376002)(366004)(451199015)(6506007)(107886003)(2906002)(5660300002)(52536014)(122000001)(83380400001)(64756008)(4326008)(8936002)(8676002)(66446008)(33656002)(38100700002)(82960400001)(9686003)(478600001)(41300700001)(38070700005)(66556008)(66946007)(26005)(55016003)(76116006)(66476007)(186003)(7696005)(71200400001)(86362001)(316002)(54906003)(110136005);DIR:OUT;SFP:1102; x-ms-exchange-antispam-messagedata-chunkcount: 1 x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?C4EgSSgVYjq/ca+nqr9gDhw9rGDVy272gpE9nvBHKRUUktyyifdd6aCCQ9MB?= =?us-ascii?Q?af7FEA3KpZig+OmWyrBFqlxm0PiOj8kIX4D4CFlUtEswlcsbTR0dI2zWaBTM?= =?us-ascii?Q?hX1zramps3W7VBcV68ahFkOLQ/q07xMh8IYEKZEKkM7coLD0t0e1dZHsBj1x?= =?us-ascii?Q?m1sK/iQSB1wpV0lhhf63CsVpxEIYwt95iu77rSyoF72ohRyU+Gka2M2FxfDj?= =?us-ascii?Q?eJKr4rcpZrROf1je9gXsPIrPYe1QTNxGoFR8shl+6mu0mOMB2WjSr+8XUJdE?= =?us-ascii?Q?Dlm0sUdud0pKGRp48yCKVYB/uJNKxmaWcmokJBGsKzRLxShp8mVpAyLiZ7HK?= =?us-ascii?Q?tGe/qvQ6EKNtCttejo8elyBlgjt5OvS30cw5fPDvHkO2d8LHRVqFqAwDPE8C?= =?us-ascii?Q?DuLGjUS8Z1k0Fb8DAWPDDh0siL62kMUkujRlDwwR4RqGrCNhAPRBd61uVc+z?= =?us-ascii?Q?jVc1yn2VrMPbK1+OPWa0vpMvLlCnIquAvrItJBdMlnnHieRl5hwB172IlC7j?= =?us-ascii?Q?fd2dVij57AjjzRwHPYCKxbhogaFZs5xGu9WjJyIUmGSiT/36gV/Hg0yqcg58?= =?us-ascii?Q?vexWTNzBLMaP1SDXAvjcHf4hgQtGgC03z//kCmrIMZ9zOtNHNokTjV0X2JxG?= =?us-ascii?Q?UW5C7SEbDzTOTGfDX7GElNh0akYXLeX4DkOxdzuBT51fXpbqHIC/GyTtuFoH?= =?us-ascii?Q?sjKkpJGm+EGkvePQSATQqNN3ZuSp0Vz4MDDXvMNn8/Vro/K4EIXZATY0/G3v?= =?us-ascii?Q?5MKNCB8jcHDyCFxWa1NqHzKgc7UgkEXdYyyaXh1KuCfcZu1syNLS9hVCMT7u?= =?us-ascii?Q?ZjOW1dcoJ3KUitjkjuACYD1gJDOEMBBu0Hz9uaFagus6mUFc/rZJxHHeV1zk?= =?us-ascii?Q?PRYXbR6/rit88dgvO06sIodGfdDDRsXcqfxrsuqcNzWdc2WNSU6x5zIj0Lre?= =?us-ascii?Q?zPirOanuP4HPleVBzql32tAJOaXtmqmLm5DPfyoSGITYiq1Snxur041NugE8?= =?us-ascii?Q?lHraYqhnodRHEfLLhUQH6GE/n1VP1SPQq3BuPVa9sbjzABXV2fRT2HPl/fre?= =?us-ascii?Q?NegAeruATPlRI8VxmrUI3jFAoKzjDldauv1yzu9D8uB9/I0jEOfAUBX1wK8o?= =?us-ascii?Q?CdTT9EM74d82oLKkNaQaEQp8rZTrtf2nGHFLP1ANz8kJpKiK4y5uiMVHllyc?= =?us-ascii?Q?oB5h6kmruxg9MumSQIBgvWqsAO/eJEBTN6DIYlX1RG4XRIGj1G2fxZbSa8s6?= =?us-ascii?Q?AMc6YwpTslK0iGAGvUaY5+jAbKuDMCaq9PYbwnKFij9fNZ0Mu6f9GMnykCJW?= =?us-ascii?Q?dr4+W9CRMzpkfElDZLrVTI3sl2xWp8xpWmzJhgdYDpP6zrwzAtMKNQlwja9b?= =?us-ascii?Q?Bddesk7ncQx1yl7BAu04trhgIoMSZwFoyaMa/AG1+07RrAlo0qszZAHh1OUi?= =?us-ascii?Q?6KOzYzb+H6/eXXBtWnH1aKuEiSu5EAlfmyCz8tyKMz0zNtpDCoXDaVUI25rM?= =?us-ascii?Q?6LEF7E+z7UVU4JwWDNqa/4Hj7yxdDLsLHb/m3b9w5BxlKI//vafm+ffw/npT?= =?us-ascii?Q?KRr/3zCpoUzUe+DZE8s=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: d62c98ab-ee90-469d-7baa-08daf53b199f X-MS-Exchange-CrossTenant-originalarrivaltime: 13 Jan 2023 07:52:20.7948 (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: mg2BDqDPm5/uAE2XftZaXuFZBKBtvrfayScEzh7R0io4wiik6c09oLMzu1tCoZj7n2a2T9lEqdh64tb3b5CiYg== X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ0PR11MB5182 Return-Path: ray.ni@intel.com X-OriginatorOrg: intel.com Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Jiaxin, I know that you will submit v3 patch set to address Laszlo's comments. Let me put my review comments on code logic here so you could take them int= o account also. 1. I suggest we describe the HOB structure in this header file in file head= er comments. For example: 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 SMBAS= E+0x10000. One of the SMM initialization from CPU perspective is to program the new SM= BASE (in TSEG range) for each CPU thread. When the SMBASE update happens in a PEI module, the PE= I module shall produce the SMM_BASE_HOB in HOB database which tells the PiSmmCpuDxeSmm dri= ver which runs at a later phase about the new SMBASE for each CPU thread. PiSmmCpuDxeSmm d= river installs the SMI handler at the SMM_BASE_HOB.SmBase[Index]+0x8000 for CPU thread Ind= ex. When the HOB doesn't exist, PiSmmCpuDxeSmm driver shall program the new SMB= ASE itself. > + > +#pragma pack(1) > +typedef struct { > + /// > + /// Describes the Number of all max supported processors. > + /// > + UINT64 NumberOfProcessors; > + /// > + /// Pointer to SmBase array for each Processors. > + /// > + UINT64 SmBase[]; > +} SMM_BASE_HOB_DATA; > +#pragma pack() > + > +extern EFI_GUID gSmmBaseHobGuid; > + > + // > + // If gSmmBaseHobGuid found, means Smm Relocation has been done. > + // > + if (GetFirstGuidHob (&gSmmBaseHobGuid) !=3D NULL) { > + mSmmRelocationDone =3D TRUE; > + } 2. Can you write the code as "mSmmRelocationDone =3D (BOOLEAN) (GetFirstGui= dHob (&gSmmBaseHobGuid) !=3D NULL)"? It removes the assumption that the initial value of mSmmRelocationDone = is FALSE. I understand it's TRUE usually. But it depends on the PE loader. > + if (GetFirstGuidHob (&gSmmBaseHobGuid) !=3D NULL) { > + mSmmRelocated =3D TRUE; > + } else { > + mSmmRelocated =3D FALSE; 3. The above code doesn't assume the initial value of global variable. Good= . Can you align the variable name between SmmCpuFeaturesLib and the PiSmm= CpuDxeSmm driver? If the two names are chosen to fix link error, there are two ways to av= oid the error: a. Add "static" and both component use mSmmRelocated. b. One can be renamed as mSmmCpuFeaturesSmmRelocated. No change to the = one in driver. > + } > + > + // > + // 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) * m= MaxNumberOfCpus); 4. I guess mSmmInitialized is already allocated in normal boot phase. Here = what we need is only to set all elements to FALSE. Will keep reviewing following changes and confirm my gue= ss. But we still cannot call Allocate in every S3 boot without freeing. It = may cause all SMRAM be allocated. > + ASSERT (mSmmInitialized !=3D NULL); > + > + mBspApicId =3D GetApicId (); > + > + // > + // Issue SMI IPI (All Excluding Self SMM IPI + BSP SMM IPI) for SMM = 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, maybe= a local function can be created to reduce the code duplication? > + if (!mSmmRelocated) { > + IsMonarch =3D mIsBsp; > + } else if (mBspApicId =3D=3D ApicId) { > + IsMonarch =3D TRUE; > + } 6. How about removing mIsBsp and always use mBspApicId even when mSmmReloca= ted=3D=3DFALSE? 7. How about renaming IsMonarch to IsBsp? >=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 t= he call is for s3 path. Can you check if we need to call it as well in S3 path when SmmRelocat= ed is TRUE? > + 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); 9. I suggest we add CpuDeadLoop() here to capture the error in release buil= d.