| Message ID | 20221020155251.398735-3-eperezma@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Shadow VirtQueue event index support | expand |
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 >
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 > > >
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 > > > > >
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 > > > > > > > >
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 > > > > > > > > > > > >
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 > > > > > > > > > > > > > > > >
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 > > > > > > > > > > > > > > > > > > > > >
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 > > > > > > > > > > > > > > > > > > > > > > > > > >
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 > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
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 > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
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 > > > > > > > > > > > > > > > > > > > > >
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,
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(-)