[PATCH] kvm tools: Make virt_queue__available return false if queue is not initialized.

Asias He

author

2011-04-10 05:02 UTC #0 lore raw

PATCH 1/1 kvm tools: Make virt_queue__available return false if queue is not initialized.

Also add a check in virt_queue__get_iov to make sure queue is initialized.

virtio_console__inject_interrupt tries to use virt queues before guest tell us to initialize them.

Besides, commit b55da01875101b55a882618f7f9af3099af21a11 kvm tools: Make virtio console device code thread-safe has made virtio console device code thread safe.

(gdb) r run -i linux-0.2.img -k ./vmlinuz-2.6.38-rc6+ -r ./initrd.img-2.6.38-rc6+ -p=init=1 -m 500 -c
Starting program: /project/rh/kvm-tools/tools/kvm/kvm run -i linux-0.2.img -k ./vmlinuz-2.6.38-rc6+ -r ./initrd.img-2.6.38-rc6+ -p=init=1 -m 500 -c [Thread debugging using libthread_db enabled]
[New Thread 0x7fffd6e2d700 (LWP 19280)]
Warning: request type 8

Program received signal SIGSEGV, Segmentation fault.
0x00000000004026ca in virt_queue__available (vq=0x60d3c8) at include/kvm/virtio.h:31
31 return vq->vring.avail->idx != vq->last_avail_idx;
(gdb)
(gdb) bt
(gdb) p *vq
$2 = {vring = {num = 0, desc = 0x0, avail = 0x0, used = 0x0}, pfn = 0, last_avail_idx = 0}

I added the check of vq->vring.avail in virt_queue__available(), but it also failed.

...
static inline bool virt_queue__available(struct virt_queue *vq) {
+ if (!vq->vring.avail)

+                return -1;
        return vq->vring.avail->idx !=  vq->last_avail_idx;

}
...

(gdb) r run -i linux-0.2.img -k ./vmlinuz-2.6.38-rc6+ -r ./initrd.img-2.6.38-rc6+ -p=init=1 -m 500 -c
Starting program: /project/rh/kvm-tools/tools/kvm/kvm run -i linux-0.2.img -k ./vmlinuz-2.6.38-rc6+ -r ./initrd.img-2.6.38-rc6+ -p=init=1 -m 500 -c [Thread debugging using libthread_db enabled]
[New Thread 0x7fffd6e2d700 (LWP 19434)]
Warning: request type 8

Program received signal SIGFPE, Arithmetic exception.
0x00000000004066cd in virt_queue__pop (queue=0x60d3c8) at include/kvm/virtio.h:21
21 return queue->vring.avail->ring[queue->last_avail_idx++ % queue->vring.num];
(gdb) bt
(gdb) p *queue
$2 = {vring = {num = 0, desc = 0x0, avail = 0x0, used = 0x0}, pfn = 0, last_avail_idx = 0}

Reported-by Amos Kong <[email protected]>

Signed-off-by Asias He <[email protected]>

---
 tools/kvm/include/kvm/virtio.h |   22 ++++------------------
 tools/kvm/virtio.c             |   32 +++++++++++++++++++++++++-------
 2 files changed, 29 insertions(+), 25 deletions(-)

tools/kvm/include/kvm/virtio.h +4 ~~-18~~

index 9f892a1..c64ae29 100644
@@ -16,23 +16,9 @@ struct virt_queue {
 	uint16_t			last_avail_idx;
 };
 
-static inline uint16_t virt_queue__pop(struct virt_queue *queue)
-{
-	return queue->vring.avail->ring[queue->last_avail_idx++ % queue->vring.num];
-}
-
-static inline struct vring_desc *virt_queue__get_desc(struct virt_queue *queue, uint16_t desc_ndx)
-{
-	return &queue->vring.desc[desc_ndx];
-}
-
-static inline bool virt_queue__available(struct virt_queue *vq)
-{
-	return vq->vring.avail->idx !=  vq->last_avail_idx;
-}
-
-struct vring_used_elem *virt_queue__set_used_elem(struct virt_queue *queue, uint32_t head, uint32_t len);
-
-uint16_t virt_queue__get_iov(struct virt_queue *queue, struct iovec iov[], uint16_t *out, uint16_t *in, struct kvm *kvm);
+uint16_t virt_queue__get_iov(struct virt_queue *vq, struct iovec iov[], uint16_t *out, uint16_t *in, struct kvm *kvm);
+struct vring_used_elem *virt_queue__set_used_elem(struct virt_queue *vq, uint32_t head, uint32_t len);
+struct vring_desc *virt_queue__get_desc(struct virt_queue *vq, uint16_t desc_ndx);
+bool virt_queue__available(struct virt_queue *vq);
 
 #endif /* KVM__VIRTIO_H */

