This change fixes an issue with virtio console port assignment on virtio-serial buses.
Currently, when trying to autoassign a virtio console device, the device cannot be
assigned to a port greater than 0 on virtio-serial buses.
You will receive the following error:
`virtio-serial-bus: A port already exists at id 0`
Therefore, the data needs to be passed back into info when allowZero is true.
We should also preserve the controller data when allowZero is true, and
propagate allowZero into virDomainVirtioSerialAddrNextFromController
to get an appropriate startPort.
Fixes: 16db8d2e ("Add functions to track virtio-serial addresses")
Signed-off-by: Aaron M. Brown <aaronmbr@linux.ibm.com>
---
src/conf/domain_addr.c | 30 ++++++++++++++++---
...rial-autoassign-address.x86_64-latest.args | 4 +--
...erial-autoassign-address.x86_64-latest.xml | 4 +--
3 files changed, 30 insertions(+), 8 deletions(-)
diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c
index 8dfa8feca0..5448d3d078 100644
--- a/src/conf/domain_addr.c
+++ b/src/conf/domain_addr.c
@@ -1692,12 +1692,16 @@ virDomainVirtioSerialAddrNext(virDomainDef *def,
static int
virDomainVirtioSerialAddrNextFromController(virDomainVirtioSerialAddrSet *addrs,
- virDomainDeviceVirtioSerialAddress *addr)
+ virDomainDeviceVirtioSerialAddress *addr,
+ bool allowZero)
{
- ssize_t port;
+ ssize_t port, startPort = 0;
ssize_t i;
virBitmap *map;
+ if (allowZero)
+ startPort = -1;
+
i = virDomainVirtioSerialAddrFindController(addrs, addr->controller);
if (i < 0) {
virReportError(VIR_ERR_INTERNAL_ERROR,
@@ -1707,7 +1711,7 @@ virDomainVirtioSerialAddrNextFromController(virDomainVirtioSerialAddrSet *addrs,
}
map = addrs->controllers[i]->ports;
- if ((port = virBitmapNextClearBit(map, 0)) <= 0) {
+ if ((port = virBitmapNextClearBit(map, startPort)) < 0) {
virReportError(VIR_ERR_XML_ERROR,
_("Unable to find a free port on virtio-serial controller %1$u"),
addr->controller);
@@ -1718,6 +1722,15 @@ virDomainVirtioSerialAddrNextFromController(virDomainVirtioSerialAddrSet *addrs,
addr->port = port;
VIR_DEBUG("Found free virtio serial controller %u port %u", addr->controller,
addr->port);
+
+ /* if this is the first virtconsole, reserve port 0 */
+ if (allowZero && port == 0) {
+ ignore_value(virBitmapSetBit(map, 0));
+ VIR_DEBUG(
+ "Port 0 reserved for the first virtconsole on vioserial controller %1$u",
+ addr->controller);
+ }
+
return 0;
}
@@ -1732,11 +1745,20 @@ virDomainVirtioSerialAddrAssign(virDomainDef *def,
virDomainDeviceInfo *ptr = allowZero ? &nfo : info;
ptr->type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_SERIAL;
+ ptr->addr.vioserial.controller = info->addr.vioserial.controller;
if (portOnly) {
if (virDomainVirtioSerialAddrNextFromController(addrs,
- &ptr->addr.vioserial) < 0)
+ &ptr->addr.vioserial,
+ allowZero) < 0)
return -1;
+
+ if (ptr == &nfo) {
+ /* pass the vioserial data back into info as info is used
+ * later for port assignment */
+ info->addr.vioserial = ptr->addr.vioserial;
+ }
+
} else {
if (virDomainVirtioSerialAddrNext(def, addrs, &ptr->addr.vioserial,
allowZero) < 0)
diff --git a/tests/qemuxmlconfdata/console-virtio-serial-autoassign-address.x86_64-latest.args b/tests/qemuxmlconfdata/console-virtio-serial-autoassign-address.x86_64-latest.args
index f987168477..4f4b95e969 100644
--- a/tests/qemuxmlconfdata/console-virtio-serial-autoassign-address.x86_64-latest.args
+++ b/tests/qemuxmlconfdata/console-virtio-serial-autoassign-address.x86_64-latest.args
@@ -35,9 +35,9 @@ XDG_CONFIG_HOME=/var/lib/libvirt/qemu/domain--1-QEMUGuest1/.config \
-chardev pty,id=charconsole1 \
-device '{"driver":"virtconsole","bus":"virtio-serial0.0","nr":0,"chardev":"charconsole1","id":"console1"}' \
-chardev pty,id=charconsole2 \
--device '{"driver":"virtconsole","bus":"virtio-serial0.0","nr":0,"chardev":"charconsole2","id":"console2"}' \
+-device '{"driver":"virtconsole","bus":"virtio-serial0.0","nr":1,"chardev":"charconsole2","id":"console2"}' \
-chardev pty,id=charconsole3 \
--device '{"driver":"virtconsole","bus":"virtio-serial0.0","nr":0,"chardev":"charconsole3","id":"console3"}' \
+-device '{"driver":"virtconsole","bus":"virtio-serial0.0","nr":2,"chardev":"charconsole3","id":"console3"}' \
-audiodev '{"id":"audio1","driver":"none"}' \
-device '{"driver":"virtio-balloon-pci","id":"balloon0","bus":"pci.0","addr":"0x3"}' \
-sandbox on,obsolete=deny,elevateprivileges=deny,spawn=deny,resourcecontrol=deny \
diff --git a/tests/qemuxmlconfdata/console-virtio-serial-autoassign-address.x86_64-latest.xml b/tests/qemuxmlconfdata/console-virtio-serial-autoassign-address.x86_64-latest.xml
index acee9999af..712d6c8a56 100644
--- a/tests/qemuxmlconfdata/console-virtio-serial-autoassign-address.x86_64-latest.xml
+++ b/tests/qemuxmlconfdata/console-virtio-serial-autoassign-address.x86_64-latest.xml
@@ -47,11 +47,11 @@
</console>
<console type='pty'>
<target type='virtio' port='2'/>
- <address type='virtio-serial' controller='0' bus='0' port='0'/>
+ <address type='virtio-serial' controller='0' bus='0' port='1'/>
</console>
<console type='pty'>
<target type='virtio' port='3'/>
- <address type='virtio-serial' controller='0' bus='0' port='0'/>
+ <address type='virtio-serial' controller='0' bus='0' port='2'/>
</console>
<input type='mouse' bus='ps2'/>
<input type='keyboard' bus='ps2'/>
--
2.39.5 (Apple Git-154)On Tue, Jul 22, 2025 at 13:50:19 -0400, Aaron M. Brown wrote:
> This change fixes an issue with virtio console port assignment on virtio-serial buses.
> Currently, when trying to autoassign a virtio console device, the device cannot be
> assigned to a port greater than 0 on virtio-serial buses.
> You will receive the following error:
>
> `virtio-serial-bus: A port already exists at id 0`
>
> Therefore, the data needs to be passed back into info when allowZero is true.
> We should also preserve the controller data when allowZero is true, and
> propagate allowZero into virDomainVirtioSerialAddrNextFromController
> to get an appropriate startPort.
>
> Fixes: 16db8d2e ("Add functions to track virtio-serial addresses")
> Signed-off-by: Aaron M. Brown <aaronmbr@linux.ibm.com>
> ---
> src/conf/domain_addr.c | 30 ++++++++++++++++---
> ...rial-autoassign-address.x86_64-latest.args | 4 +--
> ...erial-autoassign-address.x86_64-latest.xml | 4 +--
> 3 files changed, 30 insertions(+), 8 deletions(-)
>
> diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c
> index 8dfa8feca0..5448d3d078 100644
> --- a/src/conf/domain_addr.c
> +++ b/src/conf/domain_addr.c
> @@ -1692,12 +1692,16 @@ virDomainVirtioSerialAddrNext(virDomainDef *def,
>
> static int
> virDomainVirtioSerialAddrNextFromController(virDomainVirtioSerialAddrSet *addrs,
> - virDomainDeviceVirtioSerialAddress *addr)
> + virDomainDeviceVirtioSerialAddress *addr,
> + bool allowZero)
> {
> - ssize_t port;
> + ssize_t port, startPort = 0;
One definition per line.
> ssize_t i;
> virBitmap *map;
>
> + if (allowZero)
> + startPort = -1;
> +
> i = virDomainVirtioSerialAddrFindController(addrs, addr->controller);
> if (i < 0) {
> virReportError(VIR_ERR_INTERNAL_ERROR,
> @@ -1707,7 +1711,7 @@ virDomainVirtioSerialAddrNextFromController(virDomainVirtioSerialAddrSet *addrs,
> }
>
> map = addrs->controllers[i]->ports;
> - if ((port = virBitmapNextClearBit(map, 0)) <= 0) {
> + if ((port = virBitmapNextClearBit(map, startPort)) < 0) {
> virReportError(VIR_ERR_XML_ERROR,
> _("Unable to find a free port on virtio-serial controller %1$u"),
> addr->controller);
> @@ -1718,6 +1722,15 @@ virDomainVirtioSerialAddrNextFromController(virDomainVirtioSerialAddrSet *addrs,
> addr->port = port;
> VIR_DEBUG("Found free virtio serial controller %u port %u", addr->controller,
> addr->port);
> +
> + /* if this is the first virtconsole, reserve port 0 */
> + if (allowZero && port == 0) {
> + ignore_value(virBitmapSetBit(map, 0));
> + VIR_DEBUG(
> + "Port 0 reserved for the first virtconsole on vioserial controller %1$u",
> + addr->controller);
> + }
This is suspicious because this function until now just returned the
next eligible port, but now it's also reserving it which doesn't seem to
fit well here.
Can you please explain this?
Also the spacing is weird, but I can fix that.
> On Tue, Jul 22, 2025 at 13:50:19 -0400, Aaron M. Brown wrote: > > One definition per line. > Okay I will remember that going forward! > > This is suspicious because this function until now just returned the > next eligible port, but now it's also reserving it which doesn't seem to > fit well here. > > Can you please explain this? > Yes, I agree with you Peter, it is a bit unfitting, we can't process a port assigned to port 0 in virDomainVirtioSerialAddrReserve as it fails the virDomainVirtioSerialAddrIsComplete check, because that check makes sure that `info->addr.vioserial.port != 0` so such a port would show as imcomplete. And if we skip over reserving this port, that means the bitmap would show a clear bit at the first position for every subsequent device on the controller
On Thu, Jul 24, 2025 at 12:48:06 -0000, Aaron Brown wrote: > > On Tue, Jul 22, 2025 at 13:50:19 -0400, Aaron M. Brown wrote: > > > > One definition per line. > > > Okay I will remember that going forward! > > > > > This is suspicious because this function until now just returned the > > next eligible port, but now it's also reserving it which doesn't seem to > > fit well here. > > > > Can you please explain this? > > > Yes, I agree with you Peter, it is a bit unfitting, we can't process a port assigned to port 0 in virDomainVirtioSerialAddrReserve as it fails the virDomainVirtioSerialAddrIsComplete check, because that check makes sure that `info->addr.vioserial.port != 0` so such a port would show as imcomplete. > > And if we skip over reserving this port, that means the bitmap would show a clear bit at the first position for every subsequent device on the controller So while I agree that it needs to work properly with the reservation code, with the way the code is structured I don't think we should hide something which reserves ports into code that is not expected to do that.
> So while I agree that it needs to work properly with the reservation > code, with the way the code is structured I don't think we should hide > something which reserves ports into code that is not expected to do > that. Yeah, I see what you're saying. Given this, there are some options available to us. 1. We could move that conditional out into virDomainVirtioSerialAddrAssign, that might require a similar query for the controller, to ultimately get the map. 2. We could possibly modify virDomainVirtioSerialAddrReserve, and/or virDomainVirtioSerialAddrIsComplete Im open to any other suggestions as well ! Regards, Aaron
© 2016 - 2025 Red Hat, Inc.