On 08/04/2013 07:52 PM, Doug Goldstein wrote:
> On Sat, Aug 3, 2013 at 9:01 PM, Laine Stump <laine@xxxxxxxxx> 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.
Okay. I did that here so that the pcie-root test could also become a
test to assure that the implicit dmi-to-pci-bridge and pci-bridge
devices were added. I'll add that comment.
>
>>
>> <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.
Okay, I'll move it.
>> + 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.
The path to the qemu binary is part of the xml, so it's easily available
(to us, and to the user). None of the "in this QEMU binary" messages
print out the path, but I suppose it's worth discussing.
>> +
>> + /* 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.
I actually got it wrong when I first wrote it (but found the problem
during testing). I'll break it up into two nested ifs, just for sake of
understanding.
> 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.
Okay, I've made those changes plus breaking up the "if from hell".
Thanks for reviewing and testing!
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list