All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/2] vhost: cross-endian code cleanup
@ 2016-01-13 17:09 Greg Kurz
  2016-01-13 17:09 ` [PATCH 1/2] vhost: helpers to enable/disable vring endianness Greg Kurz
                   ` (2 more replies)
  0 siblings, 3 replies; 22+ messages in thread
From: Greg Kurz @ 2016-01-13 17:09 UTC (permalink / raw)
  To: Michael S. Tsirkin; +Cc: netdev, linux-kernel, kvm, virtualization

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

^ permalink raw reply	[flat|nested] 22+ messages in thread
* [PATCH 1/2] vhost: helpers to enable/disable vring endianness
  2016-01-13 17:09 [PATCH 0/2] vhost: cross-endian code cleanup Greg Kurz
@ 2016-01-13 17:09 ` Greg Kurz
  2016-01-21  9:55     ` Cornelia Huck
  2016-02-10 11:21   ` Michael S. Tsirkin
  2016-01-13 17:09 ` [PATCH 2/2] vhost: disentangle vring endianness stuff from the core code Greg Kurz
  2016-01-27 10:25 ` [PATCH 0/2] vhost: cross-endian code cleanup Greg Kurz
  2 siblings, 2 replies; 22+ messages in thread
From: Greg Kurz @ 2016-01-13 17:09 UTC (permalink / raw)
  To: Michael S. Tsirkin; +Cc: netdev, linux-kernel, kvm, virtualization

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();
 }
 
+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)

^ permalink raw reply related	[flat|nested] 22+ messages in thread
* [PATCH 2/2] vhost: disentangle vring endianness stuff from the core code
  2016-01-13 17:09 [PATCH 0/2] vhost: cross-endian code cleanup Greg Kurz
  2016-01-13 17:09 ` [PATCH 1/2] vhost: helpers to enable/disable vring endianness Greg Kurz
@ 2016-01-13 17:09 ` Greg Kurz
  2016-01-21  9:56     ` Cornelia Huck
                     ` (2 more replies)
  2016-01-27 10:25 ` [PATCH 0/2] vhost: cross-endian code cleanup Greg Kurz
  2 siblings, 3 replies; 22+ messages in thread
From: Greg Kurz @ 2016-01-13 17:09 UTC (permalink / raw)
  To: Michael S. Tsirkin; +Cc: netdev, linux-kernel, kvm, virtualization

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@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__);       \

^ permalink raw reply related	[flat|nested] 22+ messages in thread
* Re: [PATCH 1/2] vhost: helpers to enable/disable vring endianness
  2016-01-13 17:09 ` [PATCH 1/2] vhost: helpers to enable/disable vring endianness Greg Kurz
@ 2016-01-21  9:55     ` Cornelia Huck
  2016-02-10 11:21   ` Michael S. Tsirkin
  1 sibling, 0 replies; 22+ messages in thread
From: Cornelia Huck @ 2016-01-21  9:55 UTC (permalink / raw)
  To: Greg Kurz; +Cc: Michael S. Tsirkin, netdev, linux-kernel, kvm, virtualization

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

Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com>

^ permalink raw reply	[flat|nested] 22+ messages in thread
* Re: [PATCH 1/2] vhost: helpers to enable/disable vring endianness
@ 2016-01-21  9:55     ` Cornelia Huck
  0 siblings, 0 replies; 22+ messages in thread
From: Cornelia Huck @ 2016-01-21  9:55 UTC (permalink / raw)
  To: Greg Kurz; +Cc: netdev, virtualization, linux-kernel, kvm, Michael S. Tsirkin

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

Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com>

^ permalink raw reply	[flat|nested] 22+ messages in thread
* Re: [PATCH 2/2] vhost: disentangle vring endianness stuff from the core code
  2016-01-13 17:09 ` [PATCH 2/2] vhost: disentangle vring endianness stuff from the core code Greg Kurz
@ 2016-01-21  9:56     ` Cornelia Huck
  2016-02-10 11:48   ` Michael S. Tsirkin
  2016-02-10 11:48   ` Michael S. Tsirkin
  2 siblings, 0 replies; 22+ messages in thread
