[PATCH v3 0/7] vhost: support for cross endian guests

[PATCH v3 0/7] vhost: support for cross endian guests

[Greg Kurz]

Hi,

This patchset allows vhost to be used with legacy virtio when guest and host
have a different endianness.

Patches 1-6 remain the same as the previous post. Patch 7 was heavily changed
according to MST's comments.

---

Greg Kurz (7):
      virtio: introduce virtio_is_little_endian() helper
      tun: add tun_is_little_endian() helper
      macvtap: introduce macvtap_is_little_endian() helper
      vringh: introduce vringh_is_little_endian() helper
      vhost: introduce vhost_is_little_endian() helper
      virtio: add explicit big-endian support to memory accessors
      vhost: feature to set the vring endianness


 drivers/net/macvtap.c            |   11 ++++++--
 drivers/net/tun.c                |   11 ++++++--
 drivers/vhost/Kconfig            |   10 +++++++
 drivers/vhost/vhost.c            |   55 ++++++++++++++++++++++++++++++++++++++
 drivers/vhost/vhost.h            |   34 +++++++++++++++++++----
 include/linux/virtio_byteorder.h |   24 ++++++++++-------
 include/linux/virtio_config.h    |   19 +++++++++----
 include/linux/vringh.h           |   19 +++++++++----
 include/uapi/linux/vhost.h       |    5 +++
 9 files changed, 156 insertions(+), 32 deletions(-)

--
Greg

[PATCH v3 1/7] virtio: introduce virtio_is_little_endian() helper

[Greg Kurz]

Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
---
 include/linux/virtio_config.h |   17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/include/linux/virtio_config.h b/include/linux/virtio_config.h
index ca3ed78..bd1a582 100644
--- a/include/linux/virtio_config.h
+++ b/include/linux/virtio_config.h
@@ -205,35 +205,40 @@ int virtqueue_set_affinity(struct virtqueue *vq, int cpu)
 	return 0;
 }
 
+static inline bool virtio_is_little_endian(struct virtio_device *vdev)
+{
+	return virtio_has_feature(vdev, VIRTIO_F_VERSION_1);
+}
+
 /* Memory accessors */
 static inline u16 virtio16_to_cpu(struct virtio_device *vdev, __virtio16 val)
 {
-	return __virtio16_to_cpu(virtio_has_feature(vdev, VIRTIO_F_VERSION_1), val);
+	return __virtio16_to_cpu(virtio_is_little_endian(vdev), val);
 }
 
 static inline __virtio16 cpu_to_virtio16(struct virtio_device *vdev, u16 val)
 {
-	return __cpu_to_virtio16(virtio_has_feature(vdev, VIRTIO_F_VERSION_1), val);
+	return __cpu_to_virtio16(virtio_is_little_endian(vdev), val);
 }
 
 static inline u32 virtio32_to_cpu(struct virtio_device *vdev, __virtio32 val)
 {
-	return __virtio32_to_cpu(virtio_has_feature(vdev, VIRTIO_F_VERSION_1), val);
+	return __virtio32_to_cpu(virtio_is_little_endian(vdev), val);
 }
 
 static inline __virtio32 cpu_to_virtio32(struct virtio_device *vdev, u32 val)
 {
-	return __cpu_to_virtio32(virtio_has_feature(vdev, VIRTIO_F_VERSION_1), val);
+	return __cpu_to_virtio32(virtio_is_little_endian(vdev), val);
 }
 
 static inline u64 virtio64_to_cpu(struct virtio_device *vdev, __virtio64 val)
 {
-	return __virtio64_to_cpu(virtio_has_feature(vdev, VIRTIO_F_VERSION_1), val);
+	return __virtio64_to_cpu(virtio_is_little_endian(vdev), val);
 }
 
 static inline __virtio64 cpu_to_virtio64(struct virtio_device *vdev, u64 val)
 {
-	return __cpu_to_virtio64(virtio_has_feature(vdev, VIRTIO_F_VERSION_1), val);
+	return __cpu_to_virtio64(virtio_is_little_endian(vdev), val);
 }
 
 /* Config space accessors. */

[PATCH v3 2/7] tun: add tun_is_little_endian() helper

[Greg Kurz]

Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
---
 drivers/net/tun.c |    9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index 857dca4..3c3d6c0 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -206,14 +206,19 @@ struct tun_struct {
 	u32 flow_count;
 };
 
+static inline bool tun_is_little_endian(struct tun_struct *tun)
+{
+	return tun->flags & TUN_VNET_LE;
+}
+
 static inline u16 tun16_to_cpu(struct tun_struct *tun, __virtio16 val)
 {
-	return __virtio16_to_cpu(tun->flags & TUN_VNET_LE, val);
+	return __virtio16_to_cpu(tun_is_little_endian(tun), val);
 }
 
 static inline __virtio16 cpu_to_tun16(struct tun_struct *tun, u16 val)
 {
-	return __cpu_to_virtio16(tun->flags & TUN_VNET_LE, val);
+	return __cpu_to_virtio16(tun_is_little_endian(tun), val);
 }
 
 static inline u32 tun_hashfn(u32 rxhash)

[PATCH v3 3/7] macvtap: introduce macvtap_is_little_endian() helper

[Greg Kurz]

Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
---
 drivers/net/macvtap.c |    9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/net/macvtap.c b/drivers/net/macvtap.c
index 27ecc5c..a2f2958 100644
--- a/drivers/net/macvtap.c
+++ b/drivers/net/macvtap.c
@@ -49,14 +49,19 @@ struct macvtap_queue {
 
 #define MACVTAP_VNET_LE 0x80000000
 
+static inline bool macvtap_is_little_endian(struct macvtap_queue *q)
+{
+	return q->flags & MACVTAP_VNET_LE;
+}
+
 static inline u16 macvtap16_to_cpu(struct macvtap_queue *q, __virtio16 val)
 {
-	return __virtio16_to_cpu(q->flags & MACVTAP_VNET_LE, val);
+	return __virtio16_to_cpu(macvtap_is_little_endian(q), val);
 }
 
 static inline __virtio16 cpu_to_macvtap16(struct macvtap_queue *q, u16 val)
 {
-	return __cpu_to_virtio16(q->flags & MACVTAP_VNET_LE, val);
+	return __cpu_to_virtio16(macvtap_is_little_endian(q), val);
 }
 
 static struct proto macvtap_proto = {

[PATCH v3 4/7] vringh: introduce vringh_is_little_endian() helper

[Greg Kurz]

Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
---
 include/linux/vringh.h |   17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/include/linux/vringh.h b/include/linux/vringh.h
index a3fa537..3ed62ef 100644
--- a/include/linux/vringh.h
+++ b/include/linux/vringh.h
@@ -226,33 +226,38 @@ static inline void vringh_notify(struct vringh *vrh)
 		vrh->notify(vrh);
 }
 
+static inline bool vringh_is_little_endian(const struct vringh *vrh)
+{
+	return vrh->little_endian;
+}
+
 static inline u16 vringh16_to_cpu(const struct vringh *vrh, __virtio16 val)
 {
-	return __virtio16_to_cpu(vrh->little_endian, val);
+	return __virtio16_to_cpu(vringh_is_little_endian(vrh), val);
 }
 
 static inline __virtio16 cpu_to_vringh16(const struct vringh *vrh, u16 val)
 {
-	return __cpu_to_virtio16(vrh->little_endian, val);
+	return __cpu_to_virtio16(vringh_is_little_endian(vrh), val);
 }
 
 static inline u32 vringh32_to_cpu(const struct vringh *vrh, __virtio32 val)
 {
-	return __virtio32_to_cpu(vrh->little_endian, val);
+	return __virtio32_to_cpu(vringh_is_little_endian(vrh), val);
 }
 
 static inline __virtio32 cpu_to_vringh32(const struct vringh *vrh, u32 val)
 {
-	return __cpu_to_virtio32(vrh->little_endian, val);
+	return __cpu_to_virtio32(vringh_is_little_endian(vrh), val);
 }
 
 static inline u64 vringh64_to_cpu(const struct vringh *vrh, __virtio64 val)
 {
-	return __virtio64_to_cpu(vrh->little_endian, val);
+	return __virtio64_to_cpu(vringh_is_little_endian(vrh), val);
 }
 
 static inline __virtio64 cpu_to_vringh64(const struct vringh *vrh, u64 val)
 {
-	return __cpu_to_virtio64(vrh->little_endian, val);
+	return __cpu_to_virtio64(vringh_is_little_endian(vrh), val);
 }
 #endif /* _LINUX_VRINGH_H */

[PATCH v3 5/7] vhost: introduce vhost_is_little_endian() helper

[Greg Kurz]

Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
---
 drivers/vhost/vhost.h |   17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h
index 8c1c792..6a49960 100644
--- a/drivers/vhost/vhost.h
+++ b/drivers/vhost/vhost.h
@@ -173,34 +173,39 @@ static inline bool vhost_has_feature(struct vhost_virtqueue *vq, int bit)
 	return vq->acked_features & (1ULL << bit);
 }
 
+static inline bool vhost_is_little_endian(struct vhost_virtqueue *vq)
+{
+	return vhost_has_feature(vq, VIRTIO_F_VERSION_1);
+}
+
 /* Memory accessors */
 static inline u16 vhost16_to_cpu(struct vhost_virtqueue *vq, __virtio16 val)
 {
-	return __virtio16_to_cpu(vhost_has_feature(vq, VIRTIO_F_VERSION_1), val);
+	return __virtio16_to_cpu(vhost_is_little_endian(vq), val);
 }
 
 static inline __virtio16 cpu_to_vhost16(struct vhost_virtqueue *vq, u16 val)
 {
-	return __cpu_to_virtio16(vhost_has_feature(vq, VIRTIO_F_VERSION_1), val);
+	return __cpu_to_virtio16(vhost_is_little_endian(vq), val);
 }
 
 static inline u32 vhost32_to_cpu(struct vhost_virtqueue *vq, __virtio32 val)
 {
-	return __virtio32_to_cpu(vhost_has_feature(vq, VIRTIO_F_VERSION_1), val);
+	return __virtio32_to_cpu(vhost_is_little_endian(vq), val);
 }
 
 static inline __virtio32 cpu_to_vhost32(struct vhost_virtqueue *vq, u32 val)
 {
-	return __cpu_to_virtio32(vhost_has_feature(vq, VIRTIO_F_VERSION_1), val);
+	return __cpu_to_virtio32(vhost_is_little_endian(vq), val);
 }
 
 static inline u64 vhost64_to_cpu(struct vhost_virtqueue *vq, __virtio64 val)
 {
-	return __virtio64_to_cpu(vhost_has_feature(vq, VIRTIO_F_VERSION_1), val);
+	return __virtio64_to_cpu(vhost_is_little_endian(vq), val);
 }
 
 static inline __virtio64 cpu_to_vhost64(struct vhost_virtqueue *vq, u64 val)
 {
-	return __cpu_to_virtio64(vhost_has_feature(vq, VIRTIO_F_VERSION_1), val);
+	return __cpu_to_virtio64(vhost_is_little_endian(vq), val);
 }
 #endif

