* Re: [PATCHv4] virtio-spec: virtio network device multiqueue support
From: Michael S. Tsirkin @ 2012-09-19 6:12 UTC (permalink / raw)
To: Rusty Russell
Cc: kvm, netdev, rick.jones2, virtualization, levinsasha928, pbonzini,
Tom Herbert
In-Reply-To: <87wqzqpz7p.fsf@rustcorp.com.au>
On Wed, Sep 19, 2012 at 11:10:10AM +0930, Rusty Russell wrote:
> Tom Herbert <therbert@google.com> writes:
> > On Tue, Sep 11, 2012 at 10:49 PM, Rusty Russell <rusty@rustcorp.com.au>wrote:
> >> Perhaps Tom can explain how we avoid out-of-order receive for the
> >> accelerated RFS case? It's not clear to me, but we need to be able to
> >> do that for virtio-net if it implements accelerated RFS.
> >
> > AFAIK ooo RX is possible with accelerated RFS. We have an algorithm that
> > prevents this for RFS case by deferring a migration to a new queue as long
> > as it's possible that a flow might have outstanding packets on the old
> > queue. I suppose this could be implemented in the device for the HW
> > queues, but I don't think it would be easy to cover all cases where packets
> > were already in transit to the host or other cases where host and device
> > queues are out of sync.
>
> Having gone to such great lengths to avoid ooo for RFS, I don't think
> DaveM would be happy if we allow it for virtio_net.
>
> So, how *would* we implement such a thing for a "hardware" device? What
> if the device will only change the receive queue if the old receive
> queue is empty?
>
> Cheers,
> Rusty.
>
I think that would do it in most cases. Or if we want to be more
exact we could delay switching a specific flow until no
outstanding rx packets for this flow. Not sure it's worth the
hassle.
--
MST
^ permalink raw reply
* [PATCHv2] virtio_console: Add support for remoteproc serial
From: sjur.brandeland @ 2012-09-19 16:50 UTC (permalink / raw)
To: Rusty Russell
Cc: Sjur Brændeland, Michael S. Tsirkin, Linus Walleij,
linux-kernel, virtualization, Amit Shah, Sjur Brændeland
From: Sjur Brændeland <sjur.brandeland@stericsson.com>
Add a simple serial connection driver called
VIRTIO_ID_RPROC_SERIAL (0xB) 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.
This enables use of the exising virtio_console code in
the remoteproc framework.
Signed-off-by: Sjur Brændeland <sjur.brandeland@stericsson.com>
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>
---
Hi Rusty,
Changes since v1:
o Use proper macros IS_ENABLED(CONFIG_REMOTEPROC)
o Fix bug at registration of rproc_serial driver
o Always allocate PAGE_SIZE buffers for rproc_serial,
and limit write to port_fops_write to use PAGE_SIZE.
Regards,
Sjur
drivers/char/virtio_console.c | 160 ++++++++++++++++++++++++++++++++++-------
include/linux/virtio_ids.h | 1 +
2 files changed, 134 insertions(+), 27 deletions(-)
diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index cdf2f54..395522a 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -35,6 +35,8 @@
#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"
/*
@@ -323,6 +325,52 @@ static bool is_console_port(struct port *port)
return false;
}
+#if IS_ENABLED(CONFIG_REMOTEPROC)
+static inline bool is_rproc_serial(struct virtio_device *vdev)
+{
+ return vdev->id.device == VIRTIO_ID_RPROC_SERIAL;
+}
+#else
+static inline bool is_rproc_serial(struct virtio_device *vdev)
+{
+ return false;
+}
+#endif
+
+/* Allocate data buffer from DMA memory if requested */
+static inline void *
+alloc_databuf(struct virtio_device *vdev, size_t size, gfp_t flag)
+{
+ if (is_rproc_serial(vdev)) {
+ dma_addr_t dma;
+ 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, flag);
+ }
+ return kmalloc(size, flag);
+}
+
+static inline void
+free_databuf(struct virtio_device *vdev, size_t size, void *cpu_addr)
+{
+
+ if (is_rproc_serial(vdev)) {
+ struct device *dev = &vdev->dev;
+ dma_addr_t dma_handle = virt_to_bus(cpu_addr);
+ dev = dev->parent ? dev->parent : dev;
+ dev = dev->parent ? dev->parent : dev;
+ dma_free_coherent(dev, size, cpu_addr, dma_handle);
+ return;
+ }
+ kfree(cpu_addr);
+}
+
static inline bool use_multiport(struct ports_device *portdev)
{
/*
@@ -334,20 +382,22 @@ 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)
+static void
+free_buf(struct virtqueue *vq, struct port_buffer *buf, size_t buf_size)
{
- kfree(buf->buf);
+ free_databuf(vq->vdev, buf_size, buf->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;
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, GFP_KERNEL);
+ memset(buf->buf, 0, buf_size);
if (!buf->buf)
goto free_buf;
buf->len = 0;
@@ -414,7 +464,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 +535,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, PAGE_SIZE, buf);
port->outvq_full = false;
}
}
@@ -672,6 +722,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;
+ size_t buf_size;
/* Userspace could be out to fool us */
if (!count)
@@ -694,9 +746,16 @@ static ssize_t port_fops_write(struct file *filp, const char __user *ubuf,
if (!port->guest_connected)
return -ENODEV;
- count = min((size_t)(32 * 1024), count);
+ vdev = port->portdev->vdev;
+
+ if (is_rproc_serial(vdev)) {
+ count = min((size_t)PAGE_SIZE, count);
+ buf_size = PAGE_SIZE;
+ } else
+ buf_size = count = min((size_t)(32 * 1024), count);
+
+ buf = alloc_databuf(vdev, buf_size, GFP_KERNEL);
- buf = kmalloc(count, GFP_KERNEL);
if (!buf)
return -ENOMEM;
@@ -720,7 +779,7 @@ static ssize_t port_fops_write(struct file *filp, const char __user *ubuf,
goto out;
free_buf:
- kfree(buf);
+ free_databuf(vdev, buf_size, buf);
out:
return ret;
}
@@ -767,6 +826,7 @@ static int port_fops_release(struct inode *inode, struct file *filp)
spin_unlock_irq(&port->inbuf_lock);
spin_lock_irq(&port->outvq_lock);
+
reclaim_consumed_buffers(port);
spin_unlock_irq(&port->outvq_lock);
@@ -918,7 +978,8 @@ static void resize_console(struct port *port)
return;
vdev = port->portdev->vdev;
- if (virtio_has_feature(vdev, VIRTIO_CONSOLE_F_SIZE))
+ if (!is_rproc_serial(vdev) &&
+ virtio_has_feature(vdev, VIRTIO_CONSOLE_F_SIZE))
hvc_resize(port->cons.hvc, port->cons.ws);
}
@@ -1102,7 +1163,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 +1171,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++;
@@ -1198,10 +1259,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, but
+ * 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;
@@ -1234,7 +1303,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 +1345,18 @@ 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);
+
+ /*
+ * 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_databuf(port->portdev->vdev, PAGE_SIZE, buf);
+ }
+
}
/*
@@ -1478,7 +1558,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 +1754,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);
}
/*
@@ -1688,7 +1768,7 @@ static void remove_controlq_data(struct ports_device *portdev)
* config space to see how many ports the host has spawned. We
* initialize each port found.
*/
-static int __devinit virtcons_probe(struct virtio_device *vdev)
+static int virtcons_probe(struct virtio_device *vdev)
{
struct ports_device *portdev;
int err;
@@ -1724,10 +1804,12 @@ 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)
+ if (is_rproc_serial(vdev))
+ multiport = false;
+ else if (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);
@@ -1838,6 +1920,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)
{
@@ -1922,6 +2014,16 @@ static struct virtio_driver virtio_console = {
#endif
};
+static struct virtio_driver 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;
@@ -1941,12 +2043,16 @@ static int __init init(void)
INIT_LIST_HEAD(&pdrvdata.consoles);
INIT_LIST_HEAD(&pdrvdata.portdevs);
- return register_virtio_driver(&virtio_console);
+ err = register_virtio_driver(&virtio_console);
+ if (err)
+ return err;
+ return register_virtio_driver(&virtio_rproc_serial);
}
static void __exit fini(void)
{
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..07cf6f7 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 0xB /* virtio remoteproc serial link */
#endif /* _LINUX_VIRTIO_IDS_H */
--
1.7.9.5
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply related
* Re: [PATCHv2] virtio_console: Add support for remoteproc serial
From: Rusty Russell @ 2012-09-20 0:40 UTC (permalink / raw)
Cc: Sjur Brændeland, Michael S. Tsirkin, Linus Walleij,
linux-kernel, virtualization, Amit Shah, Sjur Brændeland
In-Reply-To: <1348073458-17592-1-git-send-email-sjur.brandeland@stericsson.com>
sjur.brandeland@stericsson.com writes:
> From: Sjur Brændeland <sjur.brandeland@stericsson.com>
>
> Add a simple serial connection driver called
> VIRTIO_ID_RPROC_SERIAL (0xB) 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.
>
> This enables use of the exising virtio_console code in
> the remoteproc framework.
OK, I'll let Amit comment on the console changes, but some minor style
comments below:
> +#if IS_ENABLED(CONFIG_REMOTEPROC)
> +static inline bool is_rproc_serial(struct virtio_device *vdev)
> +{
> + return vdev->id.device == VIRTIO_ID_RPROC_SERIAL;
> +}
> +#else
> +static inline bool is_rproc_serial(struct virtio_device *vdev)
> +{
> + return false;
> +}
> +#endif
I prefer to avoid inline in C files. The compiler knows, and with
inline you get no warning if it becomes unused. Also, const struct
virtio_device *.
> +/* Allocate data buffer from DMA memory if requested */
> +static inline void *
> +alloc_databuf(struct virtio_device *vdev, size_t size, gfp_t flag)
> +{
> + if (is_rproc_serial(vdev)) {
> + dma_addr_t dma;
> + 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, flag);
Wow, up 2 levels? Why 2? What's special about the grandparents?
> -static void free_buf(struct port_buffer *buf)
> +static void
> +free_buf(struct virtqueue *vq, struct port_buffer *buf, size_t buf_size)
> {
Generally prefer to indent buf and buf_size, rather than break at
free_buf.
> + buf = alloc_databuf(vdev, buf_size, GFP_KERNEL);
>
> - buf = kmalloc(count, GFP_KERNEL);
> if (!buf)
> return -ENOMEM;
This effectively adds a blank line between "buf = ..." and "if (!buf)",
but they're adjacent because they're logically grouped.
> @@ -767,6 +826,7 @@ static int port_fops_release(struct inode *inode, struct file *filp)
> spin_unlock_irq(&port->inbuf_lock);
>
> spin_lock_irq(&port->outvq_lock);
> +
> reclaim_consumed_buffers(port);
> spin_unlock_irq(&port->outvq_lock);
>
Weird whitespace addition. I know you're doing that simply to check if
I'm reading, right?
> @@ -1688,7 +1768,7 @@ static void remove_controlq_data(struct ports_device *portdev)
> * config space to see how many ports the host has spawned. We
> * initialize each port found.
> */
> -static int __devinit virtcons_probe(struct virtio_device *vdev)
> +static int virtcons_probe(struct virtio_device *vdev)
> {
> struct ports_device *portdev;
> int err;
Not sure about this change. If you actually turn off CONFIG_HOTPLUG,
I wouldn't think that remoteproc would work at all any more, since it
needs the driver core to match up devices?
> @@ -1724,10 +1804,12 @@ 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)
> + if (is_rproc_serial(vdev))
> + multiport = false;
> + else if (virtio_config_val(vdev, VIRTIO_CONSOLE_F_MULTIPORT,
> + offsetof(struct virtio_console_config,
> + max_nr_ports),
> + &portdev->config.max_nr_ports) == 0)
> multiport = true;
This is a bit weird, to double-assign multiport = false; it looks tacked
on.
How about:
/* 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;
} else {
multiport = false;
portdev->config.max_nr_ports = 1;
}
> err = init_vqs(portdev);
> @@ -1838,6 +1920,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)
> {
> @@ -1922,6 +2014,16 @@ static struct virtio_driver virtio_console = {
> #endif
> };
>
> +static struct virtio_driver 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;
> @@ -1941,12 +2043,16 @@ static int __init init(void)
> INIT_LIST_HEAD(&pdrvdata.consoles);
> INIT_LIST_HEAD(&pdrvdata.portdevs);
>
> - return register_virtio_driver(&virtio_console);
> + err = register_virtio_driver(&virtio_console);
> + if (err)
> + return err;
> + return register_virtio_driver(&virtio_rproc_serial);
Hmm, we need to cleanup if the second register fails.
> #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 0xB /* virtio remoteproc serial link */
Prefer decimal here...
Cheers,
Rusty.
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCHv2] virtio_console: Add support for remoteproc serial
From: Michael S. Tsirkin @ 2012-09-20 6:10 UTC (permalink / raw)
To: sjur.brandeland
Cc: Sjur Brændeland, Linus Walleij, linux-kernel, virtualization,
Amit Shah
In-Reply-To: <1348073458-17592-1-git-send-email-sjur.brandeland@stericsson.com>
On Wed, Sep 19, 2012 at 06:50:58PM +0200, sjur.brandeland@stericsson.com wrote:
> From: Sjur Brændeland <sjur.brandeland@stericsson.com>
>
> Add a simple serial connection driver called
> VIRTIO_ID_RPROC_SERIAL (0xB) 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.
Can't repoteproc work with control queue?
Reason I ask, there are known races when
using config space and I was working on
a spec change that uses control queue for
all config accesses.
> This enables use of the exising virtio_console code in
> the remoteproc framework.
>
> Signed-off-by: Sjur Brændeland <sjur.brandeland@stericsson.com>
> 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>
> ---
>
> Hi Rusty,
>
> Changes since v1:
> o Use proper macros IS_ENABLED(CONFIG_REMOTEPROC)
> o Fix bug at registration of rproc_serial driver
> o Always allocate PAGE_SIZE buffers for rproc_serial,
> and limit write to port_fops_write to use PAGE_SIZE.
>
> Regards,
> Sjur
>
> drivers/char/virtio_console.c | 160 ++++++++++++++++++++++++++++++++++-------
> include/linux/virtio_ids.h | 1 +
> 2 files changed, 134 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
> index cdf2f54..395522a 100644
> --- a/drivers/char/virtio_console.c
> +++ b/drivers/char/virtio_console.c
> @@ -35,6 +35,8 @@
> #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"
>
> /*
> @@ -323,6 +325,52 @@ static bool is_console_port(struct port *port)
> return false;
> }
>
> +#if IS_ENABLED(CONFIG_REMOTEPROC)
> +static inline bool is_rproc_serial(struct virtio_device *vdev)
> +{
> + return vdev->id.device == VIRTIO_ID_RPROC_SERIAL;
> +}
> +#else
> +static inline bool is_rproc_serial(struct virtio_device *vdev)
> +{
> + return false;
> +}
> +#endif
> +
> +/* Allocate data buffer from DMA memory if requested */
> +static inline void *
> +alloc_databuf(struct virtio_device *vdev, size_t size, gfp_t flag)
> +{
> + if (is_rproc_serial(vdev)) {
> + dma_addr_t dma;
> + 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, flag);
> + }
> + return kmalloc(size, flag);
> +}
> +
> +static inline void
> +free_databuf(struct virtio_device *vdev, size_t size, void *cpu_addr)
> +{
> +
> + if (is_rproc_serial(vdev)) {
> + struct device *dev = &vdev->dev;
> + dma_addr_t dma_handle = virt_to_bus(cpu_addr);
> + dev = dev->parent ? dev->parent : dev;
> + dev = dev->parent ? dev->parent : dev;
> + dma_free_coherent(dev, size, cpu_addr, dma_handle);
> + return;
> + }
> + kfree(cpu_addr);
> +}
> +
> static inline bool use_multiport(struct ports_device *portdev)
> {
> /*
> @@ -334,20 +382,22 @@ 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)
> +static void
> +free_buf(struct virtqueue *vq, struct port_buffer *buf, size_t buf_size)
> {
> - kfree(buf->buf);
> + free_databuf(vq->vdev, buf_size, buf->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;
>
> 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, GFP_KERNEL);
> + memset(buf->buf, 0, buf_size);
> if (!buf->buf)
> goto free_buf;
> buf->len = 0;
> @@ -414,7 +464,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 +535,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, PAGE_SIZE, buf);
> port->outvq_full = false;
> }
> }
> @@ -672,6 +722,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;
> + size_t buf_size;
>
> /* Userspace could be out to fool us */
> if (!count)
> @@ -694,9 +746,16 @@ static ssize_t port_fops_write(struct file *filp, const char __user *ubuf,
> if (!port->guest_connected)
> return -ENODEV;
>
> - count = min((size_t)(32 * 1024), count);
> + vdev = port->portdev->vdev;
> +
> + if (is_rproc_serial(vdev)) {
> + count = min((size_t)PAGE_SIZE, count);
> + buf_size = PAGE_SIZE;
> + } else
> + buf_size = count = min((size_t)(32 * 1024), count);
> +
> + buf = alloc_databuf(vdev, buf_size, GFP_KERNEL);
>
> - buf = kmalloc(count, GFP_KERNEL);
> if (!buf)
> return -ENOMEM;
>
> @@ -720,7 +779,7 @@ static ssize_t port_fops_write(struct file *filp, const char __user *ubuf,
> goto out;
>
> free_buf:
> - kfree(buf);
> + free_databuf(vdev, buf_size, buf);
> out:
> return ret;
> }
> @@ -767,6 +826,7 @@ static int port_fops_release(struct inode *inode, struct file *filp)
> spin_unlock_irq(&port->inbuf_lock);
>
> spin_lock_irq(&port->outvq_lock);
> +
> reclaim_consumed_buffers(port);
> spin_unlock_irq(&port->outvq_lock);
>
> @@ -918,7 +978,8 @@ static void resize_console(struct port *port)
> return;
>
> vdev = port->portdev->vdev;
> - if (virtio_has_feature(vdev, VIRTIO_CONSOLE_F_SIZE))
> + if (!is_rproc_serial(vdev) &&
> + virtio_has_feature(vdev, VIRTIO_CONSOLE_F_SIZE))
> hvc_resize(port->cons.hvc, port->cons.ws);
> }
>
> @@ -1102,7 +1163,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 +1171,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++;
> @@ -1198,10 +1259,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, but
> + * 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;
> @@ -1234,7 +1303,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 +1345,18 @@ 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);
> +
> + /*
> + * 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_databuf(port->portdev->vdev, PAGE_SIZE, buf);
> + }
> +
> }
>
> /*
> @@ -1478,7 +1558,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 +1754,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);
> }
>
> /*
> @@ -1688,7 +1768,7 @@ static void remove_controlq_data(struct ports_device *portdev)
> * config space to see how many ports the host has spawned. We
> * initialize each port found.
> */
> -static int __devinit virtcons_probe(struct virtio_device *vdev)
> +static int virtcons_probe(struct virtio_device *vdev)
> {
> struct ports_device *portdev;
> int err;
> @@ -1724,10 +1804,12 @@ 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)
> + if (is_rproc_serial(vdev))
> + multiport = false;
> + else if (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);
> @@ -1838,6 +1920,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)
> {
> @@ -1922,6 +2014,16 @@ static struct virtio_driver virtio_console = {
> #endif
> };
>
> +static struct virtio_driver 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;
> @@ -1941,12 +2043,16 @@ static int __init init(void)
> INIT_LIST_HEAD(&pdrvdata.consoles);
> INIT_LIST_HEAD(&pdrvdata.portdevs);
>
> - return register_virtio_driver(&virtio_console);
> + err = register_virtio_driver(&virtio_console);
> + if (err)
> + return err;
> + return register_virtio_driver(&virtio_rproc_serial);
> }
>
> static void __exit fini(void)
> {
> 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..07cf6f7 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 0xB /* virtio remoteproc serial link */
>
> #endif /* _LINUX_VIRTIO_IDS_H */
> --
> 1.7.9.5
^ permalink raw reply
* RE: [PATCHv2] virtio_console: Add support for remoteproc serial
From: Sjur BRENDELAND @ 2012-09-20 9:29 UTC (permalink / raw)
To: Michael S. Tsirkin, Ohad Ben-Cohen
Cc: Sjur Brændeland, Linus Walleij, linux-kernel@vger.kernel.org,
virtualization@lists.linux-foundation.org, Amit Shah
In-Reply-To: <20120920061057.GD5721@redhat.com>
> From: Michael S. Tsirkin [mailto:mst@redhat.com]
> sjur.brandeland@stericsson.com wrote:
> > From: Sjur Brændeland <sjur.brandeland@stericsson.com>
> >
> > Add a simple serial connection driver called
> > VIRTIO_ID_RPROC_SERIAL (0xB) 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.
>
> Can't repoteproc work with control queue?
> Reason I ask, there are known races when
> using config space and I was working on
> a spec change that uses control queue for
> all config accesses.
In my case rproc-serial will be used for transferring boot
image or crash-dump to a modem. In these scenarios the
execution environment is very limited (no OS) and the size
and complexity of the virtio code at the modem should be kept
to a minimum.
The usage pattern in this case is also very simple,
the virtio device is opened only once, and only reopened
after a complete modem cold-start.
So the control queue is not appropriate for my use-case,
simplicity is the most important requirement.
(And currently the virtio config-space is not supported
by remoteproc anyway).
I don't know if other potential users of rproc_serial have more
complex use-cases in mind?
Regards,
Sjur
^ permalink raw reply
* RE: [PATCHv2] virtio_console: Add support for remoteproc serial
From: Sjur BRENDELAND @ 2012-09-20 10:24 UTC (permalink / raw)
To: Rusty Russell
Cc: Sjur Brændeland, Michael S. Tsirkin, Linus Walleij,
linux-kernel@vger.kernel.org,
virtualization@lists.linux-foundation.org, Amit Shah
In-Reply-To: <871uhxplvb.fsf@rustcorp.com.au>
Hi Rusty,
> > +#if IS_ENABLED(CONFIG_REMOTEPROC)
> > +static inline bool is_rproc_serial(struct virtio_device *vdev)
> > +{
> > + return vdev->id.device == VIRTIO_ID_RPROC_SERIAL;
> > +}
> > +#else
> > +static inline bool is_rproc_serial(struct virtio_device *vdev)
> > +{
> > + return false;
> > +}
> > +#endif
>
> I prefer to avoid inline in C files. The compiler knows, and with
> inline you get no warning if it becomes unused. Also, const struct
> virtio_device *.
Sure, I'll fix this.
> > +/* Allocate data buffer from DMA memory if requested */
> > +static inline void *
> > +alloc_databuf(struct virtio_device *vdev, size_t size, gfp_t flag)
> > +{
> > + if (is_rproc_serial(vdev)) {
> > + dma_addr_t dma;
> > + 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, flag);
>
> Wow, up 2 levels? Why 2? What's special about the grandparents?
In remoteproc we have the following hierarchy:
Virtio Device -> Remoteproc Device -> Platform Device
The DMA memory is associated with the Platform Device.
In my case the platform device does dma_declare_coherent_memory()
before registering to the rproc framework. And I need to call
dma_alloc_coherent with the same device reference that originally
did declare the dma memory.
virtio_rpmsg_bus.c does the same thing. When allocating dma
memory is access the parent: dma_alloc_coherent(vdev->dev.parent->parent,...).
As mentioned in the comment, dma-coherent.c does unfortunately
not implement search for devices parent devices with DMA memory
even if the flag DMA_MEMORY_INCLUDES_CHILDREN is set. If this
feature was in place I could have dropped the genealogy research
I have implemented.
> > -static void free_buf(struct port_buffer *buf)
> > +static void
> > +free_buf(struct virtqueue *vq, struct port_buffer *buf, size_t
> buf_size)
> > {
>
> Generally prefer to indent buf and buf_size, rather than break at
> free_buf.
Ok, I'll fix this.
>
> > + buf = alloc_databuf(vdev, buf_size, GFP_KERNEL);
> >
> > - buf = kmalloc(count, GFP_KERNEL);
> > if (!buf)
> > return -ENOMEM;
>
> This effectively adds a blank line between "buf = ..." and "if (!buf)",
> but they're adjacent because they're logically grouped.
Agree, thanks.
>
> > @@ -767,6 +826,7 @@ static int port_fops_release(struct inode *inode,
> struct file *filp)
> > spin_unlock_irq(&port->inbuf_lock);
> >
> > spin_lock_irq(&port->outvq_lock);
> > +
> > reclaim_consumed_buffers(port);
> > spin_unlock_irq(&port->outvq_lock);
> >
>
> Weird whitespace addition. I know you're doing that simply to check if
> I'm reading, right?
Of course not - it's just me being space out.
> > @@ -1688,7 +1768,7 @@ static void remove_controlq_data(struct
> ports_device *portdev)
> > * config space to see how many ports the host has spawned. We
> > * initialize each port found.
> > */
> > -static int __devinit virtcons_probe(struct virtio_device *vdev)
> > +static int virtcons_probe(struct virtio_device *vdev)
> > {
> > struct ports_device *portdev;
> > int err;
>
> Not sure about this change. If you actually turn off CONFIG_HOTPLUG,
> I wouldn't think that remoteproc would work at all any more, since it
> needs the driver core to match up devices?
Hm, when adding __devinit I get a section mismatch warning in virtio_rproc_serial.
But I guess this is a false positive? I could silence this warning by annotating
virtio_rproc_serial with __refdata.
> > @@ -1724,10 +1804,12 @@ 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)
> > + if (is_rproc_serial(vdev))
> > + multiport = false;
> > + else if (virtio_config_val(vdev, VIRTIO_CONSOLE_F_MULTIPORT,
> > + offsetof(struct virtio_console_config,
> > + max_nr_ports),
> > + &portdev->config.max_nr_ports) == 0)
> > multiport = true;
>
> This is a bit weird, to double-assign multiport = false; it looks
> tacked
> on.
>
> How about:
>
> /* 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;
> } else {
> multiport = false;
> portdev->config.max_nr_ports = 1;
> }
Yes thanks, this looks much better.
>
> > err = init_vqs(portdev);
> > @@ -1838,6 +1920,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)
> > {
> > @@ -1922,6 +2014,16 @@ static struct virtio_driver virtio_console = {
> > #endif
> > };
> >
> > +static struct virtio_driver 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;
> > @@ -1941,12 +2043,16 @@ static int __init init(void)
> > INIT_LIST_HEAD(&pdrvdata.consoles);
> > INIT_LIST_HEAD(&pdrvdata.portdevs);
> >
> > - return register_virtio_driver(&virtio_console);
> > + err = register_virtio_driver(&virtio_console);
> > + if (err)
> > + return err;
> > + return register_virtio_driver(&virtio_rproc_serial);
>
> Hmm, we need to cleanup if the second register fails.
Indeed, I'll add this.
>
> > #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 0xB /* virtio remoteproc serial link
> */
>
> Prefer decimal here...
Sure,
Thank you for reviewing, I send out a new respin of this patch soon.
Regards,
Sjur
^ permalink raw reply
* [PATCHv3] virtio_console: Add support for remoteproc serial
From: sjur.brandeland @ 2012-09-20 16:34 UTC (permalink / raw)
To: Amit Shah, Rusty Russell
Cc: Sjur Brændeland, Michael S. Tsirkin, Linus Walleij,
linux-kernel, virtualization, Sjur Brændeland
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.
This enables use of the exising virtio_console code in
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: Amit Shah <amit.shah@redhat.com>
cc: Ohad Ben-Cohen <ohad@wizery.com>
cc: Linus Walleij <linus.walleij@linaro.org>
cc: virtualization@lists.linux-foundation.org
---
Hi Amit,
Changes since v2: Fixes for Rustys review comments. The only
"unresolved" issue (unless I missed something) is the quirky
handling of finding device grandparent when doing dma_alloc.
[Rusty wrote:]
>OK, I'll let Amit comment on the console changes, but some minor style
>comments below.
Amit, any chance that you might find time to review this?
Note: This patch is based on v3.6 so this patch is will conflict
with "virtio: console: fix error handling in init() function".
Get back to me if you want me to rebase to another baseline.
Thanks,
Sjur
drivers/char/virtio_console.c | 185 ++++++++++++++++++++++++++++++++++------
include/linux/virtio_ids.h | 1 +
2 files changed, 158 insertions(+), 28 deletions(-)
diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index cdf2f54..4f2f74e 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -35,6 +35,8 @@
#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"
/*
@@ -323,6 +325,55 @@ static bool is_console_port(struct port *port)
return false;
}
+#if IS_ENABLED(CONFIG_REMOTEPROC)
+static bool is_rproc_serial(const struct virtio_device *vdev)
+{
+ return vdev->id.device == VIRTIO_ID_RPROC_SERIAL;
+}
+#else
+static bool is_rproc_serial(const struct virtio_device *vdev)
+{
+ return false;
+}
+#endif
+
+/* Allocate data buffer from DMA memory if requested */
+static void *alloc_databuf(struct virtio_device *vdev, size_t size, gfp_t flag)
+{
+ if (is_rproc_serial(vdev)) {
+ dma_addr_t dma_addr;
+ struct device *dev = &vdev->dev;
+ /*
+ * 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
+ */
+ dev = dev->parent ? dev->parent : dev;
+ dev = dev->parent ? dev->parent : dev;
+ return dma_alloc_coherent(dev, size, &dma_addr, flag);
+ }
+ return kmalloc(size, flag);
+}
+
+static void free_databuf(struct virtio_device *vdev, size_t size, void *vaddr)
+{
+ if (is_rproc_serial(vdev)) {
+ struct device *dev = &vdev->dev;
+ dma_addr_t dma_addr;
+
+ dev = dev->parent ? dev->parent : dev;
+ dev = dev->parent ? dev->parent : dev;
+ dma_addr = virt_to_bus(vaddr);
+ dma_free_coherent(dev, size, vaddr, dma_addr);
+ return;
+ }
+ kfree(vaddr);
+}
+
static inline bool use_multiport(struct ports_device *portdev)
{
/*
@@ -334,22 +385,24 @@ 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)
+static void free_buf(struct virtqueue *vq, struct port_buffer *buf,
+ size_t buf_size)
{
- kfree(buf->buf);
+ free_databuf(vq->vdev, buf_size, buf->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;
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, GFP_KERNEL);
if (!buf->buf)
goto free_buf;
+ memset(buf->buf, 0, buf_size);
buf->len = 0;
buf->offset = 0;
buf->size = buf_size;
@@ -414,7 +467,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 +538,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, PAGE_SIZE, buf);
port->outvq_full = false;
}
}
@@ -672,6 +725,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;
+ size_t buf_size;
/* Userspace could be out to fool us */
if (!count)
@@ -694,9 +749,17 @@ static ssize_t port_fops_write(struct file *filp, const char __user *ubuf,
if (!port->guest_connected)
return -ENODEV;
- count = min((size_t)(32 * 1024), count);
+ vdev = port->portdev->vdev;
+
+ if (is_rproc_serial(vdev)) {
+ count = min_t(size_t, PAGE_SIZE, count);
+ buf_size = PAGE_SIZE;
+ } else {
+ count = min_t(size_t, (32 * 1024), count);
+ buf_size = count;
+ }
- buf = kmalloc(count, GFP_KERNEL);
+ buf = alloc_databuf(vdev, buf_size, GFP_KERNEL);
if (!buf)
return -ENOMEM;
@@ -720,7 +783,7 @@ static ssize_t port_fops_write(struct file *filp, const char __user *ubuf,
goto out;
free_buf:
- kfree(buf);
+ free_databuf(vdev, buf_size, buf);
out:
return ret;
}
@@ -918,7 +981,8 @@ static void resize_console(struct port *port)
return;
vdev = port->portdev->vdev;
- if (virtio_has_feature(vdev, VIRTIO_CONSOLE_F_SIZE))
+ if (!is_rproc_serial(vdev) &&
+ virtio_has_feature(vdev, VIRTIO_CONSOLE_F_SIZE))
hvc_resize(port->cons.hvc, port->cons.ws);
}
@@ -1102,7 +1166,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 +1174,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++;
@@ -1198,10 +1262,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, but
+ * 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;
@@ -1234,7 +1306,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 +1348,17 @@ 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);
+
+ /*
+ * 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_databuf(port->portdev->vdev, PAGE_SIZE, buf);
+ }
}
/*
@@ -1478,7 +1560,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 +1756,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);
}
/*
@@ -1722,13 +1804,17 @@ static int __devinit virtcons_probe(struct virtio_device *vdev)
goto free;
}
- 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;
+ } else {
+ multiport = false;
+ portdev->config.max_nr_ports = 1;
+ }
err = init_vqs(portdev);
if (err < 0) {
@@ -1838,6 +1924,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)
{
@@ -1922,6 +2018,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;
@@ -1941,12 +2051,31 @@ static int __init init(void)
INIT_LIST_HEAD(&pdrvdata.consoles);
INIT_LIST_HEAD(&pdrvdata.portdevs);
- return register_virtio_driver(&virtio_console);
+ err = register_virtio_driver(&virtio_console);
+ if (err < 0) {
+ 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);
+ class_destroy(pdrvdata.class);
+ return err;
}
static void __exit fini(void)
{
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
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply related
* [PATCHv4] virtio_console: Add support for remoteproc serial
From: sjur.brandeland @ 2012-09-24 12:33 UTC (permalink / raw)
To: Amit Shah
Cc: Arnd Bergmann, Michael S. Tsirkin, sjurbren, linux-kernel,
virtualization, Linus Walleij, Sjur Brændeland
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 v3 are mostly related to freeing of dma-buffers:
- Change port_fops_write() to use struct port_buffer when allocating
memory. This is done in order to store the dma-address of the
allocated buffers, so the correct dma-address can be passed to
dma_free_coherent().
- Added pending_free_list for port_buf. dma_free_coherent() requires
the irqs to be enabled, so if irqs are disabled we queue the buffer
on the pending_free_list and free it later when irqs are enabled.
- Remove #if around is_rproc_serial
Thanks,
Sjur
drivers/char/virtio_console.c | 222 ++++++++++++++++++++++++++++++++++++-----
include/linux/virtio_ids.h | 1 +
2 files changed, 199 insertions(+), 24 deletions(-)
diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index cdf2f54..3af9a5d 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -35,8 +35,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 rproc_enabled IS_ENABLED(CONFIG_REMOTEPROC)
+
/*
* This is a global struct for storing common data for all the devices
* this driver handles.
@@ -109,6 +113,15 @@ struct port_buffer {
size_t len;
/* 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;
};
/*
@@ -323,6 +336,11 @@ static bool is_console_port(struct port *port)
return false;
}
+static bool is_rproc_serial(const struct virtio_device *vdev)
+{
+ return rproc_enabled && vdev->id.device == VIRTIO_ID_RPROC_SERIAL;
+}
+
static inline bool use_multiport(struct ports_device *portdev)
{
/*
@@ -334,20 +352,99 @@ static inline bool use_multiport(struct ports_device *portdev)
return portdev->vdev->features[0] & (1 << VIRTIO_CONSOLE_F_MULTIPORT);
}
+static DEFINE_SPINLOCK(list_lock);
+static LIST_HEAD(pending_free_list);
+
static void free_buf(struct port_buffer *buf)
{
- kfree(buf->buf);
+ unsigned long flags;
+
+ if (!buf->dev) {
+ kfree(buf->buf);
+ goto freebuf;
+ }
+
+ BUG_ON(!rproc_enabled);
+
+ /* dma_free_coherent requires interrupts to be enabled */
+ if (rproc_enabled && !irqs_disabled()) {
+ 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);
+ goto freebuf;
+ }
+
+ /* queue up dma-buffers to be freed later */
+ spin_lock_irqsave(&list_lock, flags);
+ list_add_tail(&buf->list, &pending_free_list);
+ spin_unlock_irqrestore(&list_lock, flags);
+ return;
+
+freebuf:
kfree(buf);
}
-static struct port_buffer *alloc_buf(size_t buf_size)
+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_list))
+ return;
+
+ BUG_ON(!rproc_enabled);
+
+ /* Create a copy of the pending_free_list while holding the lock*/
+ spin_lock_irqsave(&list_lock, flags);
+ list_cut_position(&tmp_list, &pending_free_list,
+ pending_free_list.prev);
+ spin_unlock_irqrestore(&list_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)
{
struct port_buffer *buf;
+ if (is_rproc_serial(vq->vdev) && !irqs_disabled())
+ reclaim_dma_bufs();
+
buf = kmalloc(sizeof(*buf), GFP_KERNEL);
if (!buf)
goto fail;
- buf->buf = kzalloc(buf_size, GFP_KERNEL);
+
+ 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_KERNEL);
+ } else {
+ buf->buf = kmalloc(buf_size, GFP_KERNEL);
+ buf->dev = NULL;
+ }
+
if (!buf->buf)
goto free_buf;
buf->len = 0;
@@ -485,7 +582,10 @@ static void reclaim_consumed_buffers(struct port *port)
return;
}
while ((buf = virtqueue_get_buf(port->out_vq, &len))) {
- kfree(buf);
+ if (is_console_port(port))
+ kfree(buf);
+ else
+ free_buf(buf);
port->outvq_full = false;
}
}
@@ -498,6 +598,7 @@ static ssize_t send_buf(struct port *port, void *in_buf, size_t in_count,
ssize_t ret;
unsigned long flags;
unsigned int len;
+ struct port_buffer *buf = in_buf;
out_vq = port->out_vq;
@@ -505,8 +606,11 @@ static ssize_t send_buf(struct port *port, void *in_buf, size_t in_count,
reclaim_consumed_buffers(port);
- sg_init_one(sg, in_buf, in_count);
- ret = virtqueue_add_buf(out_vq, sg, 1, 0, in_buf, GFP_ATOMIC);
+ if (is_console_port(port))
+ sg_init_one(sg, in_buf, in_count);
+ else
+ sg_init_one(sg, buf->buf, in_count);
+ ret = virtqueue_add_buf(out_vq, sg, 1, 0, buf, GFP_ATOMIC);
/* Tell Host to go! */
virtqueue_kick(out_vq);
@@ -669,7 +773,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;
@@ -696,11 +800,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);
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;
@@ -720,7 +824,7 @@ static ssize_t port_fops_write(struct file *filp, const char __user *ubuf,
goto out;
free_buf:
- kfree(buf);
+ free_buf(buf);
out:
return ret;
}
@@ -770,6 +874,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
@@ -918,7 +1024,8 @@ static void resize_console(struct port *port)
return;
vdev = port->portdev->vdev;
- if (virtio_has_feature(vdev, VIRTIO_CONSOLE_F_SIZE))
+ if (!is_rproc_serial(vdev) &&
+ virtio_has_feature(vdev, VIRTIO_CONSOLE_F_SIZE))
hvc_resize(port->cons.hvc, port->cons.ws);
}
@@ -1102,10 +1209,10 @@ 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;
-
+ memset(buf->buf, 0, PAGE_SIZE);
spin_lock_irq(lock);
ret = add_inbuf(vq, buf);
if (ret < 0) {
@@ -1198,10 +1305,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, but
+ * 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;
@@ -1277,6 +1392,16 @@ 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);
+ }
}
/*
@@ -1722,13 +1847,17 @@ static int __devinit virtcons_probe(struct virtio_device *vdev)
goto free;
}
- 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;
+ } else {
+ multiport = false;
+ portdev->config.max_nr_ports = 1;
+ }
err = init_vqs(portdev);
if (err < 0) {
@@ -1838,6 +1967,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)
{
@@ -1922,6 +2061,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;
@@ -1941,12 +2094,33 @@ static int __init init(void)
INIT_LIST_HEAD(&pdrvdata.consoles);
INIT_LIST_HEAD(&pdrvdata.portdevs);
- return register_virtio_driver(&virtio_console);
+ err = register_virtio_driver(&virtio_console);
+ if (err < 0) {
+ 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);
+ class_destroy(pdrvdata.class);
+ return err;
}
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
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply related
* Re: [PATCH v10 1/5] mm: introduce a common interface for balloon pages mobility
From: Peter Zijlstra @ 2012-09-24 12:44 UTC (permalink / raw)
To: Rafael Aquini
Cc: Rik van Riel, Konrad Rzeszutek Wilk, Michael S. Tsirkin,
linux-kernel, virtualization, linux-mm, Andi Kleen, Minchan Kim,
Andrew Morton, Paul E. McKenney
In-Reply-To: <89c9f4096bbad072e155445fcdf1805d47ddf48e.1347897793.git.aquini@redhat.com>
On Mon, 2012-09-17 at 13:38 -0300, Rafael Aquini wrote:
> +static inline void assign_balloon_mapping(struct page *page,
> + struct address_space
> *mapping)
> +{
> + page->mapping = mapping;
> + smp_wmb();
> +}
> +
> +static inline void clear_balloon_mapping(struct page *page)
> +{
> + page->mapping = NULL;
> + smp_wmb();
> +}
barriers without a comment describing the data race are a mortal sin.
^ permalink raw reply
* [PATCH 0/2] virtio-mmio updates for 3.7
From: Pawel Moll @ 2012-09-24 13:33 UTC (permalink / raw)
To: Rusty Russell; +Cc: Brian Foley, Pawel Moll, virtualization
Hi Rusty,
Would you be so kind and consider getting those two small fixes into
3.7 merge? All credits go to Brian, all shame on me :-)
Cheers!
Pawel
Brian Foley (2):
virtio_mmio: fix off by one error allocating queue
virtio_mmio: Don't attempt to create empty virtqueues
drivers/virtio/virtio_mmio.c | 14 ++++++++++++--
1 file changed, 12 insertions(+), 2 deletions(-)
--
1.7.9.5
^ permalink raw reply
* [PATCH 1/2] virtio_mmio: fix off by one error allocating queue
From: Pawel Moll @ 2012-09-24 13:33 UTC (permalink / raw)
To: Rusty Russell; +Cc: Brian Foley, Pawel Moll, virtualization
In-Reply-To: <1348493622-26091-1-git-send-email-pawel.moll@arm.com>
From: Brian Foley <brian.foley@arm.com>
vm_setup_vq fails to allow VirtQueues needing only 2 pages of
storage, as it should. Found with a kernel using 64kB pages, but
can be provoked if a virtio device reports QueueNumMax where the
descriptor table and available ring fit in one page, and the used
ring on the second (<= 227 descriptors with 4kB pages and <= 3640
with 64kB pages.)
Signed-off-by: Brian Foley <brian.foley@arm.com>
Signed-off-by: Pawel Moll <pawel.moll@arm.com>
---
drivers/virtio/virtio_mmio.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c
index 453db0c..58e2d78 100644
--- a/drivers/virtio/virtio_mmio.c
+++ b/drivers/virtio/virtio_mmio.c
@@ -335,8 +335,8 @@ static struct virtqueue *vm_setup_vq(struct virtio_device *vdev, unsigned index,
while (1) {
size = PAGE_ALIGN(vring_size(info->num,
VIRTIO_MMIO_VRING_ALIGN));
- /* Already smallest possible allocation? */
- if (size <= VIRTIO_MMIO_VRING_ALIGN * 2) {
+ /* Did the last iter shrink the queue below minimum size? */
+ if (size < VIRTIO_MMIO_VRING_ALIGN * 2) {
err = -ENOMEM;
goto error_alloc_pages;
}
--
1.7.9.5
^ permalink raw reply related
* [PATCH 2/2] virtio_mmio: Don't attempt to create empty virtqueues
From: Pawel Moll @ 2012-09-24 13:33 UTC (permalink / raw)
To: Rusty Russell; +Cc: Brian Foley, Pawel Moll, virtualization
In-Reply-To: <1348493622-26091-1-git-send-email-pawel.moll@arm.com>
From: Brian Foley <brian.foley@arm.com>
If a virtio device reports a QueueNumMax of 0, vring_new_virtqueue()
doesn't check this, and thanks to an unsigned (i < num - 1) loop
guard, scribbles over memory when initialising the free list.
Avoid by not trying to create zero-descriptor queues, as there's no
way to do any I/O with one.
Signed-off-by: Brian Foley <brian.foley@arm.com>
Signed-off-by: Pawel Moll <pawel.moll@arm.com>
---
drivers/virtio/virtio_mmio.c | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c
index 58e2d78..6979c1b 100644
--- a/drivers/virtio/virtio_mmio.c
+++ b/drivers/virtio/virtio_mmio.c
@@ -332,6 +332,16 @@ static struct virtqueue *vm_setup_vq(struct virtio_device *vdev, unsigned index,
* and two rings (which makes it "alignment_size * 2")
*/
info->num = readl(vm_dev->base + VIRTIO_MMIO_QUEUE_NUM_MAX);
+
+ /* If the device reports a 0 entry queue, we won't be able to
+ * use it to perform I/O, and vring_new_virtqueue() can't create
+ * empty queues anyway, so don't bother to set up the device.
+ */
+ if (info->num == 0) {
+ err = -ENOENT;
+ goto error_alloc_pages;
+ }
+
while (1) {
size = PAGE_ALIGN(vring_size(info->num,
VIRTIO_MMIO_VRING_ALIGN));
--
1.7.9.5
^ permalink raw reply related
* Re: [PATCHv4] virtio_console: Add support for remoteproc serial
From: Amit Shah @ 2012-09-24 17:45 UTC (permalink / raw)
To: sjur.brandeland
Cc: Arnd Bergmann, Michael S. Tsirkin, sjurbren, linux-kernel,
virtualization, Linus Walleij
In-Reply-To: <1348489987-6768-1-git-send-email-sjur.brandeland@stericsson.com>
Hi Sjur,
I'm sorry for not being able to look at this earlier.
A general comment is to base this patchset on linux-next; we've been
seeing more than usual activity for virtio_console this time around.
I don't expect the conflicts to be big, though.
On (Mon) 24 Sep 2012 [14:33:07], sjur.brandeland@stericsson.com wrote:
> 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.
Any specific reason to not use the control queue? It's just another
virtio-serial port; the only special thing about it being it's an
internal channel between the device and driver.
If you're not going to implement any control commands, I guess you
could conveniently not use the actual port, but keep it around, in
case you find use for it later. The advantage will be that older
kernels will work without any updates on newer devices.
> 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 v3 are mostly related to freeing of dma-buffers:
> - Change port_fops_write() to use struct port_buffer when allocating
> memory. This is done in order to store the dma-address of the
> allocated buffers, so the correct dma-address can be passed to
> dma_free_coherent().
> - Added pending_free_list for port_buf. dma_free_coherent() requires
> the irqs to be enabled, so if irqs are disabled we queue the buffer
> on the pending_free_list and free it later when irqs are enabled.
> - Remove #if around is_rproc_serial
>
> Thanks,
> Sjur
>
> drivers/char/virtio_console.c | 222 ++++++++++++++++++++++++++++++++++++-----
> include/linux/virtio_ids.h | 1 +
> 2 files changed, 199 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
> index cdf2f54..3af9a5d 100644
> --- a/drivers/char/virtio_console.c
> +++ b/drivers/char/virtio_console.c
> @@ -35,8 +35,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 rproc_enabled IS_ENABLED(CONFIG_REMOTEPROC)
Since 'rproc_enabled' could be false, suggest using 'is_rproc_enabled'.
> +
> /*
> * This is a global struct for storing common data for all the devices
> * this driver handles.
> @@ -109,6 +113,15 @@ struct port_buffer {
> size_t len;
> /* 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;
Aha, nice. One of the comments I had with the earlier versions (I
just went through all the revisions) was that you weren't using the
port_buffer struct, and instead modifying all alloc_buf() and
free_buf() calls. This is much more saner.
> };
>
> /*
> @@ -323,6 +336,11 @@ static bool is_console_port(struct port *port)
> return false;
> }
>
> +static bool is_rproc_serial(const struct virtio_device *vdev)
> +{
> + return rproc_enabled && vdev->id.device == VIRTIO_ID_RPROC_SERIAL;
> +}
> +
> static inline bool use_multiport(struct ports_device *portdev)
> {
> /*
> @@ -334,20 +352,99 @@ static inline bool use_multiport(struct ports_device *portdev)
> return portdev->vdev->features[0] & (1 << VIRTIO_CONSOLE_F_MULTIPORT);
> }
>
> +static DEFINE_SPINLOCK(list_lock);
> +static LIST_HEAD(pending_free_list);
The names list_lock, pending_free_list are very generic in the file's
context. Please use more specific names.
> +
> static void free_buf(struct port_buffer *buf)
> {
> - kfree(buf->buf);
> + unsigned long flags;
> +
> + if (!buf->dev) {
> + kfree(buf->buf);
> + goto freebuf;
> + }
> +
> + BUG_ON(!rproc_enabled);
> +
> + /* dma_free_coherent requires interrupts to be enabled */
> + if (rproc_enabled && !irqs_disabled()) {
You don't need to check for rproc_enabled here.
Then, you can just invert the if condition (if (irqs_disabled()) and
include the relevant block here. This way, you can make do without
the goto and return mess below.
> + 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);
> + goto freebuf;
> + }
> +
> + /* queue up dma-buffers to be freed later */
> + spin_lock_irqsave(&list_lock, flags);
> + list_add_tail(&buf->list, &pending_free_list);
> + spin_unlock_irqrestore(&list_lock, flags);
> + return;
> +
> +freebuf:
> kfree(buf);
> }
>
> -static struct port_buffer *alloc_buf(size_t buf_size)
> +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_list))
> + return;
> +
> + BUG_ON(!rproc_enabled);
> +
> + /* Create a copy of the pending_free_list while holding the lock*/
> + spin_lock_irqsave(&list_lock, flags);
> + list_cut_position(&tmp_list, &pending_free_list,
> + pending_free_list.prev);
> + spin_unlock_irqrestore(&list_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)
> {
> struct port_buffer *buf;
>
> + if (is_rproc_serial(vq->vdev) && !irqs_disabled())
> + reclaim_dma_bufs();
> +
> buf = kmalloc(sizeof(*buf), GFP_KERNEL);
> if (!buf)
> goto fail;
> - buf->buf = kzalloc(buf_size, GFP_KERNEL);
> +
> + 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_KERNEL);
incorrect indentation
> + } else {
> + buf->buf = kmalloc(buf_size, GFP_KERNEL);
> + buf->dev = NULL;
> + }
> +
> if (!buf->buf)
> goto free_buf;
> buf->len = 0;
> @@ -485,7 +582,10 @@ static void reclaim_consumed_buffers(struct port *port)
> return;
> }
> while ((buf = virtqueue_get_buf(port->out_vq, &len))) {
> - kfree(buf);
> + if (is_console_port(port))
> + kfree(buf);
> + else
> + free_buf(buf);
Hm?
> port->outvq_full = false;
> }
> }
> @@ -498,6 +598,7 @@ static ssize_t send_buf(struct port *port, void *in_buf, size_t in_count,
> ssize_t ret;
> unsigned long flags;
> unsigned int len;
> + struct port_buffer *buf = in_buf;
This looks wrong: the buffer we receive here is the actual data
(buf->buf). It can never be a port_buffer (buf).
>
> out_vq = port->out_vq;
>
> @@ -505,8 +606,11 @@ static ssize_t send_buf(struct port *port, void *in_buf, size_t in_count,
>
> reclaim_consumed_buffers(port);
>
> - sg_init_one(sg, in_buf, in_count);
> - ret = virtqueue_add_buf(out_vq, sg, 1, 0, in_buf, GFP_ATOMIC);
> + if (is_console_port(port))
I think you're misinterpreting what is_console_port() is. It means if
a port is associated with an hvc/tty device.
> + sg_init_one(sg, in_buf, in_count);
> + else
> + sg_init_one(sg, buf->buf, in_count);
> + ret = virtqueue_add_buf(out_vq, sg, 1, 0, buf, GFP_ATOMIC);
>
> /* Tell Host to go! */
> virtqueue_kick(out_vq);
> @@ -669,7 +773,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;
>
> @@ -696,11 +800,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);
> 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;
> @@ -720,7 +824,7 @@ static ssize_t port_fops_write(struct file *filp, const char __user *ubuf,
> goto out;
>
> free_buf:
> - kfree(buf);
> + free_buf(buf);
> out:
> return ret;
> }
OK, I now get what you did with send_buf() above. However, send_buf()
now should be completely broken for non-rproc devices: you're
allocating a buf instead of a buf->buf and passing that on to
send_buf() as a void*. You should instead modify send_buf() to accept
a struct port_buffer instead.
Second, send_buf() receives a struct port_buffer(), but in the
'is_console_port()' case, you ignore that fact, and just pass on the
void* pointer to sg_init_one(). You should instead pass buf->buf.
> @@ -770,6 +874,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
> @@ -918,7 +1024,8 @@ static void resize_console(struct port *port)
> return;
>
> vdev = port->portdev->vdev;
> - if (virtio_has_feature(vdev, VIRTIO_CONSOLE_F_SIZE))
> + if (!is_rproc_serial(vdev) &&
> + virtio_has_feature(vdev, VIRTIO_CONSOLE_F_SIZE))
> hvc_resize(port->cons.hvc, port->cons.ws);
Why do you want to ensure !is_rproc_serial() here? As long as the
device doesn't expose the VIRTIO_CONSOLE_F_SIZE feature, you should be
fine, so this hunk can be dropped.
> }
>
> @@ -1102,10 +1209,10 @@ 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;
> -
> + memset(buf->buf, 0, PAGE_SIZE);
Why this memset here?
1. alloc_buf() already does kzalloc()
2. Is there any specific reason you want the buffer to be zeroed?
I've recently realised zeroing out the buffer before giving it to the
device serves no real purpose, and we're just slowing down the
allocation here, so I'm tempted to convert the kzalloc() to
kmalloc(), unless you have a specific need for zeroed pages.
> spin_lock_irq(lock);
> ret = add_inbuf(vq, buf);
> if (ret < 0) {
> @@ -1198,10 +1305,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, but
> + * the generic port implementation.
s/but/only
> + */
> + 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;
> @@ -1277,6 +1392,16 @@ 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);
> + }
braces around if can be dropped.
> }
>
> /*
> @@ -1722,13 +1847,17 @@ static int __devinit virtcons_probe(struct virtio_device *vdev)
> goto free;
> }
>
> - 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;
> + } else {
> + multiport = false;
> + portdev->config.max_nr_ports = 1;
> + }
Why introduce the else part at all? Let these two statements be as
they are, and just add the !is_rproc_serial check to the if statement?
>
> err = init_vqs(portdev);
> if (err < 0) {
> @@ -1838,6 +1967,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)
> {
> @@ -1922,6 +2061,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;
> @@ -1941,12 +2094,33 @@ static int __init init(void)
> INIT_LIST_HEAD(&pdrvdata.consoles);
> INIT_LIST_HEAD(&pdrvdata.portdevs);
>
> - return register_virtio_driver(&virtio_console);
> + err = register_virtio_driver(&virtio_console);
> + if (err < 0) {
> + pr_err("Error %d registering virtio driver\n", err);
> + goto free;
> + }
This hunk is already present in linux-next; rebasing over that should
get rid of it.
Thanks,
Amit
^ permalink raw reply
* RE: [PATCHv4] virtio_console: Add support for remoteproc serial
From: Sjur BRENDELAND @ 2012-09-24 21:50 UTC (permalink / raw)
To: Amit Shah
Cc: sjurbren@gmail.com, Arnd Bergmann, Michael S. Tsirkin,
Linus Walleij, linux-kernel@vger.kernel.org,
virtualization@lists.linux-foundation.org
In-Reply-To: <20120924174523.GA14390@amit.redhat.com>
Hi Amit,
> I'm sorry for not being able to look at this earlier.
No worries. I'll try to respin and retest this patch by tomorrow.
If you by any chance could find time to review so could make it in time
for 3.7 it would be great :-)
> A general comment is to base this patchset on linux-next; we've been
> seeing more than usual activity for virtio_console this time around.
> I don't expect the conflicts to be big, though.
Sure, I'll based the next patch on linux-next.
...
> > 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.
>
> Any specific reason to not use the control queue? It's just another
> virtio-serial port; the only special thing about it being it's an
> internal channel between the device and driver.
Yes, as mention to Michael earlier. I use rproc_serial for talking
to a modem running in early boot phases, before the OS has started,
or when the modem is executing it's crash handler. In both these
cases the modem run in a very limited execution environment, so
I want to keep the protocol and handling of the vqs as simple as
possible. Due to this I really don't want more than single pair
of vqs.
We also have very simple use-cases. The port is opened once
in the life-time of the modem, and only reopened after a
cold-start of the modem. So I should not get into any issues
with race conditions.
> If you're not going to implement any control commands, I guess you
> could conveniently not use the actual port, but keep it around, in
> case you find use for it later. The advantage will be that older
> kernels will work without any updates on newer devices.
With the current usage pattern I have in mind, I'd rather add this
feature later when/if needed. We can always add a new feature bit
for this if we introduce the control channel later on.
...
> > +#define rproc_enabled IS_ENABLED(CONFIG_REMOTEPROC)
>
> Since 'rproc_enabled' could be false, suggest using 'is_rproc_enabled'.
Ok, I'll change this.
...
> > @@ -109,6 +113,15 @@ struct port_buffer {
> > size_t len;
> > /* 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;
>
> Aha, nice. One of the comments I had with the earlier versions (I
> just went through all the revisions) was that you weren't using the
> port_buffer struct, and instead modifying all alloc_buf() and
> free_buf() calls. This is much more saner.
Yes, it was cleaner when I started using port_buffer more place.
And as mentioned below, I'll start using port buffer from
put_chars() as well, this will improve a few things.
> > +static DEFINE_SPINLOCK(list_lock);
> > +static LIST_HEAD(pending_free_list);
>
> The names list_lock, pending_free_list are very generic in the file's
> context. Please use more specific names.
Sure, I'll rename these.
>
> > +
> > static void free_buf(struct port_buffer *buf)
> > {
> > - kfree(buf->buf);
> > + unsigned long flags;
> > +
> > + if (!buf->dev) {
> > + kfree(buf->buf);
> > + goto freebuf;
> > + }
> > +
> > + BUG_ON(!rproc_enabled);
> > +
> > + /* dma_free_coherent requires interrupts to be enabled */
> > + if (rproc_enabled && !irqs_disabled()) {
>
> You don't need to check for rproc_enabled here.
Actually I do need this check. The reason is that I am
exploiting gcc's ability to discard dead code. When I compile
for arch's that does not have DMA, this block is dead and will be
discarded. This way I avoid the link error for the missing
symbol dma_free_coherent(). But I can add a comment on this.
> Then, you can just invert the if condition (if (irqs_disabled()) and
> include the relevant block here. This way, you can make do without
> the goto and return mess below.
Yeah, I did an earlier version without goto, but I wanted to separate
the rproc / non-rproc clearly to make it easier to see what happened
if rproc was disabled. But I'll have a stab at refactoring this code
again.
> > + 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);
> > + goto freebuf;
> > + }
> > +
...
> > + 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_KERNEL);
>
> incorrect indentation
OK, I'll fix this.
> > @@ -485,7 +582,10 @@ static void reclaim_consumed_buffers(struct port
> *port)
> > return;
> > }
> > while ((buf = virtqueue_get_buf(port->out_vq, &len))) {
> > - kfree(buf);
> > + if (is_console_port(port))
> > + kfree(buf);
> > + else
> > + free_buf(buf);
>
> Hm?
See below.
>
> > port->outvq_full = false;
> > }
> > }
> > @@ -498,6 +598,7 @@ static ssize_t send_buf(struct port *port, void
> *in_buf, size_t in_count,
> > ssize_t ret;
> > unsigned long flags;
> > unsigned int len;
> > + struct port_buffer *buf = in_buf;
>
> This looks wrong: the buffer we receive here is the actual data
> (buf->buf). It can never be a port_buffer (buf).
See below.
>
> >
> > out_vq = port->out_vq;
> >
> > @@ -505,8 +606,11 @@ static ssize_t send_buf(struct port *port, void
> *in_buf, size_t in_count,
> >
> > reclaim_consumed_buffers(port);
> >
> > - sg_init_one(sg, in_buf, in_count);
> > - ret = virtqueue_add_buf(out_vq, sg, 1, 0, in_buf, GFP_ATOMIC);
> > + if (is_console_port(port))
>
> I think you're misinterpreting what is_console_port() is. It means if
> a port is associated with an hvc/tty device.
See below.
>
> > + sg_init_one(sg, in_buf, in_count);
> > + else
> > + sg_init_one(sg, buf->buf, in_count);
> > + ret = virtqueue_add_buf(out_vq, sg, 1, 0, buf, GFP_ATOMIC);
> >
> > /* Tell Host to go! */
> > virtqueue_kick(out_vq);
> > @@ -669,7 +773,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;
> >
> > @@ -696,11 +800,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);
> > 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;
> > @@ -720,7 +824,7 @@ static ssize_t port_fops_write(struct file *filp,
> const char __user *ubuf,
> > goto out;
> >
> > free_buf:
> > - kfree(buf);
> > + free_buf(buf);
> > out:
> > return ret;
> > }
>
> OK, I now get what you did with send_buf() above. However, send_buf()
> now should be completely broken for non-rproc devices: you're
> allocating a buf instead of a buf->buf and passing that on to
> send_buf() as a void*. You should instead modify send_buf() to accept
> a struct port_buffer instead.
>
> Second, send_buf() receives a struct port_buffer(), but in the
> 'is_console_port()' case, you ignore that fact, and just pass on the
> void* pointer to sg_init_one(). You should instead pass buf->buf.
OK, so the issue here it that currently put_chars() passes a
char-buffer to send_buf() instead of a port_buffer. The tests above
tries to handle this case, distingusing between a tty and char device.
I agree that this is not the best solution.
But if I change put_chars to create a port_buffer and copy
data into it I can avoid the crap you pointed at above.
...
> > - if (virtio_has_feature(vdev, VIRTIO_CONSOLE_F_SIZE))
> > + if (!is_rproc_serial(vdev) &&
> > + virtio_has_feature(vdev, VIRTIO_CONSOLE_F_SIZE))
> > hvc_resize(port->cons.hvc, port->cons.ws);
>
> Why do you want to ensure !is_rproc_serial() here? As long as the
> device doesn't expose the VIRTIO_CONSOLE_F_SIZE feature, you should be
> fine, so this hunk can be dropped.
I need this test because virtio_check_driver_offered_feature() called
from virtio_has_feature will throw a BUG() if you test on a feature
not declared in the driver's feature-set.
> > @@ -1102,10 +1209,10 @@ 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;
> > -
> > + memset(buf->buf, 0, PAGE_SIZE);
>
> Why this memset here?
>
> 1. alloc_buf() already does kzalloc()
It used to do that, but not anymore. This patch
changes kzalloc() to kmalloc() in alloc_buf()
> 2. Is there any specific reason you want the buffer to be zeroed?
>
> I've recently realised zeroing out the buffer before giving it to the
> device serves no real purpose, and we're just slowing down the
> allocation here, so I'm tempted to convert the kzalloc() to
> kmalloc(), unless you have a specific need for zeroed pages.
Agree, the only reason is that I did memset was not to change legacy
behavior. I'd prefer to skip the memset too, so let's do that.
>
> > spin_lock_irq(lock);
> > ret = add_inbuf(vq, buf);
> > if (ret < 0) {
> > @@ -1198,10 +1305,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, but
> > + * the generic port implementation.
>
> s/but/only
OK, thanks.
>
> > + */
> > + 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;
> > @@ -1277,6 +1392,16 @@ 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);
> > + }
>
> braces around if can be dropped.
OK,
> > @@ -1722,13 +1847,17 @@ static int __devinit virtcons_probe(struct
> virtio_device *vdev)
> > goto free;
> > }
> >
> > - 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;
> > + } else {
> > + multiport = false;
> > + portdev->config.max_nr_ports = 1;
> > + }
>
> Why introduce the else part at all? Let these two statements be as
> they are, and just add the !is_rproc_serial check to the if statement?
OK, I'll keep assignments before "if" and skip the else part.
...
> - return register_virtio_driver(&virtio_console);
> > + err = register_virtio_driver(&virtio_console);
> > + if (err < 0) {
> > + pr_err("Error %d registering virtio driver\n", err);
> > + goto free;
> > + }
>
> This hunk is already present in linux-next; rebasing over that should
> get rid of it.
Sure, I'll rebase next patch to linux-next and send a new patch tomorrow.
Thanks,
Sjur
^ permalink raw reply
* Re: [PATCH 0/2] virtio-mmio updates for 3.7
From: Rusty Russell @ 2012-09-25 0:11 UTC (permalink / raw)
Cc: Brian Foley, Pawel Moll, virtualization
In-Reply-To: <1348493622-26091-1-git-send-email-pawel.moll@arm.com>
Pawel Moll <pawel.moll@arm.com> writes:
> Hi Rusty,
>
> Would you be so kind and consider getting those two small fixes into
> 3.7 merge? All credits go to Brian, all shame on me :-)
Should these also be cc'd to stable@kernel.org?
Cheers,
Rusty.
^ permalink raw reply
* Re: [PATCH v10 3/5] virtio_balloon: introduce migration primitives to balloon pages
From: Michael S. Tsirkin @ 2012-09-25 0:40 UTC (permalink / raw)
To: Rafael Aquini
Cc: Rik van Riel, Konrad Rzeszutek Wilk, linux-kernel, virtualization,
linux-mm, Peter Zijlstra, Andi Kleen, Minchan Kim, Andrew Morton,
Paul E. McKenney
In-Reply-To: <39738cbd4b596714210e453440833db7cca73172.1347897793.git.aquini@redhat.com>
On Mon, Sep 17, 2012 at 01:38:18PM -0300, Rafael Aquini wrote:
> Memory fragmentation introduced by ballooning might reduce significantly
> the number of 2MB contiguous memory blocks that can be used within a guest,
> thus imposing performance penalties associated with the reduced number of
> transparent huge pages that could be used by the guest workload.
>
> Besides making balloon pages movable at allocation time and introducing
> the necessary primitives to perform balloon page migration/compaction,
> this patch also introduces the following locking scheme, in order to
> enhance the syncronization methods for accessing elements of struct
> virtio_balloon, thus providing protection against concurrent access
> introduced by parallel memory compaction threads.
>
> - balloon_lock (mutex) : synchronizes the access demand to elements of
> struct virtio_balloon and its queue operations;
> - pages_lock (spinlock): special protection to balloon's pages bookmarking
> elements (list and atomic counters) against the
> potential memory compaction concurrency;
>
> Signed-off-by: Rafael Aquini <aquini@redhat.com>
> ---
> drivers/virtio/virtio_balloon.c | 305 +++++++++++++++++++++++++++++++++++++---
> 1 file changed, 286 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
> index 0908e60..a52c768 100644
> --- a/drivers/virtio/virtio_balloon.c
> +++ b/drivers/virtio/virtio_balloon.c
> @@ -27,6 +27,7 @@
> #include <linux/delay.h>
> #include <linux/slab.h>
> #include <linux/module.h>
> +#include <linux/balloon_compaction.h>
>
> /*
> * Balloon device works in 4K page units. So each page is pointed to by
> @@ -34,6 +35,7 @@
> * page units.
> */
> #define VIRTIO_BALLOON_PAGES_PER_PAGE (PAGE_SIZE >> VIRTIO_BALLOON_PFN_SHIFT)
> +#define VIRTIO_BALLOON_ARRAY_PFNS_MAX 256
>
> struct virtio_balloon
> {
> @@ -46,11 +48,24 @@ struct virtio_balloon
> /* The thread servicing the balloon. */
> struct task_struct *thread;
>
> + /* balloon special page->mapping */
> + struct address_space *mapping;
> +
> + /* Synchronize access/update to this struct virtio_balloon elements */
> + struct mutex balloon_lock;
Please document here nesting rules wrt page lock for this and pages_lock.
> +
> /* Waiting for host to ack the pages we released. */
> wait_queue_head_t acked;
>
> + /* Protect pages list, and pages bookeeping counters */
> + spinlock_t pages_lock;
> +
> + /* Number of balloon pages isolated from 'pages' list for compaction */
> + unsigned int num_isolated_pages;
> +
> /* Number of balloon pages we've told the Host we're not using. */
> unsigned int num_pages;
> +
> /*
> * The pages we've told the Host we're not using.
> * Each page on this list adds VIRTIO_BALLOON_PAGES_PER_PAGE
...
> @@ -122,13 +137,17 @@ static void set_page_pfns(u32 pfns[], struct page *page)
>
> static void fill_balloon(struct virtio_balloon *vb, size_t num)
> {
> + /* Get the proper GFP alloc mask from vb->mapping flags */
> + gfp_t vb_gfp_mask = mapping_gfp_mask(vb->mapping);
> +
> /* We can only do one array worth at a time. */
> num = min(num, ARRAY_SIZE(vb->pfns));
>
> + mutex_lock(&vb->balloon_lock);
> for (vb->num_pfns = 0; vb->num_pfns < num;
> vb->num_pfns += VIRTIO_BALLOON_PAGES_PER_PAGE) {
> - struct page *page = alloc_page(GFP_HIGHUSER | __GFP_NORETRY |
> - __GFP_NOMEMALLOC | __GFP_NOWARN);
> + struct page *page = alloc_page(vb_gfp_mask | __GFP_NORETRY |
> + __GFP_NOWARN | __GFP_NOMEMALLOC);
> if (!page) {
> if (printk_ratelimit())
> dev_printk(KERN_INFO, &vb->vdev->dev,
> @@ -139,9 +158,15 @@ static void fill_balloon(struct virtio_balloon *vb, size_t num)
> break;
> }
> set_page_pfns(vb->pfns + vb->num_pfns, page);
> - vb->num_pages += VIRTIO_BALLOON_PAGES_PER_PAGE;
> totalram_pages--;
> +
> + BUG_ON(!trylock_page(page));
So here page lock is nested within balloon_lock.
> + spin_lock(&vb->pages_lock);
> list_add(&page->lru, &vb->pages);
> + assign_balloon_mapping(page, vb->mapping);
> + vb->num_pages += VIRTIO_BALLOON_PAGES_PER_PAGE;
> + spin_unlock(&vb->pages_lock);
> + unlock_page(page);
> }
>
> /* Didn't get any? Oh well. */
> @@ -149,6 +174,7 @@ static void fill_balloon(struct virtio_balloon *vb, size_t num)
> return;
>
> tell_host(vb, vb->inflate_vq);
> + mutex_unlock(&vb->balloon_lock);
> }
...
> +/*
> + * virtballoon_migratepage - perform the balloon page migration on behalf of
> + * a compation thread. (called under page lock)
> + * @mapping: the page->mapping which will be assigned to the new migrated page.
> + * @newpage: page that will replace the isolated page after migration finishes.
> + * @page : the isolated (old) page that is about to be migrated to newpage.
> + * @mode : compaction mode -- not used for balloon page migration.
> + *
> + * After a ballooned page gets isolated by compaction procedures, this is the
> + * function that performs the page migration on behalf of a compaction thread
> + * The page migration for virtio balloon is done in a simple swap fashion which
> + * follows these two macro steps:
> + * 1) insert newpage into vb->pages list and update the host about it;
> + * 2) update the host about the old page removed from vb->pages list;
> + *
> + * This function preforms the balloon page migration task.
> + * Called through balloon_mapping->a_ops.
> + */
> +int virtballoon_migratepage(struct address_space *mapping,
> + struct page *newpage, struct page *page, enum migrate_mode mode)
> +{
> + struct virtio_balloon *vb = __page_balloon_device(page);
> +
> + BUG_ON(!vb);
> +
> + mutex_lock(&vb->balloon_lock);
While here balloon_lock is taken and according to documentation
this is called under page lock.
> +
> + /* balloon's page migration 1st step */
> + vb->num_pfns = VIRTIO_BALLOON_PAGES_PER_PAGE;
> + spin_lock(&vb->pages_lock);
> + list_add(&newpage->lru, &vb->pages);
> + assign_balloon_mapping(newpage, mapping);
> + vb->num_isolated_pages -= VIRTIO_BALLOON_PAGES_PER_PAGE;
> + spin_unlock(&vb->pages_lock);
> + set_page_pfns(vb->pfns, newpage);
> + tell_host(vb, vb->inflate_vq);
> +
> + /* balloon's page migration 2nd step */
> + vb->num_pfns = VIRTIO_BALLOON_PAGES_PER_PAGE;
> + clear_balloon_mapping(page);
> + set_page_pfns(vb->pfns, page);
> + tell_host(vb, vb->deflate_vq);
> +
> + mutex_unlock(&vb->balloon_lock);
> + wake_up(&vb->config_change);
> +
> + return BALLOON_MIGRATION_RETURN;
> +}
So nesting is reversed which is normally a problem.
Unfortunately lockep does not seem to work for page lock
otherwise it would detect this.
If this reversed nesting is not a problem, please add
comments in code documenting that this is intentional
and how it works.
--
MST
^ permalink raw reply
* Re: [PATCH v10 1/5] mm: introduce a common interface for balloon pages mobility
From: Michael S. Tsirkin @ 2012-09-25 1:05 UTC (permalink / raw)
To: Rafael Aquini
Cc: Rik van Riel, Konrad Rzeszutek Wilk, linux-kernel, virtualization,
linux-mm, Peter Zijlstra, Andi Kleen, Minchan Kim, Andrew Morton,
Paul E. McKenney
In-Reply-To: <20120918162420.GB1645@optiplex.redhat.com>
On Tue, Sep 18, 2012 at 01:24:21PM -0300, Rafael Aquini wrote:
> > > +static inline void assign_balloon_mapping(struct page *page,
> > > + struct address_space *mapping)
> > > +{
> > > + page->mapping = mapping;
> > > + smp_wmb();
> > > +}
> > > +
> > > +static inline void clear_balloon_mapping(struct page *page)
> > > +{
> > > + page->mapping = NULL;
> > > + smp_wmb();
> > > +}
> > > +
> > > +static inline gfp_t balloon_mapping_gfp_mask(void)
> > > +{
> > > + return GFP_HIGHUSER_MOVABLE;
> > > +}
> > > +
> > > +static inline bool __is_movable_balloon_page(struct page *page)
> > > +{
> > > + struct address_space *mapping = ACCESS_ONCE(page->mapping);
> > > + smp_read_barrier_depends();
> > > + return mapping_balloon(mapping);
> > > +}
> >
> > hm. Are these barrier tricks copied from somewhere else, or home-made?
> >
>
> They were introduced by a reviewer request to assure the proper ordering when
> inserting or deleting pages to/from a balloon device, so a given page won't get
> elected as being a balloon page before it gets inserted into the balloon's page
> list, just as it will only be deleted from the balloon's page list after it is
> decomissioned of its balloon page status (page->mapping wipe-out).
>
> Despite the mentioned operations only take place under proper locking, I thought
> it wouldn't hurt enforcing such order, thus I kept the barrier stuff. Btw,
> considering the aforementioned usage case, I just realized the
> assign_balloon_mapping() barrier is misplaced. I'll fix that and introduce
> comments on those function's usage.
If these are all under page lock these barriers just confuse things,
because they are almost never enough by themselves.
So in that case it would be better to drop them and document
usage as you are going to.
Even better would be lockdep check but unfortunately it
does not seem to be possible for page lock.
--
MST
^ permalink raw reply
* Re: [PATCH v10 0/5] make balloon pages movable by compaction
From: Michael S. Tsirkin @ 2012-09-25 1:17 UTC (permalink / raw)
To: Andrew Morton
Cc: Rik van Riel, Rafael Aquini, Konrad Rzeszutek Wilk, linux-kernel,
virtualization, linux-mm, Peter Zijlstra, Andi Kleen, Minchan Kim,
Paul E. McKenney
In-Reply-To: <20120917151531.e9ac59f2.akpm@linux-foundation.org>
On Mon, Sep 17, 2012 at 03:15:31PM -0700, Andrew Morton wrote:
> How can a patchset reach v10 and have zero Reviewed-by's?
I think the problem is, this adds an API between mm and balloon
device that is pretty complex: consider that previously we literally
only used alloc_page, __free_page and page->lru field.
So you end up with a problem: mm bits don't do anything
by themselves so mm people aren't very interested and
don't know about virtio anyway, while
we virtio device driver people lack a clue about compaction.
Having said that I think I'm getting my head about some of the issues so
I commented and the patchset is hopefully getting there.
--
MST
^ permalink raw reply
* [PATCH] virtio-blk: Disable callback in virtblk_done()
From: Asias He @ 2012-09-25 2:36 UTC (permalink / raw)
To: Rusty Russell; +Cc: virtualization, kvm, Michael S. Tsirkin
This reduces unnecessary interrupts that host could send to guest while
guest is in the progress of irq handling.
If one vcpu is handling the irq, while another interrupt comes, in
handle_edge_irq(), the guest will mask the interrupt via mask_msi_irq()
which is a very heavy operation that goes all the way down to host.
Signed-off-by: Asias He <asias@redhat.com>
---
drivers/block/virtio_blk.c | 19 +++++++++++--------
1 file changed, 11 insertions(+), 8 deletions(-)
diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
index 53b81d5..0bdde8f 100644
--- a/drivers/block/virtio_blk.c
+++ b/drivers/block/virtio_blk.c
@@ -274,15 +274,18 @@ static void virtblk_done(struct virtqueue *vq)
unsigned int len;
spin_lock_irqsave(vblk->disk->queue->queue_lock, flags);
- while ((vbr = virtqueue_get_buf(vblk->vq, &len)) != NULL) {
- if (vbr->bio) {
- virtblk_bio_done(vbr);
- bio_done = true;
- } else {
- virtblk_request_done(vbr);
- req_done = true;
+ do {
+ virtqueue_disable_cb(vq);
+ while ((vbr = virtqueue_get_buf(vblk->vq, &len)) != NULL) {
+ if (vbr->bio) {
+ virtblk_bio_done(vbr);
+ bio_done = true;
+ } else {
+ virtblk_request_done(vbr);
+ req_done = true;
+ }
}
- }
+ } while (!virtqueue_enable_cb(vq));
/* In case queue is stopped waiting for more buffers. */
if (req_done)
blk_start_queue(vblk->disk->queue);
--
1.7.11.4
^ permalink raw reply related
* Re: [PATCHv4] virtio_console: Add support for remoteproc serial
From: Amit Shah @ 2012-09-25 4:01 UTC (permalink / raw)
To: Sjur BRENDELAND
Cc: sjurbren@gmail.com, Arnd Bergmann, Michael S. Tsirkin,
Linus Walleij, linux-kernel@vger.kernel.org,
virtualization@lists.linux-foundation.org
In-Reply-To: <81C3A93C17462B4BBD7E272753C1057923BD79D728@EXDCVYMBSTM005.EQ1STM.local>
On (Mon) 24 Sep 2012 [23:50:01], Sjur BRENDELAND wrote:
> Hi Amit,
>
> > I'm sorry for not being able to look at this earlier.
>
> No worries. I'll try to respin and retest this patch by tomorrow.
> If you by any chance could find time to review so could make it in time
> for 3.7 it would be great :-)
I think it might be late for 3.7 already, I'd prefer to let this bake
for a while, ensure it passes my test suites, at the least. But I'll
let Rusty take the final call.
> > A general comment is to base this patchset on linux-next; we've been
> > seeing more than usual activity for virtio_console this time around.
> > I don't expect the conflicts to be big, though.
>
> Sure, I'll based the next patch on linux-next.
>
> ...
> > > 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.
> >
> > Any specific reason to not use the control queue? It's just another
> > virtio-serial port; the only special thing about it being it's an
> > internal channel between the device and driver.
>
> Yes, as mention to Michael earlier. I use rproc_serial for talking
> to a modem running in early boot phases, before the OS has started,
> or when the modem is executing it's crash handler. In both these
> cases the modem run in a very limited execution environment, so
> I want to keep the protocol and handling of the vqs as simple as
> possible. Due to this I really don't want more than single pair
> of vqs.
OK.
> We also have very simple use-cases. The port is opened once
> in the life-time of the modem, and only reopened after a
> cold-start of the modem. So I should not get into any issues
> with race conditions.
>
> > If you're not going to implement any control commands, I guess you
> > could conveniently not use the actual port, but keep it around, in
> > case you find use for it later. The advantage will be that older
> > kernels will work without any updates on newer devices.
>
> With the current usage pattern I have in mind, I'd rather add this
> feature later when/if needed. We can always add a new feature bit
> for this if we introduce the control channel later on.
OK.
> > > static void free_buf(struct port_buffer *buf)
> > > {
> > > - kfree(buf->buf);
> > > + unsigned long flags;
> > > +
> > > + if (!buf->dev) {
> > > + kfree(buf->buf);
> > > + goto freebuf;
> > > + }
> > > +
> > > + BUG_ON(!rproc_enabled);
> > > +
> > > + /* dma_free_coherent requires interrupts to be enabled */
> > > + if (rproc_enabled && !irqs_disabled()) {
> >
> > You don't need to check for rproc_enabled here.
>
> Actually I do need this check. The reason is that I am
> exploiting gcc's ability to discard dead code. When I compile
> for arch's that does not have DMA, this block is dead and will be
> discarded. This way I avoid the link error for the missing
> symbol dma_free_coherent(). But I can add a comment on this.
OK, I see. The BUG_ON would guarantee at run-time, but the
compile-time advantage wasn't obvious.
> > Then, you can just invert the if condition (if (irqs_disabled()) and
> > include the relevant block here. This way, you can make do without
> > the goto and return mess below.
>
> Yeah, I did an earlier version without goto, but I wanted to separate
> the rproc / non-rproc clearly to make it easier to see what happened
> if rproc was disabled. But I'll have a stab at refactoring this code
> again.
> > > @@ -485,7 +582,10 @@ static void reclaim_consumed_buffers(struct port
> > *port)
> > > return;
> > > }
> > > while ((buf = virtqueue_get_buf(port->out_vq, &len))) {
> > > - kfree(buf);
> > > + if (is_console_port(port))
> > > + kfree(buf);
> > > + else
> > > + free_buf(buf);
> >
> > Hm?
>
> See below.
>
> >
> > > port->outvq_full = false;
> > > }
> > > }
> > > @@ -498,6 +598,7 @@ static ssize_t send_buf(struct port *port, void
> > *in_buf, size_t in_count,
> > > ssize_t ret;
> > > unsigned long flags;
> > > unsigned int len;
> > > + struct port_buffer *buf = in_buf;
> >
> > This looks wrong: the buffer we receive here is the actual data
> > (buf->buf). It can never be a port_buffer (buf).
>
> See below.
>
> >
> > >
> > > out_vq = port->out_vq;
> > >
> > > @@ -505,8 +606,11 @@ static ssize_t send_buf(struct port *port, void
> > *in_buf, size_t in_count,
> > >
> > > reclaim_consumed_buffers(port);
> > >
> > > - sg_init_one(sg, in_buf, in_count);
> > > - ret = virtqueue_add_buf(out_vq, sg, 1, 0, in_buf, GFP_ATOMIC);
> > > + if (is_console_port(port))
> >
> > I think you're misinterpreting what is_console_port() is. It means if
> > a port is associated with an hvc/tty device.
>
> See below.
>
> >
> > > + sg_init_one(sg, in_buf, in_count);
> > > + else
> > > + sg_init_one(sg, buf->buf, in_count);
> > > + ret = virtqueue_add_buf(out_vq, sg, 1, 0, buf, GFP_ATOMIC);
> > >
> > > /* Tell Host to go! */
> > > virtqueue_kick(out_vq);
> > > @@ -669,7 +773,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;
> > >
> > > @@ -696,11 +800,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);
> > > 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;
> > > @@ -720,7 +824,7 @@ static ssize_t port_fops_write(struct file *filp,
> > const char __user *ubuf,
> > > goto out;
> > >
> > > free_buf:
> > > - kfree(buf);
> > > + free_buf(buf);
> > > out:
> > > return ret;
> > > }
> >
> > OK, I now get what you did with send_buf() above. However, send_buf()
> > now should be completely broken for non-rproc devices: you're
> > allocating a buf instead of a buf->buf and passing that on to
> > send_buf() as a void*. You should instead modify send_buf() to accept
> > a struct port_buffer instead.
> >
> > Second, send_buf() receives a struct port_buffer(), but in the
> > 'is_console_port()' case, you ignore that fact, and just pass on the
> > void* pointer to sg_init_one(). You should instead pass buf->buf.
>
> OK, so the issue here it that currently put_chars() passes a
> char-buffer to send_buf() instead of a port_buffer. The tests above
> tries to handle this case, distingusing between a tty and char device.
> I agree that this is not the best solution.
>
> But if I change put_chars to create a port_buffer and copy
> data into it I can avoid the crap you pointed at above.
Yes, it's much easier if we don't have special cases in these generic
routines.
> ...
> > > - if (virtio_has_feature(vdev, VIRTIO_CONSOLE_F_SIZE))
> > > + if (!is_rproc_serial(vdev) &&
> > > + virtio_has_feature(vdev, VIRTIO_CONSOLE_F_SIZE))
> > > hvc_resize(port->cons.hvc, port->cons.ws);
> >
> > Why do you want to ensure !is_rproc_serial() here? As long as the
> > device doesn't expose the VIRTIO_CONSOLE_F_SIZE feature, you should be
> > fine, so this hunk can be dropped.
>
> I need this test because virtio_check_driver_offered_feature() called
> from virtio_has_feature will throw a BUG() if you test on a feature
> not declared in the driver's feature-set.
Ah, OK.
> > > @@ -1102,10 +1209,10 @@ 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;
> > > -
> > > + memset(buf->buf, 0, PAGE_SIZE);
> >
> > Why this memset here?
> >
> > 1. alloc_buf() already does kzalloc()
>
> It used to do that, but not anymore. This patch
> changes kzalloc() to kmalloc() in alloc_buf()
Obviously I missed it :)
> > 2. Is there any specific reason you want the buffer to be zeroed?
> >
> > I've recently realised zeroing out the buffer before giving it to the
> > device serves no real purpose, and we're just slowing down the
> > allocation here, so I'm tempted to convert the kzalloc() to
> > kmalloc(), unless you have a specific need for zeroed pages.
>
> Agree, the only reason is that I did memset was not to change legacy
> behavior. I'd prefer to skip the memset too, so let's do that.
Can you please split that into a separate patch?
> > This hunk is already present in linux-next; rebasing over that should
> > get rid of it.
>
> Sure, I'll rebase next patch to linux-next and send a new patch tomorrow.
Thanks!
Amit
^ permalink raw reply
* Re: [PATCHv4] virtio_console: Add support for remoteproc serial
From: Rusty Russell @ 2012-09-25 5:25 UTC (permalink / raw)
To: Amit Shah, Sjur BRENDELAND
Cc: sjurbren@gmail.com, Arnd Bergmann, Michael S. Tsirkin,
Linus Walleij, linux-kernel@vger.kernel.org,
virtualization@lists.linux-foundation.org
In-Reply-To: <20120925040158.GC32657@amit.redhat.com>
Amit Shah <amit.shah@redhat.com> writes:
> On (Mon) 24 Sep 2012 [23:50:01], Sjur BRENDELAND wrote:
>> Hi Amit,
>>
>> > I'm sorry for not being able to look at this earlier.
>>
>> No worries. I'll try to respin and retest this patch by tomorrow.
>> If you by any chance could find time to review so could make it in time
>> for 3.7 it would be great :-)
>
> I think it might be late for 3.7 already, I'd prefer to let this bake
> for a while, ensure it passes my test suites, at the least. But I'll
> let Rusty take the final call.
It can make the next merge window, but it has to be done this week.
I'm only allowing it this late because it's a new driver.
Cheers,
Rusty.
^ permalink raw reply
* Re: [PATCHv4] virtio_console: Add support for remoteproc serial
From: Amit Shah @ 2012-09-25 6:50 UTC (permalink / raw)
To: Rusty Russell
Cc: sjurbren@gmail.com, Arnd Bergmann, Michael S. Tsirkin,
Linus Walleij, linux-kernel@vger.kernel.org,
virtualization@lists.linux-foundation.org, Sjur BRENDELAND
In-Reply-To: <87sja63c9b.fsf@rustcorp.com.au>
On (Tue) 25 Sep 2012 [14:55:04], Rusty Russell wrote:
> Amit Shah <amit.shah@redhat.com> writes:
>
> > On (Mon) 24 Sep 2012 [23:50:01], Sjur BRENDELAND wrote:
> >> Hi Amit,
> >>
> >> > I'm sorry for not being able to look at this earlier.
> >>
> >> No worries. I'll try to respin and retest this patch by tomorrow.
> >> If you by any chance could find time to review so could make it in time
> >> for 3.7 it would be great :-)
> >
> > I think it might be late for 3.7 already, I'd prefer to let this bake
> > for a while, ensure it passes my test suites, at the least. But I'll
> > let Rusty take the final call.
>
> It can make the next merge window, but it has to be done this week.
>
> I'm only allowing it this late because it's a new driver.
It does change code shared with the 'older' console and serial ports,
so the risk of regressions does exist.
Amit
^ permalink raw reply
* Re: [PATCH 0/2] virtio-mmio updates for 3.7
From: Pawel Moll @ 2012-09-25 11:12 UTC (permalink / raw)
To: Rusty Russell; +Cc: virtualization@lists.linux-foundation.org, Brian Foley
In-Reply-To: <87d31bufkl.fsf@rustcorp.com.au>
On Tue, 2012-09-25 at 01:11 +0100, Rusty Russell wrote:
> > Would you be so kind and consider getting those two small fixes into
> > 3.7 merge? All credits go to Brian, all shame on me :-)
>
> Should these also be cc'd to stable@kernel.org?
I was thinking about it, but as the problem manifests itself mainly on
architectures with large page size, the impact is limited.
But if you think it's worth it, I will repost with Cc: stable.
Cheers!
Paweł
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* [PATCHv6 0/3] virtio_console: Add rproc_serial device
From: sjur.brandeland @ 2012-09-25 13:47 UTC (permalink / raw)
To: Amit Shah
Cc: Michael S. Tsirkin, sjurbren, linux-kernel, virtualization,
Masami Hiramatsu, Linus Walleij, Sjur Brændeland
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
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* [PATCH 1/3] virtio_console:Merge struct buffer_token into struct port_buffer
From: sjur.brandeland @ 2012-09-25 13:47 UTC (permalink / raw)
To: Amit Shah
Cc: Michael S. Tsirkin, sjurbren, linux-kernel, virtualization,
Masami Hiramatsu, Linus Walleij, Sjur Brændeland
In-Reply-To: <1348580837-10919-1-git-send-email-sjur.brandeland@stericsson.com>
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
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox