[SLOF] [PATCH v2 17/19] virtio-blk: enable virtio 1.0

[Nikunj A Dadhania]

Thomas Huth <thuth at redhat.com> writes:

> On 20.01.2016 13:10, Nikunj A Dadhania wrote:
>> Signed-off-by: Nikunj A Dadhania <nikunj at linux.vnet.ibm.com>
>> ---
>>  lib/libvirtio/virtio-blk.c | 77 +++++++++++++++++++++++++++++++---------------
>>  1 file changed, 53 insertions(+), 24 deletions(-)
>> 
>> diff --git a/lib/libvirtio/virtio-blk.c b/lib/libvirtio/virtio-blk.c
>> index 9587c4d..6b19181 100644
>> --- a/lib/libvirtio/virtio-blk.c
>> +++ b/lib/libvirtio/virtio-blk.c
>> @@ -18,6 +18,7 @@
>>  #include "virtio-blk.h"
>>  
>>  #define DEFAULT_SECTOR_SIZE 512
>> +#define DRIVER_FEATURE_SUPPORT  (VIRTIO_BLK_F_BLK_SIZE | VIRTIO_F_VERSION_1)
>>  
>>  struct vqs vq;
>>  
>> @@ -33,9 +34,6 @@ virtioblk_init(struct virtio_device *dev)
>>  	int features;
>
> So "features" is apparently a 32-bit variable ...
>
>>  	uint32_t status = VIRTIO_STAT_ACKNOWLEDGE;
>>  
>> -	/* Keep it disabled until the driver is 1.0 capable */
>> -	dev->is_modern = false;
>> -
>>  	/* Reset device */
>>  	virtio_reset_device(dev);
>>  
>> @@ -46,28 +44,45 @@ virtioblk_init(struct virtio_device *dev)
>>  	status |= VIRTIO_STAT_DRIVER;
>>  	virtio_set_status(dev, status);
>>  
>> -	/* Device specific setup - we support F_BLK_SIZE */
>> -	virtio_set_guest_features(dev,  VIRTIO_BLK_F_BLK_SIZE);
>> +	if (dev->is_modern) {

Here, playing around here with scope:

>> +		uint64_t features;

>> +		/* Negotiate features and sets FEATURES_OK if successful */
>> +		if(!virtio_negotiate_guest_features(dev, DRIVER_FEATURE_SUPPORT))
>> +			goto dev_error;
>> +
>> +		virtio_get_status(dev, &status);
>> +		virtio_get_host_features_long(dev, &features);
>
> ... but the second parameter of virtio_get_host_features_long() should
> be an "uint64_t *" ... don't you get compiler warnings here???

So it would not warn.


>> +		if (features & VIRTIO_BLK_F_BLK_SIZE) {
>> +			blk_size = virtio_get_config(dev,
>> +						     offset_of(struct virtio_blk_cfg, blk_size),
>> +						     sizeof(blk_size));
>> +		}
>
> The if-statement above ...
>
>> +	} else {
>> +		/* Device specific setup - we support F_BLK_SIZE */
>> +		virtio_set_guest_features(dev,  VIRTIO_BLK_F_BLK_SIZE);
>> +
>> +		virtio_get_host_features(dev, &features);
>> +		if (features & VIRTIO_BLK_F_BLK_SIZE) {
>> +			blk_size = virtio_get_config(dev,
>> +						     offset_of(struct virtio_blk_cfg, blk_size),
>> +						     sizeof(blk_size));
>> +		}
>
> ... is the same as the if-statement here, so you could merge them by
> moving them after the "if (dev->is_modern) {...} else {...}"
> statement...

No, they are dealing with different features one 32-bit other one
64-bit. I can declare a separate variable if its causing consfusion.

>
>> +	}
>>  
>>  	if(!virtio_queue_init_vq(dev, &vq, 0))
>>  		goto dev_error;
>>  
>>  	vq_avail = virtio_get_vring_avail(dev, 0);
>> -	vq_avail->flags = VRING_AVAIL_F_NO_INTERRUPT;
>> +	vq_avail->flags = dev->is_modern ? cpu_to_le16(VRING_AVAIL_F_NO_INTERRUPT) :
>> +		VRING_AVAIL_F_NO_INTERRUPT;
>>  	vq_avail->idx = 0;
>>  
>>  	/* Tell HV that setup succeeded */
>>  	status |= VIRTIO_STAT_DRIVER_OK;
>>  	virtio_set_status(dev, status);
>>  
>> -	virtio_get_host_features(dev, &features);
>> -	if (features & VIRTIO_BLK_F_BLK_SIZE) {
>> -		blk_size = virtio_get_config(dev,
>> -				offset_of(struct virtio_blk_cfg, blk_size),
>> -				sizeof(blk_size));
>> -	}
>
> ... or keep that common code here.

As explained above, features is not the same.

> Hmm, maybe it would be better to only have one
> virtio_get_host_features() function that could handle both cases and
> simply returns the feature value instead of passing it via a pointer?
> For the is_modern case, it would return the whole 64 bits, and for the
> legacy case, only 32 bits (with the upper 32-bits cleared).

I did not think about that. Did not want to break the legacy case. Let
me think and experiment.

Regards
Nikunj