[RFC 1/2] virtio_console: Add support for DMA memory allocation

[RFC 1/2] virtio_console: Add support for DMA memory allocation

[sjur.brandeland]

From: Sjur Brændeland <sjur.brandeland@stericsson.com>

Add feature VIRTIO_CONSOLE_F_DMA_MEM. If the architecture has
DMA support and this feature bit is set, the virtio data buffers
will be allocated from DMA memory.

This is needed for using virtio_console from the remoteproc
framework.

Signed-off-by: Sjur Brændeland <sjur.brandeland@stericsson.com>
cc: Rusty Russell <rusty@rustcorp.com.au>
cc: Michael S. Tsirkin <mst@redhat.com>
cc: Ohad Ben-Cohen <ohad@wizery.com>
cc: Linus Walleij <linus.walleij@linaro.org>
cc: virtualization@lists.linux-foundation.org
---
 drivers/char/virtio_console.c  |   76 ++++++++++++++++++++++++++++++++--------
 include/linux/virtio_console.h |    1 +
 2 files changed, 62 insertions(+), 15 deletions(-)

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index cdf2f54..6bfbd09 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -35,6 +35,7 @@
 #include <linux/wait.h>
 #include <linux/workqueue.h>
 #include <linux/module.h>
+#include <linux/dma-mapping.h>
 #include "../tty/hvc/hvc_console.h"
 
 /*
@@ -334,20 +335,60 @@ static inline bool use_multiport(struct ports_device *portdev)
 	return portdev->vdev->features[0] & (1 << VIRTIO_CONSOLE_F_MULTIPORT);
 }
 
-static void free_buf(struct port_buffer *buf)
+/* Allcoate data buffer from DMA memory if requested */
+static inline void *
+alloc_databuf(struct virtio_device *vdev, size_t size, dma_addr_t *dma_handle,
+		   gfp_t flag)
 {
-	kfree(buf->buf);
+#ifdef CONFIG_HAS_DMA
+	if (virtio_has_feature(vdev, VIRTIO_CONSOLE_F_DMA_MEM)) {
+		struct device *dev = &vdev->dev;
+		/*
+		 * Allocate DMA memory from ancestors. Finding the ancestor
+		 * is a bit quirky when DMA_MEMORY_INCLUDES_CHILDREN is not
+		 * implemented.
+		 */
+		dev = dev->parent ? dev->parent : dev;
+		dev = dev->parent ? dev->parent : dev;
+		return dma_alloc_coherent(dev, size, dma_handle, flag);
+	}
+#endif
+	return kzalloc(size, flag);
+}
+
+static inline void
+free_databuf(struct virtio_device *vdev, size_t size, void *cpu_addr)
+{
+#ifdef CONFIG_HAS_DMA
+	dma_addr_t dma_handle = virt_to_bus(cpu_addr);
+	if (virtio_has_feature(vdev, VIRTIO_CONSOLE_F_DMA_MEM)) {
+		struct device *dev = &vdev->dev;
+
+		dev = dev->parent ? dev->parent : dev;
+		dev = dev->parent ? dev->parent : dev;
+		dma_free_coherent(dev, size, cpu_addr, dma_handle);
+		return;
+	}
+#endif
+	kfree(cpu_addr);
+}
+
+static void
+free_buf(struct virtqueue *vq, struct port_buffer *buf, size_t buf_size)
+{
+	free_databuf(vq->vdev, buf_size, buf);
 	kfree(buf);
 }
 
-static struct port_buffer *alloc_buf(size_t buf_size)
+static struct port_buffer *alloc_buf(struct virtqueue *vq, size_t buf_size)
 {
 	struct port_buffer *buf;
+	dma_addr_t dma;
 
 	buf = kmalloc(sizeof(*buf), GFP_KERNEL);
 	if (!buf)
 		goto fail;
-	buf->buf = kzalloc(buf_size, GFP_KERNEL);
+	buf->buf = alloc_databuf(vq->vdev, buf_size, &dma, GFP_KERNEL);
 	if (!buf->buf)
 		goto free_buf;
 	buf->len = 0;
@@ -414,7 +455,7 @@ static void discard_port_data(struct port *port)
 		port->stats.bytes_discarded += buf->len - buf->offset;
 		if (add_inbuf(port->in_vq, buf) < 0) {
 			err++;
-			free_buf(buf);
+			free_buf(port->in_vq, buf, PAGE_SIZE);
 		}
 		port->inbuf = NULL;
 		buf = get_inbuf(port);
@@ -485,7 +526,7 @@ static void reclaim_consumed_buffers(struct port *port)
 		return;
 	}
 	while ((buf = virtqueue_get_buf(port->out_vq, &len))) {
-		kfree(buf);
+		free_databuf(port->portdev->vdev, len, buf);
 		port->outvq_full = false;
 	}
 }
