[PATCH v3 24/68] accel/system: Add 'info accel' on human monitor

Philippe Mathieu-Daudé posted 68 patches 4 months, 2 weeks ago
Maintainers: Richard Henderson <richard.henderson@linaro.org>, Paolo Bonzini <pbonzini@redhat.com>, Cameron Esfahani <dirty@apple.com>, Roman Bolshakov <rbolshakov@ddn.com>, Phil Dennis-Jordan <phil@philjordan.eu>, Fabiano Rosas <farosas@suse.de>, Laurent Vivier <lvivier@redhat.com>, Stefano Stabellini <sstabellini@kernel.org>, Anthony PERARD <anthony@xenproject.org>, Paul Durrant <paul@xen.org>, "Edgar E. Iglesias" <edgar.iglesias@gmail.com>, Warner Losh <imp@bsdimp.com>, Kyle Evans <kevans@freebsd.org>, "Alex Bennée" <alex.bennee@linaro.org>, "Philippe Mathieu-Daudé" <philmd@linaro.org>, "Dr. David Alan Gilbert" <dave@treblig.org>, Eduardo Habkost <eduardo@habkost.net>, Marcel Apfelbaum <marcel.apfelbaum@gmail.com>, Yanan Wang <wangyanan55@huawei.com>, Zhao Liu <zhao1.liu@intel.com>, Reinoud Zandijk <reinoud@netbsd.org>, Sunil Muthuswamy <sunilmut@microsoft.com>, Eric Blake <eblake@redhat.com>, Markus Armbruster <armbru@redhat.com>, Peter Xu <peterx@redhat.com>, David Hildenbrand <david@redhat.com>, Peter Maydell <peter.maydell@linaro.org>, Alexander Graf <agraf@csgraf.de>
There is a newer version of this series
[PATCH v3 24/68] accel/system: Add 'info accel' on human monitor
Posted by Philippe Mathieu-Daudé 4 months, 2 weeks ago
'info accel' dispatches to the AccelOpsClass::get_stats()
and get_vcpu_stats() handlers.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Acked-by: Richard Henderson <richard.henderson@linaro.org>
---
 include/qemu/accel.h       |  1 +
 include/system/accel-ops.h |  2 ++
 accel/accel-system.c       | 28 ++++++++++++++++++++++++++++
 hmp-commands-info.hx       | 12 ++++++++++++
 4 files changed, 43 insertions(+)

