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

[Thomas Huth]

On 21.01.2016 09:36, Nikunj A Dadhania wrote:
> 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;

Ah, variable shadowing ... I missed that line, sorry!

>>> +		/* 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.

Yes, please move the "int features" to this else-branch to avoid
confusion. Thanks!

 Thomas