add CCW indirect data access support

include/hw/s390x/css.h +2 -0

+} CcwDataStreamOp;
+
 

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 <[email protected]>

Reviewed-by Dong Jia Shi <[email protected]>

Reviewed-by Pierre Morel<[email protected]>

---
  hw/s390x/css.c | 9 +++++----
  1 file changed, 5 insertions(+), 4 deletions(-)

hw/s390x/css.c +1 -0

index e8d2016563..6b0cd8861b 100644
@@ -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. */

hw/s390x/css.c +1 -0

+        sch->curr_status.scsw.count = ccw_dstream_residual_count(&sch->cds);
          ret = 0;
          break;
      }

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 <[email protected]>

Reviewed-by Pierre Morel<[email protected]>

---
 hw/s390x/virtio-ccw.c | 157 +++++++++++++++-----------------------------------
 1 file changed, 46 insertions(+), 111 deletions(-)

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 <[email protected]>

Reviewed-by Pierre Morel<[email protected]>

---
 hw/s390x/virtio-ccw.c | 157 +++++++++++++++-----------------------------------
 1 file changed, 46 insertions(+), 111 deletions(-)

(quoted diff) +0 -0

             ret = 0;
         }

Quoted 10 lines · On Tue, 19 Sep 2017 20:27:43 +0200 Halil Pasic <[email protected]> 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 <[email protected]>

Reviewed-by Pierre Morel<[email protected]>

---
 hw/s390x/virtio-ccw.c | 157 +++++++++++++++-----------------------------------
 1 file changed, 46 insertions(+), 111 deletions(-)

hw/s390x/virtio-ccw.c static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw) · line 388

Quoted 11 lines

hw/s390x/virtio-ccw.c +3 -3

@@ -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);

Hm, didn't you want to convert these as well? Or do you plan to do it as an add-on patch?

Quoted 2 lines

(quoted diff) +0 -0

             ret = 0;
         }

Quoted 14 lines · Cornelia Huck

Quoted 10 lines · On Tue, 19 Sep 2017 20:27:43 +0200 Halil Pasic <[email protected]> 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 <[email protected]>

Reviewed-by Pierre Morel<[email protected]>

---
 hw/s390x/virtio-ccw.c | 157 +++++++++++++++-----------------------------------
 1 file changed, 46 insertions(+), 111 deletions(-)

Quoted 15 lines

hw/s390x/virtio-ccw.c static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw) · line 388

Quoted 11 lines

hw/s390x/virtio-ccw.c +3 -3

@@ -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);  

Hm, didn't you want to convert these as well? Or do you plan to do it as an add-on patch?

I plan to do it as an add on patch.

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 <[email protected]>

Reviewed-by Pierre Morel<[email protected]>

---
  hw/s390x/virtio-ccw.c | 157 +++++++++++++++-----------------------------------
  1 file changed, 46 insertions(+), 111 deletions(-)

hw/s390x/virtio-ccw.c +0 -7

index b1976fdd19..d024f8b2d3 100644
@@ -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,

hw/s390x/virtio-ccw.c +4 -15

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

hw/s390x/virtio-ccw.c +6 -3

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

