[PATCH 0/3] virtio-net: graceful drop of vhost for TAP

[PATCH 0/3] virtio-net: graceful drop of vhost for TAP

[Yuri Benditovich]

This set of patches introduces graceful switch from tap-vhost to
tap-no-vhost depending on guest features. Before that the features
that vhost does not support were silently cleared in get_features.
This creates potential problem of migration from the machine where
some of virtio-net features are supported by the vhost kernel to the
machine where they are not supported (packed ring as an example).

Instead of silent masking of the features virtio-net gracefully
disables the vhost at set_features if some features acked by the
guest contradict with kernel vhost capabilities.

This set of patches also makes get_vhost_net() call (that used
everywhere) to always return actual result, i.e. initially it
returns non-NULL value and from the moment the vhost was disabled
the call will return NULL. Such a way we avoid any unexpected
calls to vhost functions.
Yuri Benditovich (3):
  vhost-net: add VIRTIO_NET_F_HASH_REPORT to the list of kernel features
  net: add ability to hide (disable) vhost_net
  virtio-net: graceful fallback to vhost=off for tap netdev

 hw/net/vhost_net.c  |  5 +++-
 hw/net/virtio-net.c | 58 ++++++++++++++++++++++++++++++++++++++-------
 include/net/net.h   |  1 +
 3 files changed, 55 insertions(+), 9 deletions(-)

-- 
2.17.1

[PATCH 1/3] vhost-net: add VIRTIO_NET_F_HASH_REPORT to the list of kernel features

[Yuri Benditovich]

In case of vhost TAP the kernel must support this feature,
otherwise the device can't offer it.

Signed-off-by: Yuri Benditovich <yuri.benditovich@daynix.com>
---
 hw/net/vhost_net.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
index 24d555e764..8282e440bd 100644
--- a/hw/net/vhost_net.c
+++ b/hw/net/vhost_net.c
@@ -45,6 +45,7 @@ static const int kernel_feature_bits[] = {
     VIRTIO_NET_F_MTU,
     VIRTIO_F_IOMMU_PLATFORM,
     VIRTIO_F_RING_PACKED,
+    VIRTIO_NET_F_HASH_REPORT,
     VHOST_INVALID_FEATURE_BIT
 };
 
-- 
2.17.1

[PATCH 2/3] net: add ability to hide (disable) vhost_net

[Yuri Benditovich]

If 'vhost_net_disabled' in the NetClientState of the
net device, get_vhost_net for TAP returns NULL. Network adapters
can use this ability to hide the vhost_net temporary between
resets in case some active features contradict with vhost.

Signed-off-by: Yuri Benditovich <yuri.benditovich@daynix.com>
---
 hw/net/vhost_net.c | 4 +++-
 include/net/net.h  | 1 +
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
index 8282e440bd..7873d27a36 100644
--- a/hw/net/vhost_net.c
+++ b/hw/net/vhost_net.c
@@ -437,7 +437,9 @@ VHostNetState *get_vhost_net(NetClientState *nc)
 
     switch (nc->info->type) {
     case NET_CLIENT_DRIVER_TAP:
-        vhost_net = tap_get_vhost_net(nc);
+        if (!nc->vhost_net_disabled) {
+            vhost_net = tap_get_vhost_net(nc);
+        }
         break;
 #ifdef CONFIG_VHOST_NET_USER
     case NET_CLIENT_DRIVER_VHOST_USER:
diff --git a/include/net/net.h b/include/net/net.h
index 919facaad2..4479bdcec0 100644
--- a/include/net/net.h
+++ b/include/net/net.h
@@ -100,6 +100,7 @@ struct NetClientState {
     int vring_enable;
     int vnet_hdr_len;
     bool is_netdev;
+    bool vhost_net_disabled;
     QTAILQ_HEAD(, NetFilterState) filters;
 };
 
-- 
2.17.1

[PATCH 3/3] virtio-net: graceful fallback to vhost=off for tap netdev

[Yuri Benditovich]

Currently virtio-net silently clears features if they are
not supported by respective vhost. This may create migration
problems in future if vhost features on the source and destination
are different. Implement graceful fallback to no-vhost mode
when some acked features contradict with vhost. The decision is
taken on set_features call and the vhost will be disabled
till next reset (or migration).
Such fallback is currently enabled only for TAP netdev.

Signed-off-by: Yuri Benditovich <yuri.benditovich@daynix.com>
---
 hw/net/virtio-net.c | 58 ++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 50 insertions(+), 8 deletions(-)

diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 5150f295e8..b353060e63 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -515,6 +515,15 @@ static RxFilterInfo *virtio_net_query_rxfilter(NetClientState *nc)
     return info;
 }
 
+static void virtio_net_allow_vhost(VirtIONet *n, bool allow)
+{
+    int i;
+    for (i = 0; i < n->max_queues; i++) {
+        NetClientState *nc = qemu_get_subqueue(n->nic, i)->peer;
+        nc->vhost_net_disabled = !allow;
+    }
+}
+
 static void virtio_net_reset(VirtIODevice *vdev)
 {
     VirtIONet *n = VIRTIO_NET(vdev);
@@ -552,6 +561,7 @@ static void virtio_net_reset(VirtIODevice *vdev)
             assert(!virtio_net_get_subqueue(nc)->async_tx.elem);
         }
     }
+    virtio_net_allow_vhost(n, true);
 }
 
 static void peer_test_vnet_hdr(VirtIONet *n)
@@ -689,6 +699,15 @@ static void virtio_net_set_queues(VirtIONet *n)
     }
 }
 
