Re: [PATCH v3 2/4] target/arm: Add support for FEAT_DIT, Data Independent Timing

[Richard Henderson]

On 2/2/21 12:21 PM, Rebecca Cran wrote:
> On 1/27/21 10:06 PM, Richard Henderson wrote:
>> In particular: CPSR.DIT (bit 21) -> SPSR_EL1.DIT (bit 24), and merging
>> PSTATE.SS into SPSR_EL1.SS (bit 21).
> 
> Thanks. I _think_ I'm understanding it better now. Would the following work? I
> don't see where I need to map PSTATE.SS into SPSR_EL1.SS though, because isn't
> that handled automatically since PSTATE maps onto SPSR?
> 
> 
> diff --git a/target/arm/helper-a64.c b/target/arm/helper-a64.c
> index a6b162049806..c1ff24d42f32 100644
> --- a/target/arm/helper-a64.c
> +++ b/target/arm/helper-a64.c
> @@ -1003,6 +1003,11 @@ void HELPER(exception_return)(CPUARMState *env, 
> uint64_t
> new_pc)
>          if (!arm_singlestep_active(env)) {
>              env->pstate &= ~PSTATE_SS;
>          }
> +
> +        if (spsr & PSTATE_DIT) {
> +            env->uncached_cpsr |= CPSR_DIT;
> +        }

This is missing the restore of PSTATE_SS for when singlestep *is* active.

> @@ -9426,6 +9426,12 @@ static void take_aarch32_exception(CPUARMState *env, 
> int
> new_mode,
>       */
>      env->pstate &= ~PSTATE_SS;
>      env->spsr = cpsr_read(env);
> +
> +    if (env->uncached_cpsr & CPSR_DIT) {
> +        env->spsr |= PSTATE_DIT;
> +        env->spsr &= ~PSTATE_SS;
> +    }

This one is incorrect because we're not storing to SPSR_ELx format, but SPSR
(the aa32 version), which has DIT at bit 21.

> @@ -9905,6 +9911,11 @@ static void arm_cpu_do_interrupt_aarch64(CPUState *cs)
>          old_mode = cpsr_read(env);
>          env->elr_el[new_el] = env->regs[15];
> 
> +        if (old_mode & CPSR_DIT) {
> +            old_mode |= PSTATE_DIT;
> +            old_mode &= ~PSTATE_SS;

This line would be clearer using CPSR_DIT.  I don't see PSTATE_SS being added
to old_mode.  Is that somewhere else, or simply missing context here?

I think it would be clearer to add some new helpers.  Naming is always
difficult, but how about:

static uint32_t cpsr_read_for_spsr_elx(CPUARMState *env)
{
    uint32_t ret = cpsr_read(env);

    /* Move DIT to the correct location for SPSR_ELx */
    if (ret & CPSR_DIT) {
        ret &= ~CPSR_DIT;
        ret |= PSTATE_DIT;
    }
    /* Merge PSTATE.SS into SPSR_ELx */
    ret |= env->pstate & PSTATE_SS;

    return ret;
}

static void cpsr_write_from_spsr_elx(CPUARMState *env,
                                     uint32_t val)
{
    uint32_t mask;

    /* Save SPSR_ELx.SS into PSTATE. */
    env->pstate = (env->pstate & ~PSTATE_SS) | (val & PSTATE_SS);
    val &= ~PSTATE_SS;

    /* Move DIT to the correct location for CPSR */
    if (val & PSTATE_DIT) {
        val &= ~PSTATE_DIT;
        val |= CPSR_DIT;
    }

    mask = aarch32_cpsr_valid_mask(env->features, \
        &env_archcpu(env)->isar);
    cpsr_write(env, val, mask, CPSRWriteRaw);
}


r~