@@ -672,6 +713,8 @@ static ssize_t port_fops_write(struct file *filp, const char __user *ubuf,
 	char *buf;
 	ssize_t ret;
 	bool nonblock;
+	struct virtio_device *vdev;
+	dma_addr_t dma;
 
 	/* Userspace could be out to fool us */
 	if (!count)
@@ -694,9 +737,10 @@ static ssize_t port_fops_write(struct file *filp, const char __user *ubuf,
 	if (!port->guest_connected)
 		return -ENODEV;
 
+	vdev = port->portdev->vdev;
 	count = min((size_t)(32 * 1024), count);
 
-	buf = kmalloc(count, GFP_KERNEL);
+	buf = alloc_databuf(vdev, count, &dma, GFP_KERNEL);
 	if (!buf)
 		return -ENOMEM;
 
@@ -720,7 +764,8 @@ static ssize_t port_fops_write(struct file *filp, const char __user *ubuf,
 		goto out;
 
 free_buf:
-	kfree(buf);
+	free_databuf(vdev, count, buf);
+
 out:
 	return ret;
 }
@@ -1102,7 +1147,7 @@ static unsigned int fill_queue(struct virtqueue *vq, spinlock_t *lock)
 
 	nr_added_bufs = 0;
 	do {
-		buf = alloc_buf(PAGE_SIZE);
+		buf = alloc_buf(vq, PAGE_SIZE);
 		if (!buf)
 			break;
 
@@ -1110,7 +1155,7 @@ static unsigned int fill_queue(struct virtqueue *vq, spinlock_t *lock)
 		ret = add_inbuf(vq, buf);
 		if (ret < 0) {
 			spin_unlock_irq(lock);
-			free_buf(buf);
+			free_buf(vq, buf, PAGE_SIZE);
 			break;
 		}
 		nr_added_bufs++;
@@ -1234,7 +1279,7 @@ static int add_port(struct ports_device *portdev, u32 id)
 
 free_inbufs:
 	while ((buf = virtqueue_detach_unused_buf(port->in_vq)))
-		free_buf(buf);
+		free_buf(port->in_vq, buf, PAGE_SIZE);
 free_device:
 	device_destroy(pdrvdata.class, port->dev->devt);
 free_cdev:
@@ -1276,7 +1321,7 @@ static void remove_port_data(struct port *port)
 
 	/* Remove buffers we queued up for the Host to send us data in. */
 	while ((buf = virtqueue_detach_unused_buf(port->in_vq)))
-		free_buf(buf);
+		free_buf(port->in_vq, buf, PAGE_SIZE);
 }
 
 /*
@@ -1478,7 +1523,7 @@ static void control_work_handler(struct work_struct *work)
 		if (add_inbuf(portdev->c_ivq, buf) < 0) {
 			dev_warn(&portdev->vdev->dev,
 				 "Error adding buffer to queue\n");
-			free_buf(buf);
+			free_buf(portdev->c_ivq, buf, PAGE_SIZE);
 		}
 	}
 	spin_unlock(&portdev->cvq_lock);
@@ -1674,10 +1719,10 @@ static void remove_controlq_data(struct ports_device *portdev)
 		return;
 
 	while ((buf = virtqueue_get_buf(portdev->c_ivq, &len)))
-		free_buf(buf);
+		free_buf(portdev->c_ivq, buf, PAGE_SIZE);
 
 	while ((buf = virtqueue_detach_unused_buf(portdev->c_ivq)))
-		free_buf(buf);
+		free_buf(portdev->c_ivq, buf, PAGE_SIZE);
 }
 
 /*
@@ -1836,6 +1881,7 @@ static struct virtio_device_id id_table[] = {
 static unsigned int features[] = {
 	VIRTIO_CONSOLE_F_SIZE,
 	VIRTIO_CONSOLE_F_MULTIPORT,
+	VIRTIO_CONSOLE_F_DMA_MEM,
 };
 
 #ifdef CONFIG_PM
diff --git a/include/linux/virtio_console.h b/include/linux/virtio_console.h
index bdf4b00..b27f7fa 100644
--- a/include/linux/virtio_console.h
+++ b/include/linux/virtio_console.h
@@ -38,6 +38,7 @@
 /* Feature bits */
 #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_F_DMA_MEM 2	/* Use DMA memory in vrings */
 
 #define VIRTIO_CONSOLE_BAD_ID		(~(u32)0)
 
-- 
1.7.5.4

[RFC 2/2] virtio_console: Add feature to disable console port

[sjur.brandeland]

From: Sjur Brændeland <sjur.brandeland@stericsson.com>

Add the feature VIRTIO_CONSOLE_F_NO_HVC. With this bit set
only port-devices are created. The console port and
port control virtio-queues are not created.

The console port is not suited for communicating
to a remote processor because of it's blocking behavior.
But the port-device supports efficient non-blocking IO
to a remote processor.

Signed-off-by: Sjur Brændeland <sjur.brandeland@stericsson.com>
cc: Rusty Russell <rusty@rustcorp.com.au>
cc: Michael S. Tsirkin <mst@redhat.com>
cc: virtualization@lists.linux-foundation.org
cc: Ohad Ben-Cohen <ohad@wizery.com>
cc: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/char/virtio_console.c  |    5 ++++-
 include/linux/virtio_console.h |    1 +
 2 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 6bfbd09..81546e9 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -1246,7 +1246,9 @@ static int add_port(struct ports_device *portdev, u32 id)
 	/*
 	 * If we're not using multiport support, this has to be a console port
 	 */
-	if (!use_multiport(port->portdev)) {
+	if (virtio_has_feature(port->portdev->vdev, VIRTIO_CONSOLE_F_NO_HVC))
+		port->host_connected = true;
+	else if (!use_multiport(port->portdev)) {
 		err = init_port_console(port);
 		if (err)
 			goto free_inbufs;
@@ -1882,6 +1884,7 @@ static unsigned int features[] = {
 	VIRTIO_CONSOLE_F_SIZE,
 	VIRTIO_CONSOLE_F_MULTIPORT,
 	VIRTIO_CONSOLE_F_DMA_MEM,
+	VIRTIO_CONSOLE_F_NO_HVC,
 };
 
 #ifdef CONFIG_PM
diff --git a/include/linux/virtio_console.h b/include/linux/virtio_console.h
index b27f7fa..a7c8974 100644
--- a/include/linux/virtio_console.h
+++ b/include/linux/virtio_console.h
@@ -39,6 +39,7 @@
 #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_F_DMA_MEM 2	/* Use DMA memory in vrings */
+#define VIRTIO_CONSOLE_F_NO_HVC 3	/* Disable use of HVC */
 
 #define VIRTIO_CONSOLE_BAD_ID		(~(u32)0)
 
-- 
1.7.5.4

Re: [RFC 1/2] virtio_console: Add support for DMA memory allocation

[Michael S. Tsirkin]

On Mon, Sep 03, 2012 at 03:51:16PM +0200, sjur.brandeland@stericsson.com wrote:
> From: Sjur Brændeland <sjur.brandeland@stericsson.com>
> 
> Add feature VIRTIO_CONSOLE_F_DMA_MEM. If the architecture has
> DMA support and this feature bit is set, the virtio data buffers
> will be allocated from DMA memory.
> 
> This is needed for using virtio_console from the remoteproc
> framework.
> 
> Signed-off-by: Sjur Brændeland <sjur.brandeland@stericsson.com>
> cc: Rusty Russell <rusty@rustcorp.com.au>
> cc: Michael S. Tsirkin <mst@redhat.com>
> cc: Ohad Ben-Cohen <ohad@wizery.com>
> cc: Linus Walleij <linus.walleij@linaro.org>
> cc: virtualization@lists.linux-foundation.org

How does access to descriptors work in this setup?

> ---
>  drivers/char/virtio_console.c  |   76 ++++++++++++++++++++++++++++++++--------
>  include/linux/virtio_console.h |    1 +
>  2 files changed, 62 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
> index cdf2f54..6bfbd09 100644
> --- a/drivers/char/virtio_console.c
> +++ b/drivers/char/virtio_console.c
> @@ -35,6 +35,7 @@
>  #include <linux/wait.h>
>  #include <linux/workqueue.h>
>  #include <linux/module.h>
> +#include <linux/dma-mapping.h>
>  #include "../tty/hvc/hvc_console.h"
>  
>  /*
> @@ -334,20 +335,60 @@ static inline bool use_multiport(struct ports_device *portdev)
>  	return portdev->vdev->features[0] & (1 << VIRTIO_CONSOLE_F_MULTIPORT);
>  }
>  
> -static void free_buf(struct port_buffer *buf)
> +/* Allcoate data buffer from DMA memory if requested */

typo

> +static inline void *
> +alloc_databuf(struct virtio_device *vdev, size_t size, dma_addr_t *dma_handle,
> +		   gfp_t flag)
>  {
> -	kfree(buf->buf);
> +#ifdef CONFIG_HAS_DMA
> +	if (virtio_has_feature(vdev, VIRTIO_CONSOLE_F_DMA_MEM)) {
> +		struct device *dev = &vdev->dev;
> +		/*
> +		 * Allocate DMA memory from ancestors. Finding the ancestor
> +		 * is a bit quirky when DMA_MEMORY_INCLUDES_CHILDREN is not
> +		 * implemented.
> +		 */
> +		dev = dev->parent ? dev->parent : dev;
> +		dev = dev->parent ? dev->parent : dev;
> +		return dma_alloc_coherent(dev, size, dma_handle, flag);
> +	}
> +#endif

Are these ifdefs really needed? If DMA_MEM is set,
can't we use dma_alloc_coherent
unconditionally?

> +	return kzalloc(size, flag);
> +}
> +
> +static inline void
> +free_databuf(struct virtio_device *vdev, size_t size, void *cpu_addr)
> +{
> +#ifdef CONFIG_HAS_DMA
> +	dma_addr_t dma_handle = virt_to_bus(cpu_addr);
> +	if (virtio_has_feature(vdev, VIRTIO_CONSOLE_F_DMA_MEM)) {
> +		struct device *dev = &vdev->dev;
> +
> +		dev = dev->parent ? dev->parent : dev;
> +		dev = dev->parent ? dev->parent : dev;
> +		dma_free_coherent(dev, size, cpu_addr, dma_handle);
> +		return;
> +	}
> +#endif
> +	kfree(cpu_addr);
> +}
> +
> +static void
> +free_buf(struct virtqueue *vq, struct port_buffer *buf, size_t buf_size)
> +{
> +	free_databuf(vq->vdev, buf_size, buf);
>  	kfree(buf);
>  }
>  
> -static struct port_buffer *alloc_buf(size_t buf_size)
> +static struct port_buffer *alloc_buf(struct virtqueue *vq, size_t buf_size)
>  {
>  	struct port_buffer *buf;
> +	dma_addr_t dma;
>  
>  	buf = kmalloc(sizeof(*buf), GFP_KERNEL);
>  	if (!buf)
>  		goto fail;
> -	buf->buf = kzalloc(buf_size, GFP_KERNEL);
> +	buf->buf = alloc_databuf(vq->vdev, buf_size, &dma, GFP_KERNEL);
>  	if (!buf->buf)
>  		goto free_buf;
>  	buf->len = 0;
> @@ -414,7 +455,7 @@ static void discard_port_data(struct port *port)
>  		port->stats.bytes_discarded += buf->len - buf->offset;
>  		if (add_inbuf(port->in_vq, buf) < 0) {
>  			err++;
> -			free_buf(buf);
> +			free_buf(port->in_vq, buf, PAGE_SIZE);
>  		}
>  		port->inbuf = NULL;
>  		buf = get_inbuf(port);
> @@ -485,7 +526,7 @@ static void reclaim_consumed_buffers(struct port *port)
>  		return;
>  	}
>  	while ((buf = virtqueue_get_buf(port->out_vq, &len))) {
> -		kfree(buf);
> +		free_databuf(port->portdev->vdev, len, buf);
>  		port->outvq_full = false;
>  	}
>  }
> @@ -672,6 +713,8 @@ static ssize_t port_fops_write(struct file *filp, const char __user *ubuf,
>  	char *buf;
>  	ssize_t ret;
>  	bool nonblock;
> +	struct virtio_device *vdev;
> +	dma_addr_t dma;
>  
>  	/* Userspace could be out to fool us */
>  	if (!count)
> @@ -694,9 +737,10 @@ static ssize_t port_fops_write(struct file *filp, const char __user *ubuf,
>  	if (!port->guest_connected)
>  		return -ENODEV;
>  
> +	vdev = port->portdev->vdev;
>  	count = min((size_t)(32 * 1024), count);
>  
> -	buf = kmalloc(count, GFP_KERNEL);
> +	buf = alloc_databuf(vdev, count, &dma, GFP_KERNEL);
>  	if (!buf)
>  		return -ENOMEM;
>  
> @@ -720,7 +764,8 @@ static ssize_t port_fops_write(struct file *filp, const char __user *ubuf,
>  		goto out;
>  
>  free_buf:
> -	kfree(buf);
> +	free_databuf(vdev, count, buf);
> +
>  out:
>  	return ret;
>  }
> @@ -1102,7 +1147,7 @@ static unsigned int fill_queue(struct virtqueue *vq, spinlock_t *lock)
>  
>  	nr_added_bufs = 0;
>  	do {
> -		buf = alloc_buf(PAGE_SIZE);
> +		buf = alloc_buf(vq, PAGE_SIZE);
>  		if (!buf)
>  			break;
>  
> @@ -1110,7 +1155,7 @@ static unsigned int fill_queue(struct virtqueue *vq, spinlock_t *lock)
>  		ret = add_inbuf(vq, buf);
>  		if (ret < 0) {
>  			spin_unlock_irq(lock);
> -			free_buf(buf);
> +			free_buf(vq, buf, PAGE_SIZE);
>  			break;
>  		}
>  		nr_added_bufs++;
> @@ -1234,7 +1279,7 @@ static int add_port(struct ports_device *portdev, u32 id)
>  
>  free_inbufs:
>  	while ((buf = virtqueue_detach_unused_buf(port->in_vq)))
> -		free_buf(buf);
> +		free_buf(port->in_vq, buf, PAGE_SIZE);
>  free_device:
>  	device_destroy(pdrvdata.class, port->dev->devt);
>  free_cdev:
> @@ -1276,7 +1321,7 @@ static void remove_port_data(struct port *port)
>  
>  	/* Remove buffers we queued up for the Host to send us data in. */
>  	while ((buf = virtqueue_detach_unused_buf(port->in_vq)))
> -		free_buf(buf);
> +		free_buf(port->in_vq, buf, PAGE_SIZE);
>  }
>  
>  /*
> @@ -1478,7 +1523,7 @@ static void control_work_handler(struct work_struct *work)
>  		if (add_inbuf(portdev->c_ivq, buf) < 0) {
>  			dev_warn(&portdev->vdev->dev,
>  				 "Error adding buffer to queue\n");
> -			free_buf(buf);
> +			free_buf(portdev->c_ivq, buf, PAGE_SIZE);
>  		}
>  	}
>  	spin_unlock(&portdev->cvq_lock);
> @@ -1674,10 +1719,10 @@ static void remove_controlq_data(struct ports_device *portdev)
>  		return;
>  
>  	while ((buf = virtqueue_get_buf(portdev->c_ivq, &len)))
> -		free_buf(buf);
> +		free_buf(portdev->c_ivq, buf, PAGE_SIZE);
>  
>  	while ((buf = virtqueue_detach_unused_buf(portdev->c_ivq)))
> -		free_buf(buf);
> +		free_buf(portdev->c_ivq, buf, PAGE_SIZE);
>  }
>  
>  /*
> @@ -1836,6 +1881,7 @@ static struct virtio_device_id id_table[] = {
>  static unsigned int features[] = {
>  	VIRTIO_CONSOLE_F_SIZE,
>  	VIRTIO_CONSOLE_F_MULTIPORT,
> +	VIRTIO_CONSOLE_F_DMA_MEM,
>  };
>  
>  #ifdef CONFIG_PM
> diff --git a/include/linux/virtio_console.h b/include/linux/virtio_console.h
> index bdf4b00..b27f7fa 100644
> --- a/include/linux/virtio_console.h
> +++ b/include/linux/virtio_console.h
> @@ -38,6 +38,7 @@
>  /* Feature bits */
>  #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_F_DMA_MEM 2	/* Use DMA memory in vrings */
>  
>  #define VIRTIO_CONSOLE_BAD_ID		(~(u32)0)
>  
> -- 
> 1.7.5.4

Re: [RFC 1/2] virtio_console: Add support for DMA memory allocation

[Sjur Brændeland]

Hi Michael,

> How does access to descriptors work in this setup?

When the ring is setup by remoteproc the descriptors are
also allocated using dma_alloc_coherent().

>> -static void free_buf(struct port_buffer *buf)
>> +/* Allcoate data buffer from DMA memory if requested */
>
> typo

Thanks.

>> +static inline void *
>> +alloc_databuf(struct virtio_device *vdev, size_t size, dma_addr_t *dma_handle,
>> +                gfp_t flag)
>>  {
>> -     kfree(buf->buf);
>> +#ifdef CONFIG_HAS_DMA
>> +     if (virtio_has_feature(vdev, VIRTIO_CONSOLE_F_DMA_MEM)) {
>> +             struct device *dev = &vdev->dev;
>> +             /*
>> +              * Allocate DMA memory from ancestors. Finding the ancestor
>> +              * is a bit quirky when DMA_MEMORY_INCLUDES_CHILDREN is not
>> +              * implemented.
>> +              */
>> +             dev = dev->parent ? dev->parent : dev;
>> +             dev = dev->parent ? dev->parent : dev;
>> +             return dma_alloc_coherent(dev, size, dma_handle, flag);
>> +     }
>> +#endif
>
> Are these ifdefs really needed? If DMA_MEM is set,
> can't we use dma_alloc_coherent
> unconditionally?

If an architecture do not support DMA you will get
a link error: "unknown symbol" for dma_alloc_coherent.

Regards,
Sjur

Re: [RFC 1/2] virtio_console: Add support for DMA memory allocation

[Michael S. Tsirkin]

On Mon, Sep 03, 2012 at 04:57:45PM +0200, Sjur Brændeland wrote:
> Hi Michael,
> 
> > How does access to descriptors work in this setup?
> 
> When the ring is setup by remoteproc the descriptors are
> also allocated using dma_alloc_coherent().
> 
> >> -static void free_buf(struct port_buffer *buf)
> >> +/* Allcoate data buffer from DMA memory if requested */
> >
> > typo
> 
> Thanks.
> 
> >> +static inline void *
> >> +alloc_databuf(struct virtio_device *vdev, size_t size, dma_addr_t *dma_handle,
> >> +                gfp_t flag)
> >>  {
> >> -     kfree(buf->buf);
> >> +#ifdef CONFIG_HAS_DMA
> >> +     if (virtio_has_feature(vdev, VIRTIO_CONSOLE_F_DMA_MEM)) {
> >> +             struct device *dev = &vdev->dev;
> >> +             /*
> >> +              * Allocate DMA memory from ancestors. Finding the ancestor
> >> +              * is a bit quirky when DMA_MEMORY_INCLUDES_CHILDREN is not
> >> +              * implemented.
> >> +              */
> >> +             dev = dev->parent ? dev->parent : dev;
> >> +             dev = dev->parent ? dev->parent : dev;
> >> +             return dma_alloc_coherent(dev, size, dma_handle, flag);
> >> +     }
> >> +#endif
> >
> > Are these ifdefs really needed? If DMA_MEM is set,
> > can't we use dma_alloc_coherent
> > unconditionally?
> 
> If an architecture do not support DMA you will get
> a link error: "unknown symbol" for dma_alloc_coherent.
> 
> Regards,
> Sjur

Yes, it even seems intentional.
But I have a question: can the device work without DMA?
Does not VIRTIO_CONSOLE_F_DMA_MEM mean dma api is required?
If yes you should just fail load.

Also let's add a wrapper at top of file so ifdefs
do not litter the code like this. For example:

#ifdef CONFIG_HAS_DMA
#define VIRTIO_CONSOLE_HAS_DMA (1)
#else
#define VIRTIO_CONSOLE_HAS_DMA (0)
#endif

Now use if instead of ifdef.

-- 
MST

Re: [RFC 1/2] virtio_console: Add support for DMA memory allocation

[Rusty Russell]

"Michael S. Tsirkin" <mst@redhat.com> writes:
> Also let's add a wrapper at top of file so ifdefs
> do not litter the code like this. For example:
>
> #ifdef CONFIG_HAS_DMA
> #define VIRTIO_CONSOLE_HAS_DMA (1)
> #else
> #define VIRTIO_CONSOLE_HAS_DMA (0)
> #endif
>
> Now use if instead of ifdef.

The driver certainly shouldn't offer VIRTIO_CONSOLE_F_DMA_MEM if you
don't have DMA!

Cheers,
Rusty.

Re: [RFC 1/2] virtio_console: Add support for DMA memory allocation

[Sjur Brændeland]

> The driver certainly shouldn't offer VIRTIO_CONSOLE_F_DMA_MEM if you
> don't have DMA!

OK, so the feature table could be done like this:

 static unsigned int features[] = {
	VIRTIO_CONSOLE_F_SIZE,
	VIRTIO_CONSOLE_F_MULTIPORT,
#if VIRTIO_CONSOLE_HAS_DMA
	VIRTIO_CONSOLE_F_DMA_MEM,
#endif
}

If the device then asks for VIRTIO_CONSOLE_F_DMA_MEM
when DMA is not supported, virtio will do BUG_ON() from
virtio_check_driver_offered_feature().

Is this acceptable or should we add a check in virtcons_probe()
and let the probing fail instead?

E.g:
	/* Refuse to bind if F_DMA_MEM request cannot be met */
	if (!VIRTIO_CONSOLE_HAS_DMA &&
	    (vdev->config->get_features(vdev) & (1 << VIRTIO_CONSOLE_F_DMA_MEM))){
		dev_err(&vdev->dev,
			"DMA_MEM requested, but arch does not support DMA\n");
		err = -EINVAL;
		goto fail;
	}

Regards,
Sjur

Re: [RFC 1/2] virtio_console: Add support for DMA memory allocation

[Michael S. Tsirkin]

On Wed, Sep 05, 2012 at 03:00:20PM +0200, Sjur Brændeland wrote:
> > The driver certainly shouldn't offer VIRTIO_CONSOLE_F_DMA_MEM if you
> > don't have DMA!
> 
> OK, so the feature table could be done like this:
> 
>  static unsigned int features[] = {
> 	VIRTIO_CONSOLE_F_SIZE,
> 	VIRTIO_CONSOLE_F_MULTIPORT,
> #if VIRTIO_CONSOLE_HAS_DMA
> 	VIRTIO_CONSOLE_F_DMA_MEM,
> #endif
> }
> 
> If the device then asks for VIRTIO_CONSOLE_F_DMA_MEM
> when DMA is not supported, virtio will do BUG_ON() from
> virtio_check_driver_offered_feature().
> 
> Is this acceptable or should we add a check in virtcons_probe()
> and let the probing fail instead?
> 
> E.g:
> 	/* Refuse to bind if F_DMA_MEM request cannot be met */
> 	if (!VIRTIO_CONSOLE_HAS_DMA &&
> 	    (vdev->config->get_features(vdev) & (1 << VIRTIO_CONSOLE_F_DMA_MEM))){
> 		dev_err(&vdev->dev,
> 			"DMA_MEM requested, but arch does not support DMA\n");
> 		err = -EINVAL;
> 		goto fail;
> 	}
> 
> Regards,
> Sjur

Failing probe would be cleaner. But there is still a problem:
old driver will happily bind to that device and then
fail to work, right?

virtio pci has revision id for this, but remoteproc doesn't
seem to have anything similar. Or did I miss it? If not -
we probably need to use a different
device id, and not a feature bit.

-- 
MST

Re: [RFC 1/2] virtio_console: Add support for DMA memory allocation

[Sjur Brændeland]

Hi Michael.

>> If the device then asks for VIRTIO_CONSOLE_F_DMA_MEM
>> when DMA is not supported, virtio will do BUG_ON() from
>> virtio_check_driver_offered_feature().
>>
>> Is this acceptable or should we add a check in virtcons_probe()
>> and let the probing fail instead?
>>
>> E.g:
>>       /* Refuse to bind if F_DMA_MEM request cannot be met */
>>       if (!VIRTIO_CONSOLE_HAS_DMA &&
>>           (vdev->config->get_features(vdev) & (1 << VIRTIO_CONSOLE_F_DMA_MEM))){
>>               dev_err(&vdev->dev,
>>                       "DMA_MEM requested, but arch does not support DMA\n");
>>               err = -EINVAL;
>>               goto fail;
>>       }
>>
>> Regards,
>> Sjur
>
> Failing probe would be cleaner. But there is still a problem:
> old driver will happily bind to that device and then
> fail to work, right?

Not just fail to work, the kernel will panic on the BUG_ON().
Remoteproc gets the virtio configuration from firmware loaded
from user space. So this type of problem might be triggered
for other virtio drivers as well.


> virtio pci has revision id for this, but remoteproc doesn't
> seem to have anything similar. Or did I miss it?

No there are currently no sanity check of
virtio type and feature bits in remoteproc.
One option may be to add this...

> If not -
> we probably need to use a different
> device id, and not a feature bit.

But if I create a new virtio console type, remoteproc
could still call the existing virtio_console with random
bad feature bits, causing kernel panic.

Even if we fix this particular problem, the general problem
still exists: bogus virtio declarations in remoteproc's firmware
may cause BUG_ON(). (Note the fundamental difference
between visualizations and remoteproc. For remoteproc
the virtio configuration comes from binaries loaded from
user space).

So maybe we should look for a more generic solution, e.g.
changing virtio probe functionality so that devices with
bad feature bits will not trigger BUG_ON(), but rather refuse
to bind the driver.

Regards,
Sjur

Re: [RFC 1/2] virtio_console: Add support for DMA memory allocation

[Michael S. Tsirkin]

On Wed, Sep 05, 2012 at 08:15:36PM +0200, Sjur Brændeland wrote:
> Hi Michael.
> 
> >> If the device then asks for VIRTIO_CONSOLE_F_DMA_MEM
> >> when DMA is not supported, virtio will do BUG_ON() from
> >> virtio_check_driver_offered_feature().
> >>
> >> Is this acceptable or should we add a check in virtcons_probe()
> >> and let the probing fail instead?
> >>
> >> E.g:
> >>       /* Refuse to bind if F_DMA_MEM request cannot be met */
> >>       if (!VIRTIO_CONSOLE_HAS_DMA &&
> >>           (vdev->config->get_features(vdev) & (1 << VIRTIO_CONSOLE_F_DMA_MEM))){
> >>               dev_err(&vdev->dev,
> >>                       "DMA_MEM requested, but arch does not support DMA\n");
> >>               err = -EINVAL;
> >>               goto fail;
> >>       }
> >>
> >> Regards,
> >> Sjur
> >
> > Failing probe would be cleaner. But there is still a problem:
> > old driver will happily bind to that device and then
> > fail to work, right?
> 
> Not just fail to work, the kernel will panic on the BUG_ON().
> Remoteproc gets the virtio configuration from firmware loaded
> from user space. So this type of problem might be triggered
> for other virtio drivers as well.

how?

> 
> > virtio pci has revision id for this, but remoteproc doesn't
> > seem to have anything similar. Or did I miss it?
> 
> No there are currently no sanity check of
> virtio type and feature bits in remoteproc.
> One option may be to add this...

you can not fix the past.

> > If not -
> > we probably need to use a different
> > device id, and not a feature bit.
> 
> But if I create a new virtio console type, remoteproc
> could still call the existing virtio_console with random
> bad feature bits, causing kernel panic.

cirtio core checks device id - this should not happen.


> Even if we fix this particular problem, the general problem
> still exists: bogus virtio declarations in remoteproc's firmware
> may cause BUG_ON().

which BUG_ON exactly?

> (Note the fundamental difference
> between visualizations and remoteproc. For remoteproc
> the virtio configuration comes from binaries loaded from
> user space).
> 
> So maybe we should look for a more generic solution, e.g.
> changing virtio probe functionality so that devices with
> bad feature bits will not trigger BUG_ON(), but rather refuse
> to bind the driver.
> 
> Regards,
> Sjur

Re: [RFC 1/2] virtio_console: Add support for DMA memory allocation

[Sjur Brændeland]

Hi Michael.

>> Not just fail to work, the kernel will panic on the BUG_ON().
>> Remoteproc gets the virtio configuration from firmware loaded
>> from user space. So this type of problem might be triggered
>> for other virtio drivers as well.
>
> how?
...
>> Even if we fix this particular problem, the general problem
>> still exists: bogus virtio declarations in remoteproc's firmware
>> may cause BUG_ON().
>
> which BUG_ON exactly?

I am afraid I have been barking up the wrong tree here.
Please ignore my previous rambling about panics related
to device features.

I hit the  BUG() in virtio_check_driver_offered_feature():
First I did not declare the feature because DMA was not set:
static unsigned int features[] = {
...
#if VIRTIO_CONSOLE_HAS_DMA
	VIRTIO_CONSOLE_F_DMA_MEM,
#endif
};

and then in probe I checked if the feature was supported:
	    virtio_has_feature(vdev, VIRTIO_CONSOLE_F_DMA_MEM)

This triggered the BUG() in virtio_check_driver_offered_feature(),
because the driver was asking for a unknown-feature.

I can get avoid this by simply checking the devices feature bits directly
instead of using virtio_has_feature().

Regards,
Sjur

Re: [RFC 1/2] virtio_console: Add support for DMA memory allocation

[Sjur Brændeland]

Hi Michael,

>> If an architecture do not support DMA you will get
>> a link error: "unknown symbol" for dma_alloc_coherent.
>
> Yes, it even seems intentional.
> But I have a question: can the device work without DMA?

The main dependency is actually on the dma-allocation.
In my case I do dma_declare_coherent_memory() with
the memory area shared with the remote device as argument.
Subsequent calls to dma_alloc_coherent() will then allocate
from this memory area.

> Does not VIRTIO_CONSOLE_F_DMA_MEM mean dma api is required?

Yes.dma_alloc_coherent to work.

> If yes you should just fail load.

Agree, I'll check on VIRTIO_CONSOLE_HAS_DMA before adding
VIRTIO_CONSOLE_F_DMA_MEM to the feature array.

> Also let's add a wrapper at top of file so ifdefs
> do not litter the code like this. For example:
>
> #ifdef CONFIG_HAS_DMA
> #define VIRTIO_CONSOLE_HAS_DMA (1)
> #else
> #define VIRTIO_CONSOLE_HAS_DMA (0)
> #endif

OK, good point. Perhaps we could even exploit gcc's
ability to remove dead code and do something like this:

       if (VIRTIO_CONSOLE_HAS_DMA &&
           virtio_has_feature(vdev, VIRTIO_CONSOLE_F_DMA_MEM)) {
...

This does not give any linker warnings when compiling for UML (no DMA).

Regards,
Sjur

Re: [RFC 1/2] virtio_console: Add support for DMA memory allocation

[Michael S. Tsirkin]

On Tue, Sep 04, 2012 at 01:28:36PM +0200, Sjur Brændeland wrote:
> Hi Michael,
> 
> >> If an architecture do not support DMA you will get
> >> a link error: "unknown symbol" for dma_alloc_coherent.
> >
> > Yes, it even seems intentional.
> > But I have a question: can the device work without DMA?
> 
> The main dependency is actually on the dma-allocation.
> In my case I do dma_declare_coherent_memory() with
> the memory area shared with the remote device as argument.
> Subsequent calls to dma_alloc_coherent() will then allocate
> from this memory area.
> 
> > Does not VIRTIO_CONSOLE_F_DMA_MEM mean dma api is required?
> 
> Yes.dma_alloc_coherent to work.
> 
> > If yes you should just fail load.
> 
> Agree, I'll check on VIRTIO_CONSOLE_HAS_DMA before adding
> VIRTIO_CONSOLE_F_DMA_MEM to the feature array.
> 
> > Also let's add a wrapper at top of file so ifdefs
> > do not litter the code like this. For example:
> >
> > #ifdef CONFIG_HAS_DMA
> > #define VIRTIO_CONSOLE_HAS_DMA (1)
> > #else
> > #define VIRTIO_CONSOLE_HAS_DMA (0)
> > #endif
> 
> OK, good point. Perhaps we could even exploit gcc's
> ability to remove dead code and do something like this:
> 
>        if (VIRTIO_CONSOLE_HAS_DMA &&
>            virtio_has_feature(vdev, VIRTIO_CONSOLE_F_DMA_MEM)) {
> ...
> 
> This does not give any linker warnings when compiling for UML (no DMA).
> 
> Regards,
> Sjur

Exactly. Though if we just fail load it will be much less code.

Generally, using a feature bit for this is a bit of a problem though:
normally driver is expected to be able to simply ignore
a feature bit. In this case driver is required to
do something so a feature bit is not a good fit.
I am not sure what the right thing to do is.

-- 
MST

Re: [RFC 1/2] virtio_console: Add support for DMA memory allocation

[Sjur Brændeland]

Hi Michael,

> Exactly. Though if we just fail load it will be much less code.
>
> Generally, using a feature bit for this is a bit of a problem though:
> normally driver is expected to be able to simply ignore
> a feature bit. In this case driver is required to
> do something so a feature bit is not a good fit.
> I am not sure what the right thing to do is.

I see - so in order to avoid the binding between driver and device
there are two options I guess. Either make virtio_dev_match() or
virtcons_probe() fail. Neither of them seems like the obvious choice.

Maybe adding a check for VIRTIO_CONSOLE_F_DMA_MEM match
between device and driver in virtcons_probe() is the lesser evil?

Regards,
Sjur

Re: [RFC 1/2] virtio_console: Add support for DMA memory allocation

[Michael S. Tsirkin]

On Tue, Sep 04, 2012 at 06:58:47PM +0200, Sjur Brændeland wrote:
> Hi Michael,
> 
> > Exactly. Though if we just fail load it will be much less code.
> >
> > Generally, using a feature bit for this is a bit of a problem though:
> > normally driver is expected to be able to simply ignore
> > a feature bit. In this case driver is required to
> > do something so a feature bit is not a good fit.
> > I am not sure what the right thing to do is.
> 
> I see - so in order to avoid the binding between driver and device
> there are two options I guess. Either make virtio_dev_match() or
> virtcons_probe() fail. Neither of them seems like the obvious choice.
> 
> Maybe adding a check for VIRTIO_CONSOLE_F_DMA_MEM match
> between device and driver in virtcons_probe() is the lesser evil?
> 
> Regards,
> Sjur

A simplest thing to do is change dev id. rusty?

-- 
MST

Re: [RFC 1/2] virtio_console: Add support for DMA memory allocation

[Rusty Russell]

"Michael S. Tsirkin" <mst@redhat.com> writes:
> On Tue, Sep 04, 2012 at 06:58:47PM +0200, Sjur Brændeland wrote:
>> Hi Michael,
>> 
>> > Exactly. Though if we just fail load it will be much less code.
>> >
>> > Generally, using a feature bit for this is a bit of a problem though:
>> > normally driver is expected to be able to simply ignore
>> > a feature bit. In this case driver is required to
>> > do something so a feature bit is not a good fit.
>> > I am not sure what the right thing to do is.
>> 
>> I see - so in order to avoid the binding between driver and device
>> there are two options I guess. Either make virtio_dev_match() or
>> virtcons_probe() fail. Neither of them seems like the obvious choice.
>> 
>> Maybe adding a check for VIRTIO_CONSOLE_F_DMA_MEM match
>> between device and driver in virtcons_probe() is the lesser evil?
>> 
>> Regards,
>> Sjur
>
> A simplest thing to do is change dev id. rusty?

For generic usage, this is correct.  But my opinion is that fallback on
feature non-ack is quality-of-implementation issue: great to have, but
there are cases where you just want to fail with "you're too old".

And in this case, an old system simply will never work.  So it's a
question of how graceful the failure is.

Can your userspace loader can refuse to proceed if the driver doesn't
ack the bits?  If so, it's simpler than a whole new ID.

Cheers,
Rusty.

Re: [RFC 1/2] virtio_console: Add support for DMA memory allocation

[Michael S. Tsirkin]

On Thu, Sep 06, 2012 at 11:34:25AM +0930, Rusty Russell wrote:
> "Michael S. Tsirkin" <mst@redhat.com> writes:
> > On Tue, Sep 04, 2012 at 06:58:47PM +0200, Sjur Brændeland wrote:
> >> Hi Michael,
> >> 
> >> > Exactly. Though if we just fail load it will be much less code.
> >> >
> >> > Generally, using a feature bit for this is a bit of a problem though:
> >> > normally driver is expected to be able to simply ignore
> >> > a feature bit. In this case driver is required to
> >> > do something so a feature bit is not a good fit.
> >> > I am not sure what the right thing to do is.
> >> 
> >> I see - so in order to avoid the binding between driver and device
> >> there are two options I guess. Either make virtio_dev_match() or
> >> virtcons_probe() fail. Neither of them seems like the obvious choice.
> >> 
> >> Maybe adding a check for VIRTIO_CONSOLE_F_DMA_MEM match
> >> between device and driver in virtcons_probe() is the lesser evil?
> >> 
> >> Regards,
> >> Sjur
> >
> > A simplest thing to do is change dev id. rusty?
> 
> For generic usage, this is correct.  But my opinion is that fallback on
> feature non-ack is quality-of-implementation issue: great to have, but
> there are cases where you just want to fail with "you're too old".
> 
> And in this case, an old system simply will never work.  So it's a
> question of how graceful the failure is.
> 
> Can your userspace loader can refuse to proceed if the driver doesn't
> ack the bits?  If so, it's simpler than a whole new ID.
> 
> Cheers,
> Rusty.

Yes but how can it signal guest that it will never proceed?

Also grep for BUG_ON in core found this:

       drv->remove(dev);

       /* Driver should have reset device. */
       BUG_ON(dev->config->get_status(dev));

I think below is what Sjur refers to.
I think below is a good idea for 3.6. Thoughts?

--->

virtio: don't crash when device is buggy

Because of a sanity check in virtio_dev_remove, a buggy device can crash
kernel.  And in case of rproc it's userspace so it's not a good idea.
We are unloading a driver so how bad can it be?
Be less aggressive in handling this error: if it's a driver bug,
warning once should be enough.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

--

diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
index c3b3f7f..1e8659c 100644
--- a/drivers/virtio/virtio.c
+++ b/drivers/virtio/virtio.c
@@ -159,7 +159,7 @@ static int virtio_dev_remove(struct device *_d)
 	drv->remove(dev);
 
 	/* Driver should have reset device. */
-	BUG_ON(dev->config->get_status(dev));
+	WARN_ON_ONCE(dev->config->get_status(dev));
 
 	/* Acknowledge the device's existence again. */
 	add_status(dev, VIRTIO_CONFIG_S_ACKNOWLEDGE);

-- 
MST

Re: [RFC 1/2] virtio_console: Add support for DMA memory allocation

[Michael S. Tsirkin]

On Thu, Sep 06, 2012 at 08:27:35AM +0300, Michael S. Tsirkin wrote:
> On Thu, Sep 06, 2012 at 11:34:25AM +0930, Rusty Russell wrote:
> > "Michael S. Tsirkin" <mst@redhat.com> writes:
> > > On Tue, Sep 04, 2012 at 06:58:47PM +0200, Sjur Brændeland wrote:
> > >> Hi Michael,
> > >> 
> > >> > Exactly. Though if we just fail load it will be much less code.
> > >> >
> > >> > Generally, using a feature bit for this is a bit of a problem though:
> > >> > normally driver is expected to be able to simply ignore
> > >> > a feature bit. In this case driver is required to
> > >> > do something so a feature bit is not a good fit.
> > >> > I am not sure what the right thing to do is.
> > >> 
> > >> I see - so in order to avoid the binding between driver and device
> > >> there are two options I guess. Either make virtio_dev_match() or
> > >> virtcons_probe() fail. Neither of them seems like the obvious choice.
> > >> 
> > >> Maybe adding a check for VIRTIO_CONSOLE_F_DMA_MEM match
> > >> between device and driver in virtcons_probe() is the lesser evil?
> > >> 
> > >> Regards,
> > >> Sjur
> > >
> > > A simplest thing to do is change dev id. rusty?
> > 
> > For generic usage, this is correct.  But my opinion is that fallback on
> > feature non-ack is quality-of-implementation issue: great to have, but
> > there are cases where you just want to fail with "you're too old".
> > 
> > And in this case, an old system simply will never work.  So it's a
> > question of how graceful the failure is.
> > 
> > Can your userspace loader can refuse to proceed if the driver doesn't
> > ack the bits?  If so, it's simpler than a whole new ID.
> > 
> > Cheers,
> > Rusty.
> 
> Yes but how can it signal guest that it will never proceed?
> 
> Also grep for BUG_ON in core found this:
> 
>        drv->remove(dev);
> 
>        /* Driver should have reset device. */
>        BUG_ON(dev->config->get_status(dev));
> 
> I think below is what Sjur refers to.
> I think below is a good idea for 3.6. Thoughts?
> 
> --->
> 
> virtio: don't crash when device is buggy
> 
> Because of a sanity check in virtio_dev_remove, a buggy device can crash
> kernel.  And in case of rproc it's userspace so it's not a good idea.
> We are unloading a driver so how bad can it be?
> Be less aggressive in handling this error: if it's a driver bug,
> warning once should be enough.
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> 
> --

Rusty?

> diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
> index c3b3f7f..1e8659c 100644
> --- a/drivers/virtio/virtio.c
> +++ b/drivers/virtio/virtio.c
> @@ -159,7 +159,7 @@ static int virtio_dev_remove(struct device *_d)
>  	drv->remove(dev);
>  
>  	/* Driver should have reset device. */
> -	BUG_ON(dev->config->get_status(dev));
> +	WARN_ON_ONCE(dev->config->get_status(dev));
>  
>  	/* Acknowledge the device's existence again. */
>  	add_status(dev, VIRTIO_CONFIG_S_ACKNOWLEDGE);
> 
> -- 
> MST

Re: [RFC 1/2] virtio_console: Add support for DMA memory allocation

[Rusty Russell]

"Michael S. Tsirkin" <mst@redhat.com> writes:
>> virtio: don't crash when device is buggy
>> 
>> Because of a sanity check in virtio_dev_remove, a buggy device can crash
>> kernel.  And in case of rproc it's userspace so it's not a good idea.
>> We are unloading a driver so how bad can it be?
>> Be less aggressive in handling this error: if it's a driver bug,
>> warning once should be enough.
>> 
>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>
> Rusty?

Thanks, applied.

I really want to implement CONFIG_VIRTIO_DEVEL_DEBUG which would
incorporate the checks in virtio_ring.c as well as this.  Then I could
also reshuffle descriptors (eg. split them) to find buggy devices like
qemu which assume the first descriptor is the struct virtio_net_hdr...

Cheers,
Rusty,

Re: [RFC 1/2] virtio_console: Add support for DMA memory allocation

[Sjur Brændeland]

Hi Ohad,

>> A simplest thing to do is change dev id. rusty?
>
> For generic usage, this is correct.  But my opinion is that fallback on
> feature non-ack is quality-of-implementation issue: great to have, but
> there are cases where you just want to fail with "you're too old".
>
> And in this case, an old system simply will never work.  So it's a
> question of how graceful the failure is.
>
> Can your userspace loader can refuse to proceed if the driver doesn't
> ack the bits?  If so, it's simpler than a whole new ID.

Ohad: Are there any way we could avoid starting up the remote processor
if mandatory virtio features (such as DMA memory) are not supported by
the virtio-driver?

Regards,
Sjur

[PATCH repost] virtio: don't crash when device is buggy

[Michael S. Tsirkin]

Because of a sanity check in virtio_dev_remove, a buggy device can crash
kernel.  And in case of rproc it's userspace so it's not a good idea.
We are unloading a driver so how bad can it be?
Be less aggressive in handling this error: if it's a driver bug,
warning once should be enough.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

--

3.6 material?

diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
index c3b3f7f..1e8659c 100644
--- a/drivers/virtio/virtio.c
+++ b/drivers/virtio/virtio.c
@@ -159,7 +159,7 @@ static int virtio_dev_remove(struct device *_d)
 	drv->remove(dev);
 
 	/* Driver should have reset device. */
-	BUG_ON(dev->config->get_status(dev));
+	WARN_ON_ONCE(dev->config->get_status(dev));
 
 	/* Acknowledge the device's existence again. */
 	add_status(dev, VIRTIO_CONFIG_S_ACKNOWLEDGE);

-- 
MST

Re: [PATCH repost] virtio: don't crash when device is buggy

[Rusty Russell]

"Michael S. Tsirkin" <mst@redhat.com> writes:

> Because of a sanity check in virtio_dev_remove, a buggy device can crash
> kernel.  And in case of rproc it's userspace so it's not a good idea.
> We are unloading a driver so how bad can it be?
> Be less aggressive in handling this error: if it's a driver bug,
> warning once should be enough.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>
> --
>
> 3.6 material?

I have already applied, this, but it's not for stable, since it's a
"theoretical bugfix".  That check has been in there forever and noone
AFAIK has actually struck it.

Cheers,
Rusty.

Re: [PATCH repost] virtio: don't crash when device is buggy

[Michael S. Tsirkin]

On Mon, Sep 17, 2012 at 01:57:17PM +0930, Rusty Russell wrote:
> "Michael S. Tsirkin" <mst@redhat.com> writes:
> 
> > Because of a sanity check in virtio_dev_remove, a buggy device can crash
> > kernel.  And in case of rproc it's userspace so it's not a good idea.
> > We are unloading a driver so how bad can it be?
> > Be less aggressive in handling this error: if it's a driver bug,
> > warning once should be enough.
> >
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> >
> > --
> >
> > 3.6 material?
> 
> I have already applied, this, but it's not for stable, since it's a
> "theoretical bugfix".  That check has been in there forever and noone
> AFAIK has actually struck it.
> 
> Cheers,
> Rusty.

Yes but can't malicious userspace trigger this with remoteproc? If yes
it's not a question of whether anyone has struck it since people don't
normally run malicious userspace :)

-- 
MST

Re: [PATCH repost] virtio: don't crash when device is buggy

[Rusty Russell]

"Michael S. Tsirkin" <mst@redhat.com> writes:

> On Mon, Sep 17, 2012 at 01:57:17PM +0930, Rusty Russell wrote:
>> "Michael S. Tsirkin" <mst@redhat.com> writes:
>> 
>> > Because of a sanity check in virtio_dev_remove, a buggy device can crash
>> > kernel.  And in case of rproc it's userspace so it's not a good idea.
>> > We are unloading a driver so how bad can it be?
>> > Be less aggressive in handling this error: if it's a driver bug,
>> > warning once should be enough.
>> >
>> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>> >
>> > --
>> >
>> > 3.6 material?
>> 
>> I have already applied, this, but it's not for stable, since it's a
>> "theoretical bugfix".  That check has been in there forever and noone
>> AFAIK has actually struck it.
>> 
>> Cheers,
>> Rusty.
>
> Yes but can't malicious userspace trigger this with remoteproc? If yes
> it's not a question of whether anyone has struck it since people don't
> normally run malicious userspace :)

I think that malicious userspace is already priveleged, isn't it?

Cheers,
Rusty.