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

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

On 28 February 2018 at 15:55, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> 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>
>

Thanks

Pushed as f3b314331ce1..44d2e8d7cab3


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