Re: [PATCH] ArmPlatformPkg/MemoryInitPeiLib: mark primary FV region as boot services data

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

On Tue, Jan 02, 2018 at 03:50:34PM +0000, Ard Biesheuvel wrote:
> Commit 8ae5fc182941 ("ArmPlatformPkg/MemoryInitPeiLib: don't reserve
> primary FV in memory") deleted the code that removes the memory covering
> the primary firmware volume from the memory map. The assumption was that
> this is no longer necessary now that we no longer expose compression and
> PE/COFF loader library code from the PrePi module to DXE core.
> 
> However, the FV is still declared, and so code may attempt to access it
> anyway, which may cause unexpected results depending on whether the
> memory has been reused for other purposes in the mean time.
> 
> So reinstate the code that splits off the resource descriptor HOB that
> describes the firmware device, but this time, don't mark the memory as
> unusable, but create a memory allocation HOB that marks the region as
> boot services data.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

So, this looks like a necessary fix.
However, there are many line-length violations below.
And it's not apparent how fixing that would make the code more
readable.

So rather than asking for a rewrite - could you split the revert
from the modification?

/
    Leif

> Vladimir, Udit, Meenakshi: please confirm whether this code works for you.
> 
>  ArmPlatformPkg/MemoryInitPei/MemoryInitPeiLib.c | 74 ++++++++++++++++++++
>  1 file changed, 74 insertions(+)
> 
> diff --git a/ArmPlatformPkg/MemoryInitPei/MemoryInitPeiLib.c b/ArmPlatformPkg/MemoryInitPei/MemoryInitPeiLib.c
> index d03214b5df66..d1b5c0be9497 100644
> --- a/ArmPlatformPkg/MemoryInitPei/MemoryInitPeiLib.c
> +++ b/ArmPlatformPkg/MemoryInitPei/MemoryInitPeiLib.c
> @@ -70,7 +70,11 @@ MemoryPeim (
>  {
>    ARM_MEMORY_REGION_DESCRIPTOR *MemoryTable;
>    EFI_RESOURCE_ATTRIBUTE_TYPE  ResourceAttributes;
> +  UINT64                       ResourceLength;
>    EFI_PEI_HOB_POINTERS         NextHob;
> +  EFI_PHYSICAL_ADDRESS         FdTop;
> +  EFI_PHYSICAL_ADDRESS         SystemMemoryTop;
> +  EFI_PHYSICAL_ADDRESS         ResourceTop;
>    BOOLEAN                      Found;
>  
>    // Get Virtual Memory Map from the Platform Library
> @@ -117,6 +121,76 @@ MemoryPeim (
>      );
>    }
>  
> +  //
> +  // Reserve the memory space occupied by the firmware volume
> +  //
> +
> +  SystemMemoryTop = (EFI_PHYSICAL_ADDRESS)PcdGet64 (PcdSystemMemoryBase) + (EFI_PHYSICAL_ADDRESS)PcdGet64 (PcdSystemMemorySize);
> +  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 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;
> +
> +    // Search for System Memory Hob that contains the firmware
> +    NextHob.Raw = GetHobList ();
> +    while ((NextHob.Raw = GetNextHob (EFI_HOB_TYPE_RESOURCE_DESCRIPTOR, NextHob.Raw)) != NULL) {
> +      if ((NextHob.ResourceDescriptor->ResourceType == EFI_RESOURCE_SYSTEM_MEMORY) &&
> +          (PcdGet64 (PcdFdBaseAddress) >= NextHob.ResourceDescriptor->PhysicalStart) &&
> +          (FdTop <= NextHob.ResourceDescriptor->PhysicalStart + NextHob.ResourceDescriptor->ResourceLength))
> +      {
> +        ResourceAttributes = NextHob.ResourceDescriptor->ResourceAttribute;
> +        ResourceLength = NextHob.ResourceDescriptor->ResourceLength;
> +        ResourceTop = NextHob.ResourceDescriptor->PhysicalStart + ResourceLength;
> +
> +        if (PcdGet64 (PcdFdBaseAddress) == NextHob.ResourceDescriptor->PhysicalStart) {
> +          if (SystemMemoryTop != FdTop) {
> +            // Create the System Memory HOB for the firmware with the non-present attribute
> +            BuildResourceDescriptorHob (EFI_RESOURCE_SYSTEM_MEMORY,
> +                                        ResourceAttributes,
> +                                        PcdGet64 (PcdFdBaseAddress),
> +                                        PcdGet32 (PcdFdSize));
> +
> +            // Top of the FD is system memory available for UEFI
> +            NextHob.ResourceDescriptor->PhysicalStart += PcdGet32(PcdFdSize);
> +            NextHob.ResourceDescriptor->ResourceLength -= PcdGet32(PcdFdSize);
> +          }
> +        } else {
> +          // Create the System Memory HOB for the firmware
> +          BuildResourceDescriptorHob (EFI_RESOURCE_SYSTEM_MEMORY,
> +                                      ResourceAttributes,
> +                                      PcdGet64 (PcdFdBaseAddress),
> +                                      PcdGet32 (PcdFdSize));
> +
> +          // Update the HOB
> +          NextHob.ResourceDescriptor->ResourceLength = PcdGet64 (PcdFdBaseAddress) - NextHob.ResourceDescriptor->PhysicalStart;
> +
> +          // If there is some memory available on the top of the FD then create a HOB
> +          if (FdTop < NextHob.ResourceDescriptor->PhysicalStart + ResourceLength) {
> +            // Create the System Memory HOB for the remaining region (top of the FD)
> +            BuildResourceDescriptorHob (EFI_RESOURCE_SYSTEM_MEMORY,
> +                                        ResourceAttributes,
> +                                        FdTop,
> +                                        ResourceTop - FdTop);
> +          }
> +        }
> +
> +        // Mark the memory covering the Firmware Device as boot services data
> +        BuildMemoryAllocationHob (PcdGet64 (PcdFdBaseAddress),
> +                                  PcdGet32 (PcdFdSize),
> +                                  EfiBootServicesData);
> +
> +        Found = TRUE;
> +        break;
> +      }
> +      NextHob.Raw = GET_NEXT_HOB (NextHob);
> +    }
> +
> +    ASSERT(Found);
> +  }
> +
>    // Build Memory Allocation Hob
>    InitMmu (MemoryTable);
>  
> -- 
> 2.11.0
>