[RFC v2 0/3] Enable virtio-fs on s390x

[RFC v2 0/3] 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 for non-legacy virtio
   devices as mandated by the spec.

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

Changelog:
 v1->v2:
  + rebased
  + drop patch "libvhost-user: print invalid address on vu_panic" as it's not related to this series
  + drop patch "[RFC 4/4] HACK: Hard-code the libvhost-user.o-cflags for s390x"
  + patch "virtio: add vhost-user-fs-ccw device": replace qdev_set_parent_bus and object_property_set_bool by qdev_realize
  + patch "libvhost-user: handle endianness as mandated by the spec":
    Drop support for legacy virtio devices
  + add patch to fence legacy virtio devices

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

Marc Hartmayer (2):
  libvhost-user: handle endianness as mandated by the spec
  libvhost-user: fence legacy virtio devices

 contrib/libvhost-user/libvhost-access.h |  71 ++++++++++++++
 contrib/libvhost-user/libvhost-user.c   | 125 +++++++++++++-----------
 hw/s390x/Makefile.objs                  |   1 +
 hw/s390x/vhost-user-fs-ccw.c            |  73 ++++++++++++++
 4 files changed, 211 insertions(+), 59 deletions(-)
 create mode 100644 contrib/libvhost-user/libvhost-access.h
 create mode 100644 hw/s390x/vhost-user-fs-ccw.c

-- 
2.25.4

[RFC v2 1/3] 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 | 73 ++++++++++++++++++++++++++++++++++++
 2 files changed, 74 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..88a7a11a34b4
--- /dev/null
+++ b/hw/s390x/vhost-user-fs-ccw.c
@@ -0,0 +1,73 @@
+/*
+ * 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_realize(vdev, BUS(&ccw_dev->bus), 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

[RFC v2 2/3] 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 for non-legacy as required by the virtio specification
[1]. We will fence non-legacy virtio devices with the upcoming patch.

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

---
Note: As we don't support legacy virtio devices there is dead code in
libvhost-access.h that could be removed. But for the sake of
completeness, I left it in the code.
---
 contrib/libvhost-user/libvhost-access.h |  61 ++++++++++++
 contrib/libvhost-user/libvhost-user.c   | 119 ++++++++++++------------
 2 files changed, 121 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..868ba3e7bfb8
--- /dev/null
+++ b/contrib/libvhost-user/libvhost-access.h
@@ -0,0 +1,61 @@
+#ifndef LIBVHOST_ACCESS_H
+
+#include "qemu/bswap.h"
+
+#include "libvhost-user.h"
+
+static inline bool vu_access_is_big_endian(VuDev *dev)
+{
+    /* Devices conforming to VIRTIO 1.0 or later are always LE. */
+    return false;
+}
+
+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 d315db139606..0214b04c5291 100644
--- a/contrib/libvhost-user/libvhost-user.c
+++ b/contrib/libvhost-user/libvhost-user.c
@@ -45,6 +45,7 @@
 #include "qemu/memfd.h"
 
 #include "libvhost-user.h"
+#include "libvhost-access.h"
 
 /* usually provided by GLib */
 #ifndef MIN
@@ -1074,7 +1075,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 +1192,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 +2020,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 +2071,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 +2124,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 +2172,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 +2186,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 +2214,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 +2278,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 +2297,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 +2362,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 +2396,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 +2477,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 +2506,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 +2517,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 +2643,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 +2713,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 +2746,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 +2773,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 v2 3/3] libvhost-user: fence legacy virtio devices

[Marc Hartmayer]

libvhost-user has no support for legacy virtio devices therefore
let's fence them.

Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com>
---
 contrib/libvhost-user/libvhost-access.h | 10 ++++++++++
 contrib/libvhost-user/libvhost-user.c   |  6 ++++++
 2 files changed, 16 insertions(+)

