Re: [libvirt] [PATCH v3 6/8] qemu: use virDomainPCIAddressIsMulti() to determine multifunction setting
On 12/20/2016 10:19 AM, Andrea Bolognani wrote:
On Mon, 2016-12-19 at 10:23 -0500, Laine Stump wrote:
> If the multifunction attribute isn't set in the config for the device
> at function 0 of a slot used for multifunction, it would previously
> have been an error. This patch will instead automatically correct the
> omission (but only if it hasn't been set at all - if someone
> explicitly has "multifunction='off'" on function 0, or
> "multifunction='on'" when function != 0, we have to assume they
have a
> reason for that).
>
> This effectively obsoletes the requirement of specifying
> multifunction='on' in the config, although you're still free to do
> so. Note that if you migrate a domain that needs an implied
> "multifunction='on'" back to any older libvirt that doesn't
have it,
> the migration will fail. (Note that this would only be an issue with a
> domain config that was *created* on a newer libvirt; any config
> created on an older libvirt and then later migrated to a newer libvirt
> would necessarily have multifunction explicitly set in the config, and
> that will not be lost during migration).
I keep forgetting our official stance on migrating to older
libvirt versions...
As far as I'm concerned, the only reason you would want to do
that is because you are upgrading your hypervisor pool and,
at some point during the process, you realize there are issues
with the upgrade and need to roll back. As you mention, that
use case would work just fine because the guests have been
defined using an older libvirt versions.
Thinking about it more - even if the domain is defined using a new
libvirt version, as long as the management is still setting
multifunction manually (which will be the case for everything except
auto-added pcie-root-ports), it will still be reflected in the config,
and a migration to a host with older libvirt will still work. This
narrows the scope of "unable to migrate from new->old libvirt even more
- it's only a problem if you define a new domain that auto-adds
pcie-root-ports, or add pcie-root-ports to an existing domain on a "new
libvirt" host, then migrate it to an "old libvirt" host. Since the
auto-adding of pci-root-ports *at all* (much less on multiple functions
of a single slot) is a new feature, and in general it's quite often not
possible to migrate domains that use new features from newer to older
libvirt, I think this is acceptable.
That said, is there any reason why this code can't be moved
to the PostParse callback, so that the multifunction property
will show up in the guest configuration and the issue will be
side-stepped entirely?
I'd rather not pollute new configurations with attributes that aren't
necessary. But more important - if you remove a device from a function
of a slot and end up leaving only the device on function 0, then it
should no longer have multifunction set; when we do the setting of
multifunction automatically at runtime, this will happen by itself, but
if we save the multifunction=on in the config, then it will require
manual intervention.
Normally I would say that it's desirable to record all option settings
in the XML in order to maintain guest ABI, but in this case the existing
setting would be incorrect once the other devices on the slot are
removed, and the act of removing the other devices changes the guest ABI
anyway, so it would/should be expected by the guest.
So does this patch get an ACK? Or am I being too cavalier about
migration compatibility?