[Qemu-devel] [PATCH v2 0/4] add CCW indirect data access support

[Qemu-devel] [PATCH v2 0/4] add CCW indirect data access support

[Halil Pasic]

Abstract
--------

The objective of this series is introducing CCW IDA (indirect data
access) support to our virtual channel subsystem implementation. Briefly
CCW IDA can be thought of as a kind of a scatter gather support for a
single CCW. If certain flags are set, the cda is to be interpreted as an
address to a list which in turn holds further addresses designating the
actual data.  Thus the scheme which we are currently using for accessing
CCW payload does not work in general case. Currently there is no
immediate need for proper IDA handling (no use case), but since it IDA is
a non-optional part of the architecture, the only way towards AR
compliance is actually implementing IDA.

Testing
-------

On request the things meant for testing from v1 were factored out
into a separate series (requested by Connie). Please look for
the series  'tests for CCW IDA' (comming soon) or use the stuff
form v1.

Changelog
---------

v1 --> v2
* factored out the stuff added only for testing
* use g_assert instead of assert
* fixed a lot's of typos
* removed some TODOs addressed by another series of mine
* refactored ccw_dstream_rw_ida (structured programming)
* done some rewording of commit message #3

Halil Pasic (4):
  s390x/css: introduce css data stream
  s390x/css: use ccw data stream
  virtio-ccw: use ccw data stream
  s390x/css: support ccw IDA

 hw/s390x/css.c         | 171 +++++++++++++++++++++++++++++++++++++++++++++++--
 hw/s390x/virtio-ccw.c  | 156 +++++++++++++-------------------------------
 include/hw/s390x/css.h |  67 +++++++++++++++++++
 3 files changed, 280 insertions(+), 114 deletions(-)

--
2.13.5

[Qemu-devel] [PATCH v2 1/4] s390x/css: introduce css data stream

[Halil Pasic]

This is a preparation for introducing handling for indirect data
addressing and modified indirect data addressing (CCW). Here we introduce
an interface which should make the addressing scheme transparent for the
client code. Here we implement only the basic scheme (no IDA or MIDA).

Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
---
 hw/s390x/css.c         | 55 +++++++++++++++++++++++++++++++++++++++++
 include/hw/s390x/css.h | 67 ++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 122 insertions(+)

diff --git a/hw/s390x/css.c b/hw/s390x/css.c
index 901dc6a0f3..e8d2016563 100644
--- a/hw/s390x/css.c
+++ b/hw/s390x/css.c
@@ -783,6 +783,61 @@ static CCW1 copy_ccw_from_guest(hwaddr addr, bool fmt1)
     }
     return ret;
 }
+/**
+ * If out of bounds marks the stream broken. If broken returns -EINVAL,
+ * otherwise the requested length (may be zero)
+ */
+static inline int cds_check_len(CcwDataStream *cds, int len)
+{
+    if (cds->at_byte + len > cds->count) {
+        cds->flags |= CDS_F_STREAM_BROKEN;
+    }
+    return cds->flags & CDS_F_STREAM_BROKEN ? -EINVAL : len;
+}
+
+static int ccw_dstream_rw_noflags(CcwDataStream *cds, void *buff, int len,
+                                  CcwDataStreamOp op)
+{
+    int ret;
+
+    ret = cds_check_len(cds, len);
+    if (ret <= 0) {
+        return ret;
+    }
+    if (op == CDS_OP_A) {
+        goto incr;
+    }
+    ret = address_space_rw(&address_space_memory, cds->cda,
+                           MEMTXATTRS_UNSPECIFIED, buff, len, op);
+    if (ret != MEMTX_OK) {
+        cds->flags |= CDS_F_STREAM_BROKEN;
+        return -EINVAL;
+    }
+incr:
+    cds->at_byte += len;
+    cds->cda += len;
+    return 0;
+}
+
+void ccw_dstream_init(CcwDataStream *cds, CCW1 const *ccw, ORB const *orb)
+{
+    /*
+     * We don't support MIDA (an optional facility) yet and we
+     * catch this earlier. Just for expressing the precondition.
+     */
+    g_assert(!(orb->ctrl1 & ORB_CTRL1_MASK_MIDAW));
+    cds->flags = (orb->ctrl0 & ORB_CTRL0_MASK_I2K ? CDS_F_I2K : 0) |
+                 (orb->ctrl0 & ORB_CTRL0_MASK_C64 ? CDS_F_C64 : 0) |
+                 (ccw->flags & CCW_FLAG_IDA ? CDS_F_IDA : 0);
+    cds->count = ccw->count;
+    cds->cda_orig = ccw->cda;
+    ccw_dstream_rewind(cds);
+    if (!(cds->flags & CDS_F_IDA)) {
+        cds->op_handler = ccw_dstream_rw_noflags;
+    } else {
+        assert(false);
+    }
+}
 
 static int css_interpret_ccw(SubchDev *sch, hwaddr ccw_addr,
                              bool suspend_allowed)
diff --git a/include/hw/s390x/css.h b/include/hw/s390x/css.h
index 0653d3c9be..79acaf99b7 100644
--- a/include/hw/s390x/css.h
+++ b/include/hw/s390x/css.h
@@ -75,6 +75,29 @@ typedef struct CMBE {
     uint32_t reserved[7];
 } QEMU_PACKED CMBE;
 
+typedef enum CcwDataStreamOp {
+    CDS_OP_R = 0,
+    CDS_OP_W = 1,
+    CDS_OP_A = 2
+} CcwDataStreamOp;
+
+/** normal usage is via SuchchDev.cds instead of instantiating */
+typedef struct CcwDataStream {
+#define CDS_F_IDA   0x01
+#define CDS_F_MIDA  0x02
+#define CDS_F_I2K   0x04
+#define CDS_F_C64   0x08
+#define CDS_F_STREAM_BROKEN  0x80
+    uint8_t flags;
+    uint8_t at_idaw;
+    uint16_t at_byte;
+    uint16_t count;
+    uint32_t cda_orig;
+    int (*op_handler)(struct CcwDataStream *cds, void *buff, int len,
+                      CcwDataStreamOp op);
+    hwaddr cda;
+} CcwDataStream;
+
 typedef struct SubchDev SubchDev;
 struct SubchDev {
     /* channel-subsystem related things: */
@@ -92,6 +115,7 @@ struct SubchDev {
     uint8_t ccw_no_data_cnt;
     uint16_t migrated_schid; /* used for missmatch detection */
     ORB orb;
+    CcwDataStream cds;
     /* transport-provided data: */
     int (*ccw_cb) (SubchDev *, CCW1);
     void (*disable_cb)(SubchDev *);
@@ -240,4 +264,47 @@ SubchDev *css_create_sch(CssDevId bus_id, bool is_virtual, bool squash_mcss,
 /** Turn on css migration */
 void css_register_vmstate(void);
 
+
+void ccw_dstream_init(CcwDataStream *cds, CCW1 const *ccw, ORB const *orb);
+
+static inline void ccw_dstream_rewind(CcwDataStream *cds)
+{
+    cds->at_byte = 0;
+    cds->at_idaw = 0;
+    cds->cda = cds->cda_orig;
+}
+
+static inline bool ccw_dstream_good(CcwDataStream *cds)
+{
+    return !(cds->flags & CDS_F_STREAM_BROKEN);
+}
+
+static inline uint16_t ccw_dstream_residual_count(CcwDataStream *cds)
+{
+    return cds->count - cds->at_byte;
+}
+
+static inline uint16_t ccw_dstream_avail(CcwDataStream *cds)
+{
+    return ccw_dstream_good(cds) ?  ccw_dstream_residual_count(cds) : 0;
+}
+
+static inline int ccw_dstream_advance(CcwDataStream *cds, int len)
+{
+    return cds->op_handler(cds, NULL, len, CDS_OP_A);
+}
+
+static inline int ccw_dstream_write_buf(CcwDataStream *cds, void *buff, int len)
+{
+    return cds->op_handler(cds, buff, len, CDS_OP_W);
+}
+
+static inline int ccw_dstream_read_buf(CcwDataStream *cds, void *buff, int len)
+{
+    return cds->op_handler(cds, buff, len, CDS_OP_R);
+}
+
+#define ccw_dstream_read(cds, v) ccw_dstream_read_buf((cds), &(v), sizeof(v))
+#define ccw_dstream_write(cds, v) ccw_dstream_write_buf((cds), &(v), sizeof(v))
+
 #endif
-- 
2.13.5

[Qemu-devel] [PATCH v2 2/4] s390x/css: use ccw data stream

[Halil Pasic]

Replace direct access which implicitly assumes no IDA
or MIDA with the new ccw data stream interface which should
cope with these transparently in the future.

Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
---
 hw/s390x/css.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/hw/s390x/css.c b/hw/s390x/css.c
index e8d2016563..6b0cd8861b 100644
--- a/hw/s390x/css.c
+++ b/hw/s390x/css.c
@@ -890,6 +890,7 @@ static int css_interpret_ccw(SubchDev *sch, hwaddr ccw_addr,
     }
 
     /* Look at the command. */
+    ccw_dstream_init(&sch->cds, &ccw, &(sch->orb));
     switch (ccw.cmd_code) {
     case CCW_CMD_NOOP:
         /* Nothing to do. */
@@ -903,8 +904,8 @@ static int css_interpret_ccw(SubchDev *sch, hwaddr ccw_addr,
             }
         }
         len = MIN(ccw.count, sizeof(sch->sense_data));
-        cpu_physical_memory_write(ccw.cda, sch->sense_data, len);
-        sch->curr_status.scsw.count = ccw.count - len;
+        ccw_dstream_write_buf(&sch->cds, sch->sense_data, len);
+        sch->curr_status.scsw.count = ccw_dstream_residual_count(&sch->cds);
         memset(sch->sense_data, 0, sizeof(sch->sense_data));
         ret = 0;
         break;
@@ -930,8 +931,8 @@ static int css_interpret_ccw(SubchDev *sch, hwaddr ccw_addr,
         } else {
             sense_id.reserved = 0;
         }
-        cpu_physical_memory_write(ccw.cda, &sense_id, len);
-        sch->curr_status.scsw.count = ccw.count - len;
+        ccw_dstream_write_buf(&sch->cds, &sense_id, len);
+        sch->curr_status.scsw.count = ccw_dstream_residual_count(&sch->cds);
         ret = 0;
         break;
     }
-- 
2.13.5

[Qemu-devel] [PATCH v2 3/4] virtio-ccw: use ccw data stream

[Halil Pasic]

Replace direct access which implicitly assumes no IDA
or MIDA with the new ccw data stream interface which should
cope with these transparently in the future.

Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>

---

v1 --> v2:
Removed todo comments on possible errno change as with
https://lists.nongnu.org/archive/html/qemu-devel/2017-09/msg02441.html
applied it does not matter any more.

Error handling: At the moment we ignore errors reported
by stream ops to keep the change minimal. An add-on
patch improving on this is to be expected later.
---
 hw/s390x/virtio-ccw.c | 156 +++++++++++++++-----------------------------------
 1 file changed, 46 insertions(+), 110 deletions(-)

diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
index b1976fdd19..a9baf6f7ab 100644
--- a/hw/s390x/virtio-ccw.c
+++ b/hw/s390x/virtio-ccw.c
@@ -287,49 +287,19 @@ static int virtio_ccw_handle_set_vq(SubchDev *sch, CCW1 ccw, bool check_len,
         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);
+        ccw_dstream_read(&sch->cds, linfo);
+        be64_to_cpus(&linfo.queue);
+        be32_to_cpus(&linfo.align);
+        be16_to_cpus(&linfo.index);
+        be16_to_cpus(&linfo.num);
         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);
+        ccw_dstream_read(&sch->cds, info);
+        be64_to_cpus(&info.desc);
+        be16_to_cpus(&info.index);
+        be16_to_cpus(&info.num);
+        be64_to_cpus(&info.avail);
+        be64_to_cpus(&info.used);
         ret = virtio_ccw_set_vqs(sch, &info, NULL);
     }
     sch->curr_status.scsw.count = 0;
@@ -342,15 +312,13 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
     VirtioRevInfo revinfo;
     uint8_t status;
     VirtioFeatDesc features;
-    void *config;
     hwaddr indicators;
     VqConfigBlock vq_config;
     VirtioCcwDevice *dev = sch->driver_data;
     VirtIODevice *vdev = virtio_ccw_get_vdev(sch);
     bool check_len;
     int len;
