add CCW indirect data access support

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

---
 hw/s390x/css.c         | 55 +++++++++++++++++++++++++++++++++++++++++
 include/hw/s390x/css.h | 67 ++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 122 insertions(+)

hw/s390x/css.c +7 -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));

hw/s390x/css.c +2 -0

+    }
+}
 
 static int css_interpret_ccw(SubchDev *sch, hwaddr ccw_addr,
                              bool suspend_allowed)

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

index 0653d3c9be..79acaf99b7 100644

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

+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: */

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

+} CcwDataStreamOp;
+

+}

* Halil Pasic <[email protected]> [2017-09-13 13:50:26 +0200]:

[...]

include/hw/s390x/css.h typedef struct CMBE { · line 81

Quoted 12 lines

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

index 0653d3c9be..79acaf99b7 100644
@@ -75,6 +75,29 @@ typedef struct CMBE {
     uint32_t reserved[7];
 } QEMU_PACKED CMBE;
 
+typedef enum CcwDataStreamOp {
+    CDS_OP_R = 0,
+    CDS_OP_W = 1,
+    CDS_OP_A = 2  

Nit:

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

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

Quoted 3 lines

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

+} CcwDataStreamOp;
+  

[...]

include/hw/s390x/css.h SubchDev *css_create_sch(CssDevId bus_id, bool is_virtual, bool squash_mcss, · line 289

Quoted 3 lines

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

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

                                     ^^

Nit.

Quoted 2 lines

+}

[...]

With or w/o responses the nit picks:

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

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

---
  hw/s390x/css.c         | 55 +++++++++++++++++++++++++++++++++++++++++
  include/hw/s390x/css.h | 67 ++++++++++++++++++++++++++++++++++++++++++++++++++
  2 files changed, 122 insertions(+)

hw/s390x/css.c +55 -0

index 901dc6a0f3..e8d2016563 100644
@@ -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)

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

index 0653d3c9be..79acaf99b7 100644
@@ -75,6 +75,29 @@ typedef struct CMBE {
      uint32_t reserved[7];
  } QEMU_PACKED CMBE;
 
