[PATCH v3 0/2] Fix the virtio features negotiation flaw

[PATCH v3 0/2] Fix the virtio features negotiation flaw

[huangy81]

From: Hyman Huang(黄勇) <huangy81@chinatelecom.cn>

v3:
-rebase on master
-code clean on [PATCH v2 1/2]: keep the commit self-consistent and
 do not modify the logic of saving acked_features. Just abstract the
 util function.
-modify the [PATCH v2 2/2] logic: change the behavior of saving
 acked_features in chr_closed_bh: saving acked_features only if
 features aren't 0. For the case of 0, we implement it in
 virtio_net_set_features function, which will save the acked_features
 in advance, including assign 0 to acked_features.

Thanks Michael for the comments and suggestions about the self-consistent
of commits. :)

Please review,

Yong

v2:
Fix the typo in subject of [PATCH v2 2/2] 

v1:
This is the version 1 of the series and it is exactly the same as
RFC version, but fixing a typo in subject, which is reported by Michael. 

As for test for the behavior suggested by Michael, IMHO, it could be
post in another series, since i found that testing the negotiation
behavior using QGraph Test Framework requires more work than i thought.

The test patch may implement the following logic...
1. Introduce a fresh new qmp command to query netdev info, which show
   the NetClient status including guest features and acked_features.
2. Using vhost-user QGraph Test to check the behavior of the vhost user
   protocol cmd VHOST_USER_SET_FEATURES. 
3. Adding acked_features into TestServer, which receive the features
   set by QEMU.
4. Compare the acked_feature in TestServer with the acked_features 
   in the output of qmp query command.

Anyway, idea above can be discussed in the future and any suggestion
are welcom. Let's fix the existing bug first, :)

Please review,

Yong

Patch for RFC can be found in the following:
https://patchew.org/QEMU/20220926063641.25038-1-huangy81@chinatelecom.cn/

This patchset aim to fix the unexpected negotiation features for
vhost-user netdev interface. 

Steps to reproduce the issue:
Prepare a vm (CentOS 8 in my work scenario) with vhost-user
backend interface and configure qemu as server mode. So dpdk
would connect qemu's unix socket periodically.

1. start vm in background and restart openvswitch service 
   concurrently and repeatedly in the process of vm start. 

2. check if negotiated virtio features of port is "0x40000000" at
   dpdk side by executing:
   ovs-vsctl list interface | grep features | grep {port_socket_path}
       
3. if features equals "0x40000000", go to the vm and check if sending 
   arp package works, executing:
   arping {IP_ADDR}
   if vm interface is configured to boot with dhcp protocol, it
   would get no ip. 

After doing above steps, we'll find the arping not work, the ovs on
host side has forwarded unexpected arp packages, which be added 0x0000
in the head of ethenet frame.  Though qemu report some error when
read/write cmd of vhost protocol during the process of vm start,
like the following:

"Failed to set msg fds"
"vhost VQ 0 ring restore failed: -22: Invalid argument (22)"

The vm does not stop or report more suggestive error message, it
seems that everthing is ok. 

The root cause is that dpdk port negotiated nothing but only one
VHOST_USER_F_PROTOCOL_FEATURES feature with vhost-user interface at
qemu side, which is an unexpected behavior. qemu only load the
VHOST_USER_F_PROTOCOL_FEATURES when VHOST_USER_SET_FEATURES and loss
the guest features configured by front-end virtio driver using the
VIRTIO_PCI_COMMON_GF addr, which is stored in acked_features field
of struct vhost_dev.

To explain how the acked_features disappear, we may need to know the
lifecyle of acked_features in vhost_dev during feature negotiation. 

1. qemu init acked_features field of struct vhost_dev in vhost_net_init()
   by calling vhost_net_ack_features(), the init value fetched from
   acked_features field of struct NetVhostUserState, which is the backup
   role after vhost stopping or unix socket closed.
   In the first time, the acked_features of struct NetVhostUserState is 0
   so the init value of vhost_dev's acked_features also 0. 

2. when guest virtio driver set features, qemu accept the features and
   call virtio_set_features to store the features as acked_features in
   vhost_dev.

3. when unix socket closed or vhost_dev device doesn't work and be
   stopped unexpectedly, qemu will call chr_closed_bh or vhost_user_stop,
   which will copy acked_features from vhost_dev to NetVhostUserState and
   cleanup the vhost_dev. Since virtio driver not allowed to set features
   once status of virtio device changes to VIRTIO_CONFIG_S_FEATURE_OK,
   qemu need to backup it in case of loss. 
    
4. once unix socket return to normal and get connected, qemu will
   call vhost_user_start to restore the vhost_dev and fetch the
   acked_features stored in NetVhostUserState previously. 

The above flow works fine in the normal scenarios, but it doesn't cover
the scenario that openvswitch service restart in the same time of
virtio features negotiation.

Let's analyze such scenario: 
       qemu                                 dpdk

   vhost_net_init()                          
         |                      systemctl stop openvswitch.service
   virtio_set_features()                     | 
         |                      systemctl start openvswitch.service
   virtio_set_status()                      

Ovs stop service before guset setting virtio features, chr_closed_bh()
be called and fetch acked_features in vhost_dev, if may store the
incomplete features to NetVhostUserState since it doesn't include
guest features, eg "0x40000000". 

Guest set virtio features with another features, eg "0x7060a782",
this value will store in acked_features of vhost_dev, which is the
right and up-to-date features.

After ovs service show up, qemu unix socket get connected and call
vhost_user_start(), which will restore acked_features of vhost_dev
using NetVhostUserState and "0x40000000" be loaded, which is obsolete.

Guest set virtio device status and therefore qemu call 
virtio_net_vhost_status finally, checking if vhost-net device has
started, start it if not, consequently the obsolete acked_features
"0x40000000" be negotiated after calling vhost_dev_set_features(). 

So the key point of solving this issue making the acked_features 
in NetVhostUserState up-to-date, these patchset provide this
solution.  

[PATCH 1/2]: Abstract the existing code of saving acked_features
             into vhost_user_save_acked_features so the next
             patch seems clean. 

[PATCH 2/2]: Save the acked_features to NetVhostUserState once
             Guest virtio driver configured. This step makes
             acked_features in NetVhostUserState up-to-date. 

Please review, any comments and suggestions are welcome. 

Best regard.

Yong

Hyman Huang (2):
  vhost-user: Refactor vhost acked features saving
  vhost-net: Fix the virtio features negotiation flaw

 hw/net/vhost_net.c       |  9 +++++++++
 hw/net/virtio-net.c      |  5 +++++
 include/net/vhost-user.h |  2 ++
 include/net/vhost_net.h  |  2 ++
 net/vhost-user.c         | 35 +++++++++++++++++++----------------
 5 files changed, 37 insertions(+), 16 deletions(-)

-- 
1.8.3.1

[PATCH v3 1/2] vhost-user: Refactor vhost acked features saving

[huangy81]

From: Hyman Huang(黄勇) <huangy81@chinatelecom.cn>

Abstract vhost acked features saving into
vhost_user_save_acked_features, export it as util function.

Signed-off-by: Hyman Huang(黄勇) <huangy81@chinatelecom.cn>
Signed-off-by: Guoyi Tu <tugy@chinatelecom.cn>
---
 include/net/vhost-user.h |  2 ++
 net/vhost-user.c         | 29 ++++++++++++++++++-----------
 2 files changed, 20 insertions(+), 11 deletions(-)

diff --git a/include/net/vhost-user.h b/include/net/vhost-user.h
index 5bcd8a6..00d4661 100644
--- a/include/net/vhost-user.h
+++ b/include/net/vhost-user.h
@@ -14,5 +14,7 @@
 struct vhost_net;
 struct vhost_net *vhost_user_get_vhost_net(NetClientState *nc);
 uint64_t vhost_user_get_acked_features(NetClientState *nc);
+void vhost_user_save_acked_features(NetClientState *nc,
+                                    bool cleanup);
 
 #endif /* VHOST_USER_H */
diff --git a/net/vhost-user.c b/net/vhost-user.c
index b1a0247..74f349c 100644
--- a/net/vhost-user.c
+++ b/net/vhost-user.c
@@ -45,24 +45,31 @@ uint64_t vhost_user_get_acked_features(NetClientState *nc)
     return s->acked_features;
 }
 
