| Message ID | 20150625152621.16966.12617.stgit@bahia.lab.toulouse-stg.fr.ibm.com |
|---|---|
| State | New |
| Headers | show |
On Thu, 25 Jun 2015 17:26:21 +0200 Greg Kurz <gkurz@linux.vnet.ibm.com> wrote: > This field comes either LE with virtio 1.0, either guest endian with legacy. > It must only be accessed with an accessor that knows about the appropriate > endianness. > > Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com> > --- > hw/virtio/dataplane/vring.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/hw/virtio/dataplane/vring.c b/hw/virtio/dataplane/vring.c > index 3fa421b9d773..a93ee2d338d7 100644 > --- a/hw/virtio/dataplane/vring.c > +++ b/hw/virtio/dataplane/vring.c > @@ -117,7 +117,7 @@ void vring_disable_notification(VirtIODevice *vdev, Vring *vring) > bool vring_enable_notification(VirtIODevice *vdev, Vring *vring) > { > if (virtio_has_feature(vdev, VIRTIO_RING_F_EVENT_IDX)) { > - vring_avail_event(&vring->vr) = vring->vr.avail->idx; > + vring_avail_event(&vring->vr) = vring_get_avail_idx(vdev, vring); > } else { > vring_clear_used_flags(vdev, vring, VRING_USED_F_NO_NOTIFY); > } > Hm... we should publish in the same endianness as the queue, shouldn't we? IOW, this seems fine. OTOH, this prompted me to check for other places where we touch vring_avail_event and it seems to me that we need to convert vring->last_avail_idx before we set it in vring_pop(). I might be confused, though :)
On Thu, 25 Jun 2015 18:34:47 +0200 Cornelia Huck <cornelia.huck@de.ibm.com> wrote: > On Thu, 25 Jun 2015 17:26:21 +0200 > Greg Kurz <gkurz@linux.vnet.ibm.com> wrote: > > > This field comes either LE with virtio 1.0, either guest endian with legacy. > > It must only be accessed with an accessor that knows about the appropriate > > endianness. > > > > Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com> > > --- > > hw/virtio/dataplane/vring.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/hw/virtio/dataplane/vring.c b/hw/virtio/dataplane/vring.c > > index 3fa421b9d773..a93ee2d338d7 100644 > > --- a/hw/virtio/dataplane/vring.c > > +++ b/hw/virtio/dataplane/vring.c > > @@ -117,7 +117,7 @@ void vring_disable_notification(VirtIODevice *vdev, Vring *vring) > > bool vring_enable_notification(VirtIODevice *vdev, Vring *vring) > > { > > if (virtio_has_feature(vdev, VIRTIO_RING_F_EVENT_IDX)) { > > - vring_avail_event(&vring->vr) = vring->vr.avail->idx; > > + vring_avail_event(&vring->vr) = vring_get_avail_idx(vdev, vring); > > } else { > > vring_clear_used_flags(vdev, vring, VRING_USED_F_NO_NOTIFY); > > } > > > Hm... we should publish in the same endianness as the queue, shouldn't > we? IOW, this seems fine. Er, by this I mean "it is fine without your patch". Long day... > > OTOH, this prompted me to check for other places where we touch > vring_avail_event and it seems to me that we need to convert > vring->last_avail_idx before we set it in vring_pop(). > > I might be confused, though :) > >
On Thu, 25 Jun 2015 18:34:47 +0200 Cornelia Huck <cornelia.huck@de.ibm.com> wrote: > On Thu, 25 Jun 2015 17:26:21 +0200 > Greg Kurz <gkurz@linux.vnet.ibm.com> wrote: > > > This field comes either LE with virtio 1.0, either guest endian with legacy. > > It must only be accessed with an accessor that knows about the appropriate > > endianness. > > > > Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com> > > --- > > hw/virtio/dataplane/vring.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/hw/virtio/dataplane/vring.c b/hw/virtio/dataplane/vring.c > > index 3fa421b9d773..a93ee2d338d7 100644 > > --- a/hw/virtio/dataplane/vring.c > > +++ b/hw/virtio/dataplane/vring.c > > @@ -117,7 +117,7 @@ void vring_disable_notification(VirtIODevice *vdev, Vring *vring) > > bool vring_enable_notification(VirtIODevice *vdev, Vring *vring) > > { > > if (virtio_has_feature(vdev, VIRTIO_RING_F_EVENT_IDX)) { > > - vring_avail_event(&vring->vr) = vring->vr.avail->idx; > > + vring_avail_event(&vring->vr) = vring_get_avail_idx(vdev, vring); > > } else { > > vring_clear_used_flags(vdev, vring, VRING_USED_F_NO_NOTIFY); > > } > > > Hm... we should publish in the same endianness as the queue, shouldn't > we? IOW, this seems fine. > > OTOH, this prompted me to check for other places where we touch > vring_avail_event and it seems to me that we need to convert > vring->last_avail_idx before we set it in vring_pop(). > > I might be confused, though :) > Heh you are the expert and it is more likely I am the confused one :) But indeed, part of this confusion comes from these lines in vring_pop(): if (virtio_has_feature(vdev, VIRTIO_RING_F_EVENT_IDX)) { vring_avail_event(&vring->vr) = vring->last_avail_idx; } and the return statement in vring_disable_notification(): return !vring_more_avail(vdev, vring); i.e. return vring_get_avail_idx(vdev, vring) == vring->last_avail_idx; which made me think last_avail_idx AND vring_avail_event(&vring->vr) have host endianness... I will try what you suggest tomorrow. Thanks for your feedback. -- Greg
On Thu, 25 Jun 2015 18:41:31 +0200 Cornelia Huck <cornelia.huck@de.ibm.com> wrote: > On Thu, 25 Jun 2015 18:34:47 +0200 > Cornelia Huck <cornelia.huck@de.ibm.com> wrote: > > > On Thu, 25 Jun 2015 17:26:21 +0200 > > Greg Kurz <gkurz@linux.vnet.ibm.com> wrote: > > > > > This field comes either LE with virtio 1.0, either guest endian with legacy. > > > It must only be accessed with an accessor that knows about the appropriate > > > endianness. > > > > > > Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com> > > > --- > > > hw/virtio/dataplane/vring.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/hw/virtio/dataplane/vring.c b/hw/virtio/dataplane/vring.c > > > index 3fa421b9d773..a93ee2d338d7 100644 > > > --- a/hw/virtio/dataplane/vring.c > > > +++ b/hw/virtio/dataplane/vring.c > > > @@ -117,7 +117,7 @@ void vring_disable_notification(VirtIODevice *vdev, Vring *vring) > > > bool vring_enable_notification(VirtIODevice *vdev, Vring *vring) > > > { > > > if (virtio_has_feature(vdev, VIRTIO_RING_F_EVENT_IDX)) { > > > - vring_avail_event(&vring->vr) = vring->vr.avail->idx; > > > + vring_avail_event(&vring->vr) = vring_get_avail_idx(vdev, vring); > > > } else { > > > vring_clear_used_flags(vdev, vring, VRING_USED_F_NO_NOTIFY); > > > } > > > > > Hm... we should publish in the same endianness as the queue, shouldn't > > we? IOW, this seems fine. > > Er, by this I mean "it is fine without your patch". Long day... > Heh your OTOH comment below made it quite clear about what "seems fine". Thanks for this clarification anyway :) > > > > OTOH, this prompted me to check for other places where we touch > > vring_avail_event and it seems to me that we need to convert > > vring->last_avail_idx before we set it in vring_pop(). > > > > I might be confused, though :) > > > > >
diff --git a/hw/virtio/dataplane/vring.c b/hw/virtio/dataplane/vring.c index 3fa421b9d773..a93ee2d338d7 100644 --- a/hw/virtio/dataplane/vring.c +++ b/hw/virtio/dataplane/vring.c @@ -117,7 +117,7 @@ void vring_disable_notification(VirtIODevice *vdev, Vring *vring) bool vring_enable_notification(VirtIODevice *vdev, Vring *vring) { if (virtio_has_feature(vdev, VIRTIO_RING_F_EVENT_IDX)) { - vring_avail_event(&vring->vr) = vring->vr.avail->idx; + vring_avail_event(&vring->vr) = vring_get_avail_idx(vdev, vring); } else { vring_clear_used_flags(vdev, vring, VRING_USED_F_NO_NOTIFY); }
This field comes either LE with virtio 1.0, either guest endian with legacy. It must only be accessed with an accessor that knows about the appropriate endianness. Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com> --- hw/virtio/dataplane/vring.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)