Re: [RFC PATCH v2 0/7] tcg-plugins: add hooks for interrupts, exceptions and traps

[Pierrick Bouvier <pierrick.bouvier@linaro.org>]

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