Re: [PATCH 17/26] MdeModulePkg PiSmmCore: Remove redundant functions

["Wang, Jian J" <jian.j.wang@intel.com>]

For HeapGuard.c part,

    Reviewed-by: Jian J Wang <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>

Regards,
Jian

From: Ni, Ruiyu
Sent: Thursday, August 09, 2018 11:12 AM
To: Zhang, Shenglei <shenglei.zhang@intel.com>; edk2-devel@lists.01.org
Cc: Dong, Eric <eric.dong@intel.com>; Zeng, Star <star.zeng@intel.com>; Wang, Jian J <jian.j.wang@intel.com>
Subject: RE: [edk2] [PATCH 17/26] MdeModulePkg PiSmmCore: Remove redundant functions

+ Wang Jian.

Thanks/Ray

> -----Original Message-----
> From: edk2-devel <edk2-devel-bounces@lists.01.org<mailto:edk2-devel-bounces@lists.01.org>> On Behalf Of shenglei
> Sent: Wednesday, August 8, 2018 4:47 PM
> To: edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
> Cc: Dong, Eric <eric.dong@intel.com<mailto:eric.dong@intel.com>>; Zeng, Star <star.zeng@intel.com<mailto:star.zeng@intel.com>>
> Subject: [edk2] [PATCH 17/26] MdeModulePkg PiSmmCore: Remove
> redundant functions
>
> The functions that are never called have been removed.
> They are IsImageInsideSmram,FindImageRecord,SmmRemoveImageRecord,
> SmmMemoryAttributesTableConsistencyCheck,DumpSmmMemoryMapEntr
> y,
> SmmMemoryMapConsistencyCheckRange,SmmMemoryMapConsistencyCh
> eck,
> DumpSmmMemoryMap,ClearGuardMapBit,SetGuardMapBit,AdjustMemory
> A,
> AdjustMemoryS,IsHeadGuard and IsTailGuard.
> https://bugzilla.tianocore.org/show_bug.cgi?id=1062
>
> Cc: Star Zeng <star.zeng@intel.com<mailto:star.zeng@intel.com>>
> Cc: Eric Dong <eric.dong@intel.com<mailto:eric.dong@intel.com>>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: shenglei <shenglei.zhang@intel.com<mailto:shenglei.zhang@intel.com>>
> ---
>  MdeModulePkg/Core/PiSmmCore/HeapGuard.c       | 166 ------------------
>  .../Core/PiSmmCore/MemoryAttributesTable.c    | 131 --------------
>  MdeModulePkg/Core/PiSmmCore/Page.c            | 121 -------------
>  3 files changed, 418 deletions(-)
>
> diff --git a/MdeModulePkg/Core/PiSmmCore/HeapGuard.c
> b/MdeModulePkg/Core/PiSmmCore/HeapGuard.c
> index d9e54b96cb..f7ae9ae286 100644
> --- a/MdeModulePkg/Core/PiSmmCore/HeapGuard.c
> +++ b/MdeModulePkg/Core/PiSmmCore/HeapGuard.c
> @@ -455,51 +455,6 @@ GetGuardMapBit (
>    return 0;
>  }
>
> -/**
> -  Set the bit in bitmap table for the given address.
> -
> -  @param[in]  Address     The address to set for.
> -
> -  @return VOID.
> -**/
> -VOID
> -EFIAPI
> -SetGuardMapBit (
> -  IN EFI_PHYSICAL_ADDRESS    Address
> -  )
> -{
> -  UINT64        *GuardMap;
> -  UINT64        BitMask;
> -
> -  FindGuardedMemoryMap (Address, TRUE, &GuardMap);
> -  if (GuardMap != NULL) {
> -    BitMask = LShiftU64 (1, GUARDED_HEAP_MAP_ENTRY_BIT_INDEX
> (Address));
> -    *GuardMap |= BitMask;
> -  }
> -}
> -
> -/**
> -  Clear the bit in bitmap table for the given address.
> -
> -  @param[in]  Address     The address to clear for.
> -
> -  @return VOID.
> -**/
> -VOID
> -EFIAPI
> -ClearGuardMapBit (
> -  IN EFI_PHYSICAL_ADDRESS    Address
> -  )
> -{
> -  UINT64        *GuardMap;
> -  UINT64        BitMask;
> -
> -  FindGuardedMemoryMap (Address, TRUE, &GuardMap);
> -  if (GuardMap != NULL) {
> -    BitMask = LShiftU64 (1, GUARDED_HEAP_MAP_ENTRY_BIT_INDEX
> (Address));
> -    *GuardMap &= ~BitMask;
> -  }
> -}
>
>  /**
>    Check to see if the page at the given address is a Guard page or not.
> @@ -526,39 +481,7 @@ IsGuardPage (
>    return ((BitMap == BIT0) || (BitMap == BIT2) || (BitMap == (BIT2 | BIT0)));  }
>
> -/**
> -  Check to see if the page at the given address is a head Guard page or not.
>
> -  @param[in]  Address     The address to check for.
> -
> -  @return TRUE  The page at Address is a head Guard page.
> -  @return FALSE The page at Address is not a head Guard page.
> -**/
> -BOOLEAN
> -EFIAPI
> -IsHeadGuard (
> -  IN EFI_PHYSICAL_ADDRESS    Address
> -  )
> -{
> -  return (GetGuardedMemoryBits (Address, 2) == BIT1); -}
> -
> -/**
> -  Check to see if the page at the given address is a tail Guard page or not.
> -
> -  @param[in]  Address     The address to check for.
> -
> -  @return TRUE  The page at Address is a tail Guard page.
> -  @return FALSE The page at Address is not a tail Guard page.
> -**/
> -BOOLEAN
> -EFIAPI
> -IsTailGuard (
> -  IN EFI_PHYSICAL_ADDRESS    Address
> -  )
> -{
> -  return (GetGuardedMemoryBits (Address - EFI_PAGE_SIZE, 2) == BIT0); -}
>
>  /**
>    Check to see if the page at the given address is guarded or not.
> @@ -864,66 +787,7 @@ UnsetGuardForMemory (
>    ClearGuardedMemoryBits(Memory, NumberOfPages);  }
>
> -/**
> -  Adjust address of free memory according to existing and/or required
> Guard.
> -
> -  This function will check if there're existing Guard pages of adjacent
> -  memory blocks, and try to use it as the Guard page of the memory to be
> -  allocated.
>
> -  @param[in]  Start           Start address of free memory block.
> -  @param[in]  Size            Size of free memory block.
> -  @param[in]  SizeRequested   Size of memory to allocate.
> -
> -  @return The end address of memory block found.
> -  @return 0 if no enough space for the required size of memory and its
> Guard.
> -**/
> -UINT64
> -AdjustMemoryS (
> -  IN UINT64                  Start,
> -  IN UINT64                  Size,
> -  IN UINT64                  SizeRequested
> -  )
> -{
> -  UINT64  Target;
> -
> -  //
> -  // UEFI spec requires that allocated pool must be 8-byte aligned. If it's
> -  // indicated to put the pool near the Tail Guard, we need extra bytes to
> -  // make sure alignment of the returned pool address.
> -  //
> -  if ((PcdGet8 (PcdHeapGuardPropertyMask) & BIT7) == 0) {
> -    SizeRequested = ALIGN_VALUE(SizeRequested, 8);
> -  }
> -
> -  Target = Start + Size - SizeRequested;
> -  ASSERT (Target >= Start);
> -  if (Target == 0) {
> -    return 0;
> -  }
> -
> -  if (!IsGuardPage (Start + Size)) {
> -    // No Guard at tail to share. One more page is needed.
> -    Target -= EFI_PAGES_TO_SIZE (1);
> -  }
> -
> -  // Out of range?
> -  if (Target < Start) {
> -    return 0;
> -  }
> -
> -  // At the edge?
> -  if (Target == Start) {
> -    if (!IsGuardPage (Target - EFI_PAGES_TO_SIZE (1))) {
> -      // No enough space for a new head Guard if no Guard at head to share.
> -      return 0;
> -    }
> -  }
> -
> -  // OK, we have enough pages for memory and its Guards. Return the End
> of the
> -  // free space.
> -  return Target + SizeRequested - 1;
> -}
>
>  /**
>    Adjust the start address and number of pages to free according to Guard.
> @@ -1049,36 +913,6 @@ AdjustMemoryF (
>    *NumberOfPages  = PagesToFree;
>  }
>
> -/**
> -  Adjust the base and number of pages to really allocate according to Guard.
> -
> -  @param[in,out]  Memory          Base address of free memory.
> -  @param[in,out]  NumberOfPages   Size of memory to allocate.
> -
> -  @return VOID.
> -**/
> -VOID
> -AdjustMemoryA (
> -  IN OUT EFI_PHYSICAL_ADDRESS    *Memory,
> -  IN OUT UINTN                   *NumberOfPages
> -  )
> -{
> -  //
> -  // FindFreePages() has already taken the Guard into account. It's safe to
> -  // adjust the start address and/or number of pages here, to make sure that
> -  // the Guards are also "allocated".
> -  //
> -  if (!IsGuardPage (*Memory + EFI_PAGES_TO_SIZE (*NumberOfPages))) {
> -    // No tail Guard, add one.
> -    *NumberOfPages += 1;
> -  }
> -
> -  if (!IsGuardPage (*Memory - EFI_PAGE_SIZE)) {
> -    // No head Guard, add one.
> -    *Memory        -= EFI_PAGE_SIZE;
> -    *NumberOfPages += 1;
> -  }
> -}
>
>  /**
>    Adjust the pool head position to make sure the Guard page is adjavent to
> diff --git a/MdeModulePkg/Core/PiSmmCore/MemoryAttributesTable.c
> b/MdeModulePkg/Core/PiSmmCore/MemoryAttributesTable.c
> index 36ccf65fa3..1682d0f9e4 100644
> --- a/MdeModulePkg/Core/PiSmmCore/MemoryAttributesTable.c
> +++ b/MdeModulePkg/Core/PiSmmCore/MemoryAttributesTable.c
> @@ -114,34 +114,6 @@ EfiSizeToPages (
>    return RShiftU64 (Size, EFI_PAGE_SHIFT) + ((((UINTN)Size) &
> EFI_PAGE_MASK) ? 1 : 0);  }
>
> -/**
> -  Check the consistency of Smm memory attributes table.
> -
> -  @param[in] MemoryAttributesTable  PI SMM memory attributes table -
> **/ -VOID -SmmMemoryAttributesTableConsistencyCheck (
> -  IN EDKII_PI_SMM_MEMORY_ATTRIBUTES_TABLE
> *MemoryAttributesTable
> -  )
> -{
> -  EFI_MEMORY_DESCRIPTOR                     *MemoryMap;
> -  UINTN                                     MemoryMapEntryCount;
> -  UINTN                                     DescriptorSize;
> -  UINTN                                     Index;
> -  UINT64                                    Address;
> -
> -  Address = 0;
> -  MemoryMapEntryCount = MemoryAttributesTable->NumberOfEntries;
> -  DescriptorSize = MemoryAttributesTable->DescriptorSize;
> -  MemoryMap = (EFI_MEMORY_DESCRIPTOR *)(MemoryAttributesTable +
> 1);
> -  for (Index = 0; Index < MemoryMapEntryCount; Index++) {
> -    if (Address != 0) {
> -      ASSERT (Address == MemoryMap->PhysicalStart);
> -    }
> -    Address = MemoryMap->PhysicalStart + EfiPagesToSize(MemoryMap-
> >NumberOfPages);
> -    MemoryMap = NEXT_MEMORY_DESCRIPTOR(MemoryMap,
> DescriptorSize);
> -  }
> -}
>
>  /**
>    Sort memory map entries based upon PhysicalStart, from low to high.
> @@ -1224,85 +1196,6 @@ Finish:
>    return ;
>  }
>
> -/**
> -  Find image record according to image base and size.
> -
> -  @param[in]  ImageBase    Base of PE image
> -  @param[in]  ImageSize    Size of PE image
> -
> -  @return image record
> -**/
> -STATIC
> -IMAGE_PROPERTIES_RECORD *
> -FindImageRecord (
> -  IN EFI_PHYSICAL_ADDRESS  ImageBase,
> -  IN UINT64                ImageSize
> -  )
> -{
> -  IMAGE_PROPERTIES_RECORD    *ImageRecord;
> -  LIST_ENTRY                 *ImageRecordLink;
> -  LIST_ENTRY                 *ImageRecordList;
> -
> -  ImageRecordList = &mImagePropertiesPrivateData.ImageRecordList;
> -
> -  for (ImageRecordLink = ImageRecordList->ForwardLink;
> -       ImageRecordLink != ImageRecordList;
> -       ImageRecordLink = ImageRecordLink->ForwardLink) {
> -    ImageRecord = CR (
> -                    ImageRecordLink,
> -                    IMAGE_PROPERTIES_RECORD,
> -                    Link,
> -                    IMAGE_PROPERTIES_RECORD_SIGNATURE
> -                    );
> -
> -    if ((ImageBase == ImageRecord->ImageBase) &&
> -        (ImageSize == ImageRecord->ImageSize)) {
> -      return ImageRecord;
> -    }
> -  }
> -
> -  return NULL;
> -}
> -
> -/**
> -  Remove Image record.
> -
> -  @param[in]  DriverEntry    Driver information
> -**/
> -VOID
> -SmmRemoveImageRecord (
> -  IN EFI_SMM_DRIVER_ENTRY  *DriverEntry
> -  )
> -{
> -  IMAGE_PROPERTIES_RECORD              *ImageRecord;
> -  LIST_ENTRY                           *CodeSegmentListHead;
> -  IMAGE_PROPERTIES_RECORD_CODE_SECTION *ImageRecordCodeSection;
> -
> -  DEBUG ((DEBUG_VERBOSE, "SMM RemoveImageRecord - 0x%x\n",
> DriverEntry));
> -  DEBUG ((DEBUG_VERBOSE, "SMM RemoveImageRecord - 0x%016lx -
> 0x%016lx\n", DriverEntry->ImageBuffer, DriverEntry->NumberOfPage));
> -
> -  ImageRecord = FindImageRecord (DriverEntry->ImageBuffer,
> EfiPagesToSize(DriverEntry->NumberOfPage));
> -  if (ImageRecord == NULL) {
> -    DEBUG ((DEBUG_ERROR, "SMM !!!!!!!! ImageRecord not
> found !!!!!!!!\n"));
> -    return ;
> -  }
> -
> -  CodeSegmentListHead = &ImageRecord->CodeSegmentList;
> -  while (!IsListEmpty (CodeSegmentListHead)) {
> -    ImageRecordCodeSection = CR (
> -                               CodeSegmentListHead->ForwardLink,
> -                               IMAGE_PROPERTIES_RECORD_CODE_SECTION,
> -                               Link,
> -                               IMAGE_PROPERTIES_RECORD_CODE_SECTION_SIGNATURE
> -                               );
> -    RemoveEntryList (&ImageRecordCodeSection->Link);
> -    FreePool (ImageRecordCodeSection);
> -  }
> -
> -  RemoveEntryList (&ImageRecord->Link);
> -  FreePool (ImageRecord);
> -  mImagePropertiesPrivateData.ImageRecordCount--;
> -}
>
>  /**
>    Publish MemoryAttributesTable to SMM configuration table.
> @@ -1386,30 +1279,6 @@ PublishMemoryAttributesTable (
>    ASSERT_EFI_ERROR (Status);
>  }
>
> -/**
> -  This function returns if image is inside SMRAM.
> -
> -  @param[in] LoadedImage LoadedImage protocol instance for an image.
> -
> -  @retval TRUE  the image is inside SMRAM.
> -  @retval FALSE the image is outside SMRAM.
> -**/
> -BOOLEAN
> -IsImageInsideSmram (
> -  IN EFI_LOADED_IMAGE_PROTOCOL   *LoadedImage
> -  )
> -{
> -  UINTN  Index;
> -
> -  for (Index = 0; Index < mFullSmramRangeCount; Index++) {
> -    if ((mFullSmramRanges[Index].PhysicalStart <= (UINTN)LoadedImage-
> >ImageBase)&&
> -        (mFullSmramRanges[Index].PhysicalStart +
> mFullSmramRanges[Index].PhysicalSize >= (UINTN)LoadedImage-
> >ImageBase + LoadedImage->ImageSize)) {
> -      return TRUE;
> -    }
> -  }
> -
> -  return FALSE;
> -}
>
>  /**
>    This function installs all SMM image record information.
> diff --git a/MdeModulePkg/Core/PiSmmCore/Page.c
> b/MdeModulePkg/Core/PiSmmCore/Page.c
> index 3699af7424..cd7d7ece0c 100644
> --- a/MdeModulePkg/Core/PiSmmCore/Page.c
> +++ b/MdeModulePkg/Core/PiSmmCore/Page.c
> @@ -451,128 +451,7 @@ GetSmmMemoryMapEntryCount (
>    return Count;
>  }
>
> -/**
> -  Dump Smm memory map entry.
> -**/
> -VOID
> -DumpSmmMemoryMapEntry (
> -  VOID
> -  )
> -{
> -  LIST_ENTRY               *Link;
> -  MEMORY_MAP               *Entry;
> -  EFI_PHYSICAL_ADDRESS     Last;
> -
> -  Last = 0;
> -  DEBUG ((DEBUG_INFO, "DumpSmmMemoryMapEntry:\n"));
> -  Link = gMemoryMap.ForwardLink;
> -  while (Link != &gMemoryMap) {
> -    Entry = CR (Link, MEMORY_MAP, Link, MEMORY_MAP_SIGNATURE);
> -    Link  = Link->ForwardLink;
> -
> -    if ((Last != 0) && (Last != (UINT64)-1)) {
> -      if (Last + 1 != Entry->Start) {
> -        Last = (UINT64)-1;
> -      } else {
> -        Last = Entry->End;
> -      }
> -    } else if (Last == 0) {
> -      Last = Entry->End;
> -    }
> -
> -    DEBUG ((DEBUG_INFO, "Entry (Link - 0x%x)\n", &Entry->Link));
> -    DEBUG ((DEBUG_INFO, "  Signature         - 0x%x\n", Entry->Signature));
> -    DEBUG ((DEBUG_INFO, "  Link.ForwardLink  - 0x%x\n", Entry-
> >Link.ForwardLink));
> -    DEBUG ((DEBUG_INFO, "  Link.BackLink     - 0x%x\n", Entry-
> >Link.BackLink));
> -    DEBUG ((DEBUG_INFO, "  Type              - 0x%x\n", Entry->Type));
> -    DEBUG ((DEBUG_INFO, "  Start             - 0x%016lx\n", Entry->Start));
> -    DEBUG ((DEBUG_INFO, "  End               - 0x%016lx\n", Entry->End));
> -  }
> -
> -  ASSERT (Last != (UINT64)-1);
> -}
> -
> -/**
> -  Dump Smm memory map.
> -**/
> -VOID
> -DumpSmmMemoryMap (
> -  VOID
> -  )
> -{
> -  LIST_ENTRY      *Node;
> -  FREE_PAGE_LIST  *Pages;
> -
> -  DEBUG ((DEBUG_INFO, "DumpSmmMemoryMap\n"));
> -
> -  Pages = NULL;
> -  Node = mSmmMemoryMap.ForwardLink;
> -  while (Node != &mSmmMemoryMap) {
> -    Pages = BASE_CR (Node, FREE_PAGE_LIST, Link);
> -    DEBUG ((DEBUG_INFO, "Pages - 0x%x\n", Pages));
> -    DEBUG ((DEBUG_INFO, "Pages->NumberOfPages - 0x%x\n", Pages-
> >NumberOfPages));
> -    Node = Node->ForwardLink;
> -  }
> -}
>
> -/**
> -  Check if a Smm base~length is in Smm memory map.
> -
> -  @param[in] Base   The base address of Smm memory to be checked.
> -  @param[in] Length THe length of Smm memory to be checked.
> -
> -  @retval TRUE  Smm base~length is in smm memory map.
> -  @retval FALSE Smm base~length is in smm memory map.
> -**/
> -BOOLEAN
> -SmmMemoryMapConsistencyCheckRange (
> -  IN EFI_PHYSICAL_ADDRESS Base,
> -  IN UINTN                Length
> -  )
> -{
> -  LIST_ENTRY               *Link;
> -  MEMORY_MAP               *Entry;
> -  BOOLEAN                  Result;
> -
> -  Result = FALSE;
> -  Link = gMemoryMap.ForwardLink;
> -  while (Link != &gMemoryMap) {
> -    Entry = CR (Link, MEMORY_MAP, Link, MEMORY_MAP_SIGNATURE);
> -    Link  = Link->ForwardLink;
> -
> -    if (Entry->Type != EfiConventionalMemory) {
> -      continue;
> -    }
> -    if (Entry->Start == Base && Entry->End == Base + Length - 1) {
> -      Result = TRUE;
> -      break;
> -    }
> -  }
> -
> -  return Result;
> -}
> -
> -/**
> -  Check the consistency of Smm memory map.
> -**/
> -VOID
> -SmmMemoryMapConsistencyCheck (
> -  VOID
> -  )
> -{
> -  LIST_ENTRY      *Node;
> -  FREE_PAGE_LIST  *Pages;
> -  BOOLEAN         Result;
> -
> -  Pages = NULL;
> -  Node = mSmmMemoryMap.ForwardLink;
> -  while (Node != &mSmmMemoryMap) {
> -    Pages = BASE_CR (Node, FREE_PAGE_LIST, Link);
> -    Result = SmmMemoryMapConsistencyCheckRange
> ((EFI_PHYSICAL_ADDRESS)(UINTN)Pages,
> (UINTN)EFI_PAGES_TO_SIZE(Pages->NumberOfPages));
> -    ASSERT (Result);
> -    Node = Node->ForwardLink;
> -  }
> -}
>
>  /**
>    Internal Function. Allocate n pages from given free page node.
> --
> 2.18.0.windows.1
>
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
> https://lists.01.org/mailman/listinfo/edk2-devel