[PATCHv6 0/3] virtio_console: Add rproc_serial device

[PATCHv6 0/3] virtio_console: Add rproc_serial device

[sjur.brandeland]

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

I thought rebasing rproc_serial to linux-next was going to be trivial.
But when starting the merge I realized that I had to refactor the
the patches from  Masami Hiramatsu. The splice support has the same issue
as I faced, with different type of buffers in the out_vq.
So I ended up refactoring the splice functionality. The code
size got smaller so hopefully this a step in the right direction.

This refactoring also make introduction of rproc_serial cleaner.

As requested I also added a patch for not initializing buffers.

I have tested the VIRTIO_CONSOLE device by looping large amount of data
through character device and tty, with lockdep and slub-debug on.
This looks stable for me. I've also done a simple test of splice.

Thanks,
Sjur

cc: Rusty Russell <rusty@rustcorp.com.au>
cc: Michael S. Tsirkin <mst@redhat.com>
cc: Amit Shah <amit.shah@redhat.com>
cc: Linus Walleij <linus.walleij@linaro.org>
cc: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>


Sjur Brændeland (3):
  virtio_console:Merge struct buffer_token into struct port_buffer
  virtio_console: Add support for remoteproc serial
  virtio_console: Don't initialize buffers to zero

 drivers/char/virtio_console.c |  318 +++++++++++++++++++++++++++++------------
 include/linux/virtio_ids.h    |    1 +
 2 files changed, 228 insertions(+), 91 deletions(-)

-- 
1.7.5.4

[PATCH 1/3] virtio_console:Merge struct buffer_token into struct port_buffer

[sjur.brandeland]

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

This merge reduces code size by unifying the approach for
sending scatter-lists and regular buffers. Any type of
write operation (splice, write, put_chars) will now allocate
a port_buffer and send_buf() and free_buf() can always be used.

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: Amit Shah <amit.shah@redhat.com>
cc: Linus Walleij <linus.walleij@linaro.org>
cc: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
---
 drivers/char/virtio_console.c |  141 ++++++++++++++++++-----------------------
 1 files changed, 62 insertions(+), 79 deletions(-)

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 8ab9c3d..f4f7b04 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -111,6 +111,11 @@ struct port_buffer {
 	size_t len;
 	/* offset in the buf from which to consume data */
 	size_t offset;
+
+	/* If sgpages == 0 then buf is used, else sg is used */
+	unsigned int sgpages;
+
+	struct scatterlist sg[1];
 };
 
 /*
@@ -338,23 +343,46 @@ static inline bool use_multiport(struct ports_device *portdev)
 
 static void free_buf(struct port_buffer *buf)
 {
+	int i;
+
 	kfree(buf->buf);
+
+	if (buf->sgpages)
+		for (i = 0; i < buf->sgpages; i++) {
+			struct page *page = sg_page(&buf->sg[i]);
+			if (!page)
+				break;
+			put_page(page);
+		}
+
 	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,
+				     int nrbufs)
 {
 	struct port_buffer *buf;
+	size_t alloc_size;
 
-	buf = kmalloc(sizeof(*buf), GFP_KERNEL);
+	/* Allocate buffer and the scatter list */
+	alloc_size = sizeof(*buf) + sizeof(struct scatterlist) * nrbufs;
+	buf = kmalloc(alloc_size, GFP_ATOMIC);
 	if (!buf)
 		goto fail;
-	buf->buf = kzalloc(buf_size, GFP_KERNEL);
+
+	buf->sgpages = nrbufs;
+	if (nrbufs > 0)
+		return buf;
+
+	buf->buf = kmalloc(buf_size, GFP_ATOMIC);
 	if (!buf->buf)
 		goto free_buf;
 	buf->len = 0;
 	buf->offset = 0;
 	buf->size = buf_size;
+
+	/* Prepare scatter buffer for sending */
+	sg_init_one(buf->sg, buf->buf, buf_size);
 	return buf;
 
 free_buf:
@@ -476,52 +504,25 @@ static ssize_t send_control_msg(struct port *port, unsigned int event,
 	return 0;
 }
 
