[PATCH v2 0/3] Emulate status feature in vhost-vdpa net

[PATCH v2 0/3] Emulate status feature in vhost-vdpa net

[Eugenio Pérez]

The net config space is already copied from the device so it can me modified
by qemu. In particular, this is already done to fix cases where the NIC does
not expose the right fields.

It's trivial to emulate _F_STATE with qemu if not supported by the device,
sice a valid approach is to always show the link as up. If the feature is
already supported by the device, no config space modification is needed.

This is a pre requisite to use other features like _F_GUEST_ANNOUNCE, since
_F_STATUS is needed for the guest to access the status.

These patches are sent on top of [1] series, so trivial conflicts could arise
if it is applied directly on master. Future versions can be not based on
it is more convenient.

v2:
* Move feature handling to vhost_net, instead of force depending on the
  backend.

Eugenio Pérez (3):
  virtio_net: Modify virtio_net_get_config to early return
  virtio_net: Handle _F_STATUS emulation in virtio_net_get_config
  vhost_vdpa: move VIRTIO_NET_F_STATUS handling to vhost_net

 hw/net/vhost_net.c  | 13 +++++++++++++
 hw/net/virtio-net.c | 39 ++++++++++++++++++++++++---------------
 net/vhost-vdpa.c    |  1 -
 3 files changed, 37 insertions(+), 16 deletions(-)

-- 
2.31.1

[PATCH v2 1/3] virtio_net: Modify virtio_net_get_config to early return

[Eugenio Pérez]

Next patches introduce more code on vhost-vdpa branch, with already have
too much indentation.

Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/net/virtio-net.c | 28 +++++++++++++++-------------
 1 file changed, 15 insertions(+), 13 deletions(-)

diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index e9f696b4cf..56ff219196 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -158,20 +158,22 @@ static void virtio_net_get_config(VirtIODevice *vdev, uint8_t *config)
     if (nc->peer && nc->peer->info->type == NET_CLIENT_DRIVER_VHOST_VDPA) {
         ret = vhost_net_get_config(get_vhost_net(nc->peer), (uint8_t *)&netcfg,
                                    n->config_size);
-        if (ret != -1) {
-            /*
-             * Some NIC/kernel combinations present 0 as the mac address.  As
-             * that is not a legal address, try to proceed with the
-             * address from the QEMU command line in the hope that the
-             * address has been configured correctly elsewhere - just not
-             * reported by the device.
-             */
-            if (memcmp(&netcfg.mac, &zero, sizeof(zero)) == 0) {
-                info_report("Zero hardware mac address detected. Ignoring.");
-                memcpy(netcfg.mac, n->mac, ETH_ALEN);
-            }
-            memcpy(config, &netcfg, n->config_size);
+        if (ret == -1) {
+            return;
         }
+
+        /*
+         * Some NIC/kernel combinations present 0 as the mac address.  As that
+         * is not a legal address, try to proceed with the address from the
+         * QEMU command line in the hope that the address has been configured
+         * correctly elsewhere - just not reported by the device.
+         */
+        if (memcmp(&netcfg.mac, &zero, sizeof(zero)) == 0) {
+            info_report("Zero hardware mac address detected. Ignoring.");
+            memcpy(netcfg.mac, n->mac, ETH_ALEN);
+        }
+
+        memcpy(config, &netcfg, n->config_size);
     }
 }
 
-- 
2.31.1

[PATCH v2 2/3] virtio_net: Handle _F_STATUS emulation in virtio_net_get_config

[Eugenio Pérez]

At this moment this code path is not reached, but vdpa devices can offer
VIRTIO_NET_F_STATUS unconditionally. While the guest must assume that
link is always up by the standard, qemu will set the status bit to 1
always in this case.

This makes little use by itself, but VIRTIO_NET_F_STATUS is needed for
the guest to read status bit VIRTIO_NET_F_GUEST_ANNOUNCE, used by feature
VIRTIO_NET_F_GUEST_ANNOUNCE. So qemu must emulate status feature in case
it needs to emulate the guest announce feature.

Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/net/virtio-net.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 56ff219196..6d4d75615b 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -156,8 +156,9 @@ static void virtio_net_get_config(VirtIODevice *vdev, uint8_t *config)
      * disconnect/reconnect a VDPA peer.
      */
     if (nc->peer && nc->peer->info->type == NET_CLIENT_DRIVER_VHOST_VDPA) {
-        ret = vhost_net_get_config(get_vhost_net(nc->peer), (uint8_t *)&netcfg,
-                                   n->config_size);
+        struct vhost_net *net = get_vhost_net(nc->peer);
+
+        ret = vhost_net_get_config(net, (uint8_t *)&netcfg, n->config_size);
         if (ret == -1) {
             return;
         }
@@ -173,6 +174,12 @@ static void virtio_net_get_config(VirtIODevice *vdev, uint8_t *config)
             memcpy(netcfg.mac, n->mac, ETH_ALEN);
         }
 
+        if (vdev->guest_features & BIT_ULL(VIRTIO_NET_F_STATUS) &&
+            !(net->dev.features & BIT_ULL(VIRTIO_NET_F_STATUS))) {
+            /* Emulating link up in qemu */
+            netcfg.status |= virtio_tswap16(vdev, VIRTIO_NET_S_LINK_UP);
+        }
+
         memcpy(config, &netcfg, n->config_size);
     }
 }
-- 
2.31.1

[PATCH v2 3/3] vhost_vdpa: move VIRTIO_NET_F_STATUS handling to vhost_net

[Eugenio Pérez]

Since it is emulated on all vhost backends it makes sense to move it.

Although this feature can be emulated by qemu it benefits from
information from the device. Ack it as long as the guest ack it.

Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
---
 hw/net/vhost_net.c | 13 +++++++++++++
 net/vhost-vdpa.c   |  1 -
 2 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
index d28f8b974b..b533744211 100644
--- a/hw/net/vhost_net.c
+++ b/hw/net/vhost_net.c
@@ -127,8 +127,21 @@ int vhost_net_set_config(struct vhost_net *net, const uint8_t *data,
 
 void vhost_net_ack_features(struct vhost_net *net, uint64_t features)
 {
+    static const int status_feature_bit[] = {
+        VIRTIO_NET_F_STATUS,
+        VHOST_INVALID_FEATURE_BIT,
+    };
+
     net->dev.acked_features = net->dev.backend_features;
     vhost_ack_features(&net->dev, vhost_net_get_feature_bits(net), features);
+    if (net->dev.features & BIT_ULL(VIRTIO_NET_F_STATUS)) {
+        /*
+         * If device support _F_STATUS qemu should ack it so it reports link
+         * status changes. If not supported qemu emulates it reporting an
+         * always up link.
+         */
+        vhost_ack_features(&net->dev, status_feature_bit, features);
+    }
 }
 
 uint64_t vhost_net_get_max_queues(VHostNetState *net)
diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
index 6d64000202..854b27186c 100644
--- a/net/vhost-vdpa.c
+++ b/net/vhost-vdpa.c
@@ -72,7 +72,6 @@ const int vdpa_feature_bits[] = {
     VIRTIO_NET_F_RSS,
     VIRTIO_NET_F_HASH_REPORT,
     VIRTIO_NET_F_GUEST_ANNOUNCE,
-    VIRTIO_NET_F_STATUS,
     VHOST_INVALID_FEATURE_BIT
 };
 
-- 
2.31.1

Re: [PATCH v2 3/3] vhost_vdpa: move VIRTIO_NET_F_STATUS handling to vhost_net

[Michael S. Tsirkin]

On Fri, Oct 28, 2022 at 05:19:17PM +0200, Eugenio Pérez wrote:
> Since it is emulated on all vhost backends it makes sense to move it.
> 
> Although this feature can be emulated by qemu it benefits from
> information from the device. Ack it as long as the guest ack it.
> 
> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>

Reviewed-by: Michael S. Tsirkin <mst@redhat.com>

So I assume Jason's tree too?


> ---
>  hw/net/vhost_net.c | 13 +++++++++++++
>  net/vhost-vdpa.c   |  1 -
>  2 files changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
> index d28f8b974b..b533744211 100644
> --- a/hw/net/vhost_net.c
> +++ b/hw/net/vhost_net.c
> @@ -127,8 +127,21 @@ int vhost_net_set_config(struct vhost_net *net, const uint8_t *data,
>  
>  void vhost_net_ack_features(struct vhost_net *net, uint64_t features)
>  {
> +    static const int status_feature_bit[] = {
> +        VIRTIO_NET_F_STATUS,
> +        VHOST_INVALID_FEATURE_BIT,
> +    };
> +
>      net->dev.acked_features = net->dev.backend_features;
>      vhost_ack_features(&net->dev, vhost_net_get_feature_bits(net), features);
> +    if (net->dev.features & BIT_ULL(VIRTIO_NET_F_STATUS)) {
> +        /*
> +         * If device support _F_STATUS qemu should ack it so it reports link
> +         * status changes. If not supported qemu emulates it reporting an
> +         * always up link.
> +         */
> +        vhost_ack_features(&net->dev, status_feature_bit, features);
> +    }
>  }
>  
>  uint64_t vhost_net_get_max_queues(VHostNetState *net)
> diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
> index 6d64000202..854b27186c 100644
> --- a/net/vhost-vdpa.c
> +++ b/net/vhost-vdpa.c
> @@ -72,7 +72,6 @@ const int vdpa_feature_bits[] = {
>      VIRTIO_NET_F_RSS,
>      VIRTIO_NET_F_HASH_REPORT,
>      VIRTIO_NET_F_GUEST_ANNOUNCE,
> -    VIRTIO_NET_F_STATUS,
>      VHOST_INVALID_FEATURE_BIT
>  };
>  
> -- 
> 2.31.1