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

[Amos Kong <akong@redhat.com>]

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
>