From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:58311) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z8Abh-0002m4-Tk for qemu-devel@nongnu.org; Thu, 25 Jun 2015 13:06:34 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Z8Abc-0005YE-Tw for qemu-devel@nongnu.org; Thu, 25 Jun 2015 13:06:33 -0400 Received: from e06smtp16.uk.ibm.com ([195.75.94.112]:57533) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z8Abc-0005Xs-L8 for qemu-devel@nongnu.org; Thu, 25 Jun 2015 13:06:28 -0400 Received: from /spool/local by e06smtp16.uk.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Thu, 25 Jun 2015 18:06:27 +0100 Received: from b06cxnps3074.portsmouth.uk.ibm.com (d06relay09.portsmouth.uk.ibm.com [9.149.109.194]) by d06dlp02.portsmouth.uk.ibm.com (Postfix) with ESMTP id 5DB332190046 for ; Thu, 25 Jun 2015 18:06:01 +0100 (BST) Received: from d06av07.portsmouth.uk.ibm.com (d06av07.portsmouth.uk.ibm.com [9.149.37.248]) by b06cxnps3074.portsmouth.uk.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id t5PH6Nij35389690 for ; Thu, 25 Jun 2015 17:06:23 GMT Received: from d06av07.portsmouth.uk.ibm.com (localhost [127.0.0.1]) by d06av07.portsmouth.uk.ibm.com (8.14.4/8.14.4/NCO v10.0 AVout) with ESMTP id t5PH6NBS014641 for ; Thu, 25 Jun 2015 13:06:23 -0400 Date: Thu, 25 Jun 2015 19:06:20 +0200 From: Greg Kurz Message-ID: <20150625190620.7403fb94@bahia.local> In-Reply-To: <20150625183447.078473c3.cornelia.huck@de.ibm.com> References: <20150625152621.16966.12617.stgit@bahia.lab.toulouse-stg.fr.ibm.com> <20150625183447.078473c3.cornelia.huck@de.ibm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] dataplane: endian fix in host notification List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Cornelia Huck Cc: qemu-devel@nongnu.org, Stefan Hajnoczi , "Michael S. Tsirkin" On Thu, 25 Jun 2015 18:34:47 +0200 Cornelia Huck wrote: > On Thu, 25 Jun 2015 17:26:21 +0200 > Greg Kurz 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 > > --- > > 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