-    hwaddr hw_len;
-    VirtioThinintInfo *thinint;
+    VirtioThinintInfo thinint;
 
     if (!dev) {
         return -EINVAL;
@@ -394,11 +362,8 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
         } else {
             VirtioDeviceClass *vdc = VIRTIO_DEVICE_GET_CLASS(vdev);
 
-            features.index = address_space_ldub(&address_space_memory,
-                                                ccw.cda
-                                                + sizeof(features.features),
-                                                MEMTXATTRS_UNSPECIFIED,
-                                                NULL);
+            ccw_dstream_advance(&sch->cds, sizeof(features.features));
+            ccw_dstream_read(&sch->cds, features.index);
             if (features.index == 0) {
                 if (dev->revision >= 1) {
                     /* Don't offer legacy features for modern devices. */
@@ -417,9 +382,9 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
                 /* Return zeroes if the guest supports more feature bits. */
                 features.features = 0;
             }
-            address_space_stl_le(&address_space_memory, ccw.cda,
-                                 features.features, MEMTXATTRS_UNSPECIFIED,
-                                 NULL);
+            ccw_dstream_rewind(&sch->cds);
+            cpu_to_le32s(&features.features);
+            ccw_dstream_write(&sch->cds, features.features);
             sch->curr_status.scsw.count = ccw.count - sizeof(features);
             ret = 0;
         }
@@ -438,15 +403,8 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
         if (!ccw.cda) {
             ret = -EFAULT;
         } else {
-            features.index = address_space_ldub(&address_space_memory,
-                                                ccw.cda
-                                                + sizeof(features.features),
-                                                MEMTXATTRS_UNSPECIFIED,
-                                                NULL);
-            features.features = address_space_ldl_le(&address_space_memory,
-                                                     ccw.cda,
-                                                     MEMTXATTRS_UNSPECIFIED,
-                                                     NULL);
+            ccw_dstream_read(&sch->cds, features);
+            le32_to_cpus(&features.features);
             if (features.index == 0) {
                 virtio_set_features(vdev,
                                     (vdev->guest_features & 0xffffffff00000000ULL) |
@@ -488,7 +446,7 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
         } else {
             virtio_bus_get_vdev_config(&dev->bus, vdev->config);
             /* XXX config space endianness */
-            cpu_physical_memory_write(ccw.cda, vdev->config, len);
+            ccw_dstream_write_buf(&sch->cds, vdev->config, len);
             sch->curr_status.scsw.count = ccw.count - len;
             ret = 0;
         }
@@ -501,21 +459,13 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
             }
         }
         len = MIN(ccw.count, vdev->config_len);
-        hw_len = len;
         if (!ccw.cda) {
             ret = -EFAULT;
         } else {
-            config = cpu_physical_memory_map(ccw.cda, &hw_len, 0);
-            if (!config) {
-                ret = -EFAULT;
-            } else {
-                len = hw_len;
-                /* XXX config space endianness */
-                memcpy(vdev->config, config, len);
-                cpu_physical_memory_unmap(config, hw_len, 0, hw_len);
+            ret = ccw_dstream_read_buf(&sch->cds, vdev->config, len);
+            if (!ret) {
                 virtio_bus_set_vdev_config(&dev->bus, vdev->config);
                 sch->curr_status.scsw.count = ccw.count - len;
-                ret = 0;
             }
         }
         break;
@@ -553,8 +503,7 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
         if (!ccw.cda) {
             ret = -EFAULT;
         } else {
-            status = address_space_ldub(&address_space_memory, ccw.cda,
-                                        MEMTXATTRS_UNSPECIFIED, NULL);
+            ccw_dstream_read(&sch->cds, status);
             if (!(status & VIRTIO_CONFIG_S_DRIVER_OK)) {
                 virtio_ccw_stop_ioeventfd(dev);
             }
@@ -597,8 +546,8 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
         if (!ccw.cda) {
             ret = -EFAULT;
         } else {
-            indicators = address_space_ldq_be(&address_space_memory, ccw.cda,
-                                              MEMTXATTRS_UNSPECIFIED, NULL);
+            ccw_dstream_read(&sch->cds, indicators);
+            be64_to_cpus(&indicators);
             dev->indicators = get_indicator(indicators, sizeof(uint64_t));
             sch->curr_status.scsw.count = ccw.count - sizeof(indicators);
             ret = 0;
@@ -618,8 +567,8 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
         if (!ccw.cda) {
             ret = -EFAULT;
         } else {
-            indicators = address_space_ldq_be(&address_space_memory, ccw.cda,
-                                              MEMTXATTRS_UNSPECIFIED, NULL);
+            ccw_dstream_read(&sch->cds, indicators);
+            be64_to_cpus(&indicators);
             dev->indicators2 = get_indicator(indicators, sizeof(uint64_t));
             sch->curr_status.scsw.count = ccw.count - sizeof(indicators);
             ret = 0;
@@ -639,67 +588,58 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
         if (!ccw.cda) {
             ret = -EFAULT;
         } else {
-            vq_config.index = address_space_lduw_be(&address_space_memory,
-                                                    ccw.cda,
-                                                    MEMTXATTRS_UNSPECIFIED,
-                                                    NULL);
+            ccw_dstream_read(&sch->cds, vq_config.index);
+            be16_to_cpus(&vq_config.index);
             if (vq_config.index >= VIRTIO_QUEUE_MAX) {
                 ret = -EINVAL;
                 break;
             }
             vq_config.num_max = virtio_queue_get_num(vdev,
                                                      vq_config.index);
-            address_space_stw_be(&address_space_memory,
-                                 ccw.cda + sizeof(vq_config.index),
-                                 vq_config.num_max,
-                                 MEMTXATTRS_UNSPECIFIED,
-                                 NULL);
+            cpu_to_be16s(&vq_config.num_max);
+            ccw_dstream_write(&sch->cds, vq_config.num_max);
             sch->curr_status.scsw.count = ccw.count - sizeof(vq_config);
             ret = 0;
         }
         break;
     case CCW_CMD_SET_IND_ADAPTER:
         if (check_len) {
-            if (ccw.count != sizeof(*thinint)) {
+            if (ccw.count != sizeof(thinint)) {
                 ret = -EINVAL;
                 break;
             }
-        } else if (ccw.count < sizeof(*thinint)) {
+        } else if (ccw.count < sizeof(thinint)) {
             /* Can't execute command. */
             ret = -EINVAL;
             break;
         }
-        len = sizeof(*thinint);
-        hw_len = len;
         if (!ccw.cda) {
             ret = -EFAULT;
         } else if (dev->indicators && !sch->thinint_active) {
             /* Trigger a command reject. */
             ret = -ENOSYS;
         } else {
-            thinint = cpu_physical_memory_map(ccw.cda, &hw_len, 0);
-            if (!thinint) {
+            if (ccw_dstream_read(&sch->cds, thinint)) {
                 ret = -EFAULT;
             } else {
-                uint64_t ind_bit = ldq_be_p(&thinint->ind_bit);
+                be64_to_cpus(&thinint.ind_bit);
+                be64_to_cpus(&thinint.summary_indicator);
+                be64_to_cpus(&thinint.device_indicator);
 
-                len = hw_len;
                 dev->summary_indicator =
-                    get_indicator(ldq_be_p(&thinint->summary_indicator),
-                                  sizeof(uint8_t));
+                    get_indicator(thinint.summary_indicator, sizeof(uint8_t));
                 dev->indicators =
-                    get_indicator(ldq_be_p(&thinint->device_indicator),
-                                  ind_bit / 8 + 1);
-                dev->thinint_isc = thinint->isc;
-                dev->routes.adapter.ind_offset = ind_bit;
+                    get_indicator(thinint.device_indicator,
+                                  thinint.ind_bit / 8 + 1);
+                dev->thinint_isc = thinint.isc;
+                dev->routes.adapter.ind_offset = thinint.ind_bit;
                 dev->routes.adapter.summary_offset = 7;
-                cpu_physical_memory_unmap(thinint, hw_len, 0, hw_len);
                 dev->routes.adapter.adapter_id = css_get_adapter_id(
                                                  CSS_IO_ADAPTER_VIRTIO,
                                                  dev->thinint_isc);
                 sch->thinint_active = ((dev->indicators != NULL) &&
                                        (dev->summary_indicator != NULL));
-                sch->curr_status.scsw.count = ccw.count - len;
+                sch->curr_status.scsw.count = ccw.count - sizeof(thinint);
                 ret = 0;
             }
         }
@@ -714,13 +654,9 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
             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);
+        ccw_dstream_read_buf(&sch->cds, &revinfo, 4);
+        be16_to_cpus(&revinfo.revision);
+        be16_to_cpus(&revinfo.length);
         if (ccw.count < len + revinfo.length ||
             (check_len && ccw.count > len + revinfo.length)) {
             ret = -EINVAL;
-- 
2.13.5

[Qemu-devel] [PATCH v2 4/4] s390x/css: support ccw IDA

[Halil Pasic]

Let's add indirect data addressing support for our virtual channel
subsystem. This implementation does no bother with any kind of
prefetching. We simply step trough the IDAL on demand.

Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
---
 hw/s390x/css.c | 109 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 108 insertions(+), 1 deletion(-)

diff --git a/hw/s390x/css.c b/hw/s390x/css.c
index 6b0cd8861b..e34b2af4eb 100644
--- a/hw/s390x/css.c
+++ b/hw/s390x/css.c
@@ -819,6 +819,113 @@ incr:
     return 0;
 }
 
+/* returns values between 1 and bsz, where bs is a power of 2 */
+static inline uint16_t ida_continuous_left(hwaddr cda, uint64_t bsz)
+{
+    return bsz - (cda & (bsz - 1));
+}
+
+static inline uint64_t ccw_ida_block_size(uint8_t flags)
+{
+    return 1ULL << (((flags ^ CDS_F_C64) & (CDS_F_C64 | CDS_F_I2K)) ? 11 : 12);
+}
+
+static inline int ida_read_next_idaw(CcwDataStream *cds)
+{
+    union {uint64_t fmt2; uint32_t fmt1; } idaw;
+    bool is_fmt2 = cds->flags & CDS_F_C64;
+    int ret;
+    hwaddr idaw_addr;
+
+    if (is_fmt2) {
+        idaw_addr = cds->cda_orig + sizeof(idaw.fmt2) * cds->at_idaw;
+        if (idaw_addr & 0x07) {
+            return -EINVAL; /* channel program check */
+        }
+        ret = address_space_rw(&address_space_memory, idaw_addr,
+                               MEMTXATTRS_UNSPECIFIED, (void *) &idaw.fmt2,
+                               sizeof(idaw.fmt2), false);
+        cds->cda = be64_to_cpu(idaw.fmt2);
+    } else {
+        idaw_addr = cds->cda_orig + sizeof(idaw.fmt1) * cds->at_idaw;
+        if (idaw_addr & 0x03) {
+            return -EINVAL; /* channel program check */
+
+        }
+        ret = address_space_rw(&address_space_memory, idaw_addr,
+                               MEMTXATTRS_UNSPECIFIED, (void *) &idaw.fmt1,
+                               sizeof(idaw.fmt1), false);
+        cds->cda = be64_to_cpu(idaw.fmt1);
+    }
+    ++(cds->at_idaw);
+    if (ret != MEMTX_OK) {
+        /* assume inaccessible address */
+        return -EINVAL; /* channel program check */
+
+    }
+    return 0;
+}
+
+static int ccw_dstream_rw_ida(CcwDataStream *cds, void *buff, int len,
+                              CcwDataStreamOp op)
+{
+    uint64_t bsz = ccw_ida_block_size(cds->flags);
+    int ret = 0;
+    uint16_t cont_left, iter_len;
+
+    ret = cds_check_len(cds, len);
+    if (ret <= 0) {
+        return ret;
+    }
+    if (!cds->at_idaw) {
+        /* read first idaw */
+        ret = ida_read_next_idaw(cds);
+        if (ret) {
+            goto err;
+        }
+        cont_left = ida_continuous_left(cds->cda, bsz);
+    } else {
+        cont_left = ida_continuous_left(cds->cda, bsz);
+        if (cont_left == bsz) {
+            ret = ida_read_next_idaw(cds);
+            if (ret) {
+                goto err;
+            }
+            if (cds->cda & (bsz - 1)) {
+                ret = -EINVAL; /* channel program check */
+                goto err;
+            }
+        }
+    }
+    do {
+        iter_len = MIN(len, cont_left);
+        if (op != CDS_OP_A) {
+            ret = address_space_rw(&address_space_memory, cds->cda,
+                                   MEMTXATTRS_UNSPECIFIED, buff, iter_len, op);
+            if (ret != MEMTX_OK) {
+                /* assume inaccessible address */
+                ret = -EINVAL; /* channel program check */
+                goto err;
+            }
+        }
+        cds->at_byte += iter_len;
+        cds->cda += iter_len;
+        len -= iter_len;
+        if (!len) {
+            break;
+        }
+        ret = ida_read_next_idaw(cds);
+        if (ret) {
+            goto err;
+        }
+        cont_left = bsz;
+    } while (true);
+    return ret;
+err:
+    cds->flags |= CDS_F_STREAM_BROKEN;
+    return ret;
+}
+
 void ccw_dstream_init(CcwDataStream *cds, CCW1 const *ccw, ORB const *orb)
 {
     /*
@@ -835,7 +942,7 @@ void ccw_dstream_init(CcwDataStream *cds, CCW1 const *ccw, ORB const *orb)
     if (!(cds->flags & CDS_F_IDA)) {
         cds->op_handler = ccw_dstream_rw_noflags;
     } else {
-        assert(false);
+        cds->op_handler = ccw_dstream_rw_ida;
     }
 }
 
-- 
2.13.5

Re: [Qemu-devel] [PATCH v2 3/4] virtio-ccw: use ccw data stream

[Cornelia Huck]

On Wed, 13 Sep 2017 13:50:28 +0200
Halil Pasic <pasic@linux.vnet.ibm.com> wrote:

> Replace direct access which implicitly assumes no IDA
> or MIDA with the new ccw data stream interface which should
> cope with these transparently in the future.
> 
> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>


> @@ -488,7 +446,7 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
>          } else {
>              virtio_bus_get_vdev_config(&dev->bus, vdev->config);
>              /* XXX config space endianness */
> -            cpu_physical_memory_write(ccw.cda, vdev->config, len);
> +            ccw_dstream_write_buf(&sch->cds, vdev->config, len);
>              sch->curr_status.scsw.count = ccw.count - len;
>              ret = 0;
>          }
> @@ -501,21 +459,13 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
>              }
>          }
>          len = MIN(ccw.count, vdev->config_len);
> -        hw_len = len;
>          if (!ccw.cda) {
>              ret = -EFAULT;
>          } else {
> -            config = cpu_physical_memory_map(ccw.cda, &hw_len, 0);
> -            if (!config) {
> -                ret = -EFAULT;
> -            } else {
> -                len = hw_len;
> -                /* XXX config space endianness */
> -                memcpy(vdev->config, config, len);
> -                cpu_physical_memory_unmap(config, hw_len, 0, hw_len);
> +            ret = ccw_dstream_read_buf(&sch->cds, vdev->config, len);
> +            if (!ret) {
>                  virtio_bus_set_vdev_config(&dev->bus, vdev->config);
>                  sch->curr_status.scsw.count = ccw.count - len;
> -                ret = 0;
>              }
>          }
>          break;

It feels a bit odd that you remove the stale XXX comment only for one
direction. Care to post a patch that removes them separately? Else I
can do that as well.

[I'd queue that patch in front of this patchset, but no need to resend
for that.]

Re: [Qemu-devel] [PATCH v2 1/4] s390x/css: introduce css data stream

[Cornelia Huck]

On Wed, 13 Sep 2017 13:50:26 +0200
Halil Pasic <pasic@linux.vnet.ibm.com> wrote:

> This is a preparation for introducing handling for indirect data
> addressing and modified indirect data addressing (CCW). Here we introduce
> an interface which should make the addressing scheme transparent for the
> client code. Here we implement only the basic scheme (no IDA or MIDA).
> 
> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
> ---
>  hw/s390x/css.c         | 55 +++++++++++++++++++++++++++++++++++++++++
>  include/hw/s390x/css.h | 67 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 122 insertions(+)
> 

> +void ccw_dstream_init(CcwDataStream *cds, CCW1 const *ccw, ORB const *orb)
> +{
> +    /*
> +     * We don't support MIDA (an optional facility) yet and we
> +     * catch this earlier. Just for expressing the precondition.
> +     */
> +    g_assert(!(orb->ctrl1 & ORB_CTRL1_MASK_MIDAW));
> +    cds->flags = (orb->ctrl0 & ORB_CTRL0_MASK_I2K ? CDS_F_I2K : 0) |
> +                 (orb->ctrl0 & ORB_CTRL0_MASK_C64 ? CDS_F_C64 : 0) |
> +                 (ccw->flags & CCW_FLAG_IDA ? CDS_F_IDA : 0);
> +    cds->count = ccw->count;
> +    cds->cda_orig = ccw->cda;
> +    ccw_dstream_rewind(cds);
> +    if (!(cds->flags & CDS_F_IDA)) {
> +        cds->op_handler = ccw_dstream_rw_noflags;
> +    } else {
> +        assert(false);

g_assert_not_reached() might have been better; but as this is removed
anyway in patch 4, it does not matter.

> +    }
> +}
>  
>  static int css_interpret_ccw(SubchDev *sch, hwaddr ccw_addr,
>                               bool suspend_allowed)
> diff --git a/include/hw/s390x/css.h b/include/hw/s390x/css.h
> index 0653d3c9be..79acaf99b7 100644
> --- a/include/hw/s390x/css.h
> +++ b/include/hw/s390x/css.h
> @@ -75,6 +75,29 @@ typedef struct CMBE {
>      uint32_t reserved[7];
>  } QEMU_PACKED CMBE;
>  
> +typedef enum CcwDataStreamOp {
> +    CDS_OP_R = 0,
> +    CDS_OP_W = 1,
> +    CDS_OP_A = 2
> +} CcwDataStreamOp;
> +
> +/** normal usage is via SuchchDev.cds instead of instantiating */

/* instead of /**? Can change while applying.

> +typedef struct CcwDataStream {
> +#define CDS_F_IDA   0x01
> +#define CDS_F_MIDA  0x02
> +#define CDS_F_I2K   0x04
> +#define CDS_F_C64   0x08
> +#define CDS_F_STREAM_BROKEN  0x80
> +    uint8_t flags;
> +    uint8_t at_idaw;
> +    uint16_t at_byte;
> +    uint16_t count;
> +    uint32_t cda_orig;
> +    int (*op_handler)(struct CcwDataStream *cds, void *buff, int len,
> +                      CcwDataStreamOp op);
> +    hwaddr cda;
> +} CcwDataStream;
> +
>  typedef struct SubchDev SubchDev;
>  struct SubchDev {
>      /* channel-subsystem related things: */

Re: [Qemu-devel] [PATCH v2 4/4] s390x/css: support ccw IDA

[Cornelia Huck]

On Wed, 13 Sep 2017 13:50:29 +0200
Halil Pasic <pasic@linux.vnet.ibm.com> wrote:

> Let's add indirect data addressing support for our virtual channel
> subsystem. This implementation does no bother with any kind of

s/no/not/

> prefetching. We simply step trough the IDAL on demand.

s/trough/through/

> 
> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
> ---
>  hw/s390x/css.c | 109 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 108 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/s390x/css.c b/hw/s390x/css.c
> index 6b0cd8861b..e34b2af4eb 100644
> --- a/hw/s390x/css.c
> +++ b/hw/s390x/css.c
> @@ -819,6 +819,113 @@ incr:
>      return 0;
>  }
>  
> +/* returns values between 1 and bsz, where bs is a power of 2 */

s/bz/bsz/ (missed the second one)

I can fix the typos while applying.

Re: [Qemu-devel] [PATCH v2 0/4] add CCW indirect data access support

[Cornelia Huck]

On Wed, 13 Sep 2017 13:50:25 +0200
Halil Pasic <pasic@linux.vnet.ibm.com> wrote:

> Abstract
> --------
> 
> The objective of this series is introducing CCW IDA (indirect data
> access) support to our virtual channel subsystem implementation. Briefly
> CCW IDA can be thought of as a kind of a scatter gather support for a
> single CCW. If certain flags are set, the cda is to be interpreted as an
> address to a list which in turn holds further addresses designating the
> actual data.  Thus the scheme which we are currently using for accessing
> CCW payload does not work in general case. Currently there is no
> immediate need for proper IDA handling (no use case), but since it IDA is
> a non-optional part of the architecture, the only way towards AR
> compliance is actually implementing IDA.
> 
> Testing
> -------
> 
> On request the things meant for testing from v1 were factored out
> into a separate series (requested by Connie). Please look for
> the series  'tests for CCW IDA' (comming soon) or use the stuff
> form v1.

Generally, looks good; currently testing it.

Would not mind some R-bs :)

Re: [Qemu-devel] [PATCH v2 0/4] add CCW indirect data access support

[Halil Pasic]

On 09/14/2017 11:15 AM, Cornelia Huck wrote:
> On Wed, 13 Sep 2017 13:50:25 +0200
> Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
> 
>> Abstract
>> --------
>>
>> The objective of this series is introducing CCW IDA (indirect data
>> access) support to our virtual channel subsystem implementation. Briefly
>> CCW IDA can be thought of as a kind of a scatter gather support for a
>> single CCW. If certain flags are set, the cda is to be interpreted as an
>> address to a list which in turn holds further addresses designating the
>> actual data.  Thus the scheme which we are currently using for accessing
>> CCW payload does not work in general case. Currently there is no
>> immediate need for proper IDA handling (no use case), but since it IDA is
>> a non-optional part of the architecture, the only way towards AR
>> compliance is actually implementing IDA.
>>
>> Testing
>> -------
>>
>> On request the things meant for testing from v1 were factored out
>> into a separate series (requested by Connie). Please look for
>> the series  'tests for CCW IDA' (comming soon) or use the stuff
>> form v1.
> 
> Generally, looks good; currently testing it.
> 
> Would not mind some R-bs :)
> 

Many thanks for the quick review! Of course I consent to every
change you have proposed to make (before applying).

About the stale comments I've just sent out a patch.

About the r-b's I think the guys in cc are the most likely
candidates. Pierre should be back starting next week. Dong
Jia I haven't seen in a while, but I don't know about anything.
Of course I would happy if somebody less expected joins in.

Thanks again!

Halil

Re: [Qemu-devel] [PATCH v2 0/4] add CCW indirect data access support

[Cornelia Huck]

On Thu, 14 Sep 2017 13:02:51 +0200
Halil Pasic <pasic@linux.vnet.ibm.com> wrote:

> About the r-b's I think the guys in cc are the most likely
> candidates. Pierre should be back starting next week. Dong
> Jia I haven't seen in a while, but I don't know about anything.

Let's see what comes in, although I don't want to delay sending the
next s390x pull request for too long. Probably next week or so. I'd be
happy to record any tags prior to that.

> Of course I would happy if somebody less expected joins in.

Seconded :)

Re: [Qemu-devel] [PATCH v2 0/4] add CCW indirect data access support

[Cornelia Huck]

On Wed, 13 Sep 2017 13:50:25 +0200
Halil Pasic <pasic@linux.vnet.ibm.com> wrote:

> Abstract
> --------
> 
> The objective of this series is introducing CCW IDA (indirect data
> access) support to our virtual channel subsystem implementation. Briefly
> CCW IDA can be thought of as a kind of a scatter gather support for a
> single CCW. If certain flags are set, the cda is to be interpreted as an
> address to a list which in turn holds further addresses designating the
> actual data.  Thus the scheme which we are currently using for accessing
> CCW payload does not work in general case. Currently there is no
> immediate need for proper IDA handling (no use case), but since it IDA is
> a non-optional part of the architecture, the only way towards AR
> compliance is actually implementing IDA.

Thanks, applied.

Re: [Qemu-devel] [PATCH v2 1/4] s390x/css: introduce css data stream

[Dong Jia Shi]

* Halil Pasic <pasic@linux.vnet.ibm.com> [2017-09-13 13:50:26 +0200]:

[...]

> diff --git a/include/hw/s390x/css.h b/include/hw/s390x/css.h
> index 0653d3c9be..79acaf99b7 100644
> --- a/include/hw/s390x/css.h
> +++ b/include/hw/s390x/css.h
> @@ -75,6 +75,29 @@ typedef struct CMBE {
>      uint32_t reserved[7];
>  } QEMU_PACKED CMBE;
> 
> +typedef enum CcwDataStreamOp {
> +    CDS_OP_R = 0,
> +    CDS_OP_W = 1,
> +    CDS_OP_A = 2

Nit:

typedef enum CcwDataStreamOp {
    CDS_OP_R, /* read */
    CDS_OP_W, /* write */
    CDS_OP_A  /* advance */
} CcwDataStreamOp;

(I just keep translating 'A' as append in my mind...)

> +} CcwDataStreamOp;
> +
[...]

> +static inline uint16_t ccw_dstream_avail(CcwDataStream *cds)
> +{
> +    return ccw_dstream_good(cds) ?  ccw_dstream_residual_count(cds) : 0;
                                     ^^
Nit.

> +}

[...]

With or w/o responses the nit picks:
Reviewed-by: Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com>

-- 
Dong Jia Shi

Re: [Qemu-devel] [PATCH v2 2/4] s390x/css: use ccw data stream

[Dong Jia Shi]

* Halil Pasic <pasic@linux.vnet.ibm.com> [2017-09-13 13:50:27 +0200]:

> Replace direct access which implicitly assumes no IDA
> or MIDA with the new ccw data stream interface which should
> cope with these transparently in the future.
> 
> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
> ---
>  hw/s390x/css.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/s390x/css.c b/hw/s390x/css.c
> index e8d2016563..6b0cd8861b 100644
> --- a/hw/s390x/css.c
> +++ b/hw/s390x/css.c
> @@ -890,6 +890,7 @@ static int css_interpret_ccw(SubchDev *sch, hwaddr ccw_addr,
>      }
> 
>      /* Look at the command. */
> +    ccw_dstream_init(&sch->cds, &ccw, &(sch->orb));
>      switch (ccw.cmd_code) {
>      case CCW_CMD_NOOP:
>          /* Nothing to do. */
> @@ -903,8 +904,8 @@ static int css_interpret_ccw(SubchDev *sch, hwaddr ccw_addr,
>              }
>          }
>          len = MIN(ccw.count, sizeof(sch->sense_data));
> -        cpu_physical_memory_write(ccw.cda, sch->sense_data, len);
> -        sch->curr_status.scsw.count = ccw.count - len;
> +        ccw_dstream_write_buf(&sch->cds, sch->sense_data, len);
> +        sch->curr_status.scsw.count = ccw_dstream_residual_count(&sch->cds);
>          memset(sch->sense_data, 0, sizeof(sch->sense_data));
>          ret = 0;
>          break;
> @@ -930,8 +931,8 @@ static int css_interpret_ccw(SubchDev *sch, hwaddr ccw_addr,
>          } else {
>              sense_id.reserved = 0;
>          }
> -        cpu_physical_memory_write(ccw.cda, &sense_id, len);
> -        sch->curr_status.scsw.count = ccw.count - len;
> +        ccw_dstream_write_buf(&sch->cds, &sense_id, len);
> +        sch->curr_status.scsw.count = ccw_dstream_residual_count(&sch->cds);
>          ret = 0;
>          break;
>      }
> -- 
> 2.13.5
> 

Reviewed-by: Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com>

-- 
Dong Jia Shi

Re: [Qemu-devel] [PATCH v2 3/4] virtio-ccw: use ccw data stream

[Dong Jia Shi]

* Halil Pasic <pasic@linux.vnet.ibm.com> [2017-09-13 13:50:28 +0200]:

> Replace direct access which implicitly assumes no IDA
> or MIDA with the new ccw data stream interface which should
> cope with these transparently in the future.
> 
> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
> 
> ---
> 
> v1 --> v2:
> Removed todo comments on possible errno change as with
> https://lists.nongnu.org/archive/html/qemu-devel/2017-09/msg02441.html
> applied it does not matter any more.
> 
> Error handling: At the moment we ignore errors reported
> by stream ops to keep the change minimal. An add-on
> patch improving on this is to be expected later.
Add a TODO somewhere around the code as a reminder?

> ---
>  hw/s390x/virtio-ccw.c | 156 +++++++++++++++-----------------------------------
>  1 file changed, 46 insertions(+), 110 deletions(-)
> 
> diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
> index b1976fdd19..a9baf6f7ab 100644
> --- a/hw/s390x/virtio-ccw.c
> +++ b/hw/s390x/virtio-ccw.c
[...]

> @@ -394,11 +362,8 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
>          } else {
>              VirtioDeviceClass *vdc = VIRTIO_DEVICE_GET_CLASS(vdev);
> 
> -            features.index = address_space_ldub(&address_space_memory,
> -                                                ccw.cda
> -                                                + sizeof(features.features),
> -                                                MEMTXATTRS_UNSPECIFIED,
> -                                                NULL);
> +            ccw_dstream_advance(&sch->cds, sizeof(features.features));
How about:
ccw_dstream_advance(&sch->cds, offsetof(VirtioFeatDesc, index));

> +            ccw_dstream_read(&sch->cds, features.index);
>              if (features.index == 0) {
>                  if (dev->revision >= 1) {
>                      /* Don't offer legacy features for modern devices. */
> @@ -417,9 +382,9 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
>                  /* Return zeroes if the guest supports more feature bits. */
>                  features.features = 0;
>              }
> -            address_space_stl_le(&address_space_memory, ccw.cda,
> -                                 features.features, MEMTXATTRS_UNSPECIFIED,
> -                                 NULL);
> +            ccw_dstream_rewind(&sch->cds);
> +            cpu_to_le32s(&features.features);
> +            ccw_dstream_write(&sch->cds, features.features);
>              sch->curr_status.scsw.count = ccw.count - sizeof(features);
How about:
sch->curr_status.scsw.count = ccw_dstream_residual_count(&sch->cds);

This also applies to other places.

>              ret = 0;
>          }
[...]

> @@ -501,21 +459,13 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
>              }
>          }
>          len = MIN(ccw.count, vdev->config_len);
> -        hw_len = len;
>          if (!ccw.cda) {
>              ret = -EFAULT;
>          } else {
> -            config = cpu_physical_memory_map(ccw.cda, &hw_len, 0);
> -            if (!config) {
> -                ret = -EFAULT;
> -            } else {
> -                len = hw_len;
> -                /* XXX config space endianness */
> -                memcpy(vdev->config, config, len);
> -                cpu_physical_memory_unmap(config, hw_len, 0, hw_len);
> +            ret = ccw_dstream_read_buf(&sch->cds, vdev->config, len);
> +            if (!ret) {
Add a TODO here, and:

if (ccw_dstream_read_buf(&sch->cds, vdev->config, len)) {
    ret = -EFAULT;
} else {
    ....
}

>                  virtio_bus_set_vdev_config(&dev->bus, vdev->config);
>                  sch->curr_status.scsw.count = ccw.count - len;
> -                ret = 0;
>              }
>          }
>          break;
[...]

> @@ -714,13 +654,9 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
>              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);
> +        ccw_dstream_read_buf(&sch->cds, &revinfo, 4);
                                                     ^
