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

[PATCH v2 0/4] 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.

The vdpa device must offer VIRTIO_NET_F_STATUS for the guest to access the
status of virtio net config where announcement status bit is set. It is
possible to emulate it as always active in case backend does not support it,
but this is left for the future, as there are not many devices not offering it
anyway.

Patch 1 is less useful now that we don't emulate _F_STATUS anymore but QEMU
coding style seems to prefer early return so leaving it in this version.

v2:
* Actually remove VIRTIO_NET_F_STATUS emulation. Comparing with v1, we offer
  the feature with virtio instead of using virtio/vhost-vdpa.

v1:
* Move code from vhost_net_get_config to virtio_net_get_config.

RFC v2:
* Add VIRTIO_NET_F_STATUS emulation.

Eugenio Pérez (4):
  virtio_net: Modify virtio_net_get_config to early return
  virtio_net: copy VIRTIO_NET_S_ANNOUNCE if device model has it
  vdpa: handle VIRTIO_NET_CTRL_ANNOUNCE in
    vhost_vdpa_net_handle_ctrl_avail
  vdpa: do not handle VIRTIO_NET_F_GUEST_ANNOUNCE in vhost-vdpa

 hw/net/virtio-net.c | 29 ++++++++++++++++-------------
 net/vhost-vdpa.c    | 16 ++++++++++++----
 2 files changed, 28 insertions(+), 17 deletions(-)

-- 
2.31.1

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

[Eugenio Pérez]

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

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

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

[PATCH v2 2/4] virtio_net: copy VIRTIO_NET_S_ANNOUNCE if device model has it

[Eugenio Pérez]

Status part of the emulated feature. It will follow device model, so we
must copy it as long as NIC device model has it set.

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

diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index eed629766f..bf71ef33e8 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -183,6 +183,7 @@ static void virtio_net_get_config(VirtIODevice *vdev, uint8_t *config)
             memcpy(netcfg.mac, n->mac, ETH_ALEN);
         }
 
+        netcfg.status |= (n->status & VIRTIO_NET_S_ANNOUNCE);
         memcpy(config, &netcfg, n->config_size);
     }
 }
-- 
2.31.1

[PATCH v2 3/4] 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 2b4b85d8f8..8172aa8449 100644
--- a/net/vhost-vdpa.c
+++ b/net/vhost-vdpa.c
@@ -489,9 +489,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

[PATCH v2 4/4] vdpa: do not handle VIRTIO_NET_F_GUEST_ANNOUNCE in vhost-vdpa

[Eugenio Pérez]

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

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 8172aa8449..79f022c2bf 100644
--- a/net/vhost-vdpa.c
+++ b/net/vhost-vdpa.c
@@ -72,7 +72,6 @@ const int vdpa_feature_bits[] = {
     VIRTIO_F_RING_RESET,
     VIRTIO_NET_F_RSS,
     VIRTIO_NET_F_HASH_REPORT,
-    VIRTIO_NET_F_GUEST_ANNOUNCE,
     VIRTIO_NET_F_STATUS,
     VHOST_INVALID_FEATURE_BIT
 };
-- 
2.31.1

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

[Jason Wang]

On Fri, Nov 25, 2022 at 1:33 AM Eugenio Pérez <eperezma@redhat.com> 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>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>

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

Thanks

> ---
>  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 aba12759d5..eed629766f 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -168,20 +168,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 v2 2/4] virtio_net: copy VIRTIO_NET_S_ANNOUNCE if device model has it

[Jason Wang]

On Fri, Nov 25, 2022 at 1:33 AM Eugenio Pérez <eperezma@redhat.com> wrote:
>
> Status part of the emulated feature. It will follow device model, so we
> must copy it as long as NIC device model has it set.
>
> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> ---
>  hw/net/virtio-net.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index eed629766f..bf71ef33e8 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -183,6 +183,7 @@ static void virtio_net_get_config(VirtIODevice *vdev, uint8_t *config)
>              memcpy(netcfg.mac, n->mac, ETH_ALEN);
>          }
>
> +        netcfg.status |= (n->status & VIRTIO_NET_S_ANNOUNCE);

