[SLOF] [PATCH v2 04/19] virtio: introduce helper for initializing virt queue

Thomas Huth thuth at redhat.com
Thu Jan 21 00:12:58 AEDT 2016


On 20.01.2016 13:10, Nikunj A Dadhania wrote:
> The routine takes care to allocate and set the queue address in the
> device. Add these calls in virtio-net, virtio-blk and virtio-9p.
> 
> With the lack of this routine, devices like virtio-blk and virtio-9p did
> not do a device reset in the driver initialization code. This helper
> will fix that problem
> 
> Change the signature of virtio_set_qaddr, accepting queue address as
> unsigned long argumet.
> 
> Signed-off-by: Nikunj A Dadhania <nikunj at linux.vnet.ibm.com>
> ---
...
> diff --git a/lib/libvirtio/virtio-9p.c b/lib/libvirtio/virtio-9p.c
> index 5a5fd01..81cedb9 100644
> --- a/lib/libvirtio/virtio-9p.c
> +++ b/lib/libvirtio/virtio-9p.c
> @@ -19,6 +19,7 @@
>  #include "virtio-9p.h"
>  #include "p9.h"
>  
> +struct vqs vq;

Defining this as global sounds wrong - what if there is more than one
virtio device?
Should this variable maybe be part of "struct virtio_device" or so instead?
If that's not feasible, I think you should at least make it "static".

>  /**
>   * Notes for 9P Server config:
> @@ -189,6 +190,12 @@ int virtio_9p_init(struct virtio_device *dev, void *tx_buf, void *rx_buf,
>  	/* Device specific setup - we do not support special features */
>  	virtio_set_guest_features(dev,  0);
>  
> +	if(!virtio_queue_init_vq(dev, &vq, 0)) {
> +		virtio_set_status(dev, VIRTIO_STAT_ACKNOWLEDGE|VIRTIO_STAT_DRIVER
> +				  |VIRTIO_STAT_FAILED);
> +		return -1;
> +	}
> +
>  	vq_avail = virtio_get_vring_avail(dev, 0);
>  	vq_avail->flags = VRING_AVAIL_F_NO_INTERRUPT;
>  	vq_avail->idx = 0;
> diff --git a/lib/libvirtio/virtio-blk.c b/lib/libvirtio/virtio-blk.c
> index 826f2ea..28d9c29 100644
> --- a/lib/libvirtio/virtio-blk.c
> +++ b/lib/libvirtio/virtio-blk.c
> @@ -18,6 +18,8 @@
>  
>  #define DEFAULT_SECTOR_SIZE 512
>  
> +struct vqs vq;

dito

>  /**
>   * Initialize virtio-block device.
>   * @param  dev  pointer to virtio device information
> @@ -44,6 +46,12 @@ virtioblk_init(struct virtio_device *dev)
>  	/* Device specific setup - we support F_BLK_SIZE */
>  	virtio_set_guest_features(dev,  VIRTIO_BLK_F_BLK_SIZE);
>  
> +	if(!virtio_queue_init_vq(dev, &vq, 0)) {
> +		virtio_set_status(dev, VIRTIO_STAT_ACKNOWLEDGE|VIRTIO_STAT_DRIVER
> +				  |VIRTIO_STAT_FAILED);
> +		return 0;
> +	}
> +
>  	vq_avail = virtio_get_vring_avail(dev, 0);
>  	vq_avail->flags = VRING_AVAIL_F_NO_INTERRUPT;
>  	vq_avail->idx = 0;

> +int virtio_queue_init_vq(struct virtio_device *dev, struct vqs *vq, unsigned int id)
> +{
> +	vq->size = virtio_get_qsize(dev, id);
> +	vq->desc = SLOF_alloc_mem_aligned(virtio_vring_size(vq->size), 4096);
> +	if (!vq->desc) {
> +		printf("memory allocation failed!\n");
> +		return false;
> +	}
> +	memset(vq->desc, 0, virtio_vring_size(vq->size));
> +	virtio_set_qaddr(dev, id, (unsigned long)vq->desc);
> +	vq->avail = virtio_get_vring_avail(dev, id);
> +	vq->used = virtio_get_vring_used(dev, id);
> +	vq->id = id;
> +	return true;
> +}

Introducing that function is basically a very good idea ... but since
most of the virtio code seems to use 0 for success and -1 for error
code, I think it would be better if you'd change the return statements
here for consistency? If you do not want to change it, could you at
least change the return type to "bool" instead of "int"?

 Thomas




More information about the SLOF mailing list