diff --git a/include/qemu/accel.h b/include/qemu/accel.h
index 065de80a87b..80bfe3c4d0f 100644
--- a/include/qemu/accel.h
+++ b/include/qemu/accel.h
@@ -41,6 +41,7 @@ typedef struct AccelClass {
     AccelOpsClass *ops;
 
     int (*init_machine)(AccelState *as, MachineState *ms);
+    void (*get_stats)(AccelState *as, GString *buf);
 
     /* system related hooks */
     void (*setup_post)(AccelState *as);
diff --git a/include/system/accel-ops.h b/include/system/accel-ops.h
index af54302409c..106ff56d880 100644
--- a/include/system/accel-ops.h
+++ b/include/system/accel-ops.h
@@ -50,6 +50,8 @@ struct AccelOpsClass {
 
     void (*handle_interrupt)(CPUState *cpu, int mask);
 
+    void (*get_vcpu_stats)(CPUState *cpu, GString *buf);
+
     /**
      * @get_virtual_clock: fetch virtual clock
      * @set_virtual_clock: set virtual clock
diff --git a/accel/accel-system.c b/accel/accel-system.c
index 11ba8e24d60..918900a0a8a 100644
--- a/accel/accel-system.c
+++ b/accel/accel-system.c
@@ -25,6 +25,8 @@
 
 #include "qemu/osdep.h"
 #include "qemu/accel.h"
+#include "qapi/type-helpers.h"
+#include "monitor/monitor.h"
 #include "hw/boards.h"
 #include "system/accel-ops.h"
 #include "system/cpus.h"
@@ -81,6 +83,26 @@ bool cpus_are_resettable(void)
     return true;
 }
 
+static HumanReadableText *hmp_info_accel(Error **errp)
+{
+    AccelState *accel = current_accel();
+    AccelClass *acc = ACCEL_GET_CLASS(accel);
+    g_autoptr(GString) buf = g_string_new("");
+
+    if (acc->get_stats) {
+        acc->get_stats(accel, buf);
+    }
+    if (acc->ops->get_vcpu_stats) {
+        CPUState *cpu;
+
+        CPU_FOREACH(cpu) {
+            acc->ops->get_vcpu_stats(cpu, buf);
+        }
+    }
+
+    return human_readable_text_from_str(buf);
+}
+
 /* initialize the arch-independent accel operation interfaces */
 void accel_init_ops_interfaces(AccelClass *ac)
 {
@@ -111,11 +133,17 @@ void accel_init_ops_interfaces(AccelClass *ac)
     cpus_register_accel(ops);
 }
 
+static void accel_ops_class_init(ObjectClass *oc, const void *data)
+{
+    monitor_register_hmp_info_hrt("accel", hmp_info_accel);
+}
+
 static const TypeInfo accel_ops_type_info = {
     .name = TYPE_ACCEL_OPS,
     .parent = TYPE_OBJECT,
     .abstract = true,
     .class_size = sizeof(AccelOpsClass),
+    .class_init = accel_ops_class_init,
 };
 
 static void accel_system_register_types(void)
diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx
index 639a450ee51..0496be6abfb 100644
--- a/hmp-commands-info.hx
+++ b/hmp-commands-info.hx
@@ -281,6 +281,18 @@ ERST
         .cmd        = hmp_info_sync_profile,
     },
 
+    {
+        .name       = "accel",
+        .args_type  = "",
+        .params     = "",
+        .help       = "show accelerator info",
+    },
+
+SRST
+  ``info accel``
+    Show accelerator info.
+ERST
+
 SRST
   ``info sync-profile [-m|-n]`` [*max*]
     Show synchronization profiling info, up to *max* entries (default: 10),
-- 
2.49.0


Re: [PATCH v3 24/68] accel/system: Add 'info accel' on human monitor
Posted by Markus Armbruster 4 months, 2 weeks ago
Philippe Mathieu-Daudé <philmd@linaro.org> writes:

> 'info accel' dispatches to the AccelOpsClass::get_stats()
> and get_vcpu_stats() handlers.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Acked-by: Richard Henderson <richard.henderson@linaro.org>

Standard question for new HMP commands that don't wrap around QMP
commands: why is the functionality not useful in QMP?
Re: [PATCH v3 24/68] accel/system: Add 'info accel' on human monitor
Posted by Philippe Mathieu-Daudé 4 months, 2 weeks ago
On 2/7/25 06:58, Markus Armbruster wrote:
> Philippe Mathieu-Daudé <philmd@linaro.org> writes:
> 
>> 'info accel' dispatches to the AccelOpsClass::get_stats()
>> and get_vcpu_stats() handlers.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> Acked-by: Richard Henderson <richard.henderson@linaro.org>
> 
> Standard question for new HMP commands that don't wrap around QMP
> commands: why is the functionality not useful in QMP?

So far the sole use of this command is to prove the 'split accel'
works by using it in a test:
https://lore.kernel.org/qemu-devel/20250620172751.94231-43-philmd@linaro.org/

Is it worth overloading QAPI and its documentation, and overload
the QMP command set (even if prefixing with experimental / hidden 'x-')?

If so, I don't mind implementing yet another "embedded plain HMP string
to QMP command" in v4, or directly export each debug statistical value
via QAPI.

Regards,

Phil.

Re: [PATCH v3 24/68] accel/system: Add 'info accel' on human monitor
Posted by Markus Armbruster 4 months, 2 weeks ago
Philippe Mathieu-Daudé <philmd@linaro.org> writes:

> On 2/7/25 06:58, Markus Armbruster wrote:
>> Philippe Mathieu-Daudé <philmd@linaro.org> writes:
>> 
>>> 'info accel' dispatches to the AccelOpsClass::get_stats()
>>> and get_vcpu_stats() handlers.
>>>
>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>> Acked-by: Richard Henderson <richard.henderson@linaro.org>
>> 
>> Standard question for new HMP commands that don't wrap around QMP
>> commands: why is the functionality not useful in QMP?
>
> So far the sole use of this command is to prove the 'split accel'
> works by using it in a test:
> https://lore.kernel.org/qemu-devel/20250620172751.94231-43-philmd@linaro.org/

Different series?  Best to add new interfaces together with their uses.

> Is it worth overloading QAPI and its documentation, and overload
> the QMP command set (even if prefixing with experimental / hidden 'x-')?
>
> If so, I don't mind implementing yet another "embedded plain HMP string
> to QMP command" in v4, or directly export each debug statistical value
> via QAPI.

Whatever the reasons for implementing something in HMP only may be, the
commit message should state them.

"Just for testing" is a valid reason for HMP-only.  It's not obvious
from the command documentation or code, though.
Re: [PATCH v3 24/68] accel/system: Add 'info accel' on human monitor
Posted by Daniel P. Berrangé 4 months, 2 weeks ago
On Wed, Jul 02, 2025 at 02:00:01PM +0200, Markus Armbruster wrote:
> Philippe Mathieu-Daudé <philmd@linaro.org> writes:
> 
> > On 2/7/25 06:58, Markus Armbruster wrote:
> >> Philippe Mathieu-Daudé <philmd@linaro.org> writes:
> >> 
> >>> 'info accel' dispatches to the AccelOpsClass::get_stats()
> >>> and get_vcpu_stats() handlers.
> >>>
> >>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> >>> Acked-by: Richard Henderson <richard.henderson@linaro.org>
> >> 
> >> Standard question for new HMP commands that don't wrap around QMP
> >> commands: why is the functionality not useful in QMP?
> >
> > So far the sole use of this command is to prove the 'split accel'
> > works by using it in a test:
> > https://lore.kernel.org/qemu-devel/20250620172751.94231-43-philmd@linaro.org/
> 
> Different series?  Best to add new interfaces together with their uses.
> 
> > Is it worth overloading QAPI and its documentation, and overload
> > the QMP command set (even if prefixing with experimental / hidden 'x-')?
> >
> > If so, I don't mind implementing yet another "embedded plain HMP string
> > to QMP command" in v4, or directly export each debug statistical value
> > via QAPI.
> 
> Whatever the reasons for implementing something in HMP only may be, the
> commit message should state them.
> 
> "Just for testing" is a valid reason for HMP-only.  It's not obvious
> from the command documentation or code, though.

IMHO there is no valid reason for continuing to add HMP-only commands
which are not backed by QMP.

A test case can easily run QMP commands, and we accept arbitrary QMP
commands with the 'x-' prefix and experimental tag, to serve as the
basis for the HMP command printing human readable text. So if anything
I'd suggest exposing a QMP command and not bothering with HMP at all.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|


Re: [PATCH v3 24/68] accel/system: Add 'info accel' on human monitor
Posted by Philippe Mathieu-Daudé 4 months, 2 weeks ago
On 2/7/25 14:13, Daniel P. Berrangé wrote:
> On Wed, Jul 02, 2025 at 02:00:01PM +0200, Markus Armbruster wrote:
>> Philippe Mathieu-Daudé <philmd@linaro.org> writes:
>>
>>> On 2/7/25 06:58, Markus Armbruster wrote:

>>>> Standard question for new HMP commands that don't wrap around QMP
>>>> commands: why is the functionality not useful in QMP?
>>>
>>> So far the sole use of this command is to prove the 'split accel'
>>> works by using it in a test:
>>> https://lore.kernel.org/qemu-devel/20250620172751.94231-43-philmd@linaro.org/
>>
>> Different series?  Best to add new interfaces together with their uses.
>>
>>> Is it worth overloading QAPI and its documentation, and overload
>>> the QMP command set (even if prefixing with experimental / hidden 'x-')?
>>>
>>> If so, I don't mind implementing yet another "embedded plain HMP string
>>> to QMP command" in v4, or directly export each debug statistical value
>>> via QAPI.
>>
>> Whatever the reasons for implementing something in HMP only may be, the
>> commit message should state them.
>>
>> "Just for testing" is a valid reason for HMP-only.  It's not obvious
>> from the command documentation or code, though.
> 
> IMHO there is no valid reason for continuing to add HMP-only commands
> which are not backed by QMP.
> 
> A test case can easily run QMP commands, and we accept arbitrary QMP
> commands with the 'x-' prefix and experimental tag, to serve as the
> basis for the HMP command printing human readable text. So if anything
> I'd suggest exposing a QMP command and not bothering with HMP at all.

Fine.