hw/s390x/virtio-ccw.c +3 -3

              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 {

hw/s390x/virtio-ccw.c +28 -50

+            le32_to_cpus(&features.features);
              if (features.index == 0) {
                  virtio_set_features(vdev,
                                      (vdev->guest_features & 0xffffffff00000000ULL) |

@@ -487,8 +445,7 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
              ret = -EFAULT;
          } 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 +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;
-                /* 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 +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);
              }

@@ -597,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;

@@ -618,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;

@@ -639,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;
              }
          }

@@ -714,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;

Quoted 25 lines · 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 <[email protected]>

Reviewed-by Pierre Morel<[email protected]>

---
  hw/s390x/virtio-ccw.c | 157 +++++++++++++++-----------------------------------

  1 file changed, 46 insertions(+), 111 deletions(-)

hw/s390x/virtio-ccw.c +0 -7

index b1976fdd19..d024f8b2d3 100644
@@ -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,

hw/s390x/virtio-ccw.c static int virtio_ccw_handle_set_vq(SubchDev *sch, CCW1 ccw, bool check_len, · line 306

Quoted 10 lines

hw/s390x/virtio-ccw.c +0 -10

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

Here again, I oversaw this.
Sorry.

hw/s390x/virtio-ccw.c static int virtio_ccw_handle_set_vq(SubchDev *sch, CCW1 ccw, bool check_len, · line 290

Quoted 1 line

hw/s390x/virtio-ccw.c +1 -0

+        ccw_dstream_read(&sch->cds, linfo);

Same as for patch 2/5:
Shouldn't you test the return value here?

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 <[email protected]>

---
 hw/s390x/css.c         | 10 ++++++++++
 include/hw/s390x/css.h |  1 +
 2 files changed, 11 insertions(+)

hw/s390x/css.c +8 -0

index 6b0cd8861b..2d37a9ddde 100644
@@ -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;
     }

hw/s390x/css.c +1 -0

                  (ccw->flags & CCW_FLAG_IDA ? CDS_F_IDA : 0);
+
     cds->count = ccw->count;
     cds->cda_orig = ccw->cda;
     ccw_dstream_rewind(cds);

include/hw/s390x/css.h +1 -0

index 078356e94c..69b374730e 100644
@@ -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;

Signature · 1 line

2.13.5

hw/s390x/css.c void ccw_dstream_init(CcwDataStream *cds, CCW1 const *ccw, ORB const *orb) · line 839

Quoted 5 lines · Halil Pasic

hw/s390x/css.c +1 -0

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

This reminds me one more question:
Calling ccw_dsteram_init() after copy_ccw_from_guest() may lead to a fmt-1 @ccw with an @orb that designates fmt-0 ccw. This sounds insane.

Quoted 10 lines · On Wed, 20 Sep 2017 15:47:51 +0800 Dong Jia Shi <[email protected]>…

hw/s390x/css.c void ccw_dstream_init(CcwDataStream *cds, CCW1 const *ccw, ORB const *orb) · line 839

Quoted 5 lines · Halil Pasic

hw/s390x/css.c +1 -0

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

This reminds me one more question:
Calling ccw_dsteram_init() after copy_ccw_from_guest() may lead to a fmt-1 @ccw with an @orb that designates fmt-0 ccw. This sounds insane.

That's just a consequence of us translating everything to format-1 ccws. A bit confusing, but no problem if we pay attention to the format bit everywhere it makes a difference.

Quoted 14 lines · Cornelia Huck

Quoted 10 lines · On Wed, 20 Sep 2017 15:47:51 +0800 Dong Jia Shi <[email protected]>…

hw/s390x/css.c void ccw_dstream_init(CcwDataStream *cds, CCW1 const *ccw, ORB const *orb) · line 839

Quoted 5 lines · Halil Pasic

hw/s390x/css.c +1 -0

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

This reminds me one more question:
Calling ccw_dsteram_init() after copy_ccw_from_guest() may lead to a fmt-1 @ccw with an @orb that designates fmt-0 ccw. This sounds insane.

Quoted 4 lines

That's just a consequence of us translating everything to format-1 ccws. A bit confusing, but no problem if we pay attention to the format bit everywhere it makes a difference.

Agree.

Halil

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 <[email protected]>

---
 hw/s390x/css.c         | 10 ++++++++++
 include/hw/s390x/css.h |  1 +
 2 files changed, 11 insertions(+)

hw/s390x/css.c +0 -0

index 6b0cd8861b..2d37a9ddde 100644

Quoted 23 lines · On Tue, 19 Sep 2017 20:27:44 +0200 Halil Pasic <[email protected]> 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 <[email protected]>

---
 hw/s390x/css.c         | 10 ++++++++++
 include/hw/s390x/css.h |  1 +
 2 files changed, 11 insertions(+)

hw/s390x/css.c +0 -0

index 6b0cd8861b..2d37a9ddde 100644
@@ -795,6 +795,11 @@ static inline int cds_check_len(CcwDataStream *cds, int len)
     return cds->flags & CDS_F_STREAM_BROKEN ? -EINVAL : len;
 }
 

hw/s390x/css.c static inline int cds_check_len(CcwDataStream *cds, int len) · line 798

Quoted 1 line

hw/s390x/css.c +1 -0

+static inline bool cds_ccw_addrs_ok(hwaddr addr, int len, bool ccw_fmt1)

cds_cda_limit_ok?

hw/s390x/css.c static inline int cds_check_len(CcwDataStream *cds, int len) · line 805

Quoted 7 lines

hw/s390x/css.c +4 -0

+{
+    return (addr + len) < (ccw_fmt1 ? (1UL << 31) : (1UL << 24));
+}
+
 static int ccw_dstream_rw_noflags(CcwDataStream *cds, void *buff, int len,
                                   CcwDataStreamOp op)
 {

Looks good.

Quoted 26 lines · Cornelia Huck

Quoted 23 lines · On Tue, 19 Sep 2017 20:27:44 +0200 Halil Pasic <[email protected]> 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 <[email protected]>

---
 hw/s390x/css.c         | 10 ++++++++++
 include/hw/s390x/css.h |  1 +
 2 files changed, 11 insertions(+)

hw/s390x/css.c +0 -0

index 6b0cd8861b..2d37a9ddde 100644
@@ -795,6 +795,11 @@ static inline int cds_check_len(CcwDataStream *cds, int len)
     return cds->flags & CDS_F_STREAM_BROKEN ? -EINVAL : len;
 }
 

Quoted 4 lines

hw/s390x/css.c static inline int cds_check_len(CcwDataStream *cds, int len) · line 798

Quoted 1 line

hw/s390x/css.c +1 -0

+static inline bool cds_ccw_addrs_ok(hwaddr addr, int len, bool ccw_fmt1)  

cds_cda_limit_ok?

I use cda to point to the 2 level in case of IDA. This is about level 1 (addressed by the ccw directly). That's why I used ccw_addrs but if you think cds_cda_limit_ok is better I can live with that.

We could also think about renaming cds->cda. Btw what does cda stand for (channel data address is my guess)?

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 <[email protected]>

Signed-off-by Cornelia Huck <[email protected]>

---
 hw/s390x/css.c | 117 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 116 insertions(+), 1 deletion(-)

hw/s390x/css.c +17 -0

index 2d37a9ddde..a3ce6d89b6 100644
@@ -827,6 +827,121 @@ 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, bool ccw_fmt1,
+                                     bool idaw_fmt_2)
+{

hw/s390x/css.c +5 -0

+
+    ret = cds_check_len(cds, len);
+    if (ret <= 0) {
+        return ret;
+    }

Quoted 39 lines · 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 <[email protected]>

Signed-off-by Cornelia Huck <[email protected]>

---
 hw/s390x/css.c | 117 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 116 insertions(+), 1 deletion(-)

hw/s390x/css.c +21 -0

index 2d37a9ddde..a3ce6d89b6 100644
@@ -827,6 +827,121 @@ 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, bool ccw_fmt1,
+                                     bool idaw_fmt_2)
+{
+    union {uint64_t fmt2; uint32_t fmt1; } idaw;
+    int ret;
+    hwaddr idaw_addr;
+

hw/s390x/css.c incr: · line 856

Quoted 6 lines

hw/s390x/css.c +6 -0

+    if (idaw_fmt_2) {
+        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,  

Ahh, just got one question here:
Do we need to considerate endianess for idaw_addr?

hw/s390x/css.c incr: · line 868

Quoted 13 lines

hw/s390x/css.c +12 -0

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

Still need to check bit 0x80000000 here I think.

hw/s390x/css.c incr: · line 874

Quoted 7 lines

hw/s390x/css.c +6 -0

+    }
+    ++(cds->at_idaw);
+    if (ret != MEMTX_OK) {
+        /* assume inaccessible address */
+        return -EINVAL; /* channel program check */
+  

Extra line.

hw/s390x/css.c incr: · line 886

Quoted 12 lines

hw/s390x/css.c +12 -0

+    }
+    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;
+    const bool idaw_fmt2 = cds->flags & CDS_F_C64;
+    bool ccw_fmt1 = cds->flags & CDS_F_FMT;  

Use 'const bool' either? Although I doubt the value of using const here. ;)