From: Cornelia Huck @ 2016-01-21  9:56 UTC (permalink / raw)
  To: Greg Kurz; +Cc: Michael S. Tsirkin, netdev, linux-kernel, kvm, virtualization

On Wed, 13 Jan 2016 18:09:47 +0100
Greg Kurz <gkurz@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@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@de.ibm.com>

^ permalink raw reply	[flat|nested] 22+ messages in thread
* Re: [PATCH 2/2] vhost: disentangle vring endianness stuff from the core code
@ 2016-01-21  9:56     ` Cornelia Huck
  0 siblings, 0 replies; 22+ messages in thread
From: Cornelia Huck @ 2016-01-21  9:56 UTC (permalink / raw)
  To: Greg Kurz; +Cc: netdev, virtualization, linux-kernel, kvm, Michael S. Tsirkin

On Wed, 13 Jan 2016 18:09:47 +0100
Greg Kurz <gkurz@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@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@de.ibm.com>

^ permalink raw reply	[flat|nested] 22+ messages in thread
* Re: [PATCH 0/2] vhost: cross-endian code cleanup
  2016-01-13 17:09 [PATCH 0/2] vhost: cross-endian code cleanup Greg Kurz
  2016-01-13 17:09 ` [PATCH 1/2] vhost: helpers to enable/disable vring endianness Greg Kurz
  2016-01-13 17:09 ` [PATCH 2/2] vhost: disentangle vring endianness stuff from the core code Greg Kurz
@ 2016-01-27 10:25 ` Greg Kurz
  2 siblings, 0 replies; 22+ messages in thread
From: Greg Kurz @ 2016-01-27 10:25 UTC (permalink / raw)
  To: Michael S. Tsirkin; +Cc: netdev, linux-kernel, kvm, virtualization

On Wed, 13 Jan 2016 18:09:34 +0100
Greg Kurz <gkurz@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@lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/virtualization
> 

^ permalink raw reply	[flat|nested] 22+ messages in thread
* Re: [PATCH 1/2] vhost: helpers to enable/disable vring endianness
  2016-01-13 17:09 ` [PATCH 1/2] vhost: helpers to enable/disable vring endianness Greg Kurz
  2016-01-21  9:55     ` Cornelia Huck
@ 2016-02-10 11:21   ` Michael S. Tsirkin
  2016-02-10 12:11     ` Greg Kurz
  1 sibling, 1 reply; 22+ messages in thread
From: Michael S. Tsirkin @ 2016-02-10 11:21 UTC (permalink / raw)
  To: Greg Kurz; +Cc: netdev, linux-kernel, kvm, 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@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)

^ permalink raw reply	[flat|nested] 22+ messages in thread
* Re: [PATCH 2/2] vhost: disentangle vring endianness stuff from the core code
  2016-01-13 17:09 ` [PATCH 2/2] vhost: disentangle vring endianness stuff from the core code Greg Kurz
  2016-01-21  9:56     ` Cornelia Huck
@ 2016-02-10 11:48   ` Michael S. Tsirkin
  2016-02-10 13:08     ` Greg Kurz
  2016-02-10 11:48   ` Michael S. Tsirkin
  2 siblings, 1 reply; 22+ messages in thread
From: Michael S. Tsirkin @ 2016-02-10 11:48 UTC (permalink / raw)
  To: Greg Kurz; +Cc: netdev, linux-kernel, kvm, virtualization

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@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__);       \

^ permalink raw reply	[flat|nested] 22+ messages in thread
* Re: [PATCH 2/2] vhost: disentangle vring endianness stuff from the core code
  2016-01-13 17:09 ` [PATCH 2/2] vhost: disentangle vring endianness stuff from the core code Greg Kurz
  2016-01-21  9:56     ` Cornelia Huck
  2016-02-10 11:48   ` Michael S. Tsirkin
