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

[Nikunj A Dadhania]

Thomas Huth <thuth at redhat.com> writes:

> 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?

I am not using this vq anywhere apart from initializing the vq
currently. So for other virtio device, same vq struct is going to be
used for the init, and a new desc is allocated.
> Should this variable maybe be part of "struct virtio_device" or so instead?

Number of vqs are not fixed per device, some have 1/2/3. One way will be
to create a per device structure like virtio_blk, and introduce a
private pointer

> If that's not feasible, I think you should at least make it "static".

This seems simple for now.

>
>>  /**
>>   * 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"?

Will  keep it int and return 0/-1

Regards
Nikunj