On 08/04/2013 05:01 PM, Doug Goldstein 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
On Sat, Aug 3, 2013 at 9:01 PM, Laine Stump <laine@laine.org> wrote: 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.