* Re: [PATCH 7/7] virtio: console: Return -EPIPE if port on the host isn't connected
From: Amit Shah @ 2010-03-23 11:06 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: virtualization
In-Reply-To: <20100323090023.GD22774@redhat.com>
On (Tue) Mar 23 2010 [11:00:23], Michael S. Tsirkin wrote:
> On Tue, Mar 23, 2010 at 02:28:00PM +0530, Amit Shah wrote:
> > The virtio-serial ports are like pipes, if there's no reader on the
> > other end, sending data might get it either ignored or the host might
> > return '0', which would make guests get -EAGAIN. Since we know the state
> > of the host port connection, it's appropriate to let the application
> > know that the other end isn't connected.
> >
> > Signed-off-by: Amit Shah <amit.shah@redhat.com>
> > ---
> > drivers/char/virtio_console.c | 3 +++
> > 1 files changed, 3 insertions(+), 0 deletions(-)
> >
> > diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
> > index 55de0b5..4562964 100644
> > --- a/drivers/char/virtio_console.c
> > +++ b/drivers/char/virtio_console.c
> > @@ -411,6 +411,9 @@ static ssize_t send_buf(struct port *port, void *in_buf, size_t in_count)
> > ssize_t ret;
> > unsigned int len;
> >
> > + if (use_multiport(port->portdev) && !port->host_connected)
> > + return -EPIPE;
> > +
>
> Hmm, do applications actually handle this error?
As Michael points out on irc, console-handling apps may not handle EPIPE
properly. So I guess the test should be reworked to:
if (!is_console_port(port) && !port->host_connected)
return -EPIPE;
Also, he points out that write(2) mentions other error messages can be
returned depending on the file type. So we could return -ENOTCONN.
Amit
^ permalink raw reply
* Re: [PATCH 7/7] virtio: console: Return -EPIPE if port on the host isn't connected
From: Gerd Hoffmann @ 2010-03-23 11:04 UTC (permalink / raw)
To: Amit Shah; +Cc: virtualization, Michael S. Tsirkin
In-Reply-To: <20100323105220.GK8675@amit-x200.redhat.com>
On 03/23/10 11:52, Amit Shah wrote:
>> With poll support done right (don't signal "writable" when not
>> connected) this shouldn't be a issue. Apps retrying over and over
>> without waiting for the filehandle becoming writable are broken by
>> design.
>
> Yeah; poll returns POLLOUT only when the host connection is open. This
> is for apps that write() without consulting poll().
Well, /me just remembers this also depends on the file mode. Returning
-EAGAIN instantly is fine for non-blocking file handles. For file
handles in blocking mode you would have to block here, waiting for the
host to connect.
cheers,
Gerd
^ permalink raw reply
* Re: [PATCH 7/7] virtio: console: Return -EPIPE if port on the host isn't connected
From: Amit Shah @ 2010-03-23 10:52 UTC (permalink / raw)
To: Gerd Hoffmann; +Cc: virtualization, Michael S. Tsirkin
In-Reply-To: <4BA89C1E.7090403@redhat.com>
On (Tue) Mar 23 2010 [11:46:54], Gerd Hoffmann wrote:
> On 03/23/10 10:13, Amit Shah wrote:
>>>> + if (use_multiport(port->portdev)&& !port->host_connected)
>>>> + return -EPIPE;
>>>> +
>>>
>>> Hmm, do applications actually handle this error?
>>
>> Don't know; there aren't any apps yet that use this functionality too.
>
> Looks sensible to me.
>
>> However, some error has to be indicated to the app otherwise it's just
>> messy if the app keeps writing and receiving EAGAIN; we just get a tight
>> loop in this case.
>
> With poll support done right (don't signal "writable" when not
> connected) this shouldn't be a issue. Apps retrying over and over
> without waiting for the filehandle becoming writable are broken by
> design.
Yeah; poll returns POLLOUT only when the host connection is open. This
is for apps that write() without consulting poll().
> But apps can't disturgish between "ring full" and "host disconnected"
> then. So think returning -EPIPE here isn't a bad choice.
Yeah; that too.
Amit
^ permalink raw reply
* Re: [PATCH 7/7] virtio: console: Return -EPIPE if port on the host isn't connected
From: Gerd Hoffmann @ 2010-03-23 10:46 UTC (permalink / raw)
To: Amit Shah; +Cc: virtualization, Michael S. Tsirkin
In-Reply-To: <20100323091352.GI8675@amit-x200.redhat.com>
On 03/23/10 10:13, Amit Shah wrote:
>>> + if (use_multiport(port->portdev)&& !port->host_connected)
>>> + return -EPIPE;
>>> +
>>
>> Hmm, do applications actually handle this error?
>
> Don't know; there aren't any apps yet that use this functionality too.
Looks sensible to me.
> However, some error has to be indicated to the app otherwise it's just
> messy if the app keeps writing and receiving EAGAIN; we just get a tight
> loop in this case.
With poll support done right (don't signal "writable" when not
connected) this shouldn't be a issue. Apps retrying over and over
without waiting for the filehandle becoming writable are broken by design.
But apps can't disturgish between "ring full" and "host disconnected"
then. So think returning -EPIPE here isn't a bad choice.
cheers,
Gerd
^ permalink raw reply
* Re: [PATCH 7/7] virtio: console: Return -EPIPE if port on the host isn't connected
From: Amit Shah @ 2010-03-23 9:13 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: virtualization
In-Reply-To: <20100323090023.GD22774@redhat.com>
On (Tue) Mar 23 2010 [11:00:23], Michael S. Tsirkin wrote:
> On Tue, Mar 23, 2010 at 02:28:00PM +0530, Amit Shah wrote:
> > The virtio-serial ports are like pipes, if there's no reader on the
> > other end, sending data might get it either ignored or the host might
> > return '0', which would make guests get -EAGAIN. Since we know the state
> > of the host port connection, it's appropriate to let the application
> > know that the other end isn't connected.
> >
> > Signed-off-by: Amit Shah <amit.shah@redhat.com>
> > ---
> > drivers/char/virtio_console.c | 3 +++
> > 1 files changed, 3 insertions(+), 0 deletions(-)
> >
> > diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
> > index 55de0b5..4562964 100644
> > --- a/drivers/char/virtio_console.c
> > +++ b/drivers/char/virtio_console.c
> > @@ -411,6 +411,9 @@ static ssize_t send_buf(struct port *port, void *in_buf, size_t in_count)
> > ssize_t ret;
> > unsigned int len;
> >
> > + if (use_multiport(port->portdev) && !port->host_connected)
> > + return -EPIPE;
> > +
>
> Hmm, do applications actually handle this error?
Don't know; there aren't any apps yet that use this functionality too.
However, some error has to be indicated to the app otherwise it's just
messy if the app keeps writing and receiving EAGAIN; we just get a tight
loop in this case.
Amit
^ permalink raw reply
* Re: [PATCH 7/7] virtio: console: Return -EPIPE if port on the host isn't connected
From: Michael S. Tsirkin @ 2010-03-23 9:00 UTC (permalink / raw)
To: Amit Shah; +Cc: virtualization
In-Reply-To: <1269334680-1369-8-git-send-email-amit.shah@redhat.com>
On Tue, Mar 23, 2010 at 02:28:00PM +0530, Amit Shah wrote:
> The virtio-serial ports are like pipes, if there's no reader on the
> other end, sending data might get it either ignored or the host might
> return '0', which would make guests get -EAGAIN. Since we know the state
> of the host port connection, it's appropriate to let the application
> know that the other end isn't connected.
>
> Signed-off-by: Amit Shah <amit.shah@redhat.com>
> ---
> drivers/char/virtio_console.c | 3 +++
> 1 files changed, 3 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
> index 55de0b5..4562964 100644
> --- a/drivers/char/virtio_console.c
> +++ b/drivers/char/virtio_console.c
> @@ -411,6 +411,9 @@ static ssize_t send_buf(struct port *port, void *in_buf, size_t in_count)
> ssize_t ret;
> unsigned int len;
>
> + if (use_multiport(port->portdev) && !port->host_connected)
> + return -EPIPE;
> +
Hmm, do applications actually handle this error?
> out_vq = port->out_vq;
>
> sg_init_one(sg, in_buf, in_count);
> --
> 1.6.2.5
^ permalink raw reply
* [PATCH 7/7] virtio: console: Return -EPIPE if port on the host isn't connected
From: Amit Shah @ 2010-03-23 8:58 UTC (permalink / raw)
To: virtualization; +Cc: Amit Shah, mst
In-Reply-To: <1269334680-1369-7-git-send-email-amit.shah@redhat.com>
The virtio-serial ports are like pipes, if there's no reader on the
other end, sending data might get it either ignored or the host might
return '0', which would make guests get -EAGAIN. Since we know the state
of the host port connection, it's appropriate to let the application
know that the other end isn't connected.
Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
drivers/char/virtio_console.c | 3 +++
1 files changed, 3 insertions(+), 0 deletions(-)
diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 55de0b5..4562964 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -411,6 +411,9 @@ static ssize_t send_buf(struct port *port, void *in_buf, size_t in_count)
ssize_t ret;
unsigned int len;
+ if (use_multiport(port->portdev) && !port->host_connected)
+ return -EPIPE;
+
out_vq = port->out_vq;
sg_init_one(sg, in_buf, in_count);
--
1.6.2.5
^ permalink raw reply related
* [PATCH 6/7] virtio: console: Don't always create a port 0 if using multiport
From: Amit Shah @ 2010-03-23 8:57 UTC (permalink / raw)
To: virtualization; +Cc: Amit Shah, mst
In-Reply-To: <1269334680-1369-6-git-send-email-amit.shah@redhat.com>
If we're using multiport, there's no point in always creating a console
port. Create the console port only if the host doesn't support
multiport.
Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
drivers/char/virtio_console.c | 23 ++++++++---------------
1 files changed, 8 insertions(+), 15 deletions(-)
diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 8b67ff1..55de0b5 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -1044,14 +1044,8 @@ static void handle_control_message(struct ports_device *portdev,
switch (cpkt->event) {
case VIRTIO_CONSOLE_PORT_ADD:
if (port) {
- /*
- * This can happen for port 0: we have to
- * create a console port during probe() as was
- * the behaviour before the MULTIPORT feature.
- * On a newer host, when the host tells us
- * that a port 0 is available, we should just
- * say we have the port all set up.
- */
+ dev_dbg(&portdev->vdev->dev,
+ "Port %u already added\n", port->id);
send_control_msg(port, VIRTIO_CONSOLE_PORT_READY, 1);
break;
}
@@ -1395,15 +1389,14 @@ static int __devinit virtcons_probe(struct virtio_device *vdev)
err = -ENOMEM;
goto free_vqs;
}
-
+ } else {
+ /*
+ * If we're running on an older host, we always want
+ * to create a console port.
+ */
+ add_port(portdev, 0);
}
- /*
- * For backward compatibility: if we're running on an older
- * host, we always want to create a console port.
- */
- add_port(portdev, 0);
-
/* Start using the new console output. */
early_put_chars = NULL;
--
1.6.2.5
^ permalink raw reply related
* [PATCH 5/7] virtio: console: Use a control message to add ports
From: Amit Shah @ 2010-03-23 8:57 UTC (permalink / raw)
To: virtualization; +Cc: Amit Shah, mst
In-Reply-To: <1269334680-1369-5-git-send-email-amit.shah@redhat.com>
Instead of the host and guest independently enumerating ports, switch to
a control message to add ports where the host supplies the port number
so there's no ambiguity or a possibility of a race between the host and
the guest port numbers.
We now no longer need the 'nr_ports' config value. Since no kernel has
been released with the MULTIPORT changes yet, we have a chance to fiddle
with the config space without adding compatibility features.
This is beneficial for management software, which would now be able to
instantiate ports at known locations and avoid problems that arise with
implicit numbering in the host and the guest. This removes the 'guessing
game' part of it, and management software can now actually indicate
which id to spawn a particular port on.
Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
drivers/char/virtio_console.c | 78 +++++++++++++++++----------------------
include/linux/virtio_console.h | 18 +++++----
2 files changed, 44 insertions(+), 52 deletions(-)
diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 8056ad9..8b67ff1 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -1034,7 +1034,7 @@ static void handle_control_message(struct ports_device *portdev,
cpkt = (struct virtio_console_control *)(buf->buf + buf->offset);
port = find_port_by_id(portdev, cpkt->id);
- if (!port) {
+ if (!port && cpkt->event != VIRTIO_CONSOLE_PORT_ADD) {
/* No valid header at start of buffer. Drop it. */
dev_dbg(&portdev->vdev->dev,
"Invalid index %u in control packet\n", cpkt->id);
@@ -1042,6 +1042,30 @@ static void handle_control_message(struct ports_device *portdev,
}
switch (cpkt->event) {
+ case VIRTIO_CONSOLE_PORT_ADD:
+ if (port) {
+ /*
+ * This can happen for port 0: we have to
+ * create a console port during probe() as was
+ * the behaviour before the MULTIPORT feature.
+ * On a newer host, when the host tells us
+ * that a port 0 is available, we should just
+ * say we have the port all set up.
+ */
+ send_control_msg(port, VIRTIO_CONSOLE_PORT_READY, 1);
+ break;
+ }
+ if (cpkt->id >= portdev->config.max_nr_ports) {
+ dev_warn(&portdev->vdev->dev,
+ "Request for adding port with out-of-bound id %u, max. supported id: %u\n",
+ cpkt->id, portdev->config.max_nr_ports - 1);
+ break;
+ }
+ add_port(portdev, cpkt->id);
+ break;
+ case VIRTIO_CONSOLE_PORT_REMOVE:
+ remove_port(port);
+ break;
case VIRTIO_CONSOLE_CONSOLE_PORT:
if (!cpkt->value)
break;
@@ -1100,32 +1124,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;
}
}
@@ -1333,7 +1331,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;
@@ -1362,29 +1359,15 @@ static int __devinit virtcons_probe(struct virtio_device *vdev)
}
multiport = false;
- portdev->config.nr_ports = 1;
portdev->config.max_nr_ports = 1;
if (virtio_has_feature(vdev, VIRTIO_CONSOLE_F_MULTIPORT)) {
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;
- }
}
/* Let the Host know we support multiple ports.*/
@@ -1412,13 +1395,20 @@ 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 backward compatibility: if we're running on an older
+ * host, we always want to create a console port.
+ */
+ add_port(portdev, 0);
/* Start using the new console output. */
early_put_chars = NULL;
+
+ __send_control_msg(portdev, VIRTIO_CONSOLE_BAD_ID,
+ VIRTIO_CONSOLE_DEVICE_READY, 1);
return 0;
free_vqs:
diff --git a/include/linux/virtio_console.h b/include/linux/virtio_console.h
index ae4f039..a85064d 100644
--- a/include/linux/virtio_console.h
+++ b/include/linux/virtio_console.h
@@ -14,6 +14,8 @@
#define VIRTIO_CONSOLE_F_SIZE 0 /* Does host provide console size? */
#define VIRTIO_CONSOLE_F_MULTIPORT 1 /* Does host provide multiple ports? */
+#define VIRTIO_CONSOLE_BAD_ID (~(u32)0)
+
struct virtio_console_config {
/* colums of the screens */
__u16 cols;
@@ -21,8 +23,6 @@ struct virtio_console_config {
__u16 rows;
/* max. number of ports this device can hold */
__u32 max_nr_ports;
- /* number of ports added so far */
- __u32 nr_ports;
} __attribute__((packed));
/*
@@ -36,12 +36,14 @@ struct virtio_console_control {
};
/* Some events for control messages */
-#define VIRTIO_CONSOLE_PORT_READY 0
-#define VIRTIO_CONSOLE_CONSOLE_PORT 1
-#define VIRTIO_CONSOLE_RESIZE 2
-#define VIRTIO_CONSOLE_PORT_OPEN 3
-#define VIRTIO_CONSOLE_PORT_NAME 4
-#define VIRTIO_CONSOLE_PORT_REMOVE 5
+#define VIRTIO_CONSOLE_DEVICE_READY 0
+#define VIRTIO_CONSOLE_PORT_ADD 1
+#define VIRTIO_CONSOLE_PORT_REMOVE 2
+#define VIRTIO_CONSOLE_PORT_READY 3
+#define VIRTIO_CONSOLE_CONSOLE_PORT 4
+#define VIRTIO_CONSOLE_RESIZE 5
+#define VIRTIO_CONSOLE_PORT_OPEN 6
+#define VIRTIO_CONSOLE_PORT_NAME 7
#ifdef __KERNEL__
int __init virtio_cons_early_init(int (*put_chars)(u32, const char *, int));
--
1.6.2.5
^ permalink raw reply related
* [PATCH 4/7] virtio: console: Move code around for future patches
From: Amit Shah @ 2010-03-23 8:57 UTC (permalink / raw)
To: virtualization; +Cc: Amit Shah, mst
In-Reply-To: <1269334680-1369-4-git-send-email-amit.shah@redhat.com>
We're going to use add_port() from handle_control_message() in the next
patch.
Move the add_port() and fill_queue(), which depends on it, above
handle_control_message() to avoid forward declarations.
Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
drivers/char/virtio_console.c | 262 ++++++++++++++++++++--------------------
1 files changed, 131 insertions(+), 131 deletions(-)
diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 99c36d4..8056ad9 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -854,6 +854,137 @@ static const struct file_operations port_debugfs_ops = {
.read = debugfs_read,
};
+static unsigned int fill_queue(struct virtqueue *vq, spinlock_t *lock)
+{
+ struct port_buffer *buf;
+ unsigned int nr_added_bufs;
+ int ret;
+
+ nr_added_bufs = 0;
+ do {
+ buf = alloc_buf(PAGE_SIZE);
+ if (!buf)
+ break;
+
+ spin_lock_irq(lock);
+ ret = add_inbuf(vq, buf);
+ if (ret < 0) {
+ spin_unlock_irq(lock);
+ free_buf(buf);
+ break;
+ }
+ nr_added_bufs++;
+ spin_unlock_irq(lock);
+ } while (ret > 0);
+
+ return nr_added_bufs;
+}
+
+static int add_port(struct ports_device *portdev, u32 id)
+{
+ char debugfs_name[16];
+ struct port *port;
+ struct port_buffer *buf;
+ dev_t devt;
+ unsigned int nr_added_bufs;
+ int err;
+
+ port = kmalloc(sizeof(*port), GFP_KERNEL);
+ if (!port) {
+ err = -ENOMEM;
+ goto fail;
+ }
+
+ port->portdev = portdev;
+ port->id = id;
+
+ port->name = NULL;
+ port->inbuf = NULL;
+ port->cons.hvc = NULL;
+
+ port->host_connected = port->guest_connected = false;
+
+ port->in_vq = portdev->in_vqs[port->id];
+ port->out_vq = portdev->out_vqs[port->id];
+
+ cdev_init(&port->cdev, &port_fops);
+
+ devt = MKDEV(portdev->chr_major, id);
+ err = cdev_add(&port->cdev, devt, 1);
+ if (err < 0) {
+ dev_err(&port->portdev->vdev->dev,
+ "Error %d adding cdev for port %u\n", err, id);
+ goto free_port;
+ }
+ port->dev = device_create(pdrvdata.class, &port->portdev->vdev->dev,
+ devt, port, "vport%up%u",
+ port->portdev->drv_index, id);
+ if (IS_ERR(port->dev)) {
+ err = PTR_ERR(port->dev);
+ dev_err(&port->portdev->vdev->dev,
+ "Error %d creating device for port %u\n",
+ err, id);
+ goto free_cdev;
+ }
+
+ spin_lock_init(&port->inbuf_lock);
+ init_waitqueue_head(&port->waitqueue);
+
+ /* Fill the in_vq with buffers so the host can send us data. */
+ nr_added_bufs = fill_queue(port->in_vq, &port->inbuf_lock);
+ if (!nr_added_bufs) {
+ dev_err(port->dev, "Error allocating inbufs\n");
+ err = -ENOMEM;
+ goto free_device;
+ }
+
+ /*
+ * If we're not using multiport support, this has to be a console port
+ */
+ if (!use_multiport(port->portdev)) {
+ err = init_port_console(port);
+ if (err)
+ goto free_inbufs;
+ }
+
+ spin_lock_irq(&portdev->ports_lock);
+ list_add_tail(&port->list, &port->portdev->ports);
+ spin_unlock_irq(&portdev->ports_lock);
+
+ /*
+ * Tell the Host we're set so that it can send us various
+ * configuration parameters for this port (eg, port name,
+ * caching, whether this is a console port, etc.)
+ */
+ send_control_msg(port, VIRTIO_CONSOLE_PORT_READY, 1);
+
+ if (pdrvdata.debugfs_dir) {
+ /*
+ * Finally, create the debugfs file that we can use to
+ * inspect a port's state at any time
+ */
+ sprintf(debugfs_name, "vport%up%u",
+ port->portdev->drv_index, id);
+ port->debugfs_file = debugfs_create_file(debugfs_name, 0444,
+ pdrvdata.debugfs_dir,
+ port,
+ &port_debugfs_ops);
+ }
+ return 0;
+
+free_inbufs:
+ while ((buf = port->in_vq->vq_ops->detach_unused_buf(port->in_vq)))
+ free_buf(buf);
+free_device:
+ device_destroy(pdrvdata.class, port->dev->devt);
+free_cdev:
+ cdev_del(&port->cdev);
+free_port:
+ kfree(port);
+fail:
+ return err;
+}
+
/* Remove all port-specific data. */
static int remove_port(struct port *port)
{
@@ -1081,137 +1212,6 @@ static void config_intr(struct virtio_device *vdev)
resize_console(find_port_by_id(portdev, 0));
}
-static unsigned int fill_queue(struct virtqueue *vq, spinlock_t *lock)
-{
- struct port_buffer *buf;
- unsigned int nr_added_bufs;
- int ret;
-
- nr_added_bufs = 0;
- do {
- buf = alloc_buf(PAGE_SIZE);
- if (!buf)
- break;
-
- spin_lock_irq(lock);
- ret = add_inbuf(vq, buf);
- if (ret < 0) {
- spin_unlock_irq(lock);
- free_buf(buf);
- break;
- }
- nr_added_bufs++;
- spin_unlock_irq(lock);
- } while (ret > 0);
-
- return nr_added_bufs;
-}
-
-static int add_port(struct ports_device *portdev, u32 id)
-{
- char debugfs_name[16];
- struct port *port;
- struct port_buffer *buf;
- dev_t devt;
- unsigned int nr_added_bufs;
- int err;
-
- port = kmalloc(sizeof(*port), GFP_KERNEL);
- if (!port) {
- err = -ENOMEM;
- goto fail;
- }
-
- port->portdev = portdev;
- port->id = id;
-
- port->name = NULL;
- port->inbuf = NULL;
- port->cons.hvc = NULL;
-
- port->host_connected = port->guest_connected = false;
-
- port->in_vq = portdev->in_vqs[port->id];
- port->out_vq = portdev->out_vqs[port->id];
-
- cdev_init(&port->cdev, &port_fops);
-
- devt = MKDEV(portdev->chr_major, id);
- err = cdev_add(&port->cdev, devt, 1);
- if (err < 0) {
- dev_err(&port->portdev->vdev->dev,
- "Error %d adding cdev for port %u\n", err, id);
- goto free_port;
- }
- port->dev = device_create(pdrvdata.class, &port->portdev->vdev->dev,
- devt, port, "vport%up%u",
- port->portdev->drv_index, id);
- if (IS_ERR(port->dev)) {
- err = PTR_ERR(port->dev);
- dev_err(&port->portdev->vdev->dev,
- "Error %d creating device for port %u\n",
- err, id);
- goto free_cdev;
- }
-
- spin_lock_init(&port->inbuf_lock);
- init_waitqueue_head(&port->waitqueue);
-
- /* Fill the in_vq with buffers so the host can send us data. */
- nr_added_bufs = fill_queue(port->in_vq, &port->inbuf_lock);
- if (!nr_added_bufs) {
- dev_err(port->dev, "Error allocating inbufs\n");
- err = -ENOMEM;
- goto free_device;
- }
-
- /*
- * If we're not using multiport support, this has to be a console port
- */
- if (!use_multiport(port->portdev)) {
- err = init_port_console(port);
- if (err)
- goto free_inbufs;
- }
-
- spin_lock_irq(&portdev->ports_lock);
- list_add_tail(&port->list, &port->portdev->ports);
- spin_unlock_irq(&portdev->ports_lock);
-
- /*
- * Tell the Host we're set so that it can send us various
- * configuration parameters for this port (eg, port name,
- * caching, whether this is a console port, etc.)
- */
- send_control_msg(port, VIRTIO_CONSOLE_PORT_READY, 1);
-
- if (pdrvdata.debugfs_dir) {
- /*
- * Finally, create the debugfs file that we can use to
- * inspect a port's state at any time
- */
- sprintf(debugfs_name, "vport%up%u",
- port->portdev->drv_index, id);
- port->debugfs_file = debugfs_create_file(debugfs_name, 0444,
- pdrvdata.debugfs_dir,
- port,
- &port_debugfs_ops);
- }
- return 0;
-
-free_inbufs:
- while ((buf = port->in_vq->vq_ops->detach_unused_buf(port->in_vq)))
- free_buf(buf);
-free_device:
- device_destroy(pdrvdata.class, port->dev->devt);
-free_cdev:
- cdev_del(&port->cdev);
-free_port:
- kfree(port);
-fail:
- return err;
-}
-
static int init_vqs(struct ports_device *portdev)
{
vq_callback_t **io_callbacks;
--
1.6.2.5
^ permalink raw reply related
* [PATCH 3/7] virtio: console: Add a __send_control_msg() that can send messages without a valid port
From: Amit Shah @ 2010-03-23 8:57 UTC (permalink / raw)
To: virtualization; +Cc: Amit Shah, mst
In-Reply-To: <1269334680-1369-3-git-send-email-amit.shah@redhat.com>
We will introduce control messages that operate on the device as a whole
rather than just ports. Make send_control_msg() a wrapper around
__send_control_msg() which does not need a valid port.
Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
drivers/char/virtio_console.c | 16 +++++++++++-----
1 files changed, 11 insertions(+), 5 deletions(-)
diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index c7894f3..99c36d4 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -372,22 +372,22 @@ out:
return ret;
}
-static ssize_t send_control_msg(struct port *port, unsigned int event,
- unsigned int value)
+static ssize_t __send_control_msg(struct ports_device *portdev, u32 port_id,
+ unsigned int event, unsigned int value)
{
struct scatterlist sg[1];
struct virtio_console_control cpkt;
struct virtqueue *vq;
unsigned int len;
- if (!use_multiport(port->portdev))
+ if (!use_multiport(portdev))
return 0;
- cpkt.id = port->id;
+ cpkt.id = port_id;
cpkt.event = event;
cpkt.value = value;
- vq = port->portdev->c_ovq;
+ vq = portdev->c_ovq;
sg_init_one(sg, &cpkt, sizeof(cpkt));
if (vq->vq_ops->add_buf(vq, sg, 1, 0, &cpkt) >= 0) {
@@ -398,6 +398,12 @@ static ssize_t send_control_msg(struct port *port, unsigned int event,
return 0;
}
+static ssize_t send_control_msg(struct port *port, unsigned int event,
+ unsigned int value)
+{
+ return __send_control_msg(port->portdev, port->id, event, value);
+}
+
static ssize_t send_buf(struct port *port, void *in_buf, size_t in_count)
{
struct scatterlist sg[1];
--
1.6.2.5
^ permalink raw reply related
* [PATCH 2/7] virtio: console: Remove config work handler
From: Amit Shah @ 2010-03-23 8:57 UTC (permalink / raw)
To: virtualization; +Cc: Amit Shah, mst
In-Reply-To: <1269334680-1369-2-git-send-email-amit.shah@redhat.com>
We're going to switch to using control messages for port hot-plug and
initial port discovery. Remove the config work handler which handled
port hot-plug so far.
Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
drivers/char/virtio_console.c | 64 +----------------------------------------
1 files changed, 1 insertions(+), 63 deletions(-)
diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 44288ce..c7894f3 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -109,7 +109,6 @@ struct ports_device {
* notification
*/
struct work_struct control_work;
- struct work_struct config_work;
struct list_head ports;
@@ -1066,10 +1065,7 @@ static void config_intr(struct virtio_device *vdev)
struct ports_device *portdev;
portdev = vdev->priv;
- if (use_multiport(portdev)) {
- /* Handle port hot-add */
- schedule_work(&portdev->config_work);
- }
+
/*
* We'll use this way of resizing only for legacy support.
* For newer userspace (VIRTIO_CONSOLE_F_MULTPORT+), use
@@ -1210,62 +1206,6 @@ fail:
return err;
}
-/*
- * The workhandler for config-space updates.
- *
- * This is called when ports are hot-added.
- */
-static void config_work_handler(struct work_struct *work)
-{
- struct virtio_console_config virtconconf;
- struct ports_device *portdev;
- struct virtio_device *vdev;
- int err;
-
- 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++;
- }
-}
-
static int init_vqs(struct ports_device *portdev)
{
vq_callback_t **io_callbacks;
@@ -1458,7 +1398,6 @@ static int __devinit virtcons_probe(struct virtio_device *vdev)
spin_lock_init(&portdev->cvq_lock);
INIT_WORK(&portdev->control_work, &control_work_handler);
- INIT_WORK(&portdev->config_work, &config_work_handler);
nr_added_bufs = fill_queue(portdev->c_ivq, &portdev->cvq_lock);
if (!nr_added_bufs) {
@@ -1498,7 +1437,6 @@ static void virtcons_remove(struct virtio_device *vdev)
portdev = vdev->priv;
cancel_work_sync(&portdev->control_work);
- cancel_work_sync(&portdev->config_work);
list_for_each_entry_safe(port, port2, &portdev->ports, list)
remove_port(port);
--
1.6.2.5
^ permalink raw reply related
* [PATCH 1/7] MAINTAINERS: Put the virtio-console entry in correct alphabetical order
From: Amit Shah @ 2010-03-23 8:57 UTC (permalink / raw)
To: virtualization; +Cc: Amit Shah, mst
In-Reply-To: <1269334680-1369-1-git-send-email-amit.shah@redhat.com>
Move around the entry for virtio-console to keep the file sorted.
Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
MAINTAINERS | 13 +++++++------
1 files changed, 7 insertions(+), 6 deletions(-)
diff --git a/MAINTAINERS b/MAINTAINERS
index 382eaa4..3306429 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2465,12 +2465,6 @@ L: linuxppc-dev@ozlabs.org
S: Odd Fixes
F: drivers/char/hvc_*
-VIRTIO CONSOLE DRIVER
-M: Amit Shah <amit.shah@redhat.com>
-L: virtualization@lists.linux-foundation.org
-S: Maintained
-F: drivers/char/virtio_console.c
-
iSCSI BOOT FIRMWARE TABLE (iBFT) DRIVER
M: Peter Jones <pjones@redhat.com>
M: Konrad Rzeszutek Wilk <konrad@kernel.org>
@@ -5952,6 +5946,13 @@ S: Maintained
F: Documentation/filesystems/vfat.txt
F: fs/fat/
+VIRTIO CONSOLE DRIVER
+M: Amit Shah <amit.shah@redhat.com>
+L: virtualization@lists.linux-foundation.org
+S: Maintained
+F: drivers/char/virtio_console.c
+F: include/linux/virtio_console.h
+
VIRTIO HOST (VHOST)
M: "Michael S. Tsirkin" <mst@redhat.com>
L: kvm@vger.kernel.org
--
1.6.2.5
^ permalink raw reply related
* [PATCH 0/7] virtio: console: Fixes, new flexible way of discovering ports
From: Amit Shah @ 2010-03-23 8:57 UTC (permalink / raw)
To: virtualization; +Cc: Amit Shah, mst
Hello all,
This new patchset switches over to using the control queue for port
discovery so that we can stay in sync with the host for port
enumeration and also don't use bitmaps to limit us to config space
sizes.
This changes the ABI, so it would be better to merge this for 2.6.34
so that we don't have to worry about released kernels with the older
ABI.
Amit Shah (7):
MAINTAINERS: Put the virtio-console entry in correct alphabetical
order
virtio: console: Remove config work handler
virtio: console: Add a __send_control_msg() that can send messages
without a valid port
virtio: console: Move code around for future patches
virtio: console: Use a control message to add ports
virtio: console: Don't always create a port 0 if using multiport
virtio: console: Return -EPIPE if port on the host isn't connected
MAINTAINERS | 13 +-
drivers/char/virtio_console.c | 418 +++++++++++++++++-----------------------
include/linux/virtio_console.h | 18 +-
3 files changed, 191 insertions(+), 258 deletions(-)
^ permalink raw reply
* Re: Virtio_console usage of early printk
From: Amit Shah @ 2010-03-23 8:55 UTC (permalink / raw)
To: François Diakhate; +Cc: linuxppc-dev, virtualization
In-Reply-To: <6ad07c641003221045m10ee5c84x3a92436582600f21@mail.gmail.com>
On (Mon) Mar 22 2010 [18:45:47], François Diakhate wrote:
> Hi all,
>
> As far as I can see, early_put_chars is not used by virtio_console
> because it checks whether there is a port available before using it.
> If I understand correctly, this makes it useless because once we have
> a port, we can use the regular virtio transport to output things to
> the console. Does the attached patch seem valid ? Feedback from s390
> and powerpc users who use this functionality would be appreciated.
>
> Thanks,
> François
> From 3961f380bbe84a1036ddfc823039cbee31b44dcb Mon Sep 17 00:00:00 2001
> From: =?utf-8?q?Fran=C3=A7ois=20Diakhat=C3=A9?= <fdiakh@gmail.com>
> Date: Thu, 18 Mar 2010 14:48:20 +0100
> Subject: virtio: console: Fix early_put_chars usage
>
> Currently early_put_chars is not used by virtio_console because it can only be used once a port has been found, at which point it's too late because it is no longer needed. This patch should fix it.
You'll have to give a signed-off-by for the patch.
Amit
^ permalink raw reply
* Re: Virtio_console usage of early printk
From: Christian Borntraeger @ 2010-03-23 8:17 UTC (permalink / raw)
To: Amit Shah; +Cc: François Diakhate, linuxppc-dev, virtualization
In-Reply-To: <20100323021620.GA4088@amit-x200.redhat.com>
Am Dienstag 23 März 2010 03:16:20 schrieb Amit Shah:
> On (Mon) Mar 22 2010 [18:45:47], François Diakhate wrote:
> > the console. Does the attached patch seem valid ? Feedback from s390
> > and powerpc users who use this functionality would be appreciated.
>
> Looks OK to me, Christian, others, could you comment on this please?
Looks good and boot tested with kuli on s390.
Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>
> > From 3961f380bbe84a1036ddfc823039cbee31b44dcb Mon Sep 17 00:00:00 2001
> > From: =?utf-8?q?Fran=C3=A7ois=20Diakhat=C3=A9?= <fdiakh@gmail.com>
> > Date: Thu, 18 Mar 2010 14:48:20 +0100
> > Subject: virtio: console: Fix early_put_chars usage
> >
> > Currently early_put_chars is not used by virtio_console because it can only be used once a port has been found, at which point it's too late because it is no longer needed. This patch should fix it.
> > ---
> > drivers/char/virtio_console.c | 6 +++---
> > 1 files changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
> > index f404ccf..691ba21 100644
> > --- a/drivers/char/virtio_console.c
> > +++ b/drivers/char/virtio_console.c
> > @@ -645,13 +645,13 @@ static int put_chars(u32 vtermno, const char *buf, int count)
> > {
> > struct port *port;
> >
> > + if (unlikely(early_put_chars))
> > + return early_put_chars(vtermno, buf, count);
> > +
> > port = find_port_by_vtermno(vtermno);
> > if (!port)
> > return 0;
> >
> > - if (unlikely(early_put_chars))
> > - return early_put_chars(vtermno, buf, count);
> > -
> > return send_buf(port, (void *)buf, count);
> > }
^ permalink raw reply
* Re: Virtio_console usage of early printk
From: Alexander Graf @ 2010-03-23 7:42 UTC (permalink / raw)
To: François Diakhate; +Cc: amit.shah, linuxppc-dev, virtualization
In-Reply-To: <6ad07c641003221045m10ee5c84x3a92436582600f21@mail.gmail.com>
On 22.03.2010, at 18:45, François Diakhate wrote:
> Hi all,
>
> As far as I can see, early_put_chars is not used by virtio_console
> because it checks whether there is a port available before using it.
> If I understand correctly, this makes it useless because once we have
> a port, we can use the regular virtio transport to output things to
> the console. Does the attached patch seem valid ? Feedback from s390
> and powerpc users who use this functionality would be appreciated.
On S390 we use the hypercall interface with characters instead of memory addresses as parameters. The hypervisor then decides if the character is an early printk or a real hypercall.
I'm not sure how PPC handles that - I never used virtio console there.
Either way, early printk does work for S390. So what exactly is this patch supposed to fix?
Alex
^ permalink raw reply
* Re: Virtio_console usage of early printk
From: Amit Shah @ 2010-03-23 2:16 UTC (permalink / raw)
To: François Diakhate
Cc: Christian Borntraeger, linuxppc-dev, virtualization
In-Reply-To: <6ad07c641003221045m10ee5c84x3a92436582600f21@mail.gmail.com>
On (Mon) Mar 22 2010 [18:45:47], François Diakhate wrote:
> Hi all,
>
> As far as I can see, early_put_chars is not used by virtio_console
> because it checks whether there is a port available before using it.
> If I understand correctly, this makes it useless because once we have
> a port, we can use the regular virtio transport to output things to
> the console. Does the attached patch seem valid ? Feedback from s390
> and powerpc users who use this functionality would be appreciated.
Looks OK to me, Christian, others, could you comment on this please?
> Thanks,
> François
> From 3961f380bbe84a1036ddfc823039cbee31b44dcb Mon Sep 17 00:00:00 2001
> From: =?utf-8?q?Fran=C3=A7ois=20Diakhat=C3=A9?= <fdiakh@gmail.com>
> Date: Thu, 18 Mar 2010 14:48:20 +0100
> Subject: virtio: console: Fix early_put_chars usage
>
> Currently early_put_chars is not used by virtio_console because it can only be used once a port has been found, at which point it's too late because it is no longer needed. This patch should fix it.
> ---
> drivers/char/virtio_console.c | 6 +++---
> 1 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
> index f404ccf..691ba21 100644
> --- a/drivers/char/virtio_console.c
> +++ b/drivers/char/virtio_console.c
> @@ -645,13 +645,13 @@ static int put_chars(u32 vtermno, const char *buf, int count)
> {
> struct port *port;
>
> + if (unlikely(early_put_chars))
> + return early_put_chars(vtermno, buf, count);
> +
> port = find_port_by_vtermno(vtermno);
> if (!port)
> return 0;
>
> - if (unlikely(early_put_chars))
> - return early_put_chars(vtermno, buf, count);
> -
> return send_buf(port, (void *)buf, count);
> }
>
> --
> 1.6.1.3
>
Amit
^ permalink raw reply
* [RFC] vhost-blk implementation
From: Badari Pulavarty @ 2010-03-23 0:34 UTC (permalink / raw)
To: virtualization, qemu-devel
Hi,
Inspired by vhost-net implementation, I did initial prototype
of vhost-blk to see if it provides any benefits over QEMU virtio-blk.
I haven't handled all the error cases, fixed naming conventions etc.,
but the implementation is stable to play with. I tried not to deviate
from vhost-net implementation where possible.
NOTE: Only change I had to make to vhost core code is to
increase VHOST_NET_MAX_SG to 130 (128+2) in vhost.h
Performance:
=============
I have done simple tests to see how it performs. I got very
encouraging results on sequential read tests. But on sequential
write tests, I see degrade over virtio-blk. I can't figure out and
explain why. Can some one shed light on whats happening here ?
Read Results:
=============
Test does read of 84GB file from the host (through virtio). I unmount
and mount the filesystem on the host to make sure there is nothing
in the page cache..
with vhost-blk:
----------------
# time dd if=/dev/vda of=/dev/null bs=128k iflag=direct
640000+0 records in
640000+0 records out
83886080000 bytes (84 GB) copied, 126.135 seconds, 665 MB/s
real 2m6.137s
user 0m0.281s
sys 0m14.725s
without vhost-blk: (virtio)
---------------------------
# time dd if=/dev/vda of=/dev/null bs=128k iflag=direct
640000+0 records in
640000+0 records out
83886080000 bytes (84 GB) copied, 275.466 seconds, 305 MB/s
real 4m35.468s
user 0m0.373s
sys 0m48.074s
Write Results:
==============
I see degraded IO performance when doing sequential IO write
tests with vhost-blk compared to virtio-blk.
# time dd of=/dev/vda if=/dev/zero bs=2M oflag=direct
I get ~110MB/sec with virtio-blk, but I get only ~60MB/sec with
vhost-blk. Wondering why ?
Comments/flames ?
Thanks,
Badari
vhost-blk is in-kernel accelerator for virtio-blk.
At this time, this is a prototype based on virtio-net.
Lots of error handling and clean up needs to be done.
Read performance is pretty good over QEMU virtio-blk, but
write performance is not anywhere close to QEMU virtio-blk.
Why ?
Signed-off-by: Badari Pulavarty <pbadari@us.ibm.com>
---
drivers/vhost/blk.c | 242 ++++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 242 insertions(+)
Index: net-next/drivers/vhost/blk.c
===================================================================
--- /dev/null 1970-01-01 00:00:00.000000000 +0000
+++ net-next/drivers/vhost/blk.c 2010-03-22 18:07:18.156584400 -0400
@@ -0,0 +1,242 @@
+ /*
+ * virtio-block server in host kernel.
+ * Inspired by vhost-net and shamlessly ripped code from it :)
+ */
+
+#include <linux/compat.h>
+#include <linux/eventfd.h>
+#include <linux/vhost.h>
+#include <linux/virtio_net.h>
+#include <linux/virtio_blk.h>
+#include <linux/mmu_context.h>
+#include <linux/miscdevice.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/workqueue.h>
+#include <linux/rcupdate.h>
+#include <linux/file.h>
+
+#include "vhost.h"
+
+#define VHOST_BLK_VQ_MAX 1
+
+struct vhost_blk {
+ struct vhost_dev dev;
+ struct vhost_virtqueue vqs[VHOST_BLK_VQ_MAX];
+ struct vhost_poll poll[VHOST_BLK_VQ_MAX];
+};
+
+static int do_handle_io(struct file *file, uint32_t type, uint64_t sector,
+ struct iovec *iov, int in)
+{
+ loff_t pos = sector << 8;
+ int ret = 0;
+
+ if (type & VIRTIO_BLK_T_FLUSH) {
+ ret = vfs_fsync(file, file->f_path.dentry, 1);
+ } else if (type & VIRTIO_BLK_T_OUT) {
+ ret = vfs_writev(file, iov, in, &pos);
+ } else {
+ ret = vfs_readv(file, iov, in, &pos);
+ }
+ return ret;
+}
+
+static void handle_blk(struct vhost_blk *blk)
+{
+ struct vhost_virtqueue *vq = &blk->dev.vqs[0];
+ unsigned head, out, in;
+ struct virtio_blk_outhdr hdr;
+ int r, nvecs;
+ uint8_t status = 0;
+
+ use_mm(blk->dev.mm);
+ mutex_lock(&vq->mutex);
+
+ vhost_disable_notify(vq);
+
+ for (;;) {
+ head = vhost_get_vq_desc(&blk->dev, vq, vq->iov,
+ ARRAY_SIZE(vq->iov),
+ &out, &in, NULL, NULL);
+ if (head == vq->num) {
+ if (unlikely(vhost_enable_notify(vq))) {
+ vhost_disable_notify(vq);
+ continue;
+ }
+ break;
+ }
+
+ BUG_ON(vq->iov[0].iov_len != 16);
+
+ r = copy_from_user(&hdr, vq->iov[0].iov_base, sizeof hdr);
+ if (r < 0) {
+ printk("copy from user failed\n");
+ vhost_discard_vq_desc(vq);
+ break;
+ }
+
+ nvecs = out - 1;
+ if (hdr.type == VIRTIO_BLK_T_IN)
+ nvecs = in - 1;
+
+ r = do_handle_io(vq->private_data, hdr.type, hdr.sector, &vq->iov[1], nvecs);
+ status = (r < 0) ? VIRTIO_BLK_S_IOERR : VIRTIO_BLK_S_OK;
+
+ nvecs++;
+ BUG_ON(vq->iov[nvecs].iov_len != 1);
+
+ if (copy_to_user(vq->iov[nvecs].iov_base, &status, sizeof status) < 0) {
+ printk("copy to user failed\n");
+ vhost_discard_vq_desc(vq);
+ break;
+ }
+ vhost_add_used_and_signal(&blk->dev, vq, head, r);
+ }
+ mutex_unlock(&vq->mutex);
+ unuse_mm(blk->dev.mm);
+}
+
+static void vhost_blk_flush(struct vhost_blk *n)
+{
+ vhost_poll_flush(n->poll);
+ vhost_poll_flush(&n->dev.vqs[0].poll);
+}
+
+static void handle_blk_kick(struct work_struct *work)
+{
+ struct vhost_virtqueue *vq;
+ struct vhost_blk *blk;
+ vq = container_of(work, struct vhost_virtqueue, poll.work);
+ blk = container_of(vq->dev, struct vhost_blk, dev);
+ handle_blk(blk);
+}
+
+static void handle_rq_blk(struct work_struct *work)
+{
+ struct vhost_blk *blk;
+ blk = container_of(work, struct vhost_blk, poll[0].work);
+ handle_blk(blk);
+}
+
+static int vhost_blk_open(struct inode *inode, struct file *f)
+{
+ struct vhost_blk *n = kmalloc(sizeof *n, GFP_KERNEL);
+ int r;
+ if (!n)
+ return -ENOMEM;
+ n->vqs[0].handle_kick = handle_blk_kick;
+ r = vhost_dev_init(&n->dev, n->vqs, VHOST_BLK_VQ_MAX);
+ if (r < 0) {
+ kfree(n);
+ return r;
+ }
+
+ vhost_poll_init(n->poll, handle_rq_blk, POLLOUT|POLLIN);
+ f->private_data = n;
+ return 0;
+}
+
+static int vhost_blk_release(struct inode *inode, struct file *f)
+{
+ struct vhost_blk *n = f->private_data;
+
+ fput(n->vqs->private_data);
+ kfree(n);
+ return 0;
+}
+
+static long vhost_blk_set_backend(struct vhost_blk *n, unsigned index, int fd)
+{
+ struct file *file;
+ struct vhost_virtqueue *vq;
+
+ file = fget(fd);
+ if (!file)
+ return -EBADF;
+
+ vq = n->vqs + index;
+ mutex_lock(&vq->mutex);
+ rcu_assign_pointer(vq->private_data, file);
+ mutex_unlock(&vq->mutex);
+ return 0;
+}
+
+
+static long vhost_blk_ioctl(struct file *f, unsigned int ioctl,
+ unsigned long arg)
+{
+ struct vhost_blk *n = f->private_data;
+ void __user *argp = (void __user *)arg;
+ struct vhost_vring_file backend;
+ int r;
+
+ switch (ioctl) {
+ case VHOST_NET_SET_BACKEND:
+ r = copy_from_user(&backend, argp, sizeof backend);
+ if (r < 0)
+ return r;
+ return vhost_blk_set_backend(n, backend.index, backend.fd);
+ default:
+ mutex_lock(&n->dev.mutex);
+ r = vhost_dev_ioctl(&n->dev, ioctl, arg);
+ vhost_blk_flush(n);
+ mutex_unlock(&n->dev.mutex);
+ return r;
+ }
+}
+
+const static struct file_operations vhost_blk_fops = {
+ .owner = THIS_MODULE,
+ .release = vhost_blk_release,
+ .open = vhost_blk_open,
+ .unlocked_ioctl = vhost_blk_ioctl,
+};
+
+static struct miscdevice vhost_blk_misc = {
+ 234,
+ "vhost-blk",
+ &vhost_blk_fops,
+};
+
+int vhost_blk_init(void)
+{
+ int r = vhost_init();
+ if (r)
+ goto err_init;
+ r = misc_register(&vhost_blk_misc);
+ if (r)
+ goto err_reg;
+ return 0;
+err_reg:
+ vhost_cleanup();
+err_init:
+ return r;
+
+}
+module_init(vhost_blk_init);
+
+void vhost_blk_exit(void)
+{
+ misc_deregister(&vhost_blk_misc);
+ vhost_cleanup();
+}
+module_exit(vhost_blk_exit);
+
+MODULE_VERSION("0.0.1");
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("Host kernel accelerator for virtio blk");
^ permalink raw reply
* Virtio_console usage of early printk
From: François Diakhate @ 2010-03-22 17:45 UTC (permalink / raw)
To: virtualization, linuxppc-dev; +Cc: amit.shah, fdiakh
[-- Attachment #1: Type: text/plain, Size: 449 bytes --]
Hi all,
As far as I can see, early_put_chars is not used by virtio_console
because it checks whether there is a port available before using it.
If I understand correctly, this makes it useless because once we have
a port, we can use the regular virtio transport to output things to
the console. Does the attached patch seem valid ? Feedback from s390
and powerpc users who use this functionality would be appreciated.
Thanks,
François
[-- Attachment #2: 0001-virtio-console-Fix-early_put_chars-usage.patch --]
[-- Type: text/x-patch, Size: 1112 bytes --]
From 3961f380bbe84a1036ddfc823039cbee31b44dcb Mon Sep 17 00:00:00 2001
From: =?utf-8?q?Fran=C3=A7ois=20Diakhat=C3=A9?= <fdiakh@gmail.com>
Date: Thu, 18 Mar 2010 14:48:20 +0100
Subject: virtio: console: Fix early_put_chars usage
Currently early_put_chars is not used by virtio_console because it can only be used once a port has been found, at which point it's too late because it is no longer needed. This patch should fix it.
---
drivers/char/virtio_console.c | 6 +++---
1 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index f404ccf..691ba21 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -645,13 +645,13 @@ static int put_chars(u32 vtermno, const char *buf, int count)
{
struct port *port;
+ if (unlikely(early_put_chars))
+ return early_put_chars(vtermno, buf, count);
+
port = find_port_by_vtermno(vtermno);
if (!port)
return 0;
- if (unlikely(early_put_chars))
- return early_put_chars(vtermno, buf, count);
-
return send_buf(port, (void *)buf, count);
}
--
1.6.1.3
[-- Attachment #3: Type: text/plain, Size: 184 bytes --]
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization
^ permalink raw reply related
* [PULL] virtio
From: Michael S. Tsirkin @ 2010-03-22 14:34 UTC (permalink / raw)
To: Rusty Russell; +Cc: Amit Shah, Linus Torvalds, linux-kernel, virtualization
Linus,
Please pull a couple of virtio-console fixes by Amit.
The following changes since commit 220bf991b0366cc50a94feede3d7341fa5710ee4:
Linux 2.6.34-rc2 (2010-03-19 18:17:57 -0700)
are available in the git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/mst/vhost.git for-linus
Amit Shah (2):
virtio: console: Generate a kobject CHANGE event on adding 'name' attribute
virtio: console: Check if port is valid in resize_console
drivers/char/virtio_console.c | 15 +++++++++++++--
1 files changed, 13 insertions(+), 2 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 12:31 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: quintela, virtualization
In-Reply-To: <20100322121632.GA17287@redhat.com>
On (Mon) Mar 22 2010 [14:16:32], Michael S. Tsirkin wrote:
> > > > 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.
>
> I guess with control queue we also simply use message per port
> state change - would that be simpler than a bitmap?
Yes, that's how hot-unplug is done currently (before this patchset).
I'll cook up a similar version like you suggest for adding ports as
well.
> > > > > > + port_bit = ffs(*map);
> > > > > > + port_bit--; /* ffs returns bit location */
>
> simple u32 port_bit = ffs(*map) - 1; not clear enough?
Well yeah; I'll just open-code it.
Amit
--
http://log.amitshah.net/
^ 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 12:16 UTC (permalink / raw)
To: Amit Shah; +Cc: quintela, virtualization
In-Reply-To: <20100322094524.GA19148@amit-x200.redhat.com>
On Mon, Mar 22, 2010 at 03:15:24PM +0530, Amit Shah wrote:
> 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.
I guess with control queue we also simply use message per port
state change - would that be simpler than a 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 */
simple u32 port_bit = ffs(*map) - 1; not clear enough?
> > > > > +
> > > > > + *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
Or just opencode it, it's two lines.
--
MST
^ permalink raw reply
* Re: [PATCH 0/6] virtio: console: Fixes, abi update
From: Amit Shah @ 2010-03-22 10:44 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: quintela, virtualization
In-Reply-To: <20100321114404.GA12348@redhat.com>
On (Sun) Mar 21 2010 [13:44:04], Michael S. Tsirkin wrote:
>
> Would you like me to queue up the first 2 patches meanwhile?
Yes, that'll be great.
Thanks,
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