[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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.


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]