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

[Alexey Kardashevskiy]

On 01/21/2016 12:12 AM, Thomas Huth wrote:
> 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?


Can SLOF possibly work with more than a single boot device at the time?


> 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".

Agree about "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"?

I vote for int/0/-1.



-- 
Alexey