diff --git a/contrib/libvhost-user/libvhost-access.h b/contrib/libvhost-user/libvhost-access.h
index 868ba3e7bfb8..aa505ea1ec02 100644
--- a/contrib/libvhost-user/libvhost-access.h
+++ b/contrib/libvhost-user/libvhost-access.h
@@ -1,11 +1,21 @@
 #ifndef LIBVHOST_ACCESS_H
 
+#include <assert.h>
+
 #include "qemu/bswap.h"
 
 #include "libvhost-user.h"
 
+static inline bool vu_has_feature(VuDev *dev, unsigned int fbit);
+
 static inline bool vu_access_is_big_endian(VuDev *dev)
 {
+    /*
+     * TODO: can probably be removed as the fencing is already done in
+     * `vu_set_features_exec`
+     */
+    assert(vu_has_feature(dev, VIRTIO_F_VERSION_1));
+
     /* Devices conforming to VIRTIO 1.0 or later are always LE. */
     return false;
 }
diff --git a/contrib/libvhost-user/libvhost-user.c b/contrib/libvhost-user/libvhost-user.c
index 0214b04c5291..93c4503b1f53 100644
--- a/contrib/libvhost-user/libvhost-user.c
+++ b/contrib/libvhost-user/libvhost-user.c
@@ -540,6 +540,12 @@ vu_set_features_exec(VuDev *dev, VhostUserMsg *vmsg)
     DPRINT("u64: 0x%016"PRIx64"\n", vmsg->payload.u64);
 
     dev->features = vmsg->payload.u64;
+    if (!vu_has_feature(dev, VIRTIO_F_VERSION_1)) {
+        /* We only support devices conforming to VIRTIO 1.0 or
+         * later */
+        vu_panic(dev, "virtio legacy devices aren't supported by libvhost-user");
+        return false;
+    }
 
     if (!(dev->features & VHOST_USER_F_PROTOCOL_FEATURES)) {
         vu_set_enable_all_rings(dev, true);
-- 
2.25.4

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

[no-reply]

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



Hi,

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

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

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

Switched to a new branch 'test'
a6d1b6d libvhost-user: fence legacy virtio devices
968857c libvhost-user: handle endianness as mandated by the spec
2c7bd4b virtio: add vhost-user-fs-ccw device

=== OUTPUT BEGIN ===
1/3 Checking commit 2c7bd4bc5216 (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)
#85: FILE: hw/s390x/vhost-user-fs-ccw.c:56:
+    device_class_set_props(dc,vhost_user_fs_ccw_properties);
                              ^

total: 1 errors, 1 warnings, 80 lines checked

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

2/3 Checking commit 968857cfd9be (libvhost-user: handle endianness as mandated by the spec)
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#29: 
new file mode 100644

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

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

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

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

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

total: 3 errors, 3 warnings, 362 lines checked

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

3/3 Checking commit a6d1b6dacac8 (libvhost-user: fence legacy virtio devices)
WARNING: Block comments use a leading /* on a separate line
#48: FILE: contrib/libvhost-user/libvhost-user.c:544:
+        /* We only support devices conforming to VIRTIO 1.0 or

WARNING: Block comments use a trailing */ on a separate line
#49: FILE: contrib/libvhost-user/libvhost-user.c:545:
+         * later */

WARNING: line over 80 characters
#50: FILE: contrib/libvhost-user/libvhost-user.c:546:
+        vu_panic(dev, "virtio legacy devices aren't supported by libvhost-user");

total: 0 errors, 3 warnings, 33 lines checked

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

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20200717092929.19453-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 v2 0/3] Enable virtio-fs on s390x

[no-reply]

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



Hi,

This series failed the docker-mingw@fedora 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
export ARCH=x86_64
make docker-image-fedora V=1 NETWORK=1
time make docker-test-mingw@fedora J=14 NETWORK=1
=== TEST SCRIPT END ===

    raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', '--label', 'com.qemu.instance.uuid=bdade74ad18e41e6bde3a38af0ee03ee', '-u', '1001', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 'TARGET_LIST=', '-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', '-e', 'SHOW_ENV=', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', '/home/patchew/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', '/var/tmp/patchew-tester-tmp-_1lsptde/src/docker-src.2020-07-17-06.22.37.2505:/var/tmp/qemu:z,ro', 'qemu/fedora', '/var/tmp/qemu/run', 'test-mingw']' returned non-zero exit status 2.