[PATCH v3 6/7] virtio: add explicit big-endian support to memory accessors

[Greg Kurz]

The current memory accessors logic is:
- little endian if little_endian
- native endian (i.e. no byteswap) if !little_endian

If we want to fully support cross-endian vhost, we also need to be
able to convert to big endian.

Instead of changing the little_endian argument to some 3-value enum, this
patch changes the logic to:
- little endian if little_endian
- big endian if !little_endian

The native endian case is handled by all users with a trivial helper. This
patch doesn't change any functionality, nor it does add overhead.

Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
---
 drivers/net/macvtap.c            |    4 +++-
 drivers/net/tun.c                |    4 +++-
 drivers/vhost/vhost.h            |    4 +++-
 include/linux/virtio_byteorder.h |   24 ++++++++++++++----------
 include/linux/virtio_config.h    |    4 +++-
 include/linux/vringh.h           |    4 +++-
 6 files changed, 29 insertions(+), 15 deletions(-)

diff --git a/drivers/net/macvtap.c b/drivers/net/macvtap.c
index a2f2958..0a03a66 100644
--- a/drivers/net/macvtap.c
+++ b/drivers/net/macvtap.c
@@ -51,7 +51,9 @@ struct macvtap_queue {
 
 static inline bool macvtap_is_little_endian(struct macvtap_queue *q)
 {
-	return q->flags & MACVTAP_VNET_LE;
+	if (q->flags & MACVTAP_VNET_LE)
+		return true;
+	return virtio_legacy_is_little_endian();
 }
 
 static inline u16 macvtap16_to_cpu(struct macvtap_queue *q, __virtio16 val)
diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index 3c3d6c0..053f9b6 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -208,7 +208,9 @@ struct tun_struct {
 
 static inline bool tun_is_little_endian(struct tun_struct *tun)
 {
-	return tun->flags & TUN_VNET_LE;
+	if (tun->flags & TUN_VNET_LE)
+		return true;
+	return virtio_legacy_is_little_endian();
 }
 
 static inline u16 tun16_to_cpu(struct tun_struct *tun, __virtio16 val)
diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h
index 6a49960..4e9a186 100644
--- a/drivers/vhost/vhost.h
+++ b/drivers/vhost/vhost.h
@@ -175,7 +175,9 @@ static inline bool vhost_has_feature(struct vhost_virtqueue *vq, int bit)
 
 static inline bool vhost_is_little_endian(struct vhost_virtqueue *vq)
 {
-	return vhost_has_feature(vq, VIRTIO_F_VERSION_1);
+	if (vhost_has_feature(vq, VIRTIO_F_VERSION_1))
+		return true;
+	return virtio_legacy_is_little_endian();
 }
 
 /* Memory accessors */
diff --git a/include/linux/virtio_byteorder.h b/include/linux/virtio_byteorder.h
index 51865d0..ce63a2c 100644
--- a/include/linux/virtio_byteorder.h
+++ b/include/linux/virtio_byteorder.h
@@ -3,17 +3,21 @@
 #include <linux/types.h>
 #include <uapi/linux/virtio_types.h>
 
-/*
- * Low-level memory accessors for handling virtio in modern little endian and in
- * compatibility native endian format.
- */
+static inline bool virtio_legacy_is_little_endian(void)
+{
+#ifdef __LITTLE_ENDIAN
+	return true;
+#else
+	return false;
+#endif
+}
 
 static inline u16 __virtio16_to_cpu(bool little_endian, __virtio16 val)
 {
 	if (little_endian)
 		return le16_to_cpu((__force __le16)val);
 	else
-		return (__force u16)val;
+		return be16_to_cpu((__force __be16)val);
 }
 
 static inline __virtio16 __cpu_to_virtio16(bool little_endian, u16 val)
@@ -21,7 +25,7 @@ static inline __virtio16 __cpu_to_virtio16(bool little_endian, u16 val)
 	if (little_endian)
 		return (__force __virtio16)cpu_to_le16(val);
 	else
-		return (__force __virtio16)val;
+		return (__force __virtio16)cpu_to_be16(val);
 }
 
 static inline u32 __virtio32_to_cpu(bool little_endian, __virtio32 val)
@@ -29,7 +33,7 @@ static inline u32 __virtio32_to_cpu(bool little_endian, __virtio32 val)
 	if (little_endian)
 		return le32_to_cpu((__force __le32)val);
 	else
-		return (__force u32)val;
+		return be32_to_cpu((__force __be32)val);
 }
 
 static inline __virtio32 __cpu_to_virtio32(bool little_endian, u32 val)
@@ -37,7 +41,7 @@ static inline __virtio32 __cpu_to_virtio32(bool little_endian, u32 val)
 	if (little_endian)
 		return (__force __virtio32)cpu_to_le32(val);
 	else
-		return (__force __virtio32)val;
+		return (__force __virtio32)cpu_to_be32(val);
 }
 
 static inline u64 __virtio64_to_cpu(bool little_endian, __virtio64 val)
@@ -45,7 +49,7 @@ static inline u64 __virtio64_to_cpu(bool little_endian, __virtio64 val)
 	if (little_endian)
 		return le64_to_cpu((__force __le64)val);
 	else
-		return (__force u64)val;
+		return be64_to_cpu((__force __be64)val);
 }
 
 static inline __virtio64 __cpu_to_virtio64(bool little_endian, u64 val)
@@ -53,7 +57,7 @@ static inline __virtio64 __cpu_to_virtio64(bool little_endian, u64 val)
 	if (little_endian)
 		return (__force __virtio64)cpu_to_le64(val);
 	else
-		return (__force __virtio64)val;
+		return (__force __virtio64)cpu_to_be64(val);
 }
 
 #endif /* _LINUX_VIRTIO_BYTEORDER */
diff --git a/include/linux/virtio_config.h b/include/linux/virtio_config.h
index bd1a582..36a6daa 100644
--- a/include/linux/virtio_config.h
+++ b/include/linux/virtio_config.h
@@ -207,7 +207,9 @@ int virtqueue_set_affinity(struct virtqueue *vq, int cpu)
 
 static inline bool virtio_is_little_endian(struct virtio_device *vdev)
 {
-	return virtio_has_feature(vdev, VIRTIO_F_VERSION_1);
+	if (virtio_has_feature(vdev, VIRTIO_F_VERSION_1))
+		return true;
+	return virtio_legacy_is_little_endian();
 }
 
 /* Memory accessors */
diff --git a/include/linux/vringh.h b/include/linux/vringh.h
index 3ed62ef..d786c2d 100644
--- a/include/linux/vringh.h
+++ b/include/linux/vringh.h
@@ -228,7 +228,9 @@ static inline void vringh_notify(struct vringh *vrh)
 
 static inline bool vringh_is_little_endian(const struct vringh *vrh)
 {
-	return vrh->little_endian;
+	if (vrh->little_endian)
+		return true;
+	return virtio_legacy_is_little_endian();
 }
 
 static inline u16 vringh16_to_cpu(const struct vringh *vrh, __virtio16 val)

[PATCH v3 7/7] vhost: feature to set the vring endianness

[Greg Kurz]

This patch brings cross-endian support to vhost when used to implement
legacy virtio devices. Since it is a relatively rare situation, the
feature availability is controlled by a kernel config option (not set
by default).

The ioctls introduced by this patch are for legacy only: virtio 1.0
devices are returned EPERM.

Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
---
 drivers/vhost/Kconfig      |   10 ++++++++
 drivers/vhost/vhost.c      |   55 ++++++++++++++++++++++++++++++++++++++++++++
 drivers/vhost/vhost.h      |   17 +++++++++++++-
 include/uapi/linux/vhost.h |    5 ++++
 4 files changed, 86 insertions(+), 1 deletion(-)

Changes since v2:
- fixed typos in Kconfig description
- renamed vq->legacy_big_endian to vq->legacy_is_little_endian
- vq->legacy_is_little_endian reset to default in vhost_vq_reset()
- dropped VHOST_F_SET_ENDIAN_LEGACY feature
- dropped struct vhost_vring_endian from the user API (re-use
  struct vhost_vring_state instead)
- added VHOST_GET_VRING_ENDIAN_LEGACY ioctl
- introduced more helpers and stubs to avoid polluting the code with ifdefs


diff --git a/drivers/vhost/Kconfig b/drivers/vhost/Kconfig
index 017a1e8..0aec88c 100644
--- a/drivers/vhost/Kconfig
+++ b/drivers/vhost/Kconfig
@@ -32,3 +32,13 @@ config VHOST
 	---help---
 	  This option is selected by any driver which needs to access
 	  the core of vhost.
+
+config VHOST_SET_ENDIAN_LEGACY
+	bool "Cross-endian support for host kernel accelerator"
+	default n
+	---help---
+	  This option allows vhost to support guests with a different byte
+	  ordering from host. It is disabled by default since it adds overhead
+	  and it is only needed by a few platforms (powerpc and arm).
+
+	  If unsure, say "N".
diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index 2ee2826..3529a3c 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -199,6 +199,7 @@ static void vhost_vq_reset(struct vhost_dev *dev,
 	vq->call = NULL;
 	vq->log_ctx = NULL;
 	vq->memory = NULL;
+	vq->legacy_is_little_endian = virtio_legacy_is_little_endian();
 }
 
 static int vhost_worker(void *data)
@@ -630,6 +631,54 @@ static long vhost_set_memory(struct vhost_dev *d, struct vhost_memory __user *m)
 	return 0;
 }
 
