On 07/23/2013 11:31 AM, Daniel P. Berrange wrote:
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
Yes, that's correct. I had originally left that code in (setting slot 0 to 0xFF) just to be paranoid, but later convinced myself I could remove it immediately rather than leaving it around to confound someone else in 6 months.
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
Yes. I'm going to add a slightly more compact version of the "287 virtio disk devices" test case from the BZ for pci-bridge. Also I think we need more test cases where the original XML has no guest-side PCI addresses specified, and we put an "xmlout" version in qemuxml2xmloutdata with pci addresses filled in; that way we can verify that the auto-allocation of slots doesn't regress. Thanks for the review. I'll add some test cases and push later tonight.