diff mbox

[v5,1/5] arm: qmp: add GICCapability struct

Message ID 1458271654-23706-2-git-send-email-peterx@redhat.com
State New, archived
Headers show

Commit Message

Peter Xu March 18, 2016, 3:27 a.m. UTC
Define new struct to describe whether we support specific GIC version.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 qapi-schema.json | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

Comments

Markus Armbruster March 22, 2016, 6:29 p.m. UTC | #1
Peter Xu <peterx@redhat.com> writes:

> Define new struct to describe whether we support specific GIC version.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  qapi-schema.json | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
>
> diff --git a/qapi-schema.json b/qapi-schema.json
> index f253a37..da9671a 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -4134,3 +4134,25 @@
>  ##
>  { 'enum': 'ReplayMode',
>    'data': [ 'none', 'record', 'play' ] }
> +
> +##
> +# @GICCapability:
> +#
> +# This struct describes capability for a specific GIC version. These
> +# bits are not only decided by QEMU/KVM software version, but also
> +# decided by the hardware that the program is running upon.
> +#
> +# @version:  version of GIC to be described.
> +#
> +# @emulated: whether current QEMU/hardware supports emulated GIC
> +#            device in user space.
> +#
> +# @kernel:   whether current QEMU/hardware supports hardware
> +#            accelerated GIC device in kernel.
> +#
> +# Since: 2.6
> +##
> +{ 'struct': 'GICCapability',
> +  'data': { 'version': 'int',
> +            'emulated': 'bool',
> +            'kernel': 'bool' } }

Are all four combinations of (emulated, kernel) possible?
Eric Blake March 22, 2016, 6:32 p.m. UTC | #2
On 03/17/2016 09:27 PM, Peter Xu wrote:
> Define new struct to describe whether we support specific GIC version.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  qapi-schema.json | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
> 
> diff --git a/qapi-schema.json b/qapi-schema.json
> index f253a37..da9671a 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -4134,3 +4134,25 @@
>  ##
>  { 'enum': 'ReplayMode',
>    'data': [ 'none', 'record', 'play' ] }
> +
> +##
> +# @GICCapability:
> +#
> +# This struct describes capability for a specific GIC version. These

Might be nice to spell out what the acronym GIC means, but that's cosmetic.

> +# bits are not only decided by QEMU/KVM software version, but also
> +# decided by the hardware that the program is running upon.
> +#
> +# @version:  version of GIC to be described.
> +#
> +# @emulated: whether current QEMU/hardware supports emulated GIC
> +#            device in user space.
> +#
> +# @kernel:   whether current QEMU/hardware supports hardware
> +#            accelerated GIC device in kernel.
> +#
> +# Since: 2.6
> +##
> +{ 'struct': 'GICCapability',
> +  'data': { 'version': 'int',
> +            'emulated': 'bool',
> +            'kernel': 'bool' } }
> 

I might have squashed this with the patch that first uses GICCapability,
as defining a type in isolation doesn't do much.

Reviewed-by: Eric Blake <eblake@redhat.com>
Markus Armbruster March 22, 2016, 6:41 p.m. UTC | #3
Markus Armbruster <armbru@redhat.com> writes:

> Peter Xu <peterx@redhat.com> writes:
>
>> Define new struct to describe whether we support specific GIC version.
>>
>> Signed-off-by: Peter Xu <peterx@redhat.com>
>> ---
>>  qapi-schema.json | 22 ++++++++++++++++++++++
>>  1 file changed, 22 insertions(+)
>>
>> diff --git a/qapi-schema.json b/qapi-schema.json
>> index f253a37..da9671a 100644
>> --- a/qapi-schema.json
>> +++ b/qapi-schema.json
>> @@ -4134,3 +4134,25 @@
>>  ##
>>  { 'enum': 'ReplayMode',
>>    'data': [ 'none', 'record', 'play' ] }
>> +
>> +##
>> +# @GICCapability:
>> +#
>> +# This struct describes capability for a specific GIC version. These
>> +# bits are not only decided by QEMU/KVM software version, but also
>> +# decided by the hardware that the program is running upon.
>> +#
>> +# @version:  version of GIC to be described.
>> +#
>> +# @emulated: whether current QEMU/hardware supports emulated GIC
>> +#            device in user space.
>> +#
>> +# @kernel:   whether current QEMU/hardware supports hardware
>> +#            accelerated GIC device in kernel.
>> +#
>> +# Since: 2.6
>> +##
>> +{ 'struct': 'GICCapability',
>> +  'data': { 'version': 'int',
>> +            'emulated': 'bool',
>> +            'kernel': 'bool' } }
>
> Are all four combinations of (emulated, kernel) possible?

Moreover, what do the combinations mean from a practical point of view?
What would a management application do with the information?
Peter Xu March 23, 2016, 2:58 a.m. UTC | #4
On Tue, Mar 22, 2016 at 07:41:17PM +0100, Markus Armbruster wrote:
> Markus Armbruster <armbru@redhat.com> writes:
> >> +##
> >> +# @GICCapability:
> >> +#
> >> +# This struct describes capability for a specific GIC version. These
> >> +# bits are not only decided by QEMU/KVM software version, but also
> >> +# decided by the hardware that the program is running upon.
> >> +#
> >> +# @version:  version of GIC to be described.
> >> +#
> >> +# @emulated: whether current QEMU/hardware supports emulated GIC
> >> +#            device in user space.
> >> +#
> >> +# @kernel:   whether current QEMU/hardware supports hardware
> >> +#            accelerated GIC device in kernel.
> >> +#
> >> +# Since: 2.6
> >> +##
> >> +{ 'struct': 'GICCapability',
> >> +  'data': { 'version': 'int',
> >> +            'emulated': 'bool',
> >> +            'kernel': 'bool' } }
> >
> > Are all four combinations of (emulated, kernel) possible?

Currently, version can only be 2 or 3. And for now, we should only
support:

- emulated/kernel support for v2
- kernel support for v3

and emulated v3 is still not supported.

> Moreover, what do the combinations mean from a practical point of view?
> What would a management application do with the information?

AFAIU, this command will mostly be used by libvirt. E.g., on ARM
host, user can choose which kind of GIC device he/she uses. However,
not all of them are supported, and it is decided by both the ARM
hardware and the software (here software should include at least
QEMU and the kernel version). With this command, libvirt can easily
know what is supported, and what is not.

So it can: warn the user before-hand during configuration like "GIC
emulated/kvm-accelerated version X is/isn't supported",

rather than: firstly, configuration success. After that, QEMU boot
failed with ambiguous error.

I can add more explainations into the commit message (some can be
added to the schema directly possibly) about why we need this
command, and what does every entry mean.

Thanks!

-- peterx
Peter Xu March 23, 2016, 3:09 a.m. UTC | #5
On Tue, Mar 22, 2016 at 12:32:28PM -0600, Eric Blake wrote:
> On 03/17/2016 09:27 PM, Peter Xu wrote:
> > +##
> > +# @GICCapability:
> > +#
> > +# This struct describes capability for a specific GIC version. These
> 
> Might be nice to spell out what the acronym GIC means, but that's cosmetic.

Ah! I thought I have added that... It's missing again. Will do in
next spin.

> 
> > +# bits are not only decided by QEMU/KVM software version, but also
> > +# decided by the hardware that the program is running upon.
> > +#
> > +# @version:  version of GIC to be described.
> > +#
> > +# @emulated: whether current QEMU/hardware supports emulated GIC
> > +#            device in user space.
> > +#
> > +# @kernel:   whether current QEMU/hardware supports hardware
> > +#            accelerated GIC device in kernel.
> > +#
> > +# Since: 2.6
> > +##
> > +{ 'struct': 'GICCapability',
> > +  'data': { 'version': 'int',
> > +            'emulated': 'bool',
> > +            'kernel': 'bool' } }
> > 
> 
> I might have squashed this with the patch that first uses GICCapability,
> as defining a type in isolation doesn't do much.

I can do the squash in next spin if you prefer that one. Actually I
got this question before, about when I should split and when to
squash. E.g., shall I make sure that I should have no "definition
only" patches in the future?

