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

[PATCH 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.

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
  vdpa: Expose VIRTIO_NET_F_STATUS unconditionally

 include/net/vhost-vdpa.h |  1 +
 hw/net/vhost_net.c       | 16 ++++++++++++++--
 hw/net/virtio-net.c      | 39 ++++++++++++++++++++++++---------------
 net/vhost-vdpa.c         |  3 +++
 4 files changed, 42 insertions(+), 17 deletions(-)

-- 
2.31.1

[PATCH 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>
---
 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

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

[Philippe Mathieu-Daudé]

On 26/10/22 11:53, Eugenio Pérez wrote:
> Next patches introduce more code on vhost-vdpa branch, with already have
> too much indentation.
> 
> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> ---
>   hw/net/virtio-net.c | 28 +++++++++++++++-------------
>   1 file changed, 15 insertions(+), 13 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

[PATCH 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>
---
 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 3/3] vdpa: Expose VIRTIO_NET_F_STATUS unconditionally

[Eugenio Pérez]

Now that qemu can handle and emulate it if the vdpa backend does not
support it we can offer it always.

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

diff --git a/include/net/vhost-vdpa.h b/include/net/vhost-vdpa.h
index b81f9a6f2a..cfbcce6427 100644
--- a/include/net/vhost-vdpa.h
+++ b/include/net/vhost-vdpa.h
@@ -17,5 +17,6 @@
 struct vhost_net *vhost_vdpa_get_vhost_net(NetClientState *nc);
 
 extern const int vdpa_feature_bits[];
+extern const uint64_t vhost_vdpa_net_added_feature_bits;
 
 #endif /* VHOST_VDPA_H */
diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
index d28f8b974b..7c15cc6e8f 100644
--- a/hw/net/vhost_net.c
+++ b/hw/net/vhost_net.c
@@ -109,10 +109,22 @@ static const int *vhost_net_get_feature_bits(struct vhost_net *net)
     return feature_bits;
 }
 
+static uint64_t vhost_net_add_feature_bits(struct vhost_net *net)
+{
+    if (net->nc->info->type == NET_CLIENT_DRIVER_VHOST_VDPA) {
+        return vhost_vdpa_net_added_feature_bits;
+    }
+
+    return 0;
+}
+
 uint64_t vhost_net_get_features(struct vhost_net *net, uint64_t features)
 {
-    return vhost_get_features(&net->dev, vhost_net_get_feature_bits(net),
-            features);
+    uint64_t ret = vhost_get_features(&net->dev,
+                                      vhost_net_get_feature_bits(net),
+                                      features);
+
+    return ret | vhost_net_add_feature_bits(net);
 }
 int vhost_net_get_config(struct vhost_net *net,  uint8_t *config,
                          uint32_t config_len)
diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
index 6d64000202..24d2857593 100644
--- a/net/vhost-vdpa.c
+++ b/net/vhost-vdpa.c
@@ -99,6 +99,9 @@ static const uint64_t vdpa_svq_device_features =
     BIT_ULL(VIRTIO_NET_F_RSC_EXT) |
     BIT_ULL(VIRTIO_NET_F_STANDBY);
 
+const uint64_t vhost_vdpa_net_added_feature_bits =
+    BIT_ULL(VIRTIO_NET_F_STATUS);
+
 VHostNetState *vhost_vdpa_get_vhost_net(NetClientState *nc)
 {
     VhostVDPAState *s = DO_UPCAST(VhostVDPAState, nc, nc);
-- 
2.31.1

Re: [PATCH 3/3] vdpa: Expose VIRTIO_NET_F_STATUS unconditionally

[Jason Wang]

在 2022/10/26 17:53, Eugenio Pérez 写道:
> Now that qemu can handle and emulate it if the vdpa backend does not
> support it we can offer it always.
>
> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>


I may miss something but isn't more easier to simply remove the 
_F_STATUS from vdpa_feature_bits[]?

Thanks


> ---
>   include/net/vhost-vdpa.h |  1 +
>   hw/net/vhost_net.c       | 16 ++++++++++++++--
>   net/vhost-vdpa.c         |  3 +++
>   3 files changed, 18 insertions(+), 2 deletions(-)
>
> diff --git a/include/net/vhost-vdpa.h b/include/net/vhost-vdpa.h
> index b81f9a6f2a..cfbcce6427 100644
> --- a/include/net/vhost-vdpa.h
> +++ b/include/net/vhost-vdpa.h
> @@ -17,5 +17,6 @@
>   struct vhost_net *vhost_vdpa_get_vhost_net(NetClientState *nc);
>   
>   extern const int vdpa_feature_bits[];
> +extern const uint64_t vhost_vdpa_net_added_feature_bits;
>   
>   #endif /* VHOST_VDPA_H */
> diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
> index d28f8b974b..7c15cc6e8f 100644
> --- a/hw/net/vhost_net.c
> +++ b/hw/net/vhost_net.c
> @@ -109,10 +109,22 @@ static const int *vhost_net_get_feature_bits(struct vhost_net *net)
>       return feature_bits;
>   }
>   
> +static uint64_t vhost_net_add_feature_bits(struct vhost_net *net)
> +{
> +    if (net->nc->info->type == NET_CLIENT_DRIVER_VHOST_VDPA) {
> +        return vhost_vdpa_net_added_feature_bits;
> +    }
> +
> +    return 0;
> +}
> +
>   uint64_t vhost_net_get_features(struct vhost_net *net, uint64_t features)
>   {
> -    return vhost_get_features(&net->dev, vhost_net_get_feature_bits(net),
> -            features);
> +    uint64_t ret = vhost_get_features(&net->dev,
> +                                      vhost_net_get_feature_bits(net),
> +                                      features);
> +
> +    return ret | vhost_net_add_feature_bits(net);
>   }
>   int vhost_net_get_config(struct vhost_net *net,  uint8_t *config,
>                            uint32_t config_len)
> diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
> index 6d64000202..24d2857593 100644
> --- a/net/vhost-vdpa.c
> +++ b/net/vhost-vdpa.c
> @@ -99,6 +99,9 @@ static const uint64_t vdpa_svq_device_features =
>       BIT_ULL(VIRTIO_NET_F_RSC_EXT) |
>       BIT_ULL(VIRTIO_NET_F_STANDBY);
>   
> +const uint64_t vhost_vdpa_net_added_feature_bits =
> +    BIT_ULL(VIRTIO_NET_F_STATUS);
> +
>   VHostNetState *vhost_vdpa_get_vhost_net(NetClientState *nc)
>   {
>       VhostVDPAState *s = DO_UPCAST(VhostVDPAState, nc, nc);

Re: [PATCH 3/3] vdpa: Expose VIRTIO_NET_F_STATUS unconditionally

[Eugenio Perez Martin]

On Thu, Oct 27, 2022 at 6:32 AM Jason Wang <jasowang@redhat.com> wrote:
>
>
> 在 2022/10/26 17:53, Eugenio Pérez 写道:
> > Now that qemu can handle and emulate it if the vdpa backend does not
> > support it we can offer it always.
> >
> > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
>
>
> I may miss something but isn't more easier to simply remove the
> _F_STATUS from vdpa_feature_bits[]?
>

How is that? if we remove it, the guest cannot ack it so it cannot
access the net status, isn't it?

The goal with this patch series is to let the guest access the status
always, even if the device doesn't support _F_STATUS.

> Thanks
>
>
> > ---
> >   include/net/vhost-vdpa.h |  1 +
> >   hw/net/vhost_net.c       | 16 ++++++++++++++--
> >   net/vhost-vdpa.c         |  3 +++
> >   3 files changed, 18 insertions(+), 2 deletions(-)
> >
> > diff --git a/include/net/vhost-vdpa.h b/include/net/vhost-vdpa.h
> > index b81f9a6f2a..cfbcce6427 100644
> > --- a/include/net/vhost-vdpa.h
> > +++ b/include/net/vhost-vdpa.h
> > @@ -17,5 +17,6 @@
> >   struct vhost_net *vhost_vdpa_get_vhost_net(NetClientState *nc);
> >
> >   extern const int vdpa_feature_bits[];
> > +extern const uint64_t vhost_vdpa_net_added_feature_bits;
> >
> >   #endif /* VHOST_VDPA_H */
> > diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
> > index d28f8b974b..7c15cc6e8f 100644
> > --- a/hw/net/vhost_net.c
> > +++ b/hw/net/vhost_net.c
> > @@ -109,10 +109,22 @@ static const int *vhost_net_get_feature_bits(struct vhost_net *net)
> >       return feature_bits;
> >   }
> >
> > +static uint64_t vhost_net_add_feature_bits(struct vhost_net *net)
> > +{
> > +    if (net->nc->info->type == NET_CLIENT_DRIVER_VHOST_VDPA) {
> > +        return vhost_vdpa_net_added_feature_bits;
> > +    }
> > +
> > +    return 0;
> > +}
> > +
> >   uint64_t vhost_net_get_features(struct vhost_net *net, uint64_t features)
> >   {
> > -    return vhost_get_features(&net->dev, vhost_net_get_feature_bits(net),
> > -            features);
> > +    uint64_t ret = vhost_get_features(&net->dev,
> > +                                      vhost_net_get_feature_bits(net),
> > +                                      features);
> > +
> > +    return ret | vhost_net_add_feature_bits(net);
> >   }
> >   int vhost_net_get_config(struct vhost_net *net,  uint8_t *config,
> >                            uint32_t config_len)
> > diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
> > index 6d64000202..24d2857593 100644
> > --- a/net/vhost-vdpa.c
> > +++ b/net/vhost-vdpa.c
> > @@ -99,6 +99,9 @@ static const uint64_t vdpa_svq_device_features =
> >       BIT_ULL(VIRTIO_NET_F_RSC_EXT) |
> >       BIT_ULL(VIRTIO_NET_F_STANDBY);
> >
> > +const uint64_t vhost_vdpa_net_added_feature_bits =
> > +    BIT_ULL(VIRTIO_NET_F_STATUS);
> > +
> >   VHostNetState *vhost_vdpa_get_vhost_net(NetClientState *nc)
> >   {
> >       VhostVDPAState *s = DO_UPCAST(VhostVDPAState, nc, nc);
>

Re: [PATCH 3/3] vdpa: Expose VIRTIO_NET_F_STATUS unconditionally

[Jason Wang]

On Thu, Oct 27, 2022 at 2:47 PM Eugenio Perez Martin
<eperezma@redhat.com> wrote:
>
> On Thu, Oct 27, 2022 at 6:32 AM Jason Wang <jasowang@redhat.com> wrote:
> >
> >
> > 在 2022/10/26 17:53, Eugenio Pérez 写道:
> > > Now that qemu can handle and emulate it if the vdpa backend does not
> > > support it we can offer it always.
> > >
> > > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> >
> >
> > I may miss something but isn't more easier to simply remove the
> > _F_STATUS from vdpa_feature_bits[]?
> >
>
> How is that? if we remove it, the guest cannot ack it so it cannot
> access the net status, isn't it?

My understanding is that the bits stored in the vdpa_feature_bits[]
are the features that must be explicitly supported by the vhost
device. So if we remove _F_STATUS, Qemu vhost code won't validate if
vhost-vdpa device has this support:

uint64_t vhost_get_features(struct vhost_dev *hdev, const int *feature_bits,
                            uint64_t features)
{
    const int *bit = feature_bits;
    while (*bit != VHOST_INVALID_FEATURE_BIT) {
        uint64_t bit_mask = (1ULL << *bit);
        if (!(hdev->features & bit_mask)) {
            features &= ~bit_mask;
        }
        bit++;
    }
    return features;
}

Thanks



>
> The goal with this patch series is to let the guest access the status
> always, even if the device doesn't support _F_STATUS.
>
> > Thanks
> >
> >
> > > ---
> > >   include/net/vhost-vdpa.h |  1 +
> > >   hw/net/vhost_net.c       | 16 ++++++++++++++--
> > >   net/vhost-vdpa.c         |  3 +++
> > >   3 files changed, 18 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/include/net/vhost-vdpa.h b/include/net/vhost-vdpa.h
> > > index b81f9a6f2a..cfbcce6427 100644
> > > --- a/include/net/vhost-vdpa.h
> > > +++ b/include/net/vhost-vdpa.h
> > > @@ -17,5 +17,6 @@
> > >   struct vhost_net *vhost_vdpa_get_vhost_net(NetClientState *nc);
> > >
> > >   extern const int vdpa_feature_bits[];
> > > +extern const uint64_t vhost_vdpa_net_added_feature_bits;
> > >
> > >   #endif /* VHOST_VDPA_H */
> > > diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
> > > index d28f8b974b..7c15cc6e8f 100644
> > > --- a/hw/net/vhost_net.c
> > > +++ b/hw/net/vhost_net.c
> > > @@ -109,10 +109,22 @@ static const int *vhost_net_get_feature_bits(struct vhost_net *net)
> > >       return feature_bits;
> > >   }
> > >
> > > +static uint64_t vhost_net_add_feature_bits(struct vhost_net *net)
> > > +{
> > > +    if (net->nc->info->type == NET_CLIENT_DRIVER_VHOST_VDPA) {
> > > +        return vhost_vdpa_net_added_feature_bits;
> > > +    }
> > > +
> > > +    return 0;
> > > +}
> > > +
> > >   uint64_t vhost_net_get_features(struct vhost_net *net, uint64_t features)
> > >   {
> > > -    return vhost_get_features(&net->dev, vhost_net_get_feature_bits(net),
> > > -            features);
> > > +    uint64_t ret = vhost_get_features(&net->dev,
> > > +                                      vhost_net_get_feature_bits(net),
> > > +                                      features);
> > > +
> > > +    return ret | vhost_net_add_feature_bits(net);
> > >   }
> > >   int vhost_net_get_config(struct vhost_net *net,  uint8_t *config,
> > >                            uint32_t config_len)
> > > diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
> > > index 6d64000202..24d2857593 100644
> > > --- a/net/vhost-vdpa.c
> > > +++ b/net/vhost-vdpa.c
> > > @@ -99,6 +99,9 @@ static const uint64_t vdpa_svq_device_features =
> > >       BIT_ULL(VIRTIO_NET_F_RSC_EXT) |
> > >       BIT_ULL(VIRTIO_NET_F_STANDBY);
> > >
> > > +const uint64_t vhost_vdpa_net_added_feature_bits =
> > > +    BIT_ULL(VIRTIO_NET_F_STATUS);
> > > +
> > >   VHostNetState *vhost_vdpa_get_vhost_net(NetClientState *nc)
> > >   {
> > >       VhostVDPAState *s = DO_UPCAST(VhostVDPAState, nc, nc);
> >
>

Re: [PATCH 3/3] vdpa: Expose VIRTIO_NET_F_STATUS unconditionally

[Eugenio Perez Martin]

On Thu, Oct 27, 2022 at 8:54 AM Jason Wang <jasowang@redhat.com> wrote:
>
> On Thu, Oct 27, 2022 at 2:47 PM Eugenio Perez Martin
> <eperezma@redhat.com> wrote:
> >
> > On Thu, Oct 27, 2022 at 6:32 AM Jason Wang <jasowang@redhat.com> wrote:
> > >
> > >
> > > 在 2022/10/26 17:53, Eugenio Pérez 写道:
> > > > Now that qemu can handle and emulate it if the vdpa backend does not
> > > > support it we can offer it always.
> > > >
> > > > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > >
> > >
> > > I may miss something but isn't more easier to simply remove the
> > > _F_STATUS from vdpa_feature_bits[]?
> > >
> >
> > How is that? if we remove it, the guest cannot ack it so it cannot
> > access the net status, isn't it?
>
> My understanding is that the bits stored in the vdpa_feature_bits[]
> are the features that must be explicitly supported by the vhost
> device.

(Non English native here, so maybe I don't get what you mean :) ) The
device may not support them. net simulator lacks some of them
actually, and it works.

From what I see these are the only features that will be forwarded to
the guest as device_features. If it is not in the list, the feature
will be masked out, as if the device does not support it.

So now _F_STATUS it was forwarded only if the device supports it. If
we remove it from bit_mask, it will never be offered to the guest. But
we want to offer it always, since we will need it for
_F_GUEST_ANNOUNCE.

Things get more complex because we actually need to ack it back if the
device offers it, so the vdpa device can report link_down. We will
only emulate LINK_UP always in the case the device does not support
_F_STATUS.

> So if we remove _F_STATUS, Qemu vhost code won't validate if
> vhost-vdpa device has this support:
>
> uint64_t vhost_get_features(struct vhost_dev *hdev, const int *feature_bits,
>                             uint64_t features)
> {
>     const int *bit = feature_bits;
>     while (*bit != VHOST_INVALID_FEATURE_BIT) {
>         uint64_t bit_mask = (1ULL << *bit);
>         if (!(hdev->features & bit_mask)) {
>             features &= ~bit_mask;
>         }
>         bit++;
>     }
>     return features;
> }
>

Now maybe I'm the one missing something, but why is this not done as a
masking directly?

Instead of making feature_bits an array of ints, to declare it as a
uint64_t with the valid feature bits and simply return features &
feature_bits.

Thanks!

> Thanks
>
>
>
> >
> > The goal with this patch series is to let the guest access the status
> > always, even if the device doesn't support _F_STATUS.
> >
> > > Thanks
> > >
> > >
> > > > ---
> > > >   include/net/vhost-vdpa.h |  1 +
> > > >   hw/net/vhost_net.c       | 16 ++++++++++++++--
> > > >   net/vhost-vdpa.c         |  3 +++
> > > >   3 files changed, 18 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/include/net/vhost-vdpa.h b/include/net/vhost-vdpa.h
> > > > index b81f9a6f2a..cfbcce6427 100644
> > > > --- a/include/net/vhost-vdpa.h
> > > > +++ b/include/net/vhost-vdpa.h
> > > > @@ -17,5 +17,6 @@
> > > >   struct vhost_net *vhost_vdpa_get_vhost_net(NetClientState *nc);
> > > >
> > > >   extern const int vdpa_feature_bits[];
> > > > +extern const uint64_t vhost_vdpa_net_added_feature_bits;
> > > >
> > > >   #endif /* VHOST_VDPA_H */
> > > > diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
> > > > index d28f8b974b..7c15cc6e8f 100644
> > > > --- a/hw/net/vhost_net.c
> > > > +++ b/hw/net/vhost_net.c
> > > > @@ -109,10 +109,22 @@ static const int *vhost_net_get_feature_bits(struct vhost_net *net)
> > > >       return feature_bits;
> > > >   }
> > > >
> > > > +static uint64_t vhost_net_add_feature_bits(struct vhost_net *net)
> > > > +{
> > > > +    if (net->nc->info->type == NET_CLIENT_DRIVER_VHOST_VDPA) {
> > > > +        return vhost_vdpa_net_added_feature_bits;
> > > > +    }
> > > > +
> > > > +    return 0;
> > > > +}
> > > > +
> > > >   uint64_t vhost_net_get_features(struct vhost_net *net, uint64_t features)
> > > >   {
> > > > -    return vhost_get_features(&net->dev, vhost_net_get_feature_bits(net),
> > > > -            features);
> > > > +    uint64_t ret = vhost_get_features(&net->dev,
> > > > +                                      vhost_net_get_feature_bits(net),
> > > > +                                      features);
> > > > +
> > > > +    return ret | vhost_net_add_feature_bits(net);
> > > >   }
> > > >   int vhost_net_get_config(struct vhost_net *net,  uint8_t *config,
> > > >                            uint32_t config_len)
> > > > diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
> > > > index 6d64000202..24d2857593 100644
> > > > --- a/net/vhost-vdpa.c
> > > > +++ b/net/vhost-vdpa.c
> > > > @@ -99,6 +99,9 @@ static const uint64_t vdpa_svq_device_features =
> > > >       BIT_ULL(VIRTIO_NET_F_RSC_EXT) |
> > > >       BIT_ULL(VIRTIO_NET_F_STANDBY);
> > > >
> > > > +const uint64_t vhost_vdpa_net_added_feature_bits =
> > > > +    BIT_ULL(VIRTIO_NET_F_STATUS);
> > > > +
> > > >   VHostNetState *vhost_vdpa_get_vhost_net(NetClientState *nc)
> > > >   {
> > > >       VhostVDPAState *s = DO_UPCAST(VhostVDPAState, nc, nc);
> > >
> >
>

Re: [PATCH 3/3] vdpa: Expose VIRTIO_NET_F_STATUS unconditionally

[Jason Wang]

On Thu, Oct 27, 2022 at 6:18 PM Eugenio Perez Martin
<eperezma@redhat.com> wrote:
>
> On Thu, Oct 27, 2022 at 8:54 AM Jason Wang <jasowang@redhat.com> wrote:
> >
> > On Thu, Oct 27, 2022 at 2:47 PM Eugenio Perez Martin
> > <eperezma@redhat.com> wrote:
> > >
> > > On Thu, Oct 27, 2022 at 6:32 AM Jason Wang <jasowang@redhat.com> wrote:
> > > >
> > > >
> > > > 在 2022/10/26 17:53, Eugenio Pérez 写道:
> > > > > Now that qemu can handle and emulate it if the vdpa backend does not
> > > > > support it we can offer it always.
> > > > >
> > > > > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > > >
> > > >
> > > > I may miss something but isn't more easier to simply remove the
> > > > _F_STATUS from vdpa_feature_bits[]?
> > > >
> > >
> > > How is that? if we remove it, the guest cannot ack it so it cannot
> > > access the net status, isn't it?
> >
> > My understanding is that the bits stored in the vdpa_feature_bits[]
> > are the features that must be explicitly supported by the vhost
> > device.
>
> (Non English native here, so maybe I don't get what you mean :) ) The
> device may not support them. net simulator lacks some of them
> actually, and it works.

Speaking too fast, I think I meant that, if the bit doesn't belong to
vdpa_feature_bits[], it is assumed to be supported by the Qemu without
the support of the vhost. So Qemu won't even try to validate if vhost
has this support. E.g for vhost-net, we only have:

static const int kernel_feature_bits[] = {
    VIRTIO_F_NOTIFY_ON_EMPTY,
    VIRTIO_RING_F_INDIRECT_DESC,
    VIRTIO_RING_F_EVENT_IDX,
    VIRTIO_NET_F_MRG_RXBUF,
    VIRTIO_F_VERSION_1,
    VIRTIO_NET_F_MTU,
    VIRTIO_F_IOMMU_PLATFORM,
    VIRTIO_F_RING_PACKED,
    VIRTIO_NET_F_HASH_REPORT,
    VHOST_INVALID_FEATURE_BIT
};

You can see there's no STATUS bit there since it is emulated by Qemu.

>
> From what I see these are the only features that will be forwarded to
> the guest as device_features. If it is not in the list, the feature
> will be masked out,

Only when there's no support for this feature from the vhost.

> as if the device does not support it.
>
> So now _F_STATUS it was forwarded only if the device supports it. If
> we remove it from bit_mask, it will never be offered to the guest. But
> we want to offer it always, since we will need it for
> _F_GUEST_ANNOUNCE.
>
> Things get more complex because we actually need to ack it back if the
> device offers it, so the vdpa device can report link_down. We will
> only emulate LINK_UP always in the case the device does not support
> _F_STATUS.
>
> > So if we remove _F_STATUS, Qemu vhost code won't validate if
> > vhost-vdpa device has this support:
> >
> > uint64_t vhost_get_features(struct vhost_dev *hdev, const int *feature_bits,
> >                             uint64_t features)
> > {
> >     const int *bit = feature_bits;
> >     while (*bit != VHOST_INVALID_FEATURE_BIT) {
> >         uint64_t bit_mask = (1ULL << *bit);
> >         if (!(hdev->features & bit_mask)) {
> >             features &= ~bit_mask;
> >         }
> >         bit++;
> >     }
> >     return features;
> > }
> >
>
> Now maybe I'm the one missing something, but why is this not done as a
> masking directly?

Not sure, the code has been there since day 0.

But you can see from the code:

1) if STATUS is in feature_bits, we need validate the hdev->features
and mask it if the vhost doesn't have the support
2) if STATUS is not, we don't do the check and driver may still see STATUS

Thanks

>
> Instead of making feature_bits an array of ints, to declare it as a
> uint64_t with the valid feature bits and simply return features &
> feature_bits.
>
> Thanks!
>
> > Thanks
> >
> >
> >
> > >
> > > The goal with this patch series is to let the guest access the status
> > > always, even if the device doesn't support _F_STATUS.
> > >
> > > > Thanks
> > > >
> > > >
> > > > > ---
> > > > >   include/net/vhost-vdpa.h |  1 +
> > > > >   hw/net/vhost_net.c       | 16 ++++++++++++++--
> > > > >   net/vhost-vdpa.c         |  3 +++
> > > > >   3 files changed, 18 insertions(+), 2 deletions(-)
> > > > >
> > > > > diff --git a/include/net/vhost-vdpa.h b/include/net/vhost-vdpa.h
> > > > > index b81f9a6f2a..cfbcce6427 100644
> > > > > --- a/include/net/vhost-vdpa.h
> > > > > +++ b/include/net/vhost-vdpa.h
> > > > > @@ -17,5 +17,6 @@
> > > > >   struct vhost_net *vhost_vdpa_get_vhost_net(NetClientState *nc);
> > > > >
> > > > >   extern const int vdpa_feature_bits[];
> > > > > +extern const uint64_t vhost_vdpa_net_added_feature_bits;
> > > > >
> > > > >   #endif /* VHOST_VDPA_H */
> > > > > diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
> > > > > index d28f8b974b..7c15cc6e8f 100644
> > > > > --- a/hw/net/vhost_net.c
> > > > > +++ b/hw/net/vhost_net.c
> > > > > @@ -109,10 +109,22 @@ static const int *vhost_net_get_feature_bits(struct vhost_net *net)
> > > > >       return feature_bits;
> > > > >   }
> > > > >
> > > > > +static uint64_t vhost_net_add_feature_bits(struct vhost_net *net)
> > > > > +{
> > > > > +    if (net->nc->info->type == NET_CLIENT_DRIVER_VHOST_VDPA) {
> > > > > +        return vhost_vdpa_net_added_feature_bits;
> > > > > +    }
> > > > > +
> > > > > +    return 0;
> > > > > +}
> > > > > +
> > > > >   uint64_t vhost_net_get_features(struct vhost_net *net, uint64_t features)
> > > > >   {
> > > > > -    return vhost_get_features(&net->dev, vhost_net_get_feature_bits(net),
> > > > > -            features);
> > > > > +    uint64_t ret = vhost_get_features(&net->dev,
> > > > > +                                      vhost_net_get_feature_bits(net),
> > > > > +                                      features);
> > > > > +
> > > > > +    return ret | vhost_net_add_feature_bits(net);
> > > > >   }
> > > > >   int vhost_net_get_config(struct vhost_net *net,  uint8_t *config,
> > > > >                            uint32_t config_len)
> > > > > diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
> > > > > index 6d64000202..24d2857593 100644
> > > > > --- a/net/vhost-vdpa.c
> > > > > +++ b/net/vhost-vdpa.c
> > > > > @@ -99,6 +99,9 @@ static const uint64_t vdpa_svq_device_features =
> > > > >       BIT_ULL(VIRTIO_NET_F_RSC_EXT) |
> > > > >       BIT_ULL(VIRTIO_NET_F_STANDBY);
> > > > >
> > > > > +const uint64_t vhost_vdpa_net_added_feature_bits =
> > > > > +    BIT_ULL(VIRTIO_NET_F_STATUS);
> > > > > +
> > > > >   VHostNetState *vhost_vdpa_get_vhost_net(NetClientState *nc)
> > > > >   {
> > > > >       VhostVDPAState *s = DO_UPCAST(VhostVDPAState, nc, nc);
> > > >
> > >
> >
>

Re: [PATCH 3/3] vdpa: Expose VIRTIO_NET_F_STATUS unconditionally

[Eugenio Perez Martin]

On Fri, Oct 28, 2022 at 3:59 AM Jason Wang <jasowang@redhat.com> wrote:
>
> On Thu, Oct 27, 2022 at 6:18 PM Eugenio Perez Martin
> <eperezma@redhat.com> wrote:
> >
> > On Thu, Oct 27, 2022 at 8:54 AM Jason Wang <jasowang@redhat.com> wrote:
> > >
> > > On Thu, Oct 27, 2022 at 2:47 PM Eugenio Perez Martin
> > > <eperezma@redhat.com> wrote:
> > > >
> > > > On Thu, Oct 27, 2022 at 6:32 AM Jason Wang <jasowang@redhat.com> wrote:
> > > > >
> > > > >
> > > > > 在 2022/10/26 17:53, Eugenio Pérez 写道:
> > > > > > Now that qemu can handle and emulate it if the vdpa backend does not
> > > > > > support it we can offer it always.
> > > > > >
> > > > > > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > > > >
> > > > >
> > > > > I may miss something but isn't more easier to simply remove the
> > > > > _F_STATUS from vdpa_feature_bits[]?
> > > > >
> > > >
> > > > How is that? if we remove it, the guest cannot ack it so it cannot
> > > > access the net status, isn't it?
> > >
> > > My understanding is that the bits stored in the vdpa_feature_bits[]
> > > are the features that must be explicitly supported by the vhost
> > > device.
> >
> > (Non English native here, so maybe I don't get what you mean :) ) The
> > device may not support them. net simulator lacks some of them
> > actually, and it works.
>
> Speaking too fast, I think I meant that, if the bit doesn't belong to
> vdpa_feature_bits[], it is assumed to be supported by the Qemu without
> the support of the vhost. So Qemu won't even try to validate if vhost
> has this support. E.g for vhost-net, we only have:
>
> static const int kernel_feature_bits[] = {
>     VIRTIO_F_NOTIFY_ON_EMPTY,
>     VIRTIO_RING_F_INDIRECT_DESC,
>     VIRTIO_RING_F_EVENT_IDX,
>     VIRTIO_NET_F_MRG_RXBUF,
>     VIRTIO_F_VERSION_1,
>     VIRTIO_NET_F_MTU,
>     VIRTIO_F_IOMMU_PLATFORM,
>     VIRTIO_F_RING_PACKED,
>     VIRTIO_NET_F_HASH_REPORT,
>     VHOST_INVALID_FEATURE_BIT
> };
>
> You can see there's no STATUS bit there since it is emulated by Qemu.
>

Ok now I get what you mean, and yes we may modify the patches in that direction.

But if we go then we need to modify how qemu ack the features, because
the features that are not in vdpa_feature_bits are not acked to the
device. More on this later.

> >
> > From what I see these are the only features that will be forwarded to
> > the guest as device_features. If it is not in the list, the feature
> > will be masked out,
>
> Only when there's no support for this feature from the vhost.
>
> > as if the device does not support it.
> >
> > So now _F_STATUS it was forwarded only if the device supports it. If
> > we remove it from bit_mask, it will never be offered to the guest. But
> > we want to offer it always, since we will need it for
> > _F_GUEST_ANNOUNCE.
> >
> > Things get more complex because we actually need to ack it back if the
> > device offers it, so the vdpa device can report link_down. We will
> > only emulate LINK_UP always in the case the device does not support
> > _F_STATUS.
> >
> > > So if we remove _F_STATUS, Qemu vhost code won't validate if
> > > vhost-vdpa device has this support:
> > >
> > > uint64_t vhost_get_features(struct vhost_dev *hdev, const int *feature_bits,
> > >                             uint64_t features)
> > > {
> > >     const int *bit = feature_bits;
> > >     while (*bit != VHOST_INVALID_FEATURE_BIT) {
> > >         uint64_t bit_mask = (1ULL << *bit);
> > >         if (!(hdev->features & bit_mask)) {
> > >             features &= ~bit_mask;
> > >         }
> > >         bit++;
> > >     }
> > >     return features;
> > > }
> > >
> >
> > Now maybe I'm the one missing something, but why is this not done as a
> > masking directly?
>
> Not sure, the code has been there since day 0.
>
> But you can see from the code:
>
> 1) if STATUS is in feature_bits, we need validate the hdev->features
> and mask it if the vhost doesn't have the support
> 2) if STATUS is not, we don't do the check and driver may still see STATUS
>

That's useful for _F_GUEST_ANNOUNCE, but we need to ack _F_STATUS for
the device if it supports it. QEMU cannot detect by itself when the
link is not up. I think that setting unconditionally
VIRTIO_NET_S_LINK_UP is actually a regression, since the guest cannot
detect the link down that way.

To enable _F_STATUS unconditionally is only done in the case the
device does not support it, because its emulation is very easy. That
way we support _F_GUEST_ANNOUNCE in all cases without device's
cooperation.

Having said that, should we go the opposite route and ack _F_STATE as
long as the device supports it? As an advantage, all backends should
support that at this moment, isn't it?

Thanks!




> Thanks
>
> >
> > Instead of making feature_bits an array of ints, to declare it as a
> > uint64_t with the valid feature bits and simply return features &
> > feature_bits.
> >
> > Thanks!
> >
> > > Thanks
> > >
> > >
> > >
> > > >
> > > > The goal with this patch series is to let the guest access the status
> > > > always, even if the device doesn't support _F_STATUS.
> > > >
> > > > > Thanks
> > > > >
> > > > >
> > > > > > ---
> > > > > >   include/net/vhost-vdpa.h |  1 +
> > > > > >   hw/net/vhost_net.c       | 16 ++++++++++++++--
> > > > > >   net/vhost-vdpa.c         |  3 +++
> > > > > >   3 files changed, 18 insertions(+), 2 deletions(-)
> > > > > >
> > > > > > diff --git a/include/net/vhost-vdpa.h b/include/net/vhost-vdpa.h
> > > > > > index b81f9a6f2a..cfbcce6427 100644
> > > > > > --- a/include/net/vhost-vdpa.h
> > > > > > +++ b/include/net/vhost-vdpa.h
> > > > > > @@ -17,5 +17,6 @@
> > > > > >   struct vhost_net *vhost_vdpa_get_vhost_net(NetClientState *nc);
> > > > > >
> > > > > >   extern const int vdpa_feature_bits[];
> > > > > > +extern const uint64_t vhost_vdpa_net_added_feature_bits;
> > > > > >
> > > > > >   #endif /* VHOST_VDPA_H */
> > > > > > diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
> > > > > > index d28f8b974b..7c15cc6e8f 100644
> > > > > > --- a/hw/net/vhost_net.c
> > > > > > +++ b/hw/net/vhost_net.c
> > > > > > @@ -109,10 +109,22 @@ static const int *vhost_net_get_feature_bits(struct vhost_net *net)
> > > > > >       return feature_bits;
> > > > > >   }
> > > > > >
> > > > > > +static uint64_t vhost_net_add_feature_bits(struct vhost_net *net)
> > > > > > +{
> > > > > > +    if (net->nc->info->type == NET_CLIENT_DRIVER_VHOST_VDPA) {
> > > > > > +        return vhost_vdpa_net_added_feature_bits;
> > > > > > +    }
> > > > > > +
> > > > > > +    return 0;
> > > > > > +}
> > > > > > +
> > > > > >   uint64_t vhost_net_get_features(struct vhost_net *net, uint64_t features)
> > > > > >   {
> > > > > > -    return vhost_get_features(&net->dev, vhost_net_get_feature_bits(net),
> > > > > > -            features);
> > > > > > +    uint64_t ret = vhost_get_features(&net->dev,
> > > > > > +                                      vhost_net_get_feature_bits(net),
> > > > > > +                                      features);
> > > > > > +
> > > > > > +    return ret | vhost_net_add_feature_bits(net);
> > > > > >   }
> > > > > >   int vhost_net_get_config(struct vhost_net *net,  uint8_t *config,
> > > > > >                            uint32_t config_len)
> > > > > > diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
> > > > > > index 6d64000202..24d2857593 100644
> > > > > > --- a/net/vhost-vdpa.c
> > > > > > +++ b/net/vhost-vdpa.c
> > > > > > @@ -99,6 +99,9 @@ static const uint64_t vdpa_svq_device_features =
> > > > > >       BIT_ULL(VIRTIO_NET_F_RSC_EXT) |
> > > > > >       BIT_ULL(VIRTIO_NET_F_STANDBY);
> > > > > >
> > > > > > +const uint64_t vhost_vdpa_net_added_feature_bits =
> > > > > > +    BIT_ULL(VIRTIO_NET_F_STATUS);
> > > > > > +
> > > > > >   VHostNetState *vhost_vdpa_get_vhost_net(NetClientState *nc)
> > > > > >   {
> > > > > >       VhostVDPAState *s = DO_UPCAST(VhostVDPAState, nc, nc);
> > > > >
> > > >
> > >
> >
>

Re: [PATCH 3/3] vdpa: Expose VIRTIO_NET_F_STATUS unconditionally

[Jason Wang]

On Fri, Oct 28, 2022 at 5:30 PM Eugenio Perez Martin
<eperezma@redhat.com> wrote:
>
> On Fri, Oct 28, 2022 at 3:59 AM Jason Wang <jasowang@redhat.com> wrote:
> >
> > On Thu, Oct 27, 2022 at 6:18 PM Eugenio Perez Martin
> > <eperezma@redhat.com> wrote:
> > >
> > > On Thu, Oct 27, 2022 at 8:54 AM Jason Wang <jasowang@redhat.com> wrote:
> > > >
> > > > On Thu, Oct 27, 2022 at 2:47 PM Eugenio Perez Martin
> > > > <eperezma@redhat.com> wrote:
> > > > >
> > > > > On Thu, Oct 27, 2022 at 6:32 AM Jason Wang <jasowang@redhat.com> wrote:
> > > > > >
> > > > > >
> > > > > > 在 2022/10/26 17:53, Eugenio Pérez 写道:
> > > > > > > Now that qemu can handle and emulate it if the vdpa backend does not
> > > > > > > support it we can offer it always.
> > > > > > >
> > > > > > > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > > > > >
> > > > > >
> > > > > > I may miss something but isn't more easier to simply remove the
> > > > > > _F_STATUS from vdpa_feature_bits[]?
> > > > > >
> > > > >
> > > > > How is that? if we remove it, the guest cannot ack it so it cannot
> > > > > access the net status, isn't it?
> > > >
> > > > My understanding is that the bits stored in the vdpa_feature_bits[]
> > > > are the features that must be explicitly supported by the vhost
> > > > device.
> > >
> > > (Non English native here, so maybe I don't get what you mean :) ) The
> > > device may not support them. net simulator lacks some of them
> > > actually, and it works.
> >
> > Speaking too fast, I think I meant that, if the bit doesn't belong to
> > vdpa_feature_bits[], it is assumed to be supported by the Qemu without
> > the support of the vhost. So Qemu won't even try to validate if vhost
> > has this support. E.g for vhost-net, we only have:
> >
> > static const int kernel_feature_bits[] = {
> >     VIRTIO_F_NOTIFY_ON_EMPTY,
> >     VIRTIO_RING_F_INDIRECT_DESC,
> >     VIRTIO_RING_F_EVENT_IDX,
> >     VIRTIO_NET_F_MRG_RXBUF,
> >     VIRTIO_F_VERSION_1,
> >     VIRTIO_NET_F_MTU,
> >     VIRTIO_F_IOMMU_PLATFORM,
> >     VIRTIO_F_RING_PACKED,
> >     VIRTIO_NET_F_HASH_REPORT,
> >     VHOST_INVALID_FEATURE_BIT
> > };
> >
> > You can see there's no STATUS bit there since it is emulated by Qemu.
> >
>
> Ok now I get what you mean, and yes we may modify the patches in that direction.
>
> But if we go then we need to modify how qemu ack the features, because
> the features that are not in vdpa_feature_bits are not acked to the
> device. More on this later.
>
> > >
> > > From what I see these are the only features that will be forwarded to
> > > the guest as device_features. If it is not in the list, the feature
> > > will be masked out,
> >
> > Only when there's no support for this feature from the vhost.
> >
> > > as if the device does not support it.
> > >
> > > So now _F_STATUS it was forwarded only if the device supports it. If
> > > we remove it from bit_mask, it will never be offered to the guest. But
> > > we want to offer it always, since we will need it for
> > > _F_GUEST_ANNOUNCE.
> > >
> > > Things get more complex because we actually need to ack it back if the
> > > device offers it, so the vdpa device can report link_down. We will
> > > only emulate LINK_UP always in the case the device does not support
> > > _F_STATUS.
> > >
> > > > So if we remove _F_STATUS, Qemu vhost code won't validate if
> > > > vhost-vdpa device has this support:
> > > >
> > > > uint64_t vhost_get_features(struct vhost_dev *hdev, const int *feature_bits,
> > > >                             uint64_t features)
> > > > {
> > > >     const int *bit = feature_bits;
> > > >     while (*bit != VHOST_INVALID_FEATURE_BIT) {
> > > >         uint64_t bit_mask = (1ULL << *bit);
> > > >         if (!(hdev->features & bit_mask)) {
> > > >             features &= ~bit_mask;
> > > >         }
> > > >         bit++;
> > > >     }
> > > >     return features;
> > > > }
> > > >
> > >
> > > Now maybe I'm the one missing something, but why is this not done as a
> > > masking directly?
> >
> > Not sure, the code has been there since day 0.
> >
> > But you can see from the code:
> >
> > 1) if STATUS is in feature_bits, we need validate the hdev->features
> > and mask it if the vhost doesn't have the support
> > 2) if STATUS is not, we don't do the check and driver may still see STATUS
> >
>
> That's useful for _F_GUEST_ANNOUNCE, but we need to ack _F_STATUS for
> the device if it supports it.

