[libvirt] [PATCH v6 10/17] qemu: auto-add pcie-root-port/dmi-to-pci-bridge controllers as needed

[Andrea Bolognani]

On Wed, 2016-11-09 at 15:23 -0500, Laine Stump wrote:
> > > +    } 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.

Sorry, I was not clear enough.

What if the function is called like

  virDomainPCIAddressSetGrow(addrs, addr,
                             VIR_PCI_CONNECT_TYPE_PCI_BRIDGE)

because *the caller* is going through a virDomainDef and,
after finding a pci-bridge in it, wants to make sure the
address set can actually fit it? The case when

  flags & VIR_PCI_CONNECT_TYPE_PCI_BRIDGE

can clearly happen, as we're handling it above, but we error
out unless the guest has a pcie-root? That's the bit that I
can't quite wrap my head around.

> > > +    } 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).

I'm clearly missing something here :(

Don't we (potentially) grow the address set using this
function every time we reserve an address for an endpoint
device or a controller? Including when we're going through
a virDomainDef, which might contain a dmi-to-pci-bridge
or a pci{,e}-expander-bus?

I'm not expecting the pci{,e}-root case to be handled, of
course, but I can't figure out why we seem to be skipping
over a bunch of perfectly valid cases.

> > > -        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 :-)

I get where you're coming from, but I don't think I agree
with your conclusions.

Sure, virDomainPCIAddressBusSetModel() might be a trivial
function *now*, but we can't guarantee the same will be
true in the future. Sure, the model we pass to it from
inside virDomainPCIAddressSetGrow() is one of very few
known-good values, but we might eg. read it from a less
reliable source somewhere down the line.

My point is, by not checking the return value we're
implicitly embedding some knowledge about
virDomainPCIAddressSetGrow() inside
virDomainPCIAddressBusSetModel(), thus creating a strong
relationship between the two functions. We should avoid
creating such relationships, because they make the code
much more difficult to reason about and might lead to
unknowingly introducing subtle bugs.

> > 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.

I would never hold my ACK hostage of a purely cosmetic
issue :)

However, as explained above, I'm still unable to wrap my
head around some details, and until I do I'm not comfortable
ACKing the patch. Hopefully you'll be able to help me clear
out any remaining doubts :)

-- 
Andrea Bolognani / Red Hat / Virtualization