[SLOF] [PATCH v2 15/19] virtio: 1.0 guest features negotiation

[Thomas Huth]

On 20.01.2016 13:10, Nikunj A Dadhania wrote:
> With virtio 1.0, there feature negotiation step needs to be completed
> before starting to notify the device.
> 
> This includes following steps:
> * Read host supported features
> * Check if virtio 1.0 is supported
> * Set guest supported features
> * Read host features and compare with the guest features.
> * Write FEATURES_OK and check it back to confirm.
> 
> Add virtio_get_status supporting routine.
> 
> Signed-off-by: Nikunj A Dadhania <nikunj at linux.vnet.ibm.com>
> ---
>  lib/libvirtio/virtio.c | 47 ++++++++++++++++++++++++++++++++++++++++++++++-
>  lib/libvirtio/virtio.h |  4 +++-
>  2 files changed, 49 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/libvirtio/virtio.c b/lib/libvirtio/virtio.c
> index b88aa61..0b939fc 100644
> --- a/lib/libvirtio/virtio.c
> +++ b/lib/libvirtio/virtio.c
> @@ -299,7 +299,7 @@ int virtio_queue_init_vq(struct virtio_device *dev, struct vqs *vq, unsigned int
>  /**
>   * Set device status bits
>   */
> -void virtio_set_status(struct virtio_device *dev, int status)
> +void virtio_set_status(struct virtio_device *dev, uint32_t status)

Why do you force an uint32_t here? That sounds like status would be a
32-bit type ... but in the PCI space, this is only a single byte...
so using "uint8_t" or keeping "int" would be better, I think.

>  {
>  	if (dev->type != VIRTIO_TYPE_PCI)
>  		return;
> @@ -311,6 +311,20 @@ void virtio_set_status(struct virtio_device *dev, int status)
>  	}
>  }
>  
> +/**
> + * Get device status bits
> + */
> +void virtio_get_status(struct virtio_device *dev, uint32_t *status)
> +{
> +	if (dev->type != VIRTIO_TYPE_PCI)
> +		return;
> +	if (dev->is_modern) {
> +		*status = ci_read_8(dev->common.addr +
> +				    offset_of(struct virtio_dev_common, dev_status));
> +	} else {
> +		*status = ci_read_8(dev->base+VIRTIOHDR_DEVICE_STATUS);
> +	}
> +}
>  
>  /**
>   * Set guest feature bits
> @@ -380,6 +394,37 @@ void virtio_get_host_features_long(struct virtio_device *dev, uint64_t *features
>  	}
>  }
>  
> +int virtio_negotiate_guest_features(struct virtio_device *dev, uint64_t features)
> +{
> +	uint64_t host_features = 0;
> +	uint32_t status;
> +
> +	/* Negotiate features */
> +	virtio_get_host_features_long(dev, &host_features);
> +	if (!(host_features & VIRTIO_F_VERSION_1)) {
> +		fprintf(stderr, "Device does not support virtio 1.0 %llx\n", host_features);
> +		return false;
> +	}
> +
> +	virtio_set_guest_features_long(dev,  features);
> +	virtio_get_host_features_long(dev, &host_features);
> +	if ((host_features & features) != features) {
> +		fprintf(stderr, "Features error %llx\n", features);
> +		return false;
> +	}
> +
> +	virtio_get_status(dev, &status);
> +	status |= VIRTIO_STAT_FEATURES_OK;
> +	virtio_set_status(dev, status);
> +
> +	/* Read back to verify the FEATURES_OK bit */
> +	virtio_get_status(dev, &status);
> +	if ((status & VIRTIO_STAT_FEATURES_OK) != VIRTIO_STAT_FEATURES_OK)
> +		return false;
> +
> +	return true;
> +}

May I suggest to either change the return type into "bool" instead of
"int", or to return 0 for success and a negative error code for failures?

 Thomas