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 4703BAC0A43 for ; Tue, 16 Apr 2024 03:20:01 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=F0MJrR7BktQwBAQ8Gk5FGrMa8VdQTI/KBvKOsCjHbe8=; 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=1713237599; v=1; b=ynNRK6kYexr+rFn3R5aOk2CGWM4//4vYNCR9bmLUn3yAGS9d9zFV2Vgzo7gkng2OgLkXOzuN ZAB7wwkV3S/KQaQkVonIW/o046wiktHzMdCI49OcjCAhR0aI7aQVKgWG/2AmD7KTyghaCI3oUsk Oij2Ga1txMyN0vDp0N+ddMzsSFw2YJ2PJK+Q0KcRU7JhWDNJDehKvdfDGZ3zw0v0ypZrSTyh063 uWZrUD5viH6XKjke5M6OoV0u27D/gGIOx3g+00QPRlJisfiNo8Y+LzfGhN4gaqq5JOTsENVVfbL gP8pqwbuNE/UML8U9V1P0HPy0dYt93XfbK3FrdkAXoBLg== X-Received: by 127.0.0.2 with SMTP id 0NGQYY7687511xOYdl9vVAfy; Mon, 15 Apr 2024 20:19:59 -0700 X-Received: from mgamail.intel.com (mgamail.intel.com [198.175.65.15]) by mx.groups.io with SMTP id smtpd.web10.11585.1713237598823363580 for ; Mon, 15 Apr 2024 20:19:59 -0700 X-CSE-ConnectionGUID: rjOlA28wS2SKNwMthyYtrA== X-CSE-MsgGUID: lSd6q5VfTuCh6F5zaz8YpA== X-IronPort-AV: E=McAfee;i="6600,9927,11045"; a="12443221" X-IronPort-AV: E=Sophos;i="6.07,204,1708416000"; d="scan'208,217";a="12443221" X-Received: from fmviesa006.fm.intel.com ([10.60.135.146]) by orvoesa107.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 15 Apr 2024 20:19:58 -0700 X-CSE-ConnectionGUID: dlf3F0UkRRyFQ4hRGa2MPw== X-CSE-MsgGUID: eedAANWFR9yI0LRuHUBhbQ== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="6.07,204,1708416000"; d="scan'208,217";a="22188963" X-Received: from orsmsx601.amr.corp.intel.com ([10.22.229.14]) by fmviesa006.fm.intel.com with ESMTP/TLS/AES256-GCM-SHA384; 15 Apr 2024 20:19:58 -0700 X-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.35; Mon, 15 Apr 2024 20:19:57 -0700 X-Received: from orsmsx612.amr.corp.intel.com (10.22.229.25) 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.35; Mon, 15 Apr 2024 20:19:57 -0700 X-Received: from orsedg603.ED.cps.intel.com (10.7.248.4) 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; Mon, 15 Apr 2024 20:19:57 -0700 X-Received: from NAM11-BN8-obe.outbound.protection.outlook.com (104.47.58.168) 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.35; Mon, 15 Apr 2024 20:19:56 -0700 X-Received: from MN6PR11MB8244.namprd11.prod.outlook.com (2603:10b6:208:470::14) by SA3PR11MB7415.namprd11.prod.outlook.com (2603:10b6:806:318::20) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.7452.49; Tue, 16 Apr 2024 03:19:54 +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; Tue, 16 Apr 2024 03:19:54 +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+ILFqMGGl Date: Tue, 16 Apr 2024 03:19:54 +0000 Message-ID: References: <20240415133021.10516-1-jiaxin.wu@intel.com> <20240415133021.10516-3-jiaxin.wu@intel.com> In-Reply-To: <20240415133021.10516-3-jiaxin.wu@intel.com> Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: msip_labels: x-ms-publictraffictype: Email x-ms-traffictypediagnostic: MN6PR11MB8244:EE_|SA3PR11MB7415:EE_ x-ms-office365-filtering-correlation-id: 7d638a01-db99-425b-ec3b-08dc5dc415cc x-ms-exchange-senderadcheck: 1 x-ms-exchange-antispam-relay: 0 x-microsoft-antispam-message-info: 4ihEca4cUmS0hLFOBdfc77ZVnouzCLXH3k8QaIVOiHQq23grnt0nL0fKbLXfI6aAQHmUI8p2NnOlXMc1imdjsvWWyU13qjuVO/NsaPCUtORe5Ori4+aTMSSmcTwqJZSRpAg3iHOssYqepZC5J2cvh57PCsK7feDftTkIzUb2BICxmL0qs6gRimv5MkDp7fex9aSPMTPS1AHFI1NEmMyAr0Wy/CfaCiC4viaTuuONfjogTvMfaooJ6Mc8udYPdyqcnjxtmGhnuUYERcqvxl0KRSODJMqUxJ/qkfbY2dhSzfgbSpzuiz6IjMe5T9LFHJK46uydv+zKE3lNwV/afJEdjPhBGAAyDJj+nimMnbKCBWb2n9oXwk+bP1W+E3ERQNovP7xV1WhtdX1Bx+c4oKBOe+TeTaYsf1snkNWtfqXp3NP7dqAgqNxBB4OGK1RxM39fW74KlrnmWDq9Wj3KdqBMX+u1f3i2M8Od5HjHwsx3+G/0asWn3dcEVwlJf0GRITo7sysp2Rcuyoz1jCMDhs4iE41u0JhVQGPgwAW8R/SjBfRBHyr7QsFFkqrqXZmUJR3kOm8dv5ML3bbzieb4TJ85RbL2/ygHgL3eXksTHjTApvQpb/KhvN2170xfKP0cP3WbViLTtib+KxMW4FRRgkveEqJ7Z1HsxN5NON2Pbp1hl0YIpu+nZlJiVh2oUIucmC2Ds07qF7l0wBS5oiC+IM93+rD+YaF5iy8AaHzt6wTeae8= x-ms-exchange-antispam-messagedata-chunkcount: 1 x-ms-exchange-antispam-messagedata-0: =?iso-8859-1?Q?cfa0BFQP423I7XGH8SuOGzzaHV3ljdP/+1i1nSJqaeUpYX26iRjyzCrG0h?= =?iso-8859-1?Q?ykN++Tb50qh8w0bSpe68o/MID4G1hlagkJvnYFq/ZVnvgEMyJ0BkgeIaIH?= =?iso-8859-1?Q?/sUdfqpRyrgKaFUCqctXRn9JCOZRpu1jerXEV4vE7b+MLxUN4yWKal6LAE?= =?iso-8859-1?Q?+H3S3GPAlbMWKGoBkCDtThIYRRsQSxFDeWLspVGduexcZy9gQc3f+6ouED?= =?iso-8859-1?Q?zlBywM+vzGIX7Vhb3VFq+n7B2+aW+0GBsSlgzcrMRalk+Xu84dSYPEV4OW?= =?iso-8859-1?Q?kclTZ30whtf1pZG8g7BkSavxe/XJKmg3KqUiDqbg87QlY7NcfQ6HJOHCFP?= =?iso-8859-1?Q?SvI665r5zreTtjdJxQmConmts0xuZ6sY/E+2rulRRvU18gcqhwT0fxQIaX?= =?iso-8859-1?Q?bUXveuJoklUTcZfmc+7WTkojV0+EEf3W5rmW796J3v+igO0xETjqK3EThd?= =?iso-8859-1?Q?FnsM7QOFSX5kARk+Uh/Yp8Kpoqdgsb4+vCpPzArPVpBBsptVHRmMI365cj?= =?iso-8859-1?Q?fMFMHFue1++am1E+edCudwvkfHlPGRPhIhtiOjTjhmlH0NdntKzEniNGny?= =?iso-8859-1?Q?wj3TzzmohB8DrEgL+phISaVLroGkmIjIkmvZKw0A7vDrITaIPYdNbeSIPP?= =?iso-8859-1?Q?3IOh6ClwW7vL0uAfhJJO5ijEgTQocoFYG83Vibp5GgdlN2m016QJTLfHaX?= =?iso-8859-1?Q?sFLHLYp7lUCmsBxJVMgwMl3aAE1ibNhzWO1k28Q5zaEVHYRVXJ4lJHSgAf?= =?iso-8859-1?Q?qIrdst9PT4wp/9FExcsKNyuvaOGnVgrHyM6KYvi7xXU/RshH0+zlUtCYzl?= =?iso-8859-1?Q?j1pC3dxWF59ykAQH4R14bvyCEPhA8LXNfpCOTSh3m3rjTbsO94otoziw+M?= =?iso-8859-1?Q?GlPKz8+Pv4Z1v0/Eo5BBzkplnBtZjTCbkL6Izhcae2pM4K5535L60kBAnC?= =?iso-8859-1?Q?JddO0qoiYnfUQ+L25xmv7mTA5G0OqVsXYvVNwQJl2wfgmuwNq+sPfsUIVq?= =?iso-8859-1?Q?jx0gLvLPDwFYcSwmQ4wO45hq8iNvqZvzYIgSUHIwoBq2F46CDEiJR7XgGB?= =?iso-8859-1?Q?vBNjTzxSZV6nxF+r8PZDsYqLFS9rqgekMXcyqP/dbxdC8PHEjoQkLRoOcJ?= =?iso-8859-1?Q?nuDTUa+CRpIeGKGSJBF37Awg8RLFNl9r7xIooIgPEJTvQu12FBYsJL9UOw?= =?iso-8859-1?Q?Cl5nIss+iv3fZ/oa8Q/G37Dj9tv99/Zw/cJtUrI2pHr26/apVKVxLjFRFR?= =?iso-8859-1?Q?NJ4rDFnCFvjZVDGFRmVAShe82S0Zgt0YVG359btnth+7gMN1Sr05xytQxt?= =?iso-8859-1?Q?y+yFkJHydpVhrsGfHgPOqOX/rSMWtxVfeg3Nt5bXpnh/cES0ByuLgqky53?= =?iso-8859-1?Q?1tJKSOpo/MpYRwpsoFIAWx+tdmntwzAcwIVxFIV6bl6d3SavfdSL84A8Az?= =?iso-8859-1?Q?o61jPCEyva1EANkeHhY82JK22JKpWmHVmEuiJXriBHgFZoLh+NpnaD0gCQ?= =?iso-8859-1?Q?yLcStwkzZk/JtjRaHA1djXk0QQUlzx9MEROaKxNUrQTYtuBYoRRjO4rXAf?= =?iso-8859-1?Q?NyuROn4hiQsNdwjz/rMYqiMyjh1RNCPlKVDG7WFn3WpaAa1fB00/SK661Q?= =?iso-8859-1?Q?78mVan8eQLfD4=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: 7d638a01-db99-425b-ec3b-08dc5dc415cc X-MS-Exchange-CrossTenant-originalarrivaltime: 16 Apr 2024 03:19:54.0421 (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: T2+8kuX2hCC7R6UXrvEPC4GnZp6TFKSQK+Daw5ZNC0o4Uof0dqqyO+QgUZdGSH0E9lHRKlYj2bTvEqCe87144A== X-MS-Exchange-Transport-CrossTenantHeadersStamped: SA3PR11MB7415 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: Mon, 15 Apr 2024 20:19:59 -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: yHH8HGKbBVGbSGBm1XwMLJQTx7686176AA= Content-Language: en-US Content-Type: multipart/alternative; boundary="_000_MN6PR11MB8244F1CEEF7C0B3740FBC0FA8C082MN6PR11MB8244namp_" X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20240206 header.b=ynNRK6kY; 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_MN6PR11MB8244F1CEEF7C0B3740FBC0FA8C082MN6PR11MB8244namp_ Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Comments below starting with [Ray.xx] Thanks, Ray /** @@ -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. +;-------------------------------------------------------------------------= ------ + +%include "StuffRsbNasm.inc" + +global ASM_PFX(gcSmmInitIdtr) +global ASM_PFX(gcSmmInitGdtr) + +extern ASM_PFX(SmmInitHandler) +extern ASM_PFX(mRebasedFlag) +extern ASM_PFX(mSmmRelocationOriginalAddress) + +global ASM_PFX(gPatchSmmCr3) +global ASM_PFX(gPatchSmmCr4) +global ASM_PFX(gPatchSmmCr0) +global ASM_PFX(gPatchSmmInitStack) +global ASM_PFX(gcSmmInitSize) +global ASM_PFX(gcSmmInitTemplate) [Ray.2] Can you create another patch after this patch to rename the global = variables/functions to avoid using the same as PiSmmCpuDxeSmm driver? + +**/ + +#ifndef INTERNAL_SMM_RELOCATION_LIB_H_ +#define INTERNAL_SMM_RELOCATION_LIB_H_ + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include [Ray.3] Can you double confirm if all above headers are needed? + +extern UINT64 *mSmBaseForAllCpus; + +extern IA32_DESCRIPTOR gcSmmInitGdtr; +extern IA32_DESCRIPTOR gcSmmInitIdtr; +extern CONST UINT16 gcSmmInitSize; +extern CONST UINT8 gcSmmInitTemplate[]; + +X86_ASSEMBLY_PATCH_LABEL gPatchSmmCr0; +X86_ASSEMBLY_PATCH_LABEL gPatchSmmCr3; +X86_ASSEMBLY_PATCH_LABEL gPatchSmmCr4; +X86_ASSEMBLY_PATCH_LABEL gPatchSmmInitStack; [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. +**/ +EFI_STATUS +SplitSmramHobForSmmRelocation ( + IN UINT64 SmmRelocationSize, + IN OUT EFI_PHYSICAL_ADDRESS *SmmRelocationStart + ) +{ + EFI_HOB_GUID_TYPE *GuidHob; + EFI_SMRAM_HOB_DESCRIPTOR_BLOCK *DescriptorBlock; + EFI_SMRAM_HOB_DESCRIPTOR_BLOCK *NewDescriptorBlock; + UINTN BufferSize; [Ray.5] How about Block, NewBlock, NewBlockSize? It's simpler and easy to u= nderstand. + UINTN SmramRanges; [Ray.6] No need SmramRanges. + + NewDescriptorBlock =3D NULL; [Ray.7] No need of this line. + + // + // Retrieve the GUID HOB data that contains the set of SMRAM descriptors + // + GuidHob =3D GetFirstGuidHob (&gEfiSmmSmramMemoryGuid); + if (GuidHob =3D=3D NULL) { + return EFI_NOT_FOUND; + } + + DescriptorBlock =3D (EFI_SMRAM_HOB_DESCRIPTOR_BLOCK *)GET_GUID_HOB_DATA = (GuidHob); + + // + // Allocate one extra EFI_SMRAM_DESCRIPTOR to describe SMRAM memory that= contains a pointer + // to the Smm relocated memory. [Ray.8] "pointer" is a bit confusing. "Allocate one extra EFI_SMRAM_DESCRIPTOR to describe smram carved out for a= ll SMBASE." + // + SmramRanges =3D DescriptorBlock->NumberOfSmmReservedRegions; + BufferSize =3D sizeof (EFI_SMRAM_HOB_DESCRIPTOR_BLOCK) + (SmramRanges *= sizeof (EFI_SMRAM_DESCRIPTOR)); + + NewDescriptorBlock =3D (EFI_SMRAM_HOB_DESCRIPTOR_BLOCK *)BuildGuidHob ( + &gEfiSmmSmramMe= moryGuid, + BufferSize + ); + ASSERT (NewDescriptorBlock !=3D NULL); + if (NewDescriptorBlock =3D=3D NULL) { + return EFI_DEVICE_ERROR; + } + + // + // Copy old EFI_SMRAM_HOB_DESCRIPTOR_BLOCK to new allocated region + // + CopyMem ((VOID *)NewDescriptorBlock, DescriptorBlock, BufferSize - sizeo= f (EFI_SMRAM_DESCRIPTOR)); + + // + // 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++; + + ASSERT (SmramRanges >=3D 1); + // + // Copy last entry to the end - we assume TSEG is last entry. + // + CopyMem (&NewDescriptorBlock->Descriptor[SmramRanges], &NewDescriptorBlo= ck->Descriptor[SmramRanges - 1], sizeof (EFI_SMRAM_DESCRIPTOR)); + + // + // Update the entry in the array with a size of SmmRelocationSize and pu= t into the ALLOCATED state + // + NewDescriptorBlock->Descriptor[SmramRanges - 1].PhysicalSize =3D SmmRelo= cationSize; + NewDescriptorBlock->Descriptor[SmramRanges - 1].RegionState |=3D EFI_ALL= OCATED; + + // + // Return the start address of Smm relocated memory in SMRAM. + // + if (SmmRelocationStart !=3D NULL) { [Ray.10] It's a local function and we know SmmRelocationStart is never NULL= . No need the if-check. +EFI_STATUS +CreateSmmBaseHob ( + IN UINT64 *SmBaseForAllCpus + ) +{ + UINTN Index; + SMM_BASE_HOB_DATA *SmmBaseHobData; + UINT32 CpuCount; + UINT32 NumberOfProcessorsInHob; + UINT32 MaxCapOfProcessorsInHob; + UINT32 HobCount; + + SmmBaseHobData =3D NULL; + CpuCount =3D 0; + NumberOfProcessorsInHob =3D 0; + MaxCapOfProcessorsInHob =3D 0; + HobCount =3D 0; + + // + // Count the HOB instance maximum capacity of CPU (MaxCapOfProcessorsInH= ob) since the max HobLength is 0xFFF8. + // + MaxCapOfProcessorsInHob =3D (0xFFF8 - sizeof (EFI_HOB_GUID_TYPE) - sizeo= f (SMM_BASE_HOB_DATA)) / sizeof (UINT64) + 1; + DEBUG ((DEBUG_INFO, "CreateSmmBaseHob - MaxCapOfProcessorsInHob: %03x\n"= , MaxCapOfProcessorsInHob)); [Ray.11] "%03x" is confusing. Log readers cannot know "10" means 10 or 16. = How about "%d"? + + // + // Create Guided SMM Base HOB Instances. + // + while (CpuCount !=3D mMaxNumberOfCpus) { + NumberOfProcessorsInHob =3D MIN ((UINT32)mMaxNumberOfCpus - CpuCount, = MaxCapOfProcessorsInHob); + + SmmBaseHobData =3D BuildGuidHob ( + &gSmmBaseHobGuid, + sizeof (SMM_BASE_HOB_DATA) + sizeof (UINT64) * Numb= erOfProcessorsInHob + ); + if (SmmBaseHobData =3D=3D NULL) { + return EFI_OUT_OF_RESOURCES; + } + + SmmBaseHobData->ProcessorIndex =3D CpuCount; + SmmBaseHobData->NumberOfProcessors =3D NumberOfProcessorsInHob; + + 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. + DEBUG ((DEBUG_INFO, "CreateSmmBaseHob - SmmBaseHobData[%d]->SmBase[%= 03x]: %08x\n", HobCount, Index, SmmBaseHobData->SmBase[Index])); [Ray.13] Same comments as above. Please use "0x" prefix if you prints hex o= therwise the log is hard to understand. + + // + // Patch ASM code template with current CR0, CR3, and CR4 values + // + PatchInstructionX86 (gPatchSmmCr0, AsmReadCr0 (), 4); + PatchInstructionX86 (gPatchSmmCr3, AsmReadCr3 (), 4); + PatchInstructionX86 (gPatchSmmCr4, AsmReadCr4 () & (~CR4_CET_ENABLE), 4)= ; [Ray.14] Similar question: Please try to remove the assumption that the lib= runs in physical memory. + + // + // 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. + // + // Get the number of processors + // + Status =3D MpServices2->GetNumberOfProcessors ( + MpServices2, + &mNumberOfCpus, + &NumberOfEnabledCpus + ); + if (EFI_ERROR (Status)) { + return Status; + } + + if (FeaturePcdGet (PcdCpuHotPlugSupport)) { + mMaxNumberOfCpus =3D PcdGet32 (PcdCpuMaxLogicalProcessorNumber); + } else { + mMaxNumberOfCpus =3D mNumberOfCpus; + } + + // + // 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. + // + // 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. +++ b/UefiCpuPkg/Library/SmmRelocationLib/SmramSaveStateConfig.c @@ -0,0 +1,139 @@ +/** @file + Config SMRAM Save State for SmmBases Relocation. + + Copyright (c) 2024, Intel Corporation. All rights reserved.
+ SPDX-License-Identifier: BSD-2-Clause-Patent + +**/ +#include "InternalSmmRelocationLib.h" +#include + +/** + Determine the mode of the CPU at the time an SMI occurs + + @retval EFI_MM_SAVE_STATE_REGISTER_LMA_32BIT 32 bit. + @retval EFI_MM_SAVE_STATE_REGISTER_LMA_64BIT 64 bit. + +**/ +UINT8 +CheckMmSaveStateRegisterLma ( [Ray.18] GetMmSaveStateRegisterLma(). I recommend to never use "check" in f= unction name. -=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 (#117838): https://edk2.groups.io/g/devel/message/117838 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_MN6PR11MB8244F1CEEF7C0B3740FBC0FA8C082MN6PR11MB8244namp_ Content-Type: text/html; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable
Comments below starting with [Ray.xx]