@ 2016-02-10 11:48   ` Michael S. Tsirkin
  2 siblings, 0 replies; 22+ messages in thread
From: Michael S. Tsirkin @ 2016-02-10 11:48 UTC (permalink / raw)
  To: Greg Kurz; +Cc: netdev, linux-kernel, kvm, virtualization

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@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__);       \

^ permalink raw reply	[flat|nested] 22+ messages in thread
* Re: [PATCH 1/2] vhost: helpers to enable/disable vring endianness
  2016-02-10 11:21   ` Michael S. Tsirkin
@ 2016-02-10 12:11     ` Greg Kurz
  2016-02-10 13:12         ` Cornelia Huck
  2016-02-10 15:08       ` Michael S. Tsirkin
  0 siblings, 2 replies; 22+ messages in thread
From: Greg Kurz @ 2016-02-10 12:11 UTC (permalink / raw)
  To: Michael S. Tsirkin; +Cc: netdev, linux-kernel, kvm, virtualization

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

^ permalink raw reply	[flat|nested] 22+ messages in thread
* Re: [PATCH 2/2] vhost: disentangle vring endianness stuff from the core code
  2016-02-10 11:48   ` Michael S. Tsirkin
@ 2016-02-10 13:08     ` Greg Kurz
  2016-02-10 13:23         ` Cornelia Huck
  0 siblings, 1 reply; 22+ messages in thread
From: Greg Kurz @ 2016-02-10 13:08 UTC (permalink / raw)
  To: Michael S. Tsirkin; +Cc: netdev, linux-kernel, kvm, virtualization

On Wed, 10 Feb 2016 13:48:09 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> 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@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.
> 

I'm afraid we can't because the following path needs the
vring endianness:

vhost_init_used()->vhost_update_used_flags()->cpu_to_vhost16()

But you are right, there is a bug: we should rollback if vhost_init_used()
fails. Something like below:

 err_used:
        vq->private_data = oldsock;
        vhost_net_enable_vq(n, vq);
+       vhost_adjust_vring_endian(vq);
        if (ubufs)
                vhost_net_ubuf_put_wait_and_free(ubufs);
 err_ubufs:

> > 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.
> 

Ok, I will rename if this patch survives the review.

> 
> > @@ -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.
> 

And so we would keep the current logic where it is up to the core
code to update is_le... that is basically getting rid of this patch :)

So IIUC you're asking for:
- fix the side-effect in vhost_init_used()
- rename vhost_init_used() to vhost_vq_init_access()

> > 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__);       \  
> 

^ permalink raw reply	[flat|nested] 22+ messages in thread
* Re: [PATCH 1/2] vhost: helpers to enable/disable vring endianness
  2016-02-10 12:11     ` Greg Kurz
@ 2016-02-10 13:12         ` Cornelia Huck
  2016-02-10 15:08       ` Michael S. Tsirkin
  1 sibling, 0 replies; 22+ messages in thread
From: Cornelia Huck @ 2016-02-10 13:12 UTC (permalink / raw)
  To: Greg Kurz; +Cc: Michael S. Tsirkin, netdev, linux-kernel, kvm, virtualization

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.

^ permalink raw reply	[flat|nested] 22+ messages in thread
* Re: [PATCH 1/2] vhost: helpers to enable/disable vring endianness
@ 2016-02-10 13:12         ` Cornelia Huck
  0 siblings, 0 replies; 22+ messages in thread
From: Cornelia Huck @ 2016-02-10 13:12 UTC (permalink / raw)
  To: Greg Kurz; +Cc: netdev, virtualization, linux-kernel, kvm, Michael S. Tsirkin

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.

^ permalink raw reply	[flat|nested] 22+ messages in thread
* Re: [PATCH 2/2] vhost: disentangle vring endianness stuff from the core code
  2016-02-10 13:08     ` Greg Kurz
@ 2016-02-10 13:23         ` Cornelia Huck
  0 siblings, 0 replies; 22+ messages in thread
From: Cornelia Huck @ 2016-02-10 13:23 UTC (permalink / raw)
  To: Greg Kurz; +Cc: Michael S. Tsirkin, netdev, linux-kernel, kvm, virtualization

