All of lore.kernel.org
 help / color / mirror / Atom feed
* [Qemu-devel] [PATCH v4 0/5] add CCW indirect data access support
@ 2017-09-21 18:08 Halil Pasic
  2017-09-21 18:08 ` [Qemu-devel] [PATCH v4 1/5] s390x/css: introduce css data stream Halil Pasic
                   ` (6 more replies)
  0 siblings, 7 replies; 16+ messages in thread
From: Halil Pasic @ 2017-09-21 18:08 UTC (permalink / raw)
  To: Cornelia Huck; +Cc: Dong Jia Shi, Pierre Morel, qemu-devel, 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.

The focus of this patch set is introducing IDA support. There seems to be
a potential for further  improvements based on the introduced
infrastructure, but such improvements are intended to be discusses
separately and realized as patches on top of this series.

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' (see [1]) or use the stuff form v1.  

[1] https://lists.nongnu.org/archive/html/qemu-devel/2017-09/msg03489.html

Changelog
---------

v3 --> v4:
* fixed the first level address limit check in patch #5
* refactored second level address limit check in patch #5
* nits in patch #5
* rebased, otherwise patches #1-#4 unchanged
* added an r-b for patch #3
v2 --> v3:
* added maximum data address checking (see patch #4) (Dong Jia)
  To not mix converting to the new infrastructure and changing
  behavior, this is done after the conversion. For IDA the same
  (on both IDAL and data  level) is now a part of the respective
  patch (was missing in v2).
* even less nits, and improved aesthetics (mostly Dong Jia)
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 (5):
  s390x/css: introduce css data stream
  s390x/css: use ccw data stream
  virtio-ccw: use ccw data stream
  390x/css: introduce maximum data address checking
  s390x/css: support ccw IDA

 hw/s390x/css.c         | 186 +++++++++++++++++++++++++++++++++++++++++++++++--
 hw/s390x/virtio-ccw.c  | 155 ++++++++++++-----------------------------
 include/hw/s390x/css.h |  68 ++++++++++++++++++
 3 files changed, 296 insertions(+), 113 deletions(-)

-- 
2.13.5

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [Qemu-devel] [PATCH v4 1/5] s390x/css: introduce css data stream
  2017-09-21 18:08 [Qemu-devel] [PATCH v4 0/5] add CCW indirect data access support Halil Pasic
@ 2017-09-21 18:08 ` Halil Pasic
  2017-09-21 18:08 ` [Qemu-devel] [PATCH v4 2/5] s390x/css: use ccw " Halil Pasic
                   ` (5 subsequent siblings)
  6 siblings, 0 replies; 16+ messages in thread
From: Halil Pasic @ 2017-09-21 18:08 UTC (permalink / raw)
  To: Cornelia Huck; +Cc: Dong Jia Shi, Pierre Morel, qemu-devel, 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>
Reviewed-by: Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com>
Reviewed-by: Pierre Morel<pmorel@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 75d4f301fb..d305d6d37d 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..078356e94c 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, /* read, false when used as is_write */
+    CDS_OP_W = 1, /* write, true when used as is_write */
+    CDS_OP_A = 2  /* advance, should not be used as is_write */
+} 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

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [Qemu-devel] [PATCH v4 2/5] s390x/css: use ccw data stream
  2017-09-21 18:08 [Qemu-devel] [PATCH v4 0/5] add CCW indirect data access support Halil Pasic
  2017-09-21 18:08 ` [Qemu-devel] [PATCH v4 1/5] s390x/css: introduce css data stream Halil Pasic
@ 2017-09-21 18:08 ` Halil Pasic
  2017-09-21 18:08 ` [Qemu-devel] [PATCH v4 3/5] virtio-ccw: " Halil Pasic
                   ` (4 subsequent siblings)
  6 siblings, 0 replies; 16+ messages in thread
From: Halil Pasic @ 2017-09-21 18:08 UTC (permalink / raw)
  To: Cornelia Huck; +Cc: Dong Jia Shi, Pierre Morel, qemu-devel, 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>
Reviewed-by: Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com>
Reviewed-by: Pierre Morel<pmorel@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 d305d6d37d..e0d989829f 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

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [Qemu-devel] [PATCH v4 3/5] virtio-ccw: use ccw data stream
  2017-09-21 18:08 [Qemu-devel] [PATCH v4 0/5] add CCW indirect data access support Halil Pasic
  2017-09-21 18:08 ` [Qemu-devel] [PATCH v4 1/5] s390x/css: introduce css data stream Halil Pasic
  2017-09-21 18:08 ` [Qemu-devel] [PATCH v4 2/5] s390x/css: use ccw " Halil Pasic
@ 2017-09-21 18:08 ` Halil Pasic
  2017-09-22 15:39   ` Pierre Morel
  2017-09-21 18:08 ` [Qemu-devel] [PATCH v4 4/5] 390x/css: introduce maximum data address checking Halil Pasic
                   ` (3 subsequent siblings)
  6 siblings, 1 reply; 16+ messages in thread
From: Halil Pasic @ 2017-09-21 18:08 UTC (permalink / raw)
  To: Cornelia Huck; +Cc: Dong Jia Shi, Pierre Morel, qemu-devel, 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>
Reviewed-by: Pierre Morel<pmorel@linux.vnet.ibm.com>
Reviewed-by: Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com>
---
 hw/s390x/virtio-ccw.c | 155 +++++++++++++++-----------------------------------
 1 file changed, 46 insertions(+), 109 deletions(-)

diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
index ff1bb1534c..e6fc25ebf4 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) |
@@ -487,7 +445,7 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
             ret = -EFAULT;
         } else {
             virtio_bus_get_vdev_config(&dev->bus, vdev->config);
-            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;
         }
@@ -500,20 +458,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;
-                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;
@@ -551,8 +502,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);
             }
@@ -595,8 +545,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;
@@ -616,8 +566,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;
@@ -637,67 +587,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;
             }
         }
@@ -712,13 +653,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

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [Qemu-devel] [PATCH v4 4/5] 390x/css: introduce maximum data address checking
  2017-09-21 18:08 [Qemu-devel] [PATCH v4 0/5] add CCW indirect data access support Halil Pasic
                   ` (2 preceding siblings ...)
  2017-09-21 18:08 ` [Qemu-devel] [PATCH v4 3/5] virtio-ccw: " Halil Pasic
@ 2017-09-21 18:08 ` Halil Pasic
  2017-09-22 12:35   ` Pierre Morel
  2017-09-25  3:05   ` Dong Jia Shi
  2017-09-21 18:08 ` [Qemu-devel] [PATCH v4 5/5] s390x/css: support ccw IDA Halil Pasic
                   ` (2 subsequent siblings)
  6 siblings, 2 replies; 16+ messages in thread
From: Halil Pasic @ 2017-09-21 18:08 UTC (permalink / raw)
  To: Cornelia Huck; +Cc: Dong Jia Shi, Pierre Morel, qemu-devel, Halil Pasic

The architecture mandates the addresses to be accessed on the first
indirection level (that is, the data addresses without IDA, and the
(M)IDAW addresses with (M)IDA) to be checked against an CCW format
dependent limit maximum address.  If a violation is detected, the storage
access is not to be performed and a channel program check needs to be
generated. As of today, we fail to do this check.

Let us stick even closer to the architecture specification.

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

diff --git a/hw/s390x/css.c b/hw/s390x/css.c
index e0d989829f..cd5580ebb8 100644
--- a/hw/s390x/css.c
+++ b/hw/s390x/css.c
@@ -795,6 +795,11 @@ static inline int cds_check_len(CcwDataStream *cds, int len)
     return cds->flags & CDS_F_STREAM_BROKEN ? -EINVAL : len;
 }
 
