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

Commit Message

Julian Ganz Dec. 2, 2024, 7:26 p.m. UTC

The plugin API allows registration of callbacks for a variety of VCPU
related events, such as VCPU reset, idle and resume. However, traps of
any kind, i.e. interrupts or exceptions, were previously not covered.
These kinds of events are arguably quite significant and usually go hand
in hand with a PC discontinuity. On most platforms, the discontinuity
also includes a transition from some "mode" to another. Thus, plugins
for the analysis of (virtualized) embedded systems may benefit from or
even require the possiblity to perform work on the occurance of an
interrupt or exception.

This change introduces the concept of such a discontinuity event in the
form of an enumeration. Currently only traps are covered. Specifically
we (loosely) define interrupts, exceptions and host calls across all
platforms. In addition, this change introduces a type to use for
callback functions related to such events. Since possible modes and the
enumeration of interupts and exceptions vary greatly between different
architectures, the callback type only receives the VCPU id, the type of
event as well as the old and new PC.
---
 include/qemu/plugin.h      |  1 +
 include/qemu/qemu-plugin.h | 43 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 44 insertions(+)

Comments

Julian Ganz Dec. 3, 2024, 8:45 a.m. UTC | #1

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.

> + */
> +typedef void (*qemu_plugin_vcpu_discon_cb_t)(qemu_plugin_id_t id,
> + unsigned int vcpu_index,
> + enum qemu_plugin_discon_type type,
> + uint64_t from_pc, uint64_t to_pc);
> +
>  /**
>  * qemu_plugin_uninstall() - Uninstall a plugin
>  * @id: this plugin's opaque ID
> -- 
> 2.45.2
>

Regards,
Julian

Pierrick Bouvier Dec. 4, 2024, 10:41 p.m. UTC | #2

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?

>> + */
>> +typedef void (*qemu_plugin_vcpu_discon_cb_t)(qemu_plugin_id_t id,
>> + unsigned int vcpu_index,
>> + enum qemu_plugin_discon_type type,
>> + uint64_t from_pc, uint64_t to_pc);
>> +
>>   /**
>>   * qemu_plugin_uninstall() - Uninstall a plugin
>>   * @id: this plugin's opaque ID
>> -- 
>> 2.45.2
>>
> 
> Regards,
> Julian
>

Pierrick Bouvier Dec. 4, 2024, 10:45 p.m. UTC | #3

Hi Julian,

thanks for the update!
Comments below.

On 12/2/24 11:26, Julian Ganz wrote:
> The plugin API allows registration of callbacks for a variety of VCPU
> related events, such as VCPU reset, idle and resume. However, traps of
> any kind, i.e. interrupts or exceptions, were previously not covered.
> These kinds of events are arguably quite significant and usually go hand
> in hand with a PC discontinuity. On most platforms, the discontinuity
> also includes a transition from some "mode" to another. Thus, plugins
> for the analysis of (virtualized) embedded systems may benefit from or
> even require the possiblity to perform work on the occurance of an
> interrupt or exception.
> 
> This change introduces the concept of such a discontinuity event in the
> form of an enumeration. Currently only traps are covered. Specifically
> we (loosely) define interrupts, exceptions and host calls across all
> platforms. In addition, this change introduces a type to use for
> callback functions related to such events. Since possible modes and the
> enumeration of interupts and exceptions vary greatly between different
> architectures, the callback type only receives the VCPU id, the type of
> event as well as the old and new PC.
> ---
>   include/qemu/plugin.h      |  1 +
>   include/qemu/qemu-plugin.h | 43 ++++++++++++++++++++++++++++++++++++++
>   2 files changed, 44 insertions(+)
> 
> diff --git a/include/qemu/plugin.h b/include/qemu/plugin.h
> index 9726a9ebf3..27a176b631 100644
> --- a/include/qemu/plugin.h
> +++ b/include/qemu/plugin.h
> @@ -59,6 +59,7 @@ union qemu_plugin_cb_sig {
>       qemu_plugin_udata_cb_t           udata;
>       qemu_plugin_vcpu_simple_cb_t     vcpu_simple;
>       qemu_plugin_vcpu_udata_cb_t      vcpu_udata;
> +    qemu_plugin_vcpu_discon_cb_t     vcpu_discon;
>       qemu_plugin_vcpu_tb_trans_cb_t   vcpu_tb_trans;
>       qemu_plugin_vcpu_mem_cb_t        vcpu_mem;
>       qemu_plugin_vcpu_syscall_cb_t    vcpu_syscall;
> 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,
>   typedef void (*qemu_plugin_vcpu_udata_cb_t)(unsigned int vcpu_index,
>                                               void *userdata);
>   
> +
> +/**
> + * enum qemu_plugin_discon_type - type of a (potential) PC discontinuity
> + *
> + * @QEMU_PLUGIN_DISCON_INTERRUPT: an interrupt, defined across all architectures
> + *                                as an asynchronous event, usually originating
> + *                                from outside the CPU
> + * @QEMU_PLUGIN_DISCON_EXCEPTION: an exception, defined across all architectures
> + *                                as a synchronous event in response to a
> + *                                specific instruction being executed
> + * @QEMU_PLUGIN_DISCON_HOSTCALL: a host call, functionally a special kind of
> + *                               exception that is not handled by code run by
> + *                               the vCPU but machinery outside the vCPU
> + * @QEMU_PLUGIN_DISCON_ALL: all types of disconinuity events currently covered
> + */
> +enum qemu_plugin_discon_type {
> +    QEMU_PLUGIN_DISCON_INTERRUPT = 1,
> +    QEMU_PLUGIN_DISCON_EXCEPTION = 2,
> +    QEMU_PLUGIN_DISCON_HOSTCALL = 4,
> +    QEMU_PLUGIN_DISCON_ALL = 7
> +};

