* [Qemu-devel] [PATCH] target/arm: crash on conditional instr in it block
@ 2018-08-14 16:54 Roman Kapl
2018-08-14 18:12 ` Peter Maydell
0 siblings, 1 reply; 5+ messages in thread
From: Roman Kapl @ 2018-08-14 16:54 UTC (permalink / raw)
Cc: Roman Kapl, Peter Maydell, qemu-arm, qemu-devel
If an instruction is conditional (like CBZ) and it is executed conditionally
(using the ITx instruction), a jump to undefined label is generated.
Fix the 'skip on condtion' code to create a new label only if it does not
already exist. Previously multiple labels were created, but only the last one of
them was set.
Signed-off-by: Roman Kapl <rka@sysgo.com>
---
target/arm/translate.c | 32 ++++++++++++++++++--------------
1 file changed, 18 insertions(+), 14 deletions(-)
diff --git a/target/arm/translate.c b/target/arm/translate.c
index f845da7c63..f7c03a36e6 100644
--- a/target/arm/translate.c
+++ b/target/arm/translate.c
@@ -8480,6 +8480,16 @@ static void gen_srs(DisasContext *s,
s->base.is_jmp = DISAS_UPDATE;
}
+/* Skip this instruction if the condition is true */
+static void arm_conditional_skip(DisasContext *s, uint32_t cond)
+{
+ if (!s->condjmp) {
+ s->condlabel = gen_new_label();
+ s->condjmp = 1;
+ }
+ arm_gen_test_cc(cond, s->condlabel);
+}
+
static void disas_arm_insn(DisasContext *s, unsigned int insn)
{
unsigned int cond, val, op1, i, shift, rm, rs, rn, rd, sh;
@@ -8709,9 +8719,7 @@ static void disas_arm_insn(DisasContext *s, unsigned int insn)
if (cond != 0xe) {
/* if not always execute, we generate a conditional jump to
next instruction */
- s->condlabel = gen_new_label();
- arm_gen_test_cc(cond ^ 1, s->condlabel);
- s->condjmp = 1;
+ arm_conditional_skip(s, cond ^ 1);
}
if ((insn & 0x0f900000) == 0x03000000) {
if ((insn & (1 << 21)) == 0) {
@@ -11205,9 +11213,7 @@ static void disas_thumb2_insn(DisasContext *s, uint32_t insn)
/* Conditional branch. */
op = (insn >> 22) & 0xf;
/* Generate a conditional jump to next instruction. */
- s->condlabel = gen_new_label();
- arm_gen_test_cc(op ^ 1, s->condlabel);
- s->condjmp = 1;
+ arm_conditional_skip(s, op ^ 1);
/* offset[11:1] = insn[10:0] */
offset = (insn & 0x7ff) << 1;
@@ -12131,8 +12137,10 @@ static void disas_thumb_insn(DisasContext *s, uint32_t insn)
case 1: case 3: case 9: case 11: /* czb */
rm = insn & 7;
tmp = load_reg(s, rm);
- s->condlabel = gen_new_label();
- s->condjmp = 1;
+ if (!s->condjmp) {
+ s->condlabel = gen_new_label();
+ s->condjmp = 1;
+ }
if (insn & (1 << 11))
tcg_gen_brcondi_i32(TCG_COND_EQ, tmp, 0, s->condlabel);
else
@@ -12295,9 +12303,7 @@ static void disas_thumb_insn(DisasContext *s, uint32_t insn)
break;
}
/* generate a conditional jump to next instruction */
- s->condlabel = gen_new_label();
- arm_gen_test_cc(cond ^ 1, s->condlabel);
- s->condjmp = 1;
+ arm_conditional_skip(s, cond ^ 1);
/* jump to the offset */
val = (uint32_t)s->pc + 2;
@@ -12676,9 +12682,7 @@ static void thumb_tr_translate_insn(DisasContextBase *dcbase, CPUState *cpu)
uint32_t cond = dc->condexec_cond;
if (cond != 0x0e) { /* Skip conditional when condition is AL. */
- dc->condlabel = gen_new_label();
- arm_gen_test_cc(cond ^ 1, dc->condlabel);
- dc->condjmp = 1;
+ arm_conditional_skip(dc, cond ^ 1);
}
}
--
2.11.0
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH] target/arm: crash on conditional instr in it block
2018-08-14 16:54 [Qemu-devel] [PATCH] target/arm: crash on conditional instr in it block Roman Kapl
@ 2018-08-14 18:12 ` Peter Maydell
2018-08-15 8:30 ` Roman Kapl
0 siblings, 1 reply; 5+ messages in thread
From: Peter Maydell @ 2018-08-14 18:12 UTC (permalink / raw)
To: Roman Kapl; +Cc: qemu-arm, QEMU Developers
On 14 August 2018 at 17:54, Roman Kapl <rka@sysgo.com> wrote:
> If an instruction is conditional (like CBZ) and it is executed conditionally
> (using the ITx instruction), a jump to undefined label is generated.
>
> Fix the 'skip on condtion' code to create a new label only if it does not
> already exist. Previously multiple labels were created, but only the last one of
> them was set.
Hi; thanks for the bug report and the patch.
This case (CBZ inside an IT block) is architecturally UNPREDICTABLE,
but we certainly shouldn't crash QEMU.
For ARMv7 we have very wide latitude in what we can do; for ARMv8
the "CONSTRAINED UNPREDICTABLE" part of the Arm ARM limits us a
bit (see section K1.1.7). We can do one of:
* generate an UNDEFINED exception
* execute the instruction as if the condition code check had passed
* execute the instruction as if the condition check failed (ie NOP)
(and we're allowed to behave differently from execution to execution
so "honour the condition code check" is ok too I think).
If we don't UNDEF then we have to be a little careful about the
IT state, but I think we're OK there.
So we could if we like choose to just UNDEF in these cases,
something like:
if (dc->condexec_mask && thumb_insn_unpredictable_in_it(dc, insn) {
gen_exception_insn(...)
}
where we currently check for "always unconditional even in
an IT block" insns.
But I guess that just bringing conditional branches into line
with what we already do for other insns in this category is
a reasonable approach. I have a comment below but otherwise
the patch looks good.
> target/arm/translate.c | 32 ++++++++++++++++++--------------
> 1 file changed, 18 insertions(+), 14 deletions(-)
>
> diff --git a/target/arm/translate.c b/target/arm/translate.c
> index f845da7c63..f7c03a36e6 100644
> --- a/target/arm/translate.c
> +++ b/target/arm/translate.c
> @@ -8480,6 +8480,16 @@ static void gen_srs(DisasContext *s,
> s->base.is_jmp = DISAS_UPDATE;
> }
>
> +/* Skip this instruction if the condition is true */
> +static void arm_conditional_skip(DisasContext *s, uint32_t cond)
> +{
> + if (!s->condjmp) {
> + s->condlabel = gen_new_label();
> + s->condjmp = 1;
> + }
> + arm_gen_test_cc(cond, s->condlabel);
> +}
This is a nice piece of refactoring. I think it would make
more sense to have it do "skip if condition is false", though:
currently every callsite has to xor the condition with 1.
(You could call it arm_skip_insn_unless(), maybe.)
> +
> static void disas_arm_insn(DisasContext *s, unsigned int insn)
> {
> unsigned int cond, val, op1, i, shift, rm, rs, rn, rd, sh;
> @@ -8709,9 +8719,7 @@ static void disas_arm_insn(DisasContext *s, unsigned int insn)
> if (cond != 0xe) {
> /* if not always execute, we generate a conditional jump to
> next instruction */
> - s->condlabel = gen_new_label();
> - arm_gen_test_cc(cond ^ 1, s->condlabel);
> - s->condjmp = 1;
> + arm_conditional_skip(s, cond ^ 1);
> }
> if ((insn & 0x0f900000) == 0x03000000) {
> if ((insn & (1 << 21)) == 0) {
> @@ -11205,9 +11213,7 @@ static void disas_thumb2_insn(DisasContext *s, uint32_t insn)
> /* Conditional branch. */
> op = (insn >> 22) & 0xf;
> /* Generate a conditional jump to next instruction. */
> - s->condlabel = gen_new_label();
> - arm_gen_test_cc(op ^ 1, s->condlabel);
> - s->condjmp = 1;
> + arm_conditional_skip(s, op ^ 1);
>
> /* offset[11:1] = insn[10:0] */
> offset = (insn & 0x7ff) << 1;
> @@ -12131,8 +12137,10 @@ static void disas_thumb_insn(DisasContext *s, uint32_t insn)
> case 1: case 3: case 9: case 11: /* czb */
> rm = insn & 7;
> tmp = load_reg(s, rm);
> - s->condlabel = gen_new_label();
> - s->condjmp = 1;
> + if (!s->condjmp) {
> + s->condlabel = gen_new_label();
> + s->condjmp = 1;
> + }
> if (insn & (1 << 11))
> tcg_gen_brcondi_i32(TCG_COND_EQ, tmp, 0, s->condlabel);
> else
> @@ -12295,9 +12303,7 @@ static void disas_thumb_insn(DisasContext *s, uint32_t insn)
> break;
> }
> /* generate a conditional jump to next instruction */
> - s->condlabel = gen_new_label();
> - arm_gen_test_cc(cond ^ 1, s->condlabel);
> - s->condjmp = 1;
> + arm_conditional_skip(s, cond ^ 1);
>
> /* jump to the offset */
> val = (uint32_t)s->pc + 2;
> @@ -12676,9 +12682,7 @@ static void thumb_tr_translate_insn(DisasContextBase *dcbase, CPUState *cpu)
> uint32_t cond = dc->condexec_cond;
>
> if (cond != 0x0e) { /* Skip conditional when condition is AL. */
> - dc->condlabel = gen_new_label();
> - arm_gen_test_cc(cond ^ 1, dc->condlabel);
> - dc->condjmp = 1;
> + arm_conditional_skip(dc, cond ^ 1);
> }
> }
>
> --
thanks
-- PMM
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH] target/arm: crash on conditional instr in it block
2018-08-14 18:12 ` Peter Maydell
@ 2018-08-15 8:30 ` Roman Kapl
2018-08-15 10:57 ` Peter Maydell
0 siblings, 1 reply; 5+ messages in thread
From: Roman Kapl @ 2018-08-15 8:30 UTC (permalink / raw)
To: Peter Maydell, Roman Kapl; +Cc: qemu-arm, QEMU Developers
Hi and thanks for review,
On 08/14/2018 08:12 PM, Peter Maydell wrote:
> On 14 August 2018 at 17:54, Roman Kapl <rka@sysgo.com> wrote:
>> If an instruction is conditional (like CBZ) and it is executed conditionally
>> (using the ITx instruction), a jump to undefined label is generated.
>>
>> Fix the 'skip on condtion' code to create a new label only if it does not
>> already exist. Previously multiple labels were created, but only the last one of
>> them was set.
>
> Hi; thanks for the bug report and the patch.
>
> This case (CBZ inside an IT block) is architecturally UNPREDICTABLE,
> but we certainly shouldn't crash QEMU.
Hm... I am not able to find that claim in my ARMv7 reference manual (but
I am no ARM expert). However, I know that the assembler won't let you
generate it. ARMv8 seems to deprecate most IT uses anyway.
>
> For ARMv7 we have very wide latitude in what we can do; for ARMv8
> the "CONSTRAINED UNPREDICTABLE" part of the Arm ARM limits us a
> bit (see section K1.1.7). We can do one of:
> * generate an UNDEFINED exception
> * execute the instruction as if the condition code check had passed
> * execute the instruction as if the condition check failed (ie NOP)
> (and we're allowed to behave differently from execution to execution
> so "honour the condition code check" is ok too I think).
>
> If we don't UNDEF then we have to be a little careful about the
> IT state, but I think we're OK there.
>
> So we could if we like choose to just UNDEF in these cases,
> something like:
> if (dc->condexec_mask && thumb_insn_unpredictable_in_it(dc, insn) {
> gen_exception_insn(...)
> }
> where we currently check for "always unconditional even in
> an IT block" insns.
>
> But I guess that just bringing conditional branches into line
> with what we already do for other insns in this category is
> a reasonable approach. I have a comment below but otherwise
> the patch looks good.
>
>> target/arm/translate.c | 32 ++++++++++++++++++--------------
>> 1 file changed, 18 insertions(+), 14 deletions(-)
>>
>> diff --git a/target/arm/translate.c b/target/arm/translate.c
>> index f845da7c63..f7c03a36e6 100644
>> --- a/target/arm/translate.c
>> +++ b/target/arm/translate.c
>> @@ -8480,6 +8480,16 @@ static void gen_srs(DisasContext *s,
>> s->base.is_jmp = DISAS_UPDATE;
>> }
>>
>> +/* Skip this instruction if the condition is true */
>> +static void arm_conditional_skip(DisasContext *s, uint32_t cond)
>> +{
>> + if (!s->condjmp) {
>> + s->condlabel = gen_new_label();
>> + s->condjmp = 1;
>> + }
>> + arm_gen_test_cc(cond, s->condlabel);
>> +}
>
> This is a nice piece of refactoring. I think it would make
> more sense to have it do "skip if condition is false", though:
> currently every callsite has to xor the condition with 1.
>
> (You could call it arm_skip_insn_unless(), maybe.)
Well, personally I am not a big fan of unless, double negation etc.
(they require an extra mental effort for me). But if you prefer, I will
send a second patch with this change.
>
>> +
>> static void disas_arm_insn(DisasContext *s, unsigned int insn)
>> {
>> unsigned int cond, val, op1, i, shift, rm, rs, rn, rd, sh;
>> @@ -8709,9 +8719,7 @@ static void disas_arm_insn(DisasContext *s, unsigned int insn)
>> if (cond != 0xe) {
>> /* if not always execute, we generate a conditional jump to
>> next instruction */
>> - s->condlabel = gen_new_label();
>> - arm_gen_test_cc(cond ^ 1, s->condlabel);
>> - s->condjmp = 1;
>> + arm_conditional_skip(s, cond ^ 1);
>> }
>> if ((insn & 0x0f900000) == 0x03000000) {
>> if ((insn & (1 << 21)) == 0) {
>> @@ -11205,9 +11213,7 @@ static void disas_thumb2_insn(DisasContext *s, uint32_t insn)
>> /* Conditional branch. */
>> op = (insn >> 22) & 0xf;
>> /* Generate a conditional jump to next instruction. */
>> - s->condlabel = gen_new_label();
>> - arm_gen_test_cc(op ^ 1, s->condlabel);
>> - s->condjmp = 1;
>> + arm_conditional_skip(s, op ^ 1);
>>
>> /* offset[11:1] = insn[10:0] */
>> offset = (insn & 0x7ff) << 1;
>> @@ -12131,8 +12137,10 @@ static void disas_thumb_insn(DisasContext *s, uint32_t insn)
>> case 1: case 3: case 9: case 11: /* czb */
>> rm = insn & 7;
>> tmp = load_reg(s, rm);
>> - s->condlabel = gen_new_label();
>> - s->condjmp = 1;
>> + if (!s->condjmp) {
>> + s->condlabel = gen_new_label();
>> + s->condjmp = 1;
>> + }
>> if (insn & (1 << 11))
>> tcg_gen_brcondi_i32(TCG_COND_EQ, tmp, 0, s->condlabel);
>> else
>> @@ -12295,9 +12303,7 @@ static void disas_thumb_insn(DisasContext *s, uint32_t insn)
>> break;
>> }
>> /* generate a conditional jump to next instruction */
>> - s->condlabel = gen_new_label();
>> - arm_gen_test_cc(cond ^ 1, s->condlabel);
>> - s->condjmp = 1;
>> + arm_conditional_skip(s, cond ^ 1);
>>
>> /* jump to the offset */
>> val = (uint32_t)s->pc + 2;
>> @@ -12676,9 +12682,7 @@ static void thumb_tr_translate_insn(DisasContextBase *dcbase, CPUState *cpu)
>> uint32_t cond = dc->condexec_cond;
>>
>> if (cond != 0x0e) { /* Skip conditional when condition is AL. */
>> - dc->condlabel = gen_new_label();
>> - arm_gen_test_cc(cond ^ 1, dc->condlabel);
>> - dc->condjmp = 1;
>> + arm_conditional_skip(dc, cond ^ 1);
>> }
>> }
>>
>> --
>
> thanks
> -- PMM
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH] target/arm: crash on conditional instr in it block
2018-08-15 8:30 ` Roman Kapl
@ 2018-08-15 10:57 ` Peter Maydell
0 siblings, 0 replies; 5+ messages in thread
From: Peter Maydell @ 2018-08-15 10:57 UTC (permalink / raw)
To: Roman Kapl; +Cc: Roman Kapl, qemu-arm, QEMU Developers
On 15 August 2018 at 09:30, Roman Kapl <roman.kapl@sysgo.com> wrote:
> Hi and thanks for review,
>
> On 08/14/2018 08:12 PM, Peter Maydell wrote:
>>
>> On 14 August 2018 at 17:54, Roman Kapl <rka@sysgo.com> wrote:
>>>
>>> If an instruction is conditional (like CBZ) and it is executed
>>> conditionally
>>> (using the ITx instruction), a jump to undefined label is generated.
>>>
>>> Fix the 'skip on condtion' code to create a new label only if it does not
>>> already exist. Previously multiple labels were created, but only the last
>>> one of
>>> them was set.
>>
>>
>> Hi; thanks for the bug report and the patch.
>>
>> This case (CBZ inside an IT block) is architecturally UNPREDICTABLE,
>> but we certainly shouldn't crash QEMU.
>
>
> Hm... I am not able to find that claim in my ARMv7 reference manual (but I
> am no ARM expert).
v7A Arm ARM DDI0406C.b, section A8.8.29 "CBNZ, CBZ":
pseudocode says "if InITBlock() then UNPREDICTABLE;".
thanks
-- PMM
^ permalink raw reply [flat|nested] 5+ messages in thread
* [Qemu-devel] [PATCH] target/arm: crash on conditional instr in it block
@ 2018-08-14 16:19 Roman Kapl
0 siblings, 0 replies; 5+ messages in thread
From: Roman Kapl @ 2018-08-14 16:19 UTC (permalink / raw)
Cc: Roman Kapl, Peter Maydell, qemu-arm, qemu-devel
If an instruction is conditional (like CBZ) and it is executed conditionally
(using the ITx instruction), a jump to undefined label is generated.
Fix the 'skip on condtion' code to create a new label only if it does not
already exist. Previously multiple labels were created, but only the last one of
them was set.
Signed-off-by: Roman Kapl <rka@sysgo.com>
---
target/arm/translate.c | 32 ++++++++++++++++++--------------
1 file changed, 18 insertions(+), 14 deletions(-)
diff --git a/target/arm/translate.c b/target/arm/translate.c
index f845da7c63..f7c03a36e6 100644
--- a/target/arm/translate.c
+++ b/target/arm/translate.c
@@ -8480,6 +8480,16 @@ static void gen_srs(DisasContext *s,
s->base.is_jmp = DISAS_UPDATE;
}
+/* Skip this instruction if the condition is true */
+static void arm_conditional_skip(DisasContext *s, uint32_t cond)
+{
+ if (!s->condjmp) {
+ s->condlabel = gen_new_label();
+ s->condjmp = 1;
+ }
+ arm_gen_test_cc(cond, s->condlabel);
+}
+
static void disas_arm_insn(DisasContext *s, unsigned int insn)
{
unsigned int cond, val, op1, i, shift, rm, rs, rn, rd, sh;
@@ -8709,9 +8719,7 @@ static void disas_arm_insn(DisasContext *s, unsigned int insn)
if (cond != 0xe) {
/* if not always execute, we generate a conditional jump to
next instruction */
- s->condlabel = gen_new_label();
- arm_gen_test_cc(cond ^ 1, s->condlabel);
- s->condjmp = 1;
+ arm_conditional_skip(s, cond ^ 1);
}
if ((insn & 0x0f900000) == 0x03000000) {
if ((insn & (1 << 21)) == 0) {
@@ -11205,9 +11213,7 @@ static void disas_thumb2_insn(DisasContext *s, uint32_t insn)
/* Conditional branch. */
op = (insn >> 22) & 0xf;
/* Generate a conditional jump to next instruction. */
- s->condlabel = gen_new_label();
- arm_gen_test_cc(op ^ 1, s->condlabel);
- s->condjmp = 1;
+ arm_conditional_skip(s, op ^ 1);
/* offset[11:1] = insn[10:0] */
offset = (insn & 0x7ff) << 1;
@@ -12131,8 +12137,10 @@ static void disas_thumb_insn(DisasContext *s, uint32_t insn)
case 1: case 3: case 9: case 11: /* czb */
rm = insn & 7;
tmp = load_reg(s, rm);
- s->condlabel = gen_new_label();
- s->condjmp = 1;
+ if (!s->condjmp) {
+ s->condlabel = gen_new_label();
+ s->condjmp = 1;
+ }
if (insn & (1 << 11))
tcg_gen_brcondi_i32(TCG_COND_EQ, tmp, 0, s->condlabel);
else
@@ -12295,9 +12303,7 @@ static void disas_thumb_insn(DisasContext *s, uint32_t insn)
break;
}
/* generate a conditional jump to next instruction */
- s->condlabel = gen_new_label();
- arm_gen_test_cc(cond ^ 1, s->condlabel);
- s->condjmp = 1;
+ arm_conditional_skip(s, cond ^ 1);
/* jump to the offset */
val = (uint32_t)s->pc + 2;
@@ -12676,9 +12682,7 @@ static void thumb_tr_translate_insn(DisasContextBase *dcbase, CPUState *cpu)
uint32_t cond = dc->condexec_cond;
if (cond != 0x0e) { /* Skip conditional when condition is AL. */
- dc->condlabel = gen_new_label();
- arm_gen_test_cc(cond ^ 1, dc->condlabel);
- dc->condjmp = 1;
+ arm_conditional_skip(dc, cond ^ 1);
}
}
--
2.11.0
^ permalink raw reply related [flat|nested] 5+ messages in thread
end of thread, other threads:[~2018-08-15 10:57 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-08-14 16:54 [Qemu-devel] [PATCH] target/arm: crash on conditional instr in it block Roman Kapl
2018-08-14 18:12 ` Peter Maydell
2018-08-15 8:30 ` Roman Kapl
2018-08-15 10:57 ` Peter Maydell
-- strict thread matches above, loose matches on Subject: below --
2018-08-14 16:19 Roman Kapl
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.