Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1472535 > unrolled thread

[PATCH] virtio_console: Stop doing DMA on the stack

Back to article view | Back to linux.kernel

Contents

  [PATCH] virtio_console: Stop doing DMA on the stack Andy Lutomirski <luto@kernel.org> - 2016-08-30 17:10 +0200
    Re: [PATCH] virtio_console: Stop doing DMA on the stack Amit Shah <amit.shah@redhat.com> - 2016-09-06 09:10 +0200
      Re: [PATCH] virtio_console: Stop doing DMA on the stack Ingo Molnar <mingo@kernel.org> - 2016-09-08 08:50 +0200
    [tip:x86/asm] virtio_console: Stop doing DMA on the stack tip-bot for Andy Lutomirski <tipbot@zytor.com> - 2016-09-08 11:50 +0200

#1472535 — [PATCH] virtio_console: Stop doing DMA on the stack

virtio_console uses a small DMA buffer for control requests.  Move
that buffer into heap memory.

Doing virtio DMA on the stack is normally okay on non-DMA-API virtio
systems (which is currently most of them), but it breaks completely
if the stack is virtually mapped.

Tested by typing both directions using picocom aimed at /dev/hvc0.

Signed-off-by: Andy Lutomirski <luto@kernel.org>
---

Hi all-

This is currently broken in tip:x86/asm.  If you (Amit) like this patch,
would it make sense for Ingo to add it to -tip?