-static void vhost_user_stop(int queues, NetClientState *ncs[])
+void vhost_user_save_acked_features(NetClientState *nc, bool cleanup)
 {
     NetVhostUserState *s;
+
+    s = DO_UPCAST(NetVhostUserState, nc, nc);
+    if (s->vhost_net) {
+        uint64_t features = vhost_net_get_acked_features(s->vhost_net);
+        if (features) {
+            s->acked_features = features;
+        }
+
+        if (cleanup) {
+            vhost_net_cleanup(s->vhost_net);
+        }
+    }
+}
+
+static void vhost_user_stop(int queues, NetClientState *ncs[])
+{
     int i;
 
     for (i = 0; i < queues; i++) {
         assert(ncs[i]->info->type == NET_CLIENT_DRIVER_VHOST_USER);
 
-        s = DO_UPCAST(NetVhostUserState, nc, ncs[i]);
-
-        if (s->vhost_net) {
-            /* save acked features */
-            uint64_t features = vhost_net_get_acked_features(s->vhost_net);
-            if (features) {
-                s->acked_features = features;
-            }
-            vhost_net_cleanup(s->vhost_net);
-        }
+        vhost_user_save_acked_features(ncs[i], true);
     }
 }
 
-- 
1.8.3.1

[PATCH v3 2/2] vhost-net: Fix the virtio features negotiation flaw

[huangy81]

From: Hyman Huang(黄勇) <huangy81@chinatelecom.cn>

Save the acked_features once it be configured by guest
virtio driver so it can't miss any features.

Note that this patch also change the features saving logic
in chr_closed_bh, which originally backup features no matter
whether the features are 0 or not, but now do it only if
features aren't 0.

As to reset acked_features to 0 if needed, Qemu always
keeping the backup acked_features up-to-date, and save the
acked_features after virtio_net_set_features in advance,
including reset acked_features to 0, so the behavior is
also covered.

Signed-off-by: Hyman Huang(黄勇) <huangy81@chinatelecom.cn>
Signed-off-by: Guoyi Tu <tugy@chinatelecom.cn>
---
 hw/net/vhost_net.c      | 9 +++++++++
 hw/net/virtio-net.c     | 5 +++++
 include/net/vhost_net.h | 2 ++
 net/vhost-user.c        | 6 +-----
 4 files changed, 17 insertions(+), 5 deletions(-)

diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
index d28f8b9..2bffc27 100644
--- a/hw/net/vhost_net.c
+++ b/hw/net/vhost_net.c
@@ -141,6 +141,15 @@ uint64_t vhost_net_get_acked_features(VHostNetState *net)
     return net->dev.acked_features;
 }
 
+void vhost_net_save_acked_features(NetClientState *nc)
+{
+    if (nc->info->type != NET_CLIENT_DRIVER_VHOST_USER) {
+        return;
+    }
+
+    vhost_user_save_acked_features(nc, false);
+}
+
 static int vhost_net_get_fd(NetClientState *backend)
 {
     switch (backend->info->type) {
diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index e9f696b..5f8f788 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -924,6 +924,11 @@ static void virtio_net_set_features(VirtIODevice *vdev, uint64_t features)
             continue;
         }
         vhost_net_ack_features(get_vhost_net(nc->peer), features);
+        /*
+         * keep acked_features in NetVhostUserState up-to-date so it
+         * can't miss any features configured by guest virtio driver.
+         */
+        vhost_net_save_acked_features(nc->peer);
     }
 
     if (virtio_has_feature(features, VIRTIO_NET_F_CTRL_VLAN)) {
diff --git a/include/net/vhost_net.h b/include/net/vhost_net.h
index 387e913..3a5579b 100644
--- a/include/net/vhost_net.h
+++ b/include/net/vhost_net.h
@@ -46,6 +46,8 @@ int vhost_set_vring_enable(NetClientState * nc, int enable);
 
 uint64_t vhost_net_get_acked_features(VHostNetState *net);
 
+void vhost_net_save_acked_features(NetClientState *nc);
+
 int vhost_net_set_mtu(struct vhost_net *net, uint16_t mtu);
 
 #endif
diff --git a/net/vhost-user.c b/net/vhost-user.c
index 74f349c..c512cc9 100644
--- a/net/vhost-user.c
+++ b/net/vhost-user.c
@@ -258,11 +258,7 @@ static void chr_closed_bh(void *opaque)
     s = DO_UPCAST(NetVhostUserState, nc, ncs[0]);
 
     for (i = queues -1; i >= 0; i--) {
-        s = DO_UPCAST(NetVhostUserState, nc, ncs[i]);
-
-        if (s->vhost_net) {
-            s->acked_features = vhost_net_get_acked_features(s->vhost_net);
-        }
+        vhost_user_save_acked_features(ncs[i], false);
     }
 
     qmp_set_link(name, false, &err);
-- 
1.8.3.1

Re: [PATCH v3 0/2] Fix the virtio features negotiation flaw

[Michael S. Tsirkin]

On Sun, Oct 30, 2022 at 09:52:37PM +0800, huangy81@chinatelecom.cn wrote:
> From: Hyman Huang(黄勇) <huangy81@chinatelecom.cn>
> 
> v3:
> -rebase on master
> -code clean on [PATCH v2 1/2]: keep the commit self-consistent and
>  do not modify the logic of saving acked_features. Just abstract the
>  util function.
> -modify the [PATCH v2 2/2] logic: change the behavior of saving
>  acked_features in chr_closed_bh: saving acked_features only if
>  features aren't 0. For the case of 0, we implement it in
>  virtio_net_set_features function, which will save the acked_features
>  in advance, including assign 0 to acked_features.
> 
> Thanks Michael for the comments and suggestions about the self-consistent
> of commits. :)


This breaks multiple build configs:

https://gitlab.com/mstredhat/qemu/-/pipelines/691382555/failures