-- peterx
Markus Armbruster March 23, 2016, 9:33 a.m. UTC | #6
Peter Xu <peterx@redhat.com> writes:

> On Tue, Mar 22, 2016 at 07:41:17PM +0100, Markus Armbruster wrote:
>> Markus Armbruster <armbru@redhat.com> writes:
>> >> +##
>> >> +# @GICCapability:
>> >> +#
>> >> +# This struct describes capability for a specific GIC version. These
>> >> +# bits are not only decided by QEMU/KVM software version, but also
>> >> +# decided by the hardware that the program is running upon.
>> >> +#
>> >> +# @version:  version of GIC to be described.
>> >> +#
>> >> +# @emulated: whether current QEMU/hardware supports emulated GIC
>> >> +#            device in user space.
>> >> +#
>> >> +# @kernel:   whether current QEMU/hardware supports hardware
>> >> +#            accelerated GIC device in kernel.
>> >> +#
>> >> +# Since: 2.6
>> >> +##
>> >> +{ 'struct': 'GICCapability',
>> >> +  'data': { 'version': 'int',
>> >> +            'emulated': 'bool',
>> >> +            'kernel': 'bool' } }
>> >
>> > Are all four combinations of (emulated, kernel) possible?
>
> Currently, version can only be 2 or 3. And for now, we should only
> support:
>
> - emulated/kernel support for v2
> - kernel support for v3
>
> and emulated v3 is still not supported.
>
>> Moreover, what do the combinations mean from a practical point of view?
>> What would a management application do with the information?
>
> AFAIU, this command will mostly be used by libvirt. E.g., on ARM
> host, user can choose which kind of GIC device he/she uses. However,
> not all of them are supported, and it is decided by both the ARM
> hardware and the software (here software should include at least
> QEMU and the kernel version). With this command, libvirt can easily
> know what is supported, and what is not.
>
> So it can: warn the user before-hand during configuration like "GIC
> emulated/kvm-accelerated version X is/isn't supported",
>
> rather than: firstly, configuration success. After that, QEMU boot
> failed with ambiguous error.
>
> I can add more explainations into the commit message (some can be
> added to the schema directly possibly) about why we need this
> command, and what does every entry mean.

So GICCapability essentially tells its users whether certain
configurations have a chance to work.

I think what's missing in your patch is the connection from
GICCapability to the actual configuration options.  As is, you just have
to know what options the presence of each possible GICCapability value
unlocks.  It needs to be documented instead.
Markus Armbruster March 23, 2016, 9:44 a.m. UTC | #7
Peter Xu <peterx@redhat.com> writes:

> On Tue, Mar 22, 2016 at 12:32:28PM -0600, Eric Blake wrote:
>> On 03/17/2016 09:27 PM, Peter Xu wrote:
>> > +##
>> > +# @GICCapability:
>> > +#
>> > +# This struct describes capability for a specific GIC version. These
>> 
>> Might be nice to spell out what the acronym GIC means, but that's cosmetic.
>
> Ah! I thought I have added that... It's missing again. Will do in
> next spin.
>
>> 
>> > +# bits are not only decided by QEMU/KVM software version, but also
>> > +# decided by the hardware that the program is running upon.
>> > +#
>> > +# @version:  version of GIC to be described.
>> > +#
>> > +# @emulated: whether current QEMU/hardware supports emulated GIC
>> > +#            device in user space.
>> > +#
>> > +# @kernel:   whether current QEMU/hardware supports hardware
>> > +#            accelerated GIC device in kernel.
>> > +#
>> > +# Since: 2.6
>> > +##
>> > +{ 'struct': 'GICCapability',
>> > +  'data': { 'version': 'int',
>> > +            'emulated': 'bool',
>> > +            'kernel': 'bool' } }
>> > 
>> 
>> I might have squashed this with the patch that first uses GICCapability,
>> as defining a type in isolation doesn't do much.
>
> I can do the squash in next spin if you prefer that one. Actually I
> got this question before, about when I should split and when to
> squash. E.g., shall I make sure that I should have no "definition
> only" patches in the future?

Depends.

