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

On 08/04/2013 05:01 PM, Doug Goldstein wrote: > 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. Yes. Totally agree. >> } >> >> 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. You can hot-plug a device into a pci-bridge, but a pci-bridge itself cannot be hot-plugged into the system. (Maybe a better way to describe it is that pci-bridge slots can accept hot-plugs, but the pci-bridge device cannot be hot-plugged. No, that's not any better. Let's see...)

> >> 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. Is this better? /* default to starting the search for a free slot from * 0000:00:00.0 */ virDevicePCIAddress a = { 0, 0, 0, 0, false }; /* except if this search is for the exact same type of device as * last time, continue the search from the previous match */ if (flags == addrs->lastFlags) a = addrs->lastaddr; I squashed that in.