thr3ads.net - Linux Virtualization - [RFC 1/2] virtio_console: Add support for DMA memory allocation [Sep 2012]

If this information is useful, please help other people find it:
Share via:

sjur.brandeland at stericsson.com

2012-Sep-03 13:51 UTC

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

From: Sjur Br?ndeland <sjur.brandeland at 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 at stericsson.com>
cc: Rusty Russell <rusty at rustcorp.com.au>
cc: Michael S. Tsirkin <mst at redhat.com>
cc: Ohad Ben-Cohen <ohad at wizery.com>
cc: Linus Walleij <linus.walleij at linaro.org>
cc: virtualization at 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

sjur.brandeland at stericsson.com

2012-Sep-03 13:51 UTC

head link

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

From: Sjur Br?ndeland <sjur.brandeland at 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 at stericsson.com>
cc: Rusty Russell <rusty at rustcorp.com.au>
cc: Michael S. Tsirkin <mst at redhat.com>
cc: virtualization at lists.linux-foundation.org
cc: Ohad Ben-Cohen <ohad at wizery.com>
cc: Linus Walleij <linus.walleij at 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

Michael S. Tsirkin

2012-Sep-03 14:30 UTC

head link

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

On Mon, Sep 03, 2012 at 03:51:16PM +0200, sjur.brandeland at stericsson.com
wrote:> From: Sjur Br?ndeland <sjur.brandeland at 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 at stericsson.com>
> cc: Rusty Russell <rusty at rustcorp.com.au>
> cc: Michael S. Tsirkin <mst at redhat.com>
> cc: Ohad Ben-Cohen <ohad at wizery.com>
> cc: Linus Walleij <linus.walleij at linaro.org>
> cc: virtualization at 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

Michael S. Tsirkin

2012-Sep-04 18:55 UTC

head link

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

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

Possibly Parallel Threads

Search for more maybe matching threads

Linux Virtualization - Sep 2012 - [RFC 1/2] virtio_console: Add support for DMA memory allocation

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

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

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

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

Possibly Parallel Threads