[libvirt] [PATCH 0/5] Allocate virtio-serial addresses

Instead of simply incrementing the port, respect maximum port values and use multiple controllers. Ján Tomko (5): Add test for virtio serial port assignment Add functions to track virtio-serial addresses Allocate virtio-serial addresses when starting a domain Expand the address set when attaching a virtio-serial controller Assign an address when hotplugging a virtio-serial device src/conf/domain_addr.c | 382 +++++++++++++++++++++ src/conf/domain_addr.h | 45 +++ src/conf/domain_conf.c | 29 -- src/libvirt_private.syms | 8 + src/qemu/qemu_command.c | 63 ++++ src/qemu/qemu_domain.c | 1 + src/qemu/qemu_domain.h | 1 + src/qemu/qemu_hotplug.c | 32 +- src/qemu/qemu_process.c | 2 + tests/qemuhotplugtest.c | 2 +- .../qemuxml2argv-channel-virtio-auto.args | 8 +- .../qemuxml2argv-channel-virtio-autoassign.args | 20 ++ .../qemuxml2argv-channel-virtio-autoassign.xml | 50 +++ tests/qemuxml2argvtest.c | 2 + .../qemuxml2xmlout-channel-virtio-auto.xml | 10 +- 15 files changed, 614 insertions(+), 41 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-channel-virtio-autoassign.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-channel-virtio-autoassign.xml -- 2.0.5

Add a test to demonstrate the effect of this series. --- .../qemuxml2argv-channel-virtio-autoassign.args | 20 +++++++++ .../qemuxml2argv-channel-virtio-autoassign.xml | 50 ++++++++++++++++++++++ tests/qemuxml2argvtest.c | 2 + 3 files changed, 72 insertions(+) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-channel-virtio-autoassign.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-channel-virtio-autoassign.xml diff --git a/tests/qemuxml2argvdata/qemuxml2argv-channel-virtio-autoassign.args b/tests/qemuxml2argvdata/qemuxml2argv-channel-virtio-autoassign.args new file mode 100644 index 0000000..d64a228 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-channel-virtio-autoassign.args @@ -0,0 +1,20 @@ +LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test QEMU_AUDIO_DRV=none \ +/usr/bin/qemu -S -M pc -m 214 -smp 1 -nographic -nodefconfig -nodefaults \ +-chardev socket,id=charmonitor,path=/tmp/test-monitor,server,nowait \ +-mon chardev=charmonitor,id=monitor,mode=readline -no-acpi -boot c \ +-device virtio-serial-pci,id=virtio-serial0,max_ports=4,vectors=4,bus=pci.0\ +,addr=0x3 -device virtio-serial-pci,id=virtio-serial1,bus=pci.0,addr=0xa \ +-usb -hda /dev/HostVG/QEMUGuest1 \ +-chardev pty,id=charchannel0 -device virtserialport,bus=virtio-serial0.0,nr=1,\ +chardev=charchannel0,id=channel0,name=org.linux-kvm.port.0 \ +-chardev pty,id=charchannel1 -device virtserialport,bus=virtio-serial0.0,nr=2,\ +chardev=charchannel1,id=channel1,name=org.linux-kvm.port.foo \ +-chardev pty,id=charchannel2 -device virtserialport,bus=virtio-serial0.0,nr=1,\ +chardev=charchannel2,id=channel2,name=org.linux-kvm.port.bar \ +-chardev pty,id=charchannel3 -device virtserialport,bus=virtio-serial0.2,nr=1,\ +chardev=charchannel3,id=channel3,name=org.linux-kvm.port.wizz \ +-chardev pty,id=charchannel4 -device virtserialport,bus=virtio-serial0.0,nr=3,\ +chardev=charchannel4,id=channel4,name=org.linux-kvm.port.ooh \ +-chardev pty,id=charchannel5 -device virtserialport,bus=virtio-serial0.0,nr=4,\ +chardev=charchannel5,id=channel5,name=org.linux-kvm.port.lla \ +-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x4 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-channel-virtio-autoassign.xml b/tests/qemuxml2argvdata/qemuxml2argv-channel-virtio-autoassign.xml new file mode 100644 index 0000000..ac0744e --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-channel-virtio-autoassign.xml @@ -0,0 +1,50 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219100</memory> + <currentMemory unit='KiB'>219100</currentMemory> + <vcpu placement='static' cpuset='1-4,8-20,525'>1</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + <disk type='block' device='disk'> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='hda' bus='ide'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> + <controller type='usb' index='0'/> + <controller type='ide' index='0'/> + <controller type='virtio-serial' index='0' ports='4' vectors='4'/> + <controller type='virtio-serial' index='1'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x0a' function='0x0'/> + </controller> + <channel type='pty'> + <target type='virtio' name='org.linux-kvm.port.0'/> + </channel> + <channel type='pty'> + <target type='virtio' name='org.linux-kvm.port.foo'/> + </channel> + <channel type='pty'> + <target type='virtio' name='org.linux-kvm.port.bar'/> + <address type='virtio-serial' controller='0' port='1'/> + </channel> + <channel type='pty'> + <target type='virtio' name='org.linux-kvm.port.wizz'/> + <address type='virtio-serial' controller='0' bus='2'/> + </channel> + <channel type='pty'> + <target type='virtio' name='org.linux-kvm.port.ooh'/> + </channel> + <channel type='pty'> + <target type='virtio' name='org.linux-kvm.port.lla'/> + </channel> + <memballoon model='virtio'/> + </devices> +</domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 681f8fe..41cadb6 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -1064,6 +1064,8 @@ mymain(void) QEMU_CAPS_DEVICE, QEMU_CAPS_CHARDEV, QEMU_CAPS_NODEFCONFIG); DO_TEST("channel-virtio-auto", QEMU_CAPS_DEVICE, QEMU_CAPS_CHARDEV, QEMU_CAPS_NODEFCONFIG); + DO_TEST("channel-virtio-autoassign", + QEMU_CAPS_DEVICE, QEMU_CAPS_CHARDEV, QEMU_CAPS_NODEFCONFIG); DO_TEST("console-virtio", QEMU_CAPS_DEVICE, QEMU_CAPS_CHARDEV, QEMU_CAPS_NODEFCONFIG); DO_TEST("console-virtio-many", -- 2.0.5

On Tue, Mar 03, 2015 at 15:44:26 +0100, Ján Tomko wrote:
Add a test to demonstrate the effect of this series. --- .../qemuxml2argv-channel-virtio-autoassign.args | 20 +++++++++ .../qemuxml2argv-channel-virtio-autoassign.xml | 50 ++++++++++++++++++++++ tests/qemuxml2argvtest.c | 2 + 3 files changed, 72 insertions(+) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-channel-virtio-autoassign.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-channel-virtio-autoassign.xml
ACK, Peter

Store the available ports of a virtio-serial controller in a virBitmap. The bitmaps are stored in a hash table - the controller index formatted as a string. Buses are not tracked, because they aren't supported by QEMU. --- src/conf/domain_addr.c | 382 +++++++++++++++++++++++++++++++++++++++++++++++ src/conf/domain_addr.h | 45 ++++++ src/libvirt_private.syms | 8 + 3 files changed, 435 insertions(+) diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c index fb4a76f..654c95a 100644 --- a/src/conf/domain_addr.c +++ b/src/conf/domain_addr.c @@ -718,3 +718,385 @@ virDomainCCWAddressSetCreate(void) virDomainCCWAddressSetFree(addrs); return NULL; } + + +#define VIR_DOMAIN_DEFAULT_VIRTIO_SERIAL_PORTS 31 + + +static void +virDomainVirtioSerialAddrHashValueFree(void *value, + const void *name ATTRIBUTE_UNUSED) +{ + virBitmapPtr map = value; + + virBitmapFree(map); +} + +/* virDomainVirtioSerialAddrSetCreate + * + * Allocates an address set for virtio serial addresses + */ +virDomainVirtioSerialAddrSetPtr +virDomainVirtioSerialAddrSetCreate(void) +{ + virDomainVirtioSerialAddrSetPtr ret = NULL; + + if (VIR_ALLOC(ret) < 0) + goto error; + + if (!(ret->used = virHashCreate(9, virDomainVirtioSerialAddrHashValueFree))) + goto error; + + return ret; + + error: + virDomainVirtioSerialAddrSetFree(ret); + return NULL; +} + +/* virDomainVirtioSerialAddrSetAddController + * + * Adds virtio serial ports of the existing controller + * to the address set. + */ +int +virDomainVirtioSerialAddrSetAddController(virDomainVirtioSerialAddrSetPtr addrs, + virDomainControllerDefPtr cont) +{ + virBitmapPtr map = NULL; + char *str = NULL; + int ret = -1; + int ports; + + if (cont->type != VIR_DOMAIN_CONTROLLER_TYPE_VIRTIO_SERIAL) + return 0; + + ports = cont->opts.vioserial.ports; + if (ports == -1) + ports = VIR_DOMAIN_DEFAULT_VIRTIO_SERIAL_PORTS; + + VIR_DEBUG("Adding virtio serial controller index %u with %d" + " ports to the address set", cont->idx, ports); + + if (!(map = virBitmapNew(ports))) + goto cleanup; + + if (virAsprintf(&str, "%u", cont->idx) < 0) + goto cleanup; + + if (virHashLookup(addrs->used, str)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("virtio serial controller with index %u " + " is already in the address set"), cont->idx); + goto cleanup; + } + if (virHashAddEntry(addrs->used, str, map) < 0) + goto cleanup; + map = NULL; + + if (!addrs->nextInit) { + addrs->next.controller = cont->idx; + addrs->nextInit = true; + } + + ret = 0; + + cleanup: + VIR_FREE(str); + virBitmapFree(map); + return ret; +} + +/* virDomainVirtioSerialAddrSetAddControllers + * + * Adds virtio serial ports of the existing controllers + * to the address set. + */ +int +virDomainVirtioSerialAddrSetAddControllers(virDomainVirtioSerialAddrSetPtr addrs, + virDomainDefPtr def) +{ + size_t i; + + for (i = 0; i < def->ncontrollers; i++) { + if (virDomainVirtioSerialAddrSetAddController(addrs, + def->controllers[i]) < 0) + return -1; + } + + return 0; +} + +void +virDomainVirtioSerialAddrSetFree(virDomainVirtioSerialAddrSetPtr addrs) +{ + if (addrs) { + virHashFree(addrs->used); + VIR_FREE(addrs); + } +} + +/* + * Eww, this function compares two unsigned integers stored as a string + */ +static int +virDomainVirtioSerialAddrCompare(const virHashKeyValuePair *a, + const virHashKeyValuePair *b) +{ + const char *key_a = a->key; + const char *key_b = b->key; + + size_t len_a = strlen(key_a); + size_t len_b = strlen(key_b); + + /* with no padding/negative numbers allowed, the longer string + * contains a larger number */ + if (len_a < len_b) + return -1; + else if (len_a > len_b) + return 1; + else + return strncmp(key_a, key_b, len_a); +} + +static int +virDomainVirtioSerialAddrNext(virDomainVirtioSerialAddrSetPtr addrs, + virDomainDeviceVirtioSerialAddress *addr, + bool allowZero) +{ + virBitmapPtr cur = NULL; + char *str = NULL; + int ret = -1; + virHashKeyValuePairPtr arr = NULL; + size_t i, ncontrollers; + size_t curidx; + ssize_t port, start = 0; + unsigned int controller; + + /* port number 0 is reserved for virtconsoles */ + if (allowZero) + start = -1; + + /* What controller was the last assigned address on? */ + if (virAsprintf(&str, "%u", addrs->next.controller) < 0) + goto cleanup; + + if (!(cur = virHashLookup(addrs->used, str))) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("The last used virtio serial controller is missing " + "from the address set hash table")); + goto cleanup; + } + + /* Look for a free port on the current controller */ + if ((port = virBitmapNextClearBit(cur, start + addrs->next.port)) >= 0) { + controller = addrs->next.controller; + goto success; + } + + ncontrollers = virHashSize(addrs->used); + arr = virHashGetItems(addrs->used, virDomainVirtioSerialAddrCompare); + if (!arr) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Unable to get the hash table as an array")); + goto cleanup; + } + + /* Find its position in the hash "array" */ + for (i = 0; i < ncontrollers; i++) { + if (arr[i].value == cur) { + curidx = i; + break; + } + } + if (i == ncontrollers) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("The last used virtio serial controller is missing from the set")); + goto cleanup; + } + + /* Search for a free port after the current controller */ + for (i = curidx; i < ncontrollers; i++) { + cur = (virBitmapPtr) arr[i].value; + if ((port = virBitmapNextClearBit(cur, start)) >= 0) { + if (virStrToLong_ui(arr[i].key, NULL, 10, &controller) < 0) + goto cleanup; + goto success; + } + } + + for (i = 0; i < curidx; i++) { + cur = (virBitmapPtr) arr[i].value; + if ((port = virBitmapNextClearBit(cur, start)) >= 0) { + if (virStrToLong_ui(arr[i].key, NULL, 10, &controller) < 0) + goto cleanup; + goto success; + } + } + + virReportError(VIR_ERR_XML_ERROR, "%s", + _("Unable to find a free virtio-serial port")); + + cleanup: + VIR_FREE(arr); + VIR_FREE(str); + return ret; + + success: + addr->bus = 0; + addr->port = port; + addr->controller = controller; + VIR_DEBUG("Found free virtio serial controller %u port %u", addr->controller, + addr->port); + ret = 0; + goto cleanup; +} + +/* virDomainVirtioSerialAddrAutoAssign + * + * reserve a virtio serial address of the device (if it has one) + * or assign a virtio serial address to the device + */ +int +virDomainVirtioSerialAddrAutoAssign(virDomainVirtioSerialAddrSetPtr addrs, + virDomainDeviceInfoPtr info, + bool allowZero) +{ + if (info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_SERIAL && + info->addr.vioserial.port) + return virDomainVirtioSerialAddrReserve(NULL, NULL, info, addrs); + else + return virDomainVirtioSerialAddrAssign(addrs, info, allowZero); +} + + +int +virDomainVirtioSerialAddrAssign(virDomainVirtioSerialAddrSetPtr addrs, + virDomainDeviceInfoPtr info, + bool allowZero) +{ + int ret = -1; + virDomainDeviceInfo nfo = { NULL }; + virDomainDeviceInfoPtr ptr = allowZero ? &nfo : info; + + ptr->type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_SERIAL; + + if (virDomainVirtioSerialAddrNext(addrs, &ptr->addr.vioserial, + allowZero) < 0) + goto cleanup; + + addrs->next = info->addr.vioserial; + + if (virDomainVirtioSerialAddrReserve(NULL, NULL, ptr, addrs) < 0) + goto cleanup; + + ret = 0; + + cleanup: + return ret; +} + +/* virDomainVirtioSerialAddrReserve + * + * Reserve the virtio serial address of the device + * + * For use with virDomainDeviceInfoIterate, + * opaque should be the address set + */ +int +virDomainVirtioSerialAddrReserve(virDomainDefPtr def ATTRIBUTE_UNUSED, + virDomainDeviceDefPtr dev ATTRIBUTE_UNUSED, + virDomainDeviceInfoPtr info, + void *data) +{ + virDomainVirtioSerialAddrSetPtr addrs = data; + virBitmapPtr map; + char *str = NULL; + int ret = -1; + bool b; + + if (info->type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_SERIAL || + info->addr.vioserial.port == 0) + return 0; + + VIR_DEBUG("Reserving virtio serial %u %u", info->addr.vioserial.controller, + info->addr.vioserial.port); + + if (virAsprintf(&str, "%u", info->addr.vioserial.controller) < 0) + goto cleanup; + + if ((map = virHashLookup(addrs->used, str)) == NULL) { + virReportError(VIR_ERR_XML_ERROR, + _("virtio serial controller %u is missing"), + info->addr.vioserial.controller); + goto cleanup; + } + + if (virBitmapGetBit(map, info->addr.vioserial.port, &b) < 0) { + virReportError(VIR_ERR_XML_ERROR, + _("virtio serial controller %u does not have port %u"), + info->addr.vioserial.controller, + info->addr.vioserial.port); + goto cleanup; + } + + if (b) { + virReportError(VIR_ERR_XML_ERROR, + _("virtio serial port %u on controller %u is already occupied"), + info->addr.vioserial.port, + info->addr.vioserial.controller); + goto cleanup; + } + + ignore_value(virBitmapSetBit(map, info->addr.vioserial.port)); + + ret = 0; + + cleanup: + VIR_FREE(str); + return ret; +} + +/* virDomainVirtioSerialAddrRelease + * + * Release the virtio serial address of the device + */ +int +virDomainVirtioSerialAddrRelease(virDomainVirtioSerialAddrSetPtr addrs, + virDomainDeviceInfoPtr info) +{ + virBitmapPtr map; + char *str = NULL; + int ret = -1; + + if (info->type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_SERIAL || + info->addr.vioserial.port == 0) + return 0; + + VIR_DEBUG("Releasing virtio serial %u %u", info->addr.vioserial.controller, + info->addr.vioserial.port); + + if (virAsprintf(&str, "%u", info->addr.vioserial.controller) < 0) + goto cleanup; + + if ((map = virHashLookup(addrs->used, str)) == NULL) { + virReportError(VIR_ERR_XML_ERROR, + _("virtio serial controller %u is missing"), + info->addr.vioserial.controller); + goto cleanup; + } + + if (virBitmapClearBit(map, info->addr.vioserial.port) < 0) { + virReportError(VIR_ERR_XML_ERROR, + _("virtio serial controller %u does not have port %u"), + info->addr.vioserial.controller, + info->addr.vioserial.port); + goto cleanup; + } + + ret = 0; + + cleanup: + VIR_FREE(str); + return ret; +} diff --git a/src/conf/domain_addr.h b/src/conf/domain_addr.h index 2c3468e..5c65a98 100644 --- a/src/conf/domain_addr.h +++ b/src/conf/domain_addr.h @@ -173,4 +173,49 @@ int virDomainCCWAddressReleaseAddr(virDomainCCWAddressSetPtr addrs, virDomainDeviceInfoPtr dev) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); virDomainCCWAddressSetPtr virDomainCCWAddressSetCreate(void); + +struct _virDomainVirtioSerialAddrSet { + virHashTablePtr used; + virDomainDeviceVirtioSerialAddress next; + bool nextInit; +}; +typedef struct _virDomainVirtioSerialAddrSet virDomainVirtioSerialAddrSet; +typedef virDomainVirtioSerialAddrSet *virDomainVirtioSerialAddrSetPtr; + +virDomainVirtioSerialAddrSetPtr +virDomainVirtioSerialAddrSetCreate(void); +int +virDomainVirtioSerialAddrSetAddController(virDomainVirtioSerialAddrSetPtr addrs, + virDomainControllerDefPtr cont) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); +int +virDomainVirtioSerialAddrSetAddControllers(virDomainVirtioSerialAddrSetPtr addrs, + virDomainDefPtr def) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); +void +virDomainVirtioSerialAddrSetFree(virDomainVirtioSerialAddrSetPtr addrs); +int +virDomainVirtioSerialAddrAutoAssign(virDomainVirtioSerialAddrSetPtr addrs, + virDomainDeviceInfoPtr info, + bool allowZero) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); + +int +virDomainVirtioSerialAddrAssign(virDomainVirtioSerialAddrSetPtr addrs, + virDomainDeviceInfoPtr info, + bool allowZero) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); + +int +virDomainVirtioSerialAddrReserve(virDomainDefPtr def, + virDomainDeviceDefPtr dev, + virDomainDeviceInfoPtr info, + void *data) + ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(4); + +int +virDomainVirtioSerialAddrRelease(virDomainVirtioSerialAddrSetPtr addrs, + virDomainDeviceInfoPtr info) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); + #endif /* __DOMAIN_ADDR_H__ */ diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 6028002..9f2996a 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -108,6 +108,14 @@ virDomainPCIAddressSetFree; virDomainPCIAddressSetGrow; virDomainPCIAddressSlotInUse; virDomainPCIAddressValidate; +virDomainVirtioSerialAddrAssign; +virDomainVirtioSerialAddrAutoAssign; +virDomainVirtioSerialAddrRelease; +virDomainVirtioSerialAddrReserve; +virDomainVirtioSerialAddrSetAddController; +virDomainVirtioSerialAddrSetAddControllers; +virDomainVirtioSerialAddrSetCreate; +virDomainVirtioSerialAddrSetFree; # conf/domain_audit.h -- 2.0.5

