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

quoted hunk

On Mon, Sep 03, 2012 at 03:51:16PM +0200, sjur.brandeland@stericsson.com wrote:
From: Sjur Brændeland <redacted>

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

This is needed for using virtio_console from the remoteproc
framework.

Signed-off-by: Sjur Brændeland <redacted>
cc: Rusty Russell <redacted>
cc: Michael S. Tsirkin <mst@redhat.com>
cc: Ohad Ben-Cohen <redacted>
cc: Linus Walleij <redacted>
cc: virtualization@lists.linux-foundation.org
How does access to descriptors work in this setup?

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

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

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

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

How does access to descriptors work in this setup?
When the ring is setup by remoteproc the descriptors are
also allocated using dma_alloc_coherent().

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

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

Regards,
Sjur
On Mon, Sep 03, 2012 at 04:57:45PM +0200, Sjur Brændeland wrote:
Hi Michael,

How does access to descriptors work in this setup?
When the ring is setup by remoteproc the descriptors are
also allocated using dma_alloc_coherent().

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

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

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

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

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

Now use if instead of ifdef.

-- 
MST
Hi Michael,

If an architecture do not support DMA you will get
a link error: "unknown symbol" for dma_alloc_coherent.
Yes, it even seems intentional.
But I have a question: can the device work without DMA?
The main dependency is actually on the dma-allocation.
In my case I do dma_declare_coherent_memory() with
the memory area shared with the remote device as argument.
Subsequent calls to dma_alloc_coherent() will then allocate
from this memory area.

Does not VIRTIO_CONSOLE_F_DMA_MEM mean dma api is required?
Yes.dma_alloc_coherent to work.

If yes you should just fail load.
Agree, I'll check on VIRTIO_CONSOLE_HAS_DMA before adding
VIRTIO_CONSOLE_F_DMA_MEM to the feature array.

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

#ifdef CONFIG_HAS_DMA
#define VIRTIO_CONSOLE_HAS_DMA (1)
#else
#define VIRTIO_CONSOLE_HAS_DMA (0)
#endif
OK, good point. Perhaps we could even exploit gcc's
ability to remove dead code and do something like this:

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

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

Regards,
Sjur
On Tue, Sep 04, 2012 at 01:28:36PM +0200, Sjur Brændeland wrote:
Hi Michael,

If an architecture do not support DMA you will get
a link error: "unknown symbol" for dma_alloc_coherent.
Yes, it even seems intentional.
But I have a question: can the device work without DMA?
The main dependency is actually on the dma-allocation.
In my case I do dma_declare_coherent_memory() with
the memory area shared with the remote device as argument.
Subsequent calls to dma_alloc_coherent() will then allocate
from this memory area.

Does not VIRTIO_CONSOLE_F_DMA_MEM mean dma api is required?
Yes.dma_alloc_coherent to work.

If yes you should just fail load.
Agree, I'll check on VIRTIO_CONSOLE_HAS_DMA before adding
VIRTIO_CONSOLE_F_DMA_MEM to the feature array.

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

#ifdef CONFIG_HAS_DMA
#define VIRTIO_CONSOLE_HAS_DMA (1)
#else
#define VIRTIO_CONSOLE_HAS_DMA (0)
#endif
OK, good point. Perhaps we could even exploit gcc's
ability to remove dead code and do something like this:

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

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

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

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

-- 
MST
Hi Michael,

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

Generally, using a feature bit for this is a bit of a problem though:
normally driver is expected to be able to simply ignore
a feature bit. In this case driver is required to
do something so a feature bit is not a good fit.
I am not sure what the right thing to do is.
I see - so in order to avoid the binding between driver and device
there are two options I guess. Either make virtio_dev_match() or
virtcons_probe() fail. Neither of them seems like the obvious choice.

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

Regards,
Sjur
On Tue, Sep 04, 2012 at 06:58:47PM +0200, Sjur Brændeland wrote:
Hi Michael,

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

Generally, using a feature bit for this is a bit of a problem though:
normally driver is expected to be able to simply ignore
a feature bit. In this case driver is required to
do something so a feature bit is not a good fit.
I am not sure what the right thing to do is.
I see - so in order to avoid the binding between driver and device
there are two options I guess. Either make virtio_dev_match() or
virtcons_probe() fail. Neither of them seems like the obvious choice.

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

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

-- 
MST
"Michael S. Tsirkin" [off-list ref] writes:
Also let's add a wrapper at top of file so ifdefs
do not litter the code like this. For example:

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

Now use if instead of ifdef.
The driver certainly shouldn't offer VIRTIO_CONSOLE_F_DMA_MEM if you
don't have DMA!

Cheers,
Rusty.
The driver certainly shouldn't offer VIRTIO_CONSOLE_F_DMA_MEM if you
don't have DMA!
OK, so the feature table could be done like this:

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

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

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

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

Regards,
Sjur
On Wed, Sep 05, 2012 at 03:00:20PM +0200, Sjur Brændeland wrote:
The driver certainly shouldn't offer VIRTIO_CONSOLE_F_DMA_MEM if you
don't have DMA!
OK, so the feature table could be done like this:

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

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

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

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

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

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

-- 
MST
Hi Michael.

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

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

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

Regards,
Sjur
Failing probe would be cleaner. But there is still a problem:
old driver will happily bind to that device and then
fail to work, right?
Not just fail to work, the kernel will panic on the BUG_ON().
Remoteproc gets the virtio configuration from firmware loaded
from user space. So this type of problem might be triggered
for other virtio drivers as well.


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

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

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

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

Regards,
Sjur
On Wed, Sep 05, 2012 at 08:15:36PM +0200, Sjur Brændeland wrote:
Hi Michael.

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

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

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

