Re: [Qemu-devel] [libvirt] [PATCH 7/9] qmp: Add runnability information to query-cpu-definitions

[Markus Armbruster]

Eduardo Habkost <address@hidden> writes:

> On Wed, May 11, 2016 at 09:11:33AM +0200, Markus Armbruster wrote:
>> Eduardo Habkost <address@hidden> writes:
>> 
>> > On Tue, May 10, 2016 at 10:23:16AM +0200, Markus Armbruster wrote:
>> >> Eduardo Habkost <address@hidden> writes:
>> >> 
>> >> > On Mon, May 09, 2016 at 09:20:15AM -0600, Eric Blake wrote:
>> >> >> On 05/06/2016 12:11 PM, Eduardo Habkost wrote:
>> >> >> > Extend query-cpu-definitions schema to allow it to return two new
>> >> >> > optional fields: "runnable" and "unavailable-features".
>> >> >> > "runnable" will tell if the CPU model can be run in the current
>> >> >> > host. "unavailable-features" will contain a list of CPU
>> >> >> > properties that are preventing the CPU model from running in the
>> >> >> > current host.
>> >> >> > 
>> >> >> > Cc: David Hildenbrand <address@hidden>
>> >> >> > Cc: Michael Mueller <address@hidden>
>> >> >> > Cc: Christian Borntraeger <address@hidden>
>> >> >> > Cc: Cornelia Huck <address@hidden>
>> >> >> > Cc: Jiri Denemark <address@hidden>
>> >> >> > Cc: address@hidden
>> >> >> > Signed-off-by: Eduardo Habkost <address@hidden>
>> >> >> > ---
>> >> >> >  qapi-schema.json | 10 +++++++++-
>> >> >> >  1 file changed, 9 insertions(+), 1 deletion(-)
>> >> >> > 
>> >> >> > diff --git a/qapi-schema.json b/qapi-schema.json
>> >> >> > index 54634c4..450e6e7 100644
>> >> >> > --- a/qapi-schema.json
>> >> >> > +++ b/qapi-schema.json
>> >> >> > @@ -2948,11 +2948,19 @@
>> >> >> >  # Virtual CPU definition.
>> >> >> >  #
>> >> >> >  # @name: the name of the CPU definition
>> >> >> > +# @runnable: true if the CPU model is runnable using the current
>> >> >> > +#            machine and accelerator. Optional. Since 2.6.
>> >> >> 
>> >> >> You've missed 2.6.  Also, the typical spelling for a per-member
>> >> >> designation is '(since 2.7)', not 'Since 2.7'
>> >> >
>> >> > Oops! I meant 2.7, and I didn't notice that it was not using the
>> >> > typical format. I will fix it, thanks.
>> >> >
>> >> >> 
>> >> >> Why is it optional? Would it hurt to always be present in qemu new
>> >> >> enough to understand why it is needed?
>> >> >
>> >> > It is optional because not all architectures will return the
>> >> > field. This series implements it only for x86.
>> >> 
>> >> Its documentation seems to suggest missing runnable has the same meaning
>> >> as runnable: false.  Is that correct?
>> >
>> > No, it means the architecture code doesn't implement the feature
>> > yet and we don't know if the CPU model is runnable or not.
>> >
>> > The day we implement the new field in all architectures, we can
>> > stop making it optional.
>> 
>> Please clarify that in the docs.  Here's my try:
>> 
>> # @runnable: #optional whether the CPU model us usable with the current
>> # machine and accelerator, only present if we know (since 2.6)
>
> Updated to:
>
> ##
> # @CpuDefinitionInfo:
> #
> # Virtual CPU definition.
> #
> # @name: the name of the CPU definition
> # @runnable: #optional. whether the CPU model us usable with the
> #            current machine and accelerator. Omitted if we don't
> #            know the answer. (since 2.7)
> # @unavailable-features: List of attributes that prevent the CPU

Unless you drop the * sigil from '*unavailable-features', you need to
insert #optional after the colon.

> #                        model from running in the current host.
> #                        (since 2.7)
> #
> # @unavailable-features is a list of QOM property names that
> # represent CPU model attributes that prevent the CPU from running.
> # If the QOM property is read-only, that means the CPU model can
> # never run in the current host. If the property is read-write, it
> # means that it MAY be possible to run the CPU model in the current
> # host if that property is changed. Management software can use it
> # as hints to suggest or choose an alternative for the user, or
> # just to generate meaningful error messages explaining why the CPU
> # model can't be used.
> #
> # Since: 1.2.0
> ##

Better.

Next issue: how @runnable and @unavailable-features are related isn't
fully documented.  Here's my guess:

Combinations possible?            @runnable
@unavailable-features       absent  false  true
absent                         yes      ?     ?
present, empty                   ?      ?     ?
present, non-empty               ?    yes    no

The '?' need to be answered, too.