+static bool can_disable_vhost(VirtIONet *n)
+{
+    NetClientState *peer = qemu_get_queue(n->nic)->peer;
+    if (!get_vhost_net(peer)) {
+        return false;
+    }
+    return !peer || peer->info->type == NET_CLIENT_DRIVER_TAP;
+}
+
 static void virtio_net_set_multiqueue(VirtIONet *n, int multiqueue);
 
 static uint64_t virtio_net_get_features(VirtIODevice *vdev, uint64_t features,
@@ -725,14 +744,14 @@ static uint64_t virtio_net_get_features(VirtIODevice *vdev, uint64_t features,
         return features;
     }
 
-    virtio_clear_feature(&features, VIRTIO_NET_F_RSS);
-    virtio_clear_feature(&features, VIRTIO_NET_F_HASH_REPORT);
-    features = vhost_net_get_features(get_vhost_net(nc->peer), features);
-    vdev->backend_features = features;
+    vdev->backend_features = vhost_net_get_features(get_vhost_net(nc->peer), features);
 
-    if (n->mtu_bypass_backend &&
-            (n->host_features & 1ULL << VIRTIO_NET_F_MTU)) {
-        features |= (1ULL << VIRTIO_NET_F_MTU);
+    if (!can_disable_vhost(n)) {
+        features = vdev->backend_features;
+        if (n->mtu_bypass_backend &&
+                (n->host_features & 1ULL << VIRTIO_NET_F_MTU)) {
+            features |= (1ULL << VIRTIO_NET_F_MTU);
+        }
     }
 
     return features;
@@ -872,10 +891,25 @@ static void failover_add_primary(VirtIONet *n, Error **errp)
     error_propagate(errp, err);
 }
 
+static bool check_vhost_features(VirtIONet *n, uint64_t features)
+{
+    NetClientState *nc = qemu_get_queue(n->nic);
+    uint64_t filtered;
+    if (n->rss_data.redirect) {
+        return false;
+    }
+    filtered = vhost_net_get_features(get_vhost_net(nc->peer), features);
+    if (filtered != features) {
+        return false;
+    }
+    return true;
+}
+
 static void virtio_net_set_features(VirtIODevice *vdev, uint64_t features)
 {
     VirtIONet *n = VIRTIO_NET(vdev);
     Error *err = NULL;
+    bool disable_vhost = false;
     int i;
 
     if (n->mtu_bypass_backend &&
@@ -894,13 +928,21 @@ static void virtio_net_set_features(VirtIODevice *vdev, uint64_t features)
                                                   VIRTIO_F_VERSION_1),
                                virtio_has_feature(features,
                                                   VIRTIO_NET_F_HASH_REPORT));
-
     n->rsc4_enabled = virtio_has_feature(features, VIRTIO_NET_F_RSC_EXT) &&
         virtio_has_feature(features, VIRTIO_NET_F_GUEST_TSO4);
     n->rsc6_enabled = virtio_has_feature(features, VIRTIO_NET_F_RSC_EXT) &&
         virtio_has_feature(features, VIRTIO_NET_F_GUEST_TSO6);
     n->rss_data.redirect = virtio_has_feature(features, VIRTIO_NET_F_RSS);
 
+    if (can_disable_vhost(n)) {
+        disable_vhost = !check_vhost_features(n, features);
+    }
+    if (disable_vhost) {
+        warn_report("Some of requested features aren't supported by vhost, "
+                    "vhost is turned off till next reset");
+        virtio_net_allow_vhost(n, false);
+    }
+
     if (n->has_vnet_hdr) {
         n->curr_guest_offloads =
             virtio_net_guest_offloads_by_features(features);
-- 
2.17.1

Re: [PATCH 3/3] virtio-net: graceful fallback to vhost=off for tap netdev

[Michael S. Tsirkin]

On Thu, Feb 04, 2021 at 10:29:15PM +0200, Yuri Benditovich wrote:
> Currently virtio-net silently clears features if they are
> not supported by respective vhost. This may create migration
> problems in future if vhost features on the source and destination
> are different. Implement graceful fallback to no-vhost mode
> when some acked features contradict with vhost. The decision is
> taken on set_features call and the vhost will be disabled
> till next reset (or migration).
> Such fallback is currently enabled only for TAP netdev.
> 
> Signed-off-by: Yuri Benditovich <yuri.benditovich@daynix.com>


Sounds good, but I don't think we should do this if
vhostforce=on is set.

Also, let's document this behaviour with the vhost option so people
are not suprized.

> ---
>  hw/net/virtio-net.c | 58 ++++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 50 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index 5150f295e8..b353060e63 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -515,6 +515,15 @@ static RxFilterInfo *virtio_net_query_rxfilter(NetClientState *nc)
>      return info;
>  }
>  
> +static void virtio_net_allow_vhost(VirtIONet *n, bool allow)
> +{
> +    int i;
> +    for (i = 0; i < n->max_queues; i++) {
> +        NetClientState *nc = qemu_get_subqueue(n->nic, i)->peer;
> +        nc->vhost_net_disabled = !allow;
> +    }
> +}
> +
>  static void virtio_net_reset(VirtIODevice *vdev)
>  {
>      VirtIONet *n = VIRTIO_NET(vdev);
> @@ -552,6 +561,7 @@ static void virtio_net_reset(VirtIODevice *vdev)
>              assert(!virtio_net_get_subqueue(nc)->async_tx.elem);
>          }
>      }
> +    virtio_net_allow_vhost(n, true);
>  }
>  
>  static void peer_test_vnet_hdr(VirtIONet *n)
> @@ -689,6 +699,15 @@ static void virtio_net_set_queues(VirtIONet *n)
>      }
>  }
>  
> +static bool can_disable_vhost(VirtIONet *n)
> +{
> +    NetClientState *peer = qemu_get_queue(n->nic)->peer;
> +    if (!get_vhost_net(peer)) {
> +        return false;
> +    }
> +    return !peer || peer->info->type == NET_CLIENT_DRIVER_TAP;
> +}
> +
>  static void virtio_net_set_multiqueue(VirtIONet *n, int multiqueue);
>  
>  static uint64_t virtio_net_get_features(VirtIODevice *vdev, uint64_t features,
> @@ -725,14 +744,14 @@ static uint64_t virtio_net_get_features(VirtIODevice *vdev, uint64_t features,
>          return features;
>      }
>  
> -    virtio_clear_feature(&features, VIRTIO_NET_F_RSS);
> -    virtio_clear_feature(&features, VIRTIO_NET_F_HASH_REPORT);
> -    features = vhost_net_get_features(get_vhost_net(nc->peer), features);
> -    vdev->backend_features = features;
> +    vdev->backend_features = vhost_net_get_features(get_vhost_net(nc->peer), features);
>  
> -    if (n->mtu_bypass_backend &&
> -            (n->host_features & 1ULL << VIRTIO_NET_F_MTU)) {
> -        features |= (1ULL << VIRTIO_NET_F_MTU);
> +    if (!can_disable_vhost(n)) {
> +        features = vdev->backend_features;
> +        if (n->mtu_bypass_backend &&
> +                (n->host_features & 1ULL << VIRTIO_NET_F_MTU)) {
> +            features |= (1ULL << VIRTIO_NET_F_MTU);
> +        }
>      }
>  
>      return features;
> @@ -872,10 +891,25 @@ static void failover_add_primary(VirtIONet *n, Error **errp)
>      error_propagate(errp, err);
>  }
>  
> +static bool check_vhost_features(VirtIONet *n, uint64_t features)
> +{
> +    NetClientState *nc = qemu_get_queue(n->nic);
> +    uint64_t filtered;
> +    if (n->rss_data.redirect) {
> +        return false;
> +    }
> +    filtered = vhost_net_get_features(get_vhost_net(nc->peer), features);
> +    if (filtered != features) {
> +        return false;
> +    }
> +    return true;
> +}
> +
>  static void virtio_net_set_features(VirtIODevice *vdev, uint64_t features)
>  {
>      VirtIONet *n = VIRTIO_NET(vdev);
>      Error *err = NULL;
> +    bool disable_vhost = false;
>      int i;
>  
>      if (n->mtu_bypass_backend &&
> @@ -894,13 +928,21 @@ static void virtio_net_set_features(VirtIODevice *vdev, uint64_t features)
>                                                    VIRTIO_F_VERSION_1),
>                                 virtio_has_feature(features,
>                                                    VIRTIO_NET_F_HASH_REPORT));
> -
>      n->rsc4_enabled = virtio_has_feature(features, VIRTIO_NET_F_RSC_EXT) &&
>          virtio_has_feature(features, VIRTIO_NET_F_GUEST_TSO4);
>      n->rsc6_enabled = virtio_has_feature(features, VIRTIO_NET_F_RSC_EXT) &&
>          virtio_has_feature(features, VIRTIO_NET_F_GUEST_TSO6);
>      n->rss_data.redirect = virtio_has_feature(features, VIRTIO_NET_F_RSS);
>  
> +    if (can_disable_vhost(n)) {
> +        disable_vhost = !check_vhost_features(n, features);
> +    }
> +    if (disable_vhost) {
> +        warn_report("Some of requested features aren't supported by vhost, "
> +                    "vhost is turned off till next reset");
> +        virtio_net_allow_vhost(n, false);
> +    }
> +
>      if (n->has_vnet_hdr) {
>          n->curr_guest_offloads =
>              virtio_net_guest_offloads_by_features(features);
> -- 
> 2.17.1

Re: [PATCH 3/3] virtio-net: graceful fallback to vhost=off for tap netdev

[Michael S. Tsirkin]

On Fri, Feb 05, 2021 at 08:38:49AM -0500, Michael S. Tsirkin wrote:
> On Thu, Feb 04, 2021 at 10:29:15PM +0200, Yuri Benditovich wrote:
> > Currently virtio-net silently clears features if they are
> > not supported by respective vhost. This may create migration
> > problems in future if vhost features on the source and destination
> > are different. Implement graceful fallback to no-vhost mode
> > when some acked features contradict with vhost. The decision is
> > taken on set_features call and the vhost will be disabled
> > till next reset (or migration).
> > Such fallback is currently enabled only for TAP netdev.
> > 
> > Signed-off-by: Yuri Benditovich <yuri.benditovich@daynix.com>
> 
> 
> Sounds good, but I don't think we should do this if
> vhostforce=on is set.
> 
> Also, let's document this behaviour with the vhost option so people
> are not suprized.

Here's another thing that bothers me.

At the moment we easily add new features, enabled by default,
as long as kernels are consistent on source and destination
everything works fine.

With this patch first time we add a new feature that kernel
does not support, vhost gets disabled. Given lots of people
update their kernels less frequently than userspace,
lots of users will start running with vhost off all of a sudden.

Don't have good suggestions yet.

-- 
MST

Re: [PATCH 3/3] virtio-net: graceful fallback to vhost=off for tap netdev

[Jason Wang]

On 2021/2/5 下午9:38, Michael S. Tsirkin wrote:
> On Thu, Feb 04, 2021 at 10:29:15PM +0200, Yuri Benditovich wrote:
>> Currently virtio-net silently clears features if they are
>> not supported by respective vhost. This may create migration
>> problems in future if vhost features on the source and destination
>> are different. Implement graceful fallback to no-vhost mode
>> when some acked features contradict with vhost. The decision is
>> taken on set_features call and the vhost will be disabled
>> till next reset (or migration).
>> Such fallback is currently enabled only for TAP netdev.
>>
>> Signed-off-by: Yuri Benditovich<yuri.benditovich@daynix.com>
> Sounds good, but I don't think we should do this if
> vhostforce=on is set.


If we do this, does it mean we won't maintain migration compatibility 
when vhostforce is on?

Thanks


>
> Also, let's document this behaviour with the vhost option so people
> are not suprized.
>

Re: [PATCH 3/3] virtio-net: graceful fallback to vhost=off for tap netdev

[Yuri Benditovich]

On Mon, Feb 8, 2021 at 5:15 AM Jason Wang <jasowang@redhat.com> wrote:
>
>
> On 2021/2/5 下午9:38, Michael S. Tsirkin wrote:
> > On Thu, Feb 04, 2021 at 10:29:15PM +0200, Yuri Benditovich wrote:
> >> Currently virtio-net silently clears features if they are
> >> not supported by respective vhost. This may create migration
> >> problems in future if vhost features on the source and destination
> >> are different. Implement graceful fallback to no-vhost mode
> >> when some acked features contradict with vhost. The decision is
> >> taken on set_features call and the vhost will be disabled
> >> till next reset (or migration).
> >> Such fallback is currently enabled only for TAP netdev.
> >>
> >> Signed-off-by: Yuri Benditovich<yuri.benditovich@daynix.com>
> > Sounds good, but I don't think we should do this if
> > vhostforce=on is set.
>
>
> If we do this, does it mean we won't maintain migration compatibility
> when vhostforce is on?

AFAIU, the 'vhostforce=on' should mean the vhost can't be disabled (if
I'm not mistaken this is typically used for vhost-user).
So we can view this case as similar to vhost-vdpa and vhost-user.

>
> Thanks
>
>
> >
> > Also, let's document this behaviour with the vhost option so people
> > are not suprized.
> >
>

Re: [PATCH 3/3] virtio-net: graceful fallback to vhost=off for tap netdev

[Jason Wang]

On 2021/2/9 上午3:46, Yuri Benditovich wrote:
> On Mon, Feb 8, 2021 at 5:15 AM Jason Wang <jasowang@redhat.com> wrote:
>>
>> On 2021/2/5 下午9:38, Michael S. Tsirkin wrote:
>>> On Thu, Feb 04, 2021 at 10:29:15PM +0200, Yuri Benditovich wrote:
>>>> Currently virtio-net silently clears features if they are
>>>> not supported by respective vhost. This may create migration
>>>> problems in future if vhost features on the source and destination
>>>> are different. Implement graceful fallback to no-vhost mode
>>>> when some acked features contradict with vhost. The decision is
>>>> taken on set_features call and the vhost will be disabled
>>>> till next reset (or migration).
>>>> Such fallback is currently enabled only for TAP netdev.
>>>>
>>>> Signed-off-by: Yuri Benditovich<yuri.benditovich@daynix.com>
>>> Sounds good, but I don't think we should do this if
>>> vhostforce=on is set.
>>
>> If we do this, does it mean we won't maintain migration compatibility
>> when vhostforce is on?
> AFAIU, the 'vhostforce=on' should mean the vhost can't be disabled (if
> I'm not mistaken this is typically used for vhost-user).
> So we can view this case as similar to vhost-vdpa and vhost-user.


Right, but since it was used by libivrt. Then it turns out to be a 
compatibility breaker.

Thanks


>
>> Thanks
>>
>>
>>> Also, let's document this behaviour with the vhost option so people
>>> are not suprized.
>>>

Re: [PATCH 3/3] virtio-net: graceful fallback to vhost=off for tap netdev

[Jason Wang]

On 2021/2/5 上午4:29, Yuri Benditovich wrote:
> Currently virtio-net silently clears features if they are
> not supported by respective vhost. This may create migration
> problems in future if vhost features on the source and destination
> are different. Implement graceful fallback to no-vhost mode
> when some acked features contradict with vhost. The decision is
> taken on set_features call and the vhost will be disabled
> till next reset (or migration).
> Such fallback is currently enabled only for TAP netdev.
>
> Signed-off-by: Yuri Benditovich <yuri.benditovich@daynix.com>
> ---
>   hw/net/virtio-net.c | 58 ++++++++++++++++++++++++++++++++++++++-------
>   1 file changed, 50 insertions(+), 8 deletions(-)
>
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index 5150f295e8..b353060e63 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -515,6 +515,15 @@ static RxFilterInfo *virtio_net_query_rxfilter(NetClientState *nc)
>       return info;
>   }
>   
> +static void virtio_net_allow_vhost(VirtIONet *n, bool allow)
> +{
> +    int i;
> +    for (i = 0; i < n->max_queues; i++) {
> +        NetClientState *nc = qemu_get_subqueue(n->nic, i)->peer;
> +        nc->vhost_net_disabled = !allow;
> +    }
> +}
> +
>   static void virtio_net_reset(VirtIODevice *vdev)
>   {
>       VirtIONet *n = VIRTIO_NET(vdev);
> @@ -552,6 +561,7 @@ static void virtio_net_reset(VirtIODevice *vdev)
>               assert(!virtio_net_get_subqueue(nc)->async_tx.elem);
>           }
>       }
> +    virtio_net_allow_vhost(n, true);
>   }
>   
>   static void peer_test_vnet_hdr(VirtIONet *n)
> @@ -689,6 +699,15 @@ static void virtio_net_set_queues(VirtIONet *n)
>       }
>   }
>   
> +static bool can_disable_vhost(VirtIONet *n)
> +{
> +    NetClientState *peer = qemu_get_queue(n->nic)->peer;
> +    if (!get_vhost_net(peer)) {
> +        return false;
> +    }
> +    return !peer || peer->info->type == NET_CLIENT_DRIVER_TAP;
> +}
> +
>   static void virtio_net_set_multiqueue(VirtIONet *n, int multiqueue);
>   
>   static uint64_t virtio_net_get_features(VirtIODevice *vdev, uint64_t features,
> @@ -725,14 +744,14 @@ static uint64_t virtio_net_get_features(VirtIODevice *vdev, uint64_t features,
>           return features;
>       }
>   
> -    virtio_clear_feature(&features, VIRTIO_NET_F_RSS);
> -    virtio_clear_feature(&features, VIRTIO_NET_F_HASH_REPORT);
> -    features = vhost_net_get_features(get_vhost_net(nc->peer), features);
> -    vdev->backend_features = features;
> +    vdev->backend_features = vhost_net_get_features(get_vhost_net(nc->peer), features);
>   
> -    if (n->mtu_bypass_backend &&
> -            (n->host_features & 1ULL << VIRTIO_NET_F_MTU)) {
> -        features |= (1ULL << VIRTIO_NET_F_MTU);
> +    if (!can_disable_vhost(n)) {
> +        features = vdev->backend_features;
> +        if (n->mtu_bypass_backend &&
> +                (n->host_features & 1ULL << VIRTIO_NET_F_MTU)) {
> +            features |= (1ULL << VIRTIO_NET_F_MTU);
> +        }
>       }
>   
>       return features;
> @@ -872,10 +891,25 @@ static void failover_add_primary(VirtIONet *n, Error **errp)
>       error_propagate(errp, err);
>   }
>   
> +static bool check_vhost_features(VirtIONet *n, uint64_t features)
> +{
> +    NetClientState *nc = qemu_get_queue(n->nic);
> +    uint64_t filtered;
> +    if (n->rss_data.redirect) {
> +        return false;
> +    }
> +    filtered = vhost_net_get_features(get_vhost_net(nc->peer), features);
> +    if (filtered != features) {
> +        return false;
> +    }
> +    return true;
> +}
> +
>   static void virtio_net_set_features(VirtIODevice *vdev, uint64_t features)
>   {
>       VirtIONet *n = VIRTIO_NET(vdev);
>       Error *err = NULL;
> +    bool disable_vhost = false;
>       int i;
>   
>       if (n->mtu_bypass_backend &&
> @@ -894,13 +928,21 @@ static void virtio_net_set_features(VirtIODevice *vdev, uint64_t features)
>                                                     VIRTIO_F_VERSION_1),
>                                  virtio_has_feature(features,
>                                                     VIRTIO_NET_F_HASH_REPORT));
> -
>       n->rsc4_enabled = virtio_has_feature(features, VIRTIO_NET_F_RSC_EXT) &&
>           virtio_has_feature(features, VIRTIO_NET_F_GUEST_TSO4);
>       n->rsc6_enabled = virtio_has_feature(features, VIRTIO_NET_F_RSC_EXT) &&
>           virtio_has_feature(features, VIRTIO_NET_F_GUEST_TSO6);
>       n->rss_data.redirect = virtio_has_feature(features, VIRTIO_NET_F_RSS);
>   
> +    if (can_disable_vhost(n)) {
> +        disable_vhost = !check_vhost_features(n, features);
> +    }
> +    if (disable_vhost) {
> +        warn_report("Some of requested features aren't supported by vhost, "
> +                    "vhost is turned off till next reset");
> +        virtio_net_allow_vhost(n, false);
> +    }


This looks more complicated than I expected. See 
virtio_net_vhost_status() we had a fallback there:

         r = vhost_net_start(vdev, n->nic->ncs, queues);
         if (r < 0) {
             error_report("unable to start vhost net: %d: "
                          "falling back on userspace virtio", -r);
             n->vhost_started = 0;
         }

I wonder if we can simply depends on this (which is proved to be work 
for the past years) by not clearing dev->acked_features like:

if (!can_disable_vhost(n)) {
     features = vhost_net_get_features(get_vhost_net(nc->peer), features);
} else {
     vdev->backend_features = features;
}

Then we probably don't need other tricks.

Thanks


> +
>       if (n->has_vnet_hdr) {
>           n->curr_guest_offloads =
>               virtio_net_guest_offloads_by_features(features);

Re: [PATCH 3/3] virtio-net: graceful fallback to vhost=off for tap netdev

[Yuri Benditovich]

On Mon, Feb 8, 2021 at 6:11 AM Jason Wang <jasowang@redhat.com> wrote:
>
>
> On 2021/2/5 上午4:29, Yuri Benditovich wrote:
> > Currently virtio-net silently clears features if they are
> > not supported by respective vhost. This may create migration
> > problems in future if vhost features on the source and destination
> > are different. Implement graceful fallback to no-vhost mode
> > when some acked features contradict with vhost. The decision is
> > taken on set_features call and the vhost will be disabled
> > till next reset (or migration).
> > Such fallback is currently enabled only for TAP netdev.
> >
> > Signed-off-by: Yuri Benditovich <yuri.benditovich@daynix.com>
> > ---
> >   hw/net/virtio-net.c | 58 ++++++++++++++++++++++++++++++++++++++-------
> >   1 file changed, 50 insertions(+), 8 deletions(-)
> >
> > diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> > index 5150f295e8..b353060e63 100644
> > --- a/hw/net/virtio-net.c
> > +++ b/hw/net/virtio-net.c
> > @@ -515,6 +515,15 @@ static RxFilterInfo *virtio_net_query_rxfilter(NetClientState *nc)
> >       return info;
> >   }
> >
> > +static void virtio_net_allow_vhost(VirtIONet *n, bool allow)
> > +{
> > +    int i;
> > +    for (i = 0; i < n->max_queues; i++) {
> > +        NetClientState *nc = qemu_get_subqueue(n->nic, i)->peer;
> > +        nc->vhost_net_disabled = !allow;
> > +    }
> > +}
> > +
> >   static void virtio_net_reset(VirtIODevice *vdev)
> >   {
> >       VirtIONet *n = VIRTIO_NET(vdev);
> > @@ -552,6 +561,7 @@ static void virtio_net_reset(VirtIODevice *vdev)
> >               assert(!virtio_net_get_subqueue(nc)->async_tx.elem);
> >           }
> >       }
> > +    virtio_net_allow_vhost(n, true);
> >   }
> >
> >   static void peer_test_vnet_hdr(VirtIONet *n)
> > @@ -689,6 +699,15 @@ static void virtio_net_set_queues(VirtIONet *n)
> >       }
> >   }
> >
> > +static bool can_disable_vhost(VirtIONet *n)
> > +{
> > +    NetClientState *peer = qemu_get_queue(n->nic)->peer;
> > +    if (!get_vhost_net(peer)) {
> > +        return false;
> > +    }
> > +    return !peer || peer->info->type == NET_CLIENT_DRIVER_TAP;
> > +}
> > +
> >   static void virtio_net_set_multiqueue(VirtIONet *n, int multiqueue);
> >
> >   static uint64_t virtio_net_get_features(VirtIODevice *vdev, uint64_t features,
> > @@ -725,14 +744,14 @@ static uint64_t virtio_net_get_features(VirtIODevice *vdev, uint64_t features,
> >           return features;
> >       }
> >
> > -    virtio_clear_feature(&features, VIRTIO_NET_F_RSS);
> > -    virtio_clear_feature(&features, VIRTIO_NET_F_HASH_REPORT);
> > -    features = vhost_net_get_features(get_vhost_net(nc->peer), features);
> > -    vdev->backend_features = features;
> > +    vdev->backend_features = vhost_net_get_features(get_vhost_net(nc->peer), features);
> >
> > -    if (n->mtu_bypass_backend &&
> > -            (n->host_features & 1ULL << VIRTIO_NET_F_MTU)) {
> > -        features |= (1ULL << VIRTIO_NET_F_MTU);
> > +    if (!can_disable_vhost(n)) {
> > +        features = vdev->backend_features;
> > +        if (n->mtu_bypass_backend &&
> > +                (n->host_features & 1ULL << VIRTIO_NET_F_MTU)) {
> > +            features |= (1ULL << VIRTIO_NET_F_MTU);
> > +        }
> >       }
> >
> >       return features;
> > @@ -872,10 +891,25 @@ static void failover_add_primary(VirtIONet *n, Error **errp)
> >       error_propagate(errp, err);
> >   }
> >
> > +static bool check_vhost_features(VirtIONet *n, uint64_t features)
> > +{
> > +    NetClientState *nc = qemu_get_queue(n->nic);
> > +    uint64_t filtered;
> > +    if (n->rss_data.redirect) {
> > +        return false;
> > +    }
> > +    filtered = vhost_net_get_features(get_vhost_net(nc->peer), features);
> > +    if (filtered != features) {
> > +        return false;
> > +    }
> > +    return true;
> > +}
> > +
> >   static void virtio_net_set_features(VirtIODevice *vdev, uint64_t features)
> >   {
> >       VirtIONet *n = VIRTIO_NET(vdev);
> >       Error *err = NULL;
> > +    bool disable_vhost = false;
> >       int i;
> >
> >       if (n->mtu_bypass_backend &&
> > @@ -894,13 +928,21 @@ static void virtio_net_set_features(VirtIODevice *vdev, uint64_t features)
> >                                                     VIRTIO_F_VERSION_1),
> >                                  virtio_has_feature(features,
> >                                                     VIRTIO_NET_F_HASH_REPORT));
> > -
> >       n->rsc4_enabled = virtio_has_feature(features, VIRTIO_NET_F_RSC_EXT) &&
> >           virtio_has_feature(features, VIRTIO_NET_F_GUEST_TSO4);
> >       n->rsc6_enabled = virtio_has_feature(features, VIRTIO_NET_F_RSC_EXT) &&
> >           virtio_has_feature(features, VIRTIO_NET_F_GUEST_TSO6);
> >       n->rss_data.redirect = virtio_has_feature(features, VIRTIO_NET_F_RSS);
> >
> > +    if (can_disable_vhost(n)) {
> > +        disable_vhost = !check_vhost_features(n, features);
> > +    }
> > +    if (disable_vhost) {
> > +        warn_report("Some of requested features aren't supported by vhost, "
> > +                    "vhost is turned off till next reset");
> > +        virtio_net_allow_vhost(n, false);
> > +    }
>
>
> This looks more complicated than I expected. See
> virtio_net_vhost_status() we had a fallback there:
>
>          r = vhost_net_start(vdev, n->nic->ncs, queues);
>          if (r < 0) {
>              error_report("unable to start vhost net: %d: "
>                           "falling back on userspace virtio", -r);
>              n->vhost_started = 0;
>          }
>
> I wonder if we can simply depends on this (which is proved to be work
> for the past years) by not clearing dev->acked_features like:
>
> if (!can_disable_vhost(n)) {
>      features = vhost_net_get_features(get_vhost_net(nc->peer), features);
> } else {
>      vdev->backend_features = features;
> }
>
> Then we probably don't need other tricks.

Of course we can.
But I would prefer to make things more clear, i.e. make
get_vhost_net() always return a meaningful result, even (as an
example) in procedures called from set_feature() after the
vhost_disabled is set.
Otherwise people can rely on get_vhost_net() and call its methods
which potentially can do something that we do not expect.
In some places in the code (also in future) we'll need to check not
only get_vhost_net() but also is_vhost_disabled. IMO, not too elegant,
but possible.
Of course, being a decision maker, you can request to do it simpler
and do not maintain a consistent result of get_vhost_net().
But then please tell it explicitly.

>
> Thanks
>
>
> > +
> >       if (n->has_vnet_hdr) {
> >           n->curr_guest_offloads =
> >               virtio_net_guest_offloads_by_features(features);
>

Re: [PATCH 3/3] virtio-net: graceful fallback to vhost=off for tap netdev

[Jason Wang]

On 2021/2/9 上午3:59, Yuri Benditovich wrote:
> On Mon, Feb 8, 2021 at 6:11 AM Jason Wang <jasowang@redhat.com> wrote:
>>
>> On 2021/2/5 上午4:29, Yuri Benditovich wrote:
>>> Currently virtio-net silently clears features if they are
>>> not supported by respective vhost. This may create migration
>>> problems in future if vhost features on the source and destination
>>> are different. Implement graceful fallback to no-vhost mode
>>> when some acked features contradict with vhost. The decision is
>>> taken on set_features call and the vhost will be disabled
>>> till next reset (or migration).
>>> Such fallback is currently enabled only for TAP netdev.
>>>
>>> Signed-off-by: Yuri Benditovich <yuri.benditovich@daynix.com>
>>> ---
>>>    hw/net/virtio-net.c | 58 ++++++++++++++++++++++++++++++++++++++-------
>>>    1 file changed, 50 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
>>> index 5150f295e8..b353060e63 100644
>>> --- a/hw/net/virtio-net.c
>>> +++ b/hw/net/virtio-net.c
>>> @@ -515,6 +515,15 @@ static RxFilterInfo *virtio_net_query_rxfilter(NetClientState *nc)
>>>        return info;
>>>    }
>>>
>>> +static void virtio_net_allow_vhost(VirtIONet *n, bool allow)
>>> +{
>>> +    int i;
>>> +    for (i = 0; i < n->max_queues; i++) {
>>> +        NetClientState *nc = qemu_get_subqueue(n->nic, i)->peer;
>>> +        nc->vhost_net_disabled = !allow;
>>> +    }
>>> +}
>>> +
>>>    static void virtio_net_reset(VirtIODevice *vdev)
>>>    {
>>>        VirtIONet *n = VIRTIO_NET(vdev);
>>> @@ -552,6 +561,7 @@ static void virtio_net_reset(VirtIODevice *vdev)
>>>                assert(!virtio_net_get_subqueue(nc)->async_tx.elem);
>>>            }
>>>        }
>>> +    virtio_net_allow_vhost(n, true);
>>>    }
>>>
>>>    static void peer_test_vnet_hdr(VirtIONet *n)
>>> @@ -689,6 +699,15 @@ static void virtio_net_set_queues(VirtIONet *n)
>>>        }
>>>    }
>>>
>>> +static bool can_disable_vhost(VirtIONet *n)
>>> +{
>>> +    NetClientState *peer = qemu_get_queue(n->nic)->peer;
>>> +    if (!get_vhost_net(peer)) {
>>> +        return false;
>>> +    }
>>> +    return !peer || peer->info->type == NET_CLIENT_DRIVER_TAP;
>>> +}
>>> +
>>>    static void virtio_net_set_multiqueue(VirtIONet *n, int multiqueue);
>>>
>>>    static uint64_t virtio_net_get_features(VirtIODevice *vdev, uint64_t features,
>>> @@ -725,14 +744,14 @@ static uint64_t virtio_net_get_features(VirtIODevice *vdev, uint64_t features,
>>>            return features;
>>>        }
>>>
>>> -    virtio_clear_feature(&features, VIRTIO_NET_F_RSS);
>>> -    virtio_clear_feature(&features, VIRTIO_NET_F_HASH_REPORT);
>>> -    features = vhost_net_get_features(get_vhost_net(nc->peer), features);
>>> -    vdev->backend_features = features;
>>> +    vdev->backend_features = vhost_net_get_features(get_vhost_net(nc->peer), features);
>>>
>>> -    if (n->mtu_bypass_backend &&
>>> -            (n->host_features & 1ULL << VIRTIO_NET_F_MTU)) {
>>> -        features |= (1ULL << VIRTIO_NET_F_MTU);
>>> +    if (!can_disable_vhost(n)) {
>>> +        features = vdev->backend_features;
>>> +        if (n->mtu_bypass_backend &&
>>> +                (n->host_features & 1ULL << VIRTIO_NET_F_MTU)) {
>>> +            features |= (1ULL << VIRTIO_NET_F_MTU);
>>> +        }
>>>        }
>>>
>>>        return features;
>>> @@ -872,10 +891,25 @@ static void failover_add_primary(VirtIONet *n, Error **errp)
>>>        error_propagate(errp, err);
>>>    }
>>>
>>> +static bool check_vhost_features(VirtIONet *n, uint64_t features)
>>> +{
>>> +    NetClientState *nc = qemu_get_queue(n->nic);
>>> +    uint64_t filtered;
>>> +    if (n->rss_data.redirect) {
>>> +        return false;
>>> +    }
>>> +    filtered = vhost_net_get_features(get_vhost_net(nc->peer), features);
>>> +    if (filtered != features) {
>>> +        return false;
>>> +    }
>>> +    return true;
>>> +}
>>> +
>>>    static void virtio_net_set_features(VirtIODevice *vdev, uint64_t features)
>>>    {
>>>        VirtIONet *n = VIRTIO_NET(vdev);
>>>        Error *err = NULL;
>>> +    bool disable_vhost = false;
>>>        int i;
>>>
>>>        if (n->mtu_bypass_backend &&
>>> @@ -894,13 +928,21 @@ static void virtio_net_set_features(VirtIODevice *vdev, uint64_t features)
>>>                                                      VIRTIO_F_VERSION_1),
>>>                                   virtio_has_feature(features,
>>>                                                      VIRTIO_NET_F_HASH_REPORT));
>>> -
>>>        n->rsc4_enabled = virtio_has_feature(features, VIRTIO_NET_F_RSC_EXT) &&
>>>            virtio_has_feature(features, VIRTIO_NET_F_GUEST_TSO4);
>>>        n->rsc6_enabled = virtio_has_feature(features, VIRTIO_NET_F_RSC_EXT) &&
>>>            virtio_has_feature(features, VIRTIO_NET_F_GUEST_TSO6);
>>>        n->rss_data.redirect = virtio_has_feature(features, VIRTIO_NET_F_RSS);
>>>
>>> +    if (can_disable_vhost(n)) {
>>> +        disable_vhost = !check_vhost_features(n, features);
>>> +    }
>>> +    if (disable_vhost) {
>>> +        warn_report("Some of requested features aren't supported by vhost, "
>>> +                    "vhost is turned off till next reset");
>>> +        virtio_net_allow_vhost(n, false);
>>> +    }
>>
>> This looks more complicated than I expected. See
>> virtio_net_vhost_status() we had a fallback there:
>>
>>           r = vhost_net_start(vdev, n->nic->ncs, queues);
>>           if (r < 0) {
>>               error_report("unable to start vhost net: %d: "
>>                            "falling back on userspace virtio", -r);
>>               n->vhost_started = 0;
>>           }
>>
>> I wonder if we can simply depends on this (which is proved to be work
>> for the past years) by not clearing dev->acked_features like:
>>
>> if (!can_disable_vhost(n)) {
>>       features = vhost_net_get_features(get_vhost_net(nc->peer), features);
>> } else {
>>       vdev->backend_features = features;
>> }
>>
>> Then we probably don't need other tricks.
> Of course we can.
> But I would prefer to make things more clear, i.e. make
> get_vhost_net() always return a meaningful result, even (as an
> example) in procedures called from set_feature() after the
> vhost_disabled is set.
> Otherwise people can rely on get_vhost_net() and call its methods
> which potentially can do something that we do not expect.
> In some places in the code (also in future) we'll need to check not
> only get_vhost_net() but also is_vhost_disabled. IMO, not too elegant,
> but possible.


I see.


> Of course, being a decision maker, you can request to do it simpler
> and do not maintain a consistent result of get_vhost_net().
> But then please tell it explicitly.


So the reason that I prefer the above simple checks is that we've 
already had two falling backs points in virtio_net_vhost_status():

1) for vnet_hdr_swap:

         if (n->needs_vnet_hdr_swap) {
             error_report("backend does not support %s vnet headers; "
                          "falling back on userspace virtio",
                          virtio_is_big_endian(vdev) ? "BE" : "LE");
             return;
         }

