* [PATCH 2/9] virtio-serial-bus: save/load: Ensure nr_ports on src and dest are same.
From: Amit Shah @ 2010-03-19 11:58 UTC (permalink / raw)
To: qemu-devel; +Cc: Amit Shah, quintela, mst, virtualization
In-Reply-To: <1268999926-29560-2-git-send-email-amit.shah@redhat.com>
The number of ports on the source as well as the destination machines
should match. If they don't, it means some ports that got hotplugged on
the source aren't instantiated on the destination. Or that ports that
were hot-unplugged on the source are created on the destination.
Signed-off-by: Amit Shah <amit.shah@redhat.com>
Reported-by: Juan Quintela <quintela@redhat.com>
---
hw/virtio-serial-bus.c | 18 ++++++++++++++++--
1 files changed, 16 insertions(+), 2 deletions(-)
diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c
index 36985a1..f43d1fc 100644
--- a/hw/virtio-serial-bus.c
+++ b/hw/virtio-serial-bus.c
@@ -401,7 +401,7 @@ static int virtio_serial_load(QEMUFile *f, void *opaque, int version_id)
{
VirtIOSerial *s = opaque;
VirtIOSerialPort *port;
- uint32_t max_nr_ports, nr_active_ports;
+ uint32_t max_nr_ports, nr_active_ports, nr_ports;
unsigned int i;
if (version_id > 2) {
@@ -418,7 +418,21 @@ static int virtio_serial_load(QEMUFile *f, void *opaque, int version_id)
/* The config space */
qemu_get_be16s(f, &s->config.cols);
qemu_get_be16s(f, &s->config.rows);
- s->config.nr_ports = qemu_get_be32(f);
+ nr_ports = qemu_get_be32(f);
+
+ if (nr_ports != s->config.nr_ports) {
+ /*
+ * Source hot-plugged/unplugged ports and we don't have all of
+ * them here.
+ *
+ * Note: This condition cannot check for all hotplug/unplug
+ * events: eg, if one port was hot-plugged and one was
+ * unplugged, the nr_ports remains the same but the port id's
+ * would have changed and we won't catch it here. A later
+ * check for !find_port_by_id() will confirm if this happened.
+ */
+ return -EINVAL;
+ }
/* Items in struct VirtIOSerial */
--
1.6.2.5
^ permalink raw reply related
* [PATCH 3/9] virtio-serial: save/load: Ensure we have hot-plugged ports instantiated
From: Amit Shah @ 2010-03-19 11:58 UTC (permalink / raw)
To: qemu-devel; +Cc: Amit Shah, quintela, mst, virtualization
In-Reply-To: <1268999926-29560-3-git-send-email-amit.shah@redhat.com>
If some ports that were hot-plugged on the source are not available on
the destination, fail migration instead of trying to deref a NULL
pointer.
Signed-off-by: Amit Shah <amit.shah@redhat.com>
Reported-by: Juan Quintela <quintela@redhat.com>
---
hw/virtio-serial-bus.c | 7 +++++++
1 files changed, 7 insertions(+), 0 deletions(-)
diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c
index f43d1fc..830eb75 100644
--- a/hw/virtio-serial-bus.c
+++ b/hw/virtio-serial-bus.c
@@ -450,6 +450,13 @@ static int virtio_serial_load(QEMUFile *f, void *opaque, int version_id)
id = qemu_get_be32(f);
port = find_port_by_id(s, id);
+ if (!port) {
+ /*
+ * The requested port was hot-plugged on the source but we
+ * don't have it
+ */
+ return -EINVAL;
+ }
port->guest_connected = qemu_get_byte(f);
}
--
1.6.2.5
^ permalink raw reply related
* [PATCH 4/9] virtio-serial: Handle scatter-gather buffers for control messages
From: Amit Shah @ 2010-03-19 11:58 UTC (permalink / raw)
To: qemu-devel; +Cc: quintela, mst, virtualization, Avi Kivity, Amit Shah
In-Reply-To: <1268999926-29560-4-git-send-email-amit.shah@redhat.com>
Current control messages are small enough to not be split into multiple
buffers but we could run into such a situation in the future or a
malicious guest could cause such a situation.
So handle the entire iov request for control messages.
Also ensure the size of the control request is >= what we expect
otherwise we risk accessing memory that we don't own.
Signed-off-by: Amit Shah <amit.shah@redhat.com>
CC: Avi Kivity <avi@redhat.com>
Reported-by: Avi Kivity <avi@redhat.com>
---
hw/virtio-serial-bus.c | 43 ++++++++++++++++++++++++++++++++++++++++---
1 files changed, 40 insertions(+), 3 deletions(-)
diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c
index 830eb75..d5887ab 100644
--- a/hw/virtio-serial-bus.c
+++ b/hw/virtio-serial-bus.c
@@ -200,7 +200,7 @@ size_t virtio_serial_guest_ready(VirtIOSerialPort *port)
}
/* Guest wants to notify us of some event */
-static void handle_control_message(VirtIOSerial *vser, void *buf)
+static void handle_control_message(VirtIOSerial *vser, void *buf, size_t len)
{
struct VirtIOSerialPort *port;
struct virtio_console_control cpkt, *gcpkt;
@@ -208,6 +208,10 @@ static void handle_control_message(VirtIOSerial *vser, void *buf)
size_t buffer_len;
gcpkt = buf;
+ if (len < sizeof(cpkt)) {
+ /* The guest sent an invalid control packet */
+ return;
+ }
port = find_port_by_id(vser, ldl_p(&gcpkt->id));
if (!port)
return;
@@ -281,12 +285,45 @@ static void control_out(VirtIODevice *vdev, VirtQueue *vq)
{
VirtQueueElement elem;
VirtIOSerial *vser;
+ uint8_t *buf;
+ size_t len;
vser = DO_UPCAST(VirtIOSerial, vdev, vdev);
+ len = 0;
+ buf = NULL;
while (virtqueue_pop(vq, &elem)) {
- handle_control_message(vser, elem.out_sg[0].iov_base);
- virtqueue_push(vq, &elem, elem.out_sg[0].iov_len);
+ unsigned int i;
+ size_t cur_len, offset;
+
+ cur_len = 0;
+ for (i = 0; i < elem.out_num; i++) {
+ cur_len += elem.out_sg[i].iov_len;
+ }
+ /*
+ * Allocate a new buf only if we didn't have one previously or
+ * if the size of the buf differs
+ */
+ if (cur_len != len) {
+ if (len) {
+ qemu_free(buf);
+ }
+ buf = qemu_malloc(cur_len);
+ }
+
+ offset = 0;
+ for (i = 0; i < elem.out_num; i++) {
+ memcpy(buf + offset, elem.out_sg[i].iov_base,
+ elem.out_sg[i].iov_len);
+ offset += elem.out_sg[i].iov_len;
+ }
+ len = cur_len;
+
+ handle_control_message(vser, buf, len);
+ virtqueue_push(vq, &elem, len);
+ }
+ if (len) {
+ qemu_free(buf);
}
virtio_notify(vdev, vq);
}
--
1.6.2.5
^ permalink raw reply related
* [PATCH 5/9] virtio-serial: Handle scatter/gather input from the guest
From: Amit Shah @ 2010-03-19 11:58 UTC (permalink / raw)
To: qemu-devel; +Cc: quintela, mst, virtualization, Avi Kivity, Amit Shah
In-Reply-To: <1268999926-29560-5-git-send-email-amit.shah@redhat.com>
Current guests don't send more than one iov but it can change later.
Ensure we handle that case.
Signed-off-by: Amit Shah <amit.shah@redhat.com>
CC: Avi Kivity <avi@redhat.com>
---
hw/virtio-serial-bus.c | 22 +++++++++++++++-------
1 files changed, 15 insertions(+), 7 deletions(-)
diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c
index d5887ab..19e8c59 100644
--- a/hw/virtio-serial-bus.c
+++ b/hw/virtio-serial-bus.c
@@ -338,11 +338,12 @@ static void handle_output(VirtIODevice *vdev, VirtQueue *vq)
while (virtqueue_pop(vq, &elem)) {
VirtIOSerialPort *port;
- size_t ret;
+ size_t len;
+ unsigned int i;
+ len = 0;
port = find_port_by_vq(vser, vq);
if (!port) {
- ret = 0;
goto next_buf;
}
@@ -352,16 +353,23 @@ static void handle_output(VirtIODevice *vdev, VirtQueue *vq)
* with it. Just ignore the data in that case.
*/
if (!port->info->have_data) {
- ret = 0;
goto next_buf;
}
- /* The guest always sends only one sg */
- ret = port->info->have_data(port, elem.out_sg[0].iov_base,
- elem.out_sg[0].iov_len);
+ for (i = 0; i < elem.out_num; i++) {
+ size_t ret;
+
+ ret = port->info->have_data(port, elem.out_sg[0].iov_base,
+ elem.out_sg[0].iov_len);
+ if (ret < elem.out_sg[0].iov_len) {
+ /* We couldn't write the entire iov; stop processing now */
+ break;
+ }
+ len += ret;
+ }
next_buf:
- virtqueue_push(vq, &elem, ret);
+ virtqueue_push(vq, &elem, len);
}
virtio_notify(vdev, vq);
}
--
1.6.2.5
^ permalink raw reply related
* [PATCH 6/9] virtio-serial: Remove redundant check for 0-sized write request
From: Amit Shah @ 2010-03-19 11:58 UTC (permalink / raw)
To: qemu-devel; +Cc: Amit Shah, quintela, mst, virtualization
In-Reply-To: <1268999926-29560-6-git-send-email-amit.shah@redhat.com>
The check for a 0-sized write request to a guest port is not necessary;
the while loop below won't be executed in this case and all will be
fine.
Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
hw/virtio-serial-bus.c | 3 ---
1 files changed, 0 insertions(+), 3 deletions(-)
diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c
index 19e8c59..2603dcd 100644
--- a/hw/virtio-serial-bus.c
+++ b/hw/virtio-serial-bus.c
@@ -83,9 +83,6 @@ static size_t write_to_port(VirtIOSerialPort *port,
if (!virtio_queue_ready(vq)) {
return 0;
}
- if (!size) {
- return 0;
- }
while (offset < size) {
int i;
--
1.6.2.5
^ permalink raw reply related
* [PATCH 7/9] virtio-serial: Update copyright year to 2010
From: Amit Shah @ 2010-03-19 11:58 UTC (permalink / raw)
To: qemu-devel; +Cc: Amit Shah, quintela, mst, virtualization
In-Reply-To: <1268999926-29560-7-git-send-email-amit.shah@redhat.com>
Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
hw/virtio-console.c | 2 +-
hw/virtio-serial-bus.c | 2 +-
hw/virtio-serial.h | 2 +-
3 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/hw/virtio-console.c b/hw/virtio-console.c
index bd44ec6..e915491 100644
--- a/hw/virtio-console.c
+++ b/hw/virtio-console.c
@@ -1,7 +1,7 @@
/*
* Virtio Console and Generic Serial Port Devices
*
- * Copyright Red Hat, Inc. 2009
+ * Copyright Red Hat, Inc. 2009, 2010
*
* Authors:
* Amit Shah <amit.shah@redhat.com>
diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c
index 2603dcd..b682b77 100644
--- a/hw/virtio-serial-bus.c
+++ b/hw/virtio-serial-bus.c
@@ -1,7 +1,7 @@
/*
* A bus for connecting virtio serial and console ports
*
- * Copyright (C) 2009 Red Hat, Inc.
+ * Copyright (C) 2009, 2010 Red Hat, Inc.
*
* Author(s):
* Amit Shah <amit.shah@redhat.com>
diff --git a/hw/virtio-serial.h b/hw/virtio-serial.h
index f297b00..632d31b 100644
--- a/hw/virtio-serial.h
+++ b/hw/virtio-serial.h
@@ -2,7 +2,7 @@
* Virtio Serial / Console Support
*
* Copyright IBM, Corp. 2008
- * Copyright Red Hat, Inc. 2009
+ * Copyright Red Hat, Inc. 2009, 2010
*
* Authors:
* Christian Ehrhardt <ehrhardt@linux.vnet.ibm.com>
--
1.6.2.5
^ permalink raw reply related
* [PATCH 8/9] virtio-serial-bus: Use a bitmap in virtio config space for active ports
From: Amit Shah @ 2010-03-19 11:58 UTC (permalink / raw)
To: qemu-devel; +Cc: Amit Shah, quintela, mst, virtualization
In-Reply-To: <1268999926-29560-8-git-send-email-amit.shah@redhat.com>
Allow the port 'id's to be set by a user on the command line. This is
needed by management apps that will want a stable port numbering scheme
for hot-plug/unplug and migration.
Since the port numbers are shared with the guest (to identify ports in
control messages), the config space now maintains a bitmap for active
ports instead of a number indicating active ports.
This also means we can just update the config for port hot-plug and
hot-unplug.
Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
hw/virtio-console.c | 2 +
hw/virtio-serial-bus.c | 141 +++++++++++++++++++++++++++---------------------
hw/virtio-serial.h | 6 ++-
3 files changed, 86 insertions(+), 63 deletions(-)
diff --git a/hw/virtio-console.c b/hw/virtio-console.c
index e915491..bbbb6b8 100644
--- a/hw/virtio-console.c
+++ b/hw/virtio-console.c
@@ -99,6 +99,7 @@ static VirtIOSerialPortInfo virtconsole_info = {
.exit = virtconsole_exitfn,
.qdev.props = (Property[]) {
DEFINE_PROP_UINT8("is_console", VirtConsole, port.is_console, 1),
+ DEFINE_PROP_UINT32("nr", VirtConsole, port.id, VIRTIO_CONSOLE_BAD_ID),
DEFINE_PROP_CHR("chardev", VirtConsole, chr),
DEFINE_PROP_STRING("name", VirtConsole, port.name),
DEFINE_PROP_END_OF_LIST(),
@@ -133,6 +134,7 @@ static VirtIOSerialPortInfo virtserialport_info = {
.init = virtserialport_initfn,
.exit = virtconsole_exitfn,
.qdev.props = (Property[]) {
+ DEFINE_PROP_UINT32("nr", VirtConsole, port.id, VIRTIO_CONSOLE_BAD_ID),
DEFINE_PROP_CHR("chardev", VirtConsole, chr),
DEFINE_PROP_STRING("name", VirtConsole, port.name),
DEFINE_PROP_END_OF_LIST(),
diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c
index b682b77..2242edf 100644
--- a/hw/virtio-serial-bus.c
+++ b/hw/virtio-serial-bus.c
@@ -48,6 +48,10 @@ static VirtIOSerialPort *find_port_by_id(VirtIOSerial *vser, uint32_t id)
{
VirtIOSerialPort *port;
+ if (id == VIRTIO_CONSOLE_BAD_ID) {
+ return NULL;
+ }
+
QTAILQ_FOREACH(port, &vser->ports, next) {
if (port->id == id)
return port;
@@ -412,13 +416,13 @@ static void virtio_serial_save(QEMUFile *f, void *opaque)
/* The config space */
qemu_put_be16s(f, &s->config.cols);
qemu_put_be16s(f, &s->config.rows);
- qemu_put_be32s(f, &s->config.nr_ports);
- /* Items in struct VirtIOSerial */
+ qemu_put_be32s(f, &s->config.max_nr_ports);
+ qemu_put_buffer(f, (uint8_t *)s->config.ports_map,
+ sizeof(uint32_t) * (s->config.max_nr_ports + 31) / 32);
- qemu_put_be32s(f, &s->bus->max_nr_ports);
+ /* Ports */
- /* Do this because we might have hot-unplugged some ports */
nr_active_ports = 0;
QTAILQ_FOREACH(port, &s->ports, next)
nr_active_ports++;
@@ -429,11 +433,6 @@ static void virtio_serial_save(QEMUFile *f, void *opaque)
* Items in struct VirtIOSerialPort.
*/
QTAILQ_FOREACH(port, &s->ports, next) {
- /*
- * We put the port number because we may not have an active
- * port at id 0 that's reserved for a console port, or in case
- * of ports that might have gotten unplugged
- */
qemu_put_be32s(f, &port->id);
qemu_put_byte(f, port->guest_connected);
}
@@ -443,7 +442,8 @@ static int virtio_serial_load(QEMUFile *f, void *opaque, int version_id)
{
VirtIOSerial *s = opaque;
VirtIOSerialPort *port;
- uint32_t max_nr_ports, nr_active_ports, nr_ports;
+ size_t ports_map_size;
+ uint32_t max_nr_ports, nr_active_ports, *ports_map;
unsigned int i;
if (version_id > 2) {
@@ -460,29 +460,28 @@ static int virtio_serial_load(QEMUFile *f, void *opaque, int version_id)
/* The config space */
qemu_get_be16s(f, &s->config.cols);
qemu_get_be16s(f, &s->config.rows);
- nr_ports = qemu_get_be32(f);
- if (nr_ports != s->config.nr_ports) {
- /*
- * Source hot-plugged/unplugged ports and we don't have all of
- * them here.
- *
- * Note: This condition cannot check for all hotplug/unplug
- * events: eg, if one port was hot-plugged and one was
- * unplugged, the nr_ports remains the same but the port id's
- * would have changed and we won't catch it here. A later
- * check for !find_port_by_id() will confirm if this happened.
- */
+ qemu_get_be32s(f, &max_nr_ports);
+ if (max_nr_ports > s->config.max_nr_ports) {
+ /* Source could have had more ports than us. Fail migration. */
return -EINVAL;
}
- /* Items in struct VirtIOSerial */
+ ports_map_size = sizeof(uint32_t) * (max_nr_ports + 31) / 32;
+ ports_map = qemu_malloc(ports_map_size);
+ qemu_get_buffer(f, (uint8_t *)ports_map, ports_map_size);
- qemu_get_be32s(f, &max_nr_ports);
- if (max_nr_ports > s->bus->max_nr_ports) {
- /* Source could have more ports than us. Fail migration. */
- return -EINVAL;
+ for (i = 0; i < (max_nr_ports + 31) / 32; i++) {
+ if (ports_map[i] != s->config.ports_map[i]) {
+ /*
+ * Ports active on source and destination don't
+ * match. Fail migration.
+ */
+ qemu_free(ports_map);
+ return -EINVAL;
+ }
}
+ qemu_free(ports_map);
qemu_get_be32s(f, &nr_active_ports);
@@ -492,20 +491,11 @@ static int virtio_serial_load(QEMUFile *f, void *opaque, int version_id)
id = qemu_get_be32(f);
port = find_port_by_id(s, id);
- if (!port) {
- /*
- * The requested port was hot-plugged on the source but we
- * don't have it
- */
- return -EINVAL;
- }
port->guest_connected = qemu_get_byte(f);
}
-
return 0;
}
-
static void virtser_bus_dev_print(Monitor *mon, DeviceState *qdev, int indent);
static struct BusInfo virtser_bus_info = {
@@ -537,6 +527,39 @@ static void virtser_bus_dev_print(Monitor *mon, DeviceState *qdev, int indent)
indent, "", port->host_connected);
}
+/* This function is only used if a port id is not provided by the user */
+static uint32_t find_free_port_id(VirtIOSerial *vser)
+{
+ unsigned int i;
+
+ for (i = 0; i < (vser->config.max_nr_ports + 31) / 32; i++) {
+ uint32_t map, bit;
+
+ map = vser->config.ports_map[i];
+ bit = ffs(~map);
+ if (bit) {
+ return (bit - 1) + i * 32;
+ }
+ }
+ return VIRTIO_CONSOLE_BAD_ID;
+}
+
+static void mark_port_added(VirtIOSerial *vser, uint32_t port_id)
+{
+ unsigned int i;
+
+ i = port_id / 32;
+ vser->config.ports_map[i] |= 1U << (port_id % 32);
+}
+
+static void mark_port_removed(VirtIOSerial *vser, uint32_t port_id)
+{
+ unsigned int i;
+
+ i = port_id / 32;
+ vser->config.ports_map[i] &= ~(1U << (port_id % 32));
+}
+
static int virtser_port_qdev_init(DeviceState *qdev, DeviceInfo *base)
{
VirtIOSerialDevice *dev = DO_UPCAST(VirtIOSerialDevice, qdev, qdev);
@@ -555,19 +578,30 @@ static int virtser_port_qdev_init(DeviceState *qdev, DeviceInfo *base)
*/
plugging_port0 = port->is_console && !find_port_by_id(port->vser, 0);
- if (port->vser->config.nr_ports == bus->max_nr_ports && !plugging_port0) {
- error_report("virtio-serial-bus: Maximum device limit reached");
+ if (find_port_by_id(port->vser, port->id)) {
+ error_report("virtio-serial-bus: A port already exists at id %u\n",
+ port->id);
return -1;
}
- dev->info = info;
+ if (plugging_port0) {
+ port->id = 0;
+ }
+
+ if (port->id == VIRTIO_CONSOLE_BAD_ID) {
+ port->id = find_free_port_id(port->vser);
+ if (port->id == VIRTIO_CONSOLE_BAD_ID) {
+ error_report("virtio-serial-bus: Maximum device limit reached\n");
+ return -1;
+ }
+ }
+
+ dev->info = info;
ret = info->init(dev);
if (ret) {
return ret;
}
- port->id = plugging_port0 ? 0 : port->vser->config.nr_ports++;
-
if (!use_multiport(port->vser)) {
/*
* Allow writes to guest in this case; we have no way of
@@ -580,6 +614,8 @@ static int virtser_port_qdev_init(DeviceState *qdev, DeviceInfo *base)
port->ivq = port->vser->ivqs[port->id];
port->ovq = port->vser->ovqs[port->id];
+ mark_port_added(port->vser, port->id);
+
/* Send an update to the guest about this new port added */
virtio_notify_config(&port->vser->vdev);
@@ -592,26 +628,9 @@ static int virtser_port_qdev_exit(DeviceState *qdev)
VirtIOSerialPort *port = DO_UPCAST(VirtIOSerialPort, dev, &dev->qdev);
VirtIOSerial *vser = port->vser;
- send_control_event(port, VIRTIO_CONSOLE_PORT_REMOVE, 1);
+ mark_port_removed(port->vser, port->id);
+ virtio_notify_config(&port->vser->vdev);
- /*
- * Don't decrement nr_ports here; thus we keep a linearly
- * increasing port id. Not utilising an id again saves us a couple
- * of complications:
- *
- * - Not having to bother about sending the port id to the guest
- * kernel on hotplug or on addition of new ports; the guest can
- * also linearly increment the port number. This is preferable
- * because the config space won't have the need to store a
- * ports_map.
- *
- * - Extra state to be stored for all the "holes" that got created
- * so that we keep filling in the ids from the least available
- * index.
- *
- * When such a functionality is desired, a control message to add
- * a port can be introduced.
- */
QTAILQ_REMOVE(&vser->ports, port, next);
if (port->info->exit)
@@ -675,7 +694,7 @@ VirtIODevice *virtio_serial_init(DeviceState *dev, uint32_t max_nr_ports)
* Reserve location 0 for a console port for backward compat
* (old kernel, new qemu)
*/
- vser->config.nr_ports = 1;
+ mark_port_added(vser, 0);
vser->vdev.get_features = get_features;
vser->vdev.get_config = get_config;
diff --git a/hw/virtio-serial.h b/hw/virtio-serial.h
index 632d31b..5f1d458 100644
--- a/hw/virtio-serial.h
+++ b/hw/virtio-serial.h
@@ -27,6 +27,9 @@
/* Features supported */
#define VIRTIO_CONSOLE_F_MULTIPORT 1
+#define VIRTIO_CONSOLE_BAD_ID (~(uint32_t)0)
+#define MAX_VIRTIO_CONSOLE_PORTS ((VIRTIO_PCI_QUEUE_MAX / 2) - 1)
+
struct virtio_console_config {
/*
* These two fields are used by VIRTIO_CONSOLE_F_SIZE which
@@ -36,7 +39,7 @@ struct virtio_console_config {
uint16_t rows;
uint32_t max_nr_ports;
- uint32_t nr_ports;
+ uint32_t ports_map[(MAX_VIRTIO_CONSOLE_PORTS + 31) / 32];
} __attribute__((packed));
struct virtio_console_control {
@@ -51,7 +54,6 @@ struct virtio_console_control {
#define VIRTIO_CONSOLE_RESIZE 2
#define VIRTIO_CONSOLE_PORT_OPEN 3
#define VIRTIO_CONSOLE_PORT_NAME 4
-#define VIRTIO_CONSOLE_PORT_REMOVE 5
/* == In-qemu interface == */
--
1.6.2.5
^ permalink raw reply related
* [PATCH 9/9] virtio-serial-bus: Let the guest know of host connection changes after migration
From: Amit Shah @ 2010-03-19 11:58 UTC (permalink / raw)
To: qemu-devel; +Cc: Amit Shah, quintela, mst, virtualization
In-Reply-To: <1268999926-29560-9-git-send-email-amit.shah@redhat.com>
If the host connection to a port is closed on the destination machine
after migration, when the connection was open on the source, the host
has to be informed of that.
Similar for a host connection open on the destination.
Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
hw/virtio-serial-bus.c | 11 +++++++++++
1 files changed, 11 insertions(+), 0 deletions(-)
diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c
index 2242edf..f5ea37e 100644
--- a/hw/virtio-serial-bus.c
+++ b/hw/virtio-serial-bus.c
@@ -435,6 +435,7 @@ static void virtio_serial_save(QEMUFile *f, void *opaque)
QTAILQ_FOREACH(port, &s->ports, next) {
qemu_put_be32s(f, &port->id);
qemu_put_byte(f, port->guest_connected);
+ qemu_put_byte(f, port->host_connected);
}
}
@@ -488,11 +489,21 @@ static int virtio_serial_load(QEMUFile *f, void *opaque, int version_id)
/* Items in struct VirtIOSerialPort */
for (i = 0; i < nr_active_ports; i++) {
uint32_t id;
+ bool host_connected;
id = qemu_get_be32(f);
port = find_port_by_id(s, id);
port->guest_connected = qemu_get_byte(f);
+ host_connected = qemu_get_byte(f);
+ if (host_connected != port->host_connected) {
+ /*
+ * We have to let the guest know of the host connection
+ * status change
+ */
+ send_control_event(port, VIRTIO_CONSOLE_PORT_OPEN,
+ port->host_connected);
+ }
}
return 0;
}
--
1.6.2.5
^ permalink raw reply related
* [PATCH 0/6] virtio: console: Fixes, abi update
From: Amit Shah @ 2010-03-19 12:06 UTC (permalink / raw)
To: virtualization; +Cc: Amit Shah, quintela, mst
Hello,
These patches fix a couple of small issues:
- generate a kobject change event so that udev is woken up on name
changes
- fix a crash after hot-unplug of the first console port and a
subsequent config update
But majorly, it reworks how ports are discovered: instead of numbering
the ports individually in the host and the guest by just incrementing
a number, we now switch to a bitmap in the config space exposed by the
host to identify active ports. This lets us maintain the same
numbering used by the host and also allows for hot-unplug via the
config space. This is needed for proper migration support after
several hot-plug/unplug operations.
I've tested these patches on my testsuite to catch any regression or
correctness issues. I've also tested all the hotplug-related changes
here.
These should go to 2.6.34, so that we don't push out a stable release
with the older interface. Michael, please forward these to Linus if
everyone is OK with these. I also have a git repo at
git://git.kernel.org/pub/scm/linux/kernel/git/amit/vs-kernel.git master
if you prefer to pull the patches.
Amit Shah (6):
virtio: console: Generate a kobject CHANGE event on adding 'name'
attribute
virtio: console: Check if port is valid in resize_console
virtio: console: Switch to using a port bitmap for port discovery
virtio: console: Separate out get_config in a separate function
virtio: console: Handle hot-plug/unplug config actions
virtio: console: Remove hot-unplug control message
drivers/char/virtio_console.c | 238 ++++++++++++++++++++++++----------------
include/linux/virtio_console.h | 15 ++-
2 files changed, 156 insertions(+), 97 deletions(-)
^ permalink raw reply
* [PATCH 1/6] virtio: console: Generate a kobject CHANGE event on adding 'name' attribute
From: Amit Shah @ 2010-03-19 12:06 UTC (permalink / raw)
To: virtualization; +Cc: Amit Shah, quintela, mst
In-Reply-To: <1269000408-29962-1-git-send-email-amit.shah@redhat.com>
When the host lets us know what 'name' a port is assigned, we create the
sysfs 'name' attribute. Generate a 'change' event after this so that
udev wakes up and acts on the rules for virtio-ports (currently there's
only one rule that creates a symlink from the 'name' to the actual char
device).
Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
drivers/char/virtio_console.c | 11 +++++++++--
1 files changed, 9 insertions(+), 2 deletions(-)
diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index f404ccf..67b474b 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -947,11 +947,18 @@ static void handle_control_message(struct ports_device *portdev,
*/
err = sysfs_create_group(&port->dev->kobj,
&port_attribute_group);
- if (err)
+ if (err) {
dev_err(port->dev,
"Error %d creating sysfs device attributes\n",
err);
-
+ } else {
+ /*
+ * Generate a udev event so that appropriate
+ * symlinks can be created based on udev
+ * rules.
+ */
+ kobject_uevent(&port->dev->kobj, KOBJ_CHANGE);
+ }
break;
case VIRTIO_CONSOLE_PORT_REMOVE:
/*
--
1.6.2.5
^ permalink raw reply related
* [PATCH 2/6] virtio: console: Check if port is valid in resize_console
From: Amit Shah @ 2010-03-19 12:06 UTC (permalink / raw)
To: virtualization; +Cc: Amit Shah, quintela, mst
In-Reply-To: <1269000408-29962-2-git-send-email-amit.shah@redhat.com>
The console port could have been hot-unplugged. Check if it is valid
before working on it.
Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
drivers/char/virtio_console.c | 4 ++++
1 files changed, 4 insertions(+), 0 deletions(-)
diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 67b474b..44288ce 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -681,6 +681,10 @@ static void resize_console(struct port *port)
struct virtio_device *vdev;
struct winsize ws;
+ /* The port could have been hot-unplugged */
+ if (!port)
+ return;
+
vdev = port->portdev->vdev;
if (virtio_has_feature(vdev, VIRTIO_CONSOLE_F_SIZE)) {
vdev->config->get(vdev,
--
1.6.2.5
^ permalink raw reply related
* [PATCH 3/6] virtio: console: Switch to using a port bitmap for port discovery
From: Amit Shah @ 2010-03-19 12:06 UTC (permalink / raw)
To: virtualization; +Cc: Amit Shah, quintela, mst
In-Reply-To: <1269000408-29962-3-git-send-email-amit.shah@redhat.com>
Ports are now discovered by their location as given by host instead of
just incrementing a number and expecting the host and guest numbers stay
in sync. They are needed to be the same because the control messages
identify ports using the port id.
This is most beneficial to management software to properly place ports
at known ids so that the ids after hot-plug/unplug operations can be
controlled. This helps migration of guests after such hot-plug/unplug
operations.
The support for port hot-plug is removed in this commit, will be added
in the following commits.
The config space is now a variable-sized array.
Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
drivers/char/virtio_console.c | 139 ++++++++++++++++++++++------------------
include/linux/virtio_console.h | 14 +++-
2 files changed, 87 insertions(+), 66 deletions(-)
diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 44288ce..dc15c92 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -120,7 +120,7 @@ struct ports_device {
spinlock_t cvq_lock;
/* The current config space is stored here */
- struct virtio_console_config config;
+ struct virtio_console_config *config;
/* The virtio device we're associated with */
struct virtio_device *vdev;
@@ -1210,6 +1210,23 @@ fail:
return err;
}
+static u32 find_next_bit_in_map(u32 *map)
+{
+ u32 port_bit;
+
+ port_bit = ffs(*map);
+ port_bit--; /* ffs returns bit location */
+
+ *map &= ~(1U << port_bit);
+
+ return port_bit;
+}
+
+static u32 get_ports_map_size(u32 max_nr_ports)
+{
+ return sizeof(u32) * ((max_nr_ports + 31)/ 32);
+}
+
/*
* The workhandler for config-space updates.
*
@@ -1225,45 +1242,8 @@ static void config_work_handler(struct work_struct *work)
portdev = container_of(work, struct ports_device, config_work);
vdev = portdev->vdev;
- vdev->config->get(vdev,
- offsetof(struct virtio_console_config, nr_ports),
- &virtconconf.nr_ports,
- sizeof(virtconconf.nr_ports));
- if (portdev->config.nr_ports == virtconconf.nr_ports) {
- /*
- * Port 0 got hot-added. Since we already did all the
- * other initialisation for it, just tell the Host
- * that the port is ready if we find the port. In
- * case the port was hot-removed earlier, we call
- * add_port to add the port.
- */
- struct port *port;
-
- port = find_port_by_id(portdev, 0);
- if (!port)
- add_port(portdev, 0);
- else
- send_control_msg(port, VIRTIO_CONSOLE_PORT_READY, 1);
- return;
- }
- if (virtconconf.nr_ports > portdev->config.max_nr_ports) {
- dev_warn(&vdev->dev,
- "More ports specified (%u) than allowed (%u)",
- portdev->config.nr_ports + 1,
- portdev->config.max_nr_ports);
- return;
- }
- if (virtconconf.nr_ports < portdev->config.nr_ports)
- return;
-
- /* Hot-add ports */
- while (virtconconf.nr_ports - portdev->config.nr_ports) {
- err = add_port(portdev, portdev->config.nr_ports);
- if (err)
- break;
- portdev->config.nr_ports++;
- }
+ /* FIXME: Handle port hotplug/unplug */
}
static int init_vqs(struct ports_device *portdev)
@@ -1274,7 +1254,7 @@ static int init_vqs(struct ports_device *portdev)
u32 i, j, nr_ports, nr_queues;
int err;
- nr_ports = portdev->config.max_nr_ports;
+ nr_ports = portdev->config->max_nr_ports;
nr_queues = use_multiport(portdev) ? (nr_ports + 1) * 2 : 2;
vqs = kmalloc(nr_queues * sizeof(struct virtqueue *), GFP_KERNEL);
@@ -1387,7 +1367,6 @@ static const struct file_operations portdev_fops = {
static int __devinit virtcons_probe(struct virtio_device *vdev)
{
struct ports_device *portdev;
- u32 i;
int err;
bool multiport;
@@ -1415,30 +1394,45 @@ static int __devinit virtcons_probe(struct virtio_device *vdev)
goto free;
}
- multiport = false;
- portdev->config.nr_ports = 1;
- portdev->config.max_nr_ports = 1;
if (virtio_has_feature(vdev, VIRTIO_CONSOLE_F_MULTIPORT)) {
+ u32 max_nr_ports;
+
multiport = true;
vdev->features[0] |= 1 << VIRTIO_CONSOLE_F_MULTIPORT;
vdev->config->get(vdev, offsetof(struct virtio_console_config,
- nr_ports),
- &portdev->config.nr_ports,
- sizeof(portdev->config.nr_ports));
- vdev->config->get(vdev, offsetof(struct virtio_console_config,
max_nr_ports),
- &portdev->config.max_nr_ports,
- sizeof(portdev->config.max_nr_ports));
- if (portdev->config.nr_ports > portdev->config.max_nr_ports) {
- dev_warn(&vdev->dev,
- "More ports (%u) specified than allowed (%u). Will init %u ports.",
- portdev->config.nr_ports,
- portdev->config.max_nr_ports,
- portdev->config.max_nr_ports);
-
- portdev->config.nr_ports = portdev->config.max_nr_ports;
+ &max_nr_ports, sizeof(max_nr_ports));
+ /*
+ * We have a variable-sized config space that's
+ * dependent on the maximum number of ports a guest
+ * can have. So we first get the max number of ports
+ * we can have and then allocate the config space
+ */
+ portdev->config = kmalloc(sizeof(struct virtio_console_config)
+ + get_ports_map_size(max_nr_ports),
+ GFP_KERNEL);
+ if (!portdev->config) {
+ err = -ENOMEM;
+ goto free_chrdev;
+ }
+
+ portdev->config->max_nr_ports = max_nr_ports;
+ vdev->config->get(vdev, offsetof(struct virtio_console_config,
+ ports_map),
+ portdev->config->ports_map,
+ get_ports_map_size(max_nr_ports));
+ } else {
+ multiport = false;
+
+ portdev->config = kmalloc(sizeof(struct virtio_console_config),
+ GFP_KERNEL);
+ if (!portdev->config) {
+ err = -ENOMEM;
+ goto free_chrdev;
}
+
+ portdev->config->max_nr_ports = 1;
}
/* Let the Host know we support multiple ports.*/
@@ -1447,14 +1441,14 @@ static int __devinit virtcons_probe(struct virtio_device *vdev)
err = init_vqs(portdev);
if (err < 0) {
dev_err(&vdev->dev, "Error %d initializing vqs\n", err);
- goto free_chrdev;
+ goto free_config;
}
spin_lock_init(&portdev->ports_lock);
INIT_LIST_HEAD(&portdev->ports);
if (multiport) {
- unsigned int nr_added_bufs;
+ unsigned int nr_added_bufs, i;
spin_lock_init(&portdev->cvq_lock);
INIT_WORK(&portdev->control_work, &control_work_handler);
@@ -1467,10 +1461,26 @@ static int __devinit virtcons_probe(struct virtio_device *vdev)
err = -ENOMEM;
goto free_vqs;
}
- }
- for (i = 0; i < portdev->config.nr_ports; i++)
- add_port(portdev, i);
+ for (i = 0; i < (portdev->config->max_nr_ports + 31) / 32; i++) {
+ u32 map, port_nr;
+
+ map = portdev->config->ports_map[i];
+ while (map) {
+ port_nr = find_next_bit_in_map(&map) + i * 32;
+
+ add_port(portdev, port_nr);
+ /*
+ * FIXME: Send a notification to the
+ * host about failed port add
+ */
+ }
+ }
+ } else {
+ err = add_port(portdev, 0);
+ if (err)
+ goto free_vqs;
+ }
/* Start using the new console output. */
early_put_chars = NULL;
@@ -1480,6 +1490,8 @@ free_vqs:
vdev->config->del_vqs(vdev);
kfree(portdev->in_vqs);
kfree(portdev->out_vqs);
+free_config:
+ kfree(portdev->config);
free_chrdev:
unregister_chrdev(portdev->chr_major, "virtio-portsdev");
free:
@@ -1514,6 +1526,7 @@ static void virtcons_remove(struct virtio_device *vdev)
vdev->config->del_vqs(vdev);
kfree(portdev->in_vqs);
kfree(portdev->out_vqs);
+ kfree(portdev->config);
kfree(portdev);
}
diff --git a/include/linux/virtio_console.h b/include/linux/virtio_console.h
index ae4f039..287ee44 100644
--- a/include/linux/virtio_console.h
+++ b/include/linux/virtio_console.h
@@ -14,15 +14,23 @@
#define VIRTIO_CONSOLE_F_SIZE 0 /* Does host provide console size? */
#define VIRTIO_CONSOLE_F_MULTIPORT 1 /* Does host provide multiple ports? */
+/*
+ * This is the config space shared between the Host and the Guest.
+ * The Host indicates to us the maximum number of ports this device
+ * can hold and a port map indicating which ports are active.
+ *
+ * The maximum number of ports is not a round number to prevent
+ * wastage of MSI vectors in case MSI is enabled for this device.
+ */
struct virtio_console_config {
/* colums of the screens */
__u16 cols;
/* rows of the screens */
__u16 rows;
- /* max. number of ports this device can hold */
+ /* max. number of ports this device can hold. */
__u32 max_nr_ports;
- /* number of ports added so far */
- __u32 nr_ports;
+ /* locations of ports in use; variable-sized array */
+ __u32 ports_map[0 /* (max_nr_ports + 31) / 2 */];
} __attribute__((packed));
/*
--
1.6.2.5
^ permalink raw reply related
* [PATCH 4/6] virtio: console: Separate out get_config in a separate function
From: Amit Shah @ 2010-03-19 12:06 UTC (permalink / raw)
To: virtualization; +Cc: Amit Shah, quintela, mst
In-Reply-To: <1269000408-29962-4-git-send-email-amit.shah@redhat.com>
Getting config information is done in probe() as well as
config_work_handler().
Separate it out so that we only code it once.
Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
drivers/char/virtio_console.c | 85 +++++++++++++++++++++++------------------
1 files changed, 48 insertions(+), 37 deletions(-)
diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index dc15c92..9f2cbf0 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -1227,6 +1227,46 @@ static u32 get_ports_map_size(u32 max_nr_ports)
return sizeof(u32) * ((max_nr_ports + 31)/ 32);
}
+static struct virtio_console_config *get_config(struct virtio_device *vdev,
+ bool multiport)
+{
+ struct virtio_console_config *config;
+ u32 max_nr_ports;
+
+ if (!multiport) {
+ config = kmalloc(sizeof(struct virtio_console_config),
+ GFP_KERNEL);
+ if (!config)
+ return NULL;
+
+ config->max_nr_ports = 1;
+ return config;
+ }
+
+ vdev->config->get(vdev, offsetof(struct virtio_console_config,
+ max_nr_ports),
+ &max_nr_ports, sizeof(max_nr_ports));
+
+ /*
+ * We have a variable-sized config space that's dependent on
+ * the maximum number of ports a guest can have. So we first
+ * get the max number of ports we can have and then allocate
+ * the config space
+ */
+ config = kmalloc(sizeof(struct virtio_console_config)
+ + get_ports_map_size(max_nr_ports),
+ GFP_KERNEL);
+ if (!config)
+ return NULL;
+
+ config->max_nr_ports = max_nr_ports;
+ vdev->config->get(vdev, offsetof(struct virtio_console_config,
+ ports_map),
+ config->ports_map,
+ get_ports_map_size(max_nr_ports));
+ return config;
+}
+
/*
* The workhandler for config-space updates.
*
@@ -1394,49 +1434,20 @@ static int __devinit virtcons_probe(struct virtio_device *vdev)
goto free;
}
+ multiport = false;
if (virtio_has_feature(vdev, VIRTIO_CONSOLE_F_MULTIPORT)) {
- u32 max_nr_ports;
-
multiport = true;
vdev->features[0] |= 1 << VIRTIO_CONSOLE_F_MULTIPORT;
- vdev->config->get(vdev, offsetof(struct virtio_console_config,
- max_nr_ports),
- &max_nr_ports, sizeof(max_nr_ports));
- /*
- * We have a variable-sized config space that's
- * dependent on the maximum number of ports a guest
- * can have. So we first get the max number of ports
- * we can have and then allocate the config space
- */
- portdev->config = kmalloc(sizeof(struct virtio_console_config)
- + get_ports_map_size(max_nr_ports),
- GFP_KERNEL);
- if (!portdev->config) {
- err = -ENOMEM;
- goto free_chrdev;
- }
-
- portdev->config->max_nr_ports = max_nr_ports;
- vdev->config->get(vdev, offsetof(struct virtio_console_config,
- ports_map),
- portdev->config->ports_map,
- get_ports_map_size(max_nr_ports));
- } else {
- multiport = false;
-
- portdev->config = kmalloc(sizeof(struct virtio_console_config),
- GFP_KERNEL);
- if (!portdev->config) {
- err = -ENOMEM;
- goto free_chrdev;
- }
-
- portdev->config->max_nr_ports = 1;
+ /* Let the Host know we support multiple ports.*/
+ vdev->config->finalize_features(vdev);
}
- /* Let the Host know we support multiple ports.*/
- vdev->config->finalize_features(vdev);
+ portdev->config = get_config(vdev, multiport);
+ if (!portdev->config) {
+ err = -ENOMEM;
+ goto free_chrdev;
+ }
err = init_vqs(portdev);
if (err < 0) {
--
1.6.2.5
^ permalink raw reply related
* [PATCH 5/6] virtio: console: Handle hot-plug/unplug config actions
From: Amit Shah @ 2010-03-19 12:06 UTC (permalink / raw)
To: virtualization; +Cc: Amit Shah, quintela, mst
In-Reply-To: <1269000408-29962-5-git-send-email-amit.shah@redhat.com>
The new way of handling hot-plug as well as unplug is via a
config interrupt.
Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
drivers/char/virtio_console.c | 53 +++++++++++++++++++++++++++++++++++++----
1 files changed, 48 insertions(+), 5 deletions(-)
diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 9f2cbf0..06ca95c 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -1268,22 +1268,65 @@ static struct virtio_console_config *get_config(struct virtio_device *vdev,
}
/*
- * The workhandler for config-space updates.
+ * The workhandler for config-space updates. Only called when
+ * multiport is supported.
*
- * This is called when ports are hot-added.
+ * This is called when ports are hot-added and hot-removed.
*/
static void config_work_handler(struct work_struct *work)
{
- struct virtio_console_config virtconconf;
+ struct virtio_console_config *config;
struct ports_device *portdev;
struct virtio_device *vdev;
- int err;
+ unsigned int i, changed;
portdev = container_of(work, struct ports_device, config_work);
vdev = portdev->vdev;
- /* FIXME: Handle port hotplug/unplug */
+ config = get_config(vdev, true);
+ if (!config) {
+ dev_warn(&vdev->dev,
+ "Running out of memory on config change events\n");
+ return;
+ }
+
+ if (config->max_nr_ports != portdev->config->max_nr_ports) {
+ dev_warn(&vdev->dev,
+ "Host updated max_nr_ports, can't handle that now.\n");
+ goto free;
+ }
+
+ changed = 0;
+ for (i = 0; i < (config->max_nr_ports + 31) / 32; i++) {
+ u32 map;
+
+ map = config->ports_map[i] ^ portdev->config->ports_map[i];
+ while (map) {
+ struct port *port;
+ u32 port_nr;
+
+ changed++;
+ port_nr = find_next_bit_in_map(&map) + i * 32;
+ port = find_port_by_id(portdev, port_nr);
+ if (port)
+ remove_port(port);
+ else
+ add_port(portdev, port_nr);
+ }
+ portdev->config->ports_map[i] = config->ports_map[i];
+ }
+ if (!changed && find_port_by_id(portdev, 0)) {
+ /*
+ * No hot-plug / unplug activity. Port 0 might have
+ * been hot-added. Just send a 'ready' message in
+ * that case, since we already have it added.
+ */
+ send_control_msg(find_port_by_id(portdev, 0),
+ VIRTIO_CONSOLE_PORT_READY, 1);
+ }
+free:
+ kfree(config);
}
static int init_vqs(struct ports_device *portdev)
--
1.6.2.5
^ permalink raw reply related
* [PATCH 6/6] virtio: console: Remove hot-unplug control message
From: Amit Shah @ 2010-03-19 12:06 UTC (permalink / raw)
To: virtualization; +Cc: Amit Shah, quintela, mst
In-Reply-To: <1269000408-29962-6-git-send-email-amit.shah@redhat.com>
Now that we handle port hot-unplug via config updates, remove the
control message that was created.
Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
drivers/char/virtio_console.c | 26 --------------------------
include/linux/virtio_console.h | 1 -
2 files changed, 0 insertions(+), 27 deletions(-)
diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 06ca95c..a810099 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -964,32 +964,6 @@ static void handle_control_message(struct ports_device *portdev,
kobject_uevent(&port->dev->kobj, KOBJ_CHANGE);
}
break;
- case VIRTIO_CONSOLE_PORT_REMOVE:
- /*
- * Hot unplug the port. We don't decrement nr_ports
- * since we don't want to deal with extra complexities
- * of using the lowest-available port id: We can just
- * pick up the nr_ports number as the id and not have
- * userspace send it to us. This helps us in two
- * ways:
- *
- * - We don't need to have a 'port_id' field in the
- * config space when a port is hot-added. This is a
- * good thing as we might queue up multiple hotplug
- * requests issued in our workqueue.
- *
- * - Another way to deal with this would have been to
- * use a bitmap of the active ports and select the
- * lowest non-active port from that map. That
- * bloats the already tight config space and we
- * would end up artificially limiting the
- * max. number of ports to sizeof(bitmap). Right
- * now we can support 2^32 ports (as the port id is
- * stored in a u32 type).
- *
- */
- remove_port(port);
- break;
}
}
diff --git a/include/linux/virtio_console.h b/include/linux/virtio_console.h
index 287ee44..eaaf04b 100644
--- a/include/linux/virtio_console.h
+++ b/include/linux/virtio_console.h
@@ -49,7 +49,6 @@ struct virtio_console_control {
#define VIRTIO_CONSOLE_RESIZE 2
#define VIRTIO_CONSOLE_PORT_OPEN 3
#define VIRTIO_CONSOLE_PORT_NAME 4
-#define VIRTIO_CONSOLE_PORT_REMOVE 5
#ifdef __KERNEL__
int __init virtio_cons_early_init(int (*put_chars)(u32, const char *, int));
--
1.6.2.5
^ permalink raw reply related
* Re: [PATCH 4/9] virtio-serial: Handle scatter-gather buffers for control messages
From: Avi Kivity @ 2010-03-20 7:40 UTC (permalink / raw)
To: Amit Shah; +Cc: mst, quintela, qemu-devel, virtualization
In-Reply-To: <1268999926-29560-5-git-send-email-amit.shah@redhat.com>
On 03/19/2010 01:58 PM, Amit Shah wrote:
> Current control messages are small enough to not be split into multiple
> buffers but we could run into such a situation in the future or a
> malicious guest could cause such a situation.
>
> So handle the entire iov request for control messages.
>
> Also ensure the size of the control request is>= what we expect
> otherwise we risk accessing memory that we don't own.
>
> Signed-off-by: Amit Shah<amit.shah@redhat.com>
> CC: Avi Kivity<avi@redhat.com>
> Reported-by: Avi Kivity<avi@redhat.com>
> ---
> hw/virtio-serial-bus.c | 43 ++++++++++++++++++++++++++++++++++++++++---
> 1 files changed, 40 insertions(+), 3 deletions(-)
>
> diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c
> index 830eb75..d5887ab 100644
> --- a/hw/virtio-serial-bus.c
> +++ b/hw/virtio-serial-bus.c
> @@ -200,7 +200,7 @@ size_t virtio_serial_guest_ready(VirtIOSerialPort *port)
> }
>
> /* Guest wants to notify us of some event */
> -static void handle_control_message(VirtIOSerial *vser, void *buf)
> +static void handle_control_message(VirtIOSerial *vser, void *buf, size_t len)
> {
> struct VirtIOSerialPort *port;
> struct virtio_console_control cpkt, *gcpkt;
> @@ -208,6 +208,10 @@ static void handle_control_message(VirtIOSerial *vser, void *buf)
> size_t buffer_len;
>
> gcpkt = buf;
> + if (len< sizeof(cpkt)) {
> + /* The guest sent an invalid control packet */
> + return;
> + }
> port = find_port_by_id(vser, ldl_p(&gcpkt->id));
> if (!port)
> return;
> @@ -281,12 +285,45 @@ static void control_out(VirtIODevice *vdev, VirtQueue *vq)
> {
> VirtQueueElement elem;
> VirtIOSerial *vser;
> + uint8_t *buf;
> + size_t len;
>
> vser = DO_UPCAST(VirtIOSerial, vdev, vdev);
>
> + len = 0;
> + buf = NULL;
> while (virtqueue_pop(vq,&elem)) {
> - handle_control_message(vser, elem.out_sg[0].iov_base);
> - virtqueue_push(vq,&elem, elem.out_sg[0].iov_len);
> + unsigned int i;
> + size_t cur_len, offset;
> +
> + cur_len = 0;
> + for (i = 0; i< elem.out_num; i++) {
> + cur_len += elem.out_sg[i].iov_len;
> + }
> + /*
> + * Allocate a new buf only if we didn't have one previously or
> + * if the size of the buf differs
> + */
> + if (cur_len != len) {
> + if (len) {
> + qemu_free(buf);
> + }
> + buf = qemu_malloc(cur_len);
> + }
> +
> + offset = 0;
> + for (i = 0; i< elem.out_num; i++) {
> + memcpy(buf + offset, elem.out_sg[i].iov_base,
> + elem.out_sg[i].iov_len);
> + offset += elem.out_sg[i].iov_len;
> + }
> + len = cur_len;
> +
> + handle_control_message(vser, buf, len);
> + virtqueue_push(vq,&elem, len);
> + }
> + if (len) {
> + qemu_free(buf);
> }
> virtio_notify(vdev, vq);
> }
>
Isn't there some virtio function to linearize requests?
--
Do not meddle in the internals of kernels, for they are subtle and quick to panic.
^ permalink raw reply
* Re: [GIT PULL] vhost-net fixes for issues in 2.6.34-rc1
From: David Miller @ 2010-03-20 21:41 UTC (permalink / raw)
To: mst
Cc: rusty, kvm, virtualization, netdev, linux-kernel, samudrala,
jirislaby, chavey, quintela, unai.uribarri, jdike
In-Reply-To: <20100318095355.GA24194@redhat.com>
From: "Michael S. Tsirkin" <mst@redhat.com>
Date: Thu, 18 Mar 2010 11:53:55 +0200
> The following tree includes patches fixing issues with vhost-net in
> 2.6.34-rc1. Please pull them for 2.6.34.
Pulled, thanks a lot.
^ permalink raw reply
* Re: [PATCH 3/6] virtio: console: Switch to using a port bitmap for port discovery
From: Michael S. Tsirkin @ 2010-03-21 11:29 UTC (permalink / raw)
To: Amit Shah; +Cc: quintela, virtualization
In-Reply-To: <1269000408-29962-4-git-send-email-amit.shah@redhat.com>
On Fri, Mar 19, 2010 at 05:36:45PM +0530, Amit Shah wrote:
> Ports are now discovered by their location as given by host instead of
> just incrementing a number and expecting the host and guest numbers stay
> in sync. They are needed to be the same because the control messages
> identify ports using the port id.
>
> This is most beneficial to management software to properly place ports
> at known ids so that the ids after hot-plug/unplug operations can be
> controlled. This helps migration of guests after such hot-plug/unplug
> operations.
>
> The support for port hot-plug is removed in this commit, will be added
> in the following commits.
It might be cleaner not to split it this way, merge the following
commits into this one or split it in a different way.
> The config space is now a variable-sized array.
I think this last bit is problematic: we won't be able to add any more data if
we have to, without a lot of complexity. Further, in the past we have
also had problems running out of config space: see
3225beaba05d4f06087593f5e903ce867b6e118a. There's also
a comment in handle_control_message which suggests we
might want a very large number of ports at some point,
if we do using config space would be a mistake.
It might be better to use a control vq for this, like virtio block ended
up doing. The comment in handle_control_message hints we don't want to
pass port id in config space, but I am not sure why we can't pass it in
message buffer.
> Signed-off-by: Amit Shah <amit.shah@redhat.com>
> ---
> drivers/char/virtio_console.c | 139 ++++++++++++++++++++++------------------
> include/linux/virtio_console.h | 14 +++-
> 2 files changed, 87 insertions(+), 66 deletions(-)
>
> diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
> index 44288ce..dc15c92 100644
> --- a/drivers/char/virtio_console.c
> +++ b/drivers/char/virtio_console.c
> @@ -120,7 +120,7 @@ struct ports_device {
> spinlock_t cvq_lock;
>
> /* The current config space is stored here */
> - struct virtio_console_config config;
> + struct virtio_console_config *config;
>
> /* The virtio device we're associated with */
> struct virtio_device *vdev;
> @@ -1210,6 +1210,23 @@ fail:
> return err;
> }
>
> +static u32 find_next_bit_in_map(u32 *map)
> +{
> + u32 port_bit;
> +
> + port_bit = ffs(*map);
> + port_bit--; /* ffs returns bit location */
> +
> + *map &= ~(1U << port_bit);
The above only works well if map is non-zero. This happens to be the
case the way we call it, but since this means the function is not
generic, it might be better to opencode it to make it obvious.
> +
> + return port_bit;
> +}
> +
> +static u32 get_ports_map_size(u32 max_nr_ports)
> +{
> + return sizeof(u32) * ((max_nr_ports + 31)/ 32);
DIV_ROUND_UP here and elsewhere?
> +}
> +
> /*
> * The workhandler for config-space updates.
> *
> @@ -1225,45 +1242,8 @@ static void config_work_handler(struct work_struct *work)
> portdev = container_of(work, struct ports_device, config_work);
>
> vdev = portdev->vdev;
> - vdev->config->get(vdev,
> - offsetof(struct virtio_console_config, nr_ports),
> - &virtconconf.nr_ports,
> - sizeof(virtconconf.nr_ports));
>
> - if (portdev->config.nr_ports == virtconconf.nr_ports) {
> - /*
> - * Port 0 got hot-added. Since we already did all the
> - * other initialisation for it, just tell the Host
> - * that the port is ready if we find the port. In
> - * case the port was hot-removed earlier, we call
> - * add_port to add the port.
> - */
> - struct port *port;
> -
> - port = find_port_by_id(portdev, 0);
> - if (!port)
> - add_port(portdev, 0);
> - else
> - send_control_msg(port, VIRTIO_CONSOLE_PORT_READY, 1);
> - return;
> - }
> - if (virtconconf.nr_ports > portdev->config.max_nr_ports) {
> - dev_warn(&vdev->dev,
> - "More ports specified (%u) than allowed (%u)",
> - portdev->config.nr_ports + 1,
> - portdev->config.max_nr_ports);
> - return;
> - }
> - if (virtconconf.nr_ports < portdev->config.nr_ports)
> - return;
> -
> - /* Hot-add ports */
> - while (virtconconf.nr_ports - portdev->config.nr_ports) {
> - err = add_port(portdev, portdev->config.nr_ports);
> - if (err)
> - break;
> - portdev->config.nr_ports++;
> - }
> + /* FIXME: Handle port hotplug/unplug */
> }
>
> static int init_vqs(struct ports_device *portdev)
> @@ -1274,7 +1254,7 @@ static int init_vqs(struct ports_device *portdev)
> u32 i, j, nr_ports, nr_queues;
> int err;
>
> - nr_ports = portdev->config.max_nr_ports;
> + nr_ports = portdev->config->max_nr_ports;
> nr_queues = use_multiport(portdev) ? (nr_ports + 1) * 2 : 2;
>
> vqs = kmalloc(nr_queues * sizeof(struct virtqueue *), GFP_KERNEL);
> @@ -1387,7 +1367,6 @@ static const struct file_operations portdev_fops = {
> static int __devinit virtcons_probe(struct virtio_device *vdev)
> {
> struct ports_device *portdev;
> - u32 i;
> int err;
> bool multiport;
>
> @@ -1415,30 +1394,45 @@ static int __devinit virtcons_probe(struct virtio_device *vdev)
> goto free;
> }
>
> - multiport = false;
> - portdev->config.nr_ports = 1;
> - portdev->config.max_nr_ports = 1;
> if (virtio_has_feature(vdev, VIRTIO_CONSOLE_F_MULTIPORT)) {
> + u32 max_nr_ports;
> +
> multiport = true;
> vdev->features[0] |= 1 << VIRTIO_CONSOLE_F_MULTIPORT;
>
> vdev->config->get(vdev, offsetof(struct virtio_console_config,
> - nr_ports),
> - &portdev->config.nr_ports,
> - sizeof(portdev->config.nr_ports));
> - vdev->config->get(vdev, offsetof(struct virtio_console_config,
> max_nr_ports),
> - &portdev->config.max_nr_ports,
> - sizeof(portdev->config.max_nr_ports));
> - if (portdev->config.nr_ports > portdev->config.max_nr_ports) {
> - dev_warn(&vdev->dev,
> - "More ports (%u) specified than allowed (%u). Will init %u ports.",
> - portdev->config.nr_ports,
> - portdev->config.max_nr_ports,
> - portdev->config.max_nr_ports);
> -
> - portdev->config.nr_ports = portdev->config.max_nr_ports;
> + &max_nr_ports, sizeof(max_nr_ports));
> + /*
> + * We have a variable-sized config space that's
> + * dependent on the maximum number of ports a guest
> + * can have. So we first get the max number of ports
> + * we can have and then allocate the config space
> + */
> + portdev->config = kmalloc(sizeof(struct virtio_console_config)
> + + get_ports_map_size(max_nr_ports),
> + GFP_KERNEL);
> + if (!portdev->config) {
> + err = -ENOMEM;
> + goto free_chrdev;
> + }
> +
> + portdev->config->max_nr_ports = max_nr_ports;
> + vdev->config->get(vdev, offsetof(struct virtio_console_config,
> + ports_map),
> + portdev->config->ports_map,
> + get_ports_map_size(max_nr_ports));
> + } else {
> + multiport = false;
> +
> + portdev->config = kmalloc(sizeof(struct virtio_console_config),
> + GFP_KERNEL);
> + if (!portdev->config) {
> + err = -ENOMEM;
> + goto free_chrdev;
> }
> +
> + portdev->config->max_nr_ports = 1;
> }
>
> /* Let the Host know we support multiple ports.*/
> @@ -1447,14 +1441,14 @@ static int __devinit virtcons_probe(struct virtio_device *vdev)
> err = init_vqs(portdev);
> if (err < 0) {
> dev_err(&vdev->dev, "Error %d initializing vqs\n", err);
> - goto free_chrdev;
> + goto free_config;
> }
>
> spin_lock_init(&portdev->ports_lock);
> INIT_LIST_HEAD(&portdev->ports);
>
> if (multiport) {
> - unsigned int nr_added_bufs;
> + unsigned int nr_added_bufs, i;
>
> spin_lock_init(&portdev->cvq_lock);
> INIT_WORK(&portdev->control_work, &control_work_handler);
> @@ -1467,10 +1461,26 @@ static int __devinit virtcons_probe(struct virtio_device *vdev)
> err = -ENOMEM;
> goto free_vqs;
> }
> - }
>
> - for (i = 0; i < portdev->config.nr_ports; i++)
> - add_port(portdev, i);
> + for (i = 0; i < (portdev->config->max_nr_ports + 31) / 32; i++) {
> + u32 map, port_nr;
> +
> + map = portdev->config->ports_map[i];
> + while (map) {
> + port_nr = find_next_bit_in_map(&map) + i * 32;
> +
> + add_port(portdev, port_nr);
> + /*
> + * FIXME: Send a notification to the
> + * host about failed port add
> + */
This FIXME is just pointing out an existing bug, correct?
> + }
> + }
> + } else {
> + err = add_port(portdev, 0);
> + if (err)
> + goto free_vqs;
> + }
>
> /* Start using the new console output. */
> early_put_chars = NULL;
> @@ -1480,6 +1490,8 @@ free_vqs:
> vdev->config->del_vqs(vdev);
> kfree(portdev->in_vqs);
> kfree(portdev->out_vqs);
> +free_config:
> + kfree(portdev->config);
> free_chrdev:
> unregister_chrdev(portdev->chr_major, "virtio-portsdev");
> free:
> @@ -1514,6 +1526,7 @@ static void virtcons_remove(struct virtio_device *vdev)
> vdev->config->del_vqs(vdev);
> kfree(portdev->in_vqs);
> kfree(portdev->out_vqs);
> + kfree(portdev->config);
>
> kfree(portdev);
> }
> diff --git a/include/linux/virtio_console.h b/include/linux/virtio_console.h
> index ae4f039..287ee44 100644
> --- a/include/linux/virtio_console.h
> +++ b/include/linux/virtio_console.h
> @@ -14,15 +14,23 @@
> #define VIRTIO_CONSOLE_F_SIZE 0 /* Does host provide console size? */
> #define VIRTIO_CONSOLE_F_MULTIPORT 1 /* Does host provide multiple ports? */
>
> +/*
> + * This is the config space shared between the Host and the Guest.
> + * The Host indicates to us the maximum number of ports this device
> + * can hold and a port map indicating which ports are active.
> + *
> + * The maximum number of ports is not a round number to prevent
> + * wastage of MSI vectors in case MSI is enabled for this device.
What does 'round number' mean in this context?
> + */
> struct virtio_console_config {
> /* colums of the screens */
> __u16 cols;
> /* rows of the screens */
> __u16 rows;
> - /* max. number of ports this device can hold */
> + /* max. number of ports this device can hold. */
> __u32 max_nr_ports;
> - /* number of ports added so far */
> - __u32 nr_ports;
> + /* locations of ports in use; variable-sized array */
It's in fact a bitmap, not an array, is it?
> + __u32 ports_map[0 /* (max_nr_ports + 31) / 2 */];
The array is in the packed structure. So I am not sure
we can legally take a pointer to ports_map like this patch does in
multiple places above: if we do compiler might assume
alignment?
> } __attribute__((packed));
>
Note that sizeof for variable sized structures is as far as I know a gcc
extension. You are supposed to use offsetof. And since packed structure
is also an extension, we should be very careful about combining them
together.
In fact, virtio seems to overuse packed structures: we probably can
save a ton of code by carefully padding everything without using
packed attribute.
> /*
> --
> 1.6.2.5
^ permalink raw reply
* Re: [PATCH 0/6] virtio: console: Fixes, abi update
From: Michael S. Tsirkin @ 2010-03-21 11:44 UTC (permalink / raw)
To: Amit Shah; +Cc: quintela, virtualization
In-Reply-To: <1269000408-29962-1-git-send-email-amit.shah@redhat.com>
On Fri, Mar 19, 2010 at 05:36:42PM +0530, Amit Shah wrote:
> Hello,
>
> These patches fix a couple of small issues:
> - generate a kobject change event so that udev is woken up on name
> changes
> - fix a crash after hot-unplug of the first console port and a
> subsequent config update
>
> But majorly, it reworks how ports are discovered: instead of numbering
> the ports individually in the host and the guest by just incrementing
> a number, we now switch to a bitmap in the config space exposed by the
> host to identify active ports. This lets us maintain the same
> numbering used by the host and also allows for hot-unplug via the
> config space. This is needed for proper migration support after
> several hot-plug/unplug operations.
>
> I've tested these patches on my testsuite to catch any regression or
> correctness issues. I've also tested all the hotplug-related changes
> here.
>
> These should go to 2.6.34, so that we don't push out a stable release
> with the older interface.
>
> Michael, please forward these to Linus if
> everyone is OK with these.
I have some concerns with the new ABI. I also expect Rusty to be back
in a couple of days, maybe he'll find some time to review the patches.
We'll also have to update the spec ...
Would you like me to queue up the first 2 patches meanwhile?
> I also have a git repo at
>
> git://git.kernel.org/pub/scm/linux/kernel/git/amit/vs-kernel.git master
>
> if you prefer to pull the patches.
> Amit Shah (6):
> virtio: console: Generate a kobject CHANGE event on adding 'name'
> attribute
> virtio: console: Check if port is valid in resize_console
> virtio: console: Switch to using a port bitmap for port discovery
> virtio: console: Separate out get_config in a separate function
> virtio: console: Handle hot-plug/unplug config actions
> virtio: console: Remove hot-unplug control message
>
> drivers/char/virtio_console.c | 238 ++++++++++++++++++++++++----------------
> include/linux/virtio_console.h | 15 ++-
> 2 files changed, 156 insertions(+), 97 deletions(-)
^ permalink raw reply
* Re: [PATCH 0/9] virtio-serial fixes, ABI updates
From: Michael S. Tsirkin @ 2010-03-21 13:47 UTC (permalink / raw)
To: Amit Shah; +Cc: quintela, qemu-devel, virtualization
In-Reply-To: <1268999926-29560-1-git-send-email-amit.shah@redhat.com>
On Fri, Mar 19, 2010 at 05:28:37PM +0530, Amit Shah wrote:
> Hello,
>
> This series fixes a few issues pointed out by Avi and Juan. Avi
> pointed out we should do full scatter/gather processing of guest data
> even if current (well-behaved) guests don't send multiple iovs per
> element.
>
> Juan pointed out a few migration-related bugs.
>
> In handling the migration fixes, I noticed hot-plug/unplug isn't
> handled perfectly for the migration case: ports are enumerated and the
> port numbering has to be consistent with the guest's numbering. If
> there's a mismatch, control messages meant for one port could be
> interpreted for another.
BTW, I think virtio serial migration code needs to be fixed
to be backwards compatible with old qemu if multiport
feature is off.
> To solve this issue, I go back to maintaining a bitmap in the config
> space for active ports. Hot-plug and unplug can be added easily via
> the config space as a result.
As I commented on the kernel driver, I'm not sure this is a good choice.
> The kernel driver has to be changed as well so that the changes are in
> sync with the changes here.
>
> I've tested these patches on my test suite that tests for correctness
> and also hot-plug/unplug cases and fixes presented here.
>
> Amit Shah (9):
> virtio-serial-bus: save/load: Ensure target has enough ports
> virtio-serial-bus: save/load: Ensure nr_ports on src and dest are
> same.
> virtio-serial: save/load: Ensure we have hot-plugged ports
> instantiated
> virtio-serial: Handle scatter-gather buffers for control messages
> virtio-serial: Handle scatter/gather input from the guest
> virtio-serial: Remove redundant check for 0-sized write request
> virtio-serial: Update copyright year to 2010
> virtio-serial-bus: Use a bitmap in virtio config space for active
> ports
> virtio-serial-bus: Let the guest know of host connection changes
> after migration
>
> hw/virtio-console.c | 4 +-
> hw/virtio-serial-bus.c | 205 ++++++++++++++++++++++++++++++++++++------------
> hw/virtio-serial.h | 8 +-
> 3 files changed, 161 insertions(+), 56 deletions(-)
^ permalink raw reply
* Re: [PATCH 3/6] virtio: console: Switch to using a port bitmap for port discovery
From: Amit Shah @ 2010-03-22 4:04 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: quintela, virtualization
In-Reply-To: <20100321112945.GA12339@redhat.com>
On (Sun) Mar 21 2010 [13:29:45], Michael S. Tsirkin wrote:
> On Fri, Mar 19, 2010 at 05:36:45PM +0530, Amit Shah wrote:
> > Ports are now discovered by their location as given by host instead of
> > just incrementing a number and expecting the host and guest numbers stay
> > in sync. They are needed to be the same because the control messages
> > identify ports using the port id.
> >
> > This is most beneficial to management software to properly place ports
> > at known ids so that the ids after hot-plug/unplug operations can be
> > controlled. This helps migration of guests after such hot-plug/unplug
> > operations.
> >
> > The support for port hot-plug is removed in this commit, will be added
> > in the following commits.
>
> It might be cleaner not to split it this way, merge the following
> commits into this one or split it in a different way.
Rusty in the past indicated he was OK with such a split since it
simplifies things and makes for easier review.
I'm fine with merging the next two patches in here too.
> > The config space is now a variable-sized array.
>
> I think this last bit is problematic: we won't be able to add any more data if
> we have to, without a lot of complexity.
Adding new fields before the bitmap should be fine as long as we
discover features and fetch the config is the right order. If, however,
another bitmap has to be added, that'll surely be painful.
However, I don't see the need to add another bitmap for sure, and I
don't think we need more config variables too. However, we have the
control channel in case this has to be expanded.
> Further, in the past we have
> also had problems running out of config space: see
> 3225beaba05d4f06087593f5e903ce867b6e118a.
How much config space is available? I guess there's enough for a ports
bitmap: without the ports, we're using 64 bits. And each port takes up
one bit. I guess we easily have 256 bits of space, so we can have 192
ports represented (at least) via the config space.
> There's also
> a comment in handle_control_message which suggests we
> might want a very large number of ports at some point,
> if we do using config space would be a mistake.
[Which comment?]
I can't certainly predict how many ports might be needed, but I think
we'll have other ways of communication if we need > 200 ports.
> It might be better to use a control vq for this, like virtio block ended
> up doing. The comment in handle_control_message hints we don't want to
> pass port id in config space, but I am not sure why we can't pass it in
> message buffer.
I'm not sure which comment you're referring to really.
> > +static u32 find_next_bit_in_map(u32 *map)
> > +{
> > + u32 port_bit;
> > +
> > + port_bit = ffs(*map);
> > + port_bit--; /* ffs returns bit location */
> > +
> > + *map &= ~(1U << port_bit);
>
> The above only works well if map is non-zero. This happens to be the
> case the way we call it, but since this means the function is not
> generic, it might be better to opencode it to make it obvious.
You're right; when I first had a bitmap-based approach, I had a
find_next_active_port() and returned VIRTIO_CONSOLE_BAD_ID in case ffs
returned 0. This is a valid case and should be fixed. I'll send out a
v2.
> > +static u32 get_ports_map_size(u32 max_nr_ports)
> > +{
> > + return sizeof(u32) * ((max_nr_ports + 31)/ 32);
>
> DIV_ROUND_UP here and elsewhere?
Ah, yes, I'll use it.
> > - for (i = 0; i < portdev->config.nr_ports; i++)
> > - add_port(portdev, i);
> > + for (i = 0; i < (portdev->config->max_nr_ports + 31) / 32; i++) {
> > + u32 map, port_nr;
> > +
> > + map = portdev->config->ports_map[i];
> > + while (map) {
> > + port_nr = find_next_bit_in_map(&map) + i * 32;
> > +
> > + add_port(portdev, port_nr);
> > + /*
> > + * FIXME: Send a notification to the
> > + * host about failed port add
> > + */
>
> This FIXME is just pointing out an existing bug, correct?
Yes, a control message indicating to the host something went wrong would
be helpful for mgmt.
> > diff --git a/include/linux/virtio_console.h b/include/linux/virtio_console.h
> > index ae4f039..287ee44 100644
> > --- a/include/linux/virtio_console.h
> > +++ b/include/linux/virtio_console.h
> > @@ -14,15 +14,23 @@
> > #define VIRTIO_CONSOLE_F_SIZE 0 /* Does host provide console size? */
> > #define VIRTIO_CONSOLE_F_MULTIPORT 1 /* Does host provide multiple ports? */
> >
> > +/*
> > + * This is the config space shared between the Host and the Guest.
> > + * The Host indicates to us the maximum number of ports this device
> > + * can hold and a port map indicating which ports are active.
> > + *
> > + * The maximum number of ports is not a round number to prevent
> > + * wastage of MSI vectors in case MSI is enabled for this device.
>
> What does 'round number' mean in this context?
Meaning restricting the 'max_nr_ports' to multiples of 32.
> > + */
> > struct virtio_console_config {
> > /* colums of the screens */
> > __u16 cols;
> > /* rows of the screens */
> > __u16 rows;
> > - /* max. number of ports this device can hold */
> > + /* max. number of ports this device can hold. */
> > __u32 max_nr_ports;
> > - /* number of ports added so far */
> > - __u32 nr_ports;
> > + /* locations of ports in use; variable-sized array */
>
> It's in fact a bitmap, not an array, is it?
hm, yeah; I'll update.
> > + __u32 ports_map[0 /* (max_nr_ports + 31) / 2 */];
>
> The array is in the packed structure. So I am not sure
> we can legally take a pointer to ports_map like this patch does in
> multiple places above: if we do compiler might assume
> alignment?
We always reference the elements in the ports_map[] in u32-sized
increments. Is there a problem?
> > } __attribute__((packed));
> >
> Note that sizeof for variable sized structures is as far as I know a gcc
> extension. You are supposed to use offsetof. And since packed structure
> is also an extension, we should be very careful about combining them
> together.
OK, I'll switch to offsetof(ports_map) where I use
sizeof(virtio_console_config) -- in the get_config() function.
> In fact, virtio seems to overuse packed structures: we probably can
> save a ton of code by carefully padding everything without using
> packed attribute.
Yeah; packing could be avoided. Maybe Rusty has an explanation for why
it was done this way.
Amit
^ permalink raw reply
* Re: [PATCH 0/9] virtio-serial fixes, ABI updates
From: Amit Shah @ 2010-03-22 4:55 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: quintela, qemu-devel, virtualization
In-Reply-To: <20100321134753.GA12758@redhat.com>
On (Sun) Mar 21 2010 [15:47:53], Michael S. Tsirkin wrote:
> On Fri, Mar 19, 2010 at 05:28:37PM +0530, Amit Shah wrote:
> > Hello,
> >
> > This series fixes a few issues pointed out by Avi and Juan. Avi
> > pointed out we should do full scatter/gather processing of guest data
> > even if current (well-behaved) guests don't send multiple iovs per
> > element.
> >
> > Juan pointed out a few migration-related bugs.
> >
> > In handling the migration fixes, I noticed hot-plug/unplug isn't
> > handled perfectly for the migration case: ports are enumerated and the
> > port numbering has to be consistent with the guest's numbering. If
> > there's a mismatch, control messages meant for one port could be
> > interpreted for another.
>
> BTW, I think virtio serial migration code needs to be fixed
> to be backwards compatible with old qemu if multiport
> feature is off.
This is already done; the savevm version number is bumped up.
> > To solve this issue, I go back to maintaining a bitmap in the config
> > space for active ports. Hot-plug and unplug can be added easily via
> > the config space as a result.
>
> As I commented on the kernel driver, I'm not sure this is a good choice.
I've replied to that comment; please let me know if it's a concern.
Also, what do others think?
Amit
^ permalink raw reply
* Re: [Qemu-devel] Re: [PATCH 4/9] virtio-serial: Handle scatter-gather buffers for control messages
From: Amit Shah @ 2010-03-22 5:18 UTC (permalink / raw)
To: Avi Kivity; +Cc: qemu-devel, quintela, virtualization, mst
In-Reply-To: <4BA47C02.3000105@redhat.com>
On (Sat) Mar 20 2010 [09:40:50], Avi Kivity wrote:
> On 03/19/2010 01:58 PM, Amit Shah wrote:
>> +
>> + offset = 0;
>> + for (i = 0; i< elem.out_num; i++) {
>> + memcpy(buf + offset, elem.out_sg[i].iov_base,
>> + elem.out_sg[i].iov_len);
>> + offset += elem.out_sg[i].iov_len;
>> + }
>> + len = cur_len;
>> +
>> + handle_control_message(vser, buf, len);
>> + virtqueue_push(vq,&elem, len);
>> + }
>> + if (len) {
>> + qemu_free(buf);
>> }
>> virtio_notify(vdev, vq);
>> }
>
> Isn't there some virtio function to linearize requests?
I don't see one.
Amit
^ permalink raw reply
* Re: [PATCH 3/6] virtio: console: Switch to using a port bitmap for port discovery
From: Michael S. Tsirkin @ 2010-03-22 8:53 UTC (permalink / raw)
To: Amit Shah; +Cc: quintela, virtualization
In-Reply-To: <20100322040401.GA24577@amit-x200.redhat.com>
On Mon, Mar 22, 2010 at 09:34:01AM +0530, Amit Shah wrote:
> On (Sun) Mar 21 2010 [13:29:45], Michael S. Tsirkin wrote:
> > On Fri, Mar 19, 2010 at 05:36:45PM +0530, Amit Shah wrote:
> > > Ports are now discovered by their location as given by host instead of
> > > just incrementing a number and expecting the host and guest numbers stay
> > > in sync. They are needed to be the same because the control messages
> > > identify ports using the port id.
> > >
> > > This is most beneficial to management software to properly place ports
> > > at known ids so that the ids after hot-plug/unplug operations can be
> > > controlled. This helps migration of guests after such hot-plug/unplug
> > > operations.
> > >
> > > The support for port hot-plug is removed in this commit, will be added
> > > in the following commits.
> >
> > It might be cleaner not to split it this way, merge the following
> > commits into this one or split it in a different way.
>
> Rusty in the past indicated he was OK with such a split since it
> simplifies things and makes for easier review.
>
> I'm fine with merging the next two patches in here too.
>
> > > The config space is now a variable-sized array.
> >
> > I think this last bit is problematic: we won't be able to add any more data if
> > we have to, without a lot of complexity.
>
> Adding new fields before the bitmap should be fine as long as we
> discover features and fetch the config is the right order. If, however,
> another bitmap has to be added, that'll surely be painful.
Well, we'll need two structures old_config and new_config,
as opposed to simply extending the existing one.
> However, I don't see the need to add another bitmap for sure, and I
> don't think we need more config variables too. However, we have the
> control channel in case this has to be expanded.
How about using it right now?
> > Further, in the past we have
> > also had problems running out of config space: see
> > 3225beaba05d4f06087593f5e903ce867b6e118a.
>
> How much config space is available? I guess there's enough for a ports
> bitmap: without the ports, we're using 64 bits. And each port takes up
> one bit. I guess we easily have 256 bits of space, so we can have 192
> ports represented (at least) via the config space.
256 bytes but we are using config space for other things as well.
> > There's also
> > a comment in handle_control_message which suggests we
> > might want a very large number of ports at some point,
> > if we do using config space would be a mistake.
>
> [Which comment?]
>
> I can't certainly predict how many ports might be needed, but I think
> we'll have other ways of communication if we need > 200 ports.
If, say, 32 bytes are sufficient, let's just reserve a fixed size
array and everything will be simpler?
> > It might be better to use a control vq for this, like virtio block ended
> > up doing. The comment in handle_control_message hints we don't want to
> > pass port id in config space, but I am not sure why we can't pass it in
> > message buffer.
>
> I'm not sure which comment you're referring to really.
We have this:
+ * Hot unplug the port. We don't decrement nr_ports
+ * since we don't want to deal with extra complexities
+ * of using the lowest-available port id: We can just
+ * pick up the nr_ports number as the id and not have
+ * userspace send it to us. This helps us in two
+ * ways:
+ *
+ * - We don't need to have a 'port_id' field in the
+ * config space when a port is hot-added. This is a
+ * good thing as we might queue up multiple hotplug
+ * requests issued in our workqueue.
+ *
+ * - Another way to deal with this would have been to
+ * use a bitmap of the active ports and select the
+ * lowest non-active port from that map. That
+ * bloats the already tight config space and we
+ * would end up artificially limiting the
+ * max. number of ports to sizeof(bitmap). Right
+ * now we can support 2^32 ports (as the port id is
+ * stored in a u32 type).
+ *
Did you change your mind then?
> > > +static u32 find_next_bit_in_map(u32 *map)
> > > +{
> > > + u32 port_bit;
> > > +
> > > + port_bit = ffs(*map);
> > > + port_bit--; /* ffs returns bit location */
> > > +
> > > + *map &= ~(1U << port_bit);
> >
> > The above only works well if map is non-zero. This happens to be the
> > case the way we call it, but since this means the function is not
> > generic, it might be better to opencode it to make it obvious.
>
> You're right; when I first had a bitmap-based approach, I had a
> find_next_active_port() and returned VIRTIO_CONSOLE_BAD_ID in case ffs
> returned 0. This is a valid case and should be fixed. I'll send out a
> v2.
>
> > > +static u32 get_ports_map_size(u32 max_nr_ports)
> > > +{
> > > + return sizeof(u32) * ((max_nr_ports + 31)/ 32);
> >
> > DIV_ROUND_UP here and elsewhere?
>
> Ah, yes, I'll use it.
>
> > > - for (i = 0; i < portdev->config.nr_ports; i++)
> > > - add_port(portdev, i);
> > > + for (i = 0; i < (portdev->config->max_nr_ports + 31) / 32; i++) {
> > > + u32 map, port_nr;
> > > +
> > > + map = portdev->config->ports_map[i];
> > > + while (map) {
> > > + port_nr = find_next_bit_in_map(&map) + i * 32;
> > > +
> > > + add_port(portdev, port_nr);
> > > + /*
> > > + * FIXME: Send a notification to the
> > > + * host about failed port add
> > > + */
> >
> > This FIXME is just pointing out an existing bug, correct?
>
> Yes, a control message indicating to the host something went wrong would
> be helpful for mgmt.
>
> > > diff --git a/include/linux/virtio_console.h b/include/linux/virtio_console.h
> > > index ae4f039..287ee44 100644
> > > --- a/include/linux/virtio_console.h
> > > +++ b/include/linux/virtio_console.h
> > > @@ -14,15 +14,23 @@
> > > #define VIRTIO_CONSOLE_F_SIZE 0 /* Does host provide console size? */
> > > #define VIRTIO_CONSOLE_F_MULTIPORT 1 /* Does host provide multiple ports? */
> > >
> > > +/*
> > > + * This is the config space shared between the Host and the Guest.
> > > + * The Host indicates to us the maximum number of ports this device
> > > + * can hold and a port map indicating which ports are active.
> > > + *
> > > + * The maximum number of ports is not a round number to prevent
> > > + * wastage of MSI vectors in case MSI is enabled for this device.
> >
> > What does 'round number' mean in this context?
>
> Meaning restricting the 'max_nr_ports' to multiples of 32.
It's an unusual use of the word :)
> > > + */
> > > struct virtio_console_config {
> > > /* colums of the screens */
> > > __u16 cols;
> > > /* rows of the screens */
> > > __u16 rows;
> > > - /* max. number of ports this device can hold */
> > > + /* max. number of ports this device can hold. */
> > > __u32 max_nr_ports;
> > > - /* number of ports added so far */
> > > - __u32 nr_ports;
> > > + /* locations of ports in use; variable-sized array */
> >
> > It's in fact a bitmap, not an array, is it?
>
> hm, yeah; I'll update.
>
> > > + __u32 ports_map[0 /* (max_nr_ports + 31) / 2 */];
> >
> > The array is in the packed structure. So I am not sure
> > we can legally take a pointer to ports_map like this patch does in
> > multiple places above: if we do compiler might assume
> > alignment?
>
> We always reference the elements in the ports_map[] in u32-sized
> increments. Is there a problem?
Hmm I'm not sure. Maybe not.
> > > } __attribute__((packed));
> > >
> > Note that sizeof for variable sized structures is as far as I know a gcc
> > extension. You are supposed to use offsetof. And since packed structure
> > is also an extension, we should be very careful about combining them
> > together.
>
> OK, I'll switch to offsetof(ports_map) where I use
> sizeof(virtio_console_config) -- in the get_config() function.
>
> > In fact, virtio seems to overuse packed structures: we probably can
> > save a ton of code by carefully padding everything without using
> > packed attribute.
>
> Yeah; packing could be avoided. Maybe Rusty has an explanation for why
> it was done this way.
>
> Amit
^ permalink raw reply
* Re: [PATCH 3/6] virtio: console: Switch to using a port bitmap for port discovery
From: Amit Shah @ 2010-03-22 9:45 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: quintela, virtualization
In-Reply-To: <20100322085336.GB16574@redhat.com>
On (Mon) Mar 22 2010 [10:53:36], Michael S. Tsirkin wrote:
> On Mon, Mar 22, 2010 at 09:34:01AM +0530, Amit Shah wrote:
> > On (Sun) Mar 21 2010 [13:29:45], Michael S. Tsirkin wrote:
> > > On Fri, Mar 19, 2010 at 05:36:45PM +0530, Amit Shah wrote:
> > > > Ports are now discovered by their location as given by host instead of
> > > > just incrementing a number and expecting the host and guest numbers stay
> > > > in sync. They are needed to be the same because the control messages
> > > > identify ports using the port id.
> > > >
> > > > This is most beneficial to management software to properly place ports
> > > > at known ids so that the ids after hot-plug/unplug operations can be
> > > > controlled. This helps migration of guests after such hot-plug/unplug
> > > > operations.
> > > >
> > > > The support for port hot-plug is removed in this commit, will be added
> > > > in the following commits.
> > >
> > > It might be cleaner not to split it this way, merge the following
> > > commits into this one or split it in a different way.
> >
> > Rusty in the past indicated he was OK with such a split since it
> > simplifies things and makes for easier review.
> >
> > I'm fine with merging the next two patches in here too.
> >
> > > > The config space is now a variable-sized array.
> > >
> > > I think this last bit is problematic: we won't be able to add any more data if
> > > we have to, without a lot of complexity.
> >
> > Adding new fields before the bitmap should be fine as long as we
> > discover features and fetch the config is the right order. If, however,
> > another bitmap has to be added, that'll surely be painful.
>
> Well, we'll need two structures old_config and new_config,
> as opposed to simply extending the existing one.
True; we'd be adding fields before the ports_map. We anyway have to keep
older fields around for backward compat, so not much changes.
> > However, I don't see the need to add another bitmap for sure, and I
> > don't think we need more config variables too. However, we have the
> > control channel in case this has to be expanded.
>
> How about using it right now?
Just that doing hot-plug/unplug via config updates seems better for some
reason.
> > > Further, in the past we have
> > > also had problems running out of config space: see
> > > 3225beaba05d4f06087593f5e903ce867b6e118a.
> >
> > How much config space is available? I guess there's enough for a ports
> > bitmap: without the ports, we're using 64 bits. And each port takes up
> > one bit. I guess we easily have 256 bits of space, so we can have 192
> > ports represented (at least) via the config space.
>
> 256 bytes but we are using config space for other things as well.
>
> > > There's also
> > > a comment in handle_control_message which suggests we
> > > might want a very large number of ports at some point,
> > > if we do using config space would be a mistake.
> >
> > [Which comment?]
> >
> > I can't certainly predict how many ports might be needed, but I think
> > we'll have other ways of communication if we need > 200 ports.
>
> If, say, 32 bytes are sufficient, let's just reserve a fixed size
> array and everything will be simpler?
Yes, 32 bytes means 256 ports. Should be OK; if not, one could add more
such devices and get more ports. But if I have to choose between fixing
the number of ports in the config space vs using the control queue for
the bitmap, I'll go for the latter. That's assuming we really really
don't want to use the config space for storing the bitmap.
> > > It might be better to use a control vq for this, like virtio block ended
> > > up doing. The comment in handle_control_message hints we don't want to
> > > pass port id in config space, but I am not sure why we can't pass it in
> > > message buffer.
> >
> > I'm not sure which comment you're referring to really.
>
> We have this:
>
> + * Hot unplug the port. We don't decrement nr_ports
> + * since we don't want to deal with extra complexities
> + * of using the lowest-available port id: We can just
> + * pick up the nr_ports number as the id and not have
> + * userspace send it to us. This helps us in two
> + * ways:
> + *
> + * - We don't need to have a 'port_id' field in the
> + * config space when a port is hot-added. This is a
> + * good thing as we might queue up multiple hotplug
> + * requests issued in our workqueue.
> + *
> + * - Another way to deal with this would have been to
> + * use a bitmap of the active ports and select the
> + * lowest non-active port from that map. That
> + * bloats the already tight config space and we
> + * would end up artificially limiting the
> + * max. number of ports to sizeof(bitmap). Right
> + * now we can support 2^32 ports (as the port id is
> + * stored in a u32 type).
> + *
>
> Did you change your mind then?
Ah, ok. This comment got removed by the last patch in this series.
Well right now the kernel can support 2^32 ports; but the downside of
doing it w/o bitmaps was a race in port number allocation between the
guest and the host. Without a bitmap, we could use a port id only once,
so after unplugging a port, a new port had to be on a new id.
With bitmaps, any free location can be re-used. So it's not all that bad
as that comment makes it sound. Also, as you suggest using the control
queue for bitmaps, we could have large bitmaps irrespective of the
config space limit.
> > > > +static u32 find_next_bit_in_map(u32 *map)
> > > > +{
> > > > + u32 port_bit;
> > > > +
> > > > + port_bit = ffs(*map);
> > > > + port_bit--; /* ffs returns bit location */
> > > > +
> > > > + *map &= ~(1U << port_bit);
> > >
> > > The above only works well if map is non-zero. This happens to be the
> > > case the way we call it, but since this means the function is not
> > > generic, it might be better to opencode it to make it obvious.
> >
> > You're right; when I first had a bitmap-based approach, I had a
> > find_next_active_port() and returned VIRTIO_CONSOLE_BAD_ID in case ffs
> > returned 0. This is a valid case and should be fixed. I'll send out a
> > v2.
BTW I just added a comment to the function mentioning it's to be called
with 'map' non-zero. Should be sufficient for the current usage.
Amit
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox