Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1331077 > unrolled thread

Re: [PATCH 1/2] vhost: helpers to enable/disable vring endianness

Started by"Michael S. Tsirkin" <mst@redhat.com>
First post2016-02-10 12:30 +0100
Last post2016-02-10 16:30 +0100
Articles 5 — 3 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 1/2] vhost: helpers to enable/disable vring endianness "Michael S. Tsirkin" <mst@redhat.com> - 2016-02-10 12:30 +0100
    Re: [PATCH 1/2] vhost: helpers to enable/disable vring endianness Greg Kurz <gkurz@linux.vnet.ibm.com> - 2016-02-10 13:20 +0100
      Re: [PATCH 1/2] vhost: helpers to enable/disable vring endianness Cornelia Huck <cornelia.huck@de.ibm.com> - 2016-02-10 14:20 +0100
      Re: [PATCH 1/2] vhost: helpers to enable/disable vring endianness "Michael S. Tsirkin" <mst@redhat.com> - 2016-02-10 16:10 +0100
        Re: [PATCH 1/2] vhost: helpers to enable/disable vring endianness Greg Kurz <gkurz@linux.vnet.ibm.com> - 2016-02-10 16:30 +0100

#1331077 — Re: [PATCH 1/2] vhost: helpers to enable/disable vring endianness

From"Michael S. Tsirkin" <mst@redhat.com>
Date2016-02-10 12:30 +0100
SubjectRe: [PATCH 1/2] vhost: helpers to enable/disable vring endianness
Message-ID<r0B86-1xR-13@gated-at.bofh.it>
On Wed, Jan 13, 2016 at 06:09:41PM +0100, Greg Kurz wrote:
> The default use case for vhost is when the host and the vring have the
> same endianness (default native endianness). But there are cases where
> they differ and vhost should byteswap when accessing the vring:
> - the host is big endian and the vring comes from a virtio 1.0 device
>   which is always little endian
> - the architecture is bi-endian and the vring comes from a legacy virtio
>   device with a different endianness than the endianness of the host (aka
>   legacy cross-endian)
> 
> These cases are handled by the vq->is_le and the optional vq->user_be,
> with the following logic:
> - if none of the fields is enabled, vhost access the vring without byteswap
> - if the vring is virtio 1.0 and the host is big endian, vq->is_le is
>   enabled to enforce little endian access to the vring
> - if the vring is legacy cross-endian, userspace enables vq->user_be
>   to inform vhost about the vring endianness. This endianness is then
>   enforced for vring accesses through vq->is_le again
> 
> The logic is unclear in the current code.
> 
> This patch introduces helpers with explicit enable and disable semantics,
> for better clarity.
> 
> No behaviour change.
> 
> Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> ---
>  drivers/vhost/vhost.c |   28 +++++++++++++++++++---------
>  1 file changed, 19 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> index ad2146a9ab2d..e02e06755ab7 100644
> --- a/drivers/vhost/vhost.c
> +++ b/drivers/vhost/vhost.c
> @@ -43,11 +43,16 @@ enum {
>  #define vhost_avail_event(vq) ((__virtio16 __user *)&vq->used->ring[vq->num])
>  
>  #ifdef CONFIG_VHOST_CROSS_ENDIAN_LEGACY
> -static void vhost_vq_reset_user_be(struct vhost_virtqueue *vq)
> +static void vhost_disable_user_be(struct vhost_virtqueue *vq)
>  {
>  	vq->user_be = !virtio_legacy_is_little_endian();
>  }
>  

Hmm this doesn't look like an improvement to me.
What does it mean to disable big endian? Make it little endian?
Existing reset seems to make sense.

> +static void vhost_enable_user_be(struct vhost_virtqueue *vq, bool user_be)
> +{
> +	vq->user_be = user_be;
> +}
> +

And this is maybe "init_user_be"?

>  static long vhost_set_vring_endian(struct vhost_virtqueue *vq, int __user *argp)
>  {
>  	struct vhost_vring_state s;
> @@ -62,7 +67,7 @@ static long vhost_set_vring_endian(struct vhost_virtqueue *vq, int __user *argp)
>  	    s.num != VHOST_VRING_BIG_ENDIAN)
>  		return -EINVAL;
>  
> -	vq->user_be = s.num;
> +	vhost_enable_user_be(vq, !!s.num);
>  
>  	return 0;
>  }
> @@ -81,7 +86,7 @@ static long vhost_get_vring_endian(struct vhost_virtqueue *vq, u32 idx,
>  	return 0;
>  }
>  
> -static void vhost_init_is_le(struct vhost_virtqueue *vq)
> +static void vhost_enable_is_le(struct vhost_virtqueue *vq)
>  {
>  	/* Note for legacy virtio: user_be is initialized at reset time
>  	 * according to the host endianness. If userspace does not set an

Same thing really. I'd rather add "reset_is_le".

> @@ -91,7 +96,7 @@ static void vhost_init_is_le(struct vhost_virtqueue *vq)
>  	vq->is_le = vhost_has_feature(vq, VIRTIO_F_VERSION_1) || !vq->user_be;
>  }
>  #else
> -static void vhost_vq_reset_user_be(struct vhost_virtqueue *vq)
> +static void vhost_disable_user_be(struct vhost_virtqueue *vq)
>  {
>  }
>  
> @@ -106,13 +111,18 @@ static long vhost_get_vring_endian(struct vhost_virtqueue *vq, u32 idx,
>  	return -ENOIOCTLCMD;
>  }
>  
> -static void vhost_init_is_le(struct vhost_virtqueue *vq)
> +static void vhost_enable_is_le(struct vhost_virtqueue *vq)
>  {
>  	if (vhost_has_feature(vq, VIRTIO_F_VERSION_1))
>  		vq->is_le = true;
>  }
>  #endif /* CONFIG_VHOST_CROSS_ENDIAN_LEGACY */
>  
> +static void vhost_disable_is_le(struct vhost_virtqueue *vq)
> +{
> +	vq->is_le = virtio_legacy_is_little_endian();
> +}
> +
>  static void vhost_poll_func(struct file *file, wait_queue_head_t *wqh,
>  			    poll_table *pt)
>  {
> @@ -276,8 +286,8 @@ static void vhost_vq_reset(struct vhost_dev *dev,
>  	vq->call = NULL;
>  	vq->log_ctx = NULL;
>  	vq->memory = NULL;
> -	vq->is_le = virtio_legacy_is_little_endian();
> -	vhost_vq_reset_user_be(vq);
> +	vhost_disable_is_le(vq);
> +	vhost_disable_user_be(vq);
>  }
>  
>  static int vhost_worker(void *data)
> @@ -1157,11 +1167,11 @@ int vhost_init_used(struct vhost_virtqueue *vq)
>  	__virtio16 last_used_idx;
>  	int r;
>  	if (!vq->private_data) {
> -		vq->is_le = virtio_legacy_is_little_endian();
> +		vhost_disable_is_le(vq);
>  		return 0;
>  	}
>  
> -	vhost_init_is_le(vq);
> +	vhost_enable_is_le(vq);
>  
>  	r = vhost_update_used_flags(vq);
>  	if (r)

[toc] | [next] | [standalone]

#1331109

FromGreg Kurz <gkurz@linux.vnet.ibm.com>
Date2016-02-10 13:20 +0100
Message-ID<r0BUu-25C-15@gated-at.bofh.it>
In reply to#1331077
On Wed, 10 Feb 2016 13:21:22 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Wed, Jan 13, 2016 at 06:09:41PM +0100, Greg Kurz wrote:
> > The default use case for vhost is when the host and the vring have the
> > same endianness (default native endianness). But there are cases where
> > they differ and vhost should byteswap when accessing the vring:
> > - the host is big endian and the vring comes from a virtio 1.0 device
> >   which is always little endian
> > - the architecture is bi-endian and the vring comes from a legacy virtio
> >   device with a different endianness than the endianness of the host (aka
> >   legacy cross-endian)
> > 
> > These cases are handled by the vq->is_le and the optional vq->user_be,
> > with the following logic:
> > - if none of the fields is enabled, vhost access the vring without byteswap
> > - if the vring is virtio 1.0 and the host is big endian, vq->is_le is
> >   enabled to enforce little endian access to the vring
> > - if the vring is legacy cross-endian, userspace enables vq->user_be
> >   to inform vhost about the vring endianness. This endianness is then
> >   enforced for vring accesses through vq->is_le again
> > 
> > The logic is unclear in the current code.
> > 
> > This patch introduces helpers with explicit enable and disable semantics,
> > for better clarity.
> > 
> > No behaviour change.
> > 
> > Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> > ---
> >  drivers/vhost/vhost.c |   28 +++++++++++++++++++---------
> >  1 file changed, 19 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> > index ad2146a9ab2d..e02e06755ab7 100644
> > --- a/drivers/vhost/vhost.c
> > +++ b/drivers/vhost/vhost.c
> > @@ -43,11 +43,16 @@ enum {
> >  #define vhost_avail_event(vq) ((__virtio16 __user *)&vq->used->ring[vq->num])
> >  
> >  #ifdef CONFIG_VHOST_CROSS_ENDIAN_LEGACY
> > -static void vhost_vq_reset_user_be(struct vhost_virtqueue *vq)
> > +static void vhost_disable_user_be(struct vhost_virtqueue *vq)
> >  {
> >  	vq->user_be = !virtio_legacy_is_little_endian();
> >  }
> >    
> 
> Hmm this doesn't look like an improvement to me.
> What does it mean to disable big endian? Make it little endian?

The default behavior for the device is native endian.

The SET_VRING_ENDIAN ioctl is used to make the device big endian
on little endian hosts, hence "enabling" cross-endian mode...

> Existing reset seems to make sense.
> 

... and we "disable" cross-endian mode on reset.

> > +static void vhost_enable_user_be(struct vhost_virtqueue *vq, bool user_be)
> > +{
> > +	vq->user_be = user_be;
> > +}
> > +  
> 
> And this is maybe "init_user_be"?
> 

Anyway I don't mind changing the names to reset/init_user_be if you think it
is clearer.

> >  static long vhost_set_vring_endian(struct vhost_virtqueue *vq, int __user *argp)
> >  {
> >  	struct vhost_vring_state s;
> > @@ -62,7 +67,7 @@ static long vhost_set_vring_endian(struct vhost_virtqueue *vq, int __user *argp)
> >  	    s.num != VHOST_VRING_BIG_ENDIAN)
> >  		return -EINVAL;
> >  
> > -	vq->user_be = s.num;
> > +	vhost_enable_user_be(vq, !!s.num);
> >  
> >  	return 0;
> >  }
> > @@ -81,7 +86,7 @@ static long vhost_get_vring_endian(struct vhost_virtqueue *vq, u32 idx,
> >  	return 0;
> >  }
> >  
> > -static void vhost_init_is_le(struct vhost_virtqueue *vq)
> > +static void vhost_enable_is_le(struct vhost_virtqueue *vq)
> >  {
> >  	/* Note for legacy virtio: user_be is initialized at reset time
> >  	 * according to the host endianness. If userspace does not set an  
> 
> Same thing really. I'd rather add "reset_is_le".
> 
> > @@ -91,7 +96,7 @@ static void vhost_init_is_le(struct vhost_virtqueue *vq)
> >  	vq->is_le = vhost_has_feature(vq, VIRTIO_F_VERSION_1) || !vq->user_be;
> >  }
> >  #else
> > -static void vhost_vq_reset_user_be(struct vhost_virtqueue *vq)
> > +static void vhost_disable_user_be(struct vhost_virtqueue *vq)
> >  {
> >  }
> >  
> > @@ -106,13 +111,18 @@ static long vhost_get_vring_endian(struct vhost_virtqueue *vq, u32 idx,
> >  	return -ENOIOCTLCMD;
> >  }
> >  
> > -static void vhost_init_is_le(struct vhost_virtqueue *vq)
> > +static void vhost_enable_is_le(struct vhost_virtqueue *vq)
> >  {
> >  	if (vhost_has_feature(vq, VIRTIO_F_VERSION_1))
> >  		vq->is_le = true;
> >  }
> >  #endif /* CONFIG_VHOST_CROSS_ENDIAN_LEGACY */
> >  
> > +static void vhost_disable_is_le(struct vhost_virtqueue *vq)
> > +{
> > +	vq->is_le = virtio_legacy_is_little_endian();
> > +}
> > +
> >  static void vhost_poll_func(struct file *file, wait_queue_head_t *wqh,
> >  			    poll_table *pt)
> >  {
> > @@ -276,8 +286,8 @@ static void vhost_vq_reset(struct vhost_dev *dev,
> >  	vq->call = NULL;
> >  	vq->log_ctx = NULL;
> >  	vq->memory = NULL;
> > -	vq->is_le = virtio_legacy_is_little_endian();
> > -	vhost_vq_reset_user_be(vq);
> > +	vhost_disable_is_le(vq);
> > +	vhost_disable_user_be(vq);
> >  }
> >  
> >  static int vhost_worker(void *data)
> > @@ -1157,11 +1167,11 @@ int vhost_init_used(struct vhost_virtqueue *vq)
> >  	__virtio16 last_used_idx;
> >  	int r;
> >  	if (!vq->private_data) {
> > -		vq->is_le = virtio_legacy_is_little_endian();
> > +		vhost_disable_is_le(vq);
> >  		return 0;
> >  	}
> >  
> > -	vhost_init_is_le(vq);
> > +	vhost_enable_is_le(vq);
> >  
> >  	r = vhost_update_used_flags(vq);
> >  	if (r)  
>

[toc] | [prev] | [next] | [standalone]

#1331154

FromCornelia Huck <cornelia.huck@de.ibm.com>
Date2016-02-10 14:20 +0100
Message-ID<r0CQy-2ES-3@gated-at.bofh.it>
In reply to#1331109
On Wed, 10 Feb 2016 13:11:34 +0100
Greg Kurz <gkurz@linux.vnet.ibm.com> wrote:

> On Wed, 10 Feb 2016 13:21:22 +0200
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Wed, Jan 13, 2016 at 06:09:41PM +0100, Greg Kurz wrote:

> > > diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> > > index ad2146a9ab2d..e02e06755ab7 100644
> > > --- a/drivers/vhost/vhost.c
> > > +++ b/drivers/vhost/vhost.c
> > > @@ -43,11 +43,16 @@ enum {
> > >  #define vhost_avail_event(vq) ((__virtio16 __user *)&vq->used->ring[vq->num])
> > >  
> > >  #ifdef CONFIG_VHOST_CROSS_ENDIAN_LEGACY
> > > -static void vhost_vq_reset_user_be(struct vhost_virtqueue *vq)
> > > +static void vhost_disable_user_be(struct vhost_virtqueue *vq)
> > >  {
> > >  	vq->user_be = !virtio_legacy_is_little_endian();
> > >  }
> > >    
> > 
> > Hmm this doesn't look like an improvement to me.
> > What does it mean to disable big endian? Make it little endian?
> 
> The default behavior for the device is native endian.
> 
> The SET_VRING_ENDIAN ioctl is used to make the device big endian
> on little endian hosts, hence "enabling" cross-endian mode...
> 
> > Existing reset seems to make sense.
> > 
> 
> ... and we "disable" cross-endian mode on reset.
> 
> > > +static void vhost_enable_user_be(struct vhost_virtqueue *vq, bool user_be)
> > > +{
> > > +	vq->user_be = user_be;
> > > +}
> > > +  
> > 
> > And this is maybe "init_user_be"?
> > 
> 
> Anyway I don't mind changing the names to reset/init_user_be if you think it
> is clearer.

FWIW, I find the enable/disable terminology less confusing.

[toc] | [prev] | [next] | [standalone]

#1331255

From"Michael S. Tsirkin" <mst@redhat.com>
Date2016-02-10 16:10 +0100
Message-ID<r0Ez0-3Nr-21@gated-at.bofh.it>
In reply to#1331109
On Wed, Feb 10, 2016 at 01:11:34PM +0100, Greg Kurz wrote:
> On Wed, 10 Feb 2016 13:21:22 +0200
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Wed, Jan 13, 2016 at 06:09:41PM +0100, Greg Kurz wrote:
> > > The default use case for vhost is when the host and the vring have the
> > > same endianness (default native endianness). But there are cases where
> > > they differ and vhost should byteswap when accessing the vring:
> > > - the host is big endian and the vring comes from a virtio 1.0 device
> > >   which is always little endian
> > > - the architecture is bi-endian and the vring comes from a legacy virtio
> > >   device with a different endianness than the endianness of the host (aka
> > >   legacy cross-endian)
> > > 
> > > These cases are handled by the vq->is_le and the optional vq->user_be,
> > > with the following logic:
> > > - if none of the fields is enabled, vhost access the vring without byteswap
> > > - if the vring is virtio 1.0 and the host is big endian, vq->is_le is
> > >   enabled to enforce little endian access to the vring
> > > - if the vring is legacy cross-endian, userspace enables vq->user_be
> > >   to inform vhost about the vring endianness. This endianness is then
> > >   enforced for vring accesses through vq->is_le again
> > > 
> > > The logic is unclear in the current code.
> > > 
> > > This patch introduces helpers with explicit enable and disable semantics,
> > > for better clarity.
> > > 
> > > No behaviour change.
> > > 
> > > Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> > > ---
> > >  drivers/vhost/vhost.c |   28 +++++++++++++++++++---------
> > >  1 file changed, 19 insertions(+), 9 deletions(-)
> > > 
> > > diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> > > index ad2146a9ab2d..e02e06755ab7 100644
> > > --- a/drivers/vhost/vhost.c
> > > +++ b/drivers/vhost/vhost.c
> > > @@ -43,11 +43,16 @@ enum {
> > >  #define vhost_avail_event(vq) ((__virtio16 __user *)&vq->used->ring[vq->num])
> > >  
> > >  #ifdef CONFIG_VHOST_CROSS_ENDIAN_LEGACY
> > > -static void vhost_vq_reset_user_be(struct vhost_virtqueue *vq)
> > > +static void vhost_disable_user_be(struct vhost_virtqueue *vq)
> > >  {
> > >  	vq->user_be = !virtio_legacy_is_little_endian();
> > >  }
> > >    
> > 
> > Hmm this doesn't look like an improvement to me.
> > What does it mean to disable big endian? Make it little endian?
> 
> The default behavior for the device is native endian.
> 
> The SET_VRING_ENDIAN ioctl is used to make the device big endian
> on little endian hosts, hence "enabling" cross-endian mode...
> 
> > Existing reset seems to make sense.
> > 
> 
> ... and we "disable" cross-endian mode on reset.

OK but that's enable cross-endian. Not enable be.  We could have
something like vhost_disable_user_byte_swap though each time we try,
the result makes my head hurt: "swap" is a relative thing and hard to
keep track of.

> > > +static void vhost_enable_user_be(struct vhost_virtqueue *vq, bool user_be)
> > > +{
> > > +	vq->user_be = user_be;
> > > +}
> > > +  
> > 
> > And this is maybe "init_user_be"?
> > 
> 
> Anyway I don't mind changing the names to reset/init_user_be if you think it
> is clearer.
> 
> > >  static long vhost_set_vring_endian(struct vhost_virtqueue *vq, int __user *argp)
> > >  {
> > >  	struct vhost_vring_state s;
> > > @@ -62,7 +67,7 @@ static long vhost_set_vring_endian(struct vhost_virtqueue *vq, int __user *argp)
> > >  	    s.num != VHOST_VRING_BIG_ENDIAN)
> > >  		return -EINVAL;
> > >  
> > > -	vq->user_be = s.num;
> > > +	vhost_enable_user_be(vq, !!s.num);
> > >  
> > >  	return 0;
> > >  }
> > > @@ -81,7 +86,7 @@ static long vhost_get_vring_endian(struct vhost_virtqueue *vq, u32 idx,
> > >  	return 0;
> > >  }
> > >  
> > > -static void vhost_init_is_le(struct vhost_virtqueue *vq)
> > > +static void vhost_enable_is_le(struct vhost_virtqueue *vq)
> > >  {
> > >  	/* Note for legacy virtio: user_be is initialized at reset time
> > >  	 * according to the host endianness. If userspace does not set an  
> > 
> > Same thing really. I'd rather add "reset_is_le".
> > 
> > > @@ -91,7 +96,7 @@ static void vhost_init_is_le(struct vhost_virtqueue *vq)
> > >  	vq->is_le = vhost_has_feature(vq, VIRTIO_F_VERSION_1) || !vq->user_be;
> > >  }
> > >  #else
> > > -static void vhost_vq_reset_user_be(struct vhost_virtqueue *vq)
> > > +static void vhost_disable_user_be(struct vhost_virtqueue *vq)
> > >  {
> > >  }
> > >  
> > > @@ -106,13 +111,18 @@ static long vhost_get_vring_endian(struct vhost_virtqueue *vq, u32 idx,
> > >  	return -ENOIOCTLCMD;
> > >  }
> > >  
> > > -static void vhost_init_is_le(struct vhost_virtqueue *vq)
> > > +static void vhost_enable_is_le(struct vhost_virtqueue *vq)
> > >  {
> > >  	if (vhost_has_feature(vq, VIRTIO_F_VERSION_1))
> > >  		vq->is_le = true;
> > >  }
> > >  #endif /* CONFIG_VHOST_CROSS_ENDIAN_LEGACY */
> > >  
> > > +static void vhost_disable_is_le(struct vhost_virtqueue *vq)
> > > +{
> > > +	vq->is_le = virtio_legacy_is_little_endian();
> > > +}
> > > +
> > >  static void vhost_poll_func(struct file *file, wait_queue_head_t *wqh,
> > >  			    poll_table *pt)
> > >  {
> > > @@ -276,8 +286,8 @@ static void vhost_vq_reset(struct vhost_dev *dev,
> > >  	vq->call = NULL;
> > >  	vq->log_ctx = NULL;
> > >  	vq->memory = NULL;
> > > -	vq->is_le = virtio_legacy_is_little_endian();
> > > -	vhost_vq_reset_user_be(vq);
> > > +	vhost_disable_is_le(vq);
> > > +	vhost_disable_user_be(vq);
> > >  }
> > >  
> > >  static int vhost_worker(void *data)
> > > @@ -1157,11 +1167,11 @@ int vhost_init_used(struct vhost_virtqueue *vq)
> > >  	__virtio16 last_used_idx;
> > >  	int r;
> > >  	if (!vq->private_data) {
> > > -		vq->is_le = virtio_legacy_is_little_endian();
> > > +		vhost_disable_is_le(vq);
> > >  		return 0;
> > >  	}
> > >  
> > > -	vhost_init_is_le(vq);
> > > +	vhost_enable_is_le(vq);
> > >  
> > >  	r = vhost_update_used_flags(vq);
> > >  	if (r)  
> >

[toc] | [prev] | [next] | [standalone]

#1331273

FromGreg Kurz <gkurz@linux.vnet.ibm.com>
Date2016-02-10 16:30 +0100
Message-ID<r0ESm-3Vo-17@gated-at.bofh.it>
In reply to#1331255
On Wed, 10 Feb 2016 17:08:52 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Wed, Feb 10, 2016 at 01:11:34PM +0100, Greg Kurz wrote:
> > On Wed, 10 Feb 2016 13:21:22 +0200
> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> >   
> > > On Wed, Jan 13, 2016 at 06:09:41PM +0100, Greg Kurz wrote:  
> > > > The default use case for vhost is when the host and the vring have the
> > > > same endianness (default native endianness). But there are cases where
> > > > they differ and vhost should byteswap when accessing the vring:
> > > > - the host is big endian and the vring comes from a virtio 1.0 device
> > > >   which is always little endian
> > > > - the architecture is bi-endian and the vring comes from a legacy virtio
> > > >   device with a different endianness than the endianness of the host (aka
> > > >   legacy cross-endian)
> > > > 
> > > > These cases are handled by the vq->is_le and the optional vq->user_be,
> > > > with the following logic:
> > > > - if none of the fields is enabled, vhost access the vring without byteswap
> > > > - if the vring is virtio 1.0 and the host is big endian, vq->is_le is
> > > >   enabled to enforce little endian access to the vring
> > > > - if the vring is legacy cross-endian, userspace enables vq->user_be
> > > >   to inform vhost about the vring endianness. This endianness is then
> > > >   enforced for vring accesses through vq->is_le again
> > > > 
> > > > The logic is unclear in the current code.
> > > > 
> > > > This patch introduces helpers with explicit enable and disable semantics,
> > > > for better clarity.
> > > > 
> > > > No behaviour change.
> > > > 
> > > > Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> > > > ---
> > > >  drivers/vhost/vhost.c |   28 +++++++++++++++++++---------
> > > >  1 file changed, 19 insertions(+), 9 deletions(-)
> > > > 
> > > > diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> > > > index ad2146a9ab2d..e02e06755ab7 100644
> > > > --- a/drivers/vhost/vhost.c
> > > > +++ b/drivers/vhost/vhost.c
> > > > @@ -43,11 +43,16 @@ enum {
> > > >  #define vhost_avail_event(vq) ((__virtio16 __user *)&vq->used->ring[vq->num])
> > > >  
> > > >  #ifdef CONFIG_VHOST_CROSS_ENDIAN_LEGACY
> > > > -static void vhost_vq_reset_user_be(struct vhost_virtqueue *vq)
> > > > +static void vhost_disable_user_be(struct vhost_virtqueue *vq)
> > > >  {
> > > >  	vq->user_be = !virtio_legacy_is_little_endian();
> > > >  }
> > > >      
> > > 
> > > Hmm this doesn't look like an improvement to me.
> > > What does it mean to disable big endian? Make it little endian?  
> > 
> > The default behavior for the device is native endian.
> > 
> > The SET_VRING_ENDIAN ioctl is used to make the device big endian
> > on little endian hosts, hence "enabling" cross-endian mode...
> >   
> > > Existing reset seems to make sense.
> > >   
> > 
> > ... and we "disable" cross-endian mode on reset.  
> 
> OK but that's enable cross-endian. Not enable be.  We could have
> something like vhost_disable_user_byte_swap though each time we try,
> the result makes my head hurt: "swap" is a relative thing and hard to
> keep track of.
> 

What about having something like below then ?

vhost_enable_cross_endian_big() to be called on little endian hosts with big endian devices

vhost_enable_cross_endian_little() to be called on big endian hosts with little endian devices

vhost_disable_cross_endian() to go back to the native endian default

> > > > +static void vhost_enable_user_be(struct vhost_virtqueue *vq, bool user_be)
> > > > +{
> > > > +	vq->user_be = user_be;
> > > > +}
> > > > +    
> > > 
> > > And this is maybe "init_user_be"?
> > >   
> > 
> > Anyway I don't mind changing the names to reset/init_user_be if you think it
> > is clearer.
> >   
> > > >  static long vhost_set_vring_endian(struct vhost_virtqueue *vq, int __user *argp)
> > > >  {
> > > >  	struct vhost_vring_state s;
> > > > @@ -62,7 +67,7 @@ static long vhost_set_vring_endian(struct vhost_virtqueue *vq, int __user *argp)
> > > >  	    s.num != VHOST_VRING_BIG_ENDIAN)
> > > >  		return -EINVAL;
> > > >  
> > > > -	vq->user_be = s.num;
> > > > +	vhost_enable_user_be(vq, !!s.num);
> > > >  
> > > >  	return 0;
> > > >  }
> > > > @@ -81,7 +86,7 @@ static long vhost_get_vring_endian(struct vhost_virtqueue *vq, u32 idx,
> > > >  	return 0;
> > > >  }
> > > >  
> > > > -static void vhost_init_is_le(struct vhost_virtqueue *vq)
> > > > +static void vhost_enable_is_le(struct vhost_virtqueue *vq)
> > > >  {
> > > >  	/* Note for legacy virtio: user_be is initialized at reset time
> > > >  	 * according to the host endianness. If userspace does not set an    
> > > 
> > > Same thing really. I'd rather add "reset_is_le".
> > >   
> > > > @@ -91,7 +96,7 @@ static void vhost_init_is_le(struct vhost_virtqueue *vq)
> > > >  	vq->is_le = vhost_has_feature(vq, VIRTIO_F_VERSION_1) || !vq->user_be;
> > > >  }
> > > >  #else
> > > > -static void vhost_vq_reset_user_be(struct vhost_virtqueue *vq)
> > > > +static void vhost_disable_user_be(struct vhost_virtqueue *vq)
> > > >  {
> > > >  }
> > > >  
> > > > @@ -106,13 +111,18 @@ static long vhost_get_vring_endian(struct vhost_virtqueue *vq, u32 idx,
> > > >  	return -ENOIOCTLCMD;
> > > >  }
> > > >  
> > > > -static void vhost_init_is_le(struct vhost_virtqueue *vq)
> > > > +static void vhost_enable_is_le(struct vhost_virtqueue *vq)
> > > >  {
> > > >  	if (vhost_has_feature(vq, VIRTIO_F_VERSION_1))
> > > >  		vq->is_le = true;
> > > >  }
> > > >  #endif /* CONFIG_VHOST_CROSS_ENDIAN_LEGACY */
> > > >  
> > > > +static void vhost_disable_is_le(struct vhost_virtqueue *vq)
> > > > +{
> > > > +	vq->is_le = virtio_legacy_is_little_endian();
> > > > +}
> > > > +
> > > >  static void vhost_poll_func(struct file *file, wait_queue_head_t *wqh,
> > > >  			    poll_table *pt)
> > > >  {
> > > > @@ -276,8 +286,8 @@ static void vhost_vq_reset(struct vhost_dev *dev,
> > > >  	vq->call = NULL;
> > > >  	vq->log_ctx = NULL;
> > > >  	vq->memory = NULL;
> > > > -	vq->is_le = virtio_legacy_is_little_endian();
> > > > -	vhost_vq_reset_user_be(vq);
> > > > +	vhost_disable_is_le(vq);
> > > > +	vhost_disable_user_be(vq);
> > > >  }
> > > >  
> > > >  static int vhost_worker(void *data)
> > > > @@ -1157,11 +1167,11 @@ int vhost_init_used(struct vhost_virtqueue *vq)
> > > >  	__virtio16 last_used_idx;
> > > >  	int r;
> > > >  	if (!vq->private_data) {
> > > > -		vq->is_le = virtio_legacy_is_little_endian();
> > > > +		vhost_disable_is_le(vq);
> > > >  		return 0;
> > > >  	}
> > > >  
> > > > -	vhost_init_is_le(vq);
> > > > +	vhost_enable_is_le(vq);
> > > >  
> > > >  	r = vhost_update_used_flags(vq);
> > > >  	if (r)    
> > >   
>

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web