A magic number? O:)

> +        be16_to_cpus(&revinfo.revision);
> +        be16_to_cpus(&revinfo.length);
>          if (ccw.count < len + revinfo.length ||
>              (check_len && ccw.count > len + revinfo.length)) {
>              ret = -EINVAL;
> -- 
> 2.13.5
> 

Otherwise, looks good.

-- 
Dong Jia Shi

Re: [Qemu-devel] [PATCH v2 4/4] s390x/css: support ccw IDA

[Dong Jia Shi]

* Halil Pasic <pasic@linux.vnet.ibm.com> [2017-09-13 13:50:29 +0200]:

> Let's add indirect data addressing support for our virtual channel
> subsystem. This implementation does no bother with any kind of
> prefetching. We simply step trough the IDAL on demand.
> 
> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
> ---
>  hw/s390x/css.c | 109 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 108 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/s390x/css.c b/hw/s390x/css.c
> index 6b0cd8861b..e34b2af4eb 100644
> --- a/hw/s390x/css.c
> +++ b/hw/s390x/css.c
> @@ -819,6 +819,113 @@ incr:
>      return 0;
>  }
> 
> +/* returns values between 1 and bsz, where bs is a power of 2 */
> +static inline uint16_t ida_continuous_left(hwaddr cda, uint64_t bsz)
> +{
> +    return bsz - (cda & (bsz - 1));
> +}
> +
> +static inline uint64_t ccw_ida_block_size(uint8_t flags)
> +{
> +    return 1ULL << (((flags ^ CDS_F_C64) & (CDS_F_C64 | CDS_F_I2K)) ? 11 : 12);
If CDS_F_C64 is set, (flags ^ CDS_F_C64) will be 0, so (1ULL << 11) will
be the result regardless the I2K flag? The logic seems wrong.

I2K is meaningful only when C64 is 1, otherwise it is ignored. The logic
here should be:
if ((flags & CDS_F_C64) && !(flags & CDS_F_I2K)) {
    return 1ULL << 12;
}
    return 1ULL << 11;

> +}
> +
> +static inline int ida_read_next_idaw(CcwDataStream *cds)
> +{
> +    union {uint64_t fmt2; uint32_t fmt1; } idaw;
                                           ^
Nit.

> +    bool is_fmt2 = cds->flags & CDS_F_C64;
> +    int ret;
> +    hwaddr idaw_addr;
> +
> +    if (is_fmt2) {
> +        idaw_addr = cds->cda_orig + sizeof(idaw.fmt2) * cds->at_idaw;
> +        if (idaw_addr & 0x07) {
> +            return -EINVAL; /* channel program check */
> +        }
> +        ret = address_space_rw(&address_space_memory, idaw_addr,
> +                               MEMTXATTRS_UNSPECIFIED, (void *) &idaw.fmt2,
> +                               sizeof(idaw.fmt2), false);
> +        cds->cda = be64_to_cpu(idaw.fmt2);
> +    } else {
> +        idaw_addr = cds->cda_orig + sizeof(idaw.fmt1) * cds->at_idaw;
> +        if (idaw_addr & 0x03) {
?:
(idaw_addr & 0x80000003)

> +            return -EINVAL; /* channel program check */
> +
> +        }
> +        ret = address_space_rw(&address_space_memory, idaw_addr,
> +                               MEMTXATTRS_UNSPECIFIED, (void *) &idaw.fmt1,
> +                               sizeof(idaw.fmt1), false);
> +        cds->cda = be64_to_cpu(idaw.fmt1);
> +    }
> +    ++(cds->at_idaw);
> +    if (ret != MEMTX_OK) {
> +        /* assume inaccessible address */
> +        return -EINVAL; /* channel program check */
> +
> +    }
> +    return 0;
> +}
> +
> +static int ccw_dstream_rw_ida(CcwDataStream *cds, void *buff, int len,
> +                              CcwDataStreamOp op)
> +{
> +    uint64_t bsz = ccw_ida_block_size(cds->flags);
> +    int ret = 0;
> +    uint16_t cont_left, iter_len;
> +
> +    ret = cds_check_len(cds, len);
> +    if (ret <= 0) {
> +        return ret;
> +    }
> +    if (!cds->at_idaw) {
> +        /* read first idaw */
> +        ret = ida_read_next_idaw(cds);
> +        if (ret) {
> +            goto err;
> +        }
> +        cont_left = ida_continuous_left(cds->cda, bsz);
> +    } else {
> +        cont_left = ida_continuous_left(cds->cda, bsz);
> +        if (cont_left == bsz) {
> +            ret = ida_read_next_idaw(cds);
> +            if (ret) {
> +                goto err;
> +            }
> +            if (cds->cda & (bsz - 1)) {
Could move this check into ida_read_next_idaw?

> +                ret = -EINVAL; /* channel program check */
> +                goto err;
> +            }
> +        }
> +    }
> +    do {
> +        iter_len = MIN(len, cont_left);
> +        if (op != CDS_OP_A) {
> +            ret = address_space_rw(&address_space_memory, cds->cda,
> +                                   MEMTXATTRS_UNSPECIFIED, buff, iter_len, op);
Ahh, now I recall that explictly defining CDS_OP_R to 0 and CDS_OP_W to
1 in 'struct CcwDataStreamOp' do have a meaning. Does it make sense to
make it more obvious by adding some comment there?

> +            if (ret != MEMTX_OK) {
> +                /* assume inaccessible address */
> +                ret = -EINVAL; /* channel program check */
> +                goto err;
> +            }
> +        }
> +        cds->at_byte += iter_len;
> +        cds->cda += iter_len;
> +        len -= iter_len;
> +        if (!len) {
> +            break;
> +        }
> +        ret = ida_read_next_idaw(cds);
> +        if (ret) {
> +            goto err;
> +        }
> +        cont_left = bsz;
> +    } while (true);
> +    return ret;
> +err:
> +    cds->flags |= CDS_F_STREAM_BROKEN;
> +    return ret;
> +}
> +
>  void ccw_dstream_init(CcwDataStream *cds, CCW1 const *ccw, ORB const *orb)
>  {
>      /*
> @@ -835,7 +942,7 @@ void ccw_dstream_init(CcwDataStream *cds, CCW1 const *ccw, ORB const *orb)
>      if (!(cds->flags & CDS_F_IDA)) {
>          cds->op_handler = ccw_dstream_rw_noflags;
>      } else {
> -        assert(false);
> +        cds->op_handler = ccw_dstream_rw_ida;
>      }
>  }
> 
> -- 
> 2.13.5
> 

Generally, the logic looks fine to me.

-- 
Dong Jia Shi

Re: [Qemu-devel] [PATCH v2 1/4] s390x/css: introduce css data stream

[Cornelia Huck]

On Tue, 19 Sep 2017 10:21:42 +0800
Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com> wrote:

> * Halil Pasic <pasic@linux.vnet.ibm.com> [2017-09-13 13:50:26 +0200]:
> 
> [...]
> 
> > diff --git a/include/hw/s390x/css.h b/include/hw/s390x/css.h
> > index 0653d3c9be..79acaf99b7 100644
> > --- a/include/hw/s390x/css.h
> > +++ b/include/hw/s390x/css.h
> > @@ -75,6 +75,29 @@ typedef struct CMBE {
> >      uint32_t reserved[7];
> >  } QEMU_PACKED CMBE;
> > 
> > +typedef enum CcwDataStreamOp {
> > +    CDS_OP_R = 0,
> > +    CDS_OP_W = 1,
> > +    CDS_OP_A = 2  
> 
> Nit:
> 
> typedef enum CcwDataStreamOp {
>     CDS_OP_R, /* read */
>     CDS_OP_W, /* write */
>     CDS_OP_A  /* advance */
> } CcwDataStreamOp;
> 
> (I just keep translating 'A' as append in my mind...)

Adding comments makes sense. Done.

> 
> > +} CcwDataStreamOp;
> > +  
> [...]
> 
> > +static inline uint16_t ccw_dstream_avail(CcwDataStream *cds)
> > +{
> > +    return ccw_dstream_good(cds) ?  ccw_dstream_residual_count(cds) : 0;  
>                                      ^^
> Nit.

Fixed.

> 
> > +}  
> 
> [...]
> 
> With or w/o responses the nit picks:
> Reviewed-by: Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com>
> 

Thanks!

Re: [Qemu-devel] [PATCH v2 3/4] virtio-ccw: use ccw data stream

[Cornelia Huck]

On Tue, 19 Sep 2017 11:37:30 +0800
Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com> wrote:

> * Halil Pasic <pasic@linux.vnet.ibm.com> [2017-09-13 13:50:28 +0200]:
> 
> > Replace direct access which implicitly assumes no IDA
> > or MIDA with the new ccw data stream interface which should
> > cope with these transparently in the future.
> > 
> > Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
> > 
> > ---
> > 
> > v1 --> v2:
> > Removed todo comments on possible errno change as with
> > https://lists.nongnu.org/archive/html/qemu-devel/2017-09/msg02441.html
> > applied it does not matter any more.
> > 
> > Error handling: At the moment we ignore errors reported
> > by stream ops to keep the change minimal. An add-on
> > patch improving on this is to be expected later.  
> Add a TODO somewhere around the code as a reminder?

I expect Halil to have it on his TODO list and send a patch later ;)

> 
> > ---
> >  hw/s390x/virtio-ccw.c | 156 +++++++++++++++-----------------------------------
> >  1 file changed, 46 insertions(+), 110 deletions(-)
> > 
> > diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
> > index b1976fdd19..a9baf6f7ab 100644
> > --- a/hw/s390x/virtio-ccw.c
> > +++ b/hw/s390x/virtio-ccw.c  
> [...]
> 
> > @@ -394,11 +362,8 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
> >          } else {
> >              VirtioDeviceClass *vdc = VIRTIO_DEVICE_GET_CLASS(vdev);
> > 
> > -            features.index = address_space_ldub(&address_space_memory,
> > -                                                ccw.cda
> > -                                                + sizeof(features.features),
> > -                                                MEMTXATTRS_UNSPECIFIED,
> > -                                                NULL);
> > +            ccw_dstream_advance(&sch->cds, sizeof(features.features));  
> How about:
> ccw_dstream_advance(&sch->cds, offsetof(VirtioFeatDesc, index));

I think keeping sizeof(features.features) is better: It matches the old
code, and we really do jump over the features flag and revisit it
later...

> 
> > +            ccw_dstream_read(&sch->cds, features.index);
> >              if (features.index == 0) {
> >                  if (dev->revision >= 1) {
> >                      /* Don't offer legacy features for modern devices. */
> > @@ -417,9 +382,9 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
> >                  /* Return zeroes if the guest supports more feature bits. */
> >                  features.features = 0;
> >              }
> > -            address_space_stl_le(&address_space_memory, ccw.cda,
> > -                                 features.features, MEMTXATTRS_UNSPECIFIED,
> > -                                 NULL);
> > +            ccw_dstream_rewind(&sch->cds);
> > +            cpu_to_le32s(&features.features);
> > +            ccw_dstream_write(&sch->cds, features.features);
> >              sch->curr_status.scsw.count = ccw.count - sizeof(features);  
> How about:
> sch->curr_status.scsw.count = ccw_dstream_residual_count(&sch->cds);

Hm, I thought I had commented on this, but I seem to have missed
these...

I'd prefer to do it as a follow-up patch.

> 
> This also applies to other places.
> 
> >              ret = 0;
> >          }  
> [...]
> 
> > @@ -501,21 +459,13 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
> >              }
> >          }
> >          len = MIN(ccw.count, vdev->config_len);
> > -        hw_len = len;
> >          if (!ccw.cda) {
> >              ret = -EFAULT;
> >          } else {
> > -            config = cpu_physical_memory_map(ccw.cda, &hw_len, 0);
> > -            if (!config) {
> > -                ret = -EFAULT;
> > -            } else {
> > -                len = hw_len;
> > -                /* XXX config space endianness */
> > -                memcpy(vdev->config, config, len);
> > -                cpu_physical_memory_unmap(config, hw_len, 0, hw_len);
> > +            ret = ccw_dstream_read_buf(&sch->cds, vdev->config, len);
> > +            if (!ret) {  
> Add a TODO here, and:
> 
> if (ccw_dstream_read_buf(&sch->cds, vdev->config, len)) {
>     ret = -EFAULT;
> } else {
>     ....
> }

I don't think that would be correct: The function will (at least
currently) return 0 or -EINVAL, and you are now mapping any error to
-EFAULT? (Not that it has an effect in the end: We map both to a
channel program check as of "s390x/css: drop data-check in
interpretation".)

> 
> >                  virtio_bus_set_vdev_config(&dev->bus, vdev->config);
> >                  sch->curr_status.scsw.count = ccw.count - len;
> > -                ret = 0;
> >              }
> >          }
> >          break;  
> [...]
> 
> > @@ -714,13 +654,9 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
> >              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);
> > +        ccw_dstream_read_buf(&sch->cds, &revinfo, 4);  
>                                                      ^
> A magic number? O:)

Hah :)

We can't use sizeof(revinfo), and sizeof(revinfo.revision) +
sizeof(revinfo.length) is a bit unwieldy. Let's keep some magic in our
code :)

