Re: [libvirt] [PATCH 4/5] qemu: set/validate slot/connection type when assigning slots for PCI devices

On Tue, Jul 23, 2013 at 10:44:54AM -0400, Laine Stump wrote: > diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c > index 059aa6a..64787b6 100644 > --- a/src/qemu/qemu_command.c > +++ b/src/qemu/qemu_command.c > @@ -1412,7 +1412,15 @@ cleanup: > #define QEMU_PCI_ADDRESS_FUNCTION_LAST 7 > > typedef struct { > - /* Each bit in a slot represents one function on that slot */ > + virDomainControllerModelPCI model; > + /* flags an min/max can be computed from model, but > + * having them ready makes life easier. > + */ > + qemuDomainPCIConnectFlags flags; > + size_t minSlot, maxSlot; /* usually 0,0 or 1,31 */ > + /* Each bit in a slot represents one function on that slot. If the > + * bit is set, that function is in use by a device. > + */ > uint8_t slots[QEMU_PCI_ADDRESS_SLOT_LAST + 1]; > } qemuDomainPCIAddressBus; > typedef qemuDomainPCIAddressBus *qemuDomainPCIAddressBusPtr; > @@ -1430,9 +1438,13 @@ struct _qemuDomainPCIAddressSet { > * Check that the PCI address is valid for use > * with the specified PCI address set. > */ > -static bool qemuPCIAddressValidate(qemuDomainPCIAddressSetPtr addrs ATTRIBUTE_UNUSED, > - virDevicePCIAddressPtr addr) > +static bool > +qemuPCIAddressValidate(qemuDomainPCIAddressSetPtr addrs ATTRIBUTE_UNUSED, > + virDevicePCIAddressPtr addr, > + qemuDomainPCIConnectFlags flags) > { > + qemuDomainPCIAddressBusPtr bus; > + > if (addrs->nbuses == 0) { > virReportError(VIR_ERR_XML_ERROR, "%s", _("No PCI buses available")); > return false; > @@ -1448,32 +1460,81 @@ static bool qemuPCIAddressValidate(qemuDomainPCIAddressSetPtr addrs ATTRIBUTE_UN > addrs->nbuses - 1); > return false; > } > - if (addr->function > QEMU_PCI_ADDRESS_FUNCTION_LAST) { > + > + bus = &addrs->buses[addr->bus]; > + > + /* assure that at least one of the requested connection types is > + * provided by this bus > + */ > + if (!(flags & bus->flags & QEMU_PCI_CONNECT_TYPES_MASK)) { > virReportError(VIR_ERR_XML_ERROR, > - _("Invalid PCI address: function must be <= %u"), > - QEMU_PCI_ADDRESS_FUNCTION_LAST); > + _("Invalid PCI address: The PCI controller " > + "providing bus %04x doesn't support " > + "connections appropriate for the device " > + "(%x required vs. %x provided by bus)"), > + addr->bus, flags & QEMU_PCI_CONNECT_TYPES_MASK, > + bus->flags & QEMU_PCI_CONNECT_TYPES_MASK); > return false; > } > - if (addr->slot > QEMU_PCI_ADDRESS_SLOT_LAST) { > + /* make sure this bus allows hot-plug if the caller demands it */ > + if ((flags & QEMU_PCI_CONNECT_HOTPLUGGABLE) && > + !(bus->flags & QEMU_PCI_CONNECT_HOTPLUGGABLE)) { > virReportError(VIR_ERR_XML_ERROR, > - _("Invalid PCI address: slot must be <= %u"), > - QEMU_PCI_ADDRESS_SLOT_LAST); > + _("Invalid PCI address: hot-pluggable slot requested, " > + "but bus %04x doesn't support hot-plug"), addr->bus); > return false; > } > - if (addr->slot == 0) { > - if (addr->bus) { > - virReportError(VIR_ERR_XML_ERROR, "%s", > - _("Slot 0 is unusable on PCI bridges")); > - } else { > - virReportError(VIR_ERR_XML_ERROR, "%s", > - _("Slot 0 on bus 0 is reserved for the host bridge")); > - } > + /* some "buses" are really just a single port */ > + if (bus->minSlot && addr->slot < bus->minSlot) { > + virReportError(VIR_ERR_XML_ERROR, > + _("Invalid PCI address: slot must be >= %zu"), > + bus->minSlot); > + return false; > + } > + if (addr->slot > bus->maxSlot) { > + virReportError(VIR_ERR_XML_ERROR, > + _("Invalid PCI address: slot must be <= %zu"), > + bus->maxSlot); > + return false; > + } > + if (addr->function > QEMU_PCI_ADDRESS_FUNCTION_LAST) { > + virReportError(VIR_ERR_XML_ERROR, > + _("Invalid PCI address: function must be <= %u"), > + QEMU_PCI_ADDRESS_FUNCTION_LAST); > + > +static int > +qemuDomainPCIAddressBusSetModel(qemuDomainPCIAddressBusPtr bus, > + virDomainControllerModelPCI model) > +{ > + switch (model) { > + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE: > + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT: > + bus->flags = (QEMU_PCI_CONNECT_HOTPLUGGABLE | > + QEMU_PCI_CONNECT_TYPE_PCI); > + bus->minSlot = 1; > + bus->maxSlot = QEMU_PCI_ADDRESS_SLOT_LAST; > + break; > + default: > + virReportError(VIR_ERR_INTERNAL_ERROR, > + _("Invalid PCI controller model %d"), model); > + return -1; > + } > + > + bus->model = model; > + return 0; > +} > + > + > /* Ensure addr fits in the address set, by expanding it if needed. > + * This will only grow if the flags say that we need a normal > + * hot-pluggable PCI slot. If we need a different type of slot, it > + * will fail. > + * > * Return value: > * -1 = OOM > * 0 = no action performed > @@ -1481,7 +1542,8 @@ static bool qemuPCIAddressValidate(qemuDomainPCIAddressSetPtr addrs ATTRIBUTE_UN > */ > static int > qemuDomainPCIAddressSetGrow(qemuDomainPCIAddressSetPtr addrs, > - virDevicePCIAddressPtr addr) > + virDevicePCIAddressPtr addr, > + qemuDomainPCIConnectFlags flags) > { > int add; > size_t i; > @@ -1490,16 +1552,33 @@ qemuDomainPCIAddressSetGrow(qemuDomainPCIAddressSetPtr addrs, > i = addrs->nbuses; > if (add <= 0) > return 0; > + > + /* auto-grow only works when we're adding plain PCI devices */ > + if (!(flags & QEMU_PCI_CONNECT_TYPE_PCI)) { > + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", > + _("Cannot automatically add a new PCI bus for a " > + "device requiring a slot other than standard PCI.")); > + return -1; > + } > + > if (VIR_EXPAND_N(addrs->buses, addrs->nbuses, add) < 0) > return -1; > - /* reserve slot 0 on the new buses */ > - for (; i < addrs->nbuses; i++) > - addrs->buses[i].slots[0] = 0xFF; > + > + for (; i < addrs->nbuses; i++) { > + /* Any time we auto-add a bus, we will want a multi-slot > + * bus. Currently the only type of bus we will auto-add is a > + * pci-bridge, which is hot-pluggable and provides standard > + * PCI slots. > + */ > + qemuDomainPCIAddressBusSetModel(&addrs->buses[i], > + VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE); > + } > return add; In this old code we're setting slot 0 to 0xFF to reserve it, and qemuDomainPCIAddressBusSetModel does not do that. I think that's ok, since qemuPCIAddressValidate uses minSlot now, but wanted to check that this was correct

ACK if you can answer that q. Definitely could do with some test coverage in the XML -> ARGV convertor for complex cases with multiple bridges in the XML