+typedef enum CcwDataStreamOp {
+    CDS_OP_R = 0,
+    CDS_OP_W = 1,
+    CDS_OP_A = 2
+} CcwDataStreamOp;
+
+/** normal usage is via SuchchDev.cds instead of instantiating */
+typedef struct CcwDataStream {
+#define CDS_F_IDA   0x01
+#define CDS_F_MIDA  0x02

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

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

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

---
  hw/s390x/css.c         | 55 +++++++++++++++++++++++++++++++++++++++++
  include/hw/s390x/css.h | 67 ++++++++++++++++++++++++++++++++++++++++++++++++++
  2 files changed, 122 insertions(+)

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

index 0653d3c9be..79acaf99b7 100644
@@ -75,6 +75,29 @@ typedef struct CMBE {
      uint32_t reserved[7];
  } QEMU_PACKED CMBE;
 
+typedef enum CcwDataStreamOp {
+    CDS_OP_R = 0,
+    CDS_OP_W = 1,
+    CDS_OP_A = 2
+} CcwDataStreamOp;
+
+/** normal usage is via SuchchDev.cds instead of instantiating */
+typedef struct CcwDataStream {
+#define CDS_F_IDA   0x01
+#define CDS_F_MIDA  0x02

include/hw/s390x/css.h typedef struct CMBE { · line 97

Quoted 10 lines

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

+#define CDS_F_I2K   0x04
+#define CDS_F_C64   0x08
+#define CDS_F_STREAM_BROKEN  0x80
+    uint8_t flags;
+    uint8_t at_idaw;
+    uint16_t at_byte;
+    uint16_t count;
+    uint32_t cda_orig;
+    int (*op_handler)(struct CcwDataStream *cds, void *buff, int len,
+                      CcwDataStreamOp op);  

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

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

include/hw/s390x/css.h typedef struct CMBE { · line 100

Quoted 6 lines

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

+    hwaddr cda;
+} CcwDataStream;
+

otherwise, LGTM

Quoted 44 lines · On Tue, 19 Sep 2017 11:11:27 +0200 Pierre Morel <[email protected]>…

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

---
   hw/s390x/css.c         | 55 +++++++++++++++++++++++++++++++++++++++++
   include/hw/s390x/css.h | 67 ++++++++++++++++++++++++++++++++++++++++++++++++++
   2 files changed, 122 insertions(+)

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

index 0653d3c9be..79acaf99b7 100644
@@ -75,6 +75,29 @@ typedef struct CMBE {
       uint32_t reserved[7];
   } QEMU_PACKED CMBE;
 
+typedef enum CcwDataStreamOp {
+    CDS_OP_R = 0,
+    CDS_OP_W = 1,
+    CDS_OP_A = 2
+} CcwDataStreamOp;
+
+/** normal usage is via SuchchDev.cds instead of instantiating */
+typedef struct CcwDataStream {
+#define CDS_F_IDA   0x01
+#define CDS_F_MIDA  0x02

include/hw/s390x/css.h typedef struct CMBE { · line 97

Quoted 10 lines

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

+#define CDS_F_I2K   0x04
+#define CDS_F_C64   0x08
+#define CDS_F_STREAM_BROKEN  0x80
+    uint8_t flags;
+    uint8_t at_idaw;
+    uint16_t at_byte;
+    uint16_t count;
+    uint32_t cda_orig;
+    int (*op_handler)(struct CcwDataStream *cds, void *buff, int len,
+                      CcwDataStreamOp op);

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

Quoted 4 lines

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

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

Quoted 3 lines

Quoted 5 lines

include/hw/s390x/css.h typedef struct CMBE { · line 100

Quoted 3 lines

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

+    hwaddr cda;
+} CcwDataStream;
+

otherwise, LGTM

Is that an ack? :)

Quoted 47 lines · Cornelia Huck

Quoted 44 lines · On Tue, 19 Sep 2017 11:11:27 +0200 Pierre Morel <[email protected]>…

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

---
   hw/s390x/css.c         | 55 +++++++++++++++++++++++++++++++++++++++++ include/hw/s390x/css.h | 67 ++++++++++++++++++++++++++++++++++++++++++++++++++

   2 files changed, 122 insertions(+)

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

index 0653d3c9be..79acaf99b7 100644
@@ -75,6 +75,29 @@ typedef struct CMBE {
        uint32_t reserved[7];
    } QEMU_PACKED CMBE;
 
+typedef enum CcwDataStreamOp {
+    CDS_OP_R = 0,
+    CDS_OP_W = 1,
+    CDS_OP_A = 2
+} CcwDataStreamOp;
+
+/** normal usage is via SuchchDev.cds instead of instantiating */
+typedef struct CcwDataStream {
+#define CDS_F_IDA   0x01
+#define CDS_F_MIDA  0x02

include/hw/s390x/css.h typedef struct CMBE { · line 97

Quoted 10 lines

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

+#define CDS_F_I2K   0x04
+#define CDS_F_C64   0x08
+#define CDS_F_STREAM_BROKEN  0x80
+    uint8_t flags;
+    uint8_t at_idaw;
+    uint16_t at_byte;
+    uint16_t count;
+    uint32_t cda_orig;
+    int (*op_handler)(struct CcwDataStream *cds, void *buff, int len,
+                      CcwDataStreamOp op);

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

Quoted 5 lines

Quoted 4 lines

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

Quoted 3 lines

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

Quoted 11 lines

Quoted 8 lines

include/hw/s390x/css.h typedef struct CMBE { · line 100

Quoted 6 lines

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

+    hwaddr cda;
+} CcwDataStream;
+

otherwise, LGTM

Is that an ack? :)

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

Quoted 44 lines · On Tue, 19 Sep 2017 11:11:27 +0200 Pierre Morel <[email protected]>…

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

---
   hw/s390x/css.c         | 55 +++++++++++++++++++++++++++++++++++++++++
   include/hw/s390x/css.h | 67 ++++++++++++++++++++++++++++++++++++++++++++++++++
   2 files changed, 122 insertions(+)

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

index 0653d3c9be..79acaf99b7 100644
@@ -75,6 +75,29 @@ typedef struct CMBE {
       uint32_t reserved[7];
   } QEMU_PACKED CMBE;
 
+typedef enum CcwDataStreamOp {
+    CDS_OP_R = 0,
+    CDS_OP_W = 1,
+    CDS_OP_A = 2
+} CcwDataStreamOp;
+
+/** normal usage is via SuchchDev.cds instead of instantiating */
+typedef struct CcwDataStream {
+#define CDS_F_IDA   0x01
+#define CDS_F_MIDA  0x02

include/hw/s390x/css.h typedef struct CMBE { · line 97

Quoted 10 lines

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

+#define CDS_F_I2K   0x04
+#define CDS_F_C64   0x08
+#define CDS_F_STREAM_BROKEN  0x80
+    uint8_t flags;
+    uint8_t at_idaw;
+    uint16_t at_byte;
+    uint16_t count;
+    uint32_t cda_orig;
+    int (*op_handler)(struct CcwDataStream *cds, void *buff, int len,
+                      CcwDataStreamOp op);

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

Quoted 4 lines

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

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

Quoted 3 lines

Quoted 5 lines

include/hw/s390x/css.h typedef struct CMBE { · line 100

Quoted 3 lines

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

+    hwaddr cda;
+} CcwDataStream;
+

otherwise, LGTM

Is that an ack? :)

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

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

hw/s390x/css.c +5 -4

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

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

Signature · 1 line

2.13.5

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

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

hw/s390x/css.c +3 -2

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

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

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

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

@@ -488,7 +446,7 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
         } else {
             virtio_bus_get_vdev_config(&dev->bus, vdev->config);
             /* XXX config space endianness */
-            cpu_physical_memory_write(ccw.cda, vdev->config, len);
+            ccw_dstream_write_buf(&sch->cds, vdev->config, len);
             sch->curr_status.scsw.count = ccw.count - len;
             ret = 0;
         }

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

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

---

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

Error handling: At the moment we ignore errors reported by stream ops to keep the change minimal. An add-on patch improving on this is to be expected later.

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

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

index b1976fdd19..a9baf6f7ab 100644

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

+            ccw_dstream_read(&sch->cds, features.index);
             if (features.index == 0) {
                 if (dev->revision >= 1) {
                     /* Don't offer legacy features for modern devices. */

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

             ret = 0;
         }

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

@@ -501,21 +459,13 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
             }
         }
         len = MIN(ccw.count, vdev->config_len);
-        hw_len = len;
         if (!ccw.cda) {
             ret = -EFAULT;
         } else {

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

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

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

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

Signature · 1 line

2.13.5

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

---

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

Quoted 3 lines

Error handling: At the moment we ignore errors reported by stream ops to keep the change minimal. An add-on patch improving on this is to be expected later.

Add a TODO somewhere around the code as a reminder?

Quoted 9 lines

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

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

index b1976fdd19..a9baf6f7ab 100644

[...]

Quoted 4 lines

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

@@ -394,11 +362,8 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
         } else {
             VirtioDeviceClass *vdc = VIRTIO_DEVICE_GET_CLASS(vdev);
 

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

Quoted 6 lines

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

-            features.index = address_space_ldub(&address_space_memory,
-                                                ccw.cda
-                                                + sizeof(features.features),
-                                                MEMTXATTRS_UNSPECIFIED,
-                                                NULL);
+            ccw_dstream_advance(&sch->cds, sizeof(features.features));  

How about:
ccw_dstream_advance(&sch->cds, offsetof(VirtioFeatDesc, index));

Quoted 10 lines

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

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

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

Quoted 6 lines

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

-                                 features.features, MEMTXATTRS_UNSPECIFIED,
-                                 NULL);
+            ccw_dstream_rewind(&sch->cds);
+            cpu_to_le32s(&features.features);
+            ccw_dstream_write(&sch->cds, features.features);
             sch->curr_status.scsw.count = ccw.count - sizeof(features);  

How about:
sch->curr_status.scsw.count = ccw_dstream_residual_count(&sch->cds);

This also applies to other places.

Quoted 2 lines

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

             ret = 0;
         }  

[...]

Quoted 8 lines

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

@@ -501,21 +459,13 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
             }
         }
         len = MIN(ccw.count, vdev->config_len);
-        hw_len = len;
         if (!ccw.cda) {
             ret = -EFAULT;
         } else {

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

Quoted 10 lines

hw/s390x/virtio-ccw.c +2 -8

-            config = cpu_physical_memory_map(ccw.cda, &hw_len, 0);
-            if (!config) {
-                ret = -EFAULT;
-            } else {
-                len = hw_len;
-                /* XXX config space endianness */
-                memcpy(vdev->config, config, len);
-                cpu_physical_memory_unmap(config, hw_len, 0, hw_len);
+            ret = ccw_dstream_read_buf(&sch->cds, vdev->config, len);
+            if (!ret) {  

Add a TODO here, and:

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

Quoted 7 lines

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

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

[...]

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

Quoted 12 lines

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

@@ -714,13 +654,9 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
             ret = -EFAULT;
             break;
         }
-        revinfo.revision =
-            address_space_lduw_be(&address_space_memory, ccw.cda,
-                                  MEMTXATTRS_UNSPECIFIED, NULL);
-        revinfo.length =
-            address_space_lduw_be(&address_space_memory,
-                                  ccw.cda + sizeof(revinfo.revision),
-                                  MEMTXATTRS_UNSPECIFIED, NULL);
+        ccw_dstream_read_buf(&sch->cds, &revinfo, 4);  

                                                     ^

A magic number? O:)

Quoted 9 lines

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

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

Signature · 1 line

2.13.5

Otherwise, looks good.

Quoted 15 lines · On Tue, 19 Sep 2017 11:37:30 +0800 Dong Jia Shi <[email protected]>…

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

---

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

Quoted 4 lines

Quoted 3 lines

Error handling: At the moment we ignore errors reported by stream ops to keep the change minimal. An add-on patch improving on this is to be expected later.

Add a TODO somewhere around the code as a reminder?

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

Quoted 15 lines

Quoted 9 lines

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

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

index b1976fdd19..a9baf6f7ab 100644

[...]

Quoted 4 lines

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

@@ -394,11 +362,8 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
         } else {
             VirtioDeviceClass *vdc = VIRTIO_DEVICE_GET_CLASS(vdev);
 

Quoted 8 lines

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

Quoted 6 lines

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

-            features.index = address_space_ldub(&address_space_memory,
-                                                ccw.cda
-                                                + sizeof(features.features),
-                                                MEMTXATTRS_UNSPECIFIED,
-                                                NULL);
+            ccw_dstream_advance(&sch->cds, sizeof(features.features));  

How about:
ccw_dstream_advance(&sch->cds, offsetof(VirtioFeatDesc, index));

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

Quoted 10 lines

Quoted 10 lines

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

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

Quoted 8 lines

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

Quoted 6 lines

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

-                                 features.features, MEMTXATTRS_UNSPECIFIED,
-                                 NULL);
+            ccw_dstream_rewind(&sch->cds);
+            cpu_to_le32s(&features.features);
+            ccw_dstream_write(&sch->cds, features.features);
             sch->curr_status.scsw.count = ccw.count - sizeof(features);  

How about:
sch->curr_status.scsw.count = ccw_dstream_residual_count(&sch->cds);

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

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

Quoted 27 lines

This also applies to other places.

Quoted 2 lines

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

             ret = 0;
         }  