> 
> > +        be16_to_cpus(&revinfo.revision);
> > +        be16_to_cpus(&revinfo.length);
> >          if (ccw.count < len + revinfo.length ||
> >              (check_len && ccw.count > len + revinfo.length)) {
> >              ret = -EINVAL;
> > -- 
> > 2.13.5
> >   
> 
> Otherwise, looks good.
> 

Can I get an ack?

Re: [Qemu-devel] [PATCH v2 1/4] s390x/css: introduce css data stream

[Pierre Morel]

On 13/09/2017 13:50, Halil Pasic wrote:
> This is a preparation for introducing handling for indirect data
> addressing and modified indirect data addressing (CCW). Here we introduce
> an interface which should make the addressing scheme transparent for the
> client code. Here we implement only the basic scheme (no IDA or MIDA).
> 
> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
> ---
>   hw/s390x/css.c         | 55 +++++++++++++++++++++++++++++++++++++++++
>   include/hw/s390x/css.h | 67 ++++++++++++++++++++++++++++++++++++++++++++++++++
>   2 files changed, 122 insertions(+)
> 
> diff --git a/hw/s390x/css.c b/hw/s390x/css.c
> index 901dc6a0f3..e8d2016563 100644
> --- a/hw/s390x/css.c
> +++ b/hw/s390x/css.c
> @@ -783,6 +783,61 @@ static CCW1 copy_ccw_from_guest(hwaddr addr, bool fmt1)
>       }
>       return ret;
>   }
> +/**
> + * If out of bounds marks the stream broken. If broken returns -EINVAL,
> + * otherwise the requested length (may be zero)
> + */
> +static inline int cds_check_len(CcwDataStream *cds, int len)
> +{
> +    if (cds->at_byte + len > cds->count) {
> +        cds->flags |= CDS_F_STREAM_BROKEN;
> +    }
> +    return cds->flags & CDS_F_STREAM_BROKEN ? -EINVAL : len;
> +}
> +
> +static int ccw_dstream_rw_noflags(CcwDataStream *cds, void *buff, int len,
> +                                  CcwDataStreamOp op)
> +{
> +    int ret;
> +
> +    ret = cds_check_len(cds, len);
> +    if (ret <= 0) {
> +        return ret;
> +    }
> +    if (op == CDS_OP_A) {
> +        goto incr;
> +    }
> +    ret = address_space_rw(&address_space_memory, cds->cda,
> +                           MEMTXATTRS_UNSPECIFIED, buff, len, op);
> +    if (ret != MEMTX_OK) {
> +        cds->flags |= CDS_F_STREAM_BROKEN;
> +        return -EINVAL;
> +    }
> +incr:
> +    cds->at_byte += len;
> +    cds->cda += len;
> +    return 0;
> +}
> +
> +void ccw_dstream_init(CcwDataStream *cds, CCW1 const *ccw, ORB const *orb)
> +{
> +    /*
> +     * We don't support MIDA (an optional facility) yet and we
> +     * catch this earlier. Just for expressing the precondition.
> +     */
> +    g_assert(!(orb->ctrl1 & ORB_CTRL1_MASK_MIDAW));
> +    cds->flags = (orb->ctrl0 & ORB_CTRL0_MASK_I2K ? CDS_F_I2K : 0) |
> +                 (orb->ctrl0 & ORB_CTRL0_MASK_C64 ? CDS_F_C64 : 0) |
> +                 (ccw->flags & CCW_FLAG_IDA ? CDS_F_IDA : 0);
> +    cds->count = ccw->count;
> +    cds->cda_orig = ccw->cda;
> +    ccw_dstream_rewind(cds);
> +    if (!(cds->flags & CDS_F_IDA)) {
> +        cds->op_handler = ccw_dstream_rw_noflags;
> +    } else {
> +        assert(false);
> +    }
> +}
> 
>   static int css_interpret_ccw(SubchDev *sch, hwaddr ccw_addr,
>                                bool suspend_allowed)
> diff --git a/include/hw/s390x/css.h b/include/hw/s390x/css.h
> index 0653d3c9be..79acaf99b7 100644
> --- a/include/hw/s390x/css.h
> +++ b/include/hw/s390x/css.h
> @@ -75,6 +75,29 @@ typedef struct CMBE {
>       uint32_t reserved[7];
>   } QEMU_PACKED CMBE;
> 
> +typedef enum CcwDataStreamOp {
> +    CDS_OP_R = 0,
> +    CDS_OP_W = 1,
> +    CDS_OP_A = 2
> +} CcwDataStreamOp;
> +
> +/** normal usage is via SuchchDev.cds instead of instantiating */
> +typedef struct CcwDataStream {
> +#define CDS_F_IDA   0x01
> +#define CDS_F_MIDA  0x02
> +#define CDS_F_I2K   0x04
> +#define CDS_F_C64   0x08
> +#define CDS_F_STREAM_BROKEN  0x80
> +    uint8_t flags;
> +    uint8_t at_idaw;
> +    uint16_t at_byte;
> +    uint16_t count;
> +    uint32_t cda_orig;
> +    int (*op_handler)(struct CcwDataStream *cds, void *buff, int len,
> +                      CcwDataStreamOp op);

I would have prefer one handler per operation instead of a operation 
parameter.

Is it possible to change the name of the "buf" argument to "arg".
I just think of the foollowing:
If one day we do not want to gather all IDAs into a single buffer but 
keep them split, we can have something like an array of buffers as argument.



> +    hwaddr cda;
> +} CcwDataStream;
> +
>   typedef struct SubchDev SubchDev;
>   struct SubchDev {
>       /* channel-subsystem related things: */
> @@ -92,6 +115,7 @@ struct SubchDev {
>       uint8_t ccw_no_data_cnt;
>       uint16_t migrated_schid; /* used for missmatch detection */
>       ORB orb;
> +    CcwDataStream cds;
>       /* transport-provided data: */
>       int (*ccw_cb) (SubchDev *, CCW1);
>       void (*disable_cb)(SubchDev *);
> @@ -240,4 +264,47 @@ SubchDev *css_create_sch(CssDevId bus_id, bool is_virtual, bool squash_mcss,
>   /** Turn on css migration */
>   void css_register_vmstate(void);
> 
> +
> +void ccw_dstream_init(CcwDataStream *cds, CCW1 const *ccw, ORB const *orb);
> +
> +static inline void ccw_dstream_rewind(CcwDataStream *cds)
> +{
> +    cds->at_byte = 0;
> +    cds->at_idaw = 0;
> +    cds->cda = cds->cda_orig;
> +}
> +
> +static inline bool ccw_dstream_good(CcwDataStream *cds)
> +{
> +    return !(cds->flags & CDS_F_STREAM_BROKEN);
> +}
> +
> +static inline uint16_t ccw_dstream_residual_count(CcwDataStream *cds)
> +{
> +    return cds->count - cds->at_byte;
> +}
> +
> +static inline uint16_t ccw_dstream_avail(CcwDataStream *cds)
> +{
> +    return ccw_dstream_good(cds) ?  ccw_dstream_residual_count(cds) : 0;
> +}
> +
> +static inline int ccw_dstream_advance(CcwDataStream *cds, int len)
> +{
> +    return cds->op_handler(cds, NULL, len, CDS_OP_A);
> +}
> +
> +static inline int ccw_dstream_write_buf(CcwDataStream *cds, void *buff, int len)
> +{
> +    return cds->op_handler(cds, buff, len, CDS_OP_W);
> +}
> +
> +static inline int ccw_dstream_read_buf(CcwDataStream *cds, void *buff, int len)
> +{
> +    return cds->op_handler(cds, buff, len, CDS_OP_R);
> +}
> +
> +#define ccw_dstream_read(cds, v) ccw_dstream_read_buf((cds), &(v), sizeof(v))
> +#define ccw_dstream_write(cds, v) ccw_dstream_write_buf((cds), &(v), sizeof(v))
> +
>   #endif
> 

otherwise, LGTM

-- 
Pierre Morel
Linux/KVM/QEMU in Böblingen - Germany

Re: [Qemu-devel] [PATCH v2 4/4] s390x/css: support ccw IDA

[Cornelia Huck]

On Tue, 19 Sep 2017 13:50:05 +0800
Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com> wrote:

> * Halil Pasic <pasic@linux.vnet.ibm.com> [2017-09-13 13:50:29 +0200]:
> 
> > Let's add indirect data addressing support for our virtual channel
> > subsystem. This implementation does no bother with any kind of
> > prefetching. We simply step trough the IDAL on demand.
> > 
> > Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
> > ---
> >  hw/s390x/css.c | 109 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 108 insertions(+), 1 deletion(-)
> > 
> > diff --git a/hw/s390x/css.c b/hw/s390x/css.c
> > index 6b0cd8861b..e34b2af4eb 100644
> > --- a/hw/s390x/css.c
> > +++ b/hw/s390x/css.c
> > @@ -819,6 +819,113 @@ incr:
> >      return 0;
> >  }
> > 
> > +/* returns values between 1 and bsz, where bs is a power of 2 */
> > +static inline uint16_t ida_continuous_left(hwaddr cda, uint64_t bsz)
> > +{
> > +    return bsz - (cda & (bsz - 1));
> > +}
> > +
> > +static inline uint64_t ccw_ida_block_size(uint8_t flags)
> > +{
> > +    return 1ULL << (((flags ^ CDS_F_C64) & (CDS_F_C64 | CDS_F_I2K)) ? 11 : 12);  
> If CDS_F_C64 is set, (flags ^ CDS_F_C64) will be 0, so (1ULL << 11) will
> be the result regardless the I2K flag? The logic seems wrong.

I've stared at that condition now for a bit, but all it managed was to
get me more confused... probably just need a break.

> 
> I2K is meaningful only when C64 is 1, otherwise it is ignored. The logic
> here should be:
> if ((flags & CDS_F_C64) && !(flags & CDS_F_I2K)) {
>     return 1ULL << 12;
> }
>     return 1ULL << 11;

But I do think your version is more readable...

> 
> > +}
> > +
> > +static inline int ida_read_next_idaw(CcwDataStream *cds)
> > +{
> > +    union {uint64_t fmt2; uint32_t fmt1; } idaw;  
>                                            ^
> Nit.
> 
> > +    bool is_fmt2 = cds->flags & CDS_F_C64;
> > +    int ret;
> > +    hwaddr idaw_addr;
> > +
> > +    if (is_fmt2) {
> > +        idaw_addr = cds->cda_orig + sizeof(idaw.fmt2) * cds->at_idaw;
> > +        if (idaw_addr & 0x07) {
> > +            return -EINVAL; /* channel program check */
> > +        }
> > +        ret = address_space_rw(&address_space_memory, idaw_addr,
> > +                               MEMTXATTRS_UNSPECIFIED, (void *) &idaw.fmt2,
> > +                               sizeof(idaw.fmt2), false);
> > +        cds->cda = be64_to_cpu(idaw.fmt2);
> > +    } else {
> > +        idaw_addr = cds->cda_orig + sizeof(idaw.fmt1) * cds->at_idaw;
> > +        if (idaw_addr & 0x03) {  
> ?:
> (idaw_addr & 0x80000003)

Yes.

> 
> > +            return -EINVAL; /* channel program check */
> > +
> > +        }
> > +        ret = address_space_rw(&address_space_memory, idaw_addr,
> > +                               MEMTXATTRS_UNSPECIFIED, (void *) &idaw.fmt1,
> > +                               sizeof(idaw.fmt1), false);
> > +        cds->cda = be64_to_cpu(idaw.fmt1);
> > +    }
> > +    ++(cds->at_idaw);
> > +    if (ret != MEMTX_OK) {
> > +        /* assume inaccessible address */
> > +        return -EINVAL; /* channel program check */
> > +
> > +    }
> > +    return 0;
> > +}
> > +
> > +static int ccw_dstream_rw_ida(CcwDataStream *cds, void *buff, int len,
> > +                              CcwDataStreamOp op)
> > +{
> > +    uint64_t bsz = ccw_ida_block_size(cds->flags);
> > +    int ret = 0;
> > +    uint16_t cont_left, iter_len;
> > +
> > +    ret = cds_check_len(cds, len);
> > +    if (ret <= 0) {
> > +        return ret;
> > +    }
> > +    if (!cds->at_idaw) {
> > +        /* read first idaw */
> > +        ret = ida_read_next_idaw(cds);
> > +        if (ret) {
> > +            goto err;
> > +        }
> > +        cont_left = ida_continuous_left(cds->cda, bsz);
> > +    } else {
> > +        cont_left = ida_continuous_left(cds->cda, bsz);
> > +        if (cont_left == bsz) {
> > +            ret = ida_read_next_idaw(cds);
> > +            if (ret) {
> > +                goto err;
> > +            }
> > +            if (cds->cda & (bsz - 1)) {  
> Could move this check into ida_read_next_idaw?

I'd like to avoid further code movement...

> 
> > +                ret = -EINVAL; /* channel program check */
> > +                goto err;
> > +            }
> > +        }
> > +    }
> > +    do {
> > +        iter_len = MIN(len, cont_left);
> > +        if (op != CDS_OP_A) {
> > +            ret = address_space_rw(&address_space_memory, cds->cda,
> > +                                   MEMTXATTRS_UNSPECIFIED, buff, iter_len, op);  
> Ahh, now I recall that explictly defining CDS_OP_R to 0 and CDS_OP_W to
> 1 in 'struct CcwDataStreamOp' do have a meaning. Does it make sense to
> make it more obvious by adding some comment there?

Would you have a good text for that?

> 
> > +            if (ret != MEMTX_OK) {
> > +                /* assume inaccessible address */
> > +                ret = -EINVAL; /* channel program check */
> > +                goto err;
> > +            }
> > +        }
> > +        cds->at_byte += iter_len;
> > +        cds->cda += iter_len;
> > +        len -= iter_len;
> > +        if (!len) {
> > +            break;
> > +        }
> > +        ret = ida_read_next_idaw(cds);
> > +        if (ret) {
> > +            goto err;
> > +        }
> > +        cont_left = bsz;
> > +    } while (true);
> > +    return ret;
> > +err:
> > +    cds->flags |= CDS_F_STREAM_BROKEN;
> > +    return ret;
> > +}
> > +
> >  void ccw_dstream_init(CcwDataStream *cds, CCW1 const *ccw, ORB const *orb)
> >  {
> >      /*
> > @@ -835,7 +942,7 @@ void ccw_dstream_init(CcwDataStream *cds, CCW1 const *ccw, ORB const *orb)
> >      if (!(cds->flags & CDS_F_IDA)) {
> >          cds->op_handler = ccw_dstream_rw_noflags;
> >      } else {
> > -        assert(false);
> > +        cds->op_handler = ccw_dstream_rw_ida;
> >      }
> >  }
> > 
> > -- 
> > 2.13.5
> >   
> 
> Generally, the logic looks fine to me.
> 

It did pass Halil's test; but that can only test fmt-2 + 4k blocks, as
this is what the kernel infrastructure provides.

Halil, do you have some more comments?

Re: [Qemu-devel] [PATCH v2 1/4] s390x/css: introduce css data stream

[Cornelia Huck]

On Tue, 19 Sep 2017 11:11:27 +0200
Pierre Morel <pmorel@linux.vnet.ibm.com> wrote:

> On 13/09/2017 13:50, Halil Pasic wrote:
> > This is a preparation for introducing handling for indirect data
> > addressing and modified indirect data addressing (CCW). Here we introduce
> > an interface which should make the addressing scheme transparent for the
> > client code. Here we implement only the basic scheme (no IDA or MIDA).
> > 
> > Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
> > ---
> >   hw/s390x/css.c         | 55 +++++++++++++++++++++++++++++++++++++++++
> >   include/hw/s390x/css.h | 67 ++++++++++++++++++++++++++++++++++++++++++++++++++
> >   2 files changed, 122 insertions(+)

> > diff --git a/include/hw/s390x/css.h b/include/hw/s390x/css.h
> > index 0653d3c9be..79acaf99b7 100644
> > --- a/include/hw/s390x/css.h
> > +++ b/include/hw/s390x/css.h
> > @@ -75,6 +75,29 @@ typedef struct CMBE {
> >       uint32_t reserved[7];
> >   } QEMU_PACKED CMBE;
> > 
> > +typedef enum CcwDataStreamOp {
> > +    CDS_OP_R = 0,
> > +    CDS_OP_W = 1,
> > +    CDS_OP_A = 2
> > +} CcwDataStreamOp;
> > +
> > +/** normal usage is via SuchchDev.cds instead of instantiating */
> > +typedef struct CcwDataStream {
> > +#define CDS_F_IDA   0x01
> > +#define CDS_F_MIDA  0x02
> > +#define CDS_F_I2K   0x04
> > +#define CDS_F_C64   0x08
> > +#define CDS_F_STREAM_BROKEN  0x80
> > +    uint8_t flags;
> > +    uint8_t at_idaw;
> > +    uint16_t at_byte;
> > +    uint16_t count;
> > +    uint32_t cda_orig;
> > +    int (*op_handler)(struct CcwDataStream *cds, void *buff, int len,
> > +                      CcwDataStreamOp op);  
> 
> I would have prefer one handler per operation instead of a operation 
> parameter.
> 
> Is it possible to change the name of the "buf" argument to "arg".
> I just think of the foollowing:
> If one day we do not want to gather all IDAs into a single buffer but 
> keep them split, we can have something like an array of buffers as argument.

It should be possible to change the internal format should we decide to
want to do something different, so I'll leave that as-is for now.

> 
> 
> 
> > +    hwaddr cda;
> > +} CcwDataStream;
> > +

> otherwise, LGTM

Is that an ack? :)

Re: [Qemu-devel] [PATCH v2 4/4] s390x/css: support ccw IDA

[Halil Pasic]

On 09/19/2017 11:48 AM, Cornelia Huck wrote:
> On Tue, 19 Sep 2017 13:50:05 +0800
> Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com> wrote:
> 
>> * Halil Pasic <pasic@linux.vnet.ibm.com> [2017-09-13 13:50:29 +0200]:
>>
>>> Let's add indirect data addressing support for our virtual channel
>>> subsystem. This implementation does no bother with any kind of
>>> prefetching. We simply step trough the IDAL on demand.
>>>
>>> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
>>> ---
>>>  hw/s390x/css.c | 109 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>>>  1 file changed, 108 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/hw/s390x/css.c b/hw/s390x/css.c
>>> index 6b0cd8861b..e34b2af4eb 100644
>>> --- a/hw/s390x/css.c
>>> +++ b/hw/s390x/css.c
>>> @@ -819,6 +819,113 @@ incr:
>>>      return 0;
>>>  }
>>>
>>> +/* returns values between 1 and bsz, where bs is a power of 2 */
>>> +static inline uint16_t ida_continuous_left(hwaddr cda, uint64_t bsz)
>>> +{
>>> +    return bsz - (cda & (bsz - 1));
>>> +}
>>> +
>>> +static inline uint64_t ccw_ida_block_size(uint8_t flags)
>>> +{
>>> +    return 1ULL << (((flags ^ CDS_F_C64) & (CDS_F_C64 | CDS_F_I2K)) ? 11 : 12);  
>> If CDS_F_C64 is set, (flags ^ CDS_F_C64) will be 0, so (1ULL << 11) will
>> be the result regardless the I2K flag? The logic seems wrong.

No. If CDS_F_C64 is set then the outcome depends on the fact if
CDS_F_I2K is set or not.
(flags & CDS_F_IK) => ((flags ^ CDS_F_C64) & CDS_F_IK)
"(flags ^ CDS_F_C64) will be 0" is wrong. flags ^ CDS_F_C64
just flips the CDS_F_C64.

OTOH if the CDS_F_C64 was not set we have the corresponding
bit set in flags ^ CDS_F_C64 so then the  CDS_F_I2K bit does
not matter: we have 1ULL << 11.

In my reading the logic is good.

> 
> I've stared at that condition now for a bit, but all it managed was to
> get me more confused... probably just need a break.
> 
>>
>> I2K is meaningful only when C64 is 1, otherwise it is ignored. The logic
>> here should be:
>> if ((flags & CDS_F_C64) && !(flags & CDS_F_I2K)) {
>>     return 1ULL << 12;
>> }
>>     return 1ULL << 11;
> 
> But I do think your version is more readable...
> 

I won't argue with this.

>>
>>> +}
>>> +
>>> +static inline int ida_read_next_idaw(CcwDataStream *cds)
>>> +{
>>> +    union {uint64_t fmt2; uint32_t fmt1; } idaw;  
>>                                            ^
>> Nit.
>>

Maybe checkpatch wanted it this way. My memories are blurry.

>>> +    bool is_fmt2 = cds->flags & CDS_F_C64;
>>> +    int ret;
>>> +    hwaddr idaw_addr;
>>> +
>>> +    if (is_fmt2) {
>>> +        idaw_addr = cds->cda_orig + sizeof(idaw.fmt2) * cds->at_idaw;
>>> +        if (idaw_addr & 0x07) {
>>> +            return -EINVAL; /* channel program check */
>>> +        }
>>> +        ret = address_space_rw(&address_space_memory, idaw_addr,
>>> +                               MEMTXATTRS_UNSPECIFIED, (void *) &idaw.fmt2,
>>> +                               sizeof(idaw.fmt2), false);
>>> +        cds->cda = be64_to_cpu(idaw.fmt2);
>>> +    } else {
>>> +        idaw_addr = cds->cda_orig + sizeof(idaw.fmt1) * cds->at_idaw;
>>> +        if (idaw_addr & 0x03) {  
>> ?:
>> (idaw_addr & 0x80000003)
> 
> Yes.
> 

I will double check this. Does not seem unreasonable but
double-checking is better.

>>
>>> +            return -EINVAL; /* channel program check */
>>> +
>>> +        }
>>> +        ret = address_space_rw(&address_space_memory, idaw_addr,
>>> +                               MEMTXATTRS_UNSPECIFIED, (void *) &idaw.fmt1,
>>> +                               sizeof(idaw.fmt1), false);
>>> +        cds->cda = be64_to_cpu(idaw.fmt1);
>>> +    }
>>> +    ++(cds->at_idaw);
>>> +    if (ret != MEMTX_OK) {
>>> +        /* assume inaccessible address */
>>> +        return -EINVAL; /* channel program check */
>>> +
>>> +    }
>>> +    return 0;
>>> +}
>>> +
>>> +static int ccw_dstream_rw_ida(CcwDataStream *cds, void *buff, int len,
>>> +                              CcwDataStreamOp op)
>>> +{
>>> +    uint64_t bsz = ccw_ida_block_size(cds->flags);
>>> +    int ret = 0;
>>> +    uint16_t cont_left, iter_len;
>>> +
>>> +    ret = cds_check_len(cds, len);
>>> +    if (ret <= 0) {
>>> +        return ret;
>>> +    }
>>> +    if (!cds->at_idaw) {
>>> +        /* read first idaw */
>>> +        ret = ida_read_next_idaw(cds);
>>> +        if (ret) {
>>> +            goto err;
>>> +        }
>>> +        cont_left = ida_continuous_left(cds->cda, bsz);
>>> +    } else {
>>> +        cont_left = ida_continuous_left(cds->cda, bsz);
>>> +        if (cont_left == bsz) {
>>> +            ret = ida_read_next_idaw(cds);
>>> +            if (ret) {
>>> +                goto err;
>>> +            }
>>> +            if (cds->cda & (bsz - 1)) {  
>> Could move this check into ida_read_next_idaw?
> 
> I'd like to avoid further code movement...
> 

The first idaw is special. I don't think moving is possible.

>>
>>> +                ret = -EINVAL; /* channel program check */
>>> +                goto err;
>>> +            }
>>> +        }
>>> +    }
>>> +    do {
>>> +        iter_len = MIN(len, cont_left);
>>> +        if (op != CDS_OP_A) {
>>> +            ret = address_space_rw(&address_space_memory, cds->cda,
>>> +                                   MEMTXATTRS_UNSPECIFIED, buff, iter_len, op);  
>> Ahh, now I recall that explictly defining CDS_OP_R to 0 and CDS_OP_W to
>> 1 in 'struct CcwDataStreamOp' do have a meaning. Does it make sense to
>> make it more obvious by adding some comment there?
> 
> Would you have a good text for that?
> 

I'm fine with clarifications.

>>
>>> +            if (ret != MEMTX_OK) {
>>> +                /* assume inaccessible address */
>>> +                ret = -EINVAL; /* channel program check */
>>> +                goto err;
>>> +            }
>>> +        }
>>> +        cds->at_byte += iter_len;
>>> +        cds->cda += iter_len;
>>> +        len -= iter_len;
>>> +        if (!len) {
>>> +            break;
>>> +        }
>>> +        ret = ida_read_next_idaw(cds);
>>> +        if (ret) {
>>> +            goto err;
>>> +        }
>>> +        cont_left = bsz;
>>> +    } while (true);
>>> +    return ret;
>>> +err:
>>> +    cds->flags |= CDS_F_STREAM_BROKEN;
>>> +    return ret;
>>> +}
>>> +
>>>  void ccw_dstream_init(CcwDataStream *cds, CCW1 const *ccw, ORB const *orb)
>>>  {
>>>      /*
>>> @@ -835,7 +942,7 @@ void ccw_dstream_init(CcwDataStream *cds, CCW1 const *ccw, ORB const *orb)
>>>      if (!(cds->flags & CDS_F_IDA)) {
>>>          cds->op_handler = ccw_dstream_rw_noflags;
>>>      } else {
>>> -        assert(false);
>>> +        cds->op_handler = ccw_dstream_rw_ida;
>>>      }
>>>  }
>>>
>>> -- 
>>> 2.13.5
>>>   
>>
>> Generally, the logic looks fine to me.
>>
> 
> It did pass Halil's test; but that can only test fmt-2 + 4k blocks, as
> this is what the kernel infrastructure provides.

Nod.

> 
> Halil, do you have some more comments?
> 

Just a question. Do I have to respin?

Halil

Re: [Qemu-devel] [PATCH v2 4/4] s390x/css: support ccw IDA

[Cornelia Huck]

On Tue, 19 Sep 2017 12:36:33 +0200
Halil Pasic <pasic@linux.vnet.ibm.com> wrote:

> On 09/19/2017 11:48 AM, Cornelia Huck wrote:
> > On Tue, 19 Sep 2017 13:50:05 +0800
> > Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com> wrote:
> >   
> >> * Halil Pasic <pasic@linux.vnet.ibm.com> [2017-09-13 13:50:29 +0200]:
> >>  
> >>> Let's add indirect data addressing support for our virtual channel
> >>> subsystem. This implementation does no bother with any kind of
> >>> prefetching. We simply step trough the IDAL on demand.
> >>>
> >>> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
> >>> ---
> >>>  hw/s390x/css.c | 109 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
> >>>  1 file changed, 108 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/hw/s390x/css.c b/hw/s390x/css.c
> >>> index 6b0cd8861b..e34b2af4eb 100644
> >>> --- a/hw/s390x/css.c
> >>> +++ b/hw/s390x/css.c
> >>> @@ -819,6 +819,113 @@ incr:
> >>>      return 0;
> >>>  }
> >>>
> >>> +/* returns values between 1 and bsz, where bs is a power of 2 */
> >>> +static inline uint16_t ida_continuous_left(hwaddr cda, uint64_t bsz)
> >>> +{
> >>> +    return bsz - (cda & (bsz - 1));
> >>> +}
> >>> +
> >>> +static inline uint64_t ccw_ida_block_size(uint8_t flags)
> >>> +{
> >>> +    return 1ULL << (((flags ^ CDS_F_C64) & (CDS_F_C64 | CDS_F_I2K)) ? 11 : 12);    
> >> If CDS_F_C64 is set, (flags ^ CDS_F_C64) will be 0, so (1ULL << 11) will
> >> be the result regardless the I2K flag? The logic seems wrong.  
> 
> No. If CDS_F_C64 is set then the outcome depends on the fact if
> CDS_F_I2K is set or not.
> (flags & CDS_F_IK) => ((flags ^ CDS_F_C64) & CDS_F_IK)
> "(flags ^ CDS_F_C64) will be 0" is wrong. flags ^ CDS_F_C64
> just flips the CDS_F_C64.
> 
> OTOH if the CDS_F_C64 was not set we have the corresponding
> bit set in flags ^ CDS_F_C64 so then the  CDS_F_I2K bit does
> not matter: we have 1ULL << 11.
> 
> In my reading the logic is good.

So I'll just leave it...

> 
> > 
> > I've stared at that condition now for a bit, but all it managed was to
> > get me more confused... probably just need a break.
> >   
> >>
> >> I2K is meaningful only when C64 is 1, otherwise it is ignored. The logic
> >> here should be:
> >> if ((flags & CDS_F_C64) && !(flags & CDS_F_I2K)) {
> >>     return 1ULL << 12;
> >> }
> >>     return 1ULL << 11;  
> > 
> > But I do think your version is more readable...
> >   
> 
> I won't argue with this.

...and we could change that in a patch on top to avoid future confusion.

> 
> >>  
> >>> +}
> >>> +
> >>> +static inline int ida_read_next_idaw(CcwDataStream *cds)
> >>> +{
> >>> +    union {uint64_t fmt2; uint32_t fmt1; } idaw;    
> >>                                            ^
> >> Nit.
> >>  
> 
> Maybe checkpatch wanted it this way. My memories are blurry.


I'd just leave it like that, tbh.

> 
> >>> +    bool is_fmt2 = cds->flags & CDS_F_C64;
> >>> +    int ret;
> >>> +    hwaddr idaw_addr;
> >>> +
> >>> +    if (is_fmt2) {
> >>> +        idaw_addr = cds->cda_orig + sizeof(idaw.fmt2) * cds->at_idaw;
> >>> +        if (idaw_addr & 0x07) {
> >>> +            return -EINVAL; /* channel program check */
> >>> +        }
> >>> +        ret = address_space_rw(&address_space_memory, idaw_addr,
> >>> +                               MEMTXATTRS_UNSPECIFIED, (void *) &idaw.fmt2,
> >>> +                               sizeof(idaw.fmt2), false);
> >>> +        cds->cda = be64_to_cpu(idaw.fmt2);
> >>> +    } else {
> >>> +        idaw_addr = cds->cda_orig + sizeof(idaw.fmt1) * cds->at_idaw;
> >>> +        if (idaw_addr & 0x03) {    
> >> ?:
> >> (idaw_addr & 0x80000003)  
> > 
> > Yes.
> >   
> 
> I will double check this. Does not seem unreasonable but
> double-checking is better.

Please let me know. I think the architecture says that the bit must be
zero, and that we may (...) generate a channel program check.

> 
> >>  
> >>> +            return -EINVAL; /* channel program check */
> >>> +
> >>> +        }
> >>> +        ret = address_space_rw(&address_space_memory, idaw_addr,
> >>> +                               MEMTXATTRS_UNSPECIFIED, (void *) &idaw.fmt1,
> >>> +                               sizeof(idaw.fmt1), false);
> >>> +        cds->cda = be64_to_cpu(idaw.fmt1);
> >>> +    }
> >>> +    ++(cds->at_idaw);
> >>> +    if (ret != MEMTX_OK) {
> >>> +        /* assume inaccessible address */
> >>> +        return -EINVAL; /* channel program check */
> >>> +
> >>> +    }
> >>> +    return 0;
> >>> +}
> >>> +
> >>> +static int ccw_dstream_rw_ida(CcwDataStream *cds, void *buff, int len,
> >>> +                              CcwDataStreamOp op)
> >>> +{
> >>> +    uint64_t bsz = ccw_ida_block_size(cds->flags);
> >>> +    int ret = 0;
> >>> +    uint16_t cont_left, iter_len;
> >>> +
> >>> +    ret = cds_check_len(cds, len);
> >>> +    if (ret <= 0) {
> >>> +        return ret;
> >>> +    }
> >>> +    if (!cds->at_idaw) {
> >>> +        /* read first idaw */
> >>> +        ret = ida_read_next_idaw(cds);
> >>> +        if (ret) {
> >>> +            goto err;
> >>> +        }
> >>> +        cont_left = ida_continuous_left(cds->cda, bsz);
> >>> +    } else {
> >>> +        cont_left = ida_continuous_left(cds->cda, bsz);
> >>> +        if (cont_left == bsz) {
> >>> +            ret = ida_read_next_idaw(cds);
> >>> +            if (ret) {
> >>> +                goto err;
> >>> +            }
> >>> +            if (cds->cda & (bsz - 1)) {    
> >> Could move this check into ida_read_next_idaw?  
> > 
> > I'd like to avoid further code movement...
> >   
> 
> The first idaw is special. I don't think moving is possible.

So, the code is correct and I'll just leave it like that.

> 
> >>  
> >>> +                ret = -EINVAL; /* channel program check */
> >>> +                goto err;
> >>> +            }
> >>> +        }
> >>> +    }
> >>> +    do {
> >>> +        iter_len = MIN(len, cont_left);
> >>> +        if (op != CDS_OP_A) {
> >>> +            ret = address_space_rw(&address_space_memory, cds->cda,
> >>> +                                   MEMTXATTRS_UNSPECIFIED, buff, iter_len, op);    
> >> Ahh, now I recall that explictly defining CDS_OP_R to 0 and CDS_OP_W to
> >> 1 in 'struct CcwDataStreamOp' do have a meaning. Does it make sense to
> >> make it more obvious by adding some comment there?  
> > 
> > Would you have a good text for that?
> >   
> 
> I'm fine with clarifications.

Let's do it as a patch on top.

> 
> >>  
> >>> +            if (ret != MEMTX_OK) {
> >>> +                /* assume inaccessible address */
> >>> +                ret = -EINVAL; /* channel program check */
> >>> +                goto err;
> >>> +            }
> >>> +        }
> >>> +        cds->at_byte += iter_len;
> >>> +        cds->cda += iter_len;
> >>> +        len -= iter_len;
> >>> +        if (!len) {
> >>> +            break;
> >>> +        }
> >>> +        ret = ida_read_next_idaw(cds);
> >>> +        if (ret) {
> >>> +            goto err;
> >>> +        }
> >>> +        cont_left = bsz;
> >>> +    } while (true);
> >>> +    return ret;
> >>> +err:
> >>> +    cds->flags |= CDS_F_STREAM_BROKEN;
> >>> +    return ret;
> >>> +}
> >>> +
> >>>  void ccw_dstream_init(CcwDataStream *cds, CCW1 const *ccw, ORB const *orb)
> >>>  {
> >>>      /*
> >>> @@ -835,7 +942,7 @@ void ccw_dstream_init(CcwDataStream *cds, CCW1 const *ccw, ORB const *orb)
> >>>      if (!(cds->flags & CDS_F_IDA)) {
> >>>          cds->op_handler = ccw_dstream_rw_noflags;
> >>>      } else {
> >>> -        assert(false);
> >>> +        cds->op_handler = ccw_dstream_rw_ida;
> >>>      }
> >>>  }
> >>>
> >>> -- 
> >>> 2.13.5
> >>>     
> >>
> >> Generally, the logic looks fine to me.
> >>  
> > 
> > It did pass Halil's test; but that can only test fmt-2 + 4k blocks, as
> > this is what the kernel infrastructure provides.  
> 
> Nod.
> 
> > 
> > Halil, do you have some more comments?
> >   
> 
> Just a question. Do I have to respin?

I don't think so. If you could confirm the check for format-1, I'll
just fixup that one and get the queued patches out of the door.

We can do more changes on top; it's not like I don't have more patches
waiting...

Re: [Qemu-devel] [PATCH v2 1/4] s390x/css: introduce css data stream

[Pierre Morel]

On 19/09/2017 11:53, Cornelia Huck wrote:
> On Tue, 19 Sep 2017 11:11:27 +0200
> Pierre Morel <pmorel@linux.vnet.ibm.com> wrote:
> 
>> On 13/09/2017 13:50, Halil Pasic wrote:
>>> This is a preparation for introducing handling for indirect data
>>> addressing and modified indirect data addressing (CCW). Here we introduce
>>> an interface which should make the addressing scheme transparent for the
>>> client code. Here we implement only the basic scheme (no IDA or MIDA).
>>>
>>> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
>>> ---
>>>    hw/s390x/css.c         | 55 +++++++++++++++++++++++++++++++++++++++++
>>>    include/hw/s390x/css.h | 67 ++++++++++++++++++++++++++++++++++++++++++++++++++
>>>    2 files changed, 122 insertions(+)
> 
>>> diff --git a/include/hw/s390x/css.h b/include/hw/s390x/css.h
>>> index 0653d3c9be..79acaf99b7 100644
>>> --- a/include/hw/s390x/css.h
>>> +++ b/include/hw/s390x/css.h
>>> @@ -75,6 +75,29 @@ typedef struct CMBE {
>>>        uint32_t reserved[7];
>>>    } QEMU_PACKED CMBE;
>>>
>>> +typedef enum CcwDataStreamOp {
>>> +    CDS_OP_R = 0,
>>> +    CDS_OP_W = 1,
>>> +    CDS_OP_A = 2
>>> +} CcwDataStreamOp;
>>> +
>>> +/** normal usage is via SuchchDev.cds instead of instantiating */
>>> +typedef struct CcwDataStream {
>>> +#define CDS_F_IDA   0x01
>>> +#define CDS_F_MIDA  0x02
>>> +#define CDS_F_I2K   0x04
>>> +#define CDS_F_C64   0x08
>>> +#define CDS_F_STREAM_BROKEN  0x80
>>> +    uint8_t flags;
>>> +    uint8_t at_idaw;
>>> +    uint16_t at_byte;
>>> +    uint16_t count;
>>> +    uint32_t cda_orig;
>>> +    int (*op_handler)(struct CcwDataStream *cds, void *buff, int len,
>>> +                      CcwDataStreamOp op);
>>
>> I would have prefer one handler per operation instead of a operation
>> parameter.
>>
>> Is it possible to change the name of the "buf" argument to "arg".
>> I just think of the foollowing:
>> If one day we do not want to gather all IDAs into a single buffer but
>> keep them split, we can have something like an array of buffers as argument.
> 
> It should be possible to change the internal format should we decide to
> want to do something different, so I'll leave that as-is for now.
> 
>>
>>
>>
>>> +    hwaddr cda;
>>> +} CcwDataStream;
>>> +
> 
>> otherwise, LGTM
> 
> Is that an ack? :)
> 

Sorry, yes it is.
I play a little with the code before giving a RB
Should be end of the day.


-- 
Pierre Morel
Linux/KVM/QEMU in Böblingen - Germany

Re: [Qemu-devel] [PATCH v2 4/4] s390x/css: support ccw IDA

[Halil Pasic]

On 09/19/2017 12:57 PM, Cornelia Huck wrote:
>>>>> +static inline int ida_read_next_idaw(CcwDataStream *cds)
>>>>> +{
>>>>> +    union {uint64_t fmt2; uint32_t fmt1; } idaw;    
>>>>                                            ^
>>>> Nit.
>>>>  
>> Maybe checkpatch wanted it this way. My memories are blurry.
> 
> I'd just leave it like that, tbh.
> 
>>>>> +    bool is_fmt2 = cds->flags & CDS_F_C64;
>>>>> +    int ret;
>>>>> +    hwaddr idaw_addr;
>>>>> +
>>>>> +    if (is_fmt2) {
>>>>> +        idaw_addr = cds->cda_orig + sizeof(idaw.fmt2) * cds->at_idaw;
>>>>> +        if (idaw_addr & 0x07) {
>>>>> +            return -EINVAL; /* channel program check */
>>>>> +        }
>>>>> +        ret = address_space_rw(&address_space_memory, idaw_addr,
>>>>> +                               MEMTXATTRS_UNSPECIFIED, (void *) &idaw.fmt2,
>>>>> +                               sizeof(idaw.fmt2), false);
>>>>> +        cds->cda = be64_to_cpu(idaw.fmt2);


>>>>> +    } else {
>>>>> +        idaw_addr = cds->cda_orig + sizeof(idaw.fmt1) * cds->at_idaw;
>>>>> +        if (idaw_addr & 0x03) {    
>>>> ?:
>>>> (idaw_addr & 0x80000003)  
>>> Yes.
>>>   
>> I will double check this. Does not seem unreasonable but
>> double-checking is better.
> Please let me know. I think the architecture says that the bit must be
> zero, and that we may (...) generate a channel program check.
> 

Not exactly. The more significant bits part of the check
depend on the ccw format. This needs to be done for both
idaw formats. I would need to introduce a new flag, or
access the SubchDev to do this properly.

Architecturally we also need to check the data addresses
from which we read so we have nothing bigger than 
(1 << 31) - 1 if we are working with format-1 idaws.

I also think we did not take proper care of proper
maximum data address checks prior CwwDataStream which also
depend on the ccw format (in absence of IDAW or MIDAW).

The ccw format dependent maximum address checks are (1 << 24) - 1
and (1 << 31) - 1 respectively for format-0 and format-1 (on
the first indirection level that is for non-IDA for the data,
and for (M)IDA for the (M)IDAWs).

Reference:
PoP pages 16-25 and 16-26 "Invalid IDAW or MIDAW Addre" and
"Invalid Data Address".

How shall we proceed?

Halil

>>>>  
>>>>> +            return -EINVAL; /* channel program check */
>>>>> +
>>>>> +        }
>>>>> +        ret = address_space_rw(&address_space_memory, idaw_addr,
>>>>> +                               MEMTXATTRS_UNSPECIFIED, (void *) &idaw.fmt1,
>>>>> +                               sizeof(idaw.fmt1), false);
>>>>> +        cds->cda = be64_to_cpu(idaw.fmt1);>>>>> +    }
>>>>> +    ++(cds->at_idaw);
>>>>> +    if (ret != MEMTX_OK) {
>>>>> +        /* assume inaccessible address */
>>>>> +        return -EINVAL; /* channel program check */
>>>>> +
>>>>> +    }
>>>>> +    return 0;
>>>>> +}

Re: [Qemu-devel] [PATCH v2 1/4] s390x/css: introduce css data stream

[Halil Pasic]

On 09/19/2017 01:41 PM, Pierre Morel wrote:
> On 19/09/2017 11:53, Cornelia Huck wrote:
>> On Tue, 19 Sep 2017 11:11:27 +0200
>> Pierre Morel <pmorel@linux.vnet.ibm.com> wrote:
>>
>>> On 13/09/2017 13:50, Halil Pasic wrote:
>>>> This is a preparation for introducing handling for indirect data
>>>> addressing and modified indirect data addressing (CCW). Here we introduce
>>>> an interface which should make the addressing scheme transparent for the
>>>> client code. Here we implement only the basic scheme (no IDA or MIDA).
>>>>
>>>> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
>>>> ---
>>>>    hw/s390x/css.c         | 55 +++++++++++++++++++++++++++++++++++++++++
>>>>    include/hw/s390x/css.h | 67 ++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>    2 files changed, 122 insertions(+)
>>
>>>> diff --git a/include/hw/s390x/css.h b/include/hw/s390x/css.h
>>>> index 0653d3c9be..79acaf99b7 100644
>>>> --- a/include/hw/s390x/css.h
>>>> +++ b/include/hw/s390x/css.h
>>>> @@ -75,6 +75,29 @@ typedef struct CMBE {
>>>>        uint32_t reserved[7];
>>>>    } QEMU_PACKED CMBE;
>>>>
>>>> +typedef enum CcwDataStreamOp {
>>>> +    CDS_OP_R = 0,
>>>> +    CDS_OP_W = 1,
>>>> +    CDS_OP_A = 2
>>>> +} CcwDataStreamOp;
>>>> +
>>>> +/** normal usage is via SuchchDev.cds instead of instantiating */
>>>> +typedef struct CcwDataStream {
>>>> +#define CDS_F_IDA   0x01
>>>> +#define CDS_F_MIDA  0x02
>>>> +#define CDS_F_I2K   0x04
>>>> +#define CDS_F_C64   0x08
>>>> +#define CDS_F_STREAM_BROKEN  0x80
>>>> +    uint8_t flags;
>>>> +    uint8_t at_idaw;
>>>> +    uint16_t at_byte;
>>>> +    uint16_t count;
>>>> +    uint32_t cda_orig;
>>>> +    int (*op_handler)(struct CcwDataStream *cds, void *buff, int len,
>>>> +                      CcwDataStreamOp op);
>>>
>>> I would have prefer one handler per operation instead of a operation
>>> parameter.
>>>
>>> Is it possible to change the name of the "buf" argument to "arg".
>>> I just think of the foollowing:
>>> If one day we do not want to gather all IDAs into a single buffer but
>>> keep them split, we can have something like an array of buffers as argument.
>>

I had a similar idea: basically indirect data access is kind of a scatter-
gather mechanism. If one just has to mediate the data (e.g. between some
backed taking sg or some facility provided by the kernel) creating a copy
of the data (what CcwDataStream currently does) is not necessarily optimal.

I've realized that while looking at the 3270 code.

So some kind of a turn a CcwDataStream into a scatter-gather list may
come along the road. But I would like to have an use-case for that first.

>> It should be possible to change the internal format should we decide to
>> want to do something different, so I'll leave that as-is for now.
>>

Fully agree! This is internal stuff we have under full control. We can
decide any time to do something completely different looking (if we are
willing to change all the client code). So I think taking out the crystal orb
(pun intended) ain't necessary (or very productive).

>>>
>>>
>>>
>>>> +    hwaddr cda;
>>>> +} CcwDataStream;
>>>> +
>>
>>> otherwise, LGTM
>>
>> Is that an ack? :)
>>
> 
> Sorry, yes it is.
> I play a little with the code before giving a RB
> Should be end of the day.
> 
> 

Re: [Qemu-devel] [PATCH v2 4/4] s390x/css: support ccw IDA

[Cornelia Huck]

On Tue, 19 Sep 2017 14:04:03 +0200
Halil Pasic <pasic@linux.vnet.ibm.com> wrote:

> On 09/19/2017 12:57 PM, Cornelia Huck wrote:
> >>>>> +static inline int ida_read_next_idaw(CcwDataStream *cds)
> >>>>> +{
> >>>>> +    union {uint64_t fmt2; uint32_t fmt1; } idaw;      
> >>>>                                            ^
> >>>> Nit.
> >>>>    
> >> Maybe checkpatch wanted it this way. My memories are blurry.  
> > 
> > I'd just leave it like that, tbh.
> >   
> >>>>> +    bool is_fmt2 = cds->flags & CDS_F_C64;
> >>>>> +    int ret;
> >>>>> +    hwaddr idaw_addr;
> >>>>> +
> >>>>> +    if (is_fmt2) {
> >>>>> +        idaw_addr = cds->cda_orig + sizeof(idaw.fmt2) * cds->at_idaw;
> >>>>> +        if (idaw_addr & 0x07) {
> >>>>> +            return -EINVAL; /* channel program check */
> >>>>> +        }
> >>>>> +        ret = address_space_rw(&address_space_memory, idaw_addr,
> >>>>> +                               MEMTXATTRS_UNSPECIFIED, (void *) &idaw.fmt2,
> >>>>> +                               sizeof(idaw.fmt2), false);
> >>>>> +        cds->cda = be64_to_cpu(idaw.fmt2);  
> 
> 
> >>>>> +    } else {
> >>>>> +        idaw_addr = cds->cda_orig + sizeof(idaw.fmt1) * cds->at_idaw;
> >>>>> +        if (idaw_addr & 0x03) {      
> >>>> ?:
> >>>> (idaw_addr & 0x80000003)    
> >>> Yes.
> >>>     
> >> I will double check this. Does not seem unreasonable but
> >> double-checking is better.  
> > Please let me know. I think the architecture says that the bit must be
> > zero, and that we may (...) generate a channel program check.
> >   
> 
> Not exactly. The more significant bits part of the check
> depend on the ccw format. This needs to be done for both
> idaw formats. I would need to introduce a new flag, or
> access the SubchDev to do this properly.
> 
> Architecturally we also need to check the data addresses
> from which we read so we have nothing bigger than 
> (1 << 31) - 1 if we are working with format-1 idaws.
> 
> I also think we did not take proper care of proper
> maximum data address checks prior CwwDataStream which also
> depend on the ccw format (in absence of IDAW or MIDAW).
> 
> The ccw format dependent maximum address checks are (1 << 24) - 1
> and (1 << 31) - 1 respectively for format-0 and format-1 (on
> the first indirection level that is for non-IDA for the data,
> and for (M)IDA for the (M)IDAWs).
> 
> Reference:
> PoP pages 16-25 and 16-26 "Invalid IDAW or MIDAW Addre" and
> "Invalid Data Address".

Sigh, when are things ever easy :(

I'll need some time to review this. But more urgently, I need a break.

> 
> How shall we proceed?

Given the discussion about this patch in particular, I'll probably
dequeue it and send it with the next batch of patches. We can also
include the other improvements, then. Not sure whether I should just
dequeue this one or the whole series (I really want to get the rest of
the patches out...)

More after the break :)

Re: [Qemu-devel] [PATCH v2 4/4] s390x/css: support ccw IDA

[Halil Pasic]

On 09/19/2017 02:23 PM, Cornelia Huck wrote:
> On Tue, 19 Sep 2017 14:04:03 +0200
> Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
> 
>> On 09/19/2017 12:57 PM, Cornelia Huck wrote:
>>>>>>> +static inline int ida_read_next_idaw(CcwDataStream *cds)
>>>>>>> +{
>>>>>>> +    union {uint64_t fmt2; uint32_t fmt1; } idaw;      
>>>>>>                                            ^
>>>>>> Nit.
>>>>>>    
>>>> Maybe checkpatch wanted it this way. My memories are blurry.  
>>>
>>> I'd just leave it like that, tbh.
>>>   
>>>>>>> +    bool is_fmt2 = cds->flags & CDS_F_C64;
>>>>>>> +    int ret;
>>>>>>> +    hwaddr idaw_addr;
>>>>>>> +
>>>>>>> +    if (is_fmt2) {
>>>>>>> +        idaw_addr = cds->cda_orig + sizeof(idaw.fmt2) * cds->at_idaw;
>>>>>>> +        if (idaw_addr & 0x07) {
>>>>>>> +            return -EINVAL; /* channel program check */
>>>>>>> +        }
>>>>>>> +        ret = address_space_rw(&address_space_memory, idaw_addr,
>>>>>>> +                               MEMTXATTRS_UNSPECIFIED, (void *) &idaw.fmt2,
>>>>>>> +                               sizeof(idaw.fmt2), false);
>>>>>>> +        cds->cda = be64_to_cpu(idaw.fmt2);  
>>
>>
>>>>>>> +    } else {
>>>>>>> +        idaw_addr = cds->cda_orig + sizeof(idaw.fmt1) * cds->at_idaw;
>>>>>>> +        if (idaw_addr & 0x03) {      
>>>>>> ?:
>>>>>> (idaw_addr & 0x80000003)    
>>>>> Yes.
>>>>>     
>>>> I will double check this. Does not seem unreasonable but
>>>> double-checking is better.  
>>> Please let me know. I think the architecture says that the bit must be
>>> zero, and that we may (...) generate a channel program check.
>>>   
>>
>> Not exactly. The more significant bits part of the check
>> depend on the ccw format. This needs to be done for both
>> idaw formats. I would need to introduce a new flag, or
>> access the SubchDev to do this properly.
>>
>> Architecturally we also need to check the data addresses
>> from which we read so we have nothing bigger than 
>> (1 << 31) - 1 if we are working with format-1 idaws.
>>
>> I also think we did not take proper care of proper
>> maximum data address checks prior CwwDataStream which also
>> depend on the ccw format (in absence of IDAW or MIDAW).
>>
>> The ccw format dependent maximum address checks are (1 << 24) - 1
>> and (1 << 31) - 1 respectively for format-0 and format-1 (on
>> the first indirection level that is for non-IDA for the data,
>> and for (M)IDA for the (M)IDAWs).
>>
>> Reference:
>> PoP pages 16-25 and 16-26 "Invalid IDAW or MIDAW Addre" and
>> "Invalid Data Address".
> 
> Sigh, when are things ever easy :(
> 
> I'll need some time to review this. But more urgently, I need a break.
> 
>>
>> How shall we proceed?
> 
> Given the discussion about this patch in particular, I'll probably
> dequeue it and send it with the next batch of patches. We can also
> include the other improvements, then. Not sure whether I should just
> dequeue this one or the whole series (I really want to get the rest of
> the patches out...)
> 
> More after the break :)
> 

I think I can fix this pretty fast, and fixing this later is
an option too in my opinion as the patch is consistent with our
prior art (we ignored this maximum address checking requirement,
and we keep ignoring it). 

I would prefer not dequeue-ing the whole series because a
3270 fix of mine (which just made the internal review) depends
on this. IMHO, it's not like the series is fundamentally broken,
not ain't an improvement at all. It is not perfect, but it's
IMHO certainly an improvement over what we have.

Regards,
Halil

Re: [Qemu-devel] [PATCH v2 3/4] virtio-ccw: use ccw data stream

[Halil Pasic]

On 09/19/2017 11:06 AM, Cornelia Huck wrote:
> On Tue, 19 Sep 2017 11:37:30 +0800
> Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com> wrote:
> 
>> * Halil Pasic <pasic@linux.vnet.ibm.com> [2017-09-13 13:50:28 +0200]:
>>
>>> Replace direct access which implicitly assumes no IDA
>>> or MIDA with the new ccw data stream interface which should
>>> cope with these transparently in the future.
>>>
>>> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
>>>
>>> ---
>>>
>>> v1 --> v2:
>>> Removed todo comments on possible errno change as with
>>> https://lists.nongnu.org/archive/html/qemu-devel/2017-09/msg02441.html
>>> applied it does not matter any more.
>>>
>>> Error handling: At the moment we ignore errors reported
>>> by stream ops to keep the change minimal. An add-on
>>> patch improving on this is to be expected later.  
>> Add a TODO somewhere around the code as a reminder?
> 
> I expect Halil to have it on his TODO list and send a patch later ;)
> 
>>
>>> ---
>>>  hw/s390x/virtio-ccw.c | 156 +++++++++++++++-----------------------------------
>>>  1 file changed, 46 insertions(+), 110 deletions(-)
>>>
>>> diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
>>> index b1976fdd19..a9baf6f7ab 100644
>>> --- a/hw/s390x/virtio-ccw.c
>>> +++ b/hw/s390x/virtio-ccw.c  
>> [...]
>>
>>> @@ -394,11 +362,8 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
>>>          } else {
>>>              VirtioDeviceClass *vdc = VIRTIO_DEVICE_GET_CLASS(vdev);
>>>
>>> -            features.index = address_space_ldub(&address_space_memory,
>>> -                                                ccw.cda
>>> -                                                + sizeof(features.features),
>>> -                                                MEMTXATTRS_UNSPECIFIED,
>>> -                                                NULL);
>>> +            ccw_dstream_advance(&sch->cds, sizeof(features.features));  
>> How about:
>> ccw_dstream_advance(&sch->cds, offsetof(VirtioFeatDesc, index));
> 
> I think keeping sizeof(features.features) is better: It matches the old
> code, and we really do jump over the features flag and revisit it
> later...
> 
>>
>>> +            ccw_dstream_read(&sch->cds, features.index);
>>>              if (features.index == 0) {
>>>                  if (dev->revision >= 1) {
>>>                      /* Don't offer legacy features for modern devices. */
>>> @@ -417,9 +382,9 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
>>>                  /* Return zeroes if the guest supports more feature bits. */
>>>                  features.features = 0;
>>>              }
>>> -            address_space_stl_le(&address_space_memory, ccw.cda,
>>> -                                 features.features, MEMTXATTRS_UNSPECIFIED,
>>> -                                 NULL);
>>> +            ccw_dstream_rewind(&sch->cds);
>>> +            cpu_to_le32s(&features.features);
>>> +            ccw_dstream_write(&sch->cds, features.features);
>>>              sch->curr_status.scsw.count = ccw.count - sizeof(features);  
>> How about:
>> sch->curr_status.scsw.count = ccw_dstream_residual_count(&sch->cds);
> 
> Hm, I thought I had commented on this, but I seem to have missed
> these...
> 
> I'd prefer to do it as a follow-up patch.
> 
>>
>> This also applies to other places.
>>
>>>              ret = 0;
>>>          }  
>> [...]
>>
>>> @@ -501,21 +459,13 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
>>>              }
>>>          }
>>>          len = MIN(ccw.count, vdev->config_len);
>>> -        hw_len = len;
>>>          if (!ccw.cda) {
>>>              ret = -EFAULT;
>>>          } else {
>>> -            config = cpu_physical_memory_map(ccw.cda, &hw_len, 0);
>>> -            if (!config) {
>>> -                ret = -EFAULT;
>>> -            } else {
>>> -                len = hw_len;
>>> -                /* XXX config space endianness */
>>> -                memcpy(vdev->config, config, len);
>>> -                cpu_physical_memory_unmap(config, hw_len, 0, hw_len);
>>> +            ret = ccw_dstream_read_buf(&sch->cds, vdev->config, len);
>>> +            if (!ret) {  
>> Add a TODO here, and:
>>
>> if (ccw_dstream_read_buf(&sch->cds, vdev->config, len)) {
>>     ret = -EFAULT;
>> } else {
>>     ....
>> }
> 
> I don't think that would be correct: The function will (at least
> currently) return 0 or -EINVAL, and you are now mapping any error to
> -EFAULT? (Not that it has an effect in the end: We map both to a
> channel program check as of "s390x/css: drop data-check in
> interpretation".)
> 
>>
>>>                  virtio_bus_set_vdev_config(&dev->bus, vdev->config);
>>>                  sch->curr_status.scsw.count = ccw.count - len;
>>> -                ret = 0;
>>>              }
>>>          }
>>>          break;  
>> [...]
>>
>>> @@ -714,13 +654,9 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
>>>              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);
>>> +        ccw_dstream_read_buf(&sch->cds, &revinfo, 4);  
>>                                                      ^
>> A magic number? O:)
> 
> Hah :)
> 
> We can't use sizeof(revinfo), and sizeof(revinfo.revision) +
> sizeof(revinfo.length) is a bit unwieldy. Let's keep some magic in our
> code :)
> 
>>
>>> +        be16_to_cpus(&revinfo.revision);
>>> +        be16_to_cpus(&revinfo.length);
>>>          if (ccw.count < len + revinfo.length ||
>>>              (check_len && ccw.count > len + revinfo.length)) {
>>>              ret = -EINVAL;
>>> -- 
>>> 2.13.5
>>>   
>>
>> Otherwise, looks good.
>>
> 
> Can I get an ack?
> 

I agree that further cleanups are possible (e.g. using
ccw_dstream_residual_count() and tightening error handling) but
I also prefer to see these as on-top, and prefer sticking to
change as little as possible in the transformation patch and stay
as close as possible to what we had before in terms of behavior).

Regards,
Halil 

Re: [Qemu-devel] [PATCH v2 4/4] s390x/css: support ccw IDA

[Pierre Morel]

On 19/09/2017 12:57, Cornelia Huck wrote:
> On Tue, 19 Sep 2017 12:36:33 +0200
> Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
> 
>> On 09/19/2017 11:48 AM, Cornelia Huck wrote:
>>> On Tue, 19 Sep 2017 13:50:05 +0800
>>> Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com> wrote:
>>>    
>>>> * Halil Pasic <pasic@linux.vnet.ibm.com> [2017-09-13 13:50:29 +0200]:
>>>>   
>>>>> Let's add indirect data addressing support for our virtual channel
>>>>> subsystem. This implementation does no bother with any kind of
>>>>> prefetching. We simply step trough the IDAL on demand.
>>>>>
>>>>> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
>>>>> ---
>>>>>   hw/s390x/css.c | 109 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>>>>>   1 file changed, 108 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/hw/s390x/css.c b/hw/s390x/css.c
>>>>> index 6b0cd8861b..e34b2af4eb 100644
>>>>> --- a/hw/s390x/css.c
>>>>> +++ b/hw/s390x/css.c
>>>>> @@ -819,6 +819,113 @@ incr:
>>>>>       return 0;
>>>>>   }
>>>>>
>>>>> +/* returns values between 1 and bsz, where bs is a power of 2 */
>>>>> +static inline uint16_t ida_continuous_left(hwaddr cda, uint64_t bsz)
>>>>> +{
>>>>> +    return bsz - (cda & (bsz - 1));
>>>>> +}
>>>>> +
>>>>> +static inline uint64_t ccw_ida_block_size(uint8_t flags)
>>>>> +{
>>>>> +    return 1ULL << (((flags ^ CDS_F_C64) & (CDS_F_C64 | CDS_F_I2K)) ? 11 : 12);
>>>> If CDS_F_C64 is set, (flags ^ CDS_F_C64) will be 0, so (1ULL << 11) will
>>>> be the result regardless the I2K flag? The logic seems wrong.
>>
>> No. If CDS_F_C64 is set then the outcome depends on the fact if
>> CDS_F_I2K is set or not.
>> (flags & CDS_F_IK) => ((flags ^ CDS_F_C64) & CDS_F_IK)
>> "(flags ^ CDS_F_C64) will be 0" is wrong. flags ^ CDS_F_C64
>> just flips the CDS_F_C64.
>>
>> OTOH if the CDS_F_C64 was not set we have the corresponding
>> bit set in flags ^ CDS_F_C64 so then the  CDS_F_I2K bit does
>> not matter: we have 1ULL << 11.
>>
>> In my reading the logic is good.
> 
> So I'll just leave it...
> 
>>
>>>
>>> I've stared at that condition now for a bit, but all it managed was to
>>> get me more confused... probably just need a break.
>>>    
>>>>
>>>> I2K is meaningful only when C64 is 1, otherwise it is ignored. The logic
>>>> here should be:
>>>> if ((flags & CDS_F_C64) && !(flags & CDS_F_I2K)) {
>>>>      return 1ULL << 12;
>>>> }
>>>>      return 1ULL << 11;
>>>
>>> But I do think your version is more readable... >>>
>>
>> I won't argue with this.

:)