Thanks,
Ray

 
 /**
@@ -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] This change is unnecessary.

+;-------------------------------------------------------------------------= ------
+
+%include "StuffRsbNasm.inc"
+
+global  ASM_PFX(gcSmmInitIdtr)
+global  ASM_PFX(gcSmmInitGdtr)
+
+extern ASM_PFX(SmmInitHandler)
+extern ASM_PFX(mRebasedFlag)
+extern ASM_PFX(mSmmRelocationOriginalAddress)
+
+global ASM_PFX(gPatchSmmCr3)
+global ASM_PFX(gPatchSmmCr4)
+global ASM_PFX(gPatchSmmCr0)
+global ASM_PFX(gPatchSmmInitStack)
+global ASM_PFX(gcSmmInitSize)
+global ASM_PFX(gcSmmInitTemplate)


[Ray.2] Can you create another patch after this patch to rename the global = variables/functions to avoid using the same as PiSmmCpuDxeSmm driver?



+
+**/
+
+#ifndef INTERNAL_SMM_RELOCATION_LIB_H_
+#define INTERNAL_SMM_RELOCATION_LIB_H_
+
+#include <PiPei.h>
+#include <Library/BaseLib.h>
+#include <Library/BaseMemoryLib.h>
+#include <Library/CpuExceptionHandlerLib.h>
+#include <Library/DebugLib.h>
+#include <Library/HobLib.h>
+#include <Library/LocalApicLib.h>
+#include <Library/MemoryAllocationLib.h>
+#include <Library/PcdLib.h>
+#include <Library/PeimEntryPoint.h>
+#include <Library/PeiServicesLib.h>
+#include <Library/SmmRelocationLib.h>
+#include <Guid/SmramMemoryReserve.h>
+#include <Guid/SmmBaseHob.h>
+#include <Register/Intel/Cpuid.h>
+#include <Register/Intel/SmramSaveStateMap.h>
+#include <Protocol/MmCpu.h>

