[PATCH v5 03/25] plugins: add hooks for new discontinuity related callbacks

Julian Ganz posted 25 patches 6 months ago
Maintainers: "Alex Bennée" <alex.bennee@linaro.org>, Alexandre Iooss <erdnaxe@crans.org>, Mahmoud Mandour <ma.mandourr@gmail.com>, Pierrick Bouvier <pierrick.bouvier@linaro.org>, Richard Henderson <richard.henderson@linaro.org>, Peter Maydell <peter.maydell@linaro.org>, Michael Rolnik <mrolnik@gmail.com>, Helge Deller <deller@gmx.de>, Paolo Bonzini <pbonzini@redhat.com>, Eduardo Habkost <eduardo@habkost.net>, Song Gao <gaosong@loongson.cn>, Laurent Vivier <laurent@vivier.eu>, "Edgar E. Iglesias" <edgar.iglesias@gmail.com>, "Philippe Mathieu-Daudé" <philmd@linaro.org>, Aurelien Jarno <aurelien@aurel32.net>, Jiaxun Yang <jiaxun.yang@flygoat.com>, Aleksandar Rikalo <arikalo@gmail.com>, Stafford Horne <shorne@gmail.com>, Nicholas Piggin <npiggin@gmail.com>, Daniel Henrique Barboza <danielhb413@gmail.com>, Palmer Dabbelt <palmer@dabbelt.com>, Alistair Francis <alistair.francis@wdc.com>, Weiwei Li <liwei1518@gmail.com>, Liu Zhiwei <zhiwei_liu@linux.alibaba.com>, Yoshinori Sato <ysato@users.sourceforge.jp>, David Hildenbrand <david@redhat.com>, Ilya Leoshkevich <iii@linux.ibm.com>, Thomas Huth <thuth@redhat.com>, Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>, Artyom Tarasenko <atar4qemu@gmail.com>, Bastian Koppelmann <kbastian@mail.uni-paderborn.de>, Max Filippov <jcmvbkbc@gmail.com>
There is a newer version of this series
[PATCH v5 03/25] plugins: add hooks for new discontinuity related callbacks
Posted by Julian Ganz 6 months ago
The plugin API allows registration of callbacks for a variety of VCPU
related events, such as VCPU reset, idle and resume. In addition, we
recently introduced API for registering callbacks for discontinuity
events, specifically for interrupts, exceptions and host calls.

This change introduces the corresponding hooks called from target
specific code inside qemu.

Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
Signed-off-by: Julian Ganz <neither@nut.email>
---
 include/qemu/plugin.h | 12 ++++++++++
 plugins/core.c        | 53 +++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 65 insertions(+)

diff --git a/include/qemu/plugin.h b/include/qemu/plugin.h
index 27a176b631..3494325039 100644
--- a/include/qemu/plugin.h
+++ b/include/qemu/plugin.h
@@ -161,6 +161,9 @@ void qemu_plugin_vcpu_exit_hook(CPUState *cpu);
 void qemu_plugin_tb_trans_cb(CPUState *cpu, struct qemu_plugin_tb *tb);
 void qemu_plugin_vcpu_idle_cb(CPUState *cpu);
 void qemu_plugin_vcpu_resume_cb(CPUState *cpu);