The general rule is to keep separate things separate, and patches
self-contained.  The narrow sense of self-contained is each patch
compiles and works.  The wider sense is each patch makes sense to its
readers on its own.  You can't always have a perfect score on the
latter, but you should try.

Adding a definition without a user is generally not advised, because you
generally need to see the user to make sense of it.

For a complex feature, adding its abstract interface before its concrete
implementation may help liberate interface review from implementation
details.

Note that your interface consists of type GICCapability and command
query-gic-capabilities.  You could add just the interface with a stub
implementation first, then flesh out the implementation.  But I doubt
the thing is complex enough to justify that.
Peter Xu March 23, 2016, 11:48 a.m. UTC | #8
On Wed, Mar 23, 2016 at 10:33:09AM +0100, Markus Armbruster wrote:
> Peter Xu <peterx@redhat.com> writes:
> 
> > On Tue, Mar 22, 2016 at 07:41:17PM +0100, Markus Armbruster wrote:
> >> Markus Armbruster <armbru@redhat.com> writes:
> >> >> +##
> >> >> +# @GICCapability:
> >> >> +#
> >> >> +# This struct describes capability for a specific GIC version. These
> >> >> +# bits are not only decided by QEMU/KVM software version, but also
> >> >> +# decided by the hardware that the program is running upon.
> >> >> +#
> >> >> +# @version:  version of GIC to be described.
> >> >> +#
> >> >> +# @emulated: whether current QEMU/hardware supports emulated GIC
> >> >> +#            device in user space.
> >> >> +#
> >> >> +# @kernel:   whether current QEMU/hardware supports hardware
> >> >> +#            accelerated GIC device in kernel.
> >> >> +#
> >> >> +# Since: 2.6
> >> >> +##
> >> >> +{ 'struct': 'GICCapability',
> >> >> +  'data': { 'version': 'int',
> >> >> +            'emulated': 'bool',
> >> >> +            'kernel': 'bool' } }

[*] Marking here...

> So GICCapability essentially tells its users whether certain
> configurations have a chance to work.
> 
> I think what's missing in your patch is the connection from
> GICCapability to the actual configuration options.  As is, you just have
> to know what options the presence of each possible GICCapability value
> unlocks.  It needs to be documented instead.

What I understand is that, above [*] should have explained what does
each entry mean. E.g., as mentioned in the qapi-schema, there are
explainations about "version", "emulated" and "kernel" key words. If
we go deeper into e.g., "emulated" key word, we will got:

"whether current QEMU/hardware supports emulated GIC device in user
 space."

So this boolean will tell just as it is explained.

