[RFC PATCH v2 0/8] Guest announce feature emulation using Shadow VirtQueue

[RFC PATCH v2 0/8] Guest announce feature emulation using Shadow VirtQueue

[Eugenio Pérez]

A gratuitous ARP is recommended after a live migration to reduce the amount of
time needed by the network links to be aware of the new location. A hypervisor
may not have the knowledge of the guest network configuration, and this is
especially true on passthrough devices, so its simpler to ask the guest to
do it.

However, the device control part of this feature can be totally emulated by
qemu and shadow virtqueue, not needing any special feature from the actual
vdpa device.

VIRTIO_NET_F_STATUS is also needed for the guest to access the status of
virtio net config where announcement status bit is set. Emulating it as
always active in case backend does not support it.

v2:
* Add VIRTIO_NET_F_STATUS emulation.

Eugenio Pérez (8):
  vdpa: Delete duplicated vdpa_feature_bits entry
  vdpa: Save emulated features list in vhost_vdpa
  vhost_net: Emulate link state up if backend doesn't expose it
  vdpa: Expose VIRTIO_NET_F_STATUS unconditionally
  vdpa: Remove shadow CVQ command check
  vdpa: handle VIRTIO_NET_CTRL_ANNOUNCE in
    vhost_vdpa_net_handle_ctrl_avail
  vhost_net: return VIRTIO_NET_S_ANNOUNCE is device model has it set
  vdpa: Offer VIRTIO_NET_F_GUEST_ANNOUNCE feature if SVQ is enabled

 include/hw/virtio/vhost-vdpa.h |  2 +
 hw/net/vhost_net.c             | 35 +++++++++++++++-
 hw/virtio/vhost-vdpa.c         |  8 ++--
 net/vhost-vdpa.c               | 74 ++++++++++------------------------
 4 files changed, 62 insertions(+), 57 deletions(-)

-- 
2.31.1

[RFC PATCH v2 1/8] vdpa: Delete duplicated vdpa_feature_bits entry

[Eugenio Pérez]

This entry was duplicated on referenced commit. Removing it.

Fixes: 402378407dbd ("vhost-vdpa: multiqueue support")
Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
---
 net/vhost-vdpa.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
