Re: [PULL 4/4] target/arm: Fix usage of MMU indexes when EL3 is AArch32

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

On 10/28/24 13:24, Peter Maydell wrote:
> On Fri, 25 Oct 2024 at 13:54, Thomas Huth <thuth@redhat.com> wrote:
>>
>> On 13/08/2024 17.20, 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.
>>>
>>> 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.
>>>
>>> The changes in this commit were created by auditing all the places
>>> where we use specific ARMMMUIdx_ values, and checking whether they
>>> needed to be changed to handle the new index value usage.
>>
>>    Hi Peter,
>>
>> this commit caused a regression with one of the Avocado tests:
>>
>>    AVOCADO_ALLOW_LARGE_STORAGE=1 avocado run
>> tests/avocado/boot_linux_console.py:BootLinuxConsole.test_arm_bpim2u_openwrt_22_03_3
>>
>> is failing now. It works still fine before this commit. Could you please
>> have a look?
> 
> Thanks for the report; I've investigated it. The cause of this specific
> failure is that regime_el() doesn't return the right answer when code
> is executing in the guest in Monitor mode. The effect is that because
> regime_el() returns 1, not 3, we look at the wrong banked registers
> and the page table walk fails when it should not. This is enough to fix:
> 
> diff --git a/target/arm/internals.h b/target/arm/internals.h
> index 203a2dae148..812487b9291 100644
> --- a/target/arm/internals.h
> +++ b/target/arm/internals.h
> @@ -926,7 +926,7 @@ static inline uint32_t regime_el(CPUARMState *env,
> ARMMMUIdx mmu_idx)
>       case ARMMMUIdx_E10_1_PAN:
>       case ARMMMUIdx_Stage1_E1:
>       case ARMMMUIdx_Stage1_E1_PAN:
> -        return arm_el_is_aa64(env, 3) || !arm_is_secure_below_el3(env) ? 1 : 3;
> +        return arm_aa32_secure_pl1_0(env) ? 3 : 1;
>       case ARMMMUIdx_MPrivNegPri:
>       case ARMMMUIdx_MUserNegPri:
>       case ARMMMUIdx_MPriv:
> 
> However, while I was thinking about this I realised that there
> are some problems with the design change this commit is trying
> to do. The idea is that we now use the same MMU indexes for
> Secure PL1&0 as we do for NonSecure PL1&0.
> 
> Small problem:
>   That means we need to flush the TLBs at any point where the CPU
>   state flips from one to the other. We already flush the TLB when
>   SCR.NS is changed, but we don't flush the TLB when we take an
>   exception from NS PL1&0 into Mon or when we return from Mon to
>   NS PL1&0. Now we need to do that, so any time we call up into
>   Mon and back we'll dump the TLBs, which is a bit sad.
>   (Also we could skip flushing all these TLBs when NS changes.)
> 
> Larger problem:
>   the ATS12NS* address translate instructions allow Mon code
>   (which is Secure) to do a stage 1+2 page table walk for NS.
>   I thought this was OK because do_ats_write() does a page
>   table walk which doesn't use the TLBs, so because it can
>   pass both the MMU index and also an ARMSecuritySpace argument
>   we can tell the table walk that we want NS stage1+2, not S.
>   But that means that all the code within the ptw that needs
>   to find e.g. the regime EL cannot do so only with an
>   mmu_idx -- all these functions like regime_sctlr(), regime_el(),
>   etc would need to pass both an mmu_idx and the security_space,
>   so they can tell whether this is a translation regime
>   controlled by EL1 or EL3 (and so whether to look at SCTLR.S
>   or SCTLR.NS, etc).
> 
> So now I'm wondering if this merge was a good idea after all.
> Should we do all that replumbing required, or should we
> instead revert this and take the "straightforward" approach
> described in the commit message above of adding some extra
> MMU indexes?

It might be easier to add the extra mmu indexes.

We'll have to re-use ARMMMUIdx_E3 for EL30_3, I think, because we only have 2 mmu_idx left 
available.


r~


> 
> (I suspect that this commit is likely also the cause of
> https://gitlab.com/qemu-project/qemu/-/issues/2588 )
> 
> -- PMM