Rethink about this, I don't see why ANNOUNCE depends on STATUS (spec
doesn't say so).

> QEMU cannot detect by itself when the
> link is not up. I think that setting unconditionally
> VIRTIO_NET_S_LINK_UP is actually a regression, since the guest cannot
> detect the link down that way.

I think the idea is to still read status from config if the device
supports this.

>
> To enable _F_STATUS unconditionally is only done in the case the
> device does not support it, because its emulation is very easy. That
> way we support _F_GUEST_ANNOUNCE in all cases without device's
> cooperation.
>
> Having said that, should we go the opposite route and ack _F_STATE as
> long as the device supports it? As an advantage, all backends should
> support that at this moment, isn't it?

So I think the method used in this patch is fine, but I wonder if it's
better to move it to the vhost layer instead of doing it in vhost_net
since we do the features validation there. We probably need another
table as input for get/set features there?

Thanks

>
> Thanks!
>
>
>
>
> > Thanks
> >
> > >
> > > Instead of making feature_bits an array of ints, to declare it as a
> > > uint64_t with the valid feature bits and simply return features &
> > > feature_bits.
> > >
> > > Thanks!
> > >
> > > > Thanks
> > > >
> > > >
> > > >
> > > > >
> > > > > The goal with this patch series is to let the guest access the status
> > > > > always, even if the device doesn't support _F_STATUS.
> > > > >
> > > > > > Thanks
> > > > > >
> > > > > >
> > > > > > > ---
> > > > > > >   include/net/vhost-vdpa.h |  1 +
> > > > > > >   hw/net/vhost_net.c       | 16 ++++++++++++++--
> > > > > > >   net/vhost-vdpa.c         |  3 +++
> > > > > > >   3 files changed, 18 insertions(+), 2 deletions(-)
> > > > > > >
> > > > > > > diff --git a/include/net/vhost-vdpa.h b/include/net/vhost-vdpa.h
> > > > > > > index b81f9a6f2a..cfbcce6427 100644
> > > > > > > --- a/include/net/vhost-vdpa.h
> > > > > > > +++ b/include/net/vhost-vdpa.h
> > > > > > > @@ -17,5 +17,6 @@
> > > > > > >   struct vhost_net *vhost_vdpa_get_vhost_net(NetClientState *nc);
> > > > > > >
> > > > > > >   extern const int vdpa_feature_bits[];
> > > > > > > +extern const uint64_t vhost_vdpa_net_added_feature_bits;
> > > > > > >
> > > > > > >   #endif /* VHOST_VDPA_H */
> > > > > > > diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
> > > > > > > index d28f8b974b..7c15cc6e8f 100644
> > > > > > > --- a/hw/net/vhost_net.c
> > > > > > > +++ b/hw/net/vhost_net.c
> > > > > > > @@ -109,10 +109,22 @@ static const int *vhost_net_get_feature_bits(struct vhost_net *net)
> > > > > > >       return feature_bits;
> > > > > > >   }
> > > > > > >
> > > > > > > +static uint64_t vhost_net_add_feature_bits(struct vhost_net *net)
> > > > > > > +{
> > > > > > > +    if (net->nc->info->type == NET_CLIENT_DRIVER_VHOST_VDPA) {
> > > > > > > +        return vhost_vdpa_net_added_feature_bits;
> > > > > > > +    }
> > > > > > > +
> > > > > > > +    return 0;
> > > > > > > +}
> > > > > > > +
> > > > > > >   uint64_t vhost_net_get_features(struct vhost_net *net, uint64_t features)
> > > > > > >   {
> > > > > > > -    return vhost_get_features(&net->dev, vhost_net_get_feature_bits(net),
> > > > > > > -            features);
> > > > > > > +    uint64_t ret = vhost_get_features(&net->dev,
> > > > > > > +                                      vhost_net_get_feature_bits(net),
> > > > > > > +                                      features);
> > > > > > > +
> > > > > > > +    return ret | vhost_net_add_feature_bits(net);
> > > > > > >   }
> > > > > > >   int vhost_net_get_config(struct vhost_net *net,  uint8_t *config,
> > > > > > >                            uint32_t config_len)
> > > > > > > diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
> > > > > > > index 6d64000202..24d2857593 100644
> > > > > > > --- a/net/vhost-vdpa.c
> > > > > > > +++ b/net/vhost-vdpa.c
> > > > > > > @@ -99,6 +99,9 @@ static const uint64_t vdpa_svq_device_features =
> > > > > > >       BIT_ULL(VIRTIO_NET_F_RSC_EXT) |
> > > > > > >       BIT_ULL(VIRTIO_NET_F_STANDBY);
> > > > > > >
> > > > > > > +const uint64_t vhost_vdpa_net_added_feature_bits =
> > > > > > > +    BIT_ULL(VIRTIO_NET_F_STATUS);
> > > > > > > +
> > > > > > >   VHostNetState *vhost_vdpa_get_vhost_net(NetClientState *nc)
> > > > > > >   {
> > > > > > >       VhostVDPAState *s = DO_UPCAST(VhostVDPAState, nc, nc);
> > > > > >
> > > > >
> > > >
> > >
> >
>

Re: [PATCH 3/3] vdpa: Expose VIRTIO_NET_F_STATUS unconditionally

[Eugenio Perez Martin]

On Tue, Nov 1, 2022 at 9:10 AM Jason Wang <jasowang@redhat.com> wrote:
>
> On Fri, Oct 28, 2022 at 5:30 PM Eugenio Perez Martin
> <eperezma@redhat.com> wrote:
> >
> > On Fri, Oct 28, 2022 at 3:59 AM Jason Wang <jasowang@redhat.com> wrote:
> > >
> > > On Thu, Oct 27, 2022 at 6:18 PM Eugenio Perez Martin
> > > <eperezma@redhat.com> wrote:
> > > >
> > > > On Thu, Oct 27, 2022 at 8:54 AM Jason Wang <jasowang@redhat.com> wrote:
> > > > >
> > > > > On Thu, Oct 27, 2022 at 2:47 PM Eugenio Perez Martin
> > > > > <eperezma@redhat.com> wrote:
> > > > > >
> > > > > > On Thu, Oct 27, 2022 at 6:32 AM Jason Wang <jasowang@redhat.com> wrote:
> > > > > > >
> > > > > > >
> > > > > > > 在 2022/10/26 17:53, Eugenio Pérez 写道:
> > > > > > > > Now that qemu can handle and emulate it if the vdpa backend does not
> > > > > > > > support it we can offer it always.
> > > > > > > >
> > > > > > > > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > > > > > >
> > > > > > >
> > > > > > > I may miss something but isn't more easier to simply remove the
> > > > > > > _F_STATUS from vdpa_feature_bits[]?
> > > > > > >
> > > > > >
> > > > > > How is that? if we remove it, the guest cannot ack it so it cannot
> > > > > > access the net status, isn't it?
> > > > >
> > > > > My understanding is that the bits stored in the vdpa_feature_bits[]
> > > > > are the features that must be explicitly supported by the vhost
> > > > > device.
> > > >
> > > > (Non English native here, so maybe I don't get what you mean :) ) The
> > > > device may not support them. net simulator lacks some of them
> > > > actually, and it works.
> > >
> > > Speaking too fast, I think I meant that, if the bit doesn't belong to
> > > vdpa_feature_bits[], it is assumed to be supported by the Qemu without
> > > the support of the vhost. So Qemu won't even try to validate if vhost
> > > has this support. E.g for vhost-net, we only have:
> > >
> > > static const int kernel_feature_bits[] = {
> > >     VIRTIO_F_NOTIFY_ON_EMPTY,
> > >     VIRTIO_RING_F_INDIRECT_DESC,
> > >     VIRTIO_RING_F_EVENT_IDX,
> > >     VIRTIO_NET_F_MRG_RXBUF,
> > >     VIRTIO_F_VERSION_1,
> > >     VIRTIO_NET_F_MTU,
> > >     VIRTIO_F_IOMMU_PLATFORM,
> > >     VIRTIO_F_RING_PACKED,
> > >     VIRTIO_NET_F_HASH_REPORT,
> > >     VHOST_INVALID_FEATURE_BIT
> > > };
> > >
> > > You can see there's no STATUS bit there since it is emulated by Qemu.
> > >
> >
> > Ok now I get what you mean, and yes we may modify the patches in that direction.
> >
> > But if we go then we need to modify how qemu ack the features, because
> > the features that are not in vdpa_feature_bits are not acked to the
> > device. More on this later.
> >
> > > >
> > > > From what I see these are the only features that will be forwarded to
> > > > the guest as device_features. If it is not in the list, the feature
> > > > will be masked out,
> > >
> > > Only when there's no support for this feature from the vhost.
> > >
> > > > as if the device does not support it.
> > > >
> > > > So now _F_STATUS it was forwarded only if the device supports it. If
> > > > we remove it from bit_mask, it will never be offered to the guest. But
> > > > we want to offer it always, since we will need it for
> > > > _F_GUEST_ANNOUNCE.
> > > >
> > > > Things get more complex because we actually need to ack it back if the
> > > > device offers it, so the vdpa device can report link_down. We will
> > > > only emulate LINK_UP always in the case the device does not support
> > > > _F_STATUS.
> > > >
> > > > > So if we remove _F_STATUS, Qemu vhost code won't validate if
> > > > > vhost-vdpa device has this support:
> > > > >
> > > > > uint64_t vhost_get_features(struct vhost_dev *hdev, const int *feature_bits,
> > > > >                             uint64_t features)
> > > > > {
> > > > >     const int *bit = feature_bits;
> > > > >     while (*bit != VHOST_INVALID_FEATURE_BIT) {
> > > > >         uint64_t bit_mask = (1ULL << *bit);
> > > > >         if (!(hdev->features & bit_mask)) {
> > > > >             features &= ~bit_mask;
> > > > >         }
> > > > >         bit++;
> > > > >     }
> > > > >     return features;
> > > > > }
> > > > >
> > > >
> > > > Now maybe I'm the one missing something, but why is this not done as a
> > > > masking directly?
> > >
> > > Not sure, the code has been there since day 0.
> > >
> > > But you can see from the code:
> > >
> > > 1) if STATUS is in feature_bits, we need validate the hdev->features
> > > and mask it if the vhost doesn't have the support
> > > 2) if STATUS is not, we don't do the check and driver may still see STATUS
> > >
> >
> > That's useful for _F_GUEST_ANNOUNCE, but we need to ack _F_STATUS for
> > the device if it supports it.
>
> Rethink about this, I don't see why ANNOUNCE depends on STATUS (spec
> doesn't say so).
>

