1. Patchew
  2. EDK2
  3. [edk2] [PATCH v2 0/4] ArmPkg, ArmVirtpkg ARM: enable strict memory protection
  4. View patch

[edk2] [PATCH v2 1/4] ArmPkg/CpuDxe ARM: avoid splitting page table sections unnecessarily

Ard Biesheuvel posted 4 patches 8 years, 11 months ago
There is a newer version of this series
[edk2] [PATCH v2 1/4] ArmPkg/CpuDxe ARM: avoid splitting page table sections unnecessarily
Posted by Ard Biesheuvel 8 years, 11 months ago 
Currently, any range passed to CpuArchProtocol::SetMemoryAttributes is
fully broken down into page mappings if the start or the size of the
region happens to be misaliged relative to the section size of 1 MB.

This is going to hurt when we enable strict memory permissions, given
that we remap the entire RAM space non-executable (modulo the code
bits) when the CpuArchProtocol is installed.

So refactor the code to iterate over the range in a way that ensures
that all naturally aligned section sized subregions are not broken up.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 ArmPkg/Drivers/CpuDxe/Arm/Mmu.c | 47 ++++++++++++++++----
 1 file changed, 39 insertions(+), 8 deletions(-)

diff --git a/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c b/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c
index 89e429925ba9..ce4d529bda67 100644
--- a/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c
+++ b/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c
@@ -679,6 +679,7 @@ SetMemoryAttributes (
   )
 {
   EFI_STATUS    Status;
+  UINT64        ChunkLength;
 
   //
   // Ignore invocations that only modify permission bits
@@ -687,14 +688,44 @@ SetMemoryAttributes (
     return EFI_SUCCESS;
   }
 
-  if(((BaseAddress & 0xFFFFF) == 0) && ((Length & 0xFFFFF) == 0)) {
-    // Is the base and length a multiple of 1 MB?
-    DEBUG ((EFI_D_PAGE, "SetMemoryAttributes(): MMU section 0x%x length 0x%x to %lx\n", (UINTN)BaseAddress, (UINTN)Length, Attributes));
-    Status = UpdateSectionEntries (BaseAddress, Length, Attributes, VirtualMask);
-  } else {
-    // Base and/or length is not a multiple of 1 MB
-    DEBUG ((EFI_D_PAGE, "SetMemoryAttributes(): MMU page 0x%x length 0x%x to %lx\n", (UINTN)BaseAddress, (UINTN)Length, Attributes));
-    Status = UpdatePageEntries (BaseAddress, Length, Attributes, VirtualMask);
+  while (Length > 0) {
+    if ((BaseAddress % TT_DESCRIPTOR_SECTION_SIZE == 0) &&
+        Length >= TT_DESCRIPTOR_SECTION_SIZE) {
+
+      ChunkLength = Length - Length % TT_DESCRIPTOR_SECTION_SIZE;
+
+      DEBUG ((DEBUG_PAGE,
+        "SetMemoryAttributes(): MMU section 0x%lx length 0x%lx to %lx\n",
+        BaseAddress, ChunkLength, Attributes));
+
+      Status = UpdateSectionEntries (BaseAddress, ChunkLength, Attributes,
+                 VirtualMask);
+    } else {
+
+      //
+      // Process page by page until the next section boundary, but only if
+      // we have more than a section's worth of area to deal with after that.
+      //
+      ChunkLength = TT_DESCRIPTOR_SECTION_SIZE -
+                    (BaseAddress % TT_DESCRIPTOR_SECTION_SIZE);
+      if (ChunkLength + TT_DESCRIPTOR_SECTION_SIZE > Length) {
+        ChunkLength = Length;
+      }
+
+      DEBUG ((DEBUG_PAGE,
+        "SetMemoryAttributes(): MMU page 0x%lx length 0x%lx to %lx\n",
+        BaseAddress, ChunkLength, Attributes));
+
+      Status = UpdatePageEntries (BaseAddress, ChunkLength, Attributes,
+                 VirtualMask);
+    }
+
+    if (EFI_ERROR (Status)) {
+      break;
+    }
+
+    BaseAddress += ChunkLength;
+    Length -= ChunkLength;
   }
 
   // Flush d-cache so descriptors make it back to uncached memory for subsequent table walks
-- 
2.7.4

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH v2 1/4] ArmPkg/CpuDxe ARM: avoid splitting page table sections unnecessarily
Posted by Leif Lindholm 8 years, 11 months ago 
On Thu, Mar 02, 2017 at 10:36:13AM +0000, Ard Biesheuvel wrote:
> Currently, any range passed to CpuArchProtocol::SetMemoryAttributes is
> fully broken down into page mappings if the start or the size of the
> region happens to be misaliged relative to the section size of 1 MB.
> 
> This is going to hurt when we enable strict memory permissions, given

"Hurt" -> "cause unnecessary performance penalties" or "hurt" ->
"break everything"?

> that we remap the entire RAM space non-executable (modulo the code
> bits) when the CpuArchProtocol is installed.
> 
> So refactor the code to iterate over the range in a way that ensures
> that all naturally aligned section sized subregions are not broken up.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

Many thanks for getting rid of the magic values, and in general making
the code more logical. One question below:

> ---
>  ArmPkg/Drivers/CpuDxe/Arm/Mmu.c | 47 ++++++++++++++++----
>  1 file changed, 39 insertions(+), 8 deletions(-)
> 
> diff --git a/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c b/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c
> index 89e429925ba9..ce4d529bda67 100644
> --- a/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c
> +++ b/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c
> @@ -679,6 +679,7 @@ SetMemoryAttributes (
>    )
>  {
>    EFI_STATUS    Status;
> +  UINT64        ChunkLength;
>  
>    //
>    // Ignore invocations that only modify permission bits
> @@ -687,14 +688,44 @@ SetMemoryAttributes (
>      return EFI_SUCCESS;
>    }
>  
> -  if(((BaseAddress & 0xFFFFF) == 0) && ((Length & 0xFFFFF) == 0)) {
> -    // Is the base and length a multiple of 1 MB?
> -    DEBUG ((EFI_D_PAGE, "SetMemoryAttributes(): MMU section 0x%x length 0x%x to %lx\n", (UINTN)BaseAddress, (UINTN)Length, Attributes));
> -    Status = UpdateSectionEntries (BaseAddress, Length, Attributes, VirtualMask);
> -  } else {
> -    // Base and/or length is not a multiple of 1 MB
> -    DEBUG ((EFI_D_PAGE, "SetMemoryAttributes(): MMU page 0x%x length 0x%x to %lx\n", (UINTN)BaseAddress, (UINTN)Length, Attributes));
> -    Status = UpdatePageEntries (BaseAddress, Length, Attributes, VirtualMask);
> +  while (Length > 0) {

Would this not end up returning an uninitialized Status if called with
Length == 0?

/
    Leif

> +    if ((BaseAddress % TT_DESCRIPTOR_SECTION_SIZE == 0) &&
> +        Length >= TT_DESCRIPTOR_SECTION_SIZE) {
> +
> +      ChunkLength = Length - Length % TT_DESCRIPTOR_SECTION_SIZE;
> +
> +      DEBUG ((DEBUG_PAGE,
> +        "SetMemoryAttributes(): MMU section 0x%lx length 0x%lx to %lx\n",
> +        BaseAddress, ChunkLength, Attributes));
> +
> +      Status = UpdateSectionEntries (BaseAddress, ChunkLength, Attributes,
> +                 VirtualMask);
> +    } else {
> +
> +      //
> +      // Process page by page until the next section boundary, but only if
> +      // we have more than a section's worth of area to deal with after that.
> +      //
> +      ChunkLength = TT_DESCRIPTOR_SECTION_SIZE -
> +                    (BaseAddress % TT_DESCRIPTOR_SECTION_SIZE);
> +      if (ChunkLength + TT_DESCRIPTOR_SECTION_SIZE > Length) {
> +        ChunkLength = Length;
> +      }
> +
> +      DEBUG ((DEBUG_PAGE,
> +        "SetMemoryAttributes(): MMU page 0x%lx length 0x%lx to %lx\n",
> +        BaseAddress, ChunkLength, Attributes));
> +
> +      Status = UpdatePageEntries (BaseAddress, ChunkLength, Attributes,
> +                 VirtualMask);
> +    }
> +
> +    if (EFI_ERROR (Status)) {
> +      break;
> +    }
> +
> +    BaseAddress += ChunkLength;
> +    Length -= ChunkLength;
>    }
>  
>    // Flush d-cache so descriptors make it back to uncached memory for subsequent table walks
> -- 
> 2.7.4
> 
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH v2 1/4] ArmPkg/CpuDxe ARM: avoid splitting page table sections unnecessarily
Posted by Ard Biesheuvel 8 years, 11 months ago 
On 6 March 2017 at 15:12, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> On Thu, Mar 02, 2017 at 10:36:13AM +0000, Ard Biesheuvel wrote:
>> Currently, any range passed to CpuArchProtocol::SetMemoryAttributes is
>> fully broken down into page mappings if the start or the size of the
>> region happens to be misaliged relative to the section size of 1 MB.
>>
>> This is going to hurt when we enable strict memory permissions, given
>
> "Hurt" -> "cause unnecessary performance penalties" or "hurt" ->
> "break everything"?
>

The former. It will map all of RAM using page mappings, which uses
more space unnecessarily

>> that we remap the entire RAM space non-executable (modulo the code
>> bits) when the CpuArchProtocol is installed.
>>
>> So refactor the code to iterate over the range in a way that ensures
>> that all naturally aligned section sized subregions are not broken up.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>
> Many thanks for getting rid of the magic values, and in general making
> the code more logical. One question below:
>
>> ---
>>  ArmPkg/Drivers/CpuDxe/Arm/Mmu.c | 47 ++++++++++++++++----
>>  1 file changed, 39 insertions(+), 8 deletions(-)
>>
>> diff --git a/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c b/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c
>> index 89e429925ba9..ce4d529bda67 100644
>> --- a/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c
>> +++ b/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c
>> @@ -679,6 +679,7 @@ SetMemoryAttributes (
>>    )
>>  {
>>    EFI_STATUS    Status;
>> +  UINT64        ChunkLength;
>>
>>    //
>>    // Ignore invocations that only modify permission bits
>> @@ -687,14 +688,44 @@ SetMemoryAttributes (
>>      return EFI_SUCCESS;
>>    }
>>
>> -  if(((BaseAddress & 0xFFFFF) == 0) && ((Length & 0xFFFFF) == 0)) {
>> -    // Is the base and length a multiple of 1 MB?
>> -    DEBUG ((EFI_D_PAGE, "SetMemoryAttributes(): MMU section 0x%x length 0x%x to %lx\n", (UINTN)BaseAddress, (UINTN)Length, Attributes));
>> -    Status = UpdateSectionEntries (BaseAddress, Length, Attributes, VirtualMask);
>> -  } else {
>> -    // Base and/or length is not a multiple of 1 MB
>> -    DEBUG ((EFI_D_PAGE, "SetMemoryAttributes(): MMU page 0x%x length 0x%x to %lx\n", (UINTN)BaseAddress, (UINTN)Length, Attributes));
>> -    Status = UpdatePageEntries (BaseAddress, Length, Attributes, VirtualMask);
>> +  while (Length > 0) {
>
> Would this not end up returning an uninitialized Status if called with
> Length == 0?
>

Yes, well spotted. I will just add an early 'return EFI_SUCCESS' for this case.


>> +    if ((BaseAddress % TT_DESCRIPTOR_SECTION_SIZE == 0) &&
>> +        Length >= TT_DESCRIPTOR_SECTION_SIZE) {
>> +
>> +      ChunkLength = Length - Length % TT_DESCRIPTOR_SECTION_SIZE;
>> +
>> +      DEBUG ((DEBUG_PAGE,
>> +        "SetMemoryAttributes(): MMU section 0x%lx length 0x%lx to %lx\n",
>> +        BaseAddress, ChunkLength, Attributes));
>> +
>> +      Status = UpdateSectionEntries (BaseAddress, ChunkLength, Attributes,
>> +                 VirtualMask);
>> +    } else {
>> +
>> +      //
>> +      // Process page by page until the next section boundary, but only if
>> +      // we have more than a section's worth of area to deal with after that.
>> +      //
>> +      ChunkLength = TT_DESCRIPTOR_SECTION_SIZE -
>> +                    (BaseAddress % TT_DESCRIPTOR_SECTION_SIZE);
>> +      if (ChunkLength + TT_DESCRIPTOR_SECTION_SIZE > Length) {
>> +        ChunkLength = Length;
>> +      }
>> +
>> +      DEBUG ((DEBUG_PAGE,
>> +        "SetMemoryAttributes(): MMU page 0x%lx length 0x%lx to %lx\n",
>> +        BaseAddress, ChunkLength, Attributes));
>> +
>> +      Status = UpdatePageEntries (BaseAddress, ChunkLength, Attributes,
>> +                 VirtualMask);
>> +    }
>> +
>> +    if (EFI_ERROR (Status)) {
>> +      break;
>> +    }
>> +
>> +    BaseAddress += ChunkLength;
>> +    Length -= ChunkLength;
>>    }
>>
>>    // Flush d-cache so descriptors make it back to uncached memory for subsequent table walks
>> --
>> 2.7.4
>>
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH v2 1/4] ArmPkg/CpuDxe ARM: avoid splitting page table sections unnecessarily
Posted by Leif Lindholm 8 years, 11 months ago 
On Mon, Mar 06, 2017 at 03:55:42PM +0100, Ard Biesheuvel wrote:
> On 6 March 2017 at 15:12, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> > On Thu, Mar 02, 2017 at 10:36:13AM +0000, Ard Biesheuvel wrote:
> >> Currently, any range passed to CpuArchProtocol::SetMemoryAttributes is
> >> fully broken down into page mappings if the start or the size of the
> >> region happens to be misaliged relative to the section size of 1 MB.
> >>
> >> This is going to hurt when we enable strict memory permissions, given
> >
> > "Hurt" -> "cause unnecessary performance penalties" or "hurt" ->
> > "break everything"?
> >
> 
> The former. It will map all of RAM using page mappings, which uses
> more space unnecessarily

OK, can you expand the statement to say "hurt performance" then?

> >> that we remap the entire RAM space non-executable (modulo the code
> >> bits) when the CpuArchProtocol is installed.
> >>
> >> So refactor the code to iterate over the range in a way that ensures
> >> that all naturally aligned section sized subregions are not broken up.
> >>
> >> Contributed-under: TianoCore Contribution Agreement 1.0
> >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> >
> > Many thanks for getting rid of the magic values, and in general making
> > the code more logical. One question below:
> >
> >> ---
> >>  ArmPkg/Drivers/CpuDxe/Arm/Mmu.c | 47 ++++++++++++++++----
> >>  1 file changed, 39 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c b/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c
> >> index 89e429925ba9..ce4d529bda67 100644
> >> --- a/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c
> >> +++ b/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c
> >> @@ -679,6 +679,7 @@ SetMemoryAttributes (
> >>    )
> >>  {
> >>    EFI_STATUS    Status;
> >> +  UINT64        ChunkLength;
> >>
> >>    //
> >>    // Ignore invocations that only modify permission bits
> >> @@ -687,14 +688,44 @@ SetMemoryAttributes (
> >>      return EFI_SUCCESS;
> >>    }
> >>
> >> -  if(((BaseAddress & 0xFFFFF) == 0) && ((Length & 0xFFFFF) == 0)) {
> >> -    // Is the base and length a multiple of 1 MB?
> >> -    DEBUG ((EFI_D_PAGE, "SetMemoryAttributes(): MMU section 0x%x length 0x%x to %lx\n", (UINTN)BaseAddress, (UINTN)Length, Attributes));
> >> -    Status = UpdateSectionEntries (BaseAddress, Length, Attributes, VirtualMask);
> >> -  } else {
> >> -    // Base and/or length is not a multiple of 1 MB
> >> -    DEBUG ((EFI_D_PAGE, "SetMemoryAttributes(): MMU page 0x%x length 0x%x to %lx\n", (UINTN)BaseAddress, (UINTN)Length, Attributes));
> >> -    Status = UpdatePageEntries (BaseAddress, Length, Attributes, VirtualMask);
> >> +  while (Length > 0) {
> >
> > Would this not end up returning an uninitialized Status if called with
> > Length == 0?
> >
> 
> Yes, well spotted. I will just add an early 'return EFI_SUCCESS' for this case.

Works for me. If you also tweak the commit message:
Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>

> >> +    if ((BaseAddress % TT_DESCRIPTOR_SECTION_SIZE == 0) &&
> >> +        Length >= TT_DESCRIPTOR_SECTION_SIZE) {
> >> +
> >> +      ChunkLength = Length - Length % TT_DESCRIPTOR_SECTION_SIZE;
> >> +
> >> +      DEBUG ((DEBUG_PAGE,
> >> +        "SetMemoryAttributes(): MMU section 0x%lx length 0x%lx to %lx\n",
> >> +        BaseAddress, ChunkLength, Attributes));
> >> +
> >> +      Status = UpdateSectionEntries (BaseAddress, ChunkLength, Attributes,
> >> +                 VirtualMask);
> >> +    } else {
> >> +
> >> +      //
> >> +      // Process page by page until the next section boundary, but only if
> >> +      // we have more than a section's worth of area to deal with after that.
> >> +      //
> >> +      ChunkLength = TT_DESCRIPTOR_SECTION_SIZE -
> >> +                    (BaseAddress % TT_DESCRIPTOR_SECTION_SIZE);
> >> +      if (ChunkLength + TT_DESCRIPTOR_SECTION_SIZE > Length) {
> >> +        ChunkLength = Length;
> >> +      }
> >> +
> >> +      DEBUG ((DEBUG_PAGE,
> >> +        "SetMemoryAttributes(): MMU page 0x%lx length 0x%lx to %lx\n",
> >> +        BaseAddress, ChunkLength, Attributes));
> >> +
> >> +      Status = UpdatePageEntries (BaseAddress, ChunkLength, Attributes,
> >> +                 VirtualMask);
> >> +    }
> >> +
> >> +    if (EFI_ERROR (Status)) {
> >> +      break;
> >> +    }
> >> +
> >> +    BaseAddress += ChunkLength;
> >> +    Length -= ChunkLength;
> >>    }
> >>
> >>    // Flush d-cache so descriptors make it back to uncached memory for subsequent table walks
> >> --
> >> 2.7.4
> >>
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Patchew 2.1-devel-8c64711

© 2016 - 2026 Red Hat, Inc.