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(-)
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 */
--
2.17.0
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 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
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
© 2016 - 2024 Red Hat, Inc.