> 
> ...and we could change that in a patch on top to avoid future confusion.
> 
>>
>>>>   
>>>>> +}
>>>>> +
>>>>> +static inline int ida_read_next_idaw(CcwDataStream *cds)
>>>>> +{
>>>>> +    union {uint64_t fmt2; uint32_t fmt1; } idaw;
>>>>                                             ^
>>>> Nit.
>>>>   
>>
>> Maybe checkpatch wanted it this way. My memories are blurry.
> 
> 
> I'd just leave it like that, tbh.
> 
>>
>>>>> +    bool is_fmt2 = cds->flags & CDS_F_C64;
>>>>> +    int ret;
>>>>> +    hwaddr idaw_addr;
>>>>> +
>>>>> +    if (is_fmt2) {
>>>>> +        idaw_addr = cds->cda_orig + sizeof(idaw.fmt2) * cds->at_idaw;
>>>>> +        if (idaw_addr & 0x07) {
>>>>> +            return -EINVAL; /* channel program check */
>>>>> +        }
>>>>> +        ret = address_space_rw(&address_space_memory, idaw_addr,
>>>>> +                               MEMTXATTRS_UNSPECIFIED, (void *) &idaw.fmt2,
>>>>> +                               sizeof(idaw.fmt2), false);
>>>>> +        cds->cda = be64_to_cpu(idaw.fmt2);
>>>>> +    } else {
>>>>> +        idaw_addr = cds->cda_orig + sizeof(idaw.fmt1) * cds->at_idaw;
>>>>> +        if (idaw_addr & 0x03) {
>>>> ?:
>>>> (idaw_addr & 0x80000003)
>>>
>>> Yes.
>>>    
>>
>> I will double check this. Does not seem unreasonable but
>> double-checking is better.
> 
> Please let me know. I think the architecture says that the bit must be
> zero, and that we may (...) generate a channel program check.
> 

Right (0x80000003) !=0 implies program check .
It is what is done , except for the bit 0 that was forgotten.

>>
>>>>   
>>>>> +            return -EINVAL; /* channel program check */
>>>>> +
>>>>> +        }
>>>>> +        ret = address_space_rw(&address_space_memory, idaw_addr,
>>>>> +                               MEMTXATTRS_UNSPECIFIED, (void *) &idaw.fmt1,
>>>>> +                               sizeof(idaw.fmt1), false);
>>>>> +        cds->cda = be64_to_cpu(idaw.fmt1);
>>>>> +    }
>>>>> +    ++(cds->at_idaw);
>>>>> +    if (ret != MEMTX_OK) {
>>>>> +        /* assume inaccessible address */
>>>>> +        return -EINVAL; /* channel program check */
>>>>> +
>>>>> +    }
>>>>> +    return 0;
>>>>> +}
>>>>> +
>>>>> +static int ccw_dstream_rw_ida(CcwDataStream *cds, void *buff, int len,
>>>>> +                              CcwDataStreamOp op)
>>>>> +{
>>>>> +    uint64_t bsz = ccw_ida_block_size(cds->flags);
>>>>> +    int ret = 0;
>>>>> +    uint16_t cont_left, iter_len;
>>>>> +
>>>>> +    ret = cds_check_len(cds, len);
>>>>> +    if (ret <= 0) {
>>>>> +        return ret;
>>>>> +    }
>>>>> +    if (!cds->at_idaw) {
>>>>> +        /* read first idaw */
>>>>> +        ret = ida_read_next_idaw(cds);
>>>>> +        if (ret) {
>>>>> +            goto err;
>>>>> +        }
>>>>> +        cont_left = ida_continuous_left(cds->cda, bsz);
>>>>> +    } else {
>>>>> +        cont_left = ida_continuous_left(cds->cda, bsz);
>>>>> +        if (cont_left == bsz) {
>>>>> +            ret = ida_read_next_idaw(cds);
>>>>> +            if (ret) {
>>>>> +                goto err;
>>>>> +            }
>>>>> +            if (cds->cda & (bsz - 1)) {
>>>> Could move this check into ida_read_next_idaw?
>>>
>>> I'd like to avoid further code movement...
>>>    
>>
>> The first idaw is special. I don't think moving is possible.
> 
> So, the code is correct and I'll just leave it like that.

hum.
It seems to me that the handling of the first IDAW is indeed a little 
bit different.
It is accessed directly from the CCW->CDA and generated dedicated status 
but I do not see the handling.

The channel status must be made pending with primary, secondary and 
alert status.

I do not find this code, may be it is somewhere before, I searched but 
did not find it.
Also, I do not find in the documentation that we have a program check 
for this case.

> 
>>
>>>>   
>>>>> +                ret = -EINVAL; /* channel program check */
>>>>> +                goto err;
>>>>> +            }
>>>>> +        }
>>>>> +    }
>>>>> +    do {
>>>>> +        iter_len = MIN(len, cont_left);
>>>>> +        if (op != CDS_OP_A) {
>>>>> +            ret = address_space_rw(&address_space_memory, cds->cda,
>>>>> +                                   MEMTXATTRS_UNSPECIFIED, buff, iter_len, op);
>>>> Ahh, now I recall that explictly defining CDS_OP_R to 0 and CDS_OP_W to
>>>> 1 in 'struct CcwDataStreamOp' do have a meaning. Does it make sense to
>>>> make it more obvious by adding some comment there?
>>>
>>> Would you have a good text for that?
>>>    
>>
>> I'm fine with clarifications.
> 
> Let's do it as a patch on top.
> 
>>
>>>>   
>>>>> +            if (ret != MEMTX_OK) {
>>>>> +                /* assume inaccessible address */
>>>>> +                ret = -EINVAL; /* channel program check */
>>>>> +                goto err;
>>>>> +            }
>>>>> +        }
>>>>> +        cds->at_byte += iter_len;
>>>>> +        cds->cda += iter_len;
>>>>> +        len -= iter_len;
>>>>> +        if (!len) {
>>>>> +            break;
>>>>> +        }
>>>>> +        ret = ida_read_next_idaw(cds);
>>>>> +        if (ret) {
>>>>> +            goto err;
>>>>> +        }
>>>>> +        cont_left = bsz;
>>>>> +    } while (true);
>>>>> +    return ret;
>>>>> +err:
>>>>> +    cds->flags |= CDS_F_STREAM_BROKEN;
>>>>> +    return ret;
>>>>> +}
>>>>> +
>>>>>   void ccw_dstream_init(CcwDataStream *cds, CCW1 const *ccw, ORB const *orb)
>>>>>   {
>>>>>       /*
>>>>> @@ -835,7 +942,7 @@ void ccw_dstream_init(CcwDataStream *cds, CCW1 const *ccw, ORB const *orb)
>>>>>       if (!(cds->flags & CDS_F_IDA)) {
>>>>>           cds->op_handler = ccw_dstream_rw_noflags;
>>>>>       } else {
>>>>> -        assert(false);
>>>>> +        cds->op_handler = ccw_dstream_rw_ida;
>>>>>       }
>>>>>   }
>>>>>
>>>>> -- 
>>>>> 2.13.5
>>>>>      
>>>>
>>>> Generally, the logic looks fine to me.
>>>>   
>>>
>>> It did pass Halil's test; but that can only test fmt-2 + 4k blocks, as
>>> this is what the kernel infrastructure provides.
>>
>> Nod.
>>
>>>
>>> Halil, do you have some more comments?
>>>    
>>
>> Just a question. Do I have to respin?
> 
> I don't think so. If you could confirm the check for format-1, I'll
> just fixup that one and get the queued patches out of the door.

generally LGTM but in my opinion the check for format-1 and the handling 
of the error status for the first IDA for format 1 and 2 must be cleared.


> 
> We can do more changes on top; it's not like I don't have more patches
> waiting...
> 


-- 
Pierre Morel
Linux/KVM/QEMU in Böblingen - Germany

Re: [Qemu-devel] [PATCH v2 1/4] s390x/css: introduce css data stream

[Pierre Morel]

On 19/09/2017 11:53, Cornelia Huck wrote:
> On Tue, 19 Sep 2017 11:11:27 +0200
> Pierre Morel <pmorel@linux.vnet.ibm.com> wrote:
> 
>> On 13/09/2017 13:50, Halil Pasic wrote:
>>> This is a preparation for introducing handling for indirect data
>>> addressing and modified indirect data addressing (CCW). Here we introduce
>>> an interface which should make the addressing scheme transparent for the
>>> client code. Here we implement only the basic scheme (no IDA or MIDA).
>>>
>>> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
>>> ---
>>>    hw/s390x/css.c         | 55 +++++++++++++++++++++++++++++++++++++++++
>>>    include/hw/s390x/css.h | 67 ++++++++++++++++++++++++++++++++++++++++++++++++++
>>>    2 files changed, 122 insertions(+)
> 
>>> diff --git a/include/hw/s390x/css.h b/include/hw/s390x/css.h
>>> index 0653d3c9be..79acaf99b7 100644
>>> --- a/include/hw/s390x/css.h
>>> +++ b/include/hw/s390x/css.h
>>> @@ -75,6 +75,29 @@ typedef struct CMBE {
>>>        uint32_t reserved[7];
>>>    } QEMU_PACKED CMBE;
>>>
>>> +typedef enum CcwDataStreamOp {
>>> +    CDS_OP_R = 0,
>>> +    CDS_OP_W = 1,
>>> +    CDS_OP_A = 2
>>> +} CcwDataStreamOp;
>>> +
>>> +/** normal usage is via SuchchDev.cds instead of instantiating */
>>> +typedef struct CcwDataStream {
>>> +#define CDS_F_IDA   0x01
>>> +#define CDS_F_MIDA  0x02
>>> +#define CDS_F_I2K   0x04
>>> +#define CDS_F_C64   0x08
>>> +#define CDS_F_STREAM_BROKEN  0x80
>>> +    uint8_t flags;
>>> +    uint8_t at_idaw;
>>> +    uint16_t at_byte;
>>> +    uint16_t count;
>>> +    uint32_t cda_orig;
>>> +    int (*op_handler)(struct CcwDataStream *cds, void *buff, int len,
>>> +                      CcwDataStreamOp op);
>>
>> I would have prefer one handler per operation instead of a operation
>> parameter.
>>
>> Is it possible to change the name of the "buf" argument to "arg".
>> I just think of the foollowing:
>> If one day we do not want to gather all IDAs into a single buffer but
>> keep them split, we can have something like an array of buffers as argument.
> 
> It should be possible to change the internal format should we decide to
> want to do something different, so I'll leave that as-is for now.
> 
>>
>>
>>
>>> +    hwaddr cda;
>>> +} CcwDataStream;
>>> +
> 
>> otherwise, LGTM
> 
> Is that an ack? :)
> 
> 

