Re: [PATCH v2 5/5] MdeModulePkg/Core: fix-up for changes introduced by freed-memory guard

[Laszlo Ersek <lersek@redhat.com>]

On 10/23/18 16:53, Jian J Wang wrote:
>> v2 changes:
>> a. Update IsHeapGuardEnabled() calling because its prototype change
> 
> Two changes are fixed up:
> a. Prototype change of function IsHeapGuardEnabled()

This kind of separation, between the patches, is wrong then. If someone
bisects the edk2 git history, and checks out the edk2 tree at patch #4
in this series, they will get a build failure.

> b. More memory map descriptors are introduced by new feature.
>    MergeMemoryMap() is updated to merge freed-pages into adjacent memory
>    descriptor to reduce the overall descriptor number.

In such cases, the usual way to structure the patch series is as follows:

- First the patch is added that makes the dependent / consumer code more
generic, so that it can later cope with the new feature. Right after
this "prep" patch, the new code paths in the consumer code are not
exercised in practice. Importantly however, there is neither a
compilation failure, nor a functionality error. It's just that the new
additions are not active yet; they work as NOPs.

- Second, the patch with the new feature is added; it basically "snaps
in place", and activates the code paths that were prepared earlier.

In large patch sets, it's not uncommon to see 5+ "prep" patches, each
equipping a separate aspect (or consumer site) for the new feature,
gradually. And then the final feature patch is plugged in.

Thanks
Laszlo

> 
> Cc: Star Zeng <star.zeng@intel.com>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Jian J Wang <jian.j.wang@intel.com>
> ---
>  MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c |  2 +-
>  MdeModulePkg/Core/Dxe/Misc/PropertiesTable.c  | 16 +++++++---------
>  2 files changed, 8 insertions(+), 10 deletions(-)
> 
> diff --git a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
> index fa8f8fe91a..6298b67db1 100644
> --- a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
> +++ b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
> @@ -1250,7 +1250,7 @@ ApplyMemoryProtectionPolicy (
>    // Don't overwrite Guard pages, which should be the first and/or last page,
>    // if any.
>    //
> -  if (IsHeapGuardEnabled ()) {
> +  if (IsHeapGuardEnabled (GUARD_HEAP_TYPE_PAGE|GUARD_HEAP_TYPE_POOL)) {
>      if (IsGuardPage (Memory))  {
>        Memory += EFI_PAGE_SIZE;
>        Length -= EFI_PAGE_SIZE;
> diff --git a/MdeModulePkg/Core/Dxe/Misc/PropertiesTable.c b/MdeModulePkg/Core/Dxe/Misc/PropertiesTable.c
> index 05eb4f422b..f6595c90ed 100644
> --- a/MdeModulePkg/Core/Dxe/Misc/PropertiesTable.c
> +++ b/MdeModulePkg/Core/Dxe/Misc/PropertiesTable.c
> @@ -32,6 +32,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
>  #include <Guid/PropertiesTable.h>
>  
>  #include "DxeMain.h"
> +#include "HeapGuard.h"
>  
>  #define PREVIOUS_MEMORY_DESCRIPTOR(MemoryDescriptor, Size) \
>    ((EFI_MEMORY_DESCRIPTOR *)((UINT8 *)(MemoryDescriptor) - (Size)))
> @@ -205,16 +206,13 @@ MergeMemoryMap (
>      NextMemoryMapEntry = NEXT_MEMORY_DESCRIPTOR (MemoryMapEntry, DescriptorSize);
>  
>      do {
> -      MemoryBlockLength = (UINT64) (EfiPagesToSize (MemoryMapEntry->NumberOfPages));
> +      MergeGuardPages (NewMemoryMapEntry, NextMemoryMapEntry->PhysicalStart);
> +      MemoryBlockLength = (UINT64) (EfiPagesToSize (NewMemoryMapEntry->NumberOfPages));
>        if (((UINTN)NextMemoryMapEntry < (UINTN)MemoryMapEnd) &&
> -          (MemoryMapEntry->Type == NextMemoryMapEntry->Type) &&
> -          (MemoryMapEntry->Attribute == NextMemoryMapEntry->Attribute) &&
> -          ((MemoryMapEntry->PhysicalStart + MemoryBlockLength) == NextMemoryMapEntry->PhysicalStart)) {
> -        MemoryMapEntry->NumberOfPages += NextMemoryMapEntry->NumberOfPages;
> -        if (NewMemoryMapEntry != MemoryMapEntry) {
> -          NewMemoryMapEntry->NumberOfPages += NextMemoryMapEntry->NumberOfPages;
> -        }
> -
> +          (NewMemoryMapEntry->Type == NextMemoryMapEntry->Type) &&
> +          (NewMemoryMapEntry->Attribute == NextMemoryMapEntry->Attribute) &&
> +          ((NewMemoryMapEntry->PhysicalStart + MemoryBlockLength) == NextMemoryMapEntry->PhysicalStart)) {
> +        NewMemoryMapEntry->NumberOfPages += NextMemoryMapEntry->NumberOfPages;
>          NextMemoryMapEntry = NEXT_MEMORY_DESCRIPTOR (NextMemoryMapEntry, DescriptorSize);
>          continue;
>        } else {
>