add CCW indirect data access support

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

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

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

hw/s390x/virtio-ccw.c +43 -105

index ff1bb1534c..e6fc25ebf4 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,
-                                            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);

Quoted 10 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]>

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

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

Reviewed-by Pierre Morel<[email protected]>

Quoted 11 lines · On Fri, 22 Sep 2017 17:39:35 +0200 Pierre Morel <[email protected]>…

Quoted 10 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]>

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

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

Quoted 5 lines

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."

Quoted 4 lines

Reviewed-by Pierre Morel<[email protected]>

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 +10 -0

index e0d989829f..cd5580ebb8 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;
      }

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

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

index 078356e94c..69b374730e 100644

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 +10 -0

index e0d989829f..cd5580ebb8 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;
     }

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

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

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

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

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.

Quoted 14 lines · On Thu, 21 Sep 2017 20:08:36 +0200 Halil Pasic <[email protected]> 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.

Quoted 4 lines

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

Quoted 25 lines · Cornelia Huck

Quoted 14 lines · On Thu, 21 Sep 2017 20:08:36 +0200 Halil Pasic <[email protected]> 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.

Quoted 4 lines

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, ...)

Quoted 2 lines

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?

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.