[...]

Quoted 8 lines

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

@@ -501,21 +459,13 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
             }
         }
         len = MIN(ccw.count, vdev->config_len);
-        hw_len = len;
         if (!ccw.cda) {
             ret = -EFAULT;
         } else {

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

Quoted 10 lines

hw/s390x/virtio-ccw.c +2 -8

-            config = cpu_physical_memory_map(ccw.cda, &hw_len, 0);
-            if (!config) {
-                ret = -EFAULT;
-            } else {
-                len = hw_len;
-                /* XXX config space endianness */
-                memcpy(vdev->config, config, len);
-                cpu_physical_memory_unmap(config, hw_len, 0, hw_len);
+            ret = ccw_dstream_read_buf(&sch->cds, vdev->config, len);
+            if (!ret) {  

Add a TODO here, and:

Quoted 5 lines

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

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

Quoted 22 lines

Quoted 7 lines

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

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

[...]

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

Quoted 12 lines

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

@@ -714,13 +654,9 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
             ret = -EFAULT;
             break;
         }
-        revinfo.revision =
-            address_space_lduw_be(&address_space_memory, ccw.cda,
-                                  MEMTXATTRS_UNSPECIFIED, NULL);
-        revinfo.length =
-            address_space_lduw_be(&address_space_memory,
-                                  ccw.cda + sizeof(revinfo.revision),
-                                  MEMTXATTRS_UNSPECIFIED, NULL);
+        ccw_dstream_read_buf(&sch->cds, &revinfo, 4);  

                                                     ^

Quoted 1 line

A magic number? O:)

Hah :)

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

Quoted 10 lines

Quoted 9 lines

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

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

Signature · 1 line

2.13.5

Quoted 2 lines