Thanks,
Andy

 drivers/char/virtio_console.c | 23 +++++++++++++++--------
 1 file changed, 15 insertions(+), 8 deletions(-)

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index d2406fe25533..5da47e26a012 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -165,6 +165,12 @@ struct ports_device {
 	 */
 	struct virtqueue *c_ivq, *c_ovq;
 
+	/*
+	 * A control packet buffer for guest->host requests, protected
+	 * by c_ovq_lock.
+	 */
+	struct virtio_console_control cpkt;
+
 	/* Array of per-port IO virtqueues */
 	struct virtqueue **in_vqs, **out_vqs;
 
@@ -560,28 +566,29 @@ static ssize_t __send_control_msg(struct ports_device *portdev, u32 port_id,
 				  unsigned int event, unsigned int value)
 {
 	struct scatterlist sg[1];
-	struct virtio_console_control cpkt;
 	struct virtqueue *vq;
 	unsigned int len;
 
 	if (!use_multiport(portdev))
 		return 0;
 
-	cpkt.id = cpu_to_virtio32(portdev->vdev, port_id);
-	cpkt.event = cpu_to_virtio16(portdev->vdev, event);
-	cpkt.value = cpu_to_virtio16(portdev->vdev, value);
-
 	vq = portdev->c_ovq;
 
-	sg_init_one(sg, &cpkt, sizeof(cpkt));
-
 	spin_lock(&portdev->c_ovq_lock);
-	if (virtqueue_add_outbuf(vq, sg, 1, &cpkt, GFP_ATOMIC) == 0) {
+
+	portdev->cpkt.id = cpu_to_virtio32(portdev->vdev, port_id);
+	portdev->cpkt.event = cpu_to_virtio16(portdev->vdev, event);
+	portdev->cpkt.value = cpu_to_virtio16(portdev->vdev, value);
+
+	sg_init_one(sg, &portdev->cpkt, sizeof(struct virtio_console_control));
+
+	if (virtqueue_add_outbuf(vq, sg, 1, &portdev->cpkt, GFP_ATOMIC) == 0) {
 		virtqueue_kick(vq);
 		while (!virtqueue_get_buf(vq, &len)
 			&& !virtqueue_is_broken(vq))
 			cpu_relax();
 	}
+
 	spin_unlock(&portdev->c_ovq_lock);
 	return 0;
 }
-- 
2.7.4

[toc] | [next] | [standalone]

#1477108

On (Tue) 30 Aug 2016 [08:04:15], Andy Lutomirski wrote:
> virtio_console uses a small DMA buffer for control requests.  Move
> that buffer into heap memory.
> 
> Doing virtio DMA on the stack is normally okay on non-DMA-API virtio
> systems (which is currently most of them), but it breaks completely
> if the stack is virtually mapped.
> 
> Tested by typing both directions using picocom aimed at /dev/hvc0.
> 
> Signed-off-by: Andy Lutomirski <luto@kernel.org>

Looks fine,

Reviewed-by: Amit Shah <amit.shah@redhat.com>

> ---
> 
> Hi all-
> 
> This is currently broken in tip:x86/asm.  If you (Amit) like this patch,
> would it make sense for Ingo to add it to -tip?

Yes, I'm fine with that.


     	 Amit

[toc] | [prev] | [next] | [standalone]

#1478811

* Amit Shah <amit.shah@redhat.com> wrote:

> On (Tue) 30 Aug 2016 [08:04:15], Andy Lutomirski wrote:
> > virtio_console uses a small DMA buffer for control requests.  Move
> > that buffer into heap memory.
> > 
> > Doing virtio DMA on the stack is normally okay on non-DMA-API virtio
> > systems (which is currently most of them), but it breaks completely
> > if the stack is virtually mapped.
> > 
> > Tested by typing both directions using picocom aimed at /dev/hvc0.
> > 
> > Signed-off-by: Andy Lutomirski <luto@kernel.org>
> 
> Looks fine,
> 
> Reviewed-by: Amit Shah <amit.shah@redhat.com>
> 
> > ---
> > 
> > Hi all-
> > 
> > This is currently broken in tip:x86/asm.  If you (Amit) like this patch,
> > would it make sense for Ingo to add it to -tip?
> 
> Yes, I'm fine with that.

Thanks! FYI, this patch now lives as:

  9472fe7040bb ("virtio_console: Stop doing DMA on the stack")

in tip:x86/asm, and is targeted for a v4.9 merge.

Thanks,

	Ingo

[toc] | [prev] | [next] | [standalone]

#1478990 — [tip:x86/asm] virtio_console: Stop doing DMA on the stack

Commit-ID:  9472fe7040bba45c6200858cbe40d643cf02bccb
Gitweb:     http://git.kernel.org/tip/9472fe7040bba45c6200858cbe40d643cf02bccb
Author:     Andy Lutomirski <luto@kernel.org>
AuthorDate: Tue, 30 Aug 2016 08:04:15 -0700
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Thu, 8 Sep 2016 08:48:35 +0200

virtio_console: Stop doing DMA on the stack

virtio_console uses a small DMA buffer for control requests.  Move
that buffer into heap memory.

Doing virtio DMA on the stack is normally okay on non-DMA-API virtio
systems (which is currently most of them), but it breaks completely
if the stack is virtually mapped (CONFIG_VMAP_STACK=y).

Tested by typing both directions using picocom aimed at /dev/hvc0.

Signed-off-by: Andy Lutomirski <luto@kernel.org>
Reviewed-by: Amit Shah <amit.shah@redhat.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: virtualization@lists.linux-foundation.org
Link: http://lkml.kernel.org/r/0afe68f9b4be6c95af9e7672b07acd0274c26dfe.1472569320.git.luto@kernel.org
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 drivers/char/virtio_console.c | 23 +++++++++++++++--------
 1 file changed, 15 insertions(+), 8 deletions(-)

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index d2406fe..5da47e26 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -165,6 +165,12 @@ struct ports_device {
 	 */
 	struct virtqueue *c_ivq, *c_ovq;
 
+	/*
+	 * A control packet buffer for guest->host requests, protected
+	 * by c_ovq_lock.
+	 */
+	struct virtio_console_control cpkt;
+
 	/* Array of per-port IO virtqueues */
 	struct virtqueue **in_vqs, **out_vqs;
 
@@ -560,28 +566,29 @@ static ssize_t __send_control_msg(struct ports_device *portdev, u32 port_id,
 				  unsigned int event, unsigned int value)
 {
 	struct scatterlist sg[1];
-	struct virtio_console_control cpkt;
 	struct virtqueue *vq;
 	unsigned int len;
 
 	if (!use_multiport(portdev))
 		return 0;
 
-	cpkt.id = cpu_to_virtio32(portdev->vdev, port_id);
-	cpkt.event = cpu_to_virtio16(portdev->vdev, event);
-	cpkt.value = cpu_to_virtio16(portdev->vdev, value);
-
 	vq = portdev->c_ovq;
 
-	sg_init_one(sg, &cpkt, sizeof(cpkt));
-
 	spin_lock(&portdev->c_ovq_lock);
-	if (virtqueue_add_outbuf(vq, sg, 1, &cpkt, GFP_ATOMIC) == 0) {
+
+	portdev->cpkt.id = cpu_to_virtio32(portdev->vdev, port_id);
+	portdev->cpkt.event = cpu_to_virtio16(portdev->vdev, event);
+	portdev->cpkt.value = cpu_to_virtio16(portdev->vdev, value);
+
+	sg_init_one(sg, &portdev->cpkt, sizeof(struct virtio_console_control));
+
+	if (virtqueue_add_outbuf(vq, sg, 1, &portdev->cpkt, GFP_ATOMIC) == 0) {
 		virtqueue_kick(vq);
 		while (!virtqueue_get_buf(vq, &len)
 			&& !virtqueue_is_broken(vq))
 			cpu_relax();
 	}
+
 	spin_unlock(&portdev->c_ovq_lock);
 	return 0;
 }

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web