Do we need to care about the endian here? We use:

    virtio_stw_p(vdev, &netcfg.status, n->status);

At the beginning of this function.

Thanks

>          memcpy(config, &netcfg, n->config_size);
>      }
>  }
> --
> 2.31.1
>

Re: [PATCH v2 3/4] vdpa: handle VIRTIO_NET_CTRL_ANNOUNCE in vhost_vdpa_net_handle_ctrl_avail

[Jason Wang]

On Fri, Nov 25, 2022 at 1:33 AM Eugenio Pérez <eperezma@redhat.com> wrote:
>
> 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 2b4b85d8f8..8172aa8449 100644
> --- a/net/vhost-vdpa.c
> +++ b/net/vhost-vdpa.c
> @@ -489,9 +489,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

s/dont/don't/

> +         * the device.
> +         */
> +        dev_written = sizeof(status);
> +        *s->status = VIRTIO_NET_OK;

I wonder if we should avoid negotiating ANNOUNCE with vDPA parents if
we do this?

Thanks

> +    } 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: [PATCH v2 4/4] vdpa: do not handle VIRTIO_NET_F_GUEST_ANNOUNCE in vhost-vdpa

[Jason Wang]

On Fri, Nov 25, 2022 at 1:33 AM Eugenio Pérez <eperezma@redhat.com> wrote:
>
> So qemu emulates it even in case the device does not support it.
>
> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>

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

Thanks

> ---
>  net/vhost-vdpa.c | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
> index 8172aa8449..79f022c2bf 100644
> --- a/net/vhost-vdpa.c
> +++ b/net/vhost-vdpa.c
> @@ -72,7 +72,6 @@ const int vdpa_feature_bits[] = {
>      VIRTIO_F_RING_RESET,
>      VIRTIO_NET_F_RSS,
>      VIRTIO_NET_F_HASH_REPORT,
> -    VIRTIO_NET_F_GUEST_ANNOUNCE,
>      VIRTIO_NET_F_STATUS,
>      VHOST_INVALID_FEATURE_BIT
>  };
> --
> 2.31.1
>

Re: [PATCH v2 3/4] vdpa: handle VIRTIO_NET_CTRL_ANNOUNCE in vhost_vdpa_net_handle_ctrl_avail

[Eugenio Perez Martin]

On Wed, Nov 30, 2022 at 8:02 AM Jason Wang <jasowang@redhat.com> wrote:
>
> On Fri, Nov 25, 2022 at 1:33 AM Eugenio Pérez <eperezma@redhat.com> wrote:
> >
> > 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 2b4b85d8f8..8172aa8449 100644
> > --- a/net/vhost-vdpa.c
> > +++ b/net/vhost-vdpa.c
> > @@ -489,9 +489,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
>
> s/dont/don't/
>

I'll correct it, thanks!

> > +         * the device.
> > +         */
> > +        dev_written = sizeof(status);
> > +        *s->status = VIRTIO_NET_OK;
>
> I wonder if we should avoid negotiating ANNOUNCE with vDPA parents if
> we do this?
>

I can re-check, but the next patch should avoid it. Even if
negotiated, the parent should never set the announce status bit, since
we never tell the device is a destination device.

But it's better to be on the safe side, I'll recheck.

Thanks!

> Thanks
>
> > +    } 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: [PATCH v2 3/4] vdpa: handle VIRTIO_NET_CTRL_ANNOUNCE in vhost_vdpa_net_handle_ctrl_avail

[Michael S. Tsirkin]

On Thu, Nov 24, 2022 at 06:33:13PM +0100, Eugenio Pérez wrote:
> 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 2b4b85d8f8..8172aa8449 100644
> --- a/net/vhost-vdpa.c
> +++ b/net/vhost-vdpa.c
> @@ -489,9 +489,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.
> +         */

Hmm I'm not sure why. We don't forward the status bit to guest?

