[libvirt] [PATCH v2 1/4] conf: restrict what type of buses will accept a pci-bridge

[Laine Stump]

On 08/16/2016 10:17 AM, Andrea Bolognani wrote:
> On Mon, 2016-08-15 at 01:50 -0400, Laine Stump wrote:
>> A pci-bridge has *almost* the same rules as a legacy PCI endpoint
>> device for where it can be automatically connected, and until now both
>> had been considered identical. There is one pairing that is okay when
>> specifically requested by the user (i.e. manual assignment), but we
>> want to avoid it when auto-assigning addresses - plugging a pci-bridge
> s/it //
>
> Possibly, I don't know, you're the native speaker ;)
>
>> directly into pcie-root (it is cleaner to plug in a dmi-to-pci-bridge,
>> then plug the pci-bridge into that).
>>   
>> In order to allow that difference, this patch makes a separate
>> CONNECT_TYPE for pci-bridge, and uses it to restrict auto-assigned
>> addresses for pci-bridges to be only on pci-root, pci-expander-bus,
>> dmi-to-pci-bridge, or on another pci-bridge.
>>   
>> NB: As with other discouraged-but-seem-to-work configurations
>> (e.g. plugging a legacy PCI device into a pcie-root-port) if someone
>> *really* wants to, they can still force a pci-bridge to be plugged
>> into pcie-root (by manually specifying its PCI address.)
>> ---
>>    src/conf/domain_addr.c | 29 ++++++++++++++++++++++-------
>>    src/conf/domain_addr.h |  4 +++-
>>    2 files changed, 25 insertions(+), 8 deletions(-)
>>   
>> diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c
>> index c533edb..88e44df 100644
>> --- a/src/conf/domain_addr.c
>> +++ b/src/conf/domain_addr.c
>> @@ -51,11 +51,14 @@ virDomainPCIControllerModelToConnectType(virDomainControllerModelPCI model)
>>            return 0;
>>    
>>        case VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE:
>> -        /* pci-bridge is treated like a standard
>> -         * PCI endpoint device, because it can plug into any
>> -         * standard PCI slot (it just can't be hotplugged).
>> +        /* pci-bridge is treated like a standard PCI endpoint device
>> +         * when its address is manually assigned, but needs special
>> +         * treatment when auto-assigning addresses (in that case we
>> +         * avoid plugging into anything except pci-root,
>> +         * dmi-to-pci-bridge, pci-expander-bus, or another
>> +         * pci-bridge).
>>             */
>> -        return VIR_PCI_CONNECT_TYPE_PCI_DEVICE;
>> +        return VIR_PCI_CONNECT_TYPE_PCI_BRIDGE;
> As mentioned as part of my review of the previous PCIe series,
> I don't think this kind of comment should be here. The function
> just maps controller model to connect type, and if anything
> this change makes the mapping even more straighforward.

That's strange. I could *swear* that I removed that comment when I was 
rebasing. Of course then I created a completely new branch and 
cherry-picked the commits I wanted using the commit id's from an 
instance of gitk that was sitting on the desktop - maybe it had been run 
prior to the deletion and I hadn't reloaded it...


>
> See below.
>
>>        case VIR_DOMAIN_CONTROLLER_MODEL_PCI_EXPANDER_BUS:
>>            return VIR_PCI_CONNECT_TYPE_PCI_EXPANDER_BUS;
>> @@ -110,6 +113,12 @@ virDomainPCIAddressFlagsCompatible(virPCIDeviceAddressPtr addr,
>>             */
>>            if (devFlags & VIR_PCI_CONNECT_HOTPLUGGABLE)
>>                busFlags |= VIR_PCI_CONNECT_HOTPLUGGABLE;
>> +        /* if the device is a pci-bridge, allow manually
>> +         * assigning to any bus that would also accept a
>> +         * standard PCI device.
>> +         */
>> +        if (devFlags & VIR_PCI_CONNECT_TYPE_PCI_BRIDGE)
>> +            devFlags |= VIR_PCI_CONNECT_TYPE_PCI_DEVICE;
> This is the kind of comment we need :)
>
> ACK with the nits fixed.
>
> -- 
> Andrea Bolognani / Red Hat / Virtualization
>