On Tue, Mar 03, 2015 at 15:44:27 +0100, Ján Tomko wrote:
Store the available ports of a virtio-serial controller in a virBitmap. The bitmaps are stored in a hash table - the controller index formatted as a string.
Buses are not tracked, because they aren't supported by QEMU. --- src/conf/domain_addr.c | 382 +++++++++++++++++++++++++++++++++++++++++++++++ src/conf/domain_addr.h | 45 ++++++ src/libvirt_private.syms | 8 + 3 files changed, 435 insertions(+)
diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c index fb4a76f..654c95a 100644 --- a/src/conf/domain_addr.c +++ b/src/conf/domain_addr.c +int
...
+virDomainVirtioSerialAddrSetAddControllers(virDomainVirtioSerialAddrSetPtr addrs, + virDomainDefPtr def) +{ + size_t i; + + for (i = 0; i < def->ncontrollers; i++) { + if (virDomainVirtioSerialAddrSetAddController(addrs, + def->controllers[i]) < 0) + return -1; + } + + return 0; +} + +void +virDomainVirtioSerialAddrSetFree(virDomainVirtioSerialAddrSetPtr addrs) +{ + if (addrs) { + virHashFree(addrs->used); + VIR_FREE(addrs); + } +} + +/* + * Eww, this function compares two unsigned integers stored as a string + */
Okay, technically that works as expected, but is it really necessary? If the user doesn't specify the serial bus to connect the port to, we can IMO attach it to any of the available ones. For that approach you can use the virHashSearch function combined with virBitmapNextClearBit. Otherwise it would be better to just use an array and not bother with a bitmap.
+static int +virDomainVirtioSerialAddrCompare(const virHashKeyValuePair *a, + const virHashKeyValuePair *b) +{ + const char *key_a = a->key; + const char *key_b = b->key; + + size_t len_a = strlen(key_a); + size_t len_b = strlen(key_b); + + /* with no padding/negative numbers allowed, the longer string + * contains a larger number */ + if (len_a < len_b) + return -1; + else if (len_a > len_b) + return 1; + else + return strncmp(key_a, key_b, len_a); +} + +static int +virDomainVirtioSerialAddrNext(virDomainVirtioSerialAddrSetPtr addrs, + virDomainDeviceVirtioSerialAddress *addr, + bool allowZero) +{ + virBitmapPtr cur = NULL; + char *str = NULL; + int ret = -1; + virHashKeyValuePairPtr arr = NULL; + size_t i, ncontrollers; + size_t curidx; + ssize_t port, start = 0; + unsigned int controller; + + /* port number 0 is reserved for virtconsoles */ + if (allowZero) + start = -1; + + /* What controller was the last assigned address on? */ + if (virAsprintf(&str, "%u", addrs->next.controller) < 0) + goto cleanup;
Also making sure that the last controller is always used doens't make that much sense IMO.
+ + if (!(cur = virHashLookup(addrs->used, str))) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("The last used virtio serial controller is missing " + "from the address set hash table")); + goto cleanup; + }
Nor finding it problematic if it's missing.
+ + /* Look for a free port on the current controller */ + if ((port = virBitmapNextClearBit(cur, start + addrs->next.port)) >= 0) { + controller = addrs->next.controller; + goto success; + } + + ncontrollers = virHashSize(addrs->used); + arr = virHashGetItems(addrs->used, virDomainVirtioSerialAddrCompare); + if (!arr) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Unable to get the hash table as an array")); + goto cleanup; + } + + /* Find its position in the hash "array" */ + for (i = 0; i < ncontrollers; i++) { + if (arr[i].value == cur) { + curidx = i; + break; + } + } + if (i == ncontrollers) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("The last used virtio serial controller is missing from the set")); + goto cleanup; + } + + /* Search for a free port after the current controller */ + for (i = curidx; i < ncontrollers; i++) { + cur = (virBitmapPtr) arr[i].value; + if ((port = virBitmapNextClearBit(cur, start)) >= 0) { + if (virStrToLong_ui(arr[i].key, NULL, 10, &controller) < 0) + goto cleanup; + goto success; + } + } + + for (i = 0; i < curidx; i++) { + cur = (virBitmapPtr) arr[i].value; + if ((port = virBitmapNextClearBit(cur, start)) >= 0) { + if (virStrToLong_ui(arr[i].key, NULL, 10, &controller) < 0) + goto cleanup; + goto success; + } + }
Or doing any of this. Finding the first available port is sufficient IMO.
+ + virReportError(VIR_ERR_XML_ERROR, "%s", + _("Unable to find a free virtio-serial port")); + + cleanup: + VIR_FREE(arr); + VIR_FREE(str); + return ret; + + success: + addr->bus = 0; + addr->port = port; + addr->controller = controller; + VIR_DEBUG("Found free virtio serial controller %u port %u", addr->controller, + addr->port); + ret = 0; + goto cleanup; +}
I think the logic can be simplified by a great extent by not caring which controller to add the port to if the user didn't specify it explictly. Peter

