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 <[email protected]>
> ---
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.
