Linux Virtualization - Jan 2016 - [PATCH 0/2] vhost: cross-endian code cleanup

This series is a respin of the following patch:

http://patchwork.ozlabs.org/patch/565921/

Patch 1 is preliminary work: it gives better names to the helpers that are
involved in cross-endian support.

Patch 2 is actually a v2 of the original patch. All devices now call a
helper in the generic code, which DTRT according to vq->private_data, as
suggested by Michael.

---

Greg Kurz (2):
      vhost: helpers to enable/disable vring endianness
      vhost: disentangle vring endianness stuff from the core code


 drivers/vhost/net.c   |    3 +++
 drivers/vhost/scsi.c  |    3 +++
 drivers/vhost/test.c  |    2 ++
 drivers/vhost/vhost.c |   40 ++++++++++++++++++++++++++++------------
 drivers/vhost/vhost.h |    1 +
 5 files changed, 37 insertions(+), 12 deletions(-)

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 at 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();
 }
 
+static void vhost_enable_user_be(struct vhost_virtqueue *vq, bool user_be)
+{
+	vq->user_be = 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
@@ -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)

The way vring endianness is being handled currently obfuscates
the code in vhost_init_used().

This patch tries to fix that by doing the following:
- move the the code that adjusts endianness to a dedicated helper
- export this helper so that backends explicitely call it

No behaviour change.

Signed-off-by: Greg Kurz <gkurz at linux.vnet.ibm.com>
---
 drivers/vhost/net.c   |    3 +++
 drivers/vhost/scsi.c  |    3 +++
 drivers/vhost/test.c  |    2 ++
 drivers/vhost/vhost.c |   16 +++++++++++-----
 drivers/vhost/vhost.h |    1 +
 5 files changed, 20 insertions(+), 5 deletions(-)

diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
index 9eda69e40678..df01c939cd00 100644
--- a/drivers/vhost/net.c
+++ b/drivers/vhost/net.c
@@ -917,6 +917,9 @@ static long vhost_net_set_backend(struct vhost_net *n,
unsigned index, int fd)
 
 		vhost_net_disable_vq(n, vq);
 		vq->private_data = sock;
+
+		vhost_adjust_vring_endian(vq);
+
 		r = vhost_init_used(vq);
 		if (r)
 			goto err_used;
diff --git a/drivers/vhost/scsi.c b/drivers/vhost/scsi.c
index 29cfc57d496e..5a8363bfcb74 100644
--- a/drivers/vhost/scsi.c
+++ b/drivers/vhost/scsi.c
@@ -1274,6 +1274,9 @@ vhost_scsi_set_endpoint(struct vhost_scsi *vs,
 			vq = &vs->vqs[i].vq;
 			mutex_lock(&vq->mutex);
 			vq->private_data = vs_tpg;
+
+			vhost_adjust_vring_endian(vq);
+
 			vhost_init_used(vq);
 			mutex_unlock(&vq->mutex);
 		}
diff --git a/drivers/vhost/test.c b/drivers/vhost/test.c
index f2882ac98726..75e3e0e9f5a8 100644
--- a/drivers/vhost/test.c
+++ b/drivers/vhost/test.c
@@ -196,6 +196,8 @@ static long vhost_test_run(struct vhost_test *n, int test)
 		oldpriv = vq->private_data;
 		vq->private_data = priv;
 
+		vhost_adjust_vring_endian(vq);
+
 		r = vhost_init_used(&n->vqs[index]);
 
 		mutex_unlock(&vq->mutex);
diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index e02e06755ab7..b0a00340309e 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -123,6 +123,15 @@ static void vhost_disable_is_le(struct vhost_virtqueue *vq)
 	vq->is_le = virtio_legacy_is_little_endian();
 }
 
+void vhost_adjust_vring_endian(struct vhost_virtqueue *vq)
+{
+	if (!vq->private_data)
+		vhost_disable_is_le(vq);
+	else
+		vhost_enable_is_le(vq);
+}
+EXPORT_SYMBOL_GPL(vhost_adjust_vring_endian);
+
 static void vhost_poll_func(struct file *file, wait_queue_head_t *wqh,
 			    poll_table *pt)
 {
@@ -1166,12 +1175,9 @@ int vhost_init_used(struct vhost_virtqueue *vq)
 {
 	__virtio16 last_used_idx;
 	int r;
-	if (!vq->private_data) {
-		vhost_disable_is_le(vq);
-		return 0;
-	}
 
-	vhost_enable_is_le(vq);
+	if (!vq->private_data)
+		return 0;
 
 	r = vhost_update_used_flags(vq);
 	if (r)
diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h
index d3f767448a72..88d86f45f756 100644
--- a/drivers/vhost/vhost.h
+++ b/drivers/vhost/vhost.h
@@ -162,6 +162,7 @@ bool vhost_enable_notify(struct vhost_dev *, struct
vhost_virtqueue *);
 
 int vhost_log_write(struct vhost_virtqueue *vq, struct vhost_log *log,
 		    unsigned int log_num, u64 len);
+void vhost_adjust_vring_endian(struct vhost_virtqueue *vq);
 
 #define vq_err(vq, fmt, ...) do {                                  \
 		pr_debug(pr_fmt(fmt), ##__VA_ARGS__);       \

On Wed, 13 Jan 2016 18:09:41 +0100
Greg Kurz <gkurz at linux.vnet.ibm.com> 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 at linux.vnet.ibm.com>
> ---
>  drivers/vhost/vhost.c |   28 +++++++++++++++++++---------
>  1 file changed, 19 insertions(+), 9 deletions(-)
Reviewed-by: Cornelia Huck <cornelia.huck at de.ibm.com>

On Wed, 13 Jan 2016 18:09:47 +0100
Greg Kurz <gkurz at linux.vnet.ibm.com> wrote:
> The way vring endianness is being handled currently obfuscates
> the code in vhost_init_used().
> 
> This patch tries to fix that by doing the following:
> - move the the code that adjusts endianness to a dedicated helper
> - export this helper so that backends explicitely call it
> 
> No behaviour change.
> 
> Signed-off-by: Greg Kurz <gkurz at linux.vnet.ibm.com>
> ---
>  drivers/vhost/net.c   |    3 +++
>  drivers/vhost/scsi.c  |    3 +++
>  drivers/vhost/test.c  |    2 ++
>  drivers/vhost/vhost.c |   16 +++++++++++-----
>  drivers/vhost/vhost.h |    1 +
>  5 files changed, 20 insertions(+), 5 deletions(-)
Reviewed-by: Cornelia Huck <cornelia.huck at de.ibm.com>

On Wed, 13 Jan 2016 18:09:34 +0100
Greg Kurz <gkurz at linux.vnet.ibm.com> wrote:
> This series is a respin of the following patch:
> 
> http://patchwork.ozlabs.org/patch/565921/
> 
> Patch 1 is preliminary work: it gives better names to the helpers that are
> involved in cross-endian support.
> 
> Patch 2 is actually a v2 of the original patch. All devices now call a
> helper in the generic code, which DTRT according to vq->private_data, as
> suggested by Michael.
> 
Hi Michael,

This is just a friendly reminder for this series, which was
reviewed by Cornelia already.

Thanks.

--
Greg
> ---
> 
> Greg Kurz (2):
>       vhost: helpers to enable/disable vring endianness
>       vhost: disentangle vring endianness stuff from the core code
> 
> 
>  drivers/vhost/net.c   |    3 +++
>  drivers/vhost/scsi.c  |    3 +++
>  drivers/vhost/test.c  |    2 ++
>  drivers/vhost/vhost.c |   40 ++++++++++++++++++++++++++++------------
>  drivers/vhost/vhost.h |    1 +
>  5 files changed, 37 insertions(+), 12 deletions(-)
> 
> _______________________________________________
> Virtualization mailing list
> Virtualization at lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/virtualization
>

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

On Wed, Jan 13, 2016 at 06:09:47PM +0100, Greg Kurz
wrote:
> The way vring endianness is being handled currently obfuscates
> the code in vhost_init_used().
> 
> This patch tries to fix that by doing the following:
> - move the the code that adjusts endianness to a dedicated helper
> - export this helper so that backends explicitely call it
> 
> No behaviour change.
> 
> Signed-off-by: Greg Kurz <gkurz at linux.vnet.ibm.com>
> ---
>  drivers/vhost/net.c   |    3 +++
>  drivers/vhost/scsi.c  |    3 +++
>  drivers/vhost/test.c  |    2 ++
>  drivers/vhost/vhost.c |   16 +++++++++++-----
>  drivers/vhost/vhost.h |    1 +
>  5 files changed, 20 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
> index 9eda69e40678..df01c939cd00 100644
> --- a/drivers/vhost/net.c
> +++ b/drivers/vhost/net.c
> @@ -917,6 +917,9 @@ static long vhost_net_set_backend(struct vhost_net *n,
unsigned index, int fd)
>  
>  		vhost_net_disable_vq(n, vq);
>  		vq->private_data = sock;
> +
> +		vhost_adjust_vring_endian(vq);
> +
>  		r = vhost_init_used(vq);
>  		if (r)
>  			goto err_used;

This is in fact a bug in existing code: if vhost_init_used
fails, it preferably should not have side-effects.
It's best to update it last thing.
> diff --git a/drivers/vhost/scsi.c b/drivers/vhost/scsi.c
> index 29cfc57d496e..5a8363bfcb74 100644
> --- a/drivers/vhost/scsi.c
> +++ b/drivers/vhost/scsi.c
> @@ -1274,6 +1274,9 @@ vhost_scsi_set_endpoint(struct vhost_scsi *vs,
>  			vq = &vs->vqs[i].vq;
>  			mutex_lock(&vq->mutex);
>  			vq->private_data = vs_tpg;
> +
> +			vhost_adjust_vring_endian(vq);
> +
>  			vhost_init_used(vq);
>  			mutex_unlock(&vq->mutex);
>  		}
> diff --git a/drivers/vhost/test.c b/drivers/vhost/test.c
> index f2882ac98726..75e3e0e9f5a8 100644
> --- a/drivers/vhost/test.c
> +++ b/drivers/vhost/test.c
> @@ -196,6 +196,8 @@ static long vhost_test_run(struct vhost_test *n, int
test)
>  		oldpriv = vq->private_data;
>  		vq->private_data = priv;
>  
> +		vhost_adjust_vring_endian(vq);
> +
>  		r = vhost_init_used(&n->vqs[index]);
>  
>  		mutex_unlock(&vq->mutex);
> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> index e02e06755ab7..b0a00340309e 100644
> --- a/drivers/vhost/vhost.c
> +++ b/drivers/vhost/vhost.c
> @@ -123,6 +123,15 @@ static void vhost_disable_is_le(struct vhost_virtqueue
*vq)
>  	vq->is_le = virtio_legacy_is_little_endian();
>  }
>  
> +void vhost_adjust_vring_endian(struct vhost_virtqueue *vq)
> +{
> +	if (!vq->private_data)
> +		vhost_disable_is_le(vq);
> +	else
> +		vhost_enable_is_le(vq);
> +}
> +EXPORT_SYMBOL_GPL(vhost_adjust_vring_endian);
> +
>  static void vhost_poll_func(struct file *file, wait_queue_head_t *wqh,
>  			    poll_table *pt)
>  {
I'd prefer "vhost_update_is_le" here. "endian" might
also mean
"user_be". But see below pls.

> @@ -1166,12 +1175,9 @@ int vhost_init_used(struct vhost_virtqueue *vq)
>  {
>  	__virtio16 last_used_idx;
>  	int r;
> -	if (!vq->private_data) {
> -		vhost_disable_is_le(vq);
> -		return 0;
> -	}
>  
> -	vhost_enable_is_le(vq);
> +	if (!vq->private_data)
> +		return 0;
>  
>  	r = vhost_update_used_flags(vq);
>  	if (r)
Looking at how callers use this, maybe we should just rename init_used
to vhost_vq_init_access. The _used suffix was a hint that we
access the vq used ring. But maybe what callers care about is
that it must be called after access_ok.
> diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h
> index d3f767448a72..88d86f45f756 100644
> --- a/drivers/vhost/vhost.h
> +++ b/drivers/vhost/vhost.h
> @@ -162,6 +162,7 @@ bool vhost_enable_notify(struct vhost_dev *, struct
vhost_virtqueue *);
>  
>  int vhost_log_write(struct vhost_virtqueue *vq, struct vhost_log *log,
>  		    unsigned int log_num, u64 len);
> +void vhost_adjust_vring_endian(struct vhost_virtqueue *vq);
>  
>  #define vq_err(vq, fmt, ...) do {                                  \
>  		pr_debug(pr_fmt(fmt), ##__VA_ARGS__);       \