qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH] accel/tcg: Call tcg_tb_insert() for one-insn TBs


From: Philippe Mathieu-Daudé
Subject: Re: [PATCH] accel/tcg: Call tcg_tb_insert() for one-insn TBs
Date: Thu, 16 Jan 2025 16:17:24 +0100
User-agent: Mozilla Thunderbird

On 16/1/25 16:09, Alex Bennée wrote:
Peter Maydell <peter.maydell@linaro.org> writes:

On Thu, 16 Jan 2025 at 11:48, Ilya Leoshkevich <iii@linux.ibm.com> wrote:

On Thu, 2025-01-16 at 11:06 +0000, Peter Maydell wrote:
The original reported problem here seems to me like it's a
problem with whatever target's frontend code this is.
This is a single instruction TB, so either:
  * the generated code for it completes the insn without
    raising an exception (no problem)
  * the generated code for it should raise an exception
    without having modified the CPU state (so there would
    be nothing to do for restore_state_to_opc)

It sounds like the target is generating code which does
something like:
  * do part of the instruction, including updating some of
    the CPU state
  * then decide it needs to raise an exception, and rely on
    the restore_state_to_opc handling to undo the state updates
    it did previously

The assumption of the "throwaway single insn TB" is that
you don't do that (i.e. that restore_state_to_opc is only
there for the benefit of multi-insn TBs).

The problem is not a partial state update in an instruction, but rather
that on some targets restore_state_to_opc is more than just a
"restore" - it is also "prepare for handling an exception", i.e.:

- arm: exception.syndrome
- hppa: unwind_breg, psw_n
- mips: btarget
- openrisc: ppc
- riscv: excp_uw2
- s390x: int_pgm_ilen

Should we move that to another TCGCPUOps handler?

Some of these may be wrong due to unfamiliarity with the respective
architectures, sorry - but this illustrates the idea.

Ah, yes, thanks for the clear explanation. The "throw away
the TB" design didn't consider that (or vice-versa).

We can certainly do with better docstrings for tcg_tb_lookup (via the
region tree) and tb_lookup (using cache and/or QHT) to make it clear the
difference between the two. I don't think we should ever use
tcg_tb_lookup for the purposes of executing a TB, just for resolution.

We have a few spare CF_ flags so maybe we could have a CF_RUNONCE flag
which is set for these TBs and assert its not set in tb_lookup along
with the current CF_INVALID flag. We could possibly set CF_INVALID
before executing the TB as we don't check the tb state from
tb_gen_code() before executing it but I guess that might be a little too
magic.

Rich, WDYT?


thanks
-- PMM





reply via email to

[Prev in Thread] Current Thread [Next in Thread]