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

[Nikunj A Dadhania]

Alexey Kardashevskiy <aik at ozlabs.ru> writes:

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

The open client interface does provide such interface where there can be
multiple device open. From slof boot perspective, its single boot
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".
>
> 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