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

[David Hildenbrand]

> It is not exactly a special case (it is a read-only property like
> any other), but it's worth mentioning. I will change it to:
> 
> # If the QOM property is read-only, that means there's no known
> # way to make the CPU model run in the current host. If
> # absolutely no extra information will be returned to explain why
> # the CPU model is not runnable, implementations may simply
> # return "type" as the property name.
> 
> >   
> > > +# If @unavailable-features is an empty list, the CPU model is
> > > +# runnable using the current host and machine-type.
> > > +# If @unavailable-features is not present, runnability
> > > +# information for the CPU model is not available.
> > >  #
> > >  # Since: 1.2.0
> > >  ##  
> > 
> > I'm happy with this interface.  Thanks!  
> 
> Thanks!
> 

Yes, sounds also good to me. For "hw generation too new" and similar errors
we will simply return "type". Thanks.

David

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

[Eduardo Habkost]

On Tue, May 31, 2016 at 03:24:50PM +0200, Markus Armbruster wrote:
> Eduardo Habkost  writes:
> 
> > On Mon, May 30, 2016 at 11:33:38AM +0200, Markus Armbruster wrote:
> [...]
> >> The new members encode an answer to the question whether a certain CPU
> >> usable with the current machine an accelerator, and if no, why.
> >> The possible answers are:
> >> 
> >> (1) Don't know.
> >> (2) Yes.
> >> (3) No, but we can't say why.
> >> (4) No, and here's a list of reasons.
> >> 
> >> The two "dunno" answers (1) and (3) exist so we don't have to boil the
> >> CPU ocean now.
> >> 
> >> Without them, the natural solution is a single member, where (4) is
> >> encoded as nonempty list, and (2) could be encoded as empty list or
> >> absent.
> >> 
> >> Now let me try to fit in (1) and (3).
> >> 
> >> The obvious way to do (1) is absent.  So let's use empty list for (2).
> >> 
> >> That leaves (3).  I think the simplest solution that could possibly work
> >> is to treat it as a special "dunno" reason: encode it just like (4), but
> >> with a special "dunno" list element.  I'd use the empty string.
> >> 
> >> Could even be used if we need to distinguish
> >> 
> >> (4a) No, and here's the *complete* list of reasons.
> >> (4b) No, and here's a possibly incomplete list of reasons.
> >> 
> >> For (4b), include the "dunno" element with the others.
> >> 
> >> Unlike the proposed solution, this one doesn't leave interface crud
> >> behind if we succeed in getting rid of (1) and (3):
> >> 
> >> * When (1) goes away, the single member becomes mandatory.
> >> 
> >> * When (3) goes away, the special "dunno" list element no longer occurs.
> >
> > I like your suggestion.
> >
> > I suggest "type" as the "dunno" element. It would keep the
> > existing "QOM property name" semantics, and it would just mean
> > "sorry, the only advice we can currently give you is to choose a
> > different CPU type". It even matches the previous documentation I
> > sent describing the meaning of read-only property names.
> >
> > Rewriting the docs again:
> >
> >  # 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: #optional List of attributes that prevent
> > +# @unavailable-features: #optional List of properties that prevent
> >  #the CPU model from running in the current
> > -#host. Present only if @runnable is false.
> > -#(since 2.7)
> > +#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
> > +# If the QOM property is read-only, that means there's no known
> > +# way to make the CPU model 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.
> 
> Should we spell out the special case "type"?

It is not exactly a special case (it is a read-only property like
any other), but it's worth mentioning. I will change it to:

# If the QOM property is read-only, that means there's no known
# way to make the CPU model run in the current host. If
# absolutely no extra information will be returned to explain why
# the CPU model is not runnable, implementations may simply
# return "type" as the property name.

> 
> > +# If @unavailable-features is an empty list, the CPU model is
> > +# runnable using the current host and machine-type.
> > +# If @unavailable-features is not present, runnability
> > +# information for the CPU model is not available.
> >  #
> >  # Since: 1.2.0
> >  ##
> 
> I'm happy with this interface.  Thanks!

Thanks!

-- 
Eduardo

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

[Markus Armbruster]