On Wed, 10 Feb 2016 14:08:43 +0100
Greg Kurz <gkurz@linux.vnet.ibm.com> wrote:

> But you are right, there is a bug: we should rollback if vhost_init_used()
> fails. Something like below:
> 
>  err_used:
>         vq->private_data = oldsock;
>         vhost_net_enable_vq(n, vq);
> +       vhost_adjust_vring_endian(vq);

Shouldn't we switch back before we reenable? Or have I lost myself in
this maze here again?

>         if (ubufs)
>                 vhost_net_ubuf_put_wait_and_free(ubufs);
>  err_ubufs:

^ permalink raw reply	[flat|nested] 22+ messages in thread
* Re: [PATCH 2/2] vhost: disentangle vring endianness stuff from the core code
@ 2016-02-10 13:23         ` Cornelia Huck
  0 siblings, 0 replies; 22+ messages in thread
From: Cornelia Huck @ 2016-02-10 13:23 UTC (permalink / raw)
  To: Greg Kurz; +Cc: netdev, virtualization, linux-kernel, kvm, Michael S. Tsirkin

On Wed, 10 Feb 2016 14:08:43 +0100
Greg Kurz <gkurz@linux.vnet.ibm.com> wrote:

> But you are right, there is a bug: we should rollback if vhost_init_used()
> fails. Something like below:
> 
>  err_used:
>         vq->private_data = oldsock;
>         vhost_net_enable_vq(n, vq);
> +       vhost_adjust_vring_endian(vq);

Shouldn't we switch back before we reenable? Or have I lost myself in
this maze here again?

>         if (ubufs)
>                 vhost_net_ubuf_put_wait_and_free(ubufs);
>  err_ubufs:

^ permalink raw reply	[flat|nested] 22+ messages in thread
* Re: [PATCH 2/2] vhost: disentangle vring endianness stuff from the core code
  2016-02-10 13:23         ` Cornelia Huck
@ 2016-02-10 13:40           ` Greg Kurz
  -1 siblings, 0 replies; 22+ messages in thread
From: Greg Kurz @ 2016-02-10 13:40 UTC (permalink / raw)
  To: Cornelia Huck
  Cc: Michael S. Tsirkin, netdev, linux-kernel, kvm, virtualization

On Wed, 10 Feb 2016 14:23:33 +0100
Cornelia Huck <cornelia.huck@de.ibm.com> wrote:

> On Wed, 10 Feb 2016 14:08:43 +0100
> Greg Kurz <gkurz@linux.vnet.ibm.com> wrote:
> 
> > But you are right, there is a bug: we should rollback if vhost_init_used()
> > fails. Something like below:
> > 
> >  err_used:
> >         vq->private_data = oldsock;
> >         vhost_net_enable_vq(n, vq);
> > +       vhost_adjust_vring_endian(vq);  
> 
> Shouldn't we switch back before we reenable? Or have I lost myself in
> this maze here again?
> 

I haven't spotted any path under vhost_net_enable_vq() that needs
the vring endianness, but indeed it looks safer to switch back
before a worker thread may be woken up.

> >         if (ubufs)
> >                 vhost_net_ubuf_put_wait_and_free(ubufs);
> >  err_ubufs:  

^ permalink raw reply	[flat|nested] 22+ messages in thread
* Re: [PATCH 2/2] vhost: disentangle vring endianness stuff from the core code
@ 2016-02-10 13:40           ` Greg Kurz
  0 siblings, 0 replies; 22+ messages in thread
From: Greg Kurz @ 2016-02-10 13:40 UTC (permalink / raw)
  To: Cornelia Huck
  Cc: netdev, virtualization, linux-kernel, kvm, Michael S. Tsirkin

On Wed, 10 Feb 2016 14:23:33 +0100
Cornelia Huck <cornelia.huck@de.ibm.com> wrote:

> On Wed, 10 Feb 2016 14:08:43 +0100
> Greg Kurz <gkurz@linux.vnet.ibm.com> wrote:
> 
> > But you are right, there is a bug: we should rollback if vhost_init_used()
> > fails. Something like below:
> > 
> >  err_used:
> >         vq->private_data = oldsock;
> >         vhost_net_enable_vq(n, vq);
> > +       vhost_adjust_vring_endian(vq);  
> 
> Shouldn't we switch back before we reenable? Or have I lost myself in
> this maze here again?
> 

I haven't spotted any path under vhost_net_enable_vq() that needs
the vring endianness, but indeed it looks safer to switch back
before a worker thread may be woken up.

> >         if (ubufs)
> >                 vhost_net_ubuf_put_wait_and_free(ubufs);
> >  err_ubufs:  

^ permalink raw reply	[flat|nested] 22+ messages in thread
* Re: [PATCH 1/2] vhost: helpers to enable/disable vring endianness
  2016-02-10 12:11     ` Greg Kurz
  2016-02-10 13:12         ` Cornelia Huck
@ 2016-02-10 15:08       ` Michael S. Tsirkin
  2016-02-10 15:27           ` Greg Kurz
  1 sibling, 1 reply; 22+ messages in thread
From: Michael S. Tsirkin @ 2016-02-10 15:08 UTC (permalink / raw)
  To: Greg Kurz; +Cc: netdev, linux-kernel, kvm, virtualization

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

^ permalink raw reply	[flat|nested] 22+ messages in thread
* Re: [PATCH 1/2] vhost: helpers to enable/disable vring endianness
  2016-02-10 15:08       ` Michael S. Tsirkin
@ 2016-02-10 15:27           ` Greg Kurz
  0 siblings, 0 replies; 22+ messages in thread
From: Greg Kurz @ 2016-02-10 15:27 UTC (permalink / raw)
  To: Michael S. Tsirkin
  Cc: netdev, linux-kernel, kvm, virtualization, Cornelia Huck

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

^ permalink raw reply	[flat|nested] 22+ messages in thread
* Re: [PATCH 1/2] vhost: helpers to enable/disable vring endianness
@ 2016-02-10 15:27           ` Greg Kurz
  0 siblings, 0 replies; 22+ messages in thread
From: Greg Kurz @ 2016-02-10 15:27 UTC (permalink / raw)
  To: Michael S. Tsirkin; +Cc: netdev, linux-kernel, kvm, virtualization

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

^ permalink raw reply	[flat|nested] 22+ messages in thread
end of thread, other threads:[~2016-02-10 15:28 UTC | newest]

Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-01-13 17:09 [PATCH 0/2] vhost: cross-endian code cleanup Greg Kurz
2016-01-13 17:09 ` [PATCH 1/2] vhost: helpers to enable/disable vring endianness Greg Kurz
2016-01-21  9:55   ` Cornelia Huck
2016-01-21  9:55     ` Cornelia Huck
2016-02-10 11:21   ` Michael S. Tsirkin
2016-02-10 12:11     ` Greg Kurz
2016-02-10 13:12       ` Cornelia Huck
2016-02-10 13:12         ` Cornelia Huck
2016-02-10 15:08       ` Michael S. Tsirkin
2016-02-10 15:27         ` Greg Kurz
2016-02-10 15:27           ` Greg Kurz
2016-01-13 17:09 ` [PATCH 2/2] vhost: disentangle vring endianness stuff from the core code Greg Kurz
2016-01-21  9:56   ` Cornelia Huck
2016-01-21  9:56     ` Cornelia Huck
2016-02-10 11:48   ` Michael S. Tsirkin
2016-02-10 13:08     ` Greg Kurz
2016-02-10 13:23       ` Cornelia Huck
2016-02-10 13:23         ` Cornelia Huck
2016-02-10 13:40         ` Greg Kurz
2016-02-10 13:40           ` Greg Kurz
2016-02-10 11:48   ` Michael S. Tsirkin
2016-01-27 10:25 ` [PATCH 0/2] vhost: cross-endian code cleanup Greg Kurz

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.