> +        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: [PATCH v2 3/4] vdpa: handle VIRTIO_NET_CTRL_ANNOUNCE in vhost_vdpa_net_handle_ctrl_avail

[Eugenio Perez Martin]

On Wed, Nov 30, 2022 at 8:10 AM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Thu, Nov 24, 2022 at 06:33:13PM +0100, Eugenio Pérez wrote:
> > 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 2b4b85d8f8..8172aa8449 100644
> > --- a/net/vhost-vdpa.c
> > +++ b/net/vhost-vdpa.c
> > @@ -489,9 +489,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.
> > +         */
>
> Hmm I'm not sure why. We don't forward the status bit to guest?
>

No, the idea is to make this feature entirely emulated by qemu so it
does not depend on device's features to support it.

Thanks!

> > +        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: [PATCH v2 3/4] vdpa: handle VIRTIO_NET_CTRL_ANNOUNCE in vhost_vdpa_net_handle_ctrl_avail

[Jason Wang]

On Wed, Nov 30, 2022 at 3:07 PM Eugenio Perez Martin
<eperezma@redhat.com> wrote:
>
> On Wed, Nov 30, 2022 at 8:02 AM Jason Wang <jasowang@redhat.com> wrote:
> >
> > On Fri, Nov 25, 2022 at 1:33 AM Eugenio Pérez <eperezma@redhat.com> wrote:
> > >
> > > 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 2b4b85d8f8..8172aa8449 100644
> > > --- a/net/vhost-vdpa.c
> > > +++ b/net/vhost-vdpa.c
> > > @@ -489,9 +489,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
> >
> > s/dont/don't/
> >
>
> I'll correct it, thanks!
>
> > > +         * the device.
> > > +         */
> > > +        dev_written = sizeof(status);
> > > +        *s->status = VIRTIO_NET_OK;
> >
> > I wonder if we should avoid negotiating ANNOUNCE with vDPA parents if
> > we do this?
> >
>
> I can re-check, but the next patch should avoid it.

Kind of, it makes sure guest can always see _F_ANNOUNCE. But does it
prevent _F_ANNOUNCE from being negotiated?

> Even if
> negotiated, the parent should never set the announce status bit, since
> we never tell the device is a destination device.

That's the point, do we have such a guarantee? Or I wonder if there's
any parent that supports _F_ANNOUNCE if yes, how it is supposed to
work?

>
> But it's better to be on the safe side, I'll recheck.

Exactly.

Thanks

>
> Thanks!
>
> > Thanks
> >
> > > +    } 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: [PATCH v2 3/4] vdpa: handle VIRTIO_NET_CTRL_ANNOUNCE in vhost_vdpa_net_handle_ctrl_avail

[Eugenio Perez Martin]

On Thu, Dec 1, 2022 at 9:39 AM Jason Wang <jasowang@redhat.com> wrote:
>
> On Wed, Nov 30, 2022 at 3:07 PM Eugenio Perez Martin
> <eperezma@redhat.com> wrote:
> >
> > On Wed, Nov 30, 2022 at 8:02 AM Jason Wang <jasowang@redhat.com> wrote:
> > >
> > > On Fri, Nov 25, 2022 at 1:33 AM Eugenio Pérez <eperezma@redhat.com> wrote:
> > > >
> > > > 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 2b4b85d8f8..8172aa8449 100644
> > > > --- a/net/vhost-vdpa.c
> > > > +++ b/net/vhost-vdpa.c
> > > > @@ -489,9 +489,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
> > >
> > > s/dont/don't/
> > >
> >
> > I'll correct it, thanks!
> >
> > > > +         * the device.
> > > > +         */
> > > > +        dev_written = sizeof(status);
> > > > +        *s->status = VIRTIO_NET_OK;
> > >
> > > I wonder if we should avoid negotiating ANNOUNCE with vDPA parents if
> > > we do this?
> > >
> >
> > I can re-check, but the next patch should avoid it.
>
> Kind of, it makes sure guest can always see _F_ANNOUNCE. But does it
> prevent _F_ANNOUNCE from being negotiated?
>

