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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 11/10/2016 09:10 AM, Andrea Bolognani wrote:
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?

Okay, two different scenarios, each for either pci-root or pcie-root:

1) pci-bridge requested with no address supplied

a) pci-root - if there is an open slot it will be assigned to the pci-bridge. If there isn't an open slot, then there is no chance for success anyway,
       so the correct error message will be correct:

          a PCI slot is neede to connect a PCI coontller model='pci-bridge'
          but none is available, and it cannot be automatically added.

     (because the only way to add a new open slot is to add a pci-bridge,
     and that's what we're already trying and failing to do).

b) pcie-root - if there is an open slot, then we would never get into ...Grow(), if there isn't then we need to add a dmi-to-pci-bridge (because we can only plug into a pci-bridge or a dmi-to-pci-bridge, and we've already established that we can't directly plug in a pci-bridge. So again, behavior is correct

2) pci-bridge was requested with user-supplied address that is on a nonexistent bus, and that bus number is >1 over the current last bus (i.e. there is a gap in the controller indexes). This would happen if, for example, someone put this in their xml:

        <controller type='pci' index='0' model='pci-root'/>
        <controller type='pci' index='2' model='pci-bridge'/>

(if libvirt was setting the index, it would have set the index for the pci-bridge to '1').

a) pci-root - okay, here's where what you're saying would apply - we need to add extra pci-bridges
        to fill in the "empty space" in the list of controllers.

    b) pcie-root - actually we have the same issue in this case.

Personally I think that it's nonsensical for a user to do that, and trying to support "empty regions" in the PCI address space by auto-adding pci-bridges is pointless busywork, but we have supported it in the past, so I guess we need to now. Sigh.


That's the bit that I
can't quite wrap my head around.

Because it's a silly thing to support. But we already do, so we have to continue :-(


+    } 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?

No. It is only called when we look for an open slot that matches the connectFlags of the device/controller, and don't find any open slot with compatible flags.


  Including when we're going through
a virDomainDef, which might contain a dmi-to-pci-bridge
or a pci{,e}-expander-bus?

Sure, it can contain one of those. But the only place you can plug in a pci[e]-expander-bus is pci[e]-root, and you can't add a new pci[e]-root, so if you've looked for an empty slot and there isn't one, then you have an error. Since the ...Grow() function is only called if you've already failed to find an existing appropriate slot, it's correct for it to give an error.

A dmi-to-pci-bridge is *slightly* different - it could also be connected to a pcie-expander-bus. But we don't automatically add those either, since they're really intended for supporting NUMA (and anyway, if there is no free slot in pcie-root to plug in a dmi-to-pci-bridge, then there is *also* no free slot to plug in a pcie-expander-bus). So again, if you've looked for an open slot to plug in a dmi-to-pci-bridge and couldn't find one, then when you call this function there is nothing productive it can do, so it's proper for it to fail and log an error.


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.

Because they're not 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.

I think that putting in error checking/recovery code for such simple cases is just adding more code to maintain and get wrong for no gain. (similar to checking the return from virBlahTypeToString() in cases where we know the enum value has already been validated). But I'll cede your point and add return checks on all the calls for virDomainPCIAddressBusSetModel().
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 :)

I'll add the return value checks, and add support for calling ...Grow() with an address > 1 over the current bus count.

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]