[libvirt] [PATCH 0/5] Support for pcie-root / Q35 machine type
This is *almost* usable. I still need to add support for a dmi-to-pci-bridge before the q35 machine type can be used. 1/5 and 3/5 are just cleaning up some things that bothered me while writing patch 4/5. 2/5 should help out other machine types (e.g. arm) just a bit, by eliminating the extra PIIX3 devices unless the machine actually has a PIIX3 chip. 4/5 has the bulk of the changes - it restructures the PCI address validation/allocation so that it doesn't assume that all PCI addresses are the same, or that all PCI slots are the same. It sets up an enum where different types of PCI addresses can be listed, and the validation code checks that the slot about to be used for a device is actually compatible with that device. 5/5 adds the pcie-root controller which is implicit in the q35 machinetype. Of course since we can only connect pcie devices to this controller, and there are no pcie devices supported, it is not really worth much. However, the next patch (in progress) will add a dmi-to-pci-bridge controller, which *can* plug into the pcie-root and provide plain PCI slots, making a working a35 possible. Note that the auto-allocation doesn't attempt to auto-allocate a slot for any type of device other than plain PCI. This means that even when the dmi-to-pci-bridge controller is added, its address on pcie-root (and a pci-bridge device that must be connected to the dmi-to-pci-bridge controller) will need to be manually specified. Auto-placement will be an exercise for later. Things missing: 1) I need a test case for auto-adding multiple bridges. I will add that this afternoon. 2) I need a test case for a q35 domain. That wil also be added this afternoon. Laine Stump (5): qemu: turn qemuDomainPCIAddressBus into a struct qemu: only check for PIIX3-specific device addrs on pc-* machinetypes qemu: make QEMU_PCI_ADDRESS_(SLOT|FUNCTION)_LAST less misleading qemu: set/validate slot/connection type when assigning slots for PCI devices conf: add pcie-root controller docs/formatdomain.html.in | 17 +- src/conf/domain_conf.c | 4 +- src/conf/domain_conf.h | 5 +- src/libvirt_private.syms | 1 + src/qemu/qemu_command.c | 559 ++++++++++++++++++++++++++++++---------------- src/qemu/qemu_command.h | 28 ++- src/qemu/qemu_domain.c | 23 +- 7 files changed, 434 insertions(+), 203 deletions(-) -- 1.7.11.7
qemuDomainPCIAddressBus was an array of QEMU_PCI_ADDRESS_SLOT_LAST uint8_t's, which worked fine as long as every PCI bus was identical. In the future, some PCI busses will allow connecting PCI devices, and some will allow PCIe devices; also some will only allow connection of a single device, while others will allow connecting 31 devices. In order to keep track of that information for each bus, we need to turn qemuDomainPCIAddressBus into a struct, for now with just one member: uint8_t slots[QEMU_PCI_ADDRESS_SLOT_LAST]; Additional members will come in later patches. The item in qemuDomainPCIAddresSet that contains the array of qemuDomainPCIAddressBus is now called "buses" to be more consistent with the already existing "nbuses" (and with the new "slots" array). --- src/qemu/qemu_command.c | 47 ++++++++++++++++++++++++----------------------- 1 file changed, 24 insertions(+), 23 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 4a49d81..7f5dab9 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -1411,15 +1411,16 @@ cleanup: #define QEMU_PCI_ADDRESS_SLOT_LAST 32 #define QEMU_PCI_ADDRESS_FUNCTION_LAST 8 -/* - * Each bit represents a function - * Each byte represents a slot - */ -typedef uint8_t qemuDomainPCIAddressBus[QEMU_PCI_ADDRESS_SLOT_LAST]; +typedef struct { + /* Each bit in a slot represents one function on that slot */ + uint8_t slots[QEMU_PCI_ADDRESS_SLOT_LAST]; +} qemuDomainPCIAddressBus; +typedef qemuDomainPCIAddressBus *qemuDomainPCIAddressBusPtr; + struct _qemuDomainPCIAddressSet { - qemuDomainPCIAddressBus *used; + qemuDomainPCIAddressBus *buses; + size_t nbuses; virDevicePCIAddress lastaddr; - size_t nbuses; /* allocation of 'used' */ bool dryRun; /* on a dry run, new buses are auto-added and addresses aren't saved in device infos */ }; @@ -1489,11 +1490,11 @@ qemuDomainPCIAddressSetGrow(qemuDomainPCIAddressSetPtr addrs, i = addrs->nbuses; if (add <= 0) return 0; - if (VIR_EXPAND_N(addrs->used, addrs->nbuses, add) < 0) + 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->used[i][0] = 0xFF; + addrs->buses[i].slots[0] = 0xFF; return add; } @@ -1559,7 +1560,7 @@ static int qemuCollectPCIAddress(virDomainDefPtr def ATTRIBUTE_UNUSED, if (!(str = qemuPCIAddressAsString(addr))) goto cleanup; - if (addrs->used[addr->bus][addr->slot] & (1 << addr->function)) { + if (addrs->buses[addr->bus].slots[addr->slot] & (1 << addr->function)) { if (info->addr.pci.function != 0) { virReportError(VIR_ERR_XML_ERROR, _("Attempted double use of PCI Address '%s' " @@ -1575,7 +1576,7 @@ static int qemuCollectPCIAddress(virDomainDefPtr def ATTRIBUTE_UNUSED, if ((info->addr.pci.function == 0) && (info->addr.pci.multi != VIR_DEVICE_ADDRESS_PCI_MULTI_ON)) { /* a function 0 w/o multifunction=on must reserve the entire slot */ - if (addrs->used[addr->bus][addr->slot]) { + if (addrs->buses[addr->bus].slots[addr->slot]) { virReportError(VIR_ERR_XML_ERROR, _("Attempted double use of PCI Address on slot '%s' " "(need \"multifunction='off'\" for device " @@ -1583,11 +1584,11 @@ static int qemuCollectPCIAddress(virDomainDefPtr def ATTRIBUTE_UNUSED, str); goto cleanup; } - addrs->used[addr->bus][addr->slot] = 0xFF; + addrs->buses[addr->bus].slots[addr->slot] = 0xFF; VIR_DEBUG("Remembering PCI slot: %s (multifunction=off)", str); } else { VIR_DEBUG("Remembering PCI addr: %s", str); - addrs->used[addr->bus][addr->slot] |= 1 << addr->function; + addrs->buses[addr->bus].slots[addr->slot] |= 1 << addr->function; } ret = 0; cleanup: @@ -1707,7 +1708,7 @@ qemuDomainPCIAddressSetPtr qemuDomainPCIAddressSetCreate(virDomainDefPtr def, if (VIR_ALLOC(addrs) < 0) goto error; - if (VIR_ALLOC_N(addrs->used, nbuses) < 0) + if (VIR_ALLOC_N(addrs->buses, nbuses) < 0) goto error; addrs->nbuses = nbuses; @@ -1716,7 +1717,7 @@ qemuDomainPCIAddressSetPtr qemuDomainPCIAddressSetCreate(virDomainDefPtr def, /* reserve slot 0 in every bus - it's used by the host bridge on bus 0 * and unusable on PCI bridges */ for (i = 0; i < nbuses; i++) - addrs->used[i][0] = 0xFF; + addrs->buses[i].slots[0] = 0xFF; if (virDomainDeviceInfoIterate(def, qemuCollectPCIAddress, addrs) < 0) goto error; @@ -1734,7 +1735,7 @@ error: static bool qemuDomainPCIAddressSlotInUse(qemuDomainPCIAddressSetPtr addrs, virDevicePCIAddressPtr addr) { - return !!addrs->used[addr->bus][addr->slot]; + return !!addrs->buses[addr->bus].slots[addr->slot]; } int qemuDomainPCIAddressReserveAddr(qemuDomainPCIAddressSetPtr addrs, @@ -1750,7 +1751,7 @@ int qemuDomainPCIAddressReserveAddr(qemuDomainPCIAddressSetPtr addrs, VIR_DEBUG("Reserving PCI addr %s", str); - if (addrs->used[addr->bus][addr->slot] & (1 << addr->function)) { + if (addrs->buses[addr->bus].slots[addr->slot] & (1 << addr->function)) { virReportError(VIR_ERR_INTERNAL_ERROR, _("unable to reserve PCI address %s"), str); VIR_FREE(str); @@ -1762,7 +1763,7 @@ int qemuDomainPCIAddressReserveAddr(qemuDomainPCIAddressSetPtr addrs, addrs->lastaddr = *addr; addrs->lastaddr.function = 0; addrs->lastaddr.multi = 0; - addrs->used[addr->bus][addr->slot] |= 1 << addr->function; + addrs->buses[addr->bus].slots[addr->slot] |= 1 << addr->function; return 0; } @@ -1779,7 +1780,7 @@ int qemuDomainPCIAddressReserveSlot(qemuDomainPCIAddressSetPtr addrs, VIR_DEBUG("Reserving PCI slot %s", str); - if (addrs->used[addr->bus][addr->slot]) { + if (addrs->buses[addr->bus].slots[addr->slot]) { virReportError(VIR_ERR_INTERNAL_ERROR, _("unable to reserve PCI slot %s"), str); VIR_FREE(str); @@ -1787,7 +1788,7 @@ int qemuDomainPCIAddressReserveSlot(qemuDomainPCIAddressSetPtr addrs, } VIR_FREE(str); - addrs->used[addr->bus][addr->slot] = 0xFF; + addrs->buses[addr->bus].slots[addr->slot] = 0xFF; return 0; } @@ -1820,7 +1821,7 @@ int qemuDomainPCIAddressEnsureAddr(qemuDomainPCIAddressSetPtr addrs, int qemuDomainPCIAddressReleaseAddr(qemuDomainPCIAddressSetPtr addrs, virDevicePCIAddressPtr addr) { - addrs->used[addr->bus][addr->slot] &= ~(1 << addr->function); + addrs->buses[addr->bus].slots[addr->slot] &= ~(1 << addr->function); return 0; } @@ -1831,7 +1832,7 @@ qemuDomainPCIAddressReleaseSlot(qemuDomainPCIAddressSetPtr addrs, if (!qemuPCIAddressValidate(addrs, addr)) return -1; - addrs->used[addr->bus][addr->slot] = 0; + addrs->buses[addr->bus].slots[addr->slot] = 0; return 0; } @@ -1840,7 +1841,7 @@ void qemuDomainPCIAddressSetFree(qemuDomainPCIAddressSetPtr addrs) if (!addrs) return; - VIR_FREE(addrs->used); + VIR_FREE(addrs->buses); VIR_FREE(addrs); } -- 1.7.11.7
On Tue, Jul 23, 2013 at 10:44:51AM -0400, Laine Stump wrote:
qemuDomainPCIAddressBus was an array of QEMU_PCI_ADDRESS_SLOT_LAST uint8_t's, which worked fine as long as every PCI bus was identical. In the future, some PCI busses will allow connecting PCI devices, and some will allow PCIe devices; also some will only allow connection of a single device, while others will allow connecting 31 devices.
In order to keep track of that information for each bus, we need to turn qemuDomainPCIAddressBus into a struct, for now with just one member:
uint8_t slots[QEMU_PCI_ADDRESS_SLOT_LAST];
Additional members will come in later patches.
The item in qemuDomainPCIAddresSet that contains the array of qemuDomainPCIAddressBus is now called "buses" to be more consistent with the already existing "nbuses" (and with the new "slots" array). --- src/qemu/qemu_command.c | 47 ++++++++++++++++++++++++----------------------- 1 file changed, 24 insertions(+), 23 deletions(-)
ACK, no functional change here. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
The implicit IDE, USB, and video controllers provided by the PIIX3 chipset in the pc-* machinetypes are not present on other machinetypes, so we shouldn't be doing the special checking for them. The diffs for this patch look hairy, but that's just because a large section was reindented (to be placed inside a conditional) and git couldn't figure out a sane diff. It really is just 1) determining if this system uses PIIX3, 2) put the stuff that's PIIX3-specific inside an if. (Note that, according to the qemuxml2argv-pseries-usb-multi test, ppc "pseries" machines also have a PIIX3 chip (since that test file adds a "piix3-uhci" usb controller). I don't know if this is really the case or not, but had to include that machine type in the checks in order for make check to succeed with no changes to the test data.) --- src/qemu/qemu_command.c | 190 +++++++++++++++++++++++++----------------------- 1 file changed, 99 insertions(+), 91 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 7f5dab9..f85e896 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -1993,111 +1993,119 @@ qemuAssignDevicePCISlots(virDomainDefPtr def, bool qemuDeviceVideoUsable = virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VIDEO_PRIMARY); virDevicePCIAddress tmp_addr; virDevicePCIAddressPtr addrptr; - - /* Verify that first IDE and USB controllers (if any) is on the PIIX3, fn 1 */ - for (i = 0; i < def->ncontrollers; i++) { - /* First IDE controller lives on the PIIX3 at slot=1, function=1 */ - if (def->controllers[i]->type == VIR_DOMAIN_CONTROLLER_TYPE_IDE && - def->controllers[i]->idx == 0) { - if (def->controllers[i]->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) { - if (def->controllers[i]->info.addr.pci.domain != 0 || - def->controllers[i]->info.addr.pci.bus != 0 || - def->controllers[i]->info.addr.pci.slot != 1 || - def->controllers[i]->info.addr.pci.function != 1) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Primary IDE controller must have PCI address 0:0:1.1")); - goto error; + bool isPIIX3 = STRPREFIX(def->os.machine, "pc-0.") || + STRPREFIX(def->os.machine, "pc-1.") || + STRPREFIX(def->os.machine, "pc-i440") || + STREQ(def->os.machine, "pc") || + STRPREFIX(def->os.machine, "rhel") || + STREQ(def->os.machine, "pseries"); + + if (isPIIX3) { + /* Verify that first IDE and USB controllers (if any) is on the PIIX3, fn 1 */ + for (i = 0; i < def->ncontrollers; i++) { + /* First IDE controller lives on the PIIX3 at slot=1, function=1 */ + if (def->controllers[i]->type == VIR_DOMAIN_CONTROLLER_TYPE_IDE && + def->controllers[i]->idx == 0) { + if (def->controllers[i]->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) { + if (def->controllers[i]->info.addr.pci.domain != 0 || + def->controllers[i]->info.addr.pci.bus != 0 || + def->controllers[i]->info.addr.pci.slot != 1 || + def->controllers[i]->info.addr.pci.function != 1) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Primary IDE controller must have PCI address 0:0:1.1")); + goto error; + } + } else { + def->controllers[i]->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI; + def->controllers[i]->info.addr.pci.domain = 0; + def->controllers[i]->info.addr.pci.bus = 0; + def->controllers[i]->info.addr.pci.slot = 1; + def->controllers[i]->info.addr.pci.function = 1; } - } else { - def->controllers[i]->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI; - def->controllers[i]->info.addr.pci.domain = 0; - def->controllers[i]->info.addr.pci.bus = 0; - def->controllers[i]->info.addr.pci.slot = 1; - def->controllers[i]->info.addr.pci.function = 1; - } - } else if (def->controllers[i]->type == VIR_DOMAIN_CONTROLLER_TYPE_USB && - def->controllers[i]->idx == 0 && - (def->controllers[i]->model == VIR_DOMAIN_CONTROLLER_MODEL_USB_PIIX3_UHCI || - def->controllers[i]->model == -1)) { - if (def->controllers[i]->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) { - if (def->controllers[i]->info.addr.pci.domain != 0 || - def->controllers[i]->info.addr.pci.bus != 0 || - def->controllers[i]->info.addr.pci.slot != 1 || - def->controllers[i]->info.addr.pci.function != 2) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("PIIX3 USB controller must have PCI address 0:0:1.2")); - goto error; + } else if (def->controllers[i]->type == VIR_DOMAIN_CONTROLLER_TYPE_USB && + def->controllers[i]->idx == 0 && + (def->controllers[i]->model == VIR_DOMAIN_CONTROLLER_MODEL_USB_PIIX3_UHCI || + def->controllers[i]->model == -1)) { + if (def->controllers[i]->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) { + if (def->controllers[i]->info.addr.pci.domain != 0 || + def->controllers[i]->info.addr.pci.bus != 0 || + def->controllers[i]->info.addr.pci.slot != 1 || + def->controllers[i]->info.addr.pci.function != 2) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("PIIX3 USB controller must have PCI address 0:0:1.2")); + goto error; + } + } else { + def->controllers[i]->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI; + def->controllers[i]->info.addr.pci.domain = 0; + def->controllers[i]->info.addr.pci.bus = 0; + def->controllers[i]->info.addr.pci.slot = 1; + def->controllers[i]->info.addr.pci.function = 2; } - } else { - def->controllers[i]->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI; - def->controllers[i]->info.addr.pci.domain = 0; - def->controllers[i]->info.addr.pci.bus = 0; - def->controllers[i]->info.addr.pci.slot = 1; - def->controllers[i]->info.addr.pci.function = 2; } } - } - - /* PIIX3 (ISA bridge, IDE controller, something else unknown, USB controller) - * hardcoded slot=1, multifunction device - */ - if (addrs->nbuses) { - memset(&tmp_addr, 0, sizeof(tmp_addr)); - tmp_addr.slot = 1; - if (qemuDomainPCIAddressReserveSlot(addrs, &tmp_addr) < 0) - goto error; - } - if (def->nvideos > 0) { - virDomainVideoDefPtr primaryVideo = def->videos[0]; - if (primaryVideo->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) { - primaryVideo->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI; - primaryVideo->info.addr.pci.domain = 0; - primaryVideo->info.addr.pci.bus = 0; - primaryVideo->info.addr.pci.slot = 2; - primaryVideo->info.addr.pci.function = 0; - addrptr = &primaryVideo->info.addr.pci; - - if (!qemuPCIAddressValidate(addrs, addrptr)) + /* PIIX3 (ISA bridge, IDE controller, something else unknown, USB controller) + * hardcoded slot=1, multifunction device + */ + if (addrs->nbuses) { + memset(&tmp_addr, 0, sizeof(tmp_addr)); + tmp_addr.slot = 1; + if (qemuDomainPCIAddressReserveSlot(addrs, &tmp_addr) < 0) goto error; + } - if (qemuDomainPCIAddressSlotInUse(addrs, addrptr)) { - if (qemuDeviceVideoUsable) { - virResetLastError(); - if (qemuDomainPCIAddressSetNextAddr(addrs, &primaryVideo->info) < 0) + if (def->nvideos > 0) { + virDomainVideoDefPtr primaryVideo = def->videos[0]; + if (primaryVideo->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) { + primaryVideo->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI; + primaryVideo->info.addr.pci.domain = 0; + primaryVideo->info.addr.pci.bus = 0; + primaryVideo->info.addr.pci.slot = 2; + primaryVideo->info.addr.pci.function = 0; + addrptr = &primaryVideo->info.addr.pci; + + if (!qemuPCIAddressValidate(addrs, addrptr)) + goto error; + + if (qemuDomainPCIAddressSlotInUse(addrs, addrptr)) { + if (qemuDeviceVideoUsable) { + virResetLastError(); + if (qemuDomainPCIAddressSetNextAddr(addrs, &primaryVideo->info) < 0) + goto error; + } else { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("PCI address 0:0:2.0 is in use, " + "QEMU needs it for primary video")); goto error; - } else { + } + } else if (qemuDomainPCIAddressReserveSlot(addrs, addrptr) < 0) { + goto error; + } + } else if (!qemuDeviceVideoUsable) { + if (primaryVideo->info.addr.pci.domain != 0 || + primaryVideo->info.addr.pci.bus != 0 || + primaryVideo->info.addr.pci.slot != 2 || + primaryVideo->info.addr.pci.function != 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("PCI address 0:0:2.0 is in use, " - "QEMU needs it for primary video")); + _("Primary video card must have PCI address 0:0:2.0")); goto error; } - } else if (qemuDomainPCIAddressReserveSlot(addrs, addrptr) < 0) { - goto error; + /* If TYPE==PCI, then qemuCollectPCIAddress() function + * has already reserved the address, so we must skip */ } - } else if (!qemuDeviceVideoUsable) { - if (primaryVideo->info.addr.pci.domain != 0 || - primaryVideo->info.addr.pci.bus != 0 || - primaryVideo->info.addr.pci.slot != 2 || - primaryVideo->info.addr.pci.function != 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Primary video card must have PCI address 0:0:2.0")); + } else if (addrs->nbuses && !qemuDeviceVideoUsable) { + memset(&tmp_addr, 0, sizeof(tmp_addr)); + tmp_addr.slot = 2; + + if (qemuDomainPCIAddressSlotInUse(addrs, &tmp_addr)) { + VIR_DEBUG("PCI address 0:0:2.0 in use, future addition of a video" + " device will not be possible without manual" + " intervention"); + virResetLastError(); + } else if (qemuDomainPCIAddressReserveSlot(addrs, &tmp_addr) < 0) { goto error; } - /* If TYPE==PCI, then qemuCollectPCIAddress() function - * has already reserved the address, so we must skip */ - } - } else if (addrs->nbuses && !qemuDeviceVideoUsable) { - memset(&tmp_addr, 0, sizeof(tmp_addr)); - tmp_addr.slot = 2; - - if (qemuDomainPCIAddressSlotInUse(addrs, &tmp_addr)) { - VIR_DEBUG("PCI address 0:0:2.0 in use, future addition of a video" - " device will not be possible without manual" - " intervention"); - virResetLastError(); - } else if (qemuDomainPCIAddressReserveSlot(addrs, &tmp_addr) < 0) { - goto error; } } -- 1.7.11.7
On Tue, Jul 23, 2013 at 10:44:52AM -0400, Laine Stump wrote:
The implicit IDE, USB, and video controllers provided by the PIIX3 chipset in the pc-* machinetypes are not present on other machinetypes, so we shouldn't be doing the special checking for them.
The diffs for this patch look hairy, but that's just because a large section was reindented (to be placed inside a conditional) and git couldn't figure out a sane diff. It really is just 1) determining if this system uses PIIX3, 2) put the stuff that's PIIX3-specific inside an if.
(Note that, according to the qemuxml2argv-pseries-usb-multi test, ppc "pseries" machines also have a PIIX3 chip (since that test file adds a "piix3-uhci" usb controller). I don't know if this is really the case or not, but had to include that machine type in the checks in order for make check to succeed with no changes to the test data.) --- src/qemu/qemu_command.c | 190 +++++++++++++++++++++++++----------------------- 1 file changed, 99 insertions(+), 91 deletions(-)
I'm thinking that it would probably be better to move all the re-indented code out into a qemuValidateDevicePCISlotsPIIX3() and just call that function from qemuAssignDevicePCISlots(). That way if we need to add more validation for other machine types in the future, we have a good modular code structure. This would probably make the diff more sane too, since you wouldn't be indenting code. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 07/23/2013 11:24 AM, Daniel P. Berrange wrote:
I'm thinking that it would probably be better to move all the re-indented code out into a qemuValidateDevicePCISlotsPIIX3() and just call that function from qemuAssignDevicePCISlots(). That way if we need to add more validation for other machine types in the future, we have a good modular code structure. This would probably make the diff more sane too, since you wouldn't be indenting code.
Ah yes, good idea! I'll do that and resend. On Tue, Jul 23, 2013 at 10:44:52AM -0400, Laine Stump wrote:
(Note that, according to the qemuxml2argv-pseries-usb-multi test, ppc "pseries" machines also have a PIIX3 chip (since that test file adds a "piix3-uhci" usb controller). I don't know if this is really the case or not, but had to include that machine type in the checks in order for make check to succeed with no changes to the test data.)
Anyone have better information about this? Does the pseries really have a PIIX3? Or was that just an arbitrary entry added for a test case?
Il 24/07/2013 00:18, Laine Stump ha scritto:
On Tue, Jul 23, 2013 at 10:44:52AM -0400, Laine Stump wrote:
(Note that, according to the qemuxml2argv-pseries-usb-multi test, ppc "pseries" machines also have a PIIX3 chip (since that test file adds a "piix3-uhci" usb controller). I don't know if this is really the case or not, but had to include that machine type in the checks in order for make check to succeed with no changes to the test data.)
Anyone have better information about this? Does the pseries really have a PIIX3? Or was that just an arbitrary entry added for a test case?
It's an arbitrary choice---the "piix3-uhci" USB controller is a random PCI device, it would just work as long as the guest has drivers. But "real" non-x86 machines are more likely to have an OHCI controller, and in fact that's what "-usb" adds by default to the pseries machine. Paolo
On Tue, Jul 23, 2013 at 06:18:14PM -0400, Laine Stump wrote:
On 07/23/2013 11:24 AM, Daniel P. Berrange wrote:
I'm thinking that it would probably be better to move all the re-indented code out into a qemuValidateDevicePCISlotsPIIX3() and just call that function from qemuAssignDevicePCISlots(). That way if we need to add more validation for other machine types in the future, we have a good modular code structure. This would probably make the diff more sane too, since you wouldn't be indenting code.
Ah yes, good idea! I'll do that and resend.
On Tue, Jul 23, 2013 at 10:44:52AM -0400, Laine Stump wrote:
(Note that, according to the qemuxml2argv-pseries-usb-multi test, ppc "pseries" machines also have a PIIX3 chip (since that test file adds a "piix3-uhci" usb controller). I don't know if this is really the case or not, but had to include that machine type in the checks in order for make check to succeed with no changes to the test data.)
Anyone have better information about this? Does the pseries really have a PIIX3? Or was that just an arbitrary entry added for a test case?
Looking at QEMU's GIT hw/ppc/* I see no reference to piix at all. It is only referenced in hw/i386/*. So I think the test case is bogus Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
Although these two enums are named ..._LAST, they really had the value of ..._SIZE. This patch changes their values so that, e.g., QEMU_PCI_ADDRESS_SLOT_LAST really is the slot number of the last slot on a PCI bus. --- src/qemu/qemu_command.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index f85e896..059aa6a 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -1408,12 +1408,12 @@ cleanup: return ret; } -#define QEMU_PCI_ADDRESS_SLOT_LAST 32 -#define QEMU_PCI_ADDRESS_FUNCTION_LAST 8 +#define QEMU_PCI_ADDRESS_SLOT_LAST 31 +#define QEMU_PCI_ADDRESS_FUNCTION_LAST 7 typedef struct { /* Each bit in a slot represents one function on that slot */ - uint8_t slots[QEMU_PCI_ADDRESS_SLOT_LAST]; + uint8_t slots[QEMU_PCI_ADDRESS_SLOT_LAST + 1]; } qemuDomainPCIAddressBus; typedef qemuDomainPCIAddressBus *qemuDomainPCIAddressBusPtr; @@ -1448,15 +1448,15 @@ static bool qemuPCIAddressValidate(qemuDomainPCIAddressSetPtr addrs ATTRIBUTE_UN addrs->nbuses - 1); return false; } - if (addr->function >= QEMU_PCI_ADDRESS_FUNCTION_LAST) { + if (addr->function > QEMU_PCI_ADDRESS_FUNCTION_LAST) { virReportError(VIR_ERR_XML_ERROR, - _("Invalid PCI address: function must be < %u"), + _("Invalid PCI address: function must be <= %u"), QEMU_PCI_ADDRESS_FUNCTION_LAST); return false; } - if (addr->slot >= QEMU_PCI_ADDRESS_SLOT_LAST) { + if (addr->slot > QEMU_PCI_ADDRESS_SLOT_LAST) { virReportError(VIR_ERR_XML_ERROR, - _("Invalid PCI address: slot must be < %u"), + _("Invalid PCI address: slot must be <= %u"), QEMU_PCI_ADDRESS_SLOT_LAST); return false; } @@ -1859,7 +1859,7 @@ qemuDomainPCIAddressGetNextSlot(qemuDomainPCIAddressSetPtr addrs, /* Start the search at the last used bus and slot */ for (a.slot++; a.bus < addrs->nbuses; a.bus++) { - for (; a.slot < QEMU_PCI_ADDRESS_SLOT_LAST; a.slot++) { + for (; a.slot <= QEMU_PCI_ADDRESS_SLOT_LAST; a.slot++) { if (!qemuDomainPCIAddressSlotInUse(addrs, &a)) goto success; @@ -1878,7 +1878,7 @@ qemuDomainPCIAddressGetNextSlot(qemuDomainPCIAddressSetPtr addrs, } else { /* Check the buses from 0 up to the last used one */ for (a.bus = 0; a.bus <= addrs->lastaddr.bus; a.bus++) { - for (a.slot = 1; a.slot < QEMU_PCI_ADDRESS_SLOT_LAST; a.slot++) { + for (a.slot = 1; a.slot <= QEMU_PCI_ADDRESS_SLOT_LAST; a.slot++) { if (!qemuDomainPCIAddressSlotInUse(addrs, &a)) goto success; -- 1.7.11.7
On Tue, Jul 23, 2013 at 10:44:53AM -0400, Laine Stump wrote:
Although these two enums are named ..._LAST, they really had the value of ..._SIZE. This patch changes their values so that, e.g., QEMU_PCI_ADDRESS_SLOT_LAST really is the slot number of the last slot on a PCI bus. --- src/qemu/qemu_command.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-)
ACK Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
Since PCI bridges, PCIe bridges, PCIe switches, and PCIe root ports all share the same namespace, they are all defined as controllers of type='pci' in libvirt (but with a differing model attribute). Each of these controllers has a certain connection type upstream, allows certain connection types downstream, and each can either allow a single downstream connection at slot 0, or connections from slot 1 - 31. Right now, we only support the pci-root and pci-bridge devices, both of which only allow PCI devices to connect, and both which have usable slots 1 - 31. In preparation for adding other types of controllers that have different capabilities, this patch 1) adds info to the qemuDomainPCIAddressBus object to indicate the capabilities, 2) sets those capabilities appropriately for pci-root and pci-bridge devices, and 3) validates that the controller being connected to is the proper type when allocating slots or validating that a user-selected slot is appropriate for a device.. Having this infrastructure in place will make it much easier to add support for the other PCI controller types. While it would be possible to do all the necessary checking by just storing the controller model in the qemyuDomainPCIAddressBus, it greatly simplifies all the validation code to also keep a "flags", "minSlot" and "maxSlot" for each - that way we can just check those attributes rather than requiring a nearly identical switch statement everywhere we need to validate compatibility. You may notice many places where the flags are seemingly hard-coded to QEMU_PCI_CONNECT_HOTPLUGGABLE | QEMU_PCI_CONNECT_TYPE_PCI This is currently the correct value for all PCI devices, and in the future will be the default, with small bits of code added to change to the flags for the few devices which are the exceptions to this rule. Finally, there are a few places with "FIXME" comments. Note that these aren't indicating places that are broken according to the currently supported devices, they are places that will need fixing when support for new PCI controller models is added. --- src/conf/domain_conf.h | 4 +- src/libvirt_private.syms | 1 + src/qemu/qemu_command.c | 319 +++++++++++++++++++++++++++++++++++------------ src/qemu/qemu_command.h | 26 +++- 4 files changed, 268 insertions(+), 82 deletions(-) diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index f265966..abf024c 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -766,12 +766,12 @@ enum virDomainControllerType { }; -enum virDomainControllerModelPCI { +typedef enum { VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT, VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE, VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST -}; +} virDomainControllerModelPCI; enum virDomainControllerModelSCSI { VIR_DOMAIN_CONTROLLER_MODEL_SCSI_AUTO, diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 76873ad..f0c9181 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -139,6 +139,7 @@ virDomainControllerDefFree; virDomainControllerFind; virDomainControllerInsert; virDomainControllerInsertPreAlloced; +virDomainControllerModelPCITypeToString; virDomainControllerModelSCSITypeFromString; virDomainControllerModelSCSITypeToString; virDomainControllerModelUSBTypeFromString; 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); return false; } return true; } + +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; } -static char *qemuPCIAddressAsString(virDevicePCIAddressPtr addr) +static char * +qemuPCIAddressAsString(virDevicePCIAddressPtr addr) { char *str; @@ -1512,15 +1591,19 @@ static char *qemuPCIAddressAsString(virDevicePCIAddressPtr addr) } -static int qemuCollectPCIAddress(virDomainDefPtr def ATTRIBUTE_UNUSED, - virDomainDeviceDefPtr device, - virDomainDeviceInfoPtr info, - void *opaque) +static int +qemuCollectPCIAddress(virDomainDefPtr def ATTRIBUTE_UNUSED, + virDomainDeviceDefPtr device, + virDomainDeviceInfoPtr info, + void *opaque) { int ret = -1; char *str = NULL; virDevicePCIAddressPtr addr = &info->addr.pci; qemuDomainPCIAddressSetPtr addrs = opaque; + /* FIXME: flags should be set according to the requirements of @device */ + qemuDomainPCIConnectFlags flags = (QEMU_PCI_CONNECT_HOTPLUGGABLE | + QEMU_PCI_CONNECT_TYPE_PCI); if ((info->type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) || ((device->type == VIR_DOMAIN_DEVICE_HOSTDEV) && @@ -1538,6 +1621,11 @@ static int qemuCollectPCIAddress(virDomainDefPtr def ATTRIBUTE_UNUSED, * * If the machine does have a PCI bus, they will get reserved * in qemuAssignDevicePCISlots(). + * + * FIXME: When we have support for a pcie-root controller at bus + * 0, we will no longer be able to skip checking of these devices, + * as they are PCI, and thus can't be connected to bus 0 if it is + * PCIe rather than PCI. */ if (device->type == VIR_DOMAIN_DEVICE_CONTROLLER && addr->domain == 0 && addr->bus == 0 && addr->slot == 1) { @@ -1551,15 +1639,22 @@ static int qemuCollectPCIAddress(virDomainDefPtr def ATTRIBUTE_UNUSED, return 0; } - if (addrs->dryRun && qemuDomainPCIAddressSetGrow(addrs, addr) < 0) + /* add an additional bus of the correct type if needed */ + if (addrs->dryRun && + qemuDomainPCIAddressSetGrow(addrs, addr, flags) < 0) return -1; - if (!qemuPCIAddressValidate(addrs, addr)) + /* verify that the address is in bounds for the chosen bus, and + * that the bus is of the correct type for the device (via + * comparing the flags). + */ + if (!qemuPCIAddressValidate(addrs, addr, flags)) return -1; if (!(str = qemuPCIAddressAsString(addr))) goto cleanup; + /* check if already in use */ if (addrs->buses[addr->bus].slots[addr->slot] & (1 << addr->function)) { if (info->addr.pci.function != 0) { virReportError(VIR_ERR_XML_ERROR, @@ -1573,6 +1668,7 @@ static int qemuCollectPCIAddress(virDomainDefPtr def ATTRIBUTE_UNUSED, goto cleanup; } + /* mark as in use */ if ((info->addr.pci.function == 0) && (info->addr.pci.multi != VIR_DEVICE_ADDRESS_PCI_MULTI_ON)) { /* a function 0 w/o multifunction=on must reserve the entire slot */ @@ -1611,6 +1707,8 @@ qemuDomainAssignPCIAddresses(virDomainDefPtr def, int nbuses = 0; size_t i; int rv; + qemuDomainPCIConnectFlags flags = (QEMU_PCI_CONNECT_HOTPLUGGABLE | + QEMU_PCI_CONNECT_TYPE_PCI); for (i = 0; i < def->ncontrollers; i++) { if (def->controllers[i]->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI) { @@ -1624,22 +1722,25 @@ qemuDomainAssignPCIAddresses(virDomainDefPtr def, if (nbuses > 0 && virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_PCI_BRIDGE)) { virDomainDeviceInfo info; + /* 1st pass to figure out how many PCI bridges we need */ if (!(addrs = qemuDomainPCIAddressSetCreate(def, nbuses, true))) goto cleanup; if (qemuAssignDevicePCISlots(def, qemuCaps, addrs) < 0) goto cleanup; /* Reserve 1 extra slot for a (potential) bridge */ - if (qemuDomainPCIAddressSetNextAddr(addrs, &info) < 0) + if (qemuDomainPCIAddressSetNextAddr(addrs, &info, flags) < 0) goto cleanup; for (i = 1; i < addrs->nbuses; i++) { + qemuDomainPCIAddressBusPtr bus = &addrs->buses[i]; + if ((rv = virDomainDefMaybeAddController( - def, VIR_DOMAIN_CONTROLLER_TYPE_PCI, - i, VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE)) < 0) + def, VIR_DOMAIN_CONTROLLER_TYPE_PCI, + i, bus->model)) < 0) goto cleanup; /* If we added a new bridge, we will need one more address */ - if (rv > 0 && qemuDomainPCIAddressSetNextAddr(addrs, &info) < 0) + if (rv > 0 && qemuDomainPCIAddressSetNextAddr(addrs, &info, flags) < 0) goto cleanup; } nbuses = addrs->nbuses; @@ -1698,9 +1799,11 @@ int qemuDomainAssignAddresses(virDomainDefPtr def, return qemuDomainAssignPCIAddresses(def, qemuCaps, obj); } -qemuDomainPCIAddressSetPtr qemuDomainPCIAddressSetCreate(virDomainDefPtr def, - unsigned int nbuses, - bool dryRun) + +qemuDomainPCIAddressSetPtr +qemuDomainPCIAddressSetCreate(virDomainDefPtr def, + unsigned int nbuses, + bool dryRun) { qemuDomainPCIAddressSetPtr addrs; size_t i; @@ -1714,10 +1817,38 @@ qemuDomainPCIAddressSetPtr qemuDomainPCIAddressSetCreate(virDomainDefPtr def, addrs->nbuses = nbuses; addrs->dryRun = dryRun; - /* reserve slot 0 in every bus - it's used by the host bridge on bus 0 - * and unusable on PCI bridges */ - for (i = 0; i < nbuses; i++) - addrs->buses[i].slots[0] = 0xFF; + /* As a safety measure, set default model='pci-root' for first pci + * controller and 'pci-bridge' for all subsequent. After setting + * those defaults, then scan the config and set the actual model + * for all addrs[idx]->bus that already have a corresponding + * controller in the config. + * + */ + if (nbuses > 0) + qemuDomainPCIAddressBusSetModel(&addrs->buses[0], + VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT); + for (i = 1; i < nbuses; i++) { + qemuDomainPCIAddressBusSetModel(&addrs->buses[i], + VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE); + } + + for (i = 0; i < def->ncontrollers; i++) { + size_t idx = def->controllers[i]->idx; + + if (def->controllers[i]->type != VIR_DOMAIN_CONTROLLER_TYPE_PCI) + continue; + + if (idx >= addrs->nbuses) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Inappropriate new pci controller index %zu" + "not found in addrs"), idx); + goto error; + } + + if (qemuDomainPCIAddressBusSetModel(&addrs->buses[idx], + def->controllers[i]->model) < 0) + goto error; + } if (virDomainDeviceInfoIterate(def, qemuCollectPCIAddress, addrs) < 0) goto error; @@ -1738,12 +1869,16 @@ static bool qemuDomainPCIAddressSlotInUse(qemuDomainPCIAddressSetPtr addrs, return !!addrs->buses[addr->bus].slots[addr->slot]; } -int qemuDomainPCIAddressReserveAddr(qemuDomainPCIAddressSetPtr addrs, - virDevicePCIAddressPtr addr) + +int +qemuDomainPCIAddressReserveAddr(qemuDomainPCIAddressSetPtr addrs, + virDevicePCIAddressPtr addr, + qemuDomainPCIConnectFlags flags) { char *str; + qemuDomainPCIAddressBusPtr bus; - if (addrs->dryRun && qemuDomainPCIAddressSetGrow(addrs, addr) < 0) + if (addrs->dryRun && qemuDomainPCIAddressSetGrow(addrs, addr, flags) < 0) return -1; if (!(str = qemuPCIAddressAsString(addr))) @@ -1751,9 +1886,19 @@ int qemuDomainPCIAddressReserveAddr(qemuDomainPCIAddressSetPtr addrs, VIR_DEBUG("Reserving PCI addr %s", str); - if (addrs->buses[addr->bus].slots[addr->slot] & (1 << addr->function)) { + bus = &addrs->buses[addr->bus]; + if ((bus->minSlot && addr->slot < bus->minSlot) || + addr->slot > bus->maxSlot) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unable to reserve PCI address %s. " + "Slot %02x can't be used on bus %04x, " + "only slots %02zx - %02zx are permitted on this bus."), + str, addr->slot, addr->bus, bus->minSlot, bus->maxSlot); + } + + if (bus->slots[addr->slot] & (1 << addr->function)) { virReportError(VIR_ERR_INTERNAL_ERROR, - _("unable to reserve PCI address %s"), str); + _("unable to reserve PCI address %s already in use"), str); VIR_FREE(str); return -1; } @@ -1763,16 +1908,19 @@ int qemuDomainPCIAddressReserveAddr(qemuDomainPCIAddressSetPtr addrs, addrs->lastaddr = *addr; addrs->lastaddr.function = 0; addrs->lastaddr.multi = 0; - addrs->buses[addr->bus].slots[addr->slot] |= 1 << addr->function; + bus->slots[addr->slot] |= 1 << addr->function; return 0; } -int qemuDomainPCIAddressReserveSlot(qemuDomainPCIAddressSetPtr addrs, - virDevicePCIAddressPtr addr) + +int +qemuDomainPCIAddressReserveSlot(qemuDomainPCIAddressSetPtr addrs, + virDevicePCIAddressPtr addr, + qemuDomainPCIConnectFlags flags) { char *str; - if (addrs->dryRun && qemuDomainPCIAddressSetGrow(addrs, addr) < 0) + if (addrs->dryRun && qemuDomainPCIAddressSetGrow(addrs, addr, flags) < 0) return -1; if (!(str = qemuPCIAddressAsString(addr))) @@ -1796,6 +1944,10 @@ int qemuDomainPCIAddressEnsureAddr(qemuDomainPCIAddressSetPtr addrs, virDomainDeviceInfoPtr dev) { int ret = 0; + /* FIXME: flags should be set according to the particular device */ + qemuDomainPCIConnectFlags flags = (QEMU_PCI_CONNECT_HOTPLUGGABLE | + QEMU_PCI_CONNECT_TYPE_PCI); + if (dev->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) { /* We do not support hotplug multi-function PCI device now, so we should * reserve the whole slot. The function of the PCI device must be 0. @@ -1807,12 +1959,12 @@ int qemuDomainPCIAddressEnsureAddr(qemuDomainPCIAddressSetPtr addrs, return -1; } - if (!qemuPCIAddressValidate(addrs, &dev->addr.pci)) + if (!qemuPCIAddressValidate(addrs, &dev->addr.pci, flags)) return -1; - ret = qemuDomainPCIAddressReserveSlot(addrs, &dev->addr.pci); + ret = qemuDomainPCIAddressReserveSlot(addrs, &dev->addr.pci, flags); } else { - ret = qemuDomainPCIAddressSetNextAddr(addrs, dev); + ret = qemuDomainPCIAddressSetNextAddr(addrs, dev, flags); } return ret; } @@ -1829,7 +1981,12 @@ static int qemuDomainPCIAddressReleaseSlot(qemuDomainPCIAddressSetPtr addrs, virDevicePCIAddressPtr addr) { - if (!qemuPCIAddressValidate(addrs, addr)) + /* permit any kind of connection type in validation, since we + * already had it, and are giving it back. + */ + qemuDomainPCIConnectFlags flags = QEMU_PCI_CONNECT_TYPES_MASK; + + if (!qemuPCIAddressValidate(addrs, addr, flags)) return -1; addrs->buses[addr->bus].slots[addr->slot] = 0; @@ -1848,7 +2005,8 @@ void qemuDomainPCIAddressSetFree(qemuDomainPCIAddressSetPtr addrs) static int qemuDomainPCIAddressGetNextSlot(qemuDomainPCIAddressSetPtr addrs, - virDevicePCIAddressPtr next_addr) + virDevicePCIAddressPtr next_addr, + qemuDomainPCIConnectFlags flags) { virDevicePCIAddress a = addrs->lastaddr; @@ -1872,7 +2030,7 @@ qemuDomainPCIAddressGetNextSlot(qemuDomainPCIAddressSetPtr addrs, /* There were no free slots after the last used one */ if (addrs->dryRun) { /* a is already set to the first new bus and slot 1 */ - if (qemuDomainPCIAddressSetGrow(addrs, &a) < 0) + if (qemuDomainPCIAddressSetGrow(addrs, &a, flags) < 0) return -1; goto success; } else { @@ -1900,14 +2058,16 @@ success: return 0; } -int qemuDomainPCIAddressSetNextAddr(qemuDomainPCIAddressSetPtr addrs, - virDomainDeviceInfoPtr dev) +int +qemuDomainPCIAddressSetNextAddr(qemuDomainPCIAddressSetPtr addrs, + virDomainDeviceInfoPtr dev, + qemuDomainPCIConnectFlags flags) { virDevicePCIAddress addr; - if (qemuDomainPCIAddressGetNextSlot(addrs, &addr) < 0) + if (qemuDomainPCIAddressGetNextSlot(addrs, &addr, flags) < 0) return -1; - if (qemuDomainPCIAddressReserveSlot(addrs, &addr) < 0) + if (qemuDomainPCIAddressReserveSlot(addrs, &addr, flags) < 0) return -1; if (!addrs->dryRun) { @@ -1991,6 +2151,7 @@ qemuAssignDevicePCISlots(virDomainDefPtr def, { size_t i, j; bool qemuDeviceVideoUsable = virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VIDEO_PRIMARY); + qemuDomainPCIConnectFlags flags; virDevicePCIAddress tmp_addr; virDevicePCIAddressPtr addrptr; bool isPIIX3 = STRPREFIX(def->os.machine, "pc-0.") || @@ -2003,6 +2164,7 @@ qemuAssignDevicePCISlots(virDomainDefPtr def, if (isPIIX3) { /* Verify that first IDE and USB controllers (if any) is on the PIIX3, fn 1 */ for (i = 0; i < def->ncontrollers; i++) { + flags = QEMU_PCI_CONNECT_HOTPLUGGABLE | QEMU_PCI_CONNECT_TYPE_PCI; /* First IDE controller lives on the PIIX3 at slot=1, function=1 */ if (def->controllers[i]->type == VIR_DOMAIN_CONTROLLER_TYPE_IDE && def->controllers[i]->idx == 0) { @@ -2045,14 +2207,16 @@ qemuAssignDevicePCISlots(virDomainDefPtr def, } } + flags = QEMU_PCI_CONNECT_HOTPLUGGABLE | QEMU_PCI_CONNECT_TYPE_PCI; + /* PIIX3 (ISA bridge, IDE controller, something else unknown, USB controller) * hardcoded slot=1, multifunction device */ if (addrs->nbuses) { memset(&tmp_addr, 0, sizeof(tmp_addr)); tmp_addr.slot = 1; - if (qemuDomainPCIAddressReserveSlot(addrs, &tmp_addr) < 0) - goto error; + if (qemuDomainPCIAddressReserveSlot(addrs, &tmp_addr, flags) < 0) + goto error; } if (def->nvideos > 0) { @@ -2065,13 +2229,13 @@ qemuAssignDevicePCISlots(virDomainDefPtr def, primaryVideo->info.addr.pci.function = 0; addrptr = &primaryVideo->info.addr.pci; - if (!qemuPCIAddressValidate(addrs, addrptr)) + if (!qemuPCIAddressValidate(addrs, addrptr, flags)) goto error; if (qemuDomainPCIAddressSlotInUse(addrs, addrptr)) { if (qemuDeviceVideoUsable) { virResetLastError(); - if (qemuDomainPCIAddressSetNextAddr(addrs, &primaryVideo->info) < 0) + if (qemuDomainPCIAddressSetNextAddr(addrs, &primaryVideo->info, flags) < 0) goto error; } else { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", @@ -2079,7 +2243,7 @@ qemuAssignDevicePCISlots(virDomainDefPtr def, "QEMU needs it for primary video")); goto error; } - } else if (qemuDomainPCIAddressReserveSlot(addrs, addrptr) < 0) { + } else if (qemuDomainPCIAddressReserveSlot(addrs, addrptr, flags) < 0) { goto error; } } else if (!qemuDeviceVideoUsable) { @@ -2103,11 +2267,12 @@ qemuAssignDevicePCISlots(virDomainDefPtr def, " device will not be possible without manual" " intervention"); virResetLastError(); - } else if (qemuDomainPCIAddressReserveSlot(addrs, &tmp_addr) < 0) { + } else if (qemuDomainPCIAddressReserveSlot(addrs, &tmp_addr, flags) < 0) { goto error; } } } + flags = QEMU_PCI_CONNECT_HOTPLUGGABLE | QEMU_PCI_CONNECT_TYPE_PCI; /* PCI controllers */ for (i = 0; i < def->ncontrollers; i++) { @@ -2116,7 +2281,7 @@ qemuAssignDevicePCISlots(virDomainDefPtr def, continue; if (def->controllers[i]->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) continue; - if (qemuDomainPCIAddressSetNextAddr(addrs, &def->controllers[i]->info) < 0) + if (qemuDomainPCIAddressSetNextAddr(addrs, &def->controllers[i]->info, flags) < 0) goto error; } } @@ -2127,7 +2292,7 @@ qemuAssignDevicePCISlots(virDomainDefPtr def, /* Only support VirtIO-9p-pci so far. If that changes, * we might need to skip devices here */ - if (qemuDomainPCIAddressSetNextAddr(addrs, &def->fss[i]->info) < 0) + if (qemuDomainPCIAddressSetNextAddr(addrs, &def->fss[i]->info, flags) < 0) goto error; } @@ -2141,7 +2306,7 @@ qemuAssignDevicePCISlots(virDomainDefPtr def, (def->nets[i]->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE)) { continue; } - if (qemuDomainPCIAddressSetNextAddr(addrs, &def->nets[i]->info) < 0) + if (qemuDomainPCIAddressSetNextAddr(addrs, &def->nets[i]->info, flags) < 0) goto error; } @@ -2154,7 +2319,7 @@ qemuAssignDevicePCISlots(virDomainDefPtr def, def->sounds[i]->model == VIR_DOMAIN_SOUND_MODEL_PCSPK) continue; - if (qemuDomainPCIAddressSetNextAddr(addrs, &def->sounds[i]->info) < 0) + if (qemuDomainPCIAddressSetNextAddr(addrs, &def->sounds[i]->info, flags) < 0) goto error; } @@ -2216,20 +2381,20 @@ qemuAssignDevicePCISlots(virDomainDefPtr def, if (addr.slot == 0) { /* This is the first part of the controller, so need * to find a free slot & then reserve a function */ - if (qemuDomainPCIAddressGetNextSlot(addrs, &tmp_addr) < 0) + if (qemuDomainPCIAddressGetNextSlot(addrs, &tmp_addr, flags) < 0) goto error; addr.bus = tmp_addr.bus; addr.slot = tmp_addr.slot; } /* Finally we can reserve the slot+function */ - if (qemuDomainPCIAddressReserveAddr(addrs, &addr) < 0) + if (qemuDomainPCIAddressReserveAddr(addrs, &addr, flags) < 0) goto error; def->controllers[i]->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI; def->controllers[i]->info.addr.pci = addr; } else { - if (qemuDomainPCIAddressSetNextAddr(addrs, &def->controllers[i]->info) < 0) + if (qemuDomainPCIAddressSetNextAddr(addrs, &def->controllers[i]->info, flags) < 0) goto error; } } @@ -2254,7 +2419,7 @@ qemuAssignDevicePCISlots(virDomainDefPtr def, goto error; } - if (qemuDomainPCIAddressSetNextAddr(addrs, &def->disks[i]->info) < 0) + if (qemuDomainPCIAddressSetNextAddr(addrs, &def->disks[i]->info, flags) < 0) goto error; } @@ -2266,7 +2431,7 @@ qemuAssignDevicePCISlots(virDomainDefPtr def, def->hostdevs[i]->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI) continue; - if (qemuDomainPCIAddressSetNextAddr(addrs, def->hostdevs[i]->info) < 0) + if (qemuDomainPCIAddressSetNextAddr(addrs, def->hostdevs[i]->info, flags) < 0) goto error; } @@ -2274,7 +2439,7 @@ qemuAssignDevicePCISlots(virDomainDefPtr def, if (def->memballoon && def->memballoon->model == VIR_DOMAIN_MEMBALLOON_MODEL_VIRTIO && def->memballoon->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) { - if (qemuDomainPCIAddressSetNextAddr(addrs, &def->memballoon->info) < 0) + if (qemuDomainPCIAddressSetNextAddr(addrs, &def->memballoon->info, flags) < 0) goto error; } @@ -2282,7 +2447,7 @@ qemuAssignDevicePCISlots(virDomainDefPtr def, if (def->rng && def->rng->model == VIR_DOMAIN_RNG_MODEL_VIRTIO && def->rng->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) { - if (qemuDomainPCIAddressSetNextAddr(addrs, &def->rng->info) < 0) + if (qemuDomainPCIAddressSetNextAddr(addrs, &def->rng->info, flags) < 0) goto error; } @@ -2290,7 +2455,7 @@ qemuAssignDevicePCISlots(virDomainDefPtr def, if (def->watchdog && def->watchdog->model != VIR_DOMAIN_WATCHDOG_MODEL_IB700 && def->watchdog->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) { - if (qemuDomainPCIAddressSetNextAddr(addrs, &def->watchdog->info) < 0) + if (qemuDomainPCIAddressSetNextAddr(addrs, &def->watchdog->info, flags) < 0) goto error; } @@ -2303,7 +2468,7 @@ qemuAssignDevicePCISlots(virDomainDefPtr def, } if (def->videos[i]->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) continue; - if (qemuDomainPCIAddressSetNextAddr(addrs, &def->videos[i]->info) < 0) + if (qemuDomainPCIAddressSetNextAddr(addrs, &def->videos[i]->info, flags) < 0) goto error; } for (i = 0; i < def->ninputs; i++) { diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h index e15fe64..ede67fe 100644 --- a/src/qemu/qemu_command.h +++ b/src/qemu/qemu_command.h @@ -225,6 +225,23 @@ void qemuDomainReleaseDeviceAddress(virDomainObjPtr vm, virDomainDeviceInfoPtr info, const char *devstr); +typedef enum { + QEMU_PCI_CONNECT_HOTPLUGGABLE = 1 << 0, + /* This bus supports hot-plug */ + QEMU_PCI_CONNECT_SINGLESLOT = 1 << 1, + /* This "bus" has only a single downstream slot/port */ + + QEMU_PCI_CONNECT_TYPE_PCI = 1 << 2, + /* PCI devices can connect to this bus */ +} qemuDomainPCIConnectFlags; + +/* a combination of all bit that describe the type of connections + * allowed, e.g. PCI, PCIe, switch + */ +#define QEMU_PCI_CONNECT_TYPES_MASK \ + QEMU_PCI_CONNECT_TYPE_PCI + + int qemuDomainAssignPCIAddresses(virDomainDefPtr def, virQEMUCapsPtr qemuCaps, virDomainObjPtr obj); @@ -232,11 +249,14 @@ qemuDomainPCIAddressSetPtr qemuDomainPCIAddressSetCreate(virDomainDefPtr def, unsigned int nbuses, bool dryRun); int qemuDomainPCIAddressReserveSlot(qemuDomainPCIAddressSetPtr addrs, - virDevicePCIAddressPtr addr); + virDevicePCIAddressPtr addr, + qemuDomainPCIConnectFlags flags); int qemuDomainPCIAddressReserveAddr(qemuDomainPCIAddressSetPtr addrs, - virDevicePCIAddressPtr addr); + virDevicePCIAddressPtr addr, + qemuDomainPCIConnectFlags flags); int qemuDomainPCIAddressSetNextAddr(qemuDomainPCIAddressSetPtr addrs, - virDomainDeviceInfoPtr dev); + virDomainDeviceInfoPtr dev, + qemuDomainPCIConnectFlags flags); int qemuDomainPCIAddressEnsureAddr(qemuDomainPCIAddressSetPtr addrs, virDomainDeviceInfoPtr dev); int qemuDomainPCIAddressReleaseAddr(qemuDomainPCIAddressSetPtr addrs, -- 1.7.11.7
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 Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
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.
This controller is implicit on q35 machinetypes. It provides 31 PCIe (*not* PCI) slots as controller 0. Currently there are no devices that can connect to pcie-root. For a usable q35 system, we still need to add a "dmi-to-pci-bridge" pci controller, which can connect to pcie-root, and provides pci slots. This patch still requires a test case, which willbe coming up, but I wanted to include it along with the previous patch to show that it's simpler to add new controller types now. --- docs/formatdomain.html.in | 17 ++++++++++++++--- src/conf/domain_conf.c | 4 +++- src/conf/domain_conf.h | 1 + src/qemu/qemu_command.c | 17 +++++++++++++---- src/qemu/qemu_command.h | 2 ++ src/qemu/qemu_domain.c | 23 +++++++++++++++++------ 6 files changed, 50 insertions(+), 14 deletions(-) diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 7601aaa..41e3e2a 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -2338,10 +2338,14 @@ <p> PCI controllers have an optional <code>model</code> attribute with - possible values <code>pci-root</code> or <code>pci-bridge</code>. - For machine types which provide an implicit pci bus, the pci-root + possible values <code>pci-root</code>, <code>pcie-root</code> + or <code>pci-bridge</code>. + For machine types which provide an implicit PCI bus, the pci-root controller with index=0 is auto-added and required to use PCI devices. - PCI root has no address. + pci-root has no address. + For machine types which provide an implicit PCI Express (PCIe) + bus, the pcie-root controller with index=0 is auto-added and + required to use PCIe devices. pcie-root has also no address. PCI bridges are auto-added if there are too many devices to fit on the one bus provided by pci-root, or a PCI bus number greater than zero was specified. @@ -2361,6 +2365,13 @@ </devices> ...</pre> +<pre> + ... + <devices> + <controller type='pci' index='0' model='pcie-root'/> + </devices> + ...</pre> + <h4><a name="elementsLease">Device leases</a></h4> <p> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 10cb7f6..605f706 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -310,6 +310,7 @@ VIR_ENUM_IMPL(virDomainController, VIR_DOMAIN_CONTROLLER_TYPE_LAST, VIR_ENUM_IMPL(virDomainControllerModelPCI, VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST, "pci-root", + "pcie-root", "pci-bridge") VIR_ENUM_IMPL(virDomainControllerModelSCSI, VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LAST, @@ -5715,9 +5716,10 @@ virDomainControllerDefParseXML(xmlNodePtr node, case VIR_DOMAIN_CONTROLLER_TYPE_PCI: switch (def->model) { case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT: + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT: if (def->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) { virReportError(VIR_ERR_XML_ERROR, "%s", - _("pci-root controller should not " + _("pci-root and pcie-root controllers should not " "have an address")); goto error; } diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index abf024c..68f36fd 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -768,6 +768,7 @@ enum virDomainControllerType { typedef enum { VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT, + VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT, VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE, VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 64787b6..7fccb98 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -1519,6 +1519,12 @@ qemuDomainPCIAddressBusSetModel(qemuDomainPCIAddressBusPtr bus, bus->minSlot = 1; bus->maxSlot = QEMU_PCI_ADDRESS_SLOT_LAST; break; + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT: + bus->flags = (QEMU_PCI_CONNECT_HOTPLUGGABLE | + QEMU_PCI_CONNECT_TYPE_PCIE); + bus->minSlot = 1; + bus->maxSlot = QEMU_PCI_ADDRESS_SLOT_LAST; + break; default: virReportError(VIR_ERR_INTERNAL_ERROR, _("Invalid PCI controller model %d"), model); @@ -2277,7 +2283,8 @@ qemuAssignDevicePCISlots(virDomainDefPtr def, /* PCI controllers */ for (i = 0; i < def->ncontrollers; i++) { if (def->controllers[i]->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI) { - if (def->controllers[i]->model == VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT) + if (def->controllers[i]->model == VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT || + def->controllers[i]->model == VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT) continue; if (def->controllers[i]->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) continue; @@ -4211,8 +4218,9 @@ qemuBuildControllerDevStr(virDomainDefPtr domainDef, def->idx, def->idx); break; case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT: + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT: virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("wrong function called for pci-root")); + _("wrong function called for pci-root/pcie-root")); return NULL; } break; @@ -7490,9 +7498,10 @@ qemuBuildCommandLine(virConnectPtr conn, continue; } - /* Skip pci-root */ + /* Skip pci-root/pcie-root */ if (cont->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI && - cont->model == VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT) { + (cont->model == VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT || + cont->model == VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT)) { continue; } diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h index ede67fe..a4574f2 100644 --- a/src/qemu/qemu_command.h +++ b/src/qemu/qemu_command.h @@ -233,6 +233,8 @@ typedef enum { QEMU_PCI_CONNECT_TYPE_PCI = 1 << 2, /* PCI devices can connect to this bus */ + QEMU_PCI_CONNECT_TYPE_PCIE = 1 << 3, + /* PCI Express devices can connect to this bus */ } qemuDomainPCIConnectFlags; /* a combination of all bit that describe the type of connections diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index da3b768..34fed56 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -700,6 +700,7 @@ qemuDomainDefPostParse(virDomainDefPtr def, void *opaque ATTRIBUTE_UNUSED) { bool addPCIRoot = false; + bool addPCIeRoot = false; /* check for emulator and create a default one if needed */ if (!def->emulator && @@ -712,10 +713,13 @@ qemuDomainDefPostParse(virDomainDefPtr def, case VIR_ARCH_X86_64: if (!def->os.machine) break; - if (STRPREFIX(def->os.machine, "pc-q35") || - STREQ(def->os.machine, "q35") || - STREQ(def->os.machine, "isapc")) + if (STREQ(def->os.machine, "isapc")) break; + if (STRPREFIX(def->os.machine, "pc-q35") || + STREQ(def->os.machine, "q35")) { + addPCIeRoot = true; + break; + } if (!STRPREFIX(def->os.machine, "pc-0.") && !STRPREFIX(def->os.machine, "pc-1.") && !STRPREFIX(def->os.machine, "pc-i440") && @@ -743,6 +747,12 @@ qemuDomainDefPostParse(virDomainDefPtr def, VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT) < 0) return -1; + if (addPCIeRoot && + virDomainDefMaybeAddController( + def, VIR_DOMAIN_CONTROLLER_TYPE_PCI, 0, + VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT) < 0) + return -1; + return 0; } @@ -1408,9 +1418,10 @@ qemuDomainDefFormatBuf(virQEMUDriverPtr driver, } if (pci && pci->idx == 0 && - pci->model == VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT) { - VIR_DEBUG("Removing default 'pci-root' from domain '%s'" - " for migration compatibility", def->name); + (pci->model == VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT || + pci->model == VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT)) { + VIR_DEBUG("Removing default pci-root/pcie-root controller from " + "domain '%s' for migration compatibility", def->name); toremove++; } else { pci = NULL; -- 1.7.11.7
On Tue, Jul 23, 2013 at 10:44:55AM -0400, Laine Stump wrote:
This controller is implicit on q35 machinetypes. It provides 31 PCIe (*not* PCI) slots as controller 0.
Currently there are no devices that can connect to pcie-root. For a usable q35 system, we still need to add a "dmi-to-pci-bridge" pci controller, which can connect to pcie-root, and provides pci slots.
Presumably you have another patch which auto-adds a 'dmi-to-pci-bridge' device when we see a q35 machine, so that auto-PCI address assignment still works ? If we relied on the user to add that device, then legacy apps will not work against a QEMU which defaults to q35, since they'll be adding PCI devices but not know that they need to add this bridge. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 07/23/2013 11:17 AM, Daniel P. Berrange wrote:
On Tue, Jul 23, 2013 at 10:44:55AM -0400, Laine Stump wrote:
This controller is implicit on q35 machinetypes. It provides 31 PCIe (*not* PCI) slots as controller 0.
Currently there are no devices that can connect to pcie-root. For a usable q35 system, we still need to add a "dmi-to-pci-bridge" pci controller, which can connect to pcie-root, and provides pci slots. Presumably you have another patch which auto-adds a 'dmi-to-pci-bridge' device when we see a q35 machine, so that auto-PCI address assignment still works ? If we relied on the user to add that device, then legacy apps will not work against a QEMU which defaults to q35, since they'll be adding PCI devices but not know that they need to add this bridge.
Yes, that is in the works, although it may not be finished today. The idea is that when you try to get a slot for a PCI device and you only have pcie slots, we would add a dmi-to-pci-bridge (which provides pci slots that *can't* be hot-plugged) on a pcie slot, then a pci-bridge (which provides hot-pluggable pci slots) on the dmi-to-pci-bridge. After that, all devices that required a hot-pluggable pci slot would automatically be directed to the pci-bridge. (Thinking more about it, I guess I do need to *always* add them even when there are no devices, to allow for hot-plugging later (since we can't hot-plug a dmi-to-pci-bridge or a pci-bridge). Fortunately that makes it simpler).
On Tue, Jul 23, 2013 at 10:44:55AM -0400, Laine Stump wrote:
This controller is implicit on q35 machinetypes. It provides 31 PCIe (*not* PCI) slots as controller 0.
Currently there are no devices that can connect to pcie-root. For a usable q35 system, we still need to add a "dmi-to-pci-bridge" pci controller, which can connect to pcie-root, and provides pci slots.
This patch still requires a test case, which willbe coming up, but I wanted to include it along with the previous patch to show that it's simpler to add new controller types now. --- docs/formatdomain.html.in | 17 ++++++++++++++--- src/conf/domain_conf.c | 4 +++- src/conf/domain_conf.h | 1 + src/qemu/qemu_command.c | 17 +++++++++++++---- src/qemu/qemu_command.h | 2 ++ src/qemu/qemu_domain.c | 23 +++++++++++++++++------ 6 files changed, 50 insertions(+), 14 deletions(-)
ACK once test case(s) are added. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
participants (3)
-
Daniel P. Berrange -
Laine Stump -
Paolo Bonzini