It should go like:
* vhost_net_ack_features calls vhost_ack_features with feature_bits =
vdpa_feature_bits and features = guest acked features.
vhost_ack_features stores in hdev->acked_features only the features
that met features & bit_mask, so it will not store _F_ANNOUNCE.
* vhost_vdpa_set_features is called from vhost_dev_set_features with
features = dev->acked_features. Both functions can add features by
themselves (VHOST_F_LOG_ALL, VIRTIO_F_IOMMU_PLATFORM), but no
_F_ANNOUNCE.

Still untested.

> > Even if
> > negotiated, the parent should never set the announce status bit, since
> > we never tell the device is a destination device.
>
> That's the point, do we have such a guarantee? Or I wonder if there's
> any parent that supports _F_ANNOUNCE if yes, how it is supposed to
> work?
>

At the moment it is impossible to work since there is no support for
config interrupt from the device. Even with config interrupt,
something external from qemu should make the device enable the status
bit, since the current migration protocol makes no difference between
to be a migration destination and to start the device from scratch.
Unless it enables the bit maliciously or by mistake.

Just for completion, the current method works with no need of vdpa
device config interrupt support thanks to being 100% emulated in qemu,
which has the support of injecting config interrupts.

Thanks!

Re: [PATCH v2 3/4] vdpa: handle VIRTIO_NET_CTRL_ANNOUNCE in vhost_vdpa_net_handle_ctrl_avail

[Jason Wang]

On Thu, Dec 1, 2022 at 5:29 PM Eugenio Perez Martin <eperezma@redhat.com> wrote:
>
> On Thu, Dec 1, 2022 at 9:39 AM Jason Wang <jasowang@redhat.com> wrote:
> >
> > On Wed, Nov 30, 2022 at 3:07 PM Eugenio Perez Martin
> > <eperezma@redhat.com> wrote:
> > >
> > > On Wed, Nov 30, 2022 at 8:02 AM Jason Wang <jasowang@redhat.com> wrote:
> > > >
> > > > On Fri, Nov 25, 2022 at 1:33 AM Eugenio Pérez <eperezma@redhat.com> wrote:
> > > > >
> > > > > 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 2b4b85d8f8..8172aa8449 100644
> > > > > --- a/net/vhost-vdpa.c
> > > > > +++ b/net/vhost-vdpa.c
> > > > > @@ -489,9 +489,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
> > > >
> > > > s/dont/don't/
> > > >
> > >
> > > I'll correct it, thanks!
> > >
> > > > > +         * the device.
> > > > > +         */
> > > > > +        dev_written = sizeof(status);
> > > > > +        *s->status = VIRTIO_NET_OK;
> > > >
> > > > I wonder if we should avoid negotiating ANNOUNCE with vDPA parents if
> > > > we do this?
> > > >
> > >
> > > I can re-check, but the next patch should avoid it.
> >
> > Kind of, it makes sure guest can always see _F_ANNOUNCE. But does it
> > prevent _F_ANNOUNCE from being negotiated?
> >
>
> It should go like:
> * vhost_net_ack_features calls vhost_ack_features with feature_bits =
> vdpa_feature_bits and features = guest acked features.
> vhost_ack_features stores in hdev->acked_features only the features
> that met features & bit_mask, so it will not store _F_ANNOUNCE.
> * vhost_vdpa_set_features is called from vhost_dev_set_features with
> features = dev->acked_features. Both functions can add features by
> themselves (VHOST_F_LOG_ALL, VIRTIO_F_IOMMU_PLATFORM), but no
> _F_ANNOUNCE.
>
> Still untested.

Ok.

>
> > > Even if
> > > negotiated, the parent should never set the announce status bit, since
> > > we never tell the device is a destination device.
> >
> > That's the point, do we have such a guarantee? Or I wonder if there's
> > any parent that supports _F_ANNOUNCE if yes, how it is supposed to
> > work?
> >
>
> At the moment it is impossible to work since there is no support for
> config interrupt from the device. Even with config interrupt,
> something external from qemu should make the device enable the status
> bit, since the current migration protocol makes no difference between
> to be a migration destination and to start the device from scratch.
> Unless it enables the bit maliciously or by mistake.
>
> Just for completion, the current method works with no need of vdpa
> device config interrupt support thanks to being 100% emulated in qemu,
> which has the support of injecting config interrupts.

Ok, rethink this feature, I think I can find one use case for
_F_ANNOUNCE, that is, the migration is totally done through the vDPA
device (DPU) itself.

I think we can go forward and revisit this issue in the future.

Thanks

>
> Thanks!
>

Re: [PATCH v2 3/4] vdpa: handle VIRTIO_NET_CTRL_ANNOUNCE in vhost_vdpa_net_handle_ctrl_avail

[Eugenio Perez Martin]

On Mon, Dec 5, 2022 at 5:27 AM Jason Wang <jasowang@redhat.com> wrote:
>
> On Thu, Dec 1, 2022 at 5:29 PM Eugenio Perez Martin <eperezma@redhat.com> wrote:
> >
> > On Thu, Dec 1, 2022 at 9:39 AM Jason Wang <jasowang@redhat.com> wrote:
> > >
> > > On Wed, Nov 30, 2022 at 3:07 PM Eugenio Perez Martin
> > > <eperezma@redhat.com> wrote:
> > > >
> > > > On Wed, Nov 30, 2022 at 8:02 AM Jason Wang <jasowang@redhat.com> wrote:
> > > > >
> > > > > On Fri, Nov 25, 2022 at 1:33 AM Eugenio Pérez <eperezma@redhat.com> wrote:
> > > > > >
> > > > > > 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 2b4b85d8f8..8172aa8449 100644
> > > > > > --- a/net/vhost-vdpa.c
> > > > > > +++ b/net/vhost-vdpa.c
> > > > > > @@ -489,9 +489,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
> > > > >
> > > > > s/dont/don't/
> > > > >
> > > >
> > > > I'll correct it, thanks!
> > > >
> > > > > > +         * the device.
> > > > > > +         */
> > > > > > +        dev_written = sizeof(status);
> > > > > > +        *s->status = VIRTIO_NET_OK;
> > > > >
> > > > > I wonder if we should avoid negotiating ANNOUNCE with vDPA parents if
> > > > > we do this?
> > > > >
> > > >
> > > > I can re-check, but the next patch should avoid it.
> > >
> > > Kind of, it makes sure guest can always see _F_ANNOUNCE. But does it
> > > prevent _F_ANNOUNCE from being negotiated?
> > >
> >
> > It should go like:
> > * vhost_net_ack_features calls vhost_ack_features with feature_bits =
> > vdpa_feature_bits and features = guest acked features.
> > vhost_ack_features stores in hdev->acked_features only the features
> > that met features & bit_mask, so it will not store _F_ANNOUNCE.
> > * vhost_vdpa_set_features is called from vhost_dev_set_features with
> > features = dev->acked_features. Both functions can add features by
> > themselves (VHOST_F_LOG_ALL, VIRTIO_F_IOMMU_PLATFORM), but no
> > _F_ANNOUNCE.
> >
> > Still untested.
>
> Ok.
>
> >
> > > > Even if
> > > > negotiated, the parent should never set the announce status bit, since
> > > > we never tell the device is a destination device.
> > >
> > > That's the point, do we have such a guarantee? Or I wonder if there's
> > > any parent that supports _F_ANNOUNCE if yes, how it is supposed to
> > > work?
> > >
> >
> > At the moment it is impossible to work since there is no support for
> > config interrupt from the device. Even with config interrupt,
> > something external from qemu should make the device enable the status
> > bit, since the current migration protocol makes no difference between
> > to be a migration destination and to start the device from scratch.
> > Unless it enables the bit maliciously or by mistake.
> >
> > Just for completion, the current method works with no need of vdpa
> > device config interrupt support thanks to being 100% emulated in qemu,
> > which has the support of injecting config interrupts.
>
> Ok, rethink this feature, I think I can find one use case for
> _F_ANNOUNCE, that is, the migration is totally done through the vDPA
> device (DPU) itself.
>

