[RFC 0/4] Enable virtio-fs on s390x

[RFC 0/4] Enable virtio-fs on s390x

[Marc Hartmayer]

This RFC is about enabling virtio-fs on s390x. For that we need
 + some shim code (first patch), and we need
 + libvhost-user to deal with virtio endiannes as mandated by the spec.
 
The second part is trickier, because unlike QEMU we are not certain
about the guest's native endianness, which is needed to handle the
legacy-interface appropriately. In fact, this is the reason why just
RFC.

One of the open questions is whether to build separate versions, one
for guest little endian and one for guest big endian, or do we want
something like a command line option? (Digression on the libvirt
modeling)

A third option would be to refuse legacy altogether.

libvhost-access.h is based on hw/virtio/virtio-access.h.


How to use?

For general instructions how to use virtio-fs (on x86) please have a
look at https://virtio-fs.gitlab.io/howto-qemu.html. Most of the
instructions can also be applied on s390x.

In short:

1. Install self-compiled QEMU with this patch series applied
2. Prepare host and guest kernel so they support virtio-fs

Start virtiofsd on the host

 $ virtiofsd -f --socket-path=/tmp/vhostqemu -o source=/tmp/shared

Now you can start QEMU in a separate shell on the host:

 $ qemu-system-s390x -machine type=s390-ccw-virtio,accel=kvm,memory-backend=mem \
   -object memory-backend-file,id=mem,size=2G,mem-path=/dev/shm/virtiofs,share=on,prealloc=on,prealloc-threads=1 \
   -chardev socket,id=char0,path=/tmp/vhostqemu -device vhost-user-fs-ccw,queue-size=1024,chardev=char0,tag=myfs \
   -drive if=virtio,file=disk.qcow2 \
   -m 2G -smp 2 -nographic

Log into the guest and mount it

 $ mount -t virtiofs myfs /mnt


Halil Pasic (1):
  virtio: add vhost-user-fs-ccw device

Marc Hartmayer (3):
  libvhost-user: print invalid address on vu_panic
  libvhost-user: handle endianness as mandated by the spec
  HACK: Hard-code the libvhost-user.o-cflags for s390x

 Makefile.objs                           |   1 +
 contrib/libvhost-user/libvhost-access.h |  87 +++++++++++++++++
 contrib/libvhost-user/libvhost-user.c   | 124 ++++++++++++------------
 hw/s390x/Makefile.objs                  |   1 +
 hw/s390x/vhost-user-fs-ccw.c            |  74 ++++++++++++++
 5 files changed, 227 insertions(+), 60 deletions(-)
 create mode 100644 contrib/libvhost-user/libvhost-access.h
 create mode 100644 hw/s390x/vhost-user-fs-ccw.c

-- 
2.25.4

[RFC 1/4] virtio: add vhost-user-fs-ccw device

[Marc Hartmayer]

From: Halil Pasic <pasic@linux.ibm.com>

Wire up the CCW device for vhost-user-fs.

Signed-off-by: Halil Pasic <pasic@linux.ibm.com>
---
 hw/s390x/Makefile.objs       |  1 +
 hw/s390x/vhost-user-fs-ccw.c | 74 ++++++++++++++++++++++++++++++++++++
 2 files changed, 75 insertions(+)
 create mode 100644 hw/s390x/vhost-user-fs-ccw.c

diff --git a/hw/s390x/Makefile.objs b/hw/s390x/Makefile.objs
index a46a1c7894e0..c4086ec3171e 100644
--- a/hw/s390x/Makefile.objs
+++ b/hw/s390x/Makefile.objs
@@ -20,6 +20,7 @@ obj-$(CONFIG_VIRTIO_NET) += virtio-ccw-net.o
 obj-$(CONFIG_VIRTIO_BLK) += virtio-ccw-blk.o
 obj-$(call land,$(CONFIG_VIRTIO_9P),$(CONFIG_VIRTFS)) += virtio-ccw-9p.o
 obj-$(CONFIG_VHOST_VSOCK) += vhost-vsock-ccw.o
+obj-$(CONFIG_VHOST_USER_FS) += vhost-user-fs-ccw.o
 endif
 obj-y += css-bridge.o
 obj-y += ccw-device.o
diff --git a/hw/s390x/vhost-user-fs-ccw.c b/hw/s390x/vhost-user-fs-ccw.c
new file mode 100644
index 000000000000..0f11a77239a5
--- /dev/null
+++ b/hw/s390x/vhost-user-fs-ccw.c
@@ -0,0 +1,74 @@
+/*
+ * Ccw transport wiring for vhost-user-fs
+ *
+ * Copyright 2020 IBM Corp.
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or (at
+ * your option) any later version. See the COPYING file in the top-level
+ * directory.
+ */
+#include "qemu/osdep.h"
+#include "hw/qdev-properties.h"
+#include "qapi/error.h"
+#include "hw/virtio/vhost-user-fs.h"
+#include "virtio-ccw.h"
+
+typedef struct VHostUserFSCcw {
+    VirtioCcwDevice parent_obj;
+    VHostUserFS vdev;
+} VHostUserFSCcw;
+
+#define TYPE_VHOST_USER_FS_CCW "vhost-user-fs-ccw"
+#define VHOST_USER_FS_CCW(obj) \
+        OBJECT_CHECK(VHostUserFSCcw, (obj), TYPE_VHOST_USER_FS_CCW)
+
+
+static Property vhost_user_fs_ccw_properties[] = {
+    DEFINE_PROP_BIT("ioeventfd", VirtioCcwDevice, flags,
+                    VIRTIO_CCW_FLAG_USE_IOEVENTFD_BIT, true),
+    DEFINE_PROP_UINT32("max_revision", VirtioCcwDevice, max_rev,
+                       VIRTIO_CCW_MAX_REV),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
+static void vhost_user_fs_ccw_realize(VirtioCcwDevice *ccw_dev, Error **errp)
+{
+    VHostUserFSCcw *dev = VHOST_USER_FS_CCW(ccw_dev);
+    DeviceState *vdev = DEVICE(&dev->vdev);
+
+    qdev_set_parent_bus(vdev, BUS(&ccw_dev->bus));
+    object_property_set_bool(OBJECT(vdev), true, "realized", errp);
+}
+
+static void vhost_user_fs_ccw_instance_init(Object *obj)
+{
+    VHostUserFSCcw *dev = VHOST_USER_FS_CCW(obj);
+
+    virtio_instance_init_common(obj, &dev->vdev, sizeof(dev->vdev),
+                                TYPE_VHOST_USER_FS);
+}
+
+static void vhost_user_fs_ccw_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+    VirtIOCCWDeviceClass *k = VIRTIO_CCW_DEVICE_CLASS(klass);
+
+    k->realize = vhost_user_fs_ccw_realize;
+    device_class_set_props(dc,vhost_user_fs_ccw_properties);
+    set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
+}
+
+static const TypeInfo vhost_user_fs_ccw = {
+    .name          = TYPE_VHOST_USER_FS_CCW,
+    .parent        = TYPE_VIRTIO_CCW_DEVICE,
+    .instance_size = sizeof(VHostUserFSCcw),
+    .instance_init = vhost_user_fs_ccw_instance_init,
+    .class_init    = vhost_user_fs_ccw_class_init,
+};
+
+static void vhost_user_fs_ccw_register(void)
+{
+    type_register_static(&vhost_user_fs_ccw);
+}
+
+type_init(vhost_user_fs_ccw_register)
-- 
2.25.4

Re: [RFC 1/4] virtio: add vhost-user-fs-ccw device

[Cornelia Huck]

On Thu, 25 Jun 2020 12:04:27 +0200
Marc Hartmayer <mhartmay@linux.ibm.com> wrote:

