Re: [edk2-devel] [PATCH] StandaloneMmPkg/Core: Remove dead code

["Ard Biesheuvel" <ardb@kernel.org>]

On Wed, 20 Dec 2023 at 10:11, Ray Ni <ray.ni@intel.com> 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 <ray.ni@intel.com>
> Cc: Sami Mujawar <sami.mujawar@arm.com>

Reviewed-by: Ard Biesheuvel <ardb@kernel.org>

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.<BR>
> +  Copyright (c) 2009 - 2023, Intel Corporation. All rights reserved.<BR>

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.<BR>
>
>    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.<BR>
> +  Copyright (c) 2009 - 2023, Intel Corporation. All rights reserved.<BR>
>    Copyright (c) 2016 - 2021, Arm Limited. All rights reserved.<BR>
>
>    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]
-=-=-=-=-=-=-=-=-=-=-=-