mbox series

[0/2] target/arm: Fix EL3-is-AArch32 mmu indexes

Message ID 20240809160430.1144805-1-peter.maydell@linaro.org
Headers show
Series target/arm: Fix EL3-is-AArch32 mmu indexes | expand

Message

Peter Maydell Aug. 9, 2024, 4:04 p.m. UTC 
Our current usage of MMU indexes when EL3 is AArch32 is confused.
Architecturally, when EL3 is AArch32, all Secure code runs under the
Secure PL1&0 translation regime:
 * code at EL3, which might be Mon, or SVC, or any of the
   other privileged modes (PL1)
 * code at EL0 (Secure PL0)

This is different from when EL3 is AArch64, in which case EL3 is its
own translation regime, and EL1 and EL0 (whether AArch32 or AArch64)
have their own regime.

We claimed to be mapping Secure PL1 to our ARMMMUIdx_EL3, but didn't
do anything special about Secure PL0, which meant it used the same
ARMMMUIdx_EL10_0 that NonSecure PL0 does.  This resulted in a bug
where arm_sctlr() incorrectly picked the NonSecure SCTLR as the
controlling register when in Secure PL0, which meant we were
spuriously generating alignment faults because we were looking at the
wrong SCTLR control bits.

The use of ARMMMUIdx_EL3 for Secure PL1 also resulted in the bug that
we wouldn't honour the PAN bit for Secure PL1, because there's no
equivalent _PAN mmu index for it.

We could fix this in one of two ways:
 * The most straightforward is to add new MMU indexes EL30_0,
   EL30_3, EL30_3_PAN to correspond to "Secure PL1&0 at PL0",
   "Secure PL1&0 at PL1", and "Secure PL1&0 at PL1 with PAN".
   This matches how we use indexes for the AArch64 regimes, and
   preserves propirties like being able to determine the privilege
   level from an MMU index without any other information. However
   it would add two MMU indexes (we can share one with ARMMMUIdx_EL3),
   and we are already using 14 of the 16 the core TLB code permits.

 * The more complicated approach is the one we take here. We use
   the same MMU indexes (E10_0, E10_1, E10_1_PAN) for Secure PL1&0
   than we do for NonSecure PL1&0. This saves on MMU indexes, but
   means we need to check in some places whether we're in the
   Secure PL1&0 regime or not before we interpret an MMU index.

Patch 1 cleans up an out of date comment about MMU index
usage; patch 2 is the actual bug fix.

This fixes the bug with the repro case in the bug report, and it
also passes "make check", but I don't have a huge range of
Secure AArch32 test images to test with. I guess it ought to go
into 9.1 as a bugfix, but the nature of the patch means it's
not very easy to be confident it doesn't introduce any new bugs...

Bernhard: I suspect this is the same bug you reported a few months
back in this thread:
https://lore.kernel.org/qemu-devel/C875173E-4B5B-4F71-8CF4-4325F7AB7629@gmail.com/
 -- if you're able to test that this patchset fixes your test
case as well, that would be great.

thanks
-- PMM

Peter Maydell (2):
  target/arm: Update translation regime comment for new features
  target/arm: Fix usage of MMU indexes when EL3 is AArch32

 target/arm/cpu.h               | 50 ++++++++++++++++++++++------------
 target/arm/internals.h         | 27 +++++++++++++++---
 target/arm/tcg/translate.h     |  2 ++
 target/arm/helper.c            | 34 +++++++++++++++--------
 target/arm/ptw.c               |  6 +++-
 target/arm/tcg/hflags.c        |  4 +++
 target/arm/tcg/translate-a64.c |  2 +-
 target/arm/tcg/translate.c     |  9 +++---
 8 files changed, 95 insertions(+), 39 deletions(-)

Comments

Bernhard Beschow Aug. 12, 2024, 8:56 a.m. UTC | #1 
Am 9. August 2024 16:04:28 UTC schrieb Peter Maydell <peter.maydell@linaro.org>:
>Our current usage of MMU indexes when EL3 is AArch32 is confused.
>Architecturally, when EL3 is AArch32, all Secure code runs under the
>Secure PL1&0 translation regime:
> * code at EL3, which might be Mon, or SVC, or any of the
>   other privileged modes (PL1)
> * code at EL0 (Secure PL0)
>
>This is different from when EL3 is AArch64, in which case EL3 is its
>own translation regime, and EL1 and EL0 (whether AArch32 or AArch64)
>have their own regime.
>
>We claimed to be mapping Secure PL1 to our ARMMMUIdx_EL3, but didn't
>do anything special about Secure PL0, which meant it used the same
>ARMMMUIdx_EL10_0 that NonSecure PL0 does.  This resulted in a bug
>where arm_sctlr() incorrectly picked the NonSecure SCTLR as the
>controlling register when in Secure PL0, which meant we were
>spuriously generating alignment faults because we were looking at the
>wrong SCTLR control bits.
>
>The use of ARMMMUIdx_EL3 for Secure PL1 also resulted in the bug that
>we wouldn't honour the PAN bit for Secure PL1, because there's no
>equivalent _PAN mmu index for it.
>
>We could fix this in one of two ways:
> * The most straightforward is to add new MMU indexes EL30_0,
>   EL30_3, EL30_3_PAN to correspond to "Secure PL1&0 at PL0",
>   "Secure PL1&0 at PL1", and "Secure PL1&0 at PL1 with PAN".
>   This matches how we use indexes for the AArch64 regimes, and
>   preserves propirties like being able to determine the privilege
>   level from an MMU index without any other information. However
>   it would add two MMU indexes (we can share one with ARMMMUIdx_EL3),
>   and we are already using 14 of the 16 the core TLB code permits.
>
> * The more complicated approach is the one we take here. We use
>   the same MMU indexes (E10_0, E10_1, E10_1_PAN) for Secure PL1&0
>   than we do for NonSecure PL1&0. This saves on MMU indexes, but
>   means we need to check in some places whether we're in the
>   Secure PL1&0 regime or not before we interpret an MMU index.
>
>Patch 1 cleans up an out of date comment about MMU index
>usage; patch 2 is the actual bug fix.
>
>This fixes the bug with the repro case in the bug report, and it
>also passes "make check", but I don't have a huge range of
>Secure AArch32 test images to test with. I guess it ought to go
>into 9.1 as a bugfix, but the nature of the patch means it's
>not very easy to be confident it doesn't introduce any new bugs...
>
>Bernhard: I suspect this is the same bug you reported a few months
>back in this thread:
>https://lore.kernel.org/qemu-devel/C875173E-4B5B-4F71-8CF4-4325F7AB7629@gmail.com/
> -- if you're able to test that this patchset fixes your test
>case as well, that would be great.

Hi Peter,

indeed this fixes my guest, too! Thanks for keeping me updated.

Series:
Tested-by: Bernhard Beschow <shentey@gmail.com>

>
>thanks
>-- PMM
>
>Peter Maydell (2):
>  target/arm: Update translation regime comment for new features
>  target/arm: Fix usage of MMU indexes when EL3 is AArch32
>
> target/arm/cpu.h               | 50 ++++++++++++++++++++++------------
> target/arm/internals.h         | 27 +++++++++++++++---
> target/arm/tcg/translate.h     |  2 ++
> target/arm/helper.c            | 34 +++++++++++++++--------
> target/arm/ptw.c               |  6 +++-
> target/arm/tcg/hflags.c        |  4 +++
> target/arm/tcg/translate-a64.c |  2 +-
> target/arm/tcg/translate.c     |  9 +++---
> 8 files changed, 95 insertions(+), 39 deletions(-)
>