[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



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
>From 9602171c257f038758fb624c0018c469c8c7f5e0 Mon Sep 17 00:00:00 2001
From: Matthew Booth <mbooth redhat com>
Date: Thu, 12 Nov 2009 17:47:15 +0000
Subject: [PATCH 2/2] Add QEMU support for virtio channel

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                               |   77 +++++++++++++++++++-
 src/qemu/qemu_conf.h                               |    3 +
 .../qemuxml2argv-channel-virtio.args               |    1 +
 tests/qemuxml2argvtest.c                           |    1 +
 4 files changed, 81 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..7e96102 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,24 @@ 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=" QEMU_VIRTIO_SERIAL_PREFIX "%d",
+                          def->idx);
+        if (def->opts.vioserial.ports != -1) {
+            virBufferVSprintf(&buf, ",max_ports=%d",
+                              def->opts.vioserial.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:
@@ -2969,6 +2986,45 @@ error:
 }
 
 
+char *
+qemuBuildVirtioSerialPortDevStr(virDomainChrDefPtr dev)
+{
+    virBuffer buf = VIR_BUFFER_INITIALIZER;
+    virBufferAddLit(&buf, "virtserialport");
+
+    if (dev->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) {
+        /* Check it's a virtio-serial address */
+        if (dev->info.type !=
+            VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_SERIAL)
+        {
+            qemuReportError(VIR_ERR_INTERNAL_ERROR,
+                            _("virtio serial device has invalid address type"));
+            goto error;
+        }
+
+        virBufferVSprintf(&buf,
+                          ",bus=" QEMU_VIRTIO_SERIAL_PREFIX "%d.%d",
+                          dev->info.addr.vioserial.controller,
+                          dev->info.addr.vioserial.bus);
+    }
+
+    virBufferVSprintf(&buf, ",chardev=%s", dev->info.alias);
+    if (dev->target.name) {
+        virBufferVSprintf(&buf, ",name=%s", dev->target.name);
+    }
+    if (virBufferError(&buf)) {
+        virReportOOMError();
+        goto error;
+    }
+
+    return virBufferContentAndReset(&buf);
+
+error:
+    virBufferFreeAndReset(&buf);
+    return NULL;
+}
+
+
 static int
 qemuBuildCpuArgStr(const struct qemud_driver *driver,
                    const virDomainDefPtr def,
@@ -3830,6 +3886,25 @@ 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);
+
+            ADD_ARG_LIT("-device");
+            if (!(devstr = qemuBuildVirtioSerialPortDevStr(channel)))
+                goto error;
+            ADD_ARG(devstr);
+            break;
         }
     }
 
diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h
index 7041489..4f35a9c 100644
--- a/src/qemu/qemu_conf.h
+++ b/src/qemu/qemu_conf.h
@@ -158,6 +158,7 @@ typedef qemuDomainPCIAddressSet *qemuDomainPCIAddressSetPtr;
 #define QEMU_CONFIG_FORMAT_ARGV "qemu-argv"
 
 #define QEMU_DRIVE_HOST_PREFIX "drive-"
+#define QEMU_VIRTIO_SERIAL_PREFIX "virtio-serial"
 
 #define qemuReportError(code, fmt...)                                   \
     virReportErrorHelper(NULL, VIR_FROM_QEMU, code, __FILE__,           \
@@ -234,6 +235,8 @@ char * qemuBuildChrChardevStr(virDomainChrDefPtr dev);
 /* Legacy, pre device support */
 char * qemuBuildChrArgStr(virDomainChrDefPtr dev, const char *prefix);
 
+char * qemuBuildVirtioSerialPortDevStr(virDomainChrDefPtr dev);
+
 /* Legacy, pre device support */
 char * qemuBuildUSBHostdevUsbDevStr(virDomainHostdevDefPtr dev);
 /* Current, best practice */
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-channel-virtio.args b/tests/qemuxml2argvdata/qemuxml2argv-channel-virtio.args
new file mode 100644
index 0000000..4097065
--- /dev/null
+++ b/tests/qemuxml2argvdata/qemuxml2argv-channel-virtio.args
@@ -0,0 +1 @@
+LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test /usr/bin/qemu -S -M pc -m 214 -smp 1 -nographic -nodefaults -monitor unix:/tmp/test-monitor,server,nowait -no-acpi -boot c -device virtio-serial-pci,id=virtio-serial0,max_ports=16,vectors=4,bus=pci.0,addr=0x4 -device virtio-serial-pci,id=virtio-serial1,bus=pci.0,addr=0xa -hda /dev/HostVG/QEMUGuest1 -chardev pty,id=channel0 -device virtserialport,chardev=channel0,name=org.linux-kvm.port.0 -chardev pty,id=channel1 -device virtserialport,bus=virtio-serial1.0,chardev=channel1,name=org.linux-kvm.port.1 -usb -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index aa42f99..a8c9ed3 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -318,6 +318,7 @@ mymain(int argc, char **argv)
     DO_TEST("console-compat-chardev", QEMUD_CMD_FLAG_CHARDEV|QEMUD_CMD_FLAG_DEVICE);
 
     DO_TEST("channel-guestfwd", QEMUD_CMD_FLAG_CHARDEV|QEMUD_CMD_FLAG_DEVICE);
+    DO_TEST("channel-virtio", QEMUD_CMD_FLAG_DEVICE);
 
     DO_TEST("watchdog", 0);
     DO_TEST("watchdog-device", QEMUD_CMD_FLAG_DEVICE);
-- 
1.6.6


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]