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>