Re: [Qemu-devel] [PATCH 3/4] target/arm: Install ASIDs for short-form from EL1

[Peter Maydell]

On 29 October 2018 at 15:53, Richard Henderson
<address@hidden> wrote:
> This is less complex than the LPAE case, but still we now avoid the
> flush in case it is only the PROCID field that is changing.
>
> Signed-off-by: Richard Henderson <address@hidden>
> ---
>  target/arm/helper.c | 34 ++++++++++++++++++++++++----------
>  1 file changed, 24 insertions(+), 10 deletions(-)
>
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index 26d6f28793..f767467dcf 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -541,17 +541,31 @@ static void fcse_write(CPUARMState *env, const 
> ARMCPRegInfo *ri, uint64_t value)
>  static void contextidr_write(CPUARMState *env, const ARMCPRegInfo *ri,
>                               uint64_t value)
>  {
> -    ARMCPU *cpu = arm_env_get_cpu(env);
> -
> -    if (raw_read(env, ri) != value && !arm_feature(env, ARM_FEATURE_PMSA)
> -        && !extended_addresses_enabled(env)) {
> -        /* For VMSA (when not using the LPAE long descriptor page table
> -         * format) this register includes the ASID, so do a TLB flush.
> -         * For PMSA it is purely a process ID and no action is needed.
> -         */
> -        tlb_flush(CPU(cpu));
> -    }
>      raw_write(env, ri, value);
> +
> +    /*
> +     * For VMSA (when not using the LPAE long descriptor page table format)
> +     * this register includes the ASID.  For PMSA it is purely a process ID
> +     * and no action is needed.
> +     */
> +    if (!arm_feature(env, ARM_FEATURE_PMSA) &&
> +        !extended_addresses_enabled(env)) {
> +        CPUState *cs = CPU(arm_env_get_cpu(env));
> +        int asid = extract32(value, 0, 8);
> +        int idxmask;
> +
> +        switch (ri->secure) {
> +        case ARM_CP_SECSTATE_S:
> +            idxmask = ARMMMUIdxBit_S1SE1 | ARMMMUIdxBit_S1SE0;
> +            break;
> +        case ARM_CP_SECSTATE_NS:
> +            idxmask = ARMMMUIdxBit_S12NSE1 | ARMMMUIdxBit_S12NSE0;
> +            break;
> +        default:
> +            g_assert_not_reached();
> +        }

If EL3 is AArch32 then changes to CONTEXTIDR(S) need to
invalidate the S1E3 MMU index.

If EL3 is not AArch32 then there is no CONTEXTIDR(S), but
if we are at EL3 then whether CONTEXTIDR applies to
ARMMMUIdxBit_S1SE1 | ARMMMUIdxBit_S1SE0 or to
ARMMMUIdxBit_S12NSE1 | ARMMMUIdxBit_S12NSE0 is tricky,
because it's up to EL3 to swap registers around depending
on whether it wants to enter EL1 in secure or nonsecure state.
I need to check more deeply about how this works.

NB also that I'm a bit suspicious of the definition of
extended_addresses_enabled(); need to check it.

> +        tlb_set_asid_for_mmuidx(cs, asid, idxmask, 0);
> +    }
>  }
>
>  /* IS variants of TLB operations must affect all cores */
> --
> 2.17.2

thanks
-- PMM