> From: Halil Pasic <pasic@linux.ibm.com>
> 
> Wire up the CCW device for vhost-user-fs.
> 
> Signed-off-by: Halil Pasic <pasic@linux.ibm.com>
> ---
>  hw/s390x/Makefile.objs       |  1 +
>  hw/s390x/vhost-user-fs-ccw.c | 74 ++++++++++++++++++++++++++++++++++++
>  2 files changed, 75 insertions(+)
>  create mode 100644 hw/s390x/vhost-user-fs-ccw.c

(...)

> +static void vhost_user_fs_ccw_instance_init(Object *obj)
> +{
> +    VHostUserFSCcw *dev = VHOST_USER_FS_CCW(obj);

If it turns out to be a bug that pci is not forcing this to virtio-1,
you should add

    ccw_dev->force_revision_1 = true;

> +
> +    virtio_instance_init_common(obj, &dev->vdev, sizeof(dev->vdev),
> +                                TYPE_VHOST_USER_FS);
> +}

(...)

LGTM.

[RFC 2/4] libvhost-user: print invalid address on vu_panic

[Marc Hartmayer]

This can be helpful for debugging.

Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com>
---
 contrib/libvhost-user/libvhost-user.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/contrib/libvhost-user/libvhost-user.c b/contrib/libvhost-user/libvhost-user.c
index d315db139606..9e8750a9dabc 100644
--- a/contrib/libvhost-user/libvhost-user.c
+++ b/contrib/libvhost-user/libvhost-user.c
@@ -2432,7 +2432,7 @@ virtqueue_map_desc(VuDev *dev,
 
         iov[num_sg].iov_base = vu_gpa_to_va(dev, &len, pa);
         if (iov[num_sg].iov_base == NULL) {
-            vu_panic(dev, "virtio: invalid address for buffers");
+            vu_panic(dev, "virtio: invalid address 0x%lx for buffers", pa);
             return;
         }
         iov[num_sg].iov_len = len;
-- 
2.25.4

[RFC 3/4] libvhost-user: handle endianness as mandated by the spec

[Marc Hartmayer]

Since virtio existed even before it got standardized, the virtio
standard defines the following types of virtio devices:

 + legacy device (pre-virtio 1.0)
 + non-legacy or VIRTIO 1.0 device
 + transitional device (which can act both as legacy and non-legacy)

Virtio 1.0 defines the fields of the virtqueues as little endian,
while legacy uses guest's native endian [1]. Currently libvhost-user
does not handle virtio endianness at all, i.e. it works only if the
native endianness matches with whatever is actually needed. That means
things break spectacularly on big-endian targets. Let us handle
virtio endianness as required by the virtio specification [1].

[1] https://docs.oasis-open.org/virtio/virtio/v1.1/cs01/virtio-v1.1-cs01.html#x1-210003

Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com>
---
 contrib/libvhost-user/libvhost-access.h |  88 +++++++++++++++++
 contrib/libvhost-user/libvhost-user.c   | 122 ++++++++++++------------
 2 files changed, 151 insertions(+), 59 deletions(-)
 create mode 100644 contrib/libvhost-user/libvhost-access.h

diff --git a/contrib/libvhost-user/libvhost-access.h b/contrib/libvhost-user/libvhost-access.h
new file mode 100644
index 000000000000..a4fc334fe134
--- /dev/null
+++ b/contrib/libvhost-user/libvhost-access.h
@@ -0,0 +1,88 @@
+#ifndef LIBVHOST_ACCESS_H
+
+#include "qemu/osdep.h"
+#include "qemu/bswap.h"
+
+#include "libvhost-user.h"
+
+/* TODO attempt to use poisoned TARGET_PPC64/ARM */
+/* #if defined(TARGET_PPC64) || defined(TARGET_ARM) */
+/* #define LEGACY_VIRTIO_IS_BIENDIAN 1 */
+/* #endif */
+
+static inline bool vu_is_big_endian(VuDev *dev)
+{
+    if (!vu_has_feature(dev, VIRTIO_F_VERSION_1)) {
+        /* TODO there is no `device_endian` attribute for VuDev */
+        /* assert(vdev->device_endian != VIRTIO_DEVICE_ENDIAN_UNKNOWN); */
+        /* return vdev->device_endian == VIRTIO_DEVICE_ENDIAN_BIG; */
+    }
+
+    /* Devices conforming to VIRTIO 1.0 or later are always LE. */
+    return false;
+}
+
+static inline bool vu_access_is_big_endian(VuDev *dev)
+{
+#if defined(LEGACY_VIRTIO_IS_BIENDIAN)
+    return vu_is_big_endian(dev);
+#elif defined(TARGET_WORDS_BIGENDIAN)
+    if (vu_has_feature(dev, VIRTIO_F_VERSION_1)) {
+        /* Devices conforming to VIRTIO 1.0 or later are always LE. */
+        return false;
+    }
+    return true;
+#else
+    return false;
+#endif
+}
+
+static inline void vu_stw_p(VuDev *vdev, void *ptr, uint16_t v)
+{
+    if (vu_access_is_big_endian(vdev)) {
+        stw_be_p(ptr, v);
+    } else {
+        stw_le_p(ptr, v);
+    }
+}
+
+static inline void vu_stl_p(VuDev *vdev, void *ptr, uint32_t v)
+{
+    if (vu_access_is_big_endian(vdev)) {
+        stl_be_p(ptr, v);
+    } else {
+        stl_le_p(ptr, v);
+    }
+}
+
+static inline void vu_stq_p(VuDev *vdev, void *ptr, uint64_t v)
+{
+    if (vu_access_is_big_endian(vdev)) {
+        stq_be_p(ptr, v);
+    } else {
+        stq_le_p(ptr, v);
+    }
+}
+
+static inline int vu_lduw_p(VuDev *vdev, const void *ptr)
+{
+    if (vu_access_is_big_endian(vdev))
+        return lduw_be_p(ptr);
+    return lduw_le_p(ptr);
+}
+
+static inline int vu_ldl_p(VuDev *vdev, const void *ptr)
+{
+    if (vu_access_is_big_endian(vdev))
+        return ldl_be_p(ptr);
+    return ldl_le_p(ptr);
+}
+
+static inline uint64_t vu_ldq_p(VuDev *vdev, const void *ptr)
+{
+    if (vu_access_is_big_endian(vdev))
+        return ldq_be_p(ptr);
+    return ldq_le_p(ptr);
+}
+
+#endif /* LIBVHOST_ACCESS_h */
diff --git a/contrib/libvhost-user/libvhost-user.c b/contrib/libvhost-user/libvhost-user.c
index 9e8750a9dabc..200a90206ed2 100644
--- a/contrib/libvhost-user/libvhost-user.c
+++ b/contrib/libvhost-user/libvhost-user.c
@@ -45,6 +45,10 @@
 #include "qemu/memfd.h"
 
 #include "libvhost-user.h"
+static inline bool has_feature(uint64_t features, unsigned int fbit);
+static inline bool vu_has_feature(VuDev *dev, unsigned int fbit);
+
+#include "libvhost-access.h"
 
 /* usually provided by GLib */
 #ifndef MIN
@@ -1074,7 +1078,7 @@ vu_set_vring_addr_exec(VuDev *dev, VhostUserMsg *vmsg)
         return false;
     }
 
-    vq->used_idx = vq->vring.used->idx;
+    vq->used_idx = vu_lduw_p(dev, &vq->vring.used->idx);
 
     if (vq->last_avail_idx != vq->used_idx) {
         bool resume = dev->iface->queue_is_processed_in_order &&
@@ -1191,7 +1195,7 @@ vu_check_queue_inflights(VuDev *dev, VuVirtq *vq)
         return 0;
     }
 
-    vq->used_idx = vq->vring.used->idx;
+    vq->used_idx = vu_lduw_p(dev, &vq->vring.used->idx);
     vq->resubmit_num = 0;
     vq->resubmit_list = NULL;
     vq->counter = 0;
