Re: [PATCH 2/2] target/arm: Add new MMU indexes for AArch32 Secure PL1&0

[Richard Henderson <richard.henderson@linaro.org>]

On 11/1/24 14:28, Peter Maydell wrote:
> 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.
> 
> Fix this by adding two new MMU indexes:
>   * ARMMMUIdx_E30_0 is for Secure PL0
>   * ARMMMUIdx_E30_3_PAN is for Secure PL1 when PAN is enabled
> The existing ARMMMUIdx_E3 is used to mean "Secure PL1 without PAN"
> (and would be named ARMMMUIdx_E30_3 in an AArch32-centric scheme).
> 
> These extra two indexes bring us up to the maximum of 16 that the
> core code can currently support.
> 
> This commit:
>   * adds the new MMU index handling to the various places
>     where we deal in MMU index values
>   * adds assertions that we aren't AArch32 EL3 in a couple of
>     places that currently use the E10 indexes, to document why
>     they don't also need to handle the E30 indexes
>   * documents in a comment why regime_has_2_ranges() doesn't need
>     updating
> 
> Notes for backporting: this commit depends on the preceding revert of
> 4c2c04746932; that revert and this commit should probably be
> backported to everywhere that we originally backported 4c2c04746932.
> 
> Cc:qemu-stable@nongnu.org
> Resolves:https://gitlab.com/qemu-project/qemu/-/issues/2326
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
> The regime_has_2_ranges() part is correct but does leave a minor
> beartrap for the future if we ever do for some reason need to
> identify "has 2 ranges" in an AArch32 codepath...
> ---

Series:
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~