+void qemu_plugin_vcpu_interrupt_cb(CPUState *cpu, uint64_t from);
+void qemu_plugin_vcpu_exception_cb(CPUState *cpu, uint64_t from);
+void qemu_plugin_vcpu_hostcall_cb(CPUState *cpu, uint64_t from);
 void
 qemu_plugin_vcpu_syscall(CPUState *cpu, int64_t num, uint64_t a1,
                          uint64_t a2, uint64_t a3, uint64_t a4, uint64_t a5,
@@ -243,6 +246,15 @@ static inline void qemu_plugin_vcpu_idle_cb(CPUState *cpu)
 static inline void qemu_plugin_vcpu_resume_cb(CPUState *cpu)
 { }
 
+static inline void qemu_plugin_vcpu_interrupt_cb(CPUState *cpu, uint64_t from)
+{ }
+
+static inline void qemu_plugin_vcpu_exception_cb(CPUState *cpu, uint64_t from)
+{ }
+
+static inline void qemu_plugin_vcpu_hostcall_cb(CPUState *cpu, uint64_t from)
+{ }
+
 static inline void
 qemu_plugin_vcpu_syscall(CPUState *cpu, int64_t num, uint64_t a1, uint64_t a2,
                          uint64_t a3, uint64_t a4, uint64_t a5, uint64_t a6,
diff --git a/plugins/core.c b/plugins/core.c
index dc1f5cb4d8..f07813d588 100644
--- a/plugins/core.c
+++ b/plugins/core.c
@@ -104,6 +104,44 @@ static void plugin_vcpu_cb__simple(CPUState *cpu, enum qemu_plugin_event ev)
     }
 }
 
