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

[Udit Kumar <udit.kumar@nxp.com>]

Thanks Ard, 
This works for us as well 
Few comments inline 


Regards
Udit

> -----Original Message-----
> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
> Sent: Tuesday, January 02, 2018 9:21 PM
> To: edk2-devel@lists.01.org
> Cc: leif.lindholm@linaro.org; vladimir.olovyannikov@broadcom.com; Udit
> Kumar <udit.kumar@nxp.com>; Meenakshi Aggarwal
> <meenakshi.aggarwal@nxp.com>; Ard Biesheuvel
> <ard.biesheuvel@linaro.org>
> Subject: [PATCH] ArmPlatformPkg/MemoryInitPeiLib: mark primary FV region
> as boot services data
> 
> 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>
> ---
> 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

Please correct comments, now this memory is present 

> +            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));

Hob List is already created for PcdSystemMemoryBase and its size 
Within this, we got Fd, then do we want to create another Hob here 

> +
> +          // 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);

IMO, only this call should be enough to protect FD area. 

> +
> +        Found = TRUE;
> +        break;
> +      }
> +      NextHob.Raw = GET_NEXT_HOB (NextHob);
> +    }
> +
> +    ASSERT(Found);
> +  }
> +
>    // Build Memory Allocation Hob
>    InitMmu (MemoryTable);
> 
> --
> 2.11.0