+#ifdef CONFIG_VHOST_SET_ENDIAN_LEGACY
+static long vhost_set_vring_endian_legacy(struct vhost_virtqueue *vq,
+					  void __user *argp)
+{
+	struct vhost_vring_state s;
+
+	if (vhost_has_feature(vq, VIRTIO_F_VERSION_1))
+		return -EPERM;
+
+	if (copy_from_user(&s, argp, sizeof(s)))
+		return -EFAULT;
+
+	vq->legacy_is_little_endian = !!s.num;
+	return 0;
+}
+
+static long vhost_get_vring_endian_legacy(struct vhost_virtqueue *vq,
+					  u32 idx,
+					  void __user *argp)
+{
+	struct vhost_vring_state s = {
+		.index = idx,
+		.num = vq->legacy_is_little_endian
+	};
+
+	if (vhost_has_feature(vq, VIRTIO_F_VERSION_1))
+		return -EPERM;
+
+	if (copy_to_user(argp, &s, sizeof(s)))
+		return -EFAULT;
+
+	return 0;
+}
+#else
+static long vhost_set_vring_endian_legacy(struct vhost_virtqueue *vq,
+					  void __user *argp)
+{
+	return 0;
+}
+
+static long vhost_get_vring_endian_legacy(struct vhost_virtqueue *vq,
+					  u32 idx,
+					  void __user *argp)
+{
+	return 0;
+}
+#endif /* CONFIG_VHOST_SET_ENDIAN_LEGACY */
+
 long vhost_vring_ioctl(struct vhost_dev *d, int ioctl, void __user *argp)
 {
 	struct file *eventfp, *filep = NULL;
@@ -806,6 +855,12 @@ long vhost_vring_ioctl(struct vhost_dev *d, int ioctl, void __user *argp)
 		} else
 			filep = eventfp;
 		break;
+	case VHOST_SET_VRING_ENDIAN_LEGACY:
+		r = vhost_set_vring_endian_legacy(vq, argp);
+		break;
+	case VHOST_GET_VRING_ENDIAN_LEGACY:
+		r = vhost_get_vring_endian_legacy(vq, idx, argp);
+		break;
 	default:
 		r = -ENOIOCTLCMD;
 	}
diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h
index 4e9a186..981ba06 100644
--- a/drivers/vhost/vhost.h
+++ b/drivers/vhost/vhost.h
@@ -106,6 +106,9 @@ struct vhost_virtqueue {
 	/* Log write descriptors */
 	void __user *log_base;
 	struct vhost_log *log;
+
+	/* We need to know the device endianness with legacy virtio. */
+	bool legacy_is_little_endian;
 };
 
 struct vhost_dev {
@@ -173,11 +176,23 @@ static inline bool vhost_has_feature(struct vhost_virtqueue *vq, int bit)
 	return vq->acked_features & (1ULL << bit);
 }
 
+#ifdef CONFIG_VHOST_SET_ENDIAN_LEGACY
+static inline bool vhost_legacy_is_little_endian(struct vhost_virtqueue *vq)
+{
+	return vq->legacy_is_little_endian;
+}
+#else
+static inline bool vhost_legacy_is_little_endian(struct vhost_virtqueue *vq)
+{
+	return virtio_legacy_is_little_endian();
+}
+#endif
+
 static inline bool vhost_is_little_endian(struct vhost_virtqueue *vq)
 {
 	if (vhost_has_feature(vq, VIRTIO_F_VERSION_1))
 		return true;
-	return virtio_legacy_is_little_endian();
+	return vhost_legacy_is_little_endian(vq);
 }
 
 /* Memory accessors */
diff --git a/include/uapi/linux/vhost.h b/include/uapi/linux/vhost.h
index bb6a5b4..1b01a72 100644
--- a/include/uapi/linux/vhost.h
+++ b/include/uapi/linux/vhost.h
@@ -103,6 +103,11 @@ struct vhost_memory {
 /* Get accessor: reads index, writes value in num */
 #define VHOST_GET_VRING_BASE _IOWR(VHOST_VIRTIO, 0x12, struct vhost_vring_state)
 
+/* Set endianness for the ring (legacy virtio only) */
+/* num is 0 for big endian, other values mean little endian */
+#define VHOST_SET_VRING_ENDIAN_LEGACY _IOW(VHOST_VIRTIO, 0x13, struct vhost_vring_state)
+#define VHOST_GET_VRING_ENDIAN_LEGACY _IOW(VHOST_VIRTIO, 0x14, struct vhost_vring_state)
+
 /* The following ioctls use eventfd file descriptors to signal and poll
  * for events. */
 

Re: [PATCH v3 7/7] vhost: feature to set the vring endianness

[Cornelia Huck]

On Tue, 07 Apr 2015 14:19:31 +0200
Greg Kurz <gkurz@linux.vnet.ibm.com> wrote:

> This patch brings cross-endian support to vhost when used to implement
> legacy virtio devices. Since it is a relatively rare situation, the
> feature availability is controlled by a kernel config option (not set
> by default).
> 
> The ioctls introduced by this patch are for legacy only: virtio 1.0
> devices are returned EPERM.
> 
> Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> ---
>  drivers/vhost/Kconfig      |   10 ++++++++
>  drivers/vhost/vhost.c      |   55 ++++++++++++++++++++++++++++++++++++++++++++
>  drivers/vhost/vhost.h      |   17 +++++++++++++-
>  include/uapi/linux/vhost.h |    5 ++++
>  4 files changed, 86 insertions(+), 1 deletion(-)

> +#ifdef CONFIG_VHOST_SET_ENDIAN_LEGACY
> +static long vhost_set_vring_endian_legacy(struct vhost_virtqueue *vq,
> +					  void __user *argp)
> +{
> +	struct vhost_vring_state s;
> +
> +	if (vhost_has_feature(vq, VIRTIO_F_VERSION_1))
> +		return -EPERM;
> +
> +	if (copy_from_user(&s, argp, sizeof(s)))
> +		return -EFAULT;
> +
> +	vq->legacy_is_little_endian = !!s.num;
> +	return 0;
> +}
> +
> +static long vhost_get_vring_endian_legacy(struct vhost_virtqueue *vq,
> +					  u32 idx,
> +					  void __user *argp)
> +{
> +	struct vhost_vring_state s = {
> +		.index = idx,
> +		.num = vq->legacy_is_little_endian
> +	};
> +
> +	if (vhost_has_feature(vq, VIRTIO_F_VERSION_1))
> +		return -EPERM;
> +
> +	if (copy_to_user(argp, &s, sizeof(s)))
> +		return -EFAULT;
> +
> +	return 0;
> +}
> +#else
> +static long vhost_set_vring_endian_legacy(struct vhost_virtqueue *vq,
> +					  void __user *argp)
> +{
> +	return 0;

I'm wondering whether this handler should return an error if the
feature is not configured for the kernel? How can the userspace caller
find out whether it has successfully prompted the kernel to handle the
endianness correctly?

> +}
> +
> +static long vhost_get_vring_endian_legacy(struct vhost_virtqueue *vq,
> +					  u32 idx,
> +					  void __user *argp)
> +{
> +	return 0;
> +}
> +#endif /* CONFIG_VHOST_SET_ENDIAN_LEGACY */

Re: [PATCH v3 7/7] vhost: feature to set the vring endianness

[Michael S. Tsirkin]

On Tue, Apr 07, 2015 at 02:19:31PM +0200, Greg Kurz wrote:
> This patch brings cross-endian support to vhost when used to implement
> legacy virtio devices. Since it is a relatively rare situation, the
> feature availability is controlled by a kernel config option (not set
> by default).
> 
> The ioctls introduced by this patch are for legacy only: virtio 1.0
> devices are returned EPERM.
> 
> Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>

EINVAL probably makes more sense?

> ---
>  drivers/vhost/Kconfig      |   10 ++++++++
>  drivers/vhost/vhost.c      |   55 ++++++++++++++++++++++++++++++++++++++++++++
>  drivers/vhost/vhost.h      |   17 +++++++++++++-
>  include/uapi/linux/vhost.h |    5 ++++
>  4 files changed, 86 insertions(+), 1 deletion(-)
> 
> Changes since v2:
> - fixed typos in Kconfig description
> - renamed vq->legacy_big_endian to vq->legacy_is_little_endian
> - vq->legacy_is_little_endian reset to default in vhost_vq_reset()
> - dropped VHOST_F_SET_ENDIAN_LEGACY feature
> - dropped struct vhost_vring_endian from the user API (re-use
>   struct vhost_vring_state instead)
> - added VHOST_GET_VRING_ENDIAN_LEGACY ioctl
> - introduced more helpers and stubs to avoid polluting the code with ifdefs
> 
> 
> diff --git a/drivers/vhost/Kconfig b/drivers/vhost/Kconfig
> index 017a1e8..0aec88c 100644
> --- a/drivers/vhost/Kconfig
> +++ b/drivers/vhost/Kconfig
> @@ -32,3 +32,13 @@ config VHOST
>  	---help---
>  	  This option is selected by any driver which needs to access
>  	  the core of vhost.
> +
> +config VHOST_SET_ENDIAN_LEGACY
> +	bool "Cross-endian support for host kernel accelerator"
> +	default n
> +	---help---
> +	  This option allows vhost to support guests with a different byte
> +	  ordering from host. It is disabled by default since it adds overhead
> +	  and it is only needed by a few platforms (powerpc and arm).
> +
> +	  If unsure, say "N".
> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> index 2ee2826..3529a3c 100644
> --- a/drivers/vhost/vhost.c
> +++ b/drivers/vhost/vhost.c
> @@ -199,6 +199,7 @@ static void vhost_vq_reset(struct vhost_dev *dev,
>  	vq->call = NULL;
>  	vq->log_ctx = NULL;
>  	vq->memory = NULL;
> +	vq->legacy_is_little_endian = virtio_legacy_is_little_endian();
>  }
>  
>  static int vhost_worker(void *data)
> @@ -630,6 +631,54 @@ static long vhost_set_memory(struct vhost_dev *d, struct vhost_memory __user *m)
>  	return 0;
>  }
>  
> +#ifdef CONFIG_VHOST_SET_ENDIAN_LEGACY
> +static long vhost_set_vring_endian_legacy(struct vhost_virtqueue *vq,
> +					  void __user *argp)
> +{
> +	struct vhost_vring_state s;
> +
> +	if (vhost_has_feature(vq, VIRTIO_F_VERSION_1))
> +		return -EPERM;

EINVAL probably makes more sense? But I'm not sure this
is helpful: one can set VIRTIO_F_VERSION_1 afterwards,
and your patch does not seem to detect this.



> +
> +	if (copy_from_user(&s, argp, sizeof(s)))
> +		return -EFAULT;
> +
> +	vq->legacy_is_little_endian = !!s.num;
> +	return 0;
> +}
> +
> +static long vhost_get_vring_endian_legacy(struct vhost_virtqueue *vq,
> +					  u32 idx,
> +					  void __user *argp)
> +{
> +	struct vhost_vring_state s = {
> +		.index = idx,
> +		.num = vq->legacy_is_little_endian
> +	};
> +
> +	if (vhost_has_feature(vq, VIRTIO_F_VERSION_1))
> +		return -EPERM;
> +
> +	if (copy_to_user(argp, &s, sizeof(s)))
> +		return -EFAULT;
> +
> +	return 0;
> +}
> +#else
> +static long vhost_set_vring_endian_legacy(struct vhost_virtqueue *vq,
> +					  void __user *argp)
> +{
> +	return 0;
> +}
> +
> +static long vhost_get_vring_endian_legacy(struct vhost_virtqueue *vq,
> +					  u32 idx,
> +					  void __user *argp)
> +{
> +	return 0;
> +}

