2:23 p.m.
On 11/09/2016 09:19 AM, Andrea Bolognani wrote:
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.
Okay.
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 :(
Yeah, I may rework the flags someday to separate out the hotplug and
aggregate_slot flags so that the connect type can be switched on.
[...]
> @@ -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 :/
Okay, I added a variation of that description.
> + } 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?
It is handled - we'll want to add a pci-bridge if flags &
VIR_PCI_TYPE_PCI_DEVICE is true (i.e. the first clause). In that case,
since pci-root has the flag VIR_PCI_CONNECT_TYPE_PCI_BRIDGE set, the for
loop will set neetDMIToPCIBridge = false, so we will end up adding just
the one pci-bridge that we need.
> + } 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?
Exactly. And since those buses already exist from the start, and can't
be added later, there wouldn't ever be a situation where we needed to
automatically grow the bus structure due to one of those devices and
growing could actually do anything (i.e. you can't add a 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...
Yes, but the only way it can fail is if an unknown controller model is
sent to it, and we can see by simple physical inspection that that isn't
the case. If this was calling off to a function in some other file where
we didn't know just how simple it is and couldn't easily see that it
would never fail, then I would agree that we should check, but in this
case it's superfluous - if this function had an ATTRIBUTE_RETURN_CHECK
on it, I would have put these calls inside ignore_value().
> }
> +
> + for (; i < addrs->nbuses; i++)
> + virDomainPCIAddressBusSetModel(&addrs->buses[i], model);
... nor here.
Same for this - the controller model is one of just a couple hardcoded
values set within the same function, not sent in from somewhere else,
and the function we're calling is a very simple function in the same file.
[...]
> @@ -947,7 +934,43 @@ qemuDomainPCIAddressSetCreate(virDomainDefPtr def,
>
> if (virDomainPCIAddressBusSetModel(&addrs->buses[idx],
cont->model) < 0)
> goto error;
> - }
Since I'm sure you'll bring up the fact that I *am* checking the return
value of virDomainPCIAddressBusSetModel here (and everywhere else in
qemu_domain_address.c), I'll point out that these calls are made from
another file in another directory, so I'm being overly paranoid, not on
any real factual basis, but just because it feels right :-)
> +
> + 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 :)
So far I've changed comments and whitespace. You're welcome to dispute
my opinion about checking return value from the SetModel function, but
otherwise there's only cosmetic differences.
--
Andrea Bolognani / Red Hat / Virtualization