[libvirt] [PATCH 0/6] auto-assign addresses when <address type='pci'/> is specified

[Laine Stump]

On 05/19/2016 01:23 PM, Ján Tomko wrote:
> On Thu, May 19, 2016 at 01:15:28PM -0400, Cole Robinson wrote:
>> On 05/18/2016 03:23 PM, Laine Stump wrote:
>>> This is an alternative to Cole's series that permits <address
>>> type='pci'/> to force assignment of a PCI address, which is
>>> particularly useful on platforms that could connect the same device in
>>> different ways (e.g. aarch64/virt).
>>>
>>> Here is Cole's last iteration of the series:
>>>
>>>    https://www.redhat.com/archives/libvir-list/2016-May/msg01088.html
>>>
>>> I had expressed a dislike of the "auto_allocate" flag that his series
>>> temporarily adds to the address (while simultaneously changing the
>>> address type to NONE) and suggested just changing all the necessary
>>> places to check for a valid PCI address instead of just checking the
>>> address type. He replied that this wasn't as simple as it looked, so I
>>> decided to try it; turns out he's right. But I still like this method
>>> better because it's not playing tricks with the address type, or
>>> adding a temporary private attribute to what should be pure config
>>> data.
>>>
>>> Your opinion may vary though :-)
>>>
> I like this series more than counting XML elements and temporarily
> changing the types.
>
>>> Note that patch 5/6 incorporates the same test case that Cole used in
>>> his penultimate patch, and I've added his patch to check the case of
>>> aarch64 at the end as well.
>>>
>> ACK series, but it's missing formatdomain.html.in changes. You can grab the
>> check from my patch #2
>>
>> I'm fine with this approach but I'll just list the downsides
>>
>> - Less generalizable, for adding additional address types in the future, but
>> that's hypothetical at this point
>> - We don't raise an explicit error for drivers that don't support this type of
>> address allocation, like libxl. If it matters we could add a domain XML parse
>> feature to throw an explicit error though
>> - checking info->type == DEVICE_ADDRESS_TYPE_PCI is now a suspect pattern and
>> needs to be considered carefully in other parts of the code
>>
>> upsides:
>> - less magic
>> - I think it will make allocating address at hotplug time simpler as well
>>
> Yes, qemu_hotplug.c has a few of those places using == DEVICE_ADDRESS_TYPE_PCI
> untouched by this series.

Since they happen after parse is finished, it should be safe.

And anyway, looking at those uses, I think what most of them are doing 
(calling virDomainPCIAddressEnsureAddr()) is 100% unnecessary, since 
it's now already done when addresses are assigned in 
qemuDomainDefAssignAddresses(), which has already been called. (Back in 
Jan 2010 when the calls to qemuDomainPCIAddressEnsureAddr() were added 
(commit d8acc4), this was not the case - they were needed in order for 
the new devices to get an address assigned, but a lot has changed since 
then - even before Cole put in the postparse callback address assignment 
stuff and called it when attaching a device, we had already been 
assigning addresses during the higher level qemuDomainAttachDeviceConfig 
for a long time (since commit f5dd58a in June 2012; long enough that the 
calls to qemuDomainCCWAddressAssign() that are also sprinkled throughout 
qemu_hotplug.c in the same vicinity as the calls to 
qemuDomainPCIAddressEnsureAddr() weren't needed *even at the time they 
were added!* (commit f94646, March 2013). This was purely cargo-cult 
coding, caused by commit f5dd58a failing to remove the calls to 
...EnsureAddr(). The interesting bit is that both of these commits were 
put in in support of s390 virtio devices.).

(Well, *that* was a nice trip down git "memory lane" (aka "blame")

So, the result is that most of the code in qemu_hotplug that requires 
checking for address type is unneeded, and I'm going to send a patch to 
remove it.