The virDomainDeviceInfoIsSet() function no longer exists. --- src/conf/device_conf.h | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/conf/device_conf.h b/src/conf/device_conf.h index a20de85..58b2c9c 100644 --- a/src/conf/device_conf.h +++ b/src/conf/device_conf.h @@ -134,10 +134,6 @@ struct _virDomainDeviceDimmAddress { typedef struct _virDomainDeviceInfo virDomainDeviceInfo; typedef virDomainDeviceInfo *virDomainDeviceInfoPtr; struct _virDomainDeviceInfo { - /* If adding to this struct, ensure that - * virDomainDeviceInfoIsSet() is updated - * to consider the new fields - */ char *alias; int type; /* virDomainDeviceAddressType */ union { -- 2.7.5

There are no external users. --- src/conf/domain_addr.c | 2 +- src/conf/domain_addr.h | 5 ----- src/libvirt_private.syms | 1 - 3 files changed, 1 insertion(+), 7 deletions(-) diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c index 639168e..9c809e8 100644 --- a/src/conf/domain_addr.c +++ b/src/conf/domain_addr.c @@ -362,7 +362,7 @@ virDomainPCIAddressBusSetModel(virDomainPCIAddressBusPtr bus, * 0 = no action performed * >0 = number of buses added */ -int +static int virDomainPCIAddressSetGrow(virDomainPCIAddressSetPtr addrs, virPCIDeviceAddressPtr addr, virDomainPCIConnectFlags flags) diff --git a/src/conf/domain_addr.h b/src/conf/domain_addr.h index f884b8a..efa97ca 100644 --- a/src/conf/domain_addr.h +++ b/src/conf/domain_addr.h @@ -145,11 +145,6 @@ bool virDomainPCIAddressSlotInUse(virDomainPCIAddressSetPtr addrs, virPCIDeviceAddressPtr addr) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); -int virDomainPCIAddressSetGrow(virDomainPCIAddressSetPtr addrs, - virPCIDeviceAddressPtr addr, - virDomainPCIConnectFlags flags) - ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); - int virDomainPCIAddressReserveAddr(virDomainPCIAddressSetPtr addrs, virPCIDeviceAddressPtr addr, virDomainPCIConnectFlags flags) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 429b095..901cc99 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -113,7 +113,6 @@ virDomainPCIAddressReserveNextAddr; virDomainPCIAddressSetAllMulti; virDomainPCIAddressSetAlloc; virDomainPCIAddressSetFree; -virDomainPCIAddressSetGrow; virDomainPCIAddressSlotInUse; virDomainPCIAddressValidate; virDomainPCIControllerModelToConnectType; -- 2.7.5

On 06/02/2017 12:07 PM, Andrea Bolognani wrote: I might have changed the other functions to put flags last (or maybe not). Anyway, consistency is good. Reviewed-by: Laine Stump <laine(a)laine.org&gt;

On 06/02/2017 12:07 PM, Andrea Bolognani wrote: Now I'm curious - a non-0 domain is *extremely* rare on x86 machines. Is it more normal on PPC? Reviewed-by: Laine Stump <laine(a)laine.org&gt;

Later on we're going to need access to information about IOMMU groups for host devices. Implement the support in virpcimock, and start using that mock library in a few QEMU test cases. --- tests/qemumemlocktest.c | 21 ++++++++++++++++++++- tests/qemuxml2argvtest.c | 25 ++++++++++++++++++++++--- tests/qemuxml2xmltest.c | 22 +++++++++++++++++++++- tests/virpcimock.c | 43 +++++++++++++++++++++++++++++++++++++------ 4 files changed, 100 insertions(+), 11 deletions(-) diff --git a/tests/qemumemlocktest.c b/tests/qemumemlocktest.c index 6cf17a4..c0f1dc3 100644 --- a/tests/qemumemlocktest.c +++ b/tests/qemumemlocktest.c @@ -56,11 +56,25 @@ testCompareMemLock(const void *data) return ret; } +# define FAKEROOTDIRTEMPLATE abs_builddir "/fakerootdir-XXXXXX" static int mymain(void) { int ret = 0; + char *fakerootdir; + + if (VIR_STRDUP_QUIET(fakerootdir, FAKEROOTDIRTEMPLATE) < 0) { + fprintf(stderr, "Out of memory\n"); + abort(); + } + + if (!mkdtemp(fakerootdir)) { + fprintf(stderr, "Cannot create fakerootdir"); + abort(); + } + + setenv("LIBVIRT_FAKE_ROOT_DIR", fakerootdir, 1); abs_top_srcdir = getenv("abs_top_srcdir"); if (!abs_top_srcdir) @@ -124,12 +138,17 @@ mymain(void) DO_TEST("pseries-hardlimit+locked+hostdev", 2147483648); DO_TEST("pseries-locked+hostdev", VIR_DOMAIN_MEMORY_PARAM_UNLIMITED); + if (getenv("LIBVIRT_SKIP_CLEANUP") == NULL) + virFileDeleteTree(fakerootdir); + qemuTestDriverFree(&driver); + VIR_FREE(fakerootdir); return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE; } -VIR_TEST_MAIN(mymain) +VIR_TEST_MAIN_PRELOAD(mymain, + abs_builddir "/.libs/virpcimock.so") #else diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index b360185..cc6a8a8 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -534,13 +534,27 @@ testCompareXMLToArgv(const void *data) return ret; } +# define FAKEROOTDIRTEMPLATE abs_builddir "/fakerootdir-XXXXXX" static int mymain(void) { int ret = 0; + char *fakerootdir; bool skipLegacyCPUs = false; + if (VIR_STRDUP_QUIET(fakerootdir, FAKEROOTDIRTEMPLATE) < 0) { + fprintf(stderr, "Out of memory\n"); + abort(); + } + + if (!mkdtemp(fakerootdir)) { + fprintf(stderr, "Cannot create fakerootdir"); + abort(); + } + + setenv("LIBVIRT_FAKE_ROOT_DIR", fakerootdir, 1); + abs_top_srcdir = getenv("abs_top_srcdir"); if (!abs_top_srcdir) abs_top_srcdir = abs_srcdir "/.."; @@ -2567,15 +2581,20 @@ mymain(void) DO_TEST_PARSE_ERROR("cpu-cache-passthrough3", QEMU_CAPS_KVM); DO_TEST_PARSE_ERROR("cpu-cache-passthrough-l3", QEMU_CAPS_KVM); + if (getenv("LIBVIRT_SKIP_CLEANUP") == NULL) + virFileDeleteTree(fakerootdir); + qemuTestDriverFree(&driver); + VIR_FREE(fakerootdir); return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE; } VIR_TEST_MAIN_PRELOAD(mymain, - abs_builddir "/.libs/qemuxml2argvmock.so", - abs_builddir "/.libs/virrandommock.so", - abs_builddir "/.libs/qemucpumock.so") + abs_builddir "/.libs/qemuxml2argvmock.so", + abs_builddir "/.libs/virrandommock.so", + abs_builddir "/.libs/qemucpumock.so", + abs_builddir "/.libs/virpcimock.so") #else diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index fff13e2..3c8dbc4 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -303,14 +303,28 @@ testInfoSet(struct testInfo *info, return -1; } +# define FAKEROOTDIRTEMPLATE abs_builddir "/fakerootdir-XXXXXX" static int mymain(void) { int ret = 0; + char *fakerootdir; struct testInfo info; virQEMUDriverConfigPtr cfg = NULL; + if (VIR_STRDUP_QUIET(fakerootdir, FAKEROOTDIRTEMPLATE) < 0) { + fprintf(stderr, "Out of memory\n"); + abort(); + } + + if (!mkdtemp(fakerootdir)) { + fprintf(stderr, "Cannot create fakerootdir"); + abort(); + } + + setenv("LIBVIRT_FAKE_ROOT_DIR", fakerootdir, 1); + memset(&info, 0, sizeof(info)); if (qemuTestDriverInit(&driver) < 0) @@ -1137,12 +1151,18 @@ mymain(void) DO_TEST("cpu-check-default-partial", NONE); DO_TEST("cpu-check-default-partial2", NONE); + if (getenv("LIBVIRT_SKIP_CLEANUP") == NULL) + virFileDeleteTree(fakerootdir); + qemuTestDriverFree(&driver); + VIR_FREE(fakerootdir); return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE; } -VIR_TEST_MAIN_PRELOAD(mymain, abs_builddir "/.libs/qemuxml2xmlmock.so") +VIR_TEST_MAIN_PRELOAD(mymain, + abs_builddir "/.libs/qemuxml2xmlmock.so", + abs_builddir "/.libs/virpcimock.so") #else diff --git a/tests/virpcimock.c b/tests/virpcimock.c index e9408aa..19b10f4 100644 --- a/tests/virpcimock.c +++ b/tests/virpcimock.c @@ -127,6 +127,7 @@ struct pciDevice { int vendor; int device; int class; + int iommuGroup; struct pciDriver *driver; /* Driver attached. NULL if attached to no driver */ }; @@ -190,6 +191,22 @@ make_file(const char *path, VIR_FREE(filepath); } +static void +make_symlink(const char *path, + const char *name, + const char *target) +{ + char *filepath = NULL; + + if (virAsprintfQuiet(&filepath, "%s/%s", path, name) < 0) + ABORT_OOM(); + + if (symlink(target, filepath) < 0) + ABORT("Unable to create symlink filepath -> target"); + + VIR_FREE(filepath); +} + static int pci_read_file(const char *path, char *buf, @@ -322,7 +339,7 @@ pci_device_new_from_stub(const struct pciDevice *data) char *id; char *c; char *configSrc; - char tmp[32]; + char tmp[256]; struct stat sb; if (VIR_STRDUP_QUIET(id, data->id) < 0) @@ -386,6 +403,20 @@ pci_device_new_from_stub(const struct pciDevice *data) ABORT("@tmp overflow"); make_file(devpath, "class", tmp, -1); + if (snprintf(tmp, sizeof(tmp), + "%s/../../../kernel/iommu_groups/%d", + devpath, dev->iommuGroup) < 0) { + ABORT("@tmp overflow"); + } + if (virFileMakePath(tmp) < 0) + ABORT("Unable to create %s", tmp); + + if (snprintf(tmp, sizeof(tmp), + "../../../kernel/iommu_groups/%d", dev->iommuGroup) < 0) { + ABORT("@tmp overflow"); + } + make_symlink(devpath, "iommu_group", tmp); + if (pci_device_autobind(dev) < 0) ABORT("Unable to bind: %s", data->id); @@ -821,12 +852,12 @@ init_env(void) MAKE_PCI_DEVICE("0000:00:02.0", 0x8086, 0x0046); MAKE_PCI_DEVICE("0000:00:03.0", 0x8086, 0x0048); MAKE_PCI_DEVICE("0001:00:00.0", 0x1014, 0x03b9, .class = 0x060400); - MAKE_PCI_DEVICE("0001:01:00.0", 0x8086, 0x105e); - MAKE_PCI_DEVICE("0001:01:00.1", 0x8086, 0x105e); + MAKE_PCI_DEVICE("0001:01:00.0", 0x8086, 0x105e, .iommuGroup = 2); + MAKE_PCI_DEVICE("0001:01:00.1", 0x8086, 0x105e, .iommuGroup = 2); MAKE_PCI_DEVICE("0005:80:00.0", 0x10b5, 0x8112, .class = 0x060400); - MAKE_PCI_DEVICE("0005:90:01.0", 0x1033, 0x0035); - MAKE_PCI_DEVICE("0005:90:01.1", 0x1033, 0x0035); - MAKE_PCI_DEVICE("0005:90:01.2", 0x1033, 0x00e0); + MAKE_PCI_DEVICE("0005:90:01.0", 0x1033, 0x0035, .iommuGroup = 3); + MAKE_PCI_DEVICE("0005:90:01.1", 0x1033, 0x0035, .iommuGroup = 3); + MAKE_PCI_DEVICE("0005:90:01.2", 0x1033, 0x00e0, .iommuGroup = 3); MAKE_PCI_DEVICE("0000:0a:01.0", 0x8086, 0x0047); MAKE_PCI_DEVICE("0000:0a:02.0", 0x8286, 0x0048); MAKE_PCI_DEVICE("0000:0a:03.0", 0x8386, 0x0048); -- 2.7.5

The current algorithm for slot allocation tries to be clever and avoid looking at buses / slots more than once unless it's necessary. Unfortunately that makes the code more complex, and it will cause problem later on in some situations unless even more complex code is added. Since the performance gains are going to be pretty modest anyway, we can just get rid of the extra complexity and use a completely straighforward implementation instead. --- src/conf/domain_addr.c | 43 +++++++------------------------------------ src/conf/domain_addr.h | 2 -- 2 files changed, 7 insertions(+), 38 deletions(-) diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c index 6fd8bfc..d173766 100644 --- a/src/conf/domain_addr.c +++ b/src/conf/domain_addr.c @@ -741,34 +741,26 @@ virDomainPCIAddressGetNextAddr(virDomainPCIAddressSetPtr addrs, virDomainPCIConnectFlags flags, int function) { - /* default to starting the search for a free slot from - * the first slot of domain 0 bus 0... - */ virPCIDeviceAddress a = { 0 }; - bool found = false; if (addrs->nbuses == 0) { virReportError(VIR_ERR_XML_ERROR, "%s", _("No PCI buses available")); goto error; } - /* ...unless this search is for the exact same type of device as - * last time, then continue the search from the slot where we - * found the previous match (it's possible there will still be a - * function available on that slot). - */ - if (flags == addrs->lastFlags) - a = addrs->lastaddr; - else - a.slot = addrs->buses[0].minSlot; - /* if the caller asks for "any function", give them function 0 */ if (function == -1) a.function = 0; else a.function = function; - while (a.bus < addrs->nbuses) { + /* "Begin at the beginning," the King said, very gravely, "and go on + * till you come to the end: then stop." */ + for (a.bus = 0; a.bus < addrs->nbuses; a.bus++) { + bool found = false; + + a.slot = addrs->buses[a.bus].minSlot; + if (virDomainPCIAddressFindUnusedFunctionOnBus(&addrs->buses[a.bus], &a, function, flags, &found) < 0) { @@ -777,10 +769,6 @@ virDomainPCIAddressGetNextAddr(virDomainPCIAddressSetPtr addrs, if (found) goto success; - - /* nothing on this bus, go to the next bus */ - if (++a.bus < addrs->nbuses) - a.slot = addrs->buses[a.bus].minSlot; } /* There were no free slots after the last used one */ @@ -791,20 +779,6 @@ virDomainPCIAddressGetNextAddr(virDomainPCIAddressSetPtr addrs, /* this device will use the first slot of the new bus */ a.slot = addrs->buses[a.bus].minSlot; goto success; - } else if (flags == addrs->lastFlags) { - /* Check the buses from 0 up to the last used one */ - for (a.bus = 0; a.bus <= addrs->lastaddr.bus; a.bus++) { - a.slot = addrs->buses[a.bus].minSlot; - - if (virDomainPCIAddressFindUnusedFunctionOnBus(&addrs->buses[a.bus], - &a, function, - flags, &found) < 0) { - goto error; - } - - if (found) - goto success; - } } virReportError(VIR_ERR_INTERNAL_ERROR, @@ -851,9 +825,6 @@ virDomainPCIAddressReserveNextAddr(virDomainPCIAddressSetPtr addrs, if (virDomainPCIAddressReserveAddrInternal(addrs, &addr, flags, false) < 0) return -1; - addrs->lastaddr = addr; - addrs->lastFlags = flags; - if (!addrs->dryRun) { dev->type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI; dev->addr.pci = addr; diff --git a/src/conf/domain_addr.h b/src/conf/domain_addr.h index 77e19bb..7704061 100644 --- a/src/conf/domain_addr.h +++ b/src/conf/domain_addr.h @@ -106,8 +106,6 @@ typedef virDomainPCIAddressBus *virDomainPCIAddressBusPtr; struct _virDomainPCIAddressSet { virDomainPCIAddressBus *buses; size_t nbuses; - virPCIDeviceAddress lastaddr; - virDomainPCIConnectFlags lastFlags; bool dryRun; /* on a dry run, new buses are auto-added and addresses aren't saved in device infos */ }; -- 2.7.5

We will soon need to be able to return a NULL pointer without the caller considering that an error: to make it possible, change the return type to int and use an out parameter for the string instead. Add some documentation for the function as well. --- src/qemu/qemu_command.c | 53 ++++++++++++++++++++++++++++++++++++------------- src/qemu/qemu_command.h | 9 +++++---- src/qemu/qemu_hotplug.c | 5 ++++- 3 files changed, 48 insertions(+), 19 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 015af10..a0403bf 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -2664,10 +2664,31 @@ qemuCheckSCSIControllerIOThreads(const virDomainDef *domainDef, } -char * +/** + * qemuBuildControllerDevStr: + * @domainDef: domain definition + * @def: controller definition + * @qemuCaps: QEMU binary capabilities + * @devstr: device string + * @nusbcontroller: number of USB controllers + * + * Turn @def into a description of the controller that QEMU will understand, + * to be used either on the command line or through the monitor. + * + * The description will be returned in @devstr and can be NULL, eg. when + * passing in one of the built-in controllers. The returned string must be + * freed by the caller. + * + * The number pointed to by @nusbcontroller will be increased by one every + * time the description for a USB controller has been generated successfully. + * + * Returns: 0 on success, <0 on failure + */ +int qemuBuildControllerDevStr(const virDomainDef *domainDef, virDomainControllerDefPtr def, virQEMUCapsPtr qemuCaps, + char **devstr, int *nusbcontroller) { virBuffer buf = VIR_BUFFER_INITIALIZER; @@ -2676,11 +2697,11 @@ qemuBuildControllerDevStr(const virDomainDef *domainDef, if (!qemuCheckCCWS390AddressSupport(domainDef, def->info, qemuCaps, "controller")) - return NULL; + return -1; if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_SCSI) { if ((qemuDomainSetSCSIControllerModel(domainDef, qemuCaps, &model)) < 0) - return NULL; + return -1; } if (!(def->type == VIR_DOMAIN_CONTROLLER_TYPE_SCSI && @@ -2688,22 +2709,22 @@ qemuBuildControllerDevStr(const virDomainDef *domainDef, if (def->queues) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("'queues' is only supported by virtio-scsi controller")); - return NULL; + return -1; } if (def->cmd_per_lun) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("'cmd_per_lun' is only supported by virtio-scsi controller")); - return NULL; + return -1; } if (def->max_sectors) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("'max_sectors' is only supported by virtio-scsi controller")); - return NULL; + return -1; } if (def->ioeventfd) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("'ioeventfd' is only supported by virtio-scsi controller")); - return NULL; + return -1; } } @@ -3114,11 +3135,12 @@ qemuBuildControllerDevStr(const virDomainDef *domainDef, if (virBufferCheckError(&buf) < 0) goto error; - return virBufferContentAndReset(&buf); + *devstr = virBufferContentAndReset(&buf); + return 0; error: virBufferFreeAndReset(&buf); - return NULL; + return -1; } @@ -3213,12 +3235,15 @@ qemuBuildControllerDevCommandLine(virCommandPtr cmd, continue; } - virCommandAddArg(cmd, "-device"); - if (!(devstr = qemuBuildControllerDevStr(def, cont, qemuCaps, - &usbcontroller))) + if (qemuBuildControllerDevStr(def, cont, qemuCaps, + &devstr, &usbcontroller) < 0) return -1; - virCommandAddArg(cmd, devstr); - VIR_FREE(devstr); + + if (devstr) { + virCommandAddArg(cmd, "-device"); + virCommandAddArg(cmd, devstr); + VIR_FREE(devstr); + } } } diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h index 09cb00e..9a2ab29 100644 --- a/src/qemu/qemu_command.h +++ b/src/qemu/qemu_command.h @@ -118,10 +118,11 @@ char *qemuBuildDriveDevStr(const virDomainDef *def, virQEMUCapsPtr qemuCaps); /* Current, best practice */ -char *qemuBuildControllerDevStr(const virDomainDef *domainDef, - virDomainControllerDefPtr def, - virQEMUCapsPtr qemuCaps, - int *nusbcontroller); +int qemuBuildControllerDevStr(const virDomainDef *domainDef, + virDomainControllerDefPtr def, + virQEMUCapsPtr qemuCaps, + char **devstr, + int *nusbcontroller); int qemuBuildMemoryBackendStr(virJSONValuePtr *backendProps, const char **backendType, diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 4a7d997..f910012 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -523,7 +523,10 @@ int qemuDomainAttachControllerDevice(virQEMUDriverPtr driver, if (qemuAssignDeviceControllerAlias(vm->def, priv->qemuCaps, controller) < 0) goto cleanup; - if (!(devstr = qemuBuildControllerDevStr(vm->def, controller, priv->qemuCaps, NULL))) + if (qemuBuildControllerDevStr(vm->def, controller, priv->qemuCaps, &devstr, NULL) < 0) + goto cleanup; + + if (!devstr) goto cleanup; if (VIR_REALLOC_N(vm->def->controllers, vm->def->ncontrollers+1) < 0) -- 2.7.5

Moving the check and rewriting it this way doesn't alter the current behavior, but will allow us to special-case pci-root down the line. --- src/qemu/qemu_command.c | 26 ++++++++++++++++++++------ 1 file changed, 20 insertions(+), 6 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index a0403bf..082ffa9 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -2828,6 +2828,26 @@ qemuBuildControllerDevStr(const virDomainDef *domainDef, case VIR_DOMAIN_CONTROLLER_TYPE_PCI: switch ((virDomainControllerModelPCI) def->model) { case VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE: + case VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE: + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT_PORT: + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_UPSTREAM_PORT: + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_DOWNSTREAM_PORT: + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_EXPANDER_BUS: + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_EXPANDER_BUS: + if (def->idx == 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("index for pci controllers of model '%s' must be > 0"), + virDomainControllerModelPCITypeToString(def->model)); + goto error; + } + break; + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT: + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT: + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST: + break; + } + switch ((virDomainControllerModelPCI) def->model) { + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE: if (def->opts.pciopts.modelName == VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_NONE || def->opts.pciopts.chassisNr == -1) { @@ -3086,12 +3106,6 @@ qemuBuildControllerDevStr(const virDomainDef *domainDef, _("wrong function called")); goto error; } - if (def->idx == 0) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("index for pci controllers of model '%s' must be > 0"), - virDomainControllerModelPCITypeToString(def->model)); - goto error; - } break; case VIR_DOMAIN_CONTROLLER_TYPE_IDE: -- 2.7.5

pSeries guests will soon be allowed to have multiple PHBs (pci-root controllers), which of course means that all but one of them will have a non-zero index; hence, we'll need to relax the current check. However, right now the check is performed in the conf module, which is generic rather than tied to the QEMU driver, and where we don't have information such as the guest machine type available. To make this change of behavior possible down the line, we need to move the check from the XML parser to the driver. Unfortunately, this means duplicating code :( --- src/bhyve/bhyve_domain.c | 15 +++++++++++++++ src/conf/domain_conf.c | 6 ------ src/libxl/libxl_domain.c | 14 ++++++++++++++ src/lxc/lxc_domain.c | 14 ++++++++++++++ src/openvz/openvz_driver.c | 14 ++++++++++++++ src/qemu/qemu_domain.c | 9 +++++++++ src/uml/uml_driver.c | 14 ++++++++++++++ src/vz/vz_driver.c | 14 ++++++++++++++ src/xen/xen_driver.c | 14 ++++++++++++++ 9 files changed, 108 insertions(+), 6 deletions(-) diff --git a/src/bhyve/bhyve_domain.c b/src/bhyve/bhyve_domain.c index 76b4fac..05c1508 100644 --- a/src/bhyve/bhyve_domain.c +++ b/src/bhyve/bhyve_domain.c @@ -122,6 +122,21 @@ bhyveDomainDeviceDefPostParse(virDomainDeviceDefPtr dev, bhyveDomainDiskDefAssignAddress(driver, disk, def) < 0) return -1; } + + if (dev->type == VIR_DOMAIN_DEVICE_CONTROLLER) { + virDomainControllerDefPtr cont = dev->data.controller; + + if (cont->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI && + (cont->model == VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT || + cont->model == VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT) && + cont->idx != 0) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("pci-root and pcie-root controllers " + "should have index 0")); + return -1; + } + } + return 0; } diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index c7e20b8..d5dff8e 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -9059,12 +9059,6 @@ virDomainControllerDefParseXML(xmlNodePtr node, "have an address")); goto error; } - if (def->idx > 0) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("pci-root and pcie-root controllers " - "should have index 0")); - goto error; - } if ((rc = virDomainParseScaledValue("./pcihole64", NULL, ctxt, &bytes, 1024, 1024ULL * ULONG_MAX, false)) < 0) diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c index 256cf1d..59ef2fb 100644 --- a/src/libxl/libxl_domain.c +++ b/src/libxl/libxl_domain.c @@ -379,6 +379,20 @@ libxlDomainDeviceDefPostParse(virDomainDeviceDefPtr dev, virDomainDiskSetFormat(disk, VIR_STORAGE_FILE_RAW); } + if (dev->type == VIR_DOMAIN_DEVICE_CONTROLLER) { + virDomainControllerDefPtr cont = dev->data.controller; + + if (cont->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI && + (cont->model == VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT || + cont->model == VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT) && + cont->idx != 0) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("pci-root and pcie-root controllers " + "should have index 0")); + return -1; + } + } + return 0; } diff --git a/src/lxc/lxc_domain.c b/src/lxc/lxc_domain.c index 3a7404f..a50f6fe 100644 --- a/src/lxc/lxc_domain.c +++ b/src/lxc/lxc_domain.c @@ -389,6 +389,20 @@ virLXCDomainDeviceDefPostParse(virDomainDeviceDefPtr dev, dev->data.chr->targetType == VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_NONE) dev->data.chr->targetType = VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_LXC; + if (dev->type == VIR_DOMAIN_DEVICE_CONTROLLER) { + virDomainControllerDefPtr cont = dev->data.controller; + + if (cont->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI && + (cont->model == VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT || + cont->model == VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT) && + cont->idx != 0) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("pci-root and pcie-root controllers " + "should have index 0")); + return -1; + } + } + return 0; } diff --git a/src/openvz/openvz_driver.c b/src/openvz/openvz_driver.c index 9c4a196..058d830 100644 --- a/src/openvz/openvz_driver.c +++ b/src/openvz/openvz_driver.c @@ -127,6 +127,20 @@ openvzDomainDeviceDefPostParse(virDomainDeviceDefPtr dev, return -1; } + if (dev->type == VIR_DOMAIN_DEVICE_CONTROLLER) { + virDomainControllerDefPtr cont = dev->data.controller; + + if (cont->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI && + (cont->model == VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT || + cont->model == VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT) && + cont->idx != 0) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("pci-root and pcie-root controllers " + "should have index 0")); + return -1; + } + } + return 0; } diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 0a85ee9..18512cb 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -3404,6 +3404,15 @@ qemuDomainControllerDefPostParse(virDomainControllerDefPtr cont, break; case VIR_DOMAIN_CONTROLLER_TYPE_PCI: + if ((cont->model == VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT || + cont->model == VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT) && + cont->idx != 0) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("pci-root and pcie-root controllers " + "should have index 0")); + return -1; + } + if (cont->model == VIR_DOMAIN_CONTROLLER_MODEL_PCI_EXPANDER_BUS && !qemuDomainIsI440FX(def)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", diff --git a/src/uml/uml_driver.c b/src/uml/uml_driver.c index 03edc89..d9df3c5 100644 --- a/src/uml/uml_driver.c +++ b/src/uml/uml_driver.c @@ -426,6 +426,20 @@ umlDomainDeviceDefPostParse(virDomainDeviceDefPtr dev, return -1; } + if (dev->type == VIR_DOMAIN_DEVICE_CONTROLLER) { + virDomainControllerDefPtr cont = dev->data.controller; + + if (cont->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI && + (cont->model == VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT || + cont->model == VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT) && + cont->idx != 0) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("pci-root and pcie-root controllers " + "should have index 0")); + return -1; + } + } + return 0; } diff --git a/src/vz/vz_driver.c b/src/vz/vz_driver.c index ef7b453..e988e7c 100644 --- a/src/vz/vz_driver.c +++ b/src/vz/vz_driver.c @@ -283,6 +283,20 @@ vzDomainDeviceDefPostParse(virDomainDeviceDefPtr dev, VIR_STRDUP(dev->data.net->model, "e1000") < 0) return -1; + if (dev->type == VIR_DOMAIN_DEVICE_CONTROLLER) { + virDomainControllerDefPtr cont = dev->data.controller; + + if (cont->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI && + (cont->model == VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT || + cont->model == VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT) && + cont->idx != 0) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("pci-root and pcie-root controllers " + "should have index 0")); + return -1; + } + } + return 0; } diff --git a/src/xen/xen_driver.c b/src/xen/xen_driver.c index 4d14089..b681764 100644 --- a/src/xen/xen_driver.c +++ b/src/xen/xen_driver.c @@ -362,6 +362,20 @@ xenDomainDeviceDefPostParse(virDomainDeviceDefPtr dev, } } + if (dev->type == VIR_DOMAIN_DEVICE_CONTROLLER) { + virDomainControllerDefPtr cont = dev->data.controller; + + if (cont->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI && + (cont->model == VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT || + cont->model == VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT) && + cont->idx != 0) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("pci-root and pcie-root controllers " + "should have index 0")); + return -1; + } + } + return 0; } -- 2.7.5

On 06/12/2017 10:08 PM, Laine Stump wrote: After looking at the next patch, I may have changed my mind. If we remove the validation here, then we would still have to add in validation when the commandline is generated. But I suppose it's better to give an error at domain definition time rather than domain runtime, so this makes sense. I don't think all of those drivers actually use PCI controllers though, do they? (Look for which ones actually call the functions to assign PCI addresses).

pSeries guests will soon be allowed to have multiple PHBs (pci-root controllers), meaning the current check on the controller index no longer applies to them. --- src/qemu/qemu_domain.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 18512cb..7dc90fd 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -3404,9 +3404,14 @@ qemuDomainControllerDefPostParse(virDomainControllerDefPtr cont, break; case VIR_DOMAIN_CONTROLLER_TYPE_PCI: - if ((cont->model == VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT || - cont->model == VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT) && - cont->idx != 0) { + + /* pSeries guests can have multiple pci-root controllers, + * but other machine types only support a single one */ + if (!qemuDomainIsPSeries(def) && + cont->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI && + (cont->model == VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT || + cont->model == VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT) && + cont->idx != 0) { virReportError(VIR_ERR_XML_ERROR, "%s", _("pci-root and pcie-root controllers " "should have index 0")); -- 2.7.5

--- docs/schemas/domaincommon.rng | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 4d9f8d1..cff595b 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -1996,6 +1996,11 @@ </attribute> </optional> <optional> + <attribute name='index'> + <ref name='uint8'/> + </attribute> + </optional> + <optional> <element name='node'> <ref name='unsignedInt'/> </element> -- 2.7.5

On 06/02/2017 12:07 PM, Andrea Bolognani wrote: You should probably say "target index" to differentiate it from the toplevel index. Same here. Other than that, what about an xml2xml test case? (as well as combining with the previous patch, and adding a sentence to formatdomain.html.in)

--- docs/schemas/domaincommon.rng | 2 ++ 1 file changed, 2 insertions(+) diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index cff595b..d565a0b 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -1953,6 +1953,8 @@ <element name="model"> <attribute name="name"> <choice> + <!-- implementations of 'pci-root' --> + <value>spapr-pci-host-bridge</value> <!-- implementations of 'pci-bridge' --> <value>pci-bridge</value> <!-- implementations of 'dmi-to-pci-bridge' --> -- 2.7.5

pSeries guests will soon need the new information; luckily, we can figure it out automatically most of the time, so users won't have to bother with it. --- src/qemu/qemu_domain_address.c | 57 +++++++++++++++++++++- .../qemuargv2xmldata/qemuargv2xml-pseries-disk.xml | 5 +- .../qemuargv2xml-pseries-nvram.xml | 5 +- .../qemuxml2xmlout-panic-pseries.xml | 5 +- .../qemuxml2xmlout-ppc64-usb-controller-legacy.xml | 5 +- .../qemuxml2xmlout-ppc64-usb-controller.xml | 5 +- .../qemuxml2xmlout-pseries-nvram.xml | 5 +- .../qemuxml2xmlout-pseries-panic-missing.xml | 5 +- .../qemuxml2xmlout-pseries-panic-no-address.xml | 5 +- 9 files changed, 87 insertions(+), 10 deletions(-) diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c index 2106b34..d46f8f7 100644 --- a/src/qemu/qemu_domain_address.c +++ b/src/qemu/qemu_domain_address.c @@ -1856,6 +1856,7 @@ qemuDomainSupportsPCI(virDomainDefPtr def, static void qemuDomainPCIControllerSetDefaultModelName(virDomainControllerDefPtr cont, + virDomainDefPtr def, virQEMUCapsPtr qemuCaps) { int *modelName = &cont->opts.pciopts.modelName; @@ -1892,6 +1893,9 @@ qemuDomainPCIControllerSetDefaultModelName(virDomainControllerDefPtr cont, *modelName = VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_PXB_PCIE; break; case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT: + if (qemuDomainIsPSeries(def)) + *modelName = VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_SPAPR_PCI_HOST_BRIDGE; + break; case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT: case VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST: break; @@ -1900,6 +1904,30 @@ qemuDomainPCIControllerSetDefaultModelName(virDomainControllerDefPtr cont, static int +qemuDomainAddressFindNewIndex(virDomainDefPtr def) +{ + int ret = 1; + size_t i; + + for (i = 0; i < def->ncontrollers; i++) { + virDomainControllerDefPtr cont = def->controllers[i]; + + if (cont->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI) { + if (cont->opts.pciopts.idx >= ret) + ret = cont->opts.pciopts.idx + 1; + } + } + + /* 0 is reserved for the implicit PHB, whereas anything lower + * than 0 or higher than 31 is invalid */ + if (ret > 31) + ret = -1; + + return ret; +} + + +static int qemuDomainAddressFindNewBusNr(virDomainDefPtr def) { /* Try to find a nice default for busNr for a new pci-expander-bus. @@ -2159,7 +2187,7 @@ qemuDomainAssignPCIAddresses(virDomainDefPtr def, * device in qemu) for any controller that doesn't yet * have it set. */ - qemuDomainPCIControllerSetDefaultModelName(cont, qemuCaps); + qemuDomainPCIControllerSetDefaultModelName(cont, def, qemuCaps); /* set defaults for any other auto-generated config * options for this controller that haven't been @@ -2196,9 +2224,34 @@ qemuDomainAssignPCIAddresses(virDomainDefPtr def, goto cleanup; } break; + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT: + if (!qemuDomainIsPSeries(def)) + break; + if (options->idx == -1) { + if (cont->idx == 0) { + /* The pcie-root controller with controller index 0 + * must always be assigned target index 0, because + * it represents the implicit PHB which is treated + * differently than all other PHBs */ + options->idx = 0; + } else { + /* For all other PHBs the target index doesn't need + * to match the controller index or have any + * particular value, really */ + options->idx = qemuDomainAddressFindNewIndex(def); + } + } + if (options->idx == -1) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("No valid index is available to " + "auto-assign to bus %d. Must be " + "manually assigned"), + addr->bus); + goto cleanup; + } + break; case VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE: case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_UPSTREAM_PORT: - case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT: case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT: case VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST: break; diff --git a/tests/qemuargv2xmldata/qemuargv2xml-pseries-disk.xml b/tests/qemuargv2xmldata/qemuargv2xml-pseries-disk.xml index 1bad8ee..601d0f7 100644 --- a/tests/qemuargv2xmldata/qemuargv2xml-pseries-disk.xml +++ b/tests/qemuargv2xmldata/qemuargv2xml-pseries-disk.xml @@ -30,7 +30,10 @@ <controller type='usb' index='0'> <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x0'/> </controller> - <controller type='pci' index='0' model='pci-root'/> + <controller type='pci' index='0' model='pci-root'> + <model name='spapr-pci-host-bridge'/> + <target index='0'/> + </controller> <controller type='scsi' index='0'> <address type='spapr-vio' reg='0x2000'/> </controller> diff --git a/tests/qemuargv2xmldata/qemuargv2xml-pseries-nvram.xml b/tests/qemuargv2xmldata/qemuargv2xml-pseries-nvram.xml index 7e9f864..7787847 100644 --- a/tests/qemuargv2xmldata/qemuargv2xml-pseries-nvram.xml +++ b/tests/qemuargv2xmldata/qemuargv2xml-pseries-nvram.xml @@ -17,7 +17,10 @@ <controller type='usb' index='0'> <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x0'/> </controller> - <controller type='pci' index='0' model='pci-root'/> + <controller type='pci' index='0' model='pci-root'> + <model name='spapr-pci-host-bridge'/> + <target index='0'/> + </controller> <memballoon model='none'/> <nvram> <address type='spapr-vio' reg='0x4000'/> diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-panic-pseries.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-panic-pseries.xml index 1ed11ce..7fb49fe 100644 --- a/tests/qemuxml2xmloutdata/qemuxml2xmlout-panic-pseries.xml +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-panic-pseries.xml @@ -17,7 +17,10 @@ <controller type='usb' index='0'> <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x0'/> </controller> - <controller type='pci' index='0' model='pci-root'/> + <controller type='pci' index='0' model='pci-root'> + <model name='spapr-pci-host-bridge'/> + <target index='0'/> + </controller> <serial type='pty'> <target port='0'/> <address type='spapr-vio' reg='0x30000000'/> diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-ppc64-usb-controller-legacy.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-ppc64-usb-controller-legacy.xml index b7bde24..673b81d 100644 --- a/tests/qemuxml2xmloutdata/qemuxml2xmlout-ppc64-usb-controller-legacy.xml +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-ppc64-usb-controller-legacy.xml @@ -22,7 +22,10 @@ <controller type='usb' index='0'> <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x0'/> </controller> - <controller type='pci' index='0' model='pci-root'/> + <controller type='pci' index='0' model='pci-root'> + <model name='spapr-pci-host-bridge'/> + <target index='0'/> + </controller> <memballoon model='virtio'> <address type='pci' domain='0x0000' bus='0x00' slot='0x06' function='0x0'/> </memballoon> diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-ppc64-usb-controller.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-ppc64-usb-controller.xml index 82aaaca..68995a9 100644 --- a/tests/qemuxml2xmloutdata/qemuxml2xmlout-ppc64-usb-controller.xml +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-ppc64-usb-controller.xml @@ -22,7 +22,10 @@ <controller type='usb' index='0' model='pci-ohci'> <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x0'/> </controller> - <controller type='pci' index='0' model='pci-root'/> + <controller type='pci' index='0' model='pci-root'> + <model name='spapr-pci-host-bridge'/> + <target index='0'/> + </controller> <memballoon model='virtio'> <address type='pci' domain='0x0000' bus='0x00' slot='0x06' function='0x0'/> </memballoon> diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-pseries-nvram.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-pseries-nvram.xml index 713f31c..f89b23b 100644 --- a/tests/qemuxml2xmloutdata/qemuxml2xmlout-pseries-nvram.xml +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-pseries-nvram.xml @@ -17,7 +17,10 @@ <controller type='usb' index='0'> <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x0'/> </controller> - <controller type='pci' index='0' model='pci-root'/> + <controller type='pci' index='0' model='pci-root'> + <model name='spapr-pci-host-bridge'/> + <target index='0'/> + </controller> <memballoon model='none'/> <nvram> <address type='spapr-vio' reg='0x4000'/> diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-pseries-panic-missing.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-pseries-panic-missing.xml index 1ed11ce..7fb49fe 100644 --- a/tests/qemuxml2xmloutdata/qemuxml2xmlout-pseries-panic-missing.xml +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-pseries-panic-missing.xml @@ -17,7 +17,10 @@ <controller type='usb' index='0'> <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x0'/> </controller> - <controller type='pci' index='0' model='pci-root'/> + <controller type='pci' index='0' model='pci-root'> + <model name='spapr-pci-host-bridge'/> + <target index='0'/> + </controller> <serial type='pty'> <target port='0'/> <address type='spapr-vio' reg='0x30000000'/> diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-pseries-panic-no-address.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-pseries-panic-no-address.xml index 1ed11ce..7fb49fe 100644 --- a/tests/qemuxml2xmloutdata/qemuxml2xmlout-pseries-panic-no-address.xml +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-pseries-panic-no-address.xml @@ -17,7 +17,10 @@ <controller type='usb' index='0'> <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x0'/> </controller> - <controller type='pci' index='0' model='pci-root'/> + <controller type='pci' index='0' model='pci-root'> + <model name='spapr-pci-host-bridge'/> + <target index='0'/> + </controller> <serial type='pty'> <target port='0'/> <address type='spapr-vio' reg='0x30000000'/> -- 2.7.5