> Please review,
> 
> Yong
> 
> v2:
> Fix the typo in subject of [PATCH v2 2/2] 
> 
> v1:
> This is the version 1 of the series and it is exactly the same as
> RFC version, but fixing a typo in subject, which is reported by Michael. 
> 
> As for test for the behavior suggested by Michael, IMHO, it could be
> post in another series, since i found that testing the negotiation
> behavior using QGraph Test Framework requires more work than i thought.
> 
> The test patch may implement the following logic...
> 1. Introduce a fresh new qmp command to query netdev info, which show
>    the NetClient status including guest features and acked_features.
> 2. Using vhost-user QGraph Test to check the behavior of the vhost user
>    protocol cmd VHOST_USER_SET_FEATURES. 
> 3. Adding acked_features into TestServer, which receive the features
>    set by QEMU.
> 4. Compare the acked_feature in TestServer with the acked_features 
>    in the output of qmp query command.
> 
> Anyway, idea above can be discussed in the future and any suggestion
> are welcom. Let's fix the existing bug first, :)
> 
> Please review,
> 
> Yong
> 
> Patch for RFC can be found in the following:
> https://patchew.org/QEMU/20220926063641.25038-1-huangy81@chinatelecom.cn/
> 
> This patchset aim to fix the unexpected negotiation features for
> vhost-user netdev interface. 
> 
> Steps to reproduce the issue:
> Prepare a vm (CentOS 8 in my work scenario) with vhost-user
> backend interface and configure qemu as server mode. So dpdk
> would connect qemu's unix socket periodically.
> 
> 1. start vm in background and restart openvswitch service 
>    concurrently and repeatedly in the process of vm start. 
> 
> 2. check if negotiated virtio features of port is "0x40000000" at
>    dpdk side by executing:
>    ovs-vsctl list interface | grep features | grep {port_socket_path}
>        
> 3. if features equals "0x40000000", go to the vm and check if sending 
>    arp package works, executing:
>    arping {IP_ADDR}
>    if vm interface is configured to boot with dhcp protocol, it
>    would get no ip. 
> 
> After doing above steps, we'll find the arping not work, the ovs on
> host side has forwarded unexpected arp packages, which be added 0x0000
> in the head of ethenet frame.  Though qemu report some error when
> read/write cmd of vhost protocol during the process of vm start,
> like the following:
> 
> "Failed to set msg fds"
> "vhost VQ 0 ring restore failed: -22: Invalid argument (22)"
> 
> The vm does not stop or report more suggestive error message, it
> seems that everthing is ok. 
> 
> The root cause is that dpdk port negotiated nothing but only one
> VHOST_USER_F_PROTOCOL_FEATURES feature with vhost-user interface at
> qemu side, which is an unexpected behavior. qemu only load the
> VHOST_USER_F_PROTOCOL_FEATURES when VHOST_USER_SET_FEATURES and loss
> the guest features configured by front-end virtio driver using the
> VIRTIO_PCI_COMMON_GF addr, which is stored in acked_features field
> of struct vhost_dev.
> 
> To explain how the acked_features disappear, we may need to know the
> lifecyle of acked_features in vhost_dev during feature negotiation. 
> 
> 1. qemu init acked_features field of struct vhost_dev in vhost_net_init()
>    by calling vhost_net_ack_features(), the init value fetched from
>    acked_features field of struct NetVhostUserState, which is the backup
>    role after vhost stopping or unix socket closed.
>    In the first time, the acked_features of struct NetVhostUserState is 0
>    so the init value of vhost_dev's acked_features also 0. 
> 
> 2. when guest virtio driver set features, qemu accept the features and
>    call virtio_set_features to store the features as acked_features in
>    vhost_dev.
> 
> 3. when unix socket closed or vhost_dev device doesn't work and be
>    stopped unexpectedly, qemu will call chr_closed_bh or vhost_user_stop,
>    which will copy acked_features from vhost_dev to NetVhostUserState and
>    cleanup the vhost_dev. Since virtio driver not allowed to set features
>    once status of virtio device changes to VIRTIO_CONFIG_S_FEATURE_OK,
>    qemu need to backup it in case of loss. 
>     
> 4. once unix socket return to normal and get connected, qemu will
>    call vhost_user_start to restore the vhost_dev and fetch the
>    acked_features stored in NetVhostUserState previously. 
> 
> The above flow works fine in the normal scenarios, but it doesn't cover
> the scenario that openvswitch service restart in the same time of
> virtio features negotiation.
> 
> Let's analyze such scenario: 
>        qemu                                 dpdk
> 
>    vhost_net_init()                          
>          |                      systemctl stop openvswitch.service
>    virtio_set_features()                     | 
>          |                      systemctl start openvswitch.service
>    virtio_set_status()                      
> 
> Ovs stop service before guset setting virtio features, chr_closed_bh()
> be called and fetch acked_features in vhost_dev, if may store the
> incomplete features to NetVhostUserState since it doesn't include
> guest features, eg "0x40000000". 
> 
> Guest set virtio features with another features, eg "0x7060a782",
> this value will store in acked_features of vhost_dev, which is the
> right and up-to-date features.
> 
> After ovs service show up, qemu unix socket get connected and call
> vhost_user_start(), which will restore acked_features of vhost_dev
> using NetVhostUserState and "0x40000000" be loaded, which is obsolete.
> 
> Guest set virtio device status and therefore qemu call 
> virtio_net_vhost_status finally, checking if vhost-net device has
> started, start it if not, consequently the obsolete acked_features
> "0x40000000" be negotiated after calling vhost_dev_set_features(). 
> 
> So the key point of solving this issue making the acked_features 
> in NetVhostUserState up-to-date, these patchset provide this
> solution.  
> 
> [PATCH 1/2]: Abstract the existing code of saving acked_features
>              into vhost_user_save_acked_features so the next
>              patch seems clean. 
> 
> [PATCH 2/2]: Save the acked_features to NetVhostUserState once
>              Guest virtio driver configured. This step makes
>              acked_features in NetVhostUserState up-to-date. 
> 
> Please review, any comments and suggestions are welcome. 
> 
> Best regard.
> 
> Yong
> 
> Hyman Huang (2):
>   vhost-user: Refactor vhost acked features saving
>   vhost-net: Fix the virtio features negotiation flaw
> 
>  hw/net/vhost_net.c       |  9 +++++++++
>  hw/net/virtio-net.c      |  5 +++++
>  include/net/vhost-user.h |  2 ++
>  include/net/vhost_net.h  |  2 ++
>  net/vhost-user.c         | 35 +++++++++++++++++++----------------
>  5 files changed, 37 insertions(+), 16 deletions(-)
> 
> -- 
> 1.8.3.1

Re: [PATCH v3 1/2] vhost-user: Refactor vhost acked features saving

[Michael S. Tsirkin]

On Sun, Oct 30, 2022 at 09:52:38PM +0800, huangy81@chinatelecom.cn wrote:
> From: Hyman Huang(黄勇) <huangy81@chinatelecom.cn>
> 
> Abstract vhost acked features saving into
> vhost_user_save_acked_features, export it as util function.
> 
> Signed-off-by: Hyman Huang(黄勇) <huangy81@chinatelecom.cn>
> Signed-off-by: Guoyi Tu <tugy@chinatelecom.cn>
> ---
>  include/net/vhost-user.h |  2 ++
>  net/vhost-user.c         | 29 ++++++++++++++++++-----------
>  2 files changed, 20 insertions(+), 11 deletions(-)
> 
> diff --git a/include/net/vhost-user.h b/include/net/vhost-user.h
> index 5bcd8a6..00d4661 100644
> --- a/include/net/vhost-user.h
> +++ b/include/net/vhost-user.h
> @@ -14,5 +14,7 @@
>  struct vhost_net;
>  struct vhost_net *vhost_user_get_vhost_net(NetClientState *nc);
>  uint64_t vhost_user_get_acked_features(NetClientState *nc);
> +void vhost_user_save_acked_features(NetClientState *nc,
> +                                    bool cleanup);
>  
>  #endif /* VHOST_USER_H */
> diff --git a/net/vhost-user.c b/net/vhost-user.c
> index b1a0247..74f349c 100644
> --- a/net/vhost-user.c
> +++ b/net/vhost-user.c
> @@ -45,24 +45,31 @@ uint64_t vhost_user_get_acked_features(NetClientState *nc)
>      return s->acked_features;
>  }
>  
> -static void vhost_user_stop(int queues, NetClientState *ncs[])
> +void vhost_user_save_acked_features(NetClientState *nc, bool cleanup)
>  {
>      NetVhostUserState *s;
> +
> +    s = DO_UPCAST(NetVhostUserState, nc, nc);
> +    if (s->vhost_net) {
> +        uint64_t features = vhost_net_get_acked_features(s->vhost_net);
> +        if (features) {
> +            s->acked_features = features;
> +        }
> +
> +        if (cleanup) {
> +            vhost_net_cleanup(s->vhost_net);
> +        }
> +    }
> +}

I can't figure out what is going on here.  Why is there a cleanup flag?
What does cleanup have to do with saving acked features?
I suspect it's better to just leave this part in the caller.

> +
> +static void vhost_user_stop(int queues, NetClientState *ncs[])
> +{
>      int i;
>  
>      for (i = 0; i < queues; i++) {
>          assert(ncs[i]->info->type == NET_CLIENT_DRIVER_VHOST_USER);
>  
> -        s = DO_UPCAST(NetVhostUserState, nc, ncs[i]);
> -
> -        if (s->vhost_net) {
> -            /* save acked features */
> -            uint64_t features = vhost_net_get_acked_features(s->vhost_net);
> -            if (features) {
> -                s->acked_features = features;
> -            }
> -            vhost_net_cleanup(s->vhost_net);
> -        }
> +        vhost_user_save_acked_features(ncs[i], true);
>      }
>  }
>  
> -- 
> 1.8.3.1

Re: [PATCH v3 2/2] vhost-net: Fix the virtio features negotiation flaw

[Michael S. Tsirkin]

On Sun, Oct 30, 2022 at 09:52:39PM +0800, huangy81@chinatelecom.cn wrote:
> From: Hyman Huang(黄勇) <huangy81@chinatelecom.cn>
> 
> Save the acked_features once it be configured by guest
> virtio driver so it can't miss any features.
> 
> Note that this patch also change the features saving logic
> in chr_closed_bh, which originally backup features no matter
> whether the features are 0 or not, but now do it only if
> features aren't 0.

I'm not sure how is this change even related to what we
are trying to do (fix a bug). Explain here?