It is needed for the guest to read the status bit:
"""
status only exists if VIRTIO_NET_F_STATUS is set. Two read-only bits
(for the driver) are currently defined for the status field:
VIRTIO_NET_S_LINK_UP and VIRTIO_NET_S_ANNOUNCE.
"""

A change on the standard could be possible, like "status only exists
if VIRTIO_NET_F_STATUS or VIRTIO_NET_F_GUEST_ANNOUNCE is set".
However, Linux drivers already expect _F_STATUS to read _S_ANNOUNCE
and to emulate _F_STATUS in case the device doesn't support it should
not be a big deal in my opinion.

> > QEMU cannot detect by itself when the
> > link is not up. I think that setting unconditionally
> > VIRTIO_NET_S_LINK_UP is actually a regression, since the guest cannot
> > detect the link down that way.
>
> I think the idea is to still read status from config if the device
> supports this.
>

Yes, that's my point. If I delete it from vdpa_feature_bits, it will
not be acked to the device, so we cannot read status from the device.

> >
> > To enable _F_STATUS unconditionally is only done in the case the
> > device does not support it, because its emulation is very easy. That
> > way we support _F_GUEST_ANNOUNCE in all cases without device's
> > cooperation.
> >
> > Having said that, should we go the opposite route and ack _F_STATE as
> > long as the device supports it? As an advantage, all backends should
> > support that at this moment, isn't it?
>
> So I think the method used in this patch is fine, but I wonder if it's
> better to move it to the vhost layer instead of doing it in vhost_net
> since we do the features validation there. We probably need another
> table as input for get/set features there?
>