Should be -ENOIOCTLCMD?

> +#endif /* CONFIG_VHOST_SET_ENDIAN_LEGACY */
> +
>  long vhost_vring_ioctl(struct vhost_dev *d, int ioctl, void __user *argp)
>  {
>  	struct file *eventfp, *filep = NULL;
> @@ -806,6 +855,12 @@ long vhost_vring_ioctl(struct vhost_dev *d, int ioctl, void __user *argp)
>  		} else
>  			filep = eventfp;
>  		break;
> +	case VHOST_SET_VRING_ENDIAN_LEGACY:
> +		r = vhost_set_vring_endian_legacy(vq, argp);
> +		break;
> +	case VHOST_GET_VRING_ENDIAN_LEGACY:
> +		r = vhost_get_vring_endian_legacy(vq, idx, argp);
> +		break;
>  	default:
>  		r = -ENOIOCTLCMD;
>  	}

I think we also want to forbid this with a running backend.

> diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h
> index 4e9a186..981ba06 100644
> --- a/drivers/vhost/vhost.h
> +++ b/drivers/vhost/vhost.h
> @@ -106,6 +106,9 @@ struct vhost_virtqueue {
>  	/* Log write descriptors */
>  	void __user *log_base;
>  	struct vhost_log *log;
> +
> +	/* We need to know the device endianness with legacy virtio. */
> +	bool legacy_is_little_endian;
>  };
>  
>  struct vhost_dev {
> @@ -173,11 +176,23 @@ static inline bool vhost_has_feature(struct vhost_virtqueue *vq, int bit)
>  	return vq->acked_features & (1ULL << bit);
>  }
>  
> +#ifdef CONFIG_VHOST_SET_ENDIAN_LEGACY
> +static inline bool vhost_legacy_is_little_endian(struct vhost_virtqueue *vq)
> +{
> +	return vq->legacy_is_little_endian;
> +}
> +#else
> +static inline bool vhost_legacy_is_little_endian(struct vhost_virtqueue *vq)
> +{
> +	return virtio_legacy_is_little_endian();
> +}
> +#endif
> +
>  static inline bool vhost_is_little_endian(struct vhost_virtqueue *vq)
>  {
>  	if (vhost_has_feature(vq, VIRTIO_F_VERSION_1))
>  		return true;
> -	return virtio_legacy_is_little_endian();
> +	return vhost_legacy_is_little_endian(vq);
>  }
>  
>  /* Memory accessors */
> diff --git a/include/uapi/linux/vhost.h b/include/uapi/linux/vhost.h
> index bb6a5b4..1b01a72 100644
> --- a/include/uapi/linux/vhost.h
> +++ b/include/uapi/linux/vhost.h
> @@ -103,6 +103,11 @@ struct vhost_memory {
>  /* Get accessor: reads index, writes value in num */
>  #define VHOST_GET_VRING_BASE _IOWR(VHOST_VIRTIO, 0x12, struct vhost_vring_state)
>  
> +/* Set endianness for the ring (legacy virtio only) */
> +/* num is 0 for big endian, other values mean little endian */

I'd prefer 0 and 1, return EINVAL on other values.

> +#define VHOST_SET_VRING_ENDIAN_LEGACY _IOW(VHOST_VIRTIO, 0x13, struct vhost_vring_state)
> +#define VHOST_GET_VRING_ENDIAN_LEGACY _IOW(VHOST_VIRTIO, 0x14, struct vhost_vring_state)
> +
>  /* The following ioctls use eventfd file descriptors to signal and poll
>   * for events. */
>  

Re: [PATCH v3 0/7] vhost: support for cross endian guests

[Michael S. Tsirkin]

On Tue, Apr 07, 2015 at 02:09:29PM +0200, Greg Kurz wrote:
> Hi,
> 
> This patchset allows vhost to be used with legacy virtio when guest and host
> have a different endianness.
> 
> Patches 1-6 remain the same as the previous post. Patch 7 was heavily changed
> according to MST's comments.

This still doesn't actually work, right?
tun and macvtap need new ioctls too ...

> ---
> 
> Greg Kurz (7):
>       virtio: introduce virtio_is_little_endian() helper
>       tun: add tun_is_little_endian() helper
>       macvtap: introduce macvtap_is_little_endian() helper
>       vringh: introduce vringh_is_little_endian() helper
>       vhost: introduce vhost_is_little_endian() helper
>       virtio: add explicit big-endian support to memory accessors
>       vhost: feature to set the vring endianness
> 
> 
>  drivers/net/macvtap.c            |   11 ++++++--
>  drivers/net/tun.c                |   11 ++++++--
>  drivers/vhost/Kconfig            |   10 +++++++
>  drivers/vhost/vhost.c            |   55 ++++++++++++++++++++++++++++++++++++++
>  drivers/vhost/vhost.h            |   34 +++++++++++++++++++----
>  include/linux/virtio_byteorder.h |   24 ++++++++++-------
>  include/linux/virtio_config.h    |   19 +++++++++----
>  include/linux/vringh.h           |   19 +++++++++----
>  include/uapi/linux/vhost.h       |    5 +++
>  9 files changed, 156 insertions(+), 32 deletions(-)
> 
> --
> Greg

Re: [PATCH v3 6/7] virtio: add explicit big-endian support to memory accessors

[Michael S. Tsirkin]

On Tue, Apr 07, 2015 at 02:15:52PM +0200, Greg Kurz wrote:
> The current memory accessors logic is:
> - little endian if little_endian
> - native endian (i.e. no byteswap) if !little_endian
> 
> If we want to fully support cross-endian vhost, we also need to be
> able to convert to big endian.
> 
> Instead of changing the little_endian argument to some 3-value enum, this
> patch changes the logic to:
> - little endian if little_endian
> - big endian if !little_endian
> 
> The native endian case is handled by all users with a trivial helper. This
> patch doesn't change any functionality, nor it does add overhead.
> 
> Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> ---
>  drivers/net/macvtap.c            |    4 +++-
>  drivers/net/tun.c                |    4 +++-
>  drivers/vhost/vhost.h            |    4 +++-
>  include/linux/virtio_byteorder.h |   24 ++++++++++++++----------
>  include/linux/virtio_config.h    |    4 +++-
>  include/linux/vringh.h           |    4 +++-
>  6 files changed, 29 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/net/macvtap.c b/drivers/net/macvtap.c
> index a2f2958..0a03a66 100644
> --- a/drivers/net/macvtap.c
> +++ b/drivers/net/macvtap.c
> @@ -51,7 +51,9 @@ struct macvtap_queue {
>  
>  static inline bool macvtap_is_little_endian(struct macvtap_queue *q)
>  {
> -	return q->flags & MACVTAP_VNET_LE;
> +	if (q->flags & MACVTAP_VNET_LE)
> +		return true;
> +	return virtio_legacy_is_little_endian();
>  }
>  
>  static inline u16 macvtap16_to_cpu(struct macvtap_queue *q, __virtio16 val)

Hmm I'm not sure how well this will work once you
actually make it dynamic.
Remains to be seen.

> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> index 3c3d6c0..053f9b6 100644
> --- a/drivers/net/tun.c
> +++ b/drivers/net/tun.c
> @@ -208,7 +208,9 @@ struct tun_struct {
>  
>  static inline bool tun_is_little_endian(struct tun_struct *tun)
>  {
> -	return tun->flags & TUN_VNET_LE;
> +	if (tun->flags & TUN_VNET_LE)
> +		return true;
> +	return virtio_legacy_is_little_endian();
>  }
>  
>  static inline u16 tun16_to_cpu(struct tun_struct *tun, __virtio16 val)
> diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h
> index 6a49960..4e9a186 100644
> --- a/drivers/vhost/vhost.h
> +++ b/drivers/vhost/vhost.h
> @@ -175,7 +175,9 @@ static inline bool vhost_has_feature(struct vhost_virtqueue *vq, int bit)
>  
>  static inline bool vhost_is_little_endian(struct vhost_virtqueue *vq)
>  {
> -	return vhost_has_feature(vq, VIRTIO_F_VERSION_1);
> +	if (vhost_has_feature(vq, VIRTIO_F_VERSION_1))
> +		return true;
> +	return virtio_legacy_is_little_endian();
>  }
>  
>  /* Memory accessors */
> diff --git a/include/linux/virtio_byteorder.h b/include/linux/virtio_byteorder.h
> index 51865d0..ce63a2c 100644
> --- a/include/linux/virtio_byteorder.h
> +++ b/include/linux/virtio_byteorder.h
> @@ -3,17 +3,21 @@
>  #include <linux/types.h>
>  #include <uapi/linux/virtio_types.h>
>  
> -/*
> - * Low-level memory accessors for handling virtio in modern little endian and in
> - * compatibility native endian format.
> - */
> +static inline bool virtio_legacy_is_little_endian(void)
> +{
> +#ifdef __LITTLE_ENDIAN
> +	return true;
> +#else
> +	return false;
> +#endif
> +}
>  
>  static inline u16 __virtio16_to_cpu(bool little_endian, __virtio16 val)
>  {
>  	if (little_endian)
>  		return le16_to_cpu((__force __le16)val);
>  	else
> -		return (__force u16)val;
> +		return be16_to_cpu((__force __be16)val);
>  }
>  
>  static inline __virtio16 __cpu_to_virtio16(bool little_endian, u16 val)
> @@ -21,7 +25,7 @@ static inline __virtio16 __cpu_to_virtio16(bool little_endian, u16 val)
>  	if (little_endian)
>  		return (__force __virtio16)cpu_to_le16(val);
>  	else
> -		return (__force __virtio16)val;
> +		return (__force __virtio16)cpu_to_be16(val);
>  }
>  
>  static inline u32 __virtio32_to_cpu(bool little_endian, __virtio32 val)
> @@ -29,7 +33,7 @@ static inline u32 __virtio32_to_cpu(bool little_endian, __virtio32 val)
>  	if (little_endian)
>  		return le32_to_cpu((__force __le32)val);
>  	else
> -		return (__force u32)val;
> +		return be32_to_cpu((__force __be32)val);
>  }
>  
>  static inline __virtio32 __cpu_to_virtio32(bool little_endian, u32 val)
> @@ -37,7 +41,7 @@ static inline __virtio32 __cpu_to_virtio32(bool little_endian, u32 val)
>  	if (little_endian)
>  		return (__force __virtio32)cpu_to_le32(val);
>  	else
> -		return (__force __virtio32)val;
> +		return (__force __virtio32)cpu_to_be32(val);
>  }
>  
>  static inline u64 __virtio64_to_cpu(bool little_endian, __virtio64 val)
> @@ -45,7 +49,7 @@ static inline u64 __virtio64_to_cpu(bool little_endian, __virtio64 val)
>  	if (little_endian)
>  		return le64_to_cpu((__force __le64)val);
>  	else
> -		return (__force u64)val;
> +		return be64_to_cpu((__force __be64)val);
>  }
>  
>  static inline __virtio64 __cpu_to_virtio64(bool little_endian, u64 val)
> @@ -53,7 +57,7 @@ static inline __virtio64 __cpu_to_virtio64(bool little_endian, u64 val)
>  	if (little_endian)
>  		return (__force __virtio64)cpu_to_le64(val);
>  	else
> -		return (__force __virtio64)val;
> +		return (__force __virtio64)cpu_to_be64(val);
>  }
>  
>  #endif /* _LINUX_VIRTIO_BYTEORDER */
> diff --git a/include/linux/virtio_config.h b/include/linux/virtio_config.h
> index bd1a582..36a6daa 100644
> --- a/include/linux/virtio_config.h
> +++ b/include/linux/virtio_config.h
> @@ -207,7 +207,9 @@ int virtqueue_set_affinity(struct virtqueue *vq, int cpu)
>  
>  static inline bool virtio_is_little_endian(struct virtio_device *vdev)
>  {
> -	return virtio_has_feature(vdev, VIRTIO_F_VERSION_1);
> +	if (virtio_has_feature(vdev, VIRTIO_F_VERSION_1))
> +		return true;
> +	return virtio_legacy_is_little_endian();
>  }
>  
>  /* Memory accessors */
> diff --git a/include/linux/vringh.h b/include/linux/vringh.h
> index 3ed62ef..d786c2d 100644
> --- a/include/linux/vringh.h
> +++ b/include/linux/vringh.h
> @@ -228,7 +228,9 @@ static inline void vringh_notify(struct vringh *vrh)
>  
>  static inline bool vringh_is_little_endian(const struct vringh *vrh)
>  {
> -	return vrh->little_endian;
> +	if (vrh->little_endian)
> +		return true;
> +	return virtio_legacy_is_little_endian();
>  }
>  
>  static inline u16 vringh16_to_cpu(const struct vringh *vrh, __virtio16 val)

