[Qemu-devel] [PATCH 0/4] virtio-ccw: virtio-1 enablement

[Qemu-devel] [PATCH 0/4] virtio-ccw: virtio-1 enablement

[Cornelia Huck]

Michael,

here's the virtio-ccw patches that Gerd had dropped due to conflicts
during his virtio-1 rebase. Patch 2 has been updated with my changes
to ensure big endian accesses (which was a separate patch before).

There might be more patches that were in my tree but are not yet upstream,
but I'm still digging myself out from under the post-vacation email pile.

I think these can still go into 2.4. Would you take them through your tree,
or should I take them through the s390x tree?

Cornelia Huck (2):
  s390x/virtio-ccw: support virtio-1 set_vq format
  s390x/virtio-ccw: enable virtio 1.0

Thomas Huth (2):
  s390x/css: Add a callback for when subchannel gets disabled
  s390x/virtio-ccw: add virtio set-revision call

 hw/s390x/css.c        |  12 +++
 hw/s390x/css.h        |   1 +
 hw/s390x/virtio-ccw.c | 212 ++++++++++++++++++++++++++++++++++++++++----------
 hw/s390x/virtio-ccw.h |   8 ++
 4 files changed, 190 insertions(+), 43 deletions(-)

-- 
2.3.8

[Qemu-devel] [PATCH 1/4] s390x/css: Add a callback for when subchannel gets disabled

[Cornelia Huck]

From: Thomas Huth <thuth@linux.vnet.ibm.com>

We need a possibility to run code when a subchannel gets disabled.
This patch adds the necessary infrastructure.

Signed-off-by: Thomas Huth <thuth@linux.vnet.ibm.com>
Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/s390x/css.c | 12 ++++++++++++
 hw/s390x/css.h |  1 +
 2 files changed, 13 insertions(+)

