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

[Andrea Bolognani]

On Mon, 2016-02-08 at 10:24 -0500, John Ferlan wrote:
> > 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).

Correct and intended. Also, I've been guaranteed that QEMU's default
will remain version 2, so this is definitely the right thing to do.

> 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?

Right, I didn't consider that. Thanks a bunch for bringing it up,
this is exactly why code reviews are a good idea! :)

I've discussed this with Michal, since my understanding of the
whole migration business is still fuzzy at best, and he seems to agree
that the change would not cause any issue because old versions of
libvirt can parse <gic version='2'/> just fine, so the ABI check will
not fail.

> Eventually that "version=2" would be written out.

That's the idea :)

In general we should really make sure that, whenever we use the
information in the domain XML to infer information that's not there,
or use default values, we write the result back to the domain XML,
so that we're free to change the defaults at a later date knowing
that existing guests will not suddenly get assigned different
hardware than what they expect.

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

See above - we're stuck with having version 2 as default exactly
because we never wrote our default to the domain XML.

Higher layers are in a better position to pick optimal defaults
like "host" anyway, so I wouldn't worry about that as long as we
expose all the appropriate knobs.

(AFAIK "host" just means "whatever's available on the host", which
of course might be 2 or 3 depending on the actual hardware.)

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

That would be nice... We could still achieve the same result by
shuffling the values around:

  typedef enum {
    VIR_GIC_VERSION_2 = 0,
    VIR_GIC_VERSION_3,
    VIR_GIC_VERSION_HOST,
    VIR_GIC_VERSION_LAST
  } virGICVersion;

but that would become a bit icky once GIC version 4 comes out,
wouldn't it? Nothing we wouldn't be able to live with, of
course.

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

See above - I believe we actually *want* the value to be printed
out.

Cheers.

-- 
Andrea Bolognani
Software Engineer - Virtualization Team