[Qemu-devel] [PATCH 0/2] virtio-net: spec compatibility fix

[Qemu-devel] [PATCH 0/2] virtio-net: spec compatibility fix

[Michael S. Tsirkin]

Virtio 1.0 spec lists VIRTIO_NET_F_GSO as a legacy-only flag,
by mistake we exposed it on the modern interface too, this
is a spec violation.

I decided it's not worth it to implement a compatibility bit here as we already
shipped virtio 1.0 support, we want to fix compatibility for old machine types
too.  While changing feature bits under guest's feet might be surprising, the
spec doesn't exactly say it's illegal.

Michael S. Tsirkin (2):
  virtio: allow per-device-class legacy features
  virtio-net: mark VIRTIO_NET_F_GSO as legacy

 include/hw/virtio/virtio.h | 5 +++++
 hw/net/virtio-net.c        | 1 +
 hw/s390x/virtio-ccw.c      | 4 +++-
 hw/virtio/virtio-pci.c     | 4 +++-
 hw/virtio/virtio.c         | 2 ++
 5 files changed, 14 insertions(+), 2 deletions(-)

-- 
MST

[Qemu-devel] [PATCH 1/2] virtio: allow per-device-class legacy features

[Michael S. Tsirkin]

Legacy features are those that transitional devices only
expose on the legacy interface.
Allow different ones per device class.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 include/hw/virtio/virtio.h | 5 +++++
 hw/s390x/virtio-ccw.c      | 4 +++-
 hw/virtio/virtio-pci.c     | 4 +++-
 hw/virtio/virtio.c         | 2 ++
 4 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index f12a1a8..bdb3c4b 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -113,6 +113,11 @@ typedef struct VirtioDeviceClass {
     void (*set_config)(VirtIODevice *vdev, const uint8_t *config);
     void (*reset)(VirtIODevice *vdev);
     void (*set_status)(VirtIODevice *vdev, uint8_t val);
+    /* For transitional devices, this is a bitmap of features
+     * that are only exposed on the legacy interface but not
+     * the modern one.
+     */
+    uint64_t legacy_features;
     /* Test and clear event pending status.
      * Should be called after unmask to avoid losing events.
      * If backend does not support masking,
diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
index 7d7f8f6..f5c1d98 100644
--- a/hw/s390x/virtio-ccw.c
+++ b/hw/s390x/virtio-ccw.c
@@ -303,6 +303,8 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
         if (!ccw.cda) {
             ret = -EFAULT;
         } else {
+            VirtioDeviceClass *vdc = VIRTIO_DEVICE_GET_CLASS(vdev);
+
             features.index = address_space_ldub(&address_space_memory,
                                                 ccw.cda
                                                 + sizeof(features.features),
@@ -312,7 +314,7 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
                 if (dev->revision >= 1) {
                     /* Don't offer legacy features for modern devices. */
                     features.features = (uint32_t)
-                        (vdev->host_features & ~VIRTIO_LEGACY_FEATURES);
+                        (vdev->host_features & ~vdc->legacy_features);
                 } else {
                     features.features = (uint32_t)vdev->host_features;
                 }
diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index 62001b4..97b32fe 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -1175,7 +1175,9 @@ static uint64_t virtio_pci_common_read(void *opaque, hwaddr addr,
         break;
     case VIRTIO_PCI_COMMON_DF:
         if (proxy->dfselect <= 1) {
-            val = (vdev->host_features & ~VIRTIO_LEGACY_FEATURES) >>
+            VirtioDeviceClass *vdc = VIRTIO_DEVICE_GET_CLASS(vdev);
+
+            val = (vdev->host_features & ~vdc->legacy_features) >>
                 (32 * proxy->dfselect);
         }
         break;
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 1df5f4e..72ee06b 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -2214,6 +2214,8 @@ static void virtio_device_class_init(ObjectClass *klass, void *data)
     dc->props = virtio_properties;
     vdc->start_ioeventfd = virtio_device_start_ioeventfd_impl;
     vdc->stop_ioeventfd = virtio_device_stop_ioeventfd_impl;
+
+    vdc->legacy_features |= VIRTIO_LEGACY_FEATURES;
 }
 
 bool virtio_device_ioeventfd_enabled(VirtIODevice *vdev)