Maybe I failed to get the point of your review comment... :( Would
you please give an example on how should I better express this
relationship?

(btw, I have updated the commit message in v6 for current patch, to
tell more about why we need this, and why we decided to add this ad
hoc command. I'd be glad if we can continue the discussion based on
that one.  Thanks!)

-- peterx
Peter Xu March 23, 2016, 11:56 a.m. UTC | #9
On Wed, Mar 23, 2016 at 10:44:04AM +0100, Markus Armbruster wrote:
> Depends.
> 
> The general rule is to keep separate things separate, and patches
> self-contained.  The narrow sense of self-contained is each patch
> compiles and works.  The wider sense is each patch makes sense to its
> readers on its own.  You can't always have a perfect score on the
> latter, but you should try.
> 
> Adding a definition without a user is generally not advised, because you
> generally need to see the user to make sense of it.
> 
> For a complex feature, adding its abstract interface before its concrete
> implementation may help liberate interface review from implementation
> details.
> 
> Note that your interface consists of type GICCapability and command
> query-gic-capabilities.  You could add just the interface with a stub
> implementation first, then flesh out the implementation.  But I doubt
> the thing is complex enough to justify that.

Thanks for the thorough explaination on this.

It's indeed not easy to figure out the best way every time for
me... Now I do feel it strange to split the first patch alone from
the 2nd one. Anyway, it's squashed into the 2nd patch in v6.

-- peterx
Markus Armbruster March 23, 2016, 12:21 p.m. UTC | #10
Peter Xu <peterx@redhat.com> writes:

> On Wed, Mar 23, 2016 at 10:33:09AM +0100, Markus Armbruster wrote:
>> Peter Xu <peterx@redhat.com> writes:
>> 
>> > On Tue, Mar 22, 2016 at 07:41:17PM +0100, Markus Armbruster wrote:
>> >> Markus Armbruster <armbru@redhat.com> writes:
>> >> >> +##
>> >> >> +# @GICCapability:
>> >> >> +#
>> >> >> +# This struct describes capability for a specific GIC version. These
>> >> >> +# bits are not only decided by QEMU/KVM software version, but also
>> >> >> +# decided by the hardware that the program is running upon.
>> >> >> +#
>> >> >> +# @version:  version of GIC to be described.
>> >> >> +#
>> >> >> +# @emulated: whether current QEMU/hardware supports emulated GIC
>> >> >> +#            device in user space.
>> >> >> +#
>> >> >> +# @kernel:   whether current QEMU/hardware supports hardware
>> >> >> +#            accelerated GIC device in kernel.
>> >> >> +#
>> >> >> +# Since: 2.6
>> >> >> +##
>> >> >> +{ 'struct': 'GICCapability',
>> >> >> +  'data': { 'version': 'int',
>> >> >> +            'emulated': 'bool',
>> >> >> +            'kernel': 'bool' } }
>
> [*] Marking here...
>
>> So GICCapability essentially tells its users whether certain
>> configurations have a chance to work.
>> 
>> I think what's missing in your patch is the connection from
>> GICCapability to the actual configuration options.  As is, you just have
>> to know what options the presence of each possible GICCapability value
>> unlocks.  It needs to be documented instead.
>
> What I understand is that, above [*] should have explained what does
> each entry mean. E.g., as mentioned in the qapi-schema, there are
> explainations about "version", "emulated" and "kernel" key words. If
> we go deeper into e.g., "emulated" key word, we will got:
>
> "whether current QEMU/hardware supports emulated GIC device in user
>  space."
>
> So this boolean will tell just as it is explained.
>
> Maybe I failed to get the point of your review comment... :( Would
> you please give an example on how should I better express this
> relationship?

Can you tell me what a management application is supposed to do with the
information returned by query-gic-capabilities?  Not just in general
terms, like "using this information, libvirt can warn the user during
configuration of guests when specified GIC device type is not supported,
but specifics.  Something like "-frobnicate mutter=mumble won't work
unless query-gic-capabilities reports emulated version 2 is supported"
for every piece of configuration that should be vetted against
query-gic-capabilities.

> (btw, I have updated the commit message in v6 for current patch, to
> tell more about why we need this, and why we decided to add this ad
> hoc command. I'd be glad if we can continue the discussion based on
> that one.  Thanks!)
Peter Xu March 23, 2016, 2:25 p.m. UTC | #11
On Wed, Mar 23, 2016 at 01:21:52PM +0100, Markus Armbruster wrote:
> Can you tell me what a management application is supposed to do with the
> information returned by query-gic-capabilities?  Not just in general
> terms, like "using this information, libvirt can warn the user during
> configuration of guests when specified GIC device type is not supported,
> but specifics.  Something like "-frobnicate mutter=mumble won't work
> unless query-gic-capabilities reports emulated version 2 is supported"
> for every piece of configuration that should be vetted against
> query-gic-capabilities.

I suppose that won't be a very big problem since possibly only
libvirt will use it... I agree that it's better to explain it more
clearly though. How about adding these lines into patch 1 commit
message:

"""
For example, if we got the query result:

{"return": {"capabilities":
  [ {"emulated": false, "version": 3, "kernel": true},
    {"emulated": true, "version": 2, "kernel": false} ] } }

Then it means that we support emulated GIC version 2 using:

  qemu-system-aarch64 -M virt,gic-version=2 ...

or kvm-accelerated GIC version 3 using:

  qemu-system-aarch64 -enable-kvm -M virt,gic-version=3 ...

If we specify other explicit GIC version rather than the above, QEMU
will not be able to boot.
"""

Thanks.

-- peterx
Markus Armbruster March 23, 2016, 3:17 p.m. UTC | #12
Peter Xu <peterx@redhat.com> writes:

> On Wed, Mar 23, 2016 at 01:21:52PM +0100, Markus Armbruster wrote:
>> Can you tell me what a management application is supposed to do with the
>> information returned by query-gic-capabilities?  Not just in general
>> terms, like "using this information, libvirt can warn the user during
>> configuration of guests when specified GIC device type is not supported,
>> but specifics.  Something like "-frobnicate mutter=mumble won't work
>> unless query-gic-capabilities reports emulated version 2 is supported"
>> for every piece of configuration that should be vetted against
>> query-gic-capabilities.
>
> I suppose that won't be a very big problem since possibly only
> libvirt will use it... I agree that it's better to explain it more
> clearly though. How about adding these lines into patch 1 commit
> message:
>
> """
> For example, if we got the query result:
>
> {"return": {"capabilities":
>   [ {"emulated": false, "version": 3, "kernel": true},
>     {"emulated": true, "version": 2, "kernel": false} ] } }
>
> Then it means that we support emulated GIC version 2 using:
>
>   qemu-system-aarch64 -M virt,gic-version=2 ...
>
> or kvm-accelerated GIC version 3 using:
>
>   qemu-system-aarch64 -enable-kvm -M virt,gic-version=3 ...

I'd say "qemu-system-aarch64 -M virt,accel=kvm,gic=version=3"

> If we specify other explicit GIC version rather than the above, QEMU
> will not be able to boot.
> """

Works for me.
Peter Xu March 24, 2016, 2:27 a.m. UTC | #13
On Wed, Mar 23, 2016 at 04:17:25PM +0100, Markus Armbruster wrote:
> Peter Xu <peterx@redhat.com> writes:
> 
> > On Wed, Mar 23, 2016 at 01:21:52PM +0100, Markus Armbruster wrote:
> >> Can you tell me what a management application is supposed to do with the
> >> information returned by query-gic-capabilities?  Not just in general
> >> terms, like "using this information, libvirt can warn the user during
> >> configuration of guests when specified GIC device type is not supported,
> >> but specifics.  Something like "-frobnicate mutter=mumble won't work
> >> unless query-gic-capabilities reports emulated version 2 is supported"
> >> for every piece of configuration that should be vetted against
> >> query-gic-capabilities.
> >
> > I suppose that won't be a very big problem since possibly only
> > libvirt will use it... I agree that it's better to explain it more
> > clearly though. How about adding these lines into patch 1 commit
> > message:
> >
> > """
> > For example, if we got the query result:
> >
> > {"return": {"capabilities":
> >   [ {"emulated": false, "version": 3, "kernel": true},
> >     {"emulated": true, "version": 2, "kernel": false} ] } }
> >
> > Then it means that we support emulated GIC version 2 using:
> >
> >   qemu-system-aarch64 -M virt,gic-version=2 ...
> >
> > or kvm-accelerated GIC version 3 using:
> >
> >   qemu-system-aarch64 -enable-kvm -M virt,gic-version=3 ...
> 
> I'd say "qemu-system-aarch64 -M virt,accel=kvm,gic=version=3"
> 
> > If we specify other explicit GIC version rather than the above, QEMU
> > will not be able to boot.
> > """
> 
> Works for me.

Thanks, will include this in v7.

-- peterx
diff mbox

Patch

diff --git a/qapi-schema.json b/qapi-schema.json
index f253a37..da9671a 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -4134,3 +4134,25 @@ 
 ##
 { 'enum': 'ReplayMode',
   'data': [ 'none', 'record', 'play' ] }
+
+##
+# @GICCapability:
+#
+# This struct describes capability for a specific GIC version. These
+# bits are not only decided by QEMU/KVM software version, but also
+# decided by the hardware that the program is running upon.
+#
+# @version:  version of GIC to be described.
+#
+# @emulated: whether current QEMU/hardware supports emulated GIC
+#            device in user space.
+#
+# @kernel:   whether current QEMU/hardware supports hardware
+#            accelerated GIC device in kernel.
+#
+# Since: 2.6
+##
+{ 'struct': 'GICCapability',
+  'data': { 'version': 'int',
+            'emulated': 'bool',
+            'kernel': 'bool' } }