Matter of style, but would be better to use:

enum qemu_plugin_discon_type {
      QEMU_PLUGIN_DISCON_INTERRUPT = 1 << 0,
      QEMU_PLUGIN_DISCON_EXCEPTION = 1 << 1,
      QEMU_PLUGIN_DISCON_HOSTCALL = 1 << 2,
      QEMU_PLUGIN_DISCON_ALL = -1
};

> +
> +/**
> + * 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
> + *

Missing those parameters when building doc.
include/qemu/qemu-plugin.h:198: warning: Function parameter or member 
'id' not described in 'qemu_plugin_vcpu_discon_cb_t'
include/qemu/qemu-plugin.h:289: warning: Function parameter or member 
'type' not described in 'qemu_plugin_register_vcpu_discon_cb'
2 warnings as Errors

> + * 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".
> + */
> +typedef void (*qemu_plugin_vcpu_discon_cb_t)(qemu_plugin_id_t id,
> +                                             unsigned int vcpu_index,
> +                                             enum qemu_plugin_discon_type type,
> +                                             uint64_t from_pc, uint64_t to_pc);
> +
>   /**
>    * qemu_plugin_uninstall() - Uninstall a plugin
>    * @id: this plugin's opaque ID

Overall I'm ok with what it looks like.

However, it seems that your conclusion was we can't always guarantee to 
have from_pc, and I'm not sure that the solution to simply drop it is 
the right one.

Julian Ganz Dec. 5, 2024, 12:40 p.m. UTC | #4

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.

Regards,
Julian Ganz

Julian Ganz Dec. 5, 2024, 12:44 p.m. UTC | #5

Hi Pierrick,

December 4, 2024 at 11:45 PM, "Pierrick Bouvier" wrote:
> On 12/2/24 11:26, Julian Ganz wrote:
> >  include/qemu/plugin.h | 1 +
> >  include/qemu/qemu-plugin.h | 43 ++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 44 insertions(+)
> >  diff --git a/include/qemu/plugin.h b/include/qemu/plugin.h
> >  index 9726a9ebf3..27a176b631 100644
> >  --- a/include/qemu/plugin.h
> >  +++ b/include/qemu/plugin.h
<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
> >  + *
> > 
> Missing those parameters when building doc.
> include/qemu/qemu-plugin.h:198: warning: Function parameter or member 'id' not described in 'qemu_plugin_vcpu_discon_cb_t'
> include/qemu/qemu-plugin.h:289: warning: Function parameter or member 'type' not described in 'qemu_plugin_register_vcpu_discon_cb'
> 2 warnings as Errors

Yes, I forgot about id. But type is clearly documented. Maybe the tool
is confused about the name and thinks it's a reserved word or something?
In that case I better change that to something else.

And note to self: also test-biuld the docs next time.

Regards,
Julian Ganz

Pierrick Bouvier Dec. 5, 2024, 5:35 p.m. UTC | #6

On 12/5/24 04:44, Julian Ganz wrote:
> Hi Pierrick,
> 
> December 4, 2024 at 11:45 PM, "Pierrick Bouvier" wrote:
>> On 12/2/24 11:26, Julian Ganz wrote:
>>>   include/qemu/plugin.h | 1 +
>>>   include/qemu/qemu-plugin.h | 43 ++++++++++++++++++++++++++++++++++++++
>>>   2 files changed, 44 insertions(+)
>>>   diff --git a/include/qemu/plugin.h b/include/qemu/plugin.h
>>>   index 9726a9ebf3..27a176b631 100644
>>>   --- a/include/qemu/plugin.h
>>>   +++ b/include/qemu/plugin.h
> <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
>>>   + *
>>>
>> Missing those parameters when building doc.
>> include/qemu/qemu-plugin.h:198: warning: Function parameter or member 'id' not described in 'qemu_plugin_vcpu_discon_cb_t'
>> include/qemu/qemu-plugin.h:289: warning: Function parameter or member 'type' not described in 'qemu_plugin_register_vcpu_discon_cb'
>> 2 warnings as Errors
> 
> Yes, I forgot about id. But type is clearly documented. Maybe the tool
> is confused about the name and thinks it's a reserved word or something?
> In that case I better change that to something else.
> 

The type (qemu_plugin_discon_type) is documented, but the parameter 
"type" is not. Even if the name is correctly chosen, you still need to 
make it appear in the doc.

> And note to self: also test-biuld the docs next time.
> 

I was bitten by this too when I started contributing to plugins, so no 
worries. It's safer to keep the docs enabled (that's the configure 
default), even if it adds some latency to the build, especially when 
touching documented headers.

> Regards,
> Julian Ganz

Pierrick Bouvier Dec. 5, 2024, 5:56 p.m. UTC | #7

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

Julian Ganz Dec. 5, 2024, 9:25 p.m. UTC | #8

Hi Pierrick,

December 5, 2024 at 6:35 PM, "Pierrick Bouvier" wrote:
> On 12/5/24 04:44, Julian Ganz wrote:
> 
> > 
> > Hi Pierrick,
> >  December 4, 2024 at 11:45 PM, "Pierrick Bouvier" wrote:
> > 
> > > 
> > > On 12/2/24 11:26, Julian Ganz wrote:
> > > 
> >  include/qemu/plugin.h | 1 +
> >  include/qemu/qemu-plugin.h | 43 ++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 44 insertions(+)
> >  diff --git a/include/qemu/plugin.h b/include/qemu/plugin.h
> >  index 9726a9ebf3..27a176b631 100644
> >  --- a/include/qemu/plugin.h
> >  +++ b/include/qemu/plugin.h
> >  <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
> >  + *
> > 
> > > 
> > > Missing those parameters when building doc.
> > >  include/qemu/qemu-plugin.h:198: warning: Function parameter or member 'id' not described in 'qemu_plugin_vcpu_discon_cb_t'
> > >  include/qemu/qemu-plugin.h:289: warning: Function parameter or member 'type' not described in 'qemu_plugin_register_vcpu_discon_cb'
> > >  2 warnings as Errors
> > > 
> >  Yes, I forgot about id. But type is clearly documented. Maybe the tool
> >  is confused about the name and thinks it's a reserved word or something?
> >  In that case I better change that to something else.
> > 
> The type (qemu_plugin_discon_type) is documented, but the parameter "type" is not. Even if the name is correctly chosen, you still need to make it appear in the doc.

Yes. I didn't realize that the two warnings were for different entities
because I somehow failed to parse the entire line.

Regards,
Julian Ganz

Julian Ganz Dec. 5, 2024, 9:50 p.m. UTC | #9

Hi Pierrick,

December 5, 2024 at 6:56 PM, "Pierrick Bouvier" wrote:
> On 12/5/24 04:40, Julian Ganz wrote:
> 
> > 
> > Hi Pierrick,
> >  December 4, 2024 at 11:41 PM, "Pierrick Bouvier" wrote:
> > > 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.

Having only one callback is not something I'm against.

> 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.

If you cannot rely on an input being a sensible value, doesn't that
render the input useless?

> 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?

I'll have to check, but problems that I discussed with a colleague
included jumps to an unmapped page resulting in the appropriate
exception. We ultimately agreed that in such a situation from_pc should
point to the jump target inside the unmapped page, instead of, say, the
jump. We assume that most targets should already behave this way without
further changes. However, in order to compute the correct from_pc, we
need to know the jump target before the exception is raised (i.e. right
after the jump instruction is executed), and that's not necessarily
straight-forward to do in a plugin.

But as I wrote before in another message, I need to take another look at
the cflow plugin.

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

I'm open to suggestions.

> 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.

Using the term "discontinuity" was, in fact, inspired by "uninferable PC
discontinuities" defined in the RISC-V ETrace spec [1], arguably a
technical document. We chose discontinuity over the notion of control
flow because the PC is not the (only) thing affected by the event. In
the case of host calls, we ideally don't even observe an effect on the
PC. Thus control flow doesn't really fit the bill for those.

Regards,
Julian Ganz

[1] https://github.com/riscv-non-isa/riscv-trace-spec

Julian Ganz Dec. 5, 2024, 10:14 p.m. UTC | #10

Hi Pierrick,

December 5, 2024 at 10:50 PM, "Julian Ganz" wrote:
> December 5, 2024 at 6:56 PM, "Pierrick Bouvier" wrote:
> > 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?
> > 
> I'll have to check, but problems that I discussed with a colleague
> included jumps to an unmapped page resulting in the appropriate
> exception. We ultimately agreed that in such a situation from_pc should
> point to the jump target inside the unmapped page, instead of, say, the
> jump. We assume that most targets should already behave this way without
> further changes. However, in order to compute the correct from_pc, we
> need to know the jump target before the exception is raised (i.e. right
> after the jump instruction is executed), and that's not necessarily
> straight-forward to do in a plugin.

Just remembered the joyful existence of double traps.  

Regards,
Julian

Pierrick Bouvier Dec. 5, 2024, 11:03 p.m. UTC | #11

On 12/5/24 13:50, Julian Ganz wrote:
> Hi Pierrick,
> 
> December 5, 2024 at 6:56 PM, "Pierrick Bouvier" wrote:
>> On 12/5/24 04:40, Julian Ganz wrote:
>>
>>>
>>> Hi Pierrick,
>>>   December 4, 2024 at 11:41 PM, "Pierrick Bouvier" wrote:
>>>> 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.
> 
> Having only one callback is not something I'm against.
> 
>> 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.
> 
> If you cannot rely on an input being a sensible value, doesn't that
> render the input useless?
> 

I agree. If for a specific event it's impossible to provide a value 
(i.e. the value has no meaning for a real cpu), it will just point that 
we need several types of data per event, and the compromise of having a 
single callback won't be possible.

We should differentiate "it's hard to find this value in QEMU" vs "this 
value does not exist in real life". The first can be solved if we put 
effort into it. And every time a cpu changes it's flow of execution, it 
makes sense to find where it was just before.

One of the end goals is to be able to build a full control flow graph, 
with edges labeled on transition type (exceptions, traps, interrupts, 
jump, fallback), which we can do with the triple {event,from,to}.

>> 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?
> 
> I'll have to check, but problems that I discussed with a colleague
> included jumps to an unmapped page resulting in the appropriate
> exception. We ultimately agreed that in such a situation from_pc should
> point to the jump target inside the unmapped page, instead of, say, the
> jump. We assume that most targets should already behave this way without
> further changes. However, in order to compute the correct from_pc, we
> need to know the jump target before the exception is raised (i.e. right
> after the jump instruction is executed), and that's not necessarily
> straight-forward to do in a plugin.

It's an interesting conversation. For the scope of this series, I agree 
you should use the jump target, which triggered the trap.

In fine, transitions should simply follow what the cpu does.

- orig_insn: jump to A
- jump_target: execute A traps
- page_fault: load page
- jump_target: come back to A

event(JUMP, orig_insn, jump_target) // not covered by this series
event(EXCEPTION, jump_target, page_fault)
... execute page_fault (with potential other transitions)
event(JUMP, end_page_fault, jump_target)

In the case of a double trap, we could follow the same logic, and 
represent the original transition that lead to the trap, and the two 
consecutive traps.

Does it make sense?

> 
> But as I wrote before in another message, I need to take another look at
> the cflow plugin.
> 
>> By the way, and if you are open to talk about naming.
> 
> I'm open to suggestions.
> 
>> 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.
> 
> Using the term "discontinuity" was, in fact, inspired by "uninferable PC
> discontinuities" defined in the RISC-V ETrace spec [1], arguably a
> technical document. We chose discontinuity over the notion of control
> flow because the PC is not the (only) thing affected by the event. In
> the case of host calls, we ideally don't even observe an effect on the
> PC. Thus control flow doesn't really fit the bill for those.
> 

I'm happy to read this spec reinvents a term clearly defined elsewhere 
(sigh...). Beyond the intellectual vocabulary creation pleasure, it's 
good to use term people can find on Google.

We can use this term though, PC discontinuity makes sense, and a 
fallback is a special case of discontinuity if we adopt this 
perspective. Thanks for explaining!

> Regards,
> Julian Ganz
> 
> [1] https://github.com/riscv-non-isa/riscv-trace-spec

Julian Ganz Dec. 6, 2024, 8:58 a.m. UTC | #12

Hi Pierrick,

December 6, 2024 at 12:03 AM, "Pierrick Bouvier" wrote:
> On 12/5/24 13:50, Julian Ganz wrote:
> >  If you cannot rely on an input being a sensible value, doesn't that
> >  render the input useless?
> > 
> I agree. If for a specific event it's impossible to provide a value (i.e. the value has no meaning for a real cpu), it will just point that we need several types of data per event, and the compromise of having a single callback won't be possible.
> 
> We should differentiate "it's hard to find this value in QEMU" vs "this value does not exist in real life". The first can be solved if we put effort into it. And every time a cpu changes it's flow of execution, it makes sense to find where it was just before.
> 
> One of the end goals is to be able to build a full control flow graph, with edges labeled on transition type (exceptions, traps, interrupts, jump, fallback), which we can do with the triple {event,from,to}.

I agree that that triple is sensible for any event type and likely
useful to plugin authors. At least if the semantics are sufficiently
uniform among event types. However, I also feel that given the actual
implementation (hooks sprinkled over target specific code) this is not
easily achievable reliably. At least testability should be a hard
requirement. Otherwise the API's reliability will inevitably deteriorate
over time without any way to tell how bad the situation got.

> > > 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?
> > > 
> >  I'll have to check, but problems that I discussed with a colleague
> >  included jumps to an unmapped page resulting in the appropriate
> >  exception. We ultimately agreed that in such a situation from_pc should
> >  point to the jump target inside the unmapped page, instead of, say, the
> >  jump. We assume that most targets should already behave this way without
> >  further changes. However, in order to compute the correct from_pc, we
> >  need to know the jump target before the exception is raised (i.e. right
> >  after the jump instruction is executed), and that's not necessarily
> >  straight-forward to do in a plugin.
> > 
> It's an interesting conversation. For the scope of this series, I agree you should use the jump target, which triggered the trap.
> 
> In fine, transitions should simply follow what the cpu does.
> 
> - orig_insn: jump to A
> - jump_target: execute A traps
> - page_fault: load page
> - jump_target: come back to A
> 
> event(JUMP, orig_insn, jump_target) // not covered by this series
> event(EXCEPTION, jump_target, page_fault)
> ... execute page_fault (with potential other transitions)
> event(JUMP, end_page_fault, jump_target)
> 
> In the case of a double trap, we could follow the same logic, and represent the original transition that lead to the trap, and the two consecutive traps.
> 
> Does it make sense?

Yes, those transitions are correct imo. And if a jump event should be
introduced at some point, the call sequence would look like that. My
issue is that testing this (in a plugin) will not be straight forward
or even impossible. And overly complex tests don't exactly provoke
confidence.

Regards,
Julian Ganz

Pierrick Bouvier Dec. 6, 2024, 6:59 p.m. UTC | #13

On 12/6/24 00:58, Julian Ganz wrote:
> Hi Pierrick,
> 
> December 6, 2024 at 12:03 AM, "Pierrick Bouvier" wrote:
>> On 12/5/24 13:50, Julian Ganz wrote:
>>>   If you cannot rely on an input being a sensible value, doesn't that
>>>   render the input useless?
>>>
>> I agree. If for a specific event it's impossible to provide a value (i.e. the value has no meaning for a real cpu), it will just point that we need several types of data per event, and the compromise of having a single callback won't be possible.
>>
>> We should differentiate "it's hard to find this value in QEMU" vs "this value does not exist in real life". The first can be solved if we put effort into it. And every time a cpu changes it's flow of execution, it makes sense to find where it was just before.
>>
>> One of the end goals is to be able to build a full control flow graph, with edges labeled on transition type (exceptions, traps, interrupts, jump, fallback), which we can do with the triple {event,from,to}.
> 
> I agree that that triple is sensible for any event type and likely
> useful to plugin authors. At least if the semantics are sufficiently
> uniform among event types. However, I also feel that given the actual
> implementation (hooks sprinkled over target specific code) this is not
> easily achievable reliably. At least testability should be a hard
> requirement. Otherwise the API's reliability will inevitably deteriorate
> over time without any way to tell how bad the situation got.
> 
>>>> 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?
>>>>
>>>   I'll have to check, but problems that I discussed with a colleague
>>>   included jumps to an unmapped page resulting in the appropriate
>>>   exception. We ultimately agreed that in such a situation from_pc should
>>>   point to the jump target inside the unmapped page, instead of, say, the
>>>   jump. We assume that most targets should already behave this way without
>>>   further changes. However, in order to compute the correct from_pc, we
>>>   need to know the jump target before the exception is raised (i.e. right
>>>   after the jump instruction is executed), and that's not necessarily
>>>   straight-forward to do in a plugin.
>>>
>> It's an interesting conversation. For the scope of this series, I agree you should use the jump target, which triggered the trap.
>>
>> In fine, transitions should simply follow what the cpu does.
>>
>> - orig_insn: jump to A
>> - jump_target: execute A traps
>> - page_fault: load page
>> - jump_target: come back to A
>>
>> event(JUMP, orig_insn, jump_target) // not covered by this series
>> event(EXCEPTION, jump_target, page_fault)
>> ... execute page_fault (with potential other transitions)
>> event(JUMP, end_page_fault, jump_target)
>>
>> In the case of a double trap, we could follow the same logic, and represent the original transition that lead to the trap, and the two consecutive traps.
>>
>> Does it make sense?
> 
> Yes, those transitions are correct imo. And if a jump event should be
> introduced at some point, the call sequence would look like that. My
> issue is that testing this (in a plugin) will not be straight forward
> or even impossible. And overly complex tests don't exactly provoke
> confidence.
> 

Instruction instrumentation is done before executing the instruction 
itself, as you can see by running:
./build/qemu-x86_64 -plugin build/tests/tcg/plugins/libinsn.so -d 
in_asm,op /usr/bin/true

I'm not entirely sure about the sequence when there is an exception 
while fetching the instruction though. You can give it a try, track the 
PC using insn instrumentation, and we can identify which cases are not 
working.

The test plugin itself is not complicated.
You'll need:
- one callback per instruction to set the expected pc (possibly 
optimized with inline operation), used to compare to from_pc, and we 
check if (optional) to_pc matches the current instruction.
- when the callback for discontinuity is called, we check if from_pc is 
matching, and register the next expected with to_pc.

We can then add tests targeting supported architectures using the 
plugin, and ensuring it never fails.
It's hard to know we don't miss events though. Except if we write manual 
assembly system mode tests, that trigger the expected behaviour. But it 
would be tedious, and I'm really not sure there is a real value with 
reduced examples like this.

> Regards,
> Julian Ganz

Julian Ganz Dec. 7, 2024, 1:38 p.m. UTC | #14

Hi Pierrick,

December 6, 2024 at 7:59 PM, "Pierrick Bouvier" wrote:
> On 12/6/24 00:58, Julian Ganz wrote:
> >  December 6, 2024 at 12:03 AM, "Pierrick Bouvier" wrote:
> > > It's an interesting conversation. For the scope of this series, I agree you should use the jump target, which triggered the trap.
> > > 
> > >  In fine, transitions should simply follow what the cpu does.
> > > 
> > >  - orig_insn: jump to A
> > >  - jump_target: execute A traps
> > >  - page_fault: load page
> > >  - jump_target: come back to A
> > > 
> > >  event(JUMP, orig_insn, jump_target) // not covered by this series
> > >  event(EXCEPTION, jump_target, page_fault)
> > >  ... execute page_fault (with potential other transitions)
> > >  event(JUMP, end_page_fault, jump_target)
> > > 
> > >  In the case of a double trap, we could follow the same logic, and represent the original transition that lead to the trap, and the two consecutive traps.
> > > 
> > >  Does it make sense?
> > > 
> >  Yes, those transitions are correct imo. And if a jump event should be
> >  introduced at some point, the call sequence would look like that. My
> >  issue is that testing this (in a plugin) will not be straight forward
> >  or even impossible. And overly complex tests don't exactly provoke
> >  confidence.
> > 
> Instruction instrumentation is done before executing the instruction itself, as you can see by running:
> ./build/qemu-x86_64 -plugin build/tests/tcg/plugins/libinsn.so -d in_asm,op /usr/bin/true
> 
> I'm not entirely sure about the sequence when there is an exception while fetching the instruction though. You can give it a try, track the PC using insn instrumentation, and we can identify which cases are not working.
> 
> The test plugin itself is not complicated.
> You'll need:
> - one callback per instruction to set the expected pc (possibly optimized with inline operation), used to compare to from_pc, and we check if (optional) to_pc matches the current instruction.

What I'm saying ist that this exactly is not feasible for quite a few
relevant instructions. As someone who isn't all too intimate with TCG
itself, it's not even clear if we can rely on jump and branch
instructions only occuring at the end of a tb. At least a superficial
glance at the documentation tells me we can, but if this should in fact
not be the case, or if someone introduces something like zero overhead
loops inside a tb, all bets are off.

> - when the callback for discontinuity is called, we check if from_pc is matching, and register the next expected with to_pc.
> 
> We can then add tests targeting supported architectures using the plugin, and ensuring it never fails.
> It's hard to know we don't miss events though. Except if we write manual assembly system mode tests, that trigger the expected behaviour. But it would be tedious, and I'm really not sure there is a real value with reduced examples like this.

So currently my thinking goes in the direction of having the plugin
print a warning every time we don't have an expected from_pc to check
against. Probably also have this be a feature you can deactivate.

Regards,
Julian Ganz

Pierrick Bouvier Dec. 9, 2024, 6:52 p.m. UTC | #15

On 12/7/24 05:38, Julian Ganz wrote:
> Hi Pierrick,
> 
> December 6, 2024 at 7:59 PM, "Pierrick Bouvier" wrote:
>> On 12/6/24 00:58, Julian Ganz wrote:
>>>   December 6, 2024 at 12:03 AM, "Pierrick Bouvier" wrote:
>>>> It's an interesting conversation. For the scope of this series, I agree you should use the jump target, which triggered the trap.
>>>>
>>>>   In fine, transitions should simply follow what the cpu does.
>>>>
>>>>   - orig_insn: jump to A
>>>>   - jump_target: execute A traps
>>>>   - page_fault: load page
>>>>   - jump_target: come back to A
>>>>
>>>>   event(JUMP, orig_insn, jump_target) // not covered by this series
>>>>   event(EXCEPTION, jump_target, page_fault)
>>>>   ... execute page_fault (with potential other transitions)
>>>>   event(JUMP, end_page_fault, jump_target)
>>>>
>>>>   In the case of a double trap, we could follow the same logic, and represent the original transition that lead to the trap, and the two consecutive traps.
>>>>
>>>>   Does it make sense?
>>>>
>>>   Yes, those transitions are correct imo. And if a jump event should be
>>>   introduced at some point, the call sequence would look like that. My
>>>   issue is that testing this (in a plugin) will not be straight forward
>>>   or even impossible. And overly complex tests don't exactly provoke
>>>   confidence.
>>>
>> Instruction instrumentation is done before executing the instruction itself, as you can see by running:
>> ./build/qemu-x86_64 -plugin build/tests/tcg/plugins/libinsn.so -d in_asm,op /usr/bin/true
>>
>> I'm not entirely sure about the sequence when there is an exception while fetching the instruction though. You can give it a try, track the PC using insn instrumentation, and we can identify which cases are not working.
>>
>> The test plugin itself is not complicated.
>> You'll need:
>> - one callback per instruction to set the expected pc (possibly optimized with inline operation), used to compare to from_pc, and we check if (optional) to_pc matches the current instruction.
> 
> What I'm saying ist that this exactly is not feasible for quite a few
> relevant instructions. As someone who isn't all too intimate with TCG
> itself, it's not even clear if we can rely on jump and branch
> instructions only occuring at the end of a tb. At least a superficial
> glance at the documentation tells me we can, but if this should in fact
> not be the case, or if someone introduces something like zero overhead
> loops inside a tb, all bets are off.
> 

TCG already has the possibility to generate jumps inside a TB 
(conditional branches - brcond, is an example).

However, for the scope of this series, it does not matter.
We only have to check the from_pc when a discontinuity is detected (and 
not all the time), so there is no need to anticipate the "next" instruction.

>> - when the callback for discontinuity is called, we check if from_pc is matching, and register the next expected with to_pc.
>>
>> We can then add tests targeting supported architectures using the plugin, and ensuring it never fails.
>> It's hard to know we don't miss events though. Except if we write manual assembly system mode tests, that trigger the expected behaviour. But it would be tedious, and I'm really not sure there is a real value with reduced examples like this.
> 
> So currently my thinking goes in the direction of having the plugin
> print a warning every time we don't have an expected from_pc to check
> against. Probably also have this be a feature you can deactivate.
> 

As it's a test plugin, I think it would be a better default to abort on 
error, and not be able to deactivate it. Any difference we find would be 
a bug anyway, so require fixing.

> Regards,
> Julian Ganz

Patch

diff --git a/include/qemu/plugin.h b/include/qemu/plugin.h
index 9726a9ebf3..27a176b631 100644
--- a/include/qemu/plugin.h
+++ b/include/qemu/plugin.h
@@ -59,6 +59,7 @@  union qemu_plugin_cb_sig {
     qemu_plugin_udata_cb_t           udata;
     qemu_plugin_vcpu_simple_cb_t     vcpu_simple;
     qemu_plugin_vcpu_udata_cb_t      vcpu_udata;
+    qemu_plugin_vcpu_discon_cb_t     vcpu_discon;
     qemu_plugin_vcpu_tb_trans_cb_t   vcpu_tb_trans;
     qemu_plugin_vcpu_mem_cb_t        vcpu_mem;
     qemu_plugin_vcpu_syscall_cb_t    vcpu_syscall;
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,
 typedef void (*qemu_plugin_vcpu_udata_cb_t)(unsigned int vcpu_index,
                                             void *userdata);
 
+
+/**
+ * enum qemu_plugin_discon_type - type of a (potential) PC discontinuity
+ *
+ * @QEMU_PLUGIN_DISCON_INTERRUPT: an interrupt, defined across all architectures
+ *                                as an asynchronous event, usually originating
+ *                                from outside the CPU
+ * @QEMU_PLUGIN_DISCON_EXCEPTION: an exception, defined across all architectures
+ *                                as a synchronous event in response to a
+ *                                specific instruction being executed
+ * @QEMU_PLUGIN_DISCON_HOSTCALL: a host call, functionally a special kind of
+ *                               exception that is not handled by code run by
+ *                               the vCPU but machinery outside the vCPU
+ * @QEMU_PLUGIN_DISCON_ALL: all types of disconinuity events currently covered
+ */
+enum qemu_plugin_discon_type {
+    QEMU_PLUGIN_DISCON_INTERRUPT = 1,
+    QEMU_PLUGIN_DISCON_EXCEPTION = 2,
+    QEMU_PLUGIN_DISCON_HOSTCALL = 4,
+    QEMU_PLUGIN_DISCON_ALL = 7
+};
+
+/**
+ * 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".
+ */
+typedef void (*qemu_plugin_vcpu_discon_cb_t)(qemu_plugin_id_t id,
+                                             unsigned int vcpu_index,
+                                             enum qemu_plugin_discon_type type,
+                                             uint64_t from_pc, uint64_t to_pc);
+
 /**
  * qemu_plugin_uninstall() - Uninstall a plugin
  * @id: this plugin's opaque ID