Re: [PATCH] qapi, machine-qmp-cmds.c: query-accelerator support

["Daniel P. Berrangé" <berrange@redhat.com>]

Markus: QAPI design Qs for you at the bottom

On Wed, Sep 25, 2024 at 10:19:33AM -0300, Daniel Henrique Barboza wrote:
> 
> 
> On 9/19/24 9:22 AM, Daniel P. Berrangé wrote:
> > On Thu, Sep 19, 2024 at 08:20:56AM -0300, Daniel Henrique Barboza wrote:
> > > Add a QMP command that shows all specific properties of the current
> > > accelerator in use.
> > 
> > Why do we need to expose /everything/ ?
> 
> I wouldn't mind pick and choose advertised properties for the accelerators
> like we do with other APIs.
> 
> This would mean that each arch should choose what to advertise or not, given that
> some accelerator properties might be relevant just for some archs. The API would
> be implemented by each arch individually.

Well with qemu-system-any we might get multiple arches reporting
info in the same binary, so we'll need to fan out to fill in the
per-arch info, after doing a common base.

Hmmm, i wonder if qemu-system-any will support mixing KVM and TCG ?
ie KVM for the host native accelerator, combined with TCG for the
foreign archs ??? Hopefully not !

> > > This can be used as a complement of other APIs like query-machines and
> > > query-cpu-model-expansion, allowing management to get a more complete
> > > picture of the running QEMU process.
> > 
> > query-machines doesn't return every single QOM property, just
> > a hand selected set of information pieces.
> > 
> > The query-cpu-model-expansion does return everything, but I
> > consider that command to be bad design, as it doesn't distinguish
> > between hardware CPU features, and QEMU QOM properties
> > 
> > > 
> > > This is the output with a x86_64 TCG guest:
> > > 
> > > $ ./build/qemu-system-x86_64 -S  -display none -accel tcg -qmp tcp:localhost:1234,server,wait=off
> > > 
> > > $ ./scripts/qmp/qmp-shell localhost:1234
> > > Welcome to the QMP low-level shell!
> > > Connected to QEMU 9.1.50
> > > 
> > > (QEMU) query-accelerator
> > > {"return": {"name": "tcg", "props": {"one-insn-per-tb": false, "thread": "multi", "tb-size": 0, "split-wx": false, "type": "tcg-accel"}}}
> > > 
> > > And for a x86_64 KVM guest:
> > > 
> > > $ ./build/qemu-system-x86_64 -S  -display none -accel kvm -qmp tcp:localhost:1234,server,wait=off
> > > 
> > > $ ./scripts/qmp/qmp-shell localhost:1234
> > > Welcome to the QMP low-level shell!
> > > Connected to QEMU 9.1.50
> > > 
> > > (QEMU) query-accelerator
> > > {"return": {"name": "KVM", "props": {"mem-container-smram[0]": "", "xen-gnttab-max-frames": 64, "device": "", "xen-version": 0, "mem-smram[0]": "", "notify-window": 0, "dirty-ring-size": 0, "kvm-shadow-mem": -1, "type": "kvm-accel", "notify-vmexit": "run", "xen-evtchn-max-pirq": 256}}}
> > > 
> > > Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> > > ---
> > >   hw/core/machine-qmp-cmds.c | 34 ++++++++++++++++++++++++++++++++++
> > >   qapi/machine.json          | 27 +++++++++++++++++++++++++++
> > >   2 files changed, 61 insertions(+)
> > > 
> > > diff --git a/hw/core/machine-qmp-cmds.c b/hw/core/machine-qmp-cmds.c
> > > index 130217da8f..eac803bf36 100644
> > > --- a/hw/core/machine-qmp-cmds.c
> > > +++ b/hw/core/machine-qmp-cmds.c
> > 
> > > +AccelInfo *qmp_query_accelerator(Error **errp)
> > > +{
> > > +    AccelState *accel = current_accel();
> > > +    AccelClass *acc = ACCEL_GET_CLASS(accel);
> > > +    AccelInfo *info = g_new0(AccelInfo, 1);
> > > +    QDict *qdict_out = qdict_new();
> > > +    ObjectPropertyIterator iter;
> > > +    ObjectProperty *prop;
> > > +
> > > +    info->name = g_strdup(acc->name);
> > > +
> > > +    object_property_iter_init(&iter, OBJECT(accel));
> > > +    while ((prop = object_property_iter_next(&iter))) {
> > > +        QObject *value;
> > > +
> > > +        if (!prop->get) {
> > > +            continue;
> > > +        }
> > > +
> > > +        value = object_property_get_qobject(OBJECT(accel), prop->name,
> > > +                                                  &error_abort);
> > > +        qdict_put_obj(qdict_out, prop->name, value);
> > > +    }
> > 
> > I'm not at all convinced trhat we should be exposing every single
> > QOM property on the accelerator class as public QMP data
> > 
> > > +
> > > +    if (!qdict_size(qdict_out)) {
> > > +        qobject_unref(qdict_out);
> > > +    } else {
> > > +        info->props = QOBJECT(qdict_out);
> > > +    }
> > > +
> > > +    return info;
> > > +}
> > > diff --git a/qapi/machine.json b/qapi/machine.json
> > > index a6b8795b09..d0d527d1eb 100644
> > > --- a/qapi/machine.json
> > > +++ b/qapi/machine.json
> > > @@ -1898,3 +1898,30 @@
> > >   { 'command': 'x-query-interrupt-controllers',
> > >     'returns': 'HumanReadableText',
> > >     'features': [ 'unstable' ]}
> > > +
> > > +##
> > > +# @AccelInfo:
> > > +#
> > > +# Information about the current accelerator.
> > > +#
> > > +# @name: the name of the current accelerator being used
> > > +#
> > > +# @props: a dictionary of the accelerator properties
> > > +#
> > > +# Since: 9.2
> > > +##
> > > +{ 'struct': 'AccelInfo',
> > > +  'data': { 'name': 'str',
> > > +            '*props': 'any' } }
> > 
> > This is way too open ended. IMHO ideally we would never add more
> > instances of the 'any' type, as it has many downsides
> > 
> >   - zero documentation about what is available
> >   - no version info about when each prop was introduced
> >   - no ability to tag fields as deprecated
> > 
> > For this new API, IMHO 'name' should be an enumeration of the
> > accelerator types, and thenm 'props' should be a discrinated
> > union of accelerator specific structs
> 
> We have accelerator properties that differs from arch to arch, e.g. x86 has properties like
> notify-vmexit, declared in kvm_arch_accel_class_init() from target/i386/kvm/kvm.c, that no
> other arch has access to. RISC-V has its own share of these properties too.
> 
> Is it possible to declare specific structs based on arch for the API? In a quick glance
> it seems like we're doing something like that with query-cpus-fast, where s390x has
> additional properties that are exposed.

