On Fri, Apr 03, 2026 at 14:33:29 +0200, Roman Bogorodskiy via Devel wrote:
Bhyve supports virtio-console devices using the following syntax:
-s 2:0,virtio-console,org.qemu.guest_agent.0=/path/to/unix/socket,other.port=/other/socket,...
There are two details about that to consider.
The first one is that only up to 16 ports per console is supported. This is different from the default (31), so update the code to manually add the virtio-serial controllers with 16 ports. For the existing controllers, make sure to set max ports to 16 or error out if ports count greater than 16 was specified.
The second one is that bhyve does not clean up UNIX sockets for these devices. So update virBhyveProcessStop() to remove leftover sockets.
Not adding capabilities probing as the virtio-console device is available on all supported FreeBSD versions and on all supported arches.
Signed-off-by: Roman Bogorodskiy <bogorodskiy@gmail.com> ---
Disclaimer: I don't have setup to test this. Unfortunately not even the test suite (bhyvexml2argvtest) seems to run on Linux.
diff --git a/src/bhyve/bhyve_command.c b/src/bhyve/bhyve_command.c index 931d7dd551..8671644cc8 100644 --- a/src/bhyve/bhyve_command.c +++ b/src/bhyve/bhyve_command.c @@ -490,6 +490,43 @@ bhyveBuildNVMeControllerArgStr(const virDomainDef *def, return 0; }
+static int +bhyveBuildVirtioSerialControllerArgStr(const virDomainDef *def, + virDomainControllerDef *controller, + struct _bhyveConn *driver G_GNUC_UNUSED, + virCommand *cmd) +{ + g_auto(virBuffer) opt = VIR_BUFFER_INITIALIZER; + size_t i; + + for (i = 0; i < def->nchannels; i++) { + virDomainChrDef *channel = def->channels[i]; + + if (channel->info.addr.vioserial.controller != controller->idx) + continue; + + if (channel->source->type != VIR_DOMAIN_CHR_TYPE_UNIX) + continue; + + if (channel->targetType != VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_VIRTIO) + continue; + + virBufferAsprintf(&opt, + ",%s=%s", + channel->target.name, + channel->source->data.nix.path); + }
Doesn't bhyve, similarly to qemu need to escape any occurence of ',' in the fiule name?
+ + if (virBufferUse(&opt) > 0) { + virCommandAddArg(cmd, "-s"); + virCommandAddArgFormat(cmd, "%d:0,virtio-console%s", + controller->info.addr.pci.slot, + virBufferContentAndReset(&opt));
This results in: +-s 2:0,virtio-console,org.qemu.guest_agent.0=/var/run/libvirt/bhyve/bhyve.agent,org.qemu.guest_agent.1=/var/run/libvirt/bhyve/bhyve.agent-2 \ Since the paths are passed from the XML and AFAIU not actually validated to be devoid of ',' chars, not escaping them will break the output if the path contains a ','.
+ } + + return 0; +} + static int bhyveBuildVirtIODiskArgStr(const virDomainDef *def G_GNUC_UNUSED, virDomainDiskDef *disk, @@ -606,9 +643,12 @@ bhyveBuildControllerArgStr(const virDomainDef *def, if (bhyveBuildNVMeControllerArgStr(def, controller, driver, cmd) < 0) return -1; break; + case VIR_DOMAIN_CONTROLLER_TYPE_VIRTIO_SERIAL: + if (bhyveBuildVirtioSerialControllerArgStr(def, controller, driver, cmd) < 0) + return -1; + break; case VIR_DOMAIN_CONTROLLER_TYPE_IDE: case VIR_DOMAIN_CONTROLLER_TYPE_FDC: - case VIR_DOMAIN_CONTROLLER_TYPE_VIRTIO_SERIAL: case VIR_DOMAIN_CONTROLLER_TYPE_CCID: case VIR_DOMAIN_CONTROLLER_TYPE_XENBUS: case VIR_DOMAIN_CONTROLLER_TYPE_LAST: diff --git a/src/bhyve/bhyve_device.c b/src/bhyve/bhyve_device.c index 54511383bb..cf3a9fa4fb 100644 --- a/src/bhyve/bhyve_device.c +++ b/src/bhyve/bhyve_device.c @@ -31,6 +31,36 @@
VIR_LOG_INIT("bhyve.bhyve_device");
+ +static int +bhyveDomainAssignVirtioSerialAddresses(virDomainDef *def) +{ + int ret = -1; + size_t i; + virDomainVirtioSerialAddrSet *addrs = NULL; + + if (!(addrs = virDomainVirtioSerialAddrSetCreateFromDomain(def))) + goto cleanup; + + VIR_DEBUG("Finished reserving existing ports"); + + for (i = 0; i < def->nchannels; i++) { + virDomainChrDef *chr = def->channels[i]; + if (chr->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CHANNEL && + chr->targetType == VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_VIRTIO && + !virDomainVirtioSerialAddrIsComplete(&chr->info) && + virDomainVirtioSerialAddrAutoAssignFromCache(def, addrs, + &chr->info, false) < 0) + goto cleanup; + } + + ret = 0; + + cleanup: + virDomainVirtioSerialAddrSetFree(addrs); + return ret; +} + static int bhyveCollectPCIAddress(virDomainDef *def G_GNUC_UNUSED, virDomainDeviceDef *device G_GNUC_UNUSED, @@ -39,7 +69,8 @@ bhyveCollectPCIAddress(virDomainDef *def G_GNUC_UNUSED, { virDomainPCIAddressSet *addrs = NULL; virPCIDeviceAddress *addr = NULL; - if (info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE) + + if (info->type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) return 0;
This almost qualifies as a separate fix :)
@@ -116,6 +147,7 @@ bhyveAssignDevicePCISlots(virDomainDef *def, (def->controllers[i]->type == VIR_DOMAIN_CONTROLLER_TYPE_SATA) || (def->controllers[i]->type == VIR_DOMAIN_CONTROLLER_TYPE_NVME) || (def->controllers[i]->type == VIR_DOMAIN_CONTROLLER_TYPE_SCSI) || + (def->controllers[i]->type == VIR_DOMAIN_CONTROLLER_TYPE_VIRTIO_SERIAL) || ((def->controllers[i]->type == VIR_DOMAIN_CONTROLLER_TYPE_USB) && (def->controllers[i]->model == VIR_DOMAIN_CONTROLLER_MODEL_USB_NEC_XHCI)) || def->controllers[i]->type == VIR_DOMAIN_CONTROLLER_TYPE_ISA) { @@ -242,5 +274,11 @@ int bhyveDomainAssignPCIAddresses(virDomainDef *def,
int bhyveDomainAssignAddresses(virDomainDef *def, virDomainObj *obj) { - return bhyveDomainAssignPCIAddresses(def, obj); + if (bhyveDomainAssignVirtioSerialAddresses(def) < 0) + return -1; + + if (bhyveDomainAssignPCIAddresses(def, obj) < 0) + return -1; + + return 0; } diff --git a/src/bhyve/bhyve_domain.c b/src/bhyve/bhyve_domain.c index 4594d7673f..e64be1f4a7 100644 --- a/src/bhyve/bhyve_domain.c +++ b/src/bhyve/bhyve_domain.c @@ -89,6 +89,9 @@ bhyveDomainDefPostParse(virDomainDef *def, { struct _bhyveConn *driver = opaque; g_autoptr(virCaps) caps = bhyveDriverGetCapabilities(driver); + size_t i, virtio_channels = 0; + size_t virtio_serial_controllers = 0, virtio_serial_existing_controllers = 0; + size_t virtio_serial_controllers_to_create = 0;
Avoid multiple declarations on a signle line.
if (!caps) return -1;
@@ -141,6 +144,27 @@ bhyveDomainDefPostParse(virDomainDef *def, def->os.loader->type = VIR_DOMAIN_LOADER_TYPE_ROM; }
+ for (i = 0; i < def->nchannels; i++) + if (def->channels[i]->targetType == VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_VIRTIO) + virtio_channels++; + + for (i = 0; i < def->ncontrollers; i++) + if (def->controllers[i]->type == VIR_DOMAIN_CONTROLLER_TYPE_VIRTIO_SERIAL) + virtio_serial_existing_controllers++; + + /* bhyve supports 16 ports per virtio-console device */ + virtio_serial_controllers = (virtio_channels / 16) + (virtio_channels % 16 != 0); + if (virtio_serial_controllers > virtio_serial_existing_controllers) { + virtio_serial_controllers_to_create = virtio_serial_controllers - virtio_serial_existing_controllers; + + for (i = 0; i < virtio_serial_controllers_to_create; i++) { + virDomainControllerDef *cont; + + cont = virDomainDefAddController(def, VIR_DOMAIN_CONTROLLER_TYPE_VIRTIO_SERIAL, -1, -1); + cont->opts.vioserial.ports = 16; + } + } + return 0; }
@@ -225,6 +249,10 @@ bhyveDomainDeviceDefPostParse(virDomainDeviceDef *dev, virReportError(VIR_ERR_XML_ERROR, "%s", _("pci-root and pcie-root controllers should have index 0")); return -1; + } else if (cont->type == VIR_DOMAIN_CONTROLLER_TYPE_VIRTIO_SERIAL) { + /* bhyve supports 16 ports per controller */ + if (cont->opts.vioserial.ports == -1) + cont->opts.vioserial.ports = 16; } }
@@ -287,13 +315,21 @@ bhyveDomainDeviceDefValidate(const virDomainDeviceDef *dev, void *parseOpaque G_GNUC_UNUSED) { switch (dev->type) { - case VIR_DOMAIN_DEVICE_CONTROLLER: - if (dev->data.controller->type == VIR_DOMAIN_CONTROLLER_TYPE_ISA && - dev->data.controller->idx != 0) { + case VIR_DOMAIN_DEVICE_CONTROLLER: { + virDomainControllerDef *controller = dev->data.controller; + + if (controller->type == VIR_DOMAIN_CONTROLLER_TYPE_ISA && + controller->idx != 0) { return -1;
This branch doesn't report errors ...
+ } else if (controller->type == VIR_DOMAIN_CONTROLLER_TYPE_VIRTIO_SERIAL) { + if (controller->opts.vioserial.ports > 16) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("Bhyve virtio-serial controller supports up to 16 ports")); + return -1;
... but this does.
+ } } break; - + } case VIR_DOMAIN_DEVICE_RNG: if (dev->data.rng->model == VIR_DOMAIN_RNG_MODEL_VIRTIO) { if (dev->data.rng->backend == VIR_DOMAIN_RNG_BACKEND_RANDOM) { diff --git a/src/bhyve/bhyve_process.c b/src/bhyve/bhyve_process.c index 1d436da609..711d1b66ea 100644 --- a/src/bhyve/bhyve_process.c +++ b/src/bhyve/bhyve_process.c @@ -473,6 +473,7 @@ virBhyveProcessStop(struct _bhyveConn *driver, virDomainObj *vm, virDomainShutoffReason reason) { + size_t i = 0; int ret = -1; g_autoptr(virCommand) cmd = NULL; bhyveDomainObjPrivate *priv = vm->privateData; @@ -512,6 +513,19 @@ virBhyveProcessStop(struct _bhyveConn *driver, } }
+ /* UNIX sockets cleanup */ + for (i = 0; i < vm->def->nchannels; i++) { + virDomainChrDef *channel = vm->def->channels[i]; + + if (channel->source->type != VIR_DOMAIN_CHR_TYPE_UNIX) + continue; + if (channel->targetType != VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_VIRTIO) + continue; + + if (virFileExists(channel->source->data.nix.path)) + virFileRemove(channel->source->data.nix.path, 0, 0);
Not a problem of this patch but the 3 code paths at the beginning of this function skip the rest of the cleanup so some resources such as allocated ports could stay around if the function fails. if (!virDomainObjIsActive(vm)) { VIR_DEBUG("VM '%s' not active", vm->def->name); return 0; } if (vm->pid == 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Invalid PID %1$d for VM"), (int)vm->pid); return -1; } if (!(cmd = virBhyveProcessBuildDestroyCmd(driver, vm->def))) return -1;
+ } + ret = 0;
virCloseCallbacksDomainRemove(vm, NULL, bhyveProcessAutoDestroy);
The rest looks good. The open question is (of this patch) is only wrt the escaping of the formatted string.