[libvirt] [PATCHv2 0/5] Allocate virtio-serial addresses
https://bugzilla.redhat.com/show_bug.cgi?id=890606 https://bugzilla.redhat.com/show_bug.cgi?id=1076708 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 | 348 +++++++++++++++++++++ src/conf/domain_addr.h | 56 ++++ src/conf/domain_conf.c | 29 -- src/libvirt_private.syms | 9 + src/qemu/qemu_command.c | 63 ++++ src/qemu/qemu_domain.c | 1 + src/qemu/qemu_domain.h | 1 + src/qemu/qemu_hotplug.c | 31 +- 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, 591 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 fcf5218..0ec68f4 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -1070,6 +1070,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
Create a sorted array of virtio-serial controllers. Each of the elements contains the controller index and a bitmap of available ports. Buses are not tracked, because they aren't supported by QEMU. --- src/conf/domain_addr.c | 348 +++++++++++++++++++++++++++++++++++++++++++++++ src/conf/domain_addr.h | 56 ++++++++ src/libvirt_private.syms | 9 ++ 3 files changed, 413 insertions(+) diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c index fb4a76f..d9d01fc 100644 --- a/src/conf/domain_addr.c +++ b/src/conf/domain_addr.c @@ -718,3 +718,351 @@ virDomainCCWAddressSetCreate(void) virDomainCCWAddressSetFree(addrs); return NULL; } + + +#define VIR_DOMAIN_DEFAULT_VIRTIO_SERIAL_PORTS 31 + + +/* virDomainVirtioSerialAddrSetCreate + * + * Allocates an address set for virtio serial addresses + */ +virDomainVirtioSerialAddrSetPtr +virDomainVirtioSerialAddrSetCreate(void) +{ + virDomainVirtioSerialAddrSetPtr ret = NULL; + + if (VIR_ALLOC(ret) < 0) + return NULL; + + return ret; +} + +static void +virDomainVirtioSerialControllerFree(virDomainVirtioSerialControllerPtr cont) +{ + if (cont) { + virBitmapFree(cont->ports); + VIR_FREE(cont); + } +} + +static ssize_t +virDomainVirtioSerialAddrPlaceController(virDomainVirtioSerialAddrSetPtr addrs, + virDomainVirtioSerialControllerPtr cont) +{ + size_t i; + + for (i = 0; i < addrs->ncontrollers; i++) { + if (addrs->controllers[i]->idx >= cont->idx) + return i; + } + return -1; +} + +static ssize_t +virDomainVirtioSerialAddrFindController(virDomainVirtioSerialAddrSetPtr addrs, + unsigned int idx) +{ + size_t i; + + for (i = 0; i < addrs->ncontrollers; i++) { + if (addrs->controllers[i]->idx == idx) + return i; + } + return -1; +} + +/* virDomainVirtioSerialAddrSetAddController + * + * Adds virtio serial ports of the existing controller + * to the address set. + */ +int +virDomainVirtioSerialAddrSetAddController(virDomainVirtioSerialAddrSetPtr addrs, + virDomainControllerDefPtr cont) +{ + int ret = -1; + int ports; + virDomainVirtioSerialControllerPtr cnt = NULL; + ssize_t insertAt; + + 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 (VIR_ALLOC(cnt) < 0) + goto cleanup; + + if (!(cnt->ports = virBitmapNew(ports))) + goto cleanup; + cnt->idx = cont->idx; + + insertAt = virDomainVirtioSerialAddrPlaceController(addrs, cnt); + if (VIR_INSERT_ELEMENT(addrs->controllers, insertAt, + addrs->ncontrollers, cnt) < 0) + goto cleanup; + + ret = 0; + + cleanup: + virDomainVirtioSerialControllerFree(cnt); + return ret; +} + +/* virDomainVirtioSerialAddrSetAddControllers + * + * Adds virtio serial ports of controllers present in the domain definition + * 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; +} + +/* virDomainVirtioSerialAddrSetRemoveController + * + * Removes a virtio serial controller from the address set. + */ +int +virDomainVirtioSerialAddrSetRemoveController(virDomainVirtioSerialAddrSetPtr addrs, + virDomainControllerDefPtr cont) +{ + int ret = -1; + ssize_t pos; + + if (cont->type != VIR_DOMAIN_CONTROLLER_TYPE_VIRTIO_SERIAL) + return 0; + + VIR_DEBUG("Removing virtio serial controller index %u " + "from the address set", cont->idx); + + pos = virDomainVirtioSerialAddrFindController(addrs, cont->idx); + + if (pos >= 0 && + VIR_DELETE_ELEMENT(addrs->controllers, pos, addrs->ncontrollers) < 0) + goto cleanup; + + ret = 0; + + cleanup: + return ret; +} + +void +virDomainVirtioSerialAddrSetFree(virDomainVirtioSerialAddrSetPtr addrs) +{ + size_t i; + if (addrs) { + for (i = 0; i < addrs->ncontrollers; i++) + virDomainVirtioSerialControllerFree(addrs->controllers[i]); + VIR_FREE(addrs); + } +} + +static int +virDomainVirtioSerialAddrNext(virDomainVirtioSerialAddrSetPtr addrs, + virDomainDeviceVirtioSerialAddress *addr, + bool allowZero) +{ + int ret = -1; + ssize_t port, start = 0; + ssize_t i; + unsigned int controller; + + /* port number 0 is reserved for virtconsoles */ + if (allowZero) + start = -1; + + if (addrs->ncontrollers == 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("no virtio-serial controllers are available")); + goto cleanup; + } + + for (i = 0; i < addrs->ncontrollers; i++) { + virBitmapPtr map = addrs->controllers[i]->ports; + if ((port = virBitmapNextClearBit(map, start)) >= 0) { + controller = addrs->controllers[i]->idx; + goto success; + } + } + + virReportError(VIR_ERR_XML_ERROR, "%s", + _("Unable to find a free virtio-serial port")); + + cleanup: + 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; + + 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; + char *str = NULL; + int ret = -1; + virBitmapPtr map = NULL; + bool b; + ssize_t i; + + 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); + + i = virDomainVirtioSerialAddrFindController(addrs, info->addr.vioserial.controller); + if (i < 0) { + virReportError(VIR_ERR_XML_ERROR, + _("virtio serial controller %u is missing"), + info->addr.vioserial.controller); + goto cleanup; + } + + map = addrs->controllers[i]->ports; + 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; + ssize_t i; + + 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); + + i = virDomainVirtioSerialAddrFindController(addrs, info->addr.vioserial.controller); + if (i < 0) { + virReportError(VIR_ERR_XML_ERROR, + _("virtio serial controller %u is missing"), + info->addr.vioserial.controller); + goto cleanup; + } + + map = addrs->controllers[i]->ports; + 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..846bf5c 100644 --- a/src/conf/domain_addr.h +++ b/src/conf/domain_addr.h @@ -173,4 +173,60 @@ int virDomainCCWAddressReleaseAddr(virDomainCCWAddressSetPtr addrs, virDomainDeviceInfoPtr dev) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); virDomainCCWAddressSetPtr virDomainCCWAddressSetCreate(void); + +struct _virDomainVirtioSerialController { + unsigned int idx; + virBitmapPtr ports; +}; + +typedef struct _virDomainVirtioSerialController virDomainVirtioSerialController; +typedef virDomainVirtioSerialController *virDomainVirtioSerialControllerPtr; + +struct _virDomainVirtioSerialAddrSet { + virDomainVirtioSerialControllerPtr *controllers; + size_t ncontrollers; +}; +typedef struct _virDomainVirtioSerialAddrSet virDomainVirtioSerialAddrSet; +typedef virDomainVirtioSerialAddrSet *virDomainVirtioSerialAddrSetPtr; + +virDomainVirtioSerialAddrSetPtr +virDomainVirtioSerialAddrSetCreate(void); +int +virDomainVirtioSerialAddrSetAddController(virDomainVirtioSerialAddrSetPtr addrs, + virDomainControllerDefPtr cont) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); +int +virDomainVirtioSerialAddrSetRemoveController(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 1fb42ac..f600838 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -108,6 +108,15 @@ virDomainPCIAddressSetFree; virDomainPCIAddressSetGrow; virDomainPCIAddressSlotInUse; virDomainPCIAddressValidate; +virDomainVirtioSerialAddrAssign; +virDomainVirtioSerialAddrAutoAssign; +virDomainVirtioSerialAddrRelease; +virDomainVirtioSerialAddrReserve; +virDomainVirtioSerialAddrSetAddController; +virDomainVirtioSerialAddrSetAddControllers; +virDomainVirtioSerialAddrSetCreate; +virDomainVirtioSerialAddrSetFree; +virDomainVirtioSerialAddrSetRemoveController; # conf/domain_audit.h -- 2.0.5
On 03/17/2015 07:41 AM, Ján Tomko wrote:
Create a sorted array of virtio-serial controllers. Each of the elements contains the controller index and a bitmap of available ports.
Buses are not tracked, because they aren't supported by QEMU. --- src/conf/domain_addr.c | 348 +++++++++++++++++++++++++++++++++++++++++++++++ src/conf/domain_addr.h | 56 ++++++++ src/libvirt_private.syms | 9 ++ 3 files changed, 413 insertions(+)
I assumed the ACK to 1/5 sticks...
diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c index fb4a76f..d9d01fc 100644 --- a/src/conf/domain_addr.c +++ b/src/conf/domain_addr.c @@ -718,3 +718,351 @@ virDomainCCWAddressSetCreate(void) virDomainCCWAddressSetFree(addrs); return NULL; } + + +#define VIR_DOMAIN_DEFAULT_VIRTIO_SERIAL_PORTS 31 + + +/* virDomainVirtioSerialAddrSetCreate + * + * Allocates an address set for virtio serial addresses + */ +virDomainVirtioSerialAddrSetPtr +virDomainVirtioSerialAddrSetCreate(void) +{ + virDomainVirtioSerialAddrSetPtr ret = NULL; + + if (VIR_ALLOC(ret) < 0) + return NULL; + + return ret; +} + +static void +virDomainVirtioSerialControllerFree(virDomainVirtioSerialControllerPtr cont)
Should the Free routine take a pointer so that when we VIR_FREE the pointer the caller doesn't have to "worry" about setting their copy to NULL?
+{ + if (cont) { + virBitmapFree(cont->ports); + VIR_FREE(cont); + } +} + +static ssize_t +virDomainVirtioSerialAddrPlaceController(virDomainVirtioSerialAddrSetPtr addrs, + virDomainVirtioSerialControllerPtr cont) +{ + size_t i; + + for (i = 0; i < addrs->ncontrollers; i++) { + if (addrs->controllers[i]->idx >= cont->idx) + return i; + }
Would entries "<controller type='virtio-serial' index='1' ports='4'>" and "<controller type='virtio-serial' index='1'"> be rejected elsewhere? I would think "index" would be unique but this algorithm seems to be fine and happy with it.
+ return -1; +} + +static ssize_t +virDomainVirtioSerialAddrFindController(virDomainVirtioSerialAddrSetPtr addrs, + unsigned int idx) +{ + size_t i; + + for (i = 0; i < addrs->ncontrollers; i++) { + if (addrs->controllers[i]->idx == idx) + return i; + } + return -1; +} + +/* virDomainVirtioSerialAddrSetAddController + * + * Adds virtio serial ports of the existing controller + * to the address set. + */ +int +virDomainVirtioSerialAddrSetAddController(virDomainVirtioSerialAddrSetPtr addrs, + virDomainControllerDefPtr cont) +{ + int ret = -1; + int ports; + virDomainVirtioSerialControllerPtr cnt = NULL; + ssize_t insertAt; + + 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 (VIR_ALLOC(cnt) < 0) + goto cleanup; + + if (!(cnt->ports = virBitmapNew(ports))) + goto cleanup; + cnt->idx = cont->idx; + + insertAt = virDomainVirtioSerialAddrPlaceController(addrs, cnt); + if (VIR_INSERT_ELEMENT(addrs->controllers, insertAt, + addrs->ncontrollers, cnt) < 0) + goto cleanup; + + ret = 0; + + cleanup: + virDomainVirtioSerialControllerFree(cnt); + return ret; +} + +/* virDomainVirtioSerialAddrSetAddControllers + * + * Adds virtio serial ports of controllers present in the domain definition + * 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; +} + +/* virDomainVirtioSerialAddrSetRemoveController + * + * Removes a virtio serial controller from the address set. + */ +int +virDomainVirtioSerialAddrSetRemoveController(virDomainVirtioSerialAddrSetPtr addrs, + virDomainControllerDefPtr cont) +{ + int ret = -1; + ssize_t pos; + + if (cont->type != VIR_DOMAIN_CONTROLLER_TYPE_VIRTIO_SERIAL) + return 0; + + VIR_DEBUG("Removing virtio serial controller index %u " + "from the address set", cont->idx); + + pos = virDomainVirtioSerialAddrFindController(addrs, cont->idx); + + if (pos >= 0 && + VIR_DELETE_ELEMENT(addrs->controllers, pos, addrs->ncontrollers) < 0) + goto cleanup; +
If 'pos' < 0, we return 0 (and perhaps leak something). OTOH, the controller was never added and the caller never checks status, maybe this should just be void (wonder why Coverity didn't whine)...
+ ret = 0; + + cleanup: + return ret; +} + +void +virDomainVirtioSerialAddrSetFree(virDomainVirtioSerialAddrSetPtr addrs) +{
same question about having Free routine take address rather than refcopy
+ size_t i; + if (addrs) { + for (i = 0; i < addrs->ncontrollers; i++) + virDomainVirtioSerialControllerFree(addrs->controllers[i]); + VIR_FREE(addrs); + } +} + +static int +virDomainVirtioSerialAddrNext(virDomainVirtioSerialAddrSetPtr addrs, + virDomainDeviceVirtioSerialAddress *addr, + bool allowZero) +{ + int ret = -1; + ssize_t port, start = 0; + ssize_t i; + unsigned int controller; + + /* port number 0 is reserved for virtconsoles */ + if (allowZero) + start = -1; + + if (addrs->ncontrollers == 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("no virtio-serial controllers are available")); + goto cleanup; + } + + for (i = 0; i < addrs->ncontrollers; i++) { + virBitmapPtr map = addrs->controllers[i]->ports; + if ((port = virBitmapNextClearBit(map, start)) >= 0) { + controller = addrs->controllers[i]->idx; + goto success; + } + } + + virReportError(VIR_ERR_XML_ERROR, "%s", + _("Unable to find a free virtio-serial port")); + + cleanup: + 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; + + 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; + char *str = NULL; + int ret = -1; + virBitmapPtr map = NULL; + bool b; + ssize_t i; + + 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); + + i = virDomainVirtioSerialAddrFindController(addrs, info->addr.vioserial.controller); + if (i < 0) { + virReportError(VIR_ERR_XML_ERROR, + _("virtio serial controller %u is missing"), + info->addr.vioserial.controller); + goto cleanup; + } + + map = addrs->controllers[i]->ports; + 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; + ssize_t i; + + 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); + + i = virDomainVirtioSerialAddrFindController(addrs, info->addr.vioserial.controller); + if (i < 0) { + virReportError(VIR_ERR_XML_ERROR, + _("virtio serial controller %u is missing"), + info->addr.vioserial.controller); + goto cleanup; + } + + map = addrs->controllers[i]->ports; + 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; + }
Should we info->addr.vioserial.port = 0 here to ensure someone doesn't end up in some loop retrying the same port? This seems reasonable; however, results in next patch having me scratching my head a bit... John
+ + ret = 0; + + cleanup: + VIR_FREE(str); + return ret; +} diff --git a/src/conf/domain_addr.h b/src/conf/domain_addr.h index 2c3468e..846bf5c 100644 --- a/src/conf/domain_addr.h +++ b/src/conf/domain_addr.h @@ -173,4 +173,60 @@ int virDomainCCWAddressReleaseAddr(virDomainCCWAddressSetPtr addrs, virDomainDeviceInfoPtr dev) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); virDomainCCWAddressSetPtr virDomainCCWAddressSetCreate(void); + +struct _virDomainVirtioSerialController { + unsigned int idx; + virBitmapPtr ports; +}; + +typedef struct _virDomainVirtioSerialController virDomainVirtioSerialController; +typedef virDomainVirtioSerialController *virDomainVirtioSerialControllerPtr; + +struct _virDomainVirtioSerialAddrSet { + virDomainVirtioSerialControllerPtr *controllers; + size_t ncontrollers; +}; +typedef struct _virDomainVirtioSerialAddrSet virDomainVirtioSerialAddrSet; +typedef virDomainVirtioSerialAddrSet *virDomainVirtioSerialAddrSetPtr; + +virDomainVirtioSerialAddrSetPtr +virDomainVirtioSerialAddrSetCreate(void); +int +virDomainVirtioSerialAddrSetAddController(virDomainVirtioSerialAddrSetPtr addrs, + virDomainControllerDefPtr cont) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); +int +virDomainVirtioSerialAddrSetRemoveController(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 1fb42ac..f600838 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -108,6 +108,15 @@ virDomainPCIAddressSetFree; virDomainPCIAddressSetGrow; virDomainPCIAddressSlotInUse; virDomainPCIAddressValidate; +virDomainVirtioSerialAddrAssign; +virDomainVirtioSerialAddrAutoAssign; +virDomainVirtioSerialAddrRelease; +virDomainVirtioSerialAddrReserve; +virDomainVirtioSerialAddrSetAddController; +virDomainVirtioSerialAddrSetAddControllers; +virDomainVirtioSerialAddrSetCreate; +virDomainVirtioSerialAddrSetFree; +virDomainVirtioSerialAddrSetRemoveController;
# conf/domain_audit.h
On Mon, Mar 23, 2015 at 05:46:19PM -0400, John Ferlan wrote:
On 03/17/2015 07:41 AM, Ján Tomko wrote:
Create a sorted array of virtio-serial controllers. Each of the elements contains the controller index and a bitmap of available ports.
Buses are not tracked, because they aren't supported by QEMU. --- src/conf/domain_addr.c | 348 +++++++++++++++++++++++++++++++++++++++++++++++ src/conf/domain_addr.h | 56 ++++++++ src/libvirt_private.syms | 9 ++ 3 files changed, 413 insertions(+)
I assumed the ACK to 1/5 sticks...
+ +static void +virDomainVirtioSerialControllerFree(virDomainVirtioSerialControllerPtr cont)
Should the Free routine take a pointer so that when we VIR_FREE the pointer the caller doesn't have to "worry" about setting their copy to NULL?
None of the callers worry about that for this function. For the other function, I like sticking to the existing convention: *Free routines usually take a copy of the address, just like virBitmapFree below. I think virFuncFree(foo->ptr); looks more tidy than virFuncFree(&(foo->ptr)); And in most of the cases, foo gets freed anyway.
+{ + if (cont) { + virBitmapFree(cont->ports); + VIR_FREE(cont); + } +} + +static ssize_t +virDomainVirtioSerialAddrPlaceController(virDomainVirtioSerialAddrSetPtr addrs, + virDomainVirtioSerialControllerPtr cont) +{ + size_t i; + + for (i = 0; i < addrs->ncontrollers; i++) { + if (addrs->controllers[i]->idx >= cont->idx) + return i; + }
Would entries "<controller type='virtio-serial' index='1' ports='4'>" and "<controller type='virtio-serial' index='1'"> be rejected elsewhere? I would think "index" would be unique but this algorithm seems to be fine and happy with it.
For user-specified controllers, duplicate indexes are rejected by virDomainDefRejectDuplicateControllers, so adding a controller with a non-unique index would be a bug in the auto-allocation logic. I'll squash this in: diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c index d9d01fc..cda9ad2 100644 --- a/src/conf/domain_addr.c +++ b/src/conf/domain_addr.c @@ -754,7 +754,14 @@ virDomainVirtioSerialAddrPlaceController(virDomainVirtioSerialAddrSetPtr addrs, size_t i; for (i = 0; i < addrs->ncontrollers; i++) { - if (addrs->controllers[i]->idx >= cont->idx) + if (addrs->controllers[i]->idx == cont->idx) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("virtio serial controller with index %u already exists" + " in the address set"), + cont->idx); + return -2; + } + if (addrs->controllers[i]->idx > cont->idx) return i; } return -1; @@ -804,7 +811,8 @@ virDomainVirtioSerialAddrSetAddController(virDomainVirtioSerialAddrSetPtr addrs, goto cleanup; cnt->idx = cont->idx; - insertAt = virDomainVirtioSerialAddrPlaceController(addrs, cnt); + if ((insertAt = virDomainVirtioSerialAddrPlaceController(addrs, cnt)) < -1) + goto cleanup; if (VIR_INSERT_ELEMENT(addrs->controllers, insertAt, addrs->ncontrollers, cnt) < 0) goto cleanup;
+/* virDomainVirtioSerialAddrSetRemoveController + * + * Removes a virtio serial controller from the address set. + */ +int +virDomainVirtioSerialAddrSetRemoveController(virDomainVirtioSerialAddrSetPtr addrs, + virDomainControllerDefPtr cont) +{ + int ret = -1; + ssize_t pos; + + if (cont->type != VIR_DOMAIN_CONTROLLER_TYPE_VIRTIO_SERIAL) + return 0; + + VIR_DEBUG("Removing virtio serial controller index %u " + "from the address set", cont->idx); + + pos = virDomainVirtioSerialAddrFindController(addrs, cont->idx); + + if (pos >= 0 && + VIR_DELETE_ELEMENT(addrs->controllers, pos, addrs->ncontrollers) < 0) + goto cleanup; +
If 'pos' < 0, we return 0 (and perhaps leak something). OTOH, the controller was never added and the caller never checks status, maybe this should just be void (wonder why Coverity didn't whine)...
There's nothing to be leaked. Coverity only whines if some callers check the return value and some don't. I'll change the return type to void.
+ +/* virDomainVirtioSerialAddrRelease + * + * Release the virtio serial address of the device + */ +int +virDomainVirtioSerialAddrRelease(virDomainVirtioSerialAddrSetPtr addrs, + virDomainDeviceInfoPtr info) +{ + virBitmapPtr map; + char *str = NULL; + int ret = -1; + ssize_t i; + + 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); + + i = virDomainVirtioSerialAddrFindController(addrs, info->addr.vioserial.controller); + if (i < 0) { + virReportError(VIR_ERR_XML_ERROR, + _("virtio serial controller %u is missing"), + info->addr.vioserial.controller); + goto cleanup; + } + + map = addrs->controllers[i]->ports; + 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; + }
Should we info->addr.vioserial.port = 0 here to ensure someone doesn't end up in some loop retrying the same port?
No, it's the caller's responsibility not to end up in an endless loop by doing the same thing over and over again. Jan
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 c75b543..89357d2 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -3402,21 +3402,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 */ @@ -14526,20 +14511,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 02105c3..e7f86e1 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -1399,6 +1399,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 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) { @@ -1645,6 +1704,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 2eacef2..cb2c166 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 ba8d398..9ad88a8 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 ae315df..5589f22 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -5205,6 +5205,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
On 03/17/2015 07:41 AM, Ján Tomko wrote:
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 c75b543..89357d2 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -3402,21 +3402,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 */ @@ -14526,20 +14511,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 02105c3..e7f86e1 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -1399,6 +1399,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;
Coverity determines addrs != NULL, but
+ + 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) {
There's a check here where the "else" is DEADCODE
+ /* if this is the live domain object, we persist the 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) { @@ -1645,6 +1704,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 2eacef2..cb2c166 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 ba8d398..9ad88a8 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 ae315df..5589f22 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -5205,6 +5205,8 @@ void qemuProcessStop(virQEMUDriverPtr driver, virDomainDefClearCCWAddresses(vm->def); virDomainCCWAddressSetFree(priv->ccwaddrs); priv->ccwaddrs = NULL; + virDomainVirtioSerialAddrSetFree(priv->vioserialaddrs); + priv->vioserialaddrs = NULL; }
qemuDomainReAttachHostDevices(driver, vm->def);
Mostly for my edification... These examples previously although indicating they were "auto-assign" (of sorts) really weren't - they were more force-assign for each example. The way to auto-assign is by setting port='0', correct? However, I'm still missing something from the *auto.args output. It seems the controller='#' is ignored and I guess I don't understand that... Sure "port='0'" (meaning first available port on the controller), but I would have expected to see : kvm.port.0 use "virtio.serial.0.0,nr=1..." (which it does) kvm.port.foo use "virtio.serial.1.0,nr=1..." (on controller 0?, but XML has controller='1') kvm.port.bar use "virtio.serial.1.0,nr=3..." (which it does) kvm.port.wizz use "virtio.serial.0.0,nr=2" (incorrect due to others) kvm.port.ooh use "virtio.serial.1.0,nr=2", (on controller 0?, but XML has controller='1' kvm.port.lla use "virtio.serial.2.0,nr=1" (on controller 0?, but XML has controller='2') Now if been if "lla" used controller='0', then I assume would nr=4 be chosen since 1,2 were auto-assigned, 3 was specifically assigned, thus 4 would be the next one. Continuing with that same logic, the *-autoassign example could have shown that if controller='0',port='2' and 'controller='1',port='1' were preassigned, then the controllers/ports assigned would be 0.0,nr=1, 0.0,nr=3, 0.0,nr=4, 1.0,nr=2 (since only 4 ports on controller='0' can be used w/ 2 be static and controller='1' having port '1' already in use). John
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>
On Mon, Mar 23, 2015 at 05:46:23PM -0400, John Ferlan wrote:
On 03/17/2015 07:41 AM, Ján Tomko wrote:
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/qemu/qemu_command.c b/src/qemu/qemu_command.c index 02105c3..e7f86e1 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -1399,6 +1399,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;
Coverity determines addrs != NULL, but
+ + 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) {
There's a check here where the "else" is DEADCODE
Right. The address set should only be generated if there is a virtio-serial controller present.
+ /* if this is the live domain object, we persist the addresses */ + virDomainVirtioSerialAddrSetFree(priv->vioserialaddrs); + priv->persistentAddrs = 1; + priv->vioserialaddrs = addrs; + addrs = NULL; + } else { + priv->persistentAddrs = 0; + } + } + ret = 0;
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index ae315df..5589f22 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -5205,6 +5205,8 @@ void qemuProcessStop(virQEMUDriverPtr driver, virDomainDefClearCCWAddresses(vm->def); virDomainCCWAddressSetFree(priv->ccwaddrs); priv->ccwaddrs = NULL; + virDomainVirtioSerialAddrSetFree(priv->vioserialaddrs); + priv->vioserialaddrs = NULL; }
qemuDomainReAttachHostDevices(driver, vm->def);
Mostly for my edification... These examples previously although indicating they were "auto-assign" (of sorts) really weren't - they were more force-assign for each example.
Force-assign after this patch? Otherwise I don't understand.
The way to auto-assign is by setting port='0', correct?
Yes.
However, I'm still missing something from the *auto.args output. It seems the controller='#' is ignored and I guess I don't understand that...
I must've overlooked that. It shouldn't be a problem to take it as a hint in virDomainVirtioSerialAddrAssign.
Sure "port='0'" (meaning first available port on the controller), but I would have expected to see :
kvm.port.0 use "virtio.serial.0.0,nr=1..." (which it does) kvm.port.foo use "virtio.serial.1.0,nr=1..." (on controller 0?, but XML has controller='1') kvm.port.bar use "virtio.serial.1.0,nr=3..." (which it does) kvm.port.wizz use "virtio.serial.0.0,nr=2" (incorrect due to others) kvm.port.ooh use "virtio.serial.1.0,nr=2", (on controller 0?, but XML has controller='1' kvm.port.lla use "virtio.serial.2.0,nr=1" (on controller 0?, but XML has controller='2')
Now if been if "lla" used controller='0', then I assume would nr=4 be chosen since 1,2 were auto-assigned, 3 was specifically assigned, thus 4 would be the next one.
Continuing with that same logic, the *-autoassign example could have shown that if controller='0',port='2' and 'controller='1',port='1' were preassigned, then the controllers/ports assigned would be 0.0,nr=1, 0.0,nr=3, 0.0,nr=4, 1.0,nr=2 (since only 4 ports on controller='0' can be used w/ 2 be static and controller='1' having port '1' already in use).
nr=4 is out of bounds for a controller with 4 ports. The ports are numbered from 0, but port number 0 can only be used for virtconsoles, not channels. Jan
On 03/24/2015 07:41 AM, Ján Tomko wrote:
On Mon, Mar 23, 2015 at 05:46:23PM -0400, John Ferlan wrote:
...
Mostly for my edification... These examples previously although indicating they were "auto-assign" (of sorts) really weren't - they were more force-assign for each example.
Force-assign after this patch? Otherwise I don't understand.
I started writing this before figuring out the second part and just didn't go back re-read my original thought... But I think I was trying to point out that the "existing" code doesn't really auto assign and after your patch these changes are doing that, hence why set to port='0'... I agree with not force-assign, but was just making sure...
The way to auto-assign is by setting port='0', correct?
Yes.
However, I'm still missing something from the *auto.args output. It seems the controller='#' is ignored and I guess I don't understand that...
I must've overlooked that. It shouldn't be a problem to take it as a hint in virDomainVirtioSerialAddrAssign.
Right so I figured out after I sent this, but figured you'd know that anyway.
Sure "port='0'" (meaning first available port on the controller), but I would have expected to see :
kvm.port.0 use "virtio.serial.0.0,nr=1..." (which it does) kvm.port.foo use "virtio.serial.1.0,nr=1..." (on controller 0?, but XML has controller='1') kvm.port.bar use "virtio.serial.1.0,nr=3..." (which it does) kvm.port.wizz use "virtio.serial.0.0,nr=2" (incorrect due to others) kvm.port.ooh use "virtio.serial.1.0,nr=2", (on controller 0?, but XML has controller='1' kvm.port.lla use "virtio.serial.2.0,nr=1" (on controller 0?, but XML has controller='2')
Now if been if "lla" used controller='0', then I assume would nr=4 be chosen since 1,2 were auto-assigned, 3 was specifically assigned, thus 4 would be the next one.
Continuing with that same logic, the *-autoassign example could have shown that if controller='0',port='2' and 'controller='1',port='1' were preassigned, then the controllers/ports assigned would be 0.0,nr=1, 0.0,nr=3, 0.0,nr=4, 1.0,nr=2 (since only 4 ports on controller='0' can be used w/ 2 be static and controller='1' having port '1' already in use).
nr=4 is out of bounds for a controller with 4 ports. The ports are numbered from 0, but port number 0 can only be used for virtconsoles, not channels.
Oh yeah - right...doh! Too much context switching sometimes causes loses of brain to finger functions... John
--- src/qemu/qemu_hotplug.c | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 7845fd1..b2f70d1 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; + bool addedToAddrSet = false; if (virDomainControllerFind(vm->def, controller->type, controller->idx) >= 0) { virReportError(VIR_ERR_OPERATION_FAILED, @@ -473,6 +474,12 @@ int qemuDomainAttachControllerDevice(virQEMUDriverPtr driver, goto cleanup; } + if (controller->type == VIR_DOMAIN_CONTROLLER_TYPE_VIRTIO_SERIAL && + virDomainVirtioSerialAddrSetAddController(priv->vioserialaddrs, + controller) < 0) + goto cleanup; + addedToAddrSet = true; + if (!(devstr = qemuBuildControllerDevStr(vm->def, controller, priv->qemuCaps, NULL))) goto cleanup; } @@ -501,6 +508,9 @@ int qemuDomainAttachControllerDevice(virQEMUDriverPtr driver, } cleanup: + if (ret != 0 && addedToAddrSet) + virDomainVirtioSerialAddrSetRemoveController(priv->vioserialaddrs, + controller); if (ret != 0 && releaseaddr) qemuDomainReleaseDeviceAddress(vm, &controller->info, NULL); -- 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 b2f70d1..3e8d398 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -1533,6 +1533,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", @@ -1543,6 +1545,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; @@ -1574,6 +1586,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; @@ -3970,10 +3984,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)
-
John Ferlan -
Ján Tomko