Re: [Qemu-devel] [RFC PATCH v2 12/15] cpu-model/s390: Extend QMP command query-cpu-definitions

Michael Mueller
2015-02-18T09:29:29Z

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On Tue, 17 Feb 2015 11:09:34 -0700
Eric Blake <eblake@redhat.com> wrote:

> On 02/17/2015 07:24 AM, Michael Mueller wrote:
> > This patch implements the QMP command 'query-cpu-definitions' in the S390
> > context. The command returns a in terms of machine release date descending
> > sorted list of cpu model names in the current host context.
>
> returns a list of cpu model names sorted by descending release dates.
>
> Does guaranteeing the sorting as part of the interface really matter, or
> would it be better to just return the list with no documented sorting
> (where callers treat it as unsorted)?

Yep, that is an implementation detail and I don't depend on that. If a sequence would be required
one cold model a field named "order". But as said, I don't require that and will update the
comment by dropping the "sorted list" part.

>
> > A consumer may
> > successfully request each listed cpu model as long for a given accelerator
> > this model is runnable.
> >
> > Thy QMP type AccelCpuModelInfo is introduced and the type CpuDefinitionInfo
> > is extended by the optional field 'accelerators'. It contains a list of named
> > accelerators and some indication whether the associated cpu model is runnable
> > or the default cpu model. The default cpu model is used either no specific cpu
> > was requested during QEMU startup or the cpu model with named 'host'.
> >
> > request:
> > {"execute": "query-cpu-definitions"}
> >
> > answer:
> > {"return":
> > [{"name":"2964-ga1","accelerators":[{"name":"kvm","runnable":false,"default":false}]},
> > {"name":"2828-ga1","accelerators":[{"name":"kvm","runnable":false,"default":false}]},
> > {"name":"2827-ga2","accelerators":[{"name":"kvm","runnable":true,"default":true}]},
> > {"name":"2827-ga1","accelerators":[{"name":"kvm","runnable":true,"default":false}]},
> > {"name":"2818-ga1","accelerators":[{"name":"kvm","runnable":true,"default":false}]},
> > ...
> > {"name":"2064-ga1","accelerators":[{"runnable":true,"name":"kvm","default":false}]}
> > ]
> > }
> >
>
> Looks okay from an interface perspective.
>
> > Signed-off-by: Michael Mueller <mimu@linux.vnet.ibm.com>
> > ---
> > qapi-schema.json | 21 +++++++++-
> > target-s390x/cpu-models.c | 15 +++++++
> > target-s390x/cpu-models.h | 1 +
> > target-s390x/cpu.c | 100 +++++++++++++++++++++++++++++++++++++++++++---
> > 4 files changed, 130 insertions(+), 7 deletions(-)
> >
> > diff --git a/qapi-schema.json b/qapi-schema.json
> > index 9431fc2..a5d38ae 100644
> > --- a/qapi-schema.json
> > +++ b/qapi-schema.json
> > @@ -2485,16 +2485,35 @@
> > 'data': ['qtest', 'tcg', 'kvm', 'xen' ] }
> >
> > ##
> > +# @AccelCpuModelInfo:
> > +#
> > +# Accelerator specific CPU model data
> > +#
> > +# @id: the accelerator id
> > +#
>
> There is no 'id' field below, did you mean 'name'?

I did rename that one time to often :-), will s/id/name/g ...

>
> > +# @default: cpu model for 'host'
> > +#
> > +# @runnable: cpu model can be activated on hosting machine
> > +#
> > +# Since: 2.3.0
> > +#
> > +##
> > +{ 'type': 'AccelCpuModelInfo',
> > + 'data': { 'name': 'AccelId', 'default': 'bool', 'runnable': 'bool' } }
> > +
> > +##
> > # @CpuDefinitionInfo:
> > #
> > # Virtual CPU definition.
> > #
> > # @name: the name of the CPU definition
> > #
> > +# @accelerators: #optional cpu model offered per accelerator (since 2.3.0)
> > +#
>
> Must the field be optional, or will we always provide it? Since this is
> an output-only field, it is okay for back-compat to make the new field
> unconditional.

It will be always provided when an accelerator supports cpu models and implements the
probing mode. As I'm currently adopting it for s390/kvm only, I can't enforce it for all
other arch/accelerator combinations as it is an extension of an existing command...

Thanks
Michael

>

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1

iQIcBAEBAgAGBQJU5Ft5AAoJELPcPLQSJsKQ520P/A3EQqyY7buhBZWwVDQcA49J
FSzjyEt2JAJmZAlMFMaxbVDwJcm5PXEbCHR1+NuDXuyEYsPxqG7TxvP+3yLR2lLa
QbucHGd9M789Tg0hy2YPifIIB93LY5Kb3SNxhL52olyIrnsovHoBCbboMlmmKTk7
KyH6q2KXddjWtZbHy9WGQY91r56yMdsbfIxbYMJuJbJ/9Hr0lh0xB6W77mL8GIrV
dbURjaZXwgoGecVAlyEQcpInS2fl6XSX7Y2rCAq9Jp/9ZNfSQdOai19Md2cQ1JLi
92qYwgT8XV6bZOHJ1E8xc7+KlJRRH4MvYbWTNCVHEA3ewE5rYkspe92fEj82GZnc
eJV+hjZcq9cNaOF8bvhpjy+9zW8WdrwsQKbXNEeJDzDmnYhaaKcdKRa6qUDwYLQW
eg+TAfO+G9YNYfEpJH5okCo3t7elpYlkdcOvNtj1X2gFAmpjkbeVOUWSD1JmtJ5u
+s3XUagvQdzoIdpsziob0NEpLU62QFcqAa3ZNSY/FE7itTMnZs2+rvbYUxGyRjqz
BbEwPDoAMcFCO6CdK/hoZxV8RbCRH+MoDy+oLKXbxsF1rJcFfe5VGUQBTbYJUNEO
l87sUJBw5AqcU5/VpnuQn/unVCupQxour63T6WxzobvFT+rpMIR8mUQXaAEe9RfJ
8G8A5VXn8C4zrC2Am5G5
=cpfo
-----END PGP SIGNATURE-----