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 <[email protected]>
> > > ---
> > > 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 :|
