[PATCH 1/2] ArmPlatformPkg/MemoryInitPeiLib: revert "don't reserve primary FV in memory"

[PATCH 1/2] ArmPlatformPkg/MemoryInitPeiLib: revert "don't reserve primary FV in memory"

[Ard Biesheuvel]

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 revert
this change in preparation of dealing with this in a better way.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 ArmPlatformPkg/MemoryInitPei/MemoryInitPeiLib.c | 69 ++++++++++++++++++++
 1 file changed, 69 insertions(+)

diff --git a/ArmPlatformPkg/MemoryInitPei/MemoryInitPeiLib.c b/ArmPlatformPkg/MemoryInitPei/MemoryInitPeiLib.c
index d03214b5df66..2feb11f21d5d 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,71 @@ MemoryPeim (
     );
   }
 
+  //
+  // Reserved 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 mark the region with the attribute non-present
+  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) {
+            NextHob.ResourceDescriptor->ResourceAttribute = ResourceAttributes & ~EFI_RESOURCE_ATTRIBUTE_PRESENT;
+          } else {
+            // Create the System Memory HOB for the firmware with the non-present attribute
+            BuildResourceDescriptorHob (EFI_RESOURCE_SYSTEM_MEMORY,
+                                        ResourceAttributes & ~EFI_RESOURCE_ATTRIBUTE_PRESENT,
+                                        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 with the non-present attribute
+          BuildResourceDescriptorHob (EFI_RESOURCE_SYSTEM_MEMORY,
+                                      ResourceAttributes & ~EFI_RESOURCE_ATTRIBUTE_PRESENT,
+                                      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);
+          }
+        }
+        Found = TRUE;
+        break;
+      }
+      NextHob.Raw = GET_NEXT_HOB (NextHob);
+    }
+
+    ASSERT(Found);
+  }
+
   // Build Memory Allocation Hob
   InitMmu (MemoryTable);
 
-- 
2.11.0

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

[Ard Biesheuvel]

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

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

[Leif Lindholm]

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>

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

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

[Ard Biesheuvel]

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