2015-04-02 13:16:46

by Gerg Kurz

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

Hi,

This patchset allows vhost to be used with legacy virtio when guest and host
have a different endianness. It is a complete rework of my initial post.

Patches 1 to 5 are preliminary work: we move the endianness check out of all
memory accessors to separate functions.

Patch 6 changes the semantics of the accessors so that they have explicit big
endian support.

Patch 7 brings the cross-endian feature, with the following changes since v1:
- conditionnal enablement through a kernel config
- introduction of a new vhost feature to advertise cross-endian to userspace

The tentative to fix vnet headers was dropped for the moment. As a consequnce,
vhost_net still fails to work with cross-endian. It will be fixed in another
patchset I'm currently working on.

---

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/net.c | 5 +++++
drivers/vhost/scsi.c | 4 ++++
drivers/vhost/test.c | 4 ++++
drivers/vhost/vhost.c | 19 +++++++++++++++++++
drivers/vhost/vhost.h | 32 +++++++++++++++++++++++++-------
include/linux/virtio_byteorder.h | 24 ++++++++++++++----------
include/linux/virtio_config.h | 19 +++++++++++++------
include/linux/vringh.h | 19 +++++++++++++------
include/uapi/linux/vhost.h | 10 ++++++++++
12 files changed, 135 insertions(+), 33 deletions(-)

--
Greg

2015-04-02 13:16:53

by Gerg Kurz

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

Signed-off-by: Greg Kurz <[email protected]>
---
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. */

2015-04-02 13:17:01

by Gerg Kurz

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

Signed-off-by: Greg Kurz <[email protected]>
---
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)

2015-04-02 13:18:21

by Gerg Kurz

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

Signed-off-by: Greg Kurz <[email protected]>
---
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 = {

2015-04-02 13:17:10

by Gerg Kurz

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

Signed-off-by: Greg Kurz <[email protected]>
---
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 */

2015-04-02 13:18:37

by Gerg Kurz

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

Signed-off-by: Greg Kurz <[email protected]>
---
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

2015-04-02 13:17:20

by Gerg Kurz

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

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 <[email protected]>
---
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)

2015-04-02 13:17:27

by Gerg Kurz

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

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

If cross-endian support is compiled in, vhost abvertises a new feature
to be negotiated with userspace. If userspace acknowledges the feature,
it can inform vhost about the endianness to use with a new ioctl.

This feature is mutually exclusive with virtio 1.0. Even if the vhost device
advertises virtio 1.0 and legacy cross-endian features, it cannot receive
aknowledgement for both at the same time.

Hot paths are being preserved from any penalty when the config option is
disabled or when virtio 1.0 is being used.

Signed-off-by: Greg Kurz <[email protected]>
---
drivers/vhost/Kconfig | 10 ++++++++++
drivers/vhost/net.c | 5 +++++
drivers/vhost/scsi.c | 4 ++++
drivers/vhost/test.c | 4 ++++
drivers/vhost/vhost.c | 19 +++++++++++++++++++
drivers/vhost/vhost.h | 13 ++++++++++++-
include/uapi/linux/vhost.h | 10 ++++++++++
7 files changed, 64 insertions(+), 1 deletion(-)

diff --git a/drivers/vhost/Kconfig b/drivers/vhost/Kconfig
index 017a1e8..5bb8da9 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. 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/net.c b/drivers/vhost/net.c
index 2bbfc25..5274a44 100644
--- a/drivers/vhost/net.c
+++ b/drivers/vhost/net.c
@@ -1011,6 +1011,11 @@ static int vhost_net_set_features(struct vhost_net *n, u64 features)
vhost_hlen = 0;
sock_hlen = hdr_len;
}
+
+ if (features & ((1ULL << VHOST_F_SET_ENDIAN_LEGACY) |
+ (1ULL << VIRTIO_F_VERSION_1)))
+ return -EINVAL;
+
mutex_lock(&n->dev.mutex);
if ((features & (1 << VHOST_F_LOG_ALL)) &&
!vhost_log_access_ok(&n->dev)) {
diff --git a/drivers/vhost/scsi.c b/drivers/vhost/scsi.c
index 71df240..b53e9c2 100644
--- a/drivers/vhost/scsi.c
+++ b/drivers/vhost/scsi.c
@@ -1544,6 +1544,10 @@ static int vhost_scsi_set_features(struct vhost_scsi *vs, u64 features)
if (features & ~VHOST_SCSI_FEATURES)
return -EOPNOTSUPP;

+ if (features & ((1ULL << VHOST_F_SET_ENDIAN_LEGACY) |
+ (1ULL << VIRTIO_F_VERSION_1)))
+ return -EINVAL;
+
mutex_lock(&vs->dev.mutex);
if ((features & (1 << VHOST_F_LOG_ALL)) &&
!vhost_log_access_ok(&vs->dev)) {
diff --git a/drivers/vhost/test.c b/drivers/vhost/test.c
index d9c501e..cfefdad 100644
--- a/drivers/vhost/test.c
+++ b/drivers/vhost/test.c
@@ -243,6 +243,10 @@ static int vhost_test_set_features(struct vhost_test *n, u64 features)
{
struct vhost_virtqueue *vq;

+ if (features & ((1ULL << VHOST_F_SET_ENDIAN_LEGACY) |
+ (1ULL << VIRTIO_F_VERSION_1)))
+ return -EINVAL;
+
mutex_lock(&n->dev.mutex);
if ((features & (1 << VHOST_F_LOG_ALL)) &&
!vhost_log_access_ok(&n->dev)) {
diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index 2ee2826..60a0f32 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_big_endian = false;
}

static int vhost_worker(void *data)
@@ -806,6 +807,24 @@ long vhost_vring_ioctl(struct vhost_dev *d, int ioctl, void __user *argp)
} else
filep = eventfp;
break;
+#ifdef CONFIG_VHOST_SET_ENDIAN_LEGACY
+ case VHOST_SET_VRING_ENDIAN_LEGACY:
+ {
+ struct vhost_vring_endian e;
+
+ if (!vhost_has_feature(vq, VHOST_F_SET_ENDIAN_LEGACY)) {
+ r = -EINVAL;
+ break;
+ }
+
+ if (copy_from_user(&e, argp, sizeof(e))) {
+ r = -EFAULT;
+ break;
+ }
+ vq->legacy_big_endian = e.is_big_endian;
+ break;
+ }
+#endif
default:
r = -ENOIOCTLCMD;
}
diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h
index 4e9a186..d50881d 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_big_endian;
};

