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 EA58EAC1862 for ; Wed, 20 Dec 2023 16:05:02 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=WOSIEMyAZePkuMbbsQGf+DKec8hVmDTJGtCHkOIxKNY=; c=relaxed/simple; d=groups.io; h=MIME-Version:References:In-Reply-To:From:Date:Message-ID:Subject:To:Cc:Precedence:List-Subscribe:List-Help:Sender:List-Id:Mailing-List:Delivered-To:Reply-To:List-Unsubscribe-Post:List-Unsubscribe:Content-Type; s=20140610; t=1703088301; v=1; b=XW2LsTotmLziJtQPIDrSGw1Qq8iSBbvYImE19wkn+pfjNgmJo0EaObk7syi3cgl8J92itFq/ s2PtCakPs1km8xFg/aYGtgKg3gddkQLYOSnKLbTnERPbfpjxBYAH0Jgk979pMOf+fPK/gQWP6At 7Zew35kIJU75+ukSIgWelg64= X-Received: by 127.0.0.2 with SMTP id O7dPYY7687511xeqPTOOfcn2; Wed, 20 Dec 2023 08:05:01 -0800 X-Received: from ams.source.kernel.org (ams.source.kernel.org [145.40.68.75]) by mx.groups.io with SMTP id smtpd.web11.24923.1703088300401603340 for ; Wed, 20 Dec 2023 08:05:00 -0800 X-Received: from smtp.kernel.org (transwarp.subspace.kernel.org [100.75.92.58]) by ams.source.kernel.org (Postfix) with ESMTP id E5233B81DCE for ; Wed, 20 Dec 2023 16:04:57 +0000 (UTC) X-Received: by smtp.kernel.org (Postfix) with ESMTPSA id 49DDAC433CA for ; Wed, 20 Dec 2023 16:04:57 +0000 (UTC) X-Received: by mail-lf1-f45.google.com with SMTP id 2adb3069b0e04-50e3c6f1c10so4575350e87.1 for ; Wed, 20 Dec 2023 08:04:57 -0800 (PST) X-Gm-Message-State: hsrOmd11nnqZD0jktvXPvRvNx7686176AA= X-Google-Smtp-Source: AGHT+IEic9l5euG2aHGPTtUpGglM8MrDuDVZ7CtAzyEw22LCkA6jPOf4+QJCO+/4dtQM+r5K4gvJ5lHNtgEqxDUcvVw= X-Received: by 2002:a05:6512:3912:b0:50e:4b95:a345 with SMTP id a18-20020a056512391200b0050e4b95a345mr729202lfu.44.1703088295382; Wed, 20 Dec 2023 08:04:55 -0800 (PST) MIME-Version: 1.0 References: <20231220091032.1985-1-ray.ni@intel.com> In-Reply-To: <20231220091032.1985-1-ray.ni@intel.com> From: "Ard Biesheuvel" Date: Wed, 20 Dec 2023 17:04:44 +0100 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [edk2-devel] [PATCH] StandaloneMmPkg/Core: Remove dead code To: Ray Ni Cc: devel@edk2.groups.io, Ard Biesheuvel , Sami Mujawar 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,ardb@kernel.org List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: Content-Type: text/plain; charset="UTF-8" X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20140610 header.b=XW2LsTot; dmarc=fail reason="SPF not aligned (relaxed), DKIM not aligned (relaxed)" header.from=kernel.org (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 On Wed, 20 Dec 2023 at 10:11, Ray Ni wrote: > > Load-module-at-fixed-address feature does not work in standalone MM core. > > The patch removes the 2 dead functions and related global variables > that are related to the feature. > > Signed-off-by: Ray Ni > Cc: Sami Mujawar Reviewed-by: Ard Biesheuvel One nit below > --- > StandaloneMmPkg/Core/Dispatcher.c | 185 +----------------------- > StandaloneMmPkg/Core/StandaloneMmCore.h | 3 +- > 2 files changed, 2 insertions(+), 186 deletions(-) > > diff --git a/StandaloneMmPkg/Core/Dispatcher.c b/StandaloneMmPkg/Core/Dispatcher.c > index 7b4a3c4c55..6d43e53851 100644 > --- a/StandaloneMmPkg/Core/Dispatcher.c > +++ b/StandaloneMmPkg/Core/Dispatcher.c > @@ -29,7 +29,7 @@ > Depex - Dependency Expresion. > > Copyright (c) 2014, Hewlett-Packard Development Company, L.P. > - Copyright (c) 2009 - 2014, Intel Corporation. All rights reserved.
> + Copyright (c) 2009 - 2023, Intel Corporation. All rights reserved.
I know this is standard procedure, but you are extending Intel's copyright claim on this file by 9 years by /removing/ 180 lines? > Copyright (c) 2016 - 2021, Arm Limited. All rights reserved.
> > SPDX-License-Identifier: BSD-2-Clause-Patent > @@ -97,189 +97,6 @@ BOOLEAN gDispatcherRunning = FALSE; > // > BOOLEAN gRequestDispatch = FALSE; > > -// > -// The global variable is defined for Loading modules at fixed address feature to track the MM code > -// memory range usage. It is a bit mapped array in which every bit indicates the correspoding > -// memory page available or not. > -// > -GLOBAL_REMOVE_IF_UNREFERENCED UINT64 *mMmCodeMemoryRangeUsageBitMap = NULL; > - > -/** > - To check memory usage bit map array to figure out if the memory range in which the image will be loaded > - is available or not. If memory range is avaliable, the function will mark the corresponding bits to 1 > - which indicates the memory range is used. The function is only invoked when load modules at fixed address > - feature is enabled. > - > - @param ImageBase The base addres the image will be loaded at. > - @param ImageSize The size of the image > - > - @retval EFI_SUCCESS The memory range the image will be loaded in is available > - @retval EFI_NOT_FOUND The memory range the image will be loaded in is not available > -**/ > -EFI_STATUS > -CheckAndMarkFixLoadingMemoryUsageBitMap ( > - IN EFI_PHYSICAL_ADDRESS ImageBase, > - IN UINTN ImageSize > - ) > -{ > - UINT32 MmCodePageNumber; > - UINT64 MmCodeSize; > - EFI_PHYSICAL_ADDRESS MmCodeBase; > - UINTN BaseOffsetPageNumber; > - UINTN TopOffsetPageNumber; > - UINTN Index; > - > - // > - // Build tool will calculate the smm code size and then patch the PcdLoadFixAddressMmCodePageNumber > - // > - MmCodePageNumber = 0; > - MmCodeSize = EFI_PAGES_TO_SIZE (MmCodePageNumber); > - MmCodeBase = gLoadModuleAtFixAddressMmramBase; > - > - // > - // If the memory usage bit map is not initialized, do it. Every bit in the array > - // indicate the status of the corresponding memory page, available or not > - // > - if (mMmCodeMemoryRangeUsageBitMap == NULL) { > - mMmCodeMemoryRangeUsageBitMap = AllocateZeroPool (((MmCodePageNumber / 64) + 1) * sizeof (UINT64)); > - } > - > - // > - // If the Dxe code memory range is not allocated or the bit map array allocation failed, return EFI_NOT_FOUND > - // > - if (mMmCodeMemoryRangeUsageBitMap == NULL) { > - return EFI_NOT_FOUND; > - } > - > - // > - // see if the memory range for loading the image is in the MM code range. > - // > - if ((MmCodeBase + MmCodeSize < ImageBase + ImageSize) || (MmCodeBase > ImageBase)) { > - return EFI_NOT_FOUND; > - } > - > - // > - // Test if the memory is available or not. > - // > - BaseOffsetPageNumber = (UINTN)EFI_SIZE_TO_PAGES ((UINT32)(ImageBase - MmCodeBase)); > - TopOffsetPageNumber = (UINTN)EFI_SIZE_TO_PAGES ((UINT32)(ImageBase + ImageSize - MmCodeBase)); > - for (Index = BaseOffsetPageNumber; Index < TopOffsetPageNumber; Index++) { > - if ((mMmCodeMemoryRangeUsageBitMap[Index / 64] & LShiftU64 (1, (Index % 64))) != 0) { > - // > - // This page is already used. > - // > - return EFI_NOT_FOUND; > - } > - } > - > - // > - // Being here means the memory range is available. So mark the bits for the memory range > - // > - for (Index = BaseOffsetPageNumber; Index < TopOffsetPageNumber; Index++) { > - mMmCodeMemoryRangeUsageBitMap[Index / 64] |= LShiftU64 (1, (Index % 64)); > - } > - > - return EFI_SUCCESS; > -} > - > -/** > - Get the fixed loading address from image header assigned by build tool. This function only be called > - when Loading module at Fixed address feature enabled. > - > - @param ImageContext Pointer to the image context structure that describes the PE/COFF > - image that needs to be examined by this function. > - @retval EFI_SUCCESS An fixed loading address is assigned to this image by build tools . > - @retval EFI_NOT_FOUND The image has no assigned fixed loadding address. > - > -**/ > -EFI_STATUS > -GetPeCoffImageFixLoadingAssignedAddress ( > - IN OUT PE_COFF_LOADER_IMAGE_CONTEXT *ImageContext > - ) > -{ > - UINTN SectionHeaderOffset; > - EFI_STATUS Status; > - EFI_IMAGE_SECTION_HEADER SectionHeader; > - EFI_IMAGE_OPTIONAL_HEADER_UNION *ImgHdr; > - EFI_PHYSICAL_ADDRESS FixLoadingAddress; > - UINT16 Index; > - UINTN Size; > - UINT16 NumberOfSections; > - UINT64 ValueInSectionHeader; > - > - FixLoadingAddress = 0; > - Status = EFI_NOT_FOUND; > - > - // > - // Get PeHeader pointer > - // > - ImgHdr = (EFI_IMAGE_OPTIONAL_HEADER_UNION *)((CHAR8 *)ImageContext->Handle + ImageContext->PeCoffHeaderOffset); > - SectionHeaderOffset = ImageContext->PeCoffHeaderOffset + sizeof (UINT32) + sizeof (EFI_IMAGE_FILE_HEADER) + > - ImgHdr->Pe32.FileHeader.SizeOfOptionalHeader; > - NumberOfSections = ImgHdr->Pe32.FileHeader.NumberOfSections; > - > - // > - // Get base address from the first section header that doesn't point to code section. > - // > - for (Index = 0; Index < NumberOfSections; Index++) { > - // > - // Read section header from file > - // > - Size = sizeof (EFI_IMAGE_SECTION_HEADER); > - Status = ImageContext->ImageRead ( > - ImageContext->Handle, > - SectionHeaderOffset, > - &Size, > - &SectionHeader > - ); > - if (EFI_ERROR (Status)) { > - return Status; > - } > - > - Status = EFI_NOT_FOUND; > - > - if ((SectionHeader.Characteristics & EFI_IMAGE_SCN_CNT_CODE) == 0) { > - // > - // Build tool will save the address in PointerToRelocations & PointerToLineNumbers fields > - // in the first section header that doesn't point to code section in image header. So there > - // is an assumption that when the feature is enabled, if a module with a loading address > - // assigned by tools, the PointerToRelocations & PointerToLineNumbers fields should not be > - // Zero, or else, these 2 fields should be set to Zero > - // > - ValueInSectionHeader = ReadUnaligned64 ((UINT64 *)&SectionHeader.PointerToRelocations); > - if (ValueInSectionHeader != 0) { > - // > - // Found first section header that doesn't point to code section in which build tool saves the > - // offset to SMRAM base as image base in PointerToRelocations & PointerToLineNumbers fields > - // > - FixLoadingAddress = (EFI_PHYSICAL_ADDRESS)(gLoadModuleAtFixAddressMmramBase + (INT64)ValueInSectionHeader); > - // > - // Check if the memory range is available. > - // > - Status = CheckAndMarkFixLoadingMemoryUsageBitMap (FixLoadingAddress, (UINTN)(ImageContext->ImageSize + ImageContext->SectionAlignment)); > - if (!EFI_ERROR (Status)) { > - // > - // The assigned address is valid. Return the specified loading address > - // > - ImageContext->ImageAddress = FixLoadingAddress; > - } > - } > - > - break; > - } > - > - SectionHeaderOffset += sizeof (EFI_IMAGE_SECTION_HEADER); > - } > - > - DEBUG (( > - DEBUG_INFO|DEBUG_LOAD, > - "LOADING MODULE FIXED INFO: Loading module at fixed address %x, Status = %r\n", > - FixLoadingAddress, > - Status > - )); > - return Status; > -} > - > /** > Loads an EFI image into SMRAM. > > diff --git a/StandaloneMmPkg/Core/StandaloneMmCore.h b/StandaloneMmPkg/Core/StandaloneMmCore.h > index da23b8dc3c..3d71bc84f8 100644 > --- a/StandaloneMmPkg/Core/StandaloneMmCore.h > +++ b/StandaloneMmPkg/Core/StandaloneMmCore.h > @@ -2,7 +2,7 @@ > The internal header file includes the common header files, defines > internal structure and functions used by MmCore module. > > - Copyright (c) 2009 - 2014, Intel Corporation. All rights reserved.
> + Copyright (c) 2009 - 2023, Intel Corporation. All rights reserved.
> Copyright (c) 2016 - 2021, Arm Limited. All rights reserved.
> > SPDX-License-Identifier: BSD-2-Clause-Patent > @@ -177,7 +177,6 @@ typedef struct { > extern MM_CORE_PRIVATE_DATA *gMmCorePrivate; > extern EFI_MM_SYSTEM_TABLE gMmCoreMmst; > extern LIST_ENTRY gHandleList; > -extern EFI_PHYSICAL_ADDRESS gLoadModuleAtFixAddressMmramBase; > > /** > Called to initialize the memory service. > -- > 2.39.1.windows.1 > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#112763): https://edk2.groups.io/g/devel/message/112763 Mute This Topic: https://groups.io/mt/103278283/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=-