Re: [libvirt] [PATCHv2 3/7] qemu: add dmi-to-pci-bridge controller

On Sat, Aug 3, 2013 at 9:01 PM, Laine Stump <laine(a)laine.org&gt; wrote: > This PCI controller, named "dmi-to-pci-bridge" in the libvirt config, > and implemented with qemu's "i82801b11-bridge" device, connects to a > PCI Express slot (e.g. one of the slots provided by the pcie-root > controller, aka "pcie.0" on the qemu commandline), and provides 31 > *non-hot-pluggable* PCI (*not* PCIe) slots, numbered 1-31. > > Any time a machine is defined which has a pcie-root controller > (i.e. any q35-based machinetype), libvirt will automatically add a > dmi-to-pci-bridge controller if one doesn't exist, and also add a > pci-bridge controller. The reasoning here is that any useful domain > will have either an immediate (startup time) or eventual (subsequent > hot-plug) need for a standard PCI slot; since the pcie-root controller > only provides PCIe slots, we need to connect a dmi-to-pci-bridge > controller to it in order to get a non-hot-plug PCI slot that we can > then use to connect a pci-bridge - the slots provided by the > pci-bridge will be both standard PCI and hot-pluggable. > > Since pci-bridge devices themselves are not hot-pluggable, any new > pci-bridge controller that is added can (and will) be plugged into the > dmi-to-pci-bridge as long as it has empty slots available. Worth noting DO_TEST_DIFFERENT to pcie-root change here since you mention changes like that in later commits.

> > <p> > PCI controllers have an optional <code>model</code> attribute with > - possible values <code>pci-root</code>, <code>pcie-root</code> > - or <code>pci-bridge</code>. > + possible values <code>pci-root</code>, <code>pcie-root</code>, > + <code>pci-bridge</code>, or <code>dmi-to-pci-bridge</code>. > For machine types which provide an implicit PCI bus, the pci-root > controller with index=0 is auto-added and required to use PCI devices. > pci-root has no address. > + PCI bridges are auto-added if there are too many devices to fit on > + the one bus provided by pci-root, or a PCI bus number greater than zero > + was specified. > PCI bridges can also be specified manually, but their addresses should > only refer to PCI buses provided by already specified PCI controllers. > Leaving gaps in the PCI controller indexes might lead to an invalid > configuration. > - (<span class="since">since 1.0.5</span>) > + (pci-root and pci-bridge <span class="since">since 1.0.5</span>). Probably belonged as part of the last patch technically, but they'll be part of the series so it should be ok.

> + case VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE: > + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE)) { > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", > + _("The dmi-to-pci-bridge (i82801b11-bridge) " > + "controller is not supported in this QEMU binary")); Do we ever print out the path to the QEMU binary used in these kinds of errors? Might be nice.

> + > + /* When a machine has a pcie-root, make sure that there is always > + * a dmi-to-pci-bridge controller added as bus 1, and a pci-bridge > + * as bus 2, so that standard PCI devices can be connected > + */ > if (addPCIeRoot && > - virDomainDefMaybeAddController( > + (virDomainDefMaybeAddController( > def, VIR_DOMAIN_CONTROLLER_TYPE_PCI, 0, > - VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT) < 0) > + VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT) < 0 || > + virDomainDefMaybeAddController( > + def, VIR_DOMAIN_CONTROLLER_TYPE_PCI, 1, > + VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE) < 0 || > + virDomainDefMaybeAddController( > + def, VIR_DOMAIN_CONTROLLER_TYPE_PCI, 2, > + VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE) < 0)) > return -1; Not going to lie, that's an if check of hell. Had to really stare for a second to make sure everything was lined up correctly.

Overall an ACK, just fix up the commit message and move the since documentation fixup to 2/7, which I think you can do without reposting.