Quoted 6 lines

hw/s390x/css.c +5 -0

+
+    ret = cds_check_len(cds, len);
+    if (ret <= 0) {
+        return ret;
+    }  

[...]

Quoted 41 lines · On Wed, 20 Sep 2017 15:42:38 +0800 Dong Jia Shi <[email protected]>…

Quoted 39 lines · 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 <[email protected]>

Signed-off-by Cornelia Huck <[email protected]>

---
 hw/s390x/css.c | 117 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 116 insertions(+), 1 deletion(-)

hw/s390x/css.c +21 -0

index 2d37a9ddde..a3ce6d89b6 100644
@@ -827,6 +827,121 @@ 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, bool ccw_fmt1,
+                                     bool idaw_fmt_2)
+{
+    union {uint64_t fmt2; uint32_t fmt1; } idaw;
+    int ret;
+    hwaddr idaw_addr;
+

Quoted 8 lines

hw/s390x/css.c incr: · line 856

Quoted 6 lines

hw/s390x/css.c +6 -0

+    if (idaw_fmt_2) {
+        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,  

Ahh, just got one question here:
Do we need to considerate endianess for idaw_addr?

That is taken care of below.

And the previous version worked on my laptop via tcg ;)

Quoted 14 lines

Quoted 14 lines

hw/s390x/css.c +12 -0

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

Quoted 1 line

Still need to check bit 0x80000000 here I think.

Yes, I think this is 'must be zero' for format-1 idaws, and not covered by the ccw-format specific checks above. (Although the PoP can be a bit confusing with many similar terms...)

Quoted 9 lines

hw/s390x/css.c incr: · line 874

Quoted 7 lines

hw/s390x/css.c +6 -0

+    }
+    ++(cds->at_idaw);
+    if (ret != MEMTX_OK) {
+        /* assume inaccessible address */
+        return -EINVAL; /* channel program check */
+  

Extra line.

Quoted 14 lines

hw/s390x/css.c incr: · line 886

Quoted 12 lines

hw/s390x/css.c +12 -0

+    }
+    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;
+    const bool idaw_fmt2 = cds->flags & CDS_F_C64;
+    bool ccw_fmt1 = cds->flags & CDS_F_FMT;  

