8:30 a.m.
Before I forget ;-) or put it off in lieu of other exciting things...
On 02/08/2016 12:20 PM, Andrea Bolognani wrote:
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.
Ahh - the benefits of intraoffice communication... Anyway, yes it's the
migration and the save/restore code that I was thinking about... If you
want to 'test' you can save a domain, then run restore after... Consider
it the poor man's migration ;-)... Something we used to call
sneakernet... Save something on one disk in the lab, then go grab that
disk and walk it over to a different (and not connected system), install
the disk there, and restore the file ;-) - the old days!
> 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.
I think writing it out is fine - the one concern being the migration or
save/restore type paths. Those usually bite later in some corner case.
I guess the let's make sure radar was triggered because this was a
change in how we store things if someone had only provided <gic> before
and we didn't have the xml2xml test for the GIC version code...
(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.
Yuck - the way patch 1 has the order is best since the "2" and "3"
match
up numerically and adding "4" is then ordered properly.
So let's see that means the original patches should be fine with the
following adjustments:
patch 1 has a typo in the commit message, then ACK
patch 4 could change the function name (use Enable)
patch 4 should use void instead of int for that function, then ACK
If we can add an xml2xml test that would help ensure something doesn't
mess this up for the future.
I feel those type changes are simple enough to not need a new series posted.
John
> 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