Re: [PATCH] target/arm: Correct AArch64.S2MinTxSZ 32-bit EL1 input size check

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

On 5/9/23 10:20, Peter Maydell wrote:
> In check_s2_mmu_setup() we have a check that is attempting to
> implement the part of AArch64.S2MinTxSZ that is specific to when EL1
> is AArch32:
> 
>      if !s1aarch64 then
>          // EL1 is AArch32
>          min_txsz = Min(min_txsz, 24);
> 
> Unfortunately we got this wrong in two ways:
> 
> (1) The minimum txsz corresponds to a maximum inputsize, but we got
> the sense of the comparison wrong and were faulting for all
> inputsizes less than 40 bits
> 
> (2) We try to implement this as an extra check that happens after
> we've done the same txsz checks we would do for an AArch64 EL1, but
> in fact the pseudocode is*loosening*  the requirements, so that txsz
> values that would fault for an AArch64 EL1 do not fault for AArch32
> EL1, because it does Min(old_min, 24), not Max(old_min, 24).
> 
> You can see this also in the text of the Arm ARM in table D8-8, which
> shows that where the implemented PA size is less than 40 bits an
> AArch32 EL1 is still OK with a configured stage2 T0SZ for a 40 bit
> IPA, whereas if EL1 is AArch64 then the T0SZ must be big enough to
> constrain the IPA to the implemented PA size.
> 
> Because of part (2), we can't do this as a separate check, but
> have to integrate it into aa64_va_parameters(). Add a new argument
> to that function to indicate that EL1 is 32-bit. All the existing
> callsites except the one in get_phys_addr_lpae() can pass 'false',
> because they are either doing a lookup for a stage 1 regime or
> else they don't care about the tsz/tsz_oob fields.
> 
> Cc:qemu-stable@nongnu.org
> Resolves:https://gitlab.com/qemu-project/qemu/-/issues/1627
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
> Since we pass the CPUARMState to aa64_va_parameters(), it would be
> possible to have that function call arm_el_is_aa64(env, 1) itself;
> but since that seems a rather non-obvious thing for the function to
> be doing and a potentially more transient (or at least "not
> configured yet") bit of CPU state than the translation regime
> configuration, I preferred to have the callers pass in the
> information explicitly.  I don't feel super strongly about this
> though, so we could do it the other way if you prefer.
> ---

I prefer the extra argument, as you've done.

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


r~