diff mbox series

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

Message ID 20180228154441.10554-2-ard.biesheuvel@linaro.org
State Accepted
Commit 44d2e8d7cab3ee1be0cf02f604d833e6b75b04b2
Headers show
Series [edk2,1/2] ArmPlatformPkg/MemoryInitPeiLib: revert "don't reserve primary FV in memory" | expand

Commit Message

Ard Biesheuvel Feb. 28, 2018, 3:44 p.m. UTC 
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(-)

-- 
2.11.0

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Comments

Leif Lindholm Feb. 28, 2018, 3:55 p.m. UTC | #1 
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

> 

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Ard Biesheuvel Feb. 28, 2018, 4:12 p.m. UTC | #2 
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

>>

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
diff mbox series

Patch

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