From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S965810AbbDVJJM (ORCPT ); Wed, 22 Apr 2015 05:09:12 -0400 Received: from e06smtp11.uk.ibm.com ([195.75.94.107]:35797 "EHLO e06smtp11.uk.ibm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756336AbbDVJJH (ORCPT ); Wed, 22 Apr 2015 05:09:07 -0400 Date: Wed, 22 Apr 2015 11:08:54 +0200 From: Greg Kurz To: "Michael S. Tsirkin" Cc: Rusty Russell , Cornelia Huck , linux-api@vger.kernel.org, linux-kernel@vger.kernel.org, kvm@vger.kernel.org, virtualization@lists.linux-foundation.org Subject: Re: [PATCH v4 7/8] vhost: feature to set the vring endianness Message-ID: <20150422110854.74988416@bahia.local> In-Reply-To: <20150421201636-mutt-send-email-mst@redhat.com> References: <20150410101500.31843.61248.stgit@bahia.local> <20150410101627.31843.21710.stgit@bahia.local> <20150421155753-mutt-send-email-mst@redhat.com> <20150421174820.78c150b7@bahia.local> <20150421201636-mutt-send-email-mst@redhat.com> Organization: IBM X-Mailer: Claws Mail 3.11.1 (GTK+ 2.24.27; x86_64-redhat-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit X-TM-AS-MML: disable X-Content-Scanned: Fidelis XPS MAILER x-cbid: 15042209-0041-0000-0000-00000427F7F3 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, 21 Apr 2015 20:25:03 +0200 "Michael S. Tsirkin" wrote: [ ... ] > > > > @@ -630,6 +634,53 @@ static long vhost_set_memory(struct vhost_dev *d, struct vhost_memory __user *m) > > > > return 0; > > > > } > > > > > > > > +#ifdef CONFIG_VHOST_SET_ENDIAN_LEGACY > > > > +static long vhost_set_vring_big_endian(struct vhost_virtqueue *vq, > > > > + int __user *argp) > > > > +{ > > > > + struct vhost_vring_state s; > > > > + > > > > + if (vq->private_data) > > > > + return -EBUSY; > > > > + > > > > + if (copy_from_user(&s, argp, sizeof(s))) > > > > + return -EFAULT; > > > > + > > > > + if (s.num && s.num != 1) > > > > > > s.num & ~0x1 > > > > > > > Since s.num is unsigned and I assume this won't change, what about > > s.num > 1 as suggested by Cornelia ? > > I just tried and gcc optimizes > s.num != 0 && s.num != 1 to s.num > 1 > > The former will be more readable once we > replace 0 and 1 with defines. > > So ignore my advice, keep code as is but use defines. > Ok. [ ... ] > > > > --- a/include/uapi/linux/vhost.h > > > > +++ b/include/uapi/linux/vhost.h > > > > @@ -103,6 +103,15 @@ struct vhost_memory { > > > > /* Get accessor: reads index, writes value in num */ > > > > #define VHOST_GET_VRING_BASE _IOWR(VHOST_VIRTIO, 0x12, struct vhost_vring_state) > > > > > > > > +/* Set the vring byte order in num. This is a legacy only API that is simply > > > > + * ignored when VIRTIO_F_VERSION_1 is set. > > > > + * 0 to set to little-endian > > > > + * 1 to set to big-endian > > > > > > How about defines for these? > > > > > > > Ok. I'll put the defines here so that all the cross-endian stuff > > lies in the same hunk. Is it ok for you ? > > Fine. > > > > > + * other values return EINVAL. > > Pls also add a note saying that not all kernel configurations support this ioctl, > but all configurations that support SET also support GET. > Ok. > > > > + */ > > > > +#define VHOST_SET_VRING_BIG_ENDIAN _IOW(VHOST_VIRTIO, 0x13, struct vhost_vring_state) > > > > +#define VHOST_GET_VRING_BIG_ENDIAN _IOW(VHOST_VIRTIO, 0x14, struct vhost_vring_state) > > > > + > > > > /* The following ioctls use eventfd file descriptors to signal and poll > > > > * for events. */ > > > > > > > > > I'm inclined to think VHOST_SET_VRING_ENDIAN is a slightly better name. > What do you think? > Or VHOST_SET_VRING_CROSS_ENDIAN ? I like the idea to keep a hint that this API is for cross-endian only... like the rest of this series. -- Greg From mboxrd@z Thu Jan 1 00:00:00 1970 From: Greg Kurz Subject: Re: [PATCH v4 7/8] vhost: feature to set the vring endianness Date: Wed, 22 Apr 2015 11:08:54 +0200 Message-ID: <20150422110854.74988416@bahia.local> References: <20150410101500.31843.61248.stgit@bahia.local> <20150410101627.31843.21710.stgit@bahia.local> <20150421155753-mutt-send-email-mst@redhat.com> <20150421174820.78c150b7@bahia.local> <20150421201636-mutt-send-email-mst@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20150421201636-mutt-send-email-mst-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> Sender: linux-api-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: "Michael S. Tsirkin" Cc: Rusty Russell , Cornelia Huck , linux-api-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, kvm-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, virtualization-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org List-Id: linux-api@vger.kernel.org On Tue, 21 Apr 2015 20:25:03 +0200 "Michael S. Tsirkin" wrote: [ ... ] > > > > @@ -630,6 +634,53 @@ static long vhost_set_memory(struct vhost_dev *d, struct vhost_memory __user *m) > > > > return 0; > > > > } > > > > > > > > +#ifdef CONFIG_VHOST_SET_ENDIAN_LEGACY > > > > +static long vhost_set_vring_big_endian(struct vhost_virtqueue *vq, > > > > + int __user *argp) > > > > +{ > > > > + struct vhost_vring_state s; > > > > + > > > > + if (vq->private_data) > > > > + return -EBUSY; > > > > + > > > > + if (copy_from_user(&s, argp, sizeof(s))) > > > > + return -EFAULT; > > > > + > > > > + if (s.num && s.num != 1) > > > > > > s.num & ~0x1 > > > > > > > Since s.num is unsigned and I assume this won't change, what about > > s.num > 1 as suggested by Cornelia ? > > I just tried and gcc optimizes > s.num != 0 && s.num != 1 to s.num > 1 > > The former will be more readable once we > replace 0 and 1 with defines. > > So ignore my advice, keep code as is but use defines. > Ok. [ ... ] > > > > --- a/include/uapi/linux/vhost.h > > > > +++ b/include/uapi/linux/vhost.h > > > > @@ -103,6 +103,15 @@ struct vhost_memory { > > > > /* Get accessor: reads index, writes value in num */ > > > > #define VHOST_GET_VRING_BASE _IOWR(VHOST_VIRTIO, 0x12, struct vhost_vring_state) > > > > > > > > +/* Set the vring byte order in num. This is a legacy only API that is simply > > > > + * ignored when VIRTIO_F_VERSION_1 is set. > > > > + * 0 to set to little-endian > > > > + * 1 to set to big-endian > > > > > > How about defines for these? > > > > > > > Ok. I'll put the defines here so that all the cross-endian stuff > > lies in the same hunk. Is it ok for you ? > > Fine. > > > > > + * other values return EINVAL. > > Pls also add a note saying that not all kernel configurations support this ioctl, > but all configurations that support SET also support GET. > Ok. > > > > + */ > > > > +#define VHOST_SET_VRING_BIG_ENDIAN _IOW(VHOST_VIRTIO, 0x13, struct vhost_vring_state) > > > > +#define VHOST_GET_VRING_BIG_ENDIAN _IOW(VHOST_VIRTIO, 0x14, struct vhost_vring_state) > > > > + > > > > /* The following ioctls use eventfd file descriptors to signal and poll > > > > * for events. */ > > > > > > > > > I'm inclined to think VHOST_SET_VRING_ENDIAN is a slightly better name. > What do you think? > Or VHOST_SET_VRING_CROSS_ENDIAN ? I like the idea to keep a hint that this API is for cross-endian only... like the rest of this series. -- Greg