[SLOF] [PATCH v3 20/22] virtio-blk: enable virtio 1.0

[Thomas Huth]

On 22.01.2016 11:55, Nikunj A Dadhania wrote:
> Signed-off-by: Nikunj A Dadhania <nikunj at linux.vnet.ibm.com>
> ---
>  lib/libvirtio/virtio-blk.c | 65 ++++++++++++++++++++++++++++++----------------
>  1 file changed, 42 insertions(+), 23 deletions(-)
> 
> diff --git a/lib/libvirtio/virtio-blk.c b/lib/libvirtio/virtio-blk.c
> index 83c3244..1ab5dcd 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)
>  
>  static struct vqs vq;
>  
> @@ -33,9 +34,6 @@ virtioblk_init(struct virtio_device *dev)
>  	int features;
>  	int 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,44 @@ 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);
> +		if (features & VIRTIO_BLK_F_BLK_SIZE) {
> +			blk_size = virtio_get_config(dev,
> +						     offset_of(struct virtio_blk_cfg, blk_size),
> +						     sizeof(blk_size));
> +		}
> +	} else {

Could you please move the "int features" here, to avoid the shadowing in
the other part of the if-statement?

> +		/* 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));
> +		}
> +	}

Also ... did you consider merging the two virtio_get_host_features...()
functions as discussed in the v2 mail thread? ... I think that really
could simplify the code here quite a bit.

 Thomas