+static inline bool cds_ccw_addrs_ok(hwaddr addr, int len, bool ccw_fmt1)
+{
+    return (addr + len) < (ccw_fmt1 ? (1UL << 31) : (1UL << 24));
+}
+
 static int ccw_dstream_rw_noflags(CcwDataStream *cds, void *buff, int len,
                                   CcwDataStreamOp op)
 {
@@ -804,6 +809,9 @@ static int ccw_dstream_rw_noflags(CcwDataStream *cds, void *buff, int len,
     if (ret <= 0) {
         return ret;
     }
+    if (!cds_ccw_addrs_ok(cds->cda, len, cds->flags & CDS_F_FMT)) {
+        return -EINVAL; /* channel program check */
+    }
     if (op == CDS_OP_A) {
         goto incr;
     }
@@ -828,7 +836,9 @@ void ccw_dstream_init(CcwDataStream *cds, CCW1 const *ccw, ORB const *orb)
     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) |
+                 (orb->ctrl0 & ORB_CTRL0_MASK_FMT ? CDS_F_FMT : 0) |
                  (ccw->flags & CCW_FLAG_IDA ? CDS_F_IDA : 0);
+
     cds->count = ccw->count;
     cds->cda_orig = ccw->cda;
     ccw_dstream_rewind(cds);
diff --git a/include/hw/s390x/css.h b/include/hw/s390x/css.h
index 078356e94c..69b374730e 100644
--- a/include/hw/s390x/css.h
+++ b/include/hw/s390x/css.h
@@ -87,6 +87,7 @@ typedef struct CcwDataStream {
 #define CDS_F_MIDA  0x02
 #define CDS_F_I2K   0x04
 #define CDS_F_C64   0x08
+#define CDS_F_FMT   0x10 /* CCW format-1 */
 #define CDS_F_STREAM_BROKEN  0x80
     uint8_t flags;
     uint8_t at_idaw;
-- 
2.13.5

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [Qemu-devel] [PATCH v4 5/5] s390x/css: support ccw IDA
  2017-09-21 18:08 [Qemu-devel] [PATCH v4 0/5] add CCW indirect data access support Halil Pasic
                   ` (3 preceding siblings ...)
  2017-09-21 18:08 ` [Qemu-devel] [PATCH v4 4/5] 390x/css: introduce maximum data address checking Halil Pasic
@ 2017-09-21 18:08 ` Halil Pasic
  2017-09-25  3:14   ` Dong Jia Shi
  2017-09-26 10:18 ` [Qemu-devel] [PATCH v4 0/5] add CCW indirect data access support Cornelia Huck
  2017-09-26 12:59 ` Cornelia Huck
  6 siblings, 1 reply; 16+ messages in thread
From: Halil Pasic @ 2017-09-21 18:08 UTC (permalink / raw)
  To: Cornelia Huck; +Cc: Dong Jia Shi, Pierre Morel, qemu-devel, Halil Pasic

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

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

diff --git a/hw/s390x/css.c b/hw/s390x/css.c
index cd5580ebb8..9079d274b8 100644
--- a/hw/s390x/css.c
+++ b/hw/s390x/css.c
@@ -827,6 +827,118 @@ incr:
     return 0;
 }
 
+/* returns values between 1 and bsz, where bsz 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)
+{
+    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;
+    int ret;
+    hwaddr idaw_addr;
+    bool idaw_fmt2 = cds->flags & CDS_F_C64;
+    bool ccw_fmt1 = cds->flags & CDS_F_FMT;
+
+    if (idaw_fmt2) {
+        idaw_addr = cds->cda_orig + sizeof(idaw.fmt2) * cds->at_idaw;
+        if (idaw_addr & 0x07 || !cds_ccw_addrs_ok(idaw_addr, 0, ccw_fmt1)) {
+            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 || !cds_ccw_addrs_ok(idaw_addr, 0, ccw_fmt1)) {
+            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);
+        if (cds->cda & 0x80000000) {
+            return -EINVAL; /* channel program check */
+        }
+    }
+    ++(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)
 {
     /*
@@ -845,7 +957,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

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: [Qemu-devel] [PATCH v4 4/5] 390x/css: introduce maximum data address checking
  2017-09-21 18:08 ` [Qemu-devel] [PATCH v4 4/5] 390x/css: introduce maximum data address checking Halil Pasic
@ 2017-09-22 12:35   ` Pierre Morel
  2017-09-25  3:05   ` Dong Jia Shi
  1 sibling, 0 replies; 16+ messages in thread
From: Pierre Morel @ 2017-09-22 12:35 UTC (permalink / raw)
  To: Halil Pasic, Cornelia Huck; +Cc: Dong Jia Shi, qemu-devel

On 21/09/2017 20:08, Halil Pasic wrote:
> The architecture mandates the addresses to be accessed on the first
> indirection level (that is, the data addresses without IDA, and the
> (M)IDAW addresses with (M)IDA) to be checked against an CCW format
> dependent limit maximum address.  If a violation is detected, the storage
> access is not to be performed and a channel program check needs to be
> generated. As of today, we fail to do this check.
> 
> Let us stick even closer to the architecture specification.
> 
> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
> ---
>   hw/s390x/css.c         | 10 ++++++++++
>   include/hw/s390x/css.h |  1 +
>   2 files changed, 11 insertions(+)
> 
> diff --git a/hw/s390x/css.c b/hw/s390x/css.c
> index e0d989829f..cd5580ebb8 100644
> --- a/hw/s390x/css.c
> +++ b/hw/s390x/css.c
> @@ -795,6 +795,11 @@ static inline int cds_check_len(CcwDataStream *cds, int len)
>       return cds->flags & CDS_F_STREAM_BROKEN ? -EINVAL : len;
>   }
> 
> +static inline bool cds_ccw_addrs_ok(hwaddr addr, int len, bool ccw_fmt1)
> +{
> +    return (addr + len) < (ccw_fmt1 ? (1UL << 31) : (1UL << 24));
> +}
> +
>   static int ccw_dstream_rw_noflags(CcwDataStream *cds, void *buff, int len,
>                                     CcwDataStreamOp op)
>   {
> @@ -804,6 +809,9 @@ static int ccw_dstream_rw_noflags(CcwDataStream *cds, void *buff, int len,
>       if (ret <= 0) {
>           return ret;
>       }
> +    if (!cds_ccw_addrs_ok(cds->cda, len, cds->flags & CDS_F_FMT)) {
> +        return -EINVAL; /* channel program check */
> +    }
>       if (op == CDS_OP_A) {
>           goto incr;
>       }
> @@ -828,7 +836,9 @@ void ccw_dstream_init(CcwDataStream *cds, CCW1 const *ccw, ORB const *orb)
>       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) |
> +                 (orb->ctrl0 & ORB_CTRL0_MASK_FMT ? CDS_F_FMT : 0) |
>                    (ccw->flags & CCW_FLAG_IDA ? CDS_F_IDA : 0);
> +
>       cds->count = ccw->count;
>       cds->cda_orig = ccw->cda;
>       ccw_dstream_rewind(cds);
> diff --git a/include/hw/s390x/css.h b/include/hw/s390x/css.h
> index 078356e94c..69b374730e 100644
> --- a/include/hw/s390x/css.h
> +++ b/include/hw/s390x/css.h
> @@ -87,6 +87,7 @@ typedef struct CcwDataStream {
>   #define CDS_F_MIDA  0x02
>   #define CDS_F_I2K   0x04
>   #define CDS_F_C64   0x08
> +#define CDS_F_FMT   0x10 /* CCW format-1 */
>   #define CDS_F_STREAM_BROKEN  0x80
>       uint8_t flags;
>       uint8_t at_idaw;
> 

LGTM

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

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [Qemu-devel] [PATCH v4 3/5] virtio-ccw: use ccw data stream
  2017-09-21 18:08 ` [Qemu-devel] [PATCH v4 3/5] virtio-ccw: " Halil Pasic
@ 2017-09-22 15:39   ` Pierre Morel
  2017-09-26 10:10     ` Cornelia Huck
  0 siblings, 1 reply; 16+ messages in thread
From: Pierre Morel @ 2017-09-22 15:39 UTC (permalink / raw)
  To: Halil Pasic, Cornelia Huck; +Cc: Dong Jia Shi, qemu-devel

On 21/09/2017 20:08, 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>
> Reviewed-by: Pierre Morel<pmorel@linux.vnet.ibm.com>
> Reviewed-by: Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com>
> ---
>   hw/s390x/virtio-ccw.c | 155 +++++++++++++++-----------------------------------
>   1 file changed, 46 insertions(+), 109 deletions(-)
> 
> diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
> index ff1bb1534c..e6fc25ebf4 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) |
> @@ -487,7 +445,7 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
>               ret = -EFAULT;
>           } else {
>               virtio_bus_get_vdev_config(&dev->bus, vdev->config);
> -            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;
>           }
> @@ -500,20 +458,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;
> -                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;
> @@ -551,8 +502,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);
>               }
> @@ -595,8 +545,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;
> @@ -616,8 +566,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;
> @@ -637,67 +587,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;
>               }
>           }
> @@ -712,13 +653,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;
> 

LGTM.
Not testing the return value for ccw_dstream_read/write_buf() bother me 
but I can understand that you will modify this later.
May be a comment?
Not a strong opinion.

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


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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [Qemu-devel] [PATCH v4 4/5] 390x/css: introduce maximum data address checking
  2017-09-21 18:08 ` [Qemu-devel] [PATCH v4 4/5] 390x/css: introduce maximum data address checking Halil Pasic
  2017-09-22 12:35   ` Pierre Morel
@ 2017-09-25  3:05   ` Dong Jia Shi
  1 sibling, 0 replies; 16+ messages in thread
From: Dong Jia Shi @ 2017-09-25  3:05 UTC (permalink / raw)
  To: Halil Pasic; +Cc: Cornelia Huck, Dong Jia Shi, Pierre Morel, qemu-devel

* Halil Pasic <pasic@linux.vnet.ibm.com> [2017-09-21 20:08:40 +0200]:

> The architecture mandates the addresses to be accessed on the first
> indirection level (that is, the data addresses without IDA, and the
> (M)IDAW addresses with (M)IDA) to be checked against an CCW format
> dependent limit maximum address.  If a violation is detected, the storage
> access is not to be performed and a channel program check needs to be
> generated. As of today, we fail to do this check.
> 
> Let us stick even closer to the architecture specification.
> 
> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
> ---
>  hw/s390x/css.c         | 10 ++++++++++
>  include/hw/s390x/css.h |  1 +
>  2 files changed, 11 insertions(+)
> 
> diff --git a/hw/s390x/css.c b/hw/s390x/css.c
> index e0d989829f..cd5580ebb8 100644
> --- a/hw/s390x/css.c
> +++ b/hw/s390x/css.c
> @@ -795,6 +795,11 @@ static inline int cds_check_len(CcwDataStream *cds, int len)
>      return cds->flags & CDS_F_STREAM_BROKEN ? -EINVAL : len;
>  }
> 
> +static inline bool cds_ccw_addrs_ok(hwaddr addr, int len, bool ccw_fmt1)
> +{
> +    return (addr + len) < (ccw_fmt1 ? (1UL << 31) : (1UL << 24));
> +}
> +
>  static int ccw_dstream_rw_noflags(CcwDataStream *cds, void *buff, int len,
>                                    CcwDataStreamOp op)
>  {
> @@ -804,6 +809,9 @@ static int ccw_dstream_rw_noflags(CcwDataStream *cds, void *buff, int len,
>      if (ret <= 0) {
>          return ret;
>      }
> +    if (!cds_ccw_addrs_ok(cds->cda, len, cds->flags & CDS_F_FMT)) {
> +        return -EINVAL; /* channel program check */
> +    }
>      if (op == CDS_OP_A) {
>          goto incr;
>      }
> @@ -828,7 +836,9 @@ void ccw_dstream_init(CcwDataStream *cds, CCW1 const *ccw, ORB const *orb)
>      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) |
> +                 (orb->ctrl0 & ORB_CTRL0_MASK_FMT ? CDS_F_FMT : 0) |
>                   (ccw->flags & CCW_FLAG_IDA ? CDS_F_IDA : 0);
> +
>      cds->count = ccw->count;
>      cds->cda_orig = ccw->cda;
>      ccw_dstream_rewind(cds);
> diff --git a/include/hw/s390x/css.h b/include/hw/s390x/css.h
> index 078356e94c..69b374730e 100644
> --- a/include/hw/s390x/css.h
> +++ b/include/hw/s390x/css.h
> @@ -87,6 +87,7 @@ typedef struct CcwDataStream {
>  #define CDS_F_MIDA  0x02
>  #define CDS_F_I2K   0x04
>  #define CDS_F_C64   0x08
> +#define CDS_F_FMT   0x10 /* CCW format-1 */
>  #define CDS_F_STREAM_BROKEN  0x80
>      uint8_t flags;
>      uint8_t at_idaw;
> -- 
> 2.13.5
> 

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

-- 
Dong Jia Shi

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [Qemu-devel] [PATCH v4 5/5] s390x/css: support ccw IDA
  2017-09-21 18:08 ` [Qemu-devel] [PATCH v4 5/5] s390x/css: support ccw IDA Halil Pasic
@ 2017-09-25  3:14   ` Dong Jia Shi
  0 siblings, 0 replies; 16+ messages in thread
From: Dong Jia Shi @ 2017-09-25  3:14 UTC (permalink / raw)
  To: Halil Pasic; +Cc: Cornelia Huck, Dong Jia Shi, Pierre Morel, qemu-devel

* Halil Pasic <pasic@linux.vnet.ibm.com> [2017-09-21 20:08:41 +0200]:

> Let's add indirect data addressing support for our virtual channel
> subsystem. This implementation does not bother with any kind of
> prefetching. We simply step through the IDAL on demand.
> 
> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
> ---
>  hw/s390x/css.c | 114 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 113 insertions(+), 1 deletion(-)
> 

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

[...]

-- 
Dong Jia Shi

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [Qemu-devel] [PATCH v4 3/5] virtio-ccw: use ccw data stream
  2017-09-22 15:39   ` Pierre Morel
@ 2017-09-26 10:10     ` Cornelia Huck
  2017-09-26 10:14       ` Halil Pasic
  0 siblings, 1 reply; 16+ messages in thread
From: Cornelia Huck @ 2017-09-26 10:10 UTC (permalink / raw)
  To: Pierre Morel; +Cc: Halil Pasic, Dong Jia Shi, qemu-devel

On Fri, 22 Sep 2017 17:39:35 +0200
Pierre Morel <pmorel@linux.vnet.ibm.com> wrote:

> On 21/09/2017 20:08, 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>
> > Reviewed-by: Pierre Morel<pmorel@linux.vnet.ibm.com>
> > Reviewed-by: Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com>
> > ---
> >   hw/s390x/virtio-ccw.c | 155 +++++++++++++++-----------------------------------
> >   1 file changed, 46 insertions(+), 109 deletions(-)

> LGTM.
> Not testing the return value for ccw_dstream_read/write_buf() bother me 
> but I can understand that you will modify this later.
> May be a comment?
> Not a strong opinion.

I can add "Note that checking the return code for ccw_dstream_* will be
done in a follow-on patch."

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [Qemu-devel] [PATCH v4 3/5] virtio-ccw: use ccw data stream
  2017-09-26 10:10     ` Cornelia Huck
@ 2017-09-26 10:14       ` Halil Pasic
  0 siblings, 0 replies; 16+ messages in thread
From: Halil Pasic @ 2017-09-26 10:14 UTC (permalink / raw)
  To: Cornelia Huck, Pierre Morel; +Cc: Dong Jia Shi, qemu-devel



On 09/26/2017 12:10 PM, Cornelia Huck wrote:
> On Fri, 22 Sep 2017 17:39:35 +0200
> Pierre Morel <pmorel@linux.vnet.ibm.com> wrote:
> 
>> On 21/09/2017 20:08, 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>
>>> Reviewed-by: Pierre Morel<pmorel@linux.vnet.ibm.com>
>>> Reviewed-by: Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com>
>>> ---
>>>   hw/s390x/virtio-ccw.c | 155 +++++++++++++++-----------------------------------
>>>   1 file changed, 46 insertions(+), 109 deletions(-)
> 
>> LGTM.
>> Not testing the return value for ccw_dstream_read/write_buf() bother me 
>> but I can understand that you will modify this later.
>> May be a comment?
>> Not a strong opinion.
> 
> I can add "Note that checking the return code for ccw_dstream_* will be
> done in a follow-on patch."
> 

I'm fine with that.

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [Qemu-devel] [PATCH v4 0/5] add CCW indirect data access support
  2017-09-21 18:08 [Qemu-devel] [PATCH v4 0/5] add CCW indirect data access support Halil Pasic
                   ` (4 preceding siblings ...)
  2017-09-21 18:08 ` [Qemu-devel] [PATCH v4 5/5] s390x/css: support ccw IDA Halil Pasic
@ 2017-09-26 10:18 ` Cornelia Huck
  2017-09-26 10:45   ` Halil Pasic
  2017-09-26 12:59 ` Cornelia Huck
  6 siblings, 1 reply; 16+ messages in thread
From: Cornelia Huck @ 2017-09-26 10:18 UTC (permalink / raw)
  To: Halil Pasic; +Cc: Dong Jia Shi, Pierre Morel, qemu-devel

On Thu, 21 Sep 2017 20:08:36 +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.
> 
> The focus of this patch set is introducing IDA support. There seems to be
> a potential for further  improvements based on the introduced
> infrastructure, but such improvements are intended to be discusses
> separately and realized as patches on top of this series.

Hm, do you have a list of what you want to do as follow-on patches?
(Checking return codes, ...)

It's easy to lose track of all this :)

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [Qemu-devel] [PATCH v4 0/5] add CCW indirect data access support
  2017-09-26 10:18 ` [Qemu-devel] [PATCH v4 0/5] add CCW indirect data access support Cornelia Huck
@ 2017-09-26 10:45   ` Halil Pasic
  2017-09-26 11:19     ` Cornelia Huck
  0 siblings, 1 reply; 16+ messages in thread
From: Halil Pasic @ 2017-09-26 10:45 UTC (permalink / raw)
  To: qemu-devel



On 09/26/2017 12:18 PM, Cornelia Huck wrote:
> On Thu, 21 Sep 2017 20:08:36 +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.
>>
>> The focus of this patch set is introducing IDA support. There seems to be
>> a potential for further  improvements based on the introduced
>> infrastructure, but such improvements are intended to be discusses
>> separately and realized as patches on top of this series.
> 
> Hm, do you have a list of what you want to do as follow-on patches?
> (Checking return codes, ...)
> 
> It's easy to lose track of all this :)
> 

These are the stuff I had in mind:
* handling errors (aka. checking error codes) in virtio-ccw
* using residual count
* converting 3270 (patches already sent)

And then some related stuff is the error reporting and handling
rework for the IO instruction handlers and for the ccw interpretation
emulation (later affects CcwDataStream).

There is probably a lot of potential for making things prettier
in virtio-ccw.c too -- I've done some experiments but ended up changing
too many things at the same time.

Btw, do we still have open issues with this series?

Regards,
Halil


 

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [Qemu-devel] [PATCH v4 0/5] add CCW indirect data access support
  2017-09-26 10:45   ` Halil Pasic
@ 2017-09-26 11:19     ` Cornelia Huck
  0 siblings, 0 replies; 16+ messages in thread
From: Cornelia Huck @ 2017-09-26 11:19 UTC (permalink / raw)
  To: Halil Pasic; +Cc: qemu-devel, Dong Jia Shi, Pierre Morel

[Restored cc:s]

On Tue, 26 Sep 2017 12:45:11 +0200
Halil Pasic <pasic@linux.vnet.ibm.com> wrote:

> On 09/26/2017 12:18 PM, Cornelia Huck wrote:
> > On Thu, 21 Sep 2017 20:08:36 +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.
> >>
> >> The focus of this patch set is introducing IDA support. There seems to be
> >> a potential for further  improvements based on the introduced
> >> infrastructure, but such improvements are intended to be discusses
> >> separately and realized as patches on top of this series.  
> > 
> > Hm, do you have a list of what you want to do as follow-on patches?
> > (Checking return codes, ...)
> > 
> > It's easy to lose track of all this :)
> >   
> 
> These are the stuff I had in mind:
> * handling errors (aka. checking error codes) in virtio-ccw
> * using residual count
> * converting 3270 (patches already sent)

The 3270 stuff is next in line for me.

> 
> And then some related stuff is the error reporting and handling
> rework for the IO instruction handlers and for the ccw interpretation
> emulation (later affects CcwDataStream).
> 
> There is probably a lot of potential for making things prettier
> in virtio-ccw.c too -- I've done some experiments but ended up changing
> too many things at the same time.

There's always potential for such things :) Let's get the previously
identified stuff out of the door first (it's not that I lack patches to
apply anyway...)

> 
> Btw, do we still have open issues with this series?

Currently going through my acceptance testing.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [Qemu-devel] [PATCH v4 0/5] add CCW indirect data access support
  2017-09-21 18:08 [Qemu-devel] [PATCH v4 0/5] add CCW indirect data access support Halil Pasic
                   ` (5 preceding siblings ...)
  2017-09-26 10:18 ` [Qemu-devel] [PATCH v4 0/5] add CCW indirect data access support Cornelia Huck
@ 2017-09-26 12:59 ` Cornelia Huck
  6 siblings, 0 replies; 16+ messages in thread
From: Cornelia Huck @ 2017-09-26 12:59 UTC (permalink / raw)
  To: Halil Pasic; +Cc: Dong Jia Shi, Pierre Morel, qemu-devel

On Thu, 21 Sep 2017 20:08:36 +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.
> 
> The focus of this patch set is introducing IDA support. There seems to be
> a potential for further  improvements based on the introduced
> infrastructure, but such improvements are intended to be discusses
> separately and realized as patches on top of this series.

Thanks, applied.

^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2017-09-26 13:00 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-09-21 18:08 [Qemu-devel] [PATCH v4 0/5] add CCW indirect data access support Halil Pasic
2017-09-21 18:08 ` [Qemu-devel] [PATCH v4 1/5] s390x/css: introduce css data stream Halil Pasic
2017-09-21 18:08 ` [Qemu-devel] [PATCH v4 2/5] s390x/css: use ccw " Halil Pasic
2017-09-21 18:08 ` [Qemu-devel] [PATCH v4 3/5] virtio-ccw: " Halil Pasic
2017-09-22 15:39   ` Pierre Morel
2017-09-26 10:10     ` Cornelia Huck
2017-09-26 10:14       ` Halil Pasic
2017-09-21 18:08 ` [Qemu-devel] [PATCH v4 4/5] 390x/css: introduce maximum data address checking Halil Pasic
2017-09-22 12:35   ` Pierre Morel
2017-09-25  3:05   ` Dong Jia Shi
2017-09-21 18:08 ` [Qemu-devel] [PATCH v4 5/5] s390x/css: support ccw IDA Halil Pasic
2017-09-25  3:14   ` Dong Jia Shi
2017-09-26 10:18 ` [Qemu-devel] [PATCH v4 0/5] add CCW indirect data access support Cornelia Huck
2017-09-26 10:45   ` Halil Pasic
2017-09-26 11:19     ` Cornelia Huck
2017-09-26 12:59 ` Cornelia Huck

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.