tools/kvm/virtio.c +25 -7

index 6249521..2a19a14 100644
@@ -4,25 +4,43 @@
 #include "kvm/kvm.h"
 #include "kvm/virtio.h"
 
-struct vring_used_elem *virt_queue__set_used_elem(struct virt_queue *queue, uint32_t head, uint32_t len)
+bool virt_queue__available(struct virt_queue *vq)
+{
+	if (!vq->vring.avail)
+		return false;
+
+	return vq->vring.avail->idx !=  vq->last_avail_idx;
+}
+
+struct vring_desc *virt_queue__get_desc(struct virt_queue *vq, uint16_t desc_ndx)
+{
+	return &vq->vring.desc[desc_ndx];
+}
+
+struct vring_used_elem *virt_queue__set_used_elem(struct virt_queue *vq, uint32_t head, uint32_t len)
 {
 	struct vring_used_elem *used_elem;
-	used_elem	= &queue->vring.used->ring[queue->vring.used->idx++ % queue->vring.num];
+	used_elem	= &vq->vring.used->ring[vq->vring.used->idx++ % vq->vring.num];
 	used_elem->id	= head;
 	used_elem->len	= len;
 	return used_elem;
 }
 
-uint16_t virt_queue__get_iov(struct virt_queue *queue, struct iovec iov[], uint16_t *out, uint16_t *in, struct kvm *kvm)
+uint16_t virt_queue__get_iov(struct virt_queue *vq, struct iovec iov[], uint16_t *out, uint16_t *in, struct kvm *kvm)
 {
 	struct vring_desc *desc;
 	uint16_t head, idx;
 
-	idx = head = virt_queue__pop(queue);
-	*out = *in = 0;
+	if (!virt_queue__available(vq))
+		return -1;
+
+	head		= vq->vring.avail->ring[vq->last_avail_idx++ % vq->vring.num];
+	idx		= head;
+	*out		= 0;
+	*in		= 0;
 
 	do {
-		desc				= virt_queue__get_desc(queue, idx);
+		desc				= virt_queue__get_desc(vq, idx);
 		iov[*out + *in].iov_base	= guest_flat_to_host(kvm, desc->addr);
 		iov[*out + *in].iov_len		= desc->len;
 		if (desc->flags & VRING_DESC_F_WRITE)

@@ -30,7 +48,7 @@ uint16_t virt_queue__get_iov(struct virt_queue *queue, struct iovec iov[], uint1
 		else
 			(*out)++;
 		if (desc->flags & VRING_DESC_F_NEXT)
-			idx = desc->next;
+			idx			= desc->next;
 		else
 			break;
 	} while (1);

Signature · 1 line

1.7.4.1

Pekka Enberg

2011-04-10 07:04 UTC #1 lore raw

Quoted 4 lines · Asias He

Also add a check in virt_queue__get_iov to make sure queue is initialized.
virtio_console__inject_interrupt tries to use virt queues before guest tell us to initialize them.

So I think we need to fix this in virtio_console__inject_interrupt() and *not* in virt_queue__get_iov() which is very low-level. Isn't it as simple as adding a ->initialized boolean flag to struct console_device for now?

Alternative, cleaner implementation is to lazily register the device to some list upon initialization. virtio_console__inject_interrupt() could the use that list for injecting interrupts instead of touching hard-coded struct console_device all the time.

But I'd personally go for the flag now.

Pekka

Amos Kong

2011-04-10 08:27 UTC #2 lore raw

Quoted 11 lines · Pekka Enberg

Quoted 4 lines · Asias He
Also add a check in virt_queue__get_iov to make sure queue is initialized.
virtio_console__inject_interrupt tries to use virt queues before guest tell us to initialize them.
So I think we need to fix this in virtio_console__inject_interrupt() and *not* in virt_queue__get_iov() which is very low-level. Isn't it as simple as adding a ->initialized boolean flag to struct console_device for now?

Quoted 8 lines

Alternative, cleaner implementation is to lazily register the device to some list upon initialization. virtio_console__inject_interrupt() could the use that list for injecting interrupts instead of touching hard-coded struct console_device all the time.
But I'd personally go for the flag now.
Pekka

Hi Asias, Pekka,

Quoted 23 lines

Besides, commit b55da01875101b55a882618f7f9af3099af21a11 kvm tools: Make virtio console device code thread-safe has made virtio console device code thread safe.
(gdb) r run -i linux-0.2.img -k ./vmlinuz-2.6.38-rc6+ -r ./initrd.img-2.6.38-rc6+ -p=init=1 -m 500 -c
Starting program: /project/rh/kvm-tools/tools/kvm/kvm run -i linux-0.2.img -k ./vmlinuz-2.6.38-rc6+ -r ./initrd.img-2.6.38-rc6+ -p=init=1 -m 500 -c [Thread debugging using libthread_db enabled]
[New Thread 0x7fffd6e2d700 (LWP 19280)]
Warning: request type 8
Program received signal SIGSEGV, Segmentation fault.
0x00000000004026ca in virt_queue__available (vq=0x60d3c8) at include/kvm/virtio.h:31
31 return vq->vring.avail->idx != vq->last_avail_idx;
(gdb)
(gdb) bt
(gdb) p *vq
$2 = {vring = {num = 0, desc = 0x0, avail = 0x0, used = 0x0}, pfn = 0, last_avail_idx = 0}
I added the check of vq->vring.avail in virt_queue__available(), but it also failed.
...
static inline bool virt_queue__available(struct virt_queue *vq) {

tools/kvm/virtio.c line 35

Quoted 2 lines

tools/kvm/virtio.c +2 -0

+        if (!vq->vring.avail)
+                return -1;

It's wrong here! it should return '0' when virt_queue is not avaiable.

static inline bool virt_queue__available(struct virt_queue *vq) {
+ if (!vq->vring.avail)

+                return 0;
        return vq->vring.avail->idx !=  vq->last_avail_idx;

}

then

59 void virtio_console__inject_interrupt(struct kvm *self)
....
71 if (term_readable(CONSOLE_VIRTIO) && virt_queue__available(vq)) {
72 head = virt_queue__get_iov(vq, iov, &out, &in, self);
^^^^ then this block will not be executed.

Quoted 123 lines

        return vq->vring.avail->idx !=  vq->last_avail_idx;

}
...

(gdb) r run -i linux-0.2.img -k ./vmlinuz-2.6.38-rc6+ -r ./initrd.img-2.6.38-rc6+ -p=init=1 -m 500 -c
Starting program: /project/rh/kvm-tools/tools/kvm/kvm run -i linux-0.2.img -k ./vmlinuz-2.6.38-rc6+ -r ./initrd.img-2.6.38-rc6+ -p=init=1 -m 500 -c [Thread debugging using libthread_db enabled]
[New Thread 0x7fffd6e2d700 (LWP 19434)]
Warning: request type 8

Program received signal SIGFPE, Arithmetic exception.
0x00000000004066cd in virt_queue__pop (queue=0x60d3c8) at include/kvm/virtio.h:21
21 return queue->vring.avail->ring[queue->last_avail_idx++ % queue->vring.num];
(gdb) bt
(gdb) p *queue
$2 = {vring = {num = 0, desc = 0x0, avail = 0x0, used = 0x0}, pfn = 0, last_avail_idx = 0}

Reported-by Amos Kong <[email protected]>

Signed-off-by Asias He <[email protected]>

---
 tools/kvm/include/kvm/virtio.h |   22 ++++------------------
 tools/kvm/virtio.c             |   32 +++++++++++++++++++++++++-------
 2 files changed, 29 insertions(+), 25 deletions(-)

tools/kvm/include/kvm/virtio.h +4 ~~-18~~

index 9f892a1..c64ae29 100644
@@ -16,23 +16,9 @@ struct virt_queue {
 	uint16_t			last_avail_idx;
 };
 
-static inline uint16_t virt_queue__pop(struct virt_queue *queue)
-{
-	return queue->vring.avail->ring[queue->last_avail_idx++ % queue->vring.num];
-}
-
-static inline struct vring_desc *virt_queue__get_desc(struct virt_queue *queue, uint16_t desc_ndx)
-{
-	return &queue->vring.desc[desc_ndx];
-}
-
-static inline bool virt_queue__available(struct virt_queue *vq)
-{
-	return vq->vring.avail->idx !=  vq->last_avail_idx;
-}
-
-struct vring_used_elem *virt_queue__set_used_elem(struct virt_queue *queue, uint32_t head, uint32_t len);
-
-uint16_t virt_queue__get_iov(struct virt_queue *queue, struct iovec iov[], uint16_t *out, uint16_t *in, struct kvm *kvm);
+uint16_t virt_queue__get_iov(struct virt_queue *vq, struct iovec iov[], uint16_t *out, uint16_t *in, struct kvm *kvm);
+struct vring_used_elem *virt_queue__set_used_elem(struct virt_queue *vq, uint32_t head, uint32_t len);
+struct vring_desc *virt_queue__get_desc(struct virt_queue *vq, uint16_t desc_ndx);
+bool virt_queue__available(struct virt_queue *vq);
 
 #endif /* KVM__VIRTIO_H */

tools/kvm/virtio.c +25 -7

index 6249521..2a19a14 100644
@@ -4,25 +4,43 @@
 #include "kvm/kvm.h"
 #include "kvm/virtio.h"
 
-struct vring_used_elem *virt_queue__set_used_elem(struct virt_queue *queue, uint32_t head, uint32_t len)
+bool virt_queue__available(struct virt_queue *vq)
+{
+	if (!vq->vring.avail)
+		return false;
+
+	return vq->vring.avail->idx !=  vq->last_avail_idx;
+}
+
+struct vring_desc *virt_queue__get_desc(struct virt_queue *vq, uint16_t desc_ndx)
+{
+	return &vq->vring.desc[desc_ndx];
+}
+
+struct vring_used_elem *virt_queue__set_used_elem(struct virt_queue *vq, uint32_t head, uint32_t len)
 {
 	struct vring_used_elem *used_elem;
-	used_elem	= &queue->vring.used->ring[queue->vring.used->idx++ % queue->vring.num];
+	used_elem	= &vq->vring.used->ring[vq->vring.used->idx++ % vq->vring.num];
 	used_elem->id	= head;
 	used_elem->len	= len;
 	return used_elem;
 }
 
-uint16_t virt_queue__get_iov(struct virt_queue *queue, struct iovec iov[], uint16_t *out, uint16_t *in, struct kvm *kvm)
+uint16_t virt_queue__get_iov(struct virt_queue *vq, struct iovec iov[], uint16_t *out, uint16_t *in, struct kvm *kvm)
 {
 	struct vring_desc *desc;
 	uint16_t head, idx;
 
-	idx = head = virt_queue__pop(queue);
-	*out = *in = 0;
+	if (!virt_queue__available(vq))
+		return -1;
+
+	head		= vq->vring.avail->ring[vq->last_avail_idx++ % vq->vring.num];
+	idx		= head;
+	*out		= 0;
+	*in		= 0;
 
 	do {
-		desc				= virt_queue__get_desc(queue, idx);
+		desc				= virt_queue__get_desc(vq, idx);
 		iov[*out + *in].iov_base	= guest_flat_to_host(kvm, desc->addr);
 		iov[*out + *in].iov_len		= desc->len;
 		if (desc->flags & VRING_DESC_F_WRITE)

@@ -30,7 +48,7 @@ uint16_t virt_queue__get_iov(struct virt_queue *queue, struct iovec iov[], uint1
 		else
 			(*out)++;
 		if (desc->flags & VRING_DESC_F_NEXT)
-			idx = desc->next;
+			idx			= desc->next;
 		else
 			break;
 	} while (1);

Signature · 1 line

1.7.4.1

Amos Kong

2011-04-10 08:33 UTC #3 lore raw

RFC PATCH v2 1/1 kvm tools: Make virt_queue__available return false if queue is not initialized.

In reply to Pekka Enberg

virtio_console__inject_interrupt tries to use virt queues before guest tell us to initialize them.

(gdb) r run -i linux-0.2.img -k ./vmlinuz-2.6.38-rc6+ -r ./initrd.img-2.6.38-rc6+ -p=init=1 -m 500 -c
Starting program: /project/rh/kvm-tools/tools/kvm/kvm run -i linux-0.2.img -k ./vmlinuz-2.6.38-rc6+ -r ./initrd.img-2.6.38-rc6+ -p=init=1 -m 500 -c [Thread debugging using libthread_db enabled]
[New Thread 0x7fffd6e2d700 (LWP 19280)]
Warning: request type 8

Program received signal SIGSEGV, Segmentation fault.
0x00000000004026ca in virt_queue__available (vq=0x60d3c8) at include/kvm/virtio.h:31
31 return vq->vring.avail->idx != vq->last_avail_idx;
(gdb)
(gdb) bt
(gdb) p *vq
$2 = {vring = {num = 0, desc = 0x0, avail = 0x0, used = 0x0}, pfn = 0, last_avail_idx = 0}

include/kvm/virtio-console.h:
59 void virtio_console__inject_interrupt(struct kvm *self)
....
71 if (term_readable(CONSOLE_VIRTIO) && virt_queue__available(vq)) {
72 head = virt_queue__get_iov(vq, iov, &out, &in, self);
^^^^ then this block will not be executed if
virtio_queue is unavaiable.

Changes from v1:
- move the check of virt_queue out of virt_queue__get_iov()

Reported-by Amos Kong <[email protected]>

Signed-off-by Asias He <[email protected]>

Signed-off-by Amos Kong <[email protected]>

---
 tools/kvm/include/kvm/virtio.h |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

tools/kvm/include/kvm/virtio.h +2 -0

index 9f892a1..c8ff376 100644
@@ -28,6 +28,8 @@ static inline struct vring_desc *virt_queue__get_desc(struct virt_queue *queue,
 
 static inline bool virt_queue__available(struct virt_queue *vq)
 {
+        if (!vq->vring.avail)
+		 return 0;
 	return vq->vring.avail->idx !=  vq->last_avail_idx;
 }

Signature · 1 line

1.7.1

Pekka Enberg

Applied

2011-04-10 08:44 UTC #4 lore raw

Quoted 26 lines · Amos Kong

virtio_console__inject_interrupt tries to use virt queues before guest tell us to initialize them.
(gdb) r run -i linux-0.2.img -k ./vmlinuz-2.6.38-rc6+ -r ./initrd.img-2.6.38-rc6+ -p=init=1 -m 500 -c
Starting program: /project/rh/kvm-tools/tools/kvm/kvm run -i linux-0.2.img -k ./vmlinuz-2.6.38-rc6+ -r ./initrd.img-2.6.38-rc6+ -p=init=1 -m 500 -c [Thread debugging using libthread_db enabled]
[New Thread 0x7fffd6e2d700 (LWP 19280)]
Warning: request type 8
Program received signal SIGSEGV, Segmentation fault.
0x00000000004026ca in virt_queue__available (vq=0x60d3c8) at include/kvm/virtio.h:31
31 return vq->vring.avail->idx != vq->last_avail_idx;
(gdb)
(gdb) bt
(gdb) p *vq
$2 = {vring = {num = 0, desc = 0x0, avail = 0x0, used = 0x0}, pfn = 0, last_avail_idx = 0}
include/kvm/virtio-console.h:
59 void virtio_console__inject_interrupt(struct kvm *self)
....
71 if (term_readable(CONSOLE_VIRTIO) && virt_queue__available(vq)) {
72 head = virt_queue__get_iov(vq, iov, &out, &in, self);
^^^^ then this block will not be executed if
virtio_queue is unavaiable.

Quoted 6 lines

Changes from v1:
- move the check of virt_queue out of virt_queue__get_iov()
Reported-by Amos Kong <[email protected]>
Signed-off-by Asias He <[email protected]>
Signed-off-by Amos Kong <[email protected]>

Applied, thanks!