We can discuss how to do it for sure. But as you pointed out,
vhost_net and virtio_net already modify the features received from the
devices, so it makes sense to me to modify the features set by the
guest.

The problem is that we need to transmit to vhost when ack _F_STATUS
and when not. The first proposal was to add a new member of vhost_vdpa
but this is not optimal.

If we add a new table it should be a static const one, and vhost_vdpa
should have a pointer to it, isn't it? Something like features that
are emulated by qemu so they must be offered always to the guest?

Thanks!

> Thanks
>
> >
> > Thanks!
> >
> >
> >
> >
> > > Thanks
> > >
> > > >
> > > > Instead of making feature_bits an array of ints, to declare it as a
> > > > uint64_t with the valid feature bits and simply return features &
> > > > feature_bits.
> > > >
> > > > Thanks!
> > > >
> > > > > Thanks
> > > > >
> > > > >
> > > > >
> > > > > >
> > > > > > The goal with this patch series is to let the guest access the status
> > > > > > always, even if the device doesn't support _F_STATUS.
> > > > > >
> > > > > > > Thanks
> > > > > > >
> > > > > > >
> > > > > > > > ---
> > > > > > > >   include/net/vhost-vdpa.h |  1 +
> > > > > > > >   hw/net/vhost_net.c       | 16 ++++++++++++++--
> > > > > > > >   net/vhost-vdpa.c         |  3 +++
> > > > > > > >   3 files changed, 18 insertions(+), 2 deletions(-)
> > > > > > > >
> > > > > > > > diff --git a/include/net/vhost-vdpa.h b/include/net/vhost-vdpa.h
> > > > > > > > index b81f9a6f2a..cfbcce6427 100644
> > > > > > > > --- a/include/net/vhost-vdpa.h
> > > > > > > > +++ b/include/net/vhost-vdpa.h
> > > > > > > > @@ -17,5 +17,6 @@
> > > > > > > >   struct vhost_net *vhost_vdpa_get_vhost_net(NetClientState *nc);
> > > > > > > >
> > > > > > > >   extern const int vdpa_feature_bits[];
> > > > > > > > +extern const uint64_t vhost_vdpa_net_added_feature_bits;
> > > > > > > >
> > > > > > > >   #endif /* VHOST_VDPA_H */
> > > > > > > > diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
> > > > > > > > index d28f8b974b..7c15cc6e8f 100644
> > > > > > > > --- a/hw/net/vhost_net.c
> > > > > > > > +++ b/hw/net/vhost_net.c
> > > > > > > > @@ -109,10 +109,22 @@ static const int *vhost_net_get_feature_bits(struct vhost_net *net)
> > > > > > > >       return feature_bits;
> > > > > > > >   }
> > > > > > > >
> > > > > > > > +static uint64_t vhost_net_add_feature_bits(struct vhost_net *net)
> > > > > > > > +{
> > > > > > > > +    if (net->nc->info->type == NET_CLIENT_DRIVER_VHOST_VDPA) {
> > > > > > > > +        return vhost_vdpa_net_added_feature_bits;
> > > > > > > > +    }
> > > > > > > > +
> > > > > > > > +    return 0;
> > > > > > > > +}
> > > > > > > > +
> > > > > > > >   uint64_t vhost_net_get_features(struct vhost_net *net, uint64_t features)
> > > > > > > >   {
> > > > > > > > -    return vhost_get_features(&net->dev, vhost_net_get_feature_bits(net),
> > > > > > > > -            features);
> > > > > > > > +    uint64_t ret = vhost_get_features(&net->dev,
> > > > > > > > +                                      vhost_net_get_feature_bits(net),
> > > > > > > > +                                      features);
> > > > > > > > +
> > > > > > > > +    return ret | vhost_net_add_feature_bits(net);
> > > > > > > >   }
> > > > > > > >   int vhost_net_get_config(struct vhost_net *net,  uint8_t *config,
> > > > > > > >                            uint32_t config_len)
> > > > > > > > diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
> > > > > > > > index 6d64000202..24d2857593 100644
> > > > > > > > --- a/net/vhost-vdpa.c
> > > > > > > > +++ b/net/vhost-vdpa.c
> > > > > > > > @@ -99,6 +99,9 @@ static const uint64_t vdpa_svq_device_features =
> > > > > > > >       BIT_ULL(VIRTIO_NET_F_RSC_EXT) |
> > > > > > > >       BIT_ULL(VIRTIO_NET_F_STANDBY);
> > > > > > > >
> > > > > > > > +const uint64_t vhost_vdpa_net_added_feature_bits =
> > > > > > > > +    BIT_ULL(VIRTIO_NET_F_STATUS);
> > > > > > > > +
> > > > > > > >   VHostNetState *vhost_vdpa_get_vhost_net(NetClientState *nc)
> > > > > > > >   {
> > > > > > > >       VhostVDPAState *s = DO_UPCAST(VhostVDPAState, nc, nc);
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>

Re: [PATCH 3/3] vdpa: Expose VIRTIO_NET_F_STATUS unconditionally

[Jason Wang]

On Wed, Nov 2, 2022 at 7:19 PM Eugenio Perez Martin <eperezma@redhat.com> wrote:
>
> On Tue, Nov 1, 2022 at 9:10 AM Jason Wang <jasowang@redhat.com> wrote:
> >
> > On Fri, Oct 28, 2022 at 5:30 PM Eugenio Perez Martin
> > <eperezma@redhat.com> wrote:
> > >
> > > On Fri, Oct 28, 2022 at 3:59 AM Jason Wang <jasowang@redhat.com> wrote:
> > > >
> > > > On Thu, Oct 27, 2022 at 6:18 PM Eugenio Perez Martin
> > > > <eperezma@redhat.com> wrote:
> > > > >
> > > > > On Thu, Oct 27, 2022 at 8:54 AM Jason Wang <jasowang@redhat.com> wrote:
> > > > > >
> > > > > > On Thu, Oct 27, 2022 at 2:47 PM Eugenio Perez Martin
> > > > > > <eperezma@redhat.com> wrote:
> > > > > > >
> > > > > > > On Thu, Oct 27, 2022 at 6:32 AM Jason Wang <jasowang@redhat.com> wrote:
> > > > > > > >
> > > > > > > >
> > > > > > > > 在 2022/10/26 17:53, Eugenio Pérez 写道:
> > > > > > > > > Now that qemu can handle and emulate it if the vdpa backend does not
> > > > > > > > > support it we can offer it always.
> > > > > > > > >
> > > > > > > > > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > > > > > > >
> > > > > > > >
> > > > > > > > I may miss something but isn't more easier to simply remove the
> > > > > > > > _F_STATUS from vdpa_feature_bits[]?
> > > > > > > >
> > > > > > >
> > > > > > > How is that? if we remove it, the guest cannot ack it so it cannot
> > > > > > > access the net status, isn't it?
> > > > > >
> > > > > > My understanding is that the bits stored in the vdpa_feature_bits[]
> > > > > > are the features that must be explicitly supported by the vhost
> > > > > > device.
> > > > >
> > > > > (Non English native here, so maybe I don't get what you mean :) ) The
> > > > > device may not support them. net simulator lacks some of them
> > > > > actually, and it works.
> > > >
> > > > Speaking too fast, I think I meant that, if the bit doesn't belong to
> > > > vdpa_feature_bits[], it is assumed to be supported by the Qemu without
> > > > the support of the vhost. So Qemu won't even try to validate if vhost
> > > > has this support. E.g for vhost-net, we only have:
> > > >
> > > > static const int kernel_feature_bits[] = {
> > > >     VIRTIO_F_NOTIFY_ON_EMPTY,
> > > >     VIRTIO_RING_F_INDIRECT_DESC,
> > > >     VIRTIO_RING_F_EVENT_IDX,
> > > >     VIRTIO_NET_F_MRG_RXBUF,
> > > >     VIRTIO_F_VERSION_1,
> > > >     VIRTIO_NET_F_MTU,
> > > >     VIRTIO_F_IOMMU_PLATFORM,
> > > >     VIRTIO_F_RING_PACKED,
> > > >     VIRTIO_NET_F_HASH_REPORT,
> > > >     VHOST_INVALID_FEATURE_BIT
> > > > };
> > > >
> > > > You can see there's no STATUS bit there since it is emulated by Qemu.
> > > >
> > >
> > > Ok now I get what you mean, and yes we may modify the patches in that direction.
> > >
> > > But if we go then we need to modify how qemu ack the features, because
> > > the features that are not in vdpa_feature_bits are not acked to the
> > > device. More on this later.
> > >
> > > > >
> > > > > From what I see these are the only features that will be forwarded to
> > > > > the guest as device_features. If it is not in the list, the feature
> > > > > will be masked out,
> > > >
> > > > Only when there's no support for this feature from the vhost.
> > > >
> > > > > as if the device does not support it.
> > > > >
> > > > > So now _F_STATUS it was forwarded only if the device supports it. If
> > > > > we remove it from bit_mask, it will never be offered to the guest. But
> > > > > we want to offer it always, since we will need it for
> > > > > _F_GUEST_ANNOUNCE.
> > > > >
> > > > > Things get more complex because we actually need to ack it back if the
> > > > > device offers it, so the vdpa device can report link_down. We will
> > > > > only emulate LINK_UP always in the case the device does not support
> > > > > _F_STATUS.
> > > > >
> > > > > > So if we remove _F_STATUS, Qemu vhost code won't validate if
> > > > > > vhost-vdpa device has this support:
> > > > > >
> > > > > > uint64_t vhost_get_features(struct vhost_dev *hdev, const int *feature_bits,
> > > > > >                             uint64_t features)
> > > > > > {
> > > > > >     const int *bit = feature_bits;
> > > > > >     while (*bit != VHOST_INVALID_FEATURE_BIT) {
> > > > > >         uint64_t bit_mask = (1ULL << *bit);
> > > > > >         if (!(hdev->features & bit_mask)) {
> > > > > >             features &= ~bit_mask;
> > > > > >         }
> > > > > >         bit++;
> > > > > >     }
> > > > > >     return features;
> > > > > > }
> > > > > >
> > > > >
> > > > > Now maybe I'm the one missing something, but why is this not done as a
> > > > > masking directly?
> > > >
> > > > Not sure, the code has been there since day 0.
> > > >
> > > > But you can see from the code:
> > > >
> > > > 1) if STATUS is in feature_bits, we need validate the hdev->features
> > > > and mask it if the vhost doesn't have the support
> > > > 2) if STATUS is not, we don't do the check and driver may still see STATUS
> > > >
> > >
> > > That's useful for _F_GUEST_ANNOUNCE, but we need to ack _F_STATUS for
> > > the device if it supports it.
> >
> > Rethink about this, I don't see why ANNOUNCE depends on STATUS (spec
> > doesn't say so).
> >
>
> It is needed for the guest to read the status bit:
> """
> status only exists if VIRTIO_NET_F_STATUS is set. Two read-only bits
> (for the driver) are currently defined for the status field:
> VIRTIO_NET_S_LINK_UP and VIRTIO_NET_S_ANNOUNCE.
> """
>
> A change on the standard could be possible, like "status only exists
> if VIRTIO_NET_F_STATUS or VIRTIO_NET_F_GUEST_ANNOUNCE is set".
> However, Linux drivers already expect _F_STATUS to read _S_ANNOUNCE
> and to emulate _F_STATUS in case the device doesn't support it should
> not be a big deal in my opinion.

