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

[Vladimir Olovyannikov <vladimir.olovyannikov@broadcom.com>]

> -----Original Message-----
> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
> Sent: Tuesday, January 2, 2018 7:51 AM
> To: edk2-devel@lists.01.org
> Cc: leif.lindholm@linaro.org; vladimir.olovyannikov@broadcom.com;
> udit.kumar@nxp.com; meenakshi.aggarwal@nxp.com; Ard Biesheuvel
> 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.
Thanks Ard,
Works for me.

Vladimir
>
>  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