+/*
+ * Disable CFI checks.
+ * The callback function has been loaded from an external library so we do not
+ * have type information
+ */
+QEMU_DISABLE_CFI
+static void plugin_vcpu_cb__discon(CPUState *cpu,
+                                   enum qemu_plugin_discon_type type,
+                                   uint64_t from)
+{
+    struct qemu_plugin_cb *cb, *next;
+    enum qemu_plugin_event ev;
+    uint64_t to = cpu->cc->get_pc(cpu);
+
+    if (cpu->cpu_index < plugin.num_vcpus) {
+        switch (type) {
+        case QEMU_PLUGIN_DISCON_INTERRUPT:
+            ev = QEMU_PLUGIN_EV_VCPU_INTERRUPT;
+            break;
+        case QEMU_PLUGIN_DISCON_EXCEPTION:
+            ev = QEMU_PLUGIN_EV_VCPU_EXCEPTION;
+            break;
+        case QEMU_PLUGIN_DISCON_HOSTCALL:
+            ev = QEMU_PLUGIN_EV_VCPU_HOSTCALL;
+            break;
+        default:
+            g_assert_not_reached();
+        }
+
+        /* iterate safely; plugins might uninstall themselves at any time */
+        QLIST_FOREACH_SAFE_RCU(cb, &plugin.cb_lists[ev], entry, next) {
+            qemu_plugin_vcpu_discon_cb_t func = cb->f.vcpu_discon;
+
+            func(cb->ctx->id, cpu->cpu_index, type, from, to);
+        }
+    }
+}
+
 /*
  * Disable CFI checks.
  * The callback function has been loaded from an external library so we do not
@@ -539,6 +577,21 @@ void qemu_plugin_vcpu_resume_cb(CPUState *cpu)
     }
 }
 
+void qemu_plugin_vcpu_interrupt_cb(CPUState *cpu, uint64_t from)
+{
+    plugin_vcpu_cb__discon(cpu, QEMU_PLUGIN_DISCON_INTERRUPT, from);
+}
+
+void qemu_plugin_vcpu_exception_cb(CPUState *cpu, uint64_t from)
+{
+    plugin_vcpu_cb__discon(cpu, QEMU_PLUGIN_DISCON_EXCEPTION, from);
+}
+
+void qemu_plugin_vcpu_hostcall_cb(CPUState *cpu, uint64_t from)
+{
+    plugin_vcpu_cb__discon(cpu, QEMU_PLUGIN_DISCON_HOSTCALL, from);
+}
+
 void qemu_plugin_register_vcpu_idle_cb(qemu_plugin_id_t id,
                                        qemu_plugin_vcpu_simple_cb_t cb)
 {
-- 
2.49.0
Re: [PATCH v5 03/25] plugins: add hooks for new discontinuity related callbacks
Posted by Richard Henderson 5 months, 3 weeks ago
On 5/19/25 16:19, Julian Ganz wrote:
> The plugin API allows registration of callbacks for a variety of VCPU
> related events, such as VCPU reset, idle and resume. In addition, we
> recently introduced API for registering callbacks for discontinuity
> events, specifically for interrupts, exceptions and host calls.
> 
> This change introduces the corresponding hooks called from target
> specific code inside qemu.
> 
> Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
> Signed-off-by: Julian Ganz <neither@nut.email>
> ---
>   include/qemu/plugin.h | 12 ++++++++++
>   plugins/core.c        | 53 +++++++++++++++++++++++++++++++++++++++++++
>   2 files changed, 65 insertions(+)
> 
> diff --git a/include/qemu/plugin.h b/include/qemu/plugin.h
> index 27a176b631..3494325039 100644
> --- a/include/qemu/plugin.h
> +++ b/include/qemu/plugin.h
> @@ -161,6 +161,9 @@ void qemu_plugin_vcpu_exit_hook(CPUState *cpu);
>   void qemu_plugin_tb_trans_cb(CPUState *cpu, struct qemu_plugin_tb *tb);
>   void qemu_plugin_vcpu_idle_cb(CPUState *cpu);
>   void qemu_plugin_vcpu_resume_cb(CPUState *cpu);
> +void qemu_plugin_vcpu_interrupt_cb(CPUState *cpu, uint64_t from);
> +void qemu_plugin_vcpu_exception_cb(CPUState *cpu, uint64_t from);
> +void qemu_plugin_vcpu_hostcall_cb(CPUState *cpu, uint64_t from);
>   void
>   qemu_plugin_vcpu_syscall(CPUState *cpu, int64_t num, uint64_t a1,
>                            uint64_t a2, uint64_t a3, uint64_t a4, uint64_t a5,
> @@ -243,6 +246,15 @@ static inline void qemu_plugin_vcpu_idle_cb(CPUState *cpu)
>   static inline void qemu_plugin_vcpu_resume_cb(CPUState *cpu)
>   { }
>   
> +static inline void qemu_plugin_vcpu_interrupt_cb(CPUState *cpu, uint64_t from)
> +{ }
> +
> +static inline void qemu_plugin_vcpu_exception_cb(CPUState *cpu, uint64_t from)
> +{ }
> +
> +static inline void qemu_plugin_vcpu_hostcall_cb(CPUState *cpu, uint64_t from)
> +{ }
> +
>   static inline void
>   qemu_plugin_vcpu_syscall(CPUState *cpu, int64_t num, uint64_t a1, uint64_t a2,
>                            uint64_t a3, uint64_t a4, uint64_t a5, uint64_t a6,
> diff --git a/plugins/core.c b/plugins/core.c
> index dc1f5cb4d8..f07813d588 100644
> --- a/plugins/core.c
> +++ b/plugins/core.c
> @@ -104,6 +104,44 @@ static void plugin_vcpu_cb__simple(CPUState *cpu, enum qemu_plugin_event ev)
>       }
>   }
>   
> +/*
> + * Disable CFI checks.
> + * The callback function has been loaded from an external library so we do not
> + * have type information
> + */
> +QEMU_DISABLE_CFI
> +static void plugin_vcpu_cb__discon(CPUState *cpu,
> +                                   enum qemu_plugin_discon_type type,
> +                                   uint64_t from)
> +{
> +    struct qemu_plugin_cb *cb, *next;
> +    enum qemu_plugin_event ev;
> +    uint64_t to = cpu->cc->get_pc(cpu);

Maybe cleaner to pass in @to.  It's readily available in the callers.


r~

> +
> +    if (cpu->cpu_index < plugin.num_vcpus) {
> +        switch (type) {
> +        case QEMU_PLUGIN_DISCON_INTERRUPT:
> +            ev = QEMU_PLUGIN_EV_VCPU_INTERRUPT;
> +            break;
> +        case QEMU_PLUGIN_DISCON_EXCEPTION:
> +            ev = QEMU_PLUGIN_EV_VCPU_EXCEPTION;
> +            break;
> +        case QEMU_PLUGIN_DISCON_HOSTCALL:
> +            ev = QEMU_PLUGIN_EV_VCPU_HOSTCALL;
> +            break;
> +        default:
> +            g_assert_not_reached();
> +        }
> +
> +        /* iterate safely; plugins might uninstall themselves at any time */
> +        QLIST_FOREACH_SAFE_RCU(cb, &plugin.cb_lists[ev], entry, next) {
> +            qemu_plugin_vcpu_discon_cb_t func = cb->f.vcpu_discon;
> +
> +            func(cb->ctx->id, cpu->cpu_index, type, from, to);
> +        }
> +    }
> +}
> +
>   /*
>    * Disable CFI checks.
>    * The callback function has been loaded from an external library so we do not
> @@ -539,6 +577,21 @@ void qemu_plugin_vcpu_resume_cb(CPUState *cpu)
>       }
>   }
>   
> +void qemu_plugin_vcpu_interrupt_cb(CPUState *cpu, uint64_t from)
> +{
> +    plugin_vcpu_cb__discon(cpu, QEMU_PLUGIN_DISCON_INTERRUPT, from);
> +}
> +
> +void qemu_plugin_vcpu_exception_cb(CPUState *cpu, uint64_t from)
> +{
> +    plugin_vcpu_cb__discon(cpu, QEMU_PLUGIN_DISCON_EXCEPTION, from);
> +}
> +
> +void qemu_plugin_vcpu_hostcall_cb(CPUState *cpu, uint64_t from)
> +{
> +    plugin_vcpu_cb__discon(cpu, QEMU_PLUGIN_DISCON_HOSTCALL, from);
> +}
> +
>   void qemu_plugin_register_vcpu_idle_cb(qemu_plugin_id_t id,
>                                          qemu_plugin_vcpu_simple_cb_t cb)
>   {
Re: [PATCH v5 03/25] plugins: add hooks for new discontinuity related callbacks
Posted by Julian Ganz 5 months, 3 weeks ago
Hi Richard,

Richard Henderson wrote:
> On 5/19/25 16:19, Julian Ganz wrote:
> > diff --git a/plugins/core.c b/plugins/core.c
> > index dc1f5cb4d8..f07813d588 100644
> > --- a/plugins/core.c
> > +++ b/plugins/core.c
> > @@ -104,6 +104,44 @@ static void plugin_vcpu_cb__simple(CPUState *cpu, enum qemu_plugin_event ev)
> >       }
> >   }
> >   
> > +/*
> > + * Disable CFI checks.
> > + * The callback function has been loaded from an external library so we do not
> > + * have type information
> > + */
> > +QEMU_DISABLE_CFI
> > +static void plugin_vcpu_cb__discon(CPUState *cpu,
> > +                                   enum qemu_plugin_discon_type type,
> > +                                   uint64_t from)
> > +{
> > +    struct qemu_plugin_cb *cb, *next;
> > +    enum qemu_plugin_event ev;
> > +    uint64_t to = cpu->cc->get_pc(cpu);
> 
> Maybe cleaner to pass in @to.  It's readily available in the callers.

We originally did do that. However, we tried to reduce the noise we have at the
hook calling site to a minimum. And for some targets we would need to consult
get_pc anyway because the PC computation... takes more than one line.

I can still add the parameter. But especially given your latest sugestion of
altering the do_interrupt interface this may not be worth it.

Regards,
Julian
Re: [PATCH v5 03/25] plugins: add hooks for new discontinuity related callbacks
Posted by Richard Henderson 5 months, 3 weeks ago
On 5/19/25 16:19, Julian Ganz wrote:
> +QEMU_DISABLE_CFI
> +static void plugin_vcpu_cb__discon(CPUState *cpu,
> +                                   enum qemu_plugin_discon_type type,
> +                                   uint64_t from)
> +{
> +    struct qemu_plugin_cb *cb, *next;
> +    enum qemu_plugin_event ev;
> +    uint64_t to = cpu->cc->get_pc(cpu);
> +
> +    if (cpu->cpu_index < plugin.num_vcpus) {
> +        switch (type) {
> +        case QEMU_PLUGIN_DISCON_INTERRUPT:
> +            ev = QEMU_PLUGIN_EV_VCPU_INTERRUPT;
> +            break;
> +        case QEMU_PLUGIN_DISCON_EXCEPTION:
> +            ev = QEMU_PLUGIN_EV_VCPU_EXCEPTION;
> +            break;
> +        case QEMU_PLUGIN_DISCON_HOSTCALL:
> +            ev = QEMU_PLUGIN_EV_VCPU_HOSTCALL;
> +            break;

No point passing in QEMU_PLUGIN_DISCON_* only to covert it to QEMU_PLUGIN_EV_*.

> +void qemu_plugin_vcpu_interrupt_cb(CPUState *cpu, uint64_t from)
> +{
> +    plugin_vcpu_cb__discon(cpu, QEMU_PLUGIN_DISCON_INTERRUPT, from);
> +}
> +
> +void qemu_plugin_vcpu_exception_cb(CPUState *cpu, uint64_t from)
> +{
> +    plugin_vcpu_cb__discon(cpu, QEMU_PLUGIN_DISCON_EXCEPTION, from);
> +}
> +
> +void qemu_plugin_vcpu_hostcall_cb(CPUState *cpu, uint64_t from)
> +{
> +    plugin_vcpu_cb__discon(cpu, QEMU_PLUGIN_DISCON_HOSTCALL, from);
> +}

Better to pass in the value we really want here.


r~
Re: [PATCH v5 03/25] plugins: add hooks for new discontinuity related callbacks
Posted by Julian Ganz 5 months, 3 weeks ago
Hi Richard,

CC-ing all the maintainers again.

Richard Henderson wrote:
> On 5/19/25 16:19, Julian Ganz wrote:
> > +QEMU_DISABLE_CFI
> > +static void plugin_vcpu_cb__discon(CPUState *cpu,
> > +                                   enum qemu_plugin_discon_type type,
> > +                                   uint64_t from)
> > +{
> > +    struct qemu_plugin_cb *cb, *next;
> > +    enum qemu_plugin_event ev;
> > +    uint64_t to = cpu->cc->get_pc(cpu);
> > +
> > +    if (cpu->cpu_index < plugin.num_vcpus) {
> > +        switch (type) {
> > +        case QEMU_PLUGIN_DISCON_INTERRUPT:
> > +            ev = QEMU_PLUGIN_EV_VCPU_INTERRUPT;
> > +            break;
> > +        case QEMU_PLUGIN_DISCON_EXCEPTION:
> > +            ev = QEMU_PLUGIN_EV_VCPU_EXCEPTION;
> > +            break;
> > +        case QEMU_PLUGIN_DISCON_HOSTCALL:
> > +            ev = QEMU_PLUGIN_EV_VCPU_HOSTCALL;
> > +            break;
> 
> No point passing in QEMU_PLUGIN_DISCON_* only to covert it to QEMU_PLUGIN_EV_*.

It easily looks that way, and I myself stubled upon this at least one or two
times, but `type` is the enum we pass to the callback a few lines down and part
of the public plugin API. `ev` on the other hand is the offset in the `cb_list`.
So some translation is neccessary, unfortunately.

Regards,
Julian
Re: [PATCH v5 03/25] plugins: add hooks for new discontinuity related callbacks
Posted by Julian Ganz 5 months, 3 weeks ago
May 25, 2025 at 10:56 PM, "Julian Ganz" wrote:
> Richard Henderson wrote:
> > On 5/19/25 16:19, Julian Ganz wrote:
> >  +QEMU_DISABLE_CFI
> >  +static void plugin_vcpu_cb__discon(CPUState *cpu,
> >  + enum qemu_plugin_discon_type type,
> >  + uint64_t from)
> >  +{
> >  + struct qemu_plugin_cb *cb, *next;
> >  + enum qemu_plugin_event ev;
> >  + uint64_t to = cpu->cc->get_pc(cpu);
> >  +
> >  + if (cpu->cpu_index < plugin.num_vcpus) {
> >  + switch (type) {
> >  + case QEMU_PLUGIN_DISCON_INTERRUPT:
> >  + ev = QEMU_PLUGIN_EV_VCPU_INTERRUPT;
> >  + break;
> >  + case QEMU_PLUGIN_DISCON_EXCEPTION:
> >  + ev = QEMU_PLUGIN_EV_VCPU_EXCEPTION;
> >  + break;
> >  + case QEMU_PLUGIN_DISCON_HOSTCALL:
> >  + ev = QEMU_PLUGIN_EV_VCPU_HOSTCALL;
> >  + break;
> >  
> >  No point passing in QEMU_PLUGIN_DISCON_* only to covert it to QEMU_PLUGIN_EV_*.
> > 
> It easily looks that way, and I myself stubled upon this at least one or two
> times, but `type` is the enum we pass to the callback a few lines down and part
> of the public plugin API. `ev` on the other hand is the offset in the `cb_list`.
> So some translation is neccessary, unfortunately.

I just realized that you probably meant that we should pass ev as a
parameter to plugin_vcpu_cb__discon. This we can obviously do.

Regards,
Julian
Re: [PATCH v5 03/25] plugins: add hooks for new discontinuity related callbacks
Posted by Richard Henderson 5 months, 3 weeks ago
On 5/26/25 09:08, Julian Ganz wrote:
> May 25, 2025 at 10:56 PM, "Julian Ganz" wrote:
>> Richard Henderson wrote:
>>> On 5/19/25 16:19, Julian Ganz wrote:
>>>   +QEMU_DISABLE_CFI
>>>   +static void plugin_vcpu_cb__discon(CPUState *cpu,
>>>   + enum qemu_plugin_discon_type type,
>>>   + uint64_t from)
>>>   +{
>>>   + struct qemu_plugin_cb *cb, *next;
>>>   + enum qemu_plugin_event ev;
>>>   + uint64_t to = cpu->cc->get_pc(cpu);
>>>   +
>>>   + if (cpu->cpu_index < plugin.num_vcpus) {
>>>   + switch (type) {
>>>   + case QEMU_PLUGIN_DISCON_INTERRUPT:
>>>   + ev = QEMU_PLUGIN_EV_VCPU_INTERRUPT;
>>>   + break;
>>>   + case QEMU_PLUGIN_DISCON_EXCEPTION:
>>>   + ev = QEMU_PLUGIN_EV_VCPU_EXCEPTION;
>>>   + break;
>>>   + case QEMU_PLUGIN_DISCON_HOSTCALL:
>>>   + ev = QEMU_PLUGIN_EV_VCPU_HOSTCALL;
>>>   + break;
>>>   
>>>   No point passing in QEMU_PLUGIN_DISCON_* only to covert it to QEMU_PLUGIN_EV_*.
>>>
>> It easily looks that way, and I myself stubled upon this at least one or two
>> times, but `type` is the enum we pass to the callback a few lines down and part
>> of the public plugin API. `ev` on the other hand is the offset in the `cb_list`.
>> So some translation is neccessary, unfortunately.
> 
> I just realized that you probably meant that we should pass ev as a
> parameter to plugin_vcpu_cb__discon. This we can obviously do.

Yes, that's what I meant.

r~