RIght, so I think we need a spec patch to clarify the dependency,
currently, spec said ANNOUNCE depends on CTRL_VQ.

>
> > > QEMU cannot detect by itself when the
> > > link is not up. I think that setting unconditionally
> > > VIRTIO_NET_S_LINK_UP is actually a regression, since the guest cannot
> > > detect the link down that way.
> >
> > I think the idea is to still read status from config if the device
> > supports this.
> >
>
> Yes, that's my point. If I delete it from vdpa_feature_bits, it will
> not be acked to the device, so we cannot read status from the device.
>
> > >
> > > To enable _F_STATUS unconditionally is only done in the case the
> > > device does not support it, because its emulation is very easy. That
> > > way we support _F_GUEST_ANNOUNCE in all cases without device's
> > > cooperation.
> > >
> > > Having said that, should we go the opposite route and ack _F_STATE as
> > > long as the device supports it? As an advantage, all backends should
> > > support that at this moment, isn't it?
> >
> > So I think the method used in this patch is fine, but I wonder if it's
> > better to move it to the vhost layer instead of doing it in vhost_net
> > since we do the features validation there. We probably need another
> > table as input for get/set features there?
> >
>
> We can discuss how to do it for sure. But as you pointed out,
> vhost_net and virtio_net already modify the features received from the
> devices, so it makes sense to me to modify the features set by the
> guest.
>
> The problem is that we need to transmit to vhost when ack _F_STATUS
> and when not. The first proposal was to add a new member of vhost_vdpa
> but this is not optimal.
>
> If we add a new table it should be a static const one, and vhost_vdpa
> should have a pointer to it, isn't it?

Yes.

> Something like features that
> are emulated by qemu so they must be offered always to the guest?

Kind of, actually it should be the features:

1) could be always seen by guest
2) when vhost device have this feature, use that
3) when vhost device doesn't have this feature, emulate one

But a question still, is there a vDPA parent that can't do _F_STATUS
now (if not, we probably don't need to bother now).

Thanks

>
> Thanks!
>
> > Thanks
> >
> > >
> > > Thanks!
> > >
> > >
> > >
> > >
> > > > Thanks
> > > >
> > > > >
> > > > > Instead of making feature_bits an array of ints, to declare it as a
> > > > > uint64_t with the valid feature bits and simply return features &
> > > > > feature_bits.
> > > > >
> > > > > Thanks!
> > > > >
> > > > > > Thanks
> > > > > >
> > > > > >
> > > > > >
> > > > > > >
> > > > > > > The goal with this patch series is to let the guest access the status
> > > > > > > always, even if the device doesn't support _F_STATUS.
> > > > > > >
> > > > > > > > Thanks
> > > > > > > >
> > > > > > > >
> > > > > > > > > ---
> > > > > > > > >   include/net/vhost-vdpa.h |  1 +
> > > > > > > > >   hw/net/vhost_net.c       | 16 ++++++++++++++--
> > > > > > > > >   net/vhost-vdpa.c         |  3 +++
> > > > > > > > >   3 files changed, 18 insertions(+), 2 deletions(-)
> > > > > > > > >
> > > > > > > > > diff --git a/include/net/vhost-vdpa.h b/include/net/vhost-vdpa.h
> > > > > > > > > index b81f9a6f2a..cfbcce6427 100644
> > > > > > > > > --- a/include/net/vhost-vdpa.h
> > > > > > > > > +++ b/include/net/vhost-vdpa.h
> > > > > > > > > @@ -17,5 +17,6 @@
> > > > > > > > >   struct vhost_net *vhost_vdpa_get_vhost_net(NetClientState *nc);
> > > > > > > > >
> > > > > > > > >   extern const int vdpa_feature_bits[];
> > > > > > > > > +extern const uint64_t vhost_vdpa_net_added_feature_bits;
> > > > > > > > >
> > > > > > > > >   #endif /* VHOST_VDPA_H */
> > > > > > > > > diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
> > > > > > > > > index d28f8b974b..7c15cc6e8f 100644
> > > > > > > > > --- a/hw/net/vhost_net.c
> > > > > > > > > +++ b/hw/net/vhost_net.c
> > > > > > > > > @@ -109,10 +109,22 @@ static const int *vhost_net_get_feature_bits(struct vhost_net *net)
> > > > > > > > >       return feature_bits;
> > > > > > > > >   }
> > > > > > > > >
> > > > > > > > > +static uint64_t vhost_net_add_feature_bits(struct vhost_net *net)
> > > > > > > > > +{
> > > > > > > > > +    if (net->nc->info->type == NET_CLIENT_DRIVER_VHOST_VDPA) {
> > > > > > > > > +        return vhost_vdpa_net_added_feature_bits;
> > > > > > > > > +    }
> > > > > > > > > +
> > > > > > > > > +    return 0;
> > > > > > > > > +}
> > > > > > > > > +
> > > > > > > > >   uint64_t vhost_net_get_features(struct vhost_net *net, uint64_t features)
> > > > > > > > >   {
> > > > > > > > > -    return vhost_get_features(&net->dev, vhost_net_get_feature_bits(net),
> > > > > > > > > -            features);
> > > > > > > > > +    uint64_t ret = vhost_get_features(&net->dev,
> > > > > > > > > +                                      vhost_net_get_feature_bits(net),
> > > > > > > > > +                                      features);
> > > > > > > > > +
> > > > > > > > > +    return ret | vhost_net_add_feature_bits(net);
> > > > > > > > >   }
> > > > > > > > >   int vhost_net_get_config(struct vhost_net *net,  uint8_t *config,
> > > > > > > > >                            uint32_t config_len)
> > > > > > > > > diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
> > > > > > > > > index 6d64000202..24d2857593 100644
> > > > > > > > > --- a/net/vhost-vdpa.c
> > > > > > > > > +++ b/net/vhost-vdpa.c
> > > > > > > > > @@ -99,6 +99,9 @@ static const uint64_t vdpa_svq_device_features =
> > > > > > > > >       BIT_ULL(VIRTIO_NET_F_RSC_EXT) |
> > > > > > > > >       BIT_ULL(VIRTIO_NET_F_STANDBY);
> > > > > > > > >
> > > > > > > > > +const uint64_t vhost_vdpa_net_added_feature_bits =
> > > > > > > > > +    BIT_ULL(VIRTIO_NET_F_STATUS);
> > > > > > > > > +
> > > > > > > > >   VHostNetState *vhost_vdpa_get_vhost_net(NetClientState *nc)
> > > > > > > > >   {
> > > > > > > > >       VhostVDPAState *s = DO_UPCAST(VhostVDPAState, nc, nc);
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>

Re: [PATCH 3/3] vdpa: Expose VIRTIO_NET_F_STATUS unconditionally

[Eugenio Perez Martin]

On Thu, Nov 3, 2022 at 4:21 AM Jason Wang <jasowang@redhat.com> wrote:
>
> On Wed, Nov 2, 2022 at 7:19 PM Eugenio Perez Martin <eperezma@redhat.com> wrote:
> >
> > On Tue, Nov 1, 2022 at 9:10 AM Jason Wang <jasowang@redhat.com> wrote:
> > >
> > > On Fri, Oct 28, 2022 at 5:30 PM Eugenio Perez Martin
> > > <eperezma@redhat.com> wrote:
> > > >
> > > > On Fri, Oct 28, 2022 at 3:59 AM Jason Wang <jasowang@redhat.com> wrote:
> > > > >
> > > > > On Thu, Oct 27, 2022 at 6:18 PM Eugenio Perez Martin
> > > > > <eperezma@redhat.com> wrote:
> > > > > >
> > > > > > On Thu, Oct 27, 2022 at 8:54 AM Jason Wang <jasowang@redhat.com> wrote:
> > > > > > >
> > > > > > > On Thu, Oct 27, 2022 at 2:47 PM Eugenio Perez Martin
> > > > > > > <eperezma@redhat.com> wrote:
> > > > > > > >
> > > > > > > > On Thu, Oct 27, 2022 at 6:32 AM Jason Wang <jasowang@redhat.com> wrote:
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > 在 2022/10/26 17:53, Eugenio Pérez 写道:
> > > > > > > > > > Now that qemu can handle and emulate it if the vdpa backend does not
> > > > > > > > > > support it we can offer it always.
> > > > > > > > > >
> > > > > > > > > > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > I may miss something but isn't more easier to simply remove the
> > > > > > > > > _F_STATUS from vdpa_feature_bits[]?
> > > > > > > > >
> > > > > > > >
> > > > > > > > How is that? if we remove it, the guest cannot ack it so it cannot
> > > > > > > > access the net status, isn't it?
> > > > > > >
> > > > > > > My understanding is that the bits stored in the vdpa_feature_bits[]
> > > > > > > are the features that must be explicitly supported by the vhost
> > > > > > > device.
> > > > > >
> > > > > > (Non English native here, so maybe I don't get what you mean :) ) The
> > > > > > device may not support them. net simulator lacks some of them
> > > > > > actually, and it works.
> > > > >
> > > > > Speaking too fast, I think I meant that, if the bit doesn't belong to
> > > > > vdpa_feature_bits[], it is assumed to be supported by the Qemu without
> > > > > the support of the vhost. So Qemu won't even try to validate if vhost
> > > > > has this support. E.g for vhost-net, we only have:
> > > > >
> > > > > static const int kernel_feature_bits[] = {
> > > > >     VIRTIO_F_NOTIFY_ON_EMPTY,
> > > > >     VIRTIO_RING_F_INDIRECT_DESC,
> > > > >     VIRTIO_RING_F_EVENT_IDX,
> > > > >     VIRTIO_NET_F_MRG_RXBUF,
> > > > >     VIRTIO_F_VERSION_1,
> > > > >     VIRTIO_NET_F_MTU,
> > > > >     VIRTIO_F_IOMMU_PLATFORM,
> > > > >     VIRTIO_F_RING_PACKED,
> > > > >     VIRTIO_NET_F_HASH_REPORT,
> > > > >     VHOST_INVALID_FEATURE_BIT
> > > > > };
> > > > >
> > > > > You can see there's no STATUS bit there since it is emulated by Qemu.
> > > > >
> > > >
> > > > Ok now I get what you mean, and yes we may modify the patches in that direction.
> > > >
> > > > But if we go then we need to modify how qemu ack the features, because
> > > > the features that are not in vdpa_feature_bits are not acked to the
> > > > device. More on this later.
> > > >
> > > > > >
> > > > > > From what I see these are the only features that will be forwarded to
> > > > > > the guest as device_features. If it is not in the list, the feature
> > > > > > will be masked out,
> > > > >
> > > > > Only when there's no support for this feature from the vhost.
> > > > >
> > > > > > as if the device does not support it.
> > > > > >
> > > > > > So now _F_STATUS it was forwarded only if the device supports it. If
> > > > > > we remove it from bit_mask, it will never be offered to the guest. But
> > > > > > we want to offer it always, since we will need it for
> > > > > > _F_GUEST_ANNOUNCE.
> > > > > >
> > > > > > Things get more complex because we actually need to ack it back if the
> > > > > > device offers it, so the vdpa device can report link_down. We will
> > > > > > only emulate LINK_UP always in the case the device does not support
> > > > > > _F_STATUS.
> > > > > >
> > > > > > > So if we remove _F_STATUS, Qemu vhost code won't validate if
> > > > > > > vhost-vdpa device has this support:
> > > > > > >
> > > > > > > uint64_t vhost_get_features(struct vhost_dev *hdev, const int *feature_bits,
> > > > > > >                             uint64_t features)
> > > > > > > {
> > > > > > >     const int *bit = feature_bits;
> > > > > > >     while (*bit != VHOST_INVALID_FEATURE_BIT) {
> > > > > > >         uint64_t bit_mask = (1ULL << *bit);
> > > > > > >         if (!(hdev->features & bit_mask)) {
> > > > > > >             features &= ~bit_mask;
> > > > > > >         }
> > > > > > >         bit++;
> > > > > > >     }
> > > > > > >     return features;
> > > > > > > }
> > > > > > >
> > > > > >
> > > > > > Now maybe I'm the one missing something, but why is this not done as a
> > > > > > masking directly?
> > > > >
> > > > > Not sure, the code has been there since day 0.
> > > > >
> > > > > But you can see from the code:
> > > > >
> > > > > 1) if STATUS is in feature_bits, we need validate the hdev->features
> > > > > and mask it if the vhost doesn't have the support
> > > > > 2) if STATUS is not, we don't do the check and driver may still see STATUS
> > > > >
> > > >
> > > > That's useful for _F_GUEST_ANNOUNCE, but we need to ack _F_STATUS for
> > > > the device if it supports it.
> > >
> > > Rethink about this, I don't see why ANNOUNCE depends on STATUS (spec
> > > doesn't say so).
> > >
> >
> > It is needed for the guest to read the status bit:
> > """
> > status only exists if VIRTIO_NET_F_STATUS is set. Two read-only bits
> > (for the driver) are currently defined for the status field:
> > VIRTIO_NET_S_LINK_UP and VIRTIO_NET_S_ANNOUNCE.
> > """
> >
> > A change on the standard could be possible, like "status only exists
> > if VIRTIO_NET_F_STATUS or VIRTIO_NET_F_GUEST_ANNOUNCE is set".
> > However, Linux drivers already expect _F_STATUS to read _S_ANNOUNCE
> > and to emulate _F_STATUS in case the device doesn't support it should
> > not be a big deal in my opinion.
>
> RIght, so I think we need a spec patch to clarify the dependency,
> currently, spec said ANNOUNCE depends on CTRL_VQ.
>