@@ -2019,35 +2023,35 @@ vu_queue_started(const VuDev *dev, const VuVirtq *vq)
 }
 
 static inline uint16_t
-vring_avail_flags(VuVirtq *vq)
+vring_avail_flags(VuDev *dev, VuVirtq *vq)
 {
-    return vq->vring.avail->flags;
+    return vu_lduw_p(dev, &vq->vring.avail->flags);
 }
 
 static inline uint16_t
-vring_avail_idx(VuVirtq *vq)
+vring_avail_idx(VuDev *dev, VuVirtq *vq)
 {
-    vq->shadow_avail_idx = vq->vring.avail->idx;
+    vq->shadow_avail_idx = vu_lduw_p(dev, &vq->vring.avail->idx);
 
     return vq->shadow_avail_idx;
 }
 
 static inline uint16_t
-vring_avail_ring(VuVirtq *vq, int i)
+vring_avail_ring(VuDev *dev, VuVirtq *vq, int i)
 {
-    return vq->vring.avail->ring[i];
+    return vu_lduw_p(dev, &vq->vring.avail->ring[i]);
 }
 
 static inline uint16_t
-vring_get_used_event(VuVirtq *vq)
+vring_get_used_event(VuDev *dev, VuVirtq *vq)
 {
-    return vring_avail_ring(vq, vq->vring.num);
+    return vring_avail_ring(dev, vq, vq->vring.num);
 }
 
 static int
 virtqueue_num_heads(VuDev *dev, VuVirtq *vq, unsigned int idx)
 {
-    uint16_t num_heads = vring_avail_idx(vq) - idx;
+    uint16_t num_heads = vring_avail_idx(dev, vq) - idx;
 
     /* Check it isn't doing very strange things with descriptor numbers. */
     if (num_heads > vq->vring.num) {
@@ -2070,7 +2074,7 @@ virtqueue_get_head(VuDev *dev, VuVirtq *vq,
 {
     /* Grab the next descriptor number they're advertising, and increment
      * the index we've seen. */
-    *head = vring_avail_ring(vq, idx % vq->vring.num);
+    *head = vring_avail_ring(dev, vq, idx % vq->vring.num);
 
     /* If their number is silly, that's a fatal mistake. */
     if (*head >= vq->vring.num) {
@@ -2123,12 +2127,12 @@ virtqueue_read_next_desc(VuDev *dev, struct vring_desc *desc,
                          int i, unsigned int max, unsigned int *next)
 {
     /* If this descriptor says it doesn't chain, we're done. */
-    if (!(desc[i].flags & VRING_DESC_F_NEXT)) {
+    if (!(vu_lduw_p(dev, &desc[i].flags) & VRING_DESC_F_NEXT)) {
         return VIRTQUEUE_READ_DESC_DONE;
     }
 
     /* Check they're not leading us off end of descriptors. */
-    *next = desc[i].next;
+    *next = vu_lduw_p(dev, &desc[i].next);
     /* Make sure compiler knows to grab that: we don't want it changing! */
     smp_wmb();
 
@@ -2171,8 +2175,8 @@ vu_queue_get_avail_bytes(VuDev *dev, VuVirtq *vq, unsigned int *in_bytes,
         }
         desc = vq->vring.desc;
 
-        if (desc[i].flags & VRING_DESC_F_INDIRECT) {
-            if (desc[i].len % sizeof(struct vring_desc)) {
+        if (vu_lduw_p(dev, &desc[i].flags) & VRING_DESC_F_INDIRECT) {
+            if (vu_ldl_p(dev, &desc[i].len) % sizeof(struct vring_desc)) {
                 vu_panic(dev, "Invalid size for indirect buffer table");
                 goto err;
             }
@@ -2185,8 +2189,8 @@ vu_queue_get_avail_bytes(VuDev *dev, VuVirtq *vq, unsigned int *in_bytes,
 
             /* loop over the indirect descriptor table */
             indirect = 1;
-            desc_addr = desc[i].addr;
-            desc_len = desc[i].len;
+            desc_addr = vu_ldq_p(dev, &desc[i].addr);
+            desc_len = vu_ldl_p(dev, &desc[i].len);
             max = desc_len / sizeof(struct vring_desc);
             read_len = desc_len;
             desc = vu_gpa_to_va(dev, &read_len, desc_addr);
@@ -2213,10 +2217,10 @@ vu_queue_get_avail_bytes(VuDev *dev, VuVirtq *vq, unsigned int *in_bytes,
                 goto err;
             }
 
-            if (desc[i].flags & VRING_DESC_F_WRITE) {
-                in_total += desc[i].len;
+            if (vu_lduw_p(dev, &desc[i].flags) & VRING_DESC_F_WRITE) {
+                in_total += vu_ldl_p(dev, &desc[i].len);
             } else {
-                out_total += desc[i].len;
+                out_total += vu_ldl_p(dev, &desc[i].len);
             }
             if (in_total >= max_in_bytes && out_total >= max_out_bytes) {
                 goto done;
@@ -2277,7 +2281,7 @@ vu_queue_empty(VuDev *dev, VuVirtq *vq)
         return false;
     }
 
-    return vring_avail_idx(vq) == vq->last_avail_idx;
+    return vring_avail_idx(dev, vq) == vq->last_avail_idx;
 }
 
 static bool
@@ -2296,14 +2300,14 @@ vring_notify(VuDev *dev, VuVirtq *vq)
     }
 
     if (!vu_has_feature(dev, VIRTIO_RING_F_EVENT_IDX)) {
-        return !(vring_avail_flags(vq) & VRING_AVAIL_F_NO_INTERRUPT);
+        return !(vring_avail_flags(dev, vq) & VRING_AVAIL_F_NO_INTERRUPT);
     }
 
     v = vq->signalled_used_valid;
     vq->signalled_used_valid = true;
     old = vq->signalled_used;
     new = vq->signalled_used = vq->used_idx;
-    return !v || vring_need_event(vring_get_used_event(vq), new, old);
+    return !v || vring_need_event(vring_get_used_event(dev, vq), new, old);
 }
 
 static void _vu_queue_notify(VuDev *dev, VuVirtq *vq, bool sync)
@@ -2361,33 +2365,33 @@ void vu_queue_notify_sync(VuDev *dev, VuVirtq *vq)
 }
 
 static inline void
-vring_used_flags_set_bit(VuVirtq *vq, int mask)
+vring_used_flags_set_bit(VuDev *dev, VuVirtq *vq, int mask)
+{
+    uint16_t *flags;
+
+    flags = (uint16_t *)(char*)vq->vring.used +
+        offsetof(struct vring_used, flags);
+    vu_stw_p(dev, flags, vu_lduw_p(dev, flags) | mask);
+}
+
+static inline void
+vring_used_flags_unset_bit(VuDev *dev, VuVirtq *vq, int mask)
 {
     uint16_t *flags;
 
     flags = (uint16_t *)((char*)vq->vring.used +
                          offsetof(struct vring_used, flags));
-    *flags |= mask;
+    vu_stw_p(dev, flags, vu_lduw_p(dev, flags) & ~mask);
 }
 
 static inline void
-vring_used_flags_unset_bit(VuVirtq *vq, int mask)
-{
-    uint16_t *flags;
-
-    flags = (uint16_t *)((char*)vq->vring.used +
-                         offsetof(struct vring_used, flags));
-    *flags &= ~mask;
-}
-
-static inline void
-vring_set_avail_event(VuVirtq *vq, uint16_t val)
+vring_set_avail_event(VuDev *dev, VuVirtq *vq, uint16_t val)
 {
     if (!vq->notification) {
         return;
     }
 
-    *((uint16_t *) &vq->vring.used->ring[vq->vring.num]) = val;
+    vu_stw_p(dev, &vq->vring.used->ring[vq->vring.num], val);
 }
 
 void
@@ -2395,11 +2399,11 @@ vu_queue_set_notification(VuDev *dev, VuVirtq *vq, int enable)
 {
     vq->notification = enable;
     if (vu_has_feature(dev, VIRTIO_RING_F_EVENT_IDX)) {
-        vring_set_avail_event(vq, vring_avail_idx(vq));
+        vring_set_avail_event(dev, vq, vring_avail_idx(dev, vq));
     } else if (enable) {
-        vring_used_flags_unset_bit(vq, VRING_USED_F_NO_NOTIFY);
+        vring_used_flags_unset_bit(dev, vq, VRING_USED_F_NO_NOTIFY);
     } else {
-        vring_used_flags_set_bit(vq, VRING_USED_F_NO_NOTIFY);
+        vring_used_flags_set_bit(dev, vq, VRING_USED_F_NO_NOTIFY);
     }
     if (enable) {
         /* Expose avail event/used flags before caller checks the avail idx. */
@@ -2476,14 +2480,14 @@ vu_queue_map_desc(VuDev *dev, VuVirtq *vq, unsigned int idx, size_t sz)
     struct vring_desc desc_buf[VIRTQUEUE_MAX_SIZE];
     int rc;
 
-    if (desc[i].flags & VRING_DESC_F_INDIRECT) {
-        if (desc[i].len % sizeof(struct vring_desc)) {
+    if (vu_lduw_p(dev, &desc[i].flags) & VRING_DESC_F_INDIRECT) {
+        if (vu_ldl_p(dev, &desc[i].len) % sizeof(struct vring_desc)) {
             vu_panic(dev, "Invalid size for indirect buffer table");
         }
 
         /* loop over the indirect descriptor table */
-        desc_addr = desc[i].addr;
-        desc_len = desc[i].len;
+        desc_addr = vu_ldq_p(dev, &desc[i].addr);
+        desc_len = vu_ldl_p(dev, &desc[i].len);
         max = desc_len / sizeof(struct vring_desc);
         read_len = desc_len;
         desc = vu_gpa_to_va(dev, &read_len, desc_addr);
@@ -2505,10 +2509,10 @@ vu_queue_map_desc(VuDev *dev, VuVirtq *vq, unsigned int idx, size_t sz)
 
     /* Collect all the descriptors */
     do {
-        if (desc[i].flags & VRING_DESC_F_WRITE) {
+        if (vu_lduw_p(dev, &desc[i].flags) & VRING_DESC_F_WRITE) {
             virtqueue_map_desc(dev, &in_num, iov + out_num,
                                VIRTQUEUE_MAX_SIZE - out_num, true,
-                               desc[i].addr, desc[i].len);
+                               vu_ldq_p(dev, &desc[i].addr), vu_ldl_p(dev, &desc[i].len));
         } else {
             if (in_num) {
                 vu_panic(dev, "Incorrect order for descriptors");
@@ -2516,7 +2520,7 @@ vu_queue_map_desc(VuDev *dev, VuVirtq *vq, unsigned int idx, size_t sz)
             }
             virtqueue_map_desc(dev, &out_num, iov,
                                VIRTQUEUE_MAX_SIZE, false,
-                               desc[i].addr, desc[i].len);
+                               vu_ldq_p(dev, &desc[i].addr), vu_ldl_p(dev, &desc[i].len));
         }
 
         /* If we've got too many, that implies a descriptor loop. */
@@ -2642,7 +2646,7 @@ vu_queue_pop(VuDev *dev, VuVirtq *vq, size_t sz)
     }
 
     if (vu_has_feature(dev, VIRTIO_RING_F_EVENT_IDX)) {
-        vring_set_avail_event(vq, vq->last_avail_idx);
+        vring_set_avail_event(dev, vq, vq->last_avail_idx);
     }
 
     elem = vu_queue_map_desc(dev, vq, head, sz);
@@ -2712,14 +2716,14 @@ vu_log_queue_fill(VuDev *dev, VuVirtq *vq,
     max = vq->vring.num;
     i = elem->index;
 
-    if (desc[i].flags & VRING_DESC_F_INDIRECT) {
-        if (desc[i].len % sizeof(struct vring_desc)) {
+    if (vu_lduw_p(dev, &desc[i].flags) & VRING_DESC_F_INDIRECT) {
+        if (vu_ldl_p(dev, &desc[i].len) % sizeof(struct vring_desc)) {
             vu_panic(dev, "Invalid size for indirect buffer table");
         }
 
         /* loop over the indirect descriptor table */
-        desc_addr = desc[i].addr;
-        desc_len = desc[i].len;
+        desc_addr = vu_ldq_p(dev, &desc[i].addr);
+        desc_len = vu_ldl_p(dev, &desc[i].len);
         max = desc_len / sizeof(struct vring_desc);
         read_len = desc_len;
         desc = vu_gpa_to_va(dev, &read_len, desc_addr);
@@ -2745,9 +2749,9 @@ vu_log_queue_fill(VuDev *dev, VuVirtq *vq,
             return;
         }
 
-        if (desc[i].flags & VRING_DESC_F_WRITE) {
-            min = MIN(desc[i].len, len);
-            vu_log_write(dev, desc[i].addr, min);
+        if (vu_lduw_p(dev, &desc[i].flags) & VRING_DESC_F_WRITE) {
+            min = MIN(vu_ldl_p(dev, &desc[i].len), len);
+            vu_log_write(dev, vu_ldq_p(dev, &desc[i].addr), min);
             len -= min;
         }
 
@@ -2772,15 +2776,15 @@ vu_queue_fill(VuDev *dev, VuVirtq *vq,
 
     idx = (idx + vq->used_idx) % vq->vring.num;
 
-    uelem.id = elem->index;
-    uelem.len = len;
+    vu_stl_p(dev, &uelem.id, elem->index);
+    vu_stl_p(dev, &uelem.len, len);
     vring_used_write(dev, vq, &uelem, idx);
 }
 
 static inline
 void vring_used_idx_set(VuDev *dev, VuVirtq *vq, uint16_t val)
 {
-    vq->vring.used->idx = val;
+    vu_stw_p(dev, &vq->vring.used->idx, val);
     vu_log_write(dev,
                  vq->vring.log_guest_addr + offsetof(struct vring_used, idx),
                  sizeof(vq->vring.used->idx));
-- 
2.25.4

[RFC 4/4] HACK: Hard-code the libvhost-user.o-cflags for s390x

[Marc Hartmayer]

This patch exists only to show the actual problem that libvhost-user
and it's users are architecture dependent as soon as we're trying to
support legacy virtio.

Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com>
---
 Makefile.objs                           | 1 +
 contrib/libvhost-user/libvhost-access.h | 7 +++----
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/Makefile.objs b/Makefile.objs
index 7ce2588b89a3..abfb1912e456 100644
--- a/Makefile.objs
+++ b/Makefile.objs
@@ -111,6 +111,7 @@ elf2dmp-obj-y = contrib/elf2dmp/
 ivshmem-client-obj-$(CONFIG_IVSHMEM) = contrib/ivshmem-client/
 ivshmem-server-obj-$(CONFIG_IVSHMEM) = contrib/ivshmem-server/
 libvhost-user-obj-y = contrib/libvhost-user/
+libvhost-user.o-cflags += -iquote $(SRC_PATH)/s390x-softmmu -DNEED_CPU_H
 vhost-user-scsi.o-cflags := $(LIBISCSI_CFLAGS)
 vhost-user-scsi.o-libs := $(LIBISCSI_LIBS)
 vhost-user-scsi-obj-y = contrib/vhost-user-scsi/
diff --git a/contrib/libvhost-user/libvhost-access.h b/contrib/libvhost-user/libvhost-access.h
index a4fc334fe134..e9451ae0fbc6 100644
--- a/contrib/libvhost-user/libvhost-access.h
+++ b/contrib/libvhost-user/libvhost-access.h
@@ -5,10 +5,9 @@
 
 #include "libvhost-user.h"
 
-/* TODO attempt to use poisoned TARGET_PPC64/ARM */
-/* #if defined(TARGET_PPC64) || defined(TARGET_ARM) */
-/* #define LEGACY_VIRTIO_IS_BIENDIAN 1 */
-/* #endif */
+#if defined(TARGET_PPC64) || defined(TARGET_ARM)
+#define LEGACY_VIRTIO_IS_BIENDIAN 1
+#endif
 
 static inline bool vu_is_big_endian(VuDev *dev)
 {
-- 
2.25.4

Re: [RFC 0/4] Enable virtio-fs on s390x

[no-reply]

Patchew URL: https://patchew.org/QEMU/20200625100430.22407-1-mhartmay@linux.ibm.com/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Subject: [RFC 0/4] Enable virtio-fs on s390x
Type: series
Message-id: 20200625100430.22407-1-mhartmay@linux.ibm.com

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 - [tag update]      patchew/20200622153318.751107-1-berrange@redhat.com -> patchew/20200622153318.751107-1-berrange@redhat.com
 * [new tag]         patchew/20200625100430.22407-1-mhartmay@linux.ibm.com -> patchew/20200625100430.22407-1-mhartmay@linux.ibm.com
 * [new tag]         patchew/20200625100811.12690-1-peter.maydell@linaro.org -> patchew/20200625100811.12690-1-peter.maydell@linaro.org
Switched to a new branch 'test'
b2cd719 HACK: Hard-code the libvhost-user.o-cflags for s390x
f0ba6d5 libvhost-user: handle endianness as mandated by the spec
5f2feba libvhost-user: print invalid address on vu_panic
19994e0 virtio: add vhost-user-fs-ccw device

=== OUTPUT BEGIN ===
1/4 Checking commit 19994e01a67c (virtio: add vhost-user-fs-ccw device)
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#25: 
new file mode 100644

ERROR: space required after that ',' (ctx:VxV)
#86: FILE: hw/s390x/vhost-user-fs-ccw.c:57:
+    device_class_set_props(dc,vhost_user_fs_ccw_properties);
                              ^

total: 1 errors, 1 warnings, 81 lines checked

Patch 1/4 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

2/4 Checking commit 5f2feba1e3c5 (libvhost-user: print invalid address on vu_panic)
3/4 Checking commit f0ba6d5325c1 (libvhost-user: handle endianness as mandated by the spec)
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#27: 
new file mode 100644

ERROR: braces {} are necessary for all arms of this statement
#100: FILE: contrib/libvhost-user/libvhost-access.h:69:
+    if (vu_access_is_big_endian(vdev))
[...]

ERROR: braces {} are necessary for all arms of this statement
#107: FILE: contrib/libvhost-user/libvhost-access.h:76:
+    if (vu_access_is_big_endian(vdev))
[...]

ERROR: braces {} are necessary for all arms of this statement
#114: FILE: contrib/libvhost-user/libvhost-access.h:83:
+    if (vu_access_is_big_endian(vdev))
[...]

WARNING: line over 80 characters
#375: FILE: contrib/libvhost-user/libvhost-user.c:2515:
+                               vu_ldq_p(dev, &desc[i].addr), vu_ldl_p(dev, &desc[i].len));

WARNING: line over 80 characters
#384: FILE: contrib/libvhost-user/libvhost-user.c:2523:
+                               vu_ldq_p(dev, &desc[i].addr), vu_ldl_p(dev, &desc[i].len));

total: 3 errors, 3 warnings, 392 lines checked

Patch 3/4 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

4/4 Checking commit b2cd71997828 (HACK: Hard-code the libvhost-user.o-cflags for s390x)
=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20200625100430.22407-1-mhartmay@linux.ibm.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [RFC 0/4] Enable virtio-fs on s390x

[no-reply]

Patchew URL: https://patchew.org/QEMU/20200625100430.22407-1-mhartmay@linux.ibm.com/



Hi,

This series failed the docker-quick@centos7 build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

=== TEST SCRIPT BEGIN ===
#!/bin/bash
make docker-image-centos7 V=1 NETWORK=1
time make docker-test-quick@centos7 SHOW_ENV=1 J=14 NETWORK=1
=== TEST SCRIPT END ===

  CC      qga/qapi-generated/qga-qapi-commands.o
  CC      qga/qapi-generated/qga-qapi-init-commands.o
In file included from /tmp/qemu-test/src/contrib/libvhost-user/libvhost-user.c:44:0:
/tmp/qemu-test/src/include/qemu/osdep.h:32:27: fatal error: config-target.h: No such file or directory
 #include "config-target.h"
                           ^
compilation terminated.
---
  BUILD   pc-bios/optionrom/linuxboot_dma.raw
  SIGN    pc-bios/optionrom/pvh.bin
  SIGN    pc-bios/optionrom/linuxboot_dma.bin
make: *** [contrib/libvhost-user/libvhost-user.o] Error 1
make: *** Waiting for unfinished jobs....
Traceback (most recent call last):
  File "./tests/docker/docker.py", line 669, in <module>
---
    raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', '--label', 'com.qemu.instance.uuid=7638277b753647899fcb0c31ef086305', '-u', '1001', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 'TARGET_LIST=', '-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', '-e', 'SHOW_ENV=1', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', '/home/patchew/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', '/var/tmp/patchew-tester-tmp-uuens814/src/docker-src.2020-06-25-06.13.41.17712:/var/tmp/qemu:z,ro', 'qemu:centos7', '/var/tmp/qemu/run', 'test-quick']' returned non-zero exit status 2.
filter=--filter=label=com.qemu.instance.uuid=7638277b753647899fcb0c31ef086305
make[1]: *** [docker-run] Error 1
make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-uuens814/src'
make: *** [docker-run-test-quick@centos7] Error 2

real    2m49.544s
user    0m8.892s


The full log is available at
http://patchew.org/logs/20200625100430.22407-1-mhartmay@linux.ibm.com/testing.docker-quick@centos7/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [RFC 0/4] Enable virtio-fs on s390x

[Cornelia Huck]

On Thu, 25 Jun 2020 12:04:26 +0200
Marc Hartmayer <mhartmay@linux.ibm.com> wrote:

> This RFC is about enabling virtio-fs on s390x. For that we need
>  + some shim code (first patch), and we need
>  + libvhost-user to deal with virtio endiannes as mandated by the spec.
>  
> The second part is trickier, because unlike QEMU we are not certain
> about the guest's native endianness, which is needed to handle the
> legacy-interface appropriately. In fact, this is the reason why just
> RFC.
> 
> One of the open questions is whether to build separate versions, one
> for guest little endian and one for guest big endian, or do we want
> something like a command line option? (Digression on the libvirt
> modeling)
> 
> A third option would be to refuse legacy altogether.

The third option looks the most tempting to me. It is a new device, so
I don't think there's much of a case to be made for pre-virtio-1
support?

Re: [RFC 0/4] Enable virtio-fs on s390x

[Halil Pasic]

On Thu, 25 Jun 2020 12:17:55 +0200
Cornelia Huck <cohuck@redhat.com> wrote:

> On Thu, 25 Jun 2020 12:04:26 +0200
> Marc Hartmayer <mhartmay@linux.ibm.com> wrote:
> 
> > This RFC is about enabling virtio-fs on s390x. For that we need
> >  + some shim code (first patch), and we need
> >  + libvhost-user to deal with virtio endiannes as mandated by the spec.
> >  
> > The second part is trickier, because unlike QEMU we are not certain
> > about the guest's native endianness, which is needed to handle the
> > legacy-interface appropriately. In fact, this is the reason why just
> > RFC.
> > 
> > One of the open questions is whether to build separate versions, one
> > for guest little endian and one for guest big endian, or do we want
> > something like a command line option? (Digression on the libvirt
> > modeling)
> > 
> > A third option would be to refuse legacy altogether.
> 
> The third option looks the most tempting to me. It is a new device, so
> I don't think there's much of a case to be made for pre-virtio-1
> support?
> 
> 

Yes, virtio-fs is a new device, but libvhost-user is not specific to
virtio-fs.

Regards,
Halil

Re: [RFC 0/4] Enable virtio-fs on s390x

[Daniel P. Berrangé]

On Thu, Jun 25, 2020 at 12:04:26PM +0200, Marc Hartmayer wrote:
> This RFC is about enabling virtio-fs on s390x. For that we need
>  + some shim code (first patch), and we need
>  + libvhost-user to deal with virtio endiannes as mandated by the spec.
>  
> The second part is trickier, because unlike QEMU we are not certain
> about the guest's native endianness, which is needed to handle the
> legacy-interface appropriately. In fact, this is the reason why just
> RFC.
> 
> One of the open questions is whether to build separate versions, one
> for guest little endian and one for guest big endian, or do we want
> something like a command line option? (Digression on the libvirt
> modeling)

When you talk about  big vs little endian, are you referring to TCG
scenarios with mixed host/guest arch, or arches which can support
either endianess, or both ? i guess it doesn't matter actually, as
I think the latter forces a specific answer.

Considering that some architectures allow the guest OS to flip between
big & little endian as they boot, libvirt cannot know what endianess
the guest is using when it launches virtiofsd. It thus cannot pick
between two different endianness builds of virtiofsd automatically.
This would force the user to tell libvirt what arch the guest is using
at the time they define the guest. This is an undesirable restriction
for use cases where the admin of the guest OS has no direct control
over the host config.

IOW, I think the only practical answer is to have a single binary that
automagically does the right thing at runtime according to guest
endianess that currently is in use.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [RFC 0/4] Enable virtio-fs on s390x

[Cornelia Huck]

On Thu, 25 Jun 2020 11:19:35 +0100
Daniel P. Berrangé <berrange@redhat.com> wrote:

> On Thu, Jun 25, 2020 at 12:04:26PM +0200, Marc Hartmayer wrote:
> > This RFC is about enabling virtio-fs on s390x. For that we need
> >  + some shim code (first patch), and we need
> >  + libvhost-user to deal with virtio endiannes as mandated by the spec.
> >  
> > The second part is trickier, because unlike QEMU we are not certain
> > about the guest's native endianness, which is needed to handle the
> > legacy-interface appropriately. In fact, this is the reason why just
> > RFC.
> > 
> > One of the open questions is whether to build separate versions, one
> > for guest little endian and one for guest big endian, or do we want
> > something like a command line option? (Digression on the libvirt
> > modeling)  
> 
> When you talk about  big vs little endian, are you referring to TCG
> scenarios with mixed host/guest arch, or arches which can support
> either endianess, or both ? i guess it doesn't matter actually, as
> I think the latter forces a specific answer.
> 
> Considering that some architectures allow the guest OS to flip between
> big & little endian as they boot, libvirt cannot know what endianess
> the guest is using when it launches virtiofsd. It thus cannot pick
> between two different endianness builds of virtiofsd automatically.
> This would force the user to tell libvirt what arch the guest is using
> at the time they define the guest. This is an undesirable restriction
> for use cases where the admin of the guest OS has no direct control
> over the host config.

Right, but that is in practice only a problem for legacy devices, isn't
it? The standard says that non-legacy devices use little-endian
everywhere; it's the legacy 'device endian' that is causing us
headaches.

Which leads to the question: Do we really need to support legacy
virtio-fs devices, or can we just force virtio-1, as many (most?) newer
virtio devices do?

> 
> IOW, I think the only practical answer is to have a single binary that
> automagically does the right thing at runtime according to guest
> endianess that currently is in use.
> 
> Regards,
> Daniel

Re: [RFC 0/4] Enable virtio-fs on s390x

[Daniel P. Berrangé]

On Thu, Jun 25, 2020 at 12:31:36PM +0200, Cornelia Huck wrote:
> On Thu, 25 Jun 2020 11:19:35 +0100
> Daniel P. Berrangé <berrange@redhat.com> wrote:
> 
> > On Thu, Jun 25, 2020 at 12:04:26PM +0200, Marc Hartmayer wrote:
> > > This RFC is about enabling virtio-fs on s390x. For that we need
> > >  + some shim code (first patch), and we need
> > >  + libvhost-user to deal with virtio endiannes as mandated by the spec.
> > >  
> > > The second part is trickier, because unlike QEMU we are not certain
> > > about the guest's native endianness, which is needed to handle the
> > > legacy-interface appropriately. In fact, this is the reason why just
> > > RFC.
> > > 
> > > One of the open questions is whether to build separate versions, one
> > > for guest little endian and one for guest big endian, or do we want
> > > something like a command line option? (Digression on the libvirt
> > > modeling)  
> > 
> > When you talk about  big vs little endian, are you referring to TCG
> > scenarios with mixed host/guest arch, or arches which can support
> > either endianess, or both ? i guess it doesn't matter actually, as
> > I think the latter forces a specific answer.
> > 
> > Considering that some architectures allow the guest OS to flip between
> > big & little endian as they boot, libvirt cannot know what endianess
> > the guest is using when it launches virtiofsd. It thus cannot pick
> > between two different endianness builds of virtiofsd automatically.
> > This would force the user to tell libvirt what arch the guest is using
> > at the time they define the guest. This is an undesirable restriction
> > for use cases where the admin of the guest OS has no direct control
> > over the host config.
> 
> Right, but that is in practice only a problem for legacy devices, isn't
> it? The standard says that non-legacy devices use little-endian
> everywhere; it's the legacy 'device endian' that is causing us
> headaches.
> 
> Which leads to the question: Do we really need to support legacy
> virtio-fs devices, or can we just force virtio-1, as many (most?) newer
> virtio devices do?

I'd hope virtio-fs is already forced to modern only, as there's no legacy
PCI ID assigned to it in the spec.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [RFC 0/4] Enable virtio-fs on s390x

[Cornelia Huck]

On Thu, 25 Jun 2020 11:39:24 +0100
Daniel P. Berrangé <berrange@redhat.com> wrote:

> On Thu, Jun 25, 2020 at 12:31:36PM +0200, Cornelia Huck wrote:
> > On Thu, 25 Jun 2020 11:19:35 +0100
> > Daniel P. Berrangé <berrange@redhat.com> wrote:
> >   
> > > On Thu, Jun 25, 2020 at 12:04:26PM +0200, Marc Hartmayer wrote:  
> > > > This RFC is about enabling virtio-fs on s390x. For that we need
> > > >  + some shim code (first patch), and we need
> > > >  + libvhost-user to deal with virtio endiannes as mandated by the spec.
> > > >  
> > > > The second part is trickier, because unlike QEMU we are not certain
> > > > about the guest's native endianness, which is needed to handle the
> > > > legacy-interface appropriately. In fact, this is the reason why just
> > > > RFC.
> > > > 
> > > > One of the open questions is whether to build separate versions, one
> > > > for guest little endian and one for guest big endian, or do we want
> > > > something like a command line option? (Digression on the libvirt
> > > > modeling)    
> > > 
> > > When you talk about  big vs little endian, are you referring to TCG
> > > scenarios with mixed host/guest arch, or arches which can support
> > > either endianess, or both ? i guess it doesn't matter actually, as
> > > I think the latter forces a specific answer.
> > > 
> > > Considering that some architectures allow the guest OS to flip between
> > > big & little endian as they boot, libvirt cannot know what endianess
> > > the guest is using when it launches virtiofsd. It thus cannot pick
> > > between two different endianness builds of virtiofsd automatically.
> > > This would force the user to tell libvirt what arch the guest is using
> > > at the time they define the guest. This is an undesirable restriction
> > > for use cases where the admin of the guest OS has no direct control
> > > over the host config.  
> > 
> > Right, but that is in practice only a problem for legacy devices, isn't
> > it? The standard says that non-legacy devices use little-endian
> > everywhere; it's the legacy 'device endian' that is causing us
> > headaches.
> > 
> > Which leads to the question: Do we really need to support legacy
> > virtio-fs devices, or can we just force virtio-1, as many (most?) newer
> > virtio devices do?  
> 
> I'd hope virtio-fs is already forced to modern only, as there's no legacy
> PCI ID assigned to it in the spec.

I did not find a call to virtio_pci_force_virtio_1(), so apparently not?

Re: [RFC 0/4] Enable virtio-fs on s390x

[Dr. David Alan Gilbert]

* Daniel P. Berrangé (berrange@redhat.com) wrote:
> On Thu, Jun 25, 2020 at 12:04:26PM +0200, Marc Hartmayer wrote:
> > This RFC is about enabling virtio-fs on s390x. For that we need
> >  + some shim code (first patch), and we need
> >  + libvhost-user to deal with virtio endiannes as mandated by the spec.
> >  
> > The second part is trickier, because unlike QEMU we are not certain
> > about the guest's native endianness, which is needed to handle the
> > legacy-interface appropriately. In fact, this is the reason why just
> > RFC.
> > 
> > One of the open questions is whether to build separate versions, one
> > for guest little endian and one for guest big endian, or do we want
> > something like a command line option? (Digression on the libvirt
> > modeling)
> 
> When you talk about  big vs little endian, are you referring to TCG
> scenarios with mixed host/guest arch, or arches which can support
> either endianess, or both ? i guess it doesn't matter actually, as
> I think the latter forces a specific answer.
> 
> Considering that some architectures allow the guest OS to flip between
> big & little endian as they boot, libvirt cannot know what endianess
> the guest is using when it launches virtiofsd. It thus cannot pick
> between two different endianness builds of virtiofsd automatically.
> This would force the user to tell libvirt what arch the guest is using
> at the time they define the guest. This is an undesirable restriction
> for use cases where the admin of the guest OS has no direct control
> over the host config.
> 
> IOW, I think the only practical answer is to have a single binary that
> automagically does the right thing at runtime according to guest
> endianess that currently is in use.

Modifying the fuse code in virtiofsd to handle cross-endian would be a
big job;  IMHO unless you really have a cross-endian need I'd just make
sure it deals ok with big<->big and little<->little.

Dave

> Regards,
> Daniel
> -- 
> |: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org         -o-            https://fstop138.berrange.com :|
> |: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
> 
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [RFC 0/4] Enable virtio-fs on s390x

[Halil Pasic]

On Thu, 25 Jun 2020 11:19:35 +0100
Daniel P. Berrangé <berrange@redhat.com> wrote:

> On Thu, Jun 25, 2020 at 12:04:26PM +0200, Marc Hartmayer wrote:
> > This RFC is about enabling virtio-fs on s390x. For that we need
> >  + some shim code (first patch), and we need
> >  + libvhost-user to deal with virtio endiannes as mandated by the spec.
> >  
> > The second part is trickier, because unlike QEMU we are not certain
> > about the guest's native endianness, which is needed to handle the
> > legacy-interface appropriately. In fact, this is the reason why just
> > RFC.
> > 
> > One of the open questions is whether to build separate versions, one
> > for guest little endian and one for guest big endian, or do we want
> > something like a command line option? (Digression on the libvirt
> > modeling)
> 
> When you talk about  big vs little endian, are you referring to TCG
> scenarios with mixed host/guest arch, or arches which can support
> either endianess, or both ? i guess it doesn't matter actually, as
> I think the latter forces a specific answer.
> 

Our primary concern is big endian cpu and little endian virtio because
virto 1.0 or better. But since we talk about a lib here, everything is
possible.

> Considering that some architectures allow the guest OS to flip between
> big & little endian as they boot, libvirt cannot know what endianess
> the guest is using when it launches virtiofsd. It thus cannot pick
> between two different endianness builds of virtiofsd automatically.
> This would force the user to tell libvirt what arch the guest is using
> at the time they define the guest. This is an undesirable restriction
> for use cases where the admin of the guest OS has no direct control
> over the host config.
> 
> IOW, I think the only practical answer is to have a single binary that
> automagically does the right thing at runtime according to guest
> endianess that currently is in use.
> 

The problem is that AFAIK there is no way to figure out what is the right
thing at the moment. I guess QEMU should now, but not virtiofsd or
whatever userspace virtio device. But I will double check if there is
something in the protocol addressing this.

Regards,
Halil

Re: [RFC 0/4] Enable virtio-fs on s390x

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1087 bytes --]

On Thu, Jun 25, 2020 at 12:04:26PM +0200, Marc Hartmayer wrote:
> This RFC is about enabling virtio-fs on s390x. For that we need
>  + some shim code (first patch), and we need
>  + libvhost-user to deal with virtio endiannes as mandated by the spec.
>  
> The second part is trickier, because unlike QEMU we are not certain
> about the guest's native endianness, which is needed to handle the
> legacy-interface appropriately. In fact, this is the reason why just
> RFC.
> 
> One of the open questions is whether to build separate versions, one
> for guest little endian and one for guest big endian, or do we want
> something like a command line option? (Digression on the libvirt
> modeling)
> 
> A third option would be to refuse legacy altogether.

I suggest the following:

1. Combinations that worked with libvhost-user in the past must not break.

2. New combinations should only support VIRTIO 1.0 and later.

This means continue to allow Legacy mode devices where they already run
today but don't add new code for the cases that didn't work.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [RFC 0/4] Enable virtio-fs on s390x

[Daniel P. Berrangé]

On Mon, Jun 29, 2020 at 01:53:05PM +0100, Stefan Hajnoczi wrote:
> On Thu, Jun 25, 2020 at 12:04:26PM +0200, Marc Hartmayer wrote:
> > This RFC is about enabling virtio-fs on s390x. For that we need
> >  + some shim code (first patch), and we need
> >  + libvhost-user to deal with virtio endiannes as mandated by the spec.
> >  
> > The second part is trickier, because unlike QEMU we are not certain
> > about the guest's native endianness, which is needed to handle the
> > legacy-interface appropriately. In fact, this is the reason why just
> > RFC.
> > 
> > One of the open questions is whether to build separate versions, one
> > for guest little endian and one for guest big endian, or do we want
> > something like a command line option? (Digression on the libvirt
> > modeling)
> > 
> > A third option would be to refuse legacy altogether.
> 
> I suggest the following:
> 
> 1. Combinations that worked with libvhost-user in the past must not break.
> 
> 2. New combinations should only support VIRTIO 1.0 and later.
> 
> This means continue to allow Legacy mode devices where they already run
> today but don't add new code for the cases that didn't work.

What I'm missing here is what PCI product ID was being used when the
current impl is in legacy/transitional mode ?

Normally legacy and transitional mode devices need an explicit PCI ID
reserved, where as modern-only devices have a PCI ID derived from their
VirtIO ID + a fixed offset.

Was this mistakenly using a VirtIO ID + fixed offset for the legacy
mode too ?

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [RFC 0/4] Enable virtio-fs on s390x

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 2220 bytes --]

On Mon, Jun 29, 2020 at 02:07:16PM +0100, Daniel P. Berrangé wrote:
> On Mon, Jun 29, 2020 at 01:53:05PM +0100, Stefan Hajnoczi wrote:
> > On Thu, Jun 25, 2020 at 12:04:26PM +0200, Marc Hartmayer wrote:
> > > This RFC is about enabling virtio-fs on s390x. For that we need
> > >  + some shim code (first patch), and we need
> > >  + libvhost-user to deal with virtio endiannes as mandated by the spec.
> > >  
> > > The second part is trickier, because unlike QEMU we are not certain
> > > about the guest's native endianness, which is needed to handle the
> > > legacy-interface appropriately. In fact, this is the reason why just
> > > RFC.
> > > 
> > > One of the open questions is whether to build separate versions, one
> > > for guest little endian and one for guest big endian, or do we want
> > > something like a command line option? (Digression on the libvirt
> > > modeling)
> > > 
> > > A third option would be to refuse legacy altogether.
> > 
> > I suggest the following:
> > 
> > 1. Combinations that worked with libvhost-user in the past must not break.
> > 
> > 2. New combinations should only support VIRTIO 1.0 and later.
> > 
> > This means continue to allow Legacy mode devices where they already run
> > today but don't add new code for the cases that didn't work.
> 
> What I'm missing here is what PCI product ID was being used when the
> current impl is in legacy/transitional mode ?
> 
> Normally legacy and transitional mode devices need an explicit PCI ID
> reserved, where as modern-only devices have a PCI ID derived from their
> VirtIO ID + a fixed offset.
> 
> Was this mistakenly using a VirtIO ID + fixed offset for the legacy
> mode too ?

vhost-user-fs-pci does not support Legacy or Transitional mode. See
hw/virtio/vhost-user-fs-pci.c:

  static const VirtioPCIDeviceTypeInfo vhost_user_fs_pci_info = {
      .base_name             = TYPE_VHOST_USER_FS_PCI,
      .non_transitional_name = "vhost-user-fs-pci",
      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
      .instance_size = sizeof(VHostUserFSPCI),
      .instance_init = vhost_user_fs_pci_instance_init,
      .class_init    = vhost_user_fs_pci_class_init,
  };

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [RFC 0/4] Enable virtio-fs on s390x

[Cornelia Huck]

[-- Attachment #1: Type: text/plain, Size: 2642 bytes --]

On Tue, 30 Jun 2020 10:04:51 +0100
Stefan Hajnoczi <stefanha@redhat.com> wrote:

> On Mon, Jun 29, 2020 at 02:07:16PM +0100, Daniel P. Berrangé wrote:
> > On Mon, Jun 29, 2020 at 01:53:05PM +0100, Stefan Hajnoczi wrote:  
> > > On Thu, Jun 25, 2020 at 12:04:26PM +0200, Marc Hartmayer wrote:  
> > > > This RFC is about enabling virtio-fs on s390x. For that we need
> > > >  + some shim code (first patch), and we need
> > > >  + libvhost-user to deal with virtio endiannes as mandated by the spec.
> > > >  
> > > > The second part is trickier, because unlike QEMU we are not certain
> > > > about the guest's native endianness, which is needed to handle the
> > > > legacy-interface appropriately. In fact, this is the reason why just
> > > > RFC.
> > > > 
> > > > One of the open questions is whether to build separate versions, one
> > > > for guest little endian and one for guest big endian, or do we want
> > > > something like a command line option? (Digression on the libvirt
> > > > modeling)
> > > > 
> > > > A third option would be to refuse legacy altogether.  
> > > 
> > > I suggest the following:
> > > 
> > > 1. Combinations that worked with libvhost-user in the past must not break.
> > > 
> > > 2. New combinations should only support VIRTIO 1.0 and later.
> > > 
> > > This means continue to allow Legacy mode devices where they already run
> > > today but don't add new code for the cases that didn't work.  
> > 
> > What I'm missing here is what PCI product ID was being used when the
> > current impl is in legacy/transitional mode ?
> > 
> > Normally legacy and transitional mode devices need an explicit PCI ID
> > reserved, where as modern-only devices have a PCI ID derived from their
> > VirtIO ID + a fixed offset.
> > 
> > Was this mistakenly using a VirtIO ID + fixed offset for the legacy
> > mode too ?  
> 
> vhost-user-fs-pci does not support Legacy or Transitional mode. See
> hw/virtio/vhost-user-fs-pci.c:
> 
>   static const VirtioPCIDeviceTypeInfo vhost_user_fs_pci_info = {
>       .base_name             = TYPE_VHOST_USER_FS_PCI,
>       .non_transitional_name = "vhost-user-fs-pci",
>       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>       .instance_size = sizeof(VHostUserFSPCI),
>       .instance_init = vhost_user_fs_pci_instance_init,
>       .class_init    = vhost_user_fs_pci_class_init,
>   };

This makes it very unlikely that someone accidentally configures
non-modern, but does not prevent it AFAICS. See
<20200630113527.7b27f34f.cohuck@redhat.com>, which I just sent.

(I may be off, because that is all very confusing...)

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [RFC 0/4] Enable virtio-fs on s390x

[Michael S. Tsirkin]

On Tue, Jun 30, 2020 at 11:39:32AM +0200, Cornelia Huck wrote:
> On Tue, 30 Jun 2020 10:04:51 +0100
> Stefan Hajnoczi <stefanha@redhat.com> wrote:
> 
> > On Mon, Jun 29, 2020 at 02:07:16PM +0100, Daniel P. Berrangé wrote:
> > > On Mon, Jun 29, 2020 at 01:53:05PM +0100, Stefan Hajnoczi wrote:  
> > > > On Thu, Jun 25, 2020 at 12:04:26PM +0200, Marc Hartmayer wrote:  
> > > > > This RFC is about enabling virtio-fs on s390x. For that we need
> > > > >  + some shim code (first patch), and we need
> > > > >  + libvhost-user to deal with virtio endiannes as mandated by the spec.
> > > > >  
> > > > > The second part is trickier, because unlike QEMU we are not certain
> > > > > about the guest's native endianness, which is needed to handle the
> > > > > legacy-interface appropriately. In fact, this is the reason why just
> > > > > RFC.
> > > > > 
> > > > > One of the open questions is whether to build separate versions, one
> > > > > for guest little endian and one for guest big endian, or do we want
> > > > > something like a command line option? (Digression on the libvirt
> > > > > modeling)
> > > > > 
> > > > > A third option would be to refuse legacy altogether.  
> > > > 
> > > > I suggest the following:
> > > > 
> > > > 1. Combinations that worked with libvhost-user in the past must not break.
> > > > 
> > > > 2. New combinations should only support VIRTIO 1.0 and later.
> > > > 
> > > > This means continue to allow Legacy mode devices where they already run
> > > > today but don't add new code for the cases that didn't work.  
> > > 
> > > What I'm missing here is what PCI product ID was being used when the
> > > current impl is in legacy/transitional mode ?
> > > 
> > > Normally legacy and transitional mode devices need an explicit PCI ID
> > > reserved, where as modern-only devices have a PCI ID derived from their
> > > VirtIO ID + a fixed offset.
> > > 
> > > Was this mistakenly using a VirtIO ID + fixed offset for the legacy
> > > mode too ?  
> > 
> > vhost-user-fs-pci does not support Legacy or Transitional mode. See
> > hw/virtio/vhost-user-fs-pci.c:
> > 
> >   static const VirtioPCIDeviceTypeInfo vhost_user_fs_pci_info = {
> >       .base_name             = TYPE_VHOST_USER_FS_PCI,
> >       .non_transitional_name = "vhost-user-fs-pci",
> >       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> >       .instance_size = sizeof(VHostUserFSPCI),
> >       .instance_init = vhost_user_fs_pci_instance_init,
> >       .class_init    = vhost_user_fs_pci_class_init,
> >   };
> 
> This makes it very unlikely that someone accidentally configures
> non-modern, but does not prevent it AFAICS. See
> <20200630113527.7b27f34f.cohuck@redhat.com>, which I just sent.
> 
> (I may be off, because that is all very confusing...)

Right. We'll block legacy for modern only devices going forward.
Going back to the patchset in question, virtio-fs is modern
only, legacy will not be supported.

-- 
MST