On Thu, Feb 18, 2010 at 01:23:57PM +0000, Matthew Booth wrote:
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?
Yep, because you'll want this method to exist if you ever add the hot-plug support, since that uses the exact same syntax as the main -device arg these days. Which is nice :-)
+ + 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?
Yep, we do for disks. If you want though, you could add more #defines to src/qemu/qemu_conf.h for that, alongside the existing #define QEMU_DRIVE_HOST_PREFIX "drive-" eg, #define QEMU_VIRTIO_SERIAL_PREFIX "virtio-serial" and reference that constant in both places
+ } + + 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; } }
Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|