* [PATCH] kvm tools: Make virt_queue__available return false if queue is not initialized.
@ 2011-04-10 5:01 Asias He
2011-04-10 7:04 ` Pekka Enberg
0 siblings, 1 reply; 5+ messages in thread
From: Asias He @ 2011-04-10 5:01 UTC (permalink / raw)
To: Pekka Enberg, Cyrill Gorcunov, Ingo Molnar, akong; +Cc: kvm, 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.
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 <akong@redhat.com>
Signed-off-by: Asias He <asias.hejun@gmail.com>
---
tools/kvm/include/kvm/virtio.h | 22 ++++------------------
tools/kvm/virtio.c | 32 +++++++++++++++++++++++++-------
2 files changed, 29 insertions(+), 25 deletions(-)
diff --git a/tools/kvm/include/kvm/virtio.h b/tools/kvm/include/kvm/virtio.h
index 9f892a1..c64ae29 100644
--- a/tools/kvm/include/kvm/virtio.h
+++ b/tools/kvm/include/kvm/virtio.h
@@ -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 */
diff --git a/tools/kvm/virtio.c b/tools/kvm/virtio.c
index 6249521..2a19a14 100644
--- a/tools/kvm/virtio.c
+++ b/tools/kvm/virtio.c
@@ -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);
--
1.7.4.1
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH] kvm tools: Make virt_queue__available return false if queue is not initialized. 2011-04-10 5:01 [PATCH] kvm tools: Make virt_queue__available return false if queue is not initialized Asias He @ 2011-04-10 7:04 ` Pekka Enberg 2011-04-10 8:27 ` Amos Kong 2011-04-10 8:33 ` [RFC] [PATCH v2] " Amos Kong 0 siblings, 2 replies; 5+ messages in thread From: Pekka Enberg @ 2011-04-10 7:04 UTC (permalink / raw) To: Asias He; +Cc: Cyrill Gorcunov, Ingo Molnar, akong, kvm On Sun, Apr 10, 2011 at 8:01 AM, Asias He <asias.hejun@gmail.com> wrote: > 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] kvm tools: Make virt_queue__available return false if queue is not initialized. 2011-04-10 7:04 ` Pekka Enberg @ 2011-04-10 8:27 ` Amos Kong 2011-04-10 8:33 ` [RFC] [PATCH v2] " Amos Kong 1 sibling, 0 replies; 5+ messages in thread From: Amos Kong @ 2011-04-10 8:27 UTC (permalink / raw) To: Pekka Enberg; +Cc: Asias He, Cyrill Gorcunov, Ingo Molnar, kvm On Sun, Apr 10, 2011 at 10:04:57AM +0300, Pekka Enberg wrote: > On Sun, Apr 10, 2011 at 8:01 AM, Asias He <asias.hejun@gmail.com> wrote: > > 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 Hi Asias, Pekka, > 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; 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. > 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 <akong@redhat.com> > Signed-off-by: Asias He <asias.hejun@gmail.com> > --- > tools/kvm/include/kvm/virtio.h | 22 ++++------------------ > tools/kvm/virtio.c | 32 +++++++++++++++++++++++++------- > 2 files changed, 29 insertions(+), 25 deletions(-) > > diff --git a/tools/kvm/include/kvm/virtio.h b/tools/kvm/include/kvm/virtio.h > index 9f892a1..c64ae29 100644 > --- a/tools/kvm/include/kvm/virtio.h > +++ b/tools/kvm/include/kvm/virtio.h > @@ -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 */ > diff --git a/tools/kvm/virtio.c b/tools/kvm/virtio.c > index 6249521..2a19a14 100644 > --- a/tools/kvm/virtio.c > +++ b/tools/kvm/virtio.c > @@ -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); > -- > 1.7.4.1 > ^ permalink raw reply [flat|nested] 5+ messages in thread
* [RFC] [PATCH v2] kvm tools: Make virt_queue__available return false if queue is not initialized. 2011-04-10 7:04 ` Pekka Enberg 2011-04-10 8:27 ` Amos Kong @ 2011-04-10 8:33 ` Amos Kong 2011-04-10 8:44 ` Pekka Enberg 1 sibling, 1 reply; 5+ messages in thread From: Amos Kong @ 2011-04-10 8:33 UTC (permalink / raw) To: Pekka Enberg; +Cc: Asias He, Cyrill Gorcunov, Ingo Molnar, kvm 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 <akong@redhat.com> Signed-off-by: Asias He <asias.hejun@gmail.com> Signed-off-by: Amos Kong <akong@redhat.com> --- tools/kvm/include/kvm/virtio.h | 2 ++ 1 files changed, 2 insertions(+), 0 deletions(-) diff --git a/tools/kvm/include/kvm/virtio.h b/tools/kvm/include/kvm/virtio.h index 9f892a1..c8ff376 100644 --- a/tools/kvm/include/kvm/virtio.h +++ b/tools/kvm/include/kvm/virtio.h @@ -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; } -- 1.7.1 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [RFC] [PATCH v2] kvm tools: Make virt_queue__available return false if queue is not initialized. 2011-04-10 8:33 ` [RFC] [PATCH v2] " Amos Kong @ 2011-04-10 8:44 ` Pekka Enberg 0 siblings, 0 replies; 5+ messages in thread From: Pekka Enberg @ 2011-04-10 8:44 UTC (permalink / raw) To: Amos Kong; +Cc: Asias He, Cyrill Gorcunov, Ingo Molnar, kvm On Sun, 10 Apr 2011, Amos Kong wrote: > 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 <akong@redhat.com> > Signed-off-by: Asias He <asias.hejun@gmail.com> > Signed-off-by: Amos Kong <akong@redhat.com> Applied, thanks! ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2011-04-10 8:44 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2011-04-10 5:01 [PATCH] kvm tools: Make virt_queue__available return false if queue is not initialized Asias He 2011-04-10 7:04 ` Pekka Enberg 2011-04-10 8:27 ` Amos Kong 2011-04-10 8:33 ` [RFC] [PATCH v2] " Amos Kong 2011-04-10 8:44 ` Pekka Enberg
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox