[PATCH 1/2] ArmPkg/ArmMmuLib ARM: add missing support for non-shareable cached mappings

[PATCH 1/2] ArmPkg/ArmMmuLib ARM: add missing support for non-shareable cached mappings

[Ard Biesheuvel]

We introduced support for non-shareable cached mappings to the AArch64
version of ArmMmuLib a while ago, but the ARM version was left behind,
so fix it.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c b/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c
index 889b22867dc7..b237321a8d8b 100644
--- a/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c
+++ b/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c
@@ -135,6 +135,11 @@ PopulateLevel2PageTable (
     case ARM_MEMORY_REGION_ATTRIBUTE_NONSECURE_WRITE_BACK:
       PageAttributes = TT_DESCRIPTOR_PAGE_WRITE_BACK;
       break;
+    case ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK_NONSHAREABLE:
+    case ARM_MEMORY_REGION_ATTRIBUTE_NONSECURE_WRITE_BACK_NONSHAREABLE:
+      PageAttributes = TT_DESCRIPTOR_PAGE_WRITE_BACK;
+      PageAttributes &= ~TT_DESCRIPTOR_PAGE_S_SHARED;
+      break;
     case ARM_MEMORY_REGION_ATTRIBUTE_WRITE_THROUGH:
     case ARM_MEMORY_REGION_ATTRIBUTE_NONSECURE_WRITE_THROUGH:
       PageAttributes = TT_DESCRIPTOR_PAGE_WRITE_THROUGH;
@@ -239,6 +244,10 @@ FillTranslationTable (
     case ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK:
       Attributes = TT_DESCRIPTOR_SECTION_WRITE_BACK(0);
       break;
+    case ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK_NONSHAREABLE:
+      Attributes = TT_DESCRIPTOR_SECTION_WRITE_BACK(0);
+      Attributes &= ~TT_DESCRIPTOR_SECTION_S_SHARED;
+      break;
     case ARM_MEMORY_REGION_ATTRIBUTE_WRITE_THROUGH:
       Attributes = TT_DESCRIPTOR_SECTION_WRITE_THROUGH(0);
       break;
@@ -251,6 +260,10 @@ FillTranslationTable (
     case ARM_MEMORY_REGION_ATTRIBUTE_NONSECURE_WRITE_BACK:
       Attributes = TT_DESCRIPTOR_SECTION_WRITE_BACK(1);
       break;
+    case ARM_MEMORY_REGION_ATTRIBUTE_NONSECURE_WRITE_BACK_NONSHAREABLE:
+      Attributes = TT_DESCRIPTOR_SECTION_WRITE_BACK(1);
+      Attributes &= ~TT_DESCRIPTOR_SECTION_S_SHARED;
+      break;
     case ARM_MEMORY_REGION_ATTRIBUTE_NONSECURE_WRITE_THROUGH:
       Attributes = TT_DESCRIPTOR_SECTION_WRITE_THROUGH(1);
       break;
-- 
2.17.1

[PATCH 2/2] ArmPkg/ArmMmuLib ARM: fix thinko in second level page table handling

[Ard Biesheuvel]

PopulateLevel2PageTable () is invoked for [parts of] mappings that
start or end on a non-1 MB aligned address (or both). The size of
the mapping depends on both the start address modulo 1 MB and the
length of the mapping, but the logic that calculates this size is
flawed: subtracting 'start address modulo 1 MB' could result in a
negative value for the remaining length, which is obviously wrong.

So instead, take either RemainLength, or the rest of the 1 MB
block, whichever is smaller.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c b/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c
index b237321a8d8b..3b3b20aa9b78 100644
--- a/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c
+++ b/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c
@@ -294,8 +294,8 @@ FillTranslationTable (
       PhysicalBase += TT_DESCRIPTOR_SECTION_SIZE;
       RemainLength -= TT_DESCRIPTOR_SECTION_SIZE;
     } else {
-      PageMapLength = MIN (RemainLength, TT_DESCRIPTOR_SECTION_SIZE) -
-                      (PhysicalBase % TT_DESCRIPTOR_SECTION_SIZE);
+      PageMapLength = MIN (RemainLength, TT_DESCRIPTOR_SECTION_SIZE -
+                                         (PhysicalBase % TT_DESCRIPTOR_SECTION_SIZE));
 
       // Case: Physical address aligned on the Section Size (1MB) && the length
       //       does not fill a section
-- 
2.17.1

Re: [PATCH 1/2] ArmPkg/ArmMmuLib ARM: add missing support for non-shareable cached mappings

[Leif Lindholm]

On Fri, Jan 04, 2019 at 07:04:31PM +0100, Ard Biesheuvel wrote:
> We introduced support for non-shareable cached mappings to the AArch64
> version of ArmMmuLib a while ago, but the ARM version was left behind,
> so fix it.

Consider adding a reference to the corresponding aarch64 commit
(829633e3a82)?

Either way:
Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>


> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c b/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c
> index 889b22867dc7..b237321a8d8b 100644
> --- a/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c
> +++ b/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c
> @@ -135,6 +135,11 @@ PopulateLevel2PageTable (
>      case ARM_MEMORY_REGION_ATTRIBUTE_NONSECURE_WRITE_BACK:
>        PageAttributes = TT_DESCRIPTOR_PAGE_WRITE_BACK;
>        break;
> +    case ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK_NONSHAREABLE:
> +    case ARM_MEMORY_REGION_ATTRIBUTE_NONSECURE_WRITE_BACK_NONSHAREABLE:
> +      PageAttributes = TT_DESCRIPTOR_PAGE_WRITE_BACK;
> +      PageAttributes &= ~TT_DESCRIPTOR_PAGE_S_SHARED;
> +      break;
>      case ARM_MEMORY_REGION_ATTRIBUTE_WRITE_THROUGH:
>      case ARM_MEMORY_REGION_ATTRIBUTE_NONSECURE_WRITE_THROUGH:
>        PageAttributes = TT_DESCRIPTOR_PAGE_WRITE_THROUGH;
> @@ -239,6 +244,10 @@ FillTranslationTable (
>      case ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK:
>        Attributes = TT_DESCRIPTOR_SECTION_WRITE_BACK(0);
>        break;
> +    case ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK_NONSHAREABLE:
> +      Attributes = TT_DESCRIPTOR_SECTION_WRITE_BACK(0);
> +      Attributes &= ~TT_DESCRIPTOR_SECTION_S_SHARED;
> +      break;
>      case ARM_MEMORY_REGION_ATTRIBUTE_WRITE_THROUGH:
>        Attributes = TT_DESCRIPTOR_SECTION_WRITE_THROUGH(0);
>        break;
> @@ -251,6 +260,10 @@ FillTranslationTable (
>      case ARM_MEMORY_REGION_ATTRIBUTE_NONSECURE_WRITE_BACK:
>        Attributes = TT_DESCRIPTOR_SECTION_WRITE_BACK(1);
>        break;
> +    case ARM_MEMORY_REGION_ATTRIBUTE_NONSECURE_WRITE_BACK_NONSHAREABLE:
> +      Attributes = TT_DESCRIPTOR_SECTION_WRITE_BACK(1);
> +      Attributes &= ~TT_DESCRIPTOR_SECTION_S_SHARED;
> +      break;
>      case ARM_MEMORY_REGION_ATTRIBUTE_NONSECURE_WRITE_THROUGH:
>        Attributes = TT_DESCRIPTOR_SECTION_WRITE_THROUGH(1);
>        break;
> -- 
> 2.17.1
> 

Re: [PATCH 2/2] ArmPkg/ArmMmuLib ARM: fix thinko in second level page table handling

[Leif Lindholm]

On Fri, Jan 04, 2019 at 07:04:32PM +0100, Ard Biesheuvel wrote:
> PopulateLevel2PageTable () is invoked for [parts of] mappings that
> start or end on a non-1 MB aligned address (or both). The size of
> the mapping depends on both the start address modulo 1 MB and the
> length of the mapping, but the logic that calculates this size is
> flawed: subtracting 'start address modulo 1 MB' could result in a
> negative value for the remaining length, which is obviously wrong.
> 
> So instead, take either RemainLength, or the rest of the 1 MB
> block, whichever is smaller.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>

> ---
>  ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c b/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c
> index b237321a8d8b..3b3b20aa9b78 100644
> --- a/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c
> +++ b/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c
> @@ -294,8 +294,8 @@ FillTranslationTable (
>        PhysicalBase += TT_DESCRIPTOR_SECTION_SIZE;
>        RemainLength -= TT_DESCRIPTOR_SECTION_SIZE;
>      } else {
> -      PageMapLength = MIN (RemainLength, TT_DESCRIPTOR_SECTION_SIZE) -
> -                      (PhysicalBase % TT_DESCRIPTOR_SECTION_SIZE);
> +      PageMapLength = MIN (RemainLength, TT_DESCRIPTOR_SECTION_SIZE -
> +                                         (PhysicalBase % TT_DESCRIPTOR_SECTION_SIZE));
>  
>        // Case: Physical address aligned on the Section Size (1MB) && the length
>        //       does not fill a section
> -- 
> 2.17.1
> 

Re: [PATCH 2/2] ArmPkg/ArmMmuLib ARM: fix thinko in second level page table handling

[Ard Biesheuvel]

On Fri, 11 Jan 2019 at 19:07, Leif Lindholm <leif.lindholm@linaro.org> wrote:
>
> On Fri, Jan 04, 2019 at 07:04:32PM +0100, Ard Biesheuvel wrote:
> > PopulateLevel2PageTable () is invoked for [parts of] mappings that
> > start or end on a non-1 MB aligned address (or both). The size of
> > the mapping depends on both the start address modulo 1 MB and the
> > length of the mapping, but the logic that calculates this size is
> > flawed: subtracting 'start address modulo 1 MB' could result in a
> > negative value for the remaining length, which is obviously wrong.
> >
> > So instead, take either RemainLength, or the rest of the 1 MB
> > block, whichever is smaller.
> >
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>
> Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
>

Series pushed as

e3ad54faa855 ArmPkg/ArmMmuLib ARM: add missing support for
non-shareable cached mappings
28ce4cb3590b ArmPkg/ArmMmuLib ARM: fix thinko in second level page
table handling

(with Eugene's tested-by added to the latter)

Thanks

> > ---
> >  ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c b/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c
> > index b237321a8d8b..3b3b20aa9b78 100644
> > --- a/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c
> > +++ b/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c
> > @@ -294,8 +294,8 @@ FillTranslationTable (
> >        PhysicalBase += TT_DESCRIPTOR_SECTION_SIZE;
> >        RemainLength -= TT_DESCRIPTOR_SECTION_SIZE;
> >      } else {
> > -      PageMapLength = MIN (RemainLength, TT_DESCRIPTOR_SECTION_SIZE) -
> > -                      (PhysicalBase % TT_DESCRIPTOR_SECTION_SIZE);
> > +      PageMapLength = MIN (RemainLength, TT_DESCRIPTOR_SECTION_SIZE -
> > +                                         (PhysicalBase % TT_DESCRIPTOR_SECTION_SIZE));
> >
> >        // Case: Physical address aligned on the Section Size (1MB) && the length
> >        //       does not fill a section
> > --
> > 2.17.1
> >