Turns out virtio console tries to take a buffer out of an active vq.
Works by sheer luck, and is explicitly forbidden by spec. And while
going over it I saw that error handling is also broken -
failure is easy to trigger if I force allocations to fail.
Lightly tested.
Michael S. Tsirkin (6):
virtio_console: don't tie bufs to a vq
virtio: add ability to iterate over vqs
virtio_console: free buffers after reset
virtio_console: drop custom control queue cleanup
virtio_console: move removal code
virtio_console: reset on out of memory
drivers/char/virtio_console.c | 155 ++++++++++++++++++++----------------------
include/linux/virtio.h | 3 +
2 files changed, 75 insertions(+), 83 deletions(-)
--
MST
When out of memory and we can't add ctrl vq buffers,
probe fails. Unfortunately the error handling is
out of spec: it calls del_vqs without bothering
to reset the device first.
To fix, call the full cleanup function in this case.
Cc: [email protected]
Signed-off-by: Michael S. Tsirkin <[email protected]>
---
drivers/char/virtio_console.c | 17 ++++++++++-------
1 file changed, 10 insertions(+), 7 deletions(-)
diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index e8480fe..2108551 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -2090,6 +2090,7 @@ static int virtcons_probe(struct virtio_device *vdev)
spin_lock_init(&portdev->ports_lock);
INIT_LIST_HEAD(&portdev->ports);
+ INIT_LIST_HEAD(&portdev->list);
virtio_device_ready(portdev->vdev);
@@ -2107,8 +2108,15 @@ static int virtcons_probe(struct virtio_device *vdev)
if (!nr_added_bufs) {
dev_err(&vdev->dev,
"Error allocating buffers for control queue\n");
- err = -ENOMEM;
- goto free_vqs;
+ /*
+ * The host might want to notify mgmt sw about device
+ * add failure.
+ */
+ __send_control_msg(portdev, VIRTIO_CONSOLE_BAD_ID,
+ VIRTIO_CONSOLE_DEVICE_READY, 0);
+ /* Device was functional: we need full cleanup. */
+ virtcons_remove(vdev);
+ return -ENOMEM;
}
} else {
/*
@@ -2139,11 +2147,6 @@ static int virtcons_probe(struct virtio_device *vdev)
return 0;
-free_vqs:
- /* The host might want to notify mgmt sw about device add failure */
- __send_control_msg(portdev, VIRTIO_CONSOLE_BAD_ID,
- VIRTIO_CONSOLE_DEVICE_READY, 0);
- remove_vqs(portdev);
free_chrdev:
unregister_chrdev(portdev->chr_major, "virtio-portsdev");
free:
--
MST
Will make it reusable for error handling.
Signed-off-by: Michael S. Tsirkin <[email protected]>
---
drivers/char/virtio_console.c | 72 +++++++++++++++++++++----------------------
1 file changed, 36 insertions(+), 36 deletions(-)
diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 2d87ce5..e8480fe 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -1988,6 +1988,42 @@ static void remove_vqs(struct ports_device *portdev)
kfree(portdev->out_vqs);
}
+static void virtcons_remove(struct virtio_device *vdev)
+{
+ struct ports_device *portdev;
+ struct port *port, *port2;
+
+ portdev = vdev->priv;
+
+ spin_lock_irq(&pdrvdata_lock);
+ list_del(&portdev->list);
+ spin_unlock_irq(&pdrvdata_lock);
+
+ /* Disable interrupts for vqs */
+ vdev->config->reset(vdev);
+ /* Finish up work that's lined up */
+ if (use_multiport(portdev))
+ cancel_work_sync(&portdev->control_work);
+ else
+ cancel_work_sync(&portdev->config_work);
+
+ list_for_each_entry_safe(port, port2, &portdev->ports, list)
+ unplug_port(port);
+
+ unregister_chrdev(portdev->chr_major, "virtio-portsdev");
+
+ /*
+ * When yanking out a device, we immediately lose the
+ * (device-side) queues. So there's no point in keeping the
+ * guest side around till we drop our final reference. This
+ * also means that any ports which are in an open state will
+ * have to just stop using the port, as the vqs are going
+ * away.
+ */
+ remove_vqs(portdev);
+ kfree(portdev);
+}
+
/*
* Once we're further in boot, we get probed like any other virtio
* device.
@@ -2116,42 +2152,6 @@ static int virtcons_probe(struct virtio_device *vdev)
return err;
}
-static void virtcons_remove(struct virtio_device *vdev)
-{
- struct ports_device *portdev;
- struct port *port, *port2;
-
- portdev = vdev->priv;
-
- spin_lock_irq(&pdrvdata_lock);
- list_del(&portdev->list);
- spin_unlock_irq(&pdrvdata_lock);
-
- /* Disable interrupts for vqs */
- vdev->config->reset(vdev);
- /* Finish up work that's lined up */
- if (use_multiport(portdev))
- cancel_work_sync(&portdev->control_work);
- else
- cancel_work_sync(&portdev->config_work);
-
- list_for_each_entry_safe(port, port2, &portdev->ports, list)
- unplug_port(port);
-
- unregister_chrdev(portdev->chr_major, "virtio-portsdev");
-
- /*
- * When yanking out a device, we immediately lose the
- * (device-side) queues. So there's no point in keeping the
- * guest side around till we drop our final reference. This
- * also means that any ports which are in an open state will
- * have to just stop using the port, as the vqs are going
- * away.
- */
- remove_vqs(portdev);
- kfree(portdev);
-}
-
static struct virtio_device_id id_table[] = {
{ VIRTIO_ID_CONSOLE, VIRTIO_DEV_ANY_ID },
{ 0 },
--
MST
For cleanup it's helpful to be able to simply scan all vqs and discard
all data. Add an iterator to do that.
Signed-off-by: Michael S. Tsirkin <[email protected]>
---
include/linux/virtio.h | 3 +++
1 file changed, 3 insertions(+)
diff --git a/include/linux/virtio.h b/include/linux/virtio.h
index 988c735..fa1b5da 100644
--- a/include/linux/virtio.h
+++ b/include/linux/virtio.h
@@ -157,6 +157,9 @@ int virtio_device_freeze(struct virtio_device *dev);
int virtio_device_restore(struct virtio_device *dev);
#endif
+#define virtio_device_for_each_vq(vdev, vq) \
+ list_for_each_entry(vq, &vdev->vqs, list)
+
/**
* virtio_driver - operations for a virtio I/O driver
* @driver: underlying device driver (populate name and owner).
--
MST
Console driver is out of spec. The spec says:
A driver MUST NOT decrement the available idx on a live
virtqueue (ie. there is no way to “unexpose” buffers).
and it does exactly that by trying to detach unused buffers
without doing a device reset first.
Defer detaching the buffers until device unplug.
Of course this means we might get an interrupt for
a vq without an attached port now. Handle that by
discarding the consumed buffer.
Reported-by: Tiwei Bie <[email protected]>
Fixes: b3258ff1d6 ("virtio: Decrement avail idx on buffer detach")
CC: [email protected]
Signed-off-by: Michael S. Tsirkin <[email protected]>
---
drivers/char/virtio_console.c | 49 +++++++++++++++++++++----------------------
1 file changed, 24 insertions(+), 25 deletions(-)
diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 3e56f32..26a66ff 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -1402,7 +1402,6 @@ 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;
@@ -1513,8 +1512,6 @@ static int add_port(struct ports_device *portdev, u32 id)
return 0;
free_inbufs:
- while ((buf = virtqueue_detach_unused_buf(port->in_vq)))
- free_buf(buf, true);
free_device:
device_destroy(pdrvdata.class, port->dev->devt);
free_cdev:
@@ -1539,34 +1536,14 @@ static void remove_port(struct kref *kref)
static void remove_port_data(struct port *port)
{
- struct port_buffer *buf;
-
spin_lock_irq(&port->inbuf_lock);
/* Remove unused data this port might have received. */
discard_port_data(port);
spin_unlock_irq(&port->inbuf_lock);
- /* Remove buffers we queued up for the Host to send us data in. */
- do {
- spin_lock_irq(&port->inbuf_lock);
- buf = virtqueue_detach_unused_buf(port->in_vq);
- spin_unlock_irq(&port->inbuf_lock);
- if (buf)
- free_buf(buf, true);
- } while (buf);
-
spin_lock_irq(&port->outvq_lock);
reclaim_consumed_buffers(port);
spin_unlock_irq(&port->outvq_lock);
-
- /* Free pending buffers from the out-queue. */
- do {
- spin_lock_irq(&port->outvq_lock);
- buf = virtqueue_detach_unused_buf(port->out_vq);
- spin_unlock_irq(&port->outvq_lock);
- if (buf)
- free_buf(buf, true);
- } while (buf);
}
/*
@@ -1791,13 +1768,24 @@ static void control_work_handler(struct work_struct *work)
spin_unlock(&portdev->c_ivq_lock);
}
+static void flush_bufs(struct virtqueue *vq, bool can_sleep)
+{
+ struct port_buffer *buf;
+ unsigned int len;
+
+ while ((buf = virtqueue_get_buf(vq, &len)))
+ free_buf(buf, can_sleep);
+}
+
static void out_intr(struct virtqueue *vq)
{
struct port *port;
port = find_port_by_vq(vq->vdev->priv, vq);
- if (!port)
+ if (!port) {
+ flush_bufs(vq, false);
return;
+ }
wake_up_interruptible(&port->waitqueue);
}
@@ -1808,8 +1796,10 @@ static void in_intr(struct virtqueue *vq)
unsigned long flags;
port = find_port_by_vq(vq->vdev->priv, vq);
- if (!port)
+ if (!port) {
+ flush_bufs(vq, false);
return;
+ }
spin_lock_irqsave(&port->inbuf_lock, flags);
port->inbuf = get_inbuf(port);
@@ -1984,6 +1974,15 @@ static const struct file_operations portdev_fops = {
static void remove_vqs(struct ports_device *portdev)
{
+ struct virtqueue *vq;
+
+ virtio_device_for_each_vq(portdev->vdev, vq) {
+ struct port_buffer *buf;
+
+ flush_bufs(vq, true);
+ while ((buf = virtqueue_detach_unused_buf(vq)))
+ free_buf(buf, true);
+ }
portdev->vdev->config->del_vqs(portdev->vdev);
kfree(portdev->in_vqs);
kfree(portdev->out_vqs);
--
MST
an allocated buffer doesn't need to be tied to a vq -
only vq->vdev is ever used. Pass the function the
just what it needs - the vdev.
Signed-off-by: Michael S. Tsirkin <[email protected]>
---
drivers/char/virtio_console.c | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 468f061..3e56f32 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -422,7 +422,7 @@ static void reclaim_dma_bufs(void)
}
}
-static struct port_buffer *alloc_buf(struct virtqueue *vq, size_t buf_size,
+static struct port_buffer *alloc_buf(struct virtio_device *vdev, size_t buf_size,
int pages)
{
struct port_buffer *buf;
@@ -445,16 +445,16 @@ static struct port_buffer *alloc_buf(struct virtqueue *vq, size_t buf_size,
return buf;
}
- if (is_rproc_serial(vq->vdev)) {
+ if (is_rproc_serial(vdev)) {
/*
* Allocate DMA memory from ancestor. When a virtio
* device is created by remoteproc, the DMA memory is
* associated with the grandparent device:
* vdev => rproc => platform-dev.
*/
- if (!vq->vdev->dev.parent || !vq->vdev->dev.parent->parent)
+ if (!vdev->dev.parent || !vdev->dev.parent->parent)
goto free_buf;
- buf->dev = vq->vdev->dev.parent->parent;
+ buf->dev = vdev->dev.parent->parent;
/* Increase device refcnt to avoid freeing it */
get_device(buf->dev);
@@ -838,7 +838,7 @@ static ssize_t port_fops_write(struct file *filp, const char __user *ubuf,
count = min((size_t)(32 * 1024), count);
- buf = alloc_buf(port->out_vq, count, 0);
+ buf = alloc_buf(port->portdev->vdev, count, 0);
if (!buf)
return -ENOMEM;
@@ -957,7 +957,7 @@ static ssize_t port_fops_splice_write(struct pipe_inode_info *pipe,
if (ret < 0)
goto error_out;
- buf = alloc_buf(port->out_vq, 0, pipe->nrbufs);
+ buf = alloc_buf(port->portdev->vdev, 0, pipe->nrbufs);
if (!buf) {
ret = -ENOMEM;
goto error_out;
@@ -1374,7 +1374,7 @@ static unsigned int fill_queue(struct virtqueue *vq, spinlock_t *lock)
nr_added_bufs = 0;
do {
- buf = alloc_buf(vq, PAGE_SIZE, 0);
+ buf = alloc_buf(vq->vdev, PAGE_SIZE, 0);
if (!buf)
break;
--
MST
We now cleanup all VQs on device removal - no need
to handle the control VQ specially.
Signed-off-by: Michael S. Tsirkin <[email protected]>
---
drivers/char/virtio_console.c | 17 -----------------
1 file changed, 17 deletions(-)
diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 26a66ff..2d87ce5 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -1988,21 +1988,6 @@ static void remove_vqs(struct ports_device *portdev)
kfree(portdev->out_vqs);
}
-static void remove_controlq_data(struct ports_device *portdev)
-{
- struct port_buffer *buf;
- unsigned int len;
-
- if (!use_multiport(portdev))
- return;
-
- while ((buf = virtqueue_get_buf(portdev->c_ivq, &len)))
- free_buf(buf, true);
-
- while ((buf = virtqueue_detach_unused_buf(portdev->c_ivq)))
- free_buf(buf, true);
-}
-
/*
* Once we're further in boot, we get probed like any other virtio
* device.
@@ -2163,7 +2148,6 @@ static void virtcons_remove(struct virtio_device *vdev)
* have to just stop using the port, as the vqs are going
* away.
*/
- remove_controlq_data(portdev);
remove_vqs(portdev);
kfree(portdev);
}
@@ -2208,7 +2192,6 @@ static int virtcons_freeze(struct virtio_device *vdev)
*/
if (use_multiport(portdev))
virtqueue_disable_cb(portdev->c_ivq);
- remove_controlq_data(portdev);
list_for_each_entry(port, &portdev->ports, list) {
virtqueue_disable_cb(port->in_vq);
--
MST
On Fri, Apr 20, 2018 at 09:18:01PM +0300, Michael S. Tsirkin wrote:
> an allocated buffer doesn't need to be tied to a vq -
> only vq->vdev is ever used. Pass the function the
> just what it needs - the vdev.
>
> Signed-off-by: Michael S. Tsirkin <[email protected]>
> ---
> drivers/char/virtio_console.c | 14 +++++++-------
> 1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
> index 468f061..3e56f32 100644
> --- a/drivers/char/virtio_console.c
> +++ b/drivers/char/virtio_console.c
> @@ -422,7 +422,7 @@ static void reclaim_dma_bufs(void)
> }
> }
>
> -static struct port_buffer *alloc_buf(struct virtqueue *vq, size_t buf_size,
> +static struct port_buffer *alloc_buf(struct virtio_device *vdev, size_t buf_size,
> int pages)
> {
> struct port_buffer *buf;
> @@ -445,16 +445,16 @@ static struct port_buffer *alloc_buf(struct virtqueue *vq, size_t buf_size,
> return buf;
> }
>
> - if (is_rproc_serial(vq->vdev)) {
> + if (is_rproc_serial(vdev)) {
> /*
> * Allocate DMA memory from ancestor. When a virtio
> * device is created by remoteproc, the DMA memory is
> * associated with the grandparent device:
> * vdev => rproc => platform-dev.
> */
> - if (!vq->vdev->dev.parent || !vq->vdev->dev.parent->parent)
> + if (!vdev->dev.parent || !vdev->dev.parent->parent)
> goto free_buf;
> - buf->dev = vq->vdev->dev.parent->parent;
> + buf->dev = vdev->dev.parent->parent;
>
> /* Increase device refcnt to avoid freeing it */
> get_device(buf->dev);
> @@ -838,7 +838,7 @@ static ssize_t port_fops_write(struct file *filp, const char __user *ubuf,
>
> count = min((size_t)(32 * 1024), count);
>
> - buf = alloc_buf(port->out_vq, count, 0);
> + buf = alloc_buf(port->portdev->vdev, count, 0);
> if (!buf)
> return -ENOMEM;
>
> @@ -957,7 +957,7 @@ static ssize_t port_fops_splice_write(struct pipe_inode_info *pipe,
> if (ret < 0)
> goto error_out;
>
> - buf = alloc_buf(port->out_vq, 0, pipe->nrbufs);
> + buf = alloc_buf(port->portdev->vdev, 0, pipe->nrbufs);
> if (!buf) {
> ret = -ENOMEM;
> goto error_out;
> @@ -1374,7 +1374,7 @@ static unsigned int fill_queue(struct virtqueue *vq, spinlock_t *lock)
>
> nr_added_bufs = 0;
> do {
> - buf = alloc_buf(vq, PAGE_SIZE, 0);
> + buf = alloc_buf(vq->vdev, PAGE_SIZE, 0);
> if (!buf)
> break;
>
> --
> MST
<formletter>
This is not the correct way to submit patches for inclusion in the
stable kernel tree. Please read:
https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
for how to do this properly.
</formletter>
On 2018年04月21日 02:18, Michael S. Tsirkin wrote:
> Console driver is out of spec. The spec says:
> A driver MUST NOT decrement the available idx on a live
> virtqueue (ie. there is no way to “unexpose” buffers).
> and it does exactly that by trying to detach unused buffers
> without doing a device reset first.
>
> Defer detaching the buffers until device unplug.
>
> Of course this means we might get an interrupt for
> a vq without an attached port now. Handle that by
> discarding the consumed buffer.
>
> Reported-by: Tiwei Bie <[email protected]>
> Fixes: b3258ff1d6 ("virtio: Decrement avail idx on buffer detach")
> CC: [email protected]
> Signed-off-by: Michael S. Tsirkin <[email protected]>
I wonder whether or not we can have some BUG_ON() in
virtqueue_detach_unused_buf() to detect such bugs (e.g by checking status?).
Thanks
> ---
> drivers/char/virtio_console.c | 49 +++++++++++++++++++++----------------------
> 1 file changed, 24 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
> index 3e56f32..26a66ff 100644
> --- a/drivers/char/virtio_console.c
> +++ b/drivers/char/virtio_console.c
> @@ -1402,7 +1402,6 @@ 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;
> @@ -1513,8 +1512,6 @@ static int add_port(struct ports_device *portdev, u32 id)
> return 0;
>
> free_inbufs:
> - while ((buf = virtqueue_detach_unused_buf(port->in_vq)))
> - free_buf(buf, true);
> free_device:
> device_destroy(pdrvdata.class, port->dev->devt);
> free_cdev:
> @@ -1539,34 +1536,14 @@ static void remove_port(struct kref *kref)
>
> static void remove_port_data(struct port *port)
> {
> - struct port_buffer *buf;
> -
> spin_lock_irq(&port->inbuf_lock);
> /* Remove unused data this port might have received. */
> discard_port_data(port);
> spin_unlock_irq(&port->inbuf_lock);
>
> - /* Remove buffers we queued up for the Host to send us data in. */
> - do {
> - spin_lock_irq(&port->inbuf_lock);
> - buf = virtqueue_detach_unused_buf(port->in_vq);
> - spin_unlock_irq(&port->inbuf_lock);
> - if (buf)
> - free_buf(buf, true);
> - } while (buf);
> -
> spin_lock_irq(&port->outvq_lock);
> reclaim_consumed_buffers(port);
> spin_unlock_irq(&port->outvq_lock);
> -
> - /* Free pending buffers from the out-queue. */
> - do {
> - spin_lock_irq(&port->outvq_lock);
> - buf = virtqueue_detach_unused_buf(port->out_vq);
> - spin_unlock_irq(&port->outvq_lock);
> - if (buf)
> - free_buf(buf, true);
> - } while (buf);
> }
>
> /*
> @@ -1791,13 +1768,24 @@ static void control_work_handler(struct work_struct *work)
> spin_unlock(&portdev->c_ivq_lock);
> }
>
> +static void flush_bufs(struct virtqueue *vq, bool can_sleep)
> +{
> + struct port_buffer *buf;
> + unsigned int len;
> +
> + while ((buf = virtqueue_get_buf(vq, &len)))
> + free_buf(buf, can_sleep);
> +}
> +
> static void out_intr(struct virtqueue *vq)
> {
> struct port *port;
>
> port = find_port_by_vq(vq->vdev->priv, vq);
> - if (!port)
> + if (!port) {
> + flush_bufs(vq, false);
> return;
> + }
>
> wake_up_interruptible(&port->waitqueue);
> }
> @@ -1808,8 +1796,10 @@ static void in_intr(struct virtqueue *vq)
> unsigned long flags;
>
> port = find_port_by_vq(vq->vdev->priv, vq);
> - if (!port)
> + if (!port) {
> + flush_bufs(vq, false);
> return;
> + }
>
> spin_lock_irqsave(&port->inbuf_lock, flags);
> port->inbuf = get_inbuf(port);
> @@ -1984,6 +1974,15 @@ static const struct file_operations portdev_fops = {
>
> static void remove_vqs(struct ports_device *portdev)
> {
> + struct virtqueue *vq;
> +
> + virtio_device_for_each_vq(portdev->vdev, vq) {
> + struct port_buffer *buf;
> +
> + flush_bufs(vq, true);
> + while ((buf = virtqueue_detach_unused_buf(vq)))
> + free_buf(buf, true);
> + }
> portdev->vdev->config->del_vqs(portdev->vdev);
> kfree(portdev->in_vqs);
> kfree(portdev->out_vqs);
On Fri, Apr 20, 2018 at 09:17:59PM +0300, Michael S. Tsirkin wrote:
> Turns out virtio console tries to take a buffer out of an active vq.
> Works by sheer luck, and is explicitly forbidden by spec. And while
> going over it I saw that error handling is also broken -
> failure is easy to trigger if I force allocations to fail.
>
> Lightly tested.
Amit - any feedback before I push these patches?
> Michael S. Tsirkin (6):
> virtio_console: don't tie bufs to a vq
> virtio: add ability to iterate over vqs
> virtio_console: free buffers after reset
> virtio_console: drop custom control queue cleanup
> virtio_console: move removal code
> virtio_console: reset on out of memory
>
> drivers/char/virtio_console.c | 155 ++++++++++++++++++++----------------------
> include/linux/virtio.h | 3 +
> 2 files changed, 75 insertions(+), 83 deletions(-)
>
> --
> MST
>
On Sat, Apr 21, 2018 at 09:30:05AM +0200, Greg Kroah-Hartman wrote:
> On Fri, Apr 20, 2018 at 09:18:01PM +0300, Michael S. Tsirkin wrote:
> > an allocated buffer doesn't need to be tied to a vq -
> > only vq->vdev is ever used. Pass the function the
> > just what it needs - the vdev.
> >
> > Signed-off-by: Michael S. Tsirkin <[email protected]>
> > ---
> > drivers/char/virtio_console.c | 14 +++++++-------
> > 1 file changed, 7 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
> > index 468f061..3e56f32 100644
> > --- a/drivers/char/virtio_console.c
> > +++ b/drivers/char/virtio_console.c
> > @@ -422,7 +422,7 @@ static void reclaim_dma_bufs(void)
> > }
> > }
> >
> > -static struct port_buffer *alloc_buf(struct virtqueue *vq, size_t buf_size,
> > +static struct port_buffer *alloc_buf(struct virtio_device *vdev, size_t buf_size,
> > int pages)
> > {
> > struct port_buffer *buf;
> > @@ -445,16 +445,16 @@ static struct port_buffer *alloc_buf(struct virtqueue *vq, size_t buf_size,
> > return buf;
> > }
> >
> > - if (is_rproc_serial(vq->vdev)) {
> > + if (is_rproc_serial(vdev)) {
> > /*
> > * Allocate DMA memory from ancestor. When a virtio
> > * device is created by remoteproc, the DMA memory is
> > * associated with the grandparent device:
> > * vdev => rproc => platform-dev.
> > */
> > - if (!vq->vdev->dev.parent || !vq->vdev->dev.parent->parent)
> > + if (!vdev->dev.parent || !vdev->dev.parent->parent)
> > goto free_buf;
> > - buf->dev = vq->vdev->dev.parent->parent;
> > + buf->dev = vdev->dev.parent->parent;
> >
> > /* Increase device refcnt to avoid freeing it */
> > get_device(buf->dev);
> > @@ -838,7 +838,7 @@ static ssize_t port_fops_write(struct file *filp, const char __user *ubuf,
> >
> > count = min((size_t)(32 * 1024), count);
> >
> > - buf = alloc_buf(port->out_vq, count, 0);
> > + buf = alloc_buf(port->portdev->vdev, count, 0);
> > if (!buf)
> > return -ENOMEM;
> >
> > @@ -957,7 +957,7 @@ static ssize_t port_fops_splice_write(struct pipe_inode_info *pipe,
> > if (ret < 0)
> > goto error_out;
> >
> > - buf = alloc_buf(port->out_vq, 0, pipe->nrbufs);
> > + buf = alloc_buf(port->portdev->vdev, 0, pipe->nrbufs);
> > if (!buf) {
> > ret = -ENOMEM;
> > goto error_out;
> > @@ -1374,7 +1374,7 @@ static unsigned int fill_queue(struct virtqueue *vq, spinlock_t *lock)
> >
> > nr_added_bufs = 0;
> > do {
> > - buf = alloc_buf(vq, PAGE_SIZE, 0);
> > + buf = alloc_buf(vq->vdev, PAGE_SIZE, 0);
> > if (!buf)
> > break;
> >
> > --
> > MST
>
> <formletter>
>
> This is not the correct way to submit patches for inclusion in the
> stable kernel tree. Please read:
> https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
> for how to do this properly.
>
> </formletter>
Thanks!
I have some questions about this one:
Cc: <[email protected]> # 3.3.x: a1f84a3: sched: Check for idle
Cc: <[email protected]> # 3.3.x: 1b9508f: sched: Rate-limit newidle
Cc: <[email protected]> # 3.3.x: fd21073: sched: Fix affinity logic
Cc: <[email protected]> # 3.3.x
Signed-off-by: Ingo Molnar <[email protected]>
1. what does the kernel version mean? can I omit it?
2. so when I rebase to add the tag, this changes commit IDs for
following tags in the same tree, breaking their tags
in the process. Pretty annoying. Any idea how to do it better?
Thanks!
--
MST
On Tue, Apr 24, 2018 at 09:56:33PM +0300, Michael S. Tsirkin wrote:
> On Sat, Apr 21, 2018 at 09:30:05AM +0200, Greg Kroah-Hartman wrote:
> > On Fri, Apr 20, 2018 at 09:18:01PM +0300, Michael S. Tsirkin wrote:
> > > an allocated buffer doesn't need to be tied to a vq -
> > > only vq->vdev is ever used. Pass the function the
> > > just what it needs - the vdev.
> > >
> > > Signed-off-by: Michael S. Tsirkin <[email protected]>
> > > ---
> > > drivers/char/virtio_console.c | 14 +++++++-------
> > > 1 file changed, 7 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
> > > index 468f061..3e56f32 100644
> > > --- a/drivers/char/virtio_console.c
> > > +++ b/drivers/char/virtio_console.c
> > > @@ -422,7 +422,7 @@ static void reclaim_dma_bufs(void)
> > > }
> > > }
> > >
> > > -static struct port_buffer *alloc_buf(struct virtqueue *vq, size_t buf_size,
> > > +static struct port_buffer *alloc_buf(struct virtio_device *vdev, size_t buf_size,
> > > int pages)
> > > {
> > > struct port_buffer *buf;
> > > @@ -445,16 +445,16 @@ static struct port_buffer *alloc_buf(struct virtqueue *vq, size_t buf_size,
> > > return buf;
> > > }
> > >
> > > - if (is_rproc_serial(vq->vdev)) {
> > > + if (is_rproc_serial(vdev)) {
> > > /*
> > > * Allocate DMA memory from ancestor. When a virtio
> > > * device is created by remoteproc, the DMA memory is
> > > * associated with the grandparent device:
> > > * vdev => rproc => platform-dev.
> > > */
> > > - if (!vq->vdev->dev.parent || !vq->vdev->dev.parent->parent)
> > > + if (!vdev->dev.parent || !vdev->dev.parent->parent)
> > > goto free_buf;
> > > - buf->dev = vq->vdev->dev.parent->parent;
> > > + buf->dev = vdev->dev.parent->parent;
> > >
> > > /* Increase device refcnt to avoid freeing it */
> > > get_device(buf->dev);
> > > @@ -838,7 +838,7 @@ static ssize_t port_fops_write(struct file *filp, const char __user *ubuf,
> > >
> > > count = min((size_t)(32 * 1024), count);
> > >
> > > - buf = alloc_buf(port->out_vq, count, 0);
> > > + buf = alloc_buf(port->portdev->vdev, count, 0);
> > > if (!buf)
> > > return -ENOMEM;
> > >
> > > @@ -957,7 +957,7 @@ static ssize_t port_fops_splice_write(struct pipe_inode_info *pipe,
> > > if (ret < 0)
> > > goto error_out;
> > >
> > > - buf = alloc_buf(port->out_vq, 0, pipe->nrbufs);
> > > + buf = alloc_buf(port->portdev->vdev, 0, pipe->nrbufs);
> > > if (!buf) {
> > > ret = -ENOMEM;
> > > goto error_out;
> > > @@ -1374,7 +1374,7 @@ static unsigned int fill_queue(struct virtqueue *vq, spinlock_t *lock)
> > >
> > > nr_added_bufs = 0;
> > > do {
> > > - buf = alloc_buf(vq, PAGE_SIZE, 0);
> > > + buf = alloc_buf(vq->vdev, PAGE_SIZE, 0);
> > > if (!buf)
> > > break;
> > >
> > > --
> > > MST
> >
> > <formletter>
> >
> > This is not the correct way to submit patches for inclusion in the
> > stable kernel tree. Please read:
> > https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
> > for how to do this properly.
> >
> > </formletter>
>
>
> Thanks!
> I have some questions about this one:
>
> Cc: <[email protected]> # 3.3.x: a1f84a3: sched: Check for idle
> Cc: <[email protected]> # 3.3.x: 1b9508f: sched: Rate-limit newidle
> Cc: <[email protected]> # 3.3.x: fd21073: sched: Fix affinity logic
> Cc: <[email protected]> # 3.3.x
> Signed-off-by: Ingo Molnar <[email protected]>
>
> 1. what does the kernel version mean? can I omit it?
Did you read the document?, it explains that the version can be used to
say "this kernel version and newer"
> 2. so when I rebase to add the tag, this changes commit IDs for
> following tags in the same tree, breaking their tags
> in the process. Pretty annoying. Any idea how to do it better?
You only put tags there if you want me to pick up pre-requisite patches
that are already in Linus's tree. If you have a patch series that all
needs to go into stable, just add the "cc: stable@" to the tags on all
of them and I'll pick them up in the correct order then.
hope this helps,
greg k-h
On 24 April 2018 11:41:29 AM GMT-07:00, "Michael S. Tsirkin" <[email protected]> wrote:
>On Fri, Apr 20, 2018 at 09:17:59PM +0300, Michael S. Tsirkin wrote:
>> Turns out virtio console tries to take a buffer out of an active vq.
>> Works by sheer luck, and is explicitly forbidden by spec. And while
>> going over it I saw that error handling is also broken -
>> failure is easy to trigger if I force allocations to fail.
>>
>> Lightly tested.
>
>Amit - any feedback before I push these patches?
I'm traveling this week, will be able to take a look early next week.
Amit
--
http://amitshah.net
On (Tue) 24 Apr 2018 [21:41:29], Michael S. Tsirkin wrote:
> On Fri, Apr 20, 2018 at 09:17:59PM +0300, Michael S. Tsirkin wrote:
> > Turns out virtio console tries to take a buffer out of an active vq.
> > Works by sheer luck, and is explicitly forbidden by spec. And while
> > going over it I saw that error handling is also broken -
> > failure is easy to trigger if I force allocations to fail.
> >
> > Lightly tested.
>
> Amit - any feedback before I push these patches?
The changes look good spec-wise.
There are a bunch of tests in avocado-vt that test virtio-console
functionality. Can you give those a try before pushing?
Amit
--
http://amitshah.net/
(apologies if you received a dup)
On (Tue) 24 Apr 2018 [21:41:29], Michael S. Tsirkin wrote:
> On Fri, Apr 20, 2018 at 09:17:59PM +0300, Michael S. Tsirkin wrote:
> > Turns out virtio console tries to take a buffer out of an active vq.
> > Works by sheer luck, and is explicitly forbidden by spec. And while
> > going over it I saw that error handling is also broken -
> > failure is easy to trigger if I force allocations to fail.
> >
> > Lightly tested.
>
> Amit - any feedback before I push these patches?
The changes look good spec-wise.
There are a bunch of tests in avocado-vt that test virtio-console
functionality. Can you give those a try before pushing?
Amit
--
http://amitshah.net/
On Thu, May 03, 2018 at 05:45:29AM +0200, Amit Shah wrote:
> (apologies if you received a dup)
>
> On (Tue) 24 Apr 2018 [21:41:29], Michael S. Tsirkin wrote:
> > On Fri, Apr 20, 2018 at 09:17:59PM +0300, Michael S. Tsirkin wrote:
> > > Turns out virtio console tries to take a buffer out of an active vq.
> > > Works by sheer luck, and is explicitly forbidden by spec. And while
> > > going over it I saw that error handling is also broken -
> > > failure is easy to trigger if I force allocations to fail.
> > >
> > > Lightly tested.
> >
> > Amit - any feedback before I push these patches?
>
> The changes look good spec-wise.
>
> There are a bunch of tests in avocado-vt that test virtio-console
> functionality. Can you give those a try before pushing?
>
> Amit
I pushed before I did that test, will try to find the time later. BTW
do you still want to be on the maintainers list?
> --
> http://amitshah.net/
On (Thu) 03 May 2018 [22:28:32], Michael S. Tsirkin wrote:
> On Thu, May 03, 2018 at 05:45:29AM +0200, Amit Shah wrote:
> > (apologies if you received a dup)
> >
> > On (Tue) 24 Apr 2018 [21:41:29], Michael S. Tsirkin wrote:
> > > On Fri, Apr 20, 2018 at 09:17:59PM +0300, Michael S. Tsirkin wrote:
> > > > Turns out virtio console tries to take a buffer out of an active vq.
> > > > Works by sheer luck, and is explicitly forbidden by spec. And while
> > > > going over it I saw that error handling is also broken -
> > > > failure is easy to trigger if I force allocations to fail.
> > > >
> > > > Lightly tested.
> > >
> > > Amit - any feedback before I push these patches?
> >
> > The changes look good spec-wise.
> >
> > There are a bunch of tests in avocado-vt that test virtio-console
> > functionality. Can you give those a try before pushing?
> >
> > Amit
>
> I pushed before I did that test, will try to find the time later. BTW
> do you still want to be on the maintainers list?
Yes, I want to be on the maintainers list; but won't mind
co-maintainers. The recent changes seem to be spec-related, and I'd
expect you to have a good handle on that anyway.
Amit
--
http://amitshah.net/
On (Thu) 03 May 2018 [22:28:32], Michael S. Tsirkin wrote:
> On Thu, May 03, 2018 at 05:45:29AM +0200, Amit Shah wrote:
> > (apologies if you received a dup)
> >
> > On (Tue) 24 Apr 2018 [21:41:29], Michael S. Tsirkin wrote:
> > > On Fri, Apr 20, 2018 at 09:17:59PM +0300, Michael S. Tsirkin wrote:
> > > > Turns out virtio console tries to take a buffer out of an active vq.
> > > > Works by sheer luck, and is explicitly forbidden by spec. And while
> > > > going over it I saw that error handling is also broken -
> > > > failure is easy to trigger if I force allocations to fail.
> > > >
> > > > Lightly tested.
> > >
> > > Amit - any feedback before I push these patches?
> >
> > The changes look good spec-wise.
> >
> > There are a bunch of tests in avocado-vt that test virtio-console
> > functionality. Can you give those a try before pushing?
> >
> > Amit
>
> I pushed before I did that test, will try to find the time later. BTW
> do you still want to be on the maintainers list?
Yes, I want to be on the maintainers list; but won't mind
co-maintainers. The recent changes seem to be spec-related, and I'd
expect you to have a good handle on that anyway.
Amit
--
http://amitshah.net/
On Sun, May 06, 2018 at 08:24:30PM +0200, Amit Shah wrote:
> On (Thu) 03 May 2018 [22:28:32], Michael S. Tsirkin wrote:
> > On Thu, May 03, 2018 at 05:45:29AM +0200, Amit Shah wrote:
> > > (apologies if you received a dup)
> > >
> > > On (Tue) 24 Apr 2018 [21:41:29], Michael S. Tsirkin wrote:
> > > > On Fri, Apr 20, 2018 at 09:17:59PM +0300, Michael S. Tsirkin wrote:
> > > > > Turns out virtio console tries to take a buffer out of an active vq.
> > > > > Works by sheer luck, and is explicitly forbidden by spec. And while
> > > > > going over it I saw that error handling is also broken -
> > > > > failure is easy to trigger if I force allocations to fail.
> > > > >
> > > > > Lightly tested.
> > > >
> > > > Amit - any feedback before I push these patches?
> > >
> > > The changes look good spec-wise.
> > >
> > > There are a bunch of tests in avocado-vt that test virtio-console
> > > functionality. Can you give those a try before pushing?
> > >
> > > Amit
> >
> > I pushed before I did that test, will try to find the time later. BTW
> > do you still want to be on the maintainers list?
>
> Yes, I want to be on the maintainers list; but won't mind
> co-maintainers. The recent changes seem to be spec-related, and I'd
> expect you to have a good handle on that anyway.
>
> Amit
If you can do extra testing that would be appreciated though.
> --
> http://amitshah.net/