[Qemu-devel] [PULL #7 0/7] virtio-serial fixes, enhancements

[Qemu-devel] [PULL #7 0/7] virtio-serial fixes, enhancements

[Amit Shah]

Hello,

This series fixes a few bugs reported against virtio-serial.  Please
apply.

The following changes since commit e0efb993b817564ef84e462ac1fe35f89b57ad7b:

  Fix conversions from pointer to int and vice versa (2011-03-20 21:39:23 +0000)

are available in the git repository at:
  git://git.kernel.org/pub/scm/virt/qemu/amit/virtio-serial.git for-anthony

Amit Shah (7):
  virtio-serial: Use a struct to pass config information from proxy
  virtio-serial: Disallow generic ports at id 0
  virtio-serial: Enable ioeventfd
  virtio-serial-bus: Simplify handle_output() function
  virtio-serial: Don't clear ->have_data() pointer after unplug
  virtio-console: Keep chardev open for other users after hot-unplug
  char: Prevent multiple devices opening same chardev

 hw/qdev-properties.c   |    7 ++++++-
 hw/virtio-console.c    |   16 ++++++++++++++--
 hw/virtio-pci.c        |   15 +++++++++------
 hw/virtio-serial-bus.c |   28 +++++++++++-----------------
 hw/virtio-serial.h     |    5 +++++
 hw/virtio.h            |    3 ++-
 qemu-char.c            |    4 ++++
 qemu-char.h            |    1 +
 8 files changed, 52 insertions(+), 27 deletions(-)

-- 
1.7.4

[Qemu-devel] [PATCH 1/7] virtio-serial: Use a struct to pass config information from proxy

[Amit Shah]

Instead of using a single variable to pass to the virtio_serial_init
function, use a struct so that expanding the number of variables to be
passed on later is easier.

Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 hw/virtio-pci.c        |   12 ++++++------
 hw/virtio-serial-bus.c |   16 ++++++++--------
 hw/virtio-serial.h     |    5 +++++
 hw/virtio.h            |    3 ++-
 4 files changed, 21 insertions(+), 15 deletions(-)

diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
index 3911b09..952b5d2 100644
--- a/hw/virtio-pci.c
+++ b/hw/virtio-pci.c
@@ -18,6 +18,7 @@
 #include "virtio.h"
 #include "virtio-blk.h"
 #include "virtio-net.h"
+#include "virtio-serial.h"
 #include "pci.h"
 #include "qemu-error.h"
 #include "msix.h"
@@ -109,8 +110,7 @@ typedef struct {
 #ifdef CONFIG_LINUX
     V9fsConf fsconf;
 #endif
-    /* Max. number of ports we can have for a the virtio-serial device */
-    uint32_t max_virtserial_ports;
+    virtio_serial_conf serial;
     virtio_net_conf net;
     bool ioeventfd_disabled;
     bool ioeventfd_started;
@@ -770,12 +770,12 @@ static int virtio_serial_init_pci(PCIDevice *pci_dev)
         proxy->class_code != PCI_CLASS_OTHERS)          /* qemu-kvm  */
         proxy->class_code = PCI_CLASS_COMMUNICATION_OTHER;
 
-    vdev = virtio_serial_init(&pci_dev->qdev, proxy->max_virtserial_ports);
+    vdev = virtio_serial_init(&pci_dev->qdev, &proxy->serial);
     if (!vdev) {
         return -1;
     }
     vdev->nvectors = proxy->nvectors == DEV_NVECTORS_UNSPECIFIED
-                                        ? proxy->max_virtserial_ports + 1
+                                        ? proxy->serial.max_virtserial_ports + 1
                                         : proxy->nvectors;
     virtio_init_pci(proxy, vdev,
                     PCI_VENDOR_ID_REDHAT_QUMRANET,
@@ -902,8 +902,8 @@ static PCIDeviceInfo virtio_info[] = {
                                DEV_NVECTORS_UNSPECIFIED),
             DEFINE_PROP_HEX32("class", VirtIOPCIProxy, class_code, 0),
             DEFINE_VIRTIO_COMMON_FEATURES(VirtIOPCIProxy, host_features),
-            DEFINE_PROP_UINT32("max_ports", VirtIOPCIProxy, max_virtserial_ports,
-                               31),
+            DEFINE_PROP_UINT32("max_ports", VirtIOPCIProxy,
+                               serial.max_virtserial_ports, 31),
             DEFINE_PROP_END_OF_LIST(),
         },
         .qdev.reset = virtio_pci_reset,
diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c
index 8446bc2..c6feb43 100644
--- a/hw/virtio-serial-bus.c
+++ b/hw/virtio-serial-bus.c
@@ -811,19 +811,19 @@ void virtio_serial_port_qdev_register(VirtIOSerialPortInfo *info)
     qdev_register(&info->qdev);
 }
 
-VirtIODevice *virtio_serial_init(DeviceState *dev, uint32_t max_nr_ports)
+VirtIODevice *virtio_serial_init(DeviceState *dev, virtio_serial_conf *conf)
 {
     VirtIOSerial *vser;
     VirtIODevice *vdev;
     uint32_t i, max_supported_ports;
 
-    if (!max_nr_ports)
+    if (!conf->max_virtserial_ports)
         return NULL;
 
     /* Each port takes 2 queues, and one pair is for the control queue */
     max_supported_ports = VIRTIO_PCI_QUEUE_MAX / 2 - 1;
 
-    if (max_nr_ports > max_supported_ports) {
+    if (conf->max_virtserial_ports > max_supported_ports) {
         error_report("maximum ports supported: %u", max_supported_ports);
         return NULL;
     }
@@ -839,9 +839,9 @@ VirtIODevice *virtio_serial_init(DeviceState *dev, uint32_t max_nr_ports)
     vser->bus->vser = vser;
     QTAILQ_INIT(&vser->ports);
 
-    vser->bus->max_nr_ports = max_nr_ports;
-    vser->ivqs = qemu_malloc(max_nr_ports * sizeof(VirtQueue *));
-    vser->ovqs = qemu_malloc(max_nr_ports * sizeof(VirtQueue *));
+    vser->bus->max_nr_ports = conf->max_virtserial_ports;
+    vser->ivqs = qemu_malloc(conf->max_virtserial_ports * sizeof(VirtQueue *));
+    vser->ovqs = qemu_malloc(conf->max_virtserial_ports * sizeof(VirtQueue *));
 
     /* Add a queue for host to guest transfers for port 0 (backward compat) */
     vser->ivqs[0] = virtio_add_queue(vdev, 128, handle_input);
@@ -866,8 +866,8 @@ VirtIODevice *virtio_serial_init(DeviceState *dev, uint32_t max_nr_ports)
         vser->ovqs[i] = virtio_add_queue(vdev, 128, handle_output);
     }
 
-    vser->config.max_nr_ports = max_nr_ports;
-    vser->ports_map = qemu_mallocz(((max_nr_ports + 31) / 32)
+    vser->config.max_nr_ports = conf->max_virtserial_ports;
+    vser->ports_map = qemu_mallocz(((conf->max_virtserial_ports + 31) / 32)
         * sizeof(vser->ports_map[0]));
     /*
      * Reserve location 0 for a console port for backward compat
diff --git a/hw/virtio-serial.h b/hw/virtio-serial.h
index 8cb9fbe..5eb948e 100644
--- a/hw/virtio-serial.h
+++ b/hw/virtio-serial.h
@@ -45,6 +45,11 @@ struct virtio_console_control {
     uint16_t value;		/* Extra information for the key */
 };
 
+struct virtio_serial_conf {
+    /* Max. number of ports we can have for a virtio-serial device */
+    uint32_t max_virtserial_ports;
+};
+
 /* Some events for the internal messages (control packets) */
 #define VIRTIO_CONSOLE_DEVICE_READY	0
 #define VIRTIO_CONSOLE_PORT_ADD		1
diff --git a/hw/virtio.h b/hw/virtio.h
index 31d16e1..d0920a8 100644
--- a/hw/virtio.h
+++ b/hw/virtio.h
@@ -195,7 +195,8 @@ VirtIODevice *virtio_blk_init(DeviceState *dev, BlockConf *conf);
 struct virtio_net_conf;
 VirtIODevice *virtio_net_init(DeviceState *dev, NICConf *conf,
                               struct virtio_net_conf *net);
-VirtIODevice *virtio_serial_init(DeviceState *dev, uint32_t max_nr_ports);
+typedef struct virtio_serial_conf virtio_serial_conf;
+VirtIODevice *virtio_serial_init(DeviceState *dev, virtio_serial_conf *serial);
 VirtIODevice *virtio_balloon_init(DeviceState *dev);
 #ifdef CONFIG_LINUX
 VirtIODevice *virtio_9p_init(DeviceState *dev, V9fsConf *conf);
-- 
1.7.4

[Qemu-devel] [PATCH 2/7] virtio-serial: Disallow generic ports at id 0

[Amit Shah]

Port 0 is reserved for virtconsole devices for backward compatibility
with the old -virtioconsole (from qemu 0.12) device type.

libvirt prior to commit 8e28c5d40200b4c5d483bd585d237b9d870372e5 used
port 0 for generic ports.  libvirt will no longer do that, but disallow
instantiating generic ports at id 0 from qemu as well.

Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 hw/virtio-console.c |    9 +++++++++
 1 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/hw/virtio-console.c b/hw/virtio-console.c
index c235b27..4440784 100644
--- a/hw/virtio-console.c
+++ b/hw/virtio-console.c
@@ -11,6 +11,7 @@
  */
 
 #include "qemu-char.h"
+#include "qemu-error.h"
 #include "virtio-serial.h"
 
 typedef struct VirtConsole {
@@ -113,6 +114,14 @@ static int virtserialport_initfn(VirtIOSerialPort *port)
 {
     VirtConsole *vcon = DO_UPCAST(VirtConsole, port, port);
 
+    if (port->id == 0) {
+        /*
+         * Disallow a generic port at id 0, that's reserved for
+         * console ports.
+         */
+        error_report("Port number 0 on virtio-serial devices reserved for virtconsole devices for backward compatibility.");
+        return -1;
+    }
     return generic_port_init(vcon, port);
 }
 
-- 
1.7.4

[Qemu-devel] [PATCH 3/7] virtio-serial: Enable ioeventfd

[Amit Shah]

Enable ioeventfd for virtio-serial devices by default.  Commit
25db9ebe15125deb32958c6df74996f745edf1f9 lists the benefits of using
ioeventfd.

Copying a file from guest to host over a virtio-serial channel didn't
show much difference in time or io_exit rate.

Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 hw/virtio-pci.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
index 952b5d2..ef65590 100644
--- a/hw/virtio-pci.c
+++ b/hw/virtio-pci.c
@@ -789,6 +789,7 @@ static int virtio_serial_exit_pci(PCIDevice *pci_dev)
 {
     VirtIOPCIProxy *proxy = DO_UPCAST(VirtIOPCIProxy, pci_dev, pci_dev);
 
+    virtio_pci_stop_ioeventfd(proxy);
     virtio_serial_exit(proxy->vdev);
     return virtio_exit_pci(pci_dev);
 }
@@ -898,6 +899,8 @@ static PCIDeviceInfo virtio_info[] = {
         .init      = virtio_serial_init_pci,
         .exit      = virtio_serial_exit_pci,
         .qdev.props = (Property[]) {
+            DEFINE_PROP_BIT("ioeventfd", VirtIOPCIProxy, flags,
+                            VIRTIO_PCI_FLAG_USE_IOEVENTFD_BIT, true),
             DEFINE_PROP_UINT32("vectors", VirtIOPCIProxy, nvectors,
                                DEV_NVECTORS_UNSPECIFIED),
             DEFINE_PROP_HEX32("class", VirtIOPCIProxy, class_code, 0),
-- 
1.7.4

[Qemu-devel] [PATCH 4/7] virtio-serial-bus: Simplify handle_output() function

[Amit Shah]

There's no code change, just re-arrangement to simplify the function
after recent modifications.

Reported-by: Juan Quintela <quintela@redhat.com>
Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 hw/virtio-serial-bus.c |   12 +++---------
 1 files changed, 3 insertions(+), 9 deletions(-)

diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c
index c6feb43..a82fbe9 100644
--- a/hw/virtio-serial-bus.c
+++ b/hw/virtio-serial-bus.c
@@ -442,25 +442,19 @@ static void handle_output(VirtIODevice *vdev, VirtQueue *vq)
 {
     VirtIOSerial *vser;
     VirtIOSerialPort *port;
-    bool discard;
 
     vser = DO_UPCAST(VirtIOSerial, vdev, vdev);
     port = find_port_by_vq(vser, vq);
 
-    discard = false;
     if (!port || !port->host_connected || !port->info->have_data) {
-        discard = true;
-    }
-
-    if (discard) {
         discard_vq_data(vq, vdev);
         return;
     }
-    if (port->throttled) {
+
+    if (!port->throttled) {
+        do_flush_queued_data(port, vq, vdev);
         return;
     }
-
-    do_flush_queued_data(port, vq, vdev);
 }
 
 static void handle_input(VirtIODevice *vdev, VirtQueue *vq)
-- 
1.7.4

[Qemu-devel] [PATCH 5/7] virtio-serial: Don't clear ->have_data() pointer after unplug

[Amit Shah]

After a port unplug operation, the port->info->have_data() pointer was
set to NULL.  The problem is, the ->info struct is shared by all ports,
effectively disabling writes to other ports.

Reported-by: juzhang <juzhang@redhat.com>
Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 hw/virtio-console.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/hw/virtio-console.c b/hw/virtio-console.c
index 4440784..be59558 100644
--- a/hw/virtio-console.c
+++ b/hw/virtio-console.c
@@ -82,7 +82,6 @@ static int virtconsole_exitfn(VirtIOSerialPort *port)
     VirtConsole *vcon = DO_UPCAST(VirtConsole, port, port);
 
     if (vcon->chr) {
-        port->info->have_data = NULL;
         qemu_chr_close(vcon->chr);
     }
 
-- 
1.7.4

[Qemu-devel] [PATCH 6/7] virtio-console: Keep chardev open for other users after hot-unplug

[Amit Shah]

After a hot-unplug operation, the previous behaviour was to close the
chardev.  That meant the chardev couldn't be re-used.  Also, since
chardev hot-plug isn't possible so far, this means virtio-console
hot-plug isn't feasible as well.

With this change, the chardev is kept around.  A new virtio-console
channel can then be hot-plugged with the same chardev and things will
continue to work.

Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 hw/virtio-console.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/hw/virtio-console.c b/hw/virtio-console.c
index be59558..6b5237b 100644
--- a/hw/virtio-console.c
+++ b/hw/virtio-console.c
@@ -82,7 +82,11 @@ static int virtconsole_exitfn(VirtIOSerialPort *port)
     VirtConsole *vcon = DO_UPCAST(VirtConsole, port, port);
 
     if (vcon->chr) {
-        qemu_chr_close(vcon->chr);
+	/*
+	 * Instead of closing the chardev, free it so it can be used
+	 * for other purposes.
+	 */
+	qemu_chr_add_handlers(vcon->chr, NULL, NULL, NULL, NULL);
     }
 
     return 0;
-- 
1.7.4

[Qemu-devel] [PATCH 7/7] char: Prevent multiple devices opening same chardev

[Amit Shah]

Prevent:

-chardev socket,path=/tmp/foo,server,nowait,id=c0 \
-device virtserialport,chardev=c0,id=vs0 \
-device virtserialport,chardev=c0,id=vs1

Reported-by: Mike Cao <bcao@redhat.com>
Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 hw/qdev-properties.c |    7 ++++++-
 qemu-char.c          |    4 ++++
 qemu-char.h          |    1 +
 3 files changed, 11 insertions(+), 1 deletions(-)

diff --git a/hw/qdev-properties.c b/hw/qdev-properties.c
index a45b61e..1088a26 100644
--- a/hw/qdev-properties.c
+++ b/hw/qdev-properties.c
@@ -351,8 +351,13 @@ static int parse_chr(DeviceState *dev, Property *prop, const char *str)
     CharDriverState **ptr = qdev_get_prop_ptr(dev, prop);
 
     *ptr = qemu_chr_find(str);
-    if (*ptr == NULL)
+    if (*ptr == NULL) {
         return -ENOENT;
+    }
+    if ((*ptr)->assigned) {
+        return -EEXIST;
+    }
+    (*ptr)->assigned = 1;
     return 0;
 }
 
diff --git a/qemu-char.c b/qemu-char.c
index cad35d7..c4557c3 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -197,6 +197,10 @@ void qemu_chr_add_handlers(CharDriverState *s,
                            IOEventHandler *fd_event,
                            void *opaque)
 {
+    if (!opaque) {
+        /* chr driver being released. */
+        s->assigned = 0;
+    }
     s->chr_can_read = fd_can_read;
     s->chr_read = fd_read;
     s->chr_event = fd_event;
diff --git a/qemu-char.h b/qemu-char.h
index 56d9954..fb96eef 100644
--- a/qemu-char.h
+++ b/qemu-char.h
@@ -70,6 +70,7 @@ struct CharDriverState {
     char *label;
     char *filename;
     int opened;
+    int assigned; /* chardev assigned to a device */
     QTAILQ_ENTRY(CharDriverState) next;
 };
 
-- 
1.7.4

[Qemu-devel] Re: [PATCH 1/7] virtio-serial: Use a struct to pass config information from proxy

[Juan Quintela]

Amit Shah <amit.shah@redhat.com> wrote:
> Instead of using a single variable to pass to the virtio_serial_init
> function, use a struct so that expanding the number of variables to be
> passed on later is easier.
>
> Signed-off-by: Amit Shah <amit.shah@redhat.com>

Reviewed-by: Juan Quintela <quintela@redhat.com>

[Qemu-devel] Re: [PULL #7 0/7] virtio-serial fixes, enhancements

[Juan Quintela]

Amit Shah <amit.shah@redhat.com> wrote:
> Hello,
>
> This series fixes a few bugs reported against virtio-serial.  Please
> apply.
>
> The following changes since commit e0efb993b817564ef84e462ac1fe35f89b57ad7b:
>
>   Fix conversions from pointer to int and vice versa (2011-03-20 21:39:23 +0000)
>
> are available in the git repository at:
>   git://git.kernel.org/pub/scm/virt/qemu/amit/virtio-serial.git for-anthony
>
> Amit Shah (7):
>   virtio-serial: Use a struct to pass config information from proxy
>   virtio-serial: Disallow generic ports at id 0
>   virtio-serial: Enable ioeventfd
>   virtio-serial-bus: Simplify handle_output() function
>   virtio-serial: Don't clear ->have_data() pointer after unplug
>   virtio-console: Keep chardev open for other users after hot-unplug
>   char: Prevent multiple devices opening same chardev
>
>  hw/qdev-properties.c   |    7 ++++++-
>  hw/virtio-console.c    |   16 ++++++++++++++--
>  hw/virtio-pci.c        |   15 +++++++++------
>  hw/virtio-serial-bus.c |   28 +++++++++++-----------------
>  hw/virtio-serial.h     |    5 +++++
>  hw/virtio.h            |    3 ++-
>  qemu-char.c            |    4 ++++
>  qemu-char.h            |    1 +
>  8 files changed, 52 insertions(+), 27 deletions(-)

Reviewed-by: Juan Quintela <quintela@redhat.com>

Comment about have_data on mail for it, but it can be improved later.

[Qemu-devel] Re: [PATCH 7/7] char: Prevent multiple devices opening same chardev

[Juan Quintela]

Amit Shah <amit.shah@redhat.com> wrote:
> Prevent:
>
> -chardev socket,path=/tmp/foo,server,nowait,id=c0 \
> -device virtserialport,chardev=c0,id=vs0 \
> -device virtserialport,chardev=c0,id=vs1
>
> Reported-by: Mike Cao <bcao@redhat.com>
> Signed-off-by: Amit Shah <amit.shah@redhat.com>

> @@ -197,6 +197,10 @@ void qemu_chr_add_handlers(CharDriverState *s,
>                             IOEventHandler *fd_event,
>                             void *opaque)
>  {
> +    if (!opaque) {
> +        /* chr driver being released. */
> +        s->assigned = 0;
> +    }
>      s->chr_can_read = fd_can_read;
>      s->chr_read = fd_read;
>      s->chr_event = fd_event;

I preffer to decide that a handler is empty when fd_can_read/fd_read and
fd_event are all NULL, and don't take into account the opaque handler.

This covers the case where opaque is NULL because state is implicit on
the other functions.

Later, Juan.

[Qemu-devel] Re: [PATCH 5/7] virtio-serial: Don't clear ->have_data() pointer after unplug

[Juan Quintela]

Amit Shah <amit.shah@redhat.com> wrote:
> After a port unplug operation, the port->info->have_data() pointer was
> set to NULL.  The problem is, the ->info struct is shared by all ports,
> effectively disabling writes to other ports.
>
> Reported-by: juzhang <juzhang@redhat.com>
> Signed-off-by: Amit Shah <amit.shah@redhat.com>
> ---
>  hw/virtio-console.c |    1 -
>  1 files changed, 0 insertions(+), 1 deletions(-)
>
> diff --git a/hw/virtio-console.c b/hw/virtio-console.c
> index 4440784..be59558 100644
> --- a/hw/virtio-console.c
> +++ b/hw/virtio-console.c
> @@ -82,7 +82,6 @@ static int virtconsole_exitfn(VirtIOSerialPort *port)
>      VirtConsole *vcon = DO_UPCAST(VirtConsole, port, port);
>  
>      if (vcon->chr) {
> -        port->info->have_data = NULL;
>          qemu_chr_close(vcon->chr);
>      }

Discussed with Amit over irc, I think that we are missing setup of
have_data for non console devices, but that is a different bug that the
one being fixed here.

Later, Juan.

[Qemu-devel] Re: [PATCH 5/7] virtio-serial: Don't clear ->have_data() pointer after unplug

[Amit Shah]

On (Wed) 23 Mar 2011 [14:33:25], Juan Quintela wrote:
> Amit Shah <amit.shah@redhat.com> wrote:
> > After a port unplug operation, the port->info->have_data() pointer was
> > set to NULL.  The problem is, the ->info struct is shared by all ports,
> > effectively disabling writes to other ports.
> >
> > Reported-by: juzhang <juzhang@redhat.com>
> > Signed-off-by: Amit Shah <amit.shah@redhat.com>
> > ---
> >  hw/virtio-console.c |    1 -
> >  1 files changed, 0 insertions(+), 1 deletions(-)
> >
> > diff --git a/hw/virtio-console.c b/hw/virtio-console.c
> > index 4440784..be59558 100644
> > --- a/hw/virtio-console.c
> > +++ b/hw/virtio-console.c
> > @@ -82,7 +82,6 @@ static int virtconsole_exitfn(VirtIOSerialPort *port)
> >      VirtConsole *vcon = DO_UPCAST(VirtConsole, port, port);
> >  
> >      if (vcon->chr) {
> > -        port->info->have_data = NULL;
> >          qemu_chr_close(vcon->chr);
> >      }
> 
> Discussed with Amit over irc, I think that we are missing setup of
> have_data for non console devices, but that is a different bug that the
> one being fixed here.

Actually other virtio_serial devices will provide their own have_data,
like spice did earlier (now it's a chardev, so it uses this code
path).

I think the bug is that we should set have_data regardless of a chardev
backend and call qemu_chr_write() in have_data only if a chardev
exists.

		Amit

[Qemu-devel] Re: [PULL #7 0/7] virtio-serial fixes, enhancements

[Amit Shah]

On (Wed) 23 Mar 2011 [14:31:09], Juan Quintela wrote:
> Reviewed-by: Juan Quintela <quintela@redhat.com>

Thanks!

Anthony, ping again for pulling this.

		Amit