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

[Andrea Bolognani]

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.

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