2) for all of the other cases:

         n->vhost_started = 1;
         r = vhost_net_start(vdev, n->nic->ncs, queues);
         if (r < 0) {
             error_report("unable to start vhost net: %d: "
                          "falling back on userspace virtio", -r);
             n->vhost_started = 0;
         }

So it's better to have a consistent way:

1) Introduce the variable disable_vhost as you suggest

or

2) falling back by checking the return status of vhost_net_start()

In this case, 2) looks much more easier than convert all the falling 
backs to 1).

Thanks


>
>> Thanks
>>
>>
>>> +
>>>        if (n->has_vnet_hdr) {
>>>            n->curr_guest_offloads =
>>>                virtio_net_guest_offloads_by_features(features);

Re: [PATCH 0/3] virtio-net: graceful drop of vhost for TAP

[Michael S. Tsirkin]

On Thu, Feb 04, 2021 at 10:29:12PM +0200, Yuri Benditovich wrote:
> This set of patches introduces graceful switch from tap-vhost to
> tap-no-vhost depending on guest features. Before that the features
> that vhost does not support were silently cleared in get_features.
> This creates potential problem of migration from the machine where
> some of virtio-net features are supported by the vhost kernel to the
> machine where they are not supported (packed ring as an example).

I still worry that adding new features will silently disable vhost for people.
Can we limit the change to when a VM is migrated in?



> Instead of silent masking of the features virtio-net gracefully
> disables the vhost at set_features if some features acked by the
> guest contradict with kernel vhost capabilities.
> 
> This set of patches also makes get_vhost_net() call (that used
> everywhere) to always return actual result, i.e. initially it
> returns non-NULL value and from the moment the vhost was disabled
> the call will return NULL. Such a way we avoid any unexpected
> calls to vhost functions.
> Yuri Benditovich (3):
>   vhost-net: add VIRTIO_NET_F_HASH_REPORT to the list of kernel features
>   net: add ability to hide (disable) vhost_net
>   virtio-net: graceful fallback to vhost=off for tap netdev
> 
>  hw/net/vhost_net.c  |  5 +++-
>  hw/net/virtio-net.c | 58 ++++++++++++++++++++++++++++++++++++++-------
>  include/net/net.h   |  1 +
>  3 files changed, 55 insertions(+), 9 deletions(-)
> 
> -- 
> 2.17.1

Re: [PATCH 0/3] virtio-net: graceful drop of vhost for TAP

[Daniel P. Berrangé]

On Tue, Feb 09, 2021 at 09:34:20AM -0500, Michael S. Tsirkin wrote:
> On Thu, Feb 04, 2021 at 10:29:12PM +0200, Yuri Benditovich wrote:
> > This set of patches introduces graceful switch from tap-vhost to
> > tap-no-vhost depending on guest features. Before that the features
> > that vhost does not support were silently cleared in get_features.
> > This creates potential problem of migration from the machine where
> > some of virtio-net features are supported by the vhost kernel to the
> > machine where they are not supported (packed ring as an example).
> 
> I still worry that adding new features will silently disable vhost for people.
> Can we limit the change to when a VM is migrated in?

Some management applications expect bi-directional live migration to
work, so taking specific actions on incoming migration only feels
dangerous. 

IMHO if the features we're adding cannot be expected to exist in
host kernels in general, then the feature should defualt to off
and require explicit user config to enable.

Downstream distros which can guarantee newer kernels can flip the
default in their custom machine types if they desire.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 0/3] virtio-net: graceful drop of vhost for TAP