Otherwise, looks good.

Can I get an ack?

Quoted 18 lines · Cornelia Huck

Quoted 15 lines · On Tue, 19 Sep 2017 11:37:30 +0800 Dong Jia Shi <[email protected]>…

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

---

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

Quoted 6 lines

Quoted 4 lines

Quoted 3 lines

Error handling: At the moment we ignore errors reported by stream ops to keep the change minimal. An add-on patch improving on this is to be expected later.

Add a TODO somewhere around the code as a reminder?

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

Quoted 25 lines

Quoted 24 lines

Quoted 10 lines

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

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

index b1976fdd19..a9baf6f7ab 100644

[...]

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

Quoted 10 lines

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

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

How about:
ccw_dstream_advance(&sch->cds, offsetof(VirtioFeatDesc, index));

Quoted 3 lines

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

Quoted 20 lines

Quoted 19 lines

Quoted 7 lines

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

+            ccw_dstream_read(&sch->cds, features.index);
             if (features.index == 0) {
                 if (dev->revision >= 1) {
                     /* Don't offer legacy features for modern devices. */

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

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

How about:
sch->curr_status.scsw.count = ccw_dstream_residual_count(&sch->cds);

Quoted 4 lines

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

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

Quoted 34 lines

Quoted 33 lines

This also applies to other places.

Quoted 2 lines

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

             ret = 0;
         }  

[...]

Quoted 8 lines

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

@@ -501,21 +459,13 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
             }
         }
         len = MIN(ccw.count, vdev->config_len);
-        hw_len = len;
         if (!ccw.cda) {
             ret = -EFAULT;
         } else {

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

Quoted 10 lines

hw/s390x/virtio-ccw.c +2 -8

-            config = cpu_physical_memory_map(ccw.cda, &hw_len, 0);
-            if (!config) {
-                ret = -EFAULT;
-            } else {
-                len = hw_len;
-                /* XXX config space endianness */
-                memcpy(vdev->config, config, len);
-                cpu_physical_memory_unmap(config, hw_len, 0, hw_len);
+            ret = ccw_dstream_read_buf(&sch->cds, vdev->config, len);
+            if (!ret) {  

Add a TODO here, and:

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

Quoted 5 lines

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

Quoted 26 lines

Quoted 25 lines

Quoted 8 lines

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

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

[...]

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

Quoted 12 lines

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

@@ -714,13 +654,9 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
             ret = -EFAULT;
             break;
         }
-        revinfo.revision =
-            address_space_lduw_be(&address_space_memory, ccw.cda,
-                                  MEMTXATTRS_UNSPECIFIED, NULL);
-        revinfo.length =
-            address_space_lduw_be(&address_space_memory,
-                                  ccw.cda + sizeof(revinfo.revision),
-                                  MEMTXATTRS_UNSPECIFIED, NULL);
+        ccw_dstream_read_buf(&sch->cds, &revinfo, 4);  

                                                     ^

A magic number? O:)

Quoted 5 lines

Hah :)

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

Quoted 11 lines

Quoted 11 lines

Quoted 10 lines

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

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

Signature · 1 line

2.13.5

Quoted 4 lines

Quoted 2 lines

Otherwise, looks good.

Can I get an ack?

Quoted 1 line

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

Regards,
Halil

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

---

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

Error handling: At the moment we ignore errors reported by stream ops to keep the change minimal. An add-on patch improving on this is to be expected later.

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

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

index b1976fdd19..a9baf6f7ab 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) |

@@ -488,7 +446,7 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
          } else {
              virtio_bus_get_vdev_config(&dev->bus, vdev->config);
              /* XXX config space endianness */
-            cpu_physical_memory_write(ccw.cda, vdev->config, len);
+            ccw_dstream_write_buf(&sch->cds, vdev->config, len);
              sch->curr_status.scsw.count = ccw.count - len;
              ret = 0;
          }

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

