Re: [Qemu-devel] [PATCH v2 09/35] target-arm: A64: Implement MSR (immediate) instructions

[Peter Crosthwaite]

On Sat, Feb 1, 2014 at 1:45 AM, Peter Maydell <address@hidden> wrote:
> Implement the MSR (immediate) instructions, which can update the
> PSTATE SP and DAIF fields.
>
> Signed-off-by: Peter Maydell <address@hidden>
> ---
>  target-arm/cpu.h           |  1 +
>  target-arm/helper.h        |  2 ++
>  target-arm/op_helper.c     | 25 +++++++++++++++++++++++++
>  target-arm/translate-a64.c | 24 +++++++++++++++++++++++-
>  4 files changed, 51 insertions(+), 1 deletion(-)
>
> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> index 385cfcd..e66d464 100644
> --- a/target-arm/cpu.h
> +++ b/target-arm/cpu.h
> @@ -426,6 +426,7 @@ int cpu_arm_handle_mmu_fault (CPUARMState *env, 
> target_ulong address, int rw,
>  #define PSTATE_Z (1U << 30)
>  #define PSTATE_N (1U << 31)
>  #define PSTATE_NZCV (PSTATE_N | PSTATE_Z | PSTATE_C | PSTATE_V)
> +#define PSTATE_DAIF (PSTATE_D | PSTATE_A | PSTATE_I | PSTATE_F)
>  #define CACHED_PSTATE_BITS (PSTATE_NZCV)
>  /* Mode values for AArch64 */
>  #define PSTATE_MODE_EL3h 13
> diff --git a/target-arm/helper.h b/target-arm/helper.h
> index 71b8411..93a27ce 100644
> --- a/target-arm/helper.h
> +++ b/target-arm/helper.h
> @@ -62,6 +62,8 @@ DEF_HELPER_2(get_cp_reg, i32, env, ptr)
>  DEF_HELPER_3(set_cp_reg64, void, env, ptr, i64)
>  DEF_HELPER_2(get_cp_reg64, i64, env, ptr)
>
> +DEF_HELPER_3(msr_i_pstate, void, env, i32, i32)
> +
>  DEF_HELPER_2(get_r13_banked, i32, env, i32)
>  DEF_HELPER_3(set_r13_banked, void, env, i32, i32)
>
> diff --git a/target-arm/op_helper.c b/target-arm/op_helper.c
> index a918e5b..c812a9f 100644
> --- a/target-arm/op_helper.c
> +++ b/target-arm/op_helper.c
> @@ -313,6 +313,31 @@ uint64_t HELPER(get_cp_reg64)(CPUARMState *env, void 
> *rip)
>      return value;
>  }
>
> +void HELPER(msr_i_pstate)(CPUARMState *env, uint32_t op, uint32_t imm)
> +{
> +    /* MSR_i to update PSTATE. This is OK from EL0 only if UMA is set.
> +     * Note that SPSel is never OK from EL0; we rely on handle_msr_i()
> +     * to catch that case at translate time.
> +     */
> +    if (arm_current_pl(env) == 0 && !(env->cp15.c1_sys & SCTLR_UMA)) {
> +        raise_exception(env, EXCP_UDEF);
> +    }
> +
> +    switch (op) {
> +    case 0x05: /* SPSel */
> +        env->pstate = deposit32(env->pstate, 0, 1, imm);

"0","1" hardcoded constants are a bit unfriendly. I guess the current
macro set doesnt define _SHIFT and _WIDTH definitions, should they be
added?

FWIW, I have this macro in my tree which makes short work of defining
mask, shift and width constants as a one liner:

/* Define SHIFT, LENGTH and MASK constants for a field within a register */

#define FIELD(reg, field, length, shift) \
enum { reg ## _ ## field ## _SHIFT = (shift)}; \
enum { reg ## _ ## field ## _LENGTH = (length)}; \
enum { reg ## _ ## field ## _MASK = (((1ULL << (length)) - 1) \
                                          << (shift)) };

Usage would be something like FIELD(PSTATE, SPSEL, 1, 0)

> +        break;
> +    case 0x1e: /* DAIFSet */
> +        env->pstate |= (imm << 6) & PSTATE_DAIF;
> +        break;
> +    case 0x1f: /* DAIFClear */
> +        env->pstate &= ~((imm << 6) & PSTATE_DAIF);

I wonder whether deposit should be extended with and/or (with
existing) versions to allow for consistency in places like this. In
SPSel we get the nice deposit based implementation but with the logic
function change here were are stuck with open codedness. Set and
clearing and fields should be common enough tree wide to warrant it
perhaps.

Regards,
Peter

> +        break;
> +    default:
> +        g_assert_not_reached();
> +    }
> +}
> +
>  /* ??? Flag setting arithmetic is awkward because we need to do comparisons.
>     The only way to do that in TCG is a conditional branch, which clobbers
>     all our temporaries.  For now implement these as helper functions.  */
> diff --git a/target-arm/translate-a64.c b/target-arm/translate-a64.c
> index d938e5e..a942609 100644
> --- a/target-arm/translate-a64.c
> +++ b/target-arm/translate-a64.c
> @@ -1106,7 +1106,29 @@ static void handle_sync(DisasContext *s, uint32_t insn,
>  static void handle_msr_i(DisasContext *s, uint32_t insn,
>                           unsigned int op1, unsigned int op2, unsigned int 
> crm)
>  {
> -    unsupported_encoding(s, insn);
> +    int op = op1 << 3 | op2;
> +    switch (op) {
> +    case 0x05: /* SPSel */
> +        if (s->current_pl == 0) {
> +            unallocated_encoding(s);
> +            return;
> +        }
> +        /* fall through */
> +    case 0x1e: /* DAIFSet */
> +    case 0x1f: /* DAIFClear */
> +    {
> +        TCGv_i32 tcg_imm = tcg_const_i32(crm);
> +        TCGv_i32 tcg_op = tcg_const_i32(op);
> +        gen_helper_msr_i_pstate(cpu_env, tcg_op, tcg_imm);
> +        tcg_temp_free_i32(tcg_imm);
> +        tcg_temp_free_i32(tcg_op);
> +        s->is_jmp = DISAS_UPDATE;
> +        break;
> +    }
> +    default:
> +        unallocated_encoding(s);
> +        return;
> +    }
>  }
>
>  static void gen_get_nzcv(TCGv_i64 tcg_rt)
> --
> 1.8.5
>
>