[PATCH 0/4] Shadow VirtQueue event index support

[PATCH 0/4] Shadow VirtQueue event index support

[Eugenio Pérez]

Event idx helps to reduce the number of notifications between the device
and the driver. It allows them to specify an index on the circular
descriptors rings where to issue the notification, instead of a single
binary indicator.

Adding support for SVQ.

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

[1] https://lists.nongnu.org/archive/html/qemu-devel/2022-10/msg03280.html

Eugenio Pérez (4):
  vhost: allocate event_idx fields on vring
  vhost: toggle device callbacks using used event idx
  vhost: use avail event idx on vhost_svq_kick
  vhost: Accept event idx flag

 hw/virtio/vhost-shadow-virtqueue.c | 39 ++++++++++++++++++++++++------
 1 file changed, 31 insertions(+), 8 deletions(-)

-- 
2.31.1

[PATCH 1/4] vhost: allocate event_idx fields on vring

[Eugenio Pérez]

There was not enough room to accomodate them.

Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
---
 hw/virtio/vhost-shadow-virtqueue.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/hw/virtio/vhost-shadow-virtqueue.c b/hw/virtio/vhost-shadow-virtqueue.c
index 596d4434d2..a518f84772 100644
--- a/hw/virtio/vhost-shadow-virtqueue.c
+++ b/hw/virtio/vhost-shadow-virtqueue.c
@@ -570,16 +570,16 @@ void vhost_svq_get_vring_addr(const VhostShadowVirtqueue *svq,
 size_t vhost_svq_driver_area_size(const VhostShadowVirtqueue *svq)
 {
     size_t desc_size = sizeof(vring_desc_t) * svq->vring.num;
-    size_t avail_size = offsetof(vring_avail_t, ring) +
-                                             sizeof(uint16_t) * svq->vring.num;
+    size_t avail_size = offsetof(vring_avail_t, ring[svq->vring.num]) +
+                                                              sizeof(uint16_t);
 
     return ROUND_UP(desc_size + avail_size, qemu_real_host_page_size());
 }
 
 size_t vhost_svq_device_area_size(const VhostShadowVirtqueue *svq)
 {
-    size_t used_size = offsetof(vring_used_t, ring) +
-                                    sizeof(vring_used_elem_t) * svq->vring.num;
+    size_t used_size = offsetof(vring_used_t, ring[svq->vring.num]) +
+                                                              sizeof(uint16_t);
     return ROUND_UP(used_size, qemu_real_host_page_size());
 }
 
-- 
2.31.1

[PATCH 2/4] vhost: toggle device callbacks using used event idx

[Eugenio Pérez]

Actually use the new field of the used ring and tell the device if SVQ
wants to be notified.

The code is not reachable at the moment.

Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
---
 hw/virtio/vhost-shadow-virtqueue.c | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/hw/virtio/vhost-shadow-virtqueue.c b/hw/virtio/vhost-shadow-virtqueue.c
index a518f84772..f5c0fad3fc 100644
--- a/hw/virtio/vhost-shadow-virtqueue.c
+++ b/hw/virtio/vhost-shadow-virtqueue.c
@@ -369,15 +369,27 @@ static bool vhost_svq_more_used(VhostShadowVirtqueue *svq)
  */
 static bool vhost_svq_enable_notification(VhostShadowVirtqueue *svq)
 {
-    svq->vring.avail->flags &= ~cpu_to_le16(VRING_AVAIL_F_NO_INTERRUPT);
-    /* Make sure the flag is written before the read of used_idx */
+    if (virtio_vdev_has_feature(svq->vdev, VIRTIO_RING_F_EVENT_IDX)) {
+        uint16_t *used_event = (uint16_t *)&svq->vring.avail->ring[svq->vring.num];
+        *used_event = svq->shadow_used_idx;
+    } else {
+        svq->vring.avail->flags &= ~cpu_to_le16(VRING_AVAIL_F_NO_INTERRUPT);
+    }
+
+    /* Make sure the event is enabled before the read of used_idx */
     smp_mb();
     return !vhost_svq_more_used(svq);
 }
 
 static void vhost_svq_disable_notification(VhostShadowVirtqueue *svq)
 {
-    svq->vring.avail->flags |= cpu_to_le16(VRING_AVAIL_F_NO_INTERRUPT);
+    /*
+     * No need to disable notification in the event idx case, since used event
+     * index is already an index too far away.
+     */
+    if (!virtio_vdev_has_feature(svq->vdev, VIRTIO_RING_F_EVENT_IDX)) {
+        svq->vring.avail->flags |= cpu_to_le16(VRING_AVAIL_F_NO_INTERRUPT);
+    }
 }
 
 static uint16_t vhost_svq_last_desc_of_chain(const VhostShadowVirtqueue *svq,
-- 
2.31.1

Re: [PATCH 2/4] vhost: toggle device callbacks using used event idx

[Jason Wang]

On Thu, Oct 20, 2022 at 11:53 PM Eugenio Pérez <eperezma@redhat.com> wrote:
>
> Actually use the new field of the used ring and tell the device if SVQ
> wants to be notified.
>
> The code is not reachable at the moment.
>
> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> ---
>  hw/virtio/vhost-shadow-virtqueue.c | 18 +++++++++++++++---
>  1 file changed, 15 insertions(+), 3 deletions(-)
>
> diff --git a/hw/virtio/vhost-shadow-virtqueue.c b/hw/virtio/vhost-shadow-virtqueue.c
> index a518f84772..f5c0fad3fc 100644
> --- a/hw/virtio/vhost-shadow-virtqueue.c
> +++ b/hw/virtio/vhost-shadow-virtqueue.c
> @@ -369,15 +369,27 @@ static bool vhost_svq_more_used(VhostShadowVirtqueue *svq)
>   */
>  static bool vhost_svq_enable_notification(VhostShadowVirtqueue *svq)
>  {
> -    svq->vring.avail->flags &= ~cpu_to_le16(VRING_AVAIL_F_NO_INTERRUPT);
> -    /* Make sure the flag is written before the read of used_idx */
> +    if (virtio_vdev_has_feature(svq->vdev, VIRTIO_RING_F_EVENT_IDX)) {
> +        uint16_t *used_event = (uint16_t *)&svq->vring.avail->ring[svq->vring.num];
> +        *used_event = svq->shadow_used_idx;

Do we need to care about the endian here?

E.g vduse has:

    *((uint16_t *)&vq->vring.used->ring[vq->vring.num]) = htole16(val);

Thanks

> +    } else {
> +        svq->vring.avail->flags &= ~cpu_to_le16(VRING_AVAIL_F_NO_INTERRUPT);
> +    }
> +
> +    /* Make sure the event is enabled before the read of used_idx */
>      smp_mb();
>      return !vhost_svq_more_used(svq);
>  }
>
>  static void vhost_svq_disable_notification(VhostShadowVirtqueue *svq)
>  {
> -    svq->vring.avail->flags |= cpu_to_le16(VRING_AVAIL_F_NO_INTERRUPT);
> +    /*
> +     * No need to disable notification in the event idx case, since used event
> +     * index is already an index too far away.
> +     */
> +    if (!virtio_vdev_has_feature(svq->vdev, VIRTIO_RING_F_EVENT_IDX)) {
> +        svq->vring.avail->flags |= cpu_to_le16(VRING_AVAIL_F_NO_INTERRUPT);
> +    }
>  }
>
>  static uint16_t vhost_svq_last_desc_of_chain(const VhostShadowVirtqueue *svq,
> --
> 2.31.1
>

Re: [PATCH 2/4] vhost: toggle device callbacks using used event idx

[Eugenio Perez Martin]

On Fri, Oct 21, 2022 at 5:40 AM Jason Wang <jasowang@redhat.com> wrote:
>
> On Thu, Oct 20, 2022 at 11:53 PM Eugenio Pérez <eperezma@redhat.com> wrote:
> >
> > Actually use the new field of the used ring and tell the device if SVQ
> > wants to be notified.
> >
> > The code is not reachable at the moment.
> >
> > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > ---
> >  hw/virtio/vhost-shadow-virtqueue.c | 18 +++++++++++++++---
> >  1 file changed, 15 insertions(+), 3 deletions(-)
> >
> > diff --git a/hw/virtio/vhost-shadow-virtqueue.c b/hw/virtio/vhost-shadow-virtqueue.c
> > index a518f84772..f5c0fad3fc 100644
> > --- a/hw/virtio/vhost-shadow-virtqueue.c
> > +++ b/hw/virtio/vhost-shadow-virtqueue.c
> > @@ -369,15 +369,27 @@ static bool vhost_svq_more_used(VhostShadowVirtqueue *svq)
> >   */
> >  static bool vhost_svq_enable_notification(VhostShadowVirtqueue *svq)
> >  {
> > -    svq->vring.avail->flags &= ~cpu_to_le16(VRING_AVAIL_F_NO_INTERRUPT);
> > -    /* Make sure the flag is written before the read of used_idx */
> > +    if (virtio_vdev_has_feature(svq->vdev, VIRTIO_RING_F_EVENT_IDX)) {
> > +        uint16_t *used_event = (uint16_t *)&svq->vring.avail->ring[svq->vring.num];
> > +        *used_event = svq->shadow_used_idx;
>
> Do we need to care about the endian here?
>
> E.g vduse has:
>
>     *((uint16_t *)&vq->vring.used->ring[vq->vring.num]) = htole16(val);
>

Good catch, I forgot about endianness.

I'll review the series, thanks!

> Thanks
>
> > +    } else {
> > +        svq->vring.avail->flags &= ~cpu_to_le16(VRING_AVAIL_F_NO_INTERRUPT);
> > +    }
> > +
> > +    /* Make sure the event is enabled before the read of used_idx */
> >      smp_mb();
> >      return !vhost_svq_more_used(svq);
> >  }
> >
> >  static void vhost_svq_disable_notification(VhostShadowVirtqueue *svq)
> >  {
> > -    svq->vring.avail->flags |= cpu_to_le16(VRING_AVAIL_F_NO_INTERRUPT);
> > +    /*
> > +     * No need to disable notification in the event idx case, since used event
> > +     * index is already an index too far away.
> > +     */
> > +    if (!virtio_vdev_has_feature(svq->vdev, VIRTIO_RING_F_EVENT_IDX)) {
> > +        svq->vring.avail->flags |= cpu_to_le16(VRING_AVAIL_F_NO_INTERRUPT);
> > +    }
> >  }
> >
> >  static uint16_t vhost_svq_last_desc_of_chain(const VhostShadowVirtqueue *svq,
> > --
> > 2.31.1
> >
>

Re: [PATCH 2/4] vhost: toggle device callbacks using used event idx

[Michael S. Tsirkin]

On Fri, Oct 21, 2022 at 09:45:14AM +0200, Eugenio Perez Martin wrote:
> On Fri, Oct 21, 2022 at 5:40 AM Jason Wang <jasowang@redhat.com> wrote:
> >
> > On Thu, Oct 20, 2022 at 11:53 PM Eugenio Pérez <eperezma@redhat.com> wrote:
> > >
> > > Actually use the new field of the used ring and tell the device if SVQ
> > > wants to be notified.
> > >
> > > The code is not reachable at the moment.
> > >
> > > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > > ---
> > >  hw/virtio/vhost-shadow-virtqueue.c | 18 +++++++++++++++---
> > >  1 file changed, 15 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/hw/virtio/vhost-shadow-virtqueue.c b/hw/virtio/vhost-shadow-virtqueue.c
> > > index a518f84772..f5c0fad3fc 100644
> > > --- a/hw/virtio/vhost-shadow-virtqueue.c
> > > +++ b/hw/virtio/vhost-shadow-virtqueue.c
> > > @@ -369,15 +369,27 @@ static bool vhost_svq_more_used(VhostShadowVirtqueue *svq)
> > >   */
> > >  static bool vhost_svq_enable_notification(VhostShadowVirtqueue *svq)
> > >  {
> > > -    svq->vring.avail->flags &= ~cpu_to_le16(VRING_AVAIL_F_NO_INTERRUPT);
> > > -    /* Make sure the flag is written before the read of used_idx */
> > > +    if (virtio_vdev_has_feature(svq->vdev, VIRTIO_RING_F_EVENT_IDX)) {
> > > +        uint16_t *used_event = (uint16_t *)&svq->vring.avail->ring[svq->vring.num];
> > > +        *used_event = svq->shadow_used_idx;
> >
> > Do we need to care about the endian here?
> >
> > E.g vduse has:
> >
> >     *((uint16_t *)&vq->vring.used->ring[vq->vring.num]) = htole16(val);
> >
> 
> Good catch, I forgot about endianness.
> 
> I'll review the series, thanks!

It's generally a waste that we don't use endian-ness annotations
the way linux does.


> > Thanks
> >
> > > +    } else {
> > > +        svq->vring.avail->flags &= ~cpu_to_le16(VRING_AVAIL_F_NO_INTERRUPT);
> > > +    }
> > > +
> > > +    /* Make sure the event is enabled before the read of used_idx */
> > >      smp_mb();
> > >      return !vhost_svq_more_used(svq);
> > >  }
> > >
> > >  static void vhost_svq_disable_notification(VhostShadowVirtqueue *svq)
> > >  {
> > > -    svq->vring.avail->flags |= cpu_to_le16(VRING_AVAIL_F_NO_INTERRUPT);
> > > +    /*
> > > +     * No need to disable notification in the event idx case, since used event
> > > +     * index is already an index too far away.
> > > +     */
> > > +    if (!virtio_vdev_has_feature(svq->vdev, VIRTIO_RING_F_EVENT_IDX)) {
> > > +        svq->vring.avail->flags |= cpu_to_le16(VRING_AVAIL_F_NO_INTERRUPT);
> > > +    }
> > >  }
> > >
> > >  static uint16_t vhost_svq_last_desc_of_chain(const VhostShadowVirtqueue *svq,
> > > --
> > > 2.31.1
> > >
> >

Re: [PATCH 2/4] vhost: toggle device callbacks using used event idx

[Jason Wang]

On Fri, Oct 21, 2022 at 4:15 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Fri, Oct 21, 2022 at 09:45:14AM +0200, Eugenio Perez Martin wrote:
> > On Fri, Oct 21, 2022 at 5:40 AM Jason Wang <jasowang@redhat.com> wrote:
> > >
> > > On Thu, Oct 20, 2022 at 11:53 PM Eugenio Pérez <eperezma@redhat.com> wrote:
> > > >
> > > > Actually use the new field of the used ring and tell the device if SVQ
> > > > wants to be notified.
> > > >
> > > > The code is not reachable at the moment.
> > > >
> > > > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > > > ---
> > > >  hw/virtio/vhost-shadow-virtqueue.c | 18 +++++++++++++++---
> > > >  1 file changed, 15 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/hw/virtio/vhost-shadow-virtqueue.c b/hw/virtio/vhost-shadow-virtqueue.c
> > > > index a518f84772..f5c0fad3fc 100644
> > > > --- a/hw/virtio/vhost-shadow-virtqueue.c
> > > > +++ b/hw/virtio/vhost-shadow-virtqueue.c
> > > > @@ -369,15 +369,27 @@ static bool vhost_svq_more_used(VhostShadowVirtqueue *svq)
> > > >   */
> > > >  static bool vhost_svq_enable_notification(VhostShadowVirtqueue *svq)
> > > >  {
> > > > -    svq->vring.avail->flags &= ~cpu_to_le16(VRING_AVAIL_F_NO_INTERRUPT);
> > > > -    /* Make sure the flag is written before the read of used_idx */
> > > > +    if (virtio_vdev_has_feature(svq->vdev, VIRTIO_RING_F_EVENT_IDX)) {
> > > > +        uint16_t *used_event = (uint16_t *)&svq->vring.avail->ring[svq->vring.num];
> > > > +        *used_event = svq->shadow_used_idx;
> > >
> > > Do we need to care about the endian here?
> > >
> > > E.g vduse has:
> > >
> > >     *((uint16_t *)&vq->vring.used->ring[vq->vring.num]) = htole16(val);
> > >
> >
> > Good catch, I forgot about endianness.
> >
> > I'll review the series, thanks!
>
> It's generally a waste that we don't use endian-ness annotations
> the way linux does.

Yes, it's worth doing something similar sometime.

Thanks

>
>
> > > Thanks
> > >
> > > > +    } else {
> > > > +        svq->vring.avail->flags &= ~cpu_to_le16(VRING_AVAIL_F_NO_INTERRUPT);
> > > > +    }
> > > > +
> > > > +    /* Make sure the event is enabled before the read of used_idx */
> > > >      smp_mb();
> > > >      return !vhost_svq_more_used(svq);
> > > >  }
> > > >
> > > >  static void vhost_svq_disable_notification(VhostShadowVirtqueue *svq)
> > > >  {
> > > > -    svq->vring.avail->flags |= cpu_to_le16(VRING_AVAIL_F_NO_INTERRUPT);
> > > > +    /*
> > > > +     * No need to disable notification in the event idx case, since used event
> > > > +     * index is already an index too far away.
> > > > +     */
> > > > +    if (!virtio_vdev_has_feature(svq->vdev, VIRTIO_RING_F_EVENT_IDX)) {
> > > > +        svq->vring.avail->flags |= cpu_to_le16(VRING_AVAIL_F_NO_INTERRUPT);
> > > > +    }
> > > >  }
> > > >
> > > >  static uint16_t vhost_svq_last_desc_of_chain(const VhostShadowVirtqueue *svq,
> > > > --
> > > > 2.31.1
> > > >
> > >
>

Re: [PATCH 2/4] vhost: toggle device callbacks using used event idx

[Eugenio Perez Martin]

On Mon, Oct 24, 2022 at 4:16 AM Jason Wang <jasowang@redhat.com> wrote:
>
> On Fri, Oct 21, 2022 at 4:15 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Fri, Oct 21, 2022 at 09:45:14AM +0200, Eugenio Perez Martin wrote:
> > > On Fri, Oct 21, 2022 at 5:40 AM Jason Wang <jasowang@redhat.com> wrote:
> > > >
> > > > On Thu, Oct 20, 2022 at 11:53 PM Eugenio Pérez <eperezma@redhat.com> wrote:
> > > > >
> > > > > Actually use the new field of the used ring and tell the device if SVQ
> > > > > wants to be notified.
> > > > >
> > > > > The code is not reachable at the moment.
> > > > >
> > > > > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > > > > ---
> > > > >  hw/virtio/vhost-shadow-virtqueue.c | 18 +++++++++++++++---
> > > > >  1 file changed, 15 insertions(+), 3 deletions(-)
> > > > >
> > > > > diff --git a/hw/virtio/vhost-shadow-virtqueue.c b/hw/virtio/vhost-shadow-virtqueue.c
> > > > > index a518f84772..f5c0fad3fc 100644
> > > > > --- a/hw/virtio/vhost-shadow-virtqueue.c
> > > > > +++ b/hw/virtio/vhost-shadow-virtqueue.c
> > > > > @@ -369,15 +369,27 @@ static bool vhost_svq_more_used(VhostShadowVirtqueue *svq)
> > > > >   */
> > > > >  static bool vhost_svq_enable_notification(VhostShadowVirtqueue *svq)
> > > > >  {
> > > > > -    svq->vring.avail->flags &= ~cpu_to_le16(VRING_AVAIL_F_NO_INTERRUPT);
> > > > > -    /* Make sure the flag is written before the read of used_idx */
> > > > > +    if (virtio_vdev_has_feature(svq->vdev, VIRTIO_RING_F_EVENT_IDX)) {
> > > > > +        uint16_t *used_event = (uint16_t *)&svq->vring.avail->ring[svq->vring.num];
> > > > > +        *used_event = svq->shadow_used_idx;
> > > >
> > > > Do we need to care about the endian here?
> > > >
> > > > E.g vduse has:
> > > >
> > > >     *((uint16_t *)&vq->vring.used->ring[vq->vring.num]) = htole16(val);
> > > >
> > >
> > > Good catch, I forgot about endianness.
> > >
> > > I'll review the series, thanks!
> >
> > It's generally a waste that we don't use endian-ness annotations
> > the way linux does.
>
> Yes, it's worth doing something similar sometime.
>

Maybe we could wrap them in some struct like virtio_le16 or virtio_16,
avoiding at least integer direct assignment? Wrappers like
cpu_to_virtio16 could return these structs and I think all compilers
should emit the same code as direct assignment.

Thanks!




> Thanks
>
> >
> >
> > > > Thanks
> > > >
> > > > > +    } else {
> > > > > +        svq->vring.avail->flags &= ~cpu_to_le16(VRING_AVAIL_F_NO_INTERRUPT);
> > > > > +    }
> > > > > +
> > > > > +    /* Make sure the event is enabled before the read of used_idx */
> > > > >      smp_mb();
> > > > >      return !vhost_svq_more_used(svq);
> > > > >  }
> > > > >
> > > > >  static void vhost_svq_disable_notification(VhostShadowVirtqueue *svq)
> > > > >  {
> > > > > -    svq->vring.avail->flags |= cpu_to_le16(VRING_AVAIL_F_NO_INTERRUPT);
> > > > > +    /*
> > > > > +     * No need to disable notification in the event idx case, since used event
> > > > > +     * index is already an index too far away.
> > > > > +     */
> > > > > +    if (!virtio_vdev_has_feature(svq->vdev, VIRTIO_RING_F_EVENT_IDX)) {
> > > > > +        svq->vring.avail->flags |= cpu_to_le16(VRING_AVAIL_F_NO_INTERRUPT);
> > > > > +    }
> > > > >  }
> > > > >
> > > > >  static uint16_t vhost_svq_last_desc_of_chain(const VhostShadowVirtqueue *svq,
> > > > > --
> > > > > 2.31.1
> > > > >
> > > >
> >
>

Re: [PATCH 2/4] vhost: toggle device callbacks using used event idx

[Michael S. Tsirkin]

On Mon, Oct 24, 2022 at 04:00:37PM +0200, Eugenio Perez Martin wrote:
> > > It's generally a waste that we don't use endian-ness annotations
> > > the way linux does.
> >
> > Yes, it's worth doing something similar sometime.
> >
> 
> Maybe we could wrap them in some struct like virtio_le16 or virtio_16,
> avoiding at least integer direct assignment? Wrappers like
> cpu_to_virtio16 could return these structs and I think all compilers
> should emit the same code as direct assignment.
> 
> Thanks!
> 

This will break bitwise operations such as | and &.
Generally Linux has solved the problem and I don't think
we should go look for another solution.


> 
> 
> > Thanks
> >
> > >
> > >
> > > > > Thanks
> > > > >
> > > > > > +    } else {
> > > > > > +        svq->vring.avail->flags &= ~cpu_to_le16(VRING_AVAIL_F_NO_INTERRUPT);
> > > > > > +    }
> > > > > > +
> > > > > > +    /* Make sure the event is enabled before the read of used_idx */
> > > > > >      smp_mb();
> > > > > >      return !vhost_svq_more_used(svq);
> > > > > >  }
> > > > > >
> > > > > >  static void vhost_svq_disable_notification(VhostShadowVirtqueue *svq)
> > > > > >  {
> > > > > > -    svq->vring.avail->flags |= cpu_to_le16(VRING_AVAIL_F_NO_INTERRUPT);
> > > > > > +    /*
> > > > > > +     * No need to disable notification in the event idx case, since used event
> > > > > > +     * index is already an index too far away.
> > > > > > +     */
> > > > > > +    if (!virtio_vdev_has_feature(svq->vdev, VIRTIO_RING_F_EVENT_IDX)) {
> > > > > > +        svq->vring.avail->flags |= cpu_to_le16(VRING_AVAIL_F_NO_INTERRUPT);
> > > > > > +    }
> > > > > >  }
> > > > > >
> > > > > >  static uint16_t vhost_svq_last_desc_of_chain(const VhostShadowVirtqueue *svq,
> > > > > > --
> > > > > > 2.31.1
> > > > > >
> > > > >
> > >
> >

Re: [PATCH 2/4] vhost: toggle device callbacks using used event idx

[Jason Wang]

On Mon, Oct 24, 2022 at 10:05 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Mon, Oct 24, 2022 at 04:00:37PM +0200, Eugenio Perez Martin wrote:
> > > > It's generally a waste that we don't use endian-ness annotations
> > > > the way linux does.
> > >
> > > Yes, it's worth doing something similar sometime.
> > >
> >
> > Maybe we could wrap them in some struct like virtio_le16 or virtio_16,
> > avoiding at least integer direct assignment? Wrappers like
> > cpu_to_virtio16 could return these structs and I think all compilers
> > should emit the same code as direct assignment.
> >
> > Thanks!
> >
>
> This will break bitwise operations such as | and &.
> Generally Linux has solved the problem and I don't think
> we should go look for another solution.

Yes, but it should not block this series (we can do that in the future
if we had bandwidth).

Thanks

>
>
> >
> >
> > > Thanks
> > >
> > > >
> > > >
> > > > > > Thanks
> > > > > >
> > > > > > > +    } else {
> > > > > > > +        svq->vring.avail->flags &= ~cpu_to_le16(VRING_AVAIL_F_NO_INTERRUPT);
> > > > > > > +    }
> > > > > > > +
> > > > > > > +    /* Make sure the event is enabled before the read of used_idx */
> > > > > > >      smp_mb();
> > > > > > >      return !vhost_svq_more_used(svq);
> > > > > > >  }
> > > > > > >
> > > > > > >  static void vhost_svq_disable_notification(VhostShadowVirtqueue *svq)
> > > > > > >  {
> > > > > > > -    svq->vring.avail->flags |= cpu_to_le16(VRING_AVAIL_F_NO_INTERRUPT);
> > > > > > > +    /*
> > > > > > > +     * No need to disable notification in the event idx case, since used event
> > > > > > > +     * index is already an index too far away.
> > > > > > > +     */
> > > > > > > +    if (!virtio_vdev_has_feature(svq->vdev, VIRTIO_RING_F_EVENT_IDX)) {
> > > > > > > +        svq->vring.avail->flags |= cpu_to_le16(VRING_AVAIL_F_NO_INTERRUPT);
> > > > > > > +    }
> > > > > > >  }
> > > > > > >
> > > > > > >  static uint16_t vhost_svq_last_desc_of_chain(const VhostShadowVirtqueue *svq,
> > > > > > > --
> > > > > > > 2.31.1
> > > > > > >
> > > > > >
> > > >
> > >
>

Re: [PATCH 2/4] vhost: toggle device callbacks using used event idx

[Michael S. Tsirkin]

On Tue, Oct 25, 2022 at 10:26:35AM +0800, Jason Wang wrote:
> On Mon, Oct 24, 2022 at 10:05 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Mon, Oct 24, 2022 at 04:00:37PM +0200, Eugenio Perez Martin wrote:
> > > > > It's generally a waste that we don't use endian-ness annotations
> > > > > the way linux does.
> > > >
> > > > Yes, it's worth doing something similar sometime.
> > > >
> > >
> > > Maybe we could wrap them in some struct like virtio_le16 or virtio_16,
> > > avoiding at least integer direct assignment? Wrappers like
> > > cpu_to_virtio16 could return these structs and I think all compilers
> > > should emit the same code as direct assignment.
> > >
> > > Thanks!
> > >
> >
> > This will break bitwise operations such as | and &.
> > Generally Linux has solved the problem and I don't think
> > we should go look for another solution.
> 
> Yes, but it should not block this series (we can do that in the future
> if we had bandwidth).
> 
> Thanks

Sorry I don't get what you are saying. Which series?
Making LE tags structs is not going to fly, this is why sparse
implements the __bitwise__ tag and in fact this is where the name comes
from - bitwise operations need to work.



> >
> >
> > >
> > >
> > > > Thanks
> > > >
> > > > >
> > > > >
> > > > > > > Thanks
> > > > > > >
> > > > > > > > +    } else {
> > > > > > > > +        svq->vring.avail->flags &= ~cpu_to_le16(VRING_AVAIL_F_NO_INTERRUPT);
> > > > > > > > +    }
> > > > > > > > +
> > > > > > > > +    /* Make sure the event is enabled before the read of used_idx */
> > > > > > > >      smp_mb();
> > > > > > > >      return !vhost_svq_more_used(svq);
> > > > > > > >  }
> > > > > > > >
> > > > > > > >  static void vhost_svq_disable_notification(VhostShadowVirtqueue *svq)
> > > > > > > >  {
> > > > > > > > -    svq->vring.avail->flags |= cpu_to_le16(VRING_AVAIL_F_NO_INTERRUPT);
> > > > > > > > +    /*
> > > > > > > > +     * No need to disable notification in the event idx case, since used event
> > > > > > > > +     * index is already an index too far away.
> > > > > > > > +     */
> > > > > > > > +    if (!virtio_vdev_has_feature(svq->vdev, VIRTIO_RING_F_EVENT_IDX)) {
> > > > > > > > +        svq->vring.avail->flags |= cpu_to_le16(VRING_AVAIL_F_NO_INTERRUPT);
> > > > > > > > +    }
> > > > > > > >  }
> > > > > > > >
> > > > > > > >  static uint16_t vhost_svq_last_desc_of_chain(const VhostShadowVirtqueue *svq,
> > > > > > > > --
> > > > > > > > 2.31.1
> > > > > > > >
> > > > > > >
> > > > >
> > > >
> >

Re: [PATCH 2/4] vhost: toggle device callbacks using used event idx

[Jason Wang]

On Tue, Oct 25, 2022 at 1:36 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Tue, Oct 25, 2022 at 10:26:35AM +0800, Jason Wang wrote:
> > On Mon, Oct 24, 2022 at 10:05 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > >
> > > On Mon, Oct 24, 2022 at 04:00:37PM +0200, Eugenio Perez Martin wrote:
> > > > > > It's generally a waste that we don't use endian-ness annotations
> > > > > > the way linux does.
> > > > >
> > > > > Yes, it's worth doing something similar sometime.
> > > > >
> > > >
> > > > Maybe we could wrap them in some struct like virtio_le16 or virtio_16,
> > > > avoiding at least integer direct assignment? Wrappers like
> > > > cpu_to_virtio16 could return these structs and I think all compilers
> > > > should emit the same code as direct assignment.
> > > >
> > > > Thanks!
> > > >
> > >
> > > This will break bitwise operations such as | and &.
> > > Generally Linux has solved the problem and I don't think
> > > we should go look for another solution.
> >
> > Yes, but it should not block this series (we can do that in the future
> > if we had bandwidth).
> >
> > Thanks
>
> Sorry I don't get what you are saying. Which series?

I meant this series.

> Making LE tags structs is not going to fly, this is why sparse
> implements the __bitwise__ tag and in fact this is where the name comes
> from - bitwise operations need to work.

Yes but do we want to add sparse checks in this series only for shadow
virtqueue code?

Thanks

>
>
>
> > >
> > >
> > > >
> > > >
> > > > > Thanks
> > > > >
> > > > > >
> > > > > >
> > > > > > > > Thanks
> > > > > > > >
> > > > > > > > > +    } else {
> > > > > > > > > +        svq->vring.avail->flags &= ~cpu_to_le16(VRING_AVAIL_F_NO_INTERRUPT);
> > > > > > > > > +    }
> > > > > > > > > +
> > > > > > > > > +    /* Make sure the event is enabled before the read of used_idx */
> > > > > > > > >      smp_mb();
> > > > > > > > >      return !vhost_svq_more_used(svq);
> > > > > > > > >  }
> > > > > > > > >
> > > > > > > > >  static void vhost_svq_disable_notification(VhostShadowVirtqueue *svq)
> > > > > > > > >  {
> > > > > > > > > -    svq->vring.avail->flags |= cpu_to_le16(VRING_AVAIL_F_NO_INTERRUPT);
> > > > > > > > > +    /*
> > > > > > > > > +     * No need to disable notification in the event idx case, since used event
> > > > > > > > > +     * index is already an index too far away.
> > > > > > > > > +     */
> > > > > > > > > +    if (!virtio_vdev_has_feature(svq->vdev, VIRTIO_RING_F_EVENT_IDX)) {
> > > > > > > > > +        svq->vring.avail->flags |= cpu_to_le16(VRING_AVAIL_F_NO_INTERRUPT);
> > > > > > > > > +    }
> > > > > > > > >  }
> > > > > > > > >
> > > > > > > > >  static uint16_t vhost_svq_last_desc_of_chain(const VhostShadowVirtqueue *svq,
> > > > > > > > > --
> > > > > > > > > 2.31.1
> > > > > > > > >
> > > > > > > >
> > > > > >
> > > > >
> > >
>

Re: [PATCH 2/4] vhost: toggle device callbacks using used event idx

[Michael S. Tsirkin]

On Tue, Oct 25, 2022 at 01:46:43PM +0800, Jason Wang wrote:
> On Tue, Oct 25, 2022 at 1:36 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Tue, Oct 25, 2022 at 10:26:35AM +0800, Jason Wang wrote:
> > > On Mon, Oct 24, 2022 at 10:05 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > >
> > > > On Mon, Oct 24, 2022 at 04:00:37PM +0200, Eugenio Perez Martin wrote:
> > > > > > > It's generally a waste that we don't use endian-ness annotations
> > > > > > > the way linux does.
> > > > > >
> > > > > > Yes, it's worth doing something similar sometime.
> > > > > >
> > > > >
> > > > > Maybe we could wrap them in some struct like virtio_le16 or virtio_16,
> > > > > avoiding at least integer direct assignment? Wrappers like
> > > > > cpu_to_virtio16 could return these structs and I think all compilers
> > > > > should emit the same code as direct assignment.
> > > > >
> > > > > Thanks!
> > > > >
> > > >
> > > > This will break bitwise operations such as | and &.
> > > > Generally Linux has solved the problem and I don't think
> > > > we should go look for another solution.
> > >
> > > Yes, but it should not block this series (we can do that in the future
> > > if we had bandwidth).
> > >
> > > Thanks
> >
> > Sorry I don't get what you are saying. Which series?
> 
> I meant this series.
> 
> > Making LE tags structs is not going to fly, this is why sparse
> > implements the __bitwise__ tag and in fact this is where the name comes
> > from - bitwise operations need to work.
> 
> Yes but do we want to add sparse checks in this series only for shadow
> virtqueue code?
> 
> Thanks

No, I think a concerted effort to add endian-ness tagging is more
appropriate. Has nothing to do with shadow vq.


> >
> >
> >
> > > >
> > > >
> > > > >
> > > > >
> > > > > > Thanks
> > > > > >
> > > > > > >
> > > > > > >
> > > > > > > > > Thanks
> > > > > > > > >
> > > > > > > > > > +    } else {
> > > > > > > > > > +        svq->vring.avail->flags &= ~cpu_to_le16(VRING_AVAIL_F_NO_INTERRUPT);
> > > > > > > > > > +    }
> > > > > > > > > > +
> > > > > > > > > > +    /* Make sure the event is enabled before the read of used_idx */
> > > > > > > > > >      smp_mb();
> > > > > > > > > >      return !vhost_svq_more_used(svq);
> > > > > > > > > >  }
> > > > > > > > > >
> > > > > > > > > >  static void vhost_svq_disable_notification(VhostShadowVirtqueue *svq)
> > > > > > > > > >  {
> > > > > > > > > > -    svq->vring.avail->flags |= cpu_to_le16(VRING_AVAIL_F_NO_INTERRUPT);
> > > > > > > > > > +    /*
> > > > > > > > > > +     * No need to disable notification in the event idx case, since used event
> > > > > > > > > > +     * index is already an index too far away.
> > > > > > > > > > +     */
> > > > > > > > > > +    if (!virtio_vdev_has_feature(svq->vdev, VIRTIO_RING_F_EVENT_IDX)) {
> > > > > > > > > > +        svq->vring.avail->flags |= cpu_to_le16(VRING_AVAIL_F_NO_INTERRUPT);
> > > > > > > > > > +    }
> > > > > > > > > >  }
> > > > > > > > > >
> > > > > > > > > >  static uint16_t vhost_svq_last_desc_of_chain(const VhostShadowVirtqueue *svq,
> > > > > > > > > > --
> > > > > > > > > > 2.31.1
> > > > > > > > > >
> > > > > > > > >
> > > > > > >
> > > > > >
> > > >
> >

Re: [PATCH 2/4] vhost: toggle device callbacks using used event idx

[Eugenio Perez Martin]

On Mon, Oct 24, 2022 at 4:06 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Mon, Oct 24, 2022 at 04:00:37PM +0200, Eugenio Perez Martin wrote:
> > > > It's generally a waste that we don't use endian-ness annotations
> > > > the way linux does.
> > >
> > > Yes, it's worth doing something similar sometime.
> > >
> >
> > Maybe we could wrap them in some struct like virtio_le16 or virtio_16,
> > avoiding at least integer direct assignment? Wrappers like
> > cpu_to_virtio16 could return these structs and I think all compilers
> > should emit the same code as direct assignment.
> >
> > Thanks!
> >
>
> This will break bitwise operations such as | and &.
> Generally Linux has solved the problem and I don't think
> we should go look for another solution.
>

That's right, we would need to do it with functions like we do with
the Int128 type. The idea is the same, do not mix operations with
bitwise integers and cpu type ones.

But I totally agree a sparse tag or similar is way more clean and convenient.


>
> >
> >
> > > Thanks
> > >
> > > >
> > > >
> > > > > > Thanks
> > > > > >
> > > > > > > +    } else {
> > > > > > > +        svq->vring.avail->flags &= ~cpu_to_le16(VRING_AVAIL_F_NO_INTERRUPT);
> > > > > > > +    }
> > > > > > > +
> > > > > > > +    /* Make sure the event is enabled before the read of used_idx */
> > > > > > >      smp_mb();
> > > > > > >      return !vhost_svq_more_used(svq);
> > > > > > >  }
> > > > > > >
> > > > > > >  static void vhost_svq_disable_notification(VhostShadowVirtqueue *svq)
> > > > > > >  {
> > > > > > > -    svq->vring.avail->flags |= cpu_to_le16(VRING_AVAIL_F_NO_INTERRUPT);
> > > > > > > +    /*
> > > > > > > +     * No need to disable notification in the event idx case, since used event
> > > > > > > +     * index is already an index too far away.
> > > > > > > +     */
> > > > > > > +    if (!virtio_vdev_has_feature(svq->vdev, VIRTIO_RING_F_EVENT_IDX)) {
> > > > > > > +        svq->vring.avail->flags |= cpu_to_le16(VRING_AVAIL_F_NO_INTERRUPT);
> > > > > > > +    }
> > > > > > >  }
> > > > > > >
> > > > > > >  static uint16_t vhost_svq_last_desc_of_chain(const VhostShadowVirtqueue *svq,
> > > > > > > --
> > > > > > > 2.31.1
> > > > > > >
> > > > > >
> > > >
> > >
>

[PATCH 4/4] vhost: Accept event idx flag

[Eugenio Pérez]

Enabling all the code path created before.

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

diff --git a/hw/virtio/vhost-shadow-virtqueue.c b/hw/virtio/vhost-shadow-virtqueue.c
index f306ebef72..5bd14cad96 100644
--- a/hw/virtio/vhost-shadow-virtqueue.c
+++ b/hw/virtio/vhost-shadow-virtqueue.c
@@ -33,6 +33,7 @@ bool vhost_svq_valid_features(uint64_t features, Error **errp)
          ++b) {
         switch (b) {
         case VIRTIO_F_ANY_LAYOUT:
+        case VIRTIO_RING_F_EVENT_IDX:
             continue;
 
         case VIRTIO_F_ACCESS_PLATFORM:
-- 
2.31.1

Re: [PATCH 0/4] Shadow VirtQueue event index support

[Michael S. Tsirkin]

On Thu, Oct 20, 2022 at 05:52:47PM +0200, Eugenio Pérez wrote:
> Event idx helps to reduce the number of notifications between the device
> and the driver. It allows them to specify an index on the circular
> descriptors rings where to issue the notification, instead of a single
> binary indicator.
> 
> Adding support for SVQ.


Jason seems to be taking this through net

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

> These patches are sent on top of [1] series, so trivial conflicts could arise
> if it is applied directly on master. Future versions can be not based on
> it is more convenient.
> 
> [1] https://lists.nongnu.org/archive/html/qemu-devel/2022-10/msg03280.html
> 
> Eugenio Pérez (4):
>   vhost: allocate event_idx fields on vring
>   vhost: toggle device callbacks using used event idx
>   vhost: use avail event idx on vhost_svq_kick
>   vhost: Accept event idx flag
> 
>  hw/virtio/vhost-shadow-virtqueue.c | 39 ++++++++++++++++++++++++------
>  1 file changed, 31 insertions(+), 8 deletions(-)
> 
> -- 
> 2.31.1
> 

Re: [PATCH 0/4] Shadow VirtQueue event index support

[Jason Wang]

在 2022/10/27 04:58, Michael S. Tsirkin 写道:
> On Thu, Oct 20, 2022 at 05:52:47PM +0200, Eugenio Pérez wrote:
>> Event idx helps to reduce the number of notifications between the device
>> and the driver. It allows them to specify an index on the circular
>> descriptors rings where to issue the notification, instead of a single
>> binary indicator.
>>
>> Adding support for SVQ.
>
> Jason seems to be taking this through net
>
> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>


Ok, I've queued this.

Eugenio, please post the fix for endian on top.

Thanks


>
>> These patches are sent on top of [1] series, so trivial conflicts could arise
>> if it is applied directly on master. Future versions can be not based on
>> it is more convenient.
>>
>> [1] https://lists.nongnu.org/archive/html/qemu-devel/2022-10/msg03280.html
>>
>> Eugenio Pérez (4):
>>    vhost: allocate event_idx fields on vring
>>    vhost: toggle device callbacks using used event idx
>>    vhost: use avail event idx on vhost_svq_kick
>>    vhost: Accept event idx flag
>>
>>   hw/virtio/vhost-shadow-virtqueue.c | 39 ++++++++++++++++++++++++------
>>   1 file changed, 31 insertions(+), 8 deletions(-)
>>
>> -- 
>> 2.31.1
>>

Re: [PATCH 0/4] Shadow VirtQueue event index support

[Eugenio Perez Martin]

On Fri, Oct 28, 2022 at 4:44 AM Jason Wang <jasowang@redhat.com> wrote:
>
>
> 在 2022/10/27 04:58, Michael S. Tsirkin 写道:
> > On Thu, Oct 20, 2022 at 05:52:47PM +0200, Eugenio Pérez wrote:
> >> Event idx helps to reduce the number of notifications between the device
> >> and the driver. It allows them to specify an index on the circular
> >> descriptors rings where to issue the notification, instead of a single
> >> binary indicator.
> >>
> >> Adding support for SVQ.
> >
> > Jason seems to be taking this through net
> >
> > Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
>
>
> Ok, I've queued this.
>
> Eugenio, please post the fix for endian on top.
>

I've got a v2 ready to send, would it be possible to send it right now
and send a v2 pull? That would save a few commits where the vdpa
devices do not work with big endian architectures.

Thanks!

Re: [PATCH 0/4] Shadow VirtQueue event index support

[Jason Wang]

On Fri, Oct 28, 2022 at 2:50 PM Eugenio Perez Martin
<eperezma@redhat.com> wrote:
>
> On Fri, Oct 28, 2022 at 4:44 AM Jason Wang <jasowang@redhat.com> wrote:
> >
> >
> > 在 2022/10/27 04:58, Michael S. Tsirkin 写道:
> > > On Thu, Oct 20, 2022 at 05:52:47PM +0200, Eugenio Pérez wrote:
> > >> Event idx helps to reduce the number of notifications between the device
> > >> and the driver. It allows them to specify an index on the circular
> > >> descriptors rings where to issue the notification, instead of a single
> > >> binary indicator.
> > >>
> > >> Adding support for SVQ.
> > >
> > > Jason seems to be taking this through net
> > >
> > > Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> >
> >
> > Ok, I've queued this.
> >
> > Eugenio, please post the fix for endian on top.
> >
>
> I've got a v2 ready to send, would it be possible to send it right now
> and send a v2 pull? That would save a few commits where the vdpa
> devices do not work with big endian architectures.

Probably too late, but I queue the fixes on top to rc1.

Thanks

>
> Thanks!
>