* [PATCH 1/3] target/avr: Call avr_cpu_do_interrupt directly
2022-08-26 20:55 [PATCH 0/3] target/avr: Fix skips vs interrupts Richard Henderson
@ 2022-08-26 20:55 ` Richard Henderson
2022-08-27 19:14 ` Michael Rolnik
2022-08-26 20:55 ` [PATCH 2/3] target/avr: Only execute one interrupt at a time Richard Henderson
` (3 subsequent siblings)
4 siblings, 1 reply; 9+ messages in thread
From: Richard Henderson @ 2022-08-26 20:55 UTC (permalink / raw)
To: qemu-devel; +Cc: mrolnik
There is no need to go through cc->tcg_ops when
we know what value that must have.
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
target/avr/helper.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/target/avr/helper.c b/target/avr/helper.c
index 82284f8997..9614ccf3e4 100644
--- a/target/avr/helper.c
+++ b/target/avr/helper.c
@@ -29,14 +29,13 @@
bool avr_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
{
bool ret = false;
- CPUClass *cc = CPU_GET_CLASS(cs);
AVRCPU *cpu = AVR_CPU(cs);
CPUAVRState *env = &cpu->env;
if (interrupt_request & CPU_INTERRUPT_RESET) {
if (cpu_interrupts_enabled(env)) {
cs->exception_index = EXCP_RESET;
- cc->tcg_ops->do_interrupt(cs);
+ avr_cpu_do_interrupt(cs);
cs->interrupt_request &= ~CPU_INTERRUPT_RESET;
@@ -47,7 +46,7 @@ bool avr_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
if (cpu_interrupts_enabled(env) && env->intsrc != 0) {
int index = ctz32(env->intsrc);
cs->exception_index = EXCP_INT(index);
- cc->tcg_ops->do_interrupt(cs);
+ avr_cpu_do_interrupt(cs);
env->intsrc &= env->intsrc - 1; /* clear the interrupt */
if (!env->intsrc) {
--
2.34.1
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH 1/3] target/avr: Call avr_cpu_do_interrupt directly
2022-08-26 20:55 ` [PATCH 1/3] target/avr: Call avr_cpu_do_interrupt directly Richard Henderson
@ 2022-08-27 19:14 ` Michael Rolnik
0 siblings, 0 replies; 9+ messages in thread
From: Michael Rolnik @ 2022-08-27 19:14 UTC (permalink / raw)
To: Richard Henderson; +Cc: qemu-devel
[-- Attachment #1: Type: text/plain, Size: 1577 bytes --]
Reviewed-by: Michael Rolnik <mrolnik@gmail.com>
On Fri, Aug 26, 2022 at 11:55 PM Richard Henderson <
richard.henderson@linaro.org> wrote:
> There is no need to go through cc->tcg_ops when
> we know what value that must have.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
> target/avr/helper.c | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/target/avr/helper.c b/target/avr/helper.c
> index 82284f8997..9614ccf3e4 100644
> --- a/target/avr/helper.c
> +++ b/target/avr/helper.c
> @@ -29,14 +29,13 @@
> bool avr_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
> {
> bool ret = false;
> - CPUClass *cc = CPU_GET_CLASS(cs);
> AVRCPU *cpu = AVR_CPU(cs);
> CPUAVRState *env = &cpu->env;
>
> if (interrupt_request & CPU_INTERRUPT_RESET) {
> if (cpu_interrupts_enabled(env)) {
> cs->exception_index = EXCP_RESET;
> - cc->tcg_ops->do_interrupt(cs);
> + avr_cpu_do_interrupt(cs);
>
> cs->interrupt_request &= ~CPU_INTERRUPT_RESET;
>
> @@ -47,7 +46,7 @@ bool avr_cpu_exec_interrupt(CPUState *cs, int
> interrupt_request)
> if (cpu_interrupts_enabled(env) && env->intsrc != 0) {
> int index = ctz32(env->intsrc);
> cs->exception_index = EXCP_INT(index);
> - cc->tcg_ops->do_interrupt(cs);
> + avr_cpu_do_interrupt(cs);
>
> env->intsrc &= env->intsrc - 1; /* clear the interrupt */
> if (!env->intsrc) {
> --
> 2.34.1
>
>
--
Best Regards,
Michael Rolnik
[-- Attachment #2: Type: text/html, Size: 2349 bytes --]
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 2/3] target/avr: Only execute one interrupt at a time
2022-08-26 20:55 [PATCH 0/3] target/avr: Fix skips vs interrupts Richard Henderson
2022-08-26 20:55 ` [PATCH 1/3] target/avr: Call avr_cpu_do_interrupt directly Richard Henderson
@ 2022-08-26 20:55 ` Richard Henderson
2022-08-27 19:13 ` Michael Rolnik
2022-08-26 20:55 ` [PATCH 3/3] target/avr: Disable interrupts when env->skip set Richard Henderson
` (2 subsequent siblings)
4 siblings, 1 reply; 9+ messages in thread
From: Richard Henderson @ 2022-08-26 20:55 UTC (permalink / raw)
To: qemu-devel; +Cc: mrolnik
We cannot deliver two interrupts simultaneously;
the first interrupt handler must execute first.
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
target/avr/helper.c | 9 +++------
1 file changed, 3 insertions(+), 6 deletions(-)
diff --git a/target/avr/helper.c b/target/avr/helper.c
index 9614ccf3e4..34f1cbffb2 100644
--- a/target/avr/helper.c
+++ b/target/avr/helper.c
@@ -28,7 +28,6 @@
bool avr_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
{
- bool ret = false;
AVRCPU *cpu = AVR_CPU(cs);
CPUAVRState *env = &cpu->env;
@@ -38,8 +37,7 @@ bool avr_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
avr_cpu_do_interrupt(cs);
cs->interrupt_request &= ~CPU_INTERRUPT_RESET;
-
- ret = true;
+ return true;
}
}
if (interrupt_request & CPU_INTERRUPT_HARD) {
@@ -52,11 +50,10 @@ bool avr_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
if (!env->intsrc) {
cs->interrupt_request &= ~CPU_INTERRUPT_HARD;
}
-
- ret = true;
+ return true;
}
}
- return ret;
+ return false;
}
void avr_cpu_do_interrupt(CPUState *cs)
--
2.34.1
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH 2/3] target/avr: Only execute one interrupt at a time
2022-08-26 20:55 ` [PATCH 2/3] target/avr: Only execute one interrupt at a time Richard Henderson
@ 2022-08-27 19:13 ` Michael Rolnik
0 siblings, 0 replies; 9+ messages in thread
From: Michael Rolnik @ 2022-08-27 19:13 UTC (permalink / raw)
To: Richard Henderson; +Cc: qemu-devel
[-- Attachment #1: Type: text/plain, Size: 1504 bytes --]
Reviewed-by: Michael Rolnik <mrolnik@gmail.com>
On Fri, Aug 26, 2022 at 11:55 PM Richard Henderson <
richard.henderson@linaro.org> wrote:
> We cannot deliver two interrupts simultaneously;
> the first interrupt handler must execute first.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
> target/avr/helper.c | 9 +++------
> 1 file changed, 3 insertions(+), 6 deletions(-)
>
> diff --git a/target/avr/helper.c b/target/avr/helper.c
> index 9614ccf3e4..34f1cbffb2 100644
> --- a/target/avr/helper.c
> +++ b/target/avr/helper.c
> @@ -28,7 +28,6 @@
>
> bool avr_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
> {
> - bool ret = false;
> AVRCPU *cpu = AVR_CPU(cs);
> CPUAVRState *env = &cpu->env;
>
> @@ -38,8 +37,7 @@ bool avr_cpu_exec_interrupt(CPUState *cs, int
> interrupt_request)
> avr_cpu_do_interrupt(cs);
>
> cs->interrupt_request &= ~CPU_INTERRUPT_RESET;
> -
> - ret = true;
> + return true;
> }
> }
> if (interrupt_request & CPU_INTERRUPT_HARD) {
> @@ -52,11 +50,10 @@ bool avr_cpu_exec_interrupt(CPUState *cs, int
> interrupt_request)
> if (!env->intsrc) {
> cs->interrupt_request &= ~CPU_INTERRUPT_HARD;
> }
> -
> - ret = true;
> + return true;
> }
> }
> - return ret;
> + return false;
> }
>
> void avr_cpu_do_interrupt(CPUState *cs)
> --
> 2.34.1
>
>
--
Best Regards,
Michael Rolnik
[-- Attachment #2: Type: text/html, Size: 2257 bytes --]
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 3/3] target/avr: Disable interrupts when env->skip set
2022-08-26 20:55 [PATCH 0/3] target/avr: Fix skips vs interrupts Richard Henderson
2022-08-26 20:55 ` [PATCH 1/3] target/avr: Call avr_cpu_do_interrupt directly Richard Henderson
2022-08-26 20:55 ` [PATCH 2/3] target/avr: Only execute one interrupt at a time Richard Henderson
@ 2022-08-26 20:55 ` Richard Henderson
2022-08-27 19:15 ` Michael Rolnik
2022-08-27 19:15 ` [PATCH 0/3] target/avr: Fix skips vs interrupts Michael Rolnik
2022-08-30 11:49 ` Philippe Mathieu-Daudé via
4 siblings, 1 reply; 9+ messages in thread
From: Richard Henderson @ 2022-08-26 20:55 UTC (permalink / raw)
To: qemu-devel; +Cc: mrolnik
This bit is not saved across interrupts, so we must
delay delivering the interrupt until the skip has
been processed.
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1118
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
target/avr/helper.c | 9 +++++++++
target/avr/translate.c | 26 ++++++++++++++++++++++----
2 files changed, 31 insertions(+), 4 deletions(-)
diff --git a/target/avr/helper.c b/target/avr/helper.c
index 34f1cbffb2..156dde4e92 100644
--- a/target/avr/helper.c
+++ b/target/avr/helper.c
@@ -31,6 +31,15 @@ bool avr_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
AVRCPU *cpu = AVR_CPU(cs);
CPUAVRState *env = &cpu->env;
+ /*
+ * We cannot separate a skip from the next instruction,
+ * as the skip would not be preserved across the interrupt.
+ * Separating the two insn normally only happens at page boundaries.
+ */
+ if (env->skip) {
+ return false;
+ }
+
if (interrupt_request & CPU_INTERRUPT_RESET) {
if (cpu_interrupts_enabled(env)) {
cs->exception_index = EXCP_RESET;
diff --git a/target/avr/translate.c b/target/avr/translate.c
index dc9c3d6bcc..026753c963 100644
--- a/target/avr/translate.c
+++ b/target/avr/translate.c
@@ -2971,8 +2971,18 @@ static void avr_tr_translate_insn(DisasContextBase *dcbase, CPUState *cs)
if (skip_label) {
canonicalize_skip(ctx);
gen_set_label(skip_label);
- if (ctx->base.is_jmp == DISAS_NORETURN) {
+
+ switch (ctx->base.is_jmp) {
+ case DISAS_NORETURN:
ctx->base.is_jmp = DISAS_CHAIN;
+ break;
+ case DISAS_NEXT:
+ if (ctx->base.tb->flags & TB_FLAGS_SKIP) {
+ ctx->base.is_jmp = DISAS_TOO_MANY;
+ }
+ break;
+ default:
+ break;
}
}
@@ -2989,6 +2999,11 @@ static void avr_tr_tb_stop(DisasContextBase *dcbase, CPUState *cs)
{
DisasContext *ctx = container_of(dcbase, DisasContext, base);
bool nonconst_skip = canonicalize_skip(ctx);
+ /*
+ * Because we disable interrupts while env->skip is set,
+ * we must return to the main loop to re-evaluate afterward.
+ */
+ bool force_exit = ctx->base.tb->flags & TB_FLAGS_SKIP;
switch (ctx->base.is_jmp) {
case DISAS_NORETURN:
@@ -2997,7 +3012,7 @@ static void avr_tr_tb_stop(DisasContextBase *dcbase, CPUState *cs)
case DISAS_NEXT:
case DISAS_TOO_MANY:
case DISAS_CHAIN:
- if (!nonconst_skip) {
+ if (!nonconst_skip && !force_exit) {
/* Note gen_goto_tb checks singlestep. */
gen_goto_tb(ctx, 1, ctx->npc);
break;
@@ -3005,8 +3020,11 @@ static void avr_tr_tb_stop(DisasContextBase *dcbase, CPUState *cs)
tcg_gen_movi_tl(cpu_pc, ctx->npc);
/* fall through */
case DISAS_LOOKUP:
- tcg_gen_lookup_and_goto_ptr();
- break;
+ if (!force_exit) {
+ tcg_gen_lookup_and_goto_ptr();
+ break;
+ }
+ /* fall through */
case DISAS_EXIT:
tcg_gen_exit_tb(NULL, 0);
break;
--
2.34.1
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH 3/3] target/avr: Disable interrupts when env->skip set
2022-08-26 20:55 ` [PATCH 3/3] target/avr: Disable interrupts when env->skip set Richard Henderson
@ 2022-08-27 19:15 ` Michael Rolnik
0 siblings, 0 replies; 9+ messages in thread
From: Michael Rolnik @ 2022-08-27 19:15 UTC (permalink / raw)
To: Richard Henderson; +Cc: qemu-devel
[-- Attachment #1: Type: text/plain, Size: 3514 bytes --]
Reviewed-by: Michael Rolnik <mrolnik@gmail.com>
On Fri, Aug 26, 2022 at 11:55 PM Richard Henderson <
richard.henderson@linaro.org> wrote:
> This bit is not saved across interrupts, so we must
> delay delivering the interrupt until the skip has
> been processed.
>
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1118
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
> target/avr/helper.c | 9 +++++++++
> target/avr/translate.c | 26 ++++++++++++++++++++++----
> 2 files changed, 31 insertions(+), 4 deletions(-)
>
> diff --git a/target/avr/helper.c b/target/avr/helper.c
> index 34f1cbffb2..156dde4e92 100644
> --- a/target/avr/helper.c
> +++ b/target/avr/helper.c
> @@ -31,6 +31,15 @@ bool avr_cpu_exec_interrupt(CPUState *cs, int
> interrupt_request)
> AVRCPU *cpu = AVR_CPU(cs);
> CPUAVRState *env = &cpu->env;
>
> + /*
> + * We cannot separate a skip from the next instruction,
> + * as the skip would not be preserved across the interrupt.
> + * Separating the two insn normally only happens at page boundaries.
> + */
> + if (env->skip) {
> + return false;
> + }
> +
> if (interrupt_request & CPU_INTERRUPT_RESET) {
> if (cpu_interrupts_enabled(env)) {
> cs->exception_index = EXCP_RESET;
> diff --git a/target/avr/translate.c b/target/avr/translate.c
> index dc9c3d6bcc..026753c963 100644
> --- a/target/avr/translate.c
> +++ b/target/avr/translate.c
> @@ -2971,8 +2971,18 @@ static void avr_tr_translate_insn(DisasContextBase
> *dcbase, CPUState *cs)
> if (skip_label) {
> canonicalize_skip(ctx);
> gen_set_label(skip_label);
> - if (ctx->base.is_jmp == DISAS_NORETURN) {
> +
> + switch (ctx->base.is_jmp) {
> + case DISAS_NORETURN:
> ctx->base.is_jmp = DISAS_CHAIN;
> + break;
> + case DISAS_NEXT:
> + if (ctx->base.tb->flags & TB_FLAGS_SKIP) {
> + ctx->base.is_jmp = DISAS_TOO_MANY;
> + }
> + break;
> + default:
> + break;
> }
> }
>
> @@ -2989,6 +2999,11 @@ static void avr_tr_tb_stop(DisasContextBase
> *dcbase, CPUState *cs)
> {
> DisasContext *ctx = container_of(dcbase, DisasContext, base);
> bool nonconst_skip = canonicalize_skip(ctx);
> + /*
> + * Because we disable interrupts while env->skip is set,
> + * we must return to the main loop to re-evaluate afterward.
> + */
> + bool force_exit = ctx->base.tb->flags & TB_FLAGS_SKIP;
>
> switch (ctx->base.is_jmp) {
> case DISAS_NORETURN:
> @@ -2997,7 +3012,7 @@ static void avr_tr_tb_stop(DisasContextBase *dcbase,
> CPUState *cs)
> case DISAS_NEXT:
> case DISAS_TOO_MANY:
> case DISAS_CHAIN:
> - if (!nonconst_skip) {
> + if (!nonconst_skip && !force_exit) {
> /* Note gen_goto_tb checks singlestep. */
> gen_goto_tb(ctx, 1, ctx->npc);
> break;
> @@ -3005,8 +3020,11 @@ static void avr_tr_tb_stop(DisasContextBase
> *dcbase, CPUState *cs)
> tcg_gen_movi_tl(cpu_pc, ctx->npc);
> /* fall through */
> case DISAS_LOOKUP:
> - tcg_gen_lookup_and_goto_ptr();
> - break;
> + if (!force_exit) {
> + tcg_gen_lookup_and_goto_ptr();
> + break;
> + }
> + /* fall through */
> case DISAS_EXIT:
> tcg_gen_exit_tb(NULL, 0);
> break;
> --
> 2.34.1
>
>
--
Best Regards,
Michael Rolnik
[-- Attachment #2: Type: text/html, Size: 4701 bytes --]
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 0/3] target/avr: Fix skips vs interrupts
2022-08-26 20:55 [PATCH 0/3] target/avr: Fix skips vs interrupts Richard Henderson
` (2 preceding siblings ...)
2022-08-26 20:55 ` [PATCH 3/3] target/avr: Disable interrupts when env->skip set Richard Henderson
@ 2022-08-27 19:15 ` Michael Rolnik
2022-08-30 11:49 ` Philippe Mathieu-Daudé via
4 siblings, 0 replies; 9+ messages in thread
From: Michael Rolnik @ 2022-08-27 19:15 UTC (permalink / raw)
To: Richard Henderson; +Cc: qemu-devel
[-- Attachment #1: Type: text/plain, Size: 614 bytes --]
Reviewed-by: Michael Rolnik <mrolnik@gmail.com>
On Fri, Aug 26, 2022 at 11:55 PM Richard Henderson <
richard.henderson@linaro.org> wrote:
> Fixes https://gitlab.com/qemu-project/qemu/-/issues/1118
>
> r~
>
> Richard Henderson (3):
> target/avr: Call avr_cpu_do_interrupt directly
> target/avr: Only execute one interrupt at a time
> target/avr: Disable interrupts when env->skip set
>
> target/avr/helper.c | 23 ++++++++++++++---------
> target/avr/translate.c | 26 ++++++++++++++++++++++----
> 2 files changed, 36 insertions(+), 13 deletions(-)
>
> --
> 2.34.1
>
>
--
Best Regards,
Michael Rolnik
[-- Attachment #2: Type: text/html, Size: 1191 bytes --]
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH 0/3] target/avr: Fix skips vs interrupts
2022-08-26 20:55 [PATCH 0/3] target/avr: Fix skips vs interrupts Richard Henderson
` (3 preceding siblings ...)
2022-08-27 19:15 ` [PATCH 0/3] target/avr: Fix skips vs interrupts Michael Rolnik
@ 2022-08-30 11:49 ` Philippe Mathieu-Daudé via
4 siblings, 0 replies; 9+ messages in thread
From: Philippe Mathieu-Daudé via @ 2022-08-30 11:49 UTC (permalink / raw)
To: Richard Henderson, qemu-devel; +Cc: mrolnik
On 26/8/22 22:55, Richard Henderson wrote:
> Fixes https://gitlab.com/qemu-project/qemu/-/issues/1118
Thanks!
>
> Richard Henderson (3):
> target/avr: Call avr_cpu_do_interrupt directly
> target/avr: Only execute one interrupt at a time
> target/avr: Disable interrupts when env->skip set
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
^ permalink raw reply [flat|nested] 9+ messages in thread