Use 'const bool' either? Although I doubt the value of using const here. ;)

Both being the same is still a good idea.

Quoted 9 lines

Quoted 6 lines

hw/s390x/css.c +5 -0

+
+    ret = cds_check_len(cds, len);
+    if (ret <= 0) {
+        return ret;
+    }  

[...]

Quoted 17 lines · Cornelia Huck

Quoted 14 lines · On Wed, 20 Sep 2017 15:42:38 +0800 Dong Jia Shi <[email protected]>…

Quoted 12 lines · Halil Pasic

hw/s390x/css.c +12 -0

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

Quoted 6 lines

Quoted 1 line

Still need to check bit 0x80000000 here I think.

Yes, I think this is 'must be zero' for format-1 idaws, and not covered by the ccw-format specific checks above. (Although the PoP can be a bit confusing with many similar terms...)

It's taken care of in ccw_dstream_rw_ida before the actual access happens. Code looks like this:

hw/s390x/css.c +4 -0

+        if (!idaw_fmt2 && (cds->cda + iter_len) >= (1ULL << 31)) {
+                ret = -EINVAL; /* channel program check */
+                goto err;
+        }

The idea was to have it similar to the non-indirect case.

Quoted 11 lines

Quoted 10 lines

hw/s390x/css.c incr: · line 886

Quoted 8 lines

hw/s390x/css.c +8 -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;
+    const bool idaw_fmt2 = cds->flags & CDS_F_C64;
+    bool ccw_fmt1 = cds->flags & CDS_F_FMT;    

Use 'const bool' either? Although I doubt the value of using const here. ;)

Quoted 2 lines

Both being the same is still a good idea.

