[v2] Guest announce feature emulation using Shadow VirtQueue

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

Posted by Eugenio Pérez 3 years, 3 months ago

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

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

Posted by Jason Wang 3 years, 3 months ago

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 6/8] vdpa: handle VIRTIO_NET_CTRL_ANNOUNCE in vhost_vdpa_net_handle_ctrl_avail

Posted by Eugenio Perez Martin 3 years, 3 months ago

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 6/8] vdpa: handle VIRTIO_NET_CTRL_ANNOUNCE in vhost_vdpa_net_handle_ctrl_avail

Posted by Jason Wang 3 years, 3 months ago

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 6/8] vdpa: handle VIRTIO_NET_CTRL_ANNOUNCE in vhost_vdpa_net_handle_ctrl_avail

Posted by Eugenio Perez Martin 3 years, 3 months ago

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 6/8] vdpa: handle VIRTIO_NET_CTRL_ANNOUNCE in vhost_vdpa_net_handle_ctrl_avail

Posted by Jason Wang 3 years, 3 months ago

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