Re: [PATCH 2/2] ArmPlatformPkg/MemoryInitPeiLib: reserve rather than remove FV memory

[Leif Lindholm <leif.lindholm@linaro.org>]

On Wed, Feb 28, 2018 at 03:44:41PM +0000, Ard Biesheuvel wrote:
> Instead of completely removing the memory occupied by the primary PrePi
> FV from the memory map, thereby making it inaccessible to the OS, mark
> it as boot services data. This will ensure that the memory is left
> untouched by the firmware, but will release it to the OS when it calls
> ExitBootServices().
> 
> Note that for reasons that are not entirely clear, this only works as
> desired if the memory allocation HOB and the resource descriptor HOB
> that describe the region are identical in offset and size, and so we
> still need to iterate over the descriptors and split them up.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

Thanks for the rework - for the series:
Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>

> ---
>  ArmPlatformPkg/MemoryInitPei/MemoryInitPeiLib.c | 21 ++++++++++++--------
>  1 file changed, 13 insertions(+), 8 deletions(-)
> 
> diff --git a/ArmPlatformPkg/MemoryInitPei/MemoryInitPeiLib.c b/ArmPlatformPkg/MemoryInitPei/MemoryInitPeiLib.c
> index 2feb11f21d5d..01fd028dbd55 100644
> --- a/ArmPlatformPkg/MemoryInitPei/MemoryInitPeiLib.c
> +++ b/ArmPlatformPkg/MemoryInitPei/MemoryInitPeiLib.c
> @@ -129,7 +129,8 @@ MemoryPeim (
>    FdTop = (EFI_PHYSICAL_ADDRESS)PcdGet64 (PcdFdBaseAddress) + (EFI_PHYSICAL_ADDRESS)PcdGet32 (PcdFdSize);
>  
>    // EDK2 does not have the concept of boot firmware copied into DRAM. To avoid the DXE
> -  // core to overwrite this area we must mark the region with the attribute non-present
> +  // core to overwrite this area we must create a memory allocation HOB for the region,
> +  // but this only works if we split off the underlying resource descriptor as well.
>    if ((PcdGet64 (PcdFdBaseAddress) >= PcdGet64 (PcdSystemMemoryBase)) && (FdTop <= SystemMemoryTop)) {
>      Found = FALSE;
>  
> @@ -145,12 +146,10 @@ MemoryPeim (
>          ResourceTop = NextHob.ResourceDescriptor->PhysicalStart + ResourceLength;
>  
>          if (PcdGet64 (PcdFdBaseAddress) == NextHob.ResourceDescriptor->PhysicalStart) {
> -          if (SystemMemoryTop == FdTop) {
> -            NextHob.ResourceDescriptor->ResourceAttribute = ResourceAttributes & ~EFI_RESOURCE_ATTRIBUTE_PRESENT;
> -          } else {
> -            // Create the System Memory HOB for the firmware with the non-present attribute
> +          if (SystemMemoryTop != FdTop) {
> +            // Create the System Memory HOB for the firmware
>              BuildResourceDescriptorHob (EFI_RESOURCE_SYSTEM_MEMORY,
> -                                        ResourceAttributes & ~EFI_RESOURCE_ATTRIBUTE_PRESENT,
> +                                        ResourceAttributes,
>                                          PcdGet64 (PcdFdBaseAddress),
>                                          PcdGet32 (PcdFdSize));
>  
> @@ -159,9 +158,9 @@ MemoryPeim (
>              NextHob.ResourceDescriptor->ResourceLength -= PcdGet32(PcdFdSize);
>            }
>          } else {
> -          // Create the System Memory HOB for the firmware with the non-present attribute
> +          // Create the System Memory HOB for the firmware
>            BuildResourceDescriptorHob (EFI_RESOURCE_SYSTEM_MEMORY,
> -                                      ResourceAttributes & ~EFI_RESOURCE_ATTRIBUTE_PRESENT,
> +                                      ResourceAttributes,
>                                        PcdGet64 (PcdFdBaseAddress),
>                                        PcdGet32 (PcdFdSize));
>  
> @@ -177,6 +176,12 @@ MemoryPeim (
>                                          ResourceTop - FdTop);
>            }
>          }
> +
> +        // Mark the memory covering the Firmware Device as boot services data
> +        BuildMemoryAllocationHob (PcdGet64 (PcdFdBaseAddress),
> +                                  PcdGet32 (PcdFdSize),
> +                                  EfiBootServicesData);
> +
>          Found = TRUE;
>          break;
>        }
> -- 
> 2.11.0
>