| Message ID | 1433169023-11617-1-git-send-email-aurelioremonda@gmail.com |
|---|---|
| State | New |
| Headers | show |
On 1 June 2015 at 15:30, Aurelio C. Remonda <aurelioremonda@gmail.com> wrote: Thanks for sending this patch. I have a few comments below. > I created an ARM_FEATURE_THUMB_DSP to be added to any non-M > thumb2-compatible CPU that uses DSP instructions. > There are 85 DSP instructions (all of them thumb2). On disas_thumb2_insn > the DSP feature is tested before the instruction is generated; if it's not > enabled then its an illegal op. Our general style for commit messages tends to be a bit more impersonal, so for instance "Create an ARM_FEATURE_THUMB_DSP" rather than "I created...". > Signed-off-by: Aurelio C. Remonda <aurelioremonda@gmail.com> As I noted in the other patch, the code that sets the feature bit for non-M-profile Thumb2 CPUs needs to go in this patch. > --- > target-arm/cpu.h | 1 + > target-arm/translate.c | 110 +++++++++++++++++++++++++++++++++++++++++++++++-- > 2 files changed, 107 insertions(+), 4 deletions(-) > > diff --git a/target-arm/cpu.h b/target-arm/cpu.h > index 21b5b8e..2e03d8e 100644 > --- a/target-arm/cpu.h > +++ b/target-arm/cpu.h > @@ -890,6 +890,7 @@ enum arm_features { > ARM_FEATURE_V8_SHA1, /* implements SHA1 part of v8 Crypto Extensions */ > ARM_FEATURE_V8_SHA256, /* implements SHA256 part of v8 Crypto Extensions */ > ARM_FEATURE_V8_PMULL, /* implements PMULL part of v8 Crypto Extensions */ > + ARM_FEATURE_THUMB_DSP, /* DSP insns supported in the Thumb encodings */ > }; > > static inline int arm_feature(CPUARMState *env, int feature) > diff --git a/target-arm/translate.c b/target-arm/translate.c > index 39692d7..2d14a2c 100644 > --- a/target-arm/translate.c > +++ b/target-arm/translate.c > @@ -9444,6 +9444,10 @@ static int disas_thumb2_insn(CPUARMState *env, DisasContext *s, uint16_t insn_hw > > op = (insn >> 21) & 0xf; > if (op == 6) { > + if (!arm_dc_feature(s, ARM_FEATURE_THUMB_DSP)) { > + /* pkhtb, pkfbt are DSP instructions */ These comments aren't really necessary I think -- it's obvious from the feature bit we're testing and from the code below what's going on. > + goto illegal_op; > + } > /* Halfword pack. */ > tmp = load_reg(s, rn); > tmp2 = load_reg(s, rm); > @@ -9518,13 +9522,35 @@ static int disas_thumb2_insn(CPUARMState *env, DisasContext *s, uint16_t insn_hw > switch (op) { > case 0: gen_sxth(tmp); break; > case 1: gen_uxth(tmp); break; > - case 2: gen_sxtb16(tmp); break; > - case 3: gen_uxtb16(tmp); break; > + case 2: > + if (!arm_dc_feature(s, ARM_FEATURE_THUMB_DSP)) { > + /* sxtab16, sxtb16 are DSP instructions */ > + tcg_temp_free_i32(tmp); > + goto illegal_op; > + } > + gen_sxtb16(tmp); > + break; > + case 3: > + if (!arm_dc_feature(s, ARM_FEATURE_THUMB_DSP)) { > + /* uxtb16, uxtab16 are DSP instructions */ > + tcg_temp_free_i32(tmp); > + goto illegal_op; > + } > + gen_uxtb16(tmp); > + break; > case 4: gen_sxtb(tmp); break; > case 5: gen_uxtb(tmp); break; > default: goto illegal_op; > } > if (rn != 15) { > + if (!arm_dc_feature(s, ARM_FEATURE_THUMB_DSP)) { > + /* sxtab, sxtah, uxtab, uxtah are DSP instructions. > + * sxtb, sxth, uxtb, uxth are not DSP according to > + * ARMv7-M Architecture Reference Manual > + */ > + tcg_temp_free_i32(tmp); > + goto illegal_op; > + } It looks like it would be fairly easy to hoist the illegal_op checks up above the load_reg() call, which then means we don't need to do a temp_free. You just need an extra switch on op like switch (op) { case 0: /* SXTAH, SXTH */ case 1: /* UXTAH, UXTH */ case 4: /* SXTAB, SXTB */ case 5: /* UXTAB, UXTB */ break; case 2: /* SXTAB16, SXTB16 */ case 3: /* UXTAB16, UXTB16 */ if (!arm_dc_feature(s, ARM_FEATURE_THUMB_DSP)) { goto illegal_op; } break; default: goto illegal_op; } (The default: case in the original switch then becomes a g_assert_not_reached();) You can check for rn != 15 up here too. > tmp2 = load_reg(s, rn); > if ((op >> 1) == 1) { > gen_add16(tmp, tmp2); > @@ -9537,6 +9563,12 @@ static int disas_thumb2_insn(CPUARMState *env, DisasContext *s, uint16_t insn_hw > break; > case 2: /* SIMD add/subtract. */ > op = (insn >> 20) & 7; > + if (!arm_dc_feature(s, ARM_FEATURE_THUMB_DSP)) { > + /* add16, sub16, asx, sax, add8, sub8 (with q, s, sh, u, uh, > + * and uq variants) and usad8, usada8 > + */ If we want to document the instructions handled by this case then the comment belongs at the start of it, before the assignment to 'op'. > + goto illegal_op; > + } > shift = (insn >> 4) & 7; > if ((op & 3) == 3 || (shift & 3) == 3) > goto illegal_op; > @@ -9550,6 +9582,10 @@ static int disas_thumb2_insn(CPUARMState *env, DisasContext *s, uint16_t insn_hw > op = ((insn >> 17) & 0x38) | ((insn >> 4) & 7); > if (op < 4) { > /* Saturating add/subtract. */ > + if (!arm_dc_feature(s, ARM_FEATURE_THUMB_DSP)) { > + /* qsub, qadd, qdadd, qdsub are DSP instructions. */ > + goto illegal_op; > + } > tmp = load_reg(s, rn); > tmp2 = load_reg(s, rm); > if (op & 1) > @@ -9575,6 +9611,11 @@ static int disas_thumb2_insn(CPUARMState *env, DisasContext *s, uint16_t insn_hw > gen_revsh(tmp); > break; > case 0x10: /* sel */ > + if (!arm_dc_feature(s, ARM_FEATURE_THUMB_DSP)) { > + /* sel is a DSP instruction. */ > + tcg_temp_free_i32(tmp); > + goto illegal_op; > + } This check could also be hoisted up to above the allocation of tmp. Your indentation on this section seems to have gone wrong. If you run your patches through scripts/checkpatch.pl they will catch this kind of style error for you. > tmp2 = load_reg(s, rm); > tmp3 = tcg_temp_new_i32(); > tcg_gen_ld_i32(tmp3, cpu_env, offsetof(CPUARMState, GE)); > @@ -9640,6 +9681,14 @@ static int disas_thumb2_insn(CPUARMState *env, DisasContext *s, uint16_t insn_hw > } > break; > case 1: /* 16 x 16 -> 32 */ > + if (!arm_dc_feature(s, ARM_FEATURE_THUMB_DSP)) { > + /* smlabb, smlabt, smlatb, smlatt, smulbb, smulbt, smultt > + * and smultb are DSP instructions > + */ > + tcg_temp_free_i32(tmp); > + tcg_temp_free_i32(tmp2); > + goto illegal_op; > + } > gen_mulxy(tmp, tmp2, op & 2, op & 1); > tcg_temp_free_i32(tmp2); > if (rs != 15) { > @@ -9650,6 +9699,12 @@ static int disas_thumb2_insn(CPUARMState *env, DisasContext *s, uint16_t insn_hw > break; > case 2: /* Dual multiply add. */ > case 4: /* Dual multiply subtract. */ > + if (!arm_dc_feature(s, ARM_FEATURE_THUMB_DSP)) { > + /* smlad, smladx, smlsd, smusd are DSP instructions */ > + tcg_temp_free_i32(tmp); > + tcg_temp_free_i32(tmp2); > + goto illegal_op; > + } > if (op) > gen_swap_half(tmp2); > gen_smul_dual(tmp, tmp2); > @@ -9672,6 +9727,12 @@ static int disas_thumb2_insn(CPUARMState *env, DisasContext *s, uint16_t insn_hw > } > break; > case 3: /* 32 * 16 -> 32msb */ > + if (!arm_dc_feature(s, ARM_FEATURE_THUMB_DSP)) { > + /* smlawb, smlawt, smulwt, smulwb are DSP instructions */ > + tcg_temp_free_i32(tmp); > + tcg_temp_free_i32(tmp2); > + goto illegal_op; > + } > if (op) > tcg_gen_sari_i32(tmp2, tmp2, 16); > else > @@ -9689,6 +9750,14 @@ static int disas_thumb2_insn(CPUARMState *env, DisasContext *s, uint16_t insn_hw > } > break; > case 5: case 6: /* 32 * 32 -> 32msb (SMMUL, SMMLA, SMMLS) */ > + if (!arm_dc_feature(s, ARM_FEATURE_THUMB_DSP)) { > + /* smmla, smmls, smmul, smuad, smmlar, > + * smmlsr, smmulr are DSP instructions > + */ > + tcg_temp_free_i32(tmp); > + tcg_temp_free_i32(tmp2); > + goto illegal_op; > + } > tmp64 = gen_muls_i64_i32(tmp, tmp2); > if (rs != 15) { > tmp = load_reg(s, rs); Again, all these checks inside this switch() would be more cleanly handled by having an initial switch() before we allocate the temporaries that deals with the illegal_op cases. > @@ -9735,6 +9804,12 @@ static int disas_thumb2_insn(CPUARMState *env, DisasContext *s, uint16_t insn_hw > store_reg(s, rd, tmp); > } else if ((op & 0xe) == 0xc) { > /* Dual multiply accumulate long. */ > + if (!arm_dc_feature(s, ARM_FEATURE_THUMB_DSP)) { > + /* smlald, smlsld are DSP instructions */ > + tcg_temp_free_i32(tmp); > + tcg_temp_free_i32(tmp2); > + goto illegal_op; > + } > if (op & 1) > gen_swap_half(tmp2); > gen_smul_dual(tmp, tmp2); > @@ -9758,6 +9833,14 @@ static int disas_thumb2_insn(CPUARMState *env, DisasContext *s, uint16_t insn_hw > } else { > if (op & 8) { > /* smlalxy */ > + if (!arm_dc_feature(s, ARM_FEATURE_THUMB_DSP)) { > + /* smlalbb, smlalbt, smlaltb, smlaltt > + * are DSP instructions > + */ > + tcg_temp_free_i32(tmp2); > + tcg_temp_free_i32(tmp); > + goto illegal_op; > + } > gen_mulxy(tmp, tmp2, op & 2, op & 1); > tcg_temp_free_i32(tmp2); > tmp64 = tcg_temp_new_i64(); Doing the illegal_op checks early for these two would require more painful surgery to the code so I think they're OK like this. > @@ -9770,6 +9853,11 @@ static int disas_thumb2_insn(CPUARMState *env, DisasContext *s, uint16_t insn_hw > } > if (op & 4) { > /* umaal */ > + if (!arm_dc_feature(s, ARM_FEATURE_THUMB_DSP)) { > + /* ummal is a DSP instruction */ > + tcg_temp_free_i64(tmp64); > + goto illegal_op; > + } > gen_addq_lo(s, tmp64, rs); > gen_addq_lo(s, tmp64, rd); > } else if (op & 0x40) { > @@ -10034,14 +10122,28 @@ static int disas_thumb2_insn(CPUARMState *env, DisasContext *s, uint16_t insn_hw > tmp2 = tcg_const_i32(imm); > if (op & 4) { > /* Unsigned. */ > - if ((op & 1) && shift == 0) > + if ((op & 1) && shift == 0){ You need a space before the '{' (again, checkpatch will tell you this). > + if (!arm_dc_feature(s, ARM_FEATURE_THUMB_DSP)) { > + /* usat16 is a DSP instruction */ > + tcg_temp_free_i32(tmp); > + tcg_temp_free_i32(tmp2); > + goto illegal_op; > + } > gen_helper_usat16(tmp, cpu_env, tmp, tmp2); > + } > else > gen_helper_usat(tmp, cpu_env, tmp, tmp2); ...and since you're touching the if() statement you need to also add braces on the else half of it. (Checkpatch again.) > } else { > /* Signed. */ > - if ((op & 1) && shift == 0) > + if ((op & 1) && shift == 0){ > + if (!arm_dc_feature(s, ARM_FEATURE_THUMB_DSP)) { > + /* ssat16 is a DSP instruction */ > + tcg_temp_free_i32(tmp); > + tcg_temp_free_i32(tmp2); > + goto illegal_op; > + } > gen_helper_ssat16(tmp, cpu_env, tmp, tmp2); > + } > else > gen_helper_ssat(tmp, cpu_env, tmp, tmp2); > } Same comments apply for this if..else. > 1.9.1 > thanks -- PMM
On Mon, Jun 1, 2015 at 10:54 AM, Peter Maydell <peter.maydell@linaro.org> wrote: > On 1 June 2015 at 15:30, Aurelio C. Remonda <aurelioremonda@gmail.com> wrote: > > Thanks for sending this patch. I have a few comments below. > >> I created an ARM_FEATURE_THUMB_DSP to be added to any non-M >> thumb2-compatible CPU that uses DSP instructions. >> There are 85 DSP instructions (all of them thumb2). On disas_thumb2_insn >> the DSP feature is tested before the instruction is generated; if it's not >> enabled then its an illegal op. > > Our general style for commit messages tends to be a bit more > impersonal, so for instance "Create an ARM_FEATURE_THUMB_DSP" > rather than "I created...". > >> Signed-off-by: Aurelio C. Remonda <aurelioremonda@gmail.com> > > As I noted in the other patch, the code that sets the feature bit > for non-M-profile Thumb2 CPUs needs to go in this patch. > >> --- >> target-arm/cpu.h | 1 + >> target-arm/translate.c | 110 +++++++++++++++++++++++++++++++++++++++++++++++-- >> 2 files changed, 107 insertions(+), 4 deletions(-) >> >> diff --git a/target-arm/cpu.h b/target-arm/cpu.h >> index 21b5b8e..2e03d8e 100644 >> --- a/target-arm/cpu.h >> +++ b/target-arm/cpu.h >> @@ -890,6 +890,7 @@ enum arm_features { >> ARM_FEATURE_V8_SHA1, /* implements SHA1 part of v8 Crypto Extensions */ >> ARM_FEATURE_V8_SHA256, /* implements SHA256 part of v8 Crypto Extensions */ >> ARM_FEATURE_V8_PMULL, /* implements PMULL part of v8 Crypto Extensions */ >> + ARM_FEATURE_THUMB_DSP, /* DSP insns supported in the Thumb encodings */ >> }; >> >> static inline int arm_feature(CPUARMState *env, int feature) >> diff --git a/target-arm/translate.c b/target-arm/translate.c >> index 39692d7..2d14a2c 100644 >> --- a/target-arm/translate.c >> +++ b/target-arm/translate.c >> @@ -9444,6 +9444,10 @@ static int disas_thumb2_insn(CPUARMState *env, DisasContext *s, uint16_t insn_hw >> >> op = (insn >> 21) & 0xf; >> if (op == 6) { >> + if (!arm_dc_feature(s, ARM_FEATURE_THUMB_DSP)) { >> + /* pkhtb, pkfbt are DSP instructions */ > > These comments aren't really necessary I think -- it's obvious from > the feature bit we're testing and from the code below what's > going on. > >> + goto illegal_op; >> + } >> /* Halfword pack. */ >> tmp = load_reg(s, rn); >> tmp2 = load_reg(s, rm); >> @@ -9518,13 +9522,35 @@ static int disas_thumb2_insn(CPUARMState *env, DisasContext *s, uint16_t insn_hw >> switch (op) { >> case 0: gen_sxth(tmp); break; >> case 1: gen_uxth(tmp); break; >> - case 2: gen_sxtb16(tmp); break; >> - case 3: gen_uxtb16(tmp); break; >> + case 2: >> + if (!arm_dc_feature(s, ARM_FEATURE_THUMB_DSP)) { >> + /* sxtab16, sxtb16 are DSP instructions */ >> + tcg_temp_free_i32(tmp); >> + goto illegal_op; >> + } >> + gen_sxtb16(tmp); >> + break; >> + case 3: >> + if (!arm_dc_feature(s, ARM_FEATURE_THUMB_DSP)) { >> + /* uxtb16, uxtab16 are DSP instructions */ >> + tcg_temp_free_i32(tmp); >> + goto illegal_op; >> + } >> + gen_uxtb16(tmp); >> + break; >> case 4: gen_sxtb(tmp); break; >> case 5: gen_uxtb(tmp); break; >> default: goto illegal_op; >> } >> if (rn != 15) { >> + if (!arm_dc_feature(s, ARM_FEATURE_THUMB_DSP)) { >> + /* sxtab, sxtah, uxtab, uxtah are DSP instructions. >> + * sxtb, sxth, uxtb, uxth are not DSP according to >> + * ARMv7-M Architecture Reference Manual >> + */ >> + tcg_temp_free_i32(tmp); >> + goto illegal_op; >> + } > > It looks like it would be fairly easy to hoist the illegal_op checks > up above the load_reg() call, which then means we don't need to do > a temp_free. You just need an extra switch on op like > > switch (op) { > case 0: /* SXTAH, SXTH */ > case 1: /* UXTAH, UXTH */ > case 4: /* SXTAB, SXTB */ > case 5: /* UXTAB, UXTB */ > break; > case 2: /* SXTAB16, SXTB16 */ > case 3: /* UXTAB16, UXTB16 */ > if (!arm_dc_feature(s, ARM_FEATURE_THUMB_DSP)) { > goto illegal_op; > } > break; > default: > goto illegal_op; > } > (The default: case in the original switch then becomes a > g_assert_not_reached();) > > You can check for rn != 15 up here too. > > >> tmp2 = load_reg(s, rn); >> if ((op >> 1) == 1) { >> gen_add16(tmp, tmp2); >> @@ -9537,6 +9563,12 @@ static int disas_thumb2_insn(CPUARMState *env, DisasContext *s, uint16_t insn_hw >> break; >> case 2: /* SIMD add/subtract. */ >> op = (insn >> 20) & 7; >> + if (!arm_dc_feature(s, ARM_FEATURE_THUMB_DSP)) { >> + /* add16, sub16, asx, sax, add8, sub8 (with q, s, sh, u, uh, >> + * and uq variants) and usad8, usada8 >> + */ > > If we want to document the instructions handled by this case then > the comment belongs at the start of it, before the assignment to 'op'. > >> + goto illegal_op; >> + } >> shift = (insn >> 4) & 7; >> if ((op & 3) == 3 || (shift & 3) == 3) >> goto illegal_op; >> @@ -9550,6 +9582,10 @@ static int disas_thumb2_insn(CPUARMState *env, DisasContext *s, uint16_t insn_hw >> op = ((insn >> 17) & 0x38) | ((insn >> 4) & 7); >> if (op < 4) { >> /* Saturating add/subtract. */ >> + if (!arm_dc_feature(s, ARM_FEATURE_THUMB_DSP)) { >> + /* qsub, qadd, qdadd, qdsub are DSP instructions. */ >> + goto illegal_op; >> + } >> tmp = load_reg(s, rn); >> tmp2 = load_reg(s, rm); >> if (op & 1) >> @@ -9575,6 +9611,11 @@ static int disas_thumb2_insn(CPUARMState *env, DisasContext *s, uint16_t insn_hw >> gen_revsh(tmp); >> break; >> case 0x10: /* sel */ >> + if (!arm_dc_feature(s, ARM_FEATURE_THUMB_DSP)) { >> + /* sel is a DSP instruction. */ >> + tcg_temp_free_i32(tmp); >> + goto illegal_op; >> + } > > This check could also be hoisted up to above the allocation of tmp. > > Your indentation on this section seems to have gone wrong. > > If you run your patches through scripts/checkpatch.pl they will catch > this kind of style error for you. > >> tmp2 = load_reg(s, rm); >> tmp3 = tcg_temp_new_i32(); >> tcg_gen_ld_i32(tmp3, cpu_env, offsetof(CPUARMState, GE)); >> @@ -9640,6 +9681,14 @@ static int disas_thumb2_insn(CPUARMState *env, DisasContext *s, uint16_t insn_hw >> } >> break; >> case 1: /* 16 x 16 -> 32 */ >> + if (!arm_dc_feature(s, ARM_FEATURE_THUMB_DSP)) { >> + /* smlabb, smlabt, smlatb, smlatt, smulbb, smulbt, smultt >> + * and smultb are DSP instructions >> + */ >> + tcg_temp_free_i32(tmp); >> + tcg_temp_free_i32(tmp2); >> + goto illegal_op; >> + } >> gen_mulxy(tmp, tmp2, op & 2, op & 1); >> tcg_temp_free_i32(tmp2); >> if (rs != 15) { >> @@ -9650,6 +9699,12 @@ static int disas_thumb2_insn(CPUARMState *env, DisasContext *s, uint16_t insn_hw >> break; >> case 2: /* Dual multiply add. */ >> case 4: /* Dual multiply subtract. */ >> + if (!arm_dc_feature(s, ARM_FEATURE_THUMB_DSP)) { >> + /* smlad, smladx, smlsd, smusd are DSP instructions */ >> + tcg_temp_free_i32(tmp); >> + tcg_temp_free_i32(tmp2); >> + goto illegal_op; >> + } >> if (op) >> gen_swap_half(tmp2); >> gen_smul_dual(tmp, tmp2); >> @@ -9672,6 +9727,12 @@ static int disas_thumb2_insn(CPUARMState *env, DisasContext *s, uint16_t insn_hw >> } >> break; >> case 3: /* 32 * 16 -> 32msb */ >> + if (!arm_dc_feature(s, ARM_FEATURE_THUMB_DSP)) { >> + /* smlawb, smlawt, smulwt, smulwb are DSP instructions */ >> + tcg_temp_free_i32(tmp); >> + tcg_temp_free_i32(tmp2); >> + goto illegal_op; >> + } >> if (op) >> tcg_gen_sari_i32(tmp2, tmp2, 16); >> else >> @@ -9689,6 +9750,14 @@ static int disas_thumb2_insn(CPUARMState *env, DisasContext *s, uint16_t insn_hw >> } >> break; >> case 5: case 6: /* 32 * 32 -> 32msb (SMMUL, SMMLA, SMMLS) */ >> + if (!arm_dc_feature(s, ARM_FEATURE_THUMB_DSP)) { >> + /* smmla, smmls, smmul, smuad, smmlar, >> + * smmlsr, smmulr are DSP instructions >> + */ >> + tcg_temp_free_i32(tmp); >> + tcg_temp_free_i32(tmp2); >> + goto illegal_op; >> + } >> tmp64 = gen_muls_i64_i32(tmp, tmp2); >> if (rs != 15) { >> tmp = load_reg(s, rs); > > Again, all these checks inside this switch() would be more cleanly handled > by having an initial switch() before we allocate the temporaries that deals > with the illegal_op cases. > >> @@ -9735,6 +9804,12 @@ static int disas_thumb2_insn(CPUARMState *env, DisasContext *s, uint16_t insn_hw >> store_reg(s, rd, tmp); >> } else if ((op & 0xe) == 0xc) { >> /* Dual multiply accumulate long. */ >> + if (!arm_dc_feature(s, ARM_FEATURE_THUMB_DSP)) { >> + /* smlald, smlsld are DSP instructions */ >> + tcg_temp_free_i32(tmp); >> + tcg_temp_free_i32(tmp2); >> + goto illegal_op; >> + } >> if (op & 1) >> gen_swap_half(tmp2); >> gen_smul_dual(tmp, tmp2); > >> @@ -9758,6 +9833,14 @@ static int disas_thumb2_insn(CPUARMState *env, DisasContext *s, uint16_t insn_hw >> } else { >> if (op & 8) { >> /* smlalxy */ >> + if (!arm_dc_feature(s, ARM_FEATURE_THUMB_DSP)) { >> + /* smlalbb, smlalbt, smlaltb, smlaltt >> + * are DSP instructions >> + */ >> + tcg_temp_free_i32(tmp2); >> + tcg_temp_free_i32(tmp); >> + goto illegal_op; >> + } >> gen_mulxy(tmp, tmp2, op & 2, op & 1); >> tcg_temp_free_i32(tmp2); >> tmp64 = tcg_temp_new_i64(); > > Doing the illegal_op checks early for these two would require > more painful surgery to the code so I think they're OK like this. > >> @@ -9770,6 +9853,11 @@ static int disas_thumb2_insn(CPUARMState *env, DisasContext *s, uint16_t insn_hw >> } >> if (op & 4) { >> /* umaal */ >> + if (!arm_dc_feature(s, ARM_FEATURE_THUMB_DSP)) { >> + /* ummal is a DSP instruction */ >> + tcg_temp_free_i64(tmp64); >> + goto illegal_op; >> + } >> gen_addq_lo(s, tmp64, rs); >> gen_addq_lo(s, tmp64, rd); >> } else if (op & 0x40) { >> @@ -10034,14 +10122,28 @@ static int disas_thumb2_insn(CPUARMState *env, DisasContext *s, uint16_t insn_hw >> tmp2 = tcg_const_i32(imm); >> if (op & 4) { >> /* Unsigned. */ >> - if ((op & 1) && shift == 0) >> + if ((op & 1) && shift == 0){ > > You need a space before the '{' (again, checkpatch will tell you this). > >> + if (!arm_dc_feature(s, ARM_FEATURE_THUMB_DSP)) { >> + /* usat16 is a DSP instruction */ >> + tcg_temp_free_i32(tmp); >> + tcg_temp_free_i32(tmp2); >> + goto illegal_op; >> + } >> gen_helper_usat16(tmp, cpu_env, tmp, tmp2); >> + } >> else >> gen_helper_usat(tmp, cpu_env, tmp, tmp2); > > ...and since you're touching the if() statement you need to also add > braces on the else half of it. (Checkpatch again.) > >> } else { >> /* Signed. */ >> - if ((op & 1) && shift == 0) >> + if ((op & 1) && shift == 0){ >> + if (!arm_dc_feature(s, ARM_FEATURE_THUMB_DSP)) { >> + /* ssat16 is a DSP instruction */ >> + tcg_temp_free_i32(tmp); >> + tcg_temp_free_i32(tmp2); >> + goto illegal_op; >> + } >> gen_helper_ssat16(tmp, cpu_env, tmp, tmp2); >> + } >> else Also note that the } is on same line as the else. End result will be: } else { Regards, Peter >> gen_helper_ssat(tmp, cpu_env, tmp, tmp2); >> } > > Same comments apply for this if..else. > >> 1.9.1 >> > > thanks > -- PMM >
diff --git a/target-arm/cpu.h b/target-arm/cpu.h index 21b5b8e..2e03d8e 100644 --- a/target-arm/cpu.h +++ b/target-arm/cpu.h @@ -890,6 +890,7 @@ enum arm_features { ARM_FEATURE_V8_SHA1, /* implements SHA1 part of v8 Crypto Extensions */ ARM_FEATURE_V8_SHA256, /* implements SHA256 part of v8 Crypto Extensions */ ARM_FEATURE_V8_PMULL, /* implements PMULL part of v8 Crypto Extensions */ + ARM_FEATURE_THUMB_DSP, /* DSP insns supported in the Thumb encodings */ }; static inline int arm_feature(CPUARMState *env, int feature) diff --git a/target-arm/translate.c b/target-arm/translate.c index 39692d7..2d14a2c 100644 --- a/target-arm/translate.c +++ b/target-arm/translate.c @@ -9444,6 +9444,10 @@ static int disas_thumb2_insn(CPUARMState *env, DisasContext *s, uint16_t insn_hw op = (insn >> 21) & 0xf; if (op == 6) { + if (!arm_dc_feature(s, ARM_FEATURE_THUMB_DSP)) { + /* pkhtb, pkfbt are DSP instructions */ + goto illegal_op; + } /* Halfword pack. */ tmp = load_reg(s, rn); tmp2 = load_reg(s, rm); @@ -9518,13 +9522,35 @@ static int disas_thumb2_insn(CPUARMState *env, DisasContext *s, uint16_t insn_hw switch (op) { case 0: gen_sxth(tmp); break; case 1: gen_uxth(tmp); break; - case 2: gen_sxtb16(tmp); break; - case 3: gen_uxtb16(tmp); break; + case 2: + if (!arm_dc_feature(s, ARM_FEATURE_THUMB_DSP)) { + /* sxtab16, sxtb16 are DSP instructions */ + tcg_temp_free_i32(tmp); + goto illegal_op; + } + gen_sxtb16(tmp); + break; + case 3: + if (!arm_dc_feature(s, ARM_FEATURE_THUMB_DSP)) { + /* uxtb16, uxtab16 are DSP instructions */ + tcg_temp_free_i32(tmp); + goto illegal_op; + } + gen_uxtb16(tmp); + break; case 4: gen_sxtb(tmp); break; case 5: gen_uxtb(tmp); break; default: goto illegal_op; } if (rn != 15) { + if (!arm_dc_feature(s, ARM_FEATURE_THUMB_DSP)) { + /* sxtab, sxtah, uxtab, uxtah are DSP instructions. + * sxtb, sxth, uxtb, uxth are not DSP according to + * ARMv7-M Architecture Reference Manual + */ + tcg_temp_free_i32(tmp); + goto illegal_op; + } tmp2 = load_reg(s, rn); if ((op >> 1) == 1) { gen_add16(tmp, tmp2); @@ -9537,6 +9563,12 @@ static int disas_thumb2_insn(CPUARMState *env, DisasContext *s, uint16_t insn_hw break; case 2: /* SIMD add/subtract. */ op = (insn >> 20) & 7; + if (!arm_dc_feature(s, ARM_FEATURE_THUMB_DSP)) { + /* add16, sub16, asx, sax, add8, sub8 (with q, s, sh, u, uh, + * and uq variants) and usad8, usada8 + */ + goto illegal_op; + } shift = (insn >> 4) & 7; if ((op & 3) == 3 || (shift & 3) == 3) goto illegal_op; @@ -9550,6 +9582,10 @@ static int disas_thumb2_insn(CPUARMState *env, DisasContext *s, uint16_t insn_hw op = ((insn >> 17) & 0x38) | ((insn >> 4) & 7); if (op < 4) { /* Saturating add/subtract. */ + if (!arm_dc_feature(s, ARM_FEATURE_THUMB_DSP)) { + /* qsub, qadd, qdadd, qdsub are DSP instructions. */ + goto illegal_op; + } tmp = load_reg(s, rn); tmp2 = load_reg(s, rm); if (op & 1) @@ -9575,6 +9611,11 @@ static int disas_thumb2_insn(CPUARMState *env, DisasContext *s, uint16_t insn_hw gen_revsh(tmp); break; case 0x10: /* sel */ + if (!arm_dc_feature(s, ARM_FEATURE_THUMB_DSP)) { + /* sel is a DSP instruction. */ + tcg_temp_free_i32(tmp); + goto illegal_op; + } tmp2 = load_reg(s, rm); tmp3 = tcg_temp_new_i32(); tcg_gen_ld_i32(tmp3, cpu_env, offsetof(CPUARMState, GE)); @@ -9640,6 +9681,14 @@ static int disas_thumb2_insn(CPUARMState *env, DisasContext *s, uint16_t insn_hw } break; case 1: /* 16 x 16 -> 32 */ + if (!arm_dc_feature(s, ARM_FEATURE_THUMB_DSP)) { + /* smlabb, smlabt, smlatb, smlatt, smulbb, smulbt, smultt + * and smultb are DSP instructions + */ + tcg_temp_free_i32(tmp); + tcg_temp_free_i32(tmp2); + goto illegal_op; + } gen_mulxy(tmp, tmp2, op & 2, op & 1); tcg_temp_free_i32(tmp2); if (rs != 15) { @@ -9650,6 +9699,12 @@ static int disas_thumb2_insn(CPUARMState *env, DisasContext *s, uint16_t insn_hw break; case 2: /* Dual multiply add. */ case 4: /* Dual multiply subtract. */ + if (!arm_dc_feature(s, ARM_FEATURE_THUMB_DSP)) { + /* smlad, smladx, smlsd, smusd are DSP instructions */ + tcg_temp_free_i32(tmp); + tcg_temp_free_i32(tmp2); + goto illegal_op; + } if (op) gen_swap_half(tmp2); gen_smul_dual(tmp, tmp2); @@ -9672,6 +9727,12 @@ static int disas_thumb2_insn(CPUARMState *env, DisasContext *s, uint16_t insn_hw } break; case 3: /* 32 * 16 -> 32msb */ + if (!arm_dc_feature(s, ARM_FEATURE_THUMB_DSP)) { + /* smlawb, smlawt, smulwt, smulwb are DSP instructions */ + tcg_temp_free_i32(tmp); + tcg_temp_free_i32(tmp2); + goto illegal_op; + } if (op) tcg_gen_sari_i32(tmp2, tmp2, 16); else @@ -9689,6 +9750,14 @@ static int disas_thumb2_insn(CPUARMState *env, DisasContext *s, uint16_t insn_hw } break; case 5: case 6: /* 32 * 32 -> 32msb (SMMUL, SMMLA, SMMLS) */ + if (!arm_dc_feature(s, ARM_FEATURE_THUMB_DSP)) { + /* smmla, smmls, smmul, smuad, smmlar, + * smmlsr, smmulr are DSP instructions + */ + tcg_temp_free_i32(tmp); + tcg_temp_free_i32(tmp2); + goto illegal_op; + } tmp64 = gen_muls_i64_i32(tmp, tmp2); if (rs != 15) { tmp = load_reg(s, rs); @@ -9735,6 +9804,12 @@ static int disas_thumb2_insn(CPUARMState *env, DisasContext *s, uint16_t insn_hw store_reg(s, rd, tmp); } else if ((op & 0xe) == 0xc) { /* Dual multiply accumulate long. */ + if (!arm_dc_feature(s, ARM_FEATURE_THUMB_DSP)) { + /* smlald, smlsld are DSP instructions */ + tcg_temp_free_i32(tmp); + tcg_temp_free_i32(tmp2); + goto illegal_op; + } if (op & 1) gen_swap_half(tmp2); gen_smul_dual(tmp, tmp2); @@ -9758,6 +9833,14 @@ static int disas_thumb2_insn(CPUARMState *env, DisasContext *s, uint16_t insn_hw } else { if (op & 8) { /* smlalxy */ + if (!arm_dc_feature(s, ARM_FEATURE_THUMB_DSP)) { + /* smlalbb, smlalbt, smlaltb, smlaltt + * are DSP instructions + */ + tcg_temp_free_i32(tmp2); + tcg_temp_free_i32(tmp); + goto illegal_op; + } gen_mulxy(tmp, tmp2, op & 2, op & 1); tcg_temp_free_i32(tmp2); tmp64 = tcg_temp_new_i64(); @@ -9770,6 +9853,11 @@ static int disas_thumb2_insn(CPUARMState *env, DisasContext *s, uint16_t insn_hw } if (op & 4) { /* umaal */ + if (!arm_dc_feature(s, ARM_FEATURE_THUMB_DSP)) { + /* ummal is a DSP instruction */ + tcg_temp_free_i64(tmp64); + goto illegal_op; + } gen_addq_lo(s, tmp64, rs); gen_addq_lo(s, tmp64, rd); } else if (op & 0x40) { @@ -10034,14 +10122,28 @@ static int disas_thumb2_insn(CPUARMState *env, DisasContext *s, uint16_t insn_hw tmp2 = tcg_const_i32(imm); if (op & 4) { /* Unsigned. */ - if ((op & 1) && shift == 0) + if ((op & 1) && shift == 0){ + if (!arm_dc_feature(s, ARM_FEATURE_THUMB_DSP)) { + /* usat16 is a DSP instruction */ + tcg_temp_free_i32(tmp); + tcg_temp_free_i32(tmp2); + goto illegal_op; + } gen_helper_usat16(tmp, cpu_env, tmp, tmp2); + } else gen_helper_usat(tmp, cpu_env, tmp, tmp2); } else { /* Signed. */ - if ((op & 1) && shift == 0) + if ((op & 1) && shift == 0){ + if (!arm_dc_feature(s, ARM_FEATURE_THUMB_DSP)) { + /* ssat16 is a DSP instruction */ + tcg_temp_free_i32(tmp); + tcg_temp_free_i32(tmp2); + goto illegal_op; + } gen_helper_ssat16(tmp, cpu_env, tmp, tmp2); + } else gen_helper_ssat(tmp, cpu_env, tmp, tmp2); }
I created an ARM_FEATURE_THUMB_DSP to be added to any non-M thumb2-compatible CPU that uses DSP instructions. There are 85 DSP instructions (all of them thumb2). On disas_thumb2_insn the DSP feature is tested before the instruction is generated; if it's not enabled then its an illegal op. Signed-off-by: Aurelio C. Remonda <aurelioremonda@gmail.com> --- target-arm/cpu.h | 1 + target-arm/translate.c | 110 +++++++++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 107 insertions(+), 4 deletions(-) 1.9.1