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

[Markus Armbruster]

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' } }
>> >
>> > 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.