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

[Laine Stump]

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.