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

[Bernhard Beschow]

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(-)
>