| Message ID | 20180612204632.28780-1-jusual@mail.ru (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Jun 12, 2018 at 11:46:32PM +0300, Julia Suvorova wrote: > ARMv6-M supports 6 Thumb2 instructions. This patch checks for these > instructions and allows their execution. > Like Thumb2 cores, ARMv6-M always interprets BL instruction as 32-bit. > > This patch is required for future Cortex-M0 support. > > Signed-off-by: Julia Suvorova <jusual@mail.ru> > --- > target/arm/translate.c | 35 ++++++++++++++++++++++++++++++----- > 1 file changed, 30 insertions(+), 5 deletions(-) Does make the undefined instruction test case that I recently posted pass (including all commented out instructions that weren't working yet)? Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
On 13.06.2018 17:05, Stefan Hajnoczi wrote: > On Tue, Jun 12, 2018 at 11:46:32PM +0300, Julia Suvorova wrote: >> ARMv6-M supports 6 Thumb2 instructions. This patch checks for these >> instructions and allows their execution. >> Like Thumb2 cores, ARMv6-M always interprets BL instruction as 32-bit. >> >> This patch is required for future Cortex-M0 support. >> >> Signed-off-by: Julia Suvorova <jusual@mail.ru> >> --- >> target/arm/translate.c | 35 ++++++++++++++++++++++++++++++----- >> 1 file changed, 30 insertions(+), 5 deletions(-) > > Does make the undefined instruction test case that I recently posted > pass (including all commented out instructions that weren't working > yet)? > > Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> > Yes, test with all instructions is passed. Best regards, Julia Suvorova.
On Wed, Jun 13, 2018 at 08:10:23PM +0300, Julia Suvorova via Qemu-devel wrote: > On 13.06.2018 17:05, Stefan Hajnoczi wrote: > > On Tue, Jun 12, 2018 at 11:46:32PM +0300, Julia Suvorova wrote: > > > ARMv6-M supports 6 Thumb2 instructions. This patch checks for these > > > instructions and allows their execution. > > > Like Thumb2 cores, ARMv6-M always interprets BL instruction as 32-bit. > > > > > > This patch is required for future Cortex-M0 support. > > > > > > Signed-off-by: Julia Suvorova <jusual@mail.ru> > > > --- > > > target/arm/translate.c | 35 ++++++++++++++++++++++++++++++----- > > > 1 file changed, 30 insertions(+), 5 deletions(-) > > > > Does make the undefined instruction test case that I recently posted > > pass (including all commented out instructions that weren't working > > yet)? > > > > Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> > > > > Yes, test with all instructions is passed. Great, I have sent a v2 of that patch. It now also includes the 6 valid 32-bit instructions to prove that your patch has enabled them. Stefan
On 12 June 2018 at 21:46, Julia Suvorova <jusual@mail.ru> wrote: > ARMv6-M supports 6 Thumb2 instructions. This patch checks for these > instructions and allows their execution. > Like Thumb2 cores, ARMv6-M always interprets BL instruction as 32-bit. > > This patch is required for future Cortex-M0 support. > > Signed-off-by: Julia Suvorova <jusual@mail.ru> > @@ -10075,6 +10076,11 @@ static void disas_thumb2_insn(DisasContext *s, uint32_t insn) > { > uint32_t imm, shift, offset; > uint32_t rd, rn, rm, rs; > + uint32_t armv6m_insn[] = {0xf3808000 /* msr */, 0xf3b08040 /* dsb */, > + 0xf3b08050 /* dmb */, 0xf3b08060 /* isb */, > + 0xf3e08000 /* mrs */, 0xf000d000 /* bl */}; > + uint32_t armv6m_mask[] = {0xffe0d000, 0xfff0d0f0, 0xfff0d0f0, > + 0xfff0d0f0, 0xffe0d000, 0xf800d000}; I think these arrays should be 'const'; we can also move them closer to their point of use, inside the scope of the if() below. Since those are trivial tweaks, I'm going to put this into target-arm.next with those changes made, if that's OK. thanks -- PMM
On 15.06.2018 13:55, Peter Maydell wrote: > On 12 June 2018 at 21:46, Julia Suvorova <jusual@mail.ru> wrote: >> ARMv6-M supports 6 Thumb2 instructions. This patch checks for these >> instructions and allows their execution. >> Like Thumb2 cores, ARMv6-M always interprets BL instruction as 32-bit. >> >> This patch is required for future Cortex-M0 support. >> >> Signed-off-by: Julia Suvorova <jusual@mail.ru> >> @@ -10075,6 +10076,11 @@ static void disas_thumb2_insn(DisasContext *s, uint32_t insn) >> { >> uint32_t imm, shift, offset; >> uint32_t rd, rn, rm, rs; >> + uint32_t armv6m_insn[] = {0xf3808000 /* msr */, 0xf3b08040 /* dsb */, >> + 0xf3b08050 /* dmb */, 0xf3b08060 /* isb */, >> + 0xf3e08000 /* mrs */, 0xf000d000 /* bl */}; >> + uint32_t armv6m_mask[] = {0xffe0d000, 0xfff0d0f0, 0xfff0d0f0, >> + 0xfff0d0f0, 0xffe0d000, 0xf800d000}; > > I think these arrays should be 'const'; we can also move them closer > to their point of use, inside the scope of the if() below. > > Since those are trivial tweaks, I'm going to put this into > target-arm.next with those changes made, if that's OK. Sure, thanks. Best regards, Julia Suvorova.
On 12 June 2018 at 21:46, Julia Suvorova <jusual@mail.ru> wrote: > @@ -10085,10 +10091,25 @@ static void disas_thumb2_insn(DisasContext *s, uint32_t insn) > int conds; > int logic_cc; > > - /* The only 32 bit insn that's allowed for Thumb1 is the combined > - * BL/BLX prefix and suffix. > + /* > + * ARMv6-M supports a limited subset of Thumb2 instructions. > + * Other Thumb1 architectures allow only 32-bit > + * combined BL/BLX prefix and suffix. > */ > - if ((insn & 0xf800e800) != 0xf000e800) { > + if (arm_dc_feature(s, ARM_FEATURE_M) && arm_dc_feature(s, ARM_FEATURE_V6)) { I realized during testing that this accidentally breaks v7M and v8M, because those cores define both ARM_FEATURE_V6 and _V7 (and _V8 for v8M), so this condition is true and we undef on the non-v6M insns for v7M and v8M too. I've fixed this in target-arm.next by changing the condition to + if (arm_dc_feature(s, ARM_FEATURE_M) && + !arm_dc_feature(s, ARM_FEATURE_V7)) { thanks -- PMM
On 06/15/2018 12:55 AM, Peter Maydell wrote: >> + uint32_t armv6m_insn[] = {0xf3808000 /* msr */, 0xf3b08040 /* dsb */, >> + 0xf3b08050 /* dmb */, 0xf3b08060 /* isb */, >> + 0xf3e08000 /* mrs */, 0xf000d000 /* bl */}; >> + uint32_t armv6m_mask[] = {0xffe0d000, 0xfff0d0f0, 0xfff0d0f0, >> + 0xfff0d0f0, 0xffe0d000, 0xf800d000}; > I think these arrays should be 'const'; we can also move them closer > to their point of use, inside the scope of the if() below. static as well. r~
On 17 June 2018 at 06:36, Richard Henderson <richard.henderson@linaro.org> wrote: > On 06/15/2018 12:55 AM, Peter Maydell wrote: >>> + uint32_t armv6m_insn[] = {0xf3808000 /* msr */, 0xf3b08040 /* dsb */, >>> + 0xf3b08050 /* dmb */, 0xf3b08060 /* isb */, >>> + 0xf3e08000 /* mrs */, 0xf000d000 /* bl */}; >>> + uint32_t armv6m_mask[] = {0xffe0d000, 0xfff0d0f0, 0xfff0d0f0, >>> + 0xfff0d0f0, 0xffe0d000, 0xf800d000}; >> I think these arrays should be 'const'; we can also move them closer >> to their point of use, inside the scope of the if() below. > > static as well. Mmm; commit is already in master though, will need a followup patch. thanks -- PMM
On 17.06.2018 19:33, Peter Maydell wrote: > On 17 June 2018 at 06:36, Richard Henderson > <richard.henderson@linaro.org> wrote: >> On 06/15/2018 12:55 AM, Peter Maydell wrote: >>>> + uint32_t armv6m_insn[] = {0xf3808000 /* msr */, 0xf3b08040 /* dsb */, >>>> + 0xf3b08050 /* dmb */, 0xf3b08060 /* isb */, >>>> + 0xf3e08000 /* mrs */, 0xf000d000 /* bl */}; >>>> + uint32_t armv6m_mask[] = {0xffe0d000, 0xfff0d0f0, 0xfff0d0f0, >>>> + 0xfff0d0f0, 0xffe0d000, 0xf800d000}; >>> I think these arrays should be 'const'; we can also move them closer >>> to their point of use, inside the scope of the if() below. >> >> static as well. > > Mmm; commit is already in master though, will need a followup patch. I can make it if you wish. In addition, we can simplify following "if" by removing ARM_FEATURE_V6 since V7M and V8M define V6: if (!arm_dc_feature(s, ARM_FEATURE_V7) && !(arm_dc_feature(s, ARM_FEATURE_V6) && arm_dc_feature(s, ARM_FEATURE_M))) { goto illegal_op; } Like this: if (!arm_dc_feature(s, ARM_FEATURE_V7) && !arm_dc_feature(s, ARM_FEATURE_M)) { goto illegal_op; } What do you think? Best regards, Julia Suvorova.
On 17 June 2018 at 19:48, Julia Suvorova <jusual@mail.ru> wrote: > I can make it if you wish. > In addition, we can simplify following "if" by removing ARM_FEATURE_V6 > since V7M and V8M define V6: > > if (!arm_dc_feature(s, ARM_FEATURE_V7) && > !(arm_dc_feature(s, ARM_FEATURE_V6) && > arm_dc_feature(s, ARM_FEATURE_M))) { > goto illegal_op; > } > > Like this: > > if (!arm_dc_feature(s, ARM_FEATURE_V7) && > !arm_dc_feature(s, ARM_FEATURE_M)) { > goto illegal_op; > } > > What do you think? Yes; that would be reasonable. I did consider making that change when I applied the patch, but decided I didn't really care much either way. So if you want to send a patch for it that's fine; if you don't, that's also fine. thanks -- PMM
diff --git a/target/arm/translate.c b/target/arm/translate.c index 0ff5edf2ce..8cae3f5ed0 100644 --- a/target/arm/translate.c +++ b/target/arm/translate.c @@ -9965,7 +9965,8 @@ static bool thumb_insn_is_16bit(DisasContext *s, uint32_t insn) * end up actually treating this as two 16-bit insns, though, * if it's half of a bl/blx pair that might span a page boundary. */ - if (arm_dc_feature(s, ARM_FEATURE_THUMB2)) { + if (arm_dc_feature(s, ARM_FEATURE_THUMB2) || + arm_dc_feature(s, ARM_FEATURE_M)) { /* Thumb2 cores (including all M profile ones) always treat * 32-bit insns as 32-bit. */ @@ -10075,6 +10076,11 @@ static void disas_thumb2_insn(DisasContext *s, uint32_t insn) { uint32_t imm, shift, offset; uint32_t rd, rn, rm, rs; + uint32_t armv6m_insn[] = {0xf3808000 /* msr */, 0xf3b08040 /* dsb */, + 0xf3b08050 /* dmb */, 0xf3b08060 /* isb */, + 0xf3e08000 /* mrs */, 0xf000d000 /* bl */}; + uint32_t armv6m_mask[] = {0xffe0d000, 0xfff0d0f0, 0xfff0d0f0, + 0xfff0d0f0, 0xffe0d000, 0xf800d000}; TCGv_i32 tmp; TCGv_i32 tmp2; TCGv_i32 tmp3; @@ -10085,10 +10091,25 @@ static void disas_thumb2_insn(DisasContext *s, uint32_t insn) int conds; int logic_cc; - /* The only 32 bit insn that's allowed for Thumb1 is the combined - * BL/BLX prefix and suffix. + /* + * ARMv6-M supports a limited subset of Thumb2 instructions. + * Other Thumb1 architectures allow only 32-bit + * combined BL/BLX prefix and suffix. */ - if ((insn & 0xf800e800) != 0xf000e800) { + if (arm_dc_feature(s, ARM_FEATURE_M) && arm_dc_feature(s, ARM_FEATURE_V6)) { + int i; + bool found = false; + + for (i = 0; i < ARRAY_SIZE(armv6m_insn); i++) { + if ((insn & armv6m_mask[i]) == armv6m_insn[i]) { + found = true; + break; + } + } + if (!found) { + goto illegal_op; + } + } else if ((insn & 0xf800e800) != 0xf000e800) { ARCH(6T2); } @@ -11009,7 +11030,11 @@ static void disas_thumb2_insn(DisasContext *s, uint32_t insn) } break; case 3: /* Special control operations. */ - ARCH(7); + if (!arm_dc_feature(s, ARM_FEATURE_V7) && + !(arm_dc_feature(s, ARM_FEATURE_V6) && + arm_dc_feature(s, ARM_FEATURE_M))) { + goto illegal_op; + } op = (insn >> 4) & 0xf; switch (op) { case 2: /* clrex */
ARMv6-M supports 6 Thumb2 instructions. This patch checks for these instructions and allows their execution. Like Thumb2 cores, ARMv6-M always interprets BL instruction as 32-bit. This patch is required for future Cortex-M0 support. Signed-off-by: Julia Suvorova <jusual@mail.ru> --- target/arm/translate.c | 35 ++++++++++++++++++++++++++++++----- 1 file changed, 30 insertions(+), 5 deletions(-)