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

[Thomas Huth]

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) {
> +		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???

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

> +	}
>  
>  	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.

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

>  	return blk_size;
> +
>  dev_error:
>  	fprintf(stderr, "Device Error \n");
>  	status |= VIRTIO_STAT_FAILED;
...

 Thomas