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

[Peter Xu]

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?

(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