| Message ID | 20241101142845.1712482-1-peter.maydell@linaro.org |
|---|---|
| Headers | show |
| Series | Fix EL3 AArch32 MMU index usage (again) | expand |
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>
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
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(-)