On Tue, 2017-06-13 at 21:52 -0400, Laine Stump wrote: Yup. Sure. Zero is going to be used for the default PHB, so I'd have to start from one instead. But I like the idea :) It's not unimportant! Comments that disagree with the code can cause a lot of confusion. Good catch :) Yes to both. I can't think of a single sensible reason why you'd want to have gaps, but it's a valid configuration. Indeed. I just realized I'm not making sure user-assigned target indexes are contained in the allowed range either, so I'll add that as well. --  Andrea Bolognani / Red Hat / Virtualization

On 06/02/2017 12:07 PM, Andrea Bolognani wrote: Reviewed-by: Laine Stump <laine(a)laine.org&gt;

On 06/02/2017 12:07 PM, Andrea Bolognani wrote: This doesn't make sense to me. My recollection (without going through the code again) is that for PCI controllers, the alias for pci-root is "pci.0" as long as the qemu binary supports multiple PCI controllers (otherwise it is just "pci"), and the alias is pcie.0 for pcie-root, then all other PCI controllers have the alias (i.e. value of "contAlias" in this function) of pci.x (where "x" is the index of the controller). What you're saying here doesn't match that. Unless you're saying that the PHBs will have aliases named something like "pci.1.0" (which isn't what I see in the test cases in 19/26). I guess I need time to digest why this is necessary. Based on my memory, it should have worked properly already...

Additional PHBs (pci-root controllers) will be created for the guest using the spapr-pci-host-bridge QEMU device, if available; the implicit default PHB, while present in the guest configuration, will be skipped. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1431193 --- src/qemu/qemu_command.c | 47 ++++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 44 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 1bc3093..b0b6c85 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -3118,6 +3118,40 @@ qemuBuildControllerDevStr(const virDomainDef *domainDef, def->opts.pciopts.numaNode); break; case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT: + if (def->opts.pciopts.modelName == VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_NONE || + def->opts.pciopts.idx == -1) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("autogenerated pci-root options not set")); + goto error; + } + + /* Skip the implicit one */ + if (def->opts.pciopts.idx == 0) + goto done; + + modelName = virDomainControllerPCIModelNameTypeToString(def->opts.pciopts.modelName); + if (!modelName) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("unknown pci-root model name value %d"), + def->opts.pciopts.modelName); + goto error; + } + if (def->opts.pciopts.modelName != VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_SPAPR_PCI_HOST_BRIDGE) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("PCI controller model name '%s' is not valid for a pci-root"), + modelName); + goto error; + } + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_SPAPR_PCI_HOST_BRIDGE)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("the spapr-pci-host-bridge controller " + "is not supported in this QEMU binary")); + goto error; + } + virBufferAsprintf(&buf, "%s,index=%d,id=%s", + modelName, def->opts.pciopts.idx, + def->info.alias); + break; case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT: case VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST: virReportError(VIR_ERR_INTERNAL_ERROR, "%s", @@ -3167,6 +3201,7 @@ qemuBuildControllerDevStr(const virDomainDef *domainDef, if (virBufferCheckError(&buf) < 0) goto error; + done: *devstr = virBufferContentAndReset(&buf); return 0; @@ -3224,10 +3259,16 @@ qemuBuildControllerDevCommandLine(virCommandPtr cmd, continue; } - /* skip pci-root/pcie-root */ + /* skip pcie-root */ if (cont->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI && - (cont->model == VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT || - cont->model == VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT)) + cont->model == VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT) + continue; + + /* Skip pci-root, except for pSeries guests (which actually + * support more than one PCI Host Bridge per guest) */ + if (!qemuDomainIsPSeries(def) && + cont->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI && + cont->model == VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT) continue; /* first SATA controller on Q35 machines is implicit */ -- 2.7.5

These tests demonstrate that, while it's now possible for the user to create PHB explicitly and manually assign devices to them, libvirt still defaults to extending the guest PCI topology using PCI bridges and making suboptimal device placement choices. The next few commits will improve on these behaviors and the tests outputs will automatically be updated to reflect this. --- .../qemuxml2argv-pseries-hostdev.args | 23 ++++ .../qemuxml2argv-pseries-hostdev.xml | 33 ++++++ .../qemuxml2argv-pseries-many-buses-1.args | 22 ++++ .../qemuxml2argv-pseries-many-buses-1.xml | 20 ++++ .../qemuxml2argv-pseries-many-buses-2.args | 22 ++++ .../qemuxml2argv-pseries-many-buses-2.xml | 18 +++ .../qemuxml2argv-pseries-many-devices.args | 53 +++++++++ .../qemuxml2argv-pseries-many-devices.xml | 49 ++++++++ tests/qemuxml2argvtest.c | 21 ++++ .../qemuxml2xmlout-pseries-hostdev.xml | 46 ++++++++ .../qemuxml2xmlout-pseries-many-buses-1.xml | 33 ++++++ .../qemuxml2xmlout-pseries-many-buses-2.xml | 34 ++++++ .../qemuxml2xmlout-pseries-many-devices.xml | 126 +++++++++++++++++++++ tests/qemuxml2xmltest.c | 21 ++++ 14 files changed, 521 insertions(+) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-pseries-hostdev.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-pseries-hostdev.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-pseries-many-buses-1.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-pseries-many-buses-1.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-pseries-many-buses-2.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-pseries-many-buses-2.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-pseries-many-devices.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-pseries-many-devices.xml create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-pseries-hostdev.xml create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-pseries-many-buses-1.xml create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-pseries-many-buses-2.xml create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-pseries-many-devices.xml diff --git a/tests/qemuxml2argvdata/qemuxml2argv-pseries-hostdev.args b/tests/qemuxml2argvdata/qemuxml2argv-pseries-hostdev.args new file mode 100644 index 0000000..dbd2964 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-pseries-hostdev.args @@ -0,0 +1,23 @@ +LC_ALL=C \ +PATH=/bin \ +HOME=/home/test \ +USER=test \ +LOGNAME=test \ +QEMU_AUDIO_DRV=none \ +/usr/bin/qemu-system-ppc64 \ +-name guest \ +-S \ +-M pseries \ +-m 512 \ +-smp 1,sockets=1,cores=1,threads=1 \ +-uuid 1ccfd97d-5eb4-478a-bbe6-88d254c16db7 \ +-nographic \ +-nodefconfig \ +-nodefaults \ +-chardev socket,id=charmonitor,path=/tmp/lib/domain--1-guest/monitor.sock,\ +server,nowait \ +-mon chardev=charmonitor,id=monitor,mode=readline \ +-boot c \ +-device vfio-pci,host=0001:01:00.0,id=hostdev0,bus=pci.0,addr=0x1 \ +-device vfio-pci,host=0005:90:01.2,id=hostdev1,bus=pci.0,addr=0x2 \ +-device vfio-pci,host=0001:01:00.1,id=hostdev2,bus=pci.0,addr=0x3 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-pseries-hostdev.xml b/tests/qemuxml2argvdata/qemuxml2argv-pseries-hostdev.xml new file mode 100644 index 0000000..2aa6ffc --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-pseries-hostdev.xml @@ -0,0 +1,33 @@ +<domain type='qemu'> + <name>guest</name> + <uuid>1ccfd97d-5eb4-478a-bbe6-88d254c16db7</uuid> + <memory unit='KiB'>524288</memory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='ppc64' machine='pseries'>hvm</type> + </os> + <devices> + <emulator>/usr/bin/qemu-system-ppc64</emulator> + <controller type='pci' model='pci-root'/> + <hostdev mode='subsystem' type='pci' managed='yes'> + <driver name='vfio'/> + <source> + <address domain='0x0001' bus='0x01' slot='0x00' function='0x0'/> + </source> + </hostdev> + <hostdev mode='subsystem' type='pci' managed='yes'> + <driver name='vfio'/> + <source> + <address domain='0x0005' bus='0x90' slot='0x01' function='0x2'/> + </source> + </hostdev> + <hostdev mode='subsystem' type='pci' managed='yes'> + <driver name='vfio'/> + <source> + <address domain='0x0001' bus='0x01' slot='0x00' function='0x1'/> + </source> + </hostdev> + <controller type='usb' model='none'/> + <memballoon model='none'/> + </devices> +</domain> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-pseries-many-buses-1.args b/tests/qemuxml2argvdata/qemuxml2argv-pseries-many-buses-1.args new file mode 100644 index 0000000..bf78fc1 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-pseries-many-buses-1.args @@ -0,0 +1,22 @@ +LC_ALL=C \ +PATH=/bin \ +HOME=/home/test \ +USER=test \ +LOGNAME=test \ +QEMU_AUDIO_DRV=none \ +/usr/bin/qemu-system-ppc64 \ +-name guest \ +-S \ +-M pseries \ +-m 512 \ +-smp 1,sockets=1,cores=1,threads=1 \ +-uuid 1ccfd97d-5eb4-478a-bbe6-88d254c16db7 \ +-nographic \ +-nodefconfig \ +-nodefaults \ +-chardev socket,id=charmonitor,path=/tmp/lib/domain--1-guest/monitor.sock,\ +server,nowait \ +-mon chardev=charmonitor,id=monitor,mode=readline \ +-boot c \ +-device pci-bridge,chassis_nr=1,id=pci.1,bus=pci.0,addr=0x1 \ +-device virtio-scsi-pci,id=scsi0,bus=pci.1,addr=0x1 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-pseries-many-buses-1.xml b/tests/qemuxml2argvdata/qemuxml2argv-pseries-many-buses-1.xml new file mode 100644 index 0000000..8755409 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-pseries-many-buses-1.xml @@ -0,0 +1,20 @@ +<domain type='qemu'> + <name>guest</name> + <uuid>1ccfd97d-5eb4-478a-bbe6-88d254c16db7</uuid> + <memory unit='KiB'>524288</memory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='ppc64' machine='pseries'>hvm</type> + </os> + <devices> + <emulator>/usr/bin/qemu-system-ppc64</emulator> + <controller type='pci' model='pci-root'/> + <!-- The SCSI controller is plugged into PCI bus 1, but since said bus + is not present in the configuration libvirt will have to add it --> + <controller type='scsi' model='virtio-scsi'> + <address type='pci' bus='1' slot='1'/> + </controller> + <controller type='usb' model='none'/> + <memballoon model='none'/> + </devices> +</domain> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-pseries-many-buses-2.args b/tests/qemuxml2argvdata/qemuxml2argv-pseries-many-buses-2.args new file mode 100644 index 0000000..1cb5831 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-pseries-many-buses-2.args @@ -0,0 +1,22 @@ +LC_ALL=C \ +PATH=/bin \ +HOME=/home/test \ +USER=test \ +LOGNAME=test \ +QEMU_AUDIO_DRV=none \ +/usr/bin/qemu-system-ppc64 \ +-name guest \ +-S \ +-M pseries \ +-m 512 \ +-smp 1,sockets=1,cores=1,threads=1 \ +-uuid 1ccfd97d-5eb4-478a-bbe6-88d254c16db7 \ +-nographic \ +-nodefconfig \ +-nodefaults \ +-chardev socket,id=charmonitor,path=/tmp/lib/domain--1-guest/monitor.sock,\ +server,nowait \ +-mon chardev=charmonitor,id=monitor,mode=readline \ +-boot c \ +-device spapr-pci-host-bridge,index=1,id=pci.2 \ +-device pci-bridge,chassis_nr=1,id=pci.1,bus=pci.0,addr=0x1 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-pseries-many-buses-2.xml b/tests/qemuxml2argvdata/qemuxml2argv-pseries-many-buses-2.xml new file mode 100644 index 0000000..7065626 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-pseries-many-buses-2.xml @@ -0,0 +1,18 @@ +<domain type='qemu'> + <name>guest</name> + <uuid>1ccfd97d-5eb4-478a-bbe6-88d254c16db7</uuid> + <memory unit='KiB'>524288</memory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='ppc64' machine='pseries'>hvm</type> + </os> + <devices> + <emulator>/usr/bin/qemu-system-ppc64</emulator> + <!-- PCI buses 0 and 2 are present in the configuration, libvirt will + have to fill in the blanks and add bus 1 --> + <controller type='pci' index='0' model='pci-root'/> + <controller type='pci' index='2' model='pci-root'/> + <controller type='usb' model='none'/> + <memballoon model='none'/> + </devices> +</domain> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-pseries-many-devices.args b/tests/qemuxml2argvdata/qemuxml2argv-pseries-many-devices.args new file mode 100644 index 0000000..1db4533 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-pseries-many-devices.args @@ -0,0 +1,53 @@ +LC_ALL=C \ +PATH=/bin \ +HOME=/home/test \ +USER=test \ +LOGNAME=test \ +QEMU_AUDIO_DRV=none \ +/usr/bin/qemu-system-ppc64 \ +-name guest \ +-S \ +-M pseries \ +-m 512 \ +-smp 1,sockets=1,cores=1,threads=1 \ +-uuid 1ccfd97d-5eb4-478a-bbe6-88d254c16db7 \ +-nographic \ +-nodefconfig \ +-nodefaults \ +-chardev socket,id=charmonitor,path=/tmp/lib/domain--1-guest/monitor.sock,\ +server,nowait \ +-mon chardev=charmonitor,id=monitor,mode=readline \ +-boot c \ +-device pci-bridge,chassis_nr=1,id=pci.1,bus=pci.0,addr=0x1 \ +-device virtio-scsi-pci,id=scsi0,bus=pci.0,addr=0x2 \ +-device virtio-scsi-pci,id=scsi1,bus=pci.0,addr=0x3 \ +-device virtio-scsi-pci,id=scsi2,bus=pci.0,addr=0x4 \ +-device virtio-scsi-pci,id=scsi3,bus=pci.0,addr=0x5 \ +-device virtio-scsi-pci,id=scsi4,bus=pci.0,addr=0x6 \ +-device virtio-scsi-pci,id=scsi5,bus=pci.0,addr=0x7 \ +-device virtio-scsi-pci,id=scsi6,bus=pci.0,addr=0x8 \ +-device virtio-scsi-pci,id=scsi7,bus=pci.0,addr=0x9 \ +-device virtio-scsi-pci,id=scsi8,bus=pci.0,addr=0xa \ +-device virtio-scsi-pci,id=scsi9,bus=pci.0,addr=0xb \ +-device virtio-scsi-pci,id=scsi10,bus=pci.0,addr=0xc \ +-device virtio-scsi-pci,id=scsi11,bus=pci.0,addr=0xd \ +-device virtio-scsi-pci,id=scsi12,bus=pci.0,addr=0xe \ +-device virtio-scsi-pci,id=scsi13,bus=pci.0,addr=0xf \ +-device virtio-scsi-pci,id=scsi14,bus=pci.0,addr=0x10 \ +-device virtio-scsi-pci,id=scsi15,bus=pci.0,addr=0x11 \ +-device virtio-scsi-pci,id=scsi16,bus=pci.0,addr=0x12 \ +-device virtio-scsi-pci,id=scsi17,bus=pci.0,addr=0x13 \ +-device virtio-scsi-pci,id=scsi18,bus=pci.0,addr=0x14 \ +-device virtio-scsi-pci,id=scsi19,bus=pci.0,addr=0x15 \ +-device virtio-scsi-pci,id=scsi20,bus=pci.0,addr=0x16 \ +-device virtio-scsi-pci,id=scsi21,bus=pci.0,addr=0x17 \ +-device virtio-scsi-pci,id=scsi22,bus=pci.0,addr=0x18 \ +-device virtio-scsi-pci,id=scsi23,bus=pci.0,addr=0x19 \ +-device virtio-scsi-pci,id=scsi24,bus=pci.0,addr=0x1a \ +-device virtio-scsi-pci,id=scsi25,bus=pci.0,addr=0x1b \ +-device virtio-scsi-pci,id=scsi26,bus=pci.0,addr=0x1c \ +-device virtio-scsi-pci,id=scsi27,bus=pci.0,addr=0x1d \ +-device virtio-scsi-pci,id=scsi28,bus=pci.0,addr=0x1e \ +-device virtio-scsi-pci,id=scsi29,bus=pci.0,addr=0x1f \ +-device virtio-scsi-pci,id=scsi30,bus=pci.1,addr=0x1 \ +-device virtio-scsi-pci,id=scsi31,bus=pci.1,addr=0x2 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-pseries-many-devices.xml b/tests/qemuxml2argvdata/qemuxml2argv-pseries-many-devices.xml new file mode 100644 index 0000000..9a3ead3 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-pseries-many-devices.xml @@ -0,0 +1,49 @@ +<domain type='qemu'> + <name>guest</name> + <uuid>1ccfd97d-5eb4-478a-bbe6-88d254c16db7</uuid> + <memory unit='KiB'>524288</memory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='ppc64' machine='pseries'>hvm</type> + </os> + <devices> + <emulator>/usr/bin/qemu-system-ppc64</emulator> + <controller type='pci' model='pci-root'/> + <!-- pci-root has 31 slots and there are 32 SCSI controllers here, so + libvirt will automatically add at least one more PCI controller --> + <controller type='scsi' model='virtio-scsi'/> + <controller type='scsi' model='virtio-scsi'/> + <controller type='scsi' model='virtio-scsi'/> + <controller type='scsi' model='virtio-scsi'/> + <controller type='scsi' model='virtio-scsi'/> + <controller type='scsi' model='virtio-scsi'/> + <controller type='scsi' model='virtio-scsi'/> + <controller type='scsi' model='virtio-scsi'/> + <controller type='scsi' model='virtio-scsi'/> + <controller type='scsi' model='virtio-scsi'/> + <controller type='scsi' model='virtio-scsi'/> + <controller type='scsi' model='virtio-scsi'/> + <controller type='scsi' model='virtio-scsi'/> + <controller type='scsi' model='virtio-scsi'/> + <controller type='scsi' model='virtio-scsi'/> + <controller type='scsi' model='virtio-scsi'/> + <controller type='scsi' model='virtio-scsi'/> + <controller type='scsi' model='virtio-scsi'/> + <controller type='scsi' model='virtio-scsi'/> + <controller type='scsi' model='virtio-scsi'/> + <controller type='scsi' model='virtio-scsi'/> + <controller type='scsi' model='virtio-scsi'/> + <controller type='scsi' model='virtio-scsi'/> + <controller type='scsi' model='virtio-scsi'/> + <controller type='scsi' model='virtio-scsi'/> + <controller type='scsi' model='virtio-scsi'/> + <controller type='scsi' model='virtio-scsi'/> + <controller type='scsi' model='virtio-scsi'/> + <controller type='scsi' model='virtio-scsi'/> + <controller type='scsi' model='virtio-scsi'/> + <controller type='scsi' model='virtio-scsi'/> + <controller type='scsi' model='virtio-scsi'/> + <controller type='usb' model='none'/> + <memballoon model='none'/> + </devices> +</domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 9996960..4e46825 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -1703,6 +1703,27 @@ mymain(void) QEMU_CAPS_NODEFCONFIG, QEMU_CAPS_CHARDEV, QEMU_CAPS_DEVICE_SPAPR_PCI_HOST_BRIDGE); + DO_TEST("pseries-many-devices", + QEMU_CAPS_CHARDEV, QEMU_CAPS_NODEFCONFIG, + QEMU_CAPS_DEVICE_SPAPR_PCI_HOST_BRIDGE, + QEMU_CAPS_DEVICE_PCI_BRIDGE, + QEMU_CAPS_VIRTIO_SCSI); + DO_TEST("pseries-many-buses-1", + QEMU_CAPS_CHARDEV, QEMU_CAPS_NODEFCONFIG, + QEMU_CAPS_DEVICE_SPAPR_PCI_HOST_BRIDGE, + QEMU_CAPS_DEVICE_PCI_BRIDGE, + QEMU_CAPS_VIRTIO_SCSI); + DO_TEST("pseries-many-buses-2", + QEMU_CAPS_CHARDEV, QEMU_CAPS_NODEFCONFIG, + QEMU_CAPS_DEVICE_SPAPR_PCI_HOST_BRIDGE, + QEMU_CAPS_DEVICE_PCI_BRIDGE, + QEMU_CAPS_VIRTIO_SCSI); + DO_TEST("pseries-hostdev", + QEMU_CAPS_CHARDEV, QEMU_CAPS_NODEFCONFIG, + QEMU_CAPS_DEVICE_SPAPR_PCI_HOST_BRIDGE, + QEMU_CAPS_HOST_PCI_MULTIDOMAIN, + QEMU_CAPS_DEVICE_VFIO_PCI); + DO_TEST("disk-ide-drive-split", QEMU_CAPS_NODEFCONFIG, QEMU_CAPS_IDE_CD); diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-pseries-hostdev.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-pseries-hostdev.xml new file mode 100644 index 0000000..a00c09c --- /dev/null +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-pseries-hostdev.xml @@ -0,0 +1,46 @@ +<domain type='qemu'> + <name>guest</name> + <uuid>1ccfd97d-5eb4-478a-bbe6-88d254c16db7</uuid> + <memory unit='KiB'>524288</memory> + <currentMemory unit='KiB'>524288</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='ppc64' machine='pseries'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu-system-ppc64</emulator> + <controller type='pci' index='0' model='pci-root'> + <model name='spapr-pci-host-bridge'/> + <target index='0'/> + </controller> + <controller type='usb' index='0' model='none'/> + <hostdev mode='subsystem' type='pci' managed='yes'> + <driver name='vfio'/> + <source> + <address domain='0x0001' bus='0x01' slot='0x00' function='0x0'/> + </source> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x0'/> + </hostdev> + <hostdev mode='subsystem' type='pci' managed='yes'> + <driver name='vfio'/> + <source> + <address domain='0x0005' bus='0x90' slot='0x01' function='0x2'/> + </source> + <address type='pci' domain='0x0000' bus='0x00' slot='0x02' function='0x0'/> + </hostdev> + <hostdev mode='subsystem' type='pci' managed='yes'> + <driver name='vfio'/> + <source> + <address domain='0x0001' bus='0x01' slot='0x00' function='0x1'/> + </source> + <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/> + </hostdev> + <memballoon model='none'/> + <panic model='pseries'/> + </devices> +</domain> diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-pseries-many-buses-1.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-pseries-many-buses-1.xml new file mode 100644 index 0000000..010a3be --- /dev/null +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-pseries-many-buses-1.xml @@ -0,0 +1,33 @@ +<domain type='qemu'> + <name>guest</name> + <uuid>1ccfd97d-5eb4-478a-bbe6-88d254c16db7</uuid> + <memory unit='KiB'>524288</memory> + <currentMemory unit='KiB'>524288</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='ppc64' machine='pseries'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu-system-ppc64</emulator> + <controller type='pci' index='0' model='pci-root'> + <model name='spapr-pci-host-bridge'/> + <target index='0'/> + </controller> + <controller type='scsi' index='0' model='virtio-scsi'> + <address type='pci' domain='0x0000' bus='0x01' slot='0x01' function='0x0'/> + </controller> + <controller type='usb' index='0' model='none'/> + <controller type='pci' index='1' model='pci-bridge'> + <model name='pci-bridge'/> + <target chassisNr='1'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x0'/> + </controller> + <memballoon model='none'/> + <panic model='pseries'/> + </devices> +</domain> diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-pseries-many-buses-2.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-pseries-many-buses-2.xml new file mode 100644 index 0000000..75dfabf --- /dev/null +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-pseries-many-buses-2.xml @@ -0,0 +1,34 @@ +<domain type='qemu'> + <name>guest</name> + <uuid>1ccfd97d-5eb4-478a-bbe6-88d254c16db7</uuid> + <memory unit='KiB'>524288</memory> + <currentMemory unit='KiB'>524288</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='ppc64' machine='pseries'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu-system-ppc64</emulator> + <controller type='pci' index='0' model='pci-root'> + <model name='spapr-pci-host-bridge'/> + <target index='0'/> + </controller> + <controller type='pci' index='2' model='pci-root'> + <model name='spapr-pci-host-bridge'/> + <target index='1'/> + </controller> + <controller type='usb' index='0' model='none'/> + <controller type='pci' index='1' model='pci-bridge'> + <model name='pci-bridge'/> + <target chassisNr='1'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x0'/> + </controller> + <memballoon model='none'/> + <panic model='pseries'/> + </devices> +</domain> diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-pseries-many-devices.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-pseries-many-devices.xml new file mode 100644 index 0000000..336373b --- /dev/null +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-pseries-many-devices.xml @@ -0,0 +1,126 @@ +<domain type='qemu'> + <name>guest</name> + <uuid>1ccfd97d-5eb4-478a-bbe6-88d254c16db7</uuid> + <memory unit='KiB'>524288</memory> + <currentMemory unit='KiB'>524288</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='ppc64' machine='pseries'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu-system-ppc64</emulator> + <controller type='pci' index='0' model='pci-root'> + <model name='spapr-pci-host-bridge'/> + <target index='0'/> + </controller> + <controller type='scsi' index='0' model='virtio-scsi'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x02' function='0x0'/> + </controller> + <controller type='scsi' index='1' model='virtio-scsi'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/> + </controller> + <controller type='scsi' index='2' model='virtio-scsi'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0x0'/> + </controller> + <controller type='scsi' index='3' model='virtio-scsi'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x05' function='0x0'/> + </controller> + <controller type='scsi' index='4' model='virtio-scsi'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x06' function='0x0'/> + </controller> + <controller type='scsi' index='5' model='virtio-scsi'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x07' function='0x0'/> + </controller> + <controller type='scsi' index='6' model='virtio-scsi'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x08' function='0x0'/> + </controller> + <controller type='scsi' index='7' model='virtio-scsi'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x09' function='0x0'/> + </controller> + <controller type='scsi' index='8' model='virtio-scsi'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x0a' function='0x0'/> + </controller> + <controller type='scsi' index='9' model='virtio-scsi'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x0b' function='0x0'/> + </controller> + <controller type='scsi' index='10' model='virtio-scsi'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x0c' function='0x0'/> + </controller> + <controller type='scsi' index='11' model='virtio-scsi'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x0d' function='0x0'/> + </controller> + <controller type='scsi' index='12' model='virtio-scsi'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x0e' function='0x0'/> + </controller> + <controller type='scsi' index='13' model='virtio-scsi'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x0f' function='0x0'/> + </controller> + <controller type='scsi' index='14' model='virtio-scsi'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x10' function='0x0'/> + </controller> + <controller type='scsi' index='15' model='virtio-scsi'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x11' function='0x0'/> + </controller> + <controller type='scsi' index='16' model='virtio-scsi'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x12' function='0x0'/> + </controller> + <controller type='scsi' index='17' model='virtio-scsi'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x13' function='0x0'/> + </controller> + <controller type='scsi' index='18' model='virtio-scsi'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x14' function='0x0'/> + </controller> + <controller type='scsi' index='19' model='virtio-scsi'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x15' function='0x0'/> + </controller> + <controller type='scsi' index='20' model='virtio-scsi'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x16' function='0x0'/> + </controller> + <controller type='scsi' index='21' model='virtio-scsi'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x17' function='0x0'/> + </controller> + <controller type='scsi' index='22' model='virtio-scsi'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x18' function='0x0'/> + </controller> + <controller type='scsi' index='23' model='virtio-scsi'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x19' function='0x0'/> + </controller> + <controller type='scsi' index='24' model='virtio-scsi'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x1a' function='0x0'/> + </controller> + <controller type='scsi' index='25' model='virtio-scsi'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x1b' function='0x0'/> + </controller> + <controller type='scsi' index='26' model='virtio-scsi'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x1c' function='0x0'/> + </controller> + <controller type='scsi' index='27' model='virtio-scsi'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x1d' function='0x0'/> + </controller> + <controller type='scsi' index='28' model='virtio-scsi'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x1e' function='0x0'/> + </controller> + <controller type='scsi' index='29' model='virtio-scsi'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x1f' function='0x0'/> + </controller> + <controller type='scsi' index='30' model='virtio-scsi'> + <address type='pci' domain='0x0000' bus='0x01' slot='0x01' function='0x0'/> + </controller> + <controller type='scsi' index='31' model='virtio-scsi'> + <address type='pci' domain='0x0000' bus='0x01' slot='0x02' function='0x0'/> + </controller> + <controller type='usb' index='0' model='none'/> + <controller type='pci' index='1' model='pci-bridge'> + <model name='pci-bridge'/> + <target chassisNr='1'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x0'/> + </controller> + <memballoon model='none'/> + <panic model='pseries'/> + </devices> +</domain> diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index 549543a..8cbada1 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -677,6 +677,27 @@ mymain(void) QEMU_CAPS_NODEFCONFIG, QEMU_CAPS_CHARDEV, QEMU_CAPS_DEVICE_SPAPR_PCI_HOST_BRIDGE); + DO_TEST("pseries-many-devices", + QEMU_CAPS_CHARDEV, QEMU_CAPS_NODEFCONFIG, + QEMU_CAPS_DEVICE_SPAPR_PCI_HOST_BRIDGE, + QEMU_CAPS_DEVICE_PCI_BRIDGE, + QEMU_CAPS_VIRTIO_SCSI); + DO_TEST("pseries-many-buses-1", + QEMU_CAPS_CHARDEV, QEMU_CAPS_NODEFCONFIG, + QEMU_CAPS_DEVICE_SPAPR_PCI_HOST_BRIDGE, + QEMU_CAPS_DEVICE_PCI_BRIDGE, + QEMU_CAPS_VIRTIO_SCSI); + DO_TEST("pseries-many-buses-2", + QEMU_CAPS_CHARDEV, QEMU_CAPS_NODEFCONFIG, + QEMU_CAPS_DEVICE_SPAPR_PCI_HOST_BRIDGE, + QEMU_CAPS_DEVICE_PCI_BRIDGE, + QEMU_CAPS_VIRTIO_SCSI); + DO_TEST("pseries-hostdev", + QEMU_CAPS_CHARDEV, QEMU_CAPS_NODEFCONFIG, + QEMU_CAPS_DEVICE_SPAPR_PCI_HOST_BRIDGE, + QEMU_CAPS_HOST_PCI_MULTIDOMAIN, + QEMU_CAPS_DEVICE_VFIO_PCI); + DO_TEST("balloon-device-auto", NONE); DO_TEST("balloon-device-period", NONE); DO_TEST("channel-virtio-auto", NONE); -- 2.7.5

