Re: [libvirt] [PATCH 2/2] Add QEMU support for virtio channel

On 18/02/10 12:39, Daniel P. Berrange wrote: > On Wed, Feb 17, 2010 at 05:11:01PM +0000, Matthew Booth wrote: >> Support virtio-serial controller and virtio channel in QEMU backend. Will output >> the following for virtio-serial controller: >> >> -device virtio-serial-pci,id=virtio-serial0,bus=pci.0,addr=0x4,max_ports=16,vectors=4 >> >> and the following for a virtio channel: >> >> -chardev pty,id=channel0 \ >> -device virtserialport,bus=virtio-serial0.0,chardev=channel0,name=org.linux-kvm.port.0 >> >> * src/qemu/qemu_conf.c: Add argument output for virtio >> * tests/qemuxml2argvtest.c >> tests/qemuxml2argvdata/qemuxml2argv-channel-virtio.args >> : Add test for QEMU command line generation >> --- >> src/qemu/qemu_conf.c | 91 +++++++++++++++++++- >> .../qemuxml2argv-channel-virtio.args | 1 + >> tests/qemuxml2argvtest.c | 1 + >> 3 files changed, 92 insertions(+), 1 deletions(-) >> create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-channel-virtio.args >> >> diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c >> index 456ee34..110409d 100644 >> --- a/src/qemu/qemu_conf.c >> +++ b/src/qemu/qemu_conf.c >> @@ -2168,7 +2168,6 @@ qemuAssignDevicePCISlots(virDomainDefPtr def, qemuDomainPCIAddressSetPtr addrs) >> } >> for (i = 0; i < def->nchannels ; i++) { >> /* Nada - none are PCI based (yet) */ >> - /* XXX virtio-serial will need one */ >> } >> if (def->watchdog && >> def->watchdog->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) { >> @@ -2465,6 +2464,23 @@ qemuBuildControllerDevStr(virDomainControllerDefPtr def) >> virBufferVSprintf(&buf, ",id=scsi%d", def->idx); >> break; >> >> + case VIR_DOMAIN_CONTROLLER_TYPE_VIRTIO_SERIAL: >> + if (def->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) { >> + virBufferAddLit(&buf, "virtio-serial-pci"); >> + } else { >> + virBufferAddLit(&buf, "virtio-serial"); >> + } >> + virBufferVSprintf(&buf, ",id=virtio-serial%d", def->idx); >> + if (def->opts.vioserial.max_ports != -1) { >> + virBufferVSprintf(&buf, ",max_ports=%d", >> + def->opts.vioserial.max_ports); >> + } >> + if (def->opts.vioserial.vectors != -1) { >> + virBufferVSprintf(&buf, ",vectors=%d", >> + def->opts.vioserial.vectors); >> + } >> + break; >> + >> /* We always get an IDE controller, whether we want it or not. */ >> case VIR_DOMAIN_CONTROLLER_TYPE_IDE: >> default: >> @@ -3830,6 +3846,79 @@ int qemudBuildCommandLine(virConnectPtr conn, >> } >> VIR_FREE(addr); >> ADD_ARG(devstr); >> + break; >> + >> + case VIR_DOMAIN_CHR_TARGET_TYPE_VIRTIO: >> + if (!(qemuCmdFlags & QEMUD_CMD_FLAG_DEVICE)) { >> + qemuReportError(VIR_ERR_NO_SUPPORT, "%s", >> + _("virtio channel requires QEMU to support -device")); >> + goto error; >> + } >> + >> + ADD_ARG_LIT("-chardev"); >> + if (!(devstr = qemuBuildChrChardevStr(channel))) >> + goto error; >> + ADD_ARG(devstr); > > It would be desirable to put the stuff that follows this point, into > a qemuBuildVirtioSerialPortDevStr() method, because the main > qemudBuildCommandLine() method is far too large & unwieldly these days. Would this comment still stand if the code below was simplified to hard-code alias?

>> + >> + virBuffer virtiodev = VIR_BUFFER_INITIALIZER; >> + virBufferAddLit(&virtiodev, "virtserialport"); >> + >> + if (channel->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) { >> + /* Check it's a virtio-serial address */ >> + if (channel->info.type != >> + VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_SERIAL) >> + { >> + qemuReportError(VIR_ERR_INTERNAL_ERROR, >> + _("virtio serial device has invalid " >> + "address type")); >> + virBufferFreeAndReset(&virtiodev); >> + goto error; >> + } >> + >> + /* Look for the virtio-serial controller */ >> + const char *vsalias = NULL; >> + int vs = 0; >> + int c; >> + for (c = 0; c < def->ncontrollers; c++) { >> + if (def->controllers[c]->type != >> + VIR_DOMAIN_CONTROLLER_TYPE_VIRTIO_SERIAL) >> + { >> + continue; >> + } >> + >> + if (vs == channel->info.addr.vioserial.controller) { >> + vsalias = def->controllers[c]->info.alias; >> + break; >> + } >> + >> + vs++; >> + } >> + >> + if (!vsalias) { >> + qemuReportError(VIR_ERR_INTERNAL_ERROR, >> + _("virtio serial device address controller " >> + "does not exist")); >> + virBufferFreeAndReset(&virtiodev); >> + goto error; >> + } > > We don't really need this loop / check there, since the XML parser routine > has already guarenteed that we have a controller associated with the port. > Just > >> + >> + virBufferVSprintf(&virtiodev, ",bus=%s.%d", >> + vsalias, channel->info.addr.vioserial.bus); > > This can just be > > virBufferVSprintf(&virtiodev, ",bus=virtio-serial%d.%d", > channel->info.addr.vioserial.controller, > channel->info.addr.vioserial.bus); Actually, this code is really to look for the controller's alias. Is it safe to hard-code it?

>> + } >> + >> + virBufferVSprintf(&virtiodev, ",chardev=%s", channel->info.alias); >> + if (channel->target.name) { >> + virBufferVSprintf(&virtiodev, ",name=%s", channel->target.name); >> + } >> + if (virBufferError(&virtiodev)) { >> + virReportOOMError(); >> + goto error; >> + } >> + >> + ADD_ARG_LIT("-device"); >> + ADD_ARG(virBufferContentAndReset(&virtiodev)); >> + >> + break; >> } >> } >>