Reviewed-by: Pierre Morel<pmorel@linux.vnet.ibm.com>


-- 
Pierre Morel
Linux/KVM/QEMU in Böblingen - Germany

Re: [Qemu-devel] [PATCH v2 2/4] s390x/css: use ccw data stream

[Pierre Morel]

On 13/09/2017 13:50, Halil Pasic wrote:
> Replace direct access which implicitly assumes no IDA
> or MIDA with the new ccw data stream interface which should
> cope with these transparently in the future.
> 
> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
> ---
>   hw/s390x/css.c | 9 +++++----
>   1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/s390x/css.c b/hw/s390x/css.c
> index e8d2016563..6b0cd8861b 100644
> --- a/hw/s390x/css.c
> +++ b/hw/s390x/css.c
> @@ -890,6 +890,7 @@ static int css_interpret_ccw(SubchDev *sch, hwaddr ccw_addr,
>       }
> 
>       /* Look at the command. */
> +    ccw_dstream_init(&sch->cds, &ccw, &(sch->orb));
>       switch (ccw.cmd_code) {
>       case CCW_CMD_NOOP:
>           /* Nothing to do. */
> @@ -903,8 +904,8 @@ static int css_interpret_ccw(SubchDev *sch, hwaddr ccw_addr,
>               }
>           }
>           len = MIN(ccw.count, sizeof(sch->sense_data));
> -        cpu_physical_memory_write(ccw.cda, sch->sense_data, len);
> -        sch->curr_status.scsw.count = ccw.count - len;
> +        ccw_dstream_write_buf(&sch->cds, sch->sense_data, len);
> +        sch->curr_status.scsw.count = ccw_dstream_residual_count(&sch->cds);
>           memset(sch->sense_data, 0, sizeof(sch->sense_data));
>           ret = 0;
>           break;
> @@ -930,8 +931,8 @@ static int css_interpret_ccw(SubchDev *sch, hwaddr ccw_addr,
>           } else {
>               sense_id.reserved = 0;
>           }
> -        cpu_physical_memory_write(ccw.cda, &sense_id, len);
> -        sch->curr_status.scsw.count = ccw.count - len;
> +        ccw_dstream_write_buf(&sch->cds, &sense_id, len);
> +        sch->curr_status.scsw.count = ccw_dstream_residual_count(&sch->cds);
>           ret = 0;
>           break;
>       }
> 