-struct buffer_token {
-	union {
-		void *buf;
-		struct scatterlist *sg;
-	} u;
-	/* If sgpages == 0 then buf is used, else sg is used */
-	unsigned int sgpages;
-};
-
-static void reclaim_sg_pages(struct scatterlist *sg, unsigned int nrpages)
-{
-	int i;
-	struct page *page;
-
-	for (i = 0; i < nrpages; i++) {
-		page = sg_page(&sg[i]);
-		if (!page)
-			break;
-		put_page(page);
-	}
-	kfree(sg);
-}
 
 /* Callers must take the port->outvq_lock */
 static void reclaim_consumed_buffers(struct port *port)
 {
-	struct buffer_token *tok;
+	struct port_buffer *buf;
 	unsigned int len;
 
 	if (!port->portdev) {
 		/* Device has been unplugged.  vqs are already gone. */
 		return;
 	}
-	while ((tok = virtqueue_get_buf(port->out_vq, &len))) {
-		if (tok->sgpages)
-			reclaim_sg_pages(tok->u.sg, tok->sgpages);
-		else
-			kfree(tok->u.buf);
-		kfree(tok);
+	while ((buf = virtqueue_get_buf(port->out_vq, &len))) {
+		free_buf(buf);
 		port->outvq_full = false;
 	}
 }
 
-static ssize_t __send_to_port(struct port *port, struct scatterlist *sg,
-			      int nents, size_t in_count,
-			      struct buffer_token *tok, bool nonblock)
+static ssize_t send_buf(struct port *port, struct port_buffer *buf, int nents,
+			      size_t in_count, bool nonblock)
 {
 	struct virtqueue *out_vq;
 	ssize_t ret;
@@ -534,7 +535,7 @@ static ssize_t __send_to_port(struct port *port, struct scatterlist *sg,
 
 	reclaim_consumed_buffers(port);
 
-	ret = virtqueue_add_buf(out_vq, sg, nents, 0, tok, GFP_ATOMIC);
+	ret = virtqueue_add_buf(out_vq, buf->sg, nents, 0, buf, GFP_ATOMIC);
 
 	/* Tell Host to go! */
 	virtqueue_kick(out_vq);
@@ -559,8 +560,11 @@ static ssize_t __send_to_port(struct port *port, struct scatterlist *sg,
 	 * we need to kmalloc a GFP_ATOMIC buffer each time the
 	 * console driver writes something out.
 	 */
-	while (!virtqueue_get_buf(out_vq, &len))
+	for (buf = virtqueue_get_buf(out_vq, &len); !buf;
+	     buf = virtqueue_get_buf(out_vq, &len))
 		cpu_relax();
+
+	free_buf(buf);
 done:
 	spin_unlock_irqrestore(&port->outvq_lock, flags);
 
@@ -572,36 +576,6 @@ done:
 	return in_count;
 }
 
-static ssize_t send_buf(struct port *port, void *in_buf, size_t in_count,
-			bool nonblock)
-{
-	struct scatterlist sg[1];
-	struct buffer_token *tok;
-
-	tok = kmalloc(sizeof(*tok), GFP_ATOMIC);
-	if (!tok)
-		return -ENOMEM;
-	tok->sgpages = 0;
-	tok->u.buf = in_buf;
-
-	sg_init_one(sg, in_buf, in_count);
-
-	return __send_to_port(port, sg, 1, in_count, tok, nonblock);
-}
-
-static ssize_t send_pages(struct port *port, struct scatterlist *sg, int nents,
-			  size_t in_count, bool nonblock)
-{
-	struct buffer_token *tok;
-
-	tok = kmalloc(sizeof(*tok), GFP_ATOMIC);
-	if (!tok)
-		return -ENOMEM;
-	tok->sgpages = nents;
-	tok->u.sg = sg;
-
-	return __send_to_port(port, sg, nents, in_count, tok, nonblock);
-}
 
 /*
  * Give out the data that's requested from the buffer that we have
@@ -748,7 +722,7 @@ static ssize_t port_fops_write(struct file *filp, const char __user *ubuf,
 			       size_t count, loff_t *offp)
 {
 	struct port *port;
-	char *buf;
+	struct port_buffer *buf;
 	ssize_t ret;
 	bool nonblock;
 
@@ -766,11 +740,11 @@ static ssize_t port_fops_write(struct file *filp, const char __user *ubuf,
 
 	count = min((size_t)(32 * 1024), count);
 
-	buf = kmalloc(count, GFP_KERNEL);
+	buf = alloc_buf(port->out_vq, count, 0);
 	if (!buf)
 		return -ENOMEM;
 
-	ret = copy_from_user(buf, ubuf, count);
+	ret = copy_from_user(buf->buf, ubuf, count);
 	if (ret) {
 		ret = -EFAULT;
 		goto free_buf;
@@ -784,13 +758,13 @@ static ssize_t port_fops_write(struct file *filp, const char __user *ubuf,
 	 * through to the host.
 	 */
 	nonblock = true;
-	ret = send_buf(port, buf, count, nonblock);
+	ret = send_buf(port, buf, 1, count, nonblock);
 
 	if (nonblock && ret > 0)
 		goto out;
 
 free_buf:
-	kfree(buf);
+	free_buf(buf);
 out:
 	return ret;
 }
@@ -856,6 +830,7 @@ static ssize_t port_fops_splice_write(struct pipe_inode_info *pipe,
 	struct port *port = filp->private_data;
 	struct sg_list sgl;
 	ssize_t ret;
+	struct port_buffer *buf;
 	struct splice_desc sd = {
 		.total_len = len,
 		.flags = flags,
@@ -867,17 +842,17 @@ static ssize_t port_fops_splice_write(struct pipe_inode_info *pipe,
 	if (ret < 0)
 		return ret;
 
+	buf = alloc_buf(port->out_vq, 0, pipe->nrbufs);
 	sgl.n = 0;
 	sgl.len = 0;
 	sgl.size = pipe->nrbufs;
-	sgl.sg = kmalloc(sizeof(struct scatterlist) * sgl.size, GFP_KERNEL);
-	if (unlikely(!sgl.sg))
-		return -ENOMEM;
-
+	sgl.sg = buf->sg;
 	sg_init_table(sgl.sg, sgl.size);
 	ret = __splice_from_pipe(pipe, &sd, pipe_to_sg);
 	if (likely(ret > 0))
-		ret = send_pages(port, sgl.sg, sgl.n, sgl.len, true);
+		ret = send_buf(port, buf, sgl.n, sgl.len, true);
+	else
+		free_buf(buf);
 
 	return ret;
 }
@@ -1031,6 +1006,7 @@ static const struct file_operations port_fops = {
 static int put_chars(u32 vtermno, const char *buf, int count)
 {
 	struct port *port;
+	struct port_buffer *port_buf;
 
 	if (unlikely(early_put_chars))
 		return early_put_chars(vtermno, buf, count);
@@ -1039,7 +1015,13 @@ static int put_chars(u32 vtermno, const char *buf, int count)
 	if (!port)
 		return -EPIPE;
 
-	return send_buf(port, (void *)buf, count, false);
+	port_buf = alloc_buf(port->out_vq, count, 0);
+	if (port_buf == NULL)
+		return -ENOMEM;
+
+	memcpy(port_buf->buf, buf, count);
+
+	return send_buf(port, port_buf, 1, count, false);
 }
 
 /*
@@ -1260,10 +1242,11 @@ 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, 0);
 		if (!buf)
 			break;
 
+		memset(buf->buf, 0, PAGE_SIZE);
 		spin_lock_irq(lock);
 		ret = add_inbuf(vq, buf);
 		if (ret < 0) {
-- 
1.7.5.4

[PATCHv5 2/3] virtio_console: Add support for remoteproc serial

[sjur.brandeland]

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

Add a simple serial connection driver called
VIRTIO_ID_RPROC_SERIAL (11) for communicating with a
remote processor in an asymmetric multi-processing
configuration.

This implementation reuses the existing virtio_console
implementation, and adds support for DMA allocation
of data buffers and disables use of tty console and
the virtio control queue.

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: Amit Shah <amit.shah@redhat.com>
cc: Ohad Ben-Cohen <ohad@wizery.com>
cc: Linus Walleij <linus.walleij@linaro.org>
cc: Arnd Bergmann <arnd@arndb.de>
---
Changes since v4:
- New baseline
- Use name is_rproc_enabled
- Renamed list and spin-lock used for pending deletion of dma buffers
- Minor style fixes: indentation, removed brace


 drivers/char/virtio_console.c |  184 +++++++++++++++++++++++++++++++++++++----
 include/linux/virtio_ids.h    |    1 +
 2 files changed, 170 insertions(+), 15 deletions(-)

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index f4f7b04..faedd2c 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -37,8 +37,12 @@
 #include <linux/wait.h>
 #include <linux/workqueue.h>
 #include <linux/module.h>
+#include <linux/dma-mapping.h>
+#include <linux/kconfig.h>
 #include "../tty/hvc/hvc_console.h"
 
+#define is_rproc_enabled IS_ENABLED(CONFIG_REMOTEPROC)
+
 /*
  * This is a global struct for storing common data for all the devices
  * this driver handles.
@@ -112,6 +116,15 @@ struct port_buffer {
 	/* offset in the buf from which to consume data */
 	size_t offset;
 
+	/* DMA address of buffer */
+	dma_addr_t dma;
+
+	/* Device we got DMA memory from */
+	struct device *dev;
+
+	/* List of pending dma buffers to free */
+	struct list_head list;
+
 	/* If sgpages == 0 then buf is used, else sg is used */
 	unsigned int sgpages;
 
@@ -330,6 +343,11 @@ static bool is_console_port(struct port *port)
 	return false;
 }
 
+static bool is_rproc_serial(const struct virtio_device *vdev)
+{
+	return is_rproc_enabled && vdev->id.device == VIRTIO_ID_RPROC_SERIAL;
+}
+
 static inline bool use_multiport(struct ports_device *portdev)
 {
 	/*
@@ -341,32 +359,84 @@ static inline bool use_multiport(struct ports_device *portdev)
 	return portdev->vdev->features[0] & (1 << VIRTIO_CONSOLE_F_MULTIPORT);
 }
 
+static DEFINE_SPINLOCK(dma_bufs_lock);
+static LIST_HEAD(pending_free_dma_bufs);
+
 static void free_buf(struct port_buffer *buf)
 {
 	int i;
+	unsigned long flags;
 
-	kfree(buf->buf);
+	if (!buf->dev)
+		kfree(buf->buf);
 
-	if (buf->sgpages)
+	if (buf->sgpages) {
 		for (i = 0; i < buf->sgpages; i++) {
 			struct page *page = sg_page(&buf->sg[i]);
 			if (!page)
 				break;
 			put_page(page);
 		}
+		return;
+	}
+
+	if (buf->dev && is_rproc_enabled) {
+
+		/* dma_free_coherent requires interrupts to be enabled. */
+		if (irqs_disabled()) {
+			/* queue up dma-buffers to be freed later */
+			spin_lock_irqsave(&dma_bufs_lock, flags);
+			list_add_tail(&buf->list, &pending_free_dma_bufs);
+			spin_unlock_irqrestore(&dma_bufs_lock, flags);
+			return;
+		}
+		dma_free_coherent(buf->dev, buf->size, buf->buf, buf->dma);
+
+		/* Release device refcnt and allow it to be freed */
+		might_sleep();
+		put_device(buf->dev);
+	}
 
 	kfree(buf);
 }
 
+static void reclaim_dma_bufs(void)
+{
+	unsigned long flags;
+	struct port_buffer *buf, *tmp;
+	LIST_HEAD(tmp_list);
+
+	WARN_ON(irqs_disabled());
+	if (list_empty(&pending_free_dma_bufs))
+		return;
+
+	BUG_ON(!is_rproc_enabled);
+
+	/* Create a copy of the pending_free_dma_bufs while holding the lock*/
+	spin_lock_irqsave(&dma_bufs_lock, flags);
+	list_cut_position(&tmp_list, &pending_free_dma_bufs,
+			  pending_free_dma_bufs.prev);
+	spin_unlock_irqrestore(&dma_bufs_lock, flags);
+
+	/* Release the dma buffers, without irqs enabled */
+	list_for_each_entry_safe(buf, tmp, &tmp_list, list) {
+		list_del(&buf->list);
+		free_buf(buf);
+	}
+}
+
 static struct port_buffer *alloc_buf(struct virtqueue *vq, size_t buf_size,
 				     int nrbufs)
 {
 	struct port_buffer *buf;
 	size_t alloc_size;
 
+	if (is_rproc_serial(vq->vdev) && !irqs_disabled())
+		reclaim_dma_bufs();
+
 	/* Allocate buffer and the scatter list */
 	alloc_size = sizeof(*buf) + sizeof(struct scatterlist) * nrbufs;
-	buf = kmalloc(alloc_size, GFP_ATOMIC);
+	buf = kzalloc(alloc_size, GFP_ATOMIC);
 	if (!buf)
 		goto fail;
 
@@ -374,11 +444,30 @@ static struct port_buffer *alloc_buf(struct virtqueue *vq, size_t buf_size,
 	if (nrbufs > 0)
 		return buf;
 
-	buf->buf = kmalloc(buf_size, GFP_ATOMIC);
+	if (is_rproc_serial(vq->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.
+		 * The code here would have been less quirky if
+		 * DMA_MEMORY_INCLUDES_CHILDREN had been supported
+		 * in dma-coherent.c
+		 */
+		if (!vq->vdev->dev.parent || !vq->vdev->dev.parent->parent)
+			goto free_buf;
+		buf->dev = vq->vdev->dev.parent->parent;
+
+		/* Increase device refcnt to avoid freeing it*/
+		get_device(buf->dev);
+		buf->buf = dma_alloc_coherent(buf->dev, buf_size, &buf->dma,
+					      GFP_ATOMIC);
+	} else {
+		buf->buf = kmalloc(buf_size, GFP_ATOMIC);
+	}
+
 	if (!buf->buf)
 		goto free_buf;
-	buf->len = 0;
-	buf->offset = 0;
 	buf->size = buf_size;
 
 	/* Prepare scatter buffer for sending */
@@ -838,6 +927,10 @@ static ssize_t port_fops_splice_write(struct pipe_inode_info *pipe,
 		.u.data = &sgl,
 	};
 
+	/* rproc_serial does not support splice */
+	if (is_rproc_serial(port->out_vq->vdev))
+		return -EINVAL;
+
 	ret = wait_port_writable(port, filp->f_flags & O_NONBLOCK);
 	if (ret < 0)
 		return ret;
@@ -902,6 +995,8 @@ static int port_fops_release(struct inode *inode, struct file *filp)
 	reclaim_consumed_buffers(port);
 	spin_unlock_irq(&port->outvq_lock);
 
+	if (is_rproc_serial(port->portdev->vdev) && !irqs_disabled())
+		reclaim_dma_bufs();
 	/*
 	 * Locks aren't necessary here as a port can't be opened after
 	 * unplug, and if a port isn't unplugged, a kref would already
@@ -1058,7 +1153,10 @@ static void resize_console(struct port *port)
 		return;
 
 	vdev = port->portdev->vdev;
-	if (virtio_has_feature(vdev, VIRTIO_CONSOLE_F_SIZE))
+
+	/* Don't test F_SIZE at all if we're rproc: not a valid feature! */
+	if (!is_rproc_serial(vdev) &&
+	    virtio_has_feature(vdev, VIRTIO_CONSOLE_F_SIZE))
 		hvc_resize(port->cons.hvc, port->cons.ws);
 }
 
@@ -1339,10 +1437,18 @@ static int add_port(struct ports_device *portdev, u32 id)
 		goto free_device;
 	}
 
-	/*
-	 * If we're not using multiport support, this has to be a console port
-	 */
-	if (!use_multiport(port->portdev)) {
+	if (is_rproc_serial(port->portdev->vdev))
+		/*
+		 * For rproc_serial assume remote processor is connected.
+		 * rproc_serial does not want the console port, only
+		 * the generic port implementation.
+		 */
+		port->host_connected = true;
+	else if (!use_multiport(port->portdev)) {
+		/*
+		 * If we're not using multiport support,
+		 * this has to be a console port.
+		 */
 		err = init_port_console(port);
 		if (err)
 			goto free_inbufs;
@@ -1418,6 +1524,15 @@ 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);
+
+	/*
+	 * Remove buffers from out queue for rproc-serial. We cannot afford
+	 * to leak any DMA mem, so reclaim this memory even if this might be
+	 * racy for the remote processor.
+	 */
+	if (is_rproc_serial(port->portdev->vdev))
+		while ((buf = virtqueue_detach_unused_buf(port->out_vq)))
+			free_buf(buf);
 }
 
 /*
@@ -1865,11 +1980,15 @@ static int __devinit virtcons_probe(struct virtio_device *vdev)
 
 	multiport = false;
 	portdev->config.max_nr_ports = 1;
-	if (virtio_config_val(vdev, VIRTIO_CONSOLE_F_MULTIPORT,
-			      offsetof(struct virtio_console_config,
-				       max_nr_ports),
-			      &portdev->config.max_nr_ports) == 0)
+
+	/* Don't test MULTIPORT at all if we're rproc: not a valid feature! */
+	if (!is_rproc_serial(vdev) &&
+	    virtio_config_val(vdev, VIRTIO_CONSOLE_F_MULTIPORT,
+				  offsetof(struct virtio_console_config,
+					   max_nr_ports),
+				  &portdev->config.max_nr_ports) == 0) {
 		multiport = true;
+	}
 
 	err = init_vqs(portdev);
 	if (err < 0) {
@@ -1979,6 +2098,16 @@ static unsigned int features[] = {
 	VIRTIO_CONSOLE_F_MULTIPORT,
 };
 
+static struct virtio_device_id rproc_serial_id_table[] = {
+#if IS_ENABLED(CONFIG_REMOTEPROC)
+	{ VIRTIO_ID_RPROC_SERIAL, VIRTIO_DEV_ANY_ID },
+#endif
+	{ 0 },
+};
+
+static unsigned int rproc_serial_features[] = {
+};
+
 #ifdef CONFIG_PM
 static int virtcons_freeze(struct virtio_device *vdev)
 {
@@ -2063,6 +2192,20 @@ static struct virtio_driver virtio_console = {
 #endif
 };
 
+/*
+ * virtio_rproc_serial refers to __devinit function which causes
+ * section mismatch warnings. So use __refdata to silence warnings.
+ */
+static struct virtio_driver __refdata virtio_rproc_serial = {
+	.feature_table = rproc_serial_features,
+	.feature_table_size = ARRAY_SIZE(rproc_serial_features),
+	.driver.name =	"virtio_rproc_serial",
+	.driver.owner =	THIS_MODULE,
+	.id_table =	rproc_serial_id_table,
+	.probe =	virtcons_probe,
+	.remove =	virtcons_remove,
+};
+
 static int __init init(void)
 {
 	int err;
@@ -2087,7 +2230,15 @@ static int __init init(void)
 		pr_err("Error %d registering virtio driver\n", err);
 		goto free;
 	}
+	err = register_virtio_driver(&virtio_rproc_serial);
+	if (err < 0) {
+		pr_err("Error %d registering virtio rproc serial driver\n",
+		       err);
+		goto unregister;
+	}
 	return 0;
+unregister:
+	unregister_virtio_driver(&virtio_console);
 free:
 	if (pdrvdata.debugfs_dir)
 		debugfs_remove_recursive(pdrvdata.debugfs_dir);
@@ -2097,7 +2248,10 @@ free:
 
 static void __exit fini(void)
 {
+	reclaim_dma_bufs();
+
 	unregister_virtio_driver(&virtio_console);
+	unregister_virtio_driver(&virtio_rproc_serial);
 
 	class_destroy(pdrvdata.class);
 	if (pdrvdata.debugfs_dir)
diff --git a/include/linux/virtio_ids.h b/include/linux/virtio_ids.h
index 270fb22..cb28b52 100644
--- a/include/linux/virtio_ids.h
+++ b/include/linux/virtio_ids.h
@@ -37,5 +37,6 @@
 #define VIRTIO_ID_RPMSG		7 /* virtio remote processor messaging */
 #define VIRTIO_ID_SCSI		8 /* virtio scsi */
 #define VIRTIO_ID_9P		9 /* 9p virtio console */
+#define VIRTIO_ID_RPROC_SERIAL	11 /* virtio remoteproc serial link */
 
 #endif /* _LINUX_VIRTIO_IDS_H */
-- 
1.7.5.4

[PATCH 3/3] virtio_console: Don't initialize buffers to zero

[sjur.brandeland]

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

Skip initializing the receive buffers.

Signed-off-by: Sjur Brændeland <sjur.brandeland@stericsson.com>
---
 drivers/char/virtio_console.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index faedd2c..e7d8787 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -1344,7 +1344,6 @@ static unsigned int fill_queue(struct virtqueue *vq, spinlock_t *lock)
 		if (!buf)
 			break;
 
-		memset(buf->buf, 0, PAGE_SIZE);
 		spin_lock_irq(lock);
 		ret = add_inbuf(vq, buf);
 		if (ret < 0) {
-- 
1.7.5.4

Re: [PATCH 1/3] virtio_console:Merge struct buffer_token into struct port_buffer

[Masami Hiramatsu]

(2012/09/25 22:47), sjur.brandeland@stericsson.com wrote:
> From: Sjur Brændeland <sjur.brandeland@stericsson.com>
> 
> This merge reduces code size by unifying the approach for
> sending scatter-lists and regular buffers. Any type of
> write operation (splice, write, put_chars) will now allocate
> a port_buffer and send_buf() and free_buf() can always be used.

Thanks!
This looks much nicer and simpler. I just have some comments below.

> 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: Amit Shah <amit.shah@redhat.com>
> cc: Linus Walleij <linus.walleij@linaro.org>
> cc: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
> ---
>  drivers/char/virtio_console.c |  141 ++++++++++++++++++-----------------------
>  1 files changed, 62 insertions(+), 79 deletions(-)
> 
> diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
> index 8ab9c3d..f4f7b04 100644
> --- a/drivers/char/virtio_console.c
> +++ b/drivers/char/virtio_console.c
> @@ -111,6 +111,11 @@ struct port_buffer {
>  	size_t len;
>  	/* offset in the buf from which to consume data */
>  	size_t offset;
> +
> +	/* If sgpages == 0 then buf is used, else sg is used */
> +	unsigned int sgpages;
> +
> +	struct scatterlist sg[1];
>  };
>  
>  /*
> @@ -338,23 +343,46 @@ static inline bool use_multiport(struct ports_device *portdev)
>  
>  static void free_buf(struct port_buffer *buf)
>  {
> +	int i;
> +
>  	kfree(buf->buf);

this should be done only when !buf->sgpages, or (see below)

> +
> +	if (buf->sgpages)
> +		for (i = 0; i < buf->sgpages; i++) {
> +			struct page *page = sg_page(&buf->sg[i]);
> +			if (!page)
> +				break;
> +			put_page(page);
> +		}
> +
>  	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,
> +				     int nrbufs)
>  {
>  	struct port_buffer *buf;
> +	size_t alloc_size;
>  
> -	buf = kmalloc(sizeof(*buf), GFP_KERNEL);
> +	/* Allocate buffer and the scatter list */
> +	alloc_size = sizeof(*buf) + sizeof(struct scatterlist) * nrbufs;

This allocates one redundant sg entry when nrbuf > 0,
but I think it is OK. (just a comment)

> +	buf = kmalloc(alloc_size, GFP_ATOMIC);

This should be kzalloc(), or buf->buf and others are not initialized,
which will cause unexpected kfree bug at kfree(buf->buf) in free_buf.

>  	if (!buf)
>  		goto fail;
> -	buf->buf = kzalloc(buf_size, GFP_KERNEL);
> +
> +	buf->sgpages = nrbufs;
> +	if (nrbufs > 0)
> +		return buf;
> +
> +	buf->buf = kmalloc(buf_size, GFP_ATOMIC);

You can also use kzalloc here as previous code does.
But if the reason why using kzalloc comes from the security,
I think kmalloc is enough here, since the host can access
all the guest pages anyway.

>  	if (!buf->buf)
>  		goto free_buf;
>  	buf->len = 0;
>  	buf->offset = 0;
>  	buf->size = buf_size;
> +
> +	/* Prepare scatter buffer for sending */
> +	sg_init_one(buf->sg, buf->buf, buf_size);
>  	return buf;
>  
>  free_buf:

Thank you,


-- 
Masami HIRAMATSU
Software Platform Research Dept. Linux Technology Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: masami.hiramatsu.pt@hitachi.com

RE: [PATCH 1/3] virtio_console:Merge struct buffer_token into struct port_buffer

[Sjur BRENDELAND]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 2641 bytes --]

> > This merge reduces code size by unifying the approach for
> > sending scatter-lists and regular buffers. Any type of
> > write operation (splice, write, put_chars) will now allocate
> > a port_buffer and send_buf() and free_buf() can always be used.
> 
> Thanks!
> This looks much nicer and simpler. I just have some comments below.

OK, good to hear that you agree to this kind of change. I'll do a respin
of this patch fixing the issues you have pointed out.

> >  static void free_buf(struct port_buffer *buf)
> >  {
> > +	int i;
> > +
> >  	kfree(buf->buf);
> 
> this should be done only when !buf->sgpages, or (see below)

Agree, I'll put this statement in an else branch to the if-statement below.

> 
> > +
> > +	if (buf->sgpages)
> > +		for (i = 0; i < buf->sgpages; i++) {
> > +			struct page *page = sg_page(&buf->sg[i]);
> > +			if (!page)
> > +				break;
> > +			put_page(page);
> > +		}
> > +
> >  	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,
> > +				     int nrbufs)
> >  {
> >  	struct port_buffer *buf;
> > +	size_t alloc_size;
> >
> > -	buf = kmalloc(sizeof(*buf), GFP_KERNEL);
> > +	/* Allocate buffer and the scatter list */
> > +	alloc_size = sizeof(*buf) + sizeof(struct scatterlist) * nrbufs;
> 
> This allocates one redundant sg entry when nrbuf > 0,
> but I think it is OK. (just a comment)

I did this on purpose for the sake of simplicity, but I can
change this to something like:
	 alloc_size = sizeof(*buf) + sizeof(buf->sg) * max(nrbufs - 1, 1);


> > +	buf = kmalloc(alloc_size, GFP_ATOMIC);
> 
> This should be kzalloc(), or buf->buf and others are not initialized,
> which will cause unexpected kfree bug at kfree(buf->buf) in free_buf.

Agree, kzalloc() is better in this case. 

> >  	if (!buf)
> >  		goto fail;
> > -	buf->buf = kzalloc(buf_size, GFP_KERNEL);
> > +
> > +	buf->sgpages = nrbufs;
> > +	if (nrbufs > 0)
> > +		return buf;
> > +
> > +	buf->buf = kmalloc(buf_size, GFP_ATOMIC);
> 
> You can also use kzalloc here as previous code does.
> But if the reason why using kzalloc comes from the security,
> I think kmalloc is enough here, since the host can access
> all the guest pages anyway.

With this new patch alloc_buf() is used both for both RX and TX.
The out_vq did previously use malloc(). But I have preserved
the legacy behavior for the in_vq by calling memset() in function
fill_queue().

Thanks,
Sjur
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH 1/3] virtio_console:Merge struct buffer_token into struct port_buffer

[Masami Hiramatsu]

(2012/09/26 16:48), Sjur BRENDELAND wrote:
>>> -static struct port_buffer *alloc_buf(size_t buf_size)
>>> +static struct port_buffer *alloc_buf(struct virtqueue *vq, size_t buf_size,
>>> +				     int nrbufs)
>>>  {
>>>  	struct port_buffer *buf;
>>> +	size_t alloc_size;
>>>
>>> -	buf = kmalloc(sizeof(*buf), GFP_KERNEL);
>>> +	/* Allocate buffer and the scatter list */
>>> +	alloc_size = sizeof(*buf) + sizeof(struct scatterlist) * nrbufs;
>>
>> This allocates one redundant sg entry when nrbuf > 0,
>> but I think it is OK. (just a comment)
> 
> I did this on purpose for the sake of simplicity, but I can
> change this to something like:
> 	 alloc_size = sizeof(*buf) + sizeof(buf->sg) * max(nrbufs - 1, 1);

You wouldn't need to change that. I think current code is enough simple
and reasonable. :)

Thanks!

-- 
Masami HIRAMATSU
Software Platform Research Dept. Linux Technology Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: masami.hiramatsu.pt@hitachi.com

RE: [PATCH 1/3] virtio_console:Merge struct buffer_token into struct port_buffer

[Rusty Russell]

Sjur BRENDELAND <sjur.brandeland@stericsson.com> writes:
>> This allocates one redundant sg entry when nrbuf > 0,
>> but I think it is OK. (just a comment)
>
> I did this on purpose for the sake of simplicity, but I can
> change this to something like:
> 	 alloc_size = sizeof(*buf) + sizeof(buf->sg) * max(nrbufs - 1, 1);

That's why we use [0] in the definition (a GCC extension).

Cheers,
Rusty.

Re: [PATCHv5 2/3] virtio_console: Add support for remoteproc serial

[Rusty Russell]

sjur.brandeland@stericsson.com writes:
> From: Sjur Brændeland <sjur.brandeland@stericsson.com>
>
> Add a simple serial connection driver called
> VIRTIO_ID_RPROC_SERIAL (11) for communicating with a
> remote processor in an asymmetric multi-processing
> configuration.
>
...
>  static struct port_buffer *alloc_buf(struct virtqueue *vq, size_t buf_size,
>  				     int nrbufs)
>  {
>  	struct port_buffer *buf;
>  	size_t alloc_size;
>  
> +	if (is_rproc_serial(vq->vdev) && !irqs_disabled())
> +		reclaim_dma_bufs();

Hmm, you need a gfp_t arg into alloc_buf; your last patch simply changed
them all to GFP_ATOMIC, which makes the console far less memory
friendly.

You check !irqs_disabled() in a couple of places; I think the caller
needs to indicate (possibly by checking for gfp == GFP_KERNEL) whether
it's safe to call reclaim_dma_bufs().

> @@ -838,6 +927,10 @@ static ssize_t port_fops_splice_write(struct pipe_inode_info *pipe,
>  		.u.data = &sgl,
>  	};
>  
> +	/* rproc_serial does not support splice */
> +	if (is_rproc_serial(port->out_vq->vdev))
> +		return -EINVAL;

Why not? ;)

Thanks,
Rusty.

Re: [PATCHv6 0/3] virtio_console: Add rproc_serial device

[Amit Shah]

On (Tue) 25 Sep 2012 [15:47:14], sjur.brandeland@stericsson.com wrote:
> From: Sjur Brændeland <sjur.brandeland@stericsson.com>
> 
> I thought rebasing rproc_serial to linux-next was going to be trivial.
> But when starting the merge I realized that I had to refactor the
> the patches from  Masami Hiramatsu. The splice support has the same issue
> as I faced, with different type of buffers in the out_vq.
> So I ended up refactoring the splice functionality. The code
> size got smaller so hopefully this a step in the right direction.
> 
> This refactoring also make introduction of rproc_serial cleaner.
> 
> As requested I also added a patch for not initializing buffers.
> 
> I have tested the VIRTIO_CONSOLE device by looping large amount of data
> through character device and tty, with lockdep and slub-debug on.
> This looks stable for me. I've also done a simple test of splice.

Thanks; I just passed these patches through my testsuite and things
are working fine.  I'll review them in some time.

		Amit

Re: [PATCH 3/3] virtio_console: Don't initialize buffers to zero

[Amit Shah]

On (Tue) 25 Sep 2012 [15:47:17], sjur.brandeland@stericsson.com wrote:
> From: Sjur Brændeland <sjur.brandeland@stericsson.com>
> 
> Skip initializing the receive buffers.

This tells 'what', but not 'why'.  Please add some more description.
For the generic virtio ports case, at least, my original thinking was
to not send random guest data to the host device.  However, we don't
have any device isolation in the host yet, and doing that will be
expensive, so it's not going to be done in the near future.. this can
be safely skipped.

Also, please make this patch 1/3, so we don't end up doing

 kzalloc->kmalloc+memset->kmalloc

Thanks,
		Amit

Re: [PATCH 1/3] virtio_console:Merge struct buffer_token into struct port_buffer

[Amit Shah]

On (Tue) 25 Sep 2012 [15:47:15], sjur.brandeland@stericsson.com wrote:
> From: Sjur Brændeland <sjur.brandeland@stericsson.com>
> 
> This merge reduces code size by unifying the approach for
> sending scatter-lists and regular buffers. Any type of
> write operation (splice, write, put_chars) will now allocate
> a port_buffer and send_buf() and free_buf() can always be used.

Thanks for this cleanup; I should've caught it at the review of the
virtio-trace patchset itself -- sorry for that.

> 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: Amit Shah <amit.shah@redhat.com>
> cc: Linus Walleij <linus.walleij@linaro.org>
> cc: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
> ---
>  drivers/char/virtio_console.c |  141 ++++++++++++++++++-----------------------
>  1 files changed, 62 insertions(+), 79 deletions(-)
> 
> diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
> index 8ab9c3d..f4f7b04 100644
> --- a/drivers/char/virtio_console.c
> +++ b/drivers/char/virtio_console.c
> @@ -111,6 +111,11 @@ struct port_buffer {
>  	size_t len;
>  	/* offset in the buf from which to consume data */
>  	size_t offset;
> +
> +	/* If sgpages == 0 then buf is used, else sg is used */
> +	unsigned int sgpages;
> +
> +	struct scatterlist sg[1];
>  };
>  
>  /*
> @@ -338,23 +343,46 @@ static inline bool use_multiport(struct ports_device *portdev)
>  
>  static void free_buf(struct port_buffer *buf)
>  {
> +	int i;

unsigned int

> +
>  	kfree(buf->buf);

buf->buf isn't set to NULL in case sgpages is > 0.  Please set
buf->buf to NULL (and initialise other fields to default values) in
alloc_buf() (and leave this as is).

> +
> +	if (buf->sgpages)

This 'if' is not necessary; just having the for loop will do the right
thing.

> +		for (i = 0; i < buf->sgpages; i++) {
> +			struct page *page = sg_page(&buf->sg[i]);
> +			if (!page)
> +				break;
> +			put_page(page);
> +		}
> +
>  	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,
> +				     int nrbufs)

Indentation is off.

>  {
>  	struct port_buffer *buf;
> +	size_t alloc_size;
>  
> -	buf = kmalloc(sizeof(*buf), GFP_KERNEL);
> +	/* Allocate buffer and the scatter list */
> +	alloc_size = sizeof(*buf) + sizeof(struct scatterlist) * nrbufs;
> +	buf = kmalloc(alloc_size, GFP_ATOMIC);

This looks hacky, along with the 'struct scatterlist sg[1]' in
port_buffer above.  Use a pointer instead?  At the least, please
include a comment in struct port_buffer mentioning sg has to be the
last element in the struct.

>  	if (!buf)
>  		goto fail;
> -	buf->buf = kzalloc(buf_size, GFP_KERNEL);
> +
> +	buf->sgpages = nrbufs;
> +	if (nrbufs > 0)
> +		return buf;
> +
> +	buf->buf = kmalloc(buf_size, GFP_ATOMIC);

That's a lot of GFP_ATOMICS; even for the cases that don't need them.
Maybe add a gfp param that only allocates GFP_ATOMIC memory from
callers in interrupt context.  All existing code got switched to using
GFP_ATOMIC buffers as well, that's definitely not good.

>  	if (!buf->buf)
>  		goto free_buf;
>  	buf->len = 0;
>  	buf->offset = 0;
>  	buf->size = buf_size;
> +
> +	/* Prepare scatter buffer for sending */
> +	sg_init_one(buf->sg, buf->buf, buf_size);
>  	return buf;
>  
>  free_buf:
> @@ -476,52 +504,25 @@ static ssize_t send_control_msg(struct port *port, unsigned int event,
>  	return 0;
>  }
>  
> -struct buffer_token {
> -	union {
> -		void *buf;
> -		struct scatterlist *sg;
> -	} u;
> -	/* If sgpages == 0 then buf is used, else sg is used */
> -	unsigned int sgpages;
> -};
> -
> -static void reclaim_sg_pages(struct scatterlist *sg, unsigned int nrpages)
> -{
> -	int i;
> -	struct page *page;
> -
> -	for (i = 0; i < nrpages; i++) {
> -		page = sg_page(&sg[i]);
> -		if (!page)
> -			break;
> -		put_page(page);
> -	}
> -	kfree(sg);
> -}
>  
>  /* Callers must take the port->outvq_lock */
>  static void reclaim_consumed_buffers(struct port *port)
>  {
> -	struct buffer_token *tok;
> +	struct port_buffer *buf;
>  	unsigned int len;
>  
>  	if (!port->portdev) {
>  		/* Device has been unplugged.  vqs are already gone. */
>  		return;
>  	}
> -	while ((tok = virtqueue_get_buf(port->out_vq, &len))) {
> -		if (tok->sgpages)
> -			reclaim_sg_pages(tok->u.sg, tok->sgpages);
> -		else
> -			kfree(tok->u.buf);
> -		kfree(tok);
> +	while ((buf = virtqueue_get_buf(port->out_vq, &len))) {
> +		free_buf(buf);
>  		port->outvq_full = false;
>  	}
>  }
>  
> -static ssize_t __send_to_port(struct port *port, struct scatterlist *sg,
> -			      int nents, size_t in_count,
> -			      struct buffer_token *tok, bool nonblock)
> +static ssize_t send_buf(struct port *port, struct port_buffer *buf, int nents,
> +			      size_t in_count, bool nonblock)

Indentation is off.

>  {
>  	struct virtqueue *out_vq;
>  	ssize_t ret;
> @@ -534,7 +535,7 @@ static ssize_t __send_to_port(struct port *port, struct scatterlist *sg,
>  
>  	reclaim_consumed_buffers(port);
>  
> -	ret = virtqueue_add_buf(out_vq, sg, nents, 0, tok, GFP_ATOMIC);
> +	ret = virtqueue_add_buf(out_vq, buf->sg, nents, 0, buf, GFP_ATOMIC);
>  
>  	/* Tell Host to go! */
>  	virtqueue_kick(out_vq);
> @@ -559,8 +560,11 @@ static ssize_t __send_to_port(struct port *port, struct scatterlist *sg,
>  	 * we need to kmalloc a GFP_ATOMIC buffer each time the
>  	 * console driver writes something out.
>  	 */
> -	while (!virtqueue_get_buf(out_vq, &len))
> +	for (buf = virtqueue_get_buf(out_vq, &len); !buf;
> +	     buf = virtqueue_get_buf(out_vq, &len))
>  		cpu_relax();

Looks awkward, how about

	while (!(buf = virtqueue_get_buf(out_vq, &len))
		cpu_relax();

> +
> +	free_buf(buf);
>  done:
>  	spin_unlock_irqrestore(&port->outvq_lock, flags);
>  
> @@ -572,36 +576,6 @@ done:
>  	return in_count;
>  }
>  
> -static ssize_t send_buf(struct port *port, void *in_buf, size_t in_count,
> -			bool nonblock)
> -{
> -	struct scatterlist sg[1];
> -	struct buffer_token *tok;
> -
> -	tok = kmalloc(sizeof(*tok), GFP_ATOMIC);
> -	if (!tok)
> -		return -ENOMEM;
> -	tok->sgpages = 0;
> -	tok->u.buf = in_buf;
> -
> -	sg_init_one(sg, in_buf, in_count);
> -
> -	return __send_to_port(port, sg, 1, in_count, tok, nonblock);
> -}
> -
> -static ssize_t send_pages(struct port *port, struct scatterlist *sg, int nents,
> -			  size_t in_count, bool nonblock)
> -{
> -	struct buffer_token *tok;
> -
> -	tok = kmalloc(sizeof(*tok), GFP_ATOMIC);
> -	if (!tok)
> -		return -ENOMEM;
> -	tok->sgpages = nents;
> -	tok->u.sg = sg;
> -
> -	return __send_to_port(port, sg, nents, in_count, tok, nonblock);
> -}
>  
>  /*
>   * Give out the data that's requested from the buffer that we have
> @@ -748,7 +722,7 @@ static ssize_t port_fops_write(struct file *filp, const char __user *ubuf,
>  			       size_t count, loff_t *offp)
>  {
>  	struct port *port;
> -	char *buf;
> +	struct port_buffer *buf;
>  	ssize_t ret;
>  	bool nonblock;
>  
> @@ -766,11 +740,11 @@ static ssize_t port_fops_write(struct file *filp, const char __user *ubuf,
>  
>  	count = min((size_t)(32 * 1024), count);
>  
> -	buf = kmalloc(count, GFP_KERNEL);
> +	buf = alloc_buf(port->out_vq, count, 0);
>  	if (!buf)
>  		return -ENOMEM;
>  
> -	ret = copy_from_user(buf, ubuf, count);
> +	ret = copy_from_user(buf->buf, ubuf, count);
>  	if (ret) {
>  		ret = -EFAULT;
>  		goto free_buf;
> @@ -784,13 +758,13 @@ static ssize_t port_fops_write(struct file *filp, const char __user *ubuf,
>  	 * through to the host.
>  	 */
>  	nonblock = true;
> -	ret = send_buf(port, buf, count, nonblock);
> +	ret = send_buf(port, buf, 1, count, nonblock);
>  
>  	if (nonblock && ret > 0)
>  		goto out;
>  
>  free_buf:
> -	kfree(buf);
> +	free_buf(buf);
>  out:
>  	return ret;
>  }
> @@ -856,6 +830,7 @@ static ssize_t port_fops_splice_write(struct pipe_inode_info *pipe,
>  	struct port *port = filp->private_data;
>  	struct sg_list sgl;
>  	ssize_t ret;
> +	struct port_buffer *buf;
>  	struct splice_desc sd = {
>  		.total_len = len,
>  		.flags = flags,
> @@ -867,17 +842,17 @@ static ssize_t port_fops_splice_write(struct pipe_inode_info *pipe,
>  	if (ret < 0)
>  		return ret;
>  
> +	buf = alloc_buf(port->out_vq, 0, pipe->nrbufs);
>  	sgl.n = 0;
>  	sgl.len = 0;
>  	sgl.size = pipe->nrbufs;
> -	sgl.sg = kmalloc(sizeof(struct scatterlist) * sgl.size, GFP_KERNEL);
> -	if (unlikely(!sgl.sg))
> -		return -ENOMEM;
> -
> +	sgl.sg = buf->sg;
>  	sg_init_table(sgl.sg, sgl.size);
>  	ret = __splice_from_pipe(pipe, &sd, pipe_to_sg);
>  	if (likely(ret > 0))
> -		ret = send_pages(port, sgl.sg, sgl.n, sgl.len, true);
> +		ret = send_buf(port, buf, sgl.n, sgl.len, true);
> +	else
> +		free_buf(buf);
>  
>  	return ret;
>  }
> @@ -1031,6 +1006,7 @@ static const struct file_operations port_fops = {
>  static int put_chars(u32 vtermno, const char *buf, int count)
>  {
>  	struct port *port;
> +	struct port_buffer *port_buf;
>  
>  	if (unlikely(early_put_chars))
>  		return early_put_chars(vtermno, buf, count);
> @@ -1039,7 +1015,13 @@ static int put_chars(u32 vtermno, const char *buf, int count)
>  	if (!port)
>  		return -EPIPE;
>  
> -	return send_buf(port, (void *)buf, count, false);
> +	port_buf = alloc_buf(port->out_vq, count, 0);
> +	if (port_buf == NULL)
> +		return -ENOMEM;
> +
> +	memcpy(port_buf->buf, buf, count);

Hm, this introduces an unnecessary copy for console ports, and is
going to make console IO slower.  Can you think of a way to avoid
this?  (Remember, this is done in interrupt context.)

		Amit

Re: [PATCH 1/3] virtio_console:Merge struct buffer_token into struct port_buffer

[Amit Shah]

On (Wed) 26 Sep 2012 [09:48:12], Sjur BRENDELAND wrote:
> > > This merge reduces code size by unifying the approach for
> > > sending scatter-lists and regular buffers. Any type of
> > > write operation (splice, write, put_chars) will now allocate
> > > a port_buffer and send_buf() and free_buf() can always be used.
> > 
> > Thanks!
> > This looks much nicer and simpler. I just have some comments below.
> 
> OK, good to hear that you agree to this kind of change. I'll do a respin
> of this patch fixing the issues you have pointed out.
> 
> > >  static void free_buf(struct port_buffer *buf)
> > >  {
> > > +	int i;
> > > +
> > >  	kfree(buf->buf);
> > 
> > this should be done only when !buf->sgpages, or (see below)
> 
> Agree, I'll put this statement in an else branch to the if-statement below.

Not necessary; see my comments in another mail.

> > > -static struct port_buffer *alloc_buf(size_t buf_size)
> > > +static struct port_buffer *alloc_buf(struct virtqueue *vq, size_t buf_size,
> > > +				     int nrbufs)
> > >  {
> > >  	struct port_buffer *buf;
> > > +	size_t alloc_size;
> > >
> > > -	buf = kmalloc(sizeof(*buf), GFP_KERNEL);
> > > +	/* Allocate buffer and the scatter list */
> > > +	alloc_size = sizeof(*buf) + sizeof(struct scatterlist) * nrbufs;
> > 
> > This allocates one redundant sg entry when nrbuf > 0,
> > but I think it is OK. (just a comment)
> 
> I did this on purpose for the sake of simplicity, but I can
> change this to something like:
> 	 alloc_size = sizeof(*buf) + sizeof(buf->sg) * max(nrbufs - 1, 1);
> 
> 
> > > +	buf = kmalloc(alloc_size, GFP_ATOMIC);
> > 
> > This should be kzalloc(), or buf->buf and others are not initialized,
> > which will cause unexpected kfree bug at kfree(buf->buf) in free_buf.
> 
> Agree, kzalloc() is better in this case. 

Not really -- one thing I've picked up from Rusty is to use kmallocs
and explicitly initialise values.  Then, missing such initialisations
(like in this case) will cause tools like valgrind to show the error
pretty quickly.

> > >  	if (!buf)
> > >  		goto fail;
> > > -	buf->buf = kzalloc(buf_size, GFP_KERNEL);
> > > +
> > > +	buf->sgpages = nrbufs;
> > > +	if (nrbufs > 0)
> > > +		return buf;
> > > +
> > > +	buf->buf = kmalloc(buf_size, GFP_ATOMIC);
> > 
> > You can also use kzalloc here as previous code does.
> > But if the reason why using kzalloc comes from the security,
> > I think kmalloc is enough here, since the host can access
> > all the guest pages anyway.
> 
> With this new patch alloc_buf() is used both for both RX and TX.
> The out_vq did previously use malloc(). But I have preserved
> the legacy behavior for the in_vq by calling memset() in function
> fill_queue().

But we're dropping the memset/kzalloc anyway.

		Amit

Re: [PATCHv5 2/3] virtio_console: Add support for remoteproc serial

[Amit Shah]

On (Tue) 25 Sep 2012 [15:47:16], sjur.brandeland@stericsson.com wrote:

> +static DEFINE_SPINLOCK(dma_bufs_lock);
> +static LIST_HEAD(pending_free_dma_bufs);
> +
>  static void free_buf(struct port_buffer *buf)
>  {
>  	int i;
> +	unsigned long flags;
>  
> -	kfree(buf->buf);
> +	if (!buf->dev)
> +		kfree(buf->buf);

Doesn't hurt to just kfree a NULL pointer; so check is not needed.

> -	if (buf->sgpages)
> +	if (buf->sgpages) {
>  		for (i = 0; i < buf->sgpages; i++) {
>  			struct page *page = sg_page(&buf->sg[i]);
>  			if (!page)
>  				break;
>  			put_page(page);
>  		}
> +		return;
> +	}

No kfree(buf)?

> +	if (buf->dev && is_rproc_enabled) {
> +
> +		/* dma_free_coherent requires interrupts to be enabled. */
> +		if (irqs_disabled()) {
> +			/* queue up dma-buffers to be freed later */
> +			spin_lock_irqsave(&dma_bufs_lock, flags);
> +			list_add_tail(&buf->list, &pending_free_dma_bufs);
> +			spin_unlock_irqrestore(&dma_bufs_lock, flags);
> +			return;
> +		}
> +		dma_free_coherent(buf->dev, buf->size, buf->buf, buf->dma);
> +
> +		/* Release device refcnt and allow it to be freed */
> +		might_sleep();
> +		put_device(buf->dev);
> +	}
>  
>  	kfree(buf);
>  }
>  
> +static void reclaim_dma_bufs(void)
> +{
> +	unsigned long flags;
> +	struct port_buffer *buf, *tmp;
> +	LIST_HEAD(tmp_list);
> +
> +	WARN_ON(irqs_disabled());
> +	if (list_empty(&pending_free_dma_bufs))
> +		return;
> +
> +	BUG_ON(!is_rproc_enabled);
> +
> +	/* Create a copy of the pending_free_dma_bufs while holding the lock*/
> +	spin_lock_irqsave(&dma_bufs_lock, flags);
> +	list_cut_position(&tmp_list, &pending_free_dma_bufs,
> +			  pending_free_dma_bufs.prev);
> +	spin_unlock_irqrestore(&dma_bufs_lock, flags);
> +
> +	/* Release the dma buffers, without irqs enabled */
> +	list_for_each_entry_safe(buf, tmp, &tmp_list, list) {
> +		list_del(&buf->list);
> +		free_buf(buf);
> +	}
> +}
> +
>  static struct port_buffer *alloc_buf(struct virtqueue *vq, size_t buf_size,
>  				     int nrbufs)
>  {
>  	struct port_buffer *buf;
>  	size_t alloc_size;
>  
> +	if (is_rproc_serial(vq->vdev) && !irqs_disabled())
> +		reclaim_dma_bufs();
> +
>  	/* Allocate buffer and the scatter list */
>  	alloc_size = sizeof(*buf) + sizeof(struct scatterlist) * nrbufs;
> -	buf = kmalloc(alloc_size, GFP_ATOMIC);
> +	buf = kzalloc(alloc_size, GFP_ATOMIC);

Why change from kmalloc to kzalloc?  It's better to split out such
changes into separate patch for easier review.

>  	if (!buf)
>  		goto fail;
>  
> @@ -374,11 +444,30 @@ static struct port_buffer *alloc_buf(struct virtqueue *vq, size_t buf_size,
>  	if (nrbufs > 0)
>  		return buf;
>  
> -	buf->buf = kmalloc(buf_size, GFP_ATOMIC);
> +	if (is_rproc_serial(vq->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.
> +		 * The code here would have been less quirky if
> +		 * DMA_MEMORY_INCLUDES_CHILDREN had been supported
> +		 * in dma-coherent.c
> +		 */
> +		if (!vq->vdev->dev.parent || !vq->vdev->dev.parent->parent)
> +			goto free_buf;
> +		buf->dev = vq->vdev->dev.parent->parent;
> +
> +		/* Increase device refcnt to avoid freeing it*/

missing space before */

> +		get_device(buf->dev);
> +		buf->buf = dma_alloc_coherent(buf->dev, buf_size, &buf->dma,
> +					      GFP_ATOMIC);
> +	} else {
> +		buf->buf = kmalloc(buf_size, GFP_ATOMIC);
> +	}
> +
>  	if (!buf->buf)
>  		goto free_buf;
> -	buf->len = 0;
> -	buf->offset = 0;

As noted in a comment to previous patch, please use kmalloc and
explicit initialisation of variables.

>  	buf->size = buf_size;
>  
>  	/* Prepare scatter buffer for sending */
> @@ -838,6 +927,10 @@ static ssize_t port_fops_splice_write(struct pipe_inode_info *pipe,
>  		.u.data = &sgl,
>  	};
>  
> +	/* rproc_serial does not support splice */
> +	if (is_rproc_serial(port->out_vq->vdev))
> +		return -EINVAL;

Will something break if this is not done?


		Amit