To allow for qemu-system-any, which will eventually have multiple arches in
one, I guess we'll need multiple levels of nesting. Perhaps  something like
this:

  { 'enum': 'AccelType',
    'data': ['tcg', 'kvm', ....] }

  { 'union': 'AccelInfo',
    'type': 'AccelType',
    'data': {
        'tcg': 'AccelInfoTCG',
	'kvm': 'AccelInfoKVM',
    } }

  { 'struct': 'AccelInfoTCGX86',
    'data': {
        'notify-vmexit': ...
    } }

  { 'struct': 'AccelInfoTCGArch',
    'data': {
       'x86': 'AccelInfoTCGX86',
       'riscv': 'AccelInfoTCGRiscV',
       ...etc...
    }

  { 'struct': 'AccelInfoTCG',
    'data': {
         ...non-arch specific fields...,
	 'arch': 'AccelInfoTCGArch',
    } }

 ...equiv AccelInfoKVM* structs....

Markus:  any other/better ideas ?

> > > +
> > > +##
> > > +# @query-accelerator:
> > > +#
> > > +# Shows information about the accelerator in use.
> > > +#
> > > +# Returns: a CpuModelExpansionInfo describing the expanded CPU model
> > > +#
> > > +# Since: 9.2
> > > +##
> > > +{ 'command': 'query-accelerator',
> > > +  'returns': 'AccelInfo' }
> > > -- 

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