Re: [PATCH v3 0/7] vhost: support for cross endian guests

[Greg Kurz]

On Tue, 7 Apr 2015 17:55:08 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Tue, Apr 07, 2015 at 02:09:29PM +0200, Greg Kurz wrote:
> > Hi,
> > 
> > This patchset allows vhost to be used with legacy virtio when guest and host
> > have a different endianness.
> > 
> > Patches 1-6 remain the same as the previous post. Patch 7 was heavily changed
> > according to MST's comments.
> 
> This still doesn't actually work, right?
> tun and macvtap need new ioctls too ...
> 

Yes they do. I already have a patch but I wasn't sure if I should send it
along this series... Since it looks like there will be a v4, I'll add the
tun/macvtap patch.

Thanks.

--
Greg

> > ---
> > 
> > Greg Kurz (7):
> >       virtio: introduce virtio_is_little_endian() helper
> >       tun: add tun_is_little_endian() helper
> >       macvtap: introduce macvtap_is_little_endian() helper
> >       vringh: introduce vringh_is_little_endian() helper
> >       vhost: introduce vhost_is_little_endian() helper
> >       virtio: add explicit big-endian support to memory accessors
> >       vhost: feature to set the vring endianness
> > 
> > 
> >  drivers/net/macvtap.c            |   11 ++++++--
> >  drivers/net/tun.c                |   11 ++++++--
> >  drivers/vhost/Kconfig            |   10 +++++++
> >  drivers/vhost/vhost.c            |   55 ++++++++++++++++++++++++++++++++++++++
> >  drivers/vhost/vhost.h            |   34 +++++++++++++++++++----
> >  include/linux/virtio_byteorder.h |   24 ++++++++++-------
> >  include/linux/virtio_config.h    |   19 +++++++++----
> >  include/linux/vringh.h           |   19 +++++++++----
> >  include/uapi/linux/vhost.h       |    5 +++
> >  9 files changed, 156 insertions(+), 32 deletions(-)
> > 
> > --
> > Greg
> 

Re: [PATCH v3 7/7] vhost: feature to set the vring endianness

[Michael S. Tsirkin]

On Tue, Apr 07, 2015 at 02:19:31PM +0200, Greg Kurz wrote:
> This patch brings cross-endian support to vhost when used to implement
> legacy virtio devices. Since it is a relatively rare situation, the
> feature availability is controlled by a kernel config option (not set
> by default).
> 
> The ioctls introduced by this patch are for legacy only: virtio 1.0
> devices are returned EPERM.
> 
> Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> ---
>  drivers/vhost/Kconfig      |   10 ++++++++
>  drivers/vhost/vhost.c      |   55 ++++++++++++++++++++++++++++++++++++++++++++
>  drivers/vhost/vhost.h      |   17 +++++++++++++-
>  include/uapi/linux/vhost.h |    5 ++++
>  4 files changed, 86 insertions(+), 1 deletion(-)
> 
> Changes since v2:
> - fixed typos in Kconfig description
> - renamed vq->legacy_big_endian to vq->legacy_is_little_endian
> - vq->legacy_is_little_endian reset to default in vhost_vq_reset()
> - dropped VHOST_F_SET_ENDIAN_LEGACY feature
> - dropped struct vhost_vring_endian from the user API (re-use
>   struct vhost_vring_state instead)
> - added VHOST_GET_VRING_ENDIAN_LEGACY ioctl
> - introduced more helpers and stubs to avoid polluting the code with ifdefs
> 
> 
> diff --git a/drivers/vhost/Kconfig b/drivers/vhost/Kconfig
> index 017a1e8..0aec88c 100644
> --- a/drivers/vhost/Kconfig
> +++ b/drivers/vhost/Kconfig
> @@ -32,3 +32,13 @@ config VHOST
>  	---help---
>  	  This option is selected by any driver which needs to access
>  	  the core of vhost.
> +
> +config VHOST_SET_ENDIAN_LEGACY
> +	bool "Cross-endian support for host kernel accelerator"
> +	default n
> +	---help---
> +	  This option allows vhost to support guests with a different byte
> +	  ordering from host. It is disabled by default since it adds overhead
> +	  and it is only needed by a few platforms (powerpc and arm).
> +
> +	  If unsure, say "N".
> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> index 2ee2826..3529a3c 100644
> --- a/drivers/vhost/vhost.c
> +++ b/drivers/vhost/vhost.c
> @@ -199,6 +199,7 @@ static void vhost_vq_reset(struct vhost_dev *dev,
>  	vq->call = NULL;
>  	vq->log_ctx = NULL;
>  	vq->memory = NULL;
> +	vq->legacy_is_little_endian = virtio_legacy_is_little_endian();
>  }
>  
>  static int vhost_worker(void *data)
> @@ -630,6 +631,54 @@ static long vhost_set_memory(struct vhost_dev *d, struct vhost_memory __user *m)
>  	return 0;
>  }
>  
> +#ifdef CONFIG_VHOST_SET_ENDIAN_LEGACY
> +static long vhost_set_vring_endian_legacy(struct vhost_virtqueue *vq,
> +					  void __user *argp)
> +{
> +	struct vhost_vring_state s;
> +
> +	if (vhost_has_feature(vq, VIRTIO_F_VERSION_1))
> +		return -EPERM;
> +
> +	if (copy_from_user(&s, argp, sizeof(s)))
> +		return -EFAULT;
> +
> +	vq->legacy_is_little_endian = !!s.num;
> +	return 0;
> +}
> +
> +static long vhost_get_vring_endian_legacy(struct vhost_virtqueue *vq,
> +					  u32 idx,
> +					  void __user *argp)
> +{
> +	struct vhost_vring_state s = {
> +		.index = idx,
> +		.num = vq->legacy_is_little_endian
> +	};
> +
> +	if (vhost_has_feature(vq, VIRTIO_F_VERSION_1))
> +		return -EPERM;
> +
> +	if (copy_to_user(argp, &s, sizeof(s)))
> +		return -EFAULT;
> +
> +	return 0;
> +}
> +#else
> +static long vhost_set_vring_endian_legacy(struct vhost_virtqueue *vq,
> +					  void __user *argp)
> +{
> +	return 0;
> +}
> +
> +static long vhost_get_vring_endian_legacy(struct vhost_virtqueue *vq,
> +					  u32 idx,
> +					  void __user *argp)
> +{
> +	return 0;
> +}
> +#endif /* CONFIG_VHOST_SET_ENDIAN_LEGACY */
> +
>  long vhost_vring_ioctl(struct vhost_dev *d, int ioctl, void __user *argp)
>  {
>  	struct file *eventfp, *filep = NULL;
> @@ -806,6 +855,12 @@ long vhost_vring_ioctl(struct vhost_dev *d, int ioctl, void __user *argp)
>  		} else
>  			filep = eventfp;
>  		break;
> +	case VHOST_SET_VRING_ENDIAN_LEGACY:
> +		r = vhost_set_vring_endian_legacy(vq, argp);
> +		break;
> +	case VHOST_GET_VRING_ENDIAN_LEGACY:
> +		r = vhost_get_vring_endian_legacy(vq, idx, argp);
> +		break;
>  	default:
>  		r = -ENOIOCTLCMD;
>  	}
> diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h
> index 4e9a186..981ba06 100644
> --- a/drivers/vhost/vhost.h
> +++ b/drivers/vhost/vhost.h
> @@ -106,6 +106,9 @@ struct vhost_virtqueue {
>  	/* Log write descriptors */
>  	void __user *log_base;
>  	struct vhost_log *log;
> +
> +	/* We need to know the device endianness with legacy virtio. */
> +	bool legacy_is_little_endian;
>  };
>  
>  struct vhost_dev {
> @@ -173,11 +176,23 @@ static inline bool vhost_has_feature(struct vhost_virtqueue *vq, int bit)
>  	return vq->acked_features & (1ULL << bit);
>  }
>  
> +#ifdef CONFIG_VHOST_SET_ENDIAN_LEGACY
> +static inline bool vhost_legacy_is_little_endian(struct vhost_virtqueue *vq)
> +{
> +	return vq->legacy_is_little_endian;
> +}
> +#else
> +static inline bool vhost_legacy_is_little_endian(struct vhost_virtqueue *vq)
> +{
> +	return virtio_legacy_is_little_endian();
> +}
> +#endif
> +
>  static inline bool vhost_is_little_endian(struct vhost_virtqueue *vq)
>  {
>  	if (vhost_has_feature(vq, VIRTIO_F_VERSION_1))
>  		return true;
> -	return virtio_legacy_is_little_endian();
> +	return vhost_legacy_is_little_endian(vq);
>  }
>  
>  /* Memory accessors */
> diff --git a/include/uapi/linux/vhost.h b/include/uapi/linux/vhost.h
> index bb6a5b4..1b01a72 100644
> --- a/include/uapi/linux/vhost.h
> +++ b/include/uapi/linux/vhost.h
> @@ -103,6 +103,11 @@ struct vhost_memory {
>  /* Get accessor: reads index, writes value in num */
>  #define VHOST_GET_VRING_BASE _IOWR(VHOST_VIRTIO, 0x12, struct vhost_vring_state)
>  
> +/* Set endianness for the ring (legacy virtio only) */
> +/* num is 0 for big endian, other values mean little endian */
> +#define VHOST_SET_VRING_ENDIAN_LEGACY _IOW(VHOST_VIRTIO, 0x13, struct vhost_vring_state)
> +#define VHOST_GET_VRING_ENDIAN_LEGACY _IOW(VHOST_VIRTIO, 0x14, struct vhost_vring_state)
> +
>  /* The following ioctls use eventfd file descriptors to signal and poll
>   * for events. */
>  

So if the config feature is enabled, you actually check two
things: 1. feature 2. ioctl

Why not override is_le when we set VIRTIO_F_VERSION_1?
I guess you are worried that setting a value and not being
able to read it back is ugly. We can add a flag to track
it though. So here's an idea
	I would probably rename to G/SET_BIG_ENDIAN then it's obvious
	it's legacy.  And just document that it's ignored with
	VIRTIO_F_VERSION_1.

	make sure it's not changed when ring is running

	simply set vq->is_le correctly when we start/stop the device
		vq->is_le = vhost_has_feature(vq, VIRTIO_F_VERSION_1) ||
			!vq->user_be;

	 static inline bool vhost_is_little_endian(struct vhost_virtqueue *vq)
	 {
		return vq->is_le;
	 }


-- 
MST

Re: [PATCH v3 7/7] vhost: feature to set the vring endianness

[Greg Kurz]

On Tue, 7 Apr 2015 17:01:31 +0200
Cornelia Huck <cornelia.huck@de.ibm.com> wrote:

> On Tue, 07 Apr 2015 14:19:31 +0200
> Greg Kurz <gkurz@linux.vnet.ibm.com> wrote:
> 
> > This patch brings cross-endian support to vhost when used to implement
> > legacy virtio devices. Since it is a relatively rare situation, the
> > feature availability is controlled by a kernel config option (not set
> > by default).
> > 
> > The ioctls introduced by this patch are for legacy only: virtio 1.0
> > devices are returned EPERM.
> > 
> > Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> > ---
> >  drivers/vhost/Kconfig      |   10 ++++++++
> >  drivers/vhost/vhost.c      |   55 ++++++++++++++++++++++++++++++++++++++++++++
> >  drivers/vhost/vhost.h      |   17 +++++++++++++-
> >  include/uapi/linux/vhost.h |    5 ++++
> >  4 files changed, 86 insertions(+), 1 deletion(-)
> 
> > +#ifdef CONFIG_VHOST_SET_ENDIAN_LEGACY
> > +static long vhost_set_vring_endian_legacy(struct vhost_virtqueue *vq,
> > +					  void __user *argp)
> > +{
> > +	struct vhost_vring_state s;
> > +
> > +	if (vhost_has_feature(vq, VIRTIO_F_VERSION_1))
> > +		return -EPERM;
> > +
> > +	if (copy_from_user(&s, argp, sizeof(s)))
> > +		return -EFAULT;
> > +
> > +	vq->legacy_is_little_endian = !!s.num;
> > +	return 0;
> > +}
> > +
> > +static long vhost_get_vring_endian_legacy(struct vhost_virtqueue *vq,
> > +					  u32 idx,
> > +					  void __user *argp)
> > +{
> > +	struct vhost_vring_state s = {
> > +		.index = idx,
> > +		.num = vq->legacy_is_little_endian
> > +	};
> > +
> > +	if (vhost_has_feature(vq, VIRTIO_F_VERSION_1))
> > +		return -EPERM;
> > +
> > +	if (copy_to_user(argp, &s, sizeof(s)))
> > +		return -EFAULT;
> > +
> > +	return 0;
> > +}
> > +#else
> > +static long vhost_set_vring_endian_legacy(struct vhost_virtqueue *vq,
> > +					  void __user *argp)
> > +{
> > +	return 0;
> 
> I'm wondering whether this handler should return an error if the
> feature is not configured for the kernel? How can the userspace caller
> find out whether it has successfully prompted the kernel to handle the
> endianness correctly?
> 

Yes you're right. I think -ENOIOCTLCMD as suggested by Michael is a good
candidate.

Thanks.

> > +}
> > +
> > +static long vhost_get_vring_endian_legacy(struct vhost_virtqueue *vq,
> > +					  u32 idx,
> > +					  void __user *argp)
> > +{
> > +	return 0;
> > +}
> > +#endif /* CONFIG_VHOST_SET_ENDIAN_LEGACY */

Re: [PATCH v3 7/7] vhost: feature to set the vring endianness

[Greg Kurz]

On Tue, 7 Apr 2015 17:52:28 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Tue, Apr 07, 2015 at 02:19:31PM +0200, Greg Kurz wrote:
> > This patch brings cross-endian support to vhost when used to implement
> > legacy virtio devices. Since it is a relatively rare situation, the
> > feature availability is controlled by a kernel config option (not set
> > by default).
> > 
> > The ioctls introduced by this patch are for legacy only: virtio 1.0
> > devices are returned EPERM.
> > 
> > Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> 
> EINVAL probably makes more sense?
> 

I had choosen EPERM because the error isn't related to the arguments
being passed by userspace. It simply does not make sense to set the
vring endianness for a virtio 1.0 device.

That being said, I am perfectly fine with EINVAL. :)