filter=--filter=label=com.qemu.instance.uuid=bdade74ad18e41e6bde3a38af0ee03ee
make[1]: *** [docker-run] Error 1
make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-_1lsptde/src'
make: *** [docker-run-test-mingw@fedora] Error 2

real    9m20.974s
user    0m9.037s


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

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

[Stefan Hajnoczi]

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

On Fri, Jul 17, 2020 at 11:29:28AM +0200, Marc Hartmayer wrote:
> 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 for non-legacy as required by the virtio specification
> [1]. We will fence non-legacy virtio devices with the upcoming patch.
> 
> [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>
> 
> ---
> Note: As we don't support legacy virtio devices there is dead code in
> libvhost-access.h that could be removed. But for the sake of
> completeness, I left it in the code.

Please remove the dead code. It is unlikely that legacy device support
will be required in the future and it will just confuse people reading
the code.

> ---
>  contrib/libvhost-user/libvhost-access.h |  61 ++++++++++++
>  contrib/libvhost-user/libvhost-user.c   | 119 ++++++++++++------------
>  2 files changed, 121 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..868ba3e7bfb8
> --- /dev/null
> +++ b/contrib/libvhost-user/libvhost-access.h
> @@ -0,0 +1,61 @@
> +#ifndef LIBVHOST_ACCESS_H

License/copyright header?

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

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

[Michael S. Tsirkin]

On Fri, Jul 17, 2020 at 11:29:28AM +0200, Marc Hartmayer wrote:
> 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 for non-legacy as required by the virtio specification
> [1]. We will fence non-legacy virtio devices with the upcoming patch.
> 
> [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>
> 
> ---
> Note: As we don't support legacy virtio devices

Who's "we" in this sentence? vhost user supports legacy generally ...

> there is dead code in
> libvhost-access.h that could be removed. But for the sake of
> completeness, I left it in the code.
> ---
>  contrib/libvhost-user/libvhost-access.h |  61 ++++++++++++
>  contrib/libvhost-user/libvhost-user.c   | 119 ++++++++++++------------
>  2 files changed, 121 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..868ba3e7bfb8
> --- /dev/null
> +++ b/contrib/libvhost-user/libvhost-access.h
> @@ -0,0 +1,61 @@
> +#ifndef LIBVHOST_ACCESS_H
> +
> +#include "qemu/bswap.h"
> +
> +#include "libvhost-user.h"
> +
> +static inline bool vu_access_is_big_endian(VuDev *dev)
> +{
> +    /* Devices conforming to VIRTIO 1.0 or later are always LE. */
> +    return false;
> +}
> +
> +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 d315db139606..0214b04c5291 100644
> --- a/contrib/libvhost-user/libvhost-user.c
> +++ b/contrib/libvhost-user/libvhost-user.c
> @@ -45,6 +45,7 @@
>  #include "qemu/memfd.h"
>  
>  #include "libvhost-user.h"
> +#include "libvhost-access.h"
>  
>  /* usually provided by GLib */
>  #ifndef MIN
> @@ -1074,7 +1075,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 +1192,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 +2020,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 +2071,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 +2124,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 +2172,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 +2186,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 +2214,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 +2278,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 +2297,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 +2362,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 +2396,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 +2477,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 +2506,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 +2517,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 +2643,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 +2713,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 +2746,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 +2773,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

Re: [RFC v2 3/3] libvhost-user: fence legacy virtio devices

[Stefan Hajnoczi]

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

On Fri, Jul 17, 2020 at 11:29:29AM +0200, Marc Hartmayer wrote:
> libvhost-user has no support for legacy virtio devices therefore
> let's fence them.
> 
> Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com>
> ---
>  contrib/libvhost-user/libvhost-access.h | 10 ++++++++++
>  contrib/libvhost-user/libvhost-user.c   |  6 ++++++
>  2 files changed, 16 insertions(+)
> 
> diff --git a/contrib/libvhost-user/libvhost-access.h b/contrib/libvhost-user/libvhost-access.h
> index 868ba3e7bfb8..aa505ea1ec02 100644
> --- a/contrib/libvhost-user/libvhost-access.h
> +++ b/contrib/libvhost-user/libvhost-access.h
> @@ -1,11 +1,21 @@
>  #ifndef LIBVHOST_ACCESS_H
>  
> +#include <assert.h>
> +
>  #include "qemu/bswap.h"
>  
>  #include "libvhost-user.h"
>  
> +static inline bool vu_has_feature(VuDev *dev, unsigned int fbit);
> +
>  static inline bool vu_access_is_big_endian(VuDev *dev)
>  {
> +    /*
> +     * TODO: can probably be removed as the fencing is already done in
> +     * `vu_set_features_exec`
> +     */
> +    assert(vu_has_feature(dev, VIRTIO_F_VERSION_1));

Great, please drop it since the memory accesses are called from
performance-critical code paths.

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

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

[Stefan Hajnoczi]

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

On Fri, Jul 17, 2020 at 11:29:27AM +0200, Marc Hartmayer 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 | 73 ++++++++++++++++++++++++++++++++++++
>  2 files changed, 74 insertions(+)
>  create mode 100644 hw/s390x/vhost-user-fs-ccw.c

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

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

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

[Halil Pasic]

On Tue, 21 Jul 2020 09:40:10 -0400
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Fri, Jul 17, 2020 at 11:29:28AM +0200, Marc Hartmayer wrote:
> > 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 for non-legacy as required by the virtio specification
> > [1]. We will fence non-legacy virtio devices with the upcoming patch.
> > 
> > [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>
> > 
> > ---
> > Note: As we don't support legacy virtio devices  
> 
> Who's "we" in this sentence? vhost user supports legacy generally ...

In that sentence "we" is the library "libvhost-user". I would like
to avoid s390x being an oddball regarding this. Marc's previous
version made an attempt at correct endianness handling for legacy
and non-legacy. That spawned a discussion on how we don't want
legacy devices in this context. This series makes what seemed to be the
consensus reached in that discussion explicit: namely that libvhost-user
does not support legacy-virtio.

Regards,
Halil

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

[Cornelia Huck]

On Fri, 17 Jul 2020 11:29: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 | 73 ++++++++++++++++++++++++++++++++++++
>  2 files changed, 74 insertions(+)
>  create mode 100644 hw/s390x/vhost-user-fs-ccw.c
> 

(...)

> diff --git a/hw/s390x/vhost-user-fs-ccw.c b/hw/s390x/vhost-user-fs-ccw.c
> new file mode 100644
> index 000000000000..88a7a11a34b4
> --- /dev/null
> +++ b/hw/s390x/vhost-user-fs-ccw.c
> @@ -0,0 +1,73 @@
> +/*
> + * Ccw transport wiring for vhost-user-fs

"virtio ccw vhost-user-fs implementation" ?

> + *
> + * 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_realize(vdev, BUS(&ccw_dev->bus), errp);
> +}
> +
> +static void vhost_user_fs_ccw_instance_init(Object *obj)
> +{
> +    VHostUserFSCcw *dev = VHOST_USER_FS_CCW(obj);
> +

This needs

    ccw_dev->force_revision_1 = true;

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

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

[Cornelia Huck]

On Tue, 21 Jul 2020 18:44:56 +0200
Halil Pasic <pasic@linux.ibm.com> wrote:

> On Tue, 21 Jul 2020 09:40:10 -0400
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Fri, Jul 17, 2020 at 11:29:28AM +0200, Marc Hartmayer wrote:  
> > > 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 for non-legacy as required by the virtio specification
> > > [1]. We will fence non-legacy virtio devices with the upcoming patch.
> > > 
> > > [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>
> > > 
> > > ---
> > > Note: As we don't support legacy virtio devices    
> > 
> > Who's "we" in this sentence? vhost user supports legacy generally ...  
> 
> In that sentence "we" is the library "libvhost-user". I would like
> to avoid s390x being an oddball regarding this. Marc's previous
> version made an attempt at correct endianness handling for legacy
> and non-legacy. That spawned a discussion on how we don't want
> legacy devices in this context. This series makes what seemed to be the
> consensus reached in that discussion explicit: namely that libvhost-user
> does not support legacy-virtio.

Can someone please clarify what libvhost-user actually supports?

virtio-fs is clearly non-legacy only, but in the changelog I also see a
mention of scsi, and that one probably does support legacy?

Can we make a distinction along the lines of:
- if we are non-legacy, we support any endianness;
- if we are legacy, we support little endian host/guest only?

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

[Marc Hartmayer]

On Tue, Jul 21, 2020 at 06:44 PM +0200, Halil Pasic <pasic@linux.ibm.com> wrote:
> On Tue, 21 Jul 2020 09:40:10 -0400
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
>
>> On Fri, Jul 17, 2020 at 11:29:28AM +0200, Marc Hartmayer wrote:
>> > 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 for non-legacy as required by the virtio specification
>> > [1]. We will fence non-legacy virtio devices with the upcoming patch.
>> > 
>> > [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>
>> > 
>> > ---
>> > Note: As we don't support legacy virtio devices  
>> 
>> Who's "we" in this sentence? vhost user supports legacy generally ...
>
> In that sentence "we" is the library "libvhost-user". I would like
> to avoid s390x being an oddball regarding this. Marc's previous
> version made an attempt at correct endianness handling for legacy
> and non-legacy. That spawned a discussion on how we don't want
> legacy devices in this context. This series makes what seemed to be the
> consensus reached in that discussion explicit: namely that libvhost-user
> does not support legacy-virtio.

Hi Michael,

Polite ping - what’s your opinion? Thanks in advance.

[…snip]

-- 
Kind regards / Beste Grüße
   Marc Hartmayer

IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Gregor Pillen 
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294

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

[Halil Pasic]

On Tue, 28 Jul 2020 12:31:51 +0200
Cornelia Huck <cohuck@redhat.com> wrote:

> On Fri, 17 Jul 2020 11:29: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 | 73 ++++++++++++++++++++++++++++++++++++
> >  2 files changed, 74 insertions(+)
> >  create mode 100644 hw/s390x/vhost-user-fs-ccw.c
> > 
> 
> (...)
> 
> > diff --git a/hw/s390x/vhost-user-fs-ccw.c b/hw/s390x/vhost-user-fs-ccw.c
> > new file mode 100644
> > index 000000000000..88a7a11a34b4
> > --- /dev/null
> > +++ b/hw/s390x/vhost-user-fs-ccw.c
> > @@ -0,0 +1,73 @@
> > +/*
> > + * Ccw transport wiring for vhost-user-fs
> 
> "virtio ccw vhost-user-fs implementation" ?
> 

Nod.

> > + *
> > + * 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_realize(vdev, BUS(&ccw_dev->bus), errp);
> > +}
> > +
> > +static void vhost_user_fs_ccw_instance_init(Object *obj)
> > +{
> > +    VHostUserFSCcw *dev = VHOST_USER_FS_CCW(obj);
> > +
> 
> This needs
> 
>     ccw_dev->force_revision_1 = true;
> 

I'm OK with that as well. Just out of curiosity, why do we need it? Is
there a virtio-ccw revision 1 feature this device inherently needs?

Regards,
Halil

[..]

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

[Halil Pasic]

On Tue, 28 Jul 2020 12:48:50 +0200
Cornelia Huck <cohuck@redhat.com> wrote:

> On Tue, 21 Jul 2020 18:44:56 +0200
> Halil Pasic <pasic@linux.ibm.com> wrote:
> 
> > On Tue, 21 Jul 2020 09:40:10 -0400
> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > 
> > > On Fri, Jul 17, 2020 at 11:29:28AM +0200, Marc Hartmayer wrote:  
> > > > 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 for non-legacy as required by the virtio specification
> > > > [1]. We will fence non-legacy virtio devices with the upcoming patch.
> > > > 
> > > > [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>
> > > > 
> > > > ---
> > > > Note: As we don't support legacy virtio devices    
> > > 
> > > Who's "we" in this sentence? vhost user supports legacy generally ...  
> > 
> > In that sentence "we" is the library "libvhost-user". I would like
> > to avoid s390x being an oddball regarding this. Marc's previous
> > version made an attempt at correct endianness handling for legacy
> > and non-legacy. That spawned a discussion on how we don't want
> > legacy devices in this context. This series makes what seemed to be the
> > consensus reached in that discussion explicit: namely that libvhost-user
> > does not support legacy-virtio.
> 
> Can someone please clarify what libvhost-user actually supports?
> 
> virtio-fs is clearly non-legacy only, but in the changelog I also see a
> mention of scsi, and that one probably does support legacy?
> 
> Can we make a distinction along the lines of:
> - if we are non-legacy, we support any endianness;
> - if we are legacy, we support little endian host/guest only?
> 

I don't like the idea of this conditional support of legacy (with a
condition being a little endian host/guest). The library is supposed
to be portable in my opinion.

Regards,
Halil

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

[Cornelia Huck]

On Tue, 28 Jul 2020 13:25:57 +0200
Halil Pasic <pasic@linux.ibm.com> wrote:

> On Tue, 28 Jul 2020 12:31:51 +0200
> Cornelia Huck <cohuck@redhat.com> wrote:
> 
> > On Fri, 17 Jul 2020 11:29: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 | 73 ++++++++++++++++++++++++++++++++++++
> > >  2 files changed, 74 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);
> > > +  
> > 
> > This needs
> > 
> >     ccw_dev->force_revision_1 = true;
> >   
> 
> I'm OK with that as well. Just out of curiosity, why do we need it? Is
> there a virtio-ccw revision 1 feature this device inherently needs?

Yes, the VERSION_1 feature :)

(All newer virtio devices are virtio-1 only, and the code has recently
started enforcing this.)

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

[Michael S. Tsirkin]

On Tue, Jul 28, 2020 at 12:52:11PM +0200, Marc Hartmayer wrote:
> On Tue, Jul 21, 2020 at 06:44 PM +0200, Halil Pasic <pasic@linux.ibm.com> wrote:
> > On Tue, 21 Jul 2020 09:40:10 -0400
> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> >
> >> On Fri, Jul 17, 2020 at 11:29:28AM +0200, Marc Hartmayer wrote:
> >> > 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 for non-legacy as required by the virtio specification
> >> > [1]. We will fence non-legacy virtio devices with the upcoming patch.
> >> > 
> >> > [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>
> >> > 
> >> > ---
> >> > Note: As we don't support legacy virtio devices  
> >> 
> >> Who's "we" in this sentence? vhost user supports legacy generally ...
> >
> > In that sentence "we" is the library "libvhost-user". I would like
> > to avoid s390x being an oddball regarding this. Marc's previous
> > version made an attempt at correct endianness handling for legacy
> > and non-legacy. That spawned a discussion on how we don't want
> > legacy devices in this context. This series makes what seemed to be the
> > consensus reached in that discussion explicit: namely that libvhost-user
> > does not support legacy-virtio.
> 
> Hi Michael,
> 
> Polite ping - what’s your opinion? Thanks in advance.
> 
> […snip]

It's a reasonable limitation, but I also don't see anything
that verifies that device is modern only.
E.g. fail setting features if VIRTIO_F_VERSION_1 is not there?


> -- 
> Kind regards / Beste Grüße
>    Marc Hartmayer
> 
> IBM Deutschland Research & Development GmbH
> Vorsitzender des Aufsichtsrats: Gregor Pillen 
> Geschäftsführung: Dirk Wittkopp
> Sitz der Gesellschaft: Böblingen
> Registergericht: Amtsgericht Stuttgart, HRB 243294

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

[Marc Hartmayer]

On Wed, Jul 29, 2020 at 10:13 AM -0400, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Tue, Jul 28, 2020 at 12:52:11PM +0200, Marc Hartmayer wrote:
>> On Tue, Jul 21, 2020 at 06:44 PM +0200, Halil Pasic <pasic@linux.ibm.com> wrote:
>> > On Tue, 21 Jul 2020 09:40:10 -0400
>> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
>> >
>> >> On Fri, Jul 17, 2020 at 11:29:28AM +0200, Marc Hartmayer wrote:
>> >> > 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 for non-legacy as required by the virtio specification
>> >> > [1]. We will fence non-legacy virtio devices with the upcoming patch.
>> >> > 
>> >> > [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>
>> >> > 
>> >> > ---
>> >> > Note: As we don't support legacy virtio devices  
>> >> 
>> >> Who's "we" in this sentence? vhost user supports legacy generally ...
>> >
>> > In that sentence "we" is the library "libvhost-user". I would like
>> > to avoid s390x being an oddball regarding this. Marc's previous
>> > version made an attempt at correct endianness handling for legacy
>> > and non-legacy. That spawned a discussion on how we don't want
>> > legacy devices in this context. This series makes what seemed to be the
>> > consensus reached in that discussion explicit: namely that libvhost-user
>> > does not support legacy-virtio.
>> 
>> Hi Michael,
>> 
>> Polite ping - what’s your opinion? Thanks in advance.
>> 
>> […snip]
>
> It's a reasonable limitation, but I also don't see anything
> that verifies that device is modern only.
> E.g. fail setting features if VIRTIO_F_VERSION_1 is not there?

That’s implemented in patch 3 to emphasize how the fencing is done. For
the next series I’ll merge patch 2 and 3.

>
>
>> -- 
>> Kind regards / Beste Grüße
>>    Marc Hartmayer
>> 
>> IBM Deutschland Research & Development GmbH
>> Vorsitzender des Aufsichtsrats: Gregor Pillen 
>> Geschäftsführung: Dirk Wittkopp
>> Sitz der Gesellschaft: Böblingen
>> Registergericht: Amtsgericht Stuttgart, HRB 243294
>
-- 
Kind regards / Beste Grüße
   Marc Hartmayer

IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Gregor Pillen 
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294

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

[Marc Hartmayer]

On Tue, Jul 21, 2020 at 01:48 PM +0100, Stefan Hajnoczi <stefanha@redhat.com> wrote:
> On Fri, Jul 17, 2020 at 11:29:28AM +0200, Marc Hartmayer wrote:
>> 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 for non-legacy as required by the virtio specification
>> [1]. We will fence non-legacy virtio devices with the upcoming patch.
>> 
>> [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>
>> 
>> ---
>> Note: As we don't support legacy virtio devices there is dead code in
>> libvhost-access.h that could be removed. But for the sake of
>> completeness, I left it in the code.
>
> Please remove the dead code. It is unlikely that legacy device support
> will be required in the future and it will just confuse people reading
> the code.

Done.

>
>> ---
>>  contrib/libvhost-user/libvhost-access.h |  61 ++++++++++++
>>  contrib/libvhost-user/libvhost-user.c   | 119 ++++++++++++------------
>>  2 files changed, 121 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..868ba3e7bfb8
>> --- /dev/null
>> +++ b/contrib/libvhost-user/libvhost-access.h
>> @@ -0,0 +1,61 @@
>> +#ifndef LIBVHOST_ACCESS_H
>
> License/copyright header?

With the dead code removed there is no reason for having
libvhost-access.h so I’ve removed it.

-- 
Kind regards / Beste Grüße
   Marc Hartmayer

IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Gregor Pillen 
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294