[edk2] ArmPkg/AArch64Mmu: use correct AP[] bits in ArmClearMemoryRegionReadOnly

Message ID 1458222900-7890-1-git-send-email-ard.biesheuvel@linaro.org
State Accepted
Commit b5d89de167a683eeba3e2dbb6557f402b952cdc2
Headers show

Commit Message

Ard Biesheuvel March 17, 2016, 1:55 p.m.
The function ArmClearMemoryRegionReadOnly() was supposed to undo the
effect of ArmSetMemoryRegionReadOnly(), but instead, it sets the permissions
to EL0-no access, EL1-read-only. Since the EL0 bit should be 1 to align
with EL2/3 (where the bit is SBO), use TT_AP_RW_RW instead, which makes the
entry read-write for EL0 when executing at EL1, and read-write for all other
levels.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

---
 ArmPkg/Library/ArmLib/AArch64/AArch64Mmu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

-- 
2.5.0

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

Comments

Leif Lindholm March 17, 2016, 3:31 p.m. | #1
On Thu, Mar 17, 2016 at 02:55:00PM +0100, Ard Biesheuvel wrote:
> The function ArmClearMemoryRegionReadOnly() was supposed to undo the

> effect of ArmSetMemoryRegionReadOnly(), but instead, it sets the permissions

> to EL0-no access, EL1-read-only. Since the EL0 bit should be 1 to align

> with EL2/3 (where the bit is SBO), use TT_AP_RW_RW instead, which makes the

> entry read-write for EL0 when executing at EL1, and read-write for all other

> levels.

> 

> Contributed-under: TianoCore Contribution Agreement 1.0

> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>


Happy with the RO->RW, confused by the NO->RW.
I presume this is about consistency?
Why do we need access for EL0?

> ---

>  ArmPkg/Library/ArmLib/AArch64/AArch64Mmu.c | 2 +-

>  1 file changed, 1 insertion(+), 1 deletion(-)

> 

> diff --git a/ArmPkg/Library/ArmLib/AArch64/AArch64Mmu.c b/ArmPkg/Library/ArmLib/AArch64/AArch64Mmu.c

> index f967a6478840..b7d23c6f3286 100644

> --- a/ArmPkg/Library/ArmLib/AArch64/AArch64Mmu.c

> +++ b/ArmPkg/Library/ArmLib/AArch64/AArch64Mmu.c

> @@ -558,7 +558,7 @@ ArmClearMemoryRegionReadOnly (

>    return SetMemoryRegionAttribute (

>             BaseAddress,

>             Length,

> -           TT_AP_NO_RO,

> +           TT_AP_RW_RW,

>             ~(TT_ADDRESS_MASK_BLOCK_ENTRY | TT_AP_MASK));

>  }

>  

> -- 

> 2.5.0

> 

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Ard Biesheuvel March 17, 2016, 3:40 p.m. | #2
On 17 March 2016 at 16:31, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> On Thu, Mar 17, 2016 at 02:55:00PM +0100, Ard Biesheuvel wrote:

>> The function ArmClearMemoryRegionReadOnly() was supposed to undo the

>> effect of ArmSetMemoryRegionReadOnly(), but instead, it sets the permissions

>> to EL0-no access, EL1-read-only. Since the EL0 bit should be 1 to align

>> with EL2/3 (where the bit is SBO), use TT_AP_RW_RW instead, which makes the

>> entry read-write for EL0 when executing at EL1, and read-write for all other

>> levels.

>>

>> Contributed-under: TianoCore Contribution Agreement 1.0

>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

>

> Happy with the RO->RW, confused by the NO->RW.

> I presume this is about consistency?

> Why do we need access for EL0?

>


We don't. But when running on EL2 or EL3, the same bit needs to be
set, so either we have two separate definitions, for EL1 and EL2/3, or
we just set the bit for EL0 as well, which shouldn't matter anyway
since we never return to EL0
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Leif Lindholm March 17, 2016, 3:41 p.m. | #3
On Thu, Mar 17, 2016 at 04:40:38PM +0100, Ard Biesheuvel wrote:
> On 17 March 2016 at 16:31, Leif Lindholm <leif.lindholm@linaro.org> wrote:

> > On Thu, Mar 17, 2016 at 02:55:00PM +0100, Ard Biesheuvel wrote:

> >> The function ArmClearMemoryRegionReadOnly() was supposed to undo the

> >> effect of ArmSetMemoryRegionReadOnly(), but instead, it sets the permissions

> >> to EL0-no access, EL1-read-only. Since the EL0 bit should be 1 to align

> >> with EL2/3 (where the bit is SBO), use TT_AP_RW_RW instead, which makes the

> >> entry read-write for EL0 when executing at EL1, and read-write for all other

> >> levels.

> >>

> >> Contributed-under: TianoCore Contribution Agreement 1.0

> >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

> >

> > Happy with the RO->RW, confused by the NO->RW.

> > I presume this is about consistency?

> > Why do we need access for EL0?

> >

> 

> We don't. But when running on EL2 or EL3, the same bit needs to be

> set, so either we have two separate definitions, for EL1 and EL2/3, or

> we just set the bit for EL0 as well, which shouldn't matter anyway

> since we never return to EL0


OK, that makes sense:
Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>

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

Patch

diff --git a/ArmPkg/Library/ArmLib/AArch64/AArch64Mmu.c b/ArmPkg/Library/ArmLib/AArch64/AArch64Mmu.c
index f967a6478840..b7d23c6f3286 100644
--- a/ArmPkg/Library/ArmLib/AArch64/AArch64Mmu.c
+++ b/ArmPkg/Library/ArmLib/AArch64/AArch64Mmu.c
@@ -558,7 +558,7 @@  ArmClearMemoryRegionReadOnly (
   return SetMemoryRegionAttribute (
            BaseAddress,
            Length,
-           TT_AP_NO_RO,
+           TT_AP_RW_RW,
            ~(TT_ADDRESS_MASK_BLOCK_ENTRY | TT_AP_MASK));
 }