[Qemu-devel] [PATCH] dataplane: endian fix in host notification

[Qemu-devel] [PATCH] dataplane: endian fix in host notification

[Greg Kurz]

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);
     }

Re: [Qemu-devel] [PATCH] dataplane: endian fix in host notification

[Cornelia Huck]

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

Re: [Qemu-devel] [PATCH] dataplane: endian fix in host notification

[Cornelia Huck]

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

Re: [Qemu-devel] [PATCH] dataplane: endian fix in host notification

[Greg Kurz]

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

Re: [Qemu-devel] [PATCH] dataplane: endian fix in host notification

[Greg Kurz]

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