Reviewed-by: Pierre Morel<pmorel@linux.vnet.ibm.com>

-- 
Pierre Morel
Linux/KVM/QEMU in Böblingen - Germany

Re: [Qemu-devel] [PATCH v2 3/4] virtio-ccw: use ccw data stream

[Pierre Morel]

On 13/09/2017 13:50, Halil Pasic wrote:
> Replace direct access which implicitly assumes no IDA
> or MIDA with the new ccw data stream interface which should
> cope with these transparently in the future.
> 
> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
> 
> ---
> 
> v1 --> v2:
> Removed todo comments on possible errno change as with
> https://lists.nongnu.org/archive/html/qemu-devel/2017-09/msg02441.html
> applied it does not matter any more.
> 
> Error handling: At the moment we ignore errors reported
> by stream ops to keep the change minimal. An add-on
> patch improving on this is to be expected later.
> ---
>   hw/s390x/virtio-ccw.c | 156 +++++++++++++++-----------------------------------
>   1 file changed, 46 insertions(+), 110 deletions(-)

:)

> 
> diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
> index b1976fdd19..a9baf6f7ab 100644
> --- a/hw/s390x/virtio-ccw.c
> +++ b/hw/s390x/virtio-ccw.c
> @@ -287,49 +287,19 @@ static int virtio_ccw_handle_set_vq(SubchDev *sch, CCW1 ccw, bool check_len,
>           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);
> +        ccw_dstream_read(&sch->cds, linfo);
> +        be64_to_cpus(&linfo.queue);
> +        be32_to_cpus(&linfo.align);
> +        be16_to_cpus(&linfo.index);
> +        be16_to_cpus(&linfo.num);
>           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);
> +        ccw_dstream_read(&sch->cds, info);
> +        be64_to_cpus(&info.desc);
> +        be16_to_cpus(&info.index);
> +        be16_to_cpus(&info.num);
> +        be64_to_cpus(&info.avail);
> +        be64_to_cpus(&info.used);
>           ret = virtio_ccw_set_vqs(sch, &info, NULL);
>       }
>       sch->curr_status.scsw.count = 0;
> @@ -342,15 +312,13 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
>       VirtioRevInfo revinfo;
>       uint8_t status;
>       VirtioFeatDesc features;
> -    void *config;
>       hwaddr indicators;
>       VqConfigBlock vq_config;
>       VirtioCcwDevice *dev = sch->driver_data;
>       VirtIODevice *vdev = virtio_ccw_get_vdev(sch);
>       bool check_len;
>       int len;
> -    hwaddr hw_len;
> -    VirtioThinintInfo *thinint;
> +    VirtioThinintInfo thinint;
> 
>       if (!dev) {
>           return -EINVAL;
> @@ -394,11 +362,8 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
>           } else {
>               VirtioDeviceClass *vdc = VIRTIO_DEVICE_GET_CLASS(vdev);
> 
> -            features.index = address_space_ldub(&address_space_memory,
> -                                                ccw.cda
> -                                                + sizeof(features.features),
> -                                                MEMTXATTRS_UNSPECIFIED,
> -                                                NULL);
> +            ccw_dstream_advance(&sch->cds, sizeof(features.features));
> +            ccw_dstream_read(&sch->cds, features.index);
>               if (features.index == 0) {
>                   if (dev->revision >= 1) {
>                       /* Don't offer legacy features for modern devices. */
> @@ -417,9 +382,9 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
>                   /* Return zeroes if the guest supports more feature bits. */
>                   features.features = 0;
>               }
> -            address_space_stl_le(&address_space_memory, ccw.cda,
> -                                 features.features, MEMTXATTRS_UNSPECIFIED,
> -                                 NULL);
> +            ccw_dstream_rewind(&sch->cds);
> +            cpu_to_le32s(&features.features);
> +            ccw_dstream_write(&sch->cds, features.features);
>               sch->curr_status.scsw.count = ccw.count - sizeof(features);
>               ret = 0;
>           }
> @@ -438,15 +403,8 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
>           if (!ccw.cda) {
>               ret = -EFAULT;
>           } else {
> -            features.index = address_space_ldub(&address_space_memory,
> -                                                ccw.cda
> -                                                + sizeof(features.features),
> -                                                MEMTXATTRS_UNSPECIFIED,
> -                                                NULL);
> -            features.features = address_space_ldl_le(&address_space_memory,
> -                                                     ccw.cda,
> -                                                     MEMTXATTRS_UNSPECIFIED,
> -                                                     NULL);
> +            ccw_dstream_read(&sch->cds, features);
> +            le32_to_cpus(&features.features);
>               if (features.index == 0) {
>                   virtio_set_features(vdev,
>                                       (vdev->guest_features & 0xffffffff00000000ULL) |
> @@ -488,7 +446,7 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
>           } else {
>               virtio_bus_get_vdev_config(&dev->bus, vdev->config);
>               /* XXX config space endianness */
> -            cpu_physical_memory_write(ccw.cda, vdev->config, len);
> +            ccw_dstream_write_buf(&sch->cds, vdev->config, len);
>               sch->curr_status.scsw.count = ccw.count - len;
>               ret = 0;
>           }
> @@ -501,21 +459,13 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
>               }
>           }
>           len = MIN(ccw.count, vdev->config_len);
> -        hw_len = len;
>           if (!ccw.cda) {
>               ret = -EFAULT;
>           } else {
> -            config = cpu_physical_memory_map(ccw.cda, &hw_len, 0);
> -            if (!config) {
> -                ret = -EFAULT;
> -            } else {
> -                len = hw_len;
> -                /* XXX config space endianness */
> -                memcpy(vdev->config, config, len);
> -                cpu_physical_memory_unmap(config, hw_len, 0, hw_len);
> +            ret = ccw_dstream_read_buf(&sch->cds, vdev->config, len);
> +            if (!ret) {
>                   virtio_bus_set_vdev_config(&dev->bus, vdev->config);
>                   sch->curr_status.scsw.count = ccw.count - len;
> -                ret = 0;
>               }
>           }
>           break;
> @@ -553,8 +503,7 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
>           if (!ccw.cda) {
>               ret = -EFAULT;
>           } else {
> -            status = address_space_ldub(&address_space_memory, ccw.cda,
> -                                        MEMTXATTRS_UNSPECIFIED, NULL);
> +            ccw_dstream_read(&sch->cds, status);
>               if (!(status & VIRTIO_CONFIG_S_DRIVER_OK)) {
>                   virtio_ccw_stop_ioeventfd(dev);
>               }
> @@ -597,8 +546,8 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
>           if (!ccw.cda) {
>               ret = -EFAULT;
>           } else {
> -            indicators = address_space_ldq_be(&address_space_memory, ccw.cda,
> -                                              MEMTXATTRS_UNSPECIFIED, NULL);
> +            ccw_dstream_read(&sch->cds, indicators);
> +            be64_to_cpus(&indicators);
>               dev->indicators = get_indicator(indicators, sizeof(uint64_t));
>               sch->curr_status.scsw.count = ccw.count - sizeof(indicators);
>               ret = 0;
> @@ -618,8 +567,8 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
>           if (!ccw.cda) {
>               ret = -EFAULT;
>           } else {
> -            indicators = address_space_ldq_be(&address_space_memory, ccw.cda,
> -                                              MEMTXATTRS_UNSPECIFIED, NULL);
> +            ccw_dstream_read(&sch->cds, indicators);
> +            be64_to_cpus(&indicators);
>               dev->indicators2 = get_indicator(indicators, sizeof(uint64_t));
>               sch->curr_status.scsw.count = ccw.count - sizeof(indicators);
>               ret = 0;
> @@ -639,67 +588,58 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
>           if (!ccw.cda) {
>               ret = -EFAULT;
>           } else {
> -            vq_config.index = address_space_lduw_be(&address_space_memory,
> -                                                    ccw.cda,
> -                                                    MEMTXATTRS_UNSPECIFIED,
> -                                                    NULL);
> +            ccw_dstream_read(&sch->cds, vq_config.index);
> +            be16_to_cpus(&vq_config.index);
>               if (vq_config.index >= VIRTIO_QUEUE_MAX) {
>                   ret = -EINVAL;
>                   break;
>               }
>               vq_config.num_max = virtio_queue_get_num(vdev,
>                                                        vq_config.index);
> -            address_space_stw_be(&address_space_memory,
> -                                 ccw.cda + sizeof(vq_config.index),
> -                                 vq_config.num_max,
> -                                 MEMTXATTRS_UNSPECIFIED,
> -                                 NULL);
> +            cpu_to_be16s(&vq_config.num_max);
> +            ccw_dstream_write(&sch->cds, vq_config.num_max);
>               sch->curr_status.scsw.count = ccw.count - sizeof(vq_config);
>               ret = 0;
>           }
>           break;
>       case CCW_CMD_SET_IND_ADAPTER:
>           if (check_len) {
> -            if (ccw.count != sizeof(*thinint)) {
> +            if (ccw.count != sizeof(thinint)) {
>                   ret = -EINVAL;
>                   break;
>               }
> -        } else if (ccw.count < sizeof(*thinint)) {
> +        } else if (ccw.count < sizeof(thinint)) {
>               /* Can't execute command. */
>               ret = -EINVAL;
>               break;
>           }
> -        len = sizeof(*thinint);
> -        hw_len = len;
>           if (!ccw.cda) {
>               ret = -EFAULT;
>           } else if (dev->indicators && !sch->thinint_active) {
>               /* Trigger a command reject. */
>               ret = -ENOSYS;
>           } else {
> -            thinint = cpu_physical_memory_map(ccw.cda, &hw_len, 0);
> -            if (!thinint) {
> +            if (ccw_dstream_read(&sch->cds, thinint)) {
>                   ret = -EFAULT;
>               } else {
> -                uint64_t ind_bit = ldq_be_p(&thinint->ind_bit);
> +                be64_to_cpus(&thinint.ind_bit);
> +                be64_to_cpus(&thinint.summary_indicator);
> +                be64_to_cpus(&thinint.device_indicator);
> 
> -                len = hw_len;
>                   dev->summary_indicator =
> -                    get_indicator(ldq_be_p(&thinint->summary_indicator),
> -                                  sizeof(uint8_t));
> +                    get_indicator(thinint.summary_indicator, sizeof(uint8_t));
>                   dev->indicators =
> -                    get_indicator(ldq_be_p(&thinint->device_indicator),
> -                                  ind_bit / 8 + 1);
> -                dev->thinint_isc = thinint->isc;
> -                dev->routes.adapter.ind_offset = ind_bit;
> +                    get_indicator(thinint.device_indicator,
> +                                  thinint.ind_bit / 8 + 1);
> +                dev->thinint_isc = thinint.isc;
> +                dev->routes.adapter.ind_offset = thinint.ind_bit;
>                   dev->routes.adapter.summary_offset = 7;
> -                cpu_physical_memory_unmap(thinint, hw_len, 0, hw_len);
>                   dev->routes.adapter.adapter_id = css_get_adapter_id(
>                                                    CSS_IO_ADAPTER_VIRTIO,
>                                                    dev->thinint_isc);
>                   sch->thinint_active = ((dev->indicators != NULL) &&
>                                          (dev->summary_indicator != NULL));
> -                sch->curr_status.scsw.count = ccw.count - len;
> +                sch->curr_status.scsw.count = ccw.count - sizeof(thinint);
>                   ret = 0;
>               }
>           }
> @@ -714,13 +654,9 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
>               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);
> +        ccw_dstream_read_buf(&sch->cds, &revinfo, 4);
> +        be16_to_cpus(&revinfo.revision);
> +        be16_to_cpus(&revinfo.length);
>           if (ccw.count < len + revinfo.length ||
>               (check_len && ccw.count > len + revinfo.length)) {
>               ret = -EINVAL;
> 

Reviewed-by: Pierre Morel<pmorel@linux.vnet.ibm.com>

-- 
Pierre Morel
Linux/KVM/QEMU in Böblingen - Germany

Re: [Qemu-devel] [PATCH v2 4/4] s390x/css: support ccw IDA

[Cornelia Huck]

On Tue, 19 Sep 2017 14:32:33 +0200
Halil Pasic <pasic@linux.vnet.ibm.com> wrote:

> On 09/19/2017 02:23 PM, Cornelia Huck wrote:
> > On Tue, 19 Sep 2017 14:04:03 +0200
> > Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
> >   
> >> On 09/19/2017 12:57 PM, Cornelia Huck wrote:  
> >>>>>>> +static inline int ida_read_next_idaw(CcwDataStream *cds)
> >>>>>>> +{
> >>>>>>> +    union {uint64_t fmt2; uint32_t fmt1; } idaw;        
> >>>>>>                                            ^
> >>>>>> Nit.
> >>>>>>      
> >>>> Maybe checkpatch wanted it this way. My memories are blurry.    
> >>>
> >>> I'd just leave it like that, tbh.
> >>>     
> >>>>>>> +    bool is_fmt2 = cds->flags & CDS_F_C64;
> >>>>>>> +    int ret;
> >>>>>>> +    hwaddr idaw_addr;
> >>>>>>> +
> >>>>>>> +    if (is_fmt2) {
> >>>>>>> +        idaw_addr = cds->cda_orig + sizeof(idaw.fmt2) * cds->at_idaw;
> >>>>>>> +        if (idaw_addr & 0x07) {
> >>>>>>> +            return -EINVAL; /* channel program check */
> >>>>>>> +        }
> >>>>>>> +        ret = address_space_rw(&address_space_memory, idaw_addr,
> >>>>>>> +                               MEMTXATTRS_UNSPECIFIED, (void *) &idaw.fmt2,
> >>>>>>> +                               sizeof(idaw.fmt2), false);
> >>>>>>> +        cds->cda = be64_to_cpu(idaw.fmt2);    
> >>
> >>  
> >>>>>>> +    } else {
> >>>>>>> +        idaw_addr = cds->cda_orig + sizeof(idaw.fmt1) * cds->at_idaw;
> >>>>>>> +        if (idaw_addr & 0x03) {        
> >>>>>> ?:
> >>>>>> (idaw_addr & 0x80000003)      
> >>>>> Yes.
> >>>>>       
> >>>> I will double check this. Does not seem unreasonable but
> >>>> double-checking is better.    
> >>> Please let me know. I think the architecture says that the bit must be
> >>> zero, and that we may (...) generate a channel program check.
> >>>     
> >>
> >> Not exactly. The more significant bits part of the check
> >> depend on the ccw format. This needs to be done for both
> >> idaw formats. I would need to introduce a new flag, or
> >> access the SubchDev to do this properly.
> >>
> >> Architecturally we also need to check the data addresses
> >> from which we read so we have nothing bigger than 
> >> (1 << 31) - 1 if we are working with format-1 idaws.
> >>
> >> I also think we did not take proper care of proper
> >> maximum data address checks prior CwwDataStream which also
> >> depend on the ccw format (in absence of IDAW or MIDAW).
> >>
> >> The ccw format dependent maximum address checks are (1 << 24) - 1
> >> and (1 << 31) - 1 respectively for format-0 and format-1 (on
> >> the first indirection level that is for non-IDA for the data,
> >> and for (M)IDA for the (M)IDAWs).
> >>
> >> Reference:
> >> PoP pages 16-25 and 16-26 "Invalid IDAW or MIDAW Addre" and
> >> "Invalid Data Address".  
> > 
> > Sigh, when are things ever easy :(
> > 
> > I'll need some time to review this. But more urgently, I need a break.
> >   
> >>
> >> How shall we proceed?  
> > 
> > Given the discussion about this patch in particular, I'll probably
> > dequeue it and send it with the next batch of patches. We can also
> > include the other improvements, then. Not sure whether I should just
> > dequeue this one or the whole series (I really want to get the rest of
> > the patches out...)
> > 
> > More after the break :)
> >   
> 
> I think I can fix this pretty fast, and fixing this later is
> an option too in my opinion as the patch is consistent with our
> prior art (we ignored this maximum address checking requirement,
> and we keep ignoring it). 
> 
> I would prefer not dequeue-ing the whole series because a
> 3270 fix of mine (which just made the internal review) depends
> on this. IMHO, it's not like the series is fundamentally broken,
> not ain't an improvement at all. It is not perfect, but it's
> IMHO certainly an improvement over what we have.

The issue is not that the series is problematic, but that I need to put
a stop on changes at some point in time and just flush my queue -
otherwise this is not workable.

Therefore, I dequeued the series for now. I'll probably do another pull
request in the next weeks anyway - so there's unlikely to be a long
delay, and we can hash out everything without pressure.

It would be best to just think this through; then you can send a v3
with the feedback addressed and your 3270 patch on top.

Re: [Qemu-devel] [PATCH v2 4/4] s390x/css: support ccw IDA

[Halil Pasic]

On 09/19/2017 03:46 PM, Pierre Morel wrote:
> On 19/09/2017 12:57, Cornelia Huck wrote:
>> On Tue, 19 Sep 2017 12:36:33 +0200
>> Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
>>
>>> On 09/19/2017 11:48 AM, Cornelia Huck wrote:
>>>> On Tue, 19 Sep 2017 13:50:05 +0800
>>>> Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com> wrote:
>>>>   
>>>>> * Halil Pasic <pasic@linux.vnet.ibm.com> [2017-09-13 13:50:29 +0200]:
>>>>>  
>>>>>> Let's add indirect data addressing support for our virtual channel
>>>>>> subsystem. This implementation does no bother with any kind of
>>>>>> prefetching. We simply step trough the IDAL on demand.
>>>>>>
>>>>>> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
>>>>>> ---
>>>>>>   hw/s390x/css.c | 109 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>>>>>>   1 file changed, 108 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/hw/s390x/css.c b/hw/s390x/css.c
>>>>>> index 6b0cd8861b..e34b2af4eb 100644
>>>>>> --- a/hw/s390x/css.c
>>>>>> +++ b/hw/s390x/css.c
>>>>>> @@ -819,6 +819,113 @@ incr:
>>>>>>       return 0;
>>>>>>   }
>>>>>>
>>>>>> +/* returns values between 1 and bsz, where bs is a power of 2 */
>>>>>> +static inline uint16_t ida_continuous_left(hwaddr cda, uint64_t bsz)
>>>>>> +{
>>>>>> +    return bsz - (cda & (bsz - 1));
>>>>>> +}
>>>>>> +
>>>>>> +static inline uint64_t ccw_ida_block_size(uint8_t flags)
>>>>>> +{
>>>>>> +    return 1ULL << (((flags ^ CDS_F_C64) & (CDS_F_C64 | CDS_F_I2K)) ? 11 : 12);
>>>>> If CDS_F_C64 is set, (flags ^ CDS_F_C64) will be 0, so (1ULL << 11) will
>>>>> be the result regardless the I2K flag? The logic seems wrong.
>>>
>>> No. If CDS_F_C64 is set then the outcome depends on the fact if
>>> CDS_F_I2K is set or not.
>>> (flags & CDS_F_IK) => ((flags ^ CDS_F_C64) & CDS_F_IK)
>>> "(flags ^ CDS_F_C64) will be 0" is wrong. flags ^ CDS_F_C64
>>> just flips the CDS_F_C64.
>>>
>>> OTOH if the CDS_F_C64 was not set we have the corresponding
>>> bit set in flags ^ CDS_F_C64 so then the  CDS_F_I2K bit does
>>> not matter: we have 1ULL << 11.
>>>
>>> In my reading the logic is good.
>>
>> So I'll just leave it...
>>
>>>
>>>>
>>>> I've stared at that condition now for a bit, but all it managed was to
>>>> get me more confused... probably just need a break.
>>>>   
>>>>>
>>>>> I2K is meaningful only when C64 is 1, otherwise it is ignored. The logic
>>>>> here should be:
>>>>> if ((flags & CDS_F_C64) && !(flags & CDS_F_I2K)) {
>>>>>      return 1ULL << 12;
>>>>> }
>>>>>      return 1ULL << 11;
>>>>
>>>> But I do think your version is more readable... >>>
>>>
>>> I won't argue with this.
> 
> :)
> 
>>
>> ...and we could change that in a patch on top to avoid future confusion.
>>
>>>
>>>>>  
>>>>>> +}
>>>>>> +
>>>>>> +static inline int ida_read_next_idaw(CcwDataStream *cds)
>>>>>> +{
>>>>>> +    union {uint64_t fmt2; uint32_t fmt1; } idaw;
>>>>>                                             ^
>>>>> Nit.
>>>>>   
>>>
>>> Maybe checkpatch wanted it this way. My memories are blurry.
>>
>>
>> I'd just leave it like that, tbh.
>>
>>>
>>>>>> +    bool is_fmt2 = cds->flags & CDS_F_C64;
>>>>>> +    int ret;
>>>>>> +    hwaddr idaw_addr;
>>>>>> +
>>>>>> +    if (is_fmt2) {
>>>>>> +        idaw_addr = cds->cda_orig + sizeof(idaw.fmt2) * cds->at_idaw;
>>>>>> +        if (idaw_addr & 0x07) {
>>>>>> +            return -EINVAL; /* channel program check */
>>>>>> +        }
>>>>>> +        ret = address_space_rw(&address_space_memory, idaw_addr,
>>>>>> +                               MEMTXATTRS_UNSPECIFIED, (void *) &idaw.fmt2,
>>>>>> +                               sizeof(idaw.fmt2), false);
>>>>>> +        cds->cda = be64_to_cpu(idaw.fmt2);
>>>>>> +    } else {
>>>>>> +        idaw_addr = cds->cda_orig + sizeof(idaw.fmt1) * cds->at_idaw;
>>>>>> +        if (idaw_addr & 0x03) {
>>>>> ?:
>>>>> (idaw_addr & 0x80000003)
>>>>
>>>> Yes.
>>>>    
>>>
>>> I will double check this. Does not seem unreasonable but
>>> double-checking is better.
>>
>> Please let me know. I think the architecture says that the bit must be
>> zero, and that we may (...) generate a channel program check.
>>
> 
> Right (0x80000003) !=0 implies program check .
> It is what is done , except for the bit 0 that was forgotten.

Reference?

I've just explained something different above (depends
on the ccw format and yes in case of format 1 I guess
the check can be done like that). If you are arguing with
that, please be more verbose.

> 
>>>
>>>>>  
>>>>>> +            return -EINVAL; /* channel program check */
>>>>>> +
>>>>>> +        }
>>>>>> +        ret = address_space_rw(&address_space_memory, idaw_addr,
>>>>>> +                               MEMTXATTRS_UNSPECIFIED, (void *) &idaw.fmt1,
>>>>>> +                               sizeof(idaw.fmt1), false);
>>>>>> +        cds->cda = be64_to_cpu(idaw.fmt1);
>>>>>> +    }
>>>>>> +    ++(cds->at_idaw);
>>>>>> +    if (ret != MEMTX_OK) {
>>>>>> +        /* assume inaccessible address */
>>>>>> +        return -EINVAL; /* channel program check */
>>>>>> +
>>>>>> +    }
>>>>>> +    return 0;
>>>>>> +}
>>>>>> +
>>>>>> +static int ccw_dstream_rw_ida(CcwDataStream *cds, void *buff, int len,
>>>>>> +                              CcwDataStreamOp op)
>>>>>> +{
>>>>>> +    uint64_t bsz = ccw_ida_block_size(cds->flags);
>>>>>> +    int ret = 0;
>>>>>> +    uint16_t cont_left, iter_len;
>>>>>> +
>>>>>> +    ret = cds_check_len(cds, len);
>>>>>> +    if (ret <= 0) {
>>>>>> +        return ret;
>>>>>> +    }
>>>>>> +    if (!cds->at_idaw) {
>>>>>> +        /* read first idaw */
>>>>>> +        ret = ida_read_next_idaw(cds);
>>>>>> +        if (ret) {
>>>>>> +            goto err;
>>>>>> +        }
>>>>>> +        cont_left = ida_continuous_left(cds->cda, bsz);
>>>>>> +    } else {
>>>>>> +        cont_left = ida_continuous_left(cds->cda, bsz);
>>>>>> +        if (cont_left == bsz) {
>>>>>> +            ret = ida_read_next_idaw(cds);
>>>>>> +            if (ret) {
>>>>>> +                goto err;
>>>>>> +            }
>>>>>> +            if (cds->cda & (bsz - 1)) {
>>>>> Could move this check into ida_read_next_idaw?
>>>>
>>>> I'd like to avoid further code movement...
>>>>    
>>>
>>> The first idaw is special. I don't think moving is possible.
>>
>> So, the code is correct and I'll just leave it like that.
> 
> hum.
> It seems to me that the handling of the first IDAW is indeed a little bit different.
> It is accessed directly from the CCW->CDA and generated dedicated status but I do not see the handling.
> 
> The channel status must be made pending with primary, secondary and alert status.
>

Reference?
 
> I do not find this code, may be it is somewhere before, I searched but did not find it.
> Also, I do not find in the documentation that we have a program check for this case.
> 

I don't understand a thing. Aren't you confusing this with some internal
discussion?

Sorry, but I think, I will ignore the comment for now.

>>
>>>
>>>>>  
>>>>>> +                ret = -EINVAL; /* channel program check */
>>>>>> +                goto err;
>>>>>> +            }
>>>>>> +        }
>>>>>> +    }
>>>>>> +    do {
>>>>>> +        iter_len = MIN(len, cont_left);
>>>>>> +        if (op != CDS_OP_A) {
>>>>>> +            ret = address_space_rw(&address_space_memory, cds->cda,
>>>>>> +                                   MEMTXATTRS_UNSPECIFIED, buff, iter_len, op);
>>>>> Ahh, now I recall that explictly defining CDS_OP_R to 0 and CDS_OP_W to
>>>>> 1 in 'struct CcwDataStreamOp' do have a meaning. Does it make sense to
>>>>> make it more obvious by adding some comment there?
>>>>
>>>> Would you have a good text for that?
>>>>    
>>>
>>> I'm fine with clarifications.
>>
>> Let's do it as a patch on top.
>>
>>>
>>>>>  
>>>>>> +            if (ret != MEMTX_OK) {
>>>>>> +                /* assume inaccessible address */
>>>>>> +                ret = -EINVAL; /* channel program check */
>>>>>> +                goto err;
>>>>>> +            }
>>>>>> +        }
>>>>>> +        cds->at_byte += iter_len;
>>>>>> +        cds->cda += iter_len;
>>>>>> +        len -= iter_len;
>>>>>> +        if (!len) {
>>>>>> +            break;
>>>>>> +        }
>>>>>> +        ret = ida_read_next_idaw(cds);
>>>>>> +        if (ret) {
>>>>>> +            goto err;
>>>>>> +        }
>>>>>> +        cont_left = bsz;
>>>>>> +    } while (true);
>>>>>> +    return ret;
>>>>>> +err:
>>>>>> +    cds->flags |= CDS_F_STREAM_BROKEN;
>>>>>> +    return ret;
>>>>>> +}
>>>>>> +
>>>>>>   void ccw_dstream_init(CcwDataStream *cds, CCW1 const *ccw, ORB const *orb)
>>>>>>   {
>>>>>>       /*
>>>>>> @@ -835,7 +942,7 @@ void ccw_dstream_init(CcwDataStream *cds, CCW1 const *ccw, ORB const *orb)
>>>>>>       if (!(cds->flags & CDS_F_IDA)) {
>>>>>>           cds->op_handler = ccw_dstream_rw_noflags;
>>>>>>       } else {
>>>>>> -        assert(false);
>>>>>> +        cds->op_handler = ccw_dstream_rw_ida;
>>>>>>       }
>>>>>>   }
>>>>>>
>>>>>> -- 
>>>>>> 2.13.5
>>>>>>      
>>>>>
>>>>> Generally, the logic looks fine to me.
>>>>>   
>>>>
>>>> It did pass Halil's test; but that can only test fmt-2 + 4k blocks, as
>>>> this is what the kernel infrastructure provides.
>>>
>>> Nod.
>>>
>>>>
>>>> Halil, do you have some more comments?
>>>>    
>>>
>>> Just a question. Do I have to respin?
>>
>> I don't think so. If you could confirm the check for format-1, I'll
>> just fixup that one and get the queued patches out of the door.
> 
> generally LGTM but in my opinion the check for format-1 and the handling of the error status for the first IDA for format 1 and 2 must be cleared.

OK, I'm not adding any r-b or ack for v3 unless told otherwise.

Thanks for your review!
Halil

Re: [Qemu-devel] [PATCH v2 4/4] s390x/css: support ccw IDA

[Halil Pasic]

On 09/19/2017 02:23 PM, Cornelia Huck wrote:
>>>>> +{
>>>>> +    union {uint64_t fmt2; uint32_t fmt1; } idaw;      
>>>>                                            ^
>>>> Nit.
>>>>    
>> Maybe checkpatch wanted it this way. My memories are blurry.  
> I'd just leave it like that, tbh.

Yes, if I remove the space before the } checkpatch.pl complains.

Going to keep it as is for v3.

Halil

Re: [Qemu-devel] [PATCH v2 3/4] virtio-ccw: use ccw data stream

[Dong Jia Shi]

* Halil Pasic <pasic@linux.vnet.ibm.com> [2017-09-19 15:30:29 +0200]:

> 
> 
> On 09/19/2017 11:06 AM, Cornelia Huck wrote:
> > On Tue, 19 Sep 2017 11:37:30 +0800
> > Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com> wrote:
> > 
> >> * Halil Pasic <pasic@linux.vnet.ibm.com> [2017-09-13 13:50:28 +0200]:
> >>
> >>> Replace direct access which implicitly assumes no IDA
> >>> or MIDA with the new ccw data stream interface which should
> >>> cope with these transparently in the future.
> >>>
> >>> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
> >>>
> >>> ---
> >>>
> >>> v1 --> v2:
> >>> Removed todo comments on possible errno change as with
> >>> https://lists.nongnu.org/archive/html/qemu-devel/2017-09/msg02441.html
> >>> applied it does not matter any more.
> >>>
> >>> Error handling: At the moment we ignore errors reported
> >>> by stream ops to keep the change minimal. An add-on
> >>> patch improving on this is to be expected later.  
> >> Add a TODO somewhere around the code as a reminder?
> > 
> > I expect Halil to have it on his TODO list and send a patch later ;)
No problem with me.

> > 
> >>
> >>> ---
> >>>  hw/s390x/virtio-ccw.c | 156 +++++++++++++++-----------------------------------
> >>>  1 file changed, 46 insertions(+), 110 deletions(-)
> >>>
> >>> diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
> >>> index b1976fdd19..a9baf6f7ab 100644
> >>> --- a/hw/s390x/virtio-ccw.c
> >>> +++ b/hw/s390x/virtio-ccw.c  
> >> [...]
> >>
> >>> @@ -394,11 +362,8 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
> >>>          } else {
> >>>              VirtioDeviceClass *vdc = VIRTIO_DEVICE_GET_CLASS(vdev);
> >>>
> >>> -            features.index = address_space_ldub(&address_space_memory,
> >>> -                                                ccw.cda
> >>> -                                                + sizeof(features.features),
> >>> -                                                MEMTXATTRS_UNSPECIFIED,
> >>> -                                                NULL);
> >>> +            ccw_dstream_advance(&sch->cds, sizeof(features.features));  
> >> How about:
> >> ccw_dstream_advance(&sch->cds, offsetof(VirtioFeatDesc, index));
> > 
> > I think keeping sizeof(features.features) is better: It matches the old
> > code, and we really do jump over the features flag and revisit it
> > later...
I was thinking 'advance' to the offset of features.index, then 'read'
the value for features.index seems more natural, and you do not need to
care too much for what exaclty the elements are that you need to
'advance'.

Not big deal for me. I could also accept the original version.

> > 
> >>
> >>> +            ccw_dstream_read(&sch->cds, features.index);
> >>>              if (features.index == 0) {
> >>>                  if (dev->revision >= 1) {
> >>>                      /* Don't offer legacy features for modern devices. */
> >>> @@ -417,9 +382,9 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
> >>>                  /* Return zeroes if the guest supports more feature bits. */
> >>>                  features.features = 0;
> >>>              }
> >>> -            address_space_stl_le(&address_space_memory, ccw.cda,
> >>> -                                 features.features, MEMTXATTRS_UNSPECIFIED,
> >>> -                                 NULL);
> >>> +            ccw_dstream_rewind(&sch->cds);
> >>> +            cpu_to_le32s(&features.features);
> >>> +            ccw_dstream_write(&sch->cds, features.features);
> >>>              sch->curr_status.scsw.count = ccw.count - sizeof(features);  
> >> How about:
> >> sch->curr_status.scsw.count = ccw_dstream_residual_count(&sch->cds);
> > 
> > Hm, I thought I had commented on this, but I seem to have missed
> > these...
> > 
> > I'd prefer to do it as a follow-up patch.
No problem.

> > 
> >>
> >> This also applies to other places.
> >>
> >>>              ret = 0;
> >>>          }  
> >> [...]
> >>
> >>> @@ -501,21 +459,13 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
> >>>              }
> >>>          }
> >>>          len = MIN(ccw.count, vdev->config_len);
> >>> -        hw_len = len;
> >>>          if (!ccw.cda) {
> >>>              ret = -EFAULT;
> >>>          } else {
> >>> -            config = cpu_physical_memory_map(ccw.cda, &hw_len, 0);
> >>> -            if (!config) {
> >>> -                ret = -EFAULT;
> >>> -            } else {
> >>> -                len = hw_len;
> >>> -                /* XXX config space endianness */
> >>> -                memcpy(vdev->config, config, len);
> >>> -                cpu_physical_memory_unmap(config, hw_len, 0, hw_len);
> >>> +            ret = ccw_dstream_read_buf(&sch->cds, vdev->config, len);
> >>> +            if (!ret) {  
> >> Add a TODO here, and:
> >>
> >> if (ccw_dstream_read_buf(&sch->cds, vdev->config, len)) {
> >>     ret = -EFAULT;
> >> } else {
> >>     ....
> >> }
> > 
> > I don't think that would be correct: The function will (at least
> > currently) return 0 or -EINVAL, and you are now mapping any error to
> > -EFAULT? (Not that it has an effect in the end: We map both to a
> > channel program check as of "s390x/css: drop data-check in
> > interpretation".)
Ah, I missed this. Now no problem from me.

> > 
> >>
> >>>                  virtio_bus_set_vdev_config(&dev->bus, vdev->config);
> >>>                  sch->curr_status.scsw.count = ccw.count - len;
> >>> -                ret = 0;
> >>>              }
> >>>          }
> >>>          break;  
> >> [...]
> >>
> >>> @@ -714,13 +654,9 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
> >>>              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);
> >>> +        ccw_dstream_read_buf(&sch->cds, &revinfo, 4);  

> >>                                                      ^
> >> A magic number? O:)
> > 
> > Hah :)
> > 
> > We can't use sizeof(revinfo), and sizeof(revinfo.revision) +
> > sizeof(revinfo.length) is a bit unwieldy. Let's keep some magic in our
> > code :)
No problem. This seems the best option.

> > 
> >>
> >>> +        be16_to_cpus(&revinfo.revision);
> >>> +        be16_to_cpus(&revinfo.length);
> >>>          if (ccw.count < len + revinfo.length ||
> >>>              (check_len && ccw.count > len + revinfo.length)) {
> >>>              ret = -EINVAL;
> >>> -- 
> >>> 2.13.5
> >>>   
> >>
> >> Otherwise, looks good.
> >>
> > 
> > Can I get an ack?
Sure:
Reviewed-by: Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com>

> > 
> 
> I agree that further cleanups are possible (e.g. using
> ccw_dstream_residual_count() and tightening error handling) but
> I also prefer to see these as on-top, and prefer sticking to
> change as little as possible in the transformation patch and stay
> as close as possible to what we had before in terms of behavior).
Understand and agree.

> 
> Regards,
> Halil 
> 

-- 
Dong Jia Shi

Re: [Qemu-devel] [PATCH v2 4/4] s390x/css: support ccw IDA

[Dong Jia Shi]

* Halil Pasic <pasic@linux.vnet.ibm.com> [2017-09-19 20:05:48 +0200]:

> 
> 
> On 09/19/2017 02:23 PM, Cornelia Huck wrote:
> >>>>> +{
> >>>>> +    union {uint64_t fmt2; uint32_t fmt1; } idaw;      
> >>>>                                            ^
> >>>> Nit.
> >>>>    
> >> Maybe checkpatch wanted it this way. My memories are blurry.  
> > I'd just leave it like that, tbh.
> 
> Yes, if I remove the space before the } checkpatch.pl complains.
> 
> Going to keep it as is for v3.
My bad. :-/

> 
> Halil

-- 
Dong Jia Shi

Re: [Qemu-devel] [PATCH v2 4/4] s390x/css: support ccw IDA

[Dong Jia Shi]

* Halil Pasic <pasic@linux.vnet.ibm.com> [2017-09-19 14:04:03 +0200]:

I have no problem with the rest parts of the discussion in this thread.

> 
> 
> On 09/19/2017 12:57 PM, Cornelia Huck wrote:
> >>>>> +static inline int ida_read_next_idaw(CcwDataStream *cds)
> >>>>> +{
> >>>>> +    union {uint64_t fmt2; uint32_t fmt1; } idaw;    
> >>>>                                            ^
> >>>> Nit.
> >>>>  
> >> Maybe checkpatch wanted it this way. My memories are blurry.
> > 
> > I'd just leave it like that, tbh.
> > 
> >>>>> +    bool is_fmt2 = cds->flags & CDS_F_C64;
> >>>>> +    int ret;
> >>>>> +    hwaddr idaw_addr;
> >>>>> +
> >>>>> +    if (is_fmt2) {
> >>>>> +        idaw_addr = cds->cda_orig + sizeof(idaw.fmt2) * cds->at_idaw;
> >>>>> +        if (idaw_addr & 0x07) {
> >>>>> +            return -EINVAL; /* channel program check */
> >>>>> +        }
> >>>>> +        ret = address_space_rw(&address_space_memory, idaw_addr,
> >>>>> +                               MEMTXATTRS_UNSPECIFIED, (void *) &idaw.fmt2,
> >>>>> +                               sizeof(idaw.fmt2), false);
> >>>>> +        cds->cda = be64_to_cpu(idaw.fmt2);
> 
> 
> >>>>> +    } else {
> >>>>> +        idaw_addr = cds->cda_orig + sizeof(idaw.fmt1) * cds->at_idaw;
> >>>>> +        if (idaw_addr & 0x03) {    
> >>>> ?:
> >>>> (idaw_addr & 0x80000003)  
> >>> Yes.
> >>>   
> >> I will double check this. Does not seem unreasonable but
> >> double-checking is better.
> > Please let me know. I think the architecture says that the bit must be
> > zero, and that we may (...) generate a channel program check.
> > 
My fault... This is the address of an IDAW, not the content (data
address) in an IDAW. So what Halil pointed out is the right direction to
go I think.

I will review in the thread of the new version (v3).

> 
> Not exactly. The more significant bits part of the check
> depend on the ccw format. This needs to be done for both
> idaw formats. I would need to introduce a new flag, or
> access the SubchDev to do this properly.
> 
> Architecturally we also need to check the data addresses
> from which we read so we have nothing bigger than 
> (1 << 31) - 1 if we are working with format-1 idaws.
Right. This is what I actually wanted to say.

> 
> I also think we did not take proper care of proper
> maximum data address checks prior CwwDataStream which also
> depend on the ccw format (in absence of IDAW or MIDAW).
> 
> The ccw format dependent maximum address checks are (1 << 24) - 1
> and (1 << 31) - 1 respectively for format-0 and format-1 (on
> the first indirection level that is for non-IDA for the data,
> and for (M)IDA for the (M)IDAWs).
> 
> Reference:
> PoP pages 16-25 and 16-26 "Invalid IDAW or MIDAW Addre" and
> "Invalid Data Address".
> 
> How shall we proceed?
> 
> Halil
> 
> >>>>  
> >>>>> +            return -EINVAL; /* channel program check */
> >>>>> +
> >>>>> +        }
> >>>>> +        ret = address_space_rw(&address_space_memory, idaw_addr,
> >>>>> +                               MEMTXATTRS_UNSPECIFIED, (void *) &idaw.fmt1,
> >>>>> +                               sizeof(idaw.fmt1), false);
> >>>>> +        cds->cda = be64_to_cpu(idaw.fmt1);>>>>> +    }
> >>>>> +    ++(cds->at_idaw);
> >>>>> +    if (ret != MEMTX_OK) {
> >>>>> +        /* assume inaccessible address */
> >>>>> +        return -EINVAL; /* channel program check */
> >>>>> +
> >>>>> +    }
> >>>>> +    return 0;
> >>>>> +}

-- 
Dong Jia Shi