On 5 February 2014 06:23, Peter Crosthwaite
<peter.crosthwaite@xxxxxxxxxx> wrote:
> On Sat, Feb 1, 2014 at 1:45 AM, Peter Maydell <peter.maydell@xxxxxxxxxx> wrote:
>> Implement the MSR (immediate) instructions, which can update the
>> PSTATE SP and DAIF fields.
>>
>> Signed-off-by: Peter Maydell <peter.maydell@xxxxxxxxxx>
>> ---
>> 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
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/cucslists/listinfo/kvmarm