> As to reset acked_features to 0 if needed, Qemu always
> keeping the backup acked_features up-to-date, and save the
> acked_features after virtio_net_set_features in advance,
> including reset acked_features to 0, so the behavior is
> also covered.
> 
> Signed-off-by: Hyman Huang(黄勇) <huangy81@chinatelecom.cn>
> Signed-off-by: Guoyi Tu <tugy@chinatelecom.cn>
> ---
>  hw/net/vhost_net.c      | 9 +++++++++
>  hw/net/virtio-net.c     | 5 +++++
>  include/net/vhost_net.h | 2 ++
>  net/vhost-user.c        | 6 +-----
>  4 files changed, 17 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
> index d28f8b9..2bffc27 100644
> --- a/hw/net/vhost_net.c
> +++ b/hw/net/vhost_net.c
> @@ -141,6 +141,15 @@ uint64_t vhost_net_get_acked_features(VHostNetState *net)
>      return net->dev.acked_features;
>  }
>  
> +void vhost_net_save_acked_features(NetClientState *nc)
> +{
> +    if (nc->info->type != NET_CLIENT_DRIVER_VHOST_USER) {
> +        return;
> +    }
> +
> +    vhost_user_save_acked_features(nc, false);
> +}
> +
>  static int vhost_net_get_fd(NetClientState *backend)
>  {
>      switch (backend->info->type) {
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index e9f696b..5f8f788 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -924,6 +924,11 @@ static void virtio_net_set_features(VirtIODevice *vdev, uint64_t features)
>              continue;
>          }
>          vhost_net_ack_features(get_vhost_net(nc->peer), features);
> +        /*
> +         * keep acked_features in NetVhostUserState up-to-date so it
> +         * can't miss any features configured by guest virtio driver.
> +         */
> +        vhost_net_save_acked_features(nc->peer);
>      }
>  
>      if (virtio_has_feature(features, VIRTIO_NET_F_CTRL_VLAN)) {
> diff --git a/include/net/vhost_net.h b/include/net/vhost_net.h
> index 387e913..3a5579b 100644
> --- a/include/net/vhost_net.h
> +++ b/include/net/vhost_net.h
> @@ -46,6 +46,8 @@ int vhost_set_vring_enable(NetClientState * nc, int enable);
>  
>  uint64_t vhost_net_get_acked_features(VHostNetState *net);
>  
> +void vhost_net_save_acked_features(NetClientState *nc);
> +
>  int vhost_net_set_mtu(struct vhost_net *net, uint16_t mtu);
>  
>  #endif
> diff --git a/net/vhost-user.c b/net/vhost-user.c
> index 74f349c..c512cc9 100644
> --- a/net/vhost-user.c
> +++ b/net/vhost-user.c
> @@ -258,11 +258,7 @@ static void chr_closed_bh(void *opaque)
>      s = DO_UPCAST(NetVhostUserState, nc, ncs[0]);
>  
>      for (i = queues -1; i >= 0; i--) {
> -        s = DO_UPCAST(NetVhostUserState, nc, ncs[i]);
> -
> -        if (s->vhost_net) {
> -            s->acked_features = vhost_net_get_acked_features(s->vhost_net);
> -        }
> +        vhost_user_save_acked_features(ncs[i], false);
>      }
>  
>      qmp_set_link(name, false, &err);


Split this last chunk into a patch of its own?

> -- 
> 1.8.3.1

Re: [PATCH v3 2/2] vhost-net: Fix the virtio features negotiation flaw

[Michael S. Tsirkin]

On Sun, Oct 30, 2022 at 09:52:39PM +0800, huangy81@chinatelecom.cn wrote:
> From: Hyman Huang(黄勇) <huangy81@chinatelecom.cn>
> 
> Save the acked_features once it be configured by guest
> virtio driver so it can't miss any features.
> 
> Note that this patch also change the features saving logic
> in chr_closed_bh, which originally backup features no matter
> whether the features are 0 or not, but now do it only if
> features aren't 0.
> 
> As to reset acked_features to 0 if needed, Qemu always
> keeping the backup acked_features up-to-date, and save the
> acked_features after virtio_net_set_features in advance,
> including reset acked_features to 0, so the behavior is
> also covered.
> 
> Signed-off-by: Hyman Huang(黄勇) <huangy81@chinatelecom.cn>
> Signed-off-by: Guoyi Tu <tugy@chinatelecom.cn>
> ---
>  hw/net/vhost_net.c      | 9 +++++++++
>  hw/net/virtio-net.c     | 5 +++++
>  include/net/vhost_net.h | 2 ++
>  net/vhost-user.c        | 6 +-----
>  4 files changed, 17 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
> index d28f8b9..2bffc27 100644
> --- a/hw/net/vhost_net.c
> +++ b/hw/net/vhost_net.c
> @@ -141,6 +141,15 @@ uint64_t vhost_net_get_acked_features(VHostNetState *net)
>      return net->dev.acked_features;
>  }
>  
> +void vhost_net_save_acked_features(NetClientState *nc)
> +{
> +    if (nc->info->type != NET_CLIENT_DRIVER_VHOST_USER) {
> +        return;
> +    }
> +
> +    vhost_user_save_acked_features(nc, false);
> +}
> +
>  static int vhost_net_get_fd(NetClientState *backend)
>  {
>      switch (backend->info->type) {
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index e9f696b..5f8f788 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -924,6 +924,11 @@ static void virtio_net_set_features(VirtIODevice *vdev, uint64_t features)
>              continue;
>          }
>          vhost_net_ack_features(get_vhost_net(nc->peer), features);
> +        /*
> +         * keep acked_features in NetVhostUserState up-to-date so it
> +         * can't miss any features configured by guest virtio driver.
> +         */
> +        vhost_net_save_acked_features(nc->peer);
>      }
>  
>      if (virtio_has_feature(features, VIRTIO_NET_F_CTRL_VLAN)) {

So when do you want to ack features but *not* save them?

Is the effect of this patch, fundamentally, that guest features
from virtio are always copied to vhost-user?
Do we even need an extra copy in vhost user then?


all this came in with:

commit a463215b087c41d7ca94e51aa347cde523831873
Author: Marc-André Lureau <marcandre.lureau@redhat.com>
Date:   Mon Jun 6 18:45:05 2016 +0200

    vhost-net: save & restore vhost-user acked features

Marc-André do you remember why we have a copy of features in vhost-user
and not just reuse the features from virtio?


> diff --git a/include/net/vhost_net.h b/include/net/vhost_net.h
> index 387e913..3a5579b 100644
> --- a/include/net/vhost_net.h
> +++ b/include/net/vhost_net.h
> @@ -46,6 +46,8 @@ int vhost_set_vring_enable(NetClientState * nc, int enable);
>  
>  uint64_t vhost_net_get_acked_features(VHostNetState *net);
>  
> +void vhost_net_save_acked_features(NetClientState *nc);
> +
>  int vhost_net_set_mtu(struct vhost_net *net, uint16_t mtu);
>  
>  #endif
> diff --git a/net/vhost-user.c b/net/vhost-user.c
> index 74f349c..c512cc9 100644
> --- a/net/vhost-user.c
> +++ b/net/vhost-user.c
> @@ -258,11 +258,7 @@ static void chr_closed_bh(void *opaque)
>      s = DO_UPCAST(NetVhostUserState, nc, ncs[0]);
>  
>      for (i = queues -1; i >= 0; i--) {
> -        s = DO_UPCAST(NetVhostUserState, nc, ncs[i]);
> -
> -        if (s->vhost_net) {
> -            s->acked_features = vhost_net_get_acked_features(s->vhost_net);
> -        }
> +        vhost_user_save_acked_features(ncs[i], false);
>      }
>  
>      qmp_set_link(name, false, &err);
> -- 
> 1.8.3.1

Re: [PATCH v3 0/2] Fix the virtio features negotiation flaw

[Hyman Huang]

在 2022/11/11 2:53, Michael S. Tsirkin 写道:
> On Sun, Oct 30, 2022 at 09:52:37PM +0800, huangy81@chinatelecom.cn wrote:
>> From: Hyman Huang(黄勇) <huangy81@chinatelecom.cn>
>>
>> v3:
>> -rebase on master
>> -code clean on [PATCH v2 1/2]: keep the commit self-consistent and
>>   do not modify the logic of saving acked_features. Just abstract the
>>   util function.
>> -modify the [PATCH v2 2/2] logic: change the behavior of saving
>>   acked_features in chr_closed_bh: saving acked_features only if
>>   features aren't 0. For the case of 0, we implement it in
>>   virtio_net_set_features function, which will save the acked_features
>>   in advance, including assign 0 to acked_features.
>>
>> Thanks Michael for the comments and suggestions about the self-consistent
>> of commits. :)
> 
> 
> This breaks multiple build configs:
> 
> https://gitlab.com/mstredhat/qemu/-/pipelines/691382555/failures

