We already attempted to set and clear can_do_io before the first
and last insns, but only used the initial value of max_insns and
the call to translator_io_start to find those insns.
Now that we track insn_start in DisasContextBase, and now that
we have emit_before_op, we can wait until we have finished
translation to identify the true first and last insns and emit
the sets of can_do_io at that time.
This fixes case of a translation block which crossed a page boundary,
and for which the second page turned out to be mmio. In this case we
truncate the block, and the previous logic for can_do_io could leave
a block with a single insn with can_do_io set to false, which would
fail an assertion in cpu_io_recompile.
Reported-by: Jørgen Hansen <Jorgen.Hansen@wdc.com>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
include/exec/translator.h | 1 -
accel/tcg/translator.c | 45 ++++++++++++++++++++-------------------
2 files changed, 23 insertions(+), 23 deletions(-)
diff --git a/include/exec/translator.h b/include/exec/translator.h
index ceaeca8c91..2c4fb818e7 100644
--- a/include/exec/translator.h
+++ b/include/exec/translator.h
@@ -87,7 +87,6 @@ typedef struct DisasContextBase {
int num_insns;
int max_insns;
bool singlestep_enabled;
- int8_t saved_can_do_io;
bool plugin_enabled;
struct TCGOp *insn_start;
void *host_addr[2];
diff --git a/accel/tcg/translator.c b/accel/tcg/translator.c
index ae61c154c2..9de0bc34c8 100644
--- a/accel/tcg/translator.c
+++ b/accel/tcg/translator.c
@@ -18,20 +18,14 @@
static void set_can_do_io(DisasContextBase *db, bool val)
{
- if (db->saved_can_do_io != val) {
- db->saved_can_do_io = val;
-
- QEMU_BUILD_BUG_ON(sizeof_field(CPUState, neg.can_do_io) != 1);
- tcg_gen_st8_i32(tcg_constant_i32(val), tcg_env,
- offsetof(ArchCPU, parent_obj.neg.can_do_io) -
- offsetof(ArchCPU, env));
- }
+ QEMU_BUILD_BUG_ON(sizeof_field(CPUState, neg.can_do_io) != 1);
+ tcg_gen_st8_i32(tcg_constant_i32(val), tcg_env,
+ offsetof(ArchCPU, parent_obj.neg.can_do_io) -
+ offsetof(ArchCPU, env));
}
bool translator_io_start(DisasContextBase *db)
{
- set_can_do_io(db, true);
-
/*
* Ensure that this instruction will be the last in the TB.
* The target may override this to something more forceful.
@@ -84,13 +78,6 @@ static TCGOp *gen_tb_start(DisasContextBase *db, uint32_t cflags)
- offsetof(ArchCPU, env));
}
- /*
- * cpu->neg.can_do_io is set automatically here at the beginning of
- * each translation block. The cost is minimal, plus it would be
- * very easy to forget doing it in the translator.
- */
- set_can_do_io(db, db->max_insns == 1);
-
return icount_start_insn;
}
@@ -129,6 +116,7 @@ void translator_loop(CPUState *cpu, TranslationBlock *tb, int *max_insns,
{
uint32_t cflags = tb_cflags(tb);
TCGOp *icount_start_insn;
+ TCGOp *first_insn_start = NULL;
bool plugin_enabled;
/* Initialize DisasContext */
@@ -139,7 +127,6 @@ void translator_loop(CPUState *cpu, TranslationBlock *tb, int *max_insns,
db->num_insns = 0;
db->max_insns = *max_insns;
db->singlestep_enabled = cflags & CF_SINGLE_STEP;
- db->saved_can_do_io = -1;
db->insn_start = NULL;
db->host_addr[0] = host_pc;
db->host_addr[1] = NULL;
@@ -159,6 +146,9 @@ void translator_loop(CPUState *cpu, TranslationBlock *tb, int *max_insns,
*max_insns = ++db->num_insns;
ops->insn_start(db, cpu);
db->insn_start = tcg_last_op();
+ if (first_insn_start == NULL) {
+ first_insn_start = db->insn_start;
+ }
tcg_debug_assert(db->is_jmp == DISAS_NEXT); /* no early exit */
if (plugin_enabled) {
@@ -171,10 +161,6 @@ void translator_loop(CPUState *cpu, TranslationBlock *tb, int *max_insns,
* done next -- either exiting this loop or locate the start of
* the next instruction.
*/
- if (db->num_insns == db->max_insns) {
- /* Accept I/O on the last instruction. */
- set_can_do_io(db, true);
- }
ops->translate_insn(db, cpu);
/*
@@ -207,6 +193,21 @@ void translator_loop(CPUState *cpu, TranslationBlock *tb, int *max_insns,
ops->tb_stop(db, cpu);
gen_tb_end(tb, cflags, icount_start_insn, db->num_insns);
+ /*
+ * Manage can_do_io for the translation block: set to false before
+ * the first insn and set to true before the last insn.
+ */
+ if (db->num_insns == 1) {
+ tcg_debug_assert(first_insn_start == db->insn_start);
+ } else {
+ tcg_debug_assert(first_insn_start != db->insn_start);
+ tcg_ctx->emit_before_op = first_insn_start;
+ set_can_do_io(db, false);
+ }
+ tcg_ctx->emit_before_op = db->insn_start;
+ set_can_do_io(db, true);
+ tcg_ctx->emit_before_op = NULL;
+
if (plugin_enabled) {
plugin_gen_tb_end(cpu, db->num_insns);
}
--
2.34.1
On Sat, Apr 06, 2024 at 12:32:48PM -1000, Richard Henderson wrote:
> We already attempted to set and clear can_do_io before the first
> and last insns, but only used the initial value of max_insns and
> the call to translator_io_start to find those insns.
>
> Now that we track insn_start in DisasContextBase, and now that
> we have emit_before_op, we can wait until we have finished
> translation to identify the true first and last insns and emit
> the sets of can_do_io at that time.
>
> This fixes case of a translation block which crossed a page boundary,
> and for which the second page turned out to be mmio.
I love when I get to say this: I knew it! :D
https://lore.kernel.org/qemu-devel/ZbvVB4J+AHkLNuE2@memverge.com/
Great fix, much appreciate the effort!
Reviewed-by: Gregory Price <gregory.price@memverge.com>
> In this case we
> truncate the block, and the previous logic for can_do_io could leave
> a block with a single insn with can_do_io set to false, which would
> fail an assertion in cpu_io_recompile.
>
> Reported-by: Jørgen Hansen <Jorgen.Hansen@wdc.com>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
> include/exec/translator.h | 1 -
> accel/tcg/translator.c | 45 ++++++++++++++++++++-------------------
> 2 files changed, 23 insertions(+), 23 deletions(-)
>
> diff --git a/include/exec/translator.h b/include/exec/translator.h
> index ceaeca8c91..2c4fb818e7 100644
> --- a/include/exec/translator.h
> +++ b/include/exec/translator.h
> @@ -87,7 +87,6 @@ typedef struct DisasContextBase {
> int num_insns;
> int max_insns;
> bool singlestep_enabled;
> - int8_t saved_can_do_io;
> bool plugin_enabled;
> struct TCGOp *insn_start;
> void *host_addr[2];
> diff --git a/accel/tcg/translator.c b/accel/tcg/translator.c
> index ae61c154c2..9de0bc34c8 100644
> --- a/accel/tcg/translator.c
> +++ b/accel/tcg/translator.c
> @@ -18,20 +18,14 @@
>
> static void set_can_do_io(DisasContextBase *db, bool val)
> {
> - if (db->saved_can_do_io != val) {
> - db->saved_can_do_io = val;
> -
> - QEMU_BUILD_BUG_ON(sizeof_field(CPUState, neg.can_do_io) != 1);
> - tcg_gen_st8_i32(tcg_constant_i32(val), tcg_env,
> - offsetof(ArchCPU, parent_obj.neg.can_do_io) -
> - offsetof(ArchCPU, env));
> - }
> + QEMU_BUILD_BUG_ON(sizeof_field(CPUState, neg.can_do_io) != 1);
> + tcg_gen_st8_i32(tcg_constant_i32(val), tcg_env,
> + offsetof(ArchCPU, parent_obj.neg.can_do_io) -
> + offsetof(ArchCPU, env));
> }
>
> bool translator_io_start(DisasContextBase *db)
> {
> - set_can_do_io(db, true);
> -
> /*
> * Ensure that this instruction will be the last in the TB.
> * The target may override this to something more forceful.
> @@ -84,13 +78,6 @@ static TCGOp *gen_tb_start(DisasContextBase *db, uint32_t cflags)
> - offsetof(ArchCPU, env));
> }
>
> - /*
> - * cpu->neg.can_do_io is set automatically here at the beginning of
> - * each translation block. The cost is minimal, plus it would be
> - * very easy to forget doing it in the translator.
> - */
> - set_can_do_io(db, db->max_insns == 1);
> -
> return icount_start_insn;
> }
>
> @@ -129,6 +116,7 @@ void translator_loop(CPUState *cpu, TranslationBlock *tb, int *max_insns,
> {
> uint32_t cflags = tb_cflags(tb);
> TCGOp *icount_start_insn;
> + TCGOp *first_insn_start = NULL;
> bool plugin_enabled;
>
> /* Initialize DisasContext */
> @@ -139,7 +127,6 @@ void translator_loop(CPUState *cpu, TranslationBlock *tb, int *max_insns,
> db->num_insns = 0;
> db->max_insns = *max_insns;
> db->singlestep_enabled = cflags & CF_SINGLE_STEP;
> - db->saved_can_do_io = -1;
> db->insn_start = NULL;
> db->host_addr[0] = host_pc;
> db->host_addr[1] = NULL;
> @@ -159,6 +146,9 @@ void translator_loop(CPUState *cpu, TranslationBlock *tb, int *max_insns,
> *max_insns = ++db->num_insns;
> ops->insn_start(db, cpu);
> db->insn_start = tcg_last_op();
> + if (first_insn_start == NULL) {
> + first_insn_start = db->insn_start;
> + }
> tcg_debug_assert(db->is_jmp == DISAS_NEXT); /* no early exit */
>
> if (plugin_enabled) {
> @@ -171,10 +161,6 @@ void translator_loop(CPUState *cpu, TranslationBlock *tb, int *max_insns,
> * done next -- either exiting this loop or locate the start of
> * the next instruction.
> */
> - if (db->num_insns == db->max_insns) {
> - /* Accept I/O on the last instruction. */
> - set_can_do_io(db, true);
> - }
> ops->translate_insn(db, cpu);
>
> /*
> @@ -207,6 +193,21 @@ void translator_loop(CPUState *cpu, TranslationBlock *tb, int *max_insns,
> ops->tb_stop(db, cpu);
> gen_tb_end(tb, cflags, icount_start_insn, db->num_insns);
>
> + /*
> + * Manage can_do_io for the translation block: set to false before
> + * the first insn and set to true before the last insn.
> + */
> + if (db->num_insns == 1) {
> + tcg_debug_assert(first_insn_start == db->insn_start);
> + } else {
> + tcg_debug_assert(first_insn_start != db->insn_start);
> + tcg_ctx->emit_before_op = first_insn_start;
> + set_can_do_io(db, false);
> + }
> + tcg_ctx->emit_before_op = db->insn_start;
> + set_can_do_io(db, true);
> + tcg_ctx->emit_before_op = NULL;
> +
> if (plugin_enabled) {
> plugin_gen_tb_end(cpu, db->num_insns);
> }
> --
> 2.34.1
>
>
On 4/7/24 00:32, Richard Henderson wrote: > We already attempted to set and clear can_do_io before the first > and last insns, but only used the initial value of max_insns and > the call to translator_io_start to find those insns. > > Now that we track insn_start in DisasContextBase, and now that > we have emit_before_op, we can wait until we have finished > translation to identify the true first and last insns and emit > the sets of can_do_io at that time. > > This fixes case of a translation block which crossed a page boundary, > and for which the second page turned out to be mmio. In this case we > truncate the block, and the previous logic for can_do_io could leave > a block with a single insn with can_do_io set to false, which would > fail an assertion in cpu_io_recompile. > > Reported-by: Jørgen Hansen <Jorgen.Hansen@wdc.com> > Signed-off-by: Richard Henderson <richard.henderson@linaro.org> > --- > include/exec/translator.h | 1 - > accel/tcg/translator.c | 45 ++++++++++++++++++++------------------- > 2 files changed, 23 insertions(+), 23 deletions(-) Thanks for the quick fix! I verified the patch series fixes the issue on my setup, and also verified that no issues were seen with full MMIO backing for the otherwise same test case. Tested-by: Jørgen Hansen <Jorgen.Hansen@wdc.com>
On 7/4/24 00:32, Richard Henderson wrote: > We already attempted to set and clear can_do_io before the first > and last insns, but only used the initial value of max_insns and > the call to translator_io_start to find those insns. > > Now that we track insn_start in DisasContextBase, and now that > we have emit_before_op, we can wait until we have finished > translation to identify the true first and last insns and emit > the sets of can_do_io at that time. > > This fixes case of a translation block which crossed a page boundary, > and for which the second page turned out to be mmio. In this case we > truncate the block, and the previous logic for can_do_io could leave > a block with a single insn with can_do_io set to false, which would > fail an assertion in cpu_io_recompile. > > Reported-by: Jørgen Hansen <Jorgen.Hansen@wdc.com> > Signed-off-by: Richard Henderson <richard.henderson@linaro.org> > --- > include/exec/translator.h | 1 - > accel/tcg/translator.c | 45 ++++++++++++++++++++------------------- > 2 files changed, 23 insertions(+), 23 deletions(-) Nice! Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
© 2016 - 2026 Red Hat, Inc.