-- 
MST

[Qemu-devel] [PATCH 2/2] virtio-net: mark VIRTIO_NET_F_GSO as legacy

[Michael S. Tsirkin]

virtio 1.0 spec says this is a legacy feature bit,
hide it from guests in legacy mode.

Note: for cross-version migration compatibility,
we keep the bit set in host_features.
The result will be that a guest migrating cross-version
will see host features change under it.
As guests only seem to read it once, this should
not be an issue. Meanwhile, will work to fix guests to
ignore this bit in virtio1 mode, too.

Cc: qemu-stable@nongnu.org
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/net/virtio-net.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 20aa63e..b68c69d 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -1942,6 +1942,7 @@ static void virtio_net_class_init(ObjectClass *klass, void *data)
     vdc->guest_notifier_pending = virtio_net_guest_notifier_pending;
     vdc->load = virtio_net_load_device;
     vdc->save = virtio_net_save_device;
+    vdc->legacy_features |= (0x1 << VIRTIO_NET_F_GSO);
 }
 
 static const TypeInfo virtio_net_info = {
-- 
MST

Re: [Qemu-devel] [PATCH 1/2] virtio: allow per-device-class legacy features

[Cornelia Huck]

On Fri, 4 Nov 2016 13:01:37 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> Legacy features are those that transitional devices only
> expose on the legacy interface.
> Allow different ones per device class.
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  include/hw/virtio/virtio.h | 5 +++++
>  hw/s390x/virtio-ccw.c      | 4 +++-
>  hw/virtio/virtio-pci.c     | 4 +++-
>  hw/virtio/virtio.c         | 2 ++
>  4 files changed, 13 insertions(+), 2 deletions(-)

Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com>

Re: [Qemu-devel] [PATCH 2/2] virtio-net: mark VIRTIO_NET_F_GSO as legacy

[Cornelia Huck]

On Fri, 4 Nov 2016 13:01:40 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> virtio 1.0 spec says this is a legacy feature bit,
> hide it from guests in legacy mode.
> 
> Note: for cross-version migration compatibility,
> we keep the bit set in host_features.
> The result will be that a guest migrating cross-version
> will see host features change under it.
> As guests only seem to read it once, this should
> not be an issue. Meanwhile, will work to fix guests to
> ignore this bit in virtio1 mode, too.

This also means that guest may see a different feature set if it is
e.g. rebooted after migration. But I agree that this should not be an
issue.

> 
> Cc: qemu-stable@nongnu.org

You also need to cc: the previous patch to stable.

> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  hw/net/virtio-net.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index 20aa63e..b68c69d 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -1942,6 +1942,7 @@ static void virtio_net_class_init(ObjectClass *klass, void *data)
>      vdc->guest_notifier_pending = virtio_net_guest_notifier_pending;
>      vdc->load = virtio_net_load_device;
>      vdc->save = virtio_net_save_device;
> +    vdc->legacy_features |= (0x1 << VIRTIO_NET_F_GSO);
>  }
> 
>  static const TypeInfo virtio_net_info = {

Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com>

Re: [Qemu-devel] [PATCH 1/2] virtio: allow per-device-class legacy features

[Jason Wang]

On 2016年11月04日 19:01, Michael S. Tsirkin wrote:
> Legacy features are those that transitional devices only
> expose on the legacy interface.
> Allow different ones per device class.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>   include/hw/virtio/virtio.h | 5 +++++
>   hw/s390x/virtio-ccw.c      | 4 +++-
>   hw/virtio/virtio-pci.c     | 4 +++-
>   hw/virtio/virtio.c         | 2 ++
>   4 files changed, 13 insertions(+), 2 deletions(-)
>
> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
> index f12a1a8..bdb3c4b 100644
> --- a/include/hw/virtio/virtio.h
> +++ b/include/hw/virtio/virtio.h
> @@ -113,6 +113,11 @@ typedef struct VirtioDeviceClass {
>       void (*set_config)(VirtIODevice *vdev, const uint8_t *config);
>       void (*reset)(VirtIODevice *vdev);
>       void (*set_status)(VirtIODevice *vdev, uint8_t val);
> +    /* For transitional devices, this is a bitmap of features
> +     * that are only exposed on the legacy interface but not
> +     * the modern one.
> +     */
> +    uint64_t legacy_features;
>       /* Test and clear event pending status.
>        * Should be called after unmask to avoid losing events.
>        * If backend does not support masking,
> diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
> index 7d7f8f6..f5c1d98 100644
> --- a/hw/s390x/virtio-ccw.c
> +++ b/hw/s390x/virtio-ccw.c
> @@ -303,6 +303,8 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
>           if (!ccw.cda) {
>               ret = -EFAULT;
>           } else {
> +            VirtioDeviceClass *vdc = VIRTIO_DEVICE_GET_CLASS(vdev);
> +
>               features.index = address_space_ldub(&address_space_memory,
>                                                   ccw.cda
>                                                   + sizeof(features.features),
> @@ -312,7 +314,7 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
>                   if (dev->revision >= 1) {
>                       /* Don't offer legacy features for modern devices. */
>                       features.features = (uint32_t)
> -                        (vdev->host_features & ~VIRTIO_LEGACY_FEATURES);
> +                        (vdev->host_features & ~vdc->legacy_features);
>                   } else {
>                       features.features = (uint32_t)vdev->host_features;
>                   }
> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> index 62001b4..97b32fe 100644
> --- a/hw/virtio/virtio-pci.c
> +++ b/hw/virtio/virtio-pci.c
> @@ -1175,7 +1175,9 @@ static uint64_t virtio_pci_common_read(void *opaque, hwaddr addr,
>           break;
>       case VIRTIO_PCI_COMMON_DF:
>           if (proxy->dfselect <= 1) {
> -            val = (vdev->host_features & ~VIRTIO_LEGACY_FEATURES) >>
> +            VirtioDeviceClass *vdc = VIRTIO_DEVICE_GET_CLASS(vdev);
> +
> +            val = (vdev->host_features & ~vdc->legacy_features) >>
>                   (32 * proxy->dfselect);
>           }
>           break;
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index 1df5f4e..72ee06b 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -2214,6 +2214,8 @@ static void virtio_device_class_init(ObjectClass *klass, void *data)
>       dc->props = virtio_properties;
>       vdc->start_ioeventfd = virtio_device_start_ioeventfd_impl;
>       vdc->stop_ioeventfd = virtio_device_stop_ioeventfd_impl;
> +
> +    vdc->legacy_features |= VIRTIO_LEGACY_FEATURES;
>   }
>   
>   bool virtio_device_ioeventfd_enabled(VirtIODevice *vdev)

Acked-by: Jason Wang <jasowang@redhat.com>

Re: [Qemu-devel] [PATCH 2/2] virtio-net: mark VIRTIO_NET_F_GSO as legacy

[Jason Wang]

On 2016年11月04日 19:01, Michael S. Tsirkin wrote:
> virtio 1.0 spec says this is a legacy feature bit,
> hide it from guests in legacy mode.
>
> Note: for cross-version migration compatibility,
> we keep the bit set in host_features.
> The result will be that a guest migrating cross-version
> will see host features change under it.
> As guests only seem to read it once, this should
> not be an issue. Meanwhile, will work to fix guests to
> ignore this bit in virtio1 mode, too.
>
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>   hw/net/virtio-net.c | 1 +
>   1 file changed, 1 insertion(+)
>
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index 20aa63e..b68c69d 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -1942,6 +1942,7 @@ static void virtio_net_class_init(ObjectClass *klass, void *data)
>       vdc->guest_notifier_pending = virtio_net_guest_notifier_pending;
>       vdc->load = virtio_net_load_device;
>       vdc->save = virtio_net_save_device;
> +    vdc->legacy_features |= (0x1 << VIRTIO_NET_F_GSO);
>   }
>   
>   static const TypeInfo virtio_net_info = {

Acked-by: Jason Wang <jasowang@redhat.com>

Re: [Qemu-devel] [PATCH 2/2] virtio-net: mark VIRTIO_NET_F_GSO as legacy

[Dr. David Alan Gilbert]

* Michael S. Tsirkin (mst@redhat.com) wrote:
> virtio 1.0 spec says this is a legacy feature bit,
> hide it from guests in legacy mode.
> 
> Note: for cross-version migration compatibility,
> we keep the bit set in host_features.
> The result will be that a guest migrating cross-version
> will see host features change under it.
> As guests only seem to read it once, this should
> not be an issue. Meanwhile, will work to fix guests to
> ignore this bit in virtio1 mode, too.

OK, but if they're actually using the feature they'll carry
on working ?

Dave

> Cc: qemu-stable@nongnu.org
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  hw/net/virtio-net.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index 20aa63e..b68c69d 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -1942,6 +1942,7 @@ static void virtio_net_class_init(ObjectClass *klass, void *data)
>      vdc->guest_notifier_pending = virtio_net_guest_notifier_pending;
>      vdc->load = virtio_net_load_device;
>      vdc->save = virtio_net_save_device;
> +    vdc->legacy_features |= (0x1 << VIRTIO_NET_F_GSO);
>  }
>  
>  static const TypeInfo virtio_net_info = {
> -- 
> MST
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH 2/2] virtio-net: mark VIRTIO_NET_F_GSO as legacy

