[PATCH v4 0/2] Fix virtio console port assignment issue
Changelog: --- v4: - Update commit messages --- v3: - Added Reviewed-By - Included CI Results Link --- v2: - Split patch into two commits - Added fixes tag --- This libvirt patch does the following: 1. fixes an issue with virtio console device port assignment on vioserial buses 2. updates console port reservation comment and changes the allowZero variable to allowPortZero for clarity Currently in libvirt, a virtio console device cannot be assigned a port number greater than zero on a vioserial bus. This leads to port collision errors when adding more than 1 virtio console device on a single vioserial bus. After applying this patch, one can add multiple console ports under a single vioserial bus. Here is a link to CI results for this series: https://gitlab.com/aaronbmalik/libvirt/-/pipelines/1832324065 Aaron M. Brown (2): virDomainVirtioSerialAddrAssign: Fix virtio console port assignment on vioserial bus domain_addr.c: Update console port reservation comment and allowZero variable for clarity src/conf/domain_addr.c | 26 ++++++++++++++++---------- 1 file changed, 16 insertions(+), 10 deletions(-) -- 2.39.5 (Apple Git-154)
This change fixes an issue with virito console port assignment on vioserial buses. Currently, a virtio console device cannot be assigned to a port greater than 0 on vioserial buses. When trying to add more than one virtio console device on a single vioserial bus, you will get a port already exists with id 0 error. Therefore, the data needs to be passed back into info when allowZero is true Fixes: 16db8d2ec540 ("Add functions to track virtio-serial addresses") Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com> Signed-off-by: Aaron M. Brown <aaronmbr@linux.ibm.com> --- src/conf/domain_addr.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c index 8dfa8feca0..bc2b0f50e8 100644 --- a/src/conf/domain_addr.c +++ b/src/conf/domain_addr.c @@ -1737,6 +1737,12 @@ virDomainVirtioSerialAddrAssign(virDomainDef *def, if (virDomainVirtioSerialAddrNextFromController(addrs, &ptr->addr.vioserial) < 0) return -1; + + if (ptr == &nfo) { + /* pass the vioserial data back into info */ + info->addr.vioserial = ptr->addr.vioserial; + } + } else { if (virDomainVirtioSerialAddrNext(def, addrs, &ptr->addr.vioserial, allowZero) < 0) -- 2.39.5 (Apple Git-154)
On Thu, May 22, 2025 at 21:27:34 -0400, Aaron M. Brown wrote:
This change fixes an issue with virito console port assignment on vioserial buses. Currently, a virtio console device cannot be assigned to a port greater than 0 on vioserial buses. When trying to add more than one virtio console device on a single vioserial bus, you will get a port already exists with id 0 error.
Ideally paste the error verbatim here rather than paraphrasing it, so that it can be looked up.
Therefore, the data needs to be passed back into info when allowZero is true
Fixes: 16db8d2ec540 ("Add functions to track virtio-serial addresses") Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com> Signed-off-by: Aaron M. Brown <aaronmbr@linux.ibm.com> --- src/conf/domain_addr.c | 6 ++++++
A change in address allocation really requires a test case showing what's happening and more importantly prevents regressions in the future. Ideally add the test before this change showing old behaviour and this patch will then modify it to show how it was fixed. qemuxmlconftest should be able to demonstrate what's going on [1]
1 file changed, 6 insertions(+)
diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c index 8dfa8feca0..bc2b0f50e8 100644 --- a/src/conf/domain_addr.c +++ b/src/conf/domain_addr.c @@ -1737,6 +1737,12 @@ virDomainVirtioSerialAddrAssign(virDomainDef *def, if (virDomainVirtioSerialAddrNextFromController(addrs, &ptr->addr.vioserial) < 0) return -1; + + if (ptr == &nfo) { + /* pass the vioserial data back into info */
The comment should also say why you're passing it back
+ info->addr.vioserial = ptr->addr.vioserial; + } + } else { if (virDomainVirtioSerialAddrNext(def, addrs, &ptr->addr.vioserial, allowZero) < 0)
[1]: - add xml to tests/qemuxmlconfdata/ with configuration that shows the problem - invoke it from tests/qemuxmlconftest.c - use VIR_TEST_REGENERATE_OUTPUT=1 PATH_TO_BUILDDIR/tests/qemuxmlconftest to generate required output files
On a Monday in 2025, Peter Krempa via Devel wrote:
On Thu, May 22, 2025 at 21:27:34 -0400, Aaron M. Brown wrote:
This change fixes an issue with virito console port assignment on vioserial buses.
s/viritio/virtio/ Jano
Currently, a virtio console device cannot be assigned to a port greater than 0 on vioserial buses. When trying to add more than one virtio console device on a single vioserial bus, you will get a port already exists with id 0 error.
Ideally paste the error verbatim here rather than paraphrasing it, so that it can be looked up.
Therefore, the data needs to be passed back into info when allowZero is true
Fixes: 16db8d2ec540 ("Add functions to track virtio-serial addresses") Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com> Signed-off-by: Aaron M. Brown <aaronmbr@linux.ibm.com> --- src/conf/domain_addr.c | 6 ++++++
Thank you Ján, I have fixed this in the next version
Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com> Signed-off-by: Aaron M. Brown <aaronmbr@linux.ibm.com> --- src/conf/domain_addr.c | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c index bc2b0f50e8..86f5cfc337 100644 --- a/src/conf/domain_addr.c +++ b/src/conf/domain_addr.c @@ -1638,14 +1638,14 @@ static int virDomainVirtioSerialAddrNext(virDomainDef *def, virDomainVirtioSerialAddrSet *addrs, virDomainDeviceVirtioSerialAddress *addr, - bool allowZero) + bool allowPortZero) { ssize_t port, startPort = 0; ssize_t i; unsigned int controller; - /* port number 0 is reserved for virtconsoles */ - if (allowZero) + /* port number 0 is reserved for the first virtconsole */ + if (allowPortZero) startPort = -1; if (addrs->ncontrollers == 0) { @@ -1725,11 +1725,11 @@ static int ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3) virDomainVirtioSerialAddrAssign(virDomainDef *def, virDomainVirtioSerialAddrSet *addrs, virDomainDeviceInfo *info, - bool allowZero, + bool allowPortZero, bool portOnly) { virDomainDeviceInfo nfo = { 0 }; - virDomainDeviceInfo *ptr = allowZero ? &nfo : info; + virDomainDeviceInfo *ptr = allowPortZero ? &nfo : info; ptr->type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_SERIAL; @@ -1745,7 +1745,7 @@ virDomainVirtioSerialAddrAssign(virDomainDef *def, } else { if (virDomainVirtioSerialAddrNext(def, addrs, &ptr->addr.vioserial, - allowZero) < 0) + allowPortZero) < 0) return -1; } @@ -1764,20 +1764,20 @@ int virDomainVirtioSerialAddrAutoAssignFromCache(virDomainDef *def, virDomainVirtioSerialAddrSet *addrs, virDomainDeviceInfo *info, - bool allowZero) + bool allowPortZero) { bool portOnly = info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_SERIAL; if (info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_SERIAL && info->addr.vioserial.port) return virDomainVirtioSerialAddrReserve(NULL, NULL, info, addrs); else - return virDomainVirtioSerialAddrAssign(def, addrs, info, allowZero, portOnly); + return virDomainVirtioSerialAddrAssign(def, addrs, info, allowPortZero, portOnly); } int virDomainVirtioSerialAddrAutoAssign(virDomainDef *def, virDomainDeviceInfo *info, - bool allowZero) + bool allowPortZero) { virDomainVirtioSerialAddrSet *addrs = NULL; int ret = -1; @@ -1785,7 +1785,7 @@ virDomainVirtioSerialAddrAutoAssign(virDomainDef *def, if (!(addrs = virDomainVirtioSerialAddrSetCreateFromDomain(def))) goto cleanup; - if (virDomainVirtioSerialAddrAutoAssignFromCache(def, addrs, info, allowZero) < 0) + if (virDomainVirtioSerialAddrAutoAssignFromCache(def, addrs, info, allowPortZero) < 0) goto cleanup; ret = 0; -- 2.39.5 (Apple Git-154)
On Thu, May 22, 2025 at 21:27:33 -0400, Aaron M. Brown wrote:
Changelog: --- v4: - Update commit messages --- v3: - Added Reviewed-By - Included CI Results Link --- v2: - Split patch into two commits - Added fixes tag ---
Hi, I didn't find any of the 3 previous versions in the archives.
My apologies Peter, this is my first time upstream. The first two versions were internally reviewed
participants (4)
-
Aaron Brown -
Aaron M. Brown
-
Ján Tomko -
Peter Krempa