On Thu, 1 Feb 2024 at 15:17, Alex Bennée <alex.ben...@linaro.org> wrote:
>
> Peter Maydell <peter.mayd...@linaro.org> writes:
> > So, that looks like:
> > * we call cpu_tb_exec(), which executes some generated code
> > * that generated code calls the lookup_tb_ptr helper to see
> > if we have a generated TB already for the address we're going
> > to execute next
> > * lookup_tb_ptr probes the TLB to see if we know the host RAM
> > address for the guest address
> > * this results in a TLB walk for an instruction fetch
> > * the page table descriptor load is to IO memory
> > * io_prepare assumes it needs to do a TLB recompile, because
> > can_do_io is clear
> >
> > I am not surprised that the corner case of "the guest put its
> > page tables in an MMIO device" has not yet come up :-)
> >
> > I'm really not sure how the icount handling should interact
> > with that...
>
> Its not just icount - we need to handle it for all modes now. That said
> seeing as we are at the end of a block shouldn't can_do_io be set?
The lookup_tb_ptr helper gets called from tcg_gen_goto_tb(),
which happens earlier than the tb_stop callback (it can
happen in the trans function for branch etc insns, for
example).
I think it should be OK to clear can_do_io at the start
of the lookup_tb_ptr helper, something like:
diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c
index 977576ca143..7818537f318 100644
--- a/accel/tcg/cpu-exec.c
+++ b/accel/tcg/cpu-exec.c
@@ -396,6 +396,15 @@ const void *HELPER(lookup_tb_ptr)(CPUArchState *env)
uint64_t cs_base;
uint32_t flags, cflags;
+ /*
+ * By definition we've just finished a TB, so I/O is OK.
+ * Avoid the possibility of calling cpu_io_recompile() if
+ * a page table walk triggered by tb_lookup() calling
+ * probe_access_internal() happens to touch an MMIO device.
+ * The next TB, if we chain to it, will clear the flag again.
+ */
+ cpu->neg.can_do_io = true;
+
cpu_get_tb_cpu_state(env, &pc, &cs_base, &flags);
cflags = curr_cflags(cpu);
-- PMM
