* [Qemu-devel] Multiple port support for virtio-console @ 2009-09-22 16:23 Amit Shah 2009-09-22 16:23 ` [Qemu-devel] [PATCH 1/4] char: Emit 'OPENED' events on char device open Amit Shah 0 siblings, 1 reply; 14+ messages in thread From: Amit Shah @ 2009-09-22 16:23 UTC (permalink / raw) To: qemu-devel Hello, This patch series converts virtio-console to qdev, adds a new virtio-console bus and spawns new ports as devices on the bus. A few regressions have been introduced (old kernel / new userspace doesn't work), I'll be looking at that and other issues. Posting this just to show the approach taken for the new bus. Amit ^ permalink raw reply [flat|nested] 14+ messages in thread
* [Qemu-devel] [PATCH 1/4] char: Emit 'OPENED' events on char device open 2009-09-22 16:23 [Qemu-devel] Multiple port support for virtio-console Amit Shah @ 2009-09-22 16:23 ` Amit Shah 2009-09-22 16:23 ` [Qemu-devel] [PATCH 2/4] qdev: add string property Amit Shah 0 siblings, 1 reply; 14+ messages in thread From: Amit Shah @ 2009-09-22 16:23 UTC (permalink / raw) To: qemu-devel; +Cc: Amit Shah Notify users of the char interface whenever the file / connection is opened. The existing RESET event is triggered when the qemu char state is reset as well; which may not be as interesting as char device open events. Signed-off-by: Amit Shah <amit.shah@redhat.com> --- qemu-char.c | 14 +++++++++++++- qemu-char.h | 1 + 2 files changed, 14 insertions(+), 1 deletions(-) diff --git a/qemu-char.c b/qemu-char.c index 8084a67..e7d091c 100644 --- a/qemu-char.c +++ b/qemu-char.c @@ -479,6 +479,8 @@ static CharDriverState *qemu_chr_open_mux(CharDriverState *drv) chr->chr_write = mux_chr_write; chr->chr_update_read_handler = mux_chr_update_read_handler; chr->chr_accept_input = mux_chr_accept_input; + + qemu_chr_event(chr, CHR_EVENT_OPENED); return chr; } @@ -627,6 +629,7 @@ static CharDriverState *qemu_chr_open_fd(int fd_in, int fd_out) chr->chr_close = fd_chr_close; qemu_chr_reset(chr); + qemu_chr_event(chr, CHR_EVENT_OPENED); return chr; } @@ -925,8 +928,10 @@ static void pty_chr_state(CharDriverState *chr, int connected) * the virtual device linked to our pty. */ qemu_mod_timer(s->timer, qemu_get_clock(rt_clock) + 1000); } else { - if (!s->connected) + if (!s->connected) { qemu_chr_reset(chr); + qemu_chr_event(chr, CHR_EVENT_OPENED); + } s->connected = 1; } } @@ -1166,6 +1171,7 @@ static CharDriverState *qemu_chr_open_tty(QemuOpts *opts) } chr->chr_ioctl = tty_serial_ioctl; qemu_chr_reset(chr); + qemu_chr_event(chr, CHR_EVENT_OPENED); return chr; } #else /* ! __linux__ && ! __sun__ */ @@ -1312,6 +1318,7 @@ static CharDriverState *qemu_chr_open_pp(QemuOpts *opts) chr->opaque = drv; qemu_chr_reset(chr); + qemu_chr_event(chr, CHR_EVENT_OPENED); return chr; } @@ -1593,6 +1600,7 @@ static CharDriverState *qemu_chr_open_win(QemuOpts *opts) return NULL; } qemu_chr_reset(chr); + qemu_chr_event(chr, CHR_EVENT_OPENED); return chr; } @@ -1693,6 +1701,7 @@ static CharDriverState *qemu_chr_open_win_pipe(QemuOpts *opts) return NULL; } qemu_chr_reset(chr); + qemu_chr_event(chr, CHR_EVENT_OPENED); return chr; } @@ -1707,6 +1716,7 @@ static CharDriverState *qemu_chr_open_win_file(HANDLE fd_out) chr->opaque = s; chr->chr_write = win_chr_write; qemu_chr_reset(chr); + qemu_chr_event(chr, CHR_EVENT_OPENED); return chr; } @@ -1828,6 +1838,7 @@ static CharDriverState *qemu_chr_open_udp(QemuOpts *opts) chr->chr_write = udp_chr_write; chr->chr_update_read_handler = udp_chr_update_read_handler; chr->chr_close = udp_chr_close; + qemu_chr_event(chr, CHR_EVENT_OPENED); return chr; return_err: @@ -2034,6 +2045,7 @@ static void tcp_chr_connect(void *opaque) qemu_set_fd_handler2(s->fd, tcp_chr_read_poll, tcp_chr_read, NULL, chr); qemu_chr_reset(chr); + qemu_chr_event(chr, CHR_EVENT_OPENED); } #define IACSET(x,a,b,c) x[0] = a; x[1] = b; x[2] = c; diff --git a/qemu-char.h b/qemu-char.h index c0654bc..43e61e0 100644 --- a/qemu-char.h +++ b/qemu-char.h @@ -14,6 +14,7 @@ #define CHR_EVENT_MUX_IN 3 /* mux-focus was set to this terminal */ #define CHR_EVENT_MUX_OUT 4 /* mux-focus will move on */ #define CHR_EVENT_CLOSED 5 /* connection closed */ +#define CHR_EVENT_OPENED 6 /* connection opened */ #define CHR_IOCTL_SERIAL_SET_PARAMS 1 -- 1.6.2.5 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* [Qemu-devel] [PATCH 2/4] qdev: add string property. 2009-09-22 16:23 ` [Qemu-devel] [PATCH 1/4] char: Emit 'OPENED' events on char device open Amit Shah @ 2009-09-22 16:23 ` Amit Shah 2009-09-22 16:23 ` [Qemu-devel] [PATCH 3/4] virtio-console: Add support for multiple ports for generic guest-host communication Amit Shah 0 siblings, 1 reply; 14+ messages in thread From: Amit Shah @ 2009-09-22 16:23 UTC (permalink / raw) To: qemu-devel; +Cc: Gerd Hoffmann From: Gerd Hoffmann <kraxel@redhat.com> --- hw/qdev-properties.c | 28 ++++++++++++++++++++++++++++ hw/qdev.h | 4 ++++ 2 files changed, 32 insertions(+), 0 deletions(-) diff --git a/hw/qdev-properties.c b/hw/qdev-properties.c index 2ecb58d..7e085f6 100644 --- a/hw/qdev-properties.c +++ b/hw/qdev-properties.c @@ -165,6 +165,34 @@ PropertyInfo qdev_prop_hex64 = { .print = print_hex64, }; +/* --- string --- */ + +static int parse_string(DeviceState *dev, Property *prop, const char *str) +{ + char **ptr = qdev_get_prop_ptr(dev, prop); + + if (*ptr) + qemu_free(*ptr); + *ptr = qemu_strdup(str); + return 0; +} + +static int print_string(DeviceState *dev, Property *prop, char *dest, size_t len) +{ + char **ptr = qdev_get_prop_ptr(dev, prop); + if (!*ptr) + return snprintf(dest, len, "<null>"); + return snprintf(dest, len, "\"%s\"", *ptr); +} + +PropertyInfo qdev_prop_string = { + .name = "string", + .type = PROP_TYPE_STRING, + .size = sizeof(char*), + .parse = parse_string, + .print = print_string, +}; + /* --- drive --- */ static int parse_drive(DeviceState *dev, Property *prop, const char *str) diff --git a/hw/qdev.h b/hw/qdev.h index 623ded5..cd9da17 100644 --- a/hw/qdev.h +++ b/hw/qdev.h @@ -68,6 +68,7 @@ enum PropertyType { PROP_TYPE_MACADDR, PROP_TYPE_DRIVE, PROP_TYPE_CHR, + PROP_TYPE_STRING, PROP_TYPE_PTR, }; @@ -161,6 +162,7 @@ extern PropertyInfo qdev_prop_int32; extern PropertyInfo qdev_prop_uint64; extern PropertyInfo qdev_prop_hex32; extern PropertyInfo qdev_prop_hex64; +extern PropertyInfo qdev_prop_string; extern PropertyInfo qdev_prop_chr; extern PropertyInfo qdev_prop_ptr; extern PropertyInfo qdev_prop_macaddr; @@ -200,6 +202,8 @@ extern PropertyInfo qdev_prop_pci_devfn; DEFINE_PROP(_n, _s, _f, qdev_prop_ptr, void*) #define DEFINE_PROP_CHR(_n, _s, _f) \ DEFINE_PROP(_n, _s, _f, qdev_prop_chr, CharDriverState*) +#define DEFINE_PROP_STRING(_n, _s, _f) \ + DEFINE_PROP(_n, _s, _f, qdev_prop_string, char*) #define DEFINE_PROP_DRIVE(_n, _s, _f) \ DEFINE_PROP(_n, _s, _f, qdev_prop_drive, DriveInfo*) #define DEFINE_PROP_MACADDR(_n, _s, _f) \ -- 1.6.2.5 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* [Qemu-devel] [PATCH 3/4] virtio-console: Add support for multiple ports for generic guest-host communication 2009-09-22 16:23 ` [Qemu-devel] [PATCH 2/4] qdev: add string property Amit Shah @ 2009-09-22 16:23 ` Amit Shah 2009-09-22 16:23 ` [Qemu-devel] [PATCH 4/4] virtio-console: Add a in-qemu api for open/read/write/close ports Amit Shah 2009-09-23 9:07 ` [Qemu-devel] [PATCH 3/4] virtio-console: Add support for multiple ports for generic guest-host communication Gerd Hoffmann 0 siblings, 2 replies; 14+ messages in thread From: Amit Shah @ 2009-09-22 16:23 UTC (permalink / raw) To: qemu-devel; +Cc: Amit Shah This interface extends the virtio-console device to handle multiple ports that present a char device from which bits can be sent and read. Sample uses for such a device can be obtaining info from the guest like the file systems used, apps installed, etc. for offline usage and logged-in users, clipboard copy-paste, etc. for online usage. For requirements, use-cases and some history see http://www.linux-kvm.org/page/VMchannel_Requirements Signed-off-by: Amit Shah <amit.shah@redhat.com> --- Makefile.target | 2 +- hw/pc.c | 9 - hw/qdev.c | 9 +- hw/virtio-console-bus.c | 111 ++++++++ hw/virtio-console.c | 646 +++++++++++++++++++++++++++++++++++++++++++---- hw/virtio-console.h | 73 ++++++ qemu-options.hx | 8 - sysemu.h | 7 +- vl.c | 37 --- 9 files changed, 781 insertions(+), 121 deletions(-) create mode 100644 hw/virtio-console-bus.c diff --git a/Makefile.target b/Makefile.target index 609015b..22a2644 100644 --- a/Makefile.target +++ b/Makefile.target @@ -173,7 +173,7 @@ obj-y = vl.o monitor.o pci.o isa_mmio.o machine.o \ gdbstub.o gdbstub-xml.o # virtio has to be here due to weird dependency between PCI and virtio-net. # need to fix this properly -obj-y += virtio-blk.o virtio-balloon.o virtio-net.o virtio-console.o virtio-pci.o +obj-y += virtio-blk.o virtio-balloon.o virtio-net.o virtio-console.o virtio-console-bus.o virtio-pci.o obj-$(CONFIG_KVM) += kvm.o kvm-all.o LIBS+=-lz diff --git a/hw/pc.c b/hw/pc.c index 240cfe0..70b1ce9 100644 --- a/hw/pc.c +++ b/hw/pc.c @@ -1463,15 +1463,6 @@ static void pc_init1(ram_addr_t ram_size, extboot_init(info->bdrv, 1); } - /* Add virtio console devices */ - if (pci_enabled) { - for(i = 0; i < MAX_VIRTIO_CONSOLES; i++) { - if (virtcon_hds[i]) { - pci_create_simple(pci_bus, -1, "virtio-console-pci"); - } - } - } - #ifdef CONFIG_KVM_DEVICE_ASSIGNMENT if (kvm_enabled()) { add_assigned_devices(pci_bus, assigned_devices, assigned_devices_index); diff --git a/hw/qdev.c b/hw/qdev.c index 43b1beb..1d79db0 100644 --- a/hw/qdev.c +++ b/hw/qdev.c @@ -243,13 +243,8 @@ void qdev_free(DeviceState *dev) CharDriverState *qdev_init_chardev(DeviceState *dev) { static int next_serial; - static int next_virtconsole; - /* FIXME: This is a nasty hack that needs to go away. */ - if (strncmp(dev->info->name, "virtio", 6) == 0) { - return virtcon_hds[next_virtconsole++]; - } else { - return serial_hds[next_serial++]; - } + + return serial_hds[next_serial++]; } BusState *qdev_get_parent_bus(DeviceState *dev) diff --git a/hw/virtio-console-bus.c b/hw/virtio-console-bus.c new file mode 100644 index 0000000..d8be5bb --- /dev/null +++ b/hw/virtio-console-bus.c @@ -0,0 +1,111 @@ +/* + * A bus for connecting virtio-console ports + * + * Copyright (c) 2009 Red Hat, Inc. + * + * Author(s): + * Amit Shah <amit.shah@redhat.com> + * + * This work is licensed under the terms of the GNU GPL, version 2. See + * the COPYING file in the top-level directory. + */ + +#include "sysbus.h" +#include "virtio-console.h" + +/* The virtio-console bus on top of which the ports will ride as devices */ +struct VirtConBus { + BusState qbus; + uint32_t assigned; +}; +static VirtConBus *virtcon_bus; + +static void virtcon_bus_dev_print(Monitor *mon, DeviceState *dev, int indent); + +static struct BusInfo virtcon_bus_info = { + .name = "virtio-console-bus", + .size = sizeof(VirtConBus), + .print_dev = virtcon_bus_dev_print, +}; + +VirtConBus *virtcon_bus_new(DeviceState *dev) +{ + if (virtcon_bus) { + fprintf(stderr, "Can't create a second virtio-console bus\n"); + return NULL; + } + if (!dev) { + dev = qdev_create(NULL, "virtio-console-sysbus"); + qdev_init(dev); + } + + virtcon_bus = FROM_QBUS(VirtConBus, qbus_create(&virtcon_bus_info, dev, NULL)); + return virtcon_bus; +} + +static int virtcon_port_qdev_init(DeviceState *qdev, DeviceInfo *base) +{ + VirtConDevice *dev = DO_UPCAST(VirtConDevice, qdev, qdev); + VirtConPortDeviceInfo *info = DO_UPCAST(VirtConPortDeviceInfo, qdev, base); + + return info->init(dev); +} + +void virtcon_port_qdev_register(VirtConPortDeviceInfo *info) +{ + info->qdev.init = virtcon_port_qdev_init; + info->qdev.bus_info = &virtcon_bus_info; + qdev_register(&info->qdev); +} + +VirtConDevice *virtcon_create(const char *name) +{ + DeviceState *dev; + + if (!virtcon_bus) { + qemu_error("Tried to create a port %s with no virtio-console bus present.\n", + name); + return NULL; + } + dev = qdev_create(&virtcon_bus->qbus, name); + return DO_UPCAST(VirtConDevice, qdev, dev); +} + +VirtConDevice *virtcon_create_simple(const char *name) +{ + VirtConDevice *dev; + + dev = virtcon_create(name); + if (qdev_init(&dev->qdev) != 0) { + qdev_free(&dev->qdev); + return NULL; + } + return dev; +} + +static void virtcon_bus_dev_print(Monitor *mon, DeviceState *dev, int indent) +{ + VirtConDevice *d = DO_UPCAST(VirtConDevice, qdev, dev); + if (&d->qdev) { + ; + } +} + +static int virtcon_bus_init(SysBusDevice *dev) +{ + return 0; +} + +static SysBusDeviceInfo virtcon_sysbus_info = { + .init = virtcon_bus_init, + .qdev.name = "virtio-console-sysbus", + .qdev.size = sizeof(SysBusDevice), + .qdev.no_user = 1, +}; + +static void virtcon_sysbus_register_devices(void) +{ + sysbus_register_withprop(&virtcon_sysbus_info); +} + +device_init(virtcon_sysbus_register_devices) diff --git a/hw/virtio-console.c b/hw/virtio-console.c index 92c953c..d750c8e 100644 --- a/hw/virtio-console.c +++ b/hw/virtio-console.c @@ -2,9 +2,11 @@ * Virtio Console Device * * Copyright IBM, Corp. 2008 + * Copyright Red Hat, Inc. 2009 * * Authors: * Christian Ehrhardt <ehrhardt@linux.vnet.ibm.com> + * Amit Shah <amit.shah@redhat.com> * * This work is licensed under the terms of the GNU GPL, version 2. See * the COPYING file in the top-level directory. @@ -12,39 +14,320 @@ */ #include "hw.h" +#include "monitor.h" +#include "qemu-queue.h" #include "qemu-char.h" +#include "sysbus.h" #include "virtio.h" #include "virtio-console.h" - typedef struct VirtIOConsole { VirtIODevice vdev; VirtQueue *ivq, *ovq; - CharDriverState *chr; + + VirtIOConsolePort* ports[MAX_VIRTIO_CONSOLE_PORTS]; + struct virtio_console_config config; + + uint32_t guest_features; } VirtIOConsole; -static VirtIOConsole *to_virtio_console(VirtIODevice *vdev) +/* This struct holds individual buffers received for each port */ +typedef struct VirtIOConsolePortBuffer { + QTAILQ_ENTRY(VirtIOConsolePortBuffer) next; + + uint8_t *buf; + + size_t len; /* length of the buffer */ + + /* The size of one write request as issued by the guest. The + * buffer could be split in this list but using the size value in + * the first buffer for each write we can identify complete + * writes + */ + size_t size; +} VirtIOConsolePortBuffer; + +struct VirtIOConsolePort { + DeviceState dev; + + VirtIOConsole *vcon; + CharDriverState *hd; + + char *name; + + QTAILQ_HEAD(, VirtIOConsolePortBuffer) unflushed_buffer_head; + + bool guest_connected; + bool host_connected; +}; + +static VirtIOConsole *virtio_console; + +static VirtIOConsolePort *get_port_from_id(VirtIOConsole *vcon, uint32_t id) +{ + if (id > MAX_VIRTIO_CONSOLE_PORTS) + return NULL; + + return vcon->ports[id]; +} + +static int get_id_from_port(VirtIOConsolePort *port) +{ + uint32_t i; + + for (i = 0; i < MAX_VIRTIO_CONSOLE_PORTS; i++) { + if (port == port->vcon->ports[i]) { + return i; + } + } + return VIRTIO_CONSOLE_BAD_ID; +} + +static bool use_multiport(void) +{ + return virtio_console->guest_features & (1 << VIRTIO_CONSOLE_F_MULTIPORT); +} + +static bool is_internal(uint32_t flags) +{ + return flags & VIRTIO_CONSOLE_ID_INTERNAL; +} + +static bool is_console(uint32_t port_nr) +{ + return false; +} + +static bool has_complete_data(VirtIOConsolePort *port) +{ + VirtIOConsolePortBuffer *buf; + size_t len, size; + + len = 0; + size = 0; + QTAILQ_FOREACH(buf, &port->unflushed_buffer_head, next) { + if (!buf->size && buf == QTAILQ_FIRST(&port->unflushed_buffer_head)) { + /* We have a buffer that's lost its way; just flush it */ + return true; + } + if (size && buf->size) { + /* Start of the next write request */ + return true; + } + if (buf->size) { + size = buf->size; + } + len += buf->len; + if (len == size) { + return true; + } + } + return false; +} + +static size_t flush_buf(VirtIOConsolePort *port, const uint8_t *buf, size_t len) +{ + if (!port->hd) { + return 0; + } + return qemu_chr_write(port->hd, buf, len); +} + +static void flush_queue(VirtIOConsolePort *port) +{ + VirtIOConsolePortBuffer *buf, *buf2; + uint8_t *outbuf; + size_t outlen, outsize; + + while (!QTAILQ_EMPTY(&port->unflushed_buffer_head)) { + if (!has_complete_data(port)) { + break; + } + + buf = QTAILQ_FIRST(&port->unflushed_buffer_head); + if (!buf->size) { + /* This is a buf that didn't get consumed as part of a + * previous data stream. Bad thing, shouldn't + * happen. But let's handle it nonetheless + */ + flush_buf(port, buf->buf, buf->len); + QTAILQ_REMOVE(&port->unflushed_buffer_head, buf, next); + qemu_free(buf->buf); + qemu_free(buf); + continue; + } + + outlen = 0; + outsize = buf->size; + outbuf = qemu_mallocz(outsize); + QTAILQ_FOREACH_SAFE(buf, &port->unflushed_buffer_head, next, buf2) { + memcpy(outbuf + outlen, buf->buf, buf->len); + outlen += buf->len; + QTAILQ_REMOVE(&port->unflushed_buffer_head, buf, next); + qemu_free(buf->buf); + qemu_free(buf); + + if (outlen == outsize) + break; + } + flush_buf(port, outbuf, outlen); + qemu_free(outbuf); + } +} + + +static size_t write_to_port(VirtIOConsolePort *port, + const uint8_t *buf, size_t size, uint32_t flags) { - return (VirtIOConsole *)vdev; + VirtQueue *vq = port->vcon->ivq; + VirtQueueElement elem; + size_t offset = 0; + size_t len = 0; + + if (!virtio_queue_ready(vq)) { + return 0; + } + + if (!use_multiport() && is_internal(flags)) { + return 0; + } + + while (offset < size) { + struct virtio_console_header header; + int i, header_len; + + header_len = use_multiport() ? sizeof(header) : 0; + + if (!virtqueue_pop(vq, &elem)) { + break; + } + if (elem.in_sg[0].iov_len < header_len) { + /* We can't even store our port number in this buffer. Bug? */ + fprintf(stderr, "virtio-console: size %zd less than expected\n", + elem.in_sg[0].iov_len); + exit(1); + } + header.id = cpu_to_le32(get_id_from_port(port)); + header.flags = flags; + memcpy(elem.in_sg[0].iov_base, &header, header_len); + + for (i = 0; offset < size && i < elem.in_num; i++) { + len = MIN(elem.in_sg[i].iov_len - header_len, size - offset); + + memcpy(elem.in_sg[i].iov_base + header_len, buf + offset, len); + offset += len; + header_len = 0; + } + header_len = use_multiport() ? sizeof(header) : 0; + virtqueue_push(vq, &elem, len + header_len); + } + virtio_notify(&port->vcon->vdev, vq); + return offset; } +/* Guest wants to notify us of some event */ +static void handle_control_message(VirtIOConsolePort *port, + struct virtio_console_control *cpkt) +{ + uint8_t *buffer; + size_t buffer_len; + + switch(cpkt->event) { + case VIRTIO_CONSOLE_PORT_OPEN: + port->guest_connected = cpkt->value; + break; + case VIRTIO_CONSOLE_PORT_NAME: + if (!port->name) { + const char *default_string = "org.qemu.console"; + + port->name = qemu_malloc(strlen(default_string) + 1); + memcpy(port->name, default_string, strlen(default_string) + 1); + } + buffer_len = sizeof(*cpkt) + strlen(port->name) + 1; + buffer = qemu_malloc(buffer_len); + + memcpy(buffer, cpkt, sizeof(*cpkt)); + memcpy(buffer + sizeof(*cpkt), port->name, strlen(port->name)); + buffer[buffer_len - 1] = 0; + + write_to_port(port, buffer, buffer_len, VIRTIO_CONSOLE_ID_INTERNAL); + qemu_free(buffer); + } +} + +/* Guest wrote something to some port. + * + * Flush the data in the entire chunk that we received rather than + * splitting it into multiple buffers. VNC clients don't consume split + * buffers + */ static void virtio_console_handle_output(VirtIODevice *vdev, VirtQueue *vq) { - VirtIOConsole *s = to_virtio_console(vdev); + VirtIOConsole *vcon; VirtQueueElement elem; + vcon = DO_UPCAST(VirtIOConsole, vdev, vdev); + while (virtqueue_pop(vq, &elem)) { - ssize_t len = 0; - int d; + VirtIOConsolePort *port; + VirtIOConsolePortBuffer *buf; + struct virtio_console_header header; + int header_len; - for (d = 0; d < elem.out_num; d++) { - len += qemu_chr_write(s->chr, (uint8_t *)elem.out_sg[d].iov_base, - elem.out_sg[d].iov_len); + buf = qemu_mallocz(sizeof(*buf)); + + if (use_multiport()) { + header_len = sizeof(header); + + memcpy(&header, elem.out_sg[0].iov_base, header_len); + port = get_port_from_id(vcon, header.id); + if (!port) { + qemu_free(buf); + goto next_buf; + } + } else { + header_len = 0; + port = get_port_from_id(vcon, 0); + } + + /* The guest always sends only one sg */ + buf->len = elem.out_sg[0].iov_len - header_len; + buf->buf = qemu_mallocz(buf->len); + memcpy(buf->buf, elem.out_sg[0].iov_base + header_len, buf->len); + + if (use_multiport() && is_internal(header.flags)) { + handle_control_message(port, + (struct virtio_console_control *)buf->buf); + qemu_free(buf->buf); + qemu_free(buf); + goto next_buf; + } + if (!use_multiport() || is_console(get_id_from_port(port))) { + flush_buf(port, buf->buf, buf->len); + qemu_free(buf->buf); + qemu_free(buf); + goto next_buf; } - virtqueue_push(vq, &elem, len); - virtio_notify(vdev, vq); + QTAILQ_INSERT_TAIL(&port->unflushed_buffer_head, buf, next); + if (use_multiport()) { + /* Only the first buffer in a stream will have this + * set. This will help us identify the first buffer and + * the remaining buffers in the stream based on length + */ + buf->size = header.size; + } else { + /* We always want to flush all the buffers in this case */ + buf->size = buf->len; + } + if (!port->host_connected) { + goto next_buf; + } + flush_queue(port); + next_buf: + virtqueue_push(vq, &elem, elem.out_sg[0].iov_len); } + virtio_notify(vdev, vq); } static void virtio_console_handle_input(VirtIODevice *vdev, VirtQueue *vq) @@ -53,94 +336,351 @@ static void virtio_console_handle_input(VirtIODevice *vdev, VirtQueue *vq) static uint32_t virtio_console_get_features(VirtIODevice *vdev) { - return 0; + return 1 << VIRTIO_CONSOLE_F_MULTIPORT; +} + +static void virtio_console_set_features(VirtIODevice *vdev, uint32_t features) +{ + virtio_console->guest_features = features; } +/* Guest requested config info */ +static void virtio_console_get_config(VirtIODevice *vdev, uint8_t *config_data) +{ + VirtIOConsole *vcon; + + vcon = DO_UPCAST(VirtIOConsole, vdev, vdev); + memcpy(config_data, &vcon->config, sizeof(struct virtio_console_config)); +} + +static void virtio_console_set_config(VirtIODevice *vdev, + const uint8_t *config_data) +{ + struct virtio_console_config config; + + memcpy(&config, config_data, sizeof(config)); +} + +static void send_control_event(VirtIOConsolePort *port, + struct virtio_console_control *cpkt) +{ + write_to_port(port, (uint8_t *)cpkt, sizeof(*cpkt), + VIRTIO_CONSOLE_ID_INTERNAL); +} + +/* Readiness of the guest to accept data on a port */ static int vcon_can_read(void *opaque) { - VirtIOConsole *s = (VirtIOConsole *) opaque; + VirtIOConsolePort *port = opaque; + VirtQueue *vq = port->vcon->ivq; + int size, header_len; - if (!virtio_queue_ready(s->ivq) || - !(s->vdev.status & VIRTIO_CONFIG_S_DRIVER_OK) || - virtio_queue_empty(s->ivq)) - return 0; + if (use_multiport()) { + header_len = sizeof(struct virtio_console_header); + } else { + header_len = 0; + } - /* current implementations have a page sized buffer. - * We fall back to a one byte per read if there is not enough room. - * It would be cool to have a function that returns the available byte - * instead of checking for a limit */ - if (virtqueue_avail_bytes(s->ivq, TARGET_PAGE_SIZE, 0)) - return TARGET_PAGE_SIZE; - if (virtqueue_avail_bytes(s->ivq, 1, 0)) - return 1; + if (!virtio_queue_ready(vq) || + !(port->vcon->vdev.status & VIRTIO_CONFIG_S_DRIVER_OK) || + virtio_queue_empty(vq)) { + return 0; + } + if (!port->guest_connected) { + return 0; + } + size = TARGET_PAGE_SIZE; + if (virtqueue_avail_bytes(vq, size, 0)) { + return size - header_len; + } + size = header_len + 1; + if (virtqueue_avail_bytes(vq, size, 0)) { + return size - header_len; + } return 0; } +/* Send data from a char device over to the guest */ static void vcon_read(void *opaque, const uint8_t *buf, int size) { - VirtIOConsole *s = (VirtIOConsole *) opaque; - VirtQueueElement elem; - int offset = 0; + VirtIOConsolePort *port = opaque; - /* The current kernel implementation has only one outstanding input - * buffer of PAGE_SIZE. Nevertheless, this function is prepared to - * handle multiple buffers with multiple sg element for input */ - while (offset < size) { - int i = 0; - if (!virtqueue_pop(s->ivq, &elem)) - break; - while (offset < size && i < elem.in_num) { - int len = MIN(elem.in_sg[i].iov_len, size - offset); - memcpy(elem.in_sg[i].iov_base, buf + offset, len); - offset += len; - i++; - } - virtqueue_push(s->ivq, &elem, size); - } - virtio_notify(&s->vdev, s->ivq); + write_to_port(port, buf, size, 0); } static void vcon_event(void *opaque, int event) { - /* we will ignore any event for the time being */ + VirtIOConsolePort *port = opaque; + struct virtio_console_control cpkt; + bool update_needed; + + cpkt.event = VIRTIO_CONSOLE_PORT_OPEN; + + update_needed = false; + switch (event) { + case CHR_EVENT_OPENED: { + cpkt.value = 1; + update_needed = true; + port->host_connected = true; + + /* Flush any buffers that were pending while the chardev was + * disconnected + */ + flush_queue(port); + break; + } + case CHR_EVENT_CLOSED: + cpkt.value = 0; + update_needed = true; + port->host_connected = false; + break; + default: + break; + } + if (!update_needed) { + return; + } + send_control_event(port, &cpkt); +} + +static void set_active_in_map(uint32_t *map, uint32_t idx) +{ + int i; + + i = idx / 32; + idx %= 32; + + map[i] |= 1U << idx; +} + +/* + * Get the guest_connected status after a migration since it's all + * packed in the same way as ports_map in the config space is packed. + * + * return VIRTIO_CONSOLE_BAD_ID if nothing was found in the map, else + * return the port number whose guest was connected before the + * migration + */ +static uint32_t find_next_active_in_map(uint32_t *map, unsigned int i) +{ + uint32_t port_nr; + + port_nr = ffs(*map); + if (!port_nr) { + return VIRTIO_CONSOLE_BAD_ID; + } + /* We used ffs above */ + port_nr--; + + *map &= ~(1U << port_nr); + + port_nr += i * 32; + return port_nr; +} + +static int vcon_port_initfn(VirtConDevice *dev) +{ + VirtIOConsolePort *port; + + port = DO_UPCAST(VirtIOConsolePort, dev, &dev->qdev); + + QTAILQ_INIT(&port->unflushed_buffer_head); + + port->vcon = virtio_console; + + qemu_chr_add_handlers(port->hd, vcon_can_read, vcon_read, vcon_event, port); + + port->vcon->ports[port->vcon->config.nr_active_ports++] = port; + + /* Send an update to the guest about this new port added */ + virtio_notify_config(&port->vcon->vdev); + + return 0; } static void virtio_console_save(QEMUFile *f, void *opaque) { VirtIOConsole *s = opaque; + uint32_t guest_connected_map[MAX_VIRTIO_CONSOLE_PORTS / 32]; + unsigned int i, nr_bufs, active_ports; + /* The virtio device */ virtio_save(&s->vdev, f); + /* The config space */ + qemu_put_be16s(f, &s->config.cols); + qemu_put_be16s(f, &s->config.rows); + qemu_put_be32s(f, &s->config.nr_active_ports); + + active_ports = le32_to_cpu(s->config.nr_active_ports); + + /* Items in struct VirtIOConsole */ + qemu_put_be32s(f, &s->guest_features); + + /* + * Items in struct VirtIOConsolePort + * guest_connected is a single bit in each port; pack all these + * bits in uint32_t words and then send them across + */ + for (i = 0; i < active_ports / 32; i++) { + guest_connected_map[i] = 0; + } + for (i = 0; i < active_ports; i++) { + if (s->ports[i]->guest_connected) { + set_active_in_map(guest_connected_map, i); + } + } + for (i = 0; i < active_ports / 32; i++) { + qemu_put_be32s(f, &guest_connected_map[i]); + } + + /* All the pending buffers from active ports */ + for (i = 0; i < active_ports; i++) { + VirtIOConsolePortBuffer *buf; + + nr_bufs = 0; + QTAILQ_FOREACH(buf, &s->ports[i]->unflushed_buffer_head, next) { + nr_bufs++; + } + /* First the port number, then the nr of bufs and then the bufs */ + qemu_put_be32s(f, &i); + qemu_put_be32s(f, &nr_bufs); + if (!nr_bufs) { + continue; + } + QTAILQ_FOREACH(buf, &s->ports[i]->unflushed_buffer_head, next) { + qemu_put_be64s(f, &buf->len); + qemu_put_be64s(f, &buf->size); + qemu_put_buffer(f, buf->buf, buf->len); + } + } } static int virtio_console_load(QEMUFile *f, void *opaque, int version_id) { VirtIOConsole *s = opaque; + uint32_t guest_connected_map[MAX_VIRTIO_CONSOLE_PORTS / 32]; + unsigned int i, active_ports; - if (version_id != 1) + if (version_id > 2) return -EINVAL; + /* The virtio device */ virtio_load(&s->vdev, f); + + if (version_id < 2) + return 0; + + /* The config space */ + qemu_get_be16s(f, &s->config.cols); + qemu_get_be16s(f, &s->config.rows); + s->config.nr_active_ports = cpu_to_le32(qemu_get_be32(f)); + + active_ports = le32_to_cpu(s->config.nr_active_ports); + + /* Items in struct VirtIOConsole */ + qemu_get_be32s(f, &virtio_console->guest_features); + + /* Items in struct VirtIOConsolePort */ + for (i = 0; i < active_ports / 32; i++) { + guest_connected_map[i] = qemu_get_be32(f); + } + + for (i = 0; i < active_ports / 32; i++) { + uint32_t port_nr, map; + + map = guest_connected_map[i]; + while (1) { + port_nr = find_next_active_in_map(&map, i); + if (port_nr == VIRTIO_CONSOLE_BAD_ID) { + break; + } + s->ports[port_nr]->guest_connected = true; + } + } + + /* All the pending buffers from active ports */ + for (i = 0; i < active_ports; i++) { + VirtIOConsolePortBuffer *buf; + unsigned int nr, nr_bufs; + + /* First the port number, then the nr of bufs and then the bufs */ + qemu_get_be32s(f, &nr); + qemu_get_be32s(f, &nr_bufs); + if (!nr_bufs) { + continue; + } + for (; nr_bufs; nr_bufs--) { + buf = qemu_malloc(sizeof(*buf)); + + qemu_get_be64s(f, &buf->len); + qemu_get_be64s(f, &buf->size); + buf->buf = qemu_malloc(buf->len); + qemu_get_buffer(f, buf->buf, buf->len); + QTAILQ_INSERT_TAIL(&s->ports[nr]->unflushed_buffer_head, buf, next); + } + } + return 0; } VirtIODevice *virtio_console_init(DeviceState *dev) { VirtIOConsole *s; + + if (virtio_console) { + /* + * Linux guests don't support more than one virtio-console devices + * at the moment + */ + return NULL; + } + + if (MAX_VIRTIO_CONSOLE_PORTS % 32) { + /* We require MAX_VIRTIO_CONSOLE_PORTS be a multiple of 32: + * We anyway use up that much space for the bitmap and it + * simplifies some calculations + */ + return NULL; + } + s = (VirtIOConsole *)virtio_common_init("virtio-console", VIRTIO_ID_CONSOLE, - 0, sizeof(VirtIOConsole)); - if (s == NULL) - return NULL; + sizeof(struct virtio_console_config), + sizeof(VirtIOConsole)); + virtio_console = s; s->vdev.get_features = virtio_console_get_features; + s->vdev.set_features = virtio_console_set_features; + s->vdev.get_config = virtio_console_get_config; + s->vdev.set_config = virtio_console_set_config; + /* Add queue for host to guest transfers */ s->ivq = virtio_add_queue(&s->vdev, 128, virtio_console_handle_input); + /* Add queue for guest to host transfers */ s->ovq = virtio_add_queue(&s->vdev, 128, virtio_console_handle_output); - s->chr = qdev_init_chardev(dev); - qemu_chr_add_handlers(s->chr, vcon_can_read, vcon_read, vcon_event, s); + register_savevm("virtio-console", -1, 2, virtio_console_save, + virtio_console_load, s); - register_savevm("virtio-console", -1, 1, virtio_console_save, virtio_console_load, s); + /* Spawn a new virtio-console bus on which the ports will ride as devices */ + virtcon_bus_new(NULL); return &s->vdev; } + +static VirtConPortDeviceInfo virtcon_port_info = { + .qdev.name = "virtconport", + .qdev.size = sizeof(VirtIOConsolePort), + .init = vcon_port_initfn, + .qdev.props = (Property[]) { + DEFINE_PROP_CHR("chardev", VirtIOConsolePort, hd), + DEFINE_PROP_STRING("name", VirtIOConsolePort, name), + DEFINE_PROP_END_OF_LIST(), + }, +}; + +static void virtcon_port_register(void) +{ + virtcon_port_qdev_register(&virtcon_port_info); +} +device_init(virtcon_port_register) diff --git a/hw/virtio-console.h b/hw/virtio-console.h index 84d0717..526713e 100644 --- a/hw/virtio-console.h +++ b/hw/virtio-console.h @@ -2,9 +2,11 @@ * Virtio Console Support * * Copyright IBM, Corp. 2008 + * Copyright Red Hat, Inc. 2009 * * Authors: * Christian Ehrhardt <ehrhardt@linux.vnet.ibm.com> + * Amit Shah <amit.shah@redhat.com> * * This work is licensed under the terms of the GNU GPL, version 2. See * the COPYING file in the top-level directory. @@ -13,7 +15,78 @@ #ifndef _QEMU_VIRTIO_CONSOLE_H #define _QEMU_VIRTIO_CONSOLE_H +#include "sysemu.h" + +/* Interface shared between the guest kernel and qemu */ + /* The ID for virtio console */ #define VIRTIO_ID_CONSOLE 3 +/* Invalid port number */ +#define VIRTIO_CONSOLE_BAD_ID (~(uint32_t)0) + +/* Features supported */ +#define VIRTIO_CONSOLE_F_MULTIPORT 1 + +struct virtio_console_config +{ + /* + * These two fields are used by VIRTIO_CONSOLE_F_SIZE which + * isn't implemented here yet + */ + uint16_t cols; + uint16_t rows; + + uint32_t nr_active_ports; +} __attribute__((packed)); + +struct virtio_console_control +{ + uint16_t event; + uint16_t value; +}; + +struct virtio_console_header { + uint32_t id; /* Port id */ + uint32_t flags; /* Some message between host and guest */ + uint32_t size; /* Size that's sent with the first buffer of each stream */ +} __attribute__((packed)); + +/* Messages between host and guest */ +#define VIRTIO_CONSOLE_ID_INTERNAL (1 << 0) + +/* Some events for the internal messages (control packets) */ +#define VIRTIO_CONSOLE_PORT_OPEN 0 +#define VIRTIO_CONSOLE_PORT_NAME 1 + +typedef struct VirtConBus VirtConBus; + +/* In-qemu interface */ + +/* Max. virtio console ports per device */ +#define MAX_VIRTIO_CONSOLE_PORTS 64 + +typedef struct VirtConDevice { + DeviceState qdev; +} VirtConDevice; + +typedef int (*virtcon_qdev_initfn)(VirtConDevice *dev); +typedef int (*virtcon_port_qdev_initfn)(VirtConDevice *dev); + +typedef struct VirtConPortDeviceInfo { + DeviceInfo qdev; + virtcon_port_qdev_initfn init; +} VirtConPortDeviceInfo; + +VirtConDevice *virtcon_create(const char *name); +VirtConDevice *virtcon_create_simple(const char *name); + +void virtcon_port_qdev_register(VirtConPortDeviceInfo *info); + +VirtConBus *virtcon_bus_new(DeviceState *dev); + +typedef struct VirtIOConsolePort VirtIOConsolePort; +void virtio_console_monitor_command(Monitor *mon, + const char *command, const char *param); + #endif diff --git a/qemu-options.hx b/qemu-options.hx index 2f4291d..ca412bf 100644 --- a/qemu-options.hx +++ b/qemu-options.hx @@ -1604,14 +1604,6 @@ character to Control-t. @end table ETEXI -DEF("virtioconsole", HAS_ARG, QEMU_OPTION_virtiocon, \ - "-virtioconsole c\n" \ - " set virtio console\n") -STEXI -@item -virtioconsole @var{c} -Set virtio console. -ETEXI - DEF("show-cursor", 0, QEMU_OPTION_show_cursor, \ "-show-cursor show cursor\n") STEXI diff --git a/sysemu.h b/sysemu.h index a8d1549..af0e7d8 100644 --- a/sysemu.h +++ b/sysemu.h @@ -2,6 +2,7 @@ #define SYSEMU_H /* Misc. things related to the system emulator. */ +#include <stdbool.h> #include "qemu-common.h" #include "qemu-option.h" #include "qemu-queue.h" @@ -233,12 +234,6 @@ extern CharDriverState *serial_hds[MAX_SERIAL_PORTS]; extern CharDriverState *parallel_hds[MAX_PARALLEL_PORTS]; -/* virtio consoles */ - -#define MAX_VIRTIO_CONSOLES 1 - -extern CharDriverState *virtcon_hds[MAX_VIRTIO_CONSOLES]; - #define TFR(expr) do { if ((expr) != -1) break; } while (errno == EINTR) #ifdef HAS_AUDIO diff --git a/vl.c b/vl.c index 3b49c56..b5215b2 100644 --- a/vl.c +++ b/vl.c @@ -213,7 +213,6 @@ static int no_frame = 0; int no_quit = 0; CharDriverState *serial_hds[MAX_SERIAL_PORTS]; CharDriverState *parallel_hds[MAX_PARALLEL_PORTS]; -CharDriverState *virtcon_hds[MAX_VIRTIO_CONSOLES]; #ifdef TARGET_I386 int win2k_install_hack = 0; int rtc_td_hack = 0; @@ -4690,8 +4689,6 @@ int main(int argc, char **argv, char **envp) int serial_device_index; const char *parallel_devices[MAX_PARALLEL_PORTS]; int parallel_device_index; - const char *virtio_consoles[MAX_VIRTIO_CONSOLES]; - int virtio_console_index; const char *loadvm = NULL; QEMUMachine *machine; const char *cpu_model; @@ -4765,10 +4762,6 @@ int main(int argc, char **argv, char **envp) parallel_devices[i] = NULL; parallel_device_index = 0; - for(i = 0; i < MAX_VIRTIO_CONSOLES; i++) - virtio_consoles[i] = NULL; - virtio_console_index = 0; - monitor_devices[0] = "vc:80Cx24C"; for (i = 1; i < MAX_MONITOR_DEVICES; i++) { monitor_devices[i] = NULL; @@ -5228,14 +5221,6 @@ int main(int argc, char **argv, char **envp) exit(1); } break; - case QEMU_OPTION_virtiocon: - if (virtio_console_index >= MAX_VIRTIO_CONSOLES) { - fprintf(stderr, "qemu: too many virtio consoles\n"); - exit(1); - } - virtio_consoles[virtio_console_index] = optarg; - virtio_console_index++; - break; case QEMU_OPTION_parallel: if (parallel_device_index >= MAX_PARALLEL_PORTS) { fprintf(stderr, "qemu: too many parallel ports\n"); @@ -5827,20 +5812,6 @@ int main(int argc, char **argv, char **envp) } } - for(i = 0; i < MAX_VIRTIO_CONSOLES; i++) { - const char *devname = virtio_consoles[i]; - if (devname && strcmp(devname, "none")) { - char label[32]; - snprintf(label, sizeof(label), "virtcon%d", i); - virtcon_hds[i] = qemu_chr_open(label, devname, NULL); - if (!virtcon_hds[i]) { - fprintf(stderr, "qemu: could not open virtio console '%s'\n", - devname); - exit(1); - } - } - } - module_call_init(MODULE_INIT_DEVICE); if (watchdog) { @@ -5968,14 +5939,6 @@ int main(int argc, char **argv, char **envp) } } - for(i = 0; i < MAX_VIRTIO_CONSOLES; i++) { - const char *devname = virtio_consoles[i]; - if (virtcon_hds[i] && devname) { - if (strstart(devname, "vc", 0)) - qemu_chr_printf(virtcon_hds[i], "virtio console%d\r\n", i); - } - } - if (gdbstub_dev && gdbserver_start(gdbstub_dev) < 0) { fprintf(stderr, "qemu: could not open gdbserver on device '%s'\n", gdbstub_dev); -- 1.6.2.5 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* [Qemu-devel] [PATCH 4/4] virtio-console: Add a in-qemu api for open/read/write/close ports 2009-09-22 16:23 ` [Qemu-devel] [PATCH 3/4] virtio-console: Add support for multiple ports for generic guest-host communication Amit Shah @ 2009-09-22 16:23 ` Amit Shah 2009-09-23 9:12 ` Gerd Hoffmann 2009-09-23 9:07 ` [Qemu-devel] [PATCH 3/4] virtio-console: Add support for multiple ports for generic guest-host communication Gerd Hoffmann 1 sibling, 1 reply; 14+ messages in thread From: Amit Shah @ 2009-09-22 16:23 UTC (permalink / raw) To: qemu-devel; +Cc: Amit Shah This is a simple-to-use api for opening a port, registering a callback for read, writing to a port and closing it. Signed-off-by: Amit Shah <amit.shah@redhat.com> --- hw/virtio-console.c | 67 ++++++++++++++++++++++++++++++++++++++++++++++++-- hw/virtio-console.h | 5 ++++ 2 files changed, 69 insertions(+), 3 deletions(-) diff --git a/hw/virtio-console.c b/hw/virtio-console.c index d750c8e..c41f13c 100644 --- a/hw/virtio-console.c +++ b/hw/virtio-console.c @@ -58,6 +58,11 @@ struct VirtIOConsolePort { QTAILQ_HEAD(, VirtIOConsolePortBuffer) unflushed_buffer_head; + /* Callback that's invoked when we have a buffer that can be consumed + * by an in-qemu user of this port + */ + size_t (*read_callback)(const uint8_t *buf, const size_t len); + bool guest_connected; bool host_connected; }; @@ -128,10 +133,15 @@ static bool has_complete_data(VirtIOConsolePort *port) static size_t flush_buf(VirtIOConsolePort *port, const uint8_t *buf, size_t len) { - if (!port->hd) { - return 0; + int ret; + + ret = 0; + if (port->read_callback) { + ret = port->read_callback(buf, len); + } else if (port->hd) { + ret = qemu_chr_write(port->hd, buf, len); } - return qemu_chr_write(port->hd, buf, len); + return ret; } static void flush_queue(VirtIOConsolePort *port) @@ -443,6 +453,57 @@ static void vcon_event(void *opaque, int event) send_control_event(port, &cpkt); } + +/* Functions for use inside qemu to open and read from/write to ports */ +VirtIOConsolePort *virtio_console_open(uint32_t id, + size_t(*read_callback)(const uint8_t*buf, + const size_t len)) +{ + VirtIOConsolePort *port; + struct virtio_console_control cpkt; + + port = get_port_from_id(virtio_console, id); + if (!port) { + return NULL; + } + /* Don't allow opening an already-open port */ + if (port->host_connected) { + return NULL; + } + port->read_callback = read_callback; + + /* Send port open notification to the guest */ + port->host_connected = true; + cpkt.event = VIRTIO_CONSOLE_PORT_OPEN; + cpkt.value = 1; + send_control_event(port, &cpkt); + + return port; +} + +void virtio_console_close(VirtIOConsolePort *port) +{ + struct virtio_console_control cpkt; + + if (!port) + return; + + port->read_callback = NULL; + + cpkt.event = VIRTIO_CONSOLE_PORT_OPEN; + cpkt.value = 0; + send_control_event(port, &cpkt); +} + +size_t virtio_console_write(VirtIOConsolePort *port, uint8_t *buf, size_t size) +{ + if (!port || !port->host_connected) { + return 0; + } + return write_to_port(port, buf, size, false); +} + + static void set_active_in_map(uint32_t *map, uint32_t idx) { int i; diff --git a/hw/virtio-console.h b/hw/virtio-console.h index 526713e..5f0f549 100644 --- a/hw/virtio-console.h +++ b/hw/virtio-console.h @@ -88,5 +88,10 @@ VirtConBus *virtcon_bus_new(DeviceState *dev); typedef struct VirtIOConsolePort VirtIOConsolePort; void virtio_console_monitor_command(Monitor *mon, const char *command, const char *param); +VirtIOConsolePort *virtio_console_open(uint32_t id, + size_t(*read_callback)(const uint8_t*buf, + const size_t len)); +void virtio_console_close(VirtIOConsolePort *port); +size_t virtio_console_write(VirtIOConsolePort *port, uint8_t *buf, size_t size); #endif -- 1.6.2.5 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH 4/4] virtio-console: Add a in-qemu api for open/read/write/close ports 2009-09-22 16:23 ` [Qemu-devel] [PATCH 4/4] virtio-console: Add a in-qemu api for open/read/write/close ports Amit Shah @ 2009-09-23 9:12 ` Gerd Hoffmann 0 siblings, 0 replies; 14+ messages in thread From: Gerd Hoffmann @ 2009-09-23 9:12 UTC (permalink / raw) To: Amit Shah; +Cc: qemu-devel > diff --git a/hw/virtio-console.c b/hw/virtio-console.c > index d750c8e..c41f13c 100644 > --- a/hw/virtio-console.c > +++ b/hw/virtio-console.c > @@ -58,6 +58,11 @@ struct VirtIOConsolePort { > + size_t (*read_callback)(const uint8_t *buf, const size_t len); > + if (port->read_callback) { > + ret = port->read_callback(buf, len); > + } else if (port->hd) { > + ret = qemu_chr_write(port->hd, buf, len); > } > - return qemu_chr_write(port->hd, buf, len); No. port->info->data_for_you() again ;) > +/* Functions for use inside qemu to open and read from/write to ports */ > +VirtIOConsolePort *virtio_console_open(uint32_t id, > + size_t(*read_callback)(const uint8_t*buf, > + const size_t len)) Anyone who wants talk using a port should be a port driver. cheers, Gerd ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH 3/4] virtio-console: Add support for multiple ports for generic guest-host communication 2009-09-22 16:23 ` [Qemu-devel] [PATCH 3/4] virtio-console: Add support for multiple ports for generic guest-host communication Amit Shah 2009-09-22 16:23 ` [Qemu-devel] [PATCH 4/4] virtio-console: Add a in-qemu api for open/read/write/close ports Amit Shah @ 2009-09-23 9:07 ` Gerd Hoffmann 2009-09-23 9:43 ` Amit Shah 1 sibling, 1 reply; 14+ messages in thread From: Gerd Hoffmann @ 2009-09-23 9:07 UTC (permalink / raw) To: Amit Shah; +Cc: qemu-devel > CharDriverState *qdev_init_chardev(DeviceState *dev) > { > static int next_serial; > - static int next_virtconsole; > - /* FIXME: This is a nasty hack that needs to go away. */ Please don't drop this comment. The whole function is a nasty hack ;) > +VirtConBus *virtcon_bus_new(DeviceState *dev) > +{ > + if (virtcon_bus) { > + fprintf(stderr, "Can't create a second virtio-console bus\n"); > + return NULL; > + } > + if (!dev) { > + dev = qdev_create(NULL, "virtio-console-sysbus"); > + qdev_init(dev); > + } Can this actually happen? I think you'll allways have a virtio-console device as parent, right? Looks like cut+pasted from isa-bus.c. The ISA bus needs this for PCI-less machines where no PCI-ISA bridge is present and thus we need a isabus-sysbus bridge to hook up the ISA bus into the device tree. Try 'info qtree' on -M pc and -M isapc to see what I mean ;) > +VirtConDevice *virtcon_create(const char *name) > +VirtConDevice *virtcon_create_simple(const char *name) These functions should get a VirtConBus passed in as first argument. Looks like cut+paste from ISA bus again. The ISA bus is special here as you never ever can have two ISA busses in a single machine. That isn't true in general though. Might also be you don't need these functions at all. They usually used in case device creation is hard-coded somewhere (like standard isa devices present in every pc) or to keep old-style init functions working in the new qdev world (isa_ne2000_init for example). Devices which are only ever created via -device don't take this code path ... > +static void virtcon_bus_dev_print(Monitor *mon, DeviceState *dev, int indent) > +{ > + VirtConDevice *d = DO_UPCAST(VirtConDevice, qdev, dev); > + if (&d->qdev) { > + ; > + } > +} print callback isn't mandatory. If you don't want to print anything just drop it. > +static int virtcon_bus_init(SysBusDevice *dev) > +{ > + return 0; > +} > + > +static SysBusDeviceInfo virtcon_sysbus_info = { > + .init = virtcon_bus_init, > + .qdev.name = "virtio-console-sysbus", > + .qdev.size = sizeof(SysBusDevice), > + .qdev.no_user = 1, > +}; > + > +static void virtcon_sysbus_register_devices(void) > +{ > + sysbus_register_withprop(&virtcon_sysbus_info); > +} See above. I'm sure you don't need that. > +struct VirtIOConsolePort { > + DeviceState dev; > + > + VirtIOConsole *vcon; > + CharDriverState *hd; This looks wrong. > + char *name; > + > + QTAILQ_HEAD(, VirtIOConsolePortBuffer) unflushed_buffer_head; > + > + bool guest_connected; > + bool host_connected; > +}; Sticking a pointer to VirtConPortDeviceInfo here is probably handy. More consistent naming please. > +static int get_id_from_port(VirtIOConsolePort *port) > +{ > + uint32_t i; > + > + for (i = 0; i< MAX_VIRTIO_CONSOLE_PORTS; i++) { > + if (port == port->vcon->ports[i]) { > + return i; > + } > + } > + return VIRTIO_CONSOLE_BAD_ID; > +} Just sick a id element into VirtIOConsolePort? > +static bool has_complete_data(VirtIOConsolePort *port) > +{ > + VirtIOConsolePortBuffer *buf; > + size_t len, size; > + > + len = 0; > + size = 0; > + QTAILQ_FOREACH(buf,&port->unflushed_buffer_head, next) { > + if (!buf->size&& buf == QTAILQ_FIRST(&port->unflushed_buffer_head)) { > + /* We have a buffer that's lost its way; just flush it */ Can this happen? If not, assert() instead? > +static size_t flush_buf(VirtIOConsolePort *port, const uint8_t *buf, size_t len) > +{ > + if (!port->hd) { > + return 0; > + } > + return qemu_chr_write(port->hd, buf, len); port->info->data_for_you(port, buf, len); > +static void flush_queue(VirtIOConsolePort *port) > +{ > + VirtIOConsolePortBuffer *buf, *buf2; > + uint8_t *outbuf; > + size_t outlen, outsize; > + > + while (!QTAILQ_EMPTY(&port->unflushed_buffer_head)) { > + if (!has_complete_data(port)) { > + break; > + } > + > + buf = QTAILQ_FIRST(&port->unflushed_buffer_head); > + if (!buf->size) { > + /* This is a buf that didn't get consumed as part of a > + * previous data stream. Bad thing, shouldn't > + * happen. But let's handle it nonetheless > + */ If it shoudn't happen, then use assert(). If it triggers, find the bug. > +/* Guest wants to notify us of some event */ > +static void handle_control_message(VirtIOConsolePort *port, > + struct virtio_console_control *cpkt) > +{ > + uint8_t *buffer; > + size_t buffer_len; > + > + switch(cpkt->event) { > + case VIRTIO_CONSOLE_PORT_OPEN: > + port->guest_connected = cpkt->value; port->info->guest_open() notify callback? > +static int vcon_port_initfn(VirtConDevice *dev) > +{ > + VirtIOConsolePort *port; > + > + port = DO_UPCAST(VirtIOConsolePort, dev,&dev->qdev); > + > + QTAILQ_INIT(&port->unflushed_buffer_head); > + > + port->vcon = virtio_console; > + > + qemu_chr_add_handlers(port->hd, vcon_can_read, vcon_read, vcon_event, port); Why have a chardev here? > +typedef struct VirtConPortDeviceInfo { > + DeviceInfo qdev; > + virtcon_port_qdev_initfn init; Stick in more function pointers here. guest_open(), data_for_you(), ... Well. The whole thing is still *way* to mixed up. It should be cleanly separated. You should be able to move the port driver(s) to a separate source file without much trouble. Only the port driver should deal with a chardev. The virtio-console core should not care at all how the data is piped to the (host side) users. It just drives the ring, forwards events, accepts data for the guest (via helper function), passes on data from the guest (via callback in VirtConPortDeviceInfo). cheers, Gerd ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH 3/4] virtio-console: Add support for multiple ports for generic guest-host communication 2009-09-23 9:07 ` [Qemu-devel] [PATCH 3/4] virtio-console: Add support for multiple ports for generic guest-host communication Gerd Hoffmann @ 2009-09-23 9:43 ` Amit Shah 2009-09-23 11:20 ` Gerd Hoffmann 0 siblings, 1 reply; 14+ messages in thread From: Amit Shah @ 2009-09-23 9:43 UTC (permalink / raw) To: Gerd Hoffmann; +Cc: qemu-devel On (Wed) Sep 23 2009 [11:07:02], Gerd Hoffmann wrote: > >> CharDriverState *qdev_init_chardev(DeviceState *dev) >> { >> static int next_serial; >> - static int next_virtconsole; >> - /* FIXME: This is a nasty hack that needs to go away. */ > > Please don't drop this comment. The whole function is a nasty hack ;) OK :-) >> +VirtConBus *virtcon_bus_new(DeviceState *dev) >> +{ >> + if (virtcon_bus) { >> + fprintf(stderr, "Can't create a second virtio-console bus\n"); >> + return NULL; >> + } >> + if (!dev) { >> + dev = qdev_create(NULL, "virtio-console-sysbus"); >> + qdev_init(dev); >> + } > > Can this actually happen? I think you'll allways have a virtio-console > device as parent, right? > > Looks like cut+pasted from isa-bus.c. The ISA bus needs this for > PCI-less machines where no PCI-ISA bridge is present and thus we need a > isabus-sysbus bridge to hook up the ISA bus into the device tree. Try > 'info qtree' on -M pc and -M isapc to see what I mean ;) You're right; I've mostly picked up the bits from isa-bus. I've not done any thinking yet. >> +VirtConDevice *virtcon_create(const char *name) >> +VirtConDevice *virtcon_create_simple(const char *name) > > These functions should get a VirtConBus passed in as first argument. > > Looks like cut+paste from ISA bus again. The ISA bus is special here as > you never ever can have two ISA busses in a single machine. That isn't > true in general though. OK; I think receiving the bus argument might be helpful when we support more than 1 device. > Might also be you don't need these functions at all. They usually used > in case device creation is hard-coded somewhere (like standard isa > devices present in every pc) or to keep old-style init functions working > in the new qdev world (isa_ne2000_init for example). Devices which are > only ever created via -device don't take this code path ... I think deprecating the old-style -virtioconsole is the better option. >> +static void virtcon_bus_dev_print(Monitor *mon, DeviceState *dev, int indent) >> +{ >> + VirtConDevice *d = DO_UPCAST(VirtConDevice, qdev, dev); >> + if (&d->qdev) { >> + ; >> + } >> +} > > print callback isn't mandatory. If you don't want to print anything > just drop it. Yeah; I thought I'd have something to be displayed here later. >> +static int virtcon_bus_init(SysBusDevice *dev) >> +{ >> + return 0; >> +} >> + >> +static SysBusDeviceInfo virtcon_sysbus_info = { >> + .init = virtcon_bus_init, >> + .qdev.name = "virtio-console-sysbus", >> + .qdev.size = sizeof(SysBusDevice), >> + .qdev.no_user = 1, >> +}; >> + >> +static void virtcon_sysbus_register_devices(void) >> +{ >> + sysbus_register_withprop(&virtcon_sysbus_info); >> +} > > See above. I'm sure you don't need that. Yeah; will drop. >> +struct VirtIOConsolePort { >> + DeviceState dev; >> + >> + VirtIOConsole *vcon; >> + CharDriverState *hd; > > This looks wrong. We shouldn't have a charstate at all? >> + char *name; >> + >> + QTAILQ_HEAD(, VirtIOConsolePortBuffer) unflushed_buffer_head; >> + >> + bool guest_connected; >> + bool host_connected; >> +}; > > Sticking a pointer to VirtConPortDeviceInfo here is probably handy. > More consistent naming please. Consistent naming for what? I've only put the new bus stuff in the new file and this file has largely remained as-is, with a few functions changed to accomodate the new init methods. >> +static int get_id_from_port(VirtIOConsolePort *port) >> +{ >> + uint32_t i; >> + >> + for (i = 0; i< MAX_VIRTIO_CONSOLE_PORTS; i++) { >> + if (port == port->vcon->ports[i]) { >> + return i; >> + } >> + } >> + return VIRTIO_CONSOLE_BAD_ID; >> +} > > Just sick a id element into VirtIOConsolePort? Yes, that's on the todo list. >> +static bool has_complete_data(VirtIOConsolePort *port) >> +{ >> + VirtIOConsolePortBuffer *buf; >> + size_t len, size; >> + >> + len = 0; >> + size = 0; >> + QTAILQ_FOREACH(buf,&port->unflushed_buffer_head, next) { >> + if (!buf->size&& buf == QTAILQ_FIRST(&port->unflushed_buffer_head)) { >> + /* We have a buffer that's lost its way; just flush it */ > > Can this happen? If not, assert() instead? Shouldn't happen; but it's not a serious thing to error out instead. >> +static size_t flush_buf(VirtIOConsolePort *port, const uint8_t *buf, size_t len) >> +{ >> + if (!port->hd) { >> + return 0; >> + } >> + return qemu_chr_write(port->hd, buf, len); > > port->info->data_for_you(port, buf, len); OK; haven't yet seen how the charstate can be made transparent. >> +static void flush_queue(VirtIOConsolePort *port) >> +{ >> + VirtIOConsolePortBuffer *buf, *buf2; >> + uint8_t *outbuf; >> + size_t outlen, outsize; >> + >> + while (!QTAILQ_EMPTY(&port->unflushed_buffer_head)) { >> + if (!has_complete_data(port)) { >> + break; >> + } >> + >> + buf = QTAILQ_FIRST(&port->unflushed_buffer_head); >> + if (!buf->size) { >> + /* This is a buf that didn't get consumed as part of a >> + * previous data stream. Bad thing, shouldn't >> + * happen. But let's handle it nonetheless >> + */ > > If it shoudn't happen, then use assert(). If it triggers, find the bug. The bug could actually be in the guest and not in qemu. Would be wrong to penalise in that case, I guess. >> +/* Guest wants to notify us of some event */ >> +static void handle_control_message(VirtIOConsolePort *port, >> + struct virtio_console_control *cpkt) >> +{ >> + uint8_t *buffer; >> + size_t buffer_len; >> + >> + switch(cpkt->event) { >> + case VIRTIO_CONSOLE_PORT_OPEN: >> + port->guest_connected = cpkt->value; > > port->info->guest_open() notify callback? You mean handle it in an async path? >> +static int vcon_port_initfn(VirtConDevice *dev) >> +{ >> + VirtIOConsolePort *port; >> + >> + port = DO_UPCAST(VirtIOConsolePort, dev,&dev->qdev); >> + >> + QTAILQ_INIT(&port->unflushed_buffer_head); >> + >> + port->vcon = virtio_console; >> + >> + qemu_chr_add_handlers(port->hd, vcon_can_read, vcon_read, vcon_event, port); > > Why have a chardev here? 1. Don't know how to make it transparent, 2. Don't know how to init these function handlers otherwise >> +typedef struct VirtConPortDeviceInfo { >> + DeviceInfo qdev; >> + virtcon_port_qdev_initfn init; > > Stick in more function pointers here. guest_open(), data_for_you(), ... > > > Well. The whole thing is still *way* to mixed up. It should be cleanly > separated. Yeah; I've only got it working with -device so far. So I guess I'll have to bug you more to get this further into shape :-) > You should be able to move the port driver(s) to a separate source file > without much trouble. Only the port driver should deal with a chardev. Oh OK; maybe I understand what you're saying about the chardevs now. > The virtio-console core should not care at all how the data is piped to > the (host side) users. It just drives the ring, forwards events, > accepts data for the guest (via helper function), passes on data from > the guest (via callback in VirtConPortDeviceInfo). Hm, let me think over this. Amit ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH 3/4] virtio-console: Add support for multiple ports for generic guest-host communication 2009-09-23 9:43 ` Amit Shah @ 2009-09-23 11:20 ` Gerd Hoffmann 2009-09-23 11:50 ` Amit Shah 0 siblings, 1 reply; 14+ messages in thread From: Gerd Hoffmann @ 2009-09-23 11:20 UTC (permalink / raw) To: Amit Shah; +Cc: qemu-devel [-- Attachment #1: Type: text/plain, Size: 4026 bytes --] >>> +struct VirtIOConsolePort { >>> + DeviceState dev; >>> + >>> + VirtIOConsole *vcon; >>> + CharDriverState *hd; >> >> This looks wrong. > > We shouldn't have a charstate at all? Not here. More below ... >>> + char *name; >>> + >>> + QTAILQ_HEAD(, VirtIOConsolePortBuffer) unflushed_buffer_head; >>> + >>> + bool guest_connected; >>> + bool host_connected; >>> +}; >> >> Sticking a pointer to VirtConPortDeviceInfo here is probably handy. >> More consistent naming please. > > Consistent naming for what? The structs. Pick a prefix (say VirtCon) and stick with that. Then for the bus implementation: VirtConBus (fine) VirtConPort (VirtIOConsolePort now) VirtConPortInfo (VirtConPortDeviceInfo now) The console port driver could name its state info this way: VirtConPortConsole (doesn't exist right now it seems ...). > I've only put the new bus stuff in the new file and this file has > largely remained as-is, with a few functions changed to accomodate the > new init methods. This is about more than just the init ... >>> +static bool has_complete_data(VirtIOConsolePort *port) >>> +{ >>> + VirtIOConsolePortBuffer *buf; >>> + size_t len, size; >>> + >>> + len = 0; >>> + size = 0; >>> + QTAILQ_FOREACH(buf,&port->unflushed_buffer_head, next) { >>> + if (!buf->size&& buf == QTAILQ_FIRST(&port->unflushed_buffer_head)) { >>> + /* We have a buffer that's lost its way; just flush it */ >> >> Can this happen? If not, assert() instead? > > Shouldn't happen; but it's not a serious thing to error out instead. It indicates a bug somewhere though, doesn't it? I'd suggest to not paper over it. >>> +static size_t flush_buf(VirtIOConsolePort *port, const uint8_t *buf, size_t len) >>> +{ >>> + if (!port->hd) { >>> + return 0; >>> + } >>> + return qemu_chr_write(port->hd, buf, len); >> >> port->info->data_for_you(port, buf, len); > > OK; haven't yet seen how the charstate can be made transparent. chardev should go into VirtConPortConsole. >> If it shoudn't happen, then use assert(). If it triggers, find the bug. > > The bug could actually be in the guest and not in qemu. Would be wrong > to penalise in that case, I guess. Ah, ok. Fine then. Aborting qemu on guest bugs would be insane. >>> +/* Guest wants to notify us of some event */ >>> +static void handle_control_message(VirtIOConsolePort *port, >>> + struct virtio_console_control *cpkt) >>> +{ >>> + uint8_t *buffer; >>> + size_t buffer_len; >>> + >>> + switch(cpkt->event) { >>> + case VIRTIO_CONSOLE_PORT_OPEN: >>> + port->guest_connected = cpkt->value; >> >> port->info->guest_open() notify callback? > > You mean handle it in an async path? No, have a way to notify the port driver about the state change. >> Stick in more function pointers here. guest_open(), data_for_you(), ... >> >> >> Well. The whole thing is still *way* to mixed up. It should be cleanly >> separated. > > Yeah; I've only got it working with -device so far. So I guess I'll have > to bug you more to get this further into shape :-) > >> You should be able to move the port driver(s) to a separate source file >> without much trouble. Only the port driver should deal with a chardev. > > Oh OK; maybe I understand what you're saying about the chardevs now. > >> The virtio-console core should not care at all how the data is piped to >> the (host side) users. It just drives the ring, forwards events, >> accepts data for the guest (via helper function), passes on data from >> the guest (via callback in VirtConPortDeviceInfo). > > Hm, let me think over this. A port driver should look roughly like the attached one. That one does something completely different: Implement a watchdog ;) Warning: didn't even compile it. The console port driver would have the chardev instead of the timer in the driver state struct and would basically forward the data between the port and the chardev. HTH, Gerd [-- Attachment #2: wdt_vmchannel.c --] [-- Type: text/x-csrc, Size: 1565 bytes --] #include "hw.h" #include "virtio-channel.h" typedef struct VirtConPortWatchdog { VirtConPort dev; QEMUTimer *timer; uint32_t timeout; } VirtConPortWatchdog; static void timer_expired(void *ptr) { VirtConPortWatchdog *s = ptr; watchdog_perform_action(); qemu_del_timer(s->timer); } static int watchdog_open(VirtConPort *dev) { VirtConPortWatchdog *s = DO_UPCAST(VirtConPortWatchdog, dev, dev); qemu_mod_timer(s->timer, qemu_get_clock (vm_clock) + s->timeout * get_ticks_per_sec()); } static int watchdog_data(VirtConPort *dev, uint8_t *buf, int len) { VirtConPortWatchdog *s = DO_UPCAST(VirtConPortWatchdog, dev, dev); qemu_mod_timer(s->timer, qemu_get_clock (vm_clock) + s->timeout * get_ticks_per_sec()); } static int watchdog_init(VirtConPort *dev) { VirtConPortWatchdog *s = DO_UPCAST(VirtConPortWatchdog, dev, dev); s->dev.name = "org.qemu.watchdog"; s->timer = qemu_new_timer(vm_clock, timer_expired, s); } VirtConPortInfo watchdog_info = { .qdev.name = "vmch-watchdog", .qdev.size = sizeof(VirtConPortWatchdog), .qdev.vmsd = /* todo */, .qdev.reset = /* todo */, .init = watchdog_init, .guest_open = watchdog_open, .data_for_you = watchdog_data, .qdev.props = (Property[]) { DEFINE_PROP_UINT32("timeout", VirtConPortWatchdog, timeout, 120), DEFINE_PROP_END_OF_LIST }, }; void watchdog_register(void) { virtcon_port_qdev_register(&watchdog_info); } device_init(watchdog_register) ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH 3/4] virtio-console: Add support for multiple ports for generic guest-host communication 2009-09-23 11:20 ` Gerd Hoffmann @ 2009-09-23 11:50 ` Amit Shah 2009-09-23 12:30 ` Gerd Hoffmann 0 siblings, 1 reply; 14+ messages in thread From: Amit Shah @ 2009-09-23 11:50 UTC (permalink / raw) To: Gerd Hoffmann; +Cc: qemu-devel On (Wed) Sep 23 2009 [13:20:33], Gerd Hoffmann wrote: > >>>> + char *name; >>>> + >>>> + QTAILQ_HEAD(, VirtIOConsolePortBuffer) unflushed_buffer_head; >>>> + >>>> + bool guest_connected; >>>> + bool host_connected; >>>> +}; >>> >>> Sticking a pointer to VirtConPortDeviceInfo here is probably handy. >>> More consistent naming please. >> >> Consistent naming for what? > > The structs. Pick a prefix (say VirtCon) and stick with that. Then for > the bus implementation: > > VirtConBus (fine) > VirtConPort (VirtIOConsolePort now) > VirtConPortInfo (VirtConPortDeviceInfo now) Yeah; I've not renamed the structs yet. > The console port driver could name its state info this way: > > VirtConPortConsole (doesn't exist right now it seems ...). You're actually suggesting to split everything -- a console (in the current sense) is a different device and a port (the new vmchannels) are different devices. Right? >>> You should be able to move the port driver(s) to a separate source file >>> without much trouble. Only the port driver should deal with a chardev. >> >> Oh OK; maybe I understand what you're saying about the chardevs now. >> >>> The virtio-console core should not care at all how the data is piped to >>> the (host side) users. It just drives the ring, forwards events, >>> accepts data for the guest (via helper function), passes on data from >>> the guest (via callback in VirtConPortDeviceInfo). >> >> Hm, let me think over this. > > A port driver should look roughly like the attached one. That one does > something completely different: Implement a watchdog ;) Warning: > didn't even compile it. > > The console port driver would have the chardev instead of the timer in > the driver state struct and would basically forward the data between the > port and the chardev. Thanks; parsing all this.. Amit ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH 3/4] virtio-console: Add support for multiple ports for generic guest-host communication 2009-09-23 11:50 ` Amit Shah @ 2009-09-23 12:30 ` Gerd Hoffmann 2009-09-23 12:40 ` Amit Shah 0 siblings, 1 reply; 14+ messages in thread From: Gerd Hoffmann @ 2009-09-23 12:30 UTC (permalink / raw) To: Amit Shah; +Cc: qemu-devel On 09/23/09 13:50, Amit Shah wrote: >> VirtConPortConsole (doesn't exist right now it seems ...). > > You're actually suggesting to split everything -- a console (in the > current sense) is a different device and a port (the new vmchannels) are > different devices. Right? Yes, you'll have two devices. #1 virtio-console which handles all virtio and is the master of the portbus. #2 VirtChanPortConsole, which links the port and the chardev. With this in place you'll have the current virtio-console functionality. And on top you can create multiple console ports. Then we can go enhance things and add more port drivers: #3: One driver which allows external users (i.e. libguestfs) by linking a chardev too and attach a name tag. That one can probably share all code except the init function with the console port driver. #4: the watchdog. #5: the vnc clipboard bits. #6: whatever else we might find useful. Ignoring backward-compatibility for now things should go like this: -device virtio-console Creates the portbus master. No ports (yet). -device vmport-console,chardev=$dev Creates a port named 'org.qemu.console' and links it to $dev. -device vmport-channel,chardev=$dev,name=$name Creates a port named $name, links it to $dev, for libguestfs and other external users. -device vmport-watchdog Creates a port named 'org.qemu.watchdog', start timer when the guest openes the device, restart the watchdog timer each time the guest writes something, trigger watchdog action when the timer expires. Note: No chardev needed here. -device vmport-clipboard vnc clipboard stuff goes here. Probably needs no chardev too. For backward-compatibility we'll allways have to create a vmport-console at port 0, so there will never be a virtio-console without a port. That is a minor bit we can fixup once the above works fine. cheers, Gerd ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH 3/4] virtio-console: Add support for multiple ports for generic guest-host communication 2009-09-23 12:30 ` Gerd Hoffmann @ 2009-09-23 12:40 ` Amit Shah 2009-09-23 13:04 ` Gerd Hoffmann 0 siblings, 1 reply; 14+ messages in thread From: Amit Shah @ 2009-09-23 12:40 UTC (permalink / raw) To: Gerd Hoffmann; +Cc: qemu-devel On (Wed) Sep 23 2009 [14:30:39], Gerd Hoffmann wrote: > On 09/23/09 13:50, Amit Shah wrote: >>> VirtConPortConsole (doesn't exist right now it seems ...). >> >> You're actually suggesting to split everything -- a console (in the >> current sense) is a different device and a port (the new vmchannels) are >> different devices. Right? > > Yes, you'll have two devices. > > #1 virtio-console which handles all virtio and is the master of > the portbus. Right; This is in place now. > #2 VirtChanPortConsole, which links the port and the chardev. OK; So based on the last mail the port functionality could be split in a separate file. > With this in place you'll have the current virtio-console functionality. > And on top you can create multiple console ports. > > Then we can go enhance things and add more port drivers: > > #3: One driver which allows external users (i.e. libguestfs) by linking > a chardev too and attach a name tag. That one can probably share > all code except the init function with the console port driver. > #4: the watchdog. > #5: the vnc clipboard bits. > #6: whatever else we might find useful. Right; this model looks good. It wasn't obvious to me the first time you mentioned it (I didn't think adding something like a VirtConPortVnc and some init functions in vnc.c would make any sense -- but actually looks like it would be cleaner to do that). > Ignoring backward-compatibility for now things should go like this: > > -device virtio-console > Creates the portbus master. No ports (yet). > > -device vmport-console,chardev=$dev > Creates a port named 'org.qemu.console' and links it to $dev. OK; that's what I have now, slightly different: -device virtio-console-pci -device virtconport,port=0,chardev=$dev > -device vmport-channel,chardev=$dev,name=$name > Creates a port named $name, links it to $dev, for libguestfs and > other external users. > > -device vmport-watchdog > Creates a port named 'org.qemu.watchdog', start timer when the guest > openes the device, restart the watchdog timer each time the guest > writes something, trigger watchdog action when the timer expires. > Note: No chardev needed here. > > -device vmport-clipboard > vnc clipboard stuff goes here. Probably needs no chardev too. > > For backward-compatibility we'll allways have to create a vmport-console > at port 0, so there will never be a virtio-console without a port. That > is a minor bit we can fixup once the above works fine. ie -virtioconsole <chardev> ? I'd suggest we just drop that in 0.12. Amit ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH 3/4] virtio-console: Add support for multiple ports for generic guest-host communication 2009-09-23 12:40 ` Amit Shah @ 2009-09-23 13:04 ` Gerd Hoffmann 2009-09-23 13:17 ` Amit Shah 0 siblings, 1 reply; 14+ messages in thread From: Gerd Hoffmann @ 2009-09-23 13:04 UTC (permalink / raw) To: Amit Shah; +Cc: qemu-devel >> -device virtio-console >> Creates the portbus master. No ports (yet). >> >> -device vmport-console,chardev=$dev >> Creates a port named 'org.qemu.console' and links it to $dev. > > OK; that's what I have now, slightly different: > > -device virtio-console-pci -device virtconport,port=0,chardev=$dev Fine as well. The exact names don't really matter, the naming for the port drivers should be consistent though (i.e. have the same prefix for all of them or something like that). Optional port number looks reasonable too. Should default to auto-allocation if not specified. >> For backward-compatibility we'll allways have to create a vmport-console >> at port 0, so there will never be a virtio-console without a port. That >> is a minor bit we can fixup once the above works fine. > > ie -virtioconsole<chardev> ? There are two kinds of backward compatibility ;) One is the qemu command line. The other is the ABI for old guest drivers. The later requires a console at port 0 ... > I'd suggest we just drop that in 0.12. Dropping the command line switch is with me. In case someone disagrees (libvirt folks?) it isn't hard to maintain compatibility though. cheers, Gerd ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH 3/4] virtio-console: Add support for multiple ports for generic guest-host communication 2009-09-23 13:04 ` Gerd Hoffmann @ 2009-09-23 13:17 ` Amit Shah 0 siblings, 0 replies; 14+ messages in thread From: Amit Shah @ 2009-09-23 13:17 UTC (permalink / raw) To: Gerd Hoffmann; +Cc: qemu-devel On (Wed) Sep 23 2009 [15:04:27], Gerd Hoffmann wrote: >>> -device virtio-console >>> Creates the portbus master. No ports (yet). >>> >>> -device vmport-console,chardev=$dev >>> Creates a port named 'org.qemu.console' and links it to $dev. >> >> OK; that's what I have now, slightly different: >> >> -device virtio-console-pci -device virtconport,port=0,chardev=$dev > > Fine as well. The exact names don't really matter, the naming for the > port drivers should be consistent though (i.e. have the same prefix for Yes. > all of them or something like that). Optional port number looks > reasonable too. Should default to auto-allocation if not specified. Once that gets decided :-) It's still not decided if we'll be optional port numbers vs assigned. >>> For backward-compatibility we'll allways have to create a vmport-console >>> at port 0, so there will never be a virtio-console without a port. That >>> is a minor bit we can fixup once the above works fine. >> >> ie -virtioconsole<chardev> ? > > There are two kinds of backward compatibility ;) One is the qemu Yes, I wanted to know which one you're thinking about. > command line. The other is the ABI for old guest drivers. The later > requires a console at port 0 ... For that, there are two options: 1. If we go with a static port function -> port number mapping, this is easy (as I already have now). 2. If we go with dynamic numbers, the guest could register the hvc console only for a device that has the name 'org.qemu.console.0'. This supports multiple ports as well. >> I'd suggest we just drop that in 0.12. > > Dropping the command line switch is with me. In case someone disagrees > (libvirt folks?) it isn't hard to maintain compatibility though. It's not much effort to maintain that, but the old and new syntaxes are very different. qemu 0.11 doesn't offer the ability to support multiple virtioconsoles, but 0.12 will. So the cmd line syntax has to change for that anyway, might as well deprecate the older syntax. Amit ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2009-09-23 13:18 UTC | newest] Thread overview: 14+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2009-09-22 16:23 [Qemu-devel] Multiple port support for virtio-console Amit Shah 2009-09-22 16:23 ` [Qemu-devel] [PATCH 1/4] char: Emit 'OPENED' events on char device open Amit Shah 2009-09-22 16:23 ` [Qemu-devel] [PATCH 2/4] qdev: add string property Amit Shah 2009-09-22 16:23 ` [Qemu-devel] [PATCH 3/4] virtio-console: Add support for multiple ports for generic guest-host communication Amit Shah 2009-09-22 16:23 ` [Qemu-devel] [PATCH 4/4] virtio-console: Add a in-qemu api for open/read/write/close ports Amit Shah 2009-09-23 9:12 ` Gerd Hoffmann 2009-09-23 9:07 ` [Qemu-devel] [PATCH 3/4] virtio-console: Add support for multiple ports for generic guest-host communication Gerd Hoffmann 2009-09-23 9:43 ` Amit Shah 2009-09-23 11:20 ` Gerd Hoffmann 2009-09-23 11:50 ` Amit Shah 2009-09-23 12:30 ` Gerd Hoffmann 2009-09-23 12:40 ` Amit Shah 2009-09-23 13:04 ` Gerd Hoffmann 2009-09-23 13:17 ` Amit Shah
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).