Daniel P. Berrangé <berra...@redhat.com> writes:
> 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 <dbarb...@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
Yes, 'any' is best avoided in stable interfaces.
>> > 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.
Yes. Schema pattern:
{ 'union: ...,
'base': {
'target': 'SysEmuTarget',
... target-independent data ... }
'data': {
T: { ... data for target T ... }
... } }
On the wire:
{"target": T,
... target-independent data ...
... data for target T}
This is for returning info on a single target.
Typically, a qemu-system-T will only ever return branch T. The other
branches are dead code. Tolerable.
With qemu-system-any, which provides multiple (eventually all, we hope)
targets, the union still has data for just one target. To get data for
multiple targets, we'd have to return several objects.
Fine for query-cpus-fast: it returns a CpuInfoFast per CPU, and each of
them uses the branch T appropriate for the CPU.
> To allow for qemu-system-any, which will eventually have multiple arches in
> one,
Actually, qemu-system-any can work almost exactly like the existing
qemu-system-T, until we mix in heterogeneous machines.
Without heterogeneous machines, qemu-system-any's target T gets fixed at
some point. From that point on, no difference to qemu-system-T.
The "almost" is for running commands before that point.
This applies to all commands that take the target as implicit input. I
believe several exist.
How accelerators will work for heterogeneous machines is not clear to
me. Until it is, I can't really advise on how to design accelerator
information for them.
> 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',
> } }
Here, you want to return data for all targets, not just one. Instead of
a sum type (union branches), you use a product type (struct members).
Feels fair.
> ...equiv AccelInfoKVM* structs....
The only target that can actually work is the one matching the host
hardware. Perhaps we want to return just that.
> Markus: any other/better ideas ?
I guess there are other ways to skin this cat. But I think there are
questions to ask before we delve deeper:
1. query-accelerator use cases? What accelerator information exactly do
we need to provide for them?
2. How fancy do we want to get for homogeneous qemu-system-any before
the target gets fixed?
3. How will accelerators work for heterogeneous qemu-system-any?
[...]