Would it be enough to expand it under the "Feature bit requirements" section?

> >
> > > > QEMU cannot detect by itself when the
> > > > link is not up. I think that setting unconditionally
> > > > VIRTIO_NET_S_LINK_UP is actually a regression, since the guest cannot
> > > > detect the link down that way.
> > >
> > > I think the idea is to still read status from config if the device
> > > supports this.
> > >
> >
> > Yes, that's my point. If I delete it from vdpa_feature_bits, it will
> > not be acked to the device, so we cannot read status from the device.
> >
> > > >
> > > > To enable _F_STATUS unconditionally is only done in the case the
> > > > device does not support it, because its emulation is very easy. That
> > > > way we support _F_GUEST_ANNOUNCE in all cases without device's
> > > > cooperation.
> > > >
> > > > Having said that, should we go the opposite route and ack _F_STATE as
> > > > long as the device supports it? As an advantage, all backends should
> > > > support that at this moment, isn't it?
> > >
> > > So I think the method used in this patch is fine, but I wonder if it's
> > > better to move it to the vhost layer instead of doing it in vhost_net
> > > since we do the features validation there. We probably need another
> > > table as input for get/set features there?
> > >
> >
> > We can discuss how to do it for sure. But as you pointed out,
> > vhost_net and virtio_net already modify the features received from the
> > devices, so it makes sense to me to modify the features set by the
> > guest.
> >
> > The problem is that we need to transmit to vhost when ack _F_STATUS
> > and when not. The first proposal was to add a new member of vhost_vdpa
> > but this is not optimal.
> >
> > If we add a new table it should be a static const one, and vhost_vdpa
> > should have a pointer to it, isn't it?
>
> Yes.
>
> > Something like features that
> > are emulated by qemu so they must be offered always to the guest?
>
> Kind of, actually it should be the features:
>
> 1) could be always seen by guest
> 2) when vhost device have this feature, use that
> 3) when vhost device doesn't have this feature, emulate one
>

I'm fine with that approach, but restricting the changes to either
vhost_net or virtio_net makes more sense to me. The net config space
interception goes to virtio_net anyway, not to vhost-vdpa.

I'll try to prepare the patches with a new array.

> But a question still, is there a vDPA parent that can't do _F_STATUS
> now (if not, we probably don't need to bother now).
>

Only mlx have _F_STATUS at this moment.

If I understand you correctly, you are proposing not to emulate
_F_STATUS if the device does not support it? To emulate _F_STATUS is
not a big deal emulating _F_GUEST_ANNOUNCE on top anyway.

Thanks!

> Thanks
>
> >
> > Thanks!
> >
> > > Thanks
> > >
> > > >
> > > > Thanks!
> > > >
> > > >
> > > >
> > > >
> > > > > Thanks
> > > > >
> > > > > >
> > > > > > Instead of making feature_bits an array of ints, to declare it as a
> > > > > > uint64_t with the valid feature bits and simply return features &
> > > > > > feature_bits.
> > > > > >
> > > > > > Thanks!
> > > > > >
> > > > > > > Thanks
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > > > >
> > > > > > > > The goal with this patch series is to let the guest access the status
> > > > > > > > always, even if the device doesn't support _F_STATUS.
> > > > > > > >
> > > > > > > > > Thanks
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > > ---
> > > > > > > > > >   include/net/vhost-vdpa.h |  1 +
> > > > > > > > > >   hw/net/vhost_net.c       | 16 ++++++++++++++--
> > > > > > > > > >   net/vhost-vdpa.c         |  3 +++
> > > > > > > > > >   3 files changed, 18 insertions(+), 2 deletions(-)
> > > > > > > > > >
> > > > > > > > > > diff --git a/include/net/vhost-vdpa.h b/include/net/vhost-vdpa.h
> > > > > > > > > > index b81f9a6f2a..cfbcce6427 100644
> > > > > > > > > > --- a/include/net/vhost-vdpa.h
> > > > > > > > > > +++ b/include/net/vhost-vdpa.h
> > > > > > > > > > @@ -17,5 +17,6 @@
> > > > > > > > > >   struct vhost_net *vhost_vdpa_get_vhost_net(NetClientState *nc);
> > > > > > > > > >
> > > > > > > > > >   extern const int vdpa_feature_bits[];
> > > > > > > > > > +extern const uint64_t vhost_vdpa_net_added_feature_bits;
> > > > > > > > > >
> > > > > > > > > >   #endif /* VHOST_VDPA_H */
> > > > > > > > > > diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
> > > > > > > > > > index d28f8b974b..7c15cc6e8f 100644
> > > > > > > > > > --- a/hw/net/vhost_net.c
> > > > > > > > > > +++ b/hw/net/vhost_net.c
> > > > > > > > > > @@ -109,10 +109,22 @@ static const int *vhost_net_get_feature_bits(struct vhost_net *net)
> > > > > > > > > >       return feature_bits;
> > > > > > > > > >   }
> > > > > > > > > >
> > > > > > > > > > +static uint64_t vhost_net_add_feature_bits(struct vhost_net *net)
> > > > > > > > > > +{
> > > > > > > > > > +    if (net->nc->info->type == NET_CLIENT_DRIVER_VHOST_VDPA) {
> > > > > > > > > > +        return vhost_vdpa_net_added_feature_bits;
> > > > > > > > > > +    }
> > > > > > > > > > +
> > > > > > > > > > +    return 0;
> > > > > > > > > > +}
> > > > > > > > > > +
> > > > > > > > > >   uint64_t vhost_net_get_features(struct vhost_net *net, uint64_t features)
> > > > > > > > > >   {
> > > > > > > > > > -    return vhost_get_features(&net->dev, vhost_net_get_feature_bits(net),
> > > > > > > > > > -            features);
> > > > > > > > > > +    uint64_t ret = vhost_get_features(&net->dev,
> > > > > > > > > > +                                      vhost_net_get_feature_bits(net),
> > > > > > > > > > +                                      features);
> > > > > > > > > > +
> > > > > > > > > > +    return ret | vhost_net_add_feature_bits(net);
> > > > > > > > > >   }
> > > > > > > > > >   int vhost_net_get_config(struct vhost_net *net,  uint8_t *config,
> > > > > > > > > >                            uint32_t config_len)
> > > > > > > > > > diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
> > > > > > > > > > index 6d64000202..24d2857593 100644
> > > > > > > > > > --- a/net/vhost-vdpa.c
> > > > > > > > > > +++ b/net/vhost-vdpa.c
> > > > > > > > > > @@ -99,6 +99,9 @@ static const uint64_t vdpa_svq_device_features =
> > > > > > > > > >       BIT_ULL(VIRTIO_NET_F_RSC_EXT) |
> > > > > > > > > >       BIT_ULL(VIRTIO_NET_F_STANDBY);
> > > > > > > > > >
> > > > > > > > > > +const uint64_t vhost_vdpa_net_added_feature_bits =
> > > > > > > > > > +    BIT_ULL(VIRTIO_NET_F_STATUS);
> > > > > > > > > > +
> > > > > > > > > >   VHostNetState *vhost_vdpa_get_vhost_net(NetClientState *nc)
> > > > > > > > > >   {
> > > > > > > > > >       VhostVDPAState *s = DO_UPCAST(VhostVDPAState, nc, nc);
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>

Re: [PATCH 3/3] vdpa: Expose VIRTIO_NET_F_STATUS unconditionally

[Jason Wang]

On Thu, Nov 3, 2022 at 4:12 PM Eugenio Perez Martin <eperezma@redhat.com> wrote:
>
> On Thu, Nov 3, 2022 at 4:21 AM Jason Wang <jasowang@redhat.com> wrote:
> >
> > On Wed, Nov 2, 2022 at 7:19 PM Eugenio Perez Martin <eperezma@redhat.com> wrote:
> > >
> > > On Tue, Nov 1, 2022 at 9:10 AM Jason Wang <jasowang@redhat.com> wrote:
> > > >
> > > > On Fri, Oct 28, 2022 at 5:30 PM Eugenio Perez Martin
> > > > <eperezma@redhat.com> wrote:
> > > > >
> > > > > On Fri, Oct 28, 2022 at 3:59 AM Jason Wang <jasowang@redhat.com> wrote:
> > > > > >
> > > > > > On Thu, Oct 27, 2022 at 6:18 PM Eugenio Perez Martin
> > > > > > <eperezma@redhat.com> wrote:
> > > > > > >
> > > > > > > On Thu, Oct 27, 2022 at 8:54 AM Jason Wang <jasowang@redhat.com> wrote:
> > > > > > > >
> > > > > > > > On Thu, Oct 27, 2022 at 2:47 PM Eugenio Perez Martin
> > > > > > > > <eperezma@redhat.com> wrote:
> > > > > > > > >
> > > > > > > > > On Thu, Oct 27, 2022 at 6:32 AM Jason Wang <jasowang@redhat.com> wrote:
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > 在 2022/10/26 17:53, Eugenio Pérez 写道:
> > > > > > > > > > > Now that qemu can handle and emulate it if the vdpa backend does not
> > > > > > > > > > > support it we can offer it always.
> > > > > > > > > > >
> > > > > > > > > > > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > I may miss something but isn't more easier to simply remove the
> > > > > > > > > > _F_STATUS from vdpa_feature_bits[]?
> > > > > > > > > >
> > > > > > > > >
> > > > > > > > > How is that? if we remove it, the guest cannot ack it so it cannot
> > > > > > > > > access the net status, isn't it?
> > > > > > > >
> > > > > > > > My understanding is that the bits stored in the vdpa_feature_bits[]
> > > > > > > > are the features that must be explicitly supported by the vhost
> > > > > > > > device.
> > > > > > >
> > > > > > > (Non English native here, so maybe I don't get what you mean :) ) The
> > > > > > > device may not support them. net simulator lacks some of them
> > > > > > > actually, and it works.
> > > > > >
> > > > > > Speaking too fast, I think I meant that, if the bit doesn't belong to
> > > > > > vdpa_feature_bits[], it is assumed to be supported by the Qemu without
> > > > > > the support of the vhost. So Qemu won't even try to validate if vhost
> > > > > > has this support. E.g for vhost-net, we only have:
> > > > > >
> > > > > > static const int kernel_feature_bits[] = {
> > > > > >     VIRTIO_F_NOTIFY_ON_EMPTY,
> > > > > >     VIRTIO_RING_F_INDIRECT_DESC,
> > > > > >     VIRTIO_RING_F_EVENT_IDX,
> > > > > >     VIRTIO_NET_F_MRG_RXBUF,
> > > > > >     VIRTIO_F_VERSION_1,
> > > > > >     VIRTIO_NET_F_MTU,
> > > > > >     VIRTIO_F_IOMMU_PLATFORM,
> > > > > >     VIRTIO_F_RING_PACKED,
> > > > > >     VIRTIO_NET_F_HASH_REPORT,
> > > > > >     VHOST_INVALID_FEATURE_BIT
> > > > > > };
> > > > > >
> > > > > > You can see there's no STATUS bit there since it is emulated by Qemu.
> > > > > >
> > > > >
> > > > > Ok now I get what you mean, and yes we may modify the patches in that direction.
> > > > >
> > > > > But if we go then we need to modify how qemu ack the features, because
> > > > > the features that are not in vdpa_feature_bits are not acked to the
> > > > > device. More on this later.
> > > > >
> > > > > > >
> > > > > > > From what I see these are the only features that will be forwarded to
> > > > > > > the guest as device_features. If it is not in the list, the feature
> > > > > > > will be masked out,
> > > > > >
> > > > > > Only when there's no support for this feature from the vhost.
> > > > > >
> > > > > > > as if the device does not support it.
> > > > > > >
> > > > > > > So now _F_STATUS it was forwarded only if the device supports it. If
> > > > > > > we remove it from bit_mask, it will never be offered to the guest. But
> > > > > > > we want to offer it always, since we will need it for
> > > > > > > _F_GUEST_ANNOUNCE.
> > > > > > >
> > > > > > > Things get more complex because we actually need to ack it back if the
> > > > > > > device offers it, so the vdpa device can report link_down. We will
> > > > > > > only emulate LINK_UP always in the case the device does not support
> > > > > > > _F_STATUS.
> > > > > > >
> > > > > > > > So if we remove _F_STATUS, Qemu vhost code won't validate if
> > > > > > > > vhost-vdpa device has this support:
> > > > > > > >
> > > > > > > > uint64_t vhost_get_features(struct vhost_dev *hdev, const int *feature_bits,
> > > > > > > >                             uint64_t features)
> > > > > > > > {
> > > > > > > >     const int *bit = feature_bits;
> > > > > > > >     while (*bit != VHOST_INVALID_FEATURE_BIT) {
> > > > > > > >         uint64_t bit_mask = (1ULL << *bit);
> > > > > > > >         if (!(hdev->features & bit_mask)) {
> > > > > > > >             features &= ~bit_mask;
> > > > > > > >         }
> > > > > > > >         bit++;
> > > > > > > >     }
> > > > > > > >     return features;
> > > > > > > > }
> > > > > > > >
> > > > > > >
> > > > > > > Now maybe I'm the one missing something, but why is this not done as a
> > > > > > > masking directly?
> > > > > >
> > > > > > Not sure, the code has been there since day 0.
> > > > > >
> > > > > > But you can see from the code:
> > > > > >
> > > > > > 1) if STATUS is in feature_bits, we need validate the hdev->features
> > > > > > and mask it if the vhost doesn't have the support
> > > > > > 2) if STATUS is not, we don't do the check and driver may still see STATUS
> > > > > >
> > > > >
> > > > > That's useful for _F_GUEST_ANNOUNCE, but we need to ack _F_STATUS for
> > > > > the device if it supports it.
> > > >
> > > > Rethink about this, I don't see why ANNOUNCE depends on STATUS (spec
> > > > doesn't say so).
> > > >
> > >
> > > It is needed for the guest to read the status bit:
> > > """
> > > status only exists if VIRTIO_NET_F_STATUS is set. Two read-only bits
> > > (for the driver) are currently defined for the status field:
> > > VIRTIO_NET_S_LINK_UP and VIRTIO_NET_S_ANNOUNCE.
> > > """
> > >
> > > A change on the standard could be possible, like "status only exists
> > > if VIRTIO_NET_F_STATUS or VIRTIO_NET_F_GUEST_ANNOUNCE is set".
> > > However, Linux drivers already expect _F_STATUS to read _S_ANNOUNCE
> > > and to emulate _F_STATUS in case the device doesn't support it should
> > > not be a big deal in my opinion.
> >
> > RIght, so I think we need a spec patch to clarify the dependency,
> > currently, spec said ANNOUNCE depends on CTRL_VQ.
> >
>
> Would it be enough to expand it under the "Feature bit requirements" section?
>