diff --git a/hw/s390x/css.c b/hw/s390x/css.c
index 5561d80..8506945 100644
--- a/hw/s390x/css.c
+++ b/hw/s390x/css.c
@@ -588,6 +588,7 @@ int css_do_msch(SubchDev *sch, const SCHIB *orig_schib)
 {
     SCSW *s = &sch->curr_status.scsw;
     PMCW *p = &sch->curr_status.pmcw;
+    uint16_t oldflags;
     int ret;
     SCHIB schib;
 
@@ -610,6 +611,7 @@ int css_do_msch(SubchDev *sch, const SCHIB *orig_schib)
     copy_schib_from_guest(&schib, orig_schib);
     /* Only update the program-modifiable fields. */
     p->intparm = schib.pmcw.intparm;
+    oldflags = p->flags;
     p->flags &= ~(PMCW_FLAGS_MASK_ISC | PMCW_FLAGS_MASK_ENA |
                   PMCW_FLAGS_MASK_LM | PMCW_FLAGS_MASK_MME |
                   PMCW_FLAGS_MASK_MP);
@@ -625,6 +627,12 @@ int css_do_msch(SubchDev *sch, const SCHIB *orig_schib)
             (PMCW_CHARS_MASK_MBFC | PMCW_CHARS_MASK_CSENSE);
     sch->curr_status.mba = schib.mba;
 
+    /* Has the channel been disabled? */
+    if (sch->disable_cb && (oldflags & PMCW_FLAGS_MASK_ENA) != 0
+        && (p->flags & PMCW_FLAGS_MASK_ENA) == 0) {
+        sch->disable_cb(sch);
+    }
+
     ret = 0;
 
 out:
@@ -1483,6 +1491,10 @@ void css_reset_sch(SubchDev *sch)
 {
     PMCW *p = &sch->curr_status.pmcw;
 
+    if ((p->flags & PMCW_FLAGS_MASK_ENA) != 0 && sch->disable_cb) {
+        sch->disable_cb(sch);
+    }
+
     p->intparm = 0;
     p->flags &= ~(PMCW_FLAGS_MASK_ISC | PMCW_FLAGS_MASK_ENA |
                   PMCW_FLAGS_MASK_LM | PMCW_FLAGS_MASK_MME |
diff --git a/hw/s390x/css.h b/hw/s390x/css.h
index 7e53148..a09bb1f 100644
--- a/hw/s390x/css.h
+++ b/hw/s390x/css.h
@@ -81,6 +81,7 @@ struct SubchDev {
     uint8_t ccw_no_data_cnt;
     /* transport-provided data: */
     int (*ccw_cb) (SubchDev *, CCW1);
+    void (*disable_cb)(SubchDev *);
     SenseId id;
     void *driver_data;
 };
-- 
2.3.8

[Qemu-devel] [PATCH 2/4] s390x/virtio-ccw: add virtio set-revision call

[Cornelia Huck]

From: Thomas Huth <thuth@linux.vnet.ibm.com>

Handle the virtio-ccw revision according to what the guest sets.
When revision 1 is selected, we have a virtio-1 standard device
with byteswapping for the virtio rings.

When a channel gets disabled, we have to revert to the legacy behavior
in case the next user of the device does not negotiate the revision 1
anymore (e.g. the boot firmware uses revision 1, but the operating
system only uses the legacy mode).

Note that revisions > 0 are still disabled.

[CH: assure memory accesses are always BE]
Signed-off-by: Thomas Huth <thuth@linux.vnet.ibm.com>
Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
---
 hw/s390x/virtio-ccw.c | 79 +++++++++++++++++++++++++++++++++++++++++++++++++--
 hw/s390x/virtio-ccw.h |  8 ++++++
 2 files changed, 85 insertions(+), 2 deletions(-)

diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
index e32ada9..e9420d5 100644
--- a/hw/s390x/virtio-ccw.c
+++ b/hw/s390x/virtio-ccw.c
@@ -21,9 +21,11 @@
 #include "hw/virtio/virtio-net.h"
 #include "hw/sysbus.h"
 #include "qemu/bitops.h"
+#include "hw/virtio/virtio-access.h"
 #include "hw/virtio/virtio-bus.h"
 #include "hw/s390x/adapter.h"
 #include "hw/s390x/s390_flic.h"
+#include "linux/virtio_config.h"
 
 #include "ioinst.h"
 #include "css.h"
@@ -261,6 +263,12 @@ typedef struct VirtioThinintInfo {
     uint8_t isc;
 } QEMU_PACKED VirtioThinintInfo;
 
+typedef struct VirtioRevInfo {
+    uint16_t revision;
+    uint16_t length;
+    uint8_t data[0];
+} QEMU_PACKED VirtioRevInfo;
+
 /* Specify where the virtqueues for the subchannel are in guest memory. */
 static int virtio_ccw_set_vqs(SubchDev *sch, uint64_t addr, uint32_t align,
                               uint16_t index, uint16_t num)
@@ -300,6 +308,7 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
 {
     int ret;
     VqInfoBlock info;
+    VirtioRevInfo revinfo;
     uint8_t status;
     VirtioFeatDesc features;
     void *config;
@@ -383,7 +392,16 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
                                                 MEMTXATTRS_UNSPECIFIED,
                                                 NULL);
             if (features.index == 0) {
-                features.features = vdev->host_features;
+                features.features = (uint32_t)vdev->host_features;
+            } else if (features.index == 1) {
+                features.features = (uint32_t)(vdev->host_features >> 32);
+                /*
+                 * Don't offer version 1 to the guest if it did not
+                 * negotiate at least revision 1.
+                 */
+                if (dev->revision <= 0) {
+                    features.features &= ~(1 << (VIRTIO_F_VERSION_1 - 32));
+                }
             } else {
                 /* Return zeroes if the guest supports more feature bits. */
                 features.features = 0;
@@ -419,7 +437,20 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
                                                      MEMTXATTRS_UNSPECIFIED,
                                                      NULL);
             if (features.index == 0) {
-                virtio_set_features(vdev, features.features);
+                virtio_set_features(vdev,
+                                    (vdev->guest_features & 0xffffffff00000000) |
+                                    features.features);
+            } else if (features.index == 1) {
+                /*
+                 * The guest should not set version 1 if it didn't
+                 * negotiate a revision >= 1.
+                 */
+                if (dev->revision <= 0) {
+                    features.features &= ~(1 << (VIRTIO_F_VERSION_1 - 32));
+                }
+                virtio_set_features(vdev,
+                                    (vdev->guest_features & 0x00000000ffffffff) |
+                                    ((uint64_t)features.features << 32));
             } else {
                 /*
                  * If the guest supports more feature bits, assert that it
@@ -640,6 +671,40 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
             }
         }
         break;
+    case CCW_CMD_SET_VIRTIO_REV:
+        len = sizeof(revinfo);
+        if (ccw.count < len) {
+            ret = -EINVAL;
+            break;
+        }
+        if (!ccw.cda) {
+            ret = -EFAULT;
+            break;
+        }
+        revinfo.revision =
+            address_space_lduw_be(&address_space_memory, ccw.cda,
+                                  MEMTXATTRS_UNSPECIFIED, NULL);
+        revinfo.length =
+            address_space_lduw_be(&address_space_memory,
+                                  ccw.cda + sizeof(revinfo.revision),
+                                  MEMTXATTRS_UNSPECIFIED, NULL);
+        if (ccw.count < len + revinfo.length ||
+            (check_len && ccw.count > len + revinfo.length)) {
+            ret = -EINVAL;
+            break;
+        }
+        /*
+         * Once we start to support revisions with additional data, we'll
+         * need to fetch it here. Nothing to do for now, though.
+         */
+        if (dev->revision >= 0 ||
+            revinfo.revision > virtio_ccw_rev_max(vdev)) {
+            ret = -ENOSYS;
+            break;
+        }
+        ret = 0;
+        dev->revision = revinfo.revision;
+        break;
     default:
         ret = -ENOSYS;
         break;
@@ -647,6 +712,13 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
     return ret;
 }
 
+static void virtio_sch_disable_cb(SubchDev *sch)
+{
+    VirtioCcwDevice *dev = sch->driver_data;
+
+    dev->revision = -1;
+}
+
 static void virtio_ccw_device_realize(VirtioCcwDevice *dev, Error **errp)
 {
     unsigned int cssid = 0;
@@ -766,12 +838,15 @@ static void virtio_ccw_device_realize(VirtioCcwDevice *dev, Error **errp)
     css_sch_build_virtual_schib(sch, 0, VIRTIO_CCW_CHPID_TYPE);
 
     sch->ccw_cb = virtio_ccw_cb;
+    sch->disable_cb = virtio_sch_disable_cb;
 
     /* Build senseid data. */
     memset(&sch->id, 0, sizeof(SenseId));
     sch->id.reserved = 0xff;
     sch->id.cu_type = VIRTIO_CCW_CU_TYPE;
 
+    dev->revision = -1;
+
     if (k->realize) {
         k->realize(dev, &err);
     }
diff --git a/hw/s390x/virtio-ccw.h b/hw/s390x/virtio-ccw.h
index d729263..692ddd7 100644
--- a/hw/s390x/virtio-ccw.h
+++ b/hw/s390x/virtio-ccw.h
@@ -41,6 +41,7 @@
 #define CCW_CMD_SET_CONF_IND 0x53
 #define CCW_CMD_READ_VQ_CONF 0x32
 #define CCW_CMD_SET_IND_ADAPTER 0x73
+#define CCW_CMD_SET_VIRTIO_REV 0x83
 
 #define TYPE_VIRTIO_CCW_DEVICE "virtio-ccw-device"
 #define VIRTIO_CCW_DEVICE(obj) \
@@ -86,6 +87,7 @@ struct VirtioCcwDevice {
     DeviceState parent_obj;
     SubchDev *sch;
     char *bus_id;
+    int revision;
     VirtioBusState bus;
     bool ioeventfd_started;
     bool ioeventfd_disabled;
@@ -99,6 +101,12 @@ struct VirtioCcwDevice {
     uint64_t ind_bit;
 };
 
+/* The maximum virtio revision we support. */
+static inline int virtio_ccw_rev_max(VirtIODevice *vdev)
+{
+    return 0;
+}
+
 /* virtual css bus type */
 typedef struct VirtualCssBus {
     BusState parent_obj;
-- 
2.3.8

[Qemu-devel] [PATCH 3/4] s390x/virtio-ccw: support virtio-1 set_vq format

[Cornelia Huck]

Support the new CCW_CMD_SET_VQ format for virtio-1 devices.

While we're at it, refactor the code a bit and enforce big endian
fields (which had always been required, even for legacy).

Reviewed-by: Thomas Huth <thuth@linux.vnet.ibm.com>
Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/s390x/virtio-ccw.c | 133 ++++++++++++++++++++++++++++++++++----------------
 1 file changed, 92 insertions(+), 41 deletions(-)

diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
index e9420d5..2fa7691 100644
--- a/hw/s390x/virtio-ccw.c
+++ b/hw/s390x/virtio-ccw.c
@@ -239,11 +239,20 @@ VirtualCssBus *virtual_css_bus_init(void)
 }
 
 /* Communication blocks used by several channel commands. */
-typedef struct VqInfoBlock {
+typedef struct VqInfoBlockLegacy {
     uint64_t queue;
     uint32_t align;
     uint16_t index;
     uint16_t num;
+} QEMU_PACKED VqInfoBlockLegacy;
+
+typedef struct VqInfoBlock {
+    uint64_t desc;
+    uint32_t res0;
+    uint16_t index;
+    uint16_t num;
+    uint64_t avail;
+    uint64_t used;
 } QEMU_PACKED VqInfoBlock;
 
 typedef struct VqConfigBlock {
@@ -270,17 +279,20 @@ typedef struct VirtioRevInfo {
 } QEMU_PACKED VirtioRevInfo;
 
 /* Specify where the virtqueues for the subchannel are in guest memory. */
-static int virtio_ccw_set_vqs(SubchDev *sch, uint64_t addr, uint32_t align,
-                              uint16_t index, uint16_t num)
+static int virtio_ccw_set_vqs(SubchDev *sch, VqInfoBlock *info,
+                              VqInfoBlockLegacy *linfo)
 {
     VirtIODevice *vdev = virtio_ccw_get_vdev(sch);
+    uint16_t index = info ? info->index : linfo->index;
+    uint16_t num = info ? info->num : linfo->num;
+    uint64_t desc = info ? info->desc : linfo->queue;
 
     if (index >= VIRTIO_CCW_QUEUE_MAX) {
         return -EINVAL;
     }
 
     /* Current code in virtio.c relies on 4K alignment. */
-    if (addr && (align != 4096)) {
+    if (linfo && desc && (linfo->align != 4096)) {
         return -EINVAL;
     }
 
@@ -288,8 +300,12 @@ static int virtio_ccw_set_vqs(SubchDev *sch, uint64_t addr, uint32_t align,
         return -EINVAL;
     }
 
-    virtio_queue_set_addr(vdev, index, addr);
-    if (!addr) {
+    if (info) {
+        virtio_queue_set_rings(vdev, index, desc, info->avail, info->used);
+    } else {
+        virtio_queue_set_addr(vdev, index, desc);
+    }
+    if (!desc) {
         virtio_queue_set_vector(vdev, index, VIRTIO_NO_VECTOR);
     } else {
         /* Fail if we don't have a big enough queue. */
@@ -304,10 +320,78 @@ static int virtio_ccw_set_vqs(SubchDev *sch, uint64_t addr, uint32_t align,
     return 0;
 }
 
-static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
+static int virtio_ccw_handle_set_vq(SubchDev *sch, CCW1 ccw, bool check_len,
+                                    bool is_legacy)
 {
     int ret;
     VqInfoBlock info;
+    VqInfoBlockLegacy linfo;
+    size_t info_len = is_legacy ? sizeof(linfo) : sizeof(info);
+
+    if (check_len) {
+        if (ccw.count != info_len) {
+            return -EINVAL;
+        }
+    } else if (ccw.count < info_len) {
+        /* Can't execute command. */
+        return -EINVAL;
+    }
+    if (!ccw.cda) {
+        return -EFAULT;
+    }
+    if (is_legacy) {
+        linfo.queue = address_space_ldq_be(&address_space_memory, ccw.cda,
+                                           MEMTXATTRS_UNSPECIFIED, NULL);
+        linfo.align = address_space_ldl_be(&address_space_memory,
+                                           ccw.cda + sizeof(linfo.queue),
+                                           MEMTXATTRS_UNSPECIFIED,
+                                           NULL);
+        linfo.index = address_space_lduw_be(&address_space_memory,
+                                            ccw.cda + sizeof(linfo.queue)
+                                            + sizeof(linfo.align),
+                                            MEMTXATTRS_UNSPECIFIED,
+                                            NULL);
+        linfo.num = address_space_lduw_be(&address_space_memory,
+                                          ccw.cda + sizeof(linfo.queue)
+                                          + sizeof(linfo.align)
+                                          + sizeof(linfo.index),
+                                          MEMTXATTRS_UNSPECIFIED,
+                                          NULL);
+        ret = virtio_ccw_set_vqs(sch, NULL, &linfo);
+    } else {
+        info.desc = address_space_ldq_be(&address_space_memory, ccw.cda,
+                                           MEMTXATTRS_UNSPECIFIED, NULL);
+        info.index = address_space_lduw_be(&address_space_memory,
+                                           ccw.cda + sizeof(info.desc)
+                                           + sizeof(info.res0),
+                                           MEMTXATTRS_UNSPECIFIED, NULL);
+        info.num = address_space_lduw_be(&address_space_memory,
+                                         ccw.cda + sizeof(info.desc)
+                                         + sizeof(info.res0)
+                                         + sizeof(info.index),
+                                         MEMTXATTRS_UNSPECIFIED, NULL);
+        info.avail = address_space_ldq_be(&address_space_memory,
+                                          ccw.cda + sizeof(info.desc)
+                                          + sizeof(info.res0)
+                                          + sizeof(info.index)
+                                          + sizeof(info.num),
+                                          MEMTXATTRS_UNSPECIFIED, NULL);
+        info.used = address_space_ldq_be(&address_space_memory,
+                                         ccw.cda + sizeof(info.desc)
+                                         + sizeof(info.res0)
+                                         + sizeof(info.index)
+                                         + sizeof(info.num)
+                                         + sizeof(info.avail),
+                                         MEMTXATTRS_UNSPECIFIED, NULL);
+        ret = virtio_ccw_set_vqs(sch, &info, NULL);
+    }
+    sch->curr_status.scsw.count = 0;
+    return ret;
+}
+
+static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
+{
+    int ret;
     VirtioRevInfo revinfo;
     uint8_t status;
     VirtioFeatDesc features;
@@ -332,40 +416,7 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
     /* Look at the command. */
     switch (ccw.cmd_code) {
     case CCW_CMD_SET_VQ:
-        if (check_len) {
-            if (ccw.count != sizeof(info)) {
-                ret = -EINVAL;
-                break;
-            }
-        } else if (ccw.count < sizeof(info)) {
-            /* Can't execute command. */
-            ret = -EINVAL;
-            break;
-        }
-        if (!ccw.cda) {
-            ret = -EFAULT;
-        } else {
-            info.queue = address_space_ldq(&address_space_memory, ccw.cda,
-                                           MEMTXATTRS_UNSPECIFIED, NULL);
-            info.align = address_space_ldl(&address_space_memory,
-                                           ccw.cda + sizeof(info.queue),
-                                           MEMTXATTRS_UNSPECIFIED,
-                                           NULL);
-            info.index = address_space_lduw(&address_space_memory,
-                                            ccw.cda + sizeof(info.queue)
-                                            + sizeof(info.align),
-                                            MEMTXATTRS_UNSPECIFIED,
-                                            NULL);
-            info.num = address_space_lduw(&address_space_memory,
-                                          ccw.cda + sizeof(info.queue)
-                                          + sizeof(info.align)
-                                          + sizeof(info.index),
-                                          MEMTXATTRS_UNSPECIFIED,
-                                          NULL);
-            ret = virtio_ccw_set_vqs(sch, info.queue, info.align, info.index,
-                                     info.num);
-            sch->curr_status.scsw.count = 0;
-        }
+        ret = virtio_ccw_handle_set_vq(sch, ccw, check_len, dev->revision < 1);
         break;
     case CCW_CMD_VDEV_RESET:
         virtio_ccw_stop_ioeventfd(dev);
-- 
2.3.8

[Qemu-devel] [PATCH 4/4] s390x/virtio-ccw: enable virtio 1.0

[Cornelia Huck]

virtio-ccw should now have everything in place to operate virtio 1.0
devices, so let's enable revision 1.

Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/s390x/virtio-ccw.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/s390x/virtio-ccw.h b/hw/s390x/virtio-ccw.h
index 692ddd7..2c31399 100644
--- a/hw/s390x/virtio-ccw.h
+++ b/hw/s390x/virtio-ccw.h
@@ -104,7 +104,7 @@ struct VirtioCcwDevice {
 /* The maximum virtio revision we support. */
 static inline int virtio_ccw_rev_max(VirtIODevice *vdev)
 {
-    return 0;
+    return vdev->host_features & (1ULL << VIRTIO_F_VERSION_1) ? 1 : 0;
 }
 
 /* virtual css bus type */
-- 
2.3.8

Re: [Qemu-devel] [PATCH 0/4] virtio-ccw: virtio-1 enablement

[Michael S. Tsirkin]

On Tue, Jun 23, 2015 at 01:17:32PM +0200, Cornelia Huck wrote:
> Michael,
> 
> here's the virtio-ccw patches that Gerd had dropped due to conflicts
> during his virtio-1 rebase. Patch 2 has been updated with my changes
> to ensure big endian accesses (which was a separate patch before).
> 
> There might be more patches that were in my tree but are not yet upstream,
> but I'm still digging myself out from under the post-vacation email pile.
> 
> I think these can still go into 2.4. Would you take them through your tree,
> or should I take them through the s390x tree?

s390x makes more sense.
For the series:

Acked-by: Michael S. Tsirkin <mst@redhat.com>


> Cornelia Huck (2):
>   s390x/virtio-ccw: support virtio-1 set_vq format
>   s390x/virtio-ccw: enable virtio 1.0
> 
> Thomas Huth (2):
>   s390x/css: Add a callback for when subchannel gets disabled
>   s390x/virtio-ccw: add virtio set-revision call
> 
>  hw/s390x/css.c        |  12 +++
>  hw/s390x/css.h        |   1 +
>  hw/s390x/virtio-ccw.c | 212 ++++++++++++++++++++++++++++++++++++++++----------
>  hw/s390x/virtio-ccw.h |   8 ++
>  4 files changed, 190 insertions(+), 43 deletions(-)
> 
> -- 
> 2.3.8

Re: [Qemu-devel] [PATCH 0/4] virtio-ccw: virtio-1 enablement

[Thomas Huth]

On Tue, 23 Jun 2015 13:17:32 +0200
Cornelia Huck <cornelia.huck@de.ibm.com> wrote:

> Michael,
> 
> here's the virtio-ccw patches that Gerd had dropped due to conflicts
> during his virtio-1 rebase. Patch 2 has been updated with my changes
> to ensure big endian accesses (which was a separate patch before).
> 
> There might be more patches that were in my tree but are not yet upstream,
> but I'm still digging myself out from under the post-vacation email pile.
> 
> I think these can still go into 2.4.

Is it safe enough to enable them by default already? If I've got
the virtio-pci code right (see the patch description at
https://lists.gnu.org/archive/html/qemu-devel/2015-06/msg01362.html),
it is still disabled there by default - and "modern will be turned on by
default once all remaining spec compilance issues are addressed".

 Thomas

Re: [Qemu-devel] [PATCH 0/4] virtio-ccw: virtio-1 enablement

[Cornelia Huck]

On Tue, 23 Jun 2015 18:36:34 +0200
Thomas Huth <thuth@redhat.com> wrote:

> On Tue, 23 Jun 2015 13:17:32 +0200
> Cornelia Huck <cornelia.huck@de.ibm.com> wrote:
> 
> > Michael,
> > 
> > here's the virtio-ccw patches that Gerd had dropped due to conflicts
> > during his virtio-1 rebase. Patch 2 has been updated with my changes
> > to ensure big endian accesses (which was a separate patch before).
> > 
> > There might be more patches that were in my tree but are not yet upstream,
> > but I'm still digging myself out from under the post-vacation email pile.
> > 
> > I think these can still go into 2.4.
> 
> Is it safe enough to enable them by default already? If I've got
> the virtio-pci code right (see the patch description at
> https://lists.gnu.org/archive/html/qemu-devel/2015-06/msg01362.html),
> it is still disabled there by default - and "modern will be turned on by
> default once all remaining spec compilance issues are addressed".

I was about to say "yes" when I noticed that we probably want to care
about migrating dev->revision as well... I'll drop patch 4 for now and
just send patches 1-3 from this series.

Re: [Qemu-devel] [PATCH 0/4] virtio-ccw: virtio-1 enablement

[Cornelia Huck]

On Tue, 23 Jun 2015 19:19:39 +0200
Cornelia Huck <cornelia.huck@de.ibm.com> wrote:

> On Tue, 23 Jun 2015 18:36:34 +0200
> Thomas Huth <thuth@redhat.com> wrote:
> 
> > On Tue, 23 Jun 2015 13:17:32 +0200
> > Cornelia Huck <cornelia.huck@de.ibm.com> wrote:
> > 
> > > Michael,
> > > 
> > > here's the virtio-ccw patches that Gerd had dropped due to conflicts
> > > during his virtio-1 rebase. Patch 2 has been updated with my changes
> > > to ensure big endian accesses (which was a separate patch before).
> > > 
> > > There might be more patches that were in my tree but are not yet upstream,
> > > but I'm still digging myself out from under the post-vacation email pile.
> > > 
> > > I think these can still go into 2.4.
> > 
> > Is it safe enough to enable them by default already? If I've got
> > the virtio-pci code right (see the patch description at
> > https://lists.gnu.org/archive/html/qemu-devel/2015-06/msg01362.html),
> > it is still disabled there by default - and "modern will be turned on by
> > default once all remaining spec compilance issues are addressed".
> 
> I was about to say "yes" when I noticed that we probably want to care
> about migrating dev->revision as well... I'll drop patch 4 for now and
> just send patches 1-3 from this series.

On a second thought, I can simply include dev->revision, as we have the
incompatible ->config_vector change for this release already. I'll
include a new patch for this and patch 4 as well, as we only need to
care about Linux guests on ccw for now and those are fine.

Re: [Qemu-devel] [PATCH 0/4] virtio-ccw: virtio-1 enablement

[Michael S. Tsirkin]

On Thu, Jun 25, 2015 at 12:24:40PM +0200, Cornelia Huck wrote:
> On Tue, 23 Jun 2015 19:19:39 +0200
> Cornelia Huck <cornelia.huck@de.ibm.com> wrote:
> 
> > On Tue, 23 Jun 2015 18:36:34 +0200
> > Thomas Huth <thuth@redhat.com> wrote:
> > 
> > > On Tue, 23 Jun 2015 13:17:32 +0200
> > > Cornelia Huck <cornelia.huck@de.ibm.com> wrote:
> > > 
> > > > Michael,
> > > > 
> > > > here's the virtio-ccw patches that Gerd had dropped due to conflicts
> > > > during his virtio-1 rebase. Patch 2 has been updated with my changes
> > > > to ensure big endian accesses (which was a separate patch before).
> > > > 
> > > > There might be more patches that were in my tree but are not yet upstream,
> > > > but I'm still digging myself out from under the post-vacation email pile.
> > > > 
> > > > I think these can still go into 2.4.
> > > 
> > > Is it safe enough to enable them by default already? If I've got
> > > the virtio-pci code right (see the patch description at
> > > https://lists.gnu.org/archive/html/qemu-devel/2015-06/msg01362.html),
> > > it is still disabled there by default - and "modern will be turned on by
> > > default once all remaining spec compilance issues are addressed".
> > 
> > I was about to say "yes" when I noticed that we probably want to care
> > about migrating dev->revision as well... I'll drop patch 4 for now and
> > just send patches 1-3 from this series.
> 
> On a second thought, I can simply include dev->revision, as we have the
> incompatible ->config_vector change for this release already. I'll
> include a new patch for this and patch 4 as well, as we only need to
> care about Linux guests on ccw for now and those are fine.

I don't think enable by default is a good idea for 2.4 since
I think we have spec compliance issues which just don't
trigger with linux guests. For example, devices poke at
rings before DRIVER_OK is set.

If we release QEMU like this, guests will have to work
around these issues, which is not nice.
Let's release 2.4 in disabled by default mode, and flip
it around for 2.5.

-- 
MST

Re: [Qemu-devel] [PATCH 0/4] virtio-ccw: virtio-1 enablement

[Cornelia Huck]

On Thu, 25 Jun 2015 14:49:00 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Thu, Jun 25, 2015 at 12:24:40PM +0200, Cornelia Huck wrote:

> > On a second thought, I can simply include dev->revision, as we have the
> > incompatible ->config_vector change for this release already. I'll
> > include a new patch for this and patch 4 as well, as we only need to
> > care about Linux guests on ccw for now and those are fine.
> 
> I don't think enable by default is a good idea for 2.4 since
> I think we have spec compliance issues which just don't
> trigger with linux guests. For example, devices poke at
> rings before DRIVER_OK is set.
> 
> If we release QEMU like this, guests will have to work
> around these issues, which is not nice.
> Let's release 2.4 in disabled by default mode, and flip
> it around for 2.5.

If you put it that way, I agree to defer the actual enablement. I'll
keep the ->revision migration patch, though.

Re: [Qemu-devel] [PATCH 0/4] virtio-ccw: virtio-1 enablement

[Michael S. Tsirkin]

On Tue, Jun 23, 2015 at 06:36:34PM +0200, Thomas Huth wrote:
> On Tue, 23 Jun 2015 13:17:32 +0200
> Cornelia Huck <cornelia.huck@de.ibm.com> wrote:
> 
> > Michael,
> > 
> > here's the virtio-ccw patches that Gerd had dropped due to conflicts
> > during his virtio-1 rebase. Patch 2 has been updated with my changes
> > to ensure big endian accesses (which was a separate patch before).
> > 
> > There might be more patches that were in my tree but are not yet upstream,
> > but I'm still digging myself out from under the post-vacation email pile.
> > 
> > I think these can still go into 2.4.
> 
> Is it safe enough to enable them by default already? If I've got
> the virtio-pci code right (see the patch description at
> https://lists.gnu.org/archive/html/qemu-devel/2015-06/msg01362.html),
> it is still disabled there by default - and "modern will be turned on by
> default once all remaining spec compilance issues are addressed".
> 
>  Thomas

I know of some compliance issues. It does work with current linux
guests.

-- 
MST