[SLOF] [PATCH v1 05/27] virtio-blk: use virt queue init and reset init

[Alexey Kardashevskiy]

On 01/14/2016 09:24 PM, Nikunj A Dadhania wrote:
> Alexey Kardashevskiy <aik at ozlabs.ru> writes:
>
>> On 01/13/2016 10:16 PM, Nikunj A Dadhania wrote:
>>> Move initialization of virt queue to the init and
>>> with this reset will start working now.
>>>
>>> Signed-off-by: Nikunj A Dadhania <nikunj at linux.vnet.ibm.com>
>>> ---
>>>    board-qemu/slof/pci-device_1af4_1001.fs |  9 ---------
>>>    lib/libvirtio/virtio-blk.c              | 13 +++++++++----
>>>    2 files changed, 9 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/board-qemu/slof/pci-device_1af4_1001.fs b/board-qemu/slof/pci-device_1af4_1001.fs
>>> index 4591ccb..db0bb3f 100644
>>> --- a/board-qemu/slof/pci-device_1af4_1001.fs
>>> +++ b/board-qemu/slof/pci-device_1af4_1001.fs
>>> @@ -22,13 +22,4 @@ pci-io-enable
>>>
>>>    s" virtio-block.fs" included
>>>
>>> -0 VALUE queue-addr
>>> -
>>> -\ Allocate memory for virtio queue:
>>> -virtiodev 0 virtio-get-qsize virtio-vring-size
>>> -alloc-mem to queue-addr
>>> -
>>> -\ Write queue address into device:
>>> -virtiodev 0 queue-addr virtio-set-qaddr
>>> -
>>
>> This seems to belong to 04/27.
>
> 04/27 introduces a generic api, so this is the right place.

No it is not. I see no point in adding a function in one patch and start 
using it in another unless these patches need to got via different trees 
which is an usual situation in the kernel development but not in SLOF.



> There are
> other drivers as well using this.

Change them all at once to use virtio_queue_init_vq(). This is a mechanical 
change, why to spread it?


>
> I am using the api here and earlier driver never called
> virtio_reset_device, because the init was done in the above code. Now
> with the api, I do virt queue init in virtio_blk_init of the driver
> after the device reset.

This execution flow change is another change, it has nothing to do with 
introduction of virtio_queue_init_vq().



>
>>
>>
>>>    pci-device-disable
>>> diff --git a/lib/libvirtio/virtio-blk.c b/lib/libvirtio/virtio-blk.c
>>> index b8c4da2..2d8982d 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;
>>> +
>>>    /**
>>>     * Initialize virtio-block device.
>>>     * @param  dev  pointer to virtio device information
>>> @@ -30,10 +32,7 @@ virtioblk_init(struct virtio_device *dev)
>>>    	int features;
>>>
>>>    	/* Reset device */
>>> -	// XXX That will clear the virtq base. We need to move
>>> -	//     initializing it to here anyway
>>> -	//
>>> -	//	 virtio_reset_device(dev);
>>> +	virtio_reset_device(dev);
>>>
>>>    	/* Acknowledge device. */
>>>    	virtio_set_status(dev, VIRTIO_STAT_ACKNOWLEDGE);
>>> @@ -44,6 +43,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;
>>> +	}
>>> +
>>
>> This is confusing chunk - you call a helper to init a queue now but I
>> assume it used to work already - how did it work? That removed chunk of
>> forth code above did the job? So this initialization has moved? It did not
>> set status before and now it does? Commit log would not hurt anyway.
>
> I guess i need to detail more in the log ?

This is a general comment - yes, you need to put lot more details in the 
commit log, especially when you split patches the way you did in this 
patchset :)


>
> "Move initialization of virt queue to the init and with this reset will start working now."


"reset" is not working without this patch?


>
>>
>>>    	vq_avail = virtio_get_vring_avail(dev, 0);
>>>    	vq_avail->flags = VRING_AVAIL_F_NO_INTERRUPT;
>>>    	vq_avail->idx = 0;
>>>
>>
>>
>> --
>> Alexey
>


-- 
Alexey