From 6b331efb733a0f913ddc0b7762a1307dec304061 Mon Sep 17 00:00:00 2001 From: Amit Shah Date: Thu, 3 Feb 2011 11:22:32 +0530 Subject: [PATCH 1/7] virtio-serial: Use a struct to pass config information from proxy 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 --- 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 3911b09b0..952b5d228 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 8446bc2d2..c6feb4316 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 8cb9fbeb5..5eb948e3f 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 31d16e1c3..d0920a84c 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); From 0b8b716d6c43b5a1d4e0293df1c77af4fff21bab Mon Sep 17 00:00:00 2001 From: Amit Shah Date: Thu, 3 Feb 2011 13:05:07 +0530 Subject: [PATCH 2/7] virtio-serial: Disallow generic ports at id 0 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 --- hw/virtio-console.c | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/hw/virtio-console.c b/hw/virtio-console.c index c235b2726..444078463 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); } From 32059220d02cf8e779bbaf9966d12501c32e2076 Mon Sep 17 00:00:00 2001 From: Amit Shah Date: Thu, 17 Feb 2011 12:31:10 +0530 Subject: [PATCH 3/7] virtio-serial: Enable ioeventfd 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 --- hw/virtio-pci.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c index 952b5d228..ef6559060 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), From e9b382b0170ee045295f2ff0ce1009a01a11eb1f Mon Sep 17 00:00:00 2001 From: Amit Shah Date: Thu, 3 Mar 2011 13:28:55 +0530 Subject: [PATCH 4/7] virtio-serial-bus: Simplify handle_output() function There's no code change, just re-arrangement to simplify the function after recent modifications. Reported-by: Juan Quintela Signed-off-by: Amit Shah --- hw/virtio-serial-bus.c | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c index c6feb4316..a82fbe9ba 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) From fee063c07f20b442ef4bedef834ab0a3bf55b562 Mon Sep 17 00:00:00 2001 From: Amit Shah Date: Thu, 3 Mar 2011 13:29:45 +0530 Subject: [PATCH 5/7] virtio-serial: Don't clear ->have_data() pointer after unplug 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 Signed-off-by: Amit Shah --- hw/virtio-console.c | 1 - 1 file changed, 1 deletion(-) diff --git a/hw/virtio-console.c b/hw/virtio-console.c index 444078463..be5955868 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); } From f9a90f189c780e066039f2de3906c73c48071bb7 Mon Sep 17 00:00:00 2001 From: Amit Shah Date: Tue, 15 Mar 2011 14:13:09 +0530 Subject: [PATCH 6/7] virtio-console: Keep chardev open for other users after hot-unplug 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 --- hw/virtio-console.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/hw/virtio-console.c b/hw/virtio-console.c index be5955868..6b5237b3c 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; From 2d6c1ef40f3678ab47a4d14fb5dadaa486bfcda6 Mon Sep 17 00:00:00 2001 From: Amit Shah Date: Thu, 10 Feb 2011 12:55:20 +0530 Subject: [PATCH 7/7] char: Prevent multiple devices opening same chardev 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 Signed-off-by: Amit Shah --- hw/qdev-properties.c | 7 ++++++- qemu-char.c | 4 ++++ qemu-char.h | 1 + 3 files changed, 11 insertions(+), 1 deletion(-) diff --git a/hw/qdev-properties.c b/hw/qdev-properties.c index a45b61e7e..1088a26f8 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 cad35d771..c4557c30d 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 56d9954c5..fb96eef3d 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; };