[Michael S. Tsirkin]

On Tue, Feb 09, 2021 at 02:51:05PM +0000, Daniel P. Berrangé wrote:
> On Tue, Feb 09, 2021 at 09:34:20AM -0500, Michael S. Tsirkin wrote:
> > On Thu, Feb 04, 2021 at 10:29:12PM +0200, Yuri Benditovich wrote:
> > > This set of patches introduces graceful switch from tap-vhost to
> > > tap-no-vhost depending on guest features. Before that the features
> > > that vhost does not support were silently cleared in get_features.
> > > This creates potential problem of migration from the machine where
> > > some of virtio-net features are supported by the vhost kernel to the
> > > machine where they are not supported (packed ring as an example).
> > 
> > I still worry that adding new features will silently disable vhost for people.
> > Can we limit the change to when a VM is migrated in?
> 
> Some management applications expect bi-directional live migration to
> work, so taking specific actions on incoming migration only feels
> dangerous. 

Could you be more specific?

Bi-directional migration is currently broken
when migrating new kernel->old kernel.

This seems to be the motivation for this patch, though I wish
it was spelled out more explicitly.

People don't complain much, but I'm fine with fixing that
with a userspace fallback.


I'd rather not force the fallback on others though: vhost is generally
specified explicitly by user while features are generally set
automatically, so this patch will make us override what user specified,
not nice.


> IMHO if the features we're adding cannot be expected to exist in
> host kernels in general, then the feature should defualt to off
> and require explicit user config to enable.
> Downstream distros which can guarantee newer kernels can flip the
> default in their custom machine types if they desire.
> 
> Regards,
> Daniel

Unfortunately that will basically mean we are stuck with no new features
for years. We did what this patch is trying to change for years now, in
particular KVM also seems to happily disable CPU features not supported
by kernel so I wonder why we can't keep doing it, with tweaks for some
corner cases.

userspace and kernel not being in 100% sync wrt features is not
a corner case though, and switching backends seems like too big
a hammer.

> -- 
> |: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org         -o-            https://fstop138.berrange.com :|
> |: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 0/3] virtio-net: graceful drop of vhost for TAP

[Daniel P. Berrangé]

On Tue, Feb 09, 2021 at 10:04:30AM -0500, Michael S. Tsirkin wrote:
> On Tue, Feb 09, 2021 at 02:51:05PM +0000, Daniel P. Berrangé wrote:
> > On Tue, Feb 09, 2021 at 09:34:20AM -0500, Michael S. Tsirkin wrote:
> > > On Thu, Feb 04, 2021 at 10:29:12PM +0200, Yuri Benditovich wrote:
> > > > This set of patches introduces graceful switch from tap-vhost to
> > > > tap-no-vhost depending on guest features. Before that the features
> > > > that vhost does not support were silently cleared in get_features.
> > > > This creates potential problem of migration from the machine where
> > > > some of virtio-net features are supported by the vhost kernel to the
> > > > machine where they are not supported (packed ring as an example).
> > > 
> > > I still worry that adding new features will silently disable vhost for people.
> > > Can we limit the change to when a VM is migrated in?
> > 
> > Some management applications expect bi-directional live migration to
> > work, so taking specific actions on incoming migration only feels
> > dangerous. 
> 
> Could you be more specific?
> 
> Bi-directional migration is currently broken
> when migrating new kernel->old kernel.

At the very least  new QEMU -> old QEMU, but in general new kernel
to old kerenl would be expected too. Assuming a QEMU command line
is using a versioned machine type, and not using host passthrough
features (-cpu host), then it should in general represent a fixed
guest ABI  independant of kernel version/features.

> > IMHO if the features we're adding cannot be expected to exist in
> > host kernels in general, then the feature should defualt to off
> > and require explicit user config to enable.
> > Downstream distros which can guarantee newer kernels can flip the
> > default in their custom machine types if they desire.
> 
> Unfortunately that will basically mean we are stuck with no new features
> for years. We did what this patch is trying to change for years now, in
> particular KVM also seems to happily disable CPU features not supported
> by kernel so I wonder why we can't keep doing it, with tweaks for some
> corner cases.

Yep, it is not great from a upstream POV.

In upstream we've traditionally not expressed a specific min
kernel version we expect to be run on. So that lmits what we
can do in upstram machine types.  Downstram distros can
expose stuff pretty much immediately if they desire.

eg if RHEL 8 adds the kernel feature in 8.5, it could in
theory add the feature by default in pc-q35-rhel8.5.0 machine
types (if we ignore pain of containers running on old kernels).
Even if we take into account mismatched usrespace/kernel, the
downstram distros can adopt much more quickly.

There is probably scope for upstream to be more explicit about
min required kernel features, but we'll always be worse upstream
because we target a broader range of platforms/distros.

> userspace and kernel not being in 100% sync wrt features is not
> a corner case though, and switching backends seems like too big
> a hammer.

Yes, the containers world in particular has made life much worse,
because the mis-matched combinations of  userspace and kernel are
much more likely. In fact I'd say out of sync kernel+userspace
is arguably going to become the common case in future.

eg worst case someone running a RHEL-8 userspace on a RHEL-7
container host or vica verca.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 0/3] virtio-net: graceful drop of vhost for TAP

[Jason Wang]

On 2021/2/9 下午11:04, Michael S. Tsirkin wrote:
> On Tue, Feb 09, 2021 at 02:51:05PM +0000, Daniel P. Berrangé wrote:
>> On Tue, Feb 09, 2021 at 09:34:20AM -0500, Michael S. Tsirkin wrote:
>>> On Thu, Feb 04, 2021 at 10:29:12PM +0200, Yuri Benditovich wrote:
>>>> This set of patches introduces graceful switch from tap-vhost to
>>>> tap-no-vhost depending on guest features. Before that the features
>>>> that vhost does not support were silently cleared in get_features.
>>>> This creates potential problem of migration from the machine where
>>>> some of virtio-net features are supported by the vhost kernel to the
>>>> machine where they are not supported (packed ring as an example).
>>> I still worry that adding new features will silently disable vhost for people.
>>> Can we limit the change to when a VM is migrated in?
>> Some management applications expect bi-directional live migration to
>> work, so taking specific actions on incoming migration only feels
>> dangerous.
> Could you be more specific?
>
> Bi-directional migration is currently broken
> when migrating new kernel->old kernel.
>
> This seems to be the motivation for this patch, though I wish
> it was spelled out more explicitly.
>
> People don't complain much, but I'm fine with fixing that
> with a userspace fallback.
>
>
> I'd rather not force the fallback on others though: vhost is generally
> specified explicitly by user while features are generally set
> automatically, so this patch will make us override what user specified,
> not nice.
>
>
>> IMHO if the features we're adding cannot be expected to exist in
>> host kernels in general, then the feature should defualt to off
>> and require explicit user config to enable.
>> Downstream distros which can guarantee newer kernels can flip the
>> default in their custom machine types if they desire.
>>
>> Regards,
>> Daniel
> Unfortunately that will basically mean we are stuck with no new features
> for years. We did what this patch is trying to change for years now, in
> particular KVM also seems to happily disable CPU features not supported
> by kernel so I wonder why we can't keep doing it, with tweaks for some
> corner cases.


It's probably not the corner case.

So my understanding is when a feature is turned on via command line, it 
should not be cleared silently otherwise we may break migration for sure.

E.g when packed=on is specified, we should disable vhost instead of 
clear it from the device.

Thanks


>
> userspace and kernel not being in 100% sync wrt features is not
> a corner case though, and switching backends seems like too big
> a hammer.
>
>> -- 
>> |: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
>> |: https://libvirt.org         -o-            https://fstop138.berrange.com :|
>> |: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
>

Re: [PATCH 0/3] virtio-net: graceful drop of vhost for TAP

[Michael S. Tsirkin]

On Wed, Feb 10, 2021 at 02:19:59PM +0800, Jason Wang wrote:
> 
> On 2021/2/9 下午11:04, Michael S. Tsirkin wrote:
> > On Tue, Feb 09, 2021 at 02:51:05PM +0000, Daniel P. Berrangé wrote:
> > > On Tue, Feb 09, 2021 at 09:34:20AM -0500, Michael S. Tsirkin wrote:
> > > > On Thu, Feb 04, 2021 at 10:29:12PM +0200, Yuri Benditovich wrote:
> > > > > This set of patches introduces graceful switch from tap-vhost to
> > > > > tap-no-vhost depending on guest features. Before that the features
> > > > > that vhost does not support were silently cleared in get_features.
> > > > > This creates potential problem of migration from the machine where
> > > > > some of virtio-net features are supported by the vhost kernel to the
> > > > > machine where they are not supported (packed ring as an example).
> > > > I still worry that adding new features will silently disable vhost for people.
> > > > Can we limit the change to when a VM is migrated in?
> > > Some management applications expect bi-directional live migration to
> > > work, so taking specific actions on incoming migration only feels
> > > dangerous.
> > Could you be more specific?
> > 
> > Bi-directional migration is currently broken
> > when migrating new kernel->old kernel.
> > 
> > This seems to be the motivation for this patch, though I wish
> > it was spelled out more explicitly.
> > 
> > People don't complain much, but I'm fine with fixing that
> > with a userspace fallback.
> > 
> > 
> > I'd rather not force the fallback on others though: vhost is generally
> > specified explicitly by user while features are generally set
> > automatically, so this patch will make us override what user specified,
> > not nice.
> > 
> > 
> > > IMHO if the features we're adding cannot be expected to exist in
> > > host kernels in general, then the feature should defualt to off
> > > and require explicit user config to enable.
> > > Downstream distros which can guarantee newer kernels can flip the
> > > default in their custom machine types if they desire.
> > > 
> > > Regards,
> > > Daniel
> > Unfortunately that will basically mean we are stuck with no new features
> > for years. We did what this patch is trying to change for years now, in
> > particular KVM also seems to happily disable CPU features not supported
> > by kernel so I wonder why we can't keep doing it, with tweaks for some
> > corner cases.
> 
> 
> It's probably not the corner case.
> 
> So my understanding is when a feature is turned on via command line,


Most people do not play with these flags on command line, the
main path to turn features on if when they default to on.

> it
> should not be cleared silently otherwise we may break migration for sure.

Not if we are careful. Setting flags is more dangerous. Clearing is
generally ok.

> E.g when packed=on is specified, we should disable vhost instead of clear it
> from the device.
> 
> Thanks

From usability POV, consider packed as an example, people only enable it
to get better performance. libvirt says:


		The attribute packed controls if QEMU should try to use packed
	virtqueues. Compared to regular split queues, packed queues consist of
	only a single descriptor ring replacing available and used ring, index
	and descriptor buffer. This can result in better cache utilization and
	performance. If packed virtqueues are actually used depends on the
	feature negotiation between QEMU, vhost backends and guest drivers.
	Possible values are on or off. 

Switching to a completely different backend clearly isn't what user intended.

> 
> > 
> > userspace and kernel not being in 100% sync wrt features is not
> > a corner case though, and switching backends seems like too big
> > a hammer.
> > 
> > > -- 
> > > |: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
> > > |: https://libvirt.org         -o-            https://fstop138.berrange.com :|
> > > |: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
> > 

Re: [PATCH 0/3] virtio-net: graceful drop of vhost for TAP

[Jason Wang]

On 2021/2/10 下午4:38, Michael S. Tsirkin wrote:
> On Wed, Feb 10, 2021 at 02:19:59PM +0800, Jason Wang wrote:
>> On 2021/2/9 下午11:04, Michael S. Tsirkin wrote:
>>> On Tue, Feb 09, 2021 at 02:51:05PM +0000, Daniel P. Berrangé wrote:
>>>> On Tue, Feb 09, 2021 at 09:34:20AM -0500, Michael S. Tsirkin wrote:
>>>>> On Thu, Feb 04, 2021 at 10:29:12PM +0200, Yuri Benditovich wrote:
>>>>>> This set of patches introduces graceful switch from tap-vhost to
>>>>>> tap-no-vhost depending on guest features. Before that the features
>>>>>> that vhost does not support were silently cleared in get_features.
>>>>>> This creates potential problem of migration from the machine where
>>>>>> some of virtio-net features are supported by the vhost kernel to the
>>>>>> machine where they are not supported (packed ring as an example).
>>>>> I still worry that adding new features will silently disable vhost for people.
>>>>> Can we limit the change to when a VM is migrated in?
>>>> Some management applications expect bi-directional live migration to
>>>> work, so taking specific actions on incoming migration only feels
>>>> dangerous.
>>> Could you be more specific?
>>>
>>> Bi-directional migration is currently broken
>>> when migrating new kernel->old kernel.
>>>
>>> This seems to be the motivation for this patch, though I wish
>>> it was spelled out more explicitly.
>>>
>>> People don't complain much, but I'm fine with fixing that
>>> with a userspace fallback.
>>>
>>>
>>> I'd rather not force the fallback on others though: vhost is generally
>>> specified explicitly by user while features are generally set
>>> automatically, so this patch will make us override what user specified,
>>> not nice.
>>>
>>>
>>>> IMHO if the features we're adding cannot be expected to exist in
>>>> host kernels in general, then the feature should defualt to off
>>>> and require explicit user config to enable.
>>>> Downstream distros which can guarantee newer kernels can flip the
>>>> default in their custom machine types if they desire.
>>>>
>>>> Regards,
>>>> Daniel
>>> Unfortunately that will basically mean we are stuck with no new features
>>> for years. We did what this patch is trying to change for years now, in
>>> particular KVM also seems to happily disable CPU features not supported
>>> by kernel so I wonder why we can't keep doing it, with tweaks for some
>>> corner cases.
>>
>> It's probably not the corner case.
>>
>> So my understanding is when a feature is turned on via command line,
>
> Most people do not play with these flags on command line, the
> main path to turn features on if when they default to on.


The problem is qemu may choose to clear feature whose default are on.


>
>> it
>> should not be cleared silently otherwise we may break migration for sure.
> Not if we are careful. Setting flags is more dangerous.


The problem is that user is expected to enable the feature for the 
device. If Qemu fail to do that, shouldn't we fail the device 
initialization?


> Clearing is
> generally ok.
>
>> E.g when packed=on is specified, we should disable vhost instead of clear it
>> from the device.
>>
>> Thanks
>  From usability POV, consider packed as an example, people only enable it
> to get better performance. libvirt says:
>
>
> 		The attribute packed controls if QEMU should try to use packed
> 	virtqueues. Compared to regular split queues, packed queues consist of
> 	only a single descriptor ring replacing available and used ring, index
> 	and descriptor buffer. This can result in better cache utilization and
> 	performance. If packed virtqueues are actually used depends on the
> 	feature negotiation between QEMU, vhost backends and guest drivers.
> 	Possible values are on or off.
>
> Switching to a completely different backend clearly isn't what user intended.


If we don't do migration, all should be fine. But the problem is the 
migration compatibility. My understanding is that libvirt will assume 
the compatibility if the same user-visible feature were set through 
cli.  So if any feature were cleared the migration compatibility will 
break. This will also be a burden to support cross vendor live migration 
in the future for vDPA.

Thanks


>
>>> userspace and kernel not being in 100% sync wrt features is not
>>> a corner case though, and switching backends seems like too big
>>> a hammer.
>>>
>>>> -- 
>>>> |: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
>>>> |: https://libvirt.org         -o-            https://fstop138.berrange.com :|
>>>> |: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 0/3] virtio-net: graceful drop of vhost for TAP

[Daniel P. Berrangé]

On Wed, Feb 10, 2021 at 02:19:59PM +0800, Jason Wang wrote:
> 
> On 2021/2/9 下午11:04, Michael S. Tsirkin wrote:
> > On Tue, Feb 09, 2021 at 02:51:05PM +0000, Daniel P. Berrangé wrote:
> > > On Tue, Feb 09, 2021 at 09:34:20AM -0500, Michael S. Tsirkin wrote:
> > > > On Thu, Feb 04, 2021 at 10:29:12PM +0200, Yuri Benditovich wrote:
> > > > > This set of patches introduces graceful switch from tap-vhost to
> > > > > tap-no-vhost depending on guest features. Before that the features
> > > > > that vhost does not support were silently cleared in get_features.
> > > > > This creates potential problem of migration from the machine where
> > > > > some of virtio-net features are supported by the vhost kernel to the
> > > > > machine where they are not supported (packed ring as an example).
> > > > I still worry that adding new features will silently disable vhost for people.
> > > > Can we limit the change to when a VM is migrated in?
> > > Some management applications expect bi-directional live migration to
> > > work, so taking specific actions on incoming migration only feels
> > > dangerous.
> > Could you be more specific?
> > 
> > Bi-directional migration is currently broken
> > when migrating new kernel->old kernel.
> > 
> > This seems to be the motivation for this patch, though I wish
> > it was spelled out more explicitly.
> > 
> > People don't complain much, but I'm fine with fixing that
> > with a userspace fallback.
> > 
> > 
> > I'd rather not force the fallback on others though: vhost is generally
> > specified explicitly by user while features are generally set
> > automatically, so this patch will make us override what user specified,
> > not nice.
> > 
> > 
> > > IMHO if the features we're adding cannot be expected to exist in
> > > host kernels in general, then the feature should defualt to off
> > > and require explicit user config to enable.
> > > Downstream distros which can guarantee newer kernels can flip the
> > > default in their custom machine types if they desire.
> > > 
> > > Regards,
> > > Daniel
> > Unfortunately that will basically mean we are stuck with no new features
> > for years. We did what this patch is trying to change for years now, in
> > particular KVM also seems to happily disable CPU features not supported
> > by kernel so I wonder why we can't keep doing it, with tweaks for some
> > corner cases.
> 
> 
> It's probably not the corner case.
> 
> So my understanding is when a feature is turned on via command line, it
> should not be cleared silently otherwise we may break migration for sure.
> 
> E.g when packed=on is specified, we should disable vhost instead of clear it
> from the device.

If something is explicitly turned on by the user, they expect that feature
to be honoured, or an error to be raised.

If something is not explicitly turned on by the user, the behaviour wrt the
default should be stable for any given machine type version.

IOW, if you disable vhost by default when packed=on is set, then you can't
later switch to letting vhost be enabled with packed=on, unless you tie
that change to a new machine type.

If the user has explicitly said  packed=on *and* vhost=on, then should
must honour that, or raise an error if the combination is unsupportable.
Silently disabling vhost, then vhost=on is not ok.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 0/3] virtio-net: graceful drop of vhost for TAP

[Yuri Benditovich]

[-- Attachment #1: Type: text/plain, Size: 4104 bytes --]

On Thu, Feb 18, 2021 at 11:35 AM Daniel P. Berrangé <berrange@redhat.com>
wrote:

> On Wed, Feb 10, 2021 at 02:19:59PM +0800, Jason Wang wrote:
> >
> > On 2021/2/9 下午11:04, Michael S. Tsirkin wrote:
> > > On Tue, Feb 09, 2021 at 02:51:05PM +0000, Daniel P. Berrangé wrote:
> > > > On Tue, Feb 09, 2021 at 09:34:20AM -0500, Michael S. Tsirkin wrote:
> > > > > On Thu, Feb 04, 2021 at 10:29:12PM +0200, Yuri Benditovich wrote:
> > > > > > This set of patches introduces graceful switch from tap-vhost to
> > > > > > tap-no-vhost depending on guest features. Before that the
> features
> > > > > > that vhost does not support were silently cleared in
> get_features.
> > > > > > This creates potential problem of migration from the machine
> where
> > > > > > some of virtio-net features are supported by the vhost kernel to
> the
> > > > > > machine where they are not supported (packed ring as an example).
> > > > > I still worry that adding new features will silently disable vhost
> for people.
> > > > > Can we limit the change to when a VM is migrated in?
> > > > Some management applications expect bi-directional live migration to
> > > > work, so taking specific actions on incoming migration only feels
> > > > dangerous.
> > > Could you be more specific?
> > >
> > > Bi-directional migration is currently broken
> > > when migrating new kernel->old kernel.
> > >
> > > This seems to be the motivation for this patch, though I wish
> > > it was spelled out more explicitly.
> > >
> > > People don't complain much, but I'm fine with fixing that
> > > with a userspace fallback.
> > >
> > >
> > > I'd rather not force the fallback on others though: vhost is generally
> > > specified explicitly by user while features are generally set
> > > automatically, so this patch will make us override what user specified,
> > > not nice.
> > >
> > >
> > > > IMHO if the features we're adding cannot be expected to exist in
> > > > host kernels in general, then the feature should defualt to off
> > > > and require explicit user config to enable.
> > > > Downstream distros which can guarantee newer kernels can flip the
> > > > default in their custom machine types if they desire.
> > > >
> > > > Regards,
> > > > Daniel
> > > Unfortunately that will basically mean we are stuck with no new
> features
> > > for years. We did what this patch is trying to change for years now, in
> > > particular KVM also seems to happily disable CPU features not supported
> > > by kernel so I wonder why we can't keep doing it, with tweaks for some
> > > corner cases.
> >
> >
> > It's probably not the corner case.
> >
> > So my understanding is when a feature is turned on via command line, it
> > should not be cleared silently otherwise we may break migration for sure.
> >
> > E.g when packed=on is specified, we should disable vhost instead of
> clear it
> > from the device.
>
> If something is explicitly turned on by the user, they expect that feature
> to be honoured, or an error to be raised.
>
> If something is not explicitly turned on by the user, the behaviour wrt the
> default should be stable for any given machine type version.
>
> IOW, if you disable vhost by default when packed=on is set, then you can't
> later switch to letting vhost be enabled with packed=on, unless you tie
> that change to a new machine type.
>
> If the user has explicitly said  packed=on *and* vhost=on, then should
> must honour that, or raise an error if the combination is unsupportable.
> Silently disabling vhost, then vhost=on is not ok.
>

If I'm not mistaken:
Inside qemu there is no possibility to determine whether the user
explicitly turned vhost on.
For qemu the vhost is off by default but libvirt creates a new profile with
vhost on.


>
> Regards,
> Daniel
> --
> |: https://berrange.com      -o-
> https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org         -o-
> https://fstop138.berrange.com :|
> |: https://entangle-photo.org    -o-
> https://www.instagram.com/dberrange :|
>
>

[-- Attachment #2: Type: text/html, Size: 5674 bytes --]

Re: [PATCH 0/3] virtio-net: graceful drop of vhost for TAP

[Daniel P. Berrangé]

On Thu, Feb 18, 2021 at 09:55:25PM +0200, Yuri Benditovich wrote:
> On Thu, Feb 18, 2021 at 11:35 AM Daniel P. Berrangé <berrange@redhat.com>
> wrote:
> 
> > On Wed, Feb 10, 2021 at 02:19:59PM +0800, Jason Wang wrote:
> > >
> > > On 2021/2/9 下午11:04, Michael S. Tsirkin wrote:
> > > > On Tue, Feb 09, 2021 at 02:51:05PM +0000, Daniel P. Berrangé wrote:
> > > > > On Tue, Feb 09, 2021 at 09:34:20AM -0500, Michael S. Tsirkin wrote:
> > > > > > On Thu, Feb 04, 2021 at 10:29:12PM +0200, Yuri Benditovich wrote:
> > > > > > > This set of patches introduces graceful switch from tap-vhost to
> > > > > > > tap-no-vhost depending on guest features. Before that the
> > features
> > > > > > > that vhost does not support were silently cleared in
> > get_features.
> > > > > > > This creates potential problem of migration from the machine
> > where
> > > > > > > some of virtio-net features are supported by the vhost kernel to
> > the
> > > > > > > machine where they are not supported (packed ring as an example).
> > > > > > I still worry that adding new features will silently disable vhost
> > for people.
> > > > > > Can we limit the change to when a VM is migrated in?
> > > > > Some management applications expect bi-directional live migration to
> > > > > work, so taking specific actions on incoming migration only feels
> > > > > dangerous.
> > > > Could you be more specific?
> > > >
> > > > Bi-directional migration is currently broken
> > > > when migrating new kernel->old kernel.
> > > >
> > > > This seems to be the motivation for this patch, though I wish
> > > > it was spelled out more explicitly.
> > > >
> > > > People don't complain much, but I'm fine with fixing that
> > > > with a userspace fallback.
> > > >
> > > >
> > > > I'd rather not force the fallback on others though: vhost is generally
> > > > specified explicitly by user while features are generally set
> > > > automatically, so this patch will make us override what user specified,
> > > > not nice.
> > > >
> > > >
> > > > > IMHO if the features we're adding cannot be expected to exist in
> > > > > host kernels in general, then the feature should defualt to off
> > > > > and require explicit user config to enable.
> > > > > Downstream distros which can guarantee newer kernels can flip the
> > > > > default in their custom machine types if they desire.
> > > > >
> > > > > Regards,
> > > > > Daniel
> > > > Unfortunately that will basically mean we are stuck with no new
> > features
> > > > for years. We did what this patch is trying to change for years now, in
> > > > particular KVM also seems to happily disable CPU features not supported
> > > > by kernel so I wonder why we can't keep doing it, with tweaks for some
> > > > corner cases.
> > >
> > >
> > > It's probably not the corner case.
> > >
> > > So my understanding is when a feature is turned on via command line, it
> > > should not be cleared silently otherwise we may break migration for sure.
> > >
> > > E.g when packed=on is specified, we should disable vhost instead of
> > clear it
> > > from the device.
> >
> > If something is explicitly turned on by the user, they expect that feature
> > to be honoured, or an error to be raised.
> >
> > If something is not explicitly turned on by the user, the behaviour wrt the
> > default should be stable for any given machine type version.
> >
> > IOW, if you disable vhost by default when packed=on is set, then you can't
> > later switch to letting vhost be enabled with packed=on, unless you tie
> > that change to a new machine type.
> >
> > If the user has explicitly said  packed=on *and* vhost=on, then should
> > must honour that, or raise an error if the combination is unsupportable.
> > Silently disabling vhost, then vhost=on is not ok.
> >
> 
> If I'm not mistaken:
> Inside qemu there is no possibility to determine whether the user
> explicitly turned vhost on.
> For qemu the vhost is off by default but libvirt creates a new profile with
> vhost on.

Yes, libvirt will always attempt to enable vhost if it is present on te
host with /dev/vhost-net, except where the user explicitly told us not
to.


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 0/3] virtio-net: graceful drop of vhost for TAP

[Daniel P. Berrangé]

On Tue, Feb 09, 2021 at 10:04:30AM -0500, Michael S. Tsirkin wrote:
> On Tue, Feb 09, 2021 at 02:51:05PM +0000, Daniel P. Berrangé wrote:
> > On Tue, Feb 09, 2021 at 09:34:20AM -0500, Michael S. Tsirkin wrote:
> > > On Thu, Feb 04, 2021 at 10:29:12PM +0200, Yuri Benditovich wrote:
> > > > This set of patches introduces graceful switch from tap-vhost to
> > > > tap-no-vhost depending on guest features. Before that the features
> > > > that vhost does not support were silently cleared in get_features.
> > > > This creates potential problem of migration from the machine where
> > > > some of virtio-net features are supported by the vhost kernel to the
> > > > machine where they are not supported (packed ring as an example).
> > > 
> > > I still worry that adding new features will silently disable vhost for people.
> > > Can we limit the change to when a VM is migrated in?
> > 
> > Some management applications expect bi-directional live migration to
> > work, so taking specific actions on incoming migration only feels
> > dangerous. 
> 
> Could you be more specific?
> 
> Bi-directional migration is currently broken
> when migrating new kernel->old kernel.
> 
> This seems to be the motivation for this patch, though I wish
> it was spelled out more explicitly.
> 
> People don't complain much, but I'm fine with fixing that
> with a userspace fallback.
> 
> 
> I'd rather not force the fallback on others though: vhost is generally
> specified explicitly by user while features are generally set
> automatically, so this patch will make us override what user specified,
> not nice.
> 
> 
> > IMHO if the features we're adding cannot be expected to exist in
> > host kernels in general, then the feature should defualt to off
> > and require explicit user config to enable.
> > Downstream distros which can guarantee newer kernels can flip the
> > default in their custom machine types if they desire.
> > 
> > Regards,
> > Daniel
> 
> Unfortunately that will basically mean we are stuck with no new features
> for years. We did what this patch is trying to change for years now, in
> particular KVM also seems to happily disable CPU features not supported
> by kernel so I wonder why we can't keep doing it, with tweaks for some
> corner cases.

I should say the kernel's continual changing in CPU features that are
exposed has been responsible for a *huge* number of bugs with live
migration compatibility. libvirt, QEMU & apps have needed to introduce
a lot of extra code to try to cope with the changing CPU features across
migration and i still goes wrong to this very day, because we have to
migrate from prehistoric QEMU versions to quite modern versions.

IOW, the CPU features approach is a perfect example of why we should
*not* introduce a kernel dependancy in more areas of QEMU feature
enablement, and instead should strictly tie feature defaults to the
machine type versions.


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|