| Message ID | 1432847398-9969-1-git-send-email-aurelioremonda@gmail.com |
|---|---|
| State | New |
| Headers | show |
> On 29 May 2015, at 00:09, Aurelio C. Remonda <aurelioremonda@gmail.com> wrote: > > This patch adds the Cortex-M4 CPU. The M4 is basically the same as the M3, > the main differences being the DSP instructions and an optional FPU. > The DSP instructions are already implemented in Qemu, as the A and R profiles > use them. great! Peter mentioned some differences in exception processing, did you check if they require changes in emulation? > The optional FPU in the M4 could be added in the future as a "Cortex-M4F" CPU. in my implementation I had a single name ("cortex-m4") and some flags, but a separate name is probably better. can we reserve { .name = "cortex-m4f", ... } for this purpose? > All we'd have to do is add the ARM_FEATURE_VFP4 to the initfn. if it is that simple, why don't we add it in for now? regards, Liviu
On 28 May 2015 at 22:22, Liviu Ionescu <ilg@livius.net> wrote: > >> On 29 May 2015, at 00:09, Aurelio C. Remonda <aurelioremonda@gmail.com> wrote: >> All we'd have to do is add the ARM_FEATURE_VFP4 to the initfn. > > if it is that simple, why don't we add it in for now? It's not -- the FPU needs support in the exception handling (lazy FPU saving is particularly interesting, but there's also a bunch of minor stuff like "expose the FPU version registers as memory mapped regs" IIRC). -- PMM
2015-05-28 18:22 GMT-03:00 Liviu Ionescu <ilg@livius.net>: > > On 29 May 2015, at 00:09, Aurelio C. Remonda <aurelioremonda@gmail.com> wrote: > > The optional FPU in the M4 could be added in the future as a "Cortex-M4F" CPU. > > in my implementation I had a single name ("cortex-m4") and some flags, but a separate name is probably better. can we reserve > > { .name = "cortex-m4f", ... } > > for this purpose? Thanks for the feedback! Yes, we could but I think its outside of the scope of this particular contribution. > > All we'd have to do is add the ARM_FEATURE_VFP4 to the initfn. > > if it is that simple, why don't we add it in for now? As Peter said, it may not actually be that simple. Perhaps we could commit this now and add the VFP in the future?
On 29 May 2015 at 13:55, aurelio remonda <aurelioremonda@gmail.com> wrote
> Perhaps we could commit this now and add the VFP in the future?
That's how we've generally worked, yes -- if you just ask
for a "Cortex-A9" or whatever you get the maximum set of
features that CPU can support (so always Neon & VFP even if
some A9 hardware is configured without VFP), and if we add
support for a feature that the hardware has after the
CPU model was first added then we add the feature to that
CPU. (So we didn't have TrustZone support when the A9
was initially put into QEMU, but when we put in the TZ
support we turned on the ARM_FEATURE_EL3 bit for the A9 by
default.)
If we need to also support CPUs of that model that don't have
a particular optional feature we can do that via a CPU property
(as we do with 'has_el3' for disabling TZ support). Then a board
that wants an M4 without FPU can set the property to false, or
a user who wants to control it from the command line can do so
via -cpu cortex-m4,has-fpu=false or similar.
-- PMM
> On 29 May 2015, at 15:55, aurelio remonda <aurelioremonda@gmail.com> wrote: > > 2015-05-28 18:22 GMT-03:00 Liviu Ionescu <ilg@livius.net>: >>> On 29 May 2015, at 00:09, Aurelio C. Remonda <aurelioremonda@gmail.com> wrote: >>> The optional FPU in the M4 could be added in the future as a "Cortex-M4F" CPU. >> >> in my implementation I had a single name ("cortex-m4") and some flags, but a separate name is probably better. can we reserve >> >> { .name = "cortex-m4f", ... } >> >> for this purpose? > > Thanks for the feedback! Yes, we could but I think its outside of the > scope of this particular contribution. ok, I already updated my code to use cortex-m4 or cortex-m4f. > >>> All we'd have to do is add the ARM_FEATURE_VFP4 to the initfn. >> >> if it is that simple, why don't we add it in for now? > > As Peter said, it may not actually be that simple. > > Perhaps we could commit this now and add the VFP in the future? this is fine with me. unfortunately I cannot review your patch, since I have not enough experience with that part of qemu. regards, Liviu
On Fri, May 29, 2015 at 4:15 PM, Liviu Ionescu <ilg@livius.net> wrote: > this is fine with me. > > unfortunately I cannot review your patch, since I have not enough experience with that part of qemu. I think Peter M is the maintainer for Target-ARM. Peter, is this OK to commit?
On 29 May 2015 at 20:16, Martin Galvan <martin.galvan@tallertechnologies.com> wrote: > On Fri, May 29, 2015 at 4:15 PM, Liviu Ionescu <ilg@livius.net> wrote: >> this is fine with me. >> >> unfortunately I cannot review your patch, since I have not enough experience with that part of qemu. > > I think Peter M is the maintainer for Target-ARM. > > Peter, is this OK to commit? The patch is on my to-review queue; unfortunately I'm quite heavily loaded with patches to review at the moment, so it may be a week or so before I can get to this one in detail. Sorry about that. At a quick glance it looks broadly OK. Personally I prefer hoisting the "UNDEF if feature missing" checks earlier in the decode function rather than having them later and having to do manual tcg_temp_free operations, but this isn't always feasible with the existing structure of the code. I might also split it into two patches: one which just adds and implements the new feature bit, and then one which creates the new M4 CPU. (That way if it turns out that we need to make other changes for the M4 before we can enable it, the first patch can still be reviewed and possibly committed even if we have to wait before putting in the new CPU itself.) thanks -- PMM
On Thu, May 28, 2015 at 2:09 PM, Aurelio C. Remonda <aurelioremonda@gmail.com> wrote: > This patch adds the Cortex-M4 CPU. The M4 is basically the same as the M3, > the main differences being the DSP instructions and an optional FPU. > The DSP instructions are already implemented in Qemu, as the A and R profiles > use them. > > I created an ARM_FEATURE_THUMB_DSP to be added to any non-M thumb2-compatible > CPU that uses DSP instructions, and I manually added it to the M4 in its initfn. > This should be a separate patch, one for your refactoring adding the new ARM_FEATURE then a second one for the cortex-m4. > The optional FPU in the M4 could be added in the future as a "Cortex-M4F" CPU. > All we'd have to do is add the ARM_FEATURE_VFP4 to the initfn. > > 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.c | 22 +++++++++ > target-arm/cpu.h | 1 + > target-arm/translate.c | 130 +++++++++++++++++++++++++++++++++++++++++++++++-- > 3 files changed, 149 insertions(+), 4 deletions(-) > > diff --git a/target-arm/cpu.c b/target-arm/cpu.c > index 3ca3fa8..9be4a06 100644 > --- a/target-arm/cpu.c > +++ b/target-arm/cpu.c > @@ -512,6 +512,10 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp) > if (arm_feature(env, ARM_FEATURE_CBAR_RO)) { > set_feature(env, ARM_FEATURE_CBAR); > } > + if (arm_feature(env,ARM_FEATURE_THUMB2) && > + !arm_feature(env,ARM_FEATURE_M)) { > + set_feature(env, ARM_FEATURE_THUMB_DSP); > + } > > if (cpu->reset_hivecs) { > cpu->reset_sctlr |= (1 << 13); > @@ -761,6 +765,22 @@ static void cortex_m3_initfn(Object *obj) > set_feature(&cpu->env, ARM_FEATURE_M); > cpu->midr = 0x410fc231; > } > +static void cortex_m4_initfn(Object *obj) > +{ > + ARMCPU *cpu = ARM_CPU(obj); > + set_feature(&cpu->env, ARM_FEATURE_V7); > + set_feature(&cpu->env, ARM_FEATURE_M); > + set_feature(&cpu->env, ARM_FEATURE_THUMB_DSP); > + cpu->midr = 0x410fc240; > + /* Main id register CPUID bit assignments > + * Bits NAME Function > + * [31:24] IMPLEMENTER Indicates implementor: 0x41 = ARM > + * [23:20] VARIANT Indicates processor revision: 0x0 = Revision 0 > + * [19:16] (Constant) Reads as 0xF > + * [15:4] PARTNO Indicates part number: 0xC24 = Cortex-M4 > + * [3:0] REVISION Indicates patch release: 0x0 = Patch 0. > + */ > +} > > static void arm_v7m_class_init(ObjectClass *oc, void *data) > { > @@ -1164,6 +1184,8 @@ static const ARMCPUInfo arm_cpus[] = { > { .name = "arm11mpcore", .initfn = arm11mpcore_initfn }, > { .name = "cortex-m3", .initfn = cortex_m3_initfn, > .class_init = arm_v7m_class_init }, > + { .name = "cortex-m4", .initfn = cortex_m4_initfn, > + .class_init = arm_v7m_class_init }, > { .name = "cortex-a8", .initfn = cortex_a8_initfn }, > { .name = "cortex-a9", .initfn = cortex_a9_initfn }, > { .name = "cortex-a15", .initfn = cortex_a15_initfn }, > diff --git a/target-arm/cpu.h b/target-arm/cpu.h > index d4a5899..fa5c3bc 100644 > --- a/target-arm/cpu.h > +++ b/target-arm/cpu.h > @@ -886,6 +886,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 */ Space after "," > }; > > static inline int arm_feature(CPUARMState *env, int feature) > diff --git a/target-arm/translate.c b/target-arm/translate.c > index 9116529..c9d2e4f 100644 > --- a/target-arm/translate.c > +++ b/target-arm/translate.c > @@ -9428,6 +9428,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); > @@ -9502,13 +9506,37 @@ 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))){ Space after ))). Does checkpatch catch this? one set of parenthesis un-needed > + /* 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; > } > + /*-sxtab->-sxtab16->*/ > 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 > + */ > + /* need to free this so there's no TCG temporary leak */ > + tcg_temp_free_i32(tmp); > + goto illegal_op; > + } > tmp2 = load_reg(s, rn); > if ((op >> 1) == 1) { > gen_add16(tmp, tmp2); > @@ -9521,6 +9549,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)) ){ same. Also extra space here. > + /* 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; > @@ -9532,8 +9566,13 @@ static int disas_thumb2_insn(CPUARMState *env, DisasContext *s, uint16_t insn_hw > break; > case 3: /* Other data processing. */ > op = ((insn >> 17) & 0x38) | ((insn >> 4) & 7); > + Out of scope change. > 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) > @@ -9559,6 +9598,12 @@ 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. */ > + /* need to free this so there's no TCG temporary leak */ > + 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)); > @@ -9624,6 +9669,15 @@ 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 > + */ > + /* need to free this so there's no TCG temporary leak */ Comment un-needed. Regards, Peter > + 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) { > @@ -9634,6 +9688,14 @@ 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 */ > + > + /* need to free this so there's no TCG temporary leak */ > + tcg_temp_free_i32(tmp); > + tcg_temp_free_i32(tmp2); > + goto illegal_op; > + } > if (op) > gen_swap_half(tmp2); > gen_smul_dual(tmp, tmp2); > @@ -9656,6 +9718,13 @@ 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 */ > + /* need to free this so there's no TCG temporary leak */ > + tcg_temp_free_i32(tmp); > + tcg_temp_free_i32(tmp2); > + goto illegal_op; > + } > if (op) > tcg_gen_sari_i32(tmp2, tmp2, 16); > else > @@ -9673,6 +9742,15 @@ 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 > + */ > + /* need to free this so there's no TCG temporary leak */ > + 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); > @@ -9719,6 +9797,13 @@ 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 */ > + /* need to free this so there's no TCG temporary leak */ > + 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); > @@ -9742,6 +9827,17 @@ 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 > + */ > + /* need to free this so there's no TCG > + * temporary leak > + */ > + 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(); > @@ -9754,6 +9850,12 @@ 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 */ > + /* need to free this so there's no TCG temporary leak */ > + tcg_temp_free_i64(tmp64); > + goto illegal_op; > + } > gen_addq_lo(s, tmp64, rs); > gen_addq_lo(s, tmp64, rd); > } else if (op & 0x40) { > @@ -10018,14 +10120,34 @@ 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 */ > + /* need to free this so there's no TCG > + * temporary leak > + */ > + 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 */ > + /* need to free this so there's no TCG > + * temporary leak > + */ > + 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); > } > -- > 1.9.1 > >
>> 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) >> @@ -9559,6 +9598,12 @@ 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. */ >> + /* need to free this so there's no TCG temporary leak */ >> + 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)); >> @@ -9624,6 +9669,15 @@ 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 >> + */ >> + /* need to free this so there's no TCG temporary leak */ > > Comment un-needed. Why do you think it's not needed? Any comments are helpful as long as they're correct. Thanks
On Sat, May 30, 2015 at 3:08 PM, aurelio remonda <aurelioremonda@gmail.com> wrote: >>> 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) >>> @@ -9559,6 +9598,12 @@ 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. */ >>> + /* need to free this so there's no TCG temporary leak */ >>> + 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)); >>> @@ -9624,6 +9669,15 @@ 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 >>> + */ >>> + /* need to free this so there's no TCG temporary leak */ >> >> Comment un-needed. > > Why do you think it's not needed? Any comments are helpful as long as > they're correct. Thanks > Not always. Comments on code are a maintainance burden. It's preferrable to have the code self document as much as possible so that changes only need to be applied once - to the code itself. The only reason to ever call tcg_temp_free is to avoid a leak so its self explanatory IMO. Regards, Peter
> On 29 May 2015, at 22:16, Martin Galvan <martin.galvan@tallertechnologies.com> wrote: > > > ... I think Peter M is the maintainer for Target-ARM. > > Peter, is this OK to commit? was this patch accepted? if so, where can I get it from? regards, Liviu
On 10 June 2015 at 10:58, Liviu Ionescu <ilg@livius.net> wrote: > >> On 29 May 2015, at 22:16, Martin Galvan <martin.galvan@tallertechnologies.com> wrote: >> >> >> ... I think Peter M is the maintainer for Target-ARM. >> >> Peter, is this OK to commit? > > was this patch accepted? No; as Peter C suggested, Martin split it into two patches. Those patches then got code review, but I don't think Martin has sent out a revised series yet. -- PMM
El 10/6/2015 7:45, "Peter Maydell" <peter.maydell@linaro.org> escribió: > > On 10 June 2015 at 10:58, Liviu Ionescu <ilg@livius.net> wrote: > > > >> On 29 May 2015, at 22:16, Martin Galvan < martin.galvan@tallertechnologies.com> wrote: > >> > >> > >> ... I think Peter M is the maintainer for Target-ARM. > >> > >> Peter, is this OK to commit? > > > > was this patch accepted? > > No; as Peter C suggested, Martin split it into two patches. > Those patches then got code review, but I don't think Martin > has sent out a revised series yet. Aurelio is the primary author of the patch. He's busy these days but he will commit the new version soon, hopefully during the weekend. > > -- PMM
On 10 June 2015 at 11:49, Daniel Gutson <daniel.gutson@tallertechnologies.com> wrote: > El 10/6/2015 7:45, "Peter Maydell" <peter.maydell@linaro.org> escribió: >> No; as Peter C suggested, Martin split it into two patches. >> Those patches then got code review, but I don't think Martin >> has sent out a revised series yet. > > Aurelio is the primary author of the patch. My apologies -- overly hasty misreading of the names in this thread. -- PMM
diff --git a/target-arm/cpu.c b/target-arm/cpu.c index 3ca3fa8..9be4a06 100644 --- a/target-arm/cpu.c +++ b/target-arm/cpu.c @@ -512,6 +512,10 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp) if (arm_feature(env, ARM_FEATURE_CBAR_RO)) { set_feature(env, ARM_FEATURE_CBAR); } + if (arm_feature(env,ARM_FEATURE_THUMB2) && + !arm_feature(env,ARM_FEATURE_M)) { + set_feature(env, ARM_FEATURE_THUMB_DSP); + } if (cpu->reset_hivecs) { cpu->reset_sctlr |= (1 << 13); @@ -761,6 +765,22 @@ static void cortex_m3_initfn(Object *obj) set_feature(&cpu->env, ARM_FEATURE_M); cpu->midr = 0x410fc231; } +static void cortex_m4_initfn(Object *obj) +{ + ARMCPU *cpu = ARM_CPU(obj); + set_feature(&cpu->env, ARM_FEATURE_V7); + set_feature(&cpu->env, ARM_FEATURE_M); + set_feature(&cpu->env, ARM_FEATURE_THUMB_DSP); + cpu->midr = 0x410fc240; + /* Main id register CPUID bit assignments + * Bits NAME Function + * [31:24] IMPLEMENTER Indicates implementor: 0x41 = ARM + * [23:20] VARIANT Indicates processor revision: 0x0 = Revision 0 + * [19:16] (Constant) Reads as 0xF + * [15:4] PARTNO Indicates part number: 0xC24 = Cortex-M4 + * [3:0] REVISION Indicates patch release: 0x0 = Patch 0. + */ +} static void arm_v7m_class_init(ObjectClass *oc, void *data) { @@ -1164,6 +1184,8 @@ static const ARMCPUInfo arm_cpus[] = { { .name = "arm11mpcore", .initfn = arm11mpcore_initfn }, { .name = "cortex-m3", .initfn = cortex_m3_initfn, .class_init = arm_v7m_class_init }, + { .name = "cortex-m4", .initfn = cortex_m4_initfn, + .class_init = arm_v7m_class_init }, { .name = "cortex-a8", .initfn = cortex_a8_initfn }, { .name = "cortex-a9", .initfn = cortex_a9_initfn }, { .name = "cortex-a15", .initfn = cortex_a15_initfn }, diff --git a/target-arm/cpu.h b/target-arm/cpu.h index d4a5899..fa5c3bc 100644 --- a/target-arm/cpu.h +++ b/target-arm/cpu.h @@ -886,6 +886,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 9116529..c9d2e4f 100644 --- a/target-arm/translate.c +++ b/target-arm/translate.c @@ -9428,6 +9428,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); @@ -9502,13 +9506,37 @@ 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; } + /*-sxtab->-sxtab16->*/ 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 + */ + /* need to free this so there's no TCG temporary leak */ + tcg_temp_free_i32(tmp); + goto illegal_op; + } tmp2 = load_reg(s, rn); if ((op >> 1) == 1) { gen_add16(tmp, tmp2); @@ -9521,6 +9549,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; @@ -9532,8 +9566,13 @@ static int disas_thumb2_insn(CPUARMState *env, DisasContext *s, uint16_t insn_hw break; case 3: /* Other data processing. */ 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) @@ -9559,6 +9598,12 @@ 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. */ + /* need to free this so there's no TCG temporary leak */ + 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)); @@ -9624,6 +9669,15 @@ 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 + */ + /* need to free this so there's no TCG temporary leak */ + 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) { @@ -9634,6 +9688,14 @@ 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 */ + + /* need to free this so there's no TCG temporary leak */ + tcg_temp_free_i32(tmp); + tcg_temp_free_i32(tmp2); + goto illegal_op; + } if (op) gen_swap_half(tmp2); gen_smul_dual(tmp, tmp2); @@ -9656,6 +9718,13 @@ 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 */ + /* need to free this so there's no TCG temporary leak */ + tcg_temp_free_i32(tmp); + tcg_temp_free_i32(tmp2); + goto illegal_op; + } if (op) tcg_gen_sari_i32(tmp2, tmp2, 16); else @@ -9673,6 +9742,15 @@ 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 + */ + /* need to free this so there's no TCG temporary leak */ + 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); @@ -9719,6 +9797,13 @@ 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 */ + /* need to free this so there's no TCG temporary leak */ + 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); @@ -9742,6 +9827,17 @@ 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 + */ + /* need to free this so there's no TCG + * temporary leak + */ + 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(); @@ -9754,6 +9850,12 @@ 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 */ + /* need to free this so there's no TCG temporary leak */ + tcg_temp_free_i64(tmp64); + goto illegal_op; + } gen_addq_lo(s, tmp64, rs); gen_addq_lo(s, tmp64, rd); } else if (op & 0x40) { @@ -10018,14 +10120,34 @@ 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 */ + /* need to free this so there's no TCG + * temporary leak + */ + 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 */ + /* need to free this so there's no TCG + * temporary leak + */ + 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); }
This patch adds the Cortex-M4 CPU. The M4 is basically the same as the M3, the main differences being the DSP instructions and an optional FPU. The DSP instructions are already implemented in Qemu, as the A and R profiles use them. I created an ARM_FEATURE_THUMB_DSP to be added to any non-M thumb2-compatible CPU that uses DSP instructions, and I manually added it to the M4 in its initfn. The optional FPU in the M4 could be added in the future as a "Cortex-M4F" CPU. All we'd have to do is add the ARM_FEATURE_VFP4 to the initfn. 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.c | 22 +++++++++ target-arm/cpu.h | 1 + target-arm/translate.c | 130 +++++++++++++++++++++++++++++++++++++++++++++++-- 3 files changed, 149 insertions(+), 4 deletions(-)