Re: [PATCH] tcg-plugins: add a hook for interrupts, exceptions and traps

[Julian Ganz <neither@nut.email>]

Hi all,

thank's for the feedback!

Maybe for broader context: at work a colleague and I are developing a
plugin that implements RISC-V specific tracing [1]. It writes a trace
to a file in the format you'd expect from actual hardware implementing
the relevant interface. Currently, it's developed on top of Xilinx'
qemu-fork with yet another bunch of un-upstreamable patches on top,
some of which introduce some API we use in the plugin. Meaning that if
I open-source the plugin one day, I'll probably need to rewrite parts
of it anyway.


On Mon, 23 Oct 2023 14:08:51 +0100
Alex Bennée <alex.bennee@linaro.org> wrote:
> 
> Julian Ganz <neither@nut.email> writes:
> 
> > Some analysis greatly benefits, or depends on, information about
> > interrupts. For example, we may need to handle the execution of a
> > new translation block differently if it is not the result of normal
> > program flow but of an interrupt.
> 
> I can see the benefit for plugins knowing the context - for QEMU
> itself there is no real difference in how it handles blocks that are
> part of an interrupt.

Yes, that I'm aware of.

> > Even with the existing interfaces, it is more or less possible to
> > discern these situations using some heuristice. For example, the PC
> > landing in a trap vector is a strong indicator that a trap, i.e. an
> > interrupt or event occured. However, such heuristics require
> > knowledge about the architecture and may be prone to errors.
> 
> Does this requirement go away if you can query the current execution
> state via registers?

So my colleague an I were discussing this again today: knowing the PC,
EPC, ecause (or their equivalent) and the priviledge/mode are not
sufficient since there are too many cases where detecting an exception
(or an interrupt) is non-trivial or impossible. For example, there's no
gurantee that ecause will ever be reset between interrupts and an
exception may occur without a PC discontinuity. Probably the worst case
would be an illegal instruction at the trap handler. But we also found
a few other cases where even if we know excactly which instructions
belonged to an ISR, we could still not tell how we ended up there with
sufficient certainty.

But I'm aware of [2] if that's what you were after, and we do need the
ability to query some registers for our plugin in addition to an
interrupt callback or similar API.


> > --- a/accel/tcg/cpu-exec.c
> > +++ b/accel/tcg/cpu-exec.c
> > @@ -754,6 +754,8 @@ static inline bool
> > cpu_handle_exception(CPUState *cpu, int *ret)
> > qemu_mutex_unlock_iothread(); cpu->exception_index = -1;
> >  
> > +            qemu_plugin_vcpu_interrupt_cb(cpu);
> > +
> >              if (unlikely(cpu->singlestep_enabled)) {
> >                  /*
> >                   * After processing the exception, ensure an
> > EXCP_DEBUG is @@ -866,6 +868,7 @@ static inline bool
> > cpu_handle_interrupt(CPUState *cpu, if
> > (need_replay_interrupt(interrupt_request)) { replay_interrupt();
> >                  }
> > +                qemu_plugin_vcpu_interrupt_cb(cpu);
> 
> My worry here is:
> 
>  a) we are conflating QEMU exceptions and interrupts as the same thing
>  b) this is leaking internal implementation details of the translator
> 
> For example EXCP_SEMIHOST isn't actually an interrupt (we don't change
> processor state or control flow). It's just the internal signalling we
> use to handle our semihosting implementation.

Yes. Funnily enough, that's more or less what we want anyway for our
plugin (because we need to handle `ebreak`s for example). However, in
this specific use-case we need to half-disassemble some instructions
anyway so we would at least have no problem finding and special-casing
`ebreaks`.

But this is indeed likely not what you want for the general case. In
some cases you may still be able to detect the absence of a PC
discontinuity and then just assume that you can ignore an interrupt,
but that would involve logic you normally wouldn't have (e.g. a
callback on individual instructions) and require some knowledge about
the architecture. Which is exctly not what you want for the TCG plugin
API.

> Similarly the CPU_INTERRUPT_EXITTB interrupt isn't really changing
> state, just ensuring we exit the run loop so house keeping is done.

Mh... that one we've not observed yet. Likely because our test-cases
(running OpenSBI with very simple payloads) were too simple.

> The "correct" way for ARM for example would be to register a helper
> function with arm_register_el_change_hook() and trigger the callbacks
> that way. However each architecture does its own IRQ modelling so you
> would need to work out where in the plumbing to do each callback.

I tried to avoid that. But it looks like I don't have a choice :/


> > --- a/include/qemu/qemu-plugin.h
> > +++ b/include/qemu/qemu-plugin.h
> > @@ -206,6 +206,17 @@ void
> > qemu_plugin_register_vcpu_idle_cb(qemu_plugin_id_t id, void
> > qemu_plugin_register_vcpu_resume_cb(qemu_plugin_id_t id,
> > qemu_plugin_vcpu_simple_cb_t cb); 
> > +/**
> > + * qemu_plugin_register_vcpu_interrupt_cb() - register a vCPU
> > interrupt callback
> > + * @id: plugin ID
> > + * @cb: callback function
> > + *
> > + * The @cb function is called every time a vCPU receives an
> > interrupt, exception
> > + * or trap.
> 
> As discussed above you would trigger for a lot more than that. You
> would also miss a lot of the other interesting transitions which
> don't need an asynchronous signal. For example
> HELPER(exception_return) happily changes control flow but doesn't
> need to use the exception mechanism to do it.

Huh. I wasn't aware of that.


> I'm not opposed to adding such a API hook to plugins but I don't think
> the current approach does what you want. If we do add a new API it is
> customary to either expand an existing plugin or add a new one to
> demonstrate the use of the API.

After receiving your feedback I agree that I was maybe a bit too eager.
I'll return to the drawing board and prepare a new series with a simple
demonstrator (maybe a simple interrupt-counter and/or plugin meassuring
the "time" spent in an inteerupt). But since I'm not too intimate with
qemu internals, this will likely take some time.

Kind regards,
Julian

[1] https://github.com/riscv-non-isa/riscv-trace-spec
[2] https://gitlab.com/qemu-project/qemu/-/issues/1706