9:29 a.m.
On 18/02/10 14:11, Daniel P. Berrange wrote:
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;
>>> }
>>> }
>>>
Update patch attached.
Matt
--
Matthew Booth, RHCA, RHCSS
Red Hat Engineering, Virtualisation Team
M: +44 (0)7977 267231
GPG ID: D33C3490
GPG FPR: 3733 612D 2D05 5458 8A8A 1600 3441 EA19 D33C 3490