Instead of always using controller 0 and incrementing port number, respect the maximum port numbers of controllers and use all of them. Ports for virtio consoles are quietly reserved, but not formatted (neither in XML nor on QEMU command line). Also rejects duplicate virtio-serial addresses. https://bugzilla.redhat.com/show_bug.cgi?id=890606 https://bugzilla.redhat.com/show_bug.cgi?id=1076708 --- src/conf/domain_conf.c | 29 ---------- src/qemu/qemu_command.c | 63 ++++++++++++++++++++++ src/qemu/qemu_domain.c | 1 + src/qemu/qemu_domain.h | 1 + src/qemu/qemu_process.c | 2 + .../qemuxml2argv-channel-virtio-auto.args | 8 +-- .../qemuxml2argv-channel-virtio-autoassign.args | 10 ++-- .../qemuxml2xmlout-channel-virtio-auto.xml | 10 ++-- 8 files changed, 81 insertions(+), 43 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 6da02b0..6b308cf 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -3349,21 +3349,6 @@ virDomainDeviceDefPostParseInternal(virDomainDeviceDefPtr dev, chr->target.port = maxport + 1; } - - if (chr->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_SERIAL && - chr->info.addr.vioserial.port == 0) { - int maxport = 0; - - for (i = 0; i < cnt; i++) { - const virDomainChrDef *thischr = arrPtr[i]; - if (thischr->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_SERIAL && - thischr->info.addr.vioserial.controller == chr->info.addr.vioserial.controller && - thischr->info.addr.vioserial.bus == chr->info.addr.vioserial.bus && - (int)thischr->info.addr.vioserial.port > maxport) - maxport = thischr->info.addr.vioserial.port; - } - chr->info.addr.vioserial.port = maxport + 1; - } } /* set default path for virtio-rng "random" backend to /dev/random */ @@ -14353,20 +14338,6 @@ virDomainDefParseXML(xmlDocPtr xml, chr->targetType == VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_VIRTIO && chr->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) chr->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_SERIAL; - - if (chr->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_SERIAL && - chr->info.addr.vioserial.port == 0) { - int maxport = 0; - for (j = 0; j < i; j++) { - virDomainChrDefPtr thischr = def->channels[j]; - if (thischr->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_SERIAL && - thischr->info.addr.vioserial.controller == chr->info.addr.vioserial.controller && - thischr->info.addr.vioserial.bus == chr->info.addr.vioserial.bus && - (int)thischr->info.addr.vioserial.port > maxport) - maxport = thischr->info.addr.vioserial.port; - } - chr->info.addr.vioserial.port = maxport + 1; - } } VIR_FREE(nodes); diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index c112619..654fba1 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -1347,6 +1347,65 @@ qemuAssignSpaprVIOAddress(virDomainDefPtr def, virDomainDeviceInfoPtr info, return 0; } + +static int +qemuDomainAssignVirtioSerialAddresses(virDomainDefPtr def, + virDomainObjPtr obj) +{ + int ret = -1; + size_t i; + virDomainVirtioSerialAddrSetPtr addrs = NULL; + qemuDomainObjPrivatePtr priv = 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) { + if (virDomainVirtioSerialAddrAssign(addrs, &chr->info, true) < 0) + goto cleanup; + } + } + + for (i = 0; i < def->nchannels; i++) { + virDomainChrDefPtr chr = def->channels[i]; + if (chr->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_SERIAL && + chr->info.addr.vioserial.port == 0 && + virDomainVirtioSerialAddrAssign(addrs, &chr->info, false) < 0) + goto cleanup; + } + + if (obj && obj->privateData) { + priv = obj->privateData; + if (addrs) { + /* if this is the live domain object, we persist the CCW addresses*/ + virDomainVirtioSerialAddrSetFree(priv->vioserialaddrs); + priv->persistentAddrs = 1; + priv->vioserialaddrs = addrs; + addrs = NULL; + } else { + priv->persistentAddrs = 0; + } + } + ret = 0; + + cleanup: + virDomainVirtioSerialAddrSetFree(addrs); + return ret; +} + + int qemuDomainAssignSpaprVIOAddresses(virDomainDefPtr def, virQEMUCapsPtr qemuCaps) { @@ -1593,6 +1652,10 @@ int qemuDomainAssignAddresses(virDomainDefPtr def, { int rc; + rc = qemuDomainAssignVirtioSerialAddresses(def, obj); + if (rc) + return rc; + rc = qemuDomainAssignSpaprVIOAddresses(def, qemuCaps); if (rc) return rc; diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index d8a2087..27af7f9 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -431,6 +431,7 @@ qemuDomainObjPrivateFree(void *data) virCgroupFree(&priv->cgroup); virDomainPCIAddressSetFree(priv->pciaddrs); virDomainCCWAddressSetFree(priv->ccwaddrs); + virDomainVirtioSerialAddrSetFree(priv->vioserialaddrs); virDomainChrSourceDefFree(priv->monConfig); qemuDomainObjFreeJob(priv); VIR_FREE(priv->vcpupids); diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index fe3e2b1..521c5ec 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -159,6 +159,7 @@ struct _qemuDomainObjPrivate { virDomainPCIAddressSetPtr pciaddrs; virDomainCCWAddressSetPtr ccwaddrs; + virDomainVirtioSerialAddrSetPtr vioserialaddrs; int persistentAddrs; virQEMUCapsPtr qemuCaps; diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index d1f089d..97523b3 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -5286,6 +5286,8 @@ void qemuProcessStop(virQEMUDriverPtr driver, virDomainDefClearCCWAddresses(vm->def); virDomainCCWAddressSetFree(priv->ccwaddrs); priv->ccwaddrs = NULL; + virDomainVirtioSerialAddrSetFree(priv->vioserialaddrs); + priv->vioserialaddrs = NULL; } qemuDomainReAttachHostDevices(driver, vm->def); diff --git a/tests/qemuxml2argvdata/qemuxml2argv-channel-virtio-auto.args b/tests/qemuxml2argvdata/qemuxml2argv-channel-virtio-auto.args index f7d7409..71edfae 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-channel-virtio-auto.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-channel-virtio-auto.args @@ -9,14 +9,14 @@ virtio-serial-pci,id=virtio-serial2,bus=pci.0,addr=0x4 -usb -hda \ /dev/HostVG/QEMUGuest1 -chardev pty,id=charchannel0 -device virtserialport,\ bus=virtio-serial0.0,nr=1,chardev=charchannel0,id=channel0,\ name=org.linux-kvm.port.0 -chardev pty,id=charchannel1 -device virtserialport,\ -bus=virtio-serial1.0,nr=1,chardev=charchannel1,id=channel1,\ +bus=virtio-serial0.0,nr=2,chardev=charchannel1,id=channel1,\ name=org.linux-kvm.port.foo -chardev pty,id=charchannel2 -device \ virtserialport,bus=virtio-serial1.0,nr=3,chardev=charchannel2,id=channel2,\ name=org.linux-kvm.port.bar -chardev pty,id=charchannel3 -device \ -virtserialport,bus=virtio-serial0.0,nr=2,chardev=charchannel3,id=channel3,\ +virtserialport,bus=virtio-serial0.0,nr=3,chardev=charchannel3,id=channel3,\ name=org.linux-kvm.port.wizz -chardev pty,id=charchannel4 -device \ -virtserialport,bus=virtio-serial1.0,nr=4,chardev=charchannel4,id=channel4,\ +virtserialport,bus=virtio-serial0.0,nr=4,chardev=charchannel4,id=channel4,\ name=org.linux-kvm.port.ooh -chardev pty,id=charchannel5 -device \ -virtserialport,bus=virtio-serial2.0,nr=1,chardev=charchannel5,id=channel5,\ +virtserialport,bus=virtio-serial0.0,nr=5,chardev=charchannel5,id=channel5,\ name=org.linux-kvm.port.lla -device virtio-balloon-pci,id=balloon0,\ bus=pci.0,addr=0x5 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-channel-virtio-autoassign.args b/tests/qemuxml2argvdata/qemuxml2argv-channel-virtio-autoassign.args index d64a228..f11039d 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-channel-virtio-autoassign.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-channel-virtio-autoassign.args @@ -5,16 +5,16 @@ LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test QEMU_AUDIO_DRV=none \ -device virtio-serial-pci,id=virtio-serial0,max_ports=4,vectors=4,bus=pci.0\ ,addr=0x3 -device virtio-serial-pci,id=virtio-serial1,bus=pci.0,addr=0xa \ -usb -hda /dev/HostVG/QEMUGuest1 \ --chardev pty,id=charchannel0 -device virtserialport,bus=virtio-serial0.0,nr=1,\ +-chardev pty,id=charchannel0 -device virtserialport,bus=virtio-serial0.0,nr=2,\ chardev=charchannel0,id=channel0,name=org.linux-kvm.port.0 \ --chardev pty,id=charchannel1 -device virtserialport,bus=virtio-serial0.0,nr=2,\ +-chardev pty,id=charchannel1 -device virtserialport,bus=virtio-serial0.0,nr=3,\ chardev=charchannel1,id=channel1,name=org.linux-kvm.port.foo \ -chardev pty,id=charchannel2 -device virtserialport,bus=virtio-serial0.0,nr=1,\ chardev=charchannel2,id=channel2,name=org.linux-kvm.port.bar \ --chardev pty,id=charchannel3 -device virtserialport,bus=virtio-serial0.2,nr=1,\ +-chardev pty,id=charchannel3 -device virtserialport,bus=virtio-serial1.0,nr=1,\ chardev=charchannel3,id=channel3,name=org.linux-kvm.port.wizz \ --chardev pty,id=charchannel4 -device virtserialport,bus=virtio-serial0.0,nr=3,\ +-chardev pty,id=charchannel4 -device virtserialport,bus=virtio-serial1.0,nr=2,\ chardev=charchannel4,id=channel4,name=org.linux-kvm.port.ooh \ --chardev pty,id=charchannel5 -device virtserialport,bus=virtio-serial0.0,nr=4,\ +-chardev pty,id=charchannel5 -device virtserialport,bus=virtio-serial1.0,nr=3,\ chardev=charchannel5,id=channel5,name=org.linux-kvm.port.lla \ -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x4 diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-channel-virtio-auto.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-channel-virtio-auto.xml index fd6b852..afe41cf 100644 --- a/tests/qemuxml2xmloutdata/qemuxml2xmlout-channel-virtio-auto.xml +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-channel-virtio-auto.xml @@ -29,11 +29,11 @@ <controller type='virtio-serial' index='2'/> <channel type='pty'> <target type='virtio' name='org.linux-kvm.port.0'/> - <address type='virtio-serial' controller='0' bus='0' port='1'/> + <address type='virtio-serial' controller='0' bus='0' port='0'/> </channel> <channel type='pty'> <target type='virtio' name='org.linux-kvm.port.foo'/> - <address type='virtio-serial' controller='1' bus='0' port='1'/> + <address type='virtio-serial' controller='1' bus='0' port='0'/> </channel> <channel type='pty'> <target type='virtio' name='org.linux-kvm.port.bar'/> @@ -41,15 +41,15 @@ </channel> <channel type='pty'> <target type='virtio' name='org.linux-kvm.port.wizz'/> - <address type='virtio-serial' controller='0' bus='0' port='2'/> + <address type='virtio-serial' controller='0' bus='0' port='0'/> </channel> <channel type='pty'> <target type='virtio' name='org.linux-kvm.port.ooh'/> - <address type='virtio-serial' controller='1' bus='0' port='4'/> + <address type='virtio-serial' controller='1' bus='0' port='0'/> </channel> <channel type='pty'> <target type='virtio' name='org.linux-kvm.port.lla'/> - <address type='virtio-serial' controller='2' bus='0' port='1'/> + <address type='virtio-serial' controller='2' bus='0' port='0'/> </channel> <memballoon model='virtio'/> </devices> -- 2.0.5

