[libvirt] [PATCH 0/4] Move ccwaddrs and pciaddrs to domainDef
This patch depends on another one, because it was created on top of the changes in the other one. Link to the dependency: https://www.redhat.com/archives/libvir-list/2016-June/msg01227.html This is the next patch of the ongoing process of abstracting device address generation. In this patch, addres sets are moved from QEMU's (ccw and pci) and bhyve's (pci only) private data to domainDef. In the future, it's likely that more hypervisors will add the feature of explicitly specifying the device addresses, so handling the addresses should be made more generic. Tomasz Flendrich (4): Move functions from qemu_domain_address to domain_addr Move ccwaddrs from QEMU's private data to domainDef Move virDomainPCIAddressSetFree from domain_addr to domain_conf Move pciaddrs from private data to domainDef src/bhyve/bhyve_device.c | 16 +- src/bhyve/bhyve_domain.c | 2 - src/bhyve/bhyve_domain.h | 3 - src/conf/domain_addr.c | 497 ++++++++++++++++++++++++++++++++++-- src/conf/domain_addr.h | 67 ++--- src/conf/domain_conf.c | 21 ++ src/conf/domain_conf.h | 51 ++++ src/libvirt_private.syms | 11 +- src/qemu/qemu_domain.c | 4 +- src/qemu/qemu_domain.h | 3 - src/qemu/qemu_domain_address.c | 561 +++-------------------------------------- src/qemu/qemu_domain_address.h | 3 +- src/qemu/qemu_hotplug.c | 23 +- src/qemu/qemu_process.c | 6 +- tests/qemuhotplugtest.c | 2 +- 15 files changed, 629 insertions(+), 641 deletions(-) -- 2.7.4 (Apple Git-66)
These functions don't use any QEMU's specific data, so they can be moved to a file which is shared between all of the hypervisors. Their names are changed from qemu* to vir*. More hypervisors will probably implement the possibility of explicitly stating the address of devices, so it makes sense to be prepared for it by making address assignment generic. After moving the rest of the functions, I will restore the "static" keyword wherever applicable. --- src/conf/domain_addr.c | 477 +++++++++++++++++++++++++++++++++++++ src/conf/domain_addr.h | 19 ++ src/libvirt_private.syms | 7 + src/qemu/qemu_domain_address.c | 517 ++--------------------------------------- 4 files changed, 519 insertions(+), 501 deletions(-) diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c index e482328..3570a62 100644 --- a/src/conf/domain_addr.c +++ b/src/conf/domain_addr.c @@ -1229,3 +1229,480 @@ virDomainVirtioSerialAddrRelease(virDomainVirtioSerialAddrSetPtr addrs, VIR_FREE(str); return ret; } + +int +virDomainAssignVirtioSerialAddresses(virDomainDefPtr def) +{ + int ret = -1; + size_t i; + virDomainVirtioSerialAddrSetPtr addrs = NULL; + + if (!(addrs = virDomainVirtioSerialAddrSetCreate())) + goto cleanup; + + if (virDomainVirtioSerialAddrSetAddControllers(addrs, def) < 0) + goto cleanup; + + if (virDomainDeviceInfoIterate(def, virDomainVirtioSerialAddrReserve, + addrs) < 0) + goto cleanup; + + VIR_DEBUG("Finished reserving existing ports"); + + for (i = 0; i < def->nconsoles; i++) { + virDomainChrDefPtr chr = def->consoles[i]; + if (chr->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CONSOLE && + chr->targetType == VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_VIRTIO && + !virDomainVirtioSerialAddrIsComplete(&chr->info) && + virDomainVirtioSerialAddrAutoAssign(def, addrs, &chr->info, true) < 0) + goto cleanup; + } + + for (i = 0; i < def->nchannels; i++) { + virDomainChrDefPtr chr = def->channels[i]; + if (chr->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CHANNEL && + chr->targetType == VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_VIRTIO && + !virDomainVirtioSerialAddrIsComplete(&chr->info) && + virDomainVirtioSerialAddrAutoAssign(def, addrs, &chr->info, false) < 0) + goto cleanup; + } + + /* we persist the addresses */ + virDomainVirtioSerialAddrSetFree(def->vioserialaddrs); + def->vioserialaddrs = addrs; + addrs = NULL; + + ret = 0; + + cleanup: + virDomainVirtioSerialAddrSetFree(addrs); + return ret; +} + +static int +virDomainSpaprVIOFindByReg(virDomainDefPtr def ATTRIBUTE_UNUSED, + virDomainDeviceDefPtr device ATTRIBUTE_UNUSED, + virDomainDeviceInfoPtr info, void *opaque) +{ + virDomainDeviceInfoPtr target = opaque; + + if (info->type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_SPAPRVIO) + return 0; + + /* Match a dev that has a reg, is not us, and has a matching reg */ + if (info->addr.spaprvio.has_reg && info != target && + info->addr.spaprvio.reg == target->addr.spaprvio.reg) + /* Has to be < 0 so virDomainDeviceInfoIterate() will exit */ + return -1; + + return 0; +} + +int +virDomainAssignSpaprVIOAddress(virDomainDefPtr def, + virDomainDeviceInfoPtr info, + unsigned long long default_reg) +{ + bool user_reg; + int ret; + + if (info->type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_SPAPRVIO) + return 0; + + /* Check if the user has assigned the reg already, if so use it */ + user_reg = info->addr.spaprvio.has_reg; + if (!user_reg) { + info->addr.spaprvio.reg = default_reg; + info->addr.spaprvio.has_reg = true; + } + + ret = virDomainDeviceInfoIterate(def, virDomainSpaprVIOFindByReg, info); + while (ret != 0) { + if (user_reg) { + virReportError(VIR_ERR_XML_ERROR, + _("spapr-vio address %#llx already in use"), + info->addr.spaprvio.reg); + return -EEXIST; + } + + /* We assigned the reg, so try a new value */ + info->addr.spaprvio.reg += 0x1000; + ret = virDomainDeviceInfoIterate(def, virDomainSpaprVIOFindByReg, + info); + } + + return 0; +} + +void +virDomainPrimeVirtioDeviceAddresses(virDomainDefPtr def, + virDomainDeviceAddressType type) +{ + /* + declare address-less virtio devices to be of address type 'type' + disks, networks, consoles, controllers, memballoon and rng in this + order + if type is ccw filesystem devices are declared to be of address type ccw + */ + size_t i; + + for (i = 0; i < def->ndisks; i++) { + if (def->disks[i]->bus == VIR_DOMAIN_DISK_BUS_VIRTIO && + def->disks[i]->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) + def->disks[i]->info.type = type; + } + + for (i = 0; i < def->nnets; i++) { + if (STREQ(def->nets[i]->model, "virtio") && + def->nets[i]->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) { + def->nets[i]->info.type = type; + } + } + + for (i = 0; i < def->ninputs; i++) { + if (def->inputs[i]->bus == VIR_DOMAIN_DISK_BUS_VIRTIO && + def->inputs[i]->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) + def->inputs[i]->info.type = type; + } + + for (i = 0; i < def->ncontrollers; i++) { + if ((def->controllers[i]->type == + VIR_DOMAIN_CONTROLLER_TYPE_VIRTIO_SERIAL || + def->controllers[i]->type == + VIR_DOMAIN_CONTROLLER_TYPE_SCSI) && + def->controllers[i]->info.type == + VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) + def->controllers[i]->info.type = type; + } + + if (def->memballoon && + def->memballoon->model == VIR_DOMAIN_MEMBALLOON_MODEL_VIRTIO && + def->memballoon->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) + def->memballoon->info.type = type; + + for (i = 0; i < def->nrngs; i++) { + if (def->rngs[i]->model == VIR_DOMAIN_RNG_MODEL_VIRTIO && + def->rngs[i]->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) + def->rngs[i]->info.type = type; + } + + if (type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW) { + for (i = 0; i < def->nfss; i++) { + if (def->fss[i]->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) + def->fss[i]->info.type = type; + } + } +} + +static int +virDomainCollectPCIAddress(virDomainDefPtr def ATTRIBUTE_UNUSED, + virDomainDeviceDefPtr device, + virDomainDeviceInfoPtr info, + void *opaque) +{ + virDomainPCIAddressSetPtr addrs = opaque; + int ret = -1; + virPCIDeviceAddressPtr addr = &info->addr.pci; + bool entireSlot; + /* flags may be changed from default below */ + virDomainPCIConnectFlags flags = (VIR_PCI_CONNECT_HOTPLUGGABLE | + VIR_PCI_CONNECT_TYPE_PCI_DEVICE); + + if (!virDeviceInfoPCIAddressPresent(info) || + ((device->type == VIR_DOMAIN_DEVICE_HOSTDEV) && + (device->data.hostdev->parent.type != VIR_DOMAIN_DEVICE_NONE))) { + /* If a hostdev has a parent, its info will be a part of the + * parent, and will have its address collected during the scan + * of the parent's device type. + */ + return 0; + } + + /* Change flags according to differing requirements of different + * devices. + */ + switch (device->type) { + case VIR_DOMAIN_DEVICE_CONTROLLER: + switch (device->data.controller->type) { + case VIR_DOMAIN_CONTROLLER_TYPE_PCI: + flags = virDomainPCIControllerModelToConnectType(device->data.controller->model); + break; + + case VIR_DOMAIN_CONTROLLER_TYPE_SATA: + /* SATA controllers aren't hot-plugged, and can be put in + * either a PCI or PCIe slot + */ + flags = (VIR_PCI_CONNECT_TYPE_PCI_DEVICE + | VIR_PCI_CONNECT_TYPE_PCIE_DEVICE); + break; + + case VIR_DOMAIN_CONTROLLER_TYPE_USB: + /* allow UHCI and EHCI controllers to be manually placed on + * the PCIe bus (but don't put them there automatically) + */ + switch (device->data.controller->model) { + case VIR_DOMAIN_CONTROLLER_MODEL_USB_EHCI: + case VIR_DOMAIN_CONTROLLER_MODEL_USB_ICH9_EHCI1: + case VIR_DOMAIN_CONTROLLER_MODEL_USB_ICH9_UHCI1: + case VIR_DOMAIN_CONTROLLER_MODEL_USB_ICH9_UHCI2: + case VIR_DOMAIN_CONTROLLER_MODEL_USB_ICH9_UHCI3: + case VIR_DOMAIN_CONTROLLER_MODEL_USB_VT82C686B_UHCI: + flags = VIR_PCI_CONNECT_TYPE_PCI_DEVICE; + break; + case VIR_DOMAIN_CONTROLLER_MODEL_USB_NEC_XHCI: + /* should this be PCIE-only? Or do we need to allow PCI + * for backward compatibility? + */ + flags = (VIR_PCI_CONNECT_TYPE_PCI_DEVICE + | VIR_PCI_CONNECT_TYPE_PCIE_DEVICE); + break; + case VIR_DOMAIN_CONTROLLER_MODEL_USB_PCI_OHCI: + case VIR_DOMAIN_CONTROLLER_MODEL_USB_PIIX3_UHCI: + case VIR_DOMAIN_CONTROLLER_MODEL_USB_PIIX4_UHCI: + /* Allow these for PCI only */ + break; + } + } + break; + + case VIR_DOMAIN_DEVICE_SOUND: + switch (device->data.sound->model) { + case VIR_DOMAIN_SOUND_MODEL_ICH6: + case VIR_DOMAIN_SOUND_MODEL_ICH9: + flags = VIR_PCI_CONNECT_TYPE_PCI_DEVICE; + break; + } + break; + + case VIR_DOMAIN_DEVICE_VIDEO: + /* video cards aren't hot-plugged, and can be put in either a + * PCI or PCIe slot + */ + flags = (VIR_PCI_CONNECT_TYPE_PCI_DEVICE + | VIR_PCI_CONNECT_TYPE_PCIE_DEVICE); + break; + } + + /* Ignore implicit controllers on slot 0:0:1.0: + * implicit IDE controller on 0:0:1.1 (no qemu command line) + * implicit USB controller on 0:0:1.2 (-usb) + * + * If the machine does have a PCI bus, they will get reserved + * in qemuDomainAssignDevicePCISlots(). + */ + + /* These are the IDE and USB controllers in the PIIX3, hardcoded + * to bus 0 slot 1. They cannot be attached to a PCIe slot, only + * PCI. + */ + if (device->type == VIR_DOMAIN_DEVICE_CONTROLLER && addr->domain == 0 && + addr->bus == 0 && addr->slot == 1) { + virDomainControllerDefPtr cont = device->data.controller; + + if ((cont->type == VIR_DOMAIN_CONTROLLER_TYPE_IDE && cont->idx == 0 && + addr->function == 1) || + (cont->type == VIR_DOMAIN_CONTROLLER_TYPE_USB && cont->idx == 0 && + (cont->model == VIR_DOMAIN_CONTROLLER_MODEL_USB_PIIX3_UHCI || + cont->model == -1) && addr->function == 2)) { + /* Note the check for nbuses > 0 - if there are no PCI + * buses, we skip this check. This is a quirk required for + * some machinetypes such as s390, which pretend to have a + * PCI bus for long enough to generate the "-usb" on the + * commandline, but that don't really care if a PCI bus + * actually exists. */ + if (addrs->nbuses > 0 && + !(addrs->buses[0].flags & VIR_PCI_CONNECT_TYPE_PCI_DEVICE)) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Bus 0 must be PCI for integrated PIIX3 " + "USB or IDE controllers")); + return -1; + } else { + return 0; + } + } + } + + entireSlot = (addr->function == 0 && + addr->multi != VIR_TRISTATE_SWITCH_ON); + + if (virDomainPCIAddressReserveAddr(addrs, addr, flags, + entireSlot, true) < 0) + goto cleanup; + + ret = 0; + cleanup: + return ret; +} + +virDomainPCIAddressSetPtr +virDomainPCIAddressSetCreate(virDomainDefPtr def, + unsigned int nbuses, + bool dryRun) +{ + virDomainPCIAddressSetPtr addrs; + size_t i; + + if ((addrs = virDomainPCIAddressSetAlloc(nbuses)) == NULL) + return NULL; + + addrs->nbuses = nbuses; + addrs->dryRun = dryRun; + + /* As a safety measure, set default model='pci-root' for first pci + * controller and 'pci-bridge' for all subsequent. After setting + * those defaults, then scan the config and set the actual model + * for all addrs[idx]->bus that already have a corresponding + * controller in the config. + * + */ + if (nbuses > 0) + virDomainPCIAddressBusSetModel(&addrs->buses[0], + VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT); + for (i = 1; i < nbuses; i++) { + virDomainPCIAddressBusSetModel(&addrs->buses[i], + VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE); + } + + for (i = 0; i < def->ncontrollers; i++) { + size_t idx = def->controllers[i]->idx; + + if (def->controllers[i]->type != VIR_DOMAIN_CONTROLLER_TYPE_PCI) + continue; + + if (idx >= addrs->nbuses) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Inappropriate new pci controller index %zu " + "not found in addrs"), idx); + goto error; + } + + if (virDomainPCIAddressBusSetModel(&addrs->buses[idx], + def->controllers[i]->model) < 0) + goto error; + } + + if (virDomainDeviceInfoIterate(def, virDomainCollectPCIAddress, addrs) < 0) + goto error; + + return addrs; + + error: + virDomainPCIAddressSetFree(addrs); + return NULL; +} + +bool +virDomainPCIBusFullyReserved(virDomainPCIAddressBusPtr bus) +{ + size_t i; + + for (i = bus->minSlot; i <= bus->maxSlot; i++) + if (!bus->slots[i]) + return false; + + return true; +} + +void +virDomainPCIControllerSetDefaultModelName(virDomainControllerDefPtr cont) +{ + int *modelName = &cont->opts.pciopts.modelName; + + /* make sure it's not already set */ + if (*modelName != VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_NONE) + return; + switch ((virDomainControllerModelPCI)cont->model) { + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE: + *modelName = VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_PCI_BRIDGE; + break; + case VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE: + *modelName = VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_I82801B11_BRIDGE; + break; + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT_PORT: + *modelName = VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_IOH3420; + break; + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_UPSTREAM_PORT: + *modelName = VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_X3130_UPSTREAM; + break; + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_DOWNSTREAM_PORT: + *modelName = VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_XIO3130_DOWNSTREAM; + break; + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_EXPANDER_BUS: + *modelName = VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_PXB; + break; + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_EXPANDER_BUS: + *modelName = VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_PXB_PCIE; + break; + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT: + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT: + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST: + break; + } +} + +int +virDomainAddressFindNewBusNr(virDomainDefPtr def) +{ +/* Try to find a nice default for busNr for a new pci-expander-bus. + * This is a bit tricky, since you need to satisfy the following: + * + * 1) There need to be enough unused bus numbers between busNr of this + * bus and busNr of the next highest bus for the guest to assign a + * unique bus number to each PCI bus that is a child of this + * bus. Each PCI controller. On top of this, the pxb device (which + * implements the pci-extender-bus) includes a pci-bridge within + * it, and that bridge also uses one bus number (so each pxb device + * requires at least 2 bus numbers). + * + * 2) There need to be enough bus numbers *below* this for all the + * child controllers of the pci-expander-bus with the next lower + * busNr (or the pci-root bus if there are no lower + * pci-expander-buses). + * + * 3) If at all possible, we want to avoid needing to change the busNr + * of a bus in the future, as that changes the guest's device ABI, + * which could potentially lead to issues with a guest OS that is + * picky about such things. + * + * Due to the impossibility of predicting what might be added to the + * config in the future, we can't make a foolproof choice, but since + * a pci-expander-bus (pxb) has slots for 32 devices, and the only + * practical use for it is to assign real devices on a particular + * NUMA node in the host, it's reasonably safe to assume it should + * never need any additional child buses (probably only a few of the + * 32 will ever be used). So for pci-expander-bus we find the lowest + * existing busNr, and set this one to the current lowest - 2 (one + * for the pxb, one for the intergrated pci-bridge), thus leaving the + * maximum possible bus numbers available for other buses plugged + * into pci-root (i.e. pci-bridges and other + * pci-expander-buses). Anyone who needs more than 32 devices + * descended from one pci-expander-bus should set the busNr manually + * in the config. + * + * There is room for more error checking here - in particular we + * can/should determine the ultimate parent (root-bus) of each PCI + * controller and determine if there is enough space for all the + * buses within the current range allotted to the bus just prior to + * this one. + */ + size_t i; + int lowestBusNr = 256; + + for (i = 0; i < def->ncontrollers; i++) { + if (def->controllers[i]->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI) { + int thisBusNr = def->controllers[i]->opts.pciopts.busNr; + + if (thisBusNr >= 0 && thisBusNr < lowestBusNr) + lowestBusNr = thisBusNr; + } + } + + /* If we already have a busNR = 1, then we can't auto-assign (0 is + * the pci[e]-root, and the others may have been assigned + * purposefully). + */ + if (lowestBusNr <= 2) + return -1; + + return lowestBusNr - 2; +} diff --git a/src/conf/domain_addr.h b/src/conf/domain_addr.h index 0fb5f85..f4a9852 100644 --- a/src/conf/domain_addr.h +++ b/src/conf/domain_addr.h @@ -220,4 +220,23 @@ virDomainVirtioSerialAddrRelease(virDomainVirtioSerialAddrSetPtr addrs, virDomainDeviceInfoPtr info) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); +int virDomainAssignVirtioSerialAddresses(virDomainDefPtr def); + +int virDomainAssignSpaprVIOAddress(virDomainDefPtr def, + virDomainDeviceInfoPtr info, + unsigned long long default_reg); + +void virDomainPrimeVirtioDeviceAddresses(virDomainDefPtr def, + virDomainDeviceAddressType type); + +virDomainPCIAddressSetPtr virDomainPCIAddressSetCreate(virDomainDefPtr def, + unsigned int nbuses, + bool dryRun); + +bool virDomainPCIBusFullyReserved(virDomainPCIAddressBusPtr bus); + +void virDomainPCIControllerSetDefaultModelName(virDomainControllerDefPtr cont); + +int virDomainAddressFindNewBusNr(virDomainDefPtr def); + #endif /* __DOMAIN_ADDR_H__ */ diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index d29eb07..7df6b35 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -87,6 +87,9 @@ virPCIDeviceAddressParseXML; # conf/domain_addr.h +virDomainAddressFindNewBusNr; +virDomainAssignSpaprVIOAddress; +virDomainAssignVirtioSerialAddresses; virDomainCCWAddressAllocate; virDomainCCWAddressAssign; virDomainCCWAddressReleaseAddr; @@ -104,11 +107,15 @@ virDomainPCIAddressReserveAddr; virDomainPCIAddressReserveNextSlot; virDomainPCIAddressReserveSlot; virDomainPCIAddressSetAlloc; +virDomainPCIAddressSetCreate; virDomainPCIAddressSetFree; virDomainPCIAddressSetGrow; virDomainPCIAddressSlotInUse; virDomainPCIAddressValidate; +virDomainPCIBusFullyReserved; virDomainPCIControllerModelToConnectType; +virDomainPCIControllerSetDefaultModelName; +virDomainPrimeVirtioDeviceAddresses; virDomainVirtioSerialAddrAssign; virDomainVirtioSerialAddrAutoAssign; virDomainVirtioSerialAddrIsComplete; diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c index 1633c9b..49f3652 100644 --- a/src/qemu/qemu_domain_address.c +++ b/src/qemu/qemu_domain_address.c @@ -107,113 +107,6 @@ qemuDomainSetSCSIControllerModel(const virDomainDef *def, static int -qemuDomainAssignVirtioSerialAddresses(virDomainDefPtr def) -{ - int ret = -1; - size_t i; - virDomainVirtioSerialAddrSetPtr addrs = NULL; - - if (!(addrs = virDomainVirtioSerialAddrSetCreate())) - goto cleanup; - - if (virDomainVirtioSerialAddrSetAddControllers(addrs, def) < 0) - goto cleanup; - - if (virDomainDeviceInfoIterate(def, virDomainVirtioSerialAddrReserve, - addrs) < 0) - goto cleanup; - - VIR_DEBUG("Finished reserving existing ports"); - - for (i = 0; i < def->nconsoles; i++) { - virDomainChrDefPtr chr = def->consoles[i]; - if (chr->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CONSOLE && - chr->targetType == VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_VIRTIO && - !virDomainVirtioSerialAddrIsComplete(&chr->info) && - virDomainVirtioSerialAddrAutoAssign(def, addrs, &chr->info, true) < 0) - goto cleanup; - } - - for (i = 0; i < def->nchannels; i++) { - virDomainChrDefPtr chr = def->channels[i]; - if (chr->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CHANNEL && - chr->targetType == VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_VIRTIO && - !virDomainVirtioSerialAddrIsComplete(&chr->info) && - virDomainVirtioSerialAddrAutoAssign(def, addrs, &chr->info, false) < 0) - goto cleanup; - } - - /* we persist the addresses */ - virDomainVirtioSerialAddrSetFree(def->vioserialaddrs); - def->vioserialaddrs = addrs; - addrs = NULL; - - ret = 0; - - cleanup: - virDomainVirtioSerialAddrSetFree(addrs); - return ret; -} - - -static int -qemuDomainSpaprVIOFindByReg(virDomainDefPtr def ATTRIBUTE_UNUSED, - virDomainDeviceDefPtr device ATTRIBUTE_UNUSED, - virDomainDeviceInfoPtr info, void *opaque) -{ - virDomainDeviceInfoPtr target = opaque; - - if (info->type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_SPAPRVIO) - return 0; - - /* Match a dev that has a reg, is not us, and has a matching reg */ - if (info->addr.spaprvio.has_reg && info != target && - info->addr.spaprvio.reg == target->addr.spaprvio.reg) - /* Has to be < 0 so virDomainDeviceInfoIterate() will exit */ - return -1; - - return 0; -} - - -static int -qemuDomainAssignSpaprVIOAddress(virDomainDefPtr def, - virDomainDeviceInfoPtr info, - unsigned long long default_reg) -{ - bool user_reg; - int ret; - - if (info->type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_SPAPRVIO) - return 0; - - /* Check if the user has assigned the reg already, if so use it */ - user_reg = info->addr.spaprvio.has_reg; - if (!user_reg) { - info->addr.spaprvio.reg = default_reg; - info->addr.spaprvio.has_reg = true; - } - - ret = virDomainDeviceInfoIterate(def, qemuDomainSpaprVIOFindByReg, info); - while (ret != 0) { - if (user_reg) { - virReportError(VIR_ERR_XML_ERROR, - _("spapr-vio address %#llx already in use"), - info->addr.spaprvio.reg); - return -EEXIST; - } - - /* We assigned the reg, so try a new value */ - info->addr.spaprvio.reg += 0x1000; - ret = virDomainDeviceInfoIterate(def, qemuDomainSpaprVIOFindByReg, - info); - } - - return 0; -} - - -static int qemuDomainAssignSpaprVIOAddresses(virDomainDefPtr def, virQEMUCapsPtr qemuCaps) { @@ -227,7 +120,7 @@ qemuDomainAssignSpaprVIOAddresses(virDomainDefPtr def, if (def->nets[i]->model && STREQ(def->nets[i]->model, "spapr-vlan")) def->nets[i]->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_SPAPRVIO; - if (qemuDomainAssignSpaprVIOAddress(def, &def->nets[i]->info, + if (virDomainAssignSpaprVIOAddress(def, &def->nets[i]->info, VIO_ADDR_NET) < 0) goto cleanup; } @@ -242,7 +135,7 @@ qemuDomainAssignSpaprVIOAddresses(virDomainDefPtr def, if (model == VIR_DOMAIN_CONTROLLER_MODEL_SCSI_IBMVSCSI && def->controllers[i]->type == VIR_DOMAIN_CONTROLLER_TYPE_SCSI) def->controllers[i]->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_SPAPRVIO; - if (qemuDomainAssignSpaprVIOAddress(def, &def->controllers[i]->info, + if (virDomainAssignSpaprVIOAddress(def, &def->controllers[i]->info, VIO_ADDR_SCSI) < 0) goto cleanup; } @@ -252,7 +145,7 @@ qemuDomainAssignSpaprVIOAddresses(virDomainDefPtr def, ARCH_IS_PPC64(def->os.arch) && STRPREFIX(def->os.machine, "pseries")) def->serials[i]->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_SPAPRVIO; - if (qemuDomainAssignSpaprVIOAddress(def, &def->serials[i]->info, + if (virDomainAssignSpaprVIOAddress(def, &def->serials[i]->info, VIO_ADDR_SERIAL) < 0) goto cleanup; } @@ -261,7 +154,7 @@ qemuDomainAssignSpaprVIOAddresses(virDomainDefPtr def, if (ARCH_IS_PPC64(def->os.arch) && STRPREFIX(def->os.machine, "pseries")) def->nvram->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_SPAPRVIO; - if (qemuDomainAssignSpaprVIOAddress(def, &def->nvram->info, + if (virDomainAssignSpaprVIOAddress(def, &def->nvram->info, VIO_ADDR_NVRAM) < 0) goto cleanup; } @@ -275,67 +168,6 @@ qemuDomainAssignSpaprVIOAddresses(virDomainDefPtr def, } -static void -qemuDomainPrimeVirtioDeviceAddresses(virDomainDefPtr def, - virDomainDeviceAddressType type) -{ - /* - declare address-less virtio devices to be of address type 'type' - disks, networks, consoles, controllers, memballoon and rng in this - order - if type is ccw filesystem devices are declared to be of address type ccw - */ - size_t i; - - for (i = 0; i < def->ndisks; i++) { - if (def->disks[i]->bus == VIR_DOMAIN_DISK_BUS_VIRTIO && - def->disks[i]->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) - def->disks[i]->info.type = type; - } - - for (i = 0; i < def->nnets; i++) { - if (STREQ(def->nets[i]->model, "virtio") && - def->nets[i]->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) { - def->nets[i]->info.type = type; - } - } - - for (i = 0; i < def->ninputs; i++) { - if (def->inputs[i]->bus == VIR_DOMAIN_DISK_BUS_VIRTIO && - def->inputs[i]->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) - def->inputs[i]->info.type = type; - } - - for (i = 0; i < def->ncontrollers; i++) { - if ((def->controllers[i]->type == - VIR_DOMAIN_CONTROLLER_TYPE_VIRTIO_SERIAL || - def->controllers[i]->type == - VIR_DOMAIN_CONTROLLER_TYPE_SCSI) && - def->controllers[i]->info.type == - VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) - def->controllers[i]->info.type = type; - } - - if (def->memballoon && - def->memballoon->model == VIR_DOMAIN_MEMBALLOON_MODEL_VIRTIO && - def->memballoon->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) - def->memballoon->info.type = type; - - for (i = 0; i < def->nrngs; i++) { - if (def->rngs[i]->model == VIR_DOMAIN_RNG_MODEL_VIRTIO && - def->rngs[i]->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) - def->rngs[i]->info.type = type; - } - - if (type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW) { - for (i = 0; i < def->nfss; i++) { - if (def->fss[i]->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) - def->fss[i]->info.type = type; - } - } -} - - /* * Three steps populating CCW devnos * 1. Allocate empty address set @@ -353,7 +185,7 @@ qemuDomainAssignS390Addresses(virDomainDefPtr def, if (qemuDomainMachineIsS390CCW(def) && virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_CCW)) { - qemuDomainPrimeVirtioDeviceAddresses( + virDomainPrimeVirtioDeviceAddresses( def, VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW); if (!(addrs = virDomainCCWAddressSetCreate())) @@ -368,7 +200,7 @@ qemuDomainAssignS390Addresses(virDomainDefPtr def, goto cleanup; } else if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_S390)) { /* deal with legacy virtio-s390 */ - qemuDomainPrimeVirtioDeviceAddresses( + virDomainPrimeVirtioDeviceAddresses( def, VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_S390); } @@ -403,211 +235,13 @@ qemuDomainAssignARMVirtioMMIOAddresses(virDomainDefPtr def, return; if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VIRTIO_MMIO)) { - qemuDomainPrimeVirtioDeviceAddresses( + virDomainPrimeVirtioDeviceAddresses( def, VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_MMIO); } } static int -qemuDomainCollectPCIAddress(virDomainDefPtr def ATTRIBUTE_UNUSED, - virDomainDeviceDefPtr device, - virDomainDeviceInfoPtr info, - void *opaque) -{ - virDomainPCIAddressSetPtr addrs = opaque; - int ret = -1; - virPCIDeviceAddressPtr addr = &info->addr.pci; - bool entireSlot; - /* flags may be changed from default below */ - virDomainPCIConnectFlags flags = (VIR_PCI_CONNECT_HOTPLUGGABLE | - VIR_PCI_CONNECT_TYPE_PCI_DEVICE); - - if (!virDeviceInfoPCIAddressPresent(info) || - ((device->type == VIR_DOMAIN_DEVICE_HOSTDEV) && - (device->data.hostdev->parent.type != VIR_DOMAIN_DEVICE_NONE))) { - /* If a hostdev has a parent, its info will be a part of the - * parent, and will have its address collected during the scan - * of the parent's device type. - */ - return 0; - } - - /* Change flags according to differing requirements of different - * devices. - */ - switch (device->type) { - case VIR_DOMAIN_DEVICE_CONTROLLER: - switch (device->data.controller->type) { - case VIR_DOMAIN_CONTROLLER_TYPE_PCI: - flags = virDomainPCIControllerModelToConnectType(device->data.controller->model); - break; - - case VIR_DOMAIN_CONTROLLER_TYPE_SATA: - /* SATA controllers aren't hot-plugged, and can be put in - * either a PCI or PCIe slot - */ - flags = (VIR_PCI_CONNECT_TYPE_PCI_DEVICE - | VIR_PCI_CONNECT_TYPE_PCIE_DEVICE); - break; - - case VIR_DOMAIN_CONTROLLER_TYPE_USB: - /* allow UHCI and EHCI controllers to be manually placed on - * the PCIe bus (but don't put them there automatically) - */ - switch (device->data.controller->model) { - case VIR_DOMAIN_CONTROLLER_MODEL_USB_EHCI: - case VIR_DOMAIN_CONTROLLER_MODEL_USB_ICH9_EHCI1: - case VIR_DOMAIN_CONTROLLER_MODEL_USB_ICH9_UHCI1: - case VIR_DOMAIN_CONTROLLER_MODEL_USB_ICH9_UHCI2: - case VIR_DOMAIN_CONTROLLER_MODEL_USB_ICH9_UHCI3: - case VIR_DOMAIN_CONTROLLER_MODEL_USB_VT82C686B_UHCI: - flags = VIR_PCI_CONNECT_TYPE_PCI_DEVICE; - break; - case VIR_DOMAIN_CONTROLLER_MODEL_USB_NEC_XHCI: - /* should this be PCIE-only? Or do we need to allow PCI - * for backward compatibility? - */ - flags = (VIR_PCI_CONNECT_TYPE_PCI_DEVICE - | VIR_PCI_CONNECT_TYPE_PCIE_DEVICE); - break; - case VIR_DOMAIN_CONTROLLER_MODEL_USB_PCI_OHCI: - case VIR_DOMAIN_CONTROLLER_MODEL_USB_PIIX3_UHCI: - case VIR_DOMAIN_CONTROLLER_MODEL_USB_PIIX4_UHCI: - /* Allow these for PCI only */ - break; - } - } - break; - - case VIR_DOMAIN_DEVICE_SOUND: - switch (device->data.sound->model) { - case VIR_DOMAIN_SOUND_MODEL_ICH6: - case VIR_DOMAIN_SOUND_MODEL_ICH9: - flags = VIR_PCI_CONNECT_TYPE_PCI_DEVICE; - break; - } - break; - - case VIR_DOMAIN_DEVICE_VIDEO: - /* video cards aren't hot-plugged, and can be put in either a - * PCI or PCIe slot - */ - flags = (VIR_PCI_CONNECT_TYPE_PCI_DEVICE - | VIR_PCI_CONNECT_TYPE_PCIE_DEVICE); - break; - } - - /* Ignore implicit controllers on slot 0:0:1.0: - * implicit IDE controller on 0:0:1.1 (no qemu command line) - * implicit USB controller on 0:0:1.2 (-usb) - * - * If the machine does have a PCI bus, they will get reserved - * in qemuDomainAssignDevicePCISlots(). - */ - - /* These are the IDE and USB controllers in the PIIX3, hardcoded - * to bus 0 slot 1. They cannot be attached to a PCIe slot, only - * PCI. - */ - if (device->type == VIR_DOMAIN_DEVICE_CONTROLLER && addr->domain == 0 && - addr->bus == 0 && addr->slot == 1) { - virDomainControllerDefPtr cont = device->data.controller; - - if ((cont->type == VIR_DOMAIN_CONTROLLER_TYPE_IDE && cont->idx == 0 && - addr->function == 1) || - (cont->type == VIR_DOMAIN_CONTROLLER_TYPE_USB && cont->idx == 0 && - (cont->model == VIR_DOMAIN_CONTROLLER_MODEL_USB_PIIX3_UHCI || - cont->model == -1) && addr->function == 2)) { - /* Note the check for nbuses > 0 - if there are no PCI - * buses, we skip this check. This is a quirk required for - * some machinetypes such as s390, which pretend to have a - * PCI bus for long enough to generate the "-usb" on the - * commandline, but that don't really care if a PCI bus - * actually exists. */ - if (addrs->nbuses > 0 && - !(addrs->buses[0].flags & VIR_PCI_CONNECT_TYPE_PCI_DEVICE)) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Bus 0 must be PCI for integrated PIIX3 " - "USB or IDE controllers")); - return -1; - } else { - return 0; - } - } - } - - entireSlot = (addr->function == 0 && - addr->multi != VIR_TRISTATE_SWITCH_ON); - - if (virDomainPCIAddressReserveAddr(addrs, addr, flags, - entireSlot, true) < 0) - goto cleanup; - - ret = 0; - cleanup: - return ret; -} - -static virDomainPCIAddressSetPtr -qemuDomainPCIAddressSetCreate(virDomainDefPtr def, - unsigned int nbuses, - bool dryRun) -{ - virDomainPCIAddressSetPtr addrs; - size_t i; - - if ((addrs = virDomainPCIAddressSetAlloc(nbuses)) == NULL) - return NULL; - - addrs->nbuses = nbuses; - addrs->dryRun = dryRun; - - /* As a safety measure, set default model='pci-root' for first pci - * controller and 'pci-bridge' for all subsequent. After setting - * those defaults, then scan the config and set the actual model - * for all addrs[idx]->bus that already have a corresponding - * controller in the config. - * - */ - if (nbuses > 0) - virDomainPCIAddressBusSetModel(&addrs->buses[0], - VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT); - for (i = 1; i < nbuses; i++) { - virDomainPCIAddressBusSetModel(&addrs->buses[i], - VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE); - } - - for (i = 0; i < def->ncontrollers; i++) { - size_t idx = def->controllers[i]->idx; - - if (def->controllers[i]->type != VIR_DOMAIN_CONTROLLER_TYPE_PCI) - continue; - - if (idx >= addrs->nbuses) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Inappropriate new pci controller index %zu " - "not found in addrs"), idx); - goto error; - } - - if (virDomainPCIAddressBusSetModel(&addrs->buses[idx], - def->controllers[i]->model) < 0) - goto error; - } - - if (virDomainDeviceInfoIterate(def, qemuDomainCollectPCIAddress, addrs) < 0) - goto error; - - return addrs; - - error: - virDomainPCIAddressSetFree(addrs); - return NULL; -} - - -static int qemuDomainValidateDevicePCISlotsPIIX3(virDomainDefPtr def, virQEMUCapsPtr qemuCaps, virDomainPCIAddressSetPtr addrs) @@ -719,7 +353,7 @@ qemuDomainValidateDevicePCISlotsPIIX3(virDomainDefPtr def, _("Primary video card must have PCI address 0:0:2.0")); goto cleanup; } - /* If TYPE == PCI, then qemuDomainCollectPCIAddress() function + /* If TYPE == PCI, then virDomainCollectPCIAddress() function * has already reserved the address, so we must skip */ } } else if (addrs->nbuses && !qemuDeviceVideoUsable) { @@ -910,7 +544,7 @@ qemuDomainValidateDevicePCISlotsQ35(virDomainDefPtr def, _("Primary video card must have PCI address 0:0:1.0")); goto cleanup; } - /* If TYPE == PCI, then qemuDomainCollectPCIAddress() function + /* If TYPE == PCI, then virDomainCollectPCIAddress() function * has already reserved the address, so we must skip */ } } else if (addrs->nbuses && !qemuDeviceVideoUsable) { @@ -952,19 +586,6 @@ qemuDomainValidateDevicePCISlotsChipsets(virDomainDefPtr def, } -static bool -qemuDomainPCIBusFullyReserved(virDomainPCIAddressBusPtr bus) -{ - size_t i; - - for (i = bus->minSlot; i <= bus->maxSlot; i++) - if (!bus->slots[i]) - return false; - - return true; -} - - /* * This assigns static PCI slots to all configured devices. * The ordering here is chosen to match the ordering used @@ -996,7 +617,7 @@ qemuDomainPCIBusFullyReserved(virDomainPCIAddressBusPtr bus) * - Watchdog * - pci serial devices * - * Prior to this function being invoked, qemuDomainCollectPCIAddress() will have + * Prior to this function being invoked, virDomainCollectPCIAddress() will have * added all existing PCI addresses from the 'def' to 'addrs'. Thus this * function must only try to reserve addresses if info.type == NONE and * skip over info.type == PCI @@ -1330,112 +951,6 @@ qemuDomainSupportsPCI(virDomainDefPtr def, } -static void -qemuDomainPCIControllerSetDefaultModelName(virDomainControllerDefPtr cont) -{ - int *modelName = &cont->opts.pciopts.modelName; - - /* make sure it's not already set */ - if (*modelName != VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_NONE) - return; - switch ((virDomainControllerModelPCI)cont->model) { - case VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE: - *modelName = VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_PCI_BRIDGE; - break; - case VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE: - *modelName = VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_I82801B11_BRIDGE; - break; - case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT_PORT: - *modelName = VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_IOH3420; - break; - case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_UPSTREAM_PORT: - *modelName = VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_X3130_UPSTREAM; - break; - case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_DOWNSTREAM_PORT: - *modelName = VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_XIO3130_DOWNSTREAM; - break; - case VIR_DOMAIN_CONTROLLER_MODEL_PCI_EXPANDER_BUS: - *modelName = VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_PXB; - break; - case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_EXPANDER_BUS: - *modelName = VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_PXB_PCIE; - break; - case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT: - case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT: - case VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST: - break; - } -} - - -static int -qemuDomainAddressFindNewBusNr(virDomainDefPtr def) -{ -/* Try to find a nice default for busNr for a new pci-expander-bus. - * This is a bit tricky, since you need to satisfy the following: - * - * 1) There need to be enough unused bus numbers between busNr of this - * bus and busNr of the next highest bus for the guest to assign a - * unique bus number to each PCI bus that is a child of this - * bus. Each PCI controller. On top of this, the pxb device (which - * implements the pci-extender-bus) includes a pci-bridge within - * it, and that bridge also uses one bus number (so each pxb device - * requires at least 2 bus numbers). - * - * 2) There need to be enough bus numbers *below* this for all the - * child controllers of the pci-expander-bus with the next lower - * busNr (or the pci-root bus if there are no lower - * pci-expander-buses). - * - * 3) If at all possible, we want to avoid needing to change the busNr - * of a bus in the future, as that changes the guest's device ABI, - * which could potentially lead to issues with a guest OS that is - * picky about such things. - * - * Due to the impossibility of predicting what might be added to the - * config in the future, we can't make a foolproof choice, but since - * a pci-expander-bus (pxb) has slots for 32 devices, and the only - * practical use for it is to assign real devices on a particular - * NUMA node in the host, it's reasonably safe to assume it should - * never need any additional child buses (probably only a few of the - * 32 will ever be used). So for pci-expander-bus we find the lowest - * existing busNr, and set this one to the current lowest - 2 (one - * for the pxb, one for the intergrated pci-bridge), thus leaving the - * maximum possible bus numbers available for other buses plugged - * into pci-root (i.e. pci-bridges and other - * pci-expander-buses). Anyone who needs more than 32 devices - * descended from one pci-expander-bus should set the busNr manually - * in the config. - * - * There is room for more error checking here - in particular we - * can/should determine the ultimate parent (root-bus) of each PCI - * controller and determine if there is enough space for all the - * buses within the current range allotted to the bus just prior to - * this one. - */ - size_t i; - int lowestBusNr = 256; - - for (i = 0; i < def->ncontrollers; i++) { - if (def->controllers[i]->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI) { - int thisBusNr = def->controllers[i]->opts.pciopts.busNr; - - if (thisBusNr >= 0 && thisBusNr < lowestBusNr) - lowestBusNr = thisBusNr; - } - } - - /* If we already have a busNR = 1, then we can't auto-assign (0 is - * the pci[e]-root, and the others may have been assigned - * purposefully). - */ - if (lowestBusNr <= 2) - return -1; - - return lowestBusNr - 2; -} - - static int qemuDomainAssignPCIAddresses(virDomainDefPtr def, virQEMUCapsPtr qemuCaps, @@ -1466,7 +981,7 @@ qemuDomainAssignPCIAddresses(virDomainDefPtr def, virDomainDeviceInfo info; /* 1st pass to figure out how many PCI bridges we need */ - if (!(addrs = qemuDomainPCIAddressSetCreate(def, nbuses, true))) + if (!(addrs = virDomainPCIAddressSetCreate(def, nbuses, true))) goto cleanup; if (qemuDomainValidateDevicePCISlotsChipsets(def, qemuCaps, @@ -1474,7 +989,7 @@ qemuDomainAssignPCIAddresses(virDomainDefPtr def, goto cleanup; for (i = 0; i < addrs->nbuses; i++) { - if (!qemuDomainPCIBusFullyReserved(&addrs->buses[i])) + if (!virDomainPCIBusFullyReserved(&addrs->buses[i])) buses_reserved = false; } @@ -1517,7 +1032,7 @@ qemuDomainAssignPCIAddresses(virDomainDefPtr def, goto cleanup; } - if (!(addrs = qemuDomainPCIAddressSetCreate(def, nbuses, false))) + if (!(addrs = virDomainPCIAddressSetCreate(def, nbuses, false))) goto cleanup; if (qemuDomainSupportsPCI(def, qemuCaps)) { @@ -1544,7 +1059,7 @@ qemuDomainAssignPCIAddresses(virDomainDefPtr def, * device in qemu) for any controller that doesn't yet * have it set. */ - qemuDomainPCIControllerSetDefaultModelName(cont); + virDomainPCIControllerSetDefaultModelName(cont); /* set defaults for any other auto-generated config * options for this controller that haven't been @@ -1570,7 +1085,7 @@ qemuDomainAssignPCIAddresses(virDomainDefPtr def, case VIR_DOMAIN_CONTROLLER_MODEL_PCI_EXPANDER_BUS: case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_EXPANDER_BUS: if (options->busNr == -1) - options->busNr = qemuDomainAddressFindNewBusNr(def); + options->busNr = virDomainAddressFindNewBusNr(def); if (options->busNr == -1) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("No free busNr lower than current " @@ -1626,7 +1141,7 @@ qemuDomainAssignAddresses(virDomainDefPtr def, virQEMUCapsPtr qemuCaps, virDomainObjPtr obj) { - if (qemuDomainAssignVirtioSerialAddresses(def) < 0) + if (virDomainAssignVirtioSerialAddresses(def) < 0) return -1; if (qemuDomainAssignSpaprVIOAddresses(def, qemuCaps) < 0) -- 2.7.4 (Apple Git-66)
The definitions of struct virDomainCCWAddressSet and virDomainCCWAddressSetFree() had to be moved from domain_addr to domain_conf, because virDomainDefFree() has to free the CCW address set when freeing domainDef. --- src/conf/domain_addr.c | 9 --------- src/conf/domain_addr.h | 7 ------- src/conf/domain_conf.c | 10 ++++++++++ src/conf/domain_conf.h | 9 +++++++++ src/libvirt_private.syms | 2 +- src/qemu/qemu_domain.c | 1 - src/qemu/qemu_domain.h | 1 - src/qemu/qemu_domain_address.c | 22 ++++++++-------------- src/qemu/qemu_hotplug.c | 8 ++++---- 9 files changed, 32 insertions(+), 37 deletions(-) diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c index 3570a62..93eebf2 100644 --- a/src/conf/domain_addr.c +++ b/src/conf/domain_addr.c @@ -805,15 +805,6 @@ virDomainCCWAddressReleaseAddr(virDomainCCWAddressSetPtr addrs, return ret; } -void virDomainCCWAddressSetFree(virDomainCCWAddressSetPtr addrs) -{ - if (!addrs) - return; - - virHashFree(addrs->defined); - VIR_FREE(addrs); -} - virDomainCCWAddressSetPtr virDomainCCWAddressSetCreate(void) { diff --git a/src/conf/domain_addr.h b/src/conf/domain_addr.h index f4a9852..cda1e3f 100644 --- a/src/conf/domain_addr.h +++ b/src/conf/domain_addr.h @@ -157,13 +157,6 @@ int virDomainPCIAddressReserveNextSlot(virDomainPCIAddressSetPtr addrs, virDomainPCIConnectFlags flags) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); -struct _virDomainCCWAddressSet { - virHashTablePtr defined; - virDomainDeviceCCWAddress next; -}; -typedef struct _virDomainCCWAddressSet virDomainCCWAddressSet; -typedef virDomainCCWAddressSet *virDomainCCWAddressSetPtr; - int virDomainCCWAddressAssign(virDomainDeviceInfoPtr dev, virDomainCCWAddressSetPtr addrs, bool autoassign) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 13230c9..12448dd 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2535,6 +2535,15 @@ virDomainVirtioSerialAddrSetFree(virDomainVirtioSerialAddrSetPtr addrs) } } +void virDomainCCWAddressSetFree(virDomainCCWAddressSetPtr addrs) +{ + if (!addrs) + return; + + virHashFree(addrs->defined); + VIR_FREE(addrs); +} + void virDomainDefFree(virDomainDefPtr def) { size_t i; @@ -2706,6 +2715,7 @@ void virDomainDefFree(virDomainDefPtr def) xmlFreeNode(def->metadata); virDomainVirtioSerialAddrSetFree(def->vioserialaddrs); + virDomainCCWAddressSetFree(def->ccwaddrs); VIR_FREE(def); } diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 2bac992..28fdb98 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2104,6 +2104,13 @@ struct _virDomainVirtioSerialAddrSet { typedef struct _virDomainVirtioSerialAddrSet virDomainVirtioSerialAddrSet; typedef virDomainVirtioSerialAddrSet *virDomainVirtioSerialAddrSetPtr; +struct _virDomainCCWAddressSet { + virHashTablePtr defined; + virDomainDeviceCCWAddress next; +}; +typedef struct _virDomainCCWAddressSet virDomainCCWAddressSet; +typedef virDomainCCWAddressSet *virDomainCCWAddressSetPtr; + typedef struct _virDomainPowerManagement virDomainPowerManagement; typedef virDomainPowerManagement *virDomainPowerManagementPtr; @@ -2271,6 +2278,7 @@ struct _virDomainDef { virDomainKeyWrapDefPtr keywrap; virDomainVirtioSerialAddrSetPtr vioserialaddrs; + virDomainCCWAddressSetPtr ccwaddrs; /* Application-specific custom metadata */ xmlNodePtr metadata; @@ -2545,6 +2553,7 @@ void virDomainDefClearDeviceAliases(virDomainDefPtr def); void virDomainTPMDefFree(virDomainTPMDefPtr def); void virDomainVirtioSerialControllerFree(virDomainVirtioSerialControllerPtr cont); void virDomainVirtioSerialAddrSetFree(virDomainVirtioSerialAddrSetPtr addrs); +void virDomainCCWAddressSetFree(virDomainCCWAddressSetPtr addrs); typedef int (*virDomainDeviceInfoCallback)(virDomainDefPtr def, virDomainDeviceDefPtr dev, diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 7df6b35..75c8f70 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -94,7 +94,6 @@ virDomainCCWAddressAllocate; virDomainCCWAddressAssign; virDomainCCWAddressReleaseAddr; virDomainCCWAddressSetCreate; -virDomainCCWAddressSetFree; virDomainCCWAddressValidate; virDomainGetBlkioParametersAssignFromDef; virDomainPCIAddressAsString; @@ -165,6 +164,7 @@ virDomainBootTypeFromString; virDomainBootTypeToString; virDomainCapabilitiesPolicyTypeToString; virDomainCapsFeatureTypeToString; +virDomainCCWAddressSetFree; virDomainChrConsoleTargetTypeFromString; virDomainChrConsoleTargetTypeToString; virDomainChrDefForeach; diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index f6ccbc0..5e3d305 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -1248,7 +1248,6 @@ qemuDomainObjPrivateFree(void *data) virCgroupFree(&priv->cgroup); virDomainPCIAddressSetFree(priv->pciaddrs); - virDomainCCWAddressSetFree(priv->ccwaddrs); virDomainChrSourceDefFree(priv->monConfig); qemuDomainObjFreeJob(priv); VIR_FREE(priv->vcpupids); diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 58221bb..4a15260 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -188,7 +188,6 @@ struct _qemuDomainObjPrivate { int *vcpupids; virDomainPCIAddressSetPtr pciaddrs; - virDomainCCWAddressSetPtr ccwaddrs; virQEMUCapsPtr qemuCaps; char *lockState; diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c index 49f3652..5d58eb1 100644 --- a/src/qemu/qemu_domain_address.c +++ b/src/qemu/qemu_domain_address.c @@ -176,12 +176,10 @@ qemuDomainAssignSpaprVIOAddresses(virDomainDefPtr def, */ static int qemuDomainAssignS390Addresses(virDomainDefPtr def, - virQEMUCapsPtr qemuCaps, - virDomainObjPtr obj) + virQEMUCapsPtr qemuCaps) { int ret = -1; virDomainCCWAddressSetPtr addrs = NULL; - qemuDomainObjPrivatePtr priv = NULL; if (qemuDomainMachineIsS390CCW(def) && virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_CCW)) { @@ -204,15 +202,11 @@ qemuDomainAssignS390Addresses(virDomainDefPtr def, def, VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_S390); } - if (obj && obj->privateData) { - priv = obj->privateData; - if (addrs) { - /* if this is the live domain object, we persist the CCW addresses*/ - virDomainCCWAddressSetFree(priv->ccwaddrs); - priv->ccwaddrs = addrs; - addrs = NULL; - } - } + /* we persist the CCW addresses*/ + virDomainCCWAddressSetFree(def->ccwaddrs); + def->ccwaddrs = addrs; + addrs = NULL; + ret = 0; cleanup: @@ -1147,7 +1141,7 @@ qemuDomainAssignAddresses(virDomainDefPtr def, if (qemuDomainAssignSpaprVIOAddresses(def, qemuCaps) < 0) return -1; - if (qemuDomainAssignS390Addresses(def, qemuCaps, obj) < 0) + if (qemuDomainAssignS390Addresses(def, qemuCaps) < 0) return -1; qemuDomainAssignARMVirtioMMIOAddresses(def, qemuCaps); @@ -1172,7 +1166,7 @@ qemuDomainReleaseDeviceAddress(virDomainObjPtr vm, if (info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW && qemuDomainMachineIsS390CCW(vm->def) && virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_VIRTIO_CCW) && - virDomainCCWAddressReleaseAddr(priv->ccwaddrs, info) < 0) + virDomainCCWAddressReleaseAddr(vm->def->ccwaddrs, info) < 0) VIR_WARN("Unable to release CCW address on %s", NULLSTR(devstr)); else if (virDeviceInfoPCIAddressPresent(info) && diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 696ec9e..bed4173 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -327,7 +327,7 @@ qemuDomainAttachVirtioDiskDevice(virConnectPtr conn, goto cleanup; if (disk->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW) { - if (virDomainCCWAddressAssign(&disk->info, priv->ccwaddrs, + if (virDomainCCWAddressAssign(&disk->info, vm->def->ccwaddrs, !disk->info.addr.ccw.assigned) < 0) goto error; } else if (!disk->info.type || @@ -457,7 +457,7 @@ int qemuDomainAttachControllerDevice(virQEMUDriverPtr driver, if (virDomainPCIAddressEnsureAddr(priv->pciaddrs, &controller->info) < 0) goto cleanup; } else if (controller->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW) { - if (virDomainCCWAddressAssign(&controller->info, priv->ccwaddrs, + if (virDomainCCWAddressAssign(&controller->info, vm->def->ccwaddrs, !controller->info.addr.ccw.assigned) < 0) goto cleanup; } @@ -944,7 +944,7 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver, if (qemuDomainMachineIsS390CCW(vm->def) && virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_VIRTIO_CCW)) { net->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW; - if (virDomainCCWAddressAssign(&net->info, priv->ccwaddrs, + if (virDomainCCWAddressAssign(&net->info, vm->def->ccwaddrs, !net->info.addr.ccw.assigned) < 0) goto cleanup; } else if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_VIRTIO_S390)) { @@ -1594,7 +1594,7 @@ qemuDomainAttachRNGDevice(virQEMUDriverPtr driver, if (virDomainPCIAddressEnsureAddr(priv->pciaddrs, &rng->info) < 0) return -1; } else if (rng->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW) { - if (virDomainCCWAddressAssign(&rng->info, priv->ccwaddrs, + if (virDomainCCWAddressAssign(&rng->info, vm->def->ccwaddrs, !rng->info.addr.ccw.assigned) < 0) return -1; } -- 2.7.4 (Apple Git-66)
pciaddrs will be moved from hypervisors' private data to domainDef, so domain_conf has to be able to free it. --- src/conf/domain_addr.c | 11 ----------- src/conf/domain_addr.h | 41 ----------------------------------------- src/conf/domain_conf.c | 10 ++++++++++ src/conf/domain_conf.h | 41 +++++++++++++++++++++++++++++++++++++++++ src/libvirt_private.syms | 2 +- 5 files changed, 52 insertions(+), 53 deletions(-) diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c index 93eebf2..09cbc73 100644 --- a/src/conf/domain_addr.c +++ b/src/conf/domain_addr.c @@ -550,17 +550,6 @@ virDomainPCIAddressSetAlloc(unsigned int nbuses) } -void -virDomainPCIAddressSetFree(virDomainPCIAddressSetPtr addrs) -{ - if (!addrs) - return; - - VIR_FREE(addrs->buses); - VIR_FREE(addrs); -} - - int virDomainPCIAddressGetNextSlot(virDomainPCIAddressSetPtr addrs, virPCIDeviceAddressPtr next_addr, diff --git a/src/conf/domain_addr.h b/src/conf/domain_addr.h index cda1e3f..e767f34 100644 --- a/src/conf/domain_addr.h +++ b/src/conf/domain_addr.h @@ -26,22 +26,8 @@ # include "domain_conf.h" -# define VIR_PCI_ADDRESS_SLOT_LAST 31 # define VIR_PCI_ADDRESS_FUNCTION_LAST 7 -typedef enum { - VIR_PCI_CONNECT_HOTPLUGGABLE = 1 << 0, /* is hotplug needed/supported */ - - /* kinds of devices as a bitmap so they can be combined (some PCI - * controllers permit connecting multiple types of devices) - */ - VIR_PCI_CONNECT_TYPE_PCI_DEVICE = 1 << 1, - VIR_PCI_CONNECT_TYPE_PCIE_DEVICE = 1 << 2, - VIR_PCI_CONNECT_TYPE_PCIE_ROOT_PORT = 1 << 3, - VIR_PCI_CONNECT_TYPE_PCIE_SWITCH_UPSTREAM_PORT = 1 << 4, - VIR_PCI_CONNECT_TYPE_PCIE_SWITCH_DOWNSTREAM_PORT = 1 << 5, -} virDomainPCIConnectFlags; - /* a combination of all bits that describe the type of connections * allowed, e.g. PCI, PCIe, switch */ @@ -62,38 +48,11 @@ typedef enum { virDomainPCIConnectFlags virDomainPCIControllerModelToConnectType(virDomainControllerModelPCI model); -typedef struct { - virDomainControllerModelPCI model; - /* flags and min/max can be computed from model, but - * having them ready makes life easier. - */ - virDomainPCIConnectFlags flags; - size_t minSlot, maxSlot; /* usually 0,0 or 0,31, or 1,31 */ - /* Each bit in a slot represents one function on that slot. If the - * bit is set, that function is in use by a device. - */ - uint8_t slots[VIR_PCI_ADDRESS_SLOT_LAST + 1]; -} virDomainPCIAddressBus; -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 */ -}; -typedef struct _virDomainPCIAddressSet virDomainPCIAddressSet; -typedef virDomainPCIAddressSet *virDomainPCIAddressSetPtr; - char *virDomainPCIAddressAsString(virPCIDeviceAddressPtr addr) ATTRIBUTE_NONNULL(1); virDomainPCIAddressSetPtr virDomainPCIAddressSetAlloc(unsigned int nbuses); -void virDomainPCIAddressSetFree(virDomainPCIAddressSetPtr addrs); - bool virDomainPCIAddressFlagsCompatible(virPCIDeviceAddressPtr addr, const char *addrStr, virDomainPCIConnectFlags busFlags, diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 12448dd..5f31a97 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2544,6 +2544,16 @@ void virDomainCCWAddressSetFree(virDomainCCWAddressSetPtr addrs) VIR_FREE(addrs); } +void +virDomainPCIAddressSetFree(virDomainPCIAddressSetPtr addrs) +{ + if (!addrs) + return; + + VIR_FREE(addrs->buses); + VIR_FREE(addrs); +} + void virDomainDefFree(virDomainDefPtr def) { size_t i; diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 28fdb98..887e061 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2111,6 +2111,46 @@ struct _virDomainCCWAddressSet { typedef struct _virDomainCCWAddressSet virDomainCCWAddressSet; typedef virDomainCCWAddressSet *virDomainCCWAddressSetPtr; +typedef enum { + VIR_PCI_CONNECT_HOTPLUGGABLE = 1 << 0, /* is hotplug needed/supported */ + + /* kinds of devices as a bitmap so they can be combined (some PCI + * controllers permit connecting multiple types of devices) + */ + VIR_PCI_CONNECT_TYPE_PCI_DEVICE = 1 << 1, + VIR_PCI_CONNECT_TYPE_PCIE_DEVICE = 1 << 2, + VIR_PCI_CONNECT_TYPE_PCIE_ROOT_PORT = 1 << 3, + VIR_PCI_CONNECT_TYPE_PCIE_SWITCH_UPSTREAM_PORT = 1 << 4, + VIR_PCI_CONNECT_TYPE_PCIE_SWITCH_DOWNSTREAM_PORT = 1 << 5, +} virDomainPCIConnectFlags; + +# define VIR_PCI_ADDRESS_SLOT_LAST 31 + +typedef struct { + virDomainControllerModelPCI model; + /* flags and min/max can be computed from model, but + * having them ready makes life easier. + */ + virDomainPCIConnectFlags flags; + size_t minSlot, maxSlot; /* usually 0,0 or 0,31, or 1,31 */ + /* Each bit in a slot represents one function on that slot. If the + * bit is set, that function is in use by a device. + */ + uint8_t slots[VIR_PCI_ADDRESS_SLOT_LAST + 1]; +} virDomainPCIAddressBus; +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 */ +}; +typedef struct _virDomainPCIAddressSet virDomainPCIAddressSet; +typedef virDomainPCIAddressSet *virDomainPCIAddressSetPtr; + typedef struct _virDomainPowerManagement virDomainPowerManagement; typedef virDomainPowerManagement *virDomainPowerManagementPtr; @@ -2554,6 +2594,7 @@ void virDomainTPMDefFree(virDomainTPMDefPtr def); void virDomainVirtioSerialControllerFree(virDomainVirtioSerialControllerPtr cont); void virDomainVirtioSerialAddrSetFree(virDomainVirtioSerialAddrSetPtr addrs); void virDomainCCWAddressSetFree(virDomainCCWAddressSetPtr addrs); +void virDomainPCIAddressSetFree(virDomainPCIAddressSetPtr addrs); typedef int (*virDomainDeviceInfoCallback)(virDomainDefPtr def, virDomainDeviceDefPtr dev, diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 75c8f70..a8109d7 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -107,7 +107,6 @@ virDomainPCIAddressReserveNextSlot; virDomainPCIAddressReserveSlot; virDomainPCIAddressSetAlloc; virDomainPCIAddressSetCreate; -virDomainPCIAddressSetFree; virDomainPCIAddressSetGrow; virDomainPCIAddressSlotInUse; virDomainPCIAddressValidate; @@ -427,6 +426,7 @@ virDomainOSTypeToString; virDomainParseMemory; virDomainPausedReasonTypeFromString; virDomainPausedReasonTypeToString; +virDomainPCIAddressSetFree; virDomainPMSuspendedReasonTypeFromString; virDomainPMSuspendedReasonTypeToString; virDomainRedirdevBusTypeFromString; -- 2.7.4 (Apple Git-66)
--- src/bhyve/bhyve_device.c | 16 +++------------- src/bhyve/bhyve_domain.c | 2 -- src/bhyve/bhyve_domain.h | 3 --- src/conf/domain_conf.c | 1 + src/conf/domain_conf.h | 1 + src/qemu/qemu_domain.c | 3 +-- src/qemu/qemu_domain.h | 2 -- src/qemu/qemu_domain_address.c | 22 ++++++++-------------- src/qemu/qemu_domain_address.h | 3 +-- src/qemu/qemu_hotplug.c | 15 +++++++-------- src/qemu/qemu_process.c | 6 +++--- tests/qemuhotplugtest.c | 2 +- 12 files changed, 26 insertions(+), 50 deletions(-) diff --git a/src/bhyve/bhyve_device.c b/src/bhyve/bhyve_device.c index 8373a5f..9ddd9aa 100644 --- a/src/bhyve/bhyve_device.c +++ b/src/bhyve/bhyve_device.c @@ -135,11 +135,9 @@ bhyveAssignDevicePCISlots(virDomainDefPtr def, return -1; } -int bhyveDomainAssignPCIAddresses(virDomainDefPtr def, - virDomainObjPtr obj) +int bhyveDomainAssignPCIAddresses(virDomainDefPtr def) { virDomainPCIAddressSetPtr addrs = NULL; - bhyveDomainObjPrivatePtr priv = NULL; int ret = -1; @@ -149,16 +147,8 @@ int bhyveDomainAssignPCIAddresses(virDomainDefPtr def, if (bhyveAssignDevicePCISlots(def, addrs) < 0) goto cleanup; - if (obj && obj->privateData) { - priv = obj->privateData; - if (addrs) { - virDomainPCIAddressSetFree(priv->pciaddrs); - priv->persistentAddrs = 1; - priv->pciaddrs = addrs; - } else { - priv->persistentAddrs = 0; - } - } + virDomainPCIAddressSetFree(def->pciaddrs); + def->pciaddrs = addrs; ret = 0; diff --git a/src/bhyve/bhyve_domain.c b/src/bhyve/bhyve_domain.c index 89cb171..81e5c8d 100644 --- a/src/bhyve/bhyve_domain.c +++ b/src/bhyve/bhyve_domain.c @@ -47,8 +47,6 @@ bhyveDomainObjPrivateFree(void *data) { bhyveDomainObjPrivatePtr priv = data; - virDomainPCIAddressSetFree(priv->pciaddrs); - VIR_FREE(priv); } diff --git a/src/bhyve/bhyve_domain.h b/src/bhyve/bhyve_domain.h index 0a60392..262605e 100644 --- a/src/bhyve/bhyve_domain.h +++ b/src/bhyve/bhyve_domain.h @@ -31,9 +31,6 @@ typedef struct _bhyveDomainObjPrivate bhyveDomainObjPrivate; typedef bhyveDomainObjPrivate *bhyveDomainObjPrivatePtr; struct _bhyveDomainObjPrivate { - virDomainPCIAddressSetPtr pciaddrs; - bool persistentAddrs; - bhyveMonitorPtr mon; }; diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 5f31a97..96731df 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2726,6 +2726,7 @@ void virDomainDefFree(virDomainDefPtr def) virDomainVirtioSerialAddrSetFree(def->vioserialaddrs); virDomainCCWAddressSetFree(def->ccwaddrs); + virDomainPCIAddressSetFree(def->pciaddrs); VIR_FREE(def); } diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 887e061..a1f14e0 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2319,6 +2319,7 @@ struct _virDomainDef { virDomainVirtioSerialAddrSetPtr vioserialaddrs; virDomainCCWAddressSetPtr ccwaddrs; + virDomainPCIAddressSetPtr pciaddrs; /* Application-specific custom metadata */ xmlNodePtr metadata; diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 5e3d305..afcb012 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -1247,7 +1247,6 @@ qemuDomainObjPrivateFree(void *data) virObjectUnref(priv->qemuCaps); virCgroupFree(&priv->cgroup); - virDomainPCIAddressSetFree(priv->pciaddrs); virDomainChrSourceDefFree(priv->monConfig); qemuDomainObjFreeJob(priv); VIR_FREE(priv->vcpupids); @@ -2433,7 +2432,7 @@ qemuDomainDefAssignAddresses(virDomainDef *def, def->emulator))) goto cleanup; - if (qemuDomainAssignAddresses(def, qemuCaps, NULL) < 0) + if (qemuDomainAssignAddresses(def, qemuCaps) < 0) goto cleanup; ret = 0; diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 4a15260..59b41de 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -187,8 +187,6 @@ struct _qemuDomainObjPrivate { int nvcpupids; int *vcpupids; - virDomainPCIAddressSetPtr pciaddrs; - virQEMUCapsPtr qemuCaps; char *lockState; diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c index 5d58eb1..fa42144 100644 --- a/src/qemu/qemu_domain_address.c +++ b/src/qemu/qemu_domain_address.c @@ -947,12 +947,10 @@ qemuDomainSupportsPCI(virDomainDefPtr def, static int qemuDomainAssignPCIAddresses(virDomainDefPtr def, - virQEMUCapsPtr qemuCaps, - virDomainObjPtr obj) + virQEMUCapsPtr qemuCaps) { int ret = -1; virDomainPCIAddressSetPtr addrs = NULL; - qemuDomainObjPrivatePtr priv = NULL; int max_idx = -1; int nbuses = 0; size_t i; @@ -1113,13 +1111,10 @@ qemuDomainAssignPCIAddresses(virDomainDefPtr def, } } - if (obj && obj->privateData) { - priv = obj->privateData; - /* if this is the live domain object, we persist the PCI addresses */ - virDomainPCIAddressSetFree(priv->pciaddrs); - priv->pciaddrs = addrs; - addrs = NULL; - } + /* we persist the PCI addresses */ + virDomainPCIAddressSetFree(def->pciaddrs); + def->pciaddrs = addrs; + addrs = NULL; ret = 0; @@ -1132,8 +1127,7 @@ qemuDomainAssignPCIAddresses(virDomainDefPtr def, int qemuDomainAssignAddresses(virDomainDefPtr def, - virQEMUCapsPtr qemuCaps, - virDomainObjPtr obj) + virQEMUCapsPtr qemuCaps) { if (virDomainAssignVirtioSerialAddresses(def) < 0) return -1; @@ -1146,7 +1140,7 @@ qemuDomainAssignAddresses(virDomainDefPtr def, qemuDomainAssignARMVirtioMMIOAddresses(def, qemuCaps); - if (qemuDomainAssignPCIAddresses(def, qemuCaps, obj) < 0) + if (qemuDomainAssignPCIAddresses(def, qemuCaps) < 0) return -1; return 0; @@ -1170,7 +1164,7 @@ qemuDomainReleaseDeviceAddress(virDomainObjPtr vm, VIR_WARN("Unable to release CCW address on %s", NULLSTR(devstr)); else if (virDeviceInfoPCIAddressPresent(info) && - virDomainPCIAddressReleaseSlot(priv->pciaddrs, + virDomainPCIAddressReleaseSlot(vm->def->pciaddrs, &info->addr.pci) < 0) VIR_WARN("Unable to release PCI address on %s", NULLSTR(devstr)); diff --git a/src/qemu/qemu_domain_address.h b/src/qemu/qemu_domain_address.h index 50019b8..03b99d9 100644 --- a/src/qemu/qemu_domain_address.h +++ b/src/qemu/qemu_domain_address.h @@ -32,8 +32,7 @@ int qemuDomainSetSCSIControllerModel(const virDomainDef *def, int *model); int qemuDomainAssignAddresses(virDomainDefPtr def, - virQEMUCapsPtr qemuCaps, - virDomainObjPtr obj) + virQEMUCapsPtr qemuCaps) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); void qemuDomainReleaseDeviceAddress(virDomainObjPtr vm, diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index bed4173..46e2143 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -332,7 +332,7 @@ qemuDomainAttachVirtioDiskDevice(virConnectPtr conn, goto error; } else if (!disk->info.type || disk->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) { - if (virDomainPCIAddressEnsureAddr(priv->pciaddrs, &disk->info) < 0) + if (virDomainPCIAddressEnsureAddr(vm->def->pciaddrs, &disk->info) < 0) goto error; } releaseaddr = true; @@ -454,7 +454,7 @@ int qemuDomainAttachControllerDevice(virQEMUDriverPtr driver, if (controller->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE || controller->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) { - if (virDomainPCIAddressEnsureAddr(priv->pciaddrs, &controller->info) < 0) + if (virDomainPCIAddressEnsureAddr(vm->def->pciaddrs, &controller->info) < 0) goto cleanup; } else if (controller->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW) { if (virDomainCCWAddressAssign(&controller->info, vm->def->ccwaddrs, @@ -951,7 +951,7 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver, virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("virtio-s390 net device cannot be hotplugged.")); goto cleanup; - } else if (virDomainPCIAddressEnsureAddr(priv->pciaddrs, &net->info) < 0) { + } else if (virDomainPCIAddressEnsureAddr(vm->def->pciaddrs, &net->info) < 0) { goto cleanup; } @@ -1230,7 +1230,7 @@ qemuDomainAttachHostPCIDevice(virQEMUDriverPtr driver, if (qemuAssignDeviceHostdevAlias(vm->def, &hostdev->info->alias, -1) < 0) goto error; - if (virDomainPCIAddressEnsureAddr(priv->pciaddrs, hostdev->info) < 0) + if (virDomainPCIAddressEnsureAddr(vm->def->pciaddrs, hostdev->info) < 0) goto error; releaseaddr = true; if (backend != VIR_DOMAIN_HOSTDEV_PCI_BACKEND_VFIO && @@ -1458,7 +1458,6 @@ qemuDomainChrRemove(virDomainDefPtr vmdef, static int qemuDomainAttachChrDeviceAssignAddr(virDomainDefPtr vmdef, - qemuDomainObjPrivatePtr priv, virDomainChrDefPtr chr) { if (chr->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CONSOLE && @@ -1470,7 +1469,7 @@ qemuDomainAttachChrDeviceAssignAddr(virDomainDefPtr vmdef, } else if (chr->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_SERIAL && chr->targetType == VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_PCI) { - if (virDomainPCIAddressEnsureAddr(priv->pciaddrs, &chr->info) < 0) + if (virDomainPCIAddressEnsureAddr(vmdef->pciaddrs, &chr->info) < 0) return -1; return 1; @@ -1510,7 +1509,7 @@ int qemuDomainAttachChrDevice(virQEMUDriverPtr driver, if (qemuAssignDeviceChrAlias(vmdef, chr, -1) < 0) goto cleanup; - if ((rc = qemuDomainAttachChrDeviceAssignAddr(vmdef, priv, chr)) < 0) + if ((rc = qemuDomainAttachChrDeviceAssignAddr(vmdef, chr)) < 0) goto cleanup; if (rc == 1) need_release = true; @@ -1591,7 +1590,7 @@ qemuDomainAttachRNGDevice(virQEMUDriverPtr driver, if (rng->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE || rng->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) { - if (virDomainPCIAddressEnsureAddr(priv->pciaddrs, &rng->info) < 0) + if (virDomainPCIAddressEnsureAddr(vm->def->pciaddrs, &rng->info) < 0) return -1; } else if (rng->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW) { if (virDomainCCWAddressAssign(&rng->info, vm->def->ccwaddrs, diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 373e7a9..f107193 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -3291,7 +3291,7 @@ qemuProcessReconnect(void *opaque) goto cleanup; } - if ((qemuDomainAssignAddresses(obj->def, priv->qemuCaps, obj)) < 0) + if ((qemuDomainAssignAddresses(obj->def, priv->qemuCaps)) < 0) goto error; /* if domain requests security driver we haven't loaded, report error, but @@ -4878,7 +4878,7 @@ qemuProcessPrepareDomain(virConnectPtr conn, * use in hotplug */ VIR_DEBUG("Assigning domain PCI addresses"); - if ((qemuDomainAssignAddresses(vm->def, priv->qemuCaps, vm)) < 0) + if ((qemuDomainAssignAddresses(vm->def, priv->qemuCaps)) < 0) goto cleanup; if (qemuAssignDeviceAliases(vm->def, priv->qemuCaps) < 0) @@ -6066,7 +6066,7 @@ int qemuProcessAttach(virConnectPtr conn ATTRIBUTE_UNUSED, * use in hotplug */ VIR_DEBUG("Assigning domain PCI addresses"); - if ((qemuDomainAssignAddresses(vm->def, priv->qemuCaps, vm)) < 0) + if ((qemuDomainAssignAddresses(vm->def, priv->qemuCaps)) < 0) goto error; if ((timestamp = virTimeStringNow()) == NULL) diff --git a/tests/qemuhotplugtest.c b/tests/qemuhotplugtest.c index 13055ab..6d90a0c 100644 --- a/tests/qemuhotplugtest.c +++ b/tests/qemuhotplugtest.c @@ -86,7 +86,7 @@ qemuHotplugCreateObjects(virDomainXMLOptionPtr xmlopt, VIR_DOMAIN_DEF_PARSE_INACTIVE))) goto cleanup; - if (qemuDomainAssignAddresses((*vm)->def, priv->qemuCaps, *vm) < 0) + if (qemuDomainAssignAddresses((*vm)->def, priv->qemuCaps) < 0) goto cleanup; if (qemuAssignDeviceAliases((*vm)->def, priv->qemuCaps) < 0) -- 2.7.4 (Apple Git-66)
On 06/19/2016 08:18 PM, Tomasz Flendrich wrote:
This patch depends on another one, because it was created on top of the changes in the other one. Link to the dependency: https://www.redhat.com/archives/libvir-list/2016-June/msg01227.html
This is the next patch of the ongoing process of abstracting device address generation.
In this patch, addres sets are moved from QEMU's (ccw and pci) and bhyve's (pci only) private data to domainDef. In the future, it's likely that more hypervisors will add the feature of explicitly specifying the device addresses, so handling the addresses should be made more generic.
Apologies if I'm missing something, I didn't look too closely at this series, however have you seen this thread? http://www.redhat.com/archives/libvir-list/2016-May/msg01071.html My understanding of the current code is that the cached vioserial/ccw/pciaddrs lists in qemu aren't actually required... they were at one point to handle older qemu, but we dropped that support. Maybe you can pick up my patches and finish off dropping of pciaddrs and ccwaddrs? I suspect the pciaddrs cache in bhyve can be dropped as well, I don't think it was ever strictly required, the code just followed the qemu example That said I think it makes sense in general to move this generic stuff out of qemu since it's generic code as you say, but better to remove the obsolete stuff first IMO Thanks, Cole
Apologies if I'm missing something, I didn't look too closely at this series, however have you seen this thread?
http://www.redhat.com/archives/libvir-list/2016-May/msg01071.html I haven’t noticed that some work has been done on that, thank you!
My understanding of the current code is that the cached vioserial/ccw/pciaddrs lists in qemu aren't actually required…they were at one point to handle
older qemu, but we dropped that support. Maybe you can pick up my patches and finish off dropping of pciaddrs and ccwaddrs? I suspect the pciaddrs cache in bhyve can be dropped as well, I don't think it was ever strictly required, the code just followed the qemu example If we could do without the caching, it would make the current code simpler. There wouldn’t be those booleans in qemu_hotplug.c that remember whether an address has to be deleted or not in case something fails. We could delete qemuDomainReleaseDeviceAddress() and a few more functions.
I examined vioserial and pci addresses and it looks like it could be done. However, I'm not an expert on qemu_hotplug yet and this is where the interesting stuff happens with addresses, so I am not entirely sure yet. I also don't know what the plans are for device addresses in the future. Perhaps there are some features that will require caching them. I think that recalculation may change the current behavior of ccw addresses. Function virDomainCCWAddressReleaseAddr() modifies addrs->next only when the address being released was the address most recently assigned. Laine, you know a lot about PCI addresses and you also mentioned that you want to modify them in the future. What do you think? Martin, should I work on that instead? Kind regards, Tomasz
On 06/20/2016 10:26 PM, Tomasz Flendrich wrote:
Apologies if I'm missing something, I didn't look too closely at this series, however have you seen this thread?
http://www.redhat.com/archives/libvir-list/2016-May/msg01071.html I haven’t noticed that some work has been done on that, thank you!
My understanding of the current code is that the cached vioserial/ccw/pciaddrs lists in qemu aren't actually required…they were at one point to handle
older qemu, but we dropped that support. Maybe you can pick up my patches and finish off dropping of pciaddrs and ccwaddrs? I suspect the pciaddrs cache in bhyve can be dropped as well, I don't think it was ever strictly required, the code just followed the qemu example If we could do without the caching, it would make the current code simpler. There wouldn’t be those booleans in qemu_hotplug.c that remember whether an address has to be deleted or not in case something fails. We could delete qemuDomainReleaseDeviceAddress() and a few more functions.
I examined vioserial and pci addresses and it looks like it could be done. However, I'm not an expert on qemu_hotplug yet and this is where the interesting stuff happens with addresses, so I am not entirely sure yet. I also don't know what the plans are for device addresses in the future. Perhaps there are some features that will require caching them.
In a way they are currently cached twice: once in these lists, and once in the runtime VM XML. That's what my vioserial series did, was replace the hotplug usage of the cached list with just fetching it on demand from the running VM XML, so at least at a high level these cached lists seem redundant because the info is tracked elsewhere.
I think that recalculation may change the current behavior of ccw addresses. Function virDomainCCWAddressReleaseAddr() modifies addrs->next only when the address being released was the address most recently assigned.
Hmm, that's interesting. Not sure what that is about. I tried adding this diff: diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c index 794270d..7d39b69 100644 --- a/src/conf/domain_addr.c +++ b/src/conf/domain_addr.c @@ -792,14 +792,7 @@ virDomainCCWAddressReleaseAddr(virDomainCCWAddressSetPtr addrs, if (!addr) return -1; - if ((ret = virHashRemoveEntry(addrs->defined, addr)) == 0 && - dev->addr.ccw.cssid == addrs->next.cssid && - dev->addr.ccw.ssid == addrs->next.ssid && - dev->addr.ccw.devno < addrs->next.devno) { - addrs->next.devno = dev->addr.ccw.devno; - addrs->next.assigned = false; - } - + ret = virHashRemoveEntry(addrs->defined, addr); VIR_FREE(addr); return ret; And the test suite doesn't break, so at least it's not something completely obvious. But then again this seems to only be via hotunplug call path, and our test suite coverage for hotplug isn't that great. This logic may in fact just be an artifact of maintaining a persistent set of addresses. If we are generating an address set on demand from the runtime XML, we probably don't need to worry about 'releasing' an address from that set at all, so it may just side step whatever issue this logic was trying to address.
Laine, you know a lot about PCI addresses and you also mentioned that you want to modify them in the future. What do you think?
Laine did have some patches that did touch some bits in this area, but only as a side effect. Thanks, Cole
On 06/20/2016 10:26 PM, Tomasz Flendrich wrote:
Apologies if I'm missing something, I didn't look too closely at this series, however have you seen this thread?
http://www.redhat.com/archives/libvir-list/2016-May/msg01071.html I haven’t noticed that some work has been done on that, thank you!
My understanding of the current code is that the cached vioserial/ccw/pciaddrs lists in qemu aren't actually required…they were at one point to handle older qemu, but we dropped that support. Maybe you can pick up my patches and finish off dropping of pciaddrs and ccwaddrs? I suspect the pciaddrs cache in bhyve can be dropped as well, I don't think it was ever strictly required, the code just followed the qemu example If we could do without the caching, it would make the current code simpler. There wouldn’t be those booleans in qemu_hotplug.c that remember whether an address has to be deleted or not in case something fails. We could delete qemuDomainReleaseDeviceAddress() and a few more functions.
I'm completely ignorant about vioserial and ccw. As far as the "cache" for pci addresses, I guess whether or not we want the cache depends completely on how long it takes to reconstruct vs. how often a device is added. There is also the issue of the mismatch between live and config devices' address use, and the inconsistency that can be caused by that (if you hotplug a device with --live, then hotplug another with --live --config, then the 2nd device will have the same address in config as the first has in the live state of the guest (more importantly, the address of the 2nd device will change the next time the domain is shutdown and restarted, which all of this address assignment stuff is intended to avoid) - I don't know if that problem would be more easily solved by a cache that is used for assigning addresses for both --live and --config, or if, as Cole suggests, it would be better just to remove the cache and rebuild the allocation table each time a new device is added (this would mean that we would need to scan through all the live devices *and* persistent devices to re-populate it)
I examined vioserial and pci addresses and it looks like it could be done. However, I'm not an expert on qemu_hotplug yet and this is where the interesting stuff happens with addresses, so I am not entirely sure yet. I also don't know what the plans are for device addresses in the future. Perhaps there are some features that will require caching them.
I think that recalculation may change the current behavior of ccw addresses. Function virDomainCCWAddressReleaseAddr() modifies addrs->next only when the address being released was the address most recently assigned.
Laine, you know a lot about PCI addresses and you also mentioned that you want to modify them in the future. What do you think?
"in the future" sounds so distant :-). Within the next week I will be modifiying the PCI address allocation to eliminate the auto-adding of pci-bridge devices whenever there aren't enough free slots, and instead add a new controller of the appropriate type for each extra slot that is needed. I haven't been able to concentrate enough to determine if that is going to cause any merge conflict with what you're going to do.
Martin, should I work on that instead?
Kind regards, Tomasz
On 06/21/2016 10:08 PM, Laine Stump wrote:
On 06/20/2016 10:26 PM, Tomasz Flendrich wrote:
Apologies if I'm missing something, I didn't look too closely at this series, however have you seen this thread?
http://www.redhat.com/archives/libvir-list/2016-May/msg01071.html I haven’t noticed that some work has been done on that, thank you!
My understanding of the current code is that the cached vioserial/ccw/pciaddrs lists in qemu aren't actually required…they were at one point to handle older qemu, but we dropped that support. Maybe you can pick up my patches and finish off dropping of pciaddrs and ccwaddrs? I suspect the pciaddrs cache in bhyve can be dropped as well, I don't think it was ever strictly required, the code just followed the qemu example If we could do without the caching, it would make the current code simpler. There wouldn’t be those booleans in qemu_hotplug.c that remember whether an address has to be deleted or not in case something fails. We could delete qemuDomainReleaseDeviceAddress() and a few more functions.
I'm completely ignorant about vioserial and ccw. As far as the "cache" for pci addresses, I guess whether or not we want the cache depends completely on how long it takes to reconstruct vs. how often a device is added.
Constructing the cache likely takes less time than parsing a single XML document... it's just iterating over the parsed XML in memory and performing the collision checks. There is also
the issue of the mismatch between live and config devices' address use, and the inconsistency that can be caused by that (if you hotplug a device with --live, then hotplug another with --live --config, then the 2nd device will have the same address in config as the first has in the live state of the guest (more importantly, the address of the 2nd device will change the next time the domain is shutdown and restarted, which all of this address assignment stuff is intended to avoid) - I don't know if that problem would be more easily solved by a cache that is used for assigning addresses for both --live and --config, or if, as Cole suggests, it would be better just to remove the cache and rebuild the allocation table each time a new device is added (this would mean that we would need to scan through all the live devices *and* persistent devices to re-populate it)
For 'config' updates we don't even use the address cache presently, at least for vioserialaddr, I didn't confirm for the other ones. It's only used for hotplug. For 'config' updates we basically just insert the device into the XML and effectively 'redefine' it, and the generic XML parsing machinery and domain callbacks assign addresses. The address list is only needed for runtime hotplug because we need to generate an address on demand, for passing to the qemu monitor commands. Thanks, Cole
On Wed, Jun 22, 2016 at 07:24:41AM -0400, Cole Robinson wrote:
On 06/21/2016 10:08 PM, Laine Stump wrote:
On 06/20/2016 10:26 PM, Tomasz Flendrich wrote:
Apologies if I'm missing something, I didn't look too closely at this series, however have you seen this thread?
http://www.redhat.com/archives/libvir-list/2016-May/msg01071.html I haven’t noticed that some work has been done on that, thank you!
Me neither, I'm kind of busier nowadays (it's 7pm and I'm yet again sitting at work even though I'm now temporarily working part-time). Sorry for that, we discussed Tomasz's work before that and I must've missed it after.
My understanding of the current code is that the cached vioserial/ccw/pciaddrs lists in qemu aren't actually required…they were at one point to handle older qemu, but we dropped that support. Maybe you can pick up my patches and finish off dropping of pciaddrs and ccwaddrs? I suspect the pciaddrs cache in bhyve can be dropped as well, I don't think it was ever strictly required, the code just followed the qemu example If we could do without the caching, it would make the current code simpler. There wouldn’t be those booleans in qemu_hotplug.c that remember whether an address has to be deleted or not in case something fails. We could delete qemuDomainReleaseDeviceAddress() and a few more functions.
I'm completely ignorant about vioserial and ccw. As far as the "cache" for pci addresses, I guess whether or not we want the cache depends completely on how long it takes to reconstruct vs. how often a device is added.
Constructing the cache likely takes less time than parsing a single XML document... it's just iterating over the parsed XML in memory and performing the collision checks.
I'm 100% for dropping any unnecessary data storage, especially if it is used wrong anyway. @Cole: it looks like there was a discussion on your series but nobody actually reviewed it. Looking at it it's pretty trivial but there are no tests. So I propose that Tomasz could pick up your work with the addition of tests that would make sure the hotplug behaviour is kept (or even fixed if there is a problem) and he would continue with other addresses afterwards. Is that OK with you or do you have some deeper plans with this area of code?
There is also
the issue of the mismatch between live and config devices' address use, and the inconsistency that can be caused by that (if you hotplug a device with --live, then hotplug another with --live --config, then the 2nd device will have the same address in config as the first has in the live state of the guest (more importantly, the address of the 2nd device will change the next time the domain is shutdown and restarted, which all of this address assignment stuff is intended to avoid) - I don't know if that problem would be more easily solved by a cache that is used for assigning addresses for both --live and --config, or if, as Cole suggests, it would be better just to remove the cache and rebuild the allocation table each time a new device is added (this would mean that we would need to scan through all the live devices *and* persistent devices to re-populate it)
For 'config' updates we don't even use the address cache presently, at least for vioserialaddr, I didn't confirm for the other ones. It's only used for hotplug. For 'config' updates we basically just insert the device into the XML and effectively 'redefine' it, and the generic XML parsing machinery and domain callbacks assign addresses.
The address list is only needed for runtime hotplug because we need to generate an address on demand, for passing to the qemu monitor commands.
We'll take that issue into account and will work towards fixing that as a part of our work. It looks like the only harder thing will be making sure the code is not total mess when assigning the addresses, especially since it needs to work with two different definitions together.
Thanks, Cole
On 06/22/2016 01:04 PM, Martin Kletzander wrote:
On Wed, Jun 22, 2016 at 07:24:41AM -0400, Cole Robinson wrote:
On 06/21/2016 10:08 PM, Laine Stump wrote:
On 06/20/2016 10:26 PM, Tomasz Flendrich wrote:
Apologies if I'm missing something, I didn't look too closely at this series, however have you seen this thread?
http://www.redhat.com/archives/libvir-list/2016-May/msg01071.html I haven’t noticed that some work has been done on that, thank you!
Me neither, I'm kind of busier nowadays (it's 7pm and I'm yet again sitting at work even though I'm now temporarily working part-time). Sorry for that, we discussed Tomasz's work before that and I must've missed it after.
My understanding of the current code is that the cached vioserial/ccw/pciaddrs lists in qemu aren't actually required…they were at one point to handle older qemu, but we dropped that support. Maybe you can pick up my patches and finish off dropping of pciaddrs and ccwaddrs? I suspect the pciaddrs cache in bhyve can be dropped as well, I don't think it was ever strictly required, the code just followed the qemu example If we could do without the caching, it would make the current code simpler. There wouldn’t be those booleans in qemu_hotplug.c that remember whether an address has to be deleted or not in case something fails. We could delete qemuDomainReleaseDeviceAddress() and a few more functions.
I'm completely ignorant about vioserial and ccw. As far as the "cache" for pci addresses, I guess whether or not we want the cache depends completely on how long it takes to reconstruct vs. how often a device is added.
Constructing the cache likely takes less time than parsing a single XML document... it's just iterating over the parsed XML in memory and performing the collision checks.
I'm 100% for dropping any unnecessary data storage, especially if it is used wrong anyway. @Cole: it looks like there was a discussion on your series but nobody actually reviewed it. Looking at it it's pretty trivial but there are no tests. So I propose that Tomasz could pick up your work with the addition of tests that would make sure the hotplug behaviour is kept (or even fixed if there is a problem) and he would continue with other addresses afterwards. Is that OK with you or do you have some deeper plans with this area of code?
No deeper plans and I wasn't expecting to get back to it any time soon. The original posting was mostly to start a discussion about whether the caching served any purpose that I was missing, so there was consensus that it was safe for removal before anyone took it any further. Adding the hotplug tests is definitely a good idea too. I suggest getting some hotplug tests for the current vioserial state, then make sure my patches don't cause any regressions, then add a patch on top that removes all the now unused vioserial addresses I alluded to in that series cover letter. I'm happy to help with review and IRC discussion too if needed.
There is also
the issue of the mismatch between live and config devices' address use, and the inconsistency that can be caused by that (if you hotplug a device with --live, then hotplug another with --live --config, then the 2nd device will have the same address in config as the first has in the live state of the guest (more importantly, the address of the 2nd device will change the next time the domain is shutdown and restarted, which all of this address assignment stuff is intended to avoid) - I don't know if that problem would be more easily solved by a cache that is used for assigning addresses for both --live and --config, or if, as Cole suggests, it would be better just to remove the cache and rebuild the allocation table each time a new device is added (this would mean that we would need to scan through all the live devices *and* persistent devices to re-populate it)
For 'config' updates we don't even use the address cache presently, at least for vioserialaddr, I didn't confirm for the other ones. It's only used for hotplug. For 'config' updates we basically just insert the device into the XML and effectively 'redefine' it, and the generic XML parsing machinery and domain callbacks assign addresses.
The address list is only needed for runtime hotplug because we need to generate an address on demand, for passing to the qemu monitor commands.
We'll take that issue into account and will work towards fixing that as a part of our work. It looks like the only harder thing will be making sure the code is not total mess when assigning the addresses, especially since it needs to work with two different definitions together.
Most of that stuff is already in place in the current code, there's completely separate code paths for config vs online hotplug. If you look at my patch #2 from that series the hotplug changes are pretty minimal actually http://www.redhat.com/archives/libvir-list/2016-May/msg01073.html There may still be unexpected hurdles that I missed, but I'm guessing not. The hardest part may in fact be writing the hotplug test cases :) - Cole
On Wed, Jun 22, 2016 at 01:14:18PM -0400, Cole Robinson wrote:
On 06/22/2016 01:04 PM, Martin Kletzander wrote:
On Wed, Jun 22, 2016 at 07:24:41AM -0400, Cole Robinson wrote:
On 06/21/2016 10:08 PM, Laine Stump wrote:
On 06/20/2016 10:26 PM, Tomasz Flendrich wrote:
Apologies if I'm missing something, I didn't look too closely at this series, however have you seen this thread?
http://www.redhat.com/archives/libvir-list/2016-May/msg01071.html I haven’t noticed that some work has been done on that, thank you!
Me neither, I'm kind of busier nowadays (it's 7pm and I'm yet again sitting at work even though I'm now temporarily working part-time). Sorry for that, we discussed Tomasz's work before that and I must've missed it after.
My understanding of the current code is that the cached vioserial/ccw/pciaddrs lists in qemu aren't actually required…they were at one point to handle older qemu, but we dropped that support. Maybe you can pick up my patches and finish off dropping of pciaddrs and ccwaddrs? I suspect the pciaddrs cache in bhyve can be dropped as well, I don't think it was ever strictly required, the code just followed the qemu example If we could do without the caching, it would make the current code simpler. There wouldn’t be those booleans in qemu_hotplug.c that remember whether an address has to be deleted or not in case something fails. We could delete qemuDomainReleaseDeviceAddress() and a few more functions.
I'm completely ignorant about vioserial and ccw. As far as the "cache" for pci addresses, I guess whether or not we want the cache depends completely on how long it takes to reconstruct vs. how often a device is added.
Constructing the cache likely takes less time than parsing a single XML document... it's just iterating over the parsed XML in memory and performing the collision checks.
I'm 100% for dropping any unnecessary data storage, especially if it is used wrong anyway. @Cole: it looks like there was a discussion on your series but nobody actually reviewed it. Looking at it it's pretty trivial but there are no tests. So I propose that Tomasz could pick up your work with the addition of tests that would make sure the hotplug behaviour is kept (or even fixed if there is a problem) and he would continue with other addresses afterwards. Is that OK with you or do you have some deeper plans with this area of code?
No deeper plans and I wasn't expecting to get back to it any time soon. The original posting was mostly to start a discussion about whether the caching served any purpose that I was missing, so there was consensus that it was safe for removal before anyone took it any further.
Adding the hotplug tests is definitely a good idea too. I suggest getting some hotplug tests for the current vioserial state, then make sure my patches don't cause any regressions, then add a patch on top that removes all the now unused vioserial addresses I alluded to in that series cover letter. I'm happy to help with review and IRC discussion too if needed.
Great, that's what I meant, we'll test-drive develop the heck out of it ;)
There is also
the issue of the mismatch between live and config devices' address use, and the inconsistency that can be caused by that (if you hotplug a device with --live, then hotplug another with --live --config, then the 2nd device will have the same address in config as the first has in the live state of the guest (more importantly, the address of the 2nd device will change the next time the domain is shutdown and restarted, which all of this address assignment stuff is intended to avoid) - I don't know if that problem would be more easily solved by a cache that is used for assigning addresses for both --live and --config, or if, as Cole suggests, it would be better just to remove the cache and rebuild the allocation table each time a new device is added (this would mean that we would need to scan through all the live devices *and* persistent devices to re-populate it)
For 'config' updates we don't even use the address cache presently, at least for vioserialaddr, I didn't confirm for the other ones. It's only used for hotplug. For 'config' updates we basically just insert the device into the XML and effectively 'redefine' it, and the generic XML parsing machinery and domain callbacks assign addresses.
The address list is only needed for runtime hotplug because we need to generate an address on demand, for passing to the qemu monitor commands.
We'll take that issue into account and will work towards fixing that as a part of our work. It looks like the only harder thing will be making sure the code is not total mess when assigning the addresses, especially since it needs to work with two different definitions together.
Most of that stuff is already in place in the current code, there's completely separate code paths for config vs online hotplug. If you look at my patch #2
That's the problem, the address will need to be created before the codes go apart. Maybe just iterating over both definitions with the callback and then finding the first free address will be enough. We'll see. Thanks for all the ideas.
from that series the hotplug changes are pretty minimal actually
http://www.redhat.com/archives/libvir-list/2016-May/msg01073.html
There may still be unexpected hurdles that I missed, but I'm guessing not. The hardest part may in fact be writing the hotplug test cases :)
- Cole
participants (4)
-
Cole Robinson -
Laine Stump -
Martin Kletzander -
Tomasz Flendrich