target/arm/cpu.h | 48 ++++++++++++------------ target/arm/internals.h | 41 +++++++++----------- target/arm/tcg/translate.h | 2 - target/arm/helper.c | 68 +++++++++++++++++++++------------- target/arm/ptw.c | 10 ++--- target/arm/tcg/hflags.c | 4 -- target/arm/tcg/op_helper.c | 14 ++++++- target/arm/tcg/translate-a64.c | 2 +- target/arm/tcg/translate.c | 12 +++--- 9 files changed, 110 insertions(+), 91 deletions(-)
In commit 4c2c0474693229 I tried to fix a problem with our
usage of MMU indexes when EL3 is AArch32. The problem we're
trying to fix is:
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.
The "spurious alignment faults" part is
https://gitlab.com/qemu-project/qemu/-/issues/2326
Commit 4c2c047469322 tried to fix this using what I described in the
commit message as a "more complicated approach", but didn't get it
right in several ways. Full detail in the commit message of patch 1,
but the major visible problem was that regime_el() would return 1
even when the CPU was in Monitor mode; this meant that page table
walks in Monitor mode would look at the wrong SCTLR, TCR, etc and
would generally fault when they should not.
Rather than trying to fix up the multiple problems with the complicated
approach, this series first reverts that commit and then fixes the
initial problem with the idea that commit 4c2c047469322 describes
as "the most straightforward" approach: we add new MMU indexes
EL30_0 and EL30_3_PAN, and use the EL3 index as EL30_3. These then
correspond to "Secure PL1&0 at PL0", "Secure PL1&0 at PL1", and
"Secure PL1&0 at PL1 with PAN", and parallel the NonSecure use
of EL10_0, EL10_1_PAN and EL10_1.
thanks
-- PMM
Peter Maydell (2):
Revert "target/arm: Fix usage of MMU indexes when EL3 is AArch32"
target/arm: Add new MMU indexes for AArch32 Secure PL1&0
target/arm/cpu.h | 48 ++++++++++++------------
target/arm/internals.h | 41 +++++++++-----------
target/arm/tcg/translate.h | 2 -
target/arm/helper.c | 68 +++++++++++++++++++++-------------
target/arm/ptw.c | 10 ++---
target/arm/tcg/hflags.c | 4 --
target/arm/tcg/op_helper.c | 14 ++++++-
target/arm/tcg/translate-a64.c | 2 +-
target/arm/tcg/translate.c | 12 +++---
9 files changed, 110 insertions(+), 91 deletions(-)
--
2.34.1
On Fri, 1 Nov 2024 at 14:28, Peter Maydell <peter.maydell@linaro.org> wrote: > > In commit 4c2c0474693229 I tried to fix a problem with our > usage of MMU indexes when EL3 is AArch32. The problem we're > trying to fix is: > > 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. > > The "spurious alignment faults" part is > https://gitlab.com/qemu-project/qemu/-/issues/2326 > > Commit 4c2c047469322 tried to fix this using what I described in the > commit message as a "more complicated approach", but didn't get it > right in several ways. Full detail in the commit message of patch 1, > but the major visible problem was that regime_el() would return 1 > even when the CPU was in Monitor mode; this meant that page table > walks in Monitor mode would look at the wrong SCTLR, TCR, etc and > would generally fault when they should not. > > Rather than trying to fix up the multiple problems with the complicated > approach, this series first reverts that commit and then fixes the > initial problem with the idea that commit 4c2c047469322 describes > as "the most straightforward" approach: we add new MMU indexes > EL30_0 and EL30_3_PAN, and use the EL3 index as EL30_3. These then > correspond to "Secure PL1&0 at PL0", "Secure PL1&0 at PL1", and > "Secure PL1&0 at PL1 with PAN", and parallel the NonSecure use > of EL10_0, EL10_1_PAN and EL10_1. The submitter tells me that this also fixes https://gitlab.com/qemu-project/qemu/-/issues/2588 (a different variety of "misbehaviour in Secure Monitor mode"). -- PMM
On 01/11/2024 15.28, Peter Maydell wrote: > In commit 4c2c0474693229 I tried to fix a problem with our > usage of MMU indexes when EL3 is AArch32. The problem we're > trying to fix is: > > 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. > > The "spurious alignment faults" part is > https://gitlab.com/qemu-project/qemu/-/issues/2326 > > Commit 4c2c047469322 tried to fix this using what I described in the > commit message as a "more complicated approach", but didn't get it > right in several ways. Full detail in the commit message of patch 1, > but the major visible problem was that regime_el() would return 1 > even when the CPU was in Monitor mode; this meant that page table > walks in Monitor mode would look at the wrong SCTLR, TCR, etc and > would generally fault when they should not. > > Rather than trying to fix up the multiple problems with the complicated > approach, this series first reverts that commit and then fixes the > initial problem with the idea that commit 4c2c047469322 describes > as "the most straightforward" approach: we add new MMU indexes > EL30_0 and EL30_3_PAN, and use the EL3 index as EL30_3. These then > correspond to "Secure PL1&0 at PL0", "Secure PL1&0 at PL1", and > "Secure PL1&0 at PL1 with PAN", and parallel the NonSecure use > of EL10_0, EL10_1_PAN and EL10_1. I can confirm that this fixes the problems with the bpim2u and orangepi Ubuntu/Armbian tests, so if that counts: Tested-by: Thomas Huth <thuth@redhat.com>
© 2016 - 2024 Red Hat, Inc.