Yes.

> > >
> > > > > QEMU cannot detect by itself when the
> > > > > link is not up. I think that setting unconditionally
> > > > > VIRTIO_NET_S_LINK_UP is actually a regression, since the guest cannot
> > > > > detect the link down that way.
> > > >
> > > > I think the idea is to still read status from config if the device
> > > > supports this.
> > > >
> > >
> > > Yes, that's my point. If I delete it from vdpa_feature_bits, it will
> > > not be acked to the device, so we cannot read status from the device.
> > >
> > > > >
> > > > > To enable _F_STATUS unconditionally is only done in the case the
> > > > > device does not support it, because its emulation is very easy. That
> > > > > way we support _F_GUEST_ANNOUNCE in all cases without device's
> > > > > cooperation.
> > > > >
> > > > > Having said that, should we go the opposite route and ack _F_STATE as
> > > > > long as the device supports it? As an advantage, all backends should
> > > > > support that at this moment, isn't it?
> > > >
> > > > So I think the method used in this patch is fine, but I wonder if it's
> > > > better to move it to the vhost layer instead of doing it in vhost_net
> > > > since we do the features validation there. We probably need another
> > > > table as input for get/set features there?
> > > >
> > >
> > > We can discuss how to do it for sure. But as you pointed out,
> > > vhost_net and virtio_net already modify the features received from the
> > > devices, so it makes sense to me to modify the features set by the
> > > guest.
> > >
> > > The problem is that we need to transmit to vhost when ack _F_STATUS
> > > and when not. The first proposal was to add a new member of vhost_vdpa
> > > but this is not optimal.
> > >
> > > If we add a new table it should be a static const one, and vhost_vdpa
> > > should have a pointer to it, isn't it?
> >
> > Yes.
> >
> > > Something like features that
> > > are emulated by qemu so they must be offered always to the guest?
> >
> > Kind of, actually it should be the features:
> >
> > 1) could be always seen by guest
> > 2) when vhost device have this feature, use that
> > 3) when vhost device doesn't have this feature, emulate one
> >
>
> I'm fine with that approach, but restricting the changes to either
> vhost_net or virtio_net makes more sense to me. The net config space
> interception goes to virtio_net anyway, not to vhost-vdpa.
>
> I'll try to prepare the patches with a new array.

I'm ok if Michael is ok with this. I think it might help if we add a
comment in general vhost code like vhost_get_features(), then readers
know that each individual vhost implementation can mediate the feature
by their own.

>
> > But a question still, is there a vDPA parent that can't do _F_STATUS
> > now (if not, we probably don't need to bother now).
> >
>
> Only mlx have _F_STATUS at this moment.
>
> If I understand you correctly, you are proposing not to emulate
> _F_STATUS if the device does not support it? To emulate _F_STATUS is
> not a big deal emulating _F_GUEST_ANNOUNCE on top anyway.

I meant if most of the vDPA parent supports _F_STATUS, there's
probably no need to emulate it:

1) simulator, not hard to add status
2) vp_vdpa, should support _F_STATUS
3) IFCVF, I guess it should have this support, Ling Shan, can you clarify this?
4) ENI, not sure but anyhow it's a legacy parent

Thanks

