Re: [Qemu-arm] [Qemu-devel] [PATCH v5 1/5] arm: qmp: add GICCapability struct

[Markus Armbruster]

Peter Xu <address@hidden> writes:

> On Wed, Mar 23, 2016 at 10:33:09AM +0100, Markus Armbruster wrote:
>> Peter Xu <address@hidden> writes:
>> 
>> > On Tue, Mar 22, 2016 at 07:41:17PM +0100, Markus Armbruster wrote:
>> >> Markus Armbruster <address@hidden> 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!)