Thanks for pointing out the error, i'll add the stub function next version.
> 
> 
>> Please review,
>>
>> Yong
>>
>> v2:
>> Fix the typo in subject of [PATCH v2 2/2]
>>
>> v1:
>> This is the version 1 of the series and it is exactly the same as
>> RFC version, but fixing a typo in subject, which is reported by Michael.
>>
>> As for test for the behavior suggested by Michael, IMHO, it could be
>> post in another series, since i found that testing the negotiation
>> behavior using QGraph Test Framework requires more work than i thought.
>>
>> The test patch may implement the following logic...
>> 1. Introduce a fresh new qmp command to query netdev info, which show
>>     the NetClient status including guest features and acked_features.
>> 2. Using vhost-user QGraph Test to check the behavior of the vhost user
>>     protocol cmd VHOST_USER_SET_FEATURES.
>> 3. Adding acked_features into TestServer, which receive the features
>>     set by QEMU.
>> 4. Compare the acked_feature in TestServer with the acked_features
>>     in the output of qmp query command.
>>
>> Anyway, idea above can be discussed in the future and any suggestion
>> are welcom. Let's fix the existing bug first, :)
>>
>> Please review,
>>
>> Yong
>>
>> Patch for RFC can be found in the following:
>> https://patchew.org/QEMU/20220926063641.25038-1-huangy81@chinatelecom.cn/
>>
>> This patchset aim to fix the unexpected negotiation features for
>> vhost-user netdev interface.
>>
>> Steps to reproduce the issue:
>> Prepare a vm (CentOS 8 in my work scenario) with vhost-user
>> backend interface and configure qemu as server mode. So dpdk
>> would connect qemu's unix socket periodically.
>>
>> 1. start vm in background and restart openvswitch service
>>     concurrently and repeatedly in the process of vm start.
>>
>> 2. check if negotiated virtio features of port is "0x40000000" at
>>     dpdk side by executing:
>>     ovs-vsctl list interface | grep features | grep {port_socket_path}
>>         
>> 3. if features equals "0x40000000", go to the vm and check if sending
>>     arp package works, executing:
>>     arping {IP_ADDR}
>>     if vm interface is configured to boot with dhcp protocol, it
>>     would get no ip.
>>
>> After doing above steps, we'll find the arping not work, the ovs on
>> host side has forwarded unexpected arp packages, which be added 0x0000
>> in the head of ethenet frame.  Though qemu report some error when
>> read/write cmd of vhost protocol during the process of vm start,
>> like the following:
>>
>> "Failed to set msg fds"
>> "vhost VQ 0 ring restore failed: -22: Invalid argument (22)"
>>
>> The vm does not stop or report more suggestive error message, it
>> seems that everthing is ok.
>>
>> The root cause is that dpdk port negotiated nothing but only one
>> VHOST_USER_F_PROTOCOL_FEATURES feature with vhost-user interface at
>> qemu side, which is an unexpected behavior. qemu only load the
>> VHOST_USER_F_PROTOCOL_FEATURES when VHOST_USER_SET_FEATURES and loss
>> the guest features configured by front-end virtio driver using the
>> VIRTIO_PCI_COMMON_GF addr, which is stored in acked_features field
>> of struct vhost_dev.
>>
>> To explain how the acked_features disappear, we may need to know the
>> lifecyle of acked_features in vhost_dev during feature negotiation.
>>
>> 1. qemu init acked_features field of struct vhost_dev in vhost_net_init()
>>     by calling vhost_net_ack_features(), the init value fetched from
>>     acked_features field of struct NetVhostUserState, which is the backup
>>     role after vhost stopping or unix socket closed.
>>     In the first time, the acked_features of struct NetVhostUserState is 0
>>     so the init value of vhost_dev's acked_features also 0.
>>
>> 2. when guest virtio driver set features, qemu accept the features and
>>     call virtio_set_features to store the features as acked_features in
>>     vhost_dev.
>>
>> 3. when unix socket closed or vhost_dev device doesn't work and be
>>     stopped unexpectedly, qemu will call chr_closed_bh or vhost_user_stop,
>>     which will copy acked_features from vhost_dev to NetVhostUserState and
>>     cleanup the vhost_dev. Since virtio driver not allowed to set features
>>     once status of virtio device changes to VIRTIO_CONFIG_S_FEATURE_OK,
>>     qemu need to backup it in case of loss.
>>      
>> 4. once unix socket return to normal and get connected, qemu will
>>     call vhost_user_start to restore the vhost_dev and fetch the
>>     acked_features stored in NetVhostUserState previously.
>>
>> The above flow works fine in the normal scenarios, but it doesn't cover
>> the scenario that openvswitch service restart in the same time of
>> virtio features negotiation.
>>
>> Let's analyze such scenario:
>>         qemu                                 dpdk
>>
>>     vhost_net_init()
>>           |                      systemctl stop openvswitch.service
>>     virtio_set_features()                     |
>>           |                      systemctl start openvswitch.service
>>     virtio_set_status()
>>
>> Ovs stop service before guset setting virtio features, chr_closed_bh()
>> be called and fetch acked_features in vhost_dev, if may store the
>> incomplete features to NetVhostUserState since it doesn't include
>> guest features, eg "0x40000000".
>>
>> Guest set virtio features with another features, eg "0x7060a782",
>> this value will store in acked_features of vhost_dev, which is the
>> right and up-to-date features.
>>
>> After ovs service show up, qemu unix socket get connected and call
>> vhost_user_start(), which will restore acked_features of vhost_dev
>> using NetVhostUserState and "0x40000000" be loaded, which is obsolete.
>>
>> Guest set virtio device status and therefore qemu call
>> virtio_net_vhost_status finally, checking if vhost-net device has
>> started, start it if not, consequently the obsolete acked_features
>> "0x40000000" be negotiated after calling vhost_dev_set_features().
>>
>> So the key point of solving this issue making the acked_features
>> in NetVhostUserState up-to-date, these patchset provide this
>> solution.
>>
>> [PATCH 1/2]: Abstract the existing code of saving acked_features
>>               into vhost_user_save_acked_features so the next
>>               patch seems clean.
>>
>> [PATCH 2/2]: Save the acked_features to NetVhostUserState once
>>               Guest virtio driver configured. This step makes
>>               acked_features in NetVhostUserState up-to-date.
>>
>> Please review, any comments and suggestions are welcome.
>>
>> Best regard.
>>
>> Yong
>>
>> Hyman Huang (2):
>>    vhost-user: Refactor vhost acked features saving
>>    vhost-net: Fix the virtio features negotiation flaw
>>
>>   hw/net/vhost_net.c       |  9 +++++++++
>>   hw/net/virtio-net.c      |  5 +++++
>>   include/net/vhost-user.h |  2 ++
>>   include/net/vhost_net.h  |  2 ++
>>   net/vhost-user.c         | 35 +++++++++++++++++++----------------
>>   5 files changed, 37 insertions(+), 16 deletions(-)
>>
>> -- 
>> 1.8.3.1
> 

-- 
Best regard

Hyman Huang(黄勇)

Re: [PATCH v3 1/2] vhost-user: Refactor vhost acked features saving

[Hyman Huang]

在 2022/11/11 2:56, Michael S. Tsirkin 写道:
> On Sun, Oct 30, 2022 at 09:52:38PM +0800, huangy81@chinatelecom.cn wrote:
>> From: Hyman Huang(黄勇) <huangy81@chinatelecom.cn>
>>
>> Abstract vhost acked features saving into
>> vhost_user_save_acked_features, export it as util function.
>>
>> Signed-off-by: Hyman Huang(黄勇) <huangy81@chinatelecom.cn>
>> Signed-off-by: Guoyi Tu <tugy@chinatelecom.cn>
>> ---
>>   include/net/vhost-user.h |  2 ++
>>   net/vhost-user.c         | 29 ++++++++++++++++++-----------
>>   2 files changed, 20 insertions(+), 11 deletions(-)
>>
>> diff --git a/include/net/vhost-user.h b/include/net/vhost-user.h
>> index 5bcd8a6..00d4661 100644
>> --- a/include/net/vhost-user.h
>> +++ b/include/net/vhost-user.h
>> @@ -14,5 +14,7 @@
>>   struct vhost_net;
>>   struct vhost_net *vhost_user_get_vhost_net(NetClientState *nc);
>>   uint64_t vhost_user_get_acked_features(NetClientState *nc);
>> +void vhost_user_save_acked_features(NetClientState *nc,
>> +                                    bool cleanup);
>>   
>>   #endif /* VHOST_USER_H */
>> diff --git a/net/vhost-user.c b/net/vhost-user.c
>> index b1a0247..74f349c 100644
>> --- a/net/vhost-user.c
>> +++ b/net/vhost-user.c
>> @@ -45,24 +45,31 @@ uint64_t vhost_user_get_acked_features(NetClientState *nc)
>>       return s->acked_features;
>>   }
>>   
>> -static void vhost_user_stop(int queues, NetClientState *ncs[])
>> +void vhost_user_save_acked_features(NetClientState *nc, bool cleanup)
>>   {
>>       NetVhostUserState *s;
>> +
>> +    s = DO_UPCAST(NetVhostUserState, nc, nc);
>> +    if (s->vhost_net) {
>> +        uint64_t features = vhost_net_get_acked_features(s->vhost_net);
>> +        if (features) {
>> +            s->acked_features = features;
>> +        }
>> +
>> +        if (cleanup) {
>> +            vhost_net_cleanup(s->vhost_net);
>> +        }
>> +    }
>> +}
> 
> I can't figure out what is going on here.  Why is there a cleanup flag?
> What does cleanup have to do with saving acked features?
> I suspect it's better to just leave this part in the caller.
> 
Indeed, i'll adjust the logic next version.
>> +
>> +static void vhost_user_stop(int queues, NetClientState *ncs[])
>> +{
>>       int i;
>>   
>>       for (i = 0; i < queues; i++) {
>>           assert(ncs[i]->info->type == NET_CLIENT_DRIVER_VHOST_USER);
>>   
>> -        s = DO_UPCAST(NetVhostUserState, nc, ncs[i]);
>> -
>> -        if (s->vhost_net) {
>> -            /* save acked features */
>> -            uint64_t features = vhost_net_get_acked_features(s->vhost_net);
>> -            if (features) {
>> -                s->acked_features = features;
>> -            }
>> -            vhost_net_cleanup(s->vhost_net);
>> -        }
>> +        vhost_user_save_acked_features(ncs[i], true);
>>       }
>>   }
>>   
>> -- 
>> 1.8.3.1
> 

