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