Yeah. For which one should I go (with const or without)?

Quoted 32 lines · On Wed, 20 Sep 2017 13:13:01 +0200 Halil Pasic <[email protected]> wrote:

Quoted 17 lines · Cornelia Huck

Quoted 14 lines · On Wed, 20 Sep 2017 15:42:38 +0800 Dong Jia Shi <[email protected]>…

Quoted 12 lines · Halil Pasic

hw/s390x/css.c +12 -0

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

Quoted 6 lines

Quoted 1 line

Still need to check bit 0x80000000 here I think.

Yes, I think this is 'must be zero' for format-1 idaws, and not covered by the ccw-format specific checks above. (Although the PoP can be a bit confusing with many similar terms...)

It's taken care of in ccw_dstream_rw_ida before the actual access happens. Code looks like this:

hw/s390x/css.c +4 -0

+        if (!idaw_fmt2 && (cds->cda + iter_len) >= (1ULL << 31)) {
+                ret = -EINVAL; /* channel program check */
+                goto err;
+        }

Quoted 1 line

The idea was to have it similar to the non-indirect case.

<looks at patch again>

Ah, I was simply looking for the wrong pattern. Looks correct.

Quoted 11 lines

Quoted 11 lines

Quoted 10 lines

hw/s390x/css.c incr: · line 886

Quoted 8 lines

hw/s390x/css.c +8 -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;
+    const bool idaw_fmt2 = cds->flags & CDS_F_C64;
+    bool ccw_fmt1 = cds->flags & CDS_F_FMT;    

Use 'const bool' either? Although I doubt the value of using const here. ;)

Quoted 4 lines

Quoted 2 lines

Both being the same is still a good idea.

Yeah. For which one should I go (with const or without)?

For the one you prefer :) (I'm not sure if the const adds value here.)

Quoted 35 lines · Cornelia Huck

Quoted 32 lines · On Wed, 20 Sep 2017 13:13:01 +0200 Halil Pasic <[email protected]> wrote:

Quoted 17 lines · Cornelia Huck

Quoted 14 lines · On Wed, 20 Sep 2017 15:42:38 +0800 Dong Jia Shi <[email protected]>…

Quoted 12 lines · Halil Pasic

hw/s390x/css.c +12 -0

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

Quoted 6 lines

Quoted 1 line

Still need to check bit 0x80000000 here I think.

Yes, I think this is 'must be zero' for format-1 idaws, and not covered by the ccw-format specific checks above. (Although the PoP can be a bit confusing with many similar terms...)

It's taken care of in ccw_dstream_rw_ida before the actual access happens. Code looks like this:

hw/s390x/css.c +4 -0

+        if (!idaw_fmt2 && (cds->cda + iter_len) >= (1ULL << 31)) {
+                ret = -EINVAL; /* channel program check */
+                goto err;
+        }

Quoted 7 lines

Quoted 1 line

The idea was to have it similar to the non-indirect case.

<looks at patch again>

Ah, I was simply looking for the wrong pattern. Looks correct.

Thinking about this some more. Since in case of IDA we are guaranteed to never cross a block boundary with a single IDAW we won't ever cross block boundary. So we can do the check in ida_read_next_idaw by checking bit 0x80000000 on the ccw->cda. So we could keep idaw_fmt2 and ccw_fmt1 local to ida_read_next_idaw and save one goto err. I think that would look a bit nicer than what I have here in v3. Agree?

Quoted 12 lines

Quoted 12 lines

Quoted 12 lines

Quoted 11 lines

hw/s390x/css.c incr: · line 886

Quoted 9 lines

hw/s390x/css.c +8 -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;
+    const bool idaw_fmt2 = cds->flags & CDS_F_C64;
+    bool ccw_fmt1 = cds->flags & CDS_F_FMT;    

Use 'const bool' either? Although I doubt the value of using const here. ;)

Quoted 7 lines

Quoted 4 lines

Quoted 2 lines

Both being the same is still a good idea.

Yeah. For which one should I go (with const or without)?