> > ---
> >  drivers/vhost/Kconfig      |   10 ++++++++
> >  drivers/vhost/vhost.c      |   55 ++++++++++++++++++++++++++++++++++++++++++++
> >  drivers/vhost/vhost.h      |   17 +++++++++++++-
> >  include/uapi/linux/vhost.h |    5 ++++
> >  4 files changed, 86 insertions(+), 1 deletion(-)
> > 
> > Changes since v2:
> > - fixed typos in Kconfig description
> > - renamed vq->legacy_big_endian to vq->legacy_is_little_endian
> > - vq->legacy_is_little_endian reset to default in vhost_vq_reset()
> > - dropped VHOST_F_SET_ENDIAN_LEGACY feature
> > - dropped struct vhost_vring_endian from the user API (re-use
> >   struct vhost_vring_state instead)
> > - added VHOST_GET_VRING_ENDIAN_LEGACY ioctl
> > - introduced more helpers and stubs to avoid polluting the code with ifdefs
> > 
> > 
> > diff --git a/drivers/vhost/Kconfig b/drivers/vhost/Kconfig
> > index 017a1e8..0aec88c 100644
> > --- a/drivers/vhost/Kconfig
> > +++ b/drivers/vhost/Kconfig
> > @@ -32,3 +32,13 @@ config VHOST
> >  	---help---
> >  	  This option is selected by any driver which needs to access
> >  	  the core of vhost.
> > +
> > +config VHOST_SET_ENDIAN_LEGACY
> > +	bool "Cross-endian support for host kernel accelerator"
> > +	default n
> > +	---help---
> > +	  This option allows vhost to support guests with a different byte
> > +	  ordering from host. It is disabled by default since it adds overhead
> > +	  and it is only needed by a few platforms (powerpc and arm).
> > +
> > +	  If unsure, say "N".
> > diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> > index 2ee2826..3529a3c 100644
> > --- a/drivers/vhost/vhost.c
> > +++ b/drivers/vhost/vhost.c
> > @@ -199,6 +199,7 @@ static void vhost_vq_reset(struct vhost_dev *dev,
> >  	vq->call = NULL;
> >  	vq->log_ctx = NULL;
> >  	vq->memory = NULL;
> > +	vq->legacy_is_little_endian = virtio_legacy_is_little_endian();
> >  }
> >  
> >  static int vhost_worker(void *data)
> > @@ -630,6 +631,54 @@ static long vhost_set_memory(struct vhost_dev *d, struct vhost_memory __user *m)
> >  	return 0;
> >  }
> >  
> > +#ifdef CONFIG_VHOST_SET_ENDIAN_LEGACY
> > +static long vhost_set_vring_endian_legacy(struct vhost_virtqueue *vq,
> > +					  void __user *argp)
> > +{
> > +	struct vhost_vring_state s;
> > +
> > +	if (vhost_has_feature(vq, VIRTIO_F_VERSION_1))
> > +		return -EPERM;
> 
> EINVAL probably makes more sense? But I'm not sure this
> is helpful: one can set VIRTIO_F_VERSION_1 afterwards,
> and your patch does not seem to detect this.
> 

Yeah, when I dropped the *bogus* feature from v2, I forgot to patch
VHOST_SET_FEATURES accordingly... But thinking about it now, the choice
to error out when setting VIRTIO_F_VERSION_1 because cross-endian legacy
was set before looks terrible... :-\

> 
> 
> > +
> > +	if (copy_from_user(&s, argp, sizeof(s)))
> > +		return -EFAULT;
> > +
> > +	vq->legacy_is_little_endian = !!s.num;
> > +	return 0;
> > +}
> > +
> > +static long vhost_get_vring_endian_legacy(struct vhost_virtqueue *vq,
> > +					  u32 idx,
> > +					  void __user *argp)
> > +{
> > +	struct vhost_vring_state s = {
> > +		.index = idx,
> > +		.num = vq->legacy_is_little_endian
> > +	};
> > +
> > +	if (vhost_has_feature(vq, VIRTIO_F_VERSION_1))
> > +		return -EPERM;
> > +
> > +	if (copy_to_user(argp, &s, sizeof(s)))
> > +		return -EFAULT;
> > +
> > +	return 0;
> > +}
> > +#else
> > +static long vhost_set_vring_endian_legacy(struct vhost_virtqueue *vq,
> > +					  void __user *argp)
> > +{
> > +	return 0;
> > +}
> > +
> > +static long vhost_get_vring_endian_legacy(struct vhost_virtqueue *vq,
> > +					  u32 idx,
> > +					  void __user *argp)
> > +{
> > +	return 0;
> > +}
> 
> Should be -ENOIOCTLCMD?
> 

