| Message ID | 1391183143-30724-10-git-send-email-peter.maydell@linaro.org |
|---|---|
| State | New |
| Headers | show |
On Sat, Feb 1, 2014 at 1:45 AM, Peter Maydell <peter.maydell@linaro.org> wrote: > Implement the MSR (immediate) instructions, which can update the > PSTATE SP and DAIF fields. > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > --- > 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 > >
On 5 February 2014 06:23, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote: > On Sat, Feb 1, 2014 at 1:45 AM, Peter Maydell <peter.maydell@linaro.org> wrote: >> Implement the MSR (immediate) instructions, which can update the >> PSTATE SP and DAIF fields. >> >> Signed-off-by: Peter Maydell <peter.maydell@linaro.org> >> --- >> 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
On 5 February 2014 06:23, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote: > On Sat, Feb 1, 2014 at 1:45 AM, Peter Maydell <peter.maydell@linaro.org> wrote: >> + 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) So, I kind of like this, but I'm a bit reluctant to tie up this patchset in "add a new generic facility to bitops.h", and current handling for pstate/cpsr has a lot of hardcoded constants for bit positions. Is it OK if we do this as a separate cleanup, or do you feel strongly we should bite the bullet and do it as part of this series? thanks -- PMM
On Sat, Feb 15, 2014 at 2:41 AM, Peter Maydell <peter.maydell@linaro.org> wrote: > On 5 February 2014 06:23, Peter Crosthwaite > <peter.crosthwaite@xilinx.com> wrote: >> On Sat, Feb 1, 2014 at 1:45 AM, Peter Maydell <peter.maydell@linaro.org> wrote: >>> + 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) > > So, I kind of like this, but I'm a bit reluctant to tie up > this patchset in "add a new generic facility to bitops.h", > and current handling for pstate/cpsr has a lot of hardcoded > constants for bit positions. Is it OK if we do this as a > separate cleanup, or do you feel strongly we should bite > the bullet and do it as part of this series? > No I think its follow up. Regards, Peter > thanks > -- PMM >
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); + break; + case 0x1e: /* DAIFSet */ + env->pstate |= (imm << 6) & PSTATE_DAIF; + break; + case 0x1f: /* DAIFClear */ + env->pstate &= ~((imm << 6) & PSTATE_DAIF); + 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)
Implement the MSR (immediate) instructions, which can update the PSTATE SP and DAIF fields. Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- 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(-)