For the one you prefer :) (I'm not sure if the const adds value here.)

I think we generally don't care about const-ness in such situations, so I think I won't use consts.

I intend to fix the issues we have found and do a v4 tomorrow, unless somebody screams -- could do it today but I would like to give Dong Jia an opportunity to react.

On the other hand waiting more that that will IMHO do us no favor either (I think of our storage/memory hierarchy).

Regards,
Halil

Quoted 6 lines · Halil Pasic

Thinking about this some more. Since in case of IDA we are guaranteed to never cross a block boundary with a single IDAW we won't ever cross block boundary. So we can do the check in ida_read_next_idaw by checking bit 0x80000000 on the ccw->cda. So we could keep idaw_fmt2 and ccw_fmt1 local to ida_read_next_idaw and save one goto err. I think that would look a bit nicer than what I have here in v3. Agree?

Agree. That would also do the check in the first place. Sounds better.

Quoted 53 lines · Cornelia Huck

Quoted 49 lines · On Wed, 20 Sep 2017 15:42:38 +0800 Dong Jia Shi <[email protected]>…

Quoted 35 lines · 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 <[email protected]>

Signed-off-by Cornelia Huck <[email protected]>

---
 hw/s390x/css.c | 117 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 116 insertions(+), 1 deletion(-)

hw/s390x/css.c +17 -0

index 2d37a9ddde..a3ce6d89b6 100644
@@ -827,6 +827,121 @@ 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, bool ccw_fmt1,
+                                     bool idaw_fmt_2)
+{

hw/s390x/css.c incr: · line 856

Quoted 10 lines

hw/s390x/css.c +10 -0

+    union {uint64_t fmt2; uint32_t fmt1; } idaw;
+    int ret;
+    hwaddr idaw_addr;
+
+    if (idaw_fmt_2) {
+        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,  

Ahh, just got one question here:
Do we need to considerate endianess for idaw_addr?

Quoted 3 lines

That is taken care of below.

And the previous version worked on my laptop via tcg ;)

Nod.

Quoted 15 lines

Quoted 15 lines

Quoted 15 lines

hw/s390x/css.c +12 -0

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

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 <[email protected]>

Signed-off-by Cornelia Huck <[email protected]>

---
 hw/s390x/css.c | 117 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 116 insertions(+), 1 deletion(-)

hw/s390x/css.c +14 -0

index 2d37a9ddde..a3ce6d89b6 100644
@@ -827,6 +827,121 @@ 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;
+}
+

hw/s390x/css.c +24 -0

+            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);
+    }
+    ++(cds->at_idaw);
+    if (ret != MEMTX_OK) {
+        /* assume inaccessible address */
+        return -EINVAL; /* channel program check */
+
+    }
+    return 0;
+}

Quoted 6 lines · On Tue, 19 Sep 2017 20:27:45 +0200 Halil Pasic <[email protected]> wrote:

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 <[email protected]>

Signed-off-by Cornelia Huck <[email protected]>

My s-o-b is a bit preliminary ;)

Quoted 31 lines

---
 hw/s390x/css.c | 117 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 116 insertions(+), 1 deletion(-)

hw/s390x/css.c +19 -0

index 2d37a9ddde..a3ce6d89b6 100644
@@ -827,6 +827,121 @@ 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, bool ccw_fmt1,
+                                     bool idaw_fmt_2)
+{
+    union {uint64_t fmt2; uint32_t fmt1; } idaw;
+    int ret;

hw/s390x/css.c incr: · line 853

Quoted 5 lines

hw/s390x/css.c +5 -0

+    hwaddr idaw_addr;
+
+    if (idaw_fmt_2) {
+        idaw_addr = cds->cda_orig + sizeof(idaw.fmt2) * cds->at_idaw;
+        if (idaw_addr & 0x07 && cds_ccw_addrs_ok(idaw_addr, 0, ccw_fmt1)) {

So, what is supposed to happen if the idaw_addr is not aligned correctly _and_ is violating the limits?

hw/s390x/css.c incr: · line 862

Quoted 10 lines

hw/s390x/css.c +9 -0

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

Quoted 15 lines

hw/s390x/css.c +15 -0

+            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;
+}