[libvirt] [PATCH 5/7] qemu: Always enable GIC on ARM virt machines

[John Ferlan]

On 02/08/2016 08:41 AM, Andrea Bolognani wrote:
> On Sun, 2016-02-07 at 09:42 -0500, John Ferlan wrote:
>> On 02/03/2016 03:26 PM, Andrea Bolognani wrote:
>>> GIC is always available to ARM virt machines, and the domain XML should
>>> reflect this fact.
>>> ---
>>>   src/qemu/qemu_domain.c                                     | 14 ++++++++++++++
>>>   .../qemuxml2argv-aarch64-aavmf-virtio-mmio.xml             |  1 +
>>>   2 files changed, 15 insertions(+)
>>>  
>>> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
>>> index d120e15..5017cbb 100644
>>> --- a/src/qemu/qemu_domain.c
>>> +++ b/src/qemu/qemu_domain.c
>>> @@ -1241,6 +1241,20 @@ qemuDomainDefAddDefaultDevices(virDomainDefPtr def,
>>>   static int
>>>   qemuDomainDefAddDefaultFeatures(virDomainDefPtr def)
>>>   {
>>> +    switch (def->os.arch) {
>>> +    case VIR_ARCH_ARMV7L:
>>> +    case VIR_ARCH_AARCH64:
>>> +        if (STREQ(def->os.machine, "virt") ||
>>> +            STRPREFIX(def->os.machine, "virt-")) {
>>> +            /* GIC is always available to ARM virt machines */
>>> +            def->features[VIR_DOMAIN_FEATURE_GIC] = VIR_TRISTATE_SWITCH_ON;
>>  
>> See once on - we then have a version='host' and we're good to go.
>>  
>> Of course, as I'm typing I realize that we wouldn't print out
>> version='host' if it were the default...  But that may not be a bad
>> thing - although we could.
> 
> That was pretty much the issue :)
> 
> For existing guests, <gic/> means <gic version='2'/>, but we pick the
> default every single time the XML is loaded instead of explicitly
> writing it out in the XML. Which was probably a mistake, because now
> we can't really change the default without affecting existing guests :(

So yes, 2 is the default, but how it's handled now changes I think.

If one supplies "<gic/>" or "<gic version='2'/>" prior to these changes,
the qemu args does not list the "gic-version=%d" as output. The note in
the code is "2 is the default, so we don't put it as option for
backwards compatibility".  After these changes, if gic_version=2 is
found, still we don't write "gic-version=2" in the command line
(allowing qemu to choose the default, I assume).

However, since 'gic_version' is only set if 'version' is found in the
XML, by changing or setting the default to 2 - wouldn't that cause an
ABI difference (e.g. virDomainDefFeaturesCheckABIStability failure)
because patch 4 says, if GIC is ON and gic_version == 0 (NONE), then set
gic_version == 2?  Eventually that "version=2" would be written out.

> 
> On the other hand, QEMU defaults to version 2 as well, so it kinda
> fits nicely I guess?

I'm not against "2" - I just wasn't keeping up with all the latest
details on that GICv3 thread...  For some reason, I had an impression
that supplying "host" was something perhaps desired.  If in the long run
"host" for qemu means 2, then great.  If in the future qemu changes
"host" to mean 3, then we don't have to change our default value or any
of the checks in qemu_command.c.

Also since "host" would be 0 (once "none" was removed) we have the added
benefit of a our allocation routines setting our default value.

I wish there were xml2xml output difference check tests prior to this
series... eg. something that would have ensured reading "<gic/>"
resulted in output of "<gic/>", which I don't think will happen after
these patches.

John
> 
> Point is, I think keeping the default to version 2 in libvirt and
> have upper layers (eg. virt-install via libosinfo) pick a better
> value when creating new guests is in line with how we do a lot of
> other stuff in libvirt.
> 
> Cheers.
> 
> -- 
> Andrea Bolognani
> Software Engineer - Virtualization Team
>