Re: [Qemu-devel] [PATCH 1.0] virtio: add and use virtio_set_features

["Michael S. Tsirkin" <mst@redhat.com>]

On Thu, Nov 24, 2011 at 01:28:52PM +0100, Paolo Bonzini wrote:
> vdev->guest_features is not masking features that are not supported by
> the guest.  Fix this by introducing a common wrapper to be used by all
> virtio bus implementations.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Yes, while I can't point to a specific problem
I think it's a very good idea to avoid invalid guest
feature values.

And removing code duplication is good too.

A minor comment below

> ---
>  hw/s390-virtio-bus.c |    5 +----
>  hw/syborg_virtio.c   |    4 +---
>  hw/virtio-pci.c      |    9 ++-------
>  hw/virtio.c          |   24 ++++++++++++++++++------
>  hw/virtio.h          |    1 +
>  5 files changed, 23 insertions(+), 20 deletions(-)
> 
> diff --git a/hw/s390-virtio-bus.c b/hw/s390-virtio-bus.c
> index 0ce6406..c4b9a99 100644
> --- a/hw/s390-virtio-bus.c
> +++ b/hw/s390-virtio-bus.c
> @@ -254,10 +254,7 @@ void s390_virtio_device_update_status(VirtIOS390Device *dev)
>      /* Update guest supported feature bitmap */
>  
>      features = bswap32(ldl_be_phys(dev->feat_offs));
> -    if (vdev->set_features) {
> -        vdev->set_features(vdev, features);
> -    }
> -    vdev->guest_features = features;
> +    virtio_set_features(vdev, features);
>  }
>  
>  VirtIOS390Device *s390_virtio_bus_console(VirtIOS390Bus *bus)
> diff --git a/hw/syborg_virtio.c b/hw/syborg_virtio.c
> index 00c7be8..6de952c 100644
> --- a/hw/syborg_virtio.c
> +++ b/hw/syborg_virtio.c
> @@ -131,9 +131,7 @@ static void syborg_virtio_writel(void *opaque, target_phys_addr_t offset,
>      }
>      switch (offset >> 2) {
>      case SYBORG_VIRTIO_GUEST_FEATURES:
> -        if (vdev->set_features)
> -            vdev->set_features(vdev, value);
> -        vdev->guest_features = value;
> +        virtio_set_features(vdev, value);
>          break;
>      case SYBORG_VIRTIO_QUEUE_BASE:
>          if (value == 0)
> diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
> index ca5923c..64c6a94 100644
> --- a/hw/virtio-pci.c
> +++ b/hw/virtio-pci.c
> @@ -285,14 +285,9 @@ static void virtio_ioport_write(void *opaque, uint32_t addr, uint32_t val)
>      case VIRTIO_PCI_GUEST_FEATURES:
>  	/* Guest does not negotiate properly?  We have to assume nothing. */
>  	if (val & (1 << VIRTIO_F_BAD_FEATURE)) {
> -	    if (vdev->bad_features)
> -		val = proxy->host_features & vdev->bad_features(vdev);
> -	    else
> -		val = 0;
> +            val = vdev->bad_features ? vdev->bad_features(vdev) : 0;
>  	}
> -        if (vdev->set_features)
> -            vdev->set_features(vdev, val);
> -        vdev->guest_features = val;
> +        virtio_set_features(vdev, val);
>          break;
>      case VIRTIO_PCI_QUEUE_PFN:
>          pa = (target_phys_addr_t)val << VIRTIO_PCI_QUEUE_ADDR_SHIFT;
> diff --git a/hw/virtio.c b/hw/virtio.c
> index 7011b5b..81ecc40 100644
> --- a/hw/virtio.c
> +++ b/hw/virtio.c
> @@ -763,12 +763,25 @@ void virtio_save(VirtIODevice *vdev, QEMUFile *f)
>      }
>  }
>  
> +int virtio_set_features(VirtIODevice *vdev, uint32_t val)
> +{
> +    uint32_t supported_features =
> +        vdev->binding->get_features(vdev->binding_opaque);
> +    bool bad = (val & ~supported_features) != 0;
> +
> +    val &= supported_features;
> +    if (vdev->set_features) {
> +        vdev->set_features(vdev, val);
> +    }
> +    vdev->guest_features = val;
> +    return bad ? -1 : 0;
> +}
> +

Most users ignore the return value anyway,
and virtio_load needs to look at supported features
mask for the diagnostic anyway. So let's use virtio_set_features
void.

As a nice side effect, virtio_load can do it's checks
before anything is set, only set features if it's ok.

>  int virtio_load(VirtIODevice *vdev, QEMUFile *f)
>  {
>      int num, i, ret;
>      uint32_t features;
> -    uint32_t supported_features =
> -        vdev->binding->get_features(vdev->binding_opaque);
> +    uint32_t supported_features;
>  
>      if (vdev->binding->load_config) {
>          ret = vdev->binding->load_config(vdev->binding_opaque, f);
> @@ -780,14 +793,13 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f)
>      qemu_get_8s(f, &vdev->isr);
>      qemu_get_be16s(f, &vdev->queue_sel);
>      qemu_get_be32s(f, &features);
> -    if (features & ~supported_features) {
> +
> +    if (virtio_set_features(vdev, features) < 0) {
> +        supported_features = vdev->binding->get_features(vdev->binding_opaque);
>          error_report("Features 0x%x unsupported. Allowed features: 0x%x",
>                       features, supported_features);
>          return -1;
>      }
> -    if (vdev->set_features)
> -        vdev->set_features(vdev, features);
> -    vdev->guest_features = features;
>      vdev->config_len = qemu_get_be32(f);
>      qemu_get_buffer(f, vdev->config, vdev->config_len);
>  
> diff --git a/hw/virtio.h b/hw/virtio.h
> index 2d18209..25f5564 100644
> --- a/hw/virtio.h
> +++ b/hw/virtio.h
> @@ -185,6 +185,7 @@ void virtio_queue_set_vector(VirtIODevice *vdev, int n, uint16_t vector);
>  void virtio_set_status(VirtIODevice *vdev, uint8_t val);
>  void virtio_reset(void *opaque);
>  void virtio_update_irq(VirtIODevice *vdev);
> +int virtio_set_features(VirtIODevice *vdev, uint32_t val);
>  
>  void virtio_bind_device(VirtIODevice *vdev, const VirtIOBindings *binding,
>                          void *opaque);
> -- 
> 1.7.7.1