On 1 June 2015 at 15:30, Aurelio C. Remonda <aurelioremo...@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 <aurelioremo...@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