Eduardo Habkost  writes:

> On Mon, May 30, 2016 at 11:33:38AM +0200, Markus Armbruster wrote:
[...]
>> The new members encode an answer to the question whether a certain CPU
>> usable with the current machine an accelerator, and if no, why.
>> The possible answers are:
>> 
>> (1) Don't know.
>> (2) Yes.
>> (3) No, but we can't say why.
>> (4) No, and here's a list of reasons.
>> 
>> The two "dunno" answers (1) and (3) exist so we don't have to boil the
>> CPU ocean now.
>> 
>> Without them, the natural solution is a single member, where (4) is
>> encoded as nonempty list, and (2) could be encoded as empty list or
>> absent.
>> 
>> Now let me try to fit in (1) and (3).
>> 
>> The obvious way to do (1) is absent.  So let's use empty list for (2).
>> 
>> That leaves (3).  I think the simplest solution that could possibly work
>> is to treat it as a special "dunno" reason: encode it just like (4), but
>> with a special "dunno" list element.  I'd use the empty string.
>> 
>> Could even be used if we need to distinguish
>> 
>> (4a) No, and here's the *complete* list of reasons.
>> (4b) No, and here's a possibly incomplete list of reasons.
>> 
>> For (4b), include the "dunno" element with the others.
>> 
>> Unlike the proposed solution, this one doesn't leave interface crud
>> behind if we succeed in getting rid of (1) and (3):
>> 
>> * When (1) goes away, the single member becomes mandatory.
>> 
>> * When (3) goes away, the special "dunno" list element no longer occurs.
>
> I like your suggestion.
>
> I suggest "type" as the "dunno" element. It would keep the
> existing "QOM property name" semantics, and it would just mean
> "sorry, the only advice we can currently give you is to choose a
> different CPU type". It even matches the previous documentation I
> sent describing the meaning of read-only property names.
>
> Rewriting the docs again:
>
>  # 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: #optional List of attributes that prevent
> +# @unavailable-features: #optional List of properties that prevent
>  #the CPU model from running in the current
> -#host. Present only if @runnable is false.
> -#(since 2.7)
> +#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
> +# If the QOM property is read-only, that means there's no known
> +# way to make the CPU model 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.

Should we spell out the special case "type"?

> +# If @unavailable-features is an empty list, the CPU model is
> +# runnable using the current host and machine-type.
> +# If @unavailable-features is not present, runnability
> +# information for the CPU model is not available.
>  #
>  # Since: 1.2.0
>  ##

I'm happy with this interface.  Thanks!

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

[Eduardo Habkost]

On Mon, May 30, 2016 at 11:33:38AM +0200, Markus Armbruster wrote:
> Eduardo Habkost  writes:
> 
> > Just noticed that I hadn't replied to this yet. Sorry for the
> > long delay!
> >
> > On Thu, May 12, 2016 at 09:46:25AM +0200, Markus Armbruster wrote:
> >> Eduardo Habkost  writes:
> > [...]
> >> > ##
> >> > # @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.
> >
> > Fixed.
> >
> >> 
> >> > #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   ?yesno
> >
> > unavailable-features should be present only if runnable is false.
> > It may be absent or empty if the architecture code still doesn't
> > provide detailed info.
> >
> > Once we have additional architectures implementing the new
> > fields, we can consider requiring unavailable-features to be
> > always present (and non-empty) if runnable is false.
> >
> > In other words:
> >
> > Combinations possible?@runnable
> > @unavailable-features   absent  false  true
> > absent yes  yes[1]  yes
> > present, empty  no  yes[1]   no
> > present, non-empty  no   yes no
> >
> > [1] I would like it to be "no", but I prefer to make it mandatory
> > only after we get some experience with other architectures.
> >
> >
> > I'm making the following changes to the documentation:
> >
> >  # Virtual CPU definition.
> >  #
> >  # @name: the name of the CPU definition
> > -# @runnable: #optional. whether the CPU model us usable with the
> > +# @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
> > -#model from running in the current host.
> > +# @unavailable-features: #optional List of attributes that prevent
> > +#the CPU model from running in the current
> > +#host. Present only if @runnable is false.
> >  #(since 2.7)
> >  #
> >  # @unavailable-features is a list of QOM property names that
> 
> "Present only if @runnable is false" makes me wonder why we need two
> separate optional members tied together with constraints.  I dislike
> such constraints, and avoid them whenever practical.
> 
> The new members encode an answer to the question whether a certain CPU
> usable with the current machine an accelerator, and if no, why.
> The possible answers are:
> 
> (1) Don't know.
> (2) Yes.
> (3) No, but we can't say why.
> (4) No, and here's a list of reasons.
> 
> The two "dunno" answers (1) and (3) exist so we don't have to boil the
> CPU ocean now.
> 
> Without them, the natural solution is a single member, where (4) is
> encoded as nonempty list, and (2) could be encoded as empty list or
> absent.
> 
> Now let me try to fit in (1) and (3).
> 
> The obvious way to do (1) is absent.  So let's use empty list for (2).
> 
> That leaves (3).  I think the simplest solution that could possibly work
> is to treat it as a special "dunno" reason: encode it just like (4), but
> with a special "dunno" list element.  I'd use the empty string.
> 
> Could even be used if we need to distinguish
> 
> (4a) No, and here's the *complete* list of reasons.
> (4b) No, and here's a possibly incomplete list of reasons.
> 
> For (4b), include the 

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

