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

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

Leif,

On 3 January 2018 at 07:44, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> On 3 January 2018 at 05:09, Udit Kumar <udit.kumar@nxp.com> wrote:
>> 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
>>
>
> Yes
>
>>> +            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
>>
>
> The resource descriptor for PcdSystemMemoryBase/Size is updated in the
> next line so it no longer covers the FD
>
>>> +
>>> +          // 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.
>>
>
> I agree, but the reality is that it is not enough. in
> CoreInitializeGcdServices, the first system memory resource descriptor
> is claimed before the memory allocations are processed, and any memory
> allocation HOBs that intersect that region are ignored.
>
>>> +
>>> +        Found = TRUE;
>>> +        break;
>>> +      }
>>> +      NextHob.Raw = GET_NEXT_HOB (NextHob);
>>> +    }
>>> +
>>> +    ASSERT(Found);
>>> +  }
>>> +
>>>    // Build Memory Allocation Hob
>>>    InitMmu (MemoryTable);
>>>
>>> --
>>> 2.11.0
>>

Any thoughts? I can resend the patch if you like.