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

[Michael Mueller <mimu@linux.vnet.ibm.com>]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=US-ASCII, Size: 4915 bytes --]

-----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-----
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