Cornelia Huck
2015-Apr-14 14:20 UTC
[PATCH v4 7/8] vhost: feature to set the vring endianness
On Fri, 10 Apr 2015 12:19:16 +0200 Greg Kurz <gkurz at linux.vnet.ibm.com> wrote:> This patch brings cross-endian support to vhost when used to implement > legacy virtio devices. Since it is a relatively rare situation, the > feature availability is controlled by a kernel config option (not set > by default). > > The vq->is_le boolean field is added to cache the endianness to be > used for ring accesses. It defaults to native endian, as expected > by legacy virtio devices. When the ring gets active, we force little > endian if the device is modern. When the ring is deactivated, we > revert to the native endian default. > > If cross-endian was compiled in, a vq->user_be boolean field is added > so that userspace may request a specific endianness. This field is > used to override the default when activating the ring of a legacy > device. It has no effect on modern devices. > > Signed-off-by: Greg Kurz <gkurz at linux.vnet.ibm.com> > --- > drivers/vhost/Kconfig | 10 ++++++ > drivers/vhost/vhost.c | 76 +++++++++++++++++++++++++++++++++++++++++++- > drivers/vhost/vhost.h | 12 +++++-- > include/uapi/linux/vhost.h | 9 +++++ > 4 files changed, 103 insertions(+), 4 deletions(-)> +#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)Checking for s.num > 1 might be more obvious at a glance?> + return -EINVAL; > + > + vq->user_be = s.num; > + > + return 0; > +} > +(...)> +#ifdef CONFIG_VHOST_SET_ENDIAN_LEGACY > +static void vhost_init_is_le(struct vhost_virtqueue *vq) > +{ > + vq->is_le = vhost_has_feature(vq, VIRTIO_F_VERSION_1) || !vq->user_be;So if the endianness is not explicitly set to BE, it will be forced to LE (instead of native endian)? Won't that break userspace that does not yet use the new interface when CONFIG_VHOST_SET_ENDIAN_LEGACY is set?> +} > +#else > +static void vhost_init_is_le(struct vhost_virtqueue *vq) > +{ > + if (vhost_has_feature(vq, VIRTIO_F_VERSION_1)) > + vq->is_le = true; > +} > +#endif > + > int vhost_init_used(struct vhost_virtqueue *vq) > { > __virtio16 last_used_idx; > int r; > - if (!vq->private_data) > + if (!vq->private_data) { > + vq->is_le = virtio_legacy_is_little_endian(); > return 0; > + } > + > + vhost_init_is_le(vq); > > r = vhost_update_used_flags(vq); > if (r)
On Tue, 14 Apr 2015 16:20:23 +0200 Cornelia Huck <cornelia.huck at de.ibm.com> wrote:> On Fri, 10 Apr 2015 12:19:16 +0200 > Greg Kurz <gkurz at linux.vnet.ibm.com> wrote: > > > This patch brings cross-endian support to vhost when used to implement > > legacy virtio devices. Since it is a relatively rare situation, the > > feature availability is controlled by a kernel config option (not set > > by default). > > > > The vq->is_le boolean field is added to cache the endianness to be > > used for ring accesses. It defaults to native endian, as expected > > by legacy virtio devices. When the ring gets active, we force little > > endian if the device is modern. When the ring is deactivated, we > > revert to the native endian default. > > > > If cross-endian was compiled in, a vq->user_be boolean field is added > > so that userspace may request a specific endianness. This field is > > used to override the default when activating the ring of a legacy > > device. It has no effect on modern devices. > > > > Signed-off-by: Greg Kurz <gkurz at linux.vnet.ibm.com> > > --- > > drivers/vhost/Kconfig | 10 ++++++ > > drivers/vhost/vhost.c | 76 +++++++++++++++++++++++++++++++++++++++++++- > > drivers/vhost/vhost.h | 12 +++++-- > > include/uapi/linux/vhost.h | 9 +++++ > > 4 files changed, 103 insertions(+), 4 deletions(-) > > > +#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) > > Checking for s.num > 1 might be more obvious at a glance? >Sure since s.num is unsigned.> > + return -EINVAL; > > + > > + vq->user_be = s.num; > > + > > + return 0; > > +} > > + > > (...) > > > +#ifdef CONFIG_VHOST_SET_ENDIAN_LEGACY > > +static void vhost_init_is_le(struct vhost_virtqueue *vq) > > +{ > > + vq->is_le = vhost_has_feature(vq, VIRTIO_F_VERSION_1) || !vq->user_be; > > So if the endianness is not explicitly set to BE, it will be forced to > LE (instead of native endian)? Won't that break userspace that does not > yet use the new interface when CONFIG_VHOST_SET_ENDIAN_LEGACY is set? >If userspace doesn't use the new interface, then vq->user_be will retain its default value that was set in vhost_vq_reset(), i.e. : vq->user_be = !virtio_legacy_is_little_endian(); This means default is native endian. What about adding this comment ? static void vhost_init_is_le(struct vhost_virtqueue *vq) { /* Note for legacy virtio: user_be is initialized in vhost_vq_reset() * according to the host endianness. If userspace does not set an * explicit endianness, the default behavior is native endian, as * expected by legacy virtio. */ vq->is_le = vhost_has_feature(vq, VIRTIO_F_VERSION_1) || !vq->user_be; }> > +} > > +#else > > +static void vhost_init_is_le(struct vhost_virtqueue *vq) > > +{ > > + if (vhost_has_feature(vq, VIRTIO_F_VERSION_1)) > > + vq->is_le = true; > > +} > > +#endif > > + > > int vhost_init_used(struct vhost_virtqueue *vq) > > { > > __virtio16 last_used_idx; > > int r; > > - if (!vq->private_data) > > + if (!vq->private_data) { > > + vq->is_le = virtio_legacy_is_little_endian(); > > return 0; > > + } > > + > > + vhost_init_is_le(vq); > > > > r = vhost_update_used_flags(vq); > > if (r)
Cornelia Huck
2015-Apr-14 15:22 UTC
[PATCH v4 7/8] vhost: feature to set the vring endianness
On Tue, 14 Apr 2015 17:13:52 +0200 Greg Kurz <gkurz at linux.vnet.ibm.com> wrote:> On Tue, 14 Apr 2015 16:20:23 +0200 > Cornelia Huck <cornelia.huck at de.ibm.com> wrote: > > > On Fri, 10 Apr 2015 12:19:16 +0200 > > Greg Kurz <gkurz at linux.vnet.ibm.com> wrote:> > > > > +#ifdef CONFIG_VHOST_SET_ENDIAN_LEGACY > > > +static void vhost_init_is_le(struct vhost_virtqueue *vq) > > > +{ > > > + vq->is_le = vhost_has_feature(vq, VIRTIO_F_VERSION_1) || !vq->user_be; > > > > So if the endianness is not explicitly set to BE, it will be forced to > > LE (instead of native endian)? Won't that break userspace that does not > > yet use the new interface when CONFIG_VHOST_SET_ENDIAN_LEGACY is set? > > > > If userspace doesn't use the new interface, then vq->user_be will retain its > default value that was set in vhost_vq_reset(), i.e. : > > vq->user_be = !virtio_legacy_is_little_endian(); > > This means default is native endian. > > What about adding this comment ? > > static void vhost_init_is_le(struct vhost_virtqueue *vq) > { > /* Note for legacy virtio: user_be is initialized in vhost_vq_reset() > * according to the host endianness. If userspace does not set an > * explicit endianness, the default behavior is native endian, as > * expected by legacy virtio. > */Good idea, as this is easy to miss.> vq->is_le = vhost_has_feature(vq, VIRTIO_F_VERSION_1) || !vq->user_be; > } > > > > +} > > > +#else > > > +static void vhost_init_is_le(struct vhost_virtqueue *vq) > > > +{ > > > + if (vhost_has_feature(vq, VIRTIO_F_VERSION_1)) > > > + vq->is_le = true; > > > +} > > > +#endif > > > + > > > int vhost_init_used(struct vhost_virtqueue *vq) > > > { > > > __virtio16 last_used_idx; > > > int r; > > > - if (!vq->private_data) > > > + if (!vq->private_data) { > > > + vq->is_le = virtio_legacy_is_little_endian(); > > > return 0; > > > + } > > > + > > > + vhost_init_is_le(vq); > > > > > > r = vhost_update_used_flags(vq); > > > if (r) >
Seemingly Similar Threads
- [PATCH v4 7/8] vhost: feature to set the vring endianness
- [PATCH v4 7/8] vhost: feature to set the vring endianness
- [PATCH v4 7/8] vhost: feature to set the vring endianness
- [PATCH v4 7/8] vhost: feature to set the vring endianness
- [PATCH v4 7/8] vhost: feature to set the vring endianness