To make sure we are on the same page, this migration would save some
things like transfer the status through qemu, but it is not possible
at the moment. A few things need to be developed for that to make it
possible.

The default behavior is to emulate the announce feature / status bit
at the moment, so no ack to the device is needed. If we want that
passthrough, a new parameter or similar needs to be developed, so the
feature is negotiated with the device and not emulated in get_config.

Is that accurate?

Thanks!

> I think we can go forward and revisit this issue in the future.
>
> Thanks
>
> >
> > Thanks!
> >
>

Re: [PATCH v2 3/4] vdpa: handle VIRTIO_NET_CTRL_ANNOUNCE in vhost_vdpa_net_handle_ctrl_avail

[Jason Wang]

On Mon, Dec 5, 2022 at 9:07 PM Eugenio Perez Martin <eperezma@redhat.com> wrote:
>
> On Mon, Dec 5, 2022 at 5:27 AM Jason Wang <jasowang@redhat.com> wrote:
> >
> > On Thu, Dec 1, 2022 at 5:29 PM Eugenio Perez Martin <eperezma@redhat.com> wrote:
> > >
> > > On Thu, Dec 1, 2022 at 9:39 AM Jason Wang <jasowang@redhat.com> wrote:
> > > >
> > > > On Wed, Nov 30, 2022 at 3:07 PM Eugenio Perez Martin
> > > > <eperezma@redhat.com> wrote:
> > > > >
> > > > > On Wed, Nov 30, 2022 at 8:02 AM Jason Wang <jasowang@redhat.com> wrote:
> > > > > >
> > > > > > On Fri, Nov 25, 2022 at 1:33 AM Eugenio Pérez <eperezma@redhat.com> wrote:
> > > > > > >
> > > > > > > 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 2b4b85d8f8..8172aa8449 100644
> > > > > > > --- a/net/vhost-vdpa.c
> > > > > > > +++ b/net/vhost-vdpa.c
> > > > > > > @@ -489,9 +489,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
> > > > > >
> > > > > > s/dont/don't/
> > > > > >
> > > > >
> > > > > I'll correct it, thanks!
> > > > >
> > > > > > > +         * the device.
> > > > > > > +         */
> > > > > > > +        dev_written = sizeof(status);
> > > > > > > +        *s->status = VIRTIO_NET_OK;
> > > > > >
> > > > > > I wonder if we should avoid negotiating ANNOUNCE with vDPA parents if
> > > > > > we do this?
> > > > > >
> > > > >
> > > > > I can re-check, but the next patch should avoid it.
> > > >
> > > > Kind of, it makes sure guest can always see _F_ANNOUNCE. But does it
> > > > prevent _F_ANNOUNCE from being negotiated?
> > > >
> > >
> > > It should go like:
> > > * vhost_net_ack_features calls vhost_ack_features with feature_bits =
> > > vdpa_feature_bits and features = guest acked features.
> > > vhost_ack_features stores in hdev->acked_features only the features
> > > that met features & bit_mask, so it will not store _F_ANNOUNCE.
> > > * vhost_vdpa_set_features is called from vhost_dev_set_features with
> > > features = dev->acked_features. Both functions can add features by
> > > themselves (VHOST_F_LOG_ALL, VIRTIO_F_IOMMU_PLATFORM), but no
> > > _F_ANNOUNCE.
> > >
> > > Still untested.
> >
> > Ok.
> >
> > >
> > > > > Even if
> > > > > negotiated, the parent should never set the announce status bit, since
> > > > > we never tell the device is a destination device.
> > > >
> > > > That's the point, do we have such a guarantee? Or I wonder if there's
> > > > any parent that supports _F_ANNOUNCE if yes, how it is supposed to
> > > > work?
> > > >
> > >
> > > At the moment it is impossible to work since there is no support for
> > > config interrupt from the device. Even with config interrupt,
> > > something external from qemu should make the device enable the status
> > > bit, since the current migration protocol makes no difference between
> > > to be a migration destination and to start the device from scratch.
> > > Unless it enables the bit maliciously or by mistake.
> > >
> > > Just for completion, the current method works with no need of vdpa
> > > device config interrupt support thanks to being 100% emulated in qemu,
> > > which has the support of injecting config interrupts.
> >
> > Ok, rethink this feature, I think I can find one use case for
> > _F_ANNOUNCE, that is, the migration is totally done through the vDPA
> > device (DPU) itself.
> >
>
> To make sure we are on the same page, this migration would save some
> things like transfer the status through qemu, but it is not possible
> at the moment. A few things need to be developed for that to make it
> possible.