[Halil Pasic]

On 11/04/2016 12:01 PM, Michael S. Tsirkin wrote:
> virtio 1.0 spec says this is a legacy feature bit,
> hide it from guests in legacy mode.
> 

Probably the other way around: since its legacy
it is supposed to be hidden from non-legacy )and
exposed to legacy) or?

> Note: for cross-version migration compatibility,
> we keep the bit set in host_features.
> The result will be that a guest migrating cross-version
> will see host features change under it.
> As guests only seem to read it once, this should
> not be an issue. Meanwhile, will work to fix guests to
> ignore this bit in virtio1 mode, too.
> 

Makes sense to me (except for the probably typo).

Regards,
Halil

> Cc: qemu-stable@nongnu.org
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  hw/net/virtio-net.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index 20aa63e..b68c69d 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -1942,6 +1942,7 @@ static void virtio_net_class_init(ObjectClass *klass, void *data)
>      vdc->guest_notifier_pending = virtio_net_guest_notifier_pending;
>      vdc->load = virtio_net_load_device;
>      vdc->save = virtio_net_save_device;
> +    vdc->legacy_features |= (0x1 << VIRTIO_NET_F_GSO);
>  }
> 
>  static const TypeInfo virtio_net_info = {
> 

Re: [Qemu-devel] [PATCH 2/2] virtio-net: mark VIRTIO_NET_F_GSO as legacy

[Halil Pasic]

On 11/07/2016 04:38 PM, Dr. David Alan Gilbert wrote:
> * Michael S. Tsirkin (mst@redhat.com) wrote:
>> > virtio 1.0 spec says this is a legacy feature bit,
>> > hide it from guests in legacy mode.
>> > 
>> > Note: for cross-version migration compatibility,
>> > we keep the bit set in host_features.
>> > The result will be that a guest migrating cross-version
>> > will see host features change under it.
>> > As guests only seem to read it once, this should
>> > not be an issue. Meanwhile, will work to fix guests to
>> > ignore this bit in virtio1 mode, too.
> OK, but if they're actually using the feature they'll carry
> on working ?
> 
> Dave
> 

The interesting scenario is when we migrate form older to newer
(otherwise the bugous situation can't emerge in the first place). Now
the change affects only what the guest sees when it reads host_features,
and that should happen only during the feature negotiation (once). That
means the change won't have any observable effect unless a reset
happens. And if we have a reset, then things are going to work out
correctly avoiding the bugous (but still working) state in both host and
guest. So yes, both will happily carry on using the feature, and carry
on working.

Halil