@@ -553,8 +503,7 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
          if (!ccw.cda) {
              ret = -EFAULT;
          } else {
-            status = address_space_ldub(&address_space_memory, ccw.cda,
-                                        MEMTXATTRS_UNSPECIFIED, NULL);
+            ccw_dstream_read(&sch->cds, status);
              if (!(status & VIRTIO_CONFIG_S_DRIVER_OK)) {
                  virtio_ccw_stop_ioeventfd(dev);
              }

@@ -597,8 +546,8 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
          if (!ccw.cda) {
              ret = -EFAULT;
          } else {
-            indicators = address_space_ldq_be(&address_space_memory, ccw.cda,
-                                              MEMTXATTRS_UNSPECIFIED, NULL);
+            ccw_dstream_read(&sch->cds, indicators);
+            be64_to_cpus(&indicators);
              dev->indicators = get_indicator(indicators, sizeof(uint64_t));
              sch->curr_status.scsw.count = ccw.count - sizeof(indicators);
              ret = 0;

@@ -618,8 +567,8 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
          if (!ccw.cda) {
              ret = -EFAULT;
          } else {
-            indicators = address_space_ldq_be(&address_space_memory, ccw.cda,
-                                              MEMTXATTRS_UNSPECIFIED, NULL);
+            ccw_dstream_read(&sch->cds, indicators);
+            be64_to_cpus(&indicators);
              dev->indicators2 = get_indicator(indicators, sizeof(uint64_t));
              sch->curr_status.scsw.count = ccw.count - sizeof(indicators);
              ret = 0;

@@ -639,67 +588,58 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
          if (!ccw.cda) {
              ret = -EFAULT;
          } else {
-            vq_config.index = address_space_lduw_be(&address_space_memory,
-                                                    ccw.cda,
-                                                    MEMTXATTRS_UNSPECIFIED,
-                                                    NULL);
+            ccw_dstream_read(&sch->cds, vq_config.index);
+            be16_to_cpus(&vq_config.index);
              if (vq_config.index >= VIRTIO_QUEUE_MAX) {
                  ret = -EINVAL;
                  break;
              }
              vq_config.num_max = virtio_queue_get_num(vdev,
                                                       vq_config.index);
-            address_space_stw_be(&address_space_memory,
-                                 ccw.cda + sizeof(vq_config.index),
-                                 vq_config.num_max,
-                                 MEMTXATTRS_UNSPECIFIED,
-                                 NULL);
+            cpu_to_be16s(&vq_config.num_max);
+            ccw_dstream_write(&sch->cds, vq_config.num_max);
              sch->curr_status.scsw.count = ccw.count - sizeof(vq_config);
              ret = 0;
          }
          break;
      case CCW_CMD_SET_IND_ADAPTER:
          if (check_len) {
-            if (ccw.count != sizeof(*thinint)) {
+            if (ccw.count != sizeof(thinint)) {
                  ret = -EINVAL;
                  break;
              }
-        } else if (ccw.count < sizeof(*thinint)) {
+        } else if (ccw.count < sizeof(thinint)) {
              /* Can't execute command. */
              ret = -EINVAL;
              break;
          }
-        len = sizeof(*thinint);
-        hw_len = len;
          if (!ccw.cda) {
              ret = -EFAULT;
          } else if (dev->indicators && !sch->thinint_active) {
              /* Trigger a command reject. */
              ret = -ENOSYS;
          } else {
-            thinint = cpu_physical_memory_map(ccw.cda, &hw_len, 0);
-            if (!thinint) {
+            if (ccw_dstream_read(&sch->cds, thinint)) {
                  ret = -EFAULT;
              } else {
-                uint64_t ind_bit = ldq_be_p(&thinint->ind_bit);
+                be64_to_cpus(&thinint.ind_bit);
+                be64_to_cpus(&thinint.summary_indicator);
+                be64_to_cpus(&thinint.device_indicator);
 
-                len = hw_len;
                  dev->summary_indicator =
-                    get_indicator(ldq_be_p(&thinint->summary_indicator),
-                                  sizeof(uint8_t));
+                    get_indicator(thinint.summary_indicator, sizeof(uint8_t));
                  dev->indicators =
-                    get_indicator(ldq_be_p(&thinint->device_indicator),
-                                  ind_bit / 8 + 1);
-                dev->thinint_isc = thinint->isc;
-                dev->routes.adapter.ind_offset = ind_bit;
+                    get_indicator(thinint.device_indicator,
+                                  thinint.ind_bit / 8 + 1);
+                dev->thinint_isc = thinint.isc;
+                dev->routes.adapter.ind_offset = thinint.ind_bit;
                  dev->routes.adapter.summary_offset = 7;
-                cpu_physical_memory_unmap(thinint, hw_len, 0, hw_len);
                  dev->routes.adapter.adapter_id = css_get_adapter_id(
                                                   CSS_IO_ADAPTER_VIRTIO,
                                                   dev->thinint_isc);
                  sch->thinint_active = ((dev->indicators != NULL) &&
                                         (dev->summary_indicator != NULL));
-                sch->curr_status.scsw.count = ccw.count - len;
+                sch->curr_status.scsw.count = ccw.count - sizeof(thinint);
                  ret = 0;
              }
          }

@@ -714,13 +654,9 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
              ret = -EFAULT;
              break;
          }
-        revinfo.revision =
-            address_space_lduw_be(&address_space_memory, ccw.cda,
-                                  MEMTXATTRS_UNSPECIFIED, NULL);

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

prefetching. We simply step trough the IDAL on demand.

Signed-off-by Halil Pasic <[email protected]>

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

hw/s390x/css.c +0 -0

index 6b0cd8861b..e34b2af4eb 100644

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

Signed-off-by Halil Pasic <[email protected]>

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

hw/s390x/css.c +0 -0

index 6b0cd8861b..e34b2af4eb 100644
@@ -819,6 +819,113 @@ incr:
     return 0;
 }

hw/s390x/css.c +6 -0

+    bool is_fmt2 = cds->flags & CDS_F_C64;
+    int ret;
+    hwaddr idaw_addr;
+
+    if (is_fmt2) {
+        idaw_addr = cds->cda_orig + sizeof(idaw.fmt2) * cds->at_idaw;

hw/s390x/css.c +33 -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;
+}
+
+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;

hw/s390x/css.c +25 -1

+            if (ret != MEMTX_OK) {
+                /* assume inaccessible address */
+                ret = -EINVAL; /* channel program check */
+                goto err;
+            }
+        }
+        cds->at_byte += iter_len;
+        cds->cda += iter_len;
+        len -= iter_len;
+        if (!len) {
+            break;
+        }
+        ret = ida_read_next_idaw(cds);
+        if (ret) {
+            goto err;
+        }
+        cont_left = bsz;
+    } while (true);
+    return ret;
+err:
+    cds->flags |= CDS_F_STREAM_BROKEN;
+    return ret;
+}
+
 void ccw_dstream_init(CcwDataStream *cds, CCW1 const *ccw, ORB const *orb)
 {
     /*

@@ -835,7 +942,7 @@ void ccw_dstream_init(CcwDataStream *cds, CCW1 const *ccw, ORB const *orb)
     if (!(cds->flags & CDS_F_IDA)) {
         cds->op_handler = ccw_dstream_rw_noflags;
     } else {
-        assert(false);
+        cds->op_handler = ccw_dstream_rw_ida;
     }
 }
 

Signature · 1 line

2.13.5

Quoted 17 lines · Halil Pasic

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

Signed-off-by Halil Pasic <[email protected]>

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

hw/s390x/css.c +0 -0

index 6b0cd8861b..e34b2af4eb 100644
@@ -819,6 +819,113 @@ incr:
     return 0;
 }
 

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

Quoted 9 lines

hw/s390x/css.c +9 -0

+/* returns values between 1 and bsz, where bs is a power of 2 */
+static inline uint16_t ida_continuous_left(hwaddr cda, uint64_t bsz)
+{
+    return bsz - (cda & (bsz - 1));
+}
+
+static inline uint64_t ccw_ida_block_size(uint8_t flags)
+{
+    return 1ULL << (((flags ^ CDS_F_C64) & (CDS_F_C64 | CDS_F_I2K)) ? 11 : 12);  

If CDS_F_C64 is set, (flags ^ CDS_F_C64) will be 0, so (1ULL << 11) will be the result regardless the I2K flag? The logic seems wrong.

I2K is meaningful only when C64 is 1, otherwise it is ignored. The logic here should be:

if ((flags & CDS_F_C64) && !(flags & CDS_F_I2K)) {
    return 1ULL << 12;
}
    return 1ULL << 11;

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

Quoted 6 lines

hw/s390x/css.c +5 -0

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

                                           ^

Nit.

Quoted 6 lines

hw/s390x/css.c +6 -0

+    bool is_fmt2 = cds->flags & CDS_F_C64;
+    int ret;
+    hwaddr idaw_addr;
+
+    if (is_fmt2) {
+        idaw_addr = cds->cda_orig + sizeof(idaw.fmt2) * cds->at_idaw;

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

Quoted 10 lines

hw/s390x/css.c +10 -0

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

?:
(idaw_addr & 0x80000003)

Quoted 37 lines

hw/s390x/css.c +36 -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;
+}
+
+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 {

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

Quoted 7 lines

hw/s390x/css.c +7 -0

+        cont_left = ida_continuous_left(cds->cda, bsz);
+        if (cont_left == bsz) {
+            ret = ida_read_next_idaw(cds);
+            if (ret) {
+                goto err;
+            }
+            if (cds->cda & (bsz - 1)) {  

Could move this check into ida_read_next_idaw?

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

Quoted 11 lines

hw/s390x/css.c +10 -0

+                ret = -EINVAL; /* channel program check */
+                goto err;
+            }
+        }
+    }
+    do {
+        iter_len = MIN(len, cont_left);
+        if (op != CDS_OP_A) {
+            ret = address_space_rw(&address_space_memory, cds->cda,
+                                   MEMTXATTRS_UNSPECIFIED, buff, iter_len, op);  

Ahh, now I recall that explictly defining CDS_OP_R to 0 and CDS_OP_W to 1 in 'struct CcwDataStreamOp' do have a meaning. Does it make sense to make it more obvious by adding some comment there?

Quoted 37 lines

hw/s390x/css.c +25 -1

+            if (ret != MEMTX_OK) {
+                /* assume inaccessible address */
+                ret = -EINVAL; /* channel program check */
+                goto err;
+            }
+        }
+        cds->at_byte += iter_len;
+        cds->cda += iter_len;
+        len -= iter_len;
+        if (!len) {
+            break;
+        }
+        ret = ida_read_next_idaw(cds);
+        if (ret) {
+            goto err;
+        }
+        cont_left = bsz;
+    } while (true);
+    return ret;
+err:
+    cds->flags |= CDS_F_STREAM_BROKEN;
+    return ret;
+}
+
 void ccw_dstream_init(CcwDataStream *cds, CCW1 const *ccw, ORB const *orb)
 {
     /*

@@ -835,7 +942,7 @@ void ccw_dstream_init(CcwDataStream *cds, CCW1 const *ccw, ORB const *orb)
     if (!(cds->flags & CDS_F_IDA)) {
         cds->op_handler = ccw_dstream_rw_noflags;
     } else {
-        assert(false);
+        cds->op_handler = ccw_dstream_rw_ida;
     }
 }
 

Quoted 3 lines

Signature · 1 line

2.13.5

Generally, the logic looks fine to me.

Quoted 19 lines · On Tue, 19 Sep 2017 13:50:05 +0800 Dong Jia Shi <[email protected]>…

Quoted 17 lines · Halil Pasic

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

Signed-off-by Halil Pasic <[email protected]>

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

hw/s390x/css.c +0 -0

index 6b0cd8861b..e34b2af4eb 100644
@@ -819,6 +819,113 @@ incr:
     return 0;
 }
 

Quoted 11 lines

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

Quoted 9 lines

hw/s390x/css.c +9 -0

+/* returns values between 1 and bsz, where bs is a power of 2 */
+static inline uint16_t ida_continuous_left(hwaddr cda, uint64_t bsz)
+{
+    return bsz - (cda & (bsz - 1));
+}
+
+static inline uint64_t ccw_ida_block_size(uint8_t flags)
+{
+    return 1ULL << (((flags ^ CDS_F_C64) & (CDS_F_C64 | CDS_F_I2K)) ? 11 : 12);  

If CDS_F_C64 is set, (flags ^ CDS_F_C64) will be 0, so (1ULL << 11) will be the result regardless the I2K flag? The logic seems wrong.

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

Quoted 1 line

Quoted 6 lines

I2K is meaningful only when C64 is 1, otherwise it is ignored. The logic here should be:

if ((flags & CDS_F_C64) && !(flags & CDS_F_I2K)) {
    return 1ULL << 12;
}
    return 1ULL << 11;

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

Quoted 9 lines

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

Quoted 6 lines

hw/s390x/css.c +5 -0

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

                                           ^

Nit.

Quoted 16 lines

Quoted 16 lines

hw/s390x/css.c +16 -0

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

Quoted 2 lines

?:
(idaw_addr & 0x80000003)

Yes.

Quoted 37 lines

Quoted 37 lines

hw/s390x/css.c +36 -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;
+}
+
+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 {

Quoted 8 lines

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

Quoted 7 lines

hw/s390x/css.c +7 -0

+        cont_left = ida_continuous_left(cds->cda, bsz);
+        if (cont_left == bsz) {
+            ret = ida_read_next_idaw(cds);
+            if (ret) {
+                goto err;
+            }
+            if (cds->cda & (bsz - 1)) {  

Could move this check into ida_read_next_idaw?

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

Quoted 11 lines

Quoted 11 lines

hw/s390x/css.c +10 -0

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

Quoted 3 lines

Ahh, now I recall that explictly defining CDS_OP_R to 0 and CDS_OP_W to 1 in 'struct CcwDataStreamOp' do have a meaning. Does it make sense to make it more obvious by adding some comment there?

Would you have a good text for that?

Quoted 37 lines

Quoted 37 lines

hw/s390x/css.c +25 -1

+            if (ret != MEMTX_OK) {
+                /* assume inaccessible address */
+                ret = -EINVAL; /* channel program check */
+                goto err;
+            }
+        }
+        cds->at_byte += iter_len;
+        cds->cda += iter_len;
+        len -= iter_len;
+        if (!len) {
+            break;
+        }
+        ret = ida_read_next_idaw(cds);
+        if (ret) {
+            goto err;
+        }
+        cont_left = bsz;
+    } while (true);
+    return ret;
+err:
+    cds->flags |= CDS_F_STREAM_BROKEN;
+    return ret;
+}
+
 void ccw_dstream_init(CcwDataStream *cds, CCW1 const *ccw, ORB const *orb)
 {
     /*

@@ -835,7 +942,7 @@ void ccw_dstream_init(CcwDataStream *cds, CCW1 const *ccw, ORB const *orb)
     if (!(cds->flags & CDS_F_IDA)) {
         cds->op_handler = ccw_dstream_rw_noflags;
     } else {
-        assert(false);
+        cds->op_handler = ccw_dstream_rw_ida;
     }
 }
 

Quoted 6 lines

Quoted 3 lines

Signature · 1 line

2.13.5

Generally, the logic looks fine to me.

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

Halil, do you have some more comments?

Quoted 22 lines · Cornelia Huck

Quoted 19 lines · On Tue, 19 Sep 2017 13:50:05 +0800 Dong Jia Shi <[email protected]>…

Quoted 17 lines · Halil Pasic

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

Signed-off-by Halil Pasic <[email protected]>

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

hw/s390x/css.c +0 -0

index 6b0cd8861b..e34b2af4eb 100644
@@ -819,6 +819,113 @@ incr:
     return 0;
 }
 

Quoted 11 lines

Quoted 11 lines

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

Quoted 9 lines

hw/s390x/css.c +9 -0

+/* returns values between 1 and bsz, where bs is a power of 2 */
+static inline uint16_t ida_continuous_left(hwaddr cda, uint64_t bsz)
+{
+    return bsz - (cda & (bsz - 1));
+}
+
+static inline uint64_t ccw_ida_block_size(uint8_t flags)
+{
+    return 1ULL << (((flags ^ CDS_F_C64) & (CDS_F_C64 | CDS_F_I2K)) ? 11 : 12);    

If CDS_F_C64 is set, (flags ^ CDS_F_C64) will be 0, so (1ULL << 11) will be the result regardless the I2K flag? The logic seems wrong.

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

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

In my reading the logic is good.

Quoted 13 lines

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

Quoted 7 lines

I2K is meaningful only when C64 is 1, otherwise it is ignored. The logic here should be:

if ((flags & CDS_F_C64) && !(flags & CDS_F_I2K)) {
    return 1ULL << 12;
}
    return 1ULL << 11;  

Quoted 2 lines

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

I won't argue with this.

Quoted 10 lines

Quoted 10 lines

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

Quoted 7 lines

hw/s390x/css.c +5 -0

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

                                           ^

Nit.

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

Quoted 17 lines

Quoted 17 lines

Quoted 17 lines

hw/s390x/css.c +16 -0

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

Quoted 5 lines

Quoted 2 lines

?:
(idaw_addr & 0x80000003)

Yes.

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

Quoted 47 lines

Quoted 46 lines

Quoted 35 lines

hw/s390x/css.c +33 -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;
+}
+
+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;

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

Quoted 10 lines

hw/s390x/css.c +10 -0

+        }
+        cont_left = ida_continuous_left(cds->cda, bsz);
+    } else {
+        cont_left = ida_continuous_left(cds->cda, bsz);
+        if (cont_left == bsz) {
+            ret = ida_read_next_idaw(cds);
+            if (ret) {
+                goto err;
+            }
+            if (cds->cda & (bsz - 1)) {    

Could move this check into ida_read_next_idaw?

Quoted 2 lines

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

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

Quoted 12 lines

Quoted 12 lines

Quoted 12 lines

hw/s390x/css.c +10 -0

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

Quoted 6 lines

Quoted 3 lines

Ahh, now I recall that explictly defining CDS_OP_R to 0 and CDS_OP_W to 1 in 'struct CcwDataStreamOp' do have a meaning. Does it make sense to make it more obvious by adding some comment there?

Would you have a good text for that?

I'm fine with clarifications.

Quoted 45 lines

Quoted 38 lines

Quoted 38 lines

hw/s390x/css.c +25 -1

+            if (ret != MEMTX_OK) {
+                /* assume inaccessible address */
+                ret = -EINVAL; /* channel program check */
+                goto err;
+            }
+        }
+        cds->at_byte += iter_len;
+        cds->cda += iter_len;
+        len -= iter_len;
+        if (!len) {
+            break;
+        }
+        ret = ida_read_next_idaw(cds);
+        if (ret) {
+            goto err;
+        }
+        cont_left = bsz;
+    } while (true);
+    return ret;
+err:
+    cds->flags |= CDS_F_STREAM_BROKEN;
+    return ret;
+}
+
 void ccw_dstream_init(CcwDataStream *cds, CCW1 const *ccw, ORB const *orb)
 {
     /*

@@ -835,7 +942,7 @@ void ccw_dstream_init(CcwDataStream *cds, CCW1 const *ccw, ORB const *orb)
     if (!(cds->flags & CDS_F_IDA)) {
         cds->op_handler = ccw_dstream_rw_noflags;
     } else {
-        assert(false);
+        cds->op_handler = ccw_dstream_rw_ida;
     }
 }
 

Quoted 6 lines

Quoted 3 lines

Signature · 1 line

2.13.5

Generally, the logic looks fine to me.

Quoted 2 lines

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

Nod.

Quoted 3 lines

Halil, do you have some more comments?

Just a question. Do I have to respin?

Quoted 7 lines

Quoted 6 lines

Quoted 6 lines

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

Quoted 3 lines

hw/s390x/css.c +3 -0

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

                                           ^

Nit.

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

I'd just leave it like that, tbh.

Quoted 18 lines

Quoted 18 lines

Quoted 18 lines

Quoted 18 lines

hw/s390x/css.c +16 -0

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

Quoted 6 lines

Quoted 4 lines

Quoted 2 lines

?:
(idaw_addr & 0x80000003)

Yes.

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

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

Quoted 16 lines

Quoted 16 lines

Quoted 16 lines

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

Quoted 28 lines · Cornelia Huck

Quoted 7 lines

Quoted 6 lines

Quoted 6 lines

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

Quoted 3 lines

hw/s390x/css.c +3 -0

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

                                           ^

Nit.

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

I'd just leave it like that, tbh.

Quoted 18 lines

Quoted 18 lines

Quoted 18 lines

Quoted 18 lines

hw/s390x/css.c +16 -0

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

Quoted 9 lines

Quoted 6 lines

Quoted 4 lines

Quoted 2 lines

?:
(idaw_addr & 0x80000003)

Yes.

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

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

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

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

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

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

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

How shall we proceed?

Quoted 52 lines · On Tue, 19 Sep 2017 14:04:03 +0200 Halil Pasic <[email protected]> wrote:

Quoted 28 lines · Cornelia Huck

Quoted 7 lines

Quoted 6 lines

Quoted 6 lines

Review

Quoted 3 lines

(quoted diff) +3 -0

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

                                           ^

Nit.

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

I'd just leave it like that, tbh.

Quoted 18 lines

Quoted 18 lines

Quoted 18 lines

Quoted 18 lines

(quoted diff) +16 -0

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

Quoted 9 lines

Quoted 6 lines

Quoted 4 lines

Quoted 2 lines

?:
(idaw_addr & 0x80000003)

Yes.

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

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

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

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

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

Quoted 8 lines

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

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

Sigh, when are things ever easy :(

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

Quoted 2 lines

How shall we proceed?

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

More after the break :)

Quoted 71 lines · Cornelia Huck

Quoted 52 lines · On Tue, 19 Sep 2017 14:04:03 +0200 Halil Pasic <[email protected]> wrote:

Quoted 28 lines · Cornelia Huck

Quoted 7 lines

Quoted 6 lines

Quoted 6 lines

Review

Quoted 3 lines

(quoted diff) +3 -0

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

                                           ^

Nit.

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

I'd just leave it like that, tbh.

Quoted 18 lines

Quoted 18 lines

Quoted 18 lines

Quoted 18 lines

(quoted diff) +16 -0

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

Quoted 9 lines

Quoted 6 lines

Quoted 4 lines

Quoted 2 lines

?:
(idaw_addr & 0x80000003)

Yes.

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

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

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

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

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

Quoted 8 lines

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

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

Sigh, when are things ever easy :(

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

Quoted 2 lines

How shall we proceed?

Quoted 8 lines

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

More after the break :)

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

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

Quoted 6 lines

Quoted 5 lines

Quoted 5 lines

Review

Quoted 2 lines

(quoted diff) +2 -0

+{
+    union {uint64_t fmt2; uint32_t fmt1; } idaw;      

                                           ^

Nit.

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

I'd just leave it like that, tbh.

Quoted 5 lines · Cornelia Huck

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Review

Quoted 2 lines

(quoted diff) +2 -0

+{
+    union {uint64_t fmt2; uint32_t fmt1; } idaw;      

                                           ^

Nit.

Quoted 2 lines

Quoted 1 line

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

I'd just leave it like that, tbh.

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

Going to keep it as is for v3.

Halil

Quoted 30 lines

Quoted 7 lines

Quoted 6 lines

Quoted 6 lines

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

Quoted 3 lines

hw/s390x/css.c +3 -0

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

                                           ^

Nit.

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

I'd just leave it like that, tbh.

Quoted 20 lines

Quoted 20 lines

Quoted 20 lines

Quoted 8 lines

hw/s390x/css.c +8 -0

+    bool is_fmt2 = cds->flags & CDS_F_C64;
+    int ret;
+    hwaddr idaw_addr;
+
+    if (is_fmt2) {
+        idaw_addr = cds->cda_orig + sizeof(idaw.fmt2) * cds->at_idaw;
+        if (idaw_addr & 0x07) {
+            return -EINVAL; /* channel program check */

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

Quoted 10 lines

hw/s390x/css.c +8 -0

+        }
+        ret = address_space_rw(&address_space_memory, idaw_addr,
+                               MEMTXATTRS_UNSPECIFIED, (void *) &idaw.fmt2,
+                               sizeof(idaw.fmt2), false);
+        cds->cda = be64_to_cpu(idaw.fmt2);
 
 
+    } else {
+        idaw_addr = cds->cda_orig + sizeof(idaw.fmt1) * cds->at_idaw;
+        if (idaw_addr & 0x03) {    

?:
(idaw_addr & 0x80000003)