Re: [libvirt] [PATCHv2 1/7] qemu: enable auto-allocate of all PCI addresses

On Sat, Aug 3, 2013 at 9:01 PM, Laine Stump <laine(a)laine.org&gt; wrote: > + if ((cont->type == VIR_DOMAIN_CONTROLLER_TYPE_IDE && cont->idx == 0 && > + addr->function == 1) || > + (cont->type == VIR_DOMAIN_CONTROLLER_TYPE_USB && cont->idx == 0 && > + (cont->model == VIR_DOMAIN_CONTROLLER_MODEL_USB_PIIX3_UHCI || > + cont->model == -1) && addr->function == 2)) { > + /* Note the check for nbuses > 0 - if there are no PCI > + * buses, we skip this check. This is a quirk required for > + * some machinetypes such as s390, which pretend to have a > + * PCI bus for long enough to generate the "-usb" on the > + * commandline, but that don't really care if a PCI bus > + * actually exists. */ > + if (addrs->nbuses > 0 && > + !(addrs->buses[0].flags & QEMU_PCI_CONNECT_TYPE_PCI)) { > + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", > + _("Bus 0 must be PCI for integrated PIIX3 " > + "USB or IDE controllers")); > + return -1; > + } else { > + return 0; > + } > + } Still very hacky but at least you improved it by providing a code comment as to why and a sensible error message if the user gets in this path. I'd still love to see us improve the situation so we didn't have to play games like this to get -usb to be emitted for different machine types.

> } > > entireSlot = (addr->function == 0 && > @@ -1695,8 +1728,7 @@ qemuDomainAssignPCIAddresses(virDomainDefPtr def, > int nbuses = 0; > size_t i; > int rv; > - qemuDomainPCIConnectFlags flags = (QEMU_PCI_CONNECT_HOTPLUGGABLE | > - QEMU_PCI_CONNECT_TYPE_PCI); > + qemuDomainPCIConnectFlags flags = QEMU_PCI_CONNECT_TYPE_PCI; So just for my edification, we previously were saying that pci-bridge devices were hotpluggable when in fact they are not? You could probably add a code comment above the default flags to that effect, but not strictly necessary.

> for (i = 0; i < def->ncontrollers; i++) { > if (def->controllers[i]->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI) { > @@ -1941,7 +1973,11 @@ int qemuDomainPCIAddressEnsureAddr(qemuDomainPCIAddressSetPtr addrs, > virDomainDeviceInfoPtr dev) > { > int ret = 0; > - /* FIXME: flags should be set according to the particular device */ > + /* Flags should be set according to the particular device, > + * but only the caller knows the type of device. Currently this > + * function is only used for hot-plug, though, and hot-plug is > + * only supported for standard PCI devices, so we can safely use > + * the setting below */ > qemuDomainPCIConnectFlags flags = (QEMU_PCI_CONNECT_HOTPLUGGABLE | > QEMU_PCI_CONNECT_TYPE_PCI); > > @@ -2005,7 +2041,13 @@ qemuDomainPCIAddressGetNextSlot(qemuDomainPCIAddressSetPtr addrs, > virDevicePCIAddressPtr next_addr, > qemuDomainPCIConnectFlags flags) > { > - virDevicePCIAddress a = addrs->lastaddr; > + virDevicePCIAddress a = { 0, 0, 0, 0, false }; Again, me being nitpicky but maybe a comment that says start the search from the top of the PCI addresses by settings this to all 0s and then below the comment says that this is a fast out when the flags match.