struct vhost_dev {
@@ -165,7 +168,11 @@ enum {
VHOST_FEATURES = (1ULL << VIRTIO_F_NOTIFY_ON_EMPTY) |
(1ULL << VIRTIO_RING_F_INDIRECT_DESC) |
(1ULL << VIRTIO_RING_F_EVENT_IDX) |
- (1ULL << VHOST_F_LOG_ALL),
+ (1ULL << VHOST_F_LOG_ALL) |
+#ifdef CONFIG_VHOST_SET_ENDIAN_LEGACY
+ (1ULL << VHOST_F_SET_ENDIAN_LEGACY) |
+#endif
+ 0ULL,
};

static inline bool vhost_has_feature(struct vhost_virtqueue *vq, int bit)
@@ -177,6 +184,10 @@ static inline bool vhost_is_little_endian(struct vhost_virtqueue *vq)
{
if (vhost_has_feature(vq, VIRTIO_F_VERSION_1))
return true;
+#ifdef CONFIG_VHOST_SET_ENDIAN_LEGACY
+ if (vhost_has_feature(vq, VHOST_F_SET_ENDIAN_LEGACY))
+ return !vq->legacy_big_endian;
+#endif
return virtio_legacy_is_little_endian();
}

diff --git a/include/uapi/linux/vhost.h b/include/uapi/linux/vhost.h
index bb6a5b4..09d2a48 100644
--- a/include/uapi/linux/vhost.h
+++ b/include/uapi/linux/vhost.h
@@ -47,6 +47,11 @@ struct vhost_vring_addr {
__u64 log_guest_addr;
};

+struct vhost_vring_endian {
+ unsigned int index;
+ bool is_big_endian;
+};
+
struct vhost_memory_region {
__u64 guest_phys_addr;
__u64 memory_size; /* bytes */
@@ -103,6 +108,9 @@ 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) */
+#define VHOST_SET_VRING_ENDIAN_LEGACY _IOW(VHOST_VIRTIO, 0x13, struct vhost_vring_endian)
+
/* The following ioctls use eventfd file descriptors to signal and poll
* for events. */

@@ -126,6 +134,8 @@ struct vhost_memory {
#define VHOST_F_LOG_ALL 26
/* vhost-net should add virtio_net_hdr for RX, and strip for TX packets. */
#define VHOST_NET_F_VIRTIO_NET_HDR 27
+/* the vring endianness can be explicitly set (legacy virtio only). */
+#define VHOST_F_SET_ENDIAN_LEGACY 28

/* VHOST_SCSI specific definitions */

2015-04-02 14:21:01

by Michael S. Tsirkin

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

On Thu, Apr 02, 2015 at 03:17:13PM +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).
>
> If cross-endian support is compiled in, vhost abvertises a new feature
> to be negotiated with userspace. If userspace acknowledges the feature,
> it can inform vhost about the endianness to use with a new ioctl.
>
> This feature is mutually exclusive with virtio 1.0. Even if the vhost device
> advertises virtio 1.0 and legacy cross-endian features, it cannot receive
> aknowledgement for both at the same time.
>
> Hot paths are being preserved from any penalty when the config option is
> disabled or when virtio 1.0 is being used.
>
> Signed-off-by: Greg Kurz <[email protected]>
> ---
> drivers/vhost/Kconfig | 10 ++++++++++
> drivers/vhost/net.c | 5 +++++
> drivers/vhost/scsi.c | 4 ++++
> drivers/vhost/test.c | 4 ++++
> drivers/vhost/vhost.c | 19 +++++++++++++++++++
> drivers/vhost/vhost.h | 13 ++++++++++++-
> include/uapi/linux/vhost.h | 10 ++++++++++
> 7 files changed, 64 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/vhost/Kconfig b/drivers/vhost/Kconfig
> index 017a1e8..5bb8da9 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".

"N"

> diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
> index 2bbfc25..5274a44 100644
> --- a/drivers/vhost/net.c
> +++ b/drivers/vhost/net.c
> @@ -1011,6 +1011,11 @@ static int vhost_net_set_features(struct vhost_net *n, u64 features)
> vhost_hlen = 0;
> sock_hlen = hdr_len;
> }
> +
> + if (features & ((1ULL << VHOST_F_SET_ENDIAN_LEGACY) |
> + (1ULL << VIRTIO_F_VERSION_1)))
> + return -EINVAL;
> +
> mutex_lock(&n->dev.mutex);
> if ((features & (1 << VHOST_F_LOG_ALL)) &&
> !vhost_log_access_ok(&n->dev)) {
> diff --git a/drivers/vhost/scsi.c b/drivers/vhost/scsi.c
> index 71df240..b53e9c2 100644
> --- a/drivers/vhost/scsi.c
> +++ b/drivers/vhost/scsi.c
> @@ -1544,6 +1544,10 @@ static int vhost_scsi_set_features(struct vhost_scsi *vs, u64 features)
> if (features & ~VHOST_SCSI_FEATURES)
> return -EOPNOTSUPP;
>
> + if (features & ((1ULL << VHOST_F_SET_ENDIAN_LEGACY) |
> + (1ULL << VIRTIO_F_VERSION_1)))
> + return -EINVAL;
> +
> mutex_lock(&vs->dev.mutex);
> if ((features & (1 << VHOST_F_LOG_ALL)) &&
> !vhost_log_access_ok(&vs->dev)) {
> diff --git a/drivers/vhost/test.c b/drivers/vhost/test.c
> index d9c501e..cfefdad 100644
> --- a/drivers/vhost/test.c
> +++ b/drivers/vhost/test.c
> @@ -243,6 +243,10 @@ static int vhost_test_set_features(struct vhost_test *n, u64 features)
> {
> struct vhost_virtqueue *vq;
>
> + if (features & ((1ULL << VHOST_F_SET_ENDIAN_LEGACY) |
> + (1ULL << VIRTIO_F_VERSION_1)))
> + return -EINVAL;
> +
> mutex_lock(&n->dev.mutex);
> if ((features & (1 << VHOST_F_LOG_ALL)) &&
> !vhost_log_access_ok(&n->dev)) {

Above seems to prevent users from specifying either
VIRTIO_F_VERSION_1 or VHOST_F_SET_ENDIAN_LEGACY.
Does it actually work?

> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> index 2ee2826..60a0f32 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_big_endian = false;
> }
>
> static int vhost_worker(void *data)
> @@ -806,6 +807,24 @@ long vhost_vring_ioctl(struct vhost_dev *d, int ioctl, void __user *argp)
> } else
> filep = eventfp;
> break;
> +#ifdef CONFIG_VHOST_SET_ENDIAN_LEGACY
> + case VHOST_SET_VRING_ENDIAN_LEGACY:
> + {
> + struct vhost_vring_endian e;
> +
> + if (!vhost_has_feature(vq, VHOST_F_SET_ENDIAN_LEGACY)) {
> + r = -EINVAL;
> + break;
> + }
> +
> + if (copy_from_user(&e, argp, sizeof(e))) {
> + r = -EFAULT;
> + break;
> + }
> + vq->legacy_big_endian = e.is_big_endian;
> + break;
> + }
> +#endif
> default:
> r = -ENOIOCTLCMD;
> }
> diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h
> index 4e9a186..d50881d 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_big_endian;
> };
>
> struct vhost_dev {
> @@ -165,7 +168,11 @@ enum {
> VHOST_FEATURES = (1ULL << VIRTIO_F_NOTIFY_ON_EMPTY) |
> (1ULL << VIRTIO_RING_F_INDIRECT_DESC) |
> (1ULL << VIRTIO_RING_F_EVENT_IDX) |
> - (1ULL << VHOST_F_LOG_ALL),
> + (1ULL << VHOST_F_LOG_ALL) |
> +#ifdef CONFIG_VHOST_SET_ENDIAN_LEGACY
> + (1ULL << VHOST_F_SET_ENDIAN_LEGACY) |
> +#endif
> + 0ULL,
> };
>
> static inline bool vhost_has_feature(struct vhost_virtqueue *vq, int bit)
> @@ -177,6 +184,10 @@ static inline bool vhost_is_little_endian(struct vhost_virtqueue *vq)
> {
> if (vhost_has_feature(vq, VIRTIO_F_VERSION_1))
> return true;
> +#ifdef CONFIG_VHOST_SET_ENDIAN_LEGACY
> + if (vhost_has_feature(vq, VHOST_F_SET_ENDIAN_LEGACY))
> + return !vq->legacy_big_endian;

why do we need to check the feature bit?
How about simple
return !vq->legacy_big_endian;
here?
All you need to do is set legacy_big_endian to
!virtio_legacy_is_little_endian() on reset.
Maybe rename to legacy_is_little_endian so you don't
need to reverse the logic.

> +#endif
> return virtio_legacy_is_little_endian();
> }
>
> diff --git a/include/uapi/linux/vhost.h b/include/uapi/linux/vhost.h
> index bb6a5b4..09d2a48 100644
> --- a/include/uapi/linux/vhost.h
> +++ b/include/uapi/linux/vhost.h
> @@ -47,6 +47,11 @@ struct vhost_vring_addr {
> __u64 log_guest_addr;
> };
>
> +struct vhost_vring_endian {
> + unsigned int index;
> + bool is_big_endian;

bool in uapi is a bad idea.
Generally, I think you can use vhost_vring_state here.

> +};
> +
> struct vhost_memory_region {
> __u64 guest_phys_addr;
> __u64 memory_size; /* bytes */
> @@ -103,6 +108,9 @@ 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) */
> +#define VHOST_SET_VRING_ENDIAN_LEGACY _IOW(VHOST_VIRTIO, 0x13, struct vhost_vring_endian)
> +
> /* The following ioctls use eventfd file descriptors to signal and poll
> * for events. */
>

You also need a GET ioctl, as a matter of policy.

> @@ -126,6 +134,8 @@ struct vhost_memory {
> #define VHOST_F_LOG_ALL 26
> /* vhost-net should add virtio_net_hdr for RX, and strip for TX packets. */
> #define VHOST_NET_F_VIRTIO_NET_HDR 27
> +/* the vring endianness can be explicitly set (legacy virtio only). */
> +#define VHOST_F_SET_ENDIAN_LEGACY 28
>
> /* VHOST_SCSI specific definitions */


VHOST_F_SET_ENDIAN_LEGACY doesn't seem too useful.
Is this so userspace can detect kernel configuration?
I think probing VHOST_SET_VRING_ENDIAN_LEGACY should
be sufficient for this.

--
MST

2015-04-02 14:23:31

by Michael S. Tsirkin

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

On Thu, Apr 02, 2015 at 03:16:37PM +0200, Greg Kurz wrote:
> Hi,
>
> This patchset allows vhost to be used with legacy virtio when guest and host
> have a different endianness. It is a complete rework of my initial post.
>
> Patches 1 to 5 are preliminary work: we move the endianness check out of all
> memory accessors to separate functions.
>
> Patch 6 changes the semantics of the accessors so that they have explicit big
> endian support.
>
> Patch 7 brings the cross-endian feature, with the following changes since v1:
> - conditionnal enablement through a kernel config
> - introduction of a new vhost feature to advertise cross-endian to userspace

I think adding ioctls is sufficient for this, we don't need to burn up a
feature bit.

> The tentative to fix vnet headers was dropped for the moment. As a consequnce,
> vhost_net still fails to work with cross-endian. It will be fixed in another
> patchset I'm currently working on.

I think I'll probably hold off on applying this
patchset until it all actually works.


> ---
>
> 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/net.c | 5 +++++
> drivers/vhost/scsi.c | 4 ++++
> drivers/vhost/test.c | 4 ++++
> drivers/vhost/vhost.c | 19 +++++++++++++++++++
> drivers/vhost/vhost.h | 32 +++++++++++++++++++++++++-------
> include/linux/virtio_byteorder.h | 24 ++++++++++++++----------
> include/linux/virtio_config.h | 19 +++++++++++++------
> include/linux/vringh.h | 19 +++++++++++++------
> include/uapi/linux/vhost.h | 10 ++++++++++
> 12 files changed, 135 insertions(+), 33 deletions(-)
>
> --
> Greg

2015-04-02 16:45:51

by Gerg Kurz

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

On Thu, 2 Apr 2015 16:20:46 +0200
"Michael S. Tsirkin" <[email protected]> wrote:

> On Thu, Apr 02, 2015 at 03:17:13PM +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).
> >
> > If cross-endian support is compiled in, vhost abvertises a new feature
> > to be negotiated with userspace. If userspace acknowledges the feature,
> > it can inform vhost about the endianness to use with a new ioctl.
> >
> > This feature is mutually exclusive with virtio 1.0. Even if the vhost device
> > advertises virtio 1.0 and legacy cross-endian features, it cannot receive
> > aknowledgement for both at the same time.
> >
> > Hot paths are being preserved from any penalty when the config option is
> > disabled or when virtio 1.0 is being used.
> >
> > Signed-off-by: Greg Kurz <[email protected]>
> > ---
> > drivers/vhost/Kconfig | 10 ++++++++++
> > drivers/vhost/net.c | 5 +++++
> > drivers/vhost/scsi.c | 4 ++++
> > drivers/vhost/test.c | 4 ++++
> > drivers/vhost/vhost.c | 19 +++++++++++++++++++
> > drivers/vhost/vhost.h | 13 ++++++++++++-
> > include/uapi/linux/vhost.h | 10 ++++++++++
> > 7 files changed, 64 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/vhost/Kconfig b/drivers/vhost/Kconfig
> > index 017a1e8..5bb8da9 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".
>
> "N"
>
> > diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
> > index 2bbfc25..5274a44 100644
> > --- a/drivers/vhost/net.c
> > +++ b/drivers/vhost/net.c
> > @@ -1011,6 +1011,11 @@ static int vhost_net_set_features(struct vhost_net *n, u64 features)
> > vhost_hlen = 0;
> > sock_hlen = hdr_len;
> > }
> > +
> > + if (features & ((1ULL << VHOST_F_SET_ENDIAN_LEGACY) |
> > + (1ULL << VIRTIO_F_VERSION_1)))
> > + return -EINVAL;
> > +
> > mutex_lock(&n->dev.mutex);
> > if ((features & (1 << VHOST_F_LOG_ALL)) &&
> > !vhost_log_access_ok(&n->dev)) {
> > diff --git a/drivers/vhost/scsi.c b/drivers/vhost/scsi.c
> > index 71df240..b53e9c2 100644
> > --- a/drivers/vhost/scsi.c
> > +++ b/drivers/vhost/scsi.c
> > @@ -1544,6 +1544,10 @@ static int vhost_scsi_set_features(struct vhost_scsi *vs, u64 features)
> > if (features & ~VHOST_SCSI_FEATURES)
> > return -EOPNOTSUPP;
> >
> > + if (features & ((1ULL << VHOST_F_SET_ENDIAN_LEGACY) |
> > + (1ULL << VIRTIO_F_VERSION_1)))
> > + return -EINVAL;
> > +
> > mutex_lock(&vs->dev.mutex);
> > if ((features & (1 << VHOST_F_LOG_ALL)) &&
> > !vhost_log_access_ok(&vs->dev)) {
> > diff --git a/drivers/vhost/test.c b/drivers/vhost/test.c
> > index d9c501e..cfefdad 100644
> > --- a/drivers/vhost/test.c
> > +++ b/drivers/vhost/test.c
> > @@ -243,6 +243,10 @@ static int vhost_test_set_features(struct vhost_test *n, u64 features)
> > {
> > struct vhost_virtqueue *vq;
> >
> > + if (features & ((1ULL << VHOST_F_SET_ENDIAN_LEGACY) |
> > + (1ULL << VIRTIO_F_VERSION_1)))
> > + return -EINVAL;
> > +
> > mutex_lock(&n->dev.mutex);
> > if ((features & (1 << VHOST_F_LOG_ALL)) &&
> > !vhost_log_access_ok(&n->dev)) {
>
> Above seems to prevent users from specifying either
> VIRTIO_F_VERSION_1 or VHOST_F_SET_ENDIAN_LEGACY.
> Does it actually work?
>

Arggg no it doesn't *of course*... I've added these bogus checks lately
and didn't re-test :(

> > diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> > index 2ee2826..60a0f32 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_big_endian = false;
> > }
> >
> > static int vhost_worker(void *data)
> > @@ -806,6 +807,24 @@ long vhost_vring_ioctl(struct vhost_dev *d, int ioctl, void __user *argp)
> > } else
> > filep = eventfp;
> > break;
> > +#ifdef CONFIG_VHOST_SET_ENDIAN_LEGACY
> > + case VHOST_SET_VRING_ENDIAN_LEGACY:
> > + {
> > + struct vhost_vring_endian e;
> > +
> > + if (!vhost_has_feature(vq, VHOST_F_SET_ENDIAN_LEGACY)) {
> > + r = -EINVAL;
> > + break;
> > + }
> > +
> > + if (copy_from_user(&e, argp, sizeof(e))) {
> > + r = -EFAULT;
> > + break;
> > + }
> > + vq->legacy_big_endian = e.is_big_endian;
> > + break;
> > + }
> > +#endif
> > default:
> > r = -ENOIOCTLCMD;
> > }
> > diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h
> > index 4e9a186..d50881d 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_big_endian;
> > };
> >
> > struct vhost_dev {
> > @@ -165,7 +168,11 @@ enum {
> > VHOST_FEATURES = (1ULL << VIRTIO_F_NOTIFY_ON_EMPTY) |
> > (1ULL << VIRTIO_RING_F_INDIRECT_DESC) |
> > (1ULL << VIRTIO_RING_F_EVENT_IDX) |
> > - (1ULL << VHOST_F_LOG_ALL),
> > + (1ULL << VHOST_F_LOG_ALL) |
> > +#ifdef CONFIG_VHOST_SET_ENDIAN_LEGACY
> > + (1ULL << VHOST_F_SET_ENDIAN_LEGACY) |
> > +#endif
> > + 0ULL,
> > };
> >
> > static inline bool vhost_has_feature(struct vhost_virtqueue *vq, int bit)
> > @@ -177,6 +184,10 @@ static inline bool vhost_is_little_endian(struct vhost_virtqueue *vq)
> > {
> > if (vhost_has_feature(vq, VIRTIO_F_VERSION_1))
> > return true;
> > +#ifdef CONFIG_VHOST_SET_ENDIAN_LEGACY
> > + if (vhost_has_feature(vq, VHOST_F_SET_ENDIAN_LEGACY))
> > + return !vq->legacy_big_endian;
>
> why do we need to check the feature bit?
> How about simple
> return !vq->legacy_big_endian;
> here?

This is a runtime feature. Even for powerpc, cross-endian won't be the
most common scenario. Userspace may have cleared the bit if it doesn't
need/want it.

> All you need to do is set legacy_big_endian to
> !virtio_legacy_is_little_endian() on reset.
> Maybe rename to legacy_is_little_endian so you don't
> need to reverse the logic.
>

Do you mean vhost doesn't need userspace to provide the endianness
to be used ? Please elaborate on the meaning of "reset" here... I
am confused.

> > +#endif
> > return virtio_legacy_is_little_endian();
> > }
> >
> > diff --git a/include/uapi/linux/vhost.h b/include/uapi/linux/vhost.h
> > index bb6a5b4..09d2a48 100644
> > --- a/include/uapi/linux/vhost.h
> > +++ b/include/uapi/linux/vhost.h
> > @@ -47,6 +47,11 @@ struct vhost_vring_addr {
> > __u64 log_guest_addr;
> > };
> >
> > +struct vhost_vring_endian {
> > + unsigned int index;
> > + bool is_big_endian;
>
> bool in uapi is a bad idea.
> Generally, I think you can use vhost_vring_state here.
>

Ok to turn is_big_endian into an int but why hijacking vhost_vring_state ?

> > +};
> > +
> > struct vhost_memory_region {
> > __u64 guest_phys_addr;
> > __u64 memory_size; /* bytes */
> > @@ -103,6 +108,9 @@ 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) */
> > +#define VHOST_SET_VRING_ENDIAN_LEGACY _IOW(VHOST_VIRTIO, 0x13, struct vhost_vring_endian)
> > +
> > /* The following ioctls use eventfd file descriptors to signal and poll
> > * for events. */
> >
>
> You also need a GET ioctl, as a matter of policy.
>

Ok. Will add.

> > @@ -126,6 +134,8 @@ struct vhost_memory {
> > #define VHOST_F_LOG_ALL 26
> > /* vhost-net should add virtio_net_hdr for RX, and strip for TX packets. */
> > #define VHOST_NET_F_VIRTIO_NET_HDR 27
> > +/* the vring endianness can be explicitly set (legacy virtio only). */
> > +#define VHOST_F_SET_ENDIAN_LEGACY 28
> >
> > /* VHOST_SCSI specific definitions */
>
>
> VHOST_F_SET_ENDIAN_LEGACY doesn't seem too useful.
> Is this so userspace can detect kernel configuration?

Yes. Also, it is quite easy to "un-ack" the feature when
it is not wanted.

> I think probing VHOST_SET_VRING_ENDIAN_LEGACY should
> be sufficient for this.
>

I'll get rid of the feature.

Thanks for your time Michael.

--
Greg

2015-04-02 18:53:30

by Michael S. Tsirkin

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

On Thu, Apr 02, 2015 at 06:45:27PM +0200, Greg Kurz wrote:
> On Thu, 2 Apr 2015 16:20:46 +0200
> "Michael S. Tsirkin" <[email protected]> wrote:
>
> > On Thu, Apr 02, 2015 at 03:17:13PM +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).
> > >
> > > If cross-endian support is compiled in, vhost abvertises a new feature
> > > to be negotiated with userspace. If userspace acknowledges the feature,
> > > it can inform vhost about the endianness to use with a new ioctl.
> > >
> > > This feature is mutually exclusive with virtio 1.0. Even if the vhost device
> > > advertises virtio 1.0 and legacy cross-endian features, it cannot receive
> > > aknowledgement for both at the same time.
> > >
> > > Hot paths are being preserved from any penalty when the config option is
> > > disabled or when virtio 1.0 is being used.
> > >
> > > Signed-off-by: Greg Kurz <[email protected]>
> > > ---
> > > drivers/vhost/Kconfig | 10 ++++++++++
> > > drivers/vhost/net.c | 5 +++++
> > > drivers/vhost/scsi.c | 4 ++++
> > > drivers/vhost/test.c | 4 ++++
> > > drivers/vhost/vhost.c | 19 +++++++++++++++++++
> > > drivers/vhost/vhost.h | 13 ++++++++++++-
> > > include/uapi/linux/vhost.h | 10 ++++++++++
> > > 7 files changed, 64 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/vhost/Kconfig b/drivers/vhost/Kconfig
> > > index 017a1e8..5bb8da9 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".
> >
> > "N"
> >
> > > diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
> > > index 2bbfc25..5274a44 100644
> > > --- a/drivers/vhost/net.c
> > > +++ b/drivers/vhost/net.c
> > > @@ -1011,6 +1011,11 @@ static int vhost_net_set_features(struct vhost_net *n, u64 features)
> > > vhost_hlen = 0;
> > > sock_hlen = hdr_len;
> > > }
> > > +
> > > + if (features & ((1ULL << VHOST_F_SET_ENDIAN_LEGACY) |
> > > + (1ULL << VIRTIO_F_VERSION_1)))
> > > + return -EINVAL;
> > > +
> > > mutex_lock(&n->dev.mutex);
> > > if ((features & (1 << VHOST_F_LOG_ALL)) &&
> > > !vhost_log_access_ok(&n->dev)) {
> > > diff --git a/drivers/vhost/scsi.c b/drivers/vhost/scsi.c
> > > index 71df240..b53e9c2 100644
> > > --- a/drivers/vhost/scsi.c
> > > +++ b/drivers/vhost/scsi.c
> > > @@ -1544,6 +1544,10 @@ static int vhost_scsi_set_features(struct vhost_scsi *vs, u64 features)
> > > if (features & ~VHOST_SCSI_FEATURES)
> > > return -EOPNOTSUPP;
> > >
> > > + if (features & ((1ULL << VHOST_F_SET_ENDIAN_LEGACY) |
> > > + (1ULL << VIRTIO_F_VERSION_1)))
> > > + return -EINVAL;
> > > +
> > > mutex_lock(&vs->dev.mutex);
> > > if ((features & (1 << VHOST_F_LOG_ALL)) &&
> > > !vhost_log_access_ok(&vs->dev)) {
> > > diff --git a/drivers/vhost/test.c b/drivers/vhost/test.c
> > > index d9c501e..cfefdad 100644
> > > --- a/drivers/vhost/test.c
> > > +++ b/drivers/vhost/test.c
> > > @@ -243,6 +243,10 @@ static int vhost_test_set_features(struct vhost_test *n, u64 features)
> > > {
> > > struct vhost_virtqueue *vq;
> > >
> > > + if (features & ((1ULL << VHOST_F_SET_ENDIAN_LEGACY) |
> > > + (1ULL << VIRTIO_F_VERSION_1)))
> > > + return -EINVAL;
> > > +
> > > mutex_lock(&n->dev.mutex);
> > > if ((features & (1 << VHOST_F_LOG_ALL)) &&
> > > !vhost_log_access_ok(&n->dev)) {
> >
> > Above seems to prevent users from specifying either
> > VIRTIO_F_VERSION_1 or VHOST_F_SET_ENDIAN_LEGACY.
> > Does it actually work?
> >
>
> Arggg no it doesn't *of course*... I've added these bogus checks lately
> and didn't re-test :(

BTW I would be happier with less ifdefs all over the code,
using some inline wrappers defined differently depending on ifdef.
For example:

#ifdef CONFIG_VHOST_SET_ENDIAN_LEGACY
bool vhost_vq_legacy_is_little_endian(vq)
{
return !vq->legacy_big_endian;
}
#else
bool vhost_vq_legacy_is_little_endian(vq)
{
return virtio_legacy_is_little_endian();
}
#endif

similarly put VHOST_SET_VRING_ENDIAN_LEGACY handling
in a separate function, with a stub definition
when functionality is configured out.

> > > diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> > > index 2ee2826..60a0f32 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_big_endian = false;
> > > }
> > >
> > > static int vhost_worker(void *data)
> > > @@ -806,6 +807,24 @@ long vhost_vring_ioctl(struct vhost_dev *d, int ioctl, void __user *argp)
> > > } else
> > > filep = eventfp;
> > > break;
> > > +#ifdef CONFIG_VHOST_SET_ENDIAN_LEGACY
> > > + case VHOST_SET_VRING_ENDIAN_LEGACY:
> > > + {
> > > + struct vhost_vring_endian e;
> > > +
> > > + if (!vhost_has_feature(vq, VHOST_F_SET_ENDIAN_LEGACY)) {
> > > + r = -EINVAL;
> > > + break;
> > > + }
> > > +
> > > + if (copy_from_user(&e, argp, sizeof(e))) {
> > > + r = -EFAULT;
> > > + break;
> > > + }
> > > + vq->legacy_big_endian = e.is_big_endian;
> > > + break;
> > > + }
> > > +#endif
> > > default:
> > > r = -ENOIOCTLCMD;
> > > }
> > > diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h
> > > index 4e9a186..d50881d 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_big_endian;
> > > };
> > >
> > > struct vhost_dev {
> > > @@ -165,7 +168,11 @@ enum {
> > > VHOST_FEATURES = (1ULL << VIRTIO_F_NOTIFY_ON_EMPTY) |
> > > (1ULL << VIRTIO_RING_F_INDIRECT_DESC) |
> > > (1ULL << VIRTIO_RING_F_EVENT_IDX) |
> > > - (1ULL << VHOST_F_LOG_ALL),
> > > + (1ULL << VHOST_F_LOG_ALL) |
> > > +#ifdef CONFIG_VHOST_SET_ENDIAN_LEGACY
> > > + (1ULL << VHOST_F_SET_ENDIAN_LEGACY) |
> > > +#endif
> > > + 0ULL,
> > > };
> > >
> > > static inline bool vhost_has_feature(struct vhost_virtqueue *vq, int bit)
> > > @@ -177,6 +184,10 @@ static inline bool vhost_is_little_endian(struct vhost_virtqueue *vq)
> > > {
> > > if (vhost_has_feature(vq, VIRTIO_F_VERSION_1))
> > > return true;
> > > +#ifdef CONFIG_VHOST_SET_ENDIAN_LEGACY
> > > + if (vhost_has_feature(vq, VHOST_F_SET_ENDIAN_LEGACY))
> > > + return !vq->legacy_big_endian;
> >
> > why do we need to check the feature bit?
> > How about simple
> > return !vq->legacy_big_endian;
> > here?
>
> This is a runtime feature. Even for powerpc, cross-endian won't be the
> most common scenario. Userspace may have cleared the bit if it doesn't
> need/want it.

Right but if we drop VHOST_F_SET_ENDIAN_LEGACY then
it's simply
#ifdef CONFIG_VHOST_SET_ENDIAN_LEGACY
return !vq->legacy_big_endian;
#else
return virtio_legacy_is_little_endian();
#endif

> > All you need to do is set legacy_big_endian to
> > !virtio_legacy_is_little_endian() on reset.
> > Maybe rename to legacy_is_little_endian so you don't
> > need to reverse the logic.
> >
>
> Do you mean vhost doesn't need userspace to provide the endianness
> to be used ? Please elaborate on the meaning of "reset" here... I
> am confused.

I refer to vhost_vq_reset here.

> > > +#endif
> > > return virtio_legacy_is_little_endian();
> > > }
> > >
> > > diff --git a/include/uapi/linux/vhost.h b/include/uapi/linux/vhost.h
> > > index bb6a5b4..09d2a48 100644
> > > --- a/include/uapi/linux/vhost.h
> > > +++ b/include/uapi/linux/vhost.h
> > > @@ -47,6 +47,11 @@ struct vhost_vring_addr {
> > > __u64 log_guest_addr;
> > > };
> > >
> > > +struct vhost_vring_endian {
> > > + unsigned int index;
> > > + bool is_big_endian;
> >
> > bool in uapi is a bad idea.
> > Generally, I think you can use vhost_vring_state here.
> >
>
> Ok to turn is_big_endian into an int but why hijacking vhost_vring_state ?

Yes.

>
> > > +};
> > > +
> > > struct vhost_memory_region {
> > > __u64 guest_phys_addr;
> > > __u64 memory_size; /* bytes */
> > > @@ -103,6 +108,9 @@ 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) */
> > > +#define VHOST_SET_VRING_ENDIAN_LEGACY _IOW(VHOST_VIRTIO, 0x13, struct vhost_vring_endian)
> > > +
> > > /* The following ioctls use eventfd file descriptors to signal and poll
> > > * for events. */
> > >
> >
> > You also need a GET ioctl, as a matter of policy.
> >
>
> Ok. Will add.
>
> > > @@ -126,6 +134,8 @@ struct vhost_memory {
> > > #define VHOST_F_LOG_ALL 26
> > > /* vhost-net should add virtio_net_hdr for RX, and strip for TX packets. */
> > > #define VHOST_NET_F_VIRTIO_NET_HDR 27
> > > +/* the vring endianness can be explicitly set (legacy virtio only). */
> > > +#define VHOST_F_SET_ENDIAN_LEGACY 28
> > >
> > > /* VHOST_SCSI specific definitions */
> >
> >
> > VHOST_F_SET_ENDIAN_LEGACY doesn't seem too useful.
> > Is this so userspace can detect kernel configuration?
>
> Yes. Also, it is quite easy to "un-ack" the feature when
> it is not wanted.
>
> > I think probing VHOST_SET_VRING_ENDIAN_LEGACY should
> > be sufficient for this.
> >
>
> I'll get rid of the feature.
>
> Thanks for your time Michael.
>
> --
> Greg