On 10/22/24 01:21, Julian Ganz wrote:
Hi, Pierrick,
October 21, 2024 at 11:59 PM, "Pierrick Bouvier" wrote:
On 10/21/24 14:02, Julian Ganz wrote:
The motivation for this API is a plugin that simulates a RISC-V tracing
unit (and produces a trace). For that we actually also needed to
track the "regular" control flow, i.e. find out whether a branch was
taken or where a jump went. That wasn't hard, especially considering
that the TCG API already gives you (more or less) basic blocks. Still,
we ended up tracing every instruction because that made some of the logic
much simpler and easier to reason about.
We realized that we need a trap API because they:
* can occur at any time/point of execusion
* usually come with additional effects such as mode changes.
Thanks for sharing your insights.
I think there is definitely value in what you offer, and I'm trying to think
how we could extend it in the future easily, without having another callback
when a new event appear. In my experience on plugins, the least callbacks we
have, and the simpler they are, the better it is.
Maybe we could have a single API like:
enum qemu_plugin_cf_event_type {
QEMU_PLUGIN_CF_INTERRUPT;
QEMU_PLUGIN_CF_TRAP;
QEMU_PLUGIN_CF_SEMIHOSTING;
};
I have considered such an enum, as an input for the callback, as a
parameter of the registration function, and both. Of course, if you were
to add a selection parameter for the registration function, you likely
want OR-able flags.
An additional input for the callback type would obviously require a new
function type just for that callback. Since the callbacks are somewhat
similar to the VCPU init, exit, resume, ... ones it felt appropriate
to use the same function type for all of them.
I tend to disagree on that. IMHO, it's better to reduce number of API
entries instead of callback types.
It's easy for a user to understand how to implement a given callback,
while it's hard to understand which API you need for which thing.
For the syscall cbs, we already have a specific callback. So why not here?
I tend to see init/exit/resume events as different because you can't get
useful information attached, except the cpu id. But for control flow
related stuff, you can be interested in having more.
I understand you're focused on those "events" for now, but while digging
into this, it seems like the initial need was to track the control flow.
So I would like to push more in this direction, and offer a more
extendable solution. Do you think the end goal for a plugin using this
information may be different? (beyond a plugin counting
trap/interrupts/semihosting event).
As for the registration, it may make the registration a bit more
convenient and maybe keep the API clutter a bit lower, but not by that
much.
It's ok for the user. But I think it's more complicated to extend, when
we'll want to introduce control flow API in the future. Do we want 5 or
6 different callbacks when people want to track fully control flow from
a plugin?
/* Sum type, a.k.a. "Rust-like" enum */
typedef struct {
enum qemu_plugin_cf_event_type ev;
union {
data_for_interrupt interrupt;
data_for_trap trap;
data_for_semihosting semihosting;
} qemu_plugin_cf_event;
/* data_for_... could contain things like from/to addresses, interrupt id, ...
*/
I don't think this is a good idea.
Traps are just too diverse, imo there is too little overlap between
different architectures, with the sole exception maybe being the PC
prior to the trap. "Interrupt id" sounds like a reasonably common
concept, but then you would need to define a mapping for each and every
architecture. What integer type do you use? In RISC-V, for example,
exceptions and interrupt "ids" are differentiated via the most
significant bit. Dou keep that or do you zero it? And then there's
ring/privilage mode, cause (sometimes for each mode), ...
I didn't want to open the per architecture pandora box :).
I don't think the plugin API itself should deal with per architecture
details like meaning of a given id. I was just thinking to push this
"raw" information to the plugin, that may/may not use architecture
specific knowledge to do its work. We already have plugins that have
similar per architecture knowledge (contrib/plugins/howvec.c) and it's
ok in some specific cases.
But having something like from/to address seems useful to start. Even if
we don't provide it for all events yet, it's ok.
It would also complicate call sites for hooks in target code. You'd
either need awkwardly long function signitures or setup code for that
struct. Both are things you don't want, as a hook call site should
never distract from the actual logic surrounding them. You could
probably have something reasonable in Rust, using a builder/command
pattern. But in C this would require too much boiler plate code than
I'd be comfortable with.
We can have one "builder" function per data type, with fixed parameters
(no varargs), it's reasonable and would scale well with new entries/data
information.
Regards,
Julian
