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

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