[Markus Armbruster]

Eduardo Habkost  writes:

> Just noticed that I hadn't replied to this yet. Sorry for the
> long delay!
>
> On Thu, May 12, 2016 at 09:46:25AM +0200, Markus Armbruster wrote:
>> Eduardo Habkost  writes:
> [...]
>> > ##
>> > # @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.
>
> Fixed.
>
>> 
>> > #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   ?yesno
>
> unavailable-features should be present only if runnable is false.
> It may be absent or empty if the architecture code still doesn't
> provide detailed info.
>
> Once we have additional architectures implementing the new
> fields, we can consider requiring unavailable-features to be
> always present (and non-empty) if runnable is false.
>
> In other words:
>
> Combinations possible?@runnable
> @unavailable-features   absent  false  true
> absent yes  yes[1]  yes
> present, empty  no  yes[1]   no
> present, non-empty  no   yes no
>
> [1] I would like it to be "no", but I prefer to make it mandatory
> only after we get some experience with other architectures.
>
>
> I'm making the following changes to the documentation:
>
>  # Virtual CPU definition.
>  #
>  # @name: the name of the CPU definition
> -# @runnable: #optional. whether the CPU model us usable with the
> +# @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
> -#model from running in the current host.
> +# @unavailable-features: #optional List of attributes that prevent
> +#the CPU model from running in the current
> +#host. Present only if @runnable is false.
>  #(since 2.7)
>  #
>  # @unavailable-features is a list of QOM property names that

"Present only if @runnable is false" makes me wonder why we need two
separate optional members tied together with constraints.  I dislike
such constraints, and avoid them whenever practical.

The new members encode an answer to the question whether a certain CPU
usable with the current machine an accelerator, and if no, why.
The possible answers are:

(1) Don't know.
(2) Yes.
(3) No, but we can't say why.
(4) No, and here's a list of reasons.

The two "dunno" answers (1) and (3) exist so we don't have to boil the
CPU ocean now.

Without them, the natural solution is a single member, where (4) is
encoded as nonempty list, and (2) could be encoded as empty list or
absent.

Now let me try to fit in (1) and (3).

The obvious way to do (1) is absent.  So let's use empty list for (2).

That leaves (3).  I think the simplest solution that could possibly work
is to treat it as a special "dunno" reason: encode it just like (4), but
with a special "dunno" list element.  I'd use the empty string.

Could even be used if we need to distinguish

(4a) No, and here's the *complete* list of reasons.
(4b) No, and here's a possibly incomplete list of reasons.

For (4b), include the "dunno" element with the others.

Unlike the proposed solution, this one doesn't leave interface crud
behind if we succeed in getting rid of (1) and (3):

* When (1) goes away, the single member becomes mandatory.

* When (3) goes away, the special "dunno" list element no longer occurs.

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