Regards,
Sjur
Failing probe would be cleaner. But there is still a problem:
old driver will happily bind to that device and then
fail to work, right?
Not just fail to work, the kernel will panic on the BUG_ON().
Remoteproc gets the virtio configuration from firmware loaded
from user space. So this type of problem might be triggered
for other virtio drivers as well.
how?


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

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


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

(Note the fundamental difference
between visualizations and remoteproc. For remoteproc
the virtio configuration comes from binaries loaded from
user space).

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

Regards,
Sjur
"Michael S. Tsirkin" [off-list ref] writes:
On Tue, Sep 04, 2012 at 06:58:47PM +0200, Sjur Brændeland wrote:
Hi Michael,

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

Generally, using a feature bit for this is a bit of a problem though:
normally driver is expected to be able to simply ignore
a feature bit. In this case driver is required to
do something so a feature bit is not a good fit.
I am not sure what the right thing to do is.
I see - so in order to avoid the binding between driver and device
there are two options I guess. Either make virtio_dev_match() or
virtcons_probe() fail. Neither of them seems like the obvious choice.

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

Regards,
Sjur
A simplest thing to do is change dev id. rusty?
For generic usage, this is correct.  But my opinion is that fallback on
feature non-ack is quality-of-implementation issue: great to have, but
there are cases where you just want to fail with "you're too old".

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

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

Cheers,
Rusty.
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
On Thu, Sep 06, 2012 at 11:34:25AM +0930, Rusty Russell wrote:
"Michael S. Tsirkin" [off-list ref] writes:
On Tue, Sep 04, 2012 at 06:58:47PM +0200, Sjur Brændeland wrote:
Hi Michael,

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

Generally, using a feature bit for this is a bit of a problem though:
normally driver is expected to be able to simply ignore
a feature bit. In this case driver is required to
do something so a feature bit is not a good fit.
I am not sure what the right thing to do is.
I see - so in order to avoid the binding between driver and device
there are two options I guess. Either make virtio_dev_match() or
virtcons_probe() fail. Neither of them seems like the obvious choice.

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

Regards,
Sjur
A simplest thing to do is change dev id. rusty?
For generic usage, this is correct.  But my opinion is that fallback on
feature non-ack is quality-of-implementation issue: great to have, but
there are cases where you just want to fail with "you're too old".

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

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

Cheers,
Rusty.
Yes but how can it signal guest that it will never proceed?

Also grep for BUG_ON in core found this:

       drv->remove(dev);

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

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

--->

virtio: don't crash when device is buggy

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

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

--

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

-- 
MST
Hi Ohad,

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

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

Can your userspace loader can refuse to proceed if the driver doesn't
ack the bits?  If so, it's simpler than a whole new ID.
Ohad: Are there any way we could avoid starting up the remote processor
if mandatory virtio features (such as DMA memory) are not supported by
the virtio-driver?

Regards,
Sjur
Hi Michael.

Not just fail to work, the kernel will panic on the BUG_ON().
Remoteproc gets the virtio configuration from firmware loaded
from user space. So this type of problem might be triggered
for other virtio drivers as well.
how?
...
Even if we fix this particular problem, the general problem
still exists: bogus virtio declarations in remoteproc's firmware
may cause BUG_ON().
which BUG_ON exactly?
I am afraid I have been barking up the wrong tree here.
Please ignore my previous rambling about panics related
to device features.

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

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

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

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

Regards,
Sjur
On Thu, Sep 06, 2012 at 08:27:35AM +0300, Michael S. Tsirkin wrote:
On Thu, Sep 06, 2012 at 11:34:25AM +0930, Rusty Russell wrote:
"Michael S. Tsirkin" [off-list ref] writes:
On Tue, Sep 04, 2012 at 06:58:47PM +0200, Sjur Brændeland wrote:
Hi Michael,

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

Generally, using a feature bit for this is a bit of a problem though:
normally driver is expected to be able to simply ignore
a feature bit. In this case driver is required to
do something so a feature bit is not a good fit.
I am not sure what the right thing to do is.
I see - so in order to avoid the binding between driver and device
there are two options I guess. Either make virtio_dev_match() or
virtcons_probe() fail. Neither of them seems like the obvious choice.

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

Regards,
Sjur
A simplest thing to do is change dev id. rusty?
For generic usage, this is correct.  But my opinion is that fallback on
feature non-ack is quality-of-implementation issue: great to have, but
there are cases where you just want to fail with "you're too old".

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

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

Cheers,
Rusty.
Yes but how can it signal guest that it will never proceed?

Also grep for BUG_ON in core found this:

       drv->remove(dev);

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

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

--->

virtio: don't crash when device is buggy

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

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

--
Rusty?

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

-- 
MST
"Michael S. Tsirkin" [off-list ref] writes:
virtio: don't crash when device is buggy

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

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Rusty?
Thanks, applied.

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

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

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

--

3.6 material?

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

-- 
MST
"Michael S. Tsirkin" [off-list ref] writes:

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

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

--

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

Cheers,
Rusty.
On Mon, Sep 17, 2012 at 01:57:17PM +0930, Rusty Russell wrote:
"Michael S. Tsirkin" [off-list ref] writes:

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

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

--

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

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

-- 
MST
"Michael S. Tsirkin" [off-list ref] writes:

On Mon, Sep 17, 2012 at 01:57:17PM +0930, Rusty Russell wrote:
"Michael S. Tsirkin" [off-list ref] writes:

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

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

--

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

Cheers,
Rusty.
Yes but can't malicious userspace trigger this with remoteproc? If yes
it's not a question of whether anyone has struck it since people don't
normally run malicious userspace :)
I think that malicious userspace is already priveleged, isn't it?

Cheers,
Rusty.

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help

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