-- 
Best regard

Hyman Huang(黄勇)

Re: [PATCH v3 2/2] vhost-net: Fix the virtio features negotiation flaw

[Hyman]

在 2022/11/11 3:00, Michael S. Tsirkin 写道:
> On Sun, Oct 30, 2022 at 09:52:39PM +0800, huangy81@chinatelecom.cn wrote:
>> From: Hyman Huang(黄勇) <huangy81@chinatelecom.cn>
>>
>> Save the acked_features once it be configured by guest
>> virtio driver so it can't miss any features.
>>
>> Note that this patch also change the features saving logic
>> in chr_closed_bh, which originally backup features no matter
>> whether the features are 0 or not, but now do it only if
>> features aren't 0.
> 
> I'm not sure how is this change even related to what we
> are trying to do (fix a bug). Explain here?
> 
For this series, all we want to do is to making sure acked_features
in the NetVhostUserState is credible and uptodate in the scenario that 
virtio features negotiation and openvswitch service restart happens 
simultaneously.

To make sure that happens, we save the acked_features to 
NetVhostUserState right after guest setting virtio-net features.

Assume that we do not save acked_features to NetVhostUserState just as 
it is, the acked_features in NetVhostUserState has chance to be assigned 
only when chr_closed_bh/vhost_user_stop happen. Note that openvswitch 
service stop will cause chr_closed_bh happens and acked_features in 
vhost_dev will be stored into NetVhostUserState, if the acked_features 
in vhost_dev are out-of-date(may be updated in the next few seconds), so 
does the acked_features in NetVhostUserState after doing the assignment, 
this is the bug.

Let's refine the scenario and derive the bug:
     qemu thread                    		       dpdk
         |                                               |
    vhost_net_init()             		    	|
         |					 	|
  assign acked_features in vhost_dev                     |
    with 0x40000000               	                |
         |                    		       openvswitch.service stop
    chr_closed_bh                                        |
         |                                              	|
  assign acked_features in				|
  NetVhostUserState with 0x40000000 			|
         |						|
    virtio_net_set_features()                     	|
         |                     				|
  assign acked_features in vhost_dev			|
    with 0x7060a782					|
         |                                      openvswitch.service start
         |                                               |
    vhost_user_start                                     |
         |                                               |
  assign acked_features in vhost_dev                     |
    with 0x40000000                                      |
         |                                               |

