On Mon, 2016-11-07 at 14:50 -0500, Laine Stump wrote:
> Previously libvirt would only add pci-bridge devices automatically
> when an address was requested for a device that required a legacy PCI
> slot and none was available. This patch expands that support to
> dmi-to-pci-bridge (which is needed in order to add a pci-bridge on a
> machine with a pcie-root), and pcie-root-port (which is needed to add
> a hotpluggable PCIe device). It does *not* automatically add
> pcie-switch-upstream-ports or pcie-switch-downstream-ports (and
> currently there are no plans for that).
[...]
> Although libvirt will now automatically create a dmi-to-pci-bridge
> when it's needed, the code still remains for now that forces a
> dmi-to-pci-bridge on all domains with pcie-root (in
> qemuDomainDefAddDefaultDevices()). That will be removed in the next
> patch.
s/in the next/in a future/
> For now, the pcie-root-ports are added one to a slot, which is a bit
> wasteful and means it will fail after 31 total PCIe devices (30 if
> there are also some PCI devices), but helps keep the changeset down
> for this patch. A future patch will have 8 pcie-root-ports sharing the
> functions on a single slot.
[...]
> @@ -82,6 +82,30 @@ virDomainPCIControllerModelToConnectType(virDomainControllerModelPCI model)
> return 0;
> }
>
> +
> +static int
> +virDomainPCIControllerConnectTypeToModel(virDomainPCIConnectFlags flags)
> +{
> + if (flags & VIR_PCI_CONNECT_TYPE_PCIE_ROOT_PORT)
> + return VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT_PORT;
Maybe adding an empty line between each branch would make
this a little more readable? Just a thought.
Also too bad these are flags and we can't use a switch()
statement here - the compiler will not warn us if we forget
to handle a VIR_PCI_CONNECT_TYPE in the future :(
[...]
> @@ -351,32 +372,73 @@ virDomainPCIAddressSetGrow(virDomainPCIAddressSetPtr addrs,
> {
> int add;
> size_t i;
> + int model;
> + bool needDMIToPCIBridge = false;
>
> add = addr->bus - addrs->nbuses + 1;
> - i = addrs->nbuses;
> if (add <= 0)
> return 0;
>
> - /* auto-grow only works when we're adding plain PCI devices */
> - if (!(flags & VIR_PCI_CONNECT_TYPE_PCI_DEVICE)) {
> - virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> - _("Cannot automatically add a new PCI bus for a "
> - "device requiring a slot other than standard PCI."));
> + if (flags & VIR_PCI_CONNECT_TYPE_PCI_DEVICE) {
> + model = VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE;
> +
> + /* if there aren't yet any buses that will accept a
> + * pci-bridge, and the caller is asking for one, we'll need to
> + * add a dmi-to-pci-bridge first.
> + */
> + needDMIToPCIBridge = true;
> + for (i = 0; i < addrs->nbuses; i++) {
> + if (addrs->buses[i].flags & VIR_PCI_CONNECT_TYPE_PCI_BRIDGE) {
> + needDMIToPCIBridge = false;
> + break;
> + }
> + }
> + if (needDMIToPCIBridge && add == 1) {
> + /* we need to add at least two buses - one dmi-to-pci,
> + * and the other the requested pci-bridge
> + */
> + add++;
> + addr->bus++;
> + }
This last part got me confused for a bit, so I wouldn't mind
having a more detailed comment here. Something like
We need to add a single pci-bridge to provide the bus our
legacy PCI device will be plugged into; however, we have
also determined that the pci-bridge we're about to add
itself needs to be plugged into a dmi-to-pci-bridge. To
accomodate both requirements, we're going to add both a
dmi-to-pci-bridge and a pci-bridge, and we're going to
bump the bus number of the device by one to account for
the extra controller.
Yeah, that's quite a mouthful :/
> + } else if (flags & VIR_PCI_CONNECT_TYPE_PCI_BRIDGE &&
> + addrs->buses[0].model == VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT) {
> + model = VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE;
Mh, what about the case where we want to add a pci-bridge
but the machine has a pci-root instead of a pcie-root?
Shouldn't that case be handled as well?
> + } else if (flags & (VIR_PCI_CONNECT_TYPE_PCIE_DEVICE |
> + VIR_PCI_CONNECT_TYPE_PCIE_SWITCH_UPSTREAM_PORT)) {
> + model = VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT_PORT;
> + } else {
> + int existingContModel = virDomainPCIControllerConnectTypeToModel(flags);
> +
> + if (existingContModel >= 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("a PCI slot is needed to connect a PCI controller "
> + "model='%s', but none is available, and it "
> + "cannot be automatically added"),
> + virDomainControllerModelPCITypeToString(existingContModel));
> + } else {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Cannot automatically add a new PCI bus for a "
> + "device with connect flags %.2x"), flags);
> + }
So we error out for dmi-to-pci-bridge and
pci{,e}-expander-bus... Shouldn't we be able to plug either
into into pcie-root or pci-root?
> return -1;
> }
>
> + i = addrs->nbuses;
> +
> if (VIR_EXPAND_N(addrs->buses, addrs->nbuses, add) < 0)
> return -1;
>
> - for (; i < addrs->nbuses; i++) {
> - /* Any time we auto-add a bus, we will want a multi-slot
> - * bus. Currently the only type of bus we will auto-add is a
> - * pci-bridge, which is hot-pluggable and provides standard
> - * PCI slots.
> + if (needDMIToPCIBridge) {
> + /* first of the new buses is dmi-to-pci-bridge, the
> + * rest are of the requested type
> */
> - virDomainPCIAddressBusSetModel(&addrs->buses[i],
> - VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE);
> + virDomainPCIAddressBusSetModel(&addrs->buses[i++],
> + VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE);
virDomainPCIAddressBusSetModel() can fail, yet you're not
checking the return value neither here...
> }
> +
> + for (; i < addrs->nbuses; i++)
> + virDomainPCIAddressBusSetModel(&addrs->buses[i], model);
... nor here.
[...]
> @@ -947,7 +934,43 @@ qemuDomainPCIAddressSetCreate(virDomainDefPtr def,
>
> if (virDomainPCIAddressBusSetModel(&addrs->buses[idx], cont->model) < 0)
> goto error;
> - }
> +
> + if (cont->model == VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT)
> + hasPCIeRoot = true;
> + }
> +
> + if (nbuses > 0 && !addrs->buses[0].model) {
> + /* This is just here to replicate a safety measure already in
> + * an older version of this code. In practice, the root bus
> + * should have already been added at index 0 prior to
> + * assigning addresses to devices.
> + */
> + if (virDomainPCIAddressBusSetModel(&addrs->buses[0],
> + VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT) < 0)
> + goto error;
> + }
> +
> + /* Now fill in a reasonable model for all the buses in the set
> + * that don't yet have a corresponding controller in the domain
> + * config.
> + */
> +
No empty line here.
Rest looks good, but no ACK for you quite yet :)
--
Andrea Bolognani / Red Hat / Virtualization
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list