Sure.

> > +#endif /* CONFIG_VHOST_SET_ENDIAN_LEGACY */
> > +
> >  long vhost_vring_ioctl(struct vhost_dev *d, int ioctl, void __user *argp)
> >  {
> >  	struct file *eventfp, *filep = NULL;
> > @@ -806,6 +855,12 @@ long vhost_vring_ioctl(struct vhost_dev *d, int ioctl, void __user *argp)
> >  		} else
> >  			filep = eventfp;
> >  		break;
> > +	case VHOST_SET_VRING_ENDIAN_LEGACY:
> > +		r = vhost_set_vring_endian_legacy(vq, argp);
> > +		break;
> > +	case VHOST_GET_VRING_ENDIAN_LEGACY:
> > +		r = vhost_get_vring_endian_legacy(vq, idx, argp);
> > +		break;
> >  	default:
> >  		r = -ENOIOCTLCMD;
> >  	}
> 
> I think we also want to forbid this with a running backend.
> 

Yes.

> > diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h
> > index 4e9a186..981ba06 100644
> > --- a/drivers/vhost/vhost.h
> > +++ b/drivers/vhost/vhost.h
> > @@ -106,6 +106,9 @@ struct vhost_virtqueue {
> >  	/* Log write descriptors */
> >  	void __user *log_base;
> >  	struct vhost_log *log;
> > +
> > +	/* We need to know the device endianness with legacy virtio. */
> > +	bool legacy_is_little_endian;
> >  };
> >  
> >  struct vhost_dev {
> > @@ -173,11 +176,23 @@ static inline bool vhost_has_feature(struct vhost_virtqueue *vq, int bit)
> >  	return vq->acked_features & (1ULL << bit);
> >  }
> >  
> > +#ifdef CONFIG_VHOST_SET_ENDIAN_LEGACY
> > +static inline bool vhost_legacy_is_little_endian(struct vhost_virtqueue *vq)
> > +{
> > +	return vq->legacy_is_little_endian;
> > +}
> > +#else
> > +static inline bool vhost_legacy_is_little_endian(struct vhost_virtqueue *vq)
> > +{
> > +	return virtio_legacy_is_little_endian();
> > +}
> > +#endif
> > +
> >  static inline bool vhost_is_little_endian(struct vhost_virtqueue *vq)
> >  {
> >  	if (vhost_has_feature(vq, VIRTIO_F_VERSION_1))
> >  		return true;
> > -	return virtio_legacy_is_little_endian();
> > +	return vhost_legacy_is_little_endian(vq);
> >  }
> >  
> >  /* Memory accessors */
> > diff --git a/include/uapi/linux/vhost.h b/include/uapi/linux/vhost.h
> > index bb6a5b4..1b01a72 100644
> > --- a/include/uapi/linux/vhost.h
> > +++ b/include/uapi/linux/vhost.h
> > @@ -103,6 +103,11 @@ struct vhost_memory {
> >  /* Get accessor: reads index, writes value in num */
> >  #define VHOST_GET_VRING_BASE _IOWR(VHOST_VIRTIO, 0x12, struct vhost_vring_state)
> >  
> > +/* Set endianness for the ring (legacy virtio only) */
> > +/* num is 0 for big endian, other values mean little endian */
> 
> I'd prefer 0 and 1, return EINVAL on other values.
> 

Ok.

> > +#define VHOST_SET_VRING_ENDIAN_LEGACY _IOW(VHOST_VIRTIO, 0x13, struct vhost_vring_state)
> > +#define VHOST_GET_VRING_ENDIAN_LEGACY _IOW(VHOST_VIRTIO, 0x14, struct vhost_vring_state)
> > +
> >  /* The following ioctls use eventfd file descriptors to signal and poll
> >   * for events. */
> >  
> 

Thanks.

--
Greg

Re: [PATCH v3 7/7] vhost: feature to set the vring endianness

[Greg Kurz]

On Tue, 7 Apr 2015 18:11:29 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Tue, Apr 07, 2015 at 02:19:31PM +0200, Greg Kurz wrote:
> > This patch brings cross-endian support to vhost when used to implement
> > legacy virtio devices. Since it is a relatively rare situation, the
> > feature availability is controlled by a kernel config option (not set
> > by default).
> > 
> > The ioctls introduced by this patch are for legacy only: virtio 1.0
> > devices are returned EPERM.
> > 
> > Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> > ---
> >  drivers/vhost/Kconfig      |   10 ++++++++
> >  drivers/vhost/vhost.c      |   55 ++++++++++++++++++++++++++++++++++++++++++++
> >  drivers/vhost/vhost.h      |   17 +++++++++++++-
> >  include/uapi/linux/vhost.h |    5 ++++
> >  4 files changed, 86 insertions(+), 1 deletion(-)
> > 
> > Changes since v2:
> > - fixed typos in Kconfig description
> > - renamed vq->legacy_big_endian to vq->legacy_is_little_endian
> > - vq->legacy_is_little_endian reset to default in vhost_vq_reset()
> > - dropped VHOST_F_SET_ENDIAN_LEGACY feature
> > - dropped struct vhost_vring_endian from the user API (re-use
> >   struct vhost_vring_state instead)
> > - added VHOST_GET_VRING_ENDIAN_LEGACY ioctl
> > - introduced more helpers and stubs to avoid polluting the code with ifdefs
> > 
> > 
> > diff --git a/drivers/vhost/Kconfig b/drivers/vhost/Kconfig
> > index 017a1e8..0aec88c 100644
> > --- a/drivers/vhost/Kconfig
> > +++ b/drivers/vhost/Kconfig
> > @@ -32,3 +32,13 @@ config VHOST
> >  	---help---
> >  	  This option is selected by any driver which needs to access
> >  	  the core of vhost.
> > +
> > +config VHOST_SET_ENDIAN_LEGACY
> > +	bool "Cross-endian support for host kernel accelerator"
> > +	default n
> > +	---help---
> > +	  This option allows vhost to support guests with a different byte
> > +	  ordering from host. It is disabled by default since it adds overhead
> > +	  and it is only needed by a few platforms (powerpc and arm).
> > +
> > +	  If unsure, say "N".
> > diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> > index 2ee2826..3529a3c 100644
> > --- a/drivers/vhost/vhost.c
> > +++ b/drivers/vhost/vhost.c
> > @@ -199,6 +199,7 @@ static void vhost_vq_reset(struct vhost_dev *dev,
> >  	vq->call = NULL;
> >  	vq->log_ctx = NULL;
> >  	vq->memory = NULL;
> > +	vq->legacy_is_little_endian = virtio_legacy_is_little_endian();
> >  }
> >  
> >  static int vhost_worker(void *data)
> > @@ -630,6 +631,54 @@ static long vhost_set_memory(struct vhost_dev *d, struct vhost_memory __user *m)
> >  	return 0;
> >  }
> >  
> > +#ifdef CONFIG_VHOST_SET_ENDIAN_LEGACY
> > +static long vhost_set_vring_endian_legacy(struct vhost_virtqueue *vq,
> > +					  void __user *argp)
> > +{
> > +	struct vhost_vring_state s;
> > +
> > +	if (vhost_has_feature(vq, VIRTIO_F_VERSION_1))
> > +		return -EPERM;
> > +
> > +	if (copy_from_user(&s, argp, sizeof(s)))
> > +		return -EFAULT;
> > +
> > +	vq->legacy_is_little_endian = !!s.num;
> > +	return 0;
> > +}
> > +
> > +static long vhost_get_vring_endian_legacy(struct vhost_virtqueue *vq,
> > +					  u32 idx,
> > +					  void __user *argp)
> > +{
> > +	struct vhost_vring_state s = {
> > +		.index = idx,
> > +		.num = vq->legacy_is_little_endian
> > +	};
> > +
> > +	if (vhost_has_feature(vq, VIRTIO_F_VERSION_1))
> > +		return -EPERM;
> > +
> > +	if (copy_to_user(argp, &s, sizeof(s)))
> > +		return -EFAULT;
> > +
> > +	return 0;
> > +}
> > +#else
> > +static long vhost_set_vring_endian_legacy(struct vhost_virtqueue *vq,
> > +					  void __user *argp)
> > +{
> > +	return 0;
> > +}
> > +
> > +static long vhost_get_vring_endian_legacy(struct vhost_virtqueue *vq,
> > +					  u32 idx,
> > +					  void __user *argp)
> > +{
> > +	return 0;
> > +}
> > +#endif /* CONFIG_VHOST_SET_ENDIAN_LEGACY */
> > +
> >  long vhost_vring_ioctl(struct vhost_dev *d, int ioctl, void __user *argp)
> >  {
> >  	struct file *eventfp, *filep = NULL;
> > @@ -806,6 +855,12 @@ long vhost_vring_ioctl(struct vhost_dev *d, int ioctl, void __user *argp)
> >  		} else
> >  			filep = eventfp;
> >  		break;
> > +	case VHOST_SET_VRING_ENDIAN_LEGACY:
> > +		r = vhost_set_vring_endian_legacy(vq, argp);
> > +		break;
> > +	case VHOST_GET_VRING_ENDIAN_LEGACY:
> > +		r = vhost_get_vring_endian_legacy(vq, idx, argp);
> > +		break;
> >  	default:
> >  		r = -ENOIOCTLCMD;
> >  	}
> > diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h
> > index 4e9a186..981ba06 100644
> > --- a/drivers/vhost/vhost.h
> > +++ b/drivers/vhost/vhost.h
> > @@ -106,6 +106,9 @@ struct vhost_virtqueue {
> >  	/* Log write descriptors */
> >  	void __user *log_base;
> >  	struct vhost_log *log;
> > +
> > +	/* We need to know the device endianness with legacy virtio. */
> > +	bool legacy_is_little_endian;
> >  };
> >  
> >  struct vhost_dev {
> > @@ -173,11 +176,23 @@ static inline bool vhost_has_feature(struct vhost_virtqueue *vq, int bit)
> >  	return vq->acked_features & (1ULL << bit);
> >  }
> >  
> > +#ifdef CONFIG_VHOST_SET_ENDIAN_LEGACY
> > +static inline bool vhost_legacy_is_little_endian(struct vhost_virtqueue *vq)
> > +{
> > +	return vq->legacy_is_little_endian;
> > +}
> > +#else
> > +static inline bool vhost_legacy_is_little_endian(struct vhost_virtqueue *vq)
> > +{
> > +	return virtio_legacy_is_little_endian();
> > +}
> > +#endif
> > +
> >  static inline bool vhost_is_little_endian(struct vhost_virtqueue *vq)
> >  {
> >  	if (vhost_has_feature(vq, VIRTIO_F_VERSION_1))
> >  		return true;
> > -	return virtio_legacy_is_little_endian();
> > +	return vhost_legacy_is_little_endian(vq);
> >  }
> >  
> >  /* Memory accessors */
> > diff --git a/include/uapi/linux/vhost.h b/include/uapi/linux/vhost.h
> > index bb6a5b4..1b01a72 100644
> > --- a/include/uapi/linux/vhost.h
> > +++ b/include/uapi/linux/vhost.h
> > @@ -103,6 +103,11 @@ struct vhost_memory {
> >  /* Get accessor: reads index, writes value in num */
> >  #define VHOST_GET_VRING_BASE _IOWR(VHOST_VIRTIO, 0x12, struct vhost_vring_state)
> >  
> > +/* Set endianness for the ring (legacy virtio only) */
> > +/* num is 0 for big endian, other values mean little endian */
> > +#define VHOST_SET_VRING_ENDIAN_LEGACY _IOW(VHOST_VIRTIO, 0x13, struct vhost_vring_state)
> > +#define VHOST_GET_VRING_ENDIAN_LEGACY _IOW(VHOST_VIRTIO, 0x14, struct vhost_vring_state)
> > +
> >  /* The following ioctls use eventfd file descriptors to signal and poll
> >   * for events. */
> >  
> 
> So if the config feature is enabled, you actually check two
> things: 1. feature 2. ioctl
> 
> Why not override is_le when we set VIRTIO_F_VERSION_1?
> I guess you are worried that setting a value and not being
> able to read it back is ugly. We can add a flag to track
> it though. So here's an idea
> 	I would probably rename to G/SET_BIG_ENDIAN then it's obvious
> 	it's legacy.  And just document that it's ignored with
> 	VIRTIO_F_VERSION_1.
> 
> 	make sure it's not changed when ring is running
> 
> 	simply set vq->is_le correctly when we start/stop the device
> 		vq->is_le = vhost_has_feature(vq, VIRTIO_F_VERSION_1) ||
> 			!vq->user_be;
> 
> 	 static inline bool vhost_is_little_endian(struct vhost_virtqueue *vq)
> 	 {
> 		return vq->is_le;
> 	 }
> 
> 

I like the idea. I'll give it a try.

Thanks for your help Michael.

--
Greg

Re: [PATCH v3 6/7] virtio: add explicit big-endian support to memory accessors

[Greg Kurz]

On Tue, 7 Apr 2015 17:56:25 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Tue, Apr 07, 2015 at 02:15:52PM +0200, Greg Kurz wrote:
> > The current memory accessors logic is:
> > - little endian if little_endian
> > - native endian (i.e. no byteswap) if !little_endian
> > 
> > If we want to fully support cross-endian vhost, we also need to be
> > able to convert to big endian.
> > 
> > Instead of changing the little_endian argument to some 3-value enum, this
> > patch changes the logic to:
> > - little endian if little_endian
> > - big endian if !little_endian
> > 
> > The native endian case is handled by all users with a trivial helper. This
> > patch doesn't change any functionality, nor it does add overhead.
> > 
> > Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> > ---
> >  drivers/net/macvtap.c            |    4 +++-
> >  drivers/net/tun.c                |    4 +++-
> >  drivers/vhost/vhost.h            |    4 +++-
> >  include/linux/virtio_byteorder.h |   24 ++++++++++++++----------
> >  include/linux/virtio_config.h    |    4 +++-
> >  include/linux/vringh.h           |    4 +++-
> >  6 files changed, 29 insertions(+), 15 deletions(-)
> > 
> > diff --git a/drivers/net/macvtap.c b/drivers/net/macvtap.c
> > index a2f2958..0a03a66 100644
> > --- a/drivers/net/macvtap.c
> > +++ b/drivers/net/macvtap.c
> > @@ -51,7 +51,9 @@ struct macvtap_queue {
> >  
> >  static inline bool macvtap_is_little_endian(struct macvtap_queue *q)
> >  {
> > -	return q->flags & MACVTAP_VNET_LE;
> > +	if (q->flags & MACVTAP_VNET_LE)
> > +		return true;
> > +	return virtio_legacy_is_little_endian();
> >  }
> >  
> >  static inline u16 macvtap16_to_cpu(struct macvtap_queue *q, __virtio16 val)
> 
> Hmm I'm not sure how well this will work once you
> actually make it dynamic.
> Remains to be seen.
> 

Oops I overlooked this mail... FWIW I could reboot/kexec from a ppc64 guest
to ppc64le and back with the following QEMU:

https://github.com/gkurz/qemu/commits/vhost/cross-endian

> > diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> > index 3c3d6c0..053f9b6 100644
> > --- a/drivers/net/tun.c
> > +++ b/drivers/net/tun.c
> > @@ -208,7 +208,9 @@ struct tun_struct {
> >  
> >  static inline bool tun_is_little_endian(struct tun_struct *tun)
> >  {
> > -	return tun->flags & TUN_VNET_LE;
> > +	if (tun->flags & TUN_VNET_LE)
> > +		return true;
> > +	return virtio_legacy_is_little_endian();
> >  }
> >  
> >  static inline u16 tun16_to_cpu(struct tun_struct *tun, __virtio16 val)
> > diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h
> > index 6a49960..4e9a186 100644
> > --- a/drivers/vhost/vhost.h
> > +++ b/drivers/vhost/vhost.h
> > @@ -175,7 +175,9 @@ static inline bool vhost_has_feature(struct vhost_virtqueue *vq, int bit)
> >  
> >  static inline bool vhost_is_little_endian(struct vhost_virtqueue *vq)
> >  {
> > -	return vhost_has_feature(vq, VIRTIO_F_VERSION_1);
> > +	if (vhost_has_feature(vq, VIRTIO_F_VERSION_1))
> > +		return true;
> > +	return virtio_legacy_is_little_endian();
> >  }
> >  
> >  /* Memory accessors */
> > diff --git a/include/linux/virtio_byteorder.h b/include/linux/virtio_byteorder.h
> > index 51865d0..ce63a2c 100644
> > --- a/include/linux/virtio_byteorder.h
> > +++ b/include/linux/virtio_byteorder.h
> > @@ -3,17 +3,21 @@
> >  #include <linux/types.h>
> >  #include <uapi/linux/virtio_types.h>
> >  
> > -/*
> > - * Low-level memory accessors for handling virtio in modern little endian and in
> > - * compatibility native endian format.
> > - */
> > +static inline bool virtio_legacy_is_little_endian(void)
> > +{
> > +#ifdef __LITTLE_ENDIAN
> > +	return true;
> > +#else
> > +	return false;
> > +#endif
> > +}
> >  
> >  static inline u16 __virtio16_to_cpu(bool little_endian, __virtio16 val)
> >  {
> >  	if (little_endian)
> >  		return le16_to_cpu((__force __le16)val);
> >  	else
> > -		return (__force u16)val;
> > +		return be16_to_cpu((__force __be16)val);
> >  }
> >  
> >  static inline __virtio16 __cpu_to_virtio16(bool little_endian, u16 val)
> > @@ -21,7 +25,7 @@ static inline __virtio16 __cpu_to_virtio16(bool little_endian, u16 val)
> >  	if (little_endian)
> >  		return (__force __virtio16)cpu_to_le16(val);
> >  	else
> > -		return (__force __virtio16)val;
> > +		return (__force __virtio16)cpu_to_be16(val);
> >  }
> >  
> >  static inline u32 __virtio32_to_cpu(bool little_endian, __virtio32 val)
> > @@ -29,7 +33,7 @@ static inline u32 __virtio32_to_cpu(bool little_endian, __virtio32 val)
> >  	if (little_endian)
> >  		return le32_to_cpu((__force __le32)val);
> >  	else
> > -		return (__force u32)val;
> > +		return be32_to_cpu((__force __be32)val);
> >  }
> >  
> >  static inline __virtio32 __cpu_to_virtio32(bool little_endian, u32 val)
> > @@ -37,7 +41,7 @@ static inline __virtio32 __cpu_to_virtio32(bool little_endian, u32 val)
> >  	if (little_endian)
> >  		return (__force __virtio32)cpu_to_le32(val);
> >  	else
> > -		return (__force __virtio32)val;
> > +		return (__force __virtio32)cpu_to_be32(val);
> >  }
> >  
> >  static inline u64 __virtio64_to_cpu(bool little_endian, __virtio64 val)
> > @@ -45,7 +49,7 @@ static inline u64 __virtio64_to_cpu(bool little_endian, __virtio64 val)
> >  	if (little_endian)
> >  		return le64_to_cpu((__force __le64)val);
> >  	else
> > -		return (__force u64)val;
> > +		return be64_to_cpu((__force __be64)val);
> >  }
> >  
> >  static inline __virtio64 __cpu_to_virtio64(bool little_endian, u64 val)
> > @@ -53,7 +57,7 @@ static inline __virtio64 __cpu_to_virtio64(bool little_endian, u64 val)
> >  	if (little_endian)
> >  		return (__force __virtio64)cpu_to_le64(val);
> >  	else
> > -		return (__force __virtio64)val;
> > +		return (__force __virtio64)cpu_to_be64(val);
> >  }
> >  
> >  #endif /* _LINUX_VIRTIO_BYTEORDER */
> > diff --git a/include/linux/virtio_config.h b/include/linux/virtio_config.h
> > index bd1a582..36a6daa 100644
> > --- a/include/linux/virtio_config.h
> > +++ b/include/linux/virtio_config.h
> > @@ -207,7 +207,9 @@ int virtqueue_set_affinity(struct virtqueue *vq, int cpu)
> >  
> >  static inline bool virtio_is_little_endian(struct virtio_device *vdev)
> >  {
> > -	return virtio_has_feature(vdev, VIRTIO_F_VERSION_1);
> > +	if (virtio_has_feature(vdev, VIRTIO_F_VERSION_1))
> > +		return true;
> > +	return virtio_legacy_is_little_endian();
> >  }
> >  
> >  /* Memory accessors */
> > diff --git a/include/linux/vringh.h b/include/linux/vringh.h
> > index 3ed62ef..d786c2d 100644
> > --- a/include/linux/vringh.h
> > +++ b/include/linux/vringh.h
> > @@ -228,7 +228,9 @@ static inline void vringh_notify(struct vringh *vrh)
> >  
> >  static inline bool vringh_is_little_endian(const struct vringh *vrh)
> >  {
> > -	return vrh->little_endian;
> > +	if (vrh->little_endian)
> > +		return true;
> > +	return virtio_legacy_is_little_endian();
> >  }
> >  
> >  static inline u16 vringh16_to_cpu(const struct vringh *vrh, __virtio16 val)
>