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

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

[Paolo Bonzini]

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

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

[Michael S. Tsirkin]

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

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

[Paolo Bonzini]

On 11/24/2011 05:42 PM, Michael S. Tsirkin wrote:
> 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.

Could some backend make it a hard failure?  If I understand correctly, 
this would have prevented the BAD_FEATURE bug too.

Paolo

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

[Michael S. Tsirkin]

On Thu, Nov 24, 2011 at 05:49:23PM +0100, Paolo Bonzini wrote:
> On 11/24/2011 05:42 PM, Michael S. Tsirkin wrote:
> >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.
> 
> Could some backend make it a hard failure?

I don't see how, there's no way to report a failure from
an io port write.

> If I understand
> correctly, this would have prevented the BAD_FEATURE bug too.
> 
> Paolo

Which bug? what would have prevented it.

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

[Paolo Bonzini]

On 11/24/2011 06:55 PM, Michael S. Tsirkin wrote:
> >  Could some backend make it a hard failure?
>
> I don't see how, there's no way to report a failure from
> an io port write.

You can exit(1), or fall back to a restricted set of features like we do 
for BAD_FEATURE.  BAD_FEATURE is a special case of features that is not 
exposed by the host, but requested by the guest.

> >  If I understand
> >  correctly, this would have prevented the BAD_FEATURE bug too.
>
> Which bug?

VIRTIO_F_BAD_FEATURE(30)
This feature should never be negotiated by the guest; doing so is an 
indication that the guest is faulty.  An experimental virtio PCI driver 
contained in Linux version 2.6.25 had this problem, and this feature bit 
can be used to detect it.

>  what would have prevented it.

exit(1) on unsupported features.

Paolo

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

[Michael S. Tsirkin]

On Fri, Nov 25, 2011 at 09:23:42AM +0100, Paolo Bonzini wrote:
> On 11/24/2011 06:55 PM, Michael S. Tsirkin wrote:
> >>  Could some backend make it a hard failure?
> >
> >I don't see how, there's no way to report a failure from
> >an io port write.
> 
> You can exit(1), or fall back to a restricted set of features like
> we do for BAD_FEATURE.  BAD_FEATURE is a special case of features
> that is not exposed by the host, but requested by the guest.

I donn't think we want to exit on BAD_FEATURE...

> >>  If I understand
> >>  correctly, this would have prevented the BAD_FEATURE bug too.
> >
> >Which bug?
> 
> VIRTIO_F_BAD_FEATURE(30)
> This feature should never be negotiated by the guest; doing so is an
> indication that the guest is faulty.  An experimental virtio PCI
> driver contained in Linux version 2.6.25 had this problem, and this
> feature bit can be used to detect it.

Ah, I understand, you mean helping debugging.
OK, but if we add exit() in the future we still do
not need to return a status from this function.

> > what would have prevented it.
> 
> exit(1) on unsupported features.
> 
> Paolo

This is somewhat problematic, for example this does not flush
out the cache and so might cause data corruption.
Yet I think it would be nice to have a 'safe_exit' functionality
to invoke on a guest bug. Or maybe it's better to stop the VM,
this way the monitor is available and debugging is easier.


-- 
MST

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

[Anthony Liguori]

On 11/25/2011 02:23 AM, Paolo Bonzini wrote:
> On 11/24/2011 06:55 PM, Michael S. Tsirkin wrote:
>> > Could some backend make it a hard failure?
>>
>> I don't see how, there's no way to report a failure from
>> an io port write.
>
> You can exit(1), or fall back to a restricted set of features like we do for
> BAD_FEATURE. BAD_FEATURE is a special case of features that is not exposed by
> the host, but requested by the guest.
>
>> > If I understand
>> > correctly, this would have prevented the BAD_FEATURE bug too.
>>
>> Which bug?
>
> VIRTIO_F_BAD_FEATURE(30)
> This feature should never be negotiated by the guest; doing so is an indication
> that the guest is faulty. An experimental virtio PCI driver contained in Linux
> version 2.6.25 had this problem, and this feature bit can be used to detect it.
>
>> what would have prevented it.
>
> exit(1) on unsupported features.

Applied.  Thanks.

Regards,

Anthony Liguori

>
> Paolo
>
>