On Mon, 2016-11-07 at 14:50 -0500, Laine Stump wrote:
Before now, all the qemu hotplug functions assumed that all devices to be hotplugged were legacy PCI endpoint devices (VIR_PCI_CONNECT_TYPE_PCI_DEVICE). This worked out "okay", because all devices *are* legacy PCI endpoint devices on x86/440fx machinetypes, and hotplug didn't work properly on machinetypes using PCIe anyway (hotplugging onto a legacy PCI slot doesn't work, and until commit b87703cf any attempt to manually specify a PCIe address for a hotplugged device would be erroneously rejected). This patch makes all qemu hotplug operations honor the pciConnectFlags set by the single all-knowing function qemuDomainDeviceCalculatePCIConnectFlags(). This is done in 3 steps, but in a single commit since we would have to touch the other points at each step anyway: 1) add a flags argument to the hypervisor-agnostic virDomainPCIAddressEnsureAddr() (previously it hardcoded ..._PCI_DEVICE) 2) add a new qemu-specific function qemuDomainEnsurePCIAddress() which gets the correct pciConnectFlags for the device from qemuDomainDeviceConnectFlags(), then calls virDomainPCIAddressEnsureAddr(). 3) in qemu_hotplug.c replace all calls to virDomainPCIAddressEnsureAddr() with calls to qemuDomainEnsurePCIAddress() So in effect, we're putting a "shim" on top of all calls to virDomainPCIAddressEnsureAddr() that sets the right pciConnectFlags. --- src/conf/domain_addr.c | 10 ++-------- src/conf/domain_addr.h | 3 ++- src/qemu/qemu_domain_address.c | 27 +++++++++++++++++++++++++++ src/qemu/qemu_domain_address.h | 4 ++++ src/qemu/qemu_hotplug.c | 23 ++++++++++++++++------- 5 files changed, 51 insertions(+), 16 deletions(-)
[...]
+/** + * qemuDomainEnsurePCIAddress: + * + * if @dev should have a PCI address but doesn't, assign an address on + * a compatible PCI bus, and set it in @dev->...info. If there is an + * address already, validate that it is on a compatible bus, based on + * @dev->...info.pciConnectFlags. + * + * @obj: the virDomainObjPtr for the domain. This will include + * qemuCaps and address cache (if there is one) + * + * @dev: the device that we need to ensure has a PCI address
Please move the description of the function arguments before the description of the function itself.
+ * returns 0 on success -1 on failure. + */ +int +qemuDomainEnsurePCIAddress(virDomainObjPtr obj, + virDomainDeviceDefPtr dev) +{ + qemuDomainObjPrivatePtr priv = obj->privateData; + virDomainDeviceInfoPtr info = virDomainDeviceGetInfo(dev);
virDomainDeviceGetInfo() can return NULL...
+ qemuDomainFillDevicePCIConnectFlags(obj->def, dev, priv->qemuCaps); + + return virDomainPCIAddressEnsureAddr(priv->pciaddrs, info, + info->pciConnectFlags);
... but you don't check for that before dereferencing info here. I guess this function will never be called on a device that doesn't have a DeviceInfo, but it's probably a good idea to explicitly check anyways in case the callers change. ACK once the comments above have been taken care of. -- Andrea Bolognani / Red Hat / Virtualization