As the step shows, if we do not keep the acked_features in 
NetVhostUserState up-to-date, the acked_features in vhost_dev may be 
reloaded with the wrong value(eg, 0x40000000) when vhost_user_start happens.
> 
> 
>> As to reset acked_features to 0 if needed, Qemu always
>> keeping the backup acked_features up-to-date, and save the
>> acked_features after virtio_net_set_features in advance,
>> including reset acked_features to 0, so the behavior is
>> also covered.
>>
>> Signed-off-by: Hyman Huang(黄勇) <huangy81@chinatelecom.cn>
>> Signed-off-by: Guoyi Tu <tugy@chinatelecom.cn>
>> ---
>>   hw/net/vhost_net.c      | 9 +++++++++
>>   hw/net/virtio-net.c     | 5 +++++
>>   include/net/vhost_net.h | 2 ++
>>   net/vhost-user.c        | 6 +-----
>>   4 files changed, 17 insertions(+), 5 deletions(-)
>>
>> diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
>> index d28f8b9..2bffc27 100644
>> --- a/hw/net/vhost_net.c
>> +++ b/hw/net/vhost_net.c
>> @@ -141,6 +141,15 @@ uint64_t vhost_net_get_acked_features(VHostNetState *net)
>>       return net->dev.acked_features;
>>   }
>>   
>> +void vhost_net_save_acked_features(NetClientState *nc)
>> +{
>> +    if (nc->info->type != NET_CLIENT_DRIVER_VHOST_USER) {
>> +        return;
>> +    }
>> +
>> +    vhost_user_save_acked_features(nc, false);
>> +}
>> +
>>   static int vhost_net_get_fd(NetClientState *backend)
>>   {
>>       switch (backend->info->type) {
>> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
>> index e9f696b..5f8f788 100644
>> --- a/hw/net/virtio-net.c
>> +++ b/hw/net/virtio-net.c
>> @@ -924,6 +924,11 @@ static void virtio_net_set_features(VirtIODevice *vdev, uint64_t features)
>>               continue;
>>           }
>>           vhost_net_ack_features(get_vhost_net(nc->peer), features);
>> +        /*
>> +         * keep acked_features in NetVhostUserState up-to-date so it
>> +         * can't miss any features configured by guest virtio driver.
>> +         */
>> +        vhost_net_save_acked_features(nc->peer);
>>       }
>>   
>>       if (virtio_has_feature(features, VIRTIO_NET_F_CTRL_VLAN)) {
>> diff --git a/include/net/vhost_net.h b/include/net/vhost_net.h
>> index 387e913..3a5579b 100644
>> --- a/include/net/vhost_net.h
>> +++ b/include/net/vhost_net.h
>> @@ -46,6 +46,8 @@ int vhost_set_vring_enable(NetClientState * nc, int enable);
>>   
>>   uint64_t vhost_net_get_acked_features(VHostNetState *net);
>>   
>> +void vhost_net_save_acked_features(NetClientState *nc);
>> +
>>   int vhost_net_set_mtu(struct vhost_net *net, uint16_t mtu);
>>   
>>   #endif
>> diff --git a/net/vhost-user.c b/net/vhost-user.c
>> index 74f349c..c512cc9 100644
>> --- a/net/vhost-user.c
>> +++ b/net/vhost-user.c
>> @@ -258,11 +258,7 @@ static void chr_closed_bh(void *opaque)
>>       s = DO_UPCAST(NetVhostUserState, nc, ncs[0]);
>>   
>>       for (i = queues -1; i >= 0; i--) {
>> -        s = DO_UPCAST(NetVhostUserState, nc, ncs[i]);
>> -
>> -        if (s->vhost_net) {
>> -            s->acked_features = vhost_net_get_acked_features(s->vhost_net);
>> -        }
>> +        vhost_user_save_acked_features(ncs[i], false);
>>       }
>>   
>>       qmp_set_link(name, false, &err);
> 
> 
> Split this last chunk into a patch of its own?
> 
>> -- 
>> 1.8.3.1
> 

Re: [PATCH v3 2/2] vhost-net: Fix the virtio features negotiation flaw

[Hyman]

The previous reply email has an text format error, please ignore and


在 2022/11/11 3:00, Michael S. Tsirkin 写道:
> On Sun, Oct 30, 2022 at 09:52:39PM +0800, huangy81@chinatelecom.cn wrote:
>> From: Hyman Huang(黄勇) <huangy81@chinatelecom.cn>
>>
>> Save the acked_features once it be configured by guest
>> virtio driver so it can't miss any features.
>>
>> Note that this patch also change the features saving logic
>> in chr_closed_bh, which originally backup features no matter
>> whether the features are 0 or not, but now do it only if
>> features aren't 0.
> 
> I'm not sure how is this change even related to what we
> are trying to do (fix a bug). Explain here?
> 
For this series, all we want to do is to making sure acked_features
in the NetVhostUserState is credible and uptodate in the scenario
that virtio features negotiation and openvswitch service restart
happens simultaneously.

To make sure that happens, we save the acked_features to
NetVhostUserState right after guest setting virtio-net features.

Assume that we do not save acked_features to NetVhostUserState just as
it is, the acked_features in NetVhostUserState has chance to be assigned
only when chr_closed_bh/vhost_user_stop happen. Note that openvswitch
service stop will cause chr_closed_bh happens and acked_features in
vhost_dev will be stored into NetVhostUserState, if the acked_features
in vhost_dev are out-of-date(may be updated in the next few seconds), so
does the acked_features in NetVhostUserState after doing the assignment,
this is the bug.

Let's refine the scenario and derive the bug:
     qemu thread                                        dpdk
         |                                               |
    vhost_net_init()                                     |
         |                                               |
  assign acked_features in vhost_dev                     |
    with 0x40000000                                      |
         |                                   openvswitch.service stop
    chr_closed_bh                                        |
         |                                               |
  assign acked_features in                               |
  NetVhostUserState with 0x40000000                      |
         |                                               |
    virtio_net_set_features()                            |
         |                                               |
  assign acked_features in vhost_dev                     |
    with 0x7060a782                                      |
         |                                      openvswitch.service start
         |                                               |
    vhost_user_start                                     |
         |                                               |
  assign acked_features in vhost_dev                     |
    with 0x40000000                                      |
         |                                               |

As the step shows, if we do not keep the acked_features in
NetVhostUserState up-to-date, the acked_features in vhost_dev may be
reloaded with the wrong value(eg, 0x40000000) when vhost_user_start
happens.
> 
> 
>> As to reset acked_features to 0 if needed, Qemu always
>> keeping the backup acked_features up-to-date, and save the
>> acked_features after virtio_net_set_features in advance,
>> including reset acked_features to 0, so the behavior is
>> also covered.
>>
>> Signed-off-by: Hyman Huang(黄勇) <huangy81@chinatelecom.cn>
>> Signed-off-by: Guoyi Tu <tugy@chinatelecom.cn>
>> ---
>>   hw/net/vhost_net.c      | 9 +++++++++
>>   hw/net/virtio-net.c     | 5 +++++
>>   include/net/vhost_net.h | 2 ++
>>   net/vhost-user.c        | 6 +-----
>>   4 files changed, 17 insertions(+), 5 deletions(-)
>>
>> diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
>> index d28f8b9..2bffc27 100644
>> --- a/hw/net/vhost_net.c
>> +++ b/hw/net/vhost_net.c
>> @@ -141,6 +141,15 @@ uint64_t vhost_net_get_acked_features(VHostNetState *net)
>>       return net->dev.acked_features;
>>   }
>>   
>> +void vhost_net_save_acked_features(NetClientState *nc)
>> +{
>> +    if (nc->info->type != NET_CLIENT_DRIVER_VHOST_USER) {
>> +        return;
>> +    }
>> +
>> +    vhost_user_save_acked_features(nc, false);
>> +}
>> +
>>   static int vhost_net_get_fd(NetClientState *backend)
>>   {
>>       switch (backend->info->type) {
>> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
>> index e9f696b..5f8f788 100644
>> --- a/hw/net/virtio-net.c
>> +++ b/hw/net/virtio-net.c
>> @@ -924,6 +924,11 @@ static void virtio_net_set_features(VirtIODevice *vdev, uint64_t features)
>>               continue;
>>           }
>>           vhost_net_ack_features(get_vhost_net(nc->peer), features);
>> +        /*
>> +         * keep acked_features in NetVhostUserState up-to-date so it
>> +         * can't miss any features configured by guest virtio driver.
>> +         */
>> +        vhost_net_save_acked_features(nc->peer);
>>       }
>>   
>>       if (virtio_has_feature(features, VIRTIO_NET_F_CTRL_VLAN)) {
>> diff --git a/include/net/vhost_net.h b/include/net/vhost_net.h
>> index 387e913..3a5579b 100644
>> --- a/include/net/vhost_net.h
>> +++ b/include/net/vhost_net.h
>> @@ -46,6 +46,8 @@ int vhost_set_vring_enable(NetClientState * nc, int enable);
>>   
>>   uint64_t vhost_net_get_acked_features(VHostNetState *net);
>>   
>> +void vhost_net_save_acked_features(NetClientState *nc);
>> +
>>   int vhost_net_set_mtu(struct vhost_net *net, uint16_t mtu);
>>   
>>   #endif
>> diff --git a/net/vhost-user.c b/net/vhost-user.c
>> index 74f349c..c512cc9 100644
>> --- a/net/vhost-user.c
>> +++ b/net/vhost-user.c
>> @@ -258,11 +258,7 @@ static void chr_closed_bh(void *opaque)
>>       s = DO_UPCAST(NetVhostUserState, nc, ncs[0]);
>>   
>>       for (i = queues -1; i >= 0; i--) {
>> -        s = DO_UPCAST(NetVhostUserState, nc, ncs[i]);
>> -
>> -        if (s->vhost_net) {
>> -            s->acked_features = vhost_net_get_acked_features(s->vhost_net);
>> -        }
>> +        vhost_user_save_acked_features(ncs[i], false);
>>       }
>>   
>>       qmp_set_link(name, false, &err);
> 
> 
> Split this last chunk into a patch of its own?
> 
>> -- 
>> 1.8.3.1
> 

Re: [PATCH v3 2/2] vhost-net: Fix the virtio features negotiation flaw

[Hyman]

在 2022/11/11 3:17, Michael S. Tsirkin 写道:
> On Sun, Oct 30, 2022 at 09:52:39PM +0800, huangy81@chinatelecom.cn wrote:
>> From: Hyman Huang(黄勇) <huangy81@chinatelecom.cn>
>>
>> Save the acked_features once it be configured by guest
>> virtio driver so it can't miss any features.
>>
>> Note that this patch also change the features saving logic
>> in chr_closed_bh, which originally backup features no matter
>> whether the features are 0 or not, but now do it only if
>> features aren't 0.
>>
>> As to reset acked_features to 0 if needed, Qemu always
>> keeping the backup acked_features up-to-date, and save the
>> acked_features after virtio_net_set_features in advance,
>> including reset acked_features to 0, so the behavior is
>> also covered.
>>
>> Signed-off-by: Hyman Huang(黄勇) <huangy81@chinatelecom.cn>
>> Signed-off-by: Guoyi Tu <tugy@chinatelecom.cn>
>> ---
>>   hw/net/vhost_net.c      | 9 +++++++++
>>   hw/net/virtio-net.c     | 5 +++++
>>   include/net/vhost_net.h | 2 ++
>>   net/vhost-user.c        | 6 +-----
>>   4 files changed, 17 insertions(+), 5 deletions(-)
>>
>> diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
>> index d28f8b9..2bffc27 100644
>> --- a/hw/net/vhost_net.c
>> +++ b/hw/net/vhost_net.c
>> @@ -141,6 +141,15 @@ uint64_t vhost_net_get_acked_features(VHostNetState *net)
>>       return net->dev.acked_features;
>>   }
>>   
>> +void vhost_net_save_acked_features(NetClientState *nc)
>> +{
>> +    if (nc->info->type != NET_CLIENT_DRIVER_VHOST_USER) {
>> +        return;
>> +    }
>> +
>> +    vhost_user_save_acked_features(nc, false);
>> +}
>> +
>>   static int vhost_net_get_fd(NetClientState *backend)
>>   {
>>       switch (backend->info->type) {
>> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
>> index e9f696b..5f8f788 100644
>> --- a/hw/net/virtio-net.c
>> +++ b/hw/net/virtio-net.c
>> @@ -924,6 +924,11 @@ static void virtio_net_set_features(VirtIODevice *vdev, uint64_t features)
>>               continue;
>>           }
>>           vhost_net_ack_features(get_vhost_net(nc->peer), features);
>> +        /*
>> +         * keep acked_features in NetVhostUserState up-to-date so it
>> +         * can't miss any features configured by guest virtio driver.
>> +         */
>> +        vhost_net_save_acked_features(nc->peer);
>>       }
>>   
>>       if (virtio_has_feature(features, VIRTIO_NET_F_CTRL_VLAN)) {
> 
> So when do you want to ack features but *not* save them?
When openvswitch restart and reconnect and Qemu start the vhost_dev， 
acked_features in vhost_dev Qemu need to be initialized and the 
initialized value be fetched from acked_features int NetVhostUserState.
At this time, acked_features  may not be up-to-date but we want it.
> 
> Is the effect of this patch, fundamentally, that guest features
> from virtio are always copied to vhost-user?
> Do we even need an extra copy in vhost user then?
> 
I'm trying to explain this from my view, please point out the mistake 
if i failed. :)

