[libvirt] [RFC PATCH 0/3] improve virtio-serial port address allocation
This series of patches improves the way in which virtio-serial port addresses are handled. See BZ1042505[1] for more background information. In a nutshell, the issue is that libvirt always assigns ports to the same virtio-serial controller (index 0). It does not take into account the maximum number of ports of the controller, nor whether there are other controllers with free ports. These patches modify the behaviour of libvirt in two ways: 1. Ports assigned out-of-range numbers are rejected right away, rather than having qemu failing at startup (see patch 2/3). 2. Ports without any explicit <address> elements are assigning a controller guaranteed to hold a free port, rather than always using controller index 0 (see patch 3/3). Care was taken to not override any field explicitly specified by the user (e.g. if the controller index is given, then that controller will be used). However, there are a few shortcomings with these patches (mostly due to my inexperience with the libvirt codebase and virtualization in general): 1. The limit of 31 ports per controller, mentioned here [2], is specific to qemu/kvm (although it is the only driver that supports virtio-serial ports for now). I've hardcoded the constant in these patches because I'm not sure how to properly contain them within qemu-specific code. There doesn't seem to be a way to query qemu to find out what the number of maximum ports are but looking at the qemu codebase indicates that the limit has always been 31. Not sure if a simple #define would be suitable for this. 2. I'm not sure how to handle the 'bus' attribute of virtio-serial port <address> elements. I haven't been able to get information regarding whether the bus is always 0, or how it affects port numbering. (So far haven't been able to start a VM with bus != 0). 3. More broadly, I'm not sure how in line these patches are with the overall design of libvirt. It seems like domain definitions should be completely independent of driver limitations, in which case enforcing a maximum number of ports at domain-definition-time might be the wrong thing to do. But since it is clear from the BZ that libvirt currently can modify the definition into an invalid unstartable state, it might be too late to go back on our word there. Advice and feedback appreciated, Jonathan [1] https://bugzilla.redhat.com/show_bug.cgi?id=1042505 [2] https://fedoraproject.org/wiki/Features/VirtioSerial?rd=VirtioSerial#Device Jonathan Lebon (3): add new function virDomainGetVirtioMaxPort check for out-of-range virtio-serial ports assign free controller to virtio-serial port src/conf/domain_conf.c | 77 ++++++++++++++++++++++++++++++++++++++++++-------- 1 file changed, 66 insertions(+), 11 deletions(-) -- 1.8.3.1
This function is then used to simplify port assignment. It will also be used in upcoming patches. --- src/conf/domain_conf.c | 30 ++++++++++++++++++++---------- 1 file changed, 20 insertions(+), 10 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index e65f3e3..2bade1b 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -10896,6 +10896,21 @@ virDomainDefMaybeAddController(virDomainDefPtr def, return 0; } +/* Returns the highest port number among all the virtio-serial channels + * using the controller with index idx (or -1 if no channel uses it). */ +static int +virDomainGetVirtioSerialMaxPort(virDomainDefPtr def, int idx) +{ + size_t i; + int maxport = -1; + for (i = 0; i < def->nchannels; i++) { + if (def->channels[i]->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_SERIAL && + def->channels[i]->info.addr.vioserial.controller == idx && + (int)def->channels[i]->info.addr.vioserial.port > maxport) + maxport = def->channels[i]->info.addr.vioserial.port; + } + return maxport; +} /* Parse a memory element located at XPATH within CTXT, and store the * result into MEM. If REQUIRED, then the value must exist; @@ -12233,18 +12248,13 @@ virDomainDefParseXML(xmlDocPtr xml, chr->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) chr->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_SERIAL; + /* assign next port available on controller */ if (chr->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_SERIAL && chr->info.addr.vioserial.port == 0) { - int maxport = 0; - size_t j; - 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; - } + int maxport = virDomainGetVirtioSerialMaxPort + (def, chr->info.addr.vioserial.controller); + if (maxport < 0) + maxport = 0; chr->info.addr.vioserial.port = maxport + 1; } } -- 1.8.3.1
Virtio-serial ports that have a number too high are rejected. The maximum port number is retrieved from the ports attribute of the controller if present, otherwise defaulting to 31. --- src/conf/domain_conf.c | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 2bade1b..bfb3a81 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -12257,6 +12257,26 @@ virDomainDefParseXML(xmlDocPtr xml, maxport = 0; chr->info.addr.vioserial.port = maxport + 1; } + + /* verify that port is not out of range */ + if (chr->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_SERIAL) { + size_t j; + int maxports = 31; + for (j = 0; j < def->ncontrollers; j++) { /* get maximum supported ports for ctlr */ + if (def->controllers[j]->type == VIR_DOMAIN_CONTROLLER_TYPE_VIRTIO_SERIAL && + def->controllers[j]->idx == chr->info.addr.vioserial.controller) { + maxports = (def->controllers[j]->opts.vioserial.ports != -1) + ? def->controllers[j]->opts.vioserial.ports : 31; + break; + } + } + if (chr->info.addr.vioserial.port >= maxports) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Port number %d of virtio-serial is too high (maximum is %d)"), + chr->info.addr.vioserial.port, maxports-1); + goto error; + } + } } VIR_FREE(nodes); -- 1.8.3.1
When no <address> element is explicitly given, the virtio-serial port is automatically assigned a controller which is known to have a free port available. --- src/conf/domain_conf.c | 27 ++++++++++++++++++++++++++- 1 file changed, 26 insertions(+), 1 deletion(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index bfb3a81..dbf7685 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -12243,10 +12243,35 @@ virDomainDefParseXML(xmlDocPtr xml, def->channels[def->nchannels++] = chr; + /* assign to next controller with free port */ if (chr->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CHANNEL && 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_NONE) { + size_t j; + int found = 0; + int maxctlrindex = -1; + for (j = 0; j < def->ncontrollers && !found; j++) { + if (def->controllers[j]->type == VIR_DOMAIN_CONTROLLER_TYPE_VIRTIO_SERIAL) { + int ports = (def->controllers[j]->opts.vioserial.ports != -1) + ? def->controllers[j]->opts.vioserial.ports : 31; + int maxport = virDomainGetVirtioSerialMaxPort + (def, def->controllers[j]->idx); + if (maxport < ports-1) { /* free port? */ + chr->info.addr.vioserial.controller = def->controllers[j]->idx; + found = 1; + } + if ((int)def->controllers[j]->idx > maxctlrindex) + maxctlrindex = def->controllers[j]->idx; + } + } + if (!found) { + maxctlrindex++; /* assign to next implicit controller with free ports */ + while (virDomainGetVirtioSerialMaxPort(def, maxctlrindex) >= 31-1) + maxctlrindex++; + chr->info.addr.vioserial.controller = maxctlrindex; + } chr->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_SERIAL; + } /* assign next port available on controller */ if (chr->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_SERIAL && -- 1.8.3.1
participants (1)
-
Jonathan Lebon