>
> Thanks!
>
> > Thanks
> >
> > >
> > > Thanks!
> > >
> > > > Thanks
> > > >
> > > > >
> > > > > Thanks!
> > > > >
> > > > >
> > > > >
> > > > >
> > > > > > Thanks
> > > > > >
> > > > > > >
> > > > > > > Instead of making feature_bits an array of ints, to declare it as a
> > > > > > > uint64_t with the valid feature bits and simply return features &
> > > > > > > feature_bits.
> > > > > > >
> > > > > > > Thanks!
> > > > > > >
> > > > > > > > Thanks
> > > > > > > >
> > > > > > > >
> > > > > > > >
> > > > > > > > >
> > > > > > > > > The goal with this patch series is to let the guest access the status
> > > > > > > > > always, even if the device doesn't support _F_STATUS.
> > > > > > > > >
> > > > > > > > > > Thanks
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > > ---
> > > > > > > > > > >   include/net/vhost-vdpa.h |  1 +
> > > > > > > > > > >   hw/net/vhost_net.c       | 16 ++++++++++++++--
> > > > > > > > > > >   net/vhost-vdpa.c         |  3 +++
> > > > > > > > > > >   3 files changed, 18 insertions(+), 2 deletions(-)
> > > > > > > > > > >
> > > > > > > > > > > diff --git a/include/net/vhost-vdpa.h b/include/net/vhost-vdpa.h
> > > > > > > > > > > index b81f9a6f2a..cfbcce6427 100644
> > > > > > > > > > > --- a/include/net/vhost-vdpa.h
> > > > > > > > > > > +++ b/include/net/vhost-vdpa.h
> > > > > > > > > > > @@ -17,5 +17,6 @@
> > > > > > > > > > >   struct vhost_net *vhost_vdpa_get_vhost_net(NetClientState *nc);
> > > > > > > > > > >
> > > > > > > > > > >   extern const int vdpa_feature_bits[];
> > > > > > > > > > > +extern const uint64_t vhost_vdpa_net_added_feature_bits;
> > > > > > > > > > >
> > > > > > > > > > >   #endif /* VHOST_VDPA_H */
> > > > > > > > > > > diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
> > > > > > > > > > > index d28f8b974b..7c15cc6e8f 100644
> > > > > > > > > > > --- a/hw/net/vhost_net.c
> > > > > > > > > > > +++ b/hw/net/vhost_net.c
> > > > > > > > > > > @@ -109,10 +109,22 @@ static const int *vhost_net_get_feature_bits(struct vhost_net *net)
> > > > > > > > > > >       return feature_bits;
> > > > > > > > > > >   }
> > > > > > > > > > >
> > > > > > > > > > > +static uint64_t vhost_net_add_feature_bits(struct vhost_net *net)
> > > > > > > > > > > +{
> > > > > > > > > > > +    if (net->nc->info->type == NET_CLIENT_DRIVER_VHOST_VDPA) {
> > > > > > > > > > > +        return vhost_vdpa_net_added_feature_bits;
> > > > > > > > > > > +    }
> > > > > > > > > > > +
> > > > > > > > > > > +    return 0;
> > > > > > > > > > > +}
> > > > > > > > > > > +
> > > > > > > > > > >   uint64_t vhost_net_get_features(struct vhost_net *net, uint64_t features)
> > > > > > > > > > >   {
> > > > > > > > > > > -    return vhost_get_features(&net->dev, vhost_net_get_feature_bits(net),
> > > > > > > > > > > -            features);
> > > > > > > > > > > +    uint64_t ret = vhost_get_features(&net->dev,
> > > > > > > > > > > +                                      vhost_net_get_feature_bits(net),
> > > > > > > > > > > +                                      features);
> > > > > > > > > > > +
> > > > > > > > > > > +    return ret | vhost_net_add_feature_bits(net);
> > > > > > > > > > >   }
> > > > > > > > > > >   int vhost_net_get_config(struct vhost_net *net,  uint8_t *config,
> > > > > > > > > > >                            uint32_t config_len)
> > > > > > > > > > > diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
> > > > > > > > > > > index 6d64000202..24d2857593 100644
> > > > > > > > > > > --- a/net/vhost-vdpa.c
> > > > > > > > > > > +++ b/net/vhost-vdpa.c
> > > > > > > > > > > @@ -99,6 +99,9 @@ static const uint64_t vdpa_svq_device_features =
> > > > > > > > > > >       BIT_ULL(VIRTIO_NET_F_RSC_EXT) |
> > > > > > > > > > >       BIT_ULL(VIRTIO_NET_F_STANDBY);
> > > > > > > > > > >
> > > > > > > > > > > +const uint64_t vhost_vdpa_net_added_feature_bits =
> > > > > > > > > > > +    BIT_ULL(VIRTIO_NET_F_STATUS);
> > > > > > > > > > > +
> > > > > > > > > > >   VHostNetState *vhost_vdpa_get_vhost_net(NetClientState *nc)
> > > > > > > > > > >   {
> > > > > > > > > > >       VhostVDPAState *s = DO_UPCAST(VhostVDPAState, nc, nc);
> > > > > > > > > >
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>

Re: [PATCH 3/3] vdpa: Expose VIRTIO_NET_F_STATUS unconditionally

[Eugenio Perez Martin]

On Mon, Nov 7, 2022 at 8:51 AM Jason Wang <jasowang@redhat.com> wrote:
>
> On Thu, Nov 3, 2022 at 4:12 PM Eugenio Perez Martin <eperezma@redhat.com> wrote:
> >
> > On Thu, Nov 3, 2022 at 4:21 AM Jason Wang <jasowang@redhat.com> wrote:
> > >
> > > On Wed, Nov 2, 2022 at 7:19 PM Eugenio Perez Martin <eperezma@redhat.com> wrote:
> > > >
> > > > On Tue, Nov 1, 2022 at 9:10 AM Jason Wang <jasowang@redhat.com> wrote:
> > > > >
> > > > > On Fri, Oct 28, 2022 at 5:30 PM Eugenio Perez Martin
> > > > > <eperezma@redhat.com> wrote:
> > > > > >
> > > > > > On Fri, Oct 28, 2022 at 3:59 AM Jason Wang <jasowang@redhat.com> wrote:
> > > > > > >
> > > > > > > On Thu, Oct 27, 2022 at 6:18 PM Eugenio Perez Martin
> > > > > > > <eperezma@redhat.com> wrote:
> > > > > > > >
> > > > > > > > On Thu, Oct 27, 2022 at 8:54 AM Jason Wang <jasowang@redhat.com> wrote:
> > > > > > > > >
> > > > > > > > > On Thu, Oct 27, 2022 at 2:47 PM Eugenio Perez Martin
> > > > > > > > > <eperezma@redhat.com> wrote:
> > > > > > > > > >
> > > > > > > > > > On Thu, Oct 27, 2022 at 6:32 AM Jason Wang <jasowang@redhat.com> wrote:
> > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > 在 2022/10/26 17:53, Eugenio Pérez 写道:
> > > > > > > > > > > > Now that qemu can handle and emulate it if the vdpa backend does not
> > > > > > > > > > > > support it we can offer it always.
> > > > > > > > > > > >
> > > > > > > > > > > > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > I may miss something but isn't more easier to simply remove the
> > > > > > > > > > > _F_STATUS from vdpa_feature_bits[]?
> > > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > How is that? if we remove it, the guest cannot ack it so it cannot
> > > > > > > > > > access the net status, isn't it?
> > > > > > > > >
> > > > > > > > > My understanding is that the bits stored in the vdpa_feature_bits[]
> > > > > > > > > are the features that must be explicitly supported by the vhost
> > > > > > > > > device.
> > > > > > > >
> > > > > > > > (Non English native here, so maybe I don't get what you mean :) ) The
> > > > > > > > device may not support them. net simulator lacks some of them
> > > > > > > > actually, and it works.
> > > > > > >
> > > > > > > Speaking too fast, I think I meant that, if the bit doesn't belong to
> > > > > > > vdpa_feature_bits[], it is assumed to be supported by the Qemu without
> > > > > > > the support of the vhost. So Qemu won't even try to validate if vhost
> > > > > > > has this support. E.g for vhost-net, we only have:
> > > > > > >
> > > > > > > static const int kernel_feature_bits[] = {
> > > > > > >     VIRTIO_F_NOTIFY_ON_EMPTY,
> > > > > > >     VIRTIO_RING_F_INDIRECT_DESC,
> > > > > > >     VIRTIO_RING_F_EVENT_IDX,
> > > > > > >     VIRTIO_NET_F_MRG_RXBUF,
> > > > > > >     VIRTIO_F_VERSION_1,
> > > > > > >     VIRTIO_NET_F_MTU,
> > > > > > >     VIRTIO_F_IOMMU_PLATFORM,
> > > > > > >     VIRTIO_F_RING_PACKED,
> > > > > > >     VIRTIO_NET_F_HASH_REPORT,
> > > > > > >     VHOST_INVALID_FEATURE_BIT
> > > > > > > };
> > > > > > >
> > > > > > > You can see there's no STATUS bit there since it is emulated by Qemu.
> > > > > > >
> > > > > >
> > > > > > Ok now I get what you mean, and yes we may modify the patches in that direction.
> > > > > >
> > > > > > But if we go then we need to modify how qemu ack the features, because
> > > > > > the features that are not in vdpa_feature_bits are not acked to the
> > > > > > device. More on this later.
> > > > > >
> > > > > > > >
> > > > > > > > From what I see these are the only features that will be forwarded to
> > > > > > > > the guest as device_features. If it is not in the list, the feature
> > > > > > > > will be masked out,
> > > > > > >
> > > > > > > Only when there's no support for this feature from the vhost.
> > > > > > >
> > > > > > > > as if the device does not support it.
> > > > > > > >
> > > > > > > > So now _F_STATUS it was forwarded only if the device supports it. If
> > > > > > > > we remove it from bit_mask, it will never be offered to the guest. But
> > > > > > > > we want to offer it always, since we will need it for
> > > > > > > > _F_GUEST_ANNOUNCE.
> > > > > > > >
> > > > > > > > Things get more complex because we actually need to ack it back if the
> > > > > > > > device offers it, so the vdpa device can report link_down. We will
> > > > > > > > only emulate LINK_UP always in the case the device does not support
> > > > > > > > _F_STATUS.
> > > > > > > >
> > > > > > > > > So if we remove _F_STATUS, Qemu vhost code won't validate if
> > > > > > > > > vhost-vdpa device has this support:
> > > > > > > > >
> > > > > > > > > uint64_t vhost_get_features(struct vhost_dev *hdev, const int *feature_bits,
> > > > > > > > >                             uint64_t features)
> > > > > > > > > {
> > > > > > > > >     const int *bit = feature_bits;
> > > > > > > > >     while (*bit != VHOST_INVALID_FEATURE_BIT) {
> > > > > > > > >         uint64_t bit_mask = (1ULL << *bit);
> > > > > > > > >         if (!(hdev->features & bit_mask)) {
> > > > > > > > >             features &= ~bit_mask;
> > > > > > > > >         }
> > > > > > > > >         bit++;
> > > > > > > > >     }
> > > > > > > > >     return features;
> > > > > > > > > }
> > > > > > > > >
> > > > > > > >
> > > > > > > > Now maybe I'm the one missing something, but why is this not done as a
> > > > > > > > masking directly?
> > > > > > >
> > > > > > > Not sure, the code has been there since day 0.
> > > > > > >
> > > > > > > But you can see from the code:
> > > > > > >
> > > > > > > 1) if STATUS is in feature_bits, we need validate the hdev->features
> > > > > > > and mask it if the vhost doesn't have the support
> > > > > > > 2) if STATUS is not, we don't do the check and driver may still see STATUS
> > > > > > >
> > > > > >
> > > > > > That's useful for _F_GUEST_ANNOUNCE, but we need to ack _F_STATUS for
> > > > > > the device if it supports it.
> > > > >
> > > > > Rethink about this, I don't see why ANNOUNCE depends on STATUS (spec
> > > > > doesn't say so).
> > > > >
> > > >
> > > > It is needed for the guest to read the status bit:
> > > > """
> > > > status only exists if VIRTIO_NET_F_STATUS is set. Two read-only bits
> > > > (for the driver) are currently defined for the status field:
> > > > VIRTIO_NET_S_LINK_UP and VIRTIO_NET_S_ANNOUNCE.
> > > > """
> > > >
> > > > A change on the standard could be possible, like "status only exists
> > > > if VIRTIO_NET_F_STATUS or VIRTIO_NET_F_GUEST_ANNOUNCE is set".
> > > > However, Linux drivers already expect _F_STATUS to read _S_ANNOUNCE
> > > > and to emulate _F_STATUS in case the device doesn't support it should
> > > > not be a big deal in my opinion.
> > >
> > > RIght, so I think we need a spec patch to clarify the dependency,
> > > currently, spec said ANNOUNCE depends on CTRL_VQ.
> > >
> >
> > Would it be enough to expand it under the "Feature bit requirements" section?
> >
>
> Yes.
>
> > > >
> > > > > > QEMU cannot detect by itself when the
> > > > > > link is not up. I think that setting unconditionally
> > > > > > VIRTIO_NET_S_LINK_UP is actually a regression, since the guest cannot
> > > > > > detect the link down that way.
> > > > >
> > > > > I think the idea is to still read status from config if the device
> > > > > supports this.
> > > > >
> > > >
> > > > Yes, that's my point. If I delete it from vdpa_feature_bits, it will
> > > > not be acked to the device, so we cannot read status from the device.
> > > >
> > > > > >
> > > > > > To enable _F_STATUS unconditionally is only done in the case the
> > > > > > device does not support it, because its emulation is very easy. That
> > > > > > way we support _F_GUEST_ANNOUNCE in all cases without device's
> > > > > > cooperation.
> > > > > >
> > > > > > Having said that, should we go the opposite route and ack _F_STATE as
> > > > > > long as the device supports it? As an advantage, all backends should
> > > > > > support that at this moment, isn't it?
> > > > >
> > > > > So I think the method used in this patch is fine, but I wonder if it's
> > > > > better to move it to the vhost layer instead of doing it in vhost_net
> > > > > since we do the features validation there. We probably need another
> > > > > table as input for get/set features there?
> > > > >
> > > >
> > > > We can discuss how to do it for sure. But as you pointed out,
> > > > vhost_net and virtio_net already modify the features received from the
> > > > devices, so it makes sense to me to modify the features set by the
> > > > guest.
> > > >
> > > > The problem is that we need to transmit to vhost when ack _F_STATUS
> > > > and when not. The first proposal was to add a new member of vhost_vdpa
> > > > but this is not optimal.
> > > >
> > > > If we add a new table it should be a static const one, and vhost_vdpa
> > > > should have a pointer to it, isn't it?
> > >
> > > Yes.
> > >
> > > > Something like features that
> > > > are emulated by qemu so they must be offered always to the guest?
> > >
> > > Kind of, actually it should be the features:
> > >
> > > 1) could be always seen by guest
> > > 2) when vhost device have this feature, use that
> > > 3) when vhost device doesn't have this feature, emulate one
> > >
> >
> > I'm fine with that approach, but restricting the changes to either
> > vhost_net or virtio_net makes more sense to me. The net config space
> > interception goes to virtio_net anyway, not to vhost-vdpa.
> >
> > I'll try to prepare the patches with a new array.
>
> I'm ok if Michael is ok with this. I think it might help if we add a
> comment in general vhost code like vhost_get_features(), then readers
> know that each individual vhost implementation can mediate the feature
> by their own.
>
> >
> > > But a question still, is there a vDPA parent that can't do _F_STATUS
> > > now (if not, we probably don't need to bother now).
> > >
> >
> > Only mlx have _F_STATUS at this moment.
> >
> > If I understand you correctly, you are proposing not to emulate
> > _F_STATUS if the device does not support it? To emulate _F_STATUS is
> > not a big deal emulating _F_GUEST_ANNOUNCE on top anyway.
>
> I meant if most of the vDPA parent supports _F_STATUS, there's
> probably no need to emulate it:
>
> 1) simulator, not hard to add status
> 2) vp_vdpa, should support _F_STATUS
> 3) IFCVF, I guess it should have this support, Ling Shan, can you clarify this?
> 4) ENI, not sure but anyhow it's a legacy parent
>

If this is the case then yes, it might make more sense to add
_F_STATUS support to at least vdpa simulator.

We can always emulate it in qemu later for sure.

Thanks!

> Thanks
>
> >
> > Thanks!
> >
> > > Thanks
> > >
> > > >
> > > > Thanks!
> > > >
> > > > > Thanks
> > > > >
> > > > > >
> > > > > > Thanks!
> > > > > >
> > > > > >
> > > > > >
> > > > > >
> > > > > > > Thanks
> > > > > > >
> > > > > > > >
> > > > > > > > Instead of making feature_bits an array of ints, to declare it as a
> > > > > > > > uint64_t with the valid feature bits and simply return features &
> > > > > > > > feature_bits.
> > > > > > > >
> > > > > > > > Thanks!
> > > > > > > >
> > > > > > > > > Thanks
> > > > > > > > >
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > The goal with this patch series is to let the guest access the status
> > > > > > > > > > always, even if the device doesn't support _F_STATUS.
> > > > > > > > > >
> > > > > > > > > > > Thanks
> > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > > ---
> > > > > > > > > > > >   include/net/vhost-vdpa.h |  1 +
> > > > > > > > > > > >   hw/net/vhost_net.c       | 16 ++++++++++++++--
> > > > > > > > > > > >   net/vhost-vdpa.c         |  3 +++
> > > > > > > > > > > >   3 files changed, 18 insertions(+), 2 deletions(-)
> > > > > > > > > > > >
> > > > > > > > > > > > diff --git a/include/net/vhost-vdpa.h b/include/net/vhost-vdpa.h
> > > > > > > > > > > > index b81f9a6f2a..cfbcce6427 100644
> > > > > > > > > > > > --- a/include/net/vhost-vdpa.h
> > > > > > > > > > > > +++ b/include/net/vhost-vdpa.h
> > > > > > > > > > > > @@ -17,5 +17,6 @@
> > > > > > > > > > > >   struct vhost_net *vhost_vdpa_get_vhost_net(NetClientState *nc);
> > > > > > > > > > > >
> > > > > > > > > > > >   extern const int vdpa_feature_bits[];
> > > > > > > > > > > > +extern const uint64_t vhost_vdpa_net_added_feature_bits;
> > > > > > > > > > > >
> > > > > > > > > > > >   #endif /* VHOST_VDPA_H */
> > > > > > > > > > > > diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
> > > > > > > > > > > > index d28f8b974b..7c15cc6e8f 100644
> > > > > > > > > > > > --- a/hw/net/vhost_net.c
> > > > > > > > > > > > +++ b/hw/net/vhost_net.c
> > > > > > > > > > > > @@ -109,10 +109,22 @@ static const int *vhost_net_get_feature_bits(struct vhost_net *net)
> > > > > > > > > > > >       return feature_bits;
> > > > > > > > > > > >   }
> > > > > > > > > > > >
> > > > > > > > > > > > +static uint64_t vhost_net_add_feature_bits(struct vhost_net *net)
> > > > > > > > > > > > +{
> > > > > > > > > > > > +    if (net->nc->info->type == NET_CLIENT_DRIVER_VHOST_VDPA) {
> > > > > > > > > > > > +        return vhost_vdpa_net_added_feature_bits;
> > > > > > > > > > > > +    }
> > > > > > > > > > > > +
> > > > > > > > > > > > +    return 0;
> > > > > > > > > > > > +}
> > > > > > > > > > > > +
> > > > > > > > > > > >   uint64_t vhost_net_get_features(struct vhost_net *net, uint64_t features)
> > > > > > > > > > > >   {
> > > > > > > > > > > > -    return vhost_get_features(&net->dev, vhost_net_get_feature_bits(net),
> > > > > > > > > > > > -            features);
> > > > > > > > > > > > +    uint64_t ret = vhost_get_features(&net->dev,
> > > > > > > > > > > > +                                      vhost_net_get_feature_bits(net),
> > > > > > > > > > > > +                                      features);
> > > > > > > > > > > > +
> > > > > > > > > > > > +    return ret | vhost_net_add_feature_bits(net);
> > > > > > > > > > > >   }
> > > > > > > > > > > >   int vhost_net_get_config(struct vhost_net *net,  uint8_t *config,
> > > > > > > > > > > >                            uint32_t config_len)
> > > > > > > > > > > > diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
> > > > > > > > > > > > index 6d64000202..24d2857593 100644
> > > > > > > > > > > > --- a/net/vhost-vdpa.c
> > > > > > > > > > > > +++ b/net/vhost-vdpa.c
> > > > > > > > > > > > @@ -99,6 +99,9 @@ static const uint64_t vdpa_svq_device_features =
> > > > > > > > > > > >       BIT_ULL(VIRTIO_NET_F_RSC_EXT) |
> > > > > > > > > > > >       BIT_ULL(VIRTIO_NET_F_STANDBY);
> > > > > > > > > > > >
> > > > > > > > > > > > +const uint64_t vhost_vdpa_net_added_feature_bits =
> > > > > > > > > > > > +    BIT_ULL(VIRTIO_NET_F_STATUS);
> > > > > > > > > > > > +
> > > > > > > > > > > >   VHostNetState *vhost_vdpa_get_vhost_net(NetClientState *nc)
> > > > > > > > > > > >   {
> > > > > > > > > > > >       VhostVDPAState *s = DO_UPCAST(VhostVDPAState, nc, nc);
> > > > > > > > > > >
> > > > > > > > > >
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>

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

[Michael S. Tsirkin]

On Wed, Oct 26, 2022 at 11:53:00AM +0200, Eugenio Pérez wrote:
> 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.

Jason seems to be taking all this work through net tree.

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



> 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
>   vdpa: Expose VIRTIO_NET_F_STATUS unconditionally
> 
>  include/net/vhost-vdpa.h |  1 +
>  hw/net/vhost_net.c       | 16 ++++++++++++++--
>  hw/net/virtio-net.c      | 39 ++++++++++++++++++++++++---------------
>  net/vhost-vdpa.c         |  3 +++
>  4 files changed, 42 insertions(+), 17 deletions(-)
> 
> -- 
> 2.31.1
>