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

[Peter Maydell]

On 5 February 2014 06:23, Peter Crosthwaite
<address@hidden> wrote:
> 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)

Hmm. I guess we could use some more consistent structure in
defining bit macros. (reg, field, start, len) would be a better
argument order though, to match the extract and deposit fns.

>> +        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.

I dunno. Deposit is a function mostly because "clear old
field to zeroes then insert new value" is complicated enough
that it's easy to miscode it. Plain force-set and force-clear
I think is simple enough (and not all that common) that I'm
happy to opencode it.

One point I need to think about a little more with the DAIF
bits is whether we should be keeping them in one place for
AArch32 and AArch64 -- at the moment an AArch32 core's
IF bits are in cpsr. Having them in one location would make
the cpu-exec.c code a bit simpler.

thanks
-- PMM