[Eduardo Habkost]

Just noticed that I hadn't replied to this yet. Sorry for the
long delay!

On Thu, May 12, 2016 at 09:46:25AM +0200, Markus Armbruster wrote:
> Eduardo Habkost  writes:
[...]
> > ##
> > # @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.

Fixed.

> 
> > #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   ?yesno

unavailable-features should be present only if runnable is false.
It may be absent or empty if the architecture code still doesn't
provide detailed info.

Once we have additional architectures implementing the new
fields, we can consider requiring unavailable-features to be
always present (and non-empty) if runnable is false.

In other words:

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

[1] I would like it to be "no", but I prefer to make it mandatory
only after we get some experience with other architectures.


I'm making the following changes to the documentation:

 # Virtual CPU definition.
 #
 # @name: the name of the CPU definition
-# @runnable: #optional. whether the CPU model us usable with the
+# @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
-#model from running in the current host.
+# @unavailable-features: #optional List of attributes that prevent
+#the CPU model from running in the current
+#host. Present only if @runnable is false.
 #(since 2.7)
 #
 # @unavailable-features is a list of QOM property names that

-- 
Eduardo

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

[Eduardo Habkost]

On Thu, May 12, 2016 at 09:19:48AM +0200, Jiri Denemark wrote:
> On Wed, May 11, 2016 at 16:35:50 -0300, Eduardo Habkost wrote:
> > # @CpuDefinitionInfo:
> > #
> > # Virtual CPU definition.
> > #
> > # @name: the name of the CPU definition
> > # @runnable: #optional. whether the CPU model us usable with the
> 
> s/ us / is /

Thanks!

> 
> > #current machine and accelerator. Omitted if we don't
> > #know the answer. (since 2.7)
> > # @unavailable-features: List of attributes that prevent the CPU
> > #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.
> 
> Any chance this could be extended to provide data about every single
> machine type rather than just the current one?

I want to do that, but it would require reorganizing
CPU/machine/accelerator code to allow probing to happen without
depending on global data (including: machine state, accelerator
state, global properties). This will probably take a while to be
implemented.

But: at least for x86, we now guarantee that runnability
shouldn't change depending on the machine-type. If a CPU model is
runnable with a machine version, it should be runnable with other
versions of the same family.

That guarantee is not documented above because I still don't know
if we can enforce it on all other architectures.

-- 
Eduardo

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

[Markus Armbruster]

Eduardo Habkost  writes:

> On Wed, May 11, 2016 at 09:11:33AM +0200, Markus Armbruster wrote:
>> Eduardo Habkost  writes:
>> 
>> > On Tue, May 10, 2016 at 10:23:16AM +0200, Markus Armbruster wrote:
>> >> Eduardo Habkost  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 
>> >> >> > Cc: Michael Mueller 
>> >> >> > Cc: Christian Borntraeger 
>> >> >> > Cc: Cornelia Huck 
>> >> >> > Cc: Jiri Denemark 
>> >> >> > Cc: libvir-l...@redhat.com
>> >> >> > Signed-off-by: Eduardo Habkost 
>> >> >> > ---
>> >> >> >  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   ?yesno

The '?' need to be answered, too.

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

[Jiri Denemark]

On Wed, May 11, 2016 at 16:35:50 -0300, Eduardo Habkost wrote:
> # @CpuDefinitionInfo:
> #
> # Virtual CPU definition.
> #
> # @name: the name of the CPU definition
> # @runnable: #optional. whether the CPU model us usable with the

s/ us / is /

> #current machine and accelerator. Omitted if we don't
> #know the answer. (since 2.7)
> # @unavailable-features: List of attributes that prevent the CPU
> #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.

Any chance this could be extended to provide data about every single
machine type rather than just the current one?

Jirka

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

[David Hildenbrand]

> 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

"List of properties" ?

> #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
> ##
> 

what about changing unavailable-features to

problematic-properties
responsible-properties

... anything else making clear that we are dealing with properties, not
only features?

Apart from that, sounds good to me.

David

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

[Eduardo Habkost]