[Ray.3] Can you double confirm if all above headers are needed?

+
+extern UINT64  *mSmBaseForAllCpus;
+
+extern IA32_DESCRIPTOR  gcSmmInitGdtr;
+extern IA32_DESCRIPTOR  gcSmmInitIdtr;
+extern CONST UINT16     gcSmmInitSize;
+extern CONST UINT8      gcSmmInitTemplate[];
+
+X86_ASSEMBLY_PATCH_LABEL  gPatchSmmCr0;
+X86_ASSEMBLY_PATCH_LABEL  gPatchSmmCr3;
+X86_ASSEMBLY_PATCH_LABEL  gPatchSmmCr4;
+X86_ASSEMBLY_PATCH_LABEL  gPatchSmmInitStack;

[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.


+**/
+EFI_STATUS
+SplitSmramHobForSmmRelocation (
+  IN     UINT64     &nbs= p;          SmmRelocationSize,=
+  IN OUT EFI_PHYSICAL_ADDRESS  *SmmRelocationStart
+  )
+{
+  EFI_HOB_GUID_TYPE        &n= bsp;      *GuidHob;
+  EFI_SMRAM_HOB_DESCRIPTOR_BLOCK  *DescriptorBlock;
+  EFI_SMRAM_HOB_DESCRIPTOR_BLOCK  *NewDescriptorBlock;
+  UINTN          &n= bsp;            = ;    BufferSize;

[Ray.5] How about Block, NewBlock, NewBlockSize? It's simpler and easy to u= nderstand.

+  UINTN          &n= bsp;            = ;    SmramRanges;
[Ray.6] No need SmramRanges.

+
+  NewDescriptorBlock =3D NULL;
[Ray.7] No need of this line.

+
+  //
+  // Retrieve the GUID HOB data that contains the set of SMRAM descri= ptors
+  //
+  GuidHob =3D GetFirstGuidHob (&gEfiSmmSmramMemoryGuid);
+  if (GuidHob =3D=3D NULL) {
+    return EFI_NOT_FOUND;
+  }
+
+  DescriptorBlock =3D (EFI_SMRAM_HOB_DESCRIPTOR_BLOCK *)GET_GUID_HOB_= DATA (GuidHob);
+
+  //
+  // Allocate one extra EFI_SMRAM_DESCRIPTOR to describe SMRAM memory= that contains a pointer
+  // to the Smm relocated memory.
[Ray.8] "pointer" is a bit confusing.
"Allocate one extra EFI_SMRAM= _DESCRIPTOR to describe smram carved out for all SMBASE."

+  //
+  SmramRanges =3D DescriptorBlock->NumberOfSmmReservedRegions;
+  BufferSize  =3D sizeof (EFI_SMRAM_HOB_DESCRIPTOR_BLOCK) + (Smr= amRanges * sizeof (EFI_SMRAM_DESCRIPTOR));
+
+  NewDescriptorBlock =3D (EFI_SMRAM_HOB_DESCRIPTOR_BLOCK *)BuildGuidH= ob (
+            &n= bsp;            = ;            &n= bsp;            = ;         &gEfiSmmSmramMemoryGu= id,
+            &n= bsp;            = ;            &n= bsp;            = ;         BufferSize
+            &n= bsp;            = ;            &n= bsp;            = ;         );
+  ASSERT (NewDescriptorBlock !=3D NULL);
+  if (NewDescriptorBlock =3D=3D NULL) {
+    return EFI_DEVICE_ERROR;
+  }
+
+  //
+  // Copy old EFI_SMRAM_HOB_DESCRIPTOR_BLOCK to new allocated region<= br> +  //
+  CopyMem ((VOID *)NewDescriptorBlock, DescriptorBlock, BufferSize - = sizeof (EFI_SMRAM_DESCRIPTOR));
+
+  //
+  // 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] NewBlock->NumberOfSmmRe= servedRegions++;


+
+  ASSERT (SmramRanges >=3D 1);
+  //
+  // Copy last entry to the end - we assume TSEG is last entry.
+  //
+  CopyMem (&NewDescriptorBlock->Descriptor[SmramRanges], &= NewDescriptorBlock->Descriptor[SmramRanges - 1], sizeof (EFI_SMRAM_DESCR= IPTOR));
+
+  //
+  // Update the entry in the array with a size of SmmRelocationSize a= nd put into the ALLOCATED state
+  //
+  NewDescriptorBlock->Descriptor[SmramRanges - 1].PhysicalSize =3D= SmmRelocationSize;
+  NewDescriptorBlock->Descriptor[SmramRanges - 1].RegionState |=3D= EFI_ALLOCATED;
+
+  //
+  // Return the start address of Smm relocated memory in SMRAM.
+  //
+  if (SmmRelocationStart !=3D NULL) {
[Ray.10] It's a local function and we know SmmRelocationStart is never NULL= . No need the if-check.

+EFI_STATUS
+CreateSmmBaseHob (
+  IN UINT64  *SmBaseForAllCpus
+  )
+{
+  UINTN          &n= bsp;   Index;
+  SMM_BASE_HOB_DATA  *SmmBaseHobData;
+  UINT32          &= nbsp;  CpuCount;
+  UINT32          &= nbsp;  NumberOfProcessorsInHob;
+  UINT32          &= nbsp;  MaxCapOfProcessorsInHob;
+  UINT32          &= nbsp;  HobCount;
+
+  SmmBaseHobData         = ; =3D NULL;
+  CpuCount          = ;      =3D 0;
+  NumberOfProcessorsInHob =3D 0;
+  MaxCapOfProcessorsInHob =3D 0;
+  HobCount          = ;      =3D 0;
+
+  //
+  // Count the HOB instance maximum capacity of CPU (MaxCapOfProcesso= rsInHob) since the max HobLength is 0xFFF8.
+  //
+  MaxCapOfProcessorsInHob =3D (0xFFF8 - sizeof (EFI_HOB_GUID_TYPE) - = sizeof (SMM_BASE_HOB_DATA)) / sizeof (UINT64) + 1;
+  DEBUG ((DEBUG_INFO, "CreateSmmBaseHob - MaxCapOfProcessorsInHo= b: %03x\n", MaxCapOfProcessorsInHob));
[Ray.11] "%03x" is confusing. Log readers cannot know "10&qu= ot; means 10 or 16. How about "%d"?

+
+  //
+  // Create Guided SMM Base HOB Instances.
+  //
+  while (CpuCount !=3D mMaxNumberOfCpus) {
+    NumberOfProcessorsInHob =3D MIN ((UINT32)mMaxNumberOfCp= us - CpuCount, MaxCapOfProcessorsInHob);
+
+    SmmBaseHobData =3D BuildGuidHob (
+            &n= bsp;          &gSmmBaseHob= Guid,
+            &n= bsp;          sizeof (SMM_BASE= _HOB_DATA) + sizeof (UINT64) * NumberOfProcessorsInHob
+            &n= bsp;          );
+    if (SmmBaseHobData =3D=3D NULL) {
+      return EFI_OUT_OF_RESOURCES;
+    }
+
+    SmmBaseHobData->ProcessorIndex   &nbs= p; =3D CpuCount;
+    SmmBaseHobData->NumberOfProcessors =3D NumberOfProce= ssorsInHob;
+
+    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] Please re-organize the code so that SmBaseForAllCpus array is not = needed. What we need is only the Cpu0SmBase and TileSize.

+      DEBUG ((DEBUG_INFO, "CreateSmmBaseHob = - SmmBaseHobData[%d]->SmBase[%03x]: %08x\n", HobCount, Index, SmmBa= seHobData->SmBase[Index]));
[Ray.13] Same comments as above. Please use "0x" prefix if you pr= ints hex otherwise the log is hard to understand.

+
+  //
+  // Patch ASM code template with current CR0, CR3, and CR4 values +  //
+  PatchInstructionX86 (gPatchSmmCr0, AsmReadCr0 (), 4);
+  PatchInstructionX86 (gPatchSmmCr3, AsmReadCr3 (), 4);
+  PatchInstructionX86 (gPatchSmmCr4, AsmReadCr4 () & (~CR4_CET_EN= ABLE), 4);
[Ray.14] Similar question: Please try to remove the assumption that the lib= runs in physical memory.


+
+  //
+  // 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] 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.

+  //
+  // Get the number of processors
+  //
+  Status =3D MpServices2->GetNumberOfProcessors (
+            &n= bsp;            = ; MpServices2,
+            &n= bsp;            = ; &mNumberOfCpus,
+            &n= bsp;            = ; &NumberOfEnabledCpus
+            &n= bsp;            = ; );
+  if (EFI_ERROR (Status)) {
+    return Status;
+  }
+
+  if (FeaturePcdGet (PcdCpuHotPlugSupport)) {
+    mMaxNumberOfCpus =3D PcdGet32 (PcdCpuMaxLogicalProcesso= rNumber);
+  } else {
+    mMaxNumberOfCpus =3D mNumberOfCpus;
+  }
+
+  //
+  // Retrieve the Processor Info for all CPUs
+  //
+  mProcessorInfo =3D (EFI_PROCESSOR_INFORMATION *)AllocatePool (sizeo= f (EFI_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.

+  //
+  // 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.

+++ b/UefiCpuPkg/Library/SmmRelocationLib/SmramSaveStateConfig.c
@@ -0,0 +1,139 @@
+/** @file
+  Config SMRAM Save State for SmmBases Relocation.
+
+  Copyright (c) 2024, Intel Corporation. All rights reserved.<BR&g= t;
+  SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+#include "InternalSmmRelocationLib.h"
+#include <Library/CpuLib.h>
+
+/**
+  Determine the mode of the CPU at the time an SMI occurs
+
+  @retval EFI_MM_SAVE_STATE_REGISTER_LMA_32BIT   32 bit. +  @retval EFI_MM_SAVE_STATE_REGISTER_LMA_64BIT   64 bit. +
+**/
+UINT8
+CheckMmSaveStateRegisterLma (
[Ray.18] GetMmSaveStateRegisterLma(). I recommend to never use "check&= quot; in function name.

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

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

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

_._,_._,_
--_000_MN6PR11MB8244F1CEEF7C0B3740FBC0FA8C082MN6PR11MB8244namp_--