[libvirt] [PATCH v3 6/8] qemu: use virDomainPCIAddressIsMulti() to determine multifunction setting

[Laine Stump]

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?