On Wed, May 11, 2016 at 09:11:33AM +0200, Markus Armbruster wrote:
> Eduardo Habkost  writes:
> 
> > On Tue, May 10, 2016 at 10:23:16AM +0200, Markus Armbruster wrote:
> >> Eduardo Habkost  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 
> >> >> > Cc: Michael Mueller 
> >> >> > Cc: Christian Borntraeger 
> >> >> > Cc: Cornelia Huck 
> >> >> > Cc: Jiri Denemark 
> >> >> > Cc: libvir-l...@redhat.com
> >> >> > Signed-off-by: Eduardo Habkost 
> >> >> > ---
> >> >> >  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
#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
##

-- 
Eduardo

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

[Markus Armbruster]

Eduardo Habkost  writes:

> On Tue, May 10, 2016 at 10:23:16AM +0200, Markus Armbruster wrote:
>> Eduardo Habkost  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 
>> >> > Cc: Michael Mueller 
>> >> > Cc: Christian Borntraeger 
>> >> > Cc: Cornelia Huck 
>> >> > Cc: Jiri Denemark 
>> >> > Cc: libvir-l...@redhat.com
>> >> > Signed-off-by: Eduardo Habkost 
>> >> > ---
>> >> >  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)

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

[Eduardo Habkost]

On Tue, May 10, 2016 at 10:23:16AM +0200, Markus Armbruster wrote:
> Eduardo Habkost  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 
> >> > Cc: Michael Mueller 
> >> > Cc: Christian Borntraeger 
> >> > Cc: Cornelia Huck 
> >> > Cc: Jiri Denemark 
> >> > Cc: libvir-l...@redhat.com
> >> > Signed-off-by: Eduardo Habkost 
> >> > ---
> >> >  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.

-- 
Eduardo

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

[Jiri Denemark]

On Tue, May 10, 2016 at 10:23:16 +0200, Markus Armbruster wrote:
> Eduardo Habkost  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 
> >> > Cc: Michael Mueller 
> >> > Cc: Christian Borntraeger 
> >> > Cc: Cornelia Huck 
> >> > Cc: Jiri Denemark 
> >> > Cc: libvir-l...@redhat.com
> >> > Signed-off-by: Eduardo Habkost 
> >> > ---
> >> >  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?

I think it would be a bug if missing runnable had the same meaning as
runnable=false. To maintain backward compatibility with older QEMU
missing runnable should mean the CPU may or may not be runnable, i.e.,
we don't know which and the user has to try to figure it out.

Jirka

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

[Markus Armbruster]

Eduardo Habkost  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 
>> > Cc: Michael Mueller 
>> > Cc: Christian Borntraeger 
>> > Cc: Cornelia Huck 
>> > Cc: Jiri Denemark 
>> > Cc: libvir-l...@redhat.com
>> > Signed-off-by: Eduardo Habkost 
>> > ---
>> >  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?

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

[Eduardo Habkost]

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 
> > Cc: Michael Mueller 
> > Cc: Christian Borntraeger 
> > Cc: Cornelia Huck 
> > Cc: Jiri Denemark 
> > Cc: libvir-l...@redhat.com
> > Signed-off-by: Eduardo Habkost 
> > ---
> >  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.

-- 
Eduardo

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

[Eric Blake]

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 
> Cc: Michael Mueller 
> Cc: Christian Borntraeger 
> Cc: Cornelia Huck 
> Cc: Jiri Denemark 
> Cc: libvir-l...@redhat.com
> Signed-off-by: Eduardo Habkost 
> ---
>  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'

Why is it optional? Would it hurt to always be present in qemu new
enough to understand why it is needed?

> +# @unavailable-features: List of properties that prevent the CPU
> +#model from running in the current host,
> +#if @runnable is false. Optional.
> +#Since 2.6.
>  #
>  # Since: 1.2.0
>  ##
>  { 'struct': 'CpuDefinitionInfo',
> -  'data': { 'name': 'str' } }
> +  'data': { 'name': 'str',
> +'*runnable': 'bool',
> +'*unavailable-features': [ 'str' ] } }
>  
>  ##
>  # @query-cpu-definitions:
> 

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature