Re: [libvirt] [PATCH 0/6] auto-assign addresses when <address type='pci'/> is specified
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.