Re: [RFC PATCH v3 01/11] plugins: add types for callbacks related to certain discontinuities

[Pierrick Bouvier]

On 12/5/24 04:40, Julian Ganz wrote:
> Hi Pierrick,
>
> December 4, 2024 at 11:41 PM, "Pierrick Bouvier" wrote:
> > On 12/3/24 00:45, Julian Ganz wrote:
> >
> > > Hi,
> > >   December 2, 2024 at 8:26 PM, "Julian Ganz" wrote:
> > >
> > > > diff --git a/include/qemu/qemu-plugin.h b/include/qemu/qemu-plugin.h
> > > >   index 0fba36ae02..9c67374b7e 100644
> > > >   --- a/include/qemu/qemu-plugin.h
> > > >   +++ b/include/qemu/qemu-plugin.h
> > > >   @@ -154,6 +154,49 @@ typedef void 
> > > > (*qemu_plugin_vcpu_simple_cb_t)(qemu_plugin_id_t id,
> > >   <snip>
> > >
> > > > +/**
> > > >   + * typedef qemu_plugin_vcpu_discon_cb_t - vcpu discontinuity callback
> > > >   + * @vcpu_index: the current vcpu context
> > > >   + * @type: the type of discontinuity
> > > >   + * @from_pc: the source of the discontinuity, e.g. the PC before the
> > > >   + * transition
> > > >   + * @to_pc: the PC pointing to the next instruction to be executed
> > > >   + *
> > > >   + * The excact semantics of @from_pc depends on @the type of discontinuity. 
> > > > For
> > > >   + * interrupts, @from_pc will point to the next instruction which would have
> > > >   + * been executed. For exceptions and host calls, @from_pc will point to the
> > > >   + * instruction that caused the exception or issued the host call. Note that
> > > >   + * in the case of exceptions, the instruction is not retired and thus not
> > > >   + * observable via general instruction exec callbacks. The same may be the 
> > > > case
> > > >   + * for some host calls such as hypervisor call "exceptions".
> > >   Some more notes about this bit: I originally tried to make the from_pc
> > >   semantics independent from the type of event, i.e. either of the two
> > >   cases. I obviously did not succeed in doing so. As, in most cases, the
> > >   instruction pointed to by from_pc is not observable via exec callbacks
> > >   I could also not test this behaviour in the testing plugin (see patch
> > >   11). I am therefore in favor for dropping the from_pc for the next
> > >   iteration of this patch series.
> > Does it mean that information returned should be dependent of type of event, as 
> > we previously discussed on v1?
>
> Yes, and I don't like it.

I respect your personal preference, but our conversation should be based 
on arguments, and not only tastes.

The important thing, from my point of view, is that the API stays easy 
to use and clear for the user. Having multiple callbacks is a headache, 
because you can't clearly group them somewhere, and force the user to 
implement all of them at once.

By having a single callback, we can force the user to handle all cases, 
thanks to the type system. The user may decide to use "default: break;" 
and that's ok, because they chose it deliberately.

I was, and I am still ok with the current approach, of having from/to 
parameters and a "simple" callback type. But remove "from" because we 
can't get it right in some cases does not seem the best decision.

Let's try to move forward, and solve the problems we have with from_pc. 
The testing part can be solved already (as explained in a previous 
message). In which cases can't you identify from_pc?

> Regards,
> Julian Ganz

By the way, and if you are open to talk about naming.

I understand why you picked up discontinuity, which is coming from a 
mathematical background. However, I rarely see this term used in the 
literature for computer science, and people use "exceptional control 
flow" to qualify interrupts and traps. In more, when we'll integrate 
classic control flow (including fallback between tb), the term 
discontinuity will lose its meaning. For this reason, I think that 
{cflow,CFLOW} makes more sense.

But, as there is some personal preference into this, I will leave the 
choice up to you.

Thanks,
Pierrick