When looking for slots suitable for a PCI device, libvirt might need to add an extra PCI controller: for pSeries guests, we want that extra controller to be a PHB (pci-root) rather than a PCI bridge. --- Better viewed with 'git diff -w'. src/conf/domain_addr.c | 56 +++++++++-------- src/conf/domain_addr.h | 2 + src/qemu/qemu_domain_address.c | 4 ++ .../qemuxml2argv-pseries-many-buses-1.args | 4 +- .../qemuxml2argv-pseries-many-devices.args | 66 ++++++++++---------- tests/qemuxml2argvtest.c | 2 - .../qemuxml2xmlout-pseries-many-buses-1.xml | 7 +-- .../qemuxml2xmlout-pseries-many-devices.xml | 71 +++++++++++----------- tests/qemuxml2xmltest.c | 2 - 9 files changed, 110 insertions(+), 104 deletions(-) diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c index d173766..e6a6399 100644 --- a/src/conf/domain_addr.c +++ b/src/conf/domain_addr.c @@ -383,33 +383,39 @@ virDomainPCIAddressSetGrow(virDomainPCIAddressSetPtr addrs, */ if (flags & VIR_PCI_CONNECT_TYPE_PCI_DEVICE) { - model = VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE; + if (addrs->multipleRootsSupported) { + /* Use a pci-root controller to expand the guest's PCI + * topology if it supports having more than one */ + model = VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT; + } else { + model = VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE; - /* if there aren't yet any buses that will accept a - * pci-bridge, and the caller is asking for one, we'll need to - * add a dmi-to-pci-bridge first. - */ - needDMIToPCIBridge = true; - for (i = 0; i < addrs->nbuses; i++) { - if (addrs->buses[i].flags & VIR_PCI_CONNECT_TYPE_PCI_BRIDGE) { - needDMIToPCIBridge = false; - break; - } - } - if (needDMIToPCIBridge && add == 1) { - /* We need to add a single pci-bridge to provide the bus - * our legacy PCI device will be plugged into; however, we - * have also determined that there isn't yet any proper - * place to connect that pci-bridge we're about to add (on - * a system with pcie-root, that "proper place" would be a - * dmi-to-pci-bridge". So, to give the pci-bridge a place - * to connect, we increase the count of buses to add, - * while also incrementing the bus number in the address - * for the device (since the pci-bridge will now be at an - * index 1 higher than the caller had anticipated). + /* if there aren't yet any buses that will accept a + * pci-bridge, and the caller is asking for one, we'll need to + * add a dmi-to-pci-bridge first. */ - add++; - addr->bus++; + needDMIToPCIBridge = true; + for (i = 0; i < addrs->nbuses; i++) { + if (addrs->buses[i].flags & VIR_PCI_CONNECT_TYPE_PCI_BRIDGE) { + needDMIToPCIBridge = false; + break; + } + } + if (needDMIToPCIBridge && add == 1) { + /* We need to add a single pci-bridge to provide the bus + * our legacy PCI device will be plugged into; however, we + * have also determined that there isn't yet any proper + * place to connect that pci-bridge we're about to add (on + * a system with pcie-root, that "proper place" would be a + * dmi-to-pci-bridge". So, to give the pci-bridge a place + * to connect, we increase the count of buses to add, + * while also incrementing the bus number in the address + * for the device (since the pci-bridge will now be at an + * index 1 higher than the caller had anticipated). + */ + add++; + addr->bus++; + } } } else if (flags & VIR_PCI_CONNECT_TYPE_PCI_BRIDGE && addrs->buses[0].model == VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT) { diff --git a/src/conf/domain_addr.h b/src/conf/domain_addr.h index 7704061..1849d2b 100644 --- a/src/conf/domain_addr.h +++ b/src/conf/domain_addr.h @@ -108,6 +108,8 @@ struct _virDomainPCIAddressSet { size_t nbuses; bool dryRun; /* on a dry run, new buses are auto-added and addresses aren't saved in device infos */ + /* If true, the guest can have multiple pci-root controllers */ + bool multipleRootsSupported; }; typedef struct _virDomainPCIAddressSet virDomainPCIAddressSet; typedef virDomainPCIAddressSet *virDomainPCIAddressSetPtr; diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c index 9585fe5..08773c0 100644 --- a/src/qemu/qemu_domain_address.c +++ b/src/qemu/qemu_domain_address.c @@ -1056,6 +1056,10 @@ qemuDomainPCIAddressSetCreate(virDomainDefPtr def, addrs->dryRun = dryRun; + /* pSeries domains support multiple pci-root controllers */ + if (qemuDomainIsPSeries(def)) + addrs->multipleRootsSupported = true; + for (i = 0; i < def->ncontrollers; i++) { virDomainControllerDefPtr cont = def->controllers[i]; size_t idx = cont->idx; diff --git a/tests/qemuxml2argvdata/qemuxml2argv-pseries-many-buses-1.args b/tests/qemuxml2argvdata/qemuxml2argv-pseries-many-buses-1.args index bf78fc1..eb5ccbd 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-pseries-many-buses-1.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-pseries-many-buses-1.args @@ -18,5 +18,5 @@ QEMU_AUDIO_DRV=none \ server,nowait \ -mon chardev=charmonitor,id=monitor,mode=readline \ -boot c \ --device pci-bridge,chassis_nr=1,id=pci.1,bus=pci.0,addr=0x1 \ --device virtio-scsi-pci,id=scsi0,bus=pci.1,addr=0x1 +-device spapr-pci-host-bridge,index=1,id=pci.1 \ +-device virtio-scsi-pci,id=scsi0,bus=pci.1.0,addr=0x1 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-pseries-many-devices.args b/tests/qemuxml2argvdata/qemuxml2argv-pseries-many-devices.args index 1db4533..f20bc52 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-pseries-many-devices.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-pseries-many-devices.args @@ -18,36 +18,36 @@ QEMU_AUDIO_DRV=none \ server,nowait \ -mon chardev=charmonitor,id=monitor,mode=readline \ -boot c \ --device pci-bridge,chassis_nr=1,id=pci.1,bus=pci.0,addr=0x1 \ --device virtio-scsi-pci,id=scsi0,bus=pci.0,addr=0x2 \ --device virtio-scsi-pci,id=scsi1,bus=pci.0,addr=0x3 \ --device virtio-scsi-pci,id=scsi2,bus=pci.0,addr=0x4 \ --device virtio-scsi-pci,id=scsi3,bus=pci.0,addr=0x5 \ --device virtio-scsi-pci,id=scsi4,bus=pci.0,addr=0x6 \ --device virtio-scsi-pci,id=scsi5,bus=pci.0,addr=0x7 \ --device virtio-scsi-pci,id=scsi6,bus=pci.0,addr=0x8 \ --device virtio-scsi-pci,id=scsi7,bus=pci.0,addr=0x9 \ --device virtio-scsi-pci,id=scsi8,bus=pci.0,addr=0xa \ --device virtio-scsi-pci,id=scsi9,bus=pci.0,addr=0xb \ --device virtio-scsi-pci,id=scsi10,bus=pci.0,addr=0xc \ --device virtio-scsi-pci,id=scsi11,bus=pci.0,addr=0xd \ --device virtio-scsi-pci,id=scsi12,bus=pci.0,addr=0xe \ --device virtio-scsi-pci,id=scsi13,bus=pci.0,addr=0xf \ --device virtio-scsi-pci,id=scsi14,bus=pci.0,addr=0x10 \ --device virtio-scsi-pci,id=scsi15,bus=pci.0,addr=0x11 \ --device virtio-scsi-pci,id=scsi16,bus=pci.0,addr=0x12 \ --device virtio-scsi-pci,id=scsi17,bus=pci.0,addr=0x13 \ --device virtio-scsi-pci,id=scsi18,bus=pci.0,addr=0x14 \ --device virtio-scsi-pci,id=scsi19,bus=pci.0,addr=0x15 \ --device virtio-scsi-pci,id=scsi20,bus=pci.0,addr=0x16 \ --device virtio-scsi-pci,id=scsi21,bus=pci.0,addr=0x17 \ --device virtio-scsi-pci,id=scsi22,bus=pci.0,addr=0x18 \ --device virtio-scsi-pci,id=scsi23,bus=pci.0,addr=0x19 \ --device virtio-scsi-pci,id=scsi24,bus=pci.0,addr=0x1a \ --device virtio-scsi-pci,id=scsi25,bus=pci.0,addr=0x1b \ --device virtio-scsi-pci,id=scsi26,bus=pci.0,addr=0x1c \ --device virtio-scsi-pci,id=scsi27,bus=pci.0,addr=0x1d \ --device virtio-scsi-pci,id=scsi28,bus=pci.0,addr=0x1e \ --device virtio-scsi-pci,id=scsi29,bus=pci.0,addr=0x1f \ --device virtio-scsi-pci,id=scsi30,bus=pci.1,addr=0x1 \ --device virtio-scsi-pci,id=scsi31,bus=pci.1,addr=0x2 +-device spapr-pci-host-bridge,index=1,id=pci.1 \ +-device virtio-scsi-pci,id=scsi0,bus=pci.0,addr=0x1 \ +-device virtio-scsi-pci,id=scsi1,bus=pci.0,addr=0x2 \ +-device virtio-scsi-pci,id=scsi2,bus=pci.0,addr=0x3 \ +-device virtio-scsi-pci,id=scsi3,bus=pci.0,addr=0x4 \ +-device virtio-scsi-pci,id=scsi4,bus=pci.0,addr=0x5 \ +-device virtio-scsi-pci,id=scsi5,bus=pci.0,addr=0x6 \ +-device virtio-scsi-pci,id=scsi6,bus=pci.0,addr=0x7 \ +-device virtio-scsi-pci,id=scsi7,bus=pci.0,addr=0x8 \ +-device virtio-scsi-pci,id=scsi8,bus=pci.0,addr=0x9 \ +-device virtio-scsi-pci,id=scsi9,bus=pci.0,addr=0xa \ +-device virtio-scsi-pci,id=scsi10,bus=pci.0,addr=0xb \ +-device virtio-scsi-pci,id=scsi11,bus=pci.0,addr=0xc \ +-device virtio-scsi-pci,id=scsi12,bus=pci.0,addr=0xd \ +-device virtio-scsi-pci,id=scsi13,bus=pci.0,addr=0xe \ +-device virtio-scsi-pci,id=scsi14,bus=pci.0,addr=0xf \ +-device virtio-scsi-pci,id=scsi15,bus=pci.0,addr=0x10 \ +-device virtio-scsi-pci,id=scsi16,bus=pci.0,addr=0x11 \ +-device virtio-scsi-pci,id=scsi17,bus=pci.0,addr=0x12 \ +-device virtio-scsi-pci,id=scsi18,bus=pci.0,addr=0x13 \ +-device virtio-scsi-pci,id=scsi19,bus=pci.0,addr=0x14 \ +-device virtio-scsi-pci,id=scsi20,bus=pci.0,addr=0x15 \ +-device virtio-scsi-pci,id=scsi21,bus=pci.0,addr=0x16 \ +-device virtio-scsi-pci,id=scsi22,bus=pci.0,addr=0x17 \ +-device virtio-scsi-pci,id=scsi23,bus=pci.0,addr=0x18 \ +-device virtio-scsi-pci,id=scsi24,bus=pci.0,addr=0x19 \ +-device virtio-scsi-pci,id=scsi25,bus=pci.0,addr=0x1a \ +-device virtio-scsi-pci,id=scsi26,bus=pci.0,addr=0x1b \ +-device virtio-scsi-pci,id=scsi27,bus=pci.0,addr=0x1c \ +-device virtio-scsi-pci,id=scsi28,bus=pci.0,addr=0x1d \ +-device virtio-scsi-pci,id=scsi29,bus=pci.0,addr=0x1e \ +-device virtio-scsi-pci,id=scsi30,bus=pci.0,addr=0x1f \ +-device virtio-scsi-pci,id=scsi31,bus=pci.1.0,addr=0x1 diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 7a6acd5..296c93b 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -1706,12 +1706,10 @@ mymain(void) DO_TEST("pseries-many-devices", QEMU_CAPS_CHARDEV, QEMU_CAPS_NODEFCONFIG, QEMU_CAPS_DEVICE_SPAPR_PCI_HOST_BRIDGE, - QEMU_CAPS_DEVICE_PCI_BRIDGE, QEMU_CAPS_VIRTIO_SCSI); DO_TEST("pseries-many-buses-1", QEMU_CAPS_CHARDEV, QEMU_CAPS_NODEFCONFIG, QEMU_CAPS_DEVICE_SPAPR_PCI_HOST_BRIDGE, - QEMU_CAPS_DEVICE_PCI_BRIDGE, QEMU_CAPS_VIRTIO_SCSI); DO_TEST("pseries-many-buses-2", QEMU_CAPS_CHARDEV, QEMU_CAPS_NODEFCONFIG, diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-pseries-many-buses-1.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-pseries-many-buses-1.xml index 010a3be..1b2845d 100644 --- a/tests/qemuxml2xmloutdata/qemuxml2xmlout-pseries-many-buses-1.xml +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-pseries-many-buses-1.xml @@ -22,10 +22,9 @@ <address type='pci' domain='0x0000' bus='0x01' slot='0x01' function='0x0'/> </controller> <controller type='usb' index='0' model='none'/> - <controller type='pci' index='1' model='pci-bridge'> - <model name='pci-bridge'/> - <target chassisNr='1'/> - <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x0'/> + <controller type='pci' index='1' model='pci-root'> + <model name='spapr-pci-host-bridge'/> + <target index='1'/> </controller> <memballoon model='none'/> <panic model='pseries'/> diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-pseries-many-devices.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-pseries-many-devices.xml index 336373b..5a78590 100644 --- a/tests/qemuxml2xmloutdata/qemuxml2xmlout-pseries-many-devices.xml +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-pseries-many-devices.xml @@ -19,106 +19,105 @@ <target index='0'/> </controller> <controller type='scsi' index='0' model='virtio-scsi'> - <address type='pci' domain='0x0000' bus='0x00' slot='0x02' function='0x0'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x0'/> </controller> <controller type='scsi' index='1' model='virtio-scsi'> - <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x02' function='0x0'/> </controller> <controller type='scsi' index='2' model='virtio-scsi'> - <address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0x0'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/> </controller> <controller type='scsi' index='3' model='virtio-scsi'> - <address type='pci' domain='0x0000' bus='0x00' slot='0x05' function='0x0'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0x0'/> </controller> <controller type='scsi' index='4' model='virtio-scsi'> - <address type='pci' domain='0x0000' bus='0x00' slot='0x06' function='0x0'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x05' function='0x0'/> </controller> <controller type='scsi' index='5' model='virtio-scsi'> - <address type='pci' domain='0x0000' bus='0x00' slot='0x07' function='0x0'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x06' function='0x0'/> </controller> <controller type='scsi' index='6' model='virtio-scsi'> - <address type='pci' domain='0x0000' bus='0x00' slot='0x08' function='0x0'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x07' function='0x0'/> </controller> <controller type='scsi' index='7' model='virtio-scsi'> - <address type='pci' domain='0x0000' bus='0x00' slot='0x09' function='0x0'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x08' function='0x0'/> </controller> <controller type='scsi' index='8' model='virtio-scsi'> - <address type='pci' domain='0x0000' bus='0x00' slot='0x0a' function='0x0'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x09' function='0x0'/> </controller> <controller type='scsi' index='9' model='virtio-scsi'> - <address type='pci' domain='0x0000' bus='0x00' slot='0x0b' function='0x0'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x0a' function='0x0'/> </controller> <controller type='scsi' index='10' model='virtio-scsi'> - <address type='pci' domain='0x0000' bus='0x00' slot='0x0c' function='0x0'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x0b' function='0x0'/> </controller> <controller type='scsi' index='11' model='virtio-scsi'> - <address type='pci' domain='0x0000' bus='0x00' slot='0x0d' function='0x0'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x0c' function='0x0'/> </controller> <controller type='scsi' index='12' model='virtio-scsi'> - <address type='pci' domain='0x0000' bus='0x00' slot='0x0e' function='0x0'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x0d' function='0x0'/> </controller> <controller type='scsi' index='13' model='virtio-scsi'> - <address type='pci' domain='0x0000' bus='0x00' slot='0x0f' function='0x0'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x0e' function='0x0'/> </controller> <controller type='scsi' index='14' model='virtio-scsi'> - <address type='pci' domain='0x0000' bus='0x00' slot='0x10' function='0x0'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x0f' function='0x0'/> </controller> <controller type='scsi' index='15' model='virtio-scsi'> - <address type='pci' domain='0x0000' bus='0x00' slot='0x11' function='0x0'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x10' function='0x0'/> </controller> <controller type='scsi' index='16' model='virtio-scsi'> - <address type='pci' domain='0x0000' bus='0x00' slot='0x12' function='0x0'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x11' function='0x0'/> </controller> <controller type='scsi' index='17' model='virtio-scsi'> - <address type='pci' domain='0x0000' bus='0x00' slot='0x13' function='0x0'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x12' function='0x0'/> </controller> <controller type='scsi' index='18' model='virtio-scsi'> - <address type='pci' domain='0x0000' bus='0x00' slot='0x14' function='0x0'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x13' function='0x0'/> </controller> <controller type='scsi' index='19' model='virtio-scsi'> - <address type='pci' domain='0x0000' bus='0x00' slot='0x15' function='0x0'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x14' function='0x0'/> </controller> <controller type='scsi' index='20' model='virtio-scsi'> - <address type='pci' domain='0x0000' bus='0x00' slot='0x16' function='0x0'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x15' function='0x0'/> </controller> <controller type='scsi' index='21' model='virtio-scsi'> - <address type='pci' domain='0x0000' bus='0x00' slot='0x17' function='0x0'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x16' function='0x0'/> </controller> <controller type='scsi' index='22' model='virtio-scsi'> - <address type='pci' domain='0x0000' bus='0x00' slot='0x18' function='0x0'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x17' function='0x0'/> </controller> <controller type='scsi' index='23' model='virtio-scsi'> - <address type='pci' domain='0x0000' bus='0x00' slot='0x19' function='0x0'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x18' function='0x0'/> </controller> <controller type='scsi' index='24' model='virtio-scsi'> - <address type='pci' domain='0x0000' bus='0x00' slot='0x1a' function='0x0'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x19' function='0x0'/> </controller> <controller type='scsi' index='25' model='virtio-scsi'> - <address type='pci' domain='0x0000' bus='0x00' slot='0x1b' function='0x0'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x1a' function='0x0'/> </controller> <controller type='scsi' index='26' model='virtio-scsi'> - <address type='pci' domain='0x0000' bus='0x00' slot='0x1c' function='0x0'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x1b' function='0x0'/> </controller> <controller type='scsi' index='27' model='virtio-scsi'> - <address type='pci' domain='0x0000' bus='0x00' slot='0x1d' function='0x0'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x1c' function='0x0'/> </controller> <controller type='scsi' index='28' model='virtio-scsi'> - <address type='pci' domain='0x0000' bus='0x00' slot='0x1e' function='0x0'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x1d' function='0x0'/> </controller> <controller type='scsi' index='29' model='virtio-scsi'> - <address type='pci' domain='0x0000' bus='0x00' slot='0x1f' function='0x0'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x1e' function='0x0'/> </controller> <controller type='scsi' index='30' model='virtio-scsi'> - <address type='pci' domain='0x0000' bus='0x01' slot='0x01' function='0x0'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x1f' function='0x0'/> </controller> <controller type='scsi' index='31' model='virtio-scsi'> - <address type='pci' domain='0x0000' bus='0x01' slot='0x02' function='0x0'/> + <address type='pci' domain='0x0000' bus='0x01' slot='0x01' function='0x0'/> </controller> <controller type='usb' index='0' model='none'/> - <controller type='pci' index='1' model='pci-bridge'> - <model name='pci-bridge'/> - <target chassisNr='1'/> - <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x0'/> + <controller type='pci' index='1' model='pci-root'> + <model name='spapr-pci-host-bridge'/> + <target index='1'/> </controller> <memballoon model='none'/> <panic model='pseries'/> diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index b75807c..15ac744 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -680,12 +680,10 @@ mymain(void) DO_TEST("pseries-many-devices", QEMU_CAPS_CHARDEV, QEMU_CAPS_NODEFCONFIG, QEMU_CAPS_DEVICE_SPAPR_PCI_HOST_BRIDGE, - QEMU_CAPS_DEVICE_PCI_BRIDGE, QEMU_CAPS_VIRTIO_SCSI); DO_TEST("pseries-many-buses-1", QEMU_CAPS_CHARDEV, QEMU_CAPS_NODEFCONFIG, QEMU_CAPS_DEVICE_SPAPR_PCI_HOST_BRIDGE, - QEMU_CAPS_DEVICE_PCI_BRIDGE, QEMU_CAPS_VIRTIO_SCSI); DO_TEST("pseries-many-buses-2", QEMU_CAPS_CHARDEV, QEMU_CAPS_NODEFCONFIG, -- 2.7.5

--- docs/schemas/domaincommon.rng | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index d565a0b..7328fd3 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -2009,6 +2009,11 @@ </optional> </element> </optional> + <optional> + <element name='isolationGroup'> + <ref name='uint8'/> + </element> + </optional> <!-- *-root controllers have an optional element "pcihole64"--> <choice> <group> -- 2.7.5

All the pieces are now in place, so we can finally start using isolation groups to achieve our initial goal, which is separating hostdevs from emulated PCI devices while keeping hostdevs that belong to the same host IOMMU group together. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1280542 --- src/qemu/qemu_domain_address.c | 79 ++++++++++++++++++++++ .../qemuxml2argv-pseries-hostdev.args | 8 ++- .../qemuxml2xmlout-pseries-hostdev.xml | 16 ++++- 3 files changed, 97 insertions(+), 6 deletions(-) diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c index 384d6fd..877ff90 100644 --- a/src/qemu/qemu_domain_address.c +++ b/src/qemu/qemu_domain_address.c @@ -901,6 +901,82 @@ qemuDomainFillAllPCIConnectFlags(virDomainDefPtr def, /** + * qemuDomainFillDeviceIsolationGroupIter: + * @def: domain definition + * @dev: device definition + * @info: device information + * @opaque: user data + * + * Fill isolation group information for a single device. + * + * You're not meant to call this directly, use + * qemuDomainFillAllIsolationGroups() instead. + * + * Return: 0 on success, <0 on failure + * */ +static int +qemuDomainFillDeviceIsolationGroupIter(virDomainDefPtr def, + virDomainDeviceDefPtr dev, + virDomainDeviceInfoPtr info, + void *opaque ATTRIBUTE_UNUSED) +{ + virDomainHostdevDefPtr hostdev; + virPCIDeviceAddressPtr hostAddr; + + /* Only pSeries guests care about isolation groups at the moment */ + if (!qemuDomainIsPSeries(def)) + return 0; + + /* Only hostdev... */ + if (dev->type != VIR_DOMAIN_DEVICE_HOSTDEV) + return 0; + + hostdev = dev->data.hostdev; + + /* ... of the PCI kind need this extra information */ + if (hostdev->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS || + hostdev->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI) { + return 0; + } + + hostAddr = &hostdev->source.subsys.u.pci.addr; + + /* The isolation group is simply the IOMMU group assigned by the host */ + info->isolationGroup = virPCIDeviceAddressGetIOMMUGroupNum(hostAddr); + + if (info->isolationGroup < 0) { + VIR_WARN("Can't look up isolation group for device %04x:%02x:%02x.%x", + hostAddr->domain, hostAddr->bus, + hostAddr->slot, hostAddr->function); + } else { + VIR_DEBUG("Isolation group for device %04x:%02x:%02x.%x is %d", + hostAddr->domain, hostAddr->bus, + hostAddr->slot, hostAddr->function, + info->isolationGroup); + } + + return info->isolationGroup; +} + + +/** + * qemuDomainFillAllIsolationGroups: + * @def: domain definition + * + * Fill isolation group information for all devices in @def. + * + * Return: 0 on success, <0 on failure + */ +static int +qemuDomainFillAllIsolationGroups(virDomainDefPtr def) +{ + return virDomainDeviceInfoIterate(def, + qemuDomainFillDeviceIsolationGroupIter, + NULL); +} + + +/** * qemuDomainFillDevicePCIConnectFlags: * * @def: the entire DomainDef @@ -2036,6 +2112,9 @@ qemuDomainAssignPCIAddresses(virDomainDefPtr def, if (qemuDomainFillAllPCIConnectFlags(def, qemuCaps, driver) < 0) goto cleanup; + if (qemuDomainFillAllIsolationGroups(def) < 0) + goto cleanup; + if (nbuses > 0) { /* 1st pass to figure out how many PCI bridges we need */ if (!(addrs = qemuDomainPCIAddressSetCreate(def, nbuses, true))) diff --git a/tests/qemuxml2argvdata/qemuxml2argv-pseries-hostdev.args b/tests/qemuxml2argvdata/qemuxml2argv-pseries-hostdev.args index dbd2964..9630a83 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-pseries-hostdev.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-pseries-hostdev.args @@ -18,6 +18,8 @@ QEMU_AUDIO_DRV=none \ server,nowait \ -mon chardev=charmonitor,id=monitor,mode=readline \ -boot c \ --device vfio-pci,host=0001:01:00.0,id=hostdev0,bus=pci.0,addr=0x1 \ --device vfio-pci,host=0005:90:01.2,id=hostdev1,bus=pci.0,addr=0x2 \ --device vfio-pci,host=0001:01:00.1,id=hostdev2,bus=pci.0,addr=0x3 +-device spapr-pci-host-bridge,index=1,id=pci.1 \ +-device spapr-pci-host-bridge,index=2,id=pci.2 \ +-device vfio-pci,host=0001:01:00.0,id=hostdev0,bus=pci.1.0,addr=0x1 \ +-device vfio-pci,host=0005:90:01.2,id=hostdev1,bus=pci.2.0,addr=0x1 \ +-device vfio-pci,host=0001:01:00.1,id=hostdev2,bus=pci.1.0,addr=0x2 diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-pseries-hostdev.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-pseries-hostdev.xml index a00c09c..7560661 100644 --- a/tests/qemuxml2xmloutdata/qemuxml2xmlout-pseries-hostdev.xml +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-pseries-hostdev.xml @@ -19,26 +19,36 @@ <target index='0'/> </controller> <controller type='usb' index='0' model='none'/> + <controller type='pci' index='1' model='pci-root'> + <model name='spapr-pci-host-bridge'/> + <target index='1'/> + <isolationGroup>2</isolationGroup> + </controller> + <controller type='pci' index='2' model='pci-root'> + <model name='spapr-pci-host-bridge'/> + <target index='2'/> + <isolationGroup>3</isolationGroup> + </controller> <hostdev mode='subsystem' type='pci' managed='yes'> <driver name='vfio'/> <source> <address domain='0x0001' bus='0x01' slot='0x00' function='0x0'/> </source> - <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x0'/> + <address type='pci' domain='0x0000' bus='0x01' slot='0x01' function='0x0'/> </hostdev> <hostdev mode='subsystem' type='pci' managed='yes'> <driver name='vfio'/> <source> <address domain='0x0005' bus='0x90' slot='0x01' function='0x2'/> </source> - <address type='pci' domain='0x0000' bus='0x00' slot='0x02' function='0x0'/> + <address type='pci' domain='0x0000' bus='0x02' slot='0x01' function='0x0'/> </hostdev> <hostdev mode='subsystem' type='pci' managed='yes'> <driver name='vfio'/> <source> <address domain='0x0001' bus='0x01' slot='0x00' function='0x1'/> </source> - <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/> + <address type='pci' domain='0x0000' bus='0x01' slot='0x02' function='0x0'/> </hostdev> <memballoon model='none'/> <panic model='pseries'/> -- 2.7.5