When socket used by vhost-user device disconnectted from openvswitch,
Qemu will stop the vhost-user and clean up the whole struct of 
vhost_dev(include vm's memory region and acked_features), once socket is 
reconnected from openvswitch, Qemu will collect vm's memory region 
dynamically but as to acked_features, IMHO, Qemu can not fetch it from 
guest features of virtio-net, because acked_features are kind of 
different from guest features(bit 30 is different at least)，so Qemu 
need an extra copy.

> 
> all this came in with:
> 
> commit a463215b087c41d7ca94e51aa347cde523831873
> Author: Marc-André Lureau <marcandre.lureau@redhat.com>
> Date:   Mon Jun 6 18:45:05 2016 +0200
> 
>      vhost-net: save & restore vhost-user acked features
> 
> Marc-André do you remember why we have a copy of features in vhost-user
> and not just reuse the features from virtio?
> 
> 
>> diff --git a/include/net/vhost_net.h b/include/net/vhost_net.h
>> index 387e913..3a5579b 100644
>> --- a/include/net/vhost_net.h
>> +++ b/include/net/vhost_net.h
>> @@ -46,6 +46,8 @@ int vhost_set_vring_enable(NetClientState * nc, int enable);
>>   
>>   uint64_t vhost_net_get_acked_features(VHostNetState *net);
>>   
>> +void vhost_net_save_acked_features(NetClientState *nc);
>> +
>>   int vhost_net_set_mtu(struct vhost_net *net, uint16_t mtu);
>>   
>>   #endif
>> diff --git a/net/vhost-user.c b/net/vhost-user.c
>> index 74f349c..c512cc9 100644
>> --- a/net/vhost-user.c
>> +++ b/net/vhost-user.c
>> @@ -258,11 +258,7 @@ static void chr_closed_bh(void *opaque)
>>       s = DO_UPCAST(NetVhostUserState, nc, ncs[0]);
>>   
>>       for (i = queues -1; i >= 0; i--) {
>> -        s = DO_UPCAST(NetVhostUserState, nc, ncs[i]);
>> -
>> -        if (s->vhost_net) {
>> -            s->acked_features = vhost_net_get_acked_features(s->vhost_net);
>> -        }
>> +        vhost_user_save_acked_features(ncs[i], false);
>>       }
>>   
>>       qmp_set_link(name, false, &err);
>> -- 
>> 1.8.3.1
> 

Re: [PATCH v3 2/2] vhost-net: Fix the virtio features negotiation flaw

[Michael S. Tsirkin]

On Mon, Nov 14, 2022 at 11:35:30PM +0800, Hyman wrote:
> 
> 
> 在 2022/11/11 3:17, Michael S. Tsirkin 写道:
> > On Sun, Oct 30, 2022 at 09:52:39PM +0800, huangy81@chinatelecom.cn wrote:
> > > From: Hyman Huang(黄勇) <huangy81@chinatelecom.cn>
> > > 
> > > Save the acked_features once it be configured by guest
> > > virtio driver so it can't miss any features.
> > > 
> > > Note that this patch also change the features saving logic
> > > in chr_closed_bh, which originally backup features no matter
> > > whether the features are 0 or not, but now do it only if
> > > features aren't 0.
> > > 
> > > As to reset acked_features to 0 if needed, Qemu always
> > > keeping the backup acked_features up-to-date, and save the
> > > acked_features after virtio_net_set_features in advance,
> > > including reset acked_features to 0, so the behavior is
> > > also covered.
> > > 
> > > Signed-off-by: Hyman Huang(黄勇) <huangy81@chinatelecom.cn>
> > > Signed-off-by: Guoyi Tu <tugy@chinatelecom.cn>
> > > ---
> > >   hw/net/vhost_net.c      | 9 +++++++++
> > >   hw/net/virtio-net.c     | 5 +++++
> > >   include/net/vhost_net.h | 2 ++
> > >   net/vhost-user.c        | 6 +-----
> > >   4 files changed, 17 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
> > > index d28f8b9..2bffc27 100644
> > > --- a/hw/net/vhost_net.c
> > > +++ b/hw/net/vhost_net.c
> > > @@ -141,6 +141,15 @@ uint64_t vhost_net_get_acked_features(VHostNetState *net)
> > >       return net->dev.acked_features;
> > >   }
> > > +void vhost_net_save_acked_features(NetClientState *nc)
> > > +{
> > > +    if (nc->info->type != NET_CLIENT_DRIVER_VHOST_USER) {
> > > +        return;
> > > +    }
> > > +
> > > +    vhost_user_save_acked_features(nc, false);
> > > +}
> > > +
> > >   static int vhost_net_get_fd(NetClientState *backend)
> > >   {
> > >       switch (backend->info->type) {
> > > diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> > > index e9f696b..5f8f788 100644
> > > --- a/hw/net/virtio-net.c
> > > +++ b/hw/net/virtio-net.c
> > > @@ -924,6 +924,11 @@ static void virtio_net_set_features(VirtIODevice *vdev, uint64_t features)
> > >               continue;
> > >           }
> > >           vhost_net_ack_features(get_vhost_net(nc->peer), features);
> > > +        /*
> > > +         * keep acked_features in NetVhostUserState up-to-date so it
> > > +         * can't miss any features configured by guest virtio driver.
> > > +         */
> > > +        vhost_net_save_acked_features(nc->peer);
> > >       }
> > >       if (virtio_has_feature(features, VIRTIO_NET_F_CTRL_VLAN)) {
> > 
> > So when do you want to ack features but *not* save them?
> When openvswitch restart and reconnect and Qemu start the vhost_dev，
> acked_features in vhost_dev Qemu need to be initialized and the initialized
> value be fetched from acked_features int NetVhostUserState.
> At this time, acked_features  may not be up-to-date but we want it.
> > 
> > Is the effect of this patch, fundamentally, that guest features
> > from virtio are always copied to vhost-user?
> > Do we even need an extra copy in vhost user then?
> > 
> I'm trying to explain this from my view, please point out the mistake if i
> failed. :)
> 
> When socket used by vhost-user device disconnectted from openvswitch,
> Qemu will stop the vhost-user and clean up the whole struct of
> vhost_dev(include vm's memory region and acked_features), once socket is
> reconnected from openvswitch, Qemu will collect vm's memory region
> dynamically but as to acked_features, IMHO, Qemu can not fetch it from guest
> features of virtio-net, because acked_features are kind of different from
> guest features(bit 30 is different at least)，so Qemu need an extra copy.

Well we already have a list of valid frontend features in
user_feature_bits.



> > 
> > all this came in with:
> > 
> > commit a463215b087c41d7ca94e51aa347cde523831873
> > Author: Marc-André Lureau <marcandre.lureau@redhat.com>
> > Date:   Mon Jun 6 18:45:05 2016 +0200
> > 
> >      vhost-net: save & restore vhost-user acked features
> > 
> > Marc-André do you remember why we have a copy of features in vhost-user
> > and not just reuse the features from virtio?
> > 
> > 
> > > diff --git a/include/net/vhost_net.h b/include/net/vhost_net.h
> > > index 387e913..3a5579b 100644
> > > --- a/include/net/vhost_net.h
> > > +++ b/include/net/vhost_net.h
> > > @@ -46,6 +46,8 @@ int vhost_set_vring_enable(NetClientState * nc, int enable);
> > >   uint64_t vhost_net_get_acked_features(VHostNetState *net);
> > > +void vhost_net_save_acked_features(NetClientState *nc);
> > > +
> > >   int vhost_net_set_mtu(struct vhost_net *net, uint16_t mtu);
> > >   #endif
> > > diff --git a/net/vhost-user.c b/net/vhost-user.c
> > > index 74f349c..c512cc9 100644
> > > --- a/net/vhost-user.c
> > > +++ b/net/vhost-user.c
> > > @@ -258,11 +258,7 @@ static void chr_closed_bh(void *opaque)
> > >       s = DO_UPCAST(NetVhostUserState, nc, ncs[0]);
> > >       for (i = queues -1; i >= 0; i--) {
> > > -        s = DO_UPCAST(NetVhostUserState, nc, ncs[i]);
> > > -
> > > -        if (s->vhost_net) {
> > > -            s->acked_features = vhost_net_get_acked_features(s->vhost_net);
> > > -        }
> > > +        vhost_user_save_acked_features(ncs[i], false);
> > >       }
> > >       qmp_set_link(name, false, &err);
> > > -- 
> > > 1.8.3.1
> >