From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail04.groups.io (mail04.groups.io [45.79.224.9]) by spool.mail.gandi.net (Postfix) with ESMTPS id DF98DAC0B43 for ; Wed, 17 Apr 2024 05:24:45 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=QlbLmBmNJ30Ryw3f91TIIQFQeb6ekfKHBwsLbKaMTqg=; c=relaxed/simple; d=groups.io; h=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:Resent-Date:Resent-From:Reply-To:List-Unsubscribe-Post:List-Unsubscribe:Content-Language:Content-Type; s=20240206; t=1713331484; v=1; b=le+7yftO+0tpiIon4FcEyM/9ky5ipdpklEnBA/eezD5sexKHZ+HrCq5q9yPqFzR4r6UINUtb 7Gv9CLTqUAWP2kbsBgU2UXf9dSTWPFkAzElkiklsdsD7YMvfpEfkQpn55HlWfdVEmYZndxabCLL 9cxbJqfOldYf6otrF1GiPJnB1STuiAc1ebDS/R5sANdEvJLIV+Sk95Htz4mB1UIh2dZ4Ss7kRkI lCoYrO6wO23xSptqiWvVoGUi8+q9kv1TG3vOK0ULeDlH9N338sCbJnvWs4DC4j2B4e1wD8EszST zHaBys58Zaga9EZRACP4B5/BeSOHR9p0rAueT+BbYIRYQ== X-Received: by 127.0.0.2 with SMTP id VikLYY7687511x1RM0wNo7Mh; Tue, 16 Apr 2024 22:24:44 -0700 X-Received: from mgamail.intel.com (mgamail.intel.com [198.175.65.16]) by mx.groups.io with SMTP id smtpd.web11.5680.1713331483333431320 for ; Tue, 16 Apr 2024 22:24:43 -0700 X-CSE-ConnectionGUID: /JAEdfNPS9qsnhV/so5gcw== X-CSE-MsgGUID: eKTTI3XlSq69CkJcStmeBg== X-IronPort-AV: E=McAfee;i="6600,9927,11046"; a="8932714" X-IronPort-AV: E=Sophos;i="6.07,208,1708416000"; d="scan'208,217";a="8932714" X-Received: from orviesa005.jf.intel.com ([10.64.159.145]) by orvoesa108.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 16 Apr 2024 22:24:43 -0700 X-CSE-ConnectionGUID: d261wElyT5SkN/Zb86kBjg== X-CSE-MsgGUID: esrZjbmNSYq6lKlkDnXqmA== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="6.07,208,1708416000"; d="scan'208,217";a="27300354" X-Received: from fmsmsx602.amr.corp.intel.com ([10.18.126.82]) by orviesa005.jf.intel.com with ESMTP/TLS/AES256-GCM-SHA384; 16 Apr 2024 22:24:43 -0700 X-Received: from fmsmsx601.amr.corp.intel.com (10.18.126.81) 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; Tue, 16 Apr 2024 22:24:41 -0700 X-Received: from fmsedg602.ED.cps.intel.com (10.1.192.136) by fmsmsx601.amr.corp.intel.com (10.18.126.81) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2507.35 via Frontend Transport; Tue, 16 Apr 2024 22:24:41 -0700 X-Received: from NAM11-DM6-obe.outbound.protection.outlook.com (104.47.57.169) by edgegateway.intel.com (192.55.55.71) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.1.2507.35; Tue, 16 Apr 2024 22:24:41 -0700 X-Received: from MN6PR11MB8244.namprd11.prod.outlook.com (2603:10b6:208:470::14) by LV8PR11MB8487.namprd11.prod.outlook.com (2603:10b6:408:1ed::17) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.7472.27; Wed, 17 Apr 2024 05:24:34 +0000 X-Received: from MN6PR11MB8244.namprd11.prod.outlook.com ([fe80::8774:81a7:c5b7:5c2c]) by MN6PR11MB8244.namprd11.prod.outlook.com ([fe80::8774:81a7:c5b7:5c2c%7]) with mapi id 15.20.7452.049; Wed, 17 Apr 2024 05:24:34 +0000 From: "Ni, Ray" To: "Wu, Jiaxin" , "devel@edk2.groups.io" CC: "Zeng, Star" , Gerd Hoffmann , "Kumar, Rahul R" Subject: Re: [edk2-devel] [PATCH v2 02/10] UefiCpuPkg/SmmRelocationLib: Add SmmRelocationLib library instance Thread-Topic: [PATCH v2 02/10] UefiCpuPkg/SmmRelocationLib: Add SmmRelocationLib library instance Thread-Index: AQHajzkmAEGxLylnaEetKyJt4Ng+ILFqMGGlgACVpfCAASenBw== Date: Wed, 17 Apr 2024 05:24:34 +0000 Message-ID: References: <20240415133021.10516-1-jiaxin.wu@intel.com> <20240415133021.10516-3-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_|LV8PR11MB8487:EE_ x-ms-office365-filtering-correlation-id: f1119727-decd-4d5c-c3ac-08dc5e9eab0c x-ms-exchange-senderadcheck: 1 x-ms-exchange-antispam-relay: 0 x-microsoft-antispam-message-info: ULP01dmEm4NPNfmfhJX9Kx2Wahmitc99VSGQmvlSymMx1Hd8smzadZAdVRYNaG0p66mNbFZImaVChbLOuFGMx0wmw42SsutEvAObpzjt9yXkXCxEQtllY814ItU5/+LQtFwrzj2XX3mmSYDnUgL9Ztl8/TaQDIxwYOgS948mOdMOcQp7mdSO4h1lx141VW7fyMek4dC896NHd4/X9TGajUa6MBTQMUTfboNF6shEK3rSDAJQx3xtTTwN8KHRVnCZQK84Gtp0lc6q/A7TXP2K7qMSDng/mt/c67g70KCz5Ck8rBoEHtzI3L1Yzt/4YIavKB/LOsKc4fy5J5TWyH5STF6NnPwxIJLL/Jp/VDWo2qHRMsF6kd/kEKo9BSbC4malnHptW7urINY7L8r+2g5cXAOfEKj9VzXenUkmfA4NCeTYEokc/WZKzaJN5f3x8RgIGxFhyq9SAesjq69eiEb76WBpxapGs/N784Te743OHfDjGlFEZqR+y/cZMRI3lgx5CDxjofJr+zV9s4I1cXJ6mSyE8y8nW1A0r87YbRZOtNucVQfhiXfFFgnTzZdYLuXQQUSz6p3gMGGMVYexgil2juSezeoGR33xgEB/v60QWqppkAmI1LFggTtnRDHPQk4Hjeo3Gx1sEO7RutwSA83WdYKLbgTogKTW/U24E19ooA5Vp4m7Wh6z/Q8wt0UzFaEyX4UPso4QUvMOFs5z8Cdtp+ym4fCvlvZaHj0DfmpJibw= x-ms-exchange-antispam-messagedata-chunkcount: 1 x-ms-exchange-antispam-messagedata-0: =?Windows-1252?Q?rx1abTB8cmq7sggj9Vj8pulX2KSPp+XGGa0jgktURAoJkLVTCqlhoqkE?= =?Windows-1252?Q?XH8ijVdsAIPIpPGPesF/IdxbKu7Uo5b7v+L+waRaHBGmM+UqHAUM0p2G?= =?Windows-1252?Q?cl3A89NpYs/XKiH5lBjl5mo7D85E32hB/2EPpCOxiU8VFJ5lNYiLXRaC?= =?Windows-1252?Q?/HMZUCfSQop0wVRvxC4zYG9XoznwW6Qto3U8eKmCsCSE6WDhC+g0BZSN?= =?Windows-1252?Q?y14fuYly0DS5vGpg3+3JPj+uxfMC3sKo5kDCDjom1E+Q/PGaILTA8Mwq?= =?Windows-1252?Q?GYruqM4exxCG1IRgpyV9g/pbaAYeeM5CF7XQcSk8M0LjN0iR52W+P3j5?= =?Windows-1252?Q?sH9dvghLmPqNf2skzMw4eChF+dMJnr20tWsC6wp7dAc/xIkMF6cGb+GN?= =?Windows-1252?Q?QIqwqCDMsUIMl6uUmL4DfKwWKyLA/CYDlYO602yha6spTVmeUSXHLQHv?= =?Windows-1252?Q?K/HSvtTM1DAfkwoCj1wOijr9P5pST5trVaYb8jK3ABKnWBW53CQU6+cA?= =?Windows-1252?Q?USk6x5QWrWqklB2T+YkefnpBMRnb/4/n+IP0dNu0XemXDGWi/hYmi+W+?= =?Windows-1252?Q?F8kbmfO+uYFx6OVmiy1yft2XiUhQ1+5f5SaKFZ4LsnGiZVRWtdfm+IYv?= =?Windows-1252?Q?u3ZlbUyO4UuQqJvDSsJLH315K2ztQXzH4ildIfEqkxHzUmpvgXJ0yWcU?= =?Windows-1252?Q?l30+oyj/M2eVTkrI15At+IJx6HvpqbQzq19RnSiJgwycV+gbJnpvzvs3?= =?Windows-1252?Q?ssZCwH6StkbDnpGio6ZlG5PngJFh2tWpLp9UnladmIIDwYwNZisQQ686?= =?Windows-1252?Q?HDdDk2Nj8HhzbshzlBywHr+LjU+CdBSZviEaXjUvSoxCL6xGcTvYV2p7?= =?Windows-1252?Q?YiRb5wSoAuj1BGEjrsQ3n5uoIb231sxcGxiNNAo+KMmADq8t1jlm+UZy?= =?Windows-1252?Q?tXoEx6JeEKHDZo4Ht65mNooQzDDgyXBD8uvVNfE2uylv7Xnjt6hMO0qJ?= =?Windows-1252?Q?d78EA9VfhOBqeEbvXqoOV+OzXqyo4XEhb7Sah3QjI1qUy0ep0MCm6HYF?= =?Windows-1252?Q?7FleptxfYHqYXZmHLA77I7s6/z5K5azcM735NNedOPIXEEbPpPLYhVjt?= =?Windows-1252?Q?8RWxFckTXzrl/FEFhFr/EhA9/YhNb8FEzGuvgJ1a6cYRjTL2AZWj4yUV?= =?Windows-1252?Q?zcVzYThhJJ3fUKX2MhJVEgf2RoTdvn5u6kH8G5ggywhmJ/mI1RILEQGR?= =?Windows-1252?Q?pjCL/uTm8YSmm37ZBPqJUyic7VXGsM0UOphXvgZ1XfAkcV+L6jYGhwKf?= =?Windows-1252?Q?rDQhzpRarjk4nXOXZdwClnbnB+vCgsdziBg1omIDnOu1w4wCLHBoOyJ3?= =?Windows-1252?Q?sTLHt9V8nmhyDgxQqgbQCdteGJR4nGu6r7Kncps3LkF0n56dF5Heloy7?= =?Windows-1252?Q?kp2qR2hWYvHd8GJUPXH3SniXyuctvsH2fylmECv6ezRqgqGwyWGEtB9r?= =?Windows-1252?Q?3aHWeTIdvrlkzJOGZDsB2IQUiaUHyI3H6n8t57JfxVQZpvX3FedM5QBu?= =?Windows-1252?Q?lxaR8IhapecKKXLjs54GpQRKMbDbUa/gAx1XNy21w1fBgYDt98i1dajF?= =?Windows-1252?Q?uaTf/zyWvqDtRXumyxTH8KY54So757vG218O67Qj4vnuGoCb6ZeRITf6?= =?Windows-1252?Q?RohoF2EY9Ng=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: f1119727-decd-4d5c-c3ac-08dc5e9eab0c X-MS-Exchange-CrossTenant-originalarrivaltime: 17 Apr 2024 05:24:34.7190 (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: nWSbL5Vp8KrNB6XIcHNYShpUQkIo5zUy217K7T7smrPLqBxEv4gWb/VYniJLKBpv2xV2WnXK4yHNS4DauY57Bg== X-MS-Exchange-Transport-CrossTenantHeadersStamped: LV8PR11MB8487 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 Resent-Date: Tue, 16 Apr 2024 22:24:43 -0700 Resent-From: ray.ni@intel.com Reply-To: devel@edk2.groups.io,ray.ni@intel.com List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: X-Gm-Message-State: zg5P5ganWCuYFjfxQFs9J1Igx7686176AA= Content-Language: en-US Content-Type: multipart/alternative; boundary="_000_MN6PR11MB8244EC302217C7999A8939A98C0F2MN6PR11MB8244namp_" X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20240206 header.b=le+7yftO; 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 45.79.224.9 as permitted sender) smtp.mailfrom=bounce@groups.io --_000_MN6PR11MB8244EC302217C7999A8939A98C0F2MN6PR11MB8244namp_ Content-Type: text/plain; charset="Windows-1252" Content-Transfer-Encoding: quoted-printable Comments below starting with [Ray] Thanks, Ray ________________________________ From: Wu, Jiaxin Sent: Tuesday, April 16, 2024 20:58 To: Ni, Ray ; devel@edk2.groups.io Cc: Zeng, Star ; Gerd Hoffmann ; Ku= mar, Rahul R Subject: RE: [PATCH v2 02/10] UefiCpuPkg/SmmRelocationLib: Add SmmRelocatio= nLib library instance /** @@ -30,11 +30,12 @@ SemaphoreHook ( { SMRAM_SAVE_STATE_MAP *CpuState; mRebasedFlag =3D RebasedFlag; - CpuState =3D (SMRAM_SAVE_STATE_MAP *)(UINTN)(SMM_DE= FAULT_SMBASE + SMRAM_SAVE_STATE_MAP_OFFSET); + CpuState =3D (SMRAM_SAVE_STATE_MAP *)(UINTN)(SMM_DEFAULT_SMBASE + SMRAM_= SAVE_STATE_MAP_OFFSET); [Ray.1] This change is unnecessary. [Jiaxin.1] it=92s not only move the code to the new lib, but also do some a= daptation. For example here: the code style change to PASS the CI. If no this change, can=92t pass the CI check. [Ray] Original code should also pass uncrustify checker. I guess you added = one blank line above so the alignment of "=3D" is not required. [Ray.4] Can you evaluate what extra changes are required if allowing the li= b runs in flash area? Basically all global variables cannot be modified at = runtime. [Jiaxin.4] The Lib needs to depend on the MP service PPI, it shall be calle= d during post-mem phase, global variables can=92t be used? [Ray] A PEIM could run in flash in post-mem phase. If you pass the values a= cross routines through parameters, most of the global variables can be avoi= ded. + UINTN SmramRanges; [Ray.6] No need SmramRanges. [Jiaxin.6] replace it with DescriptorBlock ->NumberOfSmmReservedRegions dir= ectly? [Ray] yes. + // + // Increase the number of SMRAM descriptors by 1 to make room for the AL= LOCATED descriptor of size EFI_PAGE_SIZE + // + NewDescriptorBlock->NumberOfSmmReservedRegions =3D (UINT32)(SmramRanges = + 1); [Ray.9] NewBlock->NumberOfSmmReservedRegions++; [Jiaxin.9] Agree since we copied the DescriptorBlock to NewDescriptorBlock = first. But I still think original is more easy to understand. [Ray] As long as you remove local variable SmramRanges, I am fine. + + DEBUG ((DEBUG_INFO, "CreateSmmBaseHob - SmmBaseHobData[%d]->ProcessorI= ndex: %03x\n", HobCount, SmmBaseHobData->ProcessorIndex)); + DEBUG ((DEBUG_INFO, "CreateSmmBaseHob - SmmBaseHobData[%d]->NumberOfPr= ocessors: %03x\n", HobCount, SmmBaseHobData->NumberOfProcessors)); + for (Index =3D 0; Index < SmmBaseHobData->NumberOfProcessors; Index++)= { + // + // Calculate the new SMBASE address + // + SmmBaseHobData->SmBase[Index] =3D SmBaseForAllCpus[Index + CpuCount]= ; [Ray.12] Please re-organize the code so that SmBaseForAllCpus array is not = needed. What we need is only the Cpu0SmBase and TileSize. [Jiaxin.12] do you mean calculate the value here? --> (UINTN)(Cpu0SmBase)+ = Index * TileSize - SMM_HANDLER_OFFSET ? I init the smbase value in the function of InitSmBaseForAllCpus(), the valu= e will be used in both ConfigureSmBase & CreateSmmBaseHob. How about the ConfigureSmBase function during SmmRelocateBases()? What=92s = reason you think SmBaseForAllCpus is not good? [Ray] I just want to avoid unnecessary memory allocation. + + // + // Patch SMI stack for SMM base relocation + // Note: No need allocate stack for all CPUs since the relocation + // occurs serially for each CPU + // + SmmStackSize =3D EFI_PAGES_TO_SIZE (EFI_SIZE_TO_PAGES (PcdGet32 (PcdCpuS= mmStackSize))); [Ray.15] PcdCpuSmmStackSize is configured by platform as only platform know= s what kind of SMI handlers will run in SMM env. But in this case, all code is provided by this lib and stack size should be= fixed, maybe simply 1 page is enough. [Jiaxin.15] agree, do you prefer this change as another patch or I just upd= ate the hard code value directly in this patch? [Ray] If the code is inherited from PiSmmCpuDxeSmm driver, you can do that = change in another patch. + // + // Retrieve the Processor Info for all CPUs + // + mProcessorInfo =3D (EFI_PROCESSOR_INFORMATION *)AllocatePool (sizeof (EF= I_PROCESSOR_INFORMATION) * mMaxNumberOfCpus); [Ray.16] mProcessorInfo is needed when programming the new SMBASE. Then can= you just put the new SMBASE for every CPU in the stack top, or just patch = the value in the code region? You can do that in a new patch. [Jiaxin.16] why need such behavior? I only allocate one stack for all cpus = with existing design. The reason see below as I explained: [Ray] My purpose is to avoid global variables. + // + // Initialize the SmBase for all CPUs + // + Status =3D InitSmBaseForAllCpus (&mSmBaseForAllCpus); [Ray.17] mSmBaseForAllCpus global variable is not needed. We only need Cpu0= Smbase and TileSize and the two can be local variables and passed to furthe= r routines through parameters. [Jiaxin.17] let me remove the mSmBaseForAllCpus global variable. But I stil= l think it=92s not good to calculate value in different 2 places again and = again (ConfigureSmBase & CreateSmmBaseHob). [Ray] I agree with you regarding not calculating values in more than 1 plac= e. Please think about how to avoid that. -=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 (#117893): https://edk2.groups.io/g/devel/message/117893 Mute This Topic: https://groups.io/mt/105535806/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- --_000_MN6PR11MB8244EC302217C7999A8939A98C0F2MN6PR11MB8244namp_ Content-Type: text/html; charset="Windows-1252" Content-Transfer-Encoding: quoted-printable
Comments below starting with [Ray]

Thanks,
Ray

From: Wu, Jiaxin <jiaxin.wu@intel.com>
Sent: Tuesday, April 16, 2024 20:58
To: Ni, Ray <ray.ni@intel.com>; devel@edk2.groups.io <= devel@edk2.groups.io>
Cc: Zeng, Star <star.zeng@intel.com>; Gerd Hoffmann <k= raxel@redhat.com>; Kumar, Rahul R <rahul.r.kumar@intel.com>
Subject: RE: [PATCH v2 02/10] UefiCpuPkg/SmmRelocationLib: Add = SmmRelocationLib library instance
 


 
 /**
@@ -30,11 +30,12 @@ SemaphoreHook (
 {
   SMRAM_SAVE_STATE_MAP  *CpuState;
 
   mRebasedFlag =3D RebasedFlag;
 
-  CpuState          = ;            =3D (SM= RAM_SAVE_STATE_MAP *)(UINTN)(SMM_DEFAULT_SMBASE + SMRAM_SAVE_STATE_MAP_OFFS= ET);
+  CpuState =3D (SMRAM_SAVE_STATE_MAP *)(UINTN)(SMM_DEFAULT_SMBASE + S= MRAM_SAVE_STATE_MAP_OFFSET);

 

[Ray.1] Thi= s change is unnecessary.

[Jiaxin.1] it=92s not only move= the code to the new lib, but also do some adaptation. For example here: th= e code style change to PASS the CI.

If no this change, can=92t pass t= he CI check.

[Ray] Original code should also p= ass uncrustify checker. I guess you added one blank line above so the align= ment of "=3D" is not required.


 

[Ray.4] Can= you evaluate what extra changes are required if allowing the lib runs in f= lash area? Basically all global variables cannot be modified at runtime.

[= Jiaxin.4] The Lib needs to depend on the MP service PPI, it shall be called= during post-mem phase, global variables can=92t be used?

[= Ray] A PEIM could run in flash in post-mem phase. If you pass the values ac= ross routines through parameters, most of the global variables can be avoid= ed.



+  UINTN          &n= bsp;            = ;    SmramRanges;

[Ray.6] No = need SmramRanges.

[= Jiaxin.6] replace it with DescriptorBlock ->NumberOfSmmReservedRegions d= irectly?

[Ray] yes. 



+  //
+  // Increase the number of SMRAM descriptors by 1 to make room for t= he ALLOCATED descriptor of size EFI_PAGE_SIZE
+  //
+  NewDescriptorBlock->NumberOfSmmReservedRegions =3D (UINT32)(Smra= mRanges + 1);

[Ray.9] New= Block->NumberOfSmmReservedRegions++;

[= Jiaxin.9] Agree since we copied the DescriptorBlock to NewDescriptorBlock f= irst. But I still think original is more easy to understand.

[= Ray] As long as you remove local variable SmramRanges, I am fine.

 


+
+    DEBUG ((DEBUG_INFO, "CreateSmmBaseHob - SmmBaseHob= Data[%d]->ProcessorIndex: %03x\n", HobCount, SmmBaseHobData->Pro= cessorIndex));
+    DEBUG ((DEBUG_INFO, "CreateSmmBaseHob - SmmBaseHob= Data[%d]->NumberOfProcessors: %03x\n", HobCount, SmmBaseHobData->= ;NumberOfProcessors));
+    for (Index =3D 0; Index < SmmBaseHobData->NumberO= fProcessors; Index++) {
+      //
+      // Calculate the new SMBASE address
+      //
+      SmmBaseHobData->SmBase[Index] =3D SmBase= ForAllCpus[Index + CpuCount];

[Ray.12] Pl= ease re-organize the code so that SmBaseForAllCpus array is not needed. Wha= t we need is only the Cpu0SmBase and TileSize.

[Jiaxin.12] do you mean calcula= te the value here? --> (UINTN)(Cpu0SmBase)+ Index * TileSize - SMM_HANDLER_OFFSET ?

I init the smbase value in the = function of InitSmBaseForAllCpus(), the value will be used in both Configur= eSmBase & CreateSmmBaseHob.

H= ow about the ConfigureSmBase function d= uring SmmRelocateBases()? What=92s reason you think SmBaseForAllCpus is not good?

[= Ray] I just want to avoid unnecessary memory allocation.

 


+
+  //
+  // Patch SMI stack for SMM base relocation
+  // Note: No need allocate stack for all CPUs since the relocation +  // occurs serially for each CPU
+  //
+  SmmStackSize =3D EFI_PAGES_TO_SIZE (EFI_SIZE_TO_PAGES (PcdGet32 (Pc= dCpuSmmStackSize)));

[Ray.15] Pc= dCpuSmmStackSize is configured by platform as only platform knows what kind= of SMI handlers will run in SMM env.

But in this= case, all code is provided by this lib and stack size should be fixed, may= be simply 1 page is enough.

[Jiaxin.15] agree, do you prefer = this change as another patch or I just update the hard code value directly = in this patch?

[Ray] If the code is inherited fr= om PiSmmCpuDxeSmm driver, you can do that change in another patch.


+  //
+  // Retrieve the Processor Info for all CPUs
+  //
+  mProcessorInfo =3D (EFI_PROCESSOR_INFORMATION *)AllocatePool (sizeo= f (EFI_PROCESSOR_INFORMATION) * mMaxNumberOfCpus);

[Ray.16] mP= rocessorInfo is needed when programming the new SMBASE. Then can you just p= ut the new SMBASE for every CPU in the stack top, or just patch the value in the code region?

You can do = that in a new patch.

[Jiaxin.16] why need such behavio= r? I only allocate one stack for all cpus with existing design. The reason = see below as I explained:

[Ray] My purpose is to avoid glob= al variables.

 


+  //
+  // Initialize the SmBase for all CPUs
+  //
+  Status =3D InitSmBaseForAllCpus (&mSmBaseForAllCpus);

[Ray.17] mS= mBaseForAllCpus global variable is not needed. We only need Cpu0Smbase and = TileSize and the two can be local variables and passed to further routines through parameters.

[Jiaxin.17] let me remove the mSm= BaseForAllCpus global variable. But I still thin= k it=92s not good to calculate value in different 2 places again and again = (ConfigureSmBase & CreateSmmBaseHob).

[Ray] I agree= with you regarding not calculating values in more than 1 place. Please thi= nk about how to avoid that.


 

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

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

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

_._,_._,_
--_000_MN6PR11MB8244EC302217C7999A8939A98C0F2MN6PR11MB8244namp_--