Somehow, it means the DPU is in charge of doing all the migration.

>
> The default behavior is to emulate the announce feature / status bit
> at the moment, so no ack to the device is needed. If we want that
> passthrough, a new parameter or similar needs to be developed, so the
> feature is negotiated with the device and not emulated in get_config.
>
> Is that accurate?

Yes.

Thanks

>
> Thanks!
>
> > I think we can go forward and revisit this issue in the future.
> >
> > Thanks
> >
> > >
> > > Thanks!
> > >
> >
>

Re: [PATCH v2 2/4] virtio_net: copy VIRTIO_NET_S_ANNOUNCE if device model has it

[Michael S. Tsirkin]

On Wed, Nov 30, 2022 at 02:58:35PM +0800, Jason Wang wrote:
> On Fri, Nov 25, 2022 at 1:33 AM Eugenio Pérez <eperezma@redhat.com> wrote:
> >
> > Status part of the emulated feature. It will follow device model, so we
> > must copy it as long as NIC device model has it set.
> >
> > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > ---
> >  hw/net/virtio-net.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> > index eed629766f..bf71ef33e8 100644
> > --- a/hw/net/virtio-net.c
> > +++ b/hw/net/virtio-net.c
> > @@ -183,6 +183,7 @@ static void virtio_net_get_config(VirtIODevice *vdev, uint8_t *config)
> >              memcpy(netcfg.mac, n->mac, ETH_ALEN);
> >          }
> >
> > +        netcfg.status |= (n->status & VIRTIO_NET_S_ANNOUNCE);
> 
> Do we need to care about the endian here? We use:
> 
>     virtio_stw_p(vdev, &netcfg.status, n->status);
> 
> At the beginning of this function.
> 
> Thanks


Didn't see this answered.

> >          memcpy(config, &netcfg, n->config_size);
> >      }
> >  }
> > --
> > 2.31.1
> >

Re: [PATCH v2 2/4] virtio_net: copy VIRTIO_NET_S_ANNOUNCE if device model has it

[Eugenio Perez Martin]

On Wed, Nov 30, 2022 at 7:58 AM Jason Wang <jasowang@redhat.com> wrote:
>
> On Fri, Nov 25, 2022 at 1:33 AM Eugenio Pérez <eperezma@redhat.com> wrote:
> >
> > Status part of the emulated feature. It will follow device model, so we
> > must copy it as long as NIC device model has it set.
> >
> > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > ---
> >  hw/net/virtio-net.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> > index eed629766f..bf71ef33e8 100644
> > --- a/hw/net/virtio-net.c
> > +++ b/hw/net/virtio-net.c
> > @@ -183,6 +183,7 @@ static void virtio_net_get_config(VirtIODevice *vdev, uint8_t *config)
> >              memcpy(netcfg.mac, n->mac, ETH_ALEN);
> >          }
> >
> > +        netcfg.status |= (n->status & VIRTIO_NET_S_ANNOUNCE);
>
> Do we need to care about the endian here? We use:
>
>     virtio_stw_p(vdev, &netcfg.status, n->status);
>
> At the beginning of this function.
>

Right, this is a miss. I'll fix it in the next version.

Thanks!