index 4bc3fd01a8..eebf29f5c1 100644
--- a/net/vhost-vdpa.c
+++ b/net/vhost-vdpa.c
@@ -63,7 +63,6 @@ const int vdpa_feature_bits[] = {
     VIRTIO_NET_F_CTRL_RX,
     VIRTIO_NET_F_CTRL_RX_EXTRA,
     VIRTIO_NET_F_CTRL_VLAN,
-    VIRTIO_NET_F_GUEST_ANNOUNCE,
     VIRTIO_NET_F_CTRL_MAC_ADDR,
     VIRTIO_NET_F_RSS,
     VIRTIO_NET_F_MQ,
-- 
2.31.1

[RFC PATCH v2 2/8] vdpa: Save emulated features list in vhost_vdpa

[Eugenio Pérez]

At this moment only _F_LOG is added there.

However future patches add features that depend on the kind of device.
In particular, only net devices can add VIRTIO_F_GUEST_ANNOUNCE. So
let's allow vhost_vdpa creator to set custom emulated device features.

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

diff --git a/include/hw/virtio/vhost-vdpa.h b/include/hw/virtio/vhost-vdpa.h
index 1111d85643..50083e1e3b 100644
--- a/include/hw/virtio/vhost-vdpa.h
+++ b/include/hw/virtio/vhost-vdpa.h
@@ -31,6 +31,8 @@ typedef struct vhost_vdpa {
     bool iotlb_batch_begin_sent;
     MemoryListener listener;
     struct vhost_vdpa_iova_range iova_range;
+    /* VirtIO device features that can be emulated by qemu */
+    uint64_t added_features;
     uint64_t acked_features;
     bool shadow_vqs_enabled;
     /* IOVA mapping used by the Shadow Virtqueue */
diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
index 7468e44b87..ddb5e29288 100644
--- a/hw/virtio/vhost-vdpa.c
+++ b/hw/virtio/vhost-vdpa.c
@@ -660,8 +660,8 @@ static int vhost_vdpa_set_features(struct vhost_dev *dev,
 
         v->acked_features = features;
 
-        /* We must not ack _F_LOG if SVQ is enabled */
-        features &= ~BIT_ULL(VHOST_F_LOG_ALL);
+        /* Do not ack features emulated by qemu */
+        features &= ~v->added_features;
     }
 
     trace_vhost_vdpa_set_features(dev, features);
@@ -1244,8 +1244,8 @@ static int vhost_vdpa_get_features(struct vhost_dev *dev,
     int ret = vhost_vdpa_get_dev_features(dev, features);
 
     if (ret == 0 && v->shadow_vqs_enabled) {
-        /* Add SVQ logging capabilities */
-        *features |= BIT_ULL(VHOST_F_LOG_ALL);
+        /* Add emulated capabilities */
+        *features |= v->added_features;
     }
 
     return ret;
diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
index eebf29f5c1..3803452800 100644
--- a/net/vhost-vdpa.c
+++ b/net/vhost-vdpa.c
@@ -599,6 +599,10 @@ static NetClientState *net_vhost_vdpa_init(NetClientState *peer,
     s->vhost_vdpa.index = queue_pair_index;
     s->vhost_vdpa.shadow_vqs_enabled = svq;
     s->vhost_vdpa.iova_tree = iova_tree;
+    if (svq) {
+        /* Add SVQ logging capabilities */
+        s->vhost_vdpa.added_features |= BIT_ULL(VHOST_F_LOG_ALL);
+    }
     if (!is_datapath) {
         s->cvq_cmd_out_buffer = qemu_memalign(qemu_real_host_page_size(),
                                             vhost_vdpa_net_cvq_cmd_page_len());
-- 
2.31.1

[RFC PATCH v2 3/8] vhost_net: Emulate link state up if backend doesn't expose it

[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/vhost_net.c | 27 ++++++++++++++++++++++++++-
 1 file changed, 26 insertions(+), 1 deletion(-)

diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
index d28f8b974b..5660606c1d 100644
--- a/hw/net/vhost_net.c
+++ b/hw/net/vhost_net.c
@@ -117,7 +117,32 @@ uint64_t vhost_net_get_features(struct vhost_net *net, uint64_t features)
 int vhost_net_get_config(struct vhost_net *net,  uint8_t *config,
                          uint32_t config_len)
 {
-    return vhost_dev_get_config(&net->dev, config, config_len, NULL);
+    VirtIODevice *vdev;
+    int r = vhost_dev_get_config(&net->dev, config, config_len, NULL);
+
+    if (unlikely(r != 0)) {
+        return r;
+    }
+
+    if (config_len < endof(struct virtio_net_config, status)) {
+        return 0;
+    }
+
+    /*
+     * TODO: Perform this only if vhost_vdpa.
+     */
+    vdev = net->dev.vdev;
+    if (!vdev) {
+        /* Device is starting */
+        return 0;
+    }
+
+    if ((net->dev.acked_features & BIT_ULL(VIRTIO_NET_F_STATUS)) &&
+        !(net->dev.features & BIT_ULL(VIRTIO_NET_F_STATUS))) {
+        ((struct virtio_net_config *)config)->status |= VIRTIO_NET_S_LINK_UP;
+    }
+
+    return 0;
 }
 int vhost_net_set_config(struct vhost_net *net, const uint8_t *data,
                          uint32_t offset, uint32_t size, uint32_t flags)
-- 
2.31.1

[RFC PATCH v2 4/8] 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>
---
 net/vhost-vdpa.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
index 3803452800..fca21d5b79 100644
--- a/net/vhost-vdpa.c
+++ b/net/vhost-vdpa.c
@@ -602,6 +602,9 @@ static NetClientState *net_vhost_vdpa_init(NetClientState *peer,
     if (svq) {
         /* Add SVQ logging capabilities */
         s->vhost_vdpa.added_features |= BIT_ULL(VHOST_F_LOG_ALL);
+
+        /* VIRTIO_NET_F_STATUS is mandatory for _F_GUEST_ANNOUNCE. */
+        s->vhost_vdpa.added_features |= BIT_ULL(VIRTIO_NET_F_STATUS);
     }
     if (!is_datapath) {
         s->cvq_cmd_out_buffer = qemu_memalign(qemu_real_host_page_size(),
-- 
2.31.1

[RFC PATCH v2 5/8] vdpa: Remove shadow CVQ command check

[Eugenio Pérez]

The guest will see undefined behavior if it issue not negotiate
commands, bit it is expected somehow.

Simplify code deleting this check.

Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
---
 net/vhost-vdpa.c | 48 ------------------------------------------------
 1 file changed, 48 deletions(-)

diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
index fca21d5b79..3374c21b4d 100644
--- a/net/vhost-vdpa.c
+++ b/net/vhost-vdpa.c
@@ -461,48 +461,6 @@ static NetClientInfo net_vhost_vdpa_cvq_info = {
     .check_peer_type = vhost_vdpa_check_peer_type,
 };
 
-/**
- * Do not forward commands not supported by SVQ. Otherwise, the device could
- * accept it and qemu would not know how to update the device model.
- */
-static bool vhost_vdpa_net_cvq_validate_cmd(const void *out_buf, size_t len)
-{
-    struct virtio_net_ctrl_hdr ctrl;
-
-    if (unlikely(len < sizeof(ctrl))) {
-        qemu_log_mask(LOG_GUEST_ERROR,
-                      "%s: invalid legnth of out buffer %zu\n", __func__, len);
-        return false;
-    }
-
-    memcpy(&ctrl, out_buf, sizeof(ctrl));
-    switch (ctrl.class) {
-    case VIRTIO_NET_CTRL_MAC:
-        switch (ctrl.cmd) {
-        case VIRTIO_NET_CTRL_MAC_ADDR_SET:
-            return true;
-        default:
-            qemu_log_mask(LOG_GUEST_ERROR, "%s: invalid mac cmd %u\n",
-                          __func__, ctrl.cmd);
-        };
-        break;
-    case VIRTIO_NET_CTRL_MQ:
-        switch (ctrl.cmd) {
-        case VIRTIO_NET_CTRL_MQ_VQ_PAIRS_SET:
-            return true;
-        default:
-            qemu_log_mask(LOG_GUEST_ERROR, "%s: invalid mq cmd %u\n",
-                          __func__, ctrl.cmd);
-        };
-        break;
-    default:
-        qemu_log_mask(LOG_GUEST_ERROR, "%s: invalid control class %u\n",
-                      __func__, ctrl.class);
-    };
-
-    return false;
-}
-
 /**
  * Validate and copy control virtqueue commands.
  *
@@ -526,16 +484,10 @@ static int vhost_vdpa_net_handle_ctrl_avail(VhostShadowVirtqueue *svq,
         .iov_len = sizeof(status),
     };
     ssize_t dev_written = -EINVAL;
-    bool ok;
 
     out.iov_len = iov_to_buf(elem->out_sg, elem->out_num, 0,
                              s->cvq_cmd_out_buffer,
                              vhost_vdpa_net_cvq_cmd_len());
-    ok = vhost_vdpa_net_cvq_validate_cmd(s->cvq_cmd_out_buffer, out.iov_len);
-    if (unlikely(!ok)) {
-        goto out;
-    }
-
     dev_written = vhost_vdpa_net_cvq_add(s, out.iov_len, sizeof(status));
     if (unlikely(dev_written < 0)) {
         goto out;
-- 
2.31.1

[RFC PATCH v2 6/8] vdpa: handle VIRTIO_NET_CTRL_ANNOUNCE in vhost_vdpa_net_handle_ctrl_avail

[Eugenio Pérez]

Since this capability is emulated by qemu shadowed CVQ cannot forward it
to the device. Process all that command within qemu.

Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
---
 net/vhost-vdpa.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
index 3374c21b4d..5fda405a66 100644
--- a/net/vhost-vdpa.c
+++ b/net/vhost-vdpa.c
@@ -488,9 +488,18 @@ static int vhost_vdpa_net_handle_ctrl_avail(VhostShadowVirtqueue *svq,
     out.iov_len = iov_to_buf(elem->out_sg, elem->out_num, 0,
                              s->cvq_cmd_out_buffer,
                              vhost_vdpa_net_cvq_cmd_len());
-    dev_written = vhost_vdpa_net_cvq_add(s, out.iov_len, sizeof(status));
-    if (unlikely(dev_written < 0)) {
-        goto out;
+    if (*(uint8_t *)s->cvq_cmd_out_buffer == VIRTIO_NET_CTRL_ANNOUNCE) {
+        /*
+         * Guest announce capability is emulated by qemu, so dont forward to
+         * the device.
+         */
+        dev_written = sizeof(status);
+        *s->status = VIRTIO_NET_OK;
+    } else {
+        dev_written = vhost_vdpa_net_cvq_add(s, out.iov_len, sizeof(status));
+        if (unlikely(dev_written < 0)) {
+            goto out;
+        }
     }
 
     if (unlikely(dev_written < sizeof(status))) {
-- 
2.31.1

[RFC PATCH v2 7/8] vhost_net: return VIRTIO_NET_S_ANNOUNCE is device model has it set

[Eugenio Pérez]

Temporal, as this affects other vhost backends and we must check status
feature first.

Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
---
 hw/net/vhost_net.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
index 5660606c1d..300f370e2a 100644
--- a/hw/net/vhost_net.c
+++ b/hw/net/vhost_net.c
@@ -118,6 +118,7 @@ int vhost_net_get_config(struct vhost_net *net,  uint8_t *config,
                          uint32_t config_len)
 {
     VirtIODevice *vdev;
+    VirtIONet *n;
     int r = vhost_dev_get_config(&net->dev, config, config_len, NULL);
 
     if (unlikely(r != 0)) {
@@ -142,6 +143,13 @@ int vhost_net_get_config(struct vhost_net *net,  uint8_t *config,
         ((struct virtio_net_config *)config)->status |= VIRTIO_NET_S_LINK_UP;
     }
 
+    if (!(net->dev.acked_features & BIT_ULL(VIRTIO_NET_F_GUEST_ANNOUNCE))) {
+        return 0;
+    }
+
+    n = VIRTIO_NET(vdev);
+    ((struct virtio_net_config *)config)->status |=
+                                           (n->status & VIRTIO_NET_S_ANNOUNCE);
     return 0;
 }
 int vhost_net_set_config(struct vhost_net *net, const uint8_t *data,
-- 
2.31.1

[RFC PATCH v2 8/8] vdpa: Offer VIRTIO_NET_F_GUEST_ANNOUNCE feature if SVQ is enabled

[Eugenio Pérez]

So qemu emulates it in case the device does not support it.

Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
---
 net/vhost-vdpa.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
index 5fda405a66..64442e8455 100644
--- a/net/vhost-vdpa.c
+++ b/net/vhost-vdpa.c
@@ -566,6 +566,9 @@ static NetClientState *net_vhost_vdpa_init(NetClientState *peer,
 
         /* VIRTIO_NET_F_STATUS is mandatory for _F_GUEST_ANNOUNCE. */
         s->vhost_vdpa.added_features |= BIT_ULL(VIRTIO_NET_F_STATUS);
+
+        /* We can emulate guest announce shadowing CVQ */
+        s->vhost_vdpa.added_features |= BIT_ULL(VIRTIO_NET_F_GUEST_ANNOUNCE);
     }
     if (!is_datapath) {
         s->cvq_cmd_out_buffer = qemu_memalign(qemu_real_host_page_size(),
-- 
2.31.1

Re: [RFC PATCH v2 1/8] vdpa: Delete duplicated vdpa_feature_bits entry

[Jason Wang]

On Wed, Oct 19, 2022 at 8:52 PM Eugenio Pérez <eperezma@redhat.com> wrote:
>
> This entry was duplicated on referenced commit. Removing it.
>
> Fixes: 402378407dbd ("vhost-vdpa: multiqueue support")
> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>

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

> ---
>  net/vhost-vdpa.c | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
> index 4bc3fd01a8..eebf29f5c1 100644
> --- a/net/vhost-vdpa.c
> +++ b/net/vhost-vdpa.c
> @@ -63,7 +63,6 @@ const int vdpa_feature_bits[] = {
>      VIRTIO_NET_F_CTRL_RX,
>      VIRTIO_NET_F_CTRL_RX_EXTRA,
>      VIRTIO_NET_F_CTRL_VLAN,
> -    VIRTIO_NET_F_GUEST_ANNOUNCE,
>      VIRTIO_NET_F_CTRL_MAC_ADDR,
>      VIRTIO_NET_F_RSS,
>      VIRTIO_NET_F_MQ,
> --
> 2.31.1
>

Re: [RFC PATCH v2 2/8] vdpa: Save emulated features list in vhost_vdpa

[Jason Wang]

On Wed, Oct 19, 2022 at 8:52 PM Eugenio Pérez <eperezma@redhat.com> wrote:
>
> At this moment only _F_LOG is added there.
>
> However future patches add features that depend on the kind of device.
> In particular, only net devices can add VIRTIO_F_GUEST_ANNOUNCE. So
> let's allow vhost_vdpa creator to set custom emulated device features.
>
> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> ---
>  include/hw/virtio/vhost-vdpa.h | 2 ++
>  hw/virtio/vhost-vdpa.c         | 8 ++++----
>  net/vhost-vdpa.c               | 4 ++++
>  3 files changed, 10 insertions(+), 4 deletions(-)
>
> diff --git a/include/hw/virtio/vhost-vdpa.h b/include/hw/virtio/vhost-vdpa.h
> index 1111d85643..50083e1e3b 100644
> --- a/include/hw/virtio/vhost-vdpa.h
> +++ b/include/hw/virtio/vhost-vdpa.h
> @@ -31,6 +31,8 @@ typedef struct vhost_vdpa {
>      bool iotlb_batch_begin_sent;
>      MemoryListener listener;
>      struct vhost_vdpa_iova_range iova_range;
> +    /* VirtIO device features that can be emulated by qemu */
> +    uint64_t added_features;

Any reason we need a per vhost_vdpa storage for this? Or is there a
chance that this field could be different among the devices?

Thanks

>      uint64_t acked_features;
>      bool shadow_vqs_enabled;
>      /* IOVA mapping used by the Shadow Virtqueue */
> diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
> index 7468e44b87..ddb5e29288 100644
> --- a/hw/virtio/vhost-vdpa.c
> +++ b/hw/virtio/vhost-vdpa.c
> @@ -660,8 +660,8 @@ static int vhost_vdpa_set_features(struct vhost_dev *dev,
>
>          v->acked_features = features;
>
> -        /* We must not ack _F_LOG if SVQ is enabled */
> -        features &= ~BIT_ULL(VHOST_F_LOG_ALL);
> +        /* Do not ack features emulated by qemu */
> +        features &= ~v->added_features;
>      }
>
>      trace_vhost_vdpa_set_features(dev, features);
> @@ -1244,8 +1244,8 @@ static int vhost_vdpa_get_features(struct vhost_dev *dev,
>      int ret = vhost_vdpa_get_dev_features(dev, features);
>
>      if (ret == 0 && v->shadow_vqs_enabled) {
> -        /* Add SVQ logging capabilities */
> -        *features |= BIT_ULL(VHOST_F_LOG_ALL);
> +        /* Add emulated capabilities */
> +        *features |= v->added_features;
>      }
>
>      return ret;
> diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
> index eebf29f5c1..3803452800 100644
> --- a/net/vhost-vdpa.c
> +++ b/net/vhost-vdpa.c
> @@ -599,6 +599,10 @@ static NetClientState *net_vhost_vdpa_init(NetClientState *peer,
>      s->vhost_vdpa.index = queue_pair_index;
>      s->vhost_vdpa.shadow_vqs_enabled = svq;
>      s->vhost_vdpa.iova_tree = iova_tree;
> +    if (svq) {
> +        /* Add SVQ logging capabilities */
> +        s->vhost_vdpa.added_features |= BIT_ULL(VHOST_F_LOG_ALL);
> +    }
>      if (!is_datapath) {
>          s->cvq_cmd_out_buffer = qemu_memalign(qemu_real_host_page_size(),
>                                              vhost_vdpa_net_cvq_cmd_page_len());
> --
> 2.31.1
>

Re: [RFC PATCH v2 0/8] Guest announce feature emulation using Shadow VirtQueue

[Jason Wang]

On Wed, Oct 19, 2022 at 8:52 PM Eugenio Pérez <eperezma@redhat.com> wrote:
>
> A gratuitous ARP is recommended after a live migration to reduce the amount of
> time needed by the network links to be aware of the new location.

A question: I think we need to deal with the case when GUSET_ANNOUNCE
is not negotiated? E.d sending the gARP by ourselves via vhost-vDPA?

Thanks

> A hypervisor
> may not have the knowledge of the guest network configuration, and this is
> especially true on passthrough devices, so its simpler to ask the guest to
> do it.
>
> However, the device control part of this feature can be totally emulated by
> qemu and shadow virtqueue, not needing any special feature from the actual
> vdpa device.
>
> VIRTIO_NET_F_STATUS is also needed for the guest to access the status of
> virtio net config where announcement status bit is set. Emulating it as
> always active in case backend does not support it.
>
> v2:
> * Add VIRTIO_NET_F_STATUS emulation.
>
> Eugenio Pérez (8):
>   vdpa: Delete duplicated vdpa_feature_bits entry
>   vdpa: Save emulated features list in vhost_vdpa
>   vhost_net: Emulate link state up if backend doesn't expose it
>   vdpa: Expose VIRTIO_NET_F_STATUS unconditionally
>   vdpa: Remove shadow CVQ command check
>   vdpa: handle VIRTIO_NET_CTRL_ANNOUNCE in
>     vhost_vdpa_net_handle_ctrl_avail
>   vhost_net: return VIRTIO_NET_S_ANNOUNCE is device model has it set
>   vdpa: Offer VIRTIO_NET_F_GUEST_ANNOUNCE feature if SVQ is enabled
>
>  include/hw/virtio/vhost-vdpa.h |  2 +
>  hw/net/vhost_net.c             | 35 +++++++++++++++-
>  hw/virtio/vhost-vdpa.c         |  8 ++--
>  net/vhost-vdpa.c               | 74 ++++++++++------------------------
>  4 files changed, 62 insertions(+), 57 deletions(-)
>
> --
> 2.31.1
>
>

Re: [RFC PATCH v2 3/8] vhost_net: Emulate link state up if backend doesn't expose it

[Jason Wang]

On Wed, Oct 19, 2022 at 8:52 PM Eugenio Pérez <eperezma@redhat.com> wrote:
>
> At this moment this code path is not reached, but vdpa devices can offer
> VIRTIO_NET_F_STATUS unconditionally.

So I guess what you mean is that, for the parent that doesn't support
VIRTIO_NET_F_STATUS, emulate one for making sure the ANNOUCNE to work.
This is safe since the spec said the driver will assume the link is
always up if without this feature.

> 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/vhost_net.c | 27 ++++++++++++++++++++++++++-
>  1 file changed, 26 insertions(+), 1 deletion(-)
>
> diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
> index d28f8b974b..5660606c1d 100644
> --- a/hw/net/vhost_net.c
> +++ b/hw/net/vhost_net.c
> @@ -117,7 +117,32 @@ uint64_t vhost_net_get_features(struct vhost_net *net, uint64_t features)
>  int vhost_net_get_config(struct vhost_net *net,  uint8_t *config,
>                           uint32_t config_len)
>  {
> -    return vhost_dev_get_config(&net->dev, config, config_len, NULL);
> +    VirtIODevice *vdev;
> +    int r = vhost_dev_get_config(&net->dev, config, config_len, NULL);
> +
> +    if (unlikely(r != 0)) {
> +        return r;
> +    }
> +
> +    if (config_len < endof(struct virtio_net_config, status)) {
> +        return 0;
> +    }
> +
> +    /*
> +     * TODO: Perform this only if vhost_vdpa.
> +     */

Cindy adds some mediation codes for vhost-vDPA in
virtio_net_get_config(), so I believe it can be done there?

Thanks

> +    vdev = net->dev.vdev;
> +    if (!vdev) {
> +        /* Device is starting */
> +        return 0;
> +    }
> +
> +    if ((net->dev.acked_features & BIT_ULL(VIRTIO_NET_F_STATUS)) &&
> +        !(net->dev.features & BIT_ULL(VIRTIO_NET_F_STATUS))) {
> +        ((struct virtio_net_config *)config)->status |= VIRTIO_NET_S_LINK_UP;
> +    }
> +
> +    return 0;
>  }
>  int vhost_net_set_config(struct vhost_net *net, const uint8_t *data,
>                           uint32_t offset, uint32_t size, uint32_t flags)
> --
> 2.31.1
>

Re: [RFC PATCH v2 5/8] vdpa: Remove shadow CVQ command check

[Jason Wang]

On Wed, Oct 19, 2022 at 8:52 PM Eugenio Pérez <eperezma@redhat.com> wrote:
>
> The guest will see undefined behavior if it issue not negotiate
> commands, bit it is expected somehow.
>
> Simplify code deleting this check.
>
> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>

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

> ---
>  net/vhost-vdpa.c | 48 ------------------------------------------------
>  1 file changed, 48 deletions(-)
>
> diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
> index fca21d5b79..3374c21b4d 100644
> --- a/net/vhost-vdpa.c
> +++ b/net/vhost-vdpa.c
> @@ -461,48 +461,6 @@ static NetClientInfo net_vhost_vdpa_cvq_info = {
>      .check_peer_type = vhost_vdpa_check_peer_type,
>  };
>
> -/**
> - * Do not forward commands not supported by SVQ. Otherwise, the device could
> - * accept it and qemu would not know how to update the device model.
> - */
> -static bool vhost_vdpa_net_cvq_validate_cmd(const void *out_buf, size_t len)
> -{
> -    struct virtio_net_ctrl_hdr ctrl;
> -
> -    if (unlikely(len < sizeof(ctrl))) {
> -        qemu_log_mask(LOG_GUEST_ERROR,
> -                      "%s: invalid legnth of out buffer %zu\n", __func__, len);
> -        return false;
> -    }
> -
> -    memcpy(&ctrl, out_buf, sizeof(ctrl));
> -    switch (ctrl.class) {
> -    case VIRTIO_NET_CTRL_MAC:
> -        switch (ctrl.cmd) {
> -        case VIRTIO_NET_CTRL_MAC_ADDR_SET:
> -            return true;
> -        default:
> -            qemu_log_mask(LOG_GUEST_ERROR, "%s: invalid mac cmd %u\n",
> -                          __func__, ctrl.cmd);
> -        };
> -        break;
> -    case VIRTIO_NET_CTRL_MQ:
> -        switch (ctrl.cmd) {
> -        case VIRTIO_NET_CTRL_MQ_VQ_PAIRS_SET:
> -            return true;
> -        default:
> -            qemu_log_mask(LOG_GUEST_ERROR, "%s: invalid mq cmd %u\n",
> -                          __func__, ctrl.cmd);
> -        };
> -        break;
> -    default:
> -        qemu_log_mask(LOG_GUEST_ERROR, "%s: invalid control class %u\n",
> -                      __func__, ctrl.class);
> -    };
> -
> -    return false;
> -}
> -
>  /**
>   * Validate and copy control virtqueue commands.
>   *
> @@ -526,16 +484,10 @@ static int vhost_vdpa_net_handle_ctrl_avail(VhostShadowVirtqueue *svq,
>          .iov_len = sizeof(status),
>      };
>      ssize_t dev_written = -EINVAL;
> -    bool ok;
>
>      out.iov_len = iov_to_buf(elem->out_sg, elem->out_num, 0,
>                               s->cvq_cmd_out_buffer,
>                               vhost_vdpa_net_cvq_cmd_len());
> -    ok = vhost_vdpa_net_cvq_validate_cmd(s->cvq_cmd_out_buffer, out.iov_len);
> -    if (unlikely(!ok)) {
> -        goto out;
> -    }
> -
>      dev_written = vhost_vdpa_net_cvq_add(s, out.iov_len, sizeof(status));
>      if (unlikely(dev_written < 0)) {
>          goto out;
> --
> 2.31.1
>

Re: [RFC PATCH v2 6/8] vdpa: handle VIRTIO_NET_CTRL_ANNOUNCE in vhost_vdpa_net_handle_ctrl_avail

[Jason Wang]

On Wed, Oct 19, 2022 at 8:52 PM Eugenio Pérez <eperezma@redhat.com> wrote:
>
> Since this capability is emulated by qemu shadowed CVQ cannot forward it
> to the device.

I wonder what happens for a device that has GUEST_ANNOUNCE support on its own?

> Process all that command within qemu.
>
> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> ---
>  net/vhost-vdpa.c | 15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)
>
> diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
> index 3374c21b4d..5fda405a66 100644
> --- a/net/vhost-vdpa.c
> +++ b/net/vhost-vdpa.c
> @@ -488,9 +488,18 @@ static int vhost_vdpa_net_handle_ctrl_avail(VhostShadowVirtqueue *svq,
>      out.iov_len = iov_to_buf(elem->out_sg, elem->out_num, 0,
>                               s->cvq_cmd_out_buffer,
>                               vhost_vdpa_net_cvq_cmd_len());
> -    dev_written = vhost_vdpa_net_cvq_add(s, out.iov_len, sizeof(status));
> -    if (unlikely(dev_written < 0)) {
> -        goto out;
> +    if (*(uint8_t *)s->cvq_cmd_out_buffer == VIRTIO_NET_CTRL_ANNOUNCE) {

Interesting, I thought we can do better by forbidding the code that
goes into vhost-vDPA specific code, everything should be set at
virtio-net.c level.

Thanks

> +        /*
> +         * Guest announce capability is emulated by qemu, so dont forward to
> +         * the device.
> +         */
> +        dev_written = sizeof(status);
> +        *s->status = VIRTIO_NET_OK;
> +    } else {
> +        dev_written = vhost_vdpa_net_cvq_add(s, out.iov_len, sizeof(status));
> +        if (unlikely(dev_written < 0)) {
> +            goto out;
> +        }
>      }
>
>      if (unlikely(dev_written < sizeof(status))) {
> --
> 2.31.1
>

Re: [RFC PATCH v2 7/8] vhost_net: return VIRTIO_NET_S_ANNOUNCE is device model has it set

[Jason Wang]

On Wed, Oct 19, 2022 at 8:52 PM Eugenio Pérez <eperezma@redhat.com> wrote:
>
> Temporal, as this affects other vhost backends and we must check status
> feature first.
>
> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> ---
>  hw/net/vhost_net.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
> index 5660606c1d..300f370e2a 100644
> --- a/hw/net/vhost_net.c
> +++ b/hw/net/vhost_net.c
> @@ -118,6 +118,7 @@ int vhost_net_get_config(struct vhost_net *net,  uint8_t *config,
>                           uint32_t config_len)
>  {
>      VirtIODevice *vdev;
> +    VirtIONet *n;
>      int r = vhost_dev_get_config(&net->dev, config, config_len, NULL);
>
>      if (unlikely(r != 0)) {
> @@ -142,6 +143,13 @@ int vhost_net_get_config(struct vhost_net *net,  uint8_t *config,
>          ((struct virtio_net_config *)config)->status |= VIRTIO_NET_S_LINK_UP;
>      }
>
> +    if (!(net->dev.acked_features & BIT_ULL(VIRTIO_NET_F_GUEST_ANNOUNCE))) {
> +        return 0;
> +    }
> +
> +    n = VIRTIO_NET(vdev);
> +    ((struct virtio_net_config *)config)->status |=
> +                                           (n->status & VIRTIO_NET_S_ANNOUNCE);

Similar to the previous patch, it would be better to move this to virtio-net.c.

Thanks

>      return 0;
>  }
>  int vhost_net_set_config(struct vhost_net *net, const uint8_t *data,
> --
> 2.31.1
>

Re: [RFC PATCH v2 2/8] vdpa: Save emulated features list in vhost_vdpa

[Eugenio Perez Martin]

On Thu, Oct 20, 2022 at 6:23 AM Jason Wang <jasowang@redhat.com> wrote:
>
> On Wed, Oct 19, 2022 at 8:52 PM Eugenio Pérez <eperezma@redhat.com> wrote:
> >
> > At this moment only _F_LOG is added there.
> >
> > However future patches add features that depend on the kind of device.
> > In particular, only net devices can add VIRTIO_F_GUEST_ANNOUNCE. So
> > let's allow vhost_vdpa creator to set custom emulated device features.
> >
> > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > ---
> >  include/hw/virtio/vhost-vdpa.h | 2 ++
> >  hw/virtio/vhost-vdpa.c         | 8 ++++----
> >  net/vhost-vdpa.c               | 4 ++++
> >  3 files changed, 10 insertions(+), 4 deletions(-)
> >
> > diff --git a/include/hw/virtio/vhost-vdpa.h b/include/hw/virtio/vhost-vdpa.h
> > index 1111d85643..50083e1e3b 100644
> > --- a/include/hw/virtio/vhost-vdpa.h
> > +++ b/include/hw/virtio/vhost-vdpa.h
> > @@ -31,6 +31,8 @@ typedef struct vhost_vdpa {
> >      bool iotlb_batch_begin_sent;
> >      MemoryListener listener;
> >      struct vhost_vdpa_iova_range iova_range;
> > +    /* VirtIO device features that can be emulated by qemu */
> > +    uint64_t added_features;
>
> Any reason we need a per vhost_vdpa storage for this? Or is there a
> chance that this field could be different among the devices?
>

Yes, one device could support SVQ and the other one could not support
it because of different feature sets for example.

Thanks!

> Thanks
>
> >      uint64_t acked_features;
> >      bool shadow_vqs_enabled;
> >      /* IOVA mapping used by the Shadow Virtqueue */
> > diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
> > index 7468e44b87..ddb5e29288 100644
> > --- a/hw/virtio/vhost-vdpa.c
> > +++ b/hw/virtio/vhost-vdpa.c
> > @@ -660,8 +660,8 @@ static int vhost_vdpa_set_features(struct vhost_dev *dev,
> >
> >          v->acked_features = features;
> >
> > -        /* We must not ack _F_LOG if SVQ is enabled */
> > -        features &= ~BIT_ULL(VHOST_F_LOG_ALL);
> > +        /* Do not ack features emulated by qemu */
> > +        features &= ~v->added_features;
> >      }
> >
> >      trace_vhost_vdpa_set_features(dev, features);
> > @@ -1244,8 +1244,8 @@ static int vhost_vdpa_get_features(struct vhost_dev *dev,
> >      int ret = vhost_vdpa_get_dev_features(dev, features);
> >
> >      if (ret == 0 && v->shadow_vqs_enabled) {
> > -        /* Add SVQ logging capabilities */
> > -        *features |= BIT_ULL(VHOST_F_LOG_ALL);
> > +        /* Add emulated capabilities */
> > +        *features |= v->added_features;
> >      }
> >
> >      return ret;
> > diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
> > index eebf29f5c1..3803452800 100644
> > --- a/net/vhost-vdpa.c
> > +++ b/net/vhost-vdpa.c
> > @@ -599,6 +599,10 @@ static NetClientState *net_vhost_vdpa_init(NetClientState *peer,
> >      s->vhost_vdpa.index = queue_pair_index;
> >      s->vhost_vdpa.shadow_vqs_enabled = svq;
> >      s->vhost_vdpa.iova_tree = iova_tree;
> > +    if (svq) {
> > +        /* Add SVQ logging capabilities */
> > +        s->vhost_vdpa.added_features |= BIT_ULL(VHOST_F_LOG_ALL);
> > +    }
> >      if (!is_datapath) {
> >          s->cvq_cmd_out_buffer = qemu_memalign(qemu_real_host_page_size(),
> >                                              vhost_vdpa_net_cvq_cmd_page_len());
> > --
> > 2.31.1
> >
>

Re: [RFC PATCH v2 3/8] vhost_net: Emulate link state up if backend doesn't expose it

[Eugenio Perez Martin]

On Thu, Oct 20, 2022 at 6:30 AM Jason Wang <jasowang@redhat.com> wrote:
>
> On Wed, Oct 19, 2022 at 8:52 PM Eugenio Pérez <eperezma@redhat.com> wrote:
> >
> > At this moment this code path is not reached, but vdpa devices can offer
> > VIRTIO_NET_F_STATUS unconditionally.
>
> So I guess what you mean is that, for the parent that doesn't support
> VIRTIO_NET_F_STATUS, emulate one for making sure the ANNOUCNE to work.
> This is safe since the spec said the driver will assume the link is
> always up if without this feature.
>

Right.

> > 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/vhost_net.c | 27 ++++++++++++++++++++++++++-
> >  1 file changed, 26 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
> > index d28f8b974b..5660606c1d 100644
> > --- a/hw/net/vhost_net.c
> > +++ b/hw/net/vhost_net.c
> > @@ -117,7 +117,32 @@ uint64_t vhost_net_get_features(struct vhost_net *net, uint64_t features)
> >  int vhost_net_get_config(struct vhost_net *net,  uint8_t *config,
> >                           uint32_t config_len)
> >  {
> > -    return vhost_dev_get_config(&net->dev, config, config_len, NULL);
> > +    VirtIODevice *vdev;
> > +    int r = vhost_dev_get_config(&net->dev, config, config_len, NULL);
> > +
> > +    if (unlikely(r != 0)) {
> > +        return r;
> > +    }
> > +
> > +    if (config_len < endof(struct virtio_net_config, status)) {
> > +        return 0;
> > +    }
> > +
> > +    /*
> > +     * TODO: Perform this only if vhost_vdpa.
> > +     */
>
> Cindy adds some mediation codes for vhost-vDPA in
> virtio_net_get_config(), so I believe it can be done there?
>

Sure, let me take a look.

Thanks!


> Thanks
>
> > +    vdev = net->dev.vdev;
> > +    if (!vdev) {
> > +        /* Device is starting */
> > +        return 0;
> > +    }
> > +
> > +    if ((net->dev.acked_features & BIT_ULL(VIRTIO_NET_F_STATUS)) &&
> > +        !(net->dev.features & BIT_ULL(VIRTIO_NET_F_STATUS))) {
> > +        ((struct virtio_net_config *)config)->status |= VIRTIO_NET_S_LINK_UP;
> > +    }
> > +
> > +    return 0;
> >  }
> >  int vhost_net_set_config(struct vhost_net *net, const uint8_t *data,
> >                           uint32_t offset, uint32_t size, uint32_t flags)
> > --
> > 2.31.1
> >
>

Re: [RFC PATCH v2 6/8] vdpa: handle VIRTIO_NET_CTRL_ANNOUNCE in vhost_vdpa_net_handle_ctrl_avail

[Eugenio Perez Martin]

On Thu, Oct 20, 2022 at 6:35 AM Jason Wang <jasowang@redhat.com> wrote:
>
> On Wed, Oct 19, 2022 at 8:52 PM Eugenio Pérez <eperezma@redhat.com> wrote:
> >
> > Since this capability is emulated by qemu shadowed CVQ cannot forward it
> > to the device.
>
> I wonder what happens for a device that has GUEST_ANNOUNCE support on its own?
>

If SVQ is enabled the feature is always emulated by qemu by this series.

if SVQ is disabled then the device is the one in charge of all of it.

> > Process all that command within qemu.
> >
> > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > ---
> >  net/vhost-vdpa.c | 15 ++++++++++++---
> >  1 file changed, 12 insertions(+), 3 deletions(-)
> >
> > diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
> > index 3374c21b4d..5fda405a66 100644
> > --- a/net/vhost-vdpa.c
> > +++ b/net/vhost-vdpa.c
> > @@ -488,9 +488,18 @@ static int vhost_vdpa_net_handle_ctrl_avail(VhostShadowVirtqueue *svq,
> >      out.iov_len = iov_to_buf(elem->out_sg, elem->out_num, 0,
> >                               s->cvq_cmd_out_buffer,
> >                               vhost_vdpa_net_cvq_cmd_len());
> > -    dev_written = vhost_vdpa_net_cvq_add(s, out.iov_len, sizeof(status));
> > -    if (unlikely(dev_written < 0)) {
> > -        goto out;
> > +    if (*(uint8_t *)s->cvq_cmd_out_buffer == VIRTIO_NET_CTRL_ANNOUNCE) {
>
> Interesting, I thought we can do better by forbidding the code that
> goes into vhost-vDPA specific code, everything should be set at
> virtio-net.c level.
>

Do you mean to move the SVQ processing to each handle_output? It's
somehow on the roadmap but I'm not sure if it has more priority than
implementing the different features.

Thanks!


> Thanks
>
> > +        /*
> > +         * Guest announce capability is emulated by qemu, so dont forward to
> > +         * the device.
> > +         */
> > +        dev_written = sizeof(status);
> > +        *s->status = VIRTIO_NET_OK;
> > +    } else {
> > +        dev_written = vhost_vdpa_net_cvq_add(s, out.iov_len, sizeof(status));
> > +        if (unlikely(dev_written < 0)) {
> > +            goto out;
> > +        }
> >      }
> >
> >      if (unlikely(dev_written < sizeof(status))) {
> > --
> > 2.31.1
> >
>

Re: [RFC PATCH v2 0/8] Guest announce feature emulation using Shadow VirtQueue

[Eugenio Perez Martin]

On Thu, Oct 20, 2022 at 6:24 AM Jason Wang <jasowang@redhat.com> wrote:
>
> On Wed, Oct 19, 2022 at 8:52 PM Eugenio Pérez <eperezma@redhat.com> wrote:
> >
> > A gratuitous ARP is recommended after a live migration to reduce the amount of
> > time needed by the network links to be aware of the new location.
>
> A question: I think we need to deal with the case when GUSET_ANNOUNCE
> is not negotiated? E.d sending the gARP by ourselves via vhost-vDPA?
>

That is possible and I totally agree to implement it on top of this
series, but it complicates the code in two ways:

1. The startup will be slower and more complicated.
We can only send the gARP from qemu using SVQ. At this point SVQ in
dataplane is always enabled as long as cmdline x-svq is on, but in the
final form it will not be the case, and dataplane will be passthrough.

If we want to send gARP from qemu, this will imply to start the device
all in SVQ, and then reset it to its final configuration.

2. Qemu may not know the actual guest state
For example, regarding the mac filtering qemu will simply allow all
unicast if too many macs are configured in the device, not saving them
individually.

However, I think it is better than nothing for the guest that does not
support GUEST_ANNOUNCE so, as said, I'm ok to implement it on top of
this series for sure. But I think other features should have more
priority, isn't it?

Thanks!

> Thanks
>
> > A hypervisor
> > may not have the knowledge of the guest network configuration, and this is
> > especially true on passthrough devices, so its simpler to ask the guest to
> > do it.
> >
> > However, the device control part of this feature can be totally emulated by
> > qemu and shadow virtqueue, not needing any special feature from the actual
> > vdpa device.
> >
> > VIRTIO_NET_F_STATUS is also needed for the guest to access the status of
> > virtio net config where announcement status bit is set. Emulating it as
> > always active in case backend does not support it.
> >
> > v2:
> > * Add VIRTIO_NET_F_STATUS emulation.
> >
> > Eugenio Pérez (8):
> >   vdpa: Delete duplicated vdpa_feature_bits entry
> >   vdpa: Save emulated features list in vhost_vdpa
> >   vhost_net: Emulate link state up if backend doesn't expose it
> >   vdpa: Expose VIRTIO_NET_F_STATUS unconditionally
> >   vdpa: Remove shadow CVQ command check
> >   vdpa: handle VIRTIO_NET_CTRL_ANNOUNCE in
> >     vhost_vdpa_net_handle_ctrl_avail
> >   vhost_net: return VIRTIO_NET_S_ANNOUNCE is device model has it set
> >   vdpa: Offer VIRTIO_NET_F_GUEST_ANNOUNCE feature if SVQ is enabled
> >
> >  include/hw/virtio/vhost-vdpa.h |  2 +
> >  hw/net/vhost_net.c             | 35 +++++++++++++++-
> >  hw/virtio/vhost-vdpa.c         |  8 ++--
> >  net/vhost-vdpa.c               | 74 ++++++++++------------------------
> >  4 files changed, 62 insertions(+), 57 deletions(-)
> >
> > --
> > 2.31.1
> >
> >
>

Re: [RFC PATCH v2 2/8] vdpa: Save emulated features list in vhost_vdpa

[Jason Wang]

On Thu, Oct 20, 2022 at 2:34 PM Eugenio Perez Martin
<eperezma@redhat.com> wrote:
>
> On Thu, Oct 20, 2022 at 6:23 AM Jason Wang <jasowang@redhat.com> wrote:
> >
> > On Wed, Oct 19, 2022 at 8:52 PM Eugenio Pérez <eperezma@redhat.com> wrote:
> > >
> > > At this moment only _F_LOG is added there.
> > >
> > > However future patches add features that depend on the kind of device.
> > > In particular, only net devices can add VIRTIO_F_GUEST_ANNOUNCE. So
> > > let's allow vhost_vdpa creator to set custom emulated device features.
> > >
> > > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > > ---
> > >  include/hw/virtio/vhost-vdpa.h | 2 ++
> > >  hw/virtio/vhost-vdpa.c         | 8 ++++----
> > >  net/vhost-vdpa.c               | 4 ++++
> > >  3 files changed, 10 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/include/hw/virtio/vhost-vdpa.h b/include/hw/virtio/vhost-vdpa.h
> > > index 1111d85643..50083e1e3b 100644
> > > --- a/include/hw/virtio/vhost-vdpa.h
> > > +++ b/include/hw/virtio/vhost-vdpa.h
> > > @@ -31,6 +31,8 @@ typedef struct vhost_vdpa {
> > >      bool iotlb_batch_begin_sent;
> > >      MemoryListener listener;
> > >      struct vhost_vdpa_iova_range iova_range;
> > > +    /* VirtIO device features that can be emulated by qemu */
> > > +    uint64_t added_features;
> >
> > Any reason we need a per vhost_vdpa storage for this? Or is there a
> > chance that this field could be different among the devices?
> >
>
> Yes, one device could support SVQ and the other one could not support
> it because of different feature sets for example.

Right, but for those devices that don't support SVQ, we don't even
need mediation for feature like F_LOG and _F_STATUS?

Thanks

>
> Thanks!
>
> > Thanks
> >
> > >      uint64_t acked_features;
> > >      bool shadow_vqs_enabled;
> > >      /* IOVA mapping used by the Shadow Virtqueue */
> > > diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
> > > index 7468e44b87..ddb5e29288 100644
> > > --- a/hw/virtio/vhost-vdpa.c
> > > +++ b/hw/virtio/vhost-vdpa.c
> > > @@ -660,8 +660,8 @@ static int vhost_vdpa_set_features(struct vhost_dev *dev,
> > >
> > >          v->acked_features = features;
> > >
> > > -        /* We must not ack _F_LOG if SVQ is enabled */
> > > -        features &= ~BIT_ULL(VHOST_F_LOG_ALL);
> > > +        /* Do not ack features emulated by qemu */
> > > +        features &= ~v->added_features;
> > >      }
> > >
> > >      trace_vhost_vdpa_set_features(dev, features);
> > > @@ -1244,8 +1244,8 @@ static int vhost_vdpa_get_features(struct vhost_dev *dev,
> > >      int ret = vhost_vdpa_get_dev_features(dev, features);
> > >
> > >      if (ret == 0 && v->shadow_vqs_enabled) {
> > > -        /* Add SVQ logging capabilities */
> > > -        *features |= BIT_ULL(VHOST_F_LOG_ALL);
> > > +        /* Add emulated capabilities */
> > > +        *features |= v->added_features;
> > >      }
> > >
> > >      return ret;
> > > diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
> > > index eebf29f5c1..3803452800 100644
> > > --- a/net/vhost-vdpa.c
> > > +++ b/net/vhost-vdpa.c
> > > @@ -599,6 +599,10 @@ static NetClientState *net_vhost_vdpa_init(NetClientState *peer,
> > >      s->vhost_vdpa.index = queue_pair_index;
> > >      s->vhost_vdpa.shadow_vqs_enabled = svq;
> > >      s->vhost_vdpa.iova_tree = iova_tree;
> > > +    if (svq) {
> > > +        /* Add SVQ logging capabilities */
> > > +        s->vhost_vdpa.added_features |= BIT_ULL(VHOST_F_LOG_ALL);
> > > +    }
> > >      if (!is_datapath) {
> > >          s->cvq_cmd_out_buffer = qemu_memalign(qemu_real_host_page_size(),
> > >                                              vhost_vdpa_net_cvq_cmd_page_len());
> > > --
> > > 2.31.1
> > >
> >
>

Re: [RFC PATCH v2 6/8] vdpa: handle VIRTIO_NET_CTRL_ANNOUNCE in vhost_vdpa_net_handle_ctrl_avail

[Jason Wang]

On Thu, Oct 20, 2022 at 3:01 PM Eugenio Perez Martin
<eperezma@redhat.com> wrote:
>
> On Thu, Oct 20, 2022 at 6:35 AM Jason Wang <jasowang@redhat.com> wrote:
> >
> > On Wed, Oct 19, 2022 at 8:52 PM Eugenio Pérez <eperezma@redhat.com> wrote:
> > >
> > > Since this capability is emulated by qemu shadowed CVQ cannot forward it
> > > to the device.
> >
> > I wonder what happens for a device that has GUEST_ANNOUNCE support on its own?
> >
>
> If SVQ is enabled the feature is always emulated by qemu by this series.
>
> if SVQ is disabled then the device is the one in charge of all of it.

Ok, I see.

>
> > > Process all that command within qemu.
> > >
> > > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > > ---
> > >  net/vhost-vdpa.c | 15 ++++++++++++---
> > >  1 file changed, 12 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
> > > index 3374c21b4d..5fda405a66 100644
> > > --- a/net/vhost-vdpa.c
> > > +++ b/net/vhost-vdpa.c
> > > @@ -488,9 +488,18 @@ static int vhost_vdpa_net_handle_ctrl_avail(VhostShadowVirtqueue *svq,
> > >      out.iov_len = iov_to_buf(elem->out_sg, elem->out_num, 0,
> > >                               s->cvq_cmd_out_buffer,
> > >                               vhost_vdpa_net_cvq_cmd_len());
> > > -    dev_written = vhost_vdpa_net_cvq_add(s, out.iov_len, sizeof(status));
> > > -    if (unlikely(dev_written < 0)) {
> > > -        goto out;
> > > +    if (*(uint8_t *)s->cvq_cmd_out_buffer == VIRTIO_NET_CTRL_ANNOUNCE) {
> >
> > Interesting, I thought we can do better by forbidding the code that
> > goes into vhost-vDPA specific code, everything should be set at
> > virtio-net.c level.
> >
>
> Do you mean to move the SVQ processing to each handle_output? It's
> somehow on the roadmap but I'm not sure if it has more priority than
> implementing the different features.

Right, but I think we need to find a way to eliminate the casting here.

Thanks

>
> Thanks!
>
>
> > Thanks
> >
> > > +        /*
> > > +         * Guest announce capability is emulated by qemu, so dont forward to
> > > +         * the device.
> > > +         */
> > > +        dev_written = sizeof(status);
> > > +        *s->status = VIRTIO_NET_OK;
> > > +    } else {
> > > +        dev_written = vhost_vdpa_net_cvq_add(s, out.iov_len, sizeof(status));
> > > +        if (unlikely(dev_written < 0)) {
> > > +            goto out;
> > > +        }
> > >      }
> > >
> > >      if (unlikely(dev_written < sizeof(status))) {
> > > --
> > > 2.31.1
> > >
> >
>

Re: [RFC PATCH v2 2/8] vdpa: Save emulated features list in vhost_vdpa

[Eugenio Perez Martin]

On Fri, Oct 21, 2022 at 4:57 AM Jason Wang <jasowang@redhat.com> wrote:
>
> On Thu, Oct 20, 2022 at 2:34 PM Eugenio Perez Martin
> <eperezma@redhat.com> wrote:
> >
> > On Thu, Oct 20, 2022 at 6:23 AM Jason Wang <jasowang@redhat.com> wrote:
> > >
> > > On Wed, Oct 19, 2022 at 8:52 PM Eugenio Pérez <eperezma@redhat.com> wrote:
> > > >
> > > > At this moment only _F_LOG is added there.
> > > >
> > > > However future patches add features that depend on the kind of device.
> > > > In particular, only net devices can add VIRTIO_F_GUEST_ANNOUNCE. So
> > > > let's allow vhost_vdpa creator to set custom emulated device features.
> > > >
> > > > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > > > ---
> > > >  include/hw/virtio/vhost-vdpa.h | 2 ++
> > > >  hw/virtio/vhost-vdpa.c         | 8 ++++----
> > > >  net/vhost-vdpa.c               | 4 ++++
> > > >  3 files changed, 10 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/include/hw/virtio/vhost-vdpa.h b/include/hw/virtio/vhost-vdpa.h
> > > > index 1111d85643..50083e1e3b 100644
> > > > --- a/include/hw/virtio/vhost-vdpa.h
> > > > +++ b/include/hw/virtio/vhost-vdpa.h
> > > > @@ -31,6 +31,8 @@ typedef struct vhost_vdpa {
> > > >      bool iotlb_batch_begin_sent;
> > > >      MemoryListener listener;
> > > >      struct vhost_vdpa_iova_range iova_range;
> > > > +    /* VirtIO device features that can be emulated by qemu */
> > > > +    uint64_t added_features;
> > >
> > > Any reason we need a per vhost_vdpa storage for this? Or is there a
> > > chance that this field could be different among the devices?
> > >
> >
> > Yes, one device could support SVQ and the other one could not support
> > it because of different feature sets for example.
>
> Right, but for those devices that don't support SVQ, we don't even
> need mediation for feature like F_LOG and _F_STATUS?
>

No, and we cannot offer it to the guest either.

> Thanks
>
> >
> > Thanks!
> >
> > > Thanks
> > >
> > > >      uint64_t acked_features;
> > > >      bool shadow_vqs_enabled;
> > > >      /* IOVA mapping used by the Shadow Virtqueue */
> > > > diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
> > > > index 7468e44b87..ddb5e29288 100644
> > > > --- a/hw/virtio/vhost-vdpa.c
> > > > +++ b/hw/virtio/vhost-vdpa.c
> > > > @@ -660,8 +660,8 @@ static int vhost_vdpa_set_features(struct vhost_dev *dev,
> > > >
> > > >          v->acked_features = features;
> > > >
> > > > -        /* We must not ack _F_LOG if SVQ is enabled */
> > > > -        features &= ~BIT_ULL(VHOST_F_LOG_ALL);
> > > > +        /* Do not ack features emulated by qemu */
> > > > +        features &= ~v->added_features;
> > > >      }
> > > >
> > > >      trace_vhost_vdpa_set_features(dev, features);
> > > > @@ -1244,8 +1244,8 @@ static int vhost_vdpa_get_features(struct vhost_dev *dev,
> > > >      int ret = vhost_vdpa_get_dev_features(dev, features);
> > > >
> > > >      if (ret == 0 && v->shadow_vqs_enabled) {
> > > > -        /* Add SVQ logging capabilities */
> > > > -        *features |= BIT_ULL(VHOST_F_LOG_ALL);
> > > > +        /* Add emulated capabilities */
> > > > +        *features |= v->added_features;
> > > >      }
> > > >
> > > >      return ret;
> > > > diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
> > > > index eebf29f5c1..3803452800 100644
> > > > --- a/net/vhost-vdpa.c
> > > > +++ b/net/vhost-vdpa.c
> > > > @@ -599,6 +599,10 @@ static NetClientState *net_vhost_vdpa_init(NetClientState *peer,
> > > >      s->vhost_vdpa.index = queue_pair_index;
> > > >      s->vhost_vdpa.shadow_vqs_enabled = svq;
> > > >      s->vhost_vdpa.iova_tree = iova_tree;
> > > > +    if (svq) {
> > > > +        /* Add SVQ logging capabilities */
> > > > +        s->vhost_vdpa.added_features |= BIT_ULL(VHOST_F_LOG_ALL);
> > > > +    }
> > > >      if (!is_datapath) {
> > > >          s->cvq_cmd_out_buffer = qemu_memalign(qemu_real_host_page_size(),
> > > >                                              vhost_vdpa_net_cvq_cmd_page_len());
> > > > --
> > > > 2.31.1
> > > >
> > >
> >
>

Re: [RFC PATCH v2 6/8] vdpa: handle VIRTIO_NET_CTRL_ANNOUNCE in vhost_vdpa_net_handle_ctrl_avail

[Eugenio Perez Martin]

On Fri, Oct 21, 2022 at 5:02 AM Jason Wang <jasowang@redhat.com> wrote:
>
> On Thu, Oct 20, 2022 at 3:01 PM Eugenio Perez Martin
> <eperezma@redhat.com> wrote:
> >
> > On Thu, Oct 20, 2022 at 6:35 AM Jason Wang <jasowang@redhat.com> wrote:
> > >
> > > On Wed, Oct 19, 2022 at 8:52 PM Eugenio Pérez <eperezma@redhat.com> wrote:
> > > >
> > > > Since this capability is emulated by qemu shadowed CVQ cannot forward it
> > > > to the device.
> > >
> > > I wonder what happens for a device that has GUEST_ANNOUNCE support on its own?
> > >
> >
> > If SVQ is enabled the feature is always emulated by qemu by this series.
> >
> > if SVQ is disabled then the device is the one in charge of all of it.
>
> Ok, I see.
>
> >
> > > > Process all that command within qemu.
> > > >
> > > > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > > > ---
> > > >  net/vhost-vdpa.c | 15 ++++++++++++---
> > > >  1 file changed, 12 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
> > > > index 3374c21b4d..5fda405a66 100644
> > > > --- a/net/vhost-vdpa.c
> > > > +++ b/net/vhost-vdpa.c
> > > > @@ -488,9 +488,18 @@ static int vhost_vdpa_net_handle_ctrl_avail(VhostShadowVirtqueue *svq,
> > > >      out.iov_len = iov_to_buf(elem->out_sg, elem->out_num, 0,
> > > >                               s->cvq_cmd_out_buffer,
> > > >                               vhost_vdpa_net_cvq_cmd_len());
> > > > -    dev_written = vhost_vdpa_net_cvq_add(s, out.iov_len, sizeof(status));
> > > > -    if (unlikely(dev_written < 0)) {
> > > > -        goto out;
> > > > +    if (*(uint8_t *)s->cvq_cmd_out_buffer == VIRTIO_NET_CTRL_ANNOUNCE) {
> > >
> > > Interesting, I thought we can do better by forbidding the code that
> > > goes into vhost-vDPA specific code, everything should be set at
> > > virtio-net.c level.
> > >
> >
> > Do you mean to move the SVQ processing to each handle_output? It's
> > somehow on the roadmap but I'm not sure if it has more priority than
> > implementing the different features.
>
> Right, but I think we need to find a way to eliminate the casting here.
>

Would it work to use it this way?
uint8_t *virtio_net_ctrl_class = s->cvq_cmd_out_buffer
if (*virtio_net_ctrl_class == VIRTIO_NET_CTRL_ANNOUNCE) {
  ...
}

> Thanks
>
> >
> > Thanks!
> >
> >
> > > Thanks
> > >
> > > > +        /*
> > > > +         * Guest announce capability is emulated by qemu, so dont forward to
> > > > +         * the device.
> > > > +         */
> > > > +        dev_written = sizeof(status);
> > > > +        *s->status = VIRTIO_NET_OK;
> > > > +    } else {
> > > > +        dev_written = vhost_vdpa_net_cvq_add(s, out.iov_len, sizeof(status));
> > > > +        if (unlikely(dev_written < 0)) {
> > > > +            goto out;
> > > > +        }
> > > >      }
> > > >
> > > >      if (unlikely(dev_written < sizeof(status))) {
> > > > --
> > > > 2.31.1
> > > >
> > >
> >
>

Re: [RFC PATCH v2 2/8] vdpa: Save emulated features list in vhost_vdpa

[Jason Wang]

On Fri, Oct 21, 2022 at 4:56 PM Eugenio Perez Martin
<eperezma@redhat.com> wrote:
>
> On Fri, Oct 21, 2022 at 4:57 AM Jason Wang <jasowang@redhat.com> wrote:
> >
> > On Thu, Oct 20, 2022 at 2:34 PM Eugenio Perez Martin
> > <eperezma@redhat.com> wrote:
> > >
> > > On Thu, Oct 20, 2022 at 6:23 AM Jason Wang <jasowang@redhat.com> wrote:
> > > >
> > > > On Wed, Oct 19, 2022 at 8:52 PM Eugenio Pérez <eperezma@redhat.com> wrote:
> > > > >
> > > > > At this moment only _F_LOG is added there.
> > > > >
> > > > > However future patches add features that depend on the kind of device.
> > > > > In particular, only net devices can add VIRTIO_F_GUEST_ANNOUNCE. So
> > > > > let's allow vhost_vdpa creator to set custom emulated device features.
> > > > >
> > > > > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > > > > ---
> > > > >  include/hw/virtio/vhost-vdpa.h | 2 ++
> > > > >  hw/virtio/vhost-vdpa.c         | 8 ++++----
> > > > >  net/vhost-vdpa.c               | 4 ++++
> > > > >  3 files changed, 10 insertions(+), 4 deletions(-)
> > > > >
> > > > > diff --git a/include/hw/virtio/vhost-vdpa.h b/include/hw/virtio/vhost-vdpa.h
> > > > > index 1111d85643..50083e1e3b 100644
> > > > > --- a/include/hw/virtio/vhost-vdpa.h
> > > > > +++ b/include/hw/virtio/vhost-vdpa.h
> > > > > @@ -31,6 +31,8 @@ typedef struct vhost_vdpa {
> > > > >      bool iotlb_batch_begin_sent;
> > > > >      MemoryListener listener;
> > > > >      struct vhost_vdpa_iova_range iova_range;
> > > > > +    /* VirtIO device features that can be emulated by qemu */
> > > > > +    uint64_t added_features;
> > > >
> > > > Any reason we need a per vhost_vdpa storage for this? Or is there a
> > > > chance that this field could be different among the devices?
> > > >
> > >
> > > Yes, one device could support SVQ and the other one could not support
> > > it because of different feature sets for example.
> >
> > Right, but for those devices that don't support SVQ, we don't even
> > need mediation for feature like F_LOG and _F_STATUS?
> >
>
> No, and we cannot offer it to the guest either.

Just to make sure we are on the same page, what I meant is, consider
in the future SVQ get the support of all features, so we can remove
this field? This is because _F_STATUS can be mediated unconditionally
anyhow.

Thanks

>
> > Thanks
> >
> > >
> > > Thanks!
> > >
> > > > Thanks
> > > >
> > > > >      uint64_t acked_features;
> > > > >      bool shadow_vqs_enabled;
> > > > >      /* IOVA mapping used by the Shadow Virtqueue */
> > > > > diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
> > > > > index 7468e44b87..ddb5e29288 100644
> > > > > --- a/hw/virtio/vhost-vdpa.c
> > > > > +++ b/hw/virtio/vhost-vdpa.c
> > > > > @@ -660,8 +660,8 @@ static int vhost_vdpa_set_features(struct vhost_dev *dev,
> > > > >
> > > > >          v->acked_features = features;
> > > > >
> > > > > -        /* We must not ack _F_LOG if SVQ is enabled */
> > > > > -        features &= ~BIT_ULL(VHOST_F_LOG_ALL);
> > > > > +        /* Do not ack features emulated by qemu */
> > > > > +        features &= ~v->added_features;
> > > > >      }
> > > > >
> > > > >      trace_vhost_vdpa_set_features(dev, features);
> > > > > @@ -1244,8 +1244,8 @@ static int vhost_vdpa_get_features(struct vhost_dev *dev,
> > > > >      int ret = vhost_vdpa_get_dev_features(dev, features);
> > > > >
> > > > >      if (ret == 0 && v->shadow_vqs_enabled) {
> > > > > -        /* Add SVQ logging capabilities */
> > > > > -        *features |= BIT_ULL(VHOST_F_LOG_ALL);
> > > > > +        /* Add emulated capabilities */
> > > > > +        *features |= v->added_features;
> > > > >      }
> > > > >
> > > > >      return ret;
> > > > > diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
> > > > > index eebf29f5c1..3803452800 100644
> > > > > --- a/net/vhost-vdpa.c
> > > > > +++ b/net/vhost-vdpa.c
> > > > > @@ -599,6 +599,10 @@ static NetClientState *net_vhost_vdpa_init(NetClientState *peer,
> > > > >      s->vhost_vdpa.index = queue_pair_index;
> > > > >      s->vhost_vdpa.shadow_vqs_enabled = svq;
> > > > >      s->vhost_vdpa.iova_tree = iova_tree;
> > > > > +    if (svq) {
> > > > > +        /* Add SVQ logging capabilities */
> > > > > +        s->vhost_vdpa.added_features |= BIT_ULL(VHOST_F_LOG_ALL);
> > > > > +    }
> > > > >      if (!is_datapath) {
> > > > >          s->cvq_cmd_out_buffer = qemu_memalign(qemu_real_host_page_size(),
> > > > >                                              vhost_vdpa_net_cvq_cmd_page_len());
> > > > > --
> > > > > 2.31.1
> > > > >
> > > >
> > >
> >
>

Re: [RFC PATCH v2 6/8] vdpa: handle VIRTIO_NET_CTRL_ANNOUNCE in vhost_vdpa_net_handle_ctrl_avail

[Jason Wang]

On Fri, Oct 21, 2022 at 5:05 PM Eugenio Perez Martin
<eperezma@redhat.com> wrote:
>
> On Fri, Oct 21, 2022 at 5:02 AM Jason Wang <jasowang@redhat.com> wrote:
> >
> > On Thu, Oct 20, 2022 at 3:01 PM Eugenio Perez Martin
> > <eperezma@redhat.com> wrote:
> > >
> > > On Thu, Oct 20, 2022 at 6:35 AM Jason Wang <jasowang@redhat.com> wrote:
> > > >
> > > > On Wed, Oct 19, 2022 at 8:52 PM Eugenio Pérez <eperezma@redhat.com> wrote:
> > > > >
> > > > > Since this capability is emulated by qemu shadowed CVQ cannot forward it
> > > > > to the device.
> > > >
> > > > I wonder what happens for a device that has GUEST_ANNOUNCE support on its own?
> > > >
> > >
> > > If SVQ is enabled the feature is always emulated by qemu by this series.
> > >
> > > if SVQ is disabled then the device is the one in charge of all of it.
> >
> > Ok, I see.
> >
> > >
> > > > > Process all that command within qemu.
> > > > >
> > > > > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > > > > ---
> > > > >  net/vhost-vdpa.c | 15 ++++++++++++---
> > > > >  1 file changed, 12 insertions(+), 3 deletions(-)
> > > > >
> > > > > diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
> > > > > index 3374c21b4d..5fda405a66 100644
> > > > > --- a/net/vhost-vdpa.c
> > > > > +++ b/net/vhost-vdpa.c
> > > > > @@ -488,9 +488,18 @@ static int vhost_vdpa_net_handle_ctrl_avail(VhostShadowVirtqueue *svq,
> > > > >      out.iov_len = iov_to_buf(elem->out_sg, elem->out_num, 0,
> > > > >                               s->cvq_cmd_out_buffer,
> > > > >                               vhost_vdpa_net_cvq_cmd_len());
> > > > > -    dev_written = vhost_vdpa_net_cvq_add(s, out.iov_len, sizeof(status));
> > > > > -    if (unlikely(dev_written < 0)) {
> > > > > -        goto out;
> > > > > +    if (*(uint8_t *)s->cvq_cmd_out_buffer == VIRTIO_NET_CTRL_ANNOUNCE) {
> > > >
> > > > Interesting, I thought we can do better by forbidding the code that
> > > > goes into vhost-vDPA specific code, everything should be set at
> > > > virtio-net.c level.
> > > >
> > >
> > > Do you mean to move the SVQ processing to each handle_output? It's
> > > somehow on the roadmap but I'm not sure if it has more priority than
> > > implementing the different features.
> >
> > Right, but I think we need to find a way to eliminate the casting here.
> >
>
> Would it work to use it this way?
> uint8_t *virtio_net_ctrl_class = s->cvq_cmd_out_buffer
> if (*virtio_net_ctrl_class == VIRTIO_NET_CTRL_ANNOUNCE) {
>   ...
> }

Something like this, yes.

Thanks

>
> > Thanks
> >
> > >
> > > Thanks!
> > >
> > >
> > > > Thanks
> > > >
> > > > > +        /*
> > > > > +         * Guest announce capability is emulated by qemu, so dont forward to
> > > > > +         * the device.
> > > > > +         */
> > > > > +        dev_written = sizeof(status);
> > > > > +        *s->status = VIRTIO_NET_OK;
> > > > > +    } else {
> > > > > +        dev_written = vhost_vdpa_net_cvq_add(s, out.iov_len, sizeof(status));
> > > > > +        if (unlikely(dev_written < 0)) {
> > > > > +            goto out;
> > > > > +        }
> > > > >      }
> > > > >
> > > > >      if (unlikely(dev_written < sizeof(status))) {
> > > > > --
> > > > > 2.31.1
> > > > >
> > > >
> > >
> >
>

Re: [RFC PATCH v2 2/8] vdpa: Save emulated features list in vhost_vdpa

[Eugenio Perez Martin]

On Mon, Oct 24, 2022 at 4:14 AM Jason Wang <jasowang@redhat.com> wrote:
>
> On Fri, Oct 21, 2022 at 4:56 PM Eugenio Perez Martin
> <eperezma@redhat.com> wrote:
> >
> > On Fri, Oct 21, 2022 at 4:57 AM Jason Wang <jasowang@redhat.com> wrote:
> > >
> > > On Thu, Oct 20, 2022 at 2:34 PM Eugenio Perez Martin
> > > <eperezma@redhat.com> wrote:
> > > >
> > > > On Thu, Oct 20, 2022 at 6:23 AM Jason Wang <jasowang@redhat.com> wrote:
> > > > >
> > > > > On Wed, Oct 19, 2022 at 8:52 PM Eugenio Pérez <eperezma@redhat.com> wrote:
> > > > > >
> > > > > > At this moment only _F_LOG is added there.
> > > > > >
> > > > > > However future patches add features that depend on the kind of device.
> > > > > > In particular, only net devices can add VIRTIO_F_GUEST_ANNOUNCE. So
> > > > > > let's allow vhost_vdpa creator to set custom emulated device features.
> > > > > >
> > > > > > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > > > > > ---
> > > > > >  include/hw/virtio/vhost-vdpa.h | 2 ++
> > > > > >  hw/virtio/vhost-vdpa.c         | 8 ++++----
> > > > > >  net/vhost-vdpa.c               | 4 ++++
> > > > > >  3 files changed, 10 insertions(+), 4 deletions(-)
> > > > > >
> > > > > > diff --git a/include/hw/virtio/vhost-vdpa.h b/include/hw/virtio/vhost-vdpa.h
> > > > > > index 1111d85643..50083e1e3b 100644
> > > > > > --- a/include/hw/virtio/vhost-vdpa.h
> > > > > > +++ b/include/hw/virtio/vhost-vdpa.h
> > > > > > @@ -31,6 +31,8 @@ typedef struct vhost_vdpa {
> > > > > >      bool iotlb_batch_begin_sent;
> > > > > >      MemoryListener listener;
> > > > > >      struct vhost_vdpa_iova_range iova_range;
> > > > > > +    /* VirtIO device features that can be emulated by qemu */
> > > > > > +    uint64_t added_features;
> > > > >
> > > > > Any reason we need a per vhost_vdpa storage for this? Or is there a
> > > > > chance that this field could be different among the devices?
> > > > >
> > > >
> > > > Yes, one device could support SVQ and the other one could not support
> > > > it because of different feature sets for example.
> > >
> > > Right, but for those devices that don't support SVQ, we don't even
> > > need mediation for feature like F_LOG and _F_STATUS?
> > >
> >
> > No, and we cannot offer it to the guest either.
>
> Just to make sure we are on the same page, what I meant is, consider
> in the future SVQ get the support of all features, so we can remove
> this field? This is because _F_STATUS can be mediated unconditionally
> anyhow.
>

For _F_STATUS that is right. But we cannot handle full
_F_GUEST_ANNOUNCE since control SVQ (will) needs features from the
device that cannot be emulated, like ASID.

I think your point is "Since qemu cannot migrate these devices it will
never set VIRTIO_NET_S_ANNOUNCE, so the guest will never send
VIRTIO_NET_CTRL_ANNOUNCE messages". And I think that is totally right,
but I still feel it is weird to expose it if we cannot handle it.

Maybe a good first step is to move added_features to vhost_net, or
maybe to convert it to "bool guest_announce_emulated" or something
similar?  This way hw/virtio/vhost-vdpa is totally unaware of this and
changes are more self contained.

Thanks!



> Thanks
>
> >
> > > Thanks
> > >
> > > >
> > > > Thanks!
> > > >
> > > > > Thanks
> > > > >
> > > > > >      uint64_t acked_features;
> > > > > >      bool shadow_vqs_enabled;
> > > > > >      /* IOVA mapping used by the Shadow Virtqueue */
> > > > > > diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
> > > > > > index 7468e44b87..ddb5e29288 100644
> > > > > > --- a/hw/virtio/vhost-vdpa.c
> > > > > > +++ b/hw/virtio/vhost-vdpa.c
> > > > > > @@ -660,8 +660,8 @@ static int vhost_vdpa_set_features(struct vhost_dev *dev,
> > > > > >
> > > > > >          v->acked_features = features;
> > > > > >
> > > > > > -        /* We must not ack _F_LOG if SVQ is enabled */
> > > > > > -        features &= ~BIT_ULL(VHOST_F_LOG_ALL);
> > > > > > +        /* Do not ack features emulated by qemu */
> > > > > > +        features &= ~v->added_features;
> > > > > >      }
> > > > > >
> > > > > >      trace_vhost_vdpa_set_features(dev, features);
> > > > > > @@ -1244,8 +1244,8 @@ static int vhost_vdpa_get_features(struct vhost_dev *dev,
> > > > > >      int ret = vhost_vdpa_get_dev_features(dev, features);
> > > > > >
> > > > > >      if (ret == 0 && v->shadow_vqs_enabled) {
> > > > > > -        /* Add SVQ logging capabilities */
> > > > > > -        *features |= BIT_ULL(VHOST_F_LOG_ALL);
> > > > > > +        /* Add emulated capabilities */
> > > > > > +        *features |= v->added_features;
> > > > > >      }
> > > > > >
> > > > > >      return ret;
> > > > > > diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
> > > > > > index eebf29f5c1..3803452800 100644
> > > > > > --- a/net/vhost-vdpa.c
> > > > > > +++ b/net/vhost-vdpa.c
> > > > > > @@ -599,6 +599,10 @@ static NetClientState *net_vhost_vdpa_init(NetClientState *peer,
> > > > > >      s->vhost_vdpa.index = queue_pair_index;
> > > > > >      s->vhost_vdpa.shadow_vqs_enabled = svq;
> > > > > >      s->vhost_vdpa.iova_tree = iova_tree;
> > > > > > +    if (svq) {
> > > > > > +        /* Add SVQ logging capabilities */
> > > > > > +        s->vhost_vdpa.added_features |= BIT_ULL(VHOST_F_LOG_ALL);
> > > > > > +    }
> > > > > >      if (!is_datapath) {
> > > > > >          s->cvq_cmd_out_buffer = qemu_memalign(qemu_real_host_page_size(),
> > > > > >                                              vhost_vdpa_net_cvq_cmd_page_len());
> > > > > > --
> > > > > > 2.31.1
> > > > > >
> > > > >
> > > >
> > >
> >
>

Re: [RFC PATCH v2 2/8] vdpa: Save emulated features list in vhost_vdpa

[Jason Wang]

On Mon, Oct 24, 2022 at 5:26 PM Eugenio Perez Martin
<eperezma@redhat.com> wrote:
>
> On Mon, Oct 24, 2022 at 4:14 AM Jason Wang <jasowang@redhat.com> wrote:
> >
> > On Fri, Oct 21, 2022 at 4:56 PM Eugenio Perez Martin
> > <eperezma@redhat.com> wrote:
> > >
> > > On Fri, Oct 21, 2022 at 4:57 AM Jason Wang <jasowang@redhat.com> wrote:
> > > >
> > > > On Thu, Oct 20, 2022 at 2:34 PM Eugenio Perez Martin
> > > > <eperezma@redhat.com> wrote:
> > > > >
> > > > > On Thu, Oct 20, 2022 at 6:23 AM Jason Wang <jasowang@redhat.com> wrote:
> > > > > >
> > > > > > On Wed, Oct 19, 2022 at 8:52 PM Eugenio Pérez <eperezma@redhat.com> wrote:
> > > > > > >
> > > > > > > At this moment only _F_LOG is added there.
> > > > > > >
> > > > > > > However future patches add features that depend on the kind of device.
> > > > > > > In particular, only net devices can add VIRTIO_F_GUEST_ANNOUNCE. So
> > > > > > > let's allow vhost_vdpa creator to set custom emulated device features.
> > > > > > >
> > > > > > > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > > > > > > ---
> > > > > > >  include/hw/virtio/vhost-vdpa.h | 2 ++
> > > > > > >  hw/virtio/vhost-vdpa.c         | 8 ++++----
> > > > > > >  net/vhost-vdpa.c               | 4 ++++
> > > > > > >  3 files changed, 10 insertions(+), 4 deletions(-)
> > > > > > >
> > > > > > > diff --git a/include/hw/virtio/vhost-vdpa.h b/include/hw/virtio/vhost-vdpa.h
> > > > > > > index 1111d85643..50083e1e3b 100644
> > > > > > > --- a/include/hw/virtio/vhost-vdpa.h
> > > > > > > +++ b/include/hw/virtio/vhost-vdpa.h
> > > > > > > @@ -31,6 +31,8 @@ typedef struct vhost_vdpa {
> > > > > > >      bool iotlb_batch_begin_sent;
> > > > > > >      MemoryListener listener;
> > > > > > >      struct vhost_vdpa_iova_range iova_range;
> > > > > > > +    /* VirtIO device features that can be emulated by qemu */
> > > > > > > +    uint64_t added_features;
> > > > > >
> > > > > > Any reason we need a per vhost_vdpa storage for this? Or is there a
> > > > > > chance that this field could be different among the devices?
> > > > > >
> > > > >
> > > > > Yes, one device could support SVQ and the other one could not support
> > > > > it because of different feature sets for example.
> > > >
> > > > Right, but for those devices that don't support SVQ, we don't even
> > > > need mediation for feature like F_LOG and _F_STATUS?
> > > >
> > >
> > > No, and we cannot offer it to the guest either.
> >
> > Just to make sure we are on the same page, what I meant is, consider
> > in the future SVQ get the support of all features, so we can remove
> > this field? This is because _F_STATUS can be mediated unconditionally
> > anyhow.
> >
>
> For _F_STATUS that is right. But we cannot handle full
> _F_GUEST_ANNOUNCE since control SVQ (will) needs features from the
> device that cannot be emulated, like ASID.
>
> I think your point is "Since qemu cannot migrate these devices it will
> never set VIRTIO_NET_S_ANNOUNCE, so the guest will never send
> VIRTIO_NET_CTRL_ANNOUNCE messages". And I think that is totally right,
> but I still feel it is weird to expose it if we cannot handle it.
>
> Maybe a good first step is to move added_features to vhost_net, or
> maybe to convert it to "bool guest_announce_emulated" or something
> similar?  This way hw/virtio/vhost-vdpa is totally unaware of this and
> changes are more self contained.

This reminds me of something. For vhost, if Qemu can handle some
feature bits, we don't need to validate if vhost has such support.

E.g we don't have _F_SATAUS and _F_GUEST_ANNOUNCE in kernel_feature_bits.

I wonder if we can do something similar for vhost-vDPA? Then we don't
need to bother new fields.

Thanks

>
> Thanks!
>
>
>
> > Thanks
> >
> > >
> > > > Thanks
> > > >
> > > > >
> > > > > Thanks!
> > > > >
> > > > > > Thanks
> > > > > >
> > > > > > >      uint64_t acked_features;
> > > > > > >      bool shadow_vqs_enabled;
> > > > > > >      /* IOVA mapping used by the Shadow Virtqueue */
> > > > > > > diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
> > > > > > > index 7468e44b87..ddb5e29288 100644
> > > > > > > --- a/hw/virtio/vhost-vdpa.c
> > > > > > > +++ b/hw/virtio/vhost-vdpa.c
> > > > > > > @@ -660,8 +660,8 @@ static int vhost_vdpa_set_features(struct vhost_dev *dev,
> > > > > > >
> > > > > > >          v->acked_features = features;
> > > > > > >
> > > > > > > -        /* We must not ack _F_LOG if SVQ is enabled */
> > > > > > > -        features &= ~BIT_ULL(VHOST_F_LOG_ALL);
> > > > > > > +        /* Do not ack features emulated by qemu */
> > > > > > > +        features &= ~v->added_features;
> > > > > > >      }
> > > > > > >
> > > > > > >      trace_vhost_vdpa_set_features(dev, features);
> > > > > > > @@ -1244,8 +1244,8 @@ static int vhost_vdpa_get_features(struct vhost_dev *dev,
> > > > > > >      int ret = vhost_vdpa_get_dev_features(dev, features);
> > > > > > >
> > > > > > >      if (ret == 0 && v->shadow_vqs_enabled) {
> > > > > > > -        /* Add SVQ logging capabilities */
> > > > > > > -        *features |= BIT_ULL(VHOST_F_LOG_ALL);
> > > > > > > +        /* Add emulated capabilities */
> > > > > > > +        *features |= v->added_features;
> > > > > > >      }
> > > > > > >
> > > > > > >      return ret;
> > > > > > > diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
> > > > > > > index eebf29f5c1..3803452800 100644
> > > > > > > --- a/net/vhost-vdpa.c
> > > > > > > +++ b/net/vhost-vdpa.c
> > > > > > > @@ -599,6 +599,10 @@ static NetClientState *net_vhost_vdpa_init(NetClientState *peer,
> > > > > > >      s->vhost_vdpa.index = queue_pair_index;
> > > > > > >      s->vhost_vdpa.shadow_vqs_enabled = svq;
> > > > > > >      s->vhost_vdpa.iova_tree = iova_tree;
> > > > > > > +    if (svq) {
> > > > > > > +        /* Add SVQ logging capabilities */
> > > > > > > +        s->vhost_vdpa.added_features |= BIT_ULL(VHOST_F_LOG_ALL);
> > > > > > > +    }
> > > > > > >      if (!is_datapath) {
> > > > > > >          s->cvq_cmd_out_buffer = qemu_memalign(qemu_real_host_page_size(),
> > > > > > >                                              vhost_vdpa_net_cvq_cmd_page_len());
> > > > > > > --
> > > > > > > 2.31.1
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>

Re: [RFC PATCH v2 2/8] vdpa: Save emulated features list in vhost_vdpa

[Eugenio Perez Martin]

On Tue, Oct 25, 2022 at 4:45 AM Jason Wang <jasowang@redhat.com> wrote:
>
> On Mon, Oct 24, 2022 at 5:26 PM Eugenio Perez Martin
> <eperezma@redhat.com> wrote:
> >
> > On Mon, Oct 24, 2022 at 4:14 AM Jason Wang <jasowang@redhat.com> wrote:
> > >
> > > On Fri, Oct 21, 2022 at 4:56 PM Eugenio Perez Martin
> > > <eperezma@redhat.com> wrote:
> > > >
> > > > On Fri, Oct 21, 2022 at 4:57 AM Jason Wang <jasowang@redhat.com> wrote:
> > > > >
> > > > > On Thu, Oct 20, 2022 at 2:34 PM Eugenio Perez Martin
> > > > > <eperezma@redhat.com> wrote:
> > > > > >
> > > > > > On Thu, Oct 20, 2022 at 6:23 AM Jason Wang <jasowang@redhat.com> wrote:
> > > > > > >
> > > > > > > On Wed, Oct 19, 2022 at 8:52 PM Eugenio Pérez <eperezma@redhat.com> wrote:
> > > > > > > >
> > > > > > > > At this moment only _F_LOG is added there.
> > > > > > > >
> > > > > > > > However future patches add features that depend on the kind of device.
> > > > > > > > In particular, only net devices can add VIRTIO_F_GUEST_ANNOUNCE. So
> > > > > > > > let's allow vhost_vdpa creator to set custom emulated device features.
> > > > > > > >
> > > > > > > > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > > > > > > > ---
> > > > > > > >  include/hw/virtio/vhost-vdpa.h | 2 ++
> > > > > > > >  hw/virtio/vhost-vdpa.c         | 8 ++++----
> > > > > > > >  net/vhost-vdpa.c               | 4 ++++
> > > > > > > >  3 files changed, 10 insertions(+), 4 deletions(-)
> > > > > > > >
> > > > > > > > diff --git a/include/hw/virtio/vhost-vdpa.h b/include/hw/virtio/vhost-vdpa.h
> > > > > > > > index 1111d85643..50083e1e3b 100644
> > > > > > > > --- a/include/hw/virtio/vhost-vdpa.h
> > > > > > > > +++ b/include/hw/virtio/vhost-vdpa.h
> > > > > > > > @@ -31,6 +31,8 @@ typedef struct vhost_vdpa {
> > > > > > > >      bool iotlb_batch_begin_sent;
> > > > > > > >      MemoryListener listener;
> > > > > > > >      struct vhost_vdpa_iova_range iova_range;
> > > > > > > > +    /* VirtIO device features that can be emulated by qemu */
> > > > > > > > +    uint64_t added_features;
> > > > > > >
> > > > > > > Any reason we need a per vhost_vdpa storage for this? Or is there a
> > > > > > > chance that this field could be different among the devices?
> > > > > > >
> > > > > >
> > > > > > Yes, one device could support SVQ and the other one could not support
> > > > > > it because of different feature sets for example.
> > > > >
> > > > > Right, but for those devices that don't support SVQ, we don't even
> > > > > need mediation for feature like F_LOG and _F_STATUS?
> > > > >
> > > >
> > > > No, and we cannot offer it to the guest either.
> > >
> > > Just to make sure we are on the same page, what I meant is, consider
> > > in the future SVQ get the support of all features, so we can remove
> > > this field? This is because _F_STATUS can be mediated unconditionally
> > > anyhow.
> > >
> >
> > For _F_STATUS that is right. But we cannot handle full
> > _F_GUEST_ANNOUNCE since control SVQ (will) needs features from the
> > device that cannot be emulated, like ASID.
> >
> > I think your point is "Since qemu cannot migrate these devices it will
> > never set VIRTIO_NET_S_ANNOUNCE, so the guest will never send
> > VIRTIO_NET_CTRL_ANNOUNCE messages". And I think that is totally right,
> > but I still feel it is weird to expose it if we cannot handle it.
> >
> > Maybe a good first step is to move added_features to vhost_net, or
> > maybe to convert it to "bool guest_announce_emulated" or something
> > similar?  This way hw/virtio/vhost-vdpa is totally unaware of this and
> > changes are more self contained.
>
> This reminds me of something. For vhost, if Qemu can handle some
> feature bits, we don't need to validate if vhost has such support.
>
> E.g we don't have _F_SATAUS and _F_GUEST_ANNOUNCE in kernel_feature_bits.
>
> I wonder if we can do something similar for vhost-vDPA? Then we don't
> need to bother new fields.
>

That is valid for _F_GUEST_ANNOUNCE, because all of it is emulated and
we must forbid the ack of it for simplicity.

But _F_STATUS has two parts, one bit that must be exposed to the guest
from the physical NIC, since qemu cannot know when to change it
(VIRTIO_NET_S_LINK_UP) and another one that must be 100% emulated
(VIRTIO_NET_S_ANNOUNCE). VIRTIO_NET_F_STATUS is 100% emulated only if
it is not offered by the device.

I'm moving to vhost_net for the next series, and I'll try to make all
of this more explicit.

Thanks!