--- src/qemu/qemu_hotplug.c | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 08047ce..aec541a 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -435,6 +435,7 @@ int qemuDomainAttachControllerDevice(virQEMUDriverPtr driver, char *devstr = NULL; qemuDomainObjPrivatePtr priv = vm->privateData; bool releaseaddr = false; + char *contstr = NULL; if (virDomainControllerFind(vm->def, controller->type, controller->idx) >= 0) { virReportError(VIR_ERR_OPERATION_FAILED, @@ -473,6 +474,13 @@ int qemuDomainAttachControllerDevice(virQEMUDriverPtr driver, goto cleanup; } + if (controller->type == VIR_DOMAIN_CONTROLLER_TYPE_VIRTIO_SERIAL && + virDomainVirtioSerialAddrSetAddController(priv->vioserialaddrs, + controller) < 0) + goto cleanup; + if (virAsprintf(&contstr, "%u", controller->idx) < 0) + goto cleanup; + if (!(devstr = qemuBuildControllerDevStr(vm->def, controller, priv->qemuCaps, NULL))) goto cleanup; } @@ -501,10 +509,13 @@ int qemuDomainAttachControllerDevice(virQEMUDriverPtr driver, } cleanup: + if (ret != 0 && contstr) + virHashRemoveEntry(priv->vioserialaddrs->used, contstr); if (ret != 0 && releaseaddr) qemuDomainReleaseDeviceAddress(vm, &controller->info, NULL); VIR_FREE(devstr); + VIR_FREE(contstr); return ret; } -- 2.0.5

--- src/qemu/qemu_hotplug.c | 21 +++++++++++++++++++-- tests/qemuhotplugtest.c | 2 +- 2 files changed, 20 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index aec541a..66e052e 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -1532,6 +1532,8 @@ int qemuDomainAttachChrDevice(virQEMUDriverPtr driver, virDomainDefPtr vmdef = vm->def; char *devstr = NULL; char *charAlias = NULL; + bool need_release = false; + bool allowZero = false; if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE)) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", @@ -1542,6 +1544,16 @@ int qemuDomainAttachChrDevice(virQEMUDriverPtr driver, if (qemuAssignDeviceChrAlias(vmdef, chr, -1) < 0) goto cleanup; + if (chr->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CONSOLE && + chr->targetType == VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_VIRTIO) + allowZero = true; + + if (virDomainVirtioSerialAddrAutoAssign(priv->vioserialaddrs, + &chr->info, + allowZero) < 0) + goto cleanup; + need_release = true; + if (qemuBuildChrDeviceStr(&devstr, vm->def, chr, priv->qemuCaps) < 0) goto cleanup; @@ -1573,6 +1585,8 @@ int qemuDomainAttachChrDevice(virQEMUDriverPtr driver, cleanup: if (ret < 0 && virDomainObjIsActive(vm)) qemuDomainChrInsertPreAllocCleanup(vm->def, chr); + if (ret < 0 && need_release) + virDomainVirtioSerialAddrRelease(priv->vioserialaddrs, &chr->info); VIR_FREE(charAlias); VIR_FREE(devstr); return ret; @@ -3969,10 +3983,13 @@ int qemuDomainDetachChrDevice(virQEMUDriverPtr driver, goto cleanup; rc = qemuDomainWaitForDeviceRemoval(vm); - if (rc == 0 || rc == 1) + if (rc == 0 || rc == 1) { + virDomainVirtioSerialAddrRelease(priv->vioserialaddrs, &tmpChr->info); ret = qemuDomainRemoveChrDevice(driver, vm, tmpChr); - else + } else { ret = 0; + } + cleanup: qemuDomainResetDeviceRemoval(vm); diff --git a/tests/qemuhotplugtest.c b/tests/qemuhotplugtest.c index 12a7f71..ea2cf77 100644 --- a/tests/qemuhotplugtest.c +++ b/tests/qemuhotplugtest.c @@ -86,7 +86,7 @@ qemuHotplugCreateObjects(virDomainXMLOptionPtr xmlopt, if (event) virQEMUCapsSet(priv->qemuCaps, QEMU_CAPS_DEVICE_DEL_EVENT); - if (qemuDomainAssignPCIAddresses((*vm)->def, priv->qemuCaps, *vm) < 0) + if (qemuDomainAssignAddresses((*vm)->def, priv->qemuCaps, *vm) < 0) goto cleanup; if (qemuAssignDeviceAliases((*vm)->def, priv->qemuCaps) < 0) -- 2.0.5
participants (2)
-
Ján Tomko
-
Peter Krempa