[Qemu-devel] [PATCH 0/4] s390x/css: ccw interpretation fixes

[Qemu-devel] [PATCH 0/4] s390x/css: ccw interpretation fixes

[Halil Pasic]

Here come some bug fixes concerning CCW interpretation.

Patch #1 was already posted as a stand alone patch, is (kind of)
required by patch #4 so it's included in this series too. 

Halil Pasic (4):
  s390x/css: drop data-check in interpretation
  s390x/css: fix NULL handling for CCW addresses
  s390x/css: remove dubious error handling branch
  s390x/css: fix incorrect length indication

 hw/s390x/3270-ccw.c    |  24 +++++-----
 hw/s390x/css.c         |  79 ++++++++++++++----------------
 hw/s390x/virtio-ccw.c  | 128 ++++++++++++++++++++++++-------------------------
 include/hw/s390x/css.h |  13 ++++-
 4 files changed, 125 insertions(+), 119 deletions(-)

-- 
2.13.5

[Qemu-devel] [PATCH 1/4] s390x/css: drop data-check in interpretation

[Halil Pasic]

The architecture says that channel-data check is indicating that
an uncorrected storage (memory) error has been detected in regard
to the data residing in main storage (memory) that is currently
used for an I/O operation. The described detection is done using
the CBC technology.

The ccw interpretation code is however generating a channel-data check
effectively when the (device specific) ccw_cb returns -EFAULT.  In case
of virtio-ccw devices this happens when mapping memory fails, or when a
NULL pointer is encountered. So this behavior is not architecture
conform.

Furthermore the best fit for these situations (null pointer, mapping a
piece of guest memory fails) from architectural perspective the condition
described as the channel subsystem refers to a location that is not
available, which when encountered shall result in a channel-program
check.

To fix this, all we have to do is to get rid of the switch case matching
-EFAULT: the default is generating a channel-program check.

---

Was posted as stand alone patch. See:
http://patchwork.ozlabs.org/patch/810995/

If you are going to, *please review there*.

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

diff --git a/hw/s390x/css.c b/hw/s390x/css.c
index 901dc6a0f3..09f6ba0310 100644
--- a/hw/s390x/css.c
+++ b/hw/s390x/css.c
@@ -980,15 +980,6 @@ static void sch_handle_start_func_virtual(SubchDev *sch)
                     SCSW_STCTL_ALERT | SCSW_STCTL_STATUS_PEND;
             s->cpa = sch->channel_prog + 8;
             break;
-        case -EFAULT:
-            /* memory problem, generate channel data check */
-            s->ctrl &= ~SCSW_ACTL_START_PEND;
-            s->cstat = SCSW_CSTAT_DATA_CHECK;
-            s->ctrl &= ~SCSW_CTRL_MASK_STCTL;
-            s->ctrl |= SCSW_STCTL_PRIMARY | SCSW_STCTL_SECONDARY |
-                    SCSW_STCTL_ALERT | SCSW_STCTL_STATUS_PEND;
-            s->cpa = sch->channel_prog + 8;
-            break;
         case -EBUSY:
             /* subchannel busy, generate deferred cc 1 */
             s->flags &= ~SCSW_FLAGS_MASK_CC;
-- 
2.13.5

[Qemu-devel] [PATCH 2/4] s390x/css: fix NULL handling for CCW addresses

[Halil Pasic]

Back then in the time of df1fe5bb49 ("s390: Virtual channel subsystem
support.", 2013-01-24) -EIO used to map to a channel-program check (via
the default label of the switch statement).  Then 2dc95b4cac
("s390x/3270: 3270 data stream handling", 2016-04-01) came along
and that changed dramatically.

Let us roll back this undesired side effect, and go back to
channel-program check.

Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
Fixes: 2dc95b4cac "s390x/3270: 3270 data stream handling"
---

I'm not sure 0 as CCW address  it's strictly illegal. Yes 0 is an
unlikely address for a CCW but I would appreciate a PoP reference
clarifying this...

Another reason to not use Unix/POSIX error codes like this.
---
 hw/s390x/css.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/s390x/css.c b/hw/s390x/css.c
index 09f6ba0310..a44d87ab3e 100644
--- a/hw/s390x/css.c
+++ b/hw/s390x/css.c
@@ -793,7 +793,7 @@ static int css_interpret_ccw(SubchDev *sch, hwaddr ccw_addr,
     CCW1 ccw;
 
     if (!ccw_addr) {
-        return -EIO;
+        return -EINVAL; /* channel-program check */
     }
     /* Check doubleword aligned and 31 or 24 (fmt 0) bit addressable. */
     if (ccw_addr & (sch->ccw_fmt_1 ? 0x80000007 : 0xff000007)) {
-- 
2.13.5

[Qemu-devel] [PATCH 3/4] s390x/css: remove dubious error handling branch

[Halil Pasic]

The case in question actually never happens. Let us get rid of the dead
code.

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

diff --git a/hw/s390x/css.c b/hw/s390x/css.c
index a44d87ab3e..a9cdd54efc 100644
--- a/hw/s390x/css.c
+++ b/hw/s390x/css.c
@@ -980,13 +980,6 @@ static void sch_handle_start_func_virtual(SubchDev *sch)
                     SCSW_STCTL_ALERT | SCSW_STCTL_STATUS_PEND;
             s->cpa = sch->channel_prog + 8;
             break;
-        case -EBUSY:
-            /* subchannel busy, generate deferred cc 1 */
-            s->flags &= ~SCSW_FLAGS_MASK_CC;
-            s->flags |= (1 << 8);
-            s->ctrl &= ~SCSW_CTRL_MASK_STCTL;
-            s->ctrl |= SCSW_STCTL_ALERT | SCSW_STCTL_STATUS_PEND;
-            break;
         case -EINPROGRESS:
             /* channel program has been suspended */
             s->ctrl &= ~SCSW_ACTL_START_PEND;
-- 
2.13.5

[Qemu-devel] [PATCH 4/4] s390x/css: fix incorrect length indication

[Halil Pasic]

We report incorrect length via SCSW program check instead of incorrect
length check (SCWS word 2 bit 10 instead of bit 9). Since we have there
is no fitting errno for incorrect length, and since I don't like what we
do with the errno's, as part of the fix, errnos used for control flow in
ccw interpretation are replaced with an enum using more speaking names.

For virtio, if incorrect length checking is suppressed we keep the
current behavior (channel-program check).

Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
---
 hw/s390x/3270-ccw.c    |  24 +++++-----
 hw/s390x/css.c         |  67 +++++++++++++++-----------
 hw/s390x/virtio-ccw.c  | 128 ++++++++++++++++++++++++-------------------------
 include/hw/s390x/css.h |  13 ++++-
 4 files changed, 127 insertions(+), 105 deletions(-)

diff --git a/hw/s390x/3270-ccw.c b/hw/s390x/3270-ccw.c
index 1554aa2484..5f9efe7ac6 100644
--- a/hw/s390x/3270-ccw.c
+++ b/hw/s390x/3270-ccw.c
@@ -18,46 +18,48 @@
 #include "hw/s390x/3270-ccw.h"
 
 /* Handle READ ccw commands from guest */
-static int handle_payload_3270_read(EmulatedCcw3270Device *dev, CCW1 *ccw)
+static CcwProcStatus handle_payload_3270_read(EmulatedCcw3270Device *dev,
+                                              CCW1 *ccw)
 {
     EmulatedCcw3270Class *ck = EMULATED_CCW_3270_GET_CLASS(dev);
     CcwDevice *ccw_dev = CCW_DEVICE(dev);
     int len;
 
     if (!ccw->cda) {
-        return -EFAULT;
+        return CSS_DO_PGM_CHK;
     }
 
     len = ck->read_payload_3270(dev, ccw->cda, ccw->count);
     ccw_dev->sch->curr_status.scsw.count = ccw->count - len;
 
-    return 0;
+    return CSS_DO_SUCCESS;
 }
 
 /* Handle WRITE ccw commands to write data to client */
-static int handle_payload_3270_write(EmulatedCcw3270Device *dev, CCW1 *ccw)
+static CcwProcStatus handle_payload_3270_write(EmulatedCcw3270Device *dev,
+                                               CCW1 *ccw)
 {
     EmulatedCcw3270Class *ck = EMULATED_CCW_3270_GET_CLASS(dev);
     CcwDevice *ccw_dev = CCW_DEVICE(dev);
     int len;
 
     if (!ccw->cda) {
-        return -EFAULT;
+        return CSS_DO_PGM_CHK;
     }
 
     len = ck->write_payload_3270(dev, ccw->cmd_code, ccw->cda, ccw->count);
 
     if (len <= 0) {
-        return -EIO;
+        return CSS_E_CUSTOM;
     }
 
     ccw_dev->sch->curr_status.scsw.count = ccw->count - len;
-    return 0;
+    return CSS_DO_SUCCESS;
 }
 
-static int emulated_ccw_3270_cb(SubchDev *sch, CCW1 ccw)
+static CcwProcStatus emulated_ccw_3270_cb(SubchDev *sch, CCW1 ccw)
 {
-    int rc = 0;
+    CcwProcStatus rc = CSS_DO_SUCCESS;
     EmulatedCcw3270Device *dev = sch->driver_data;
 
     switch (ccw.cmd_code) {
@@ -72,11 +74,11 @@ static int emulated_ccw_3270_cb(SubchDev *sch, CCW1 ccw)
         rc = handle_payload_3270_read(dev, &ccw);
         break;
     default:
-        rc = -ENOSYS;
+        rc = CSS_DO_UNIT_CHK_REJ;
         break;
     }
 
-    if (rc == -EIO) {
+    if (rc == CSS_E_CUSTOM) {
         /* I/O error, specific devices generate specific conditions */
         SCSW *s = &sch->curr_status.scsw;
 
diff --git a/hw/s390x/css.c b/hw/s390x/css.c
index a9cdd54efc..875e9b00b8 100644
--- a/hw/s390x/css.c
+++ b/hw/s390x/css.c
@@ -784,20 +784,20 @@ static CCW1 copy_ccw_from_guest(hwaddr addr, bool fmt1)
     return ret;
 }
 
-static int css_interpret_ccw(SubchDev *sch, hwaddr ccw_addr,
+static CcwProcStatus css_interpret_ccw(SubchDev *sch, hwaddr ccw_addr,
                              bool suspend_allowed)
 {
-    int ret;
+    CcwProcStatus ret;
     bool check_len;
     int len;
     CCW1 ccw;
 
     if (!ccw_addr) {
-        return -EINVAL; /* channel-program check */
+        return CSS_DO_PGM_CHK;
     }
     /* Check doubleword aligned and 31 or 24 (fmt 0) bit addressable. */
     if (ccw_addr & (sch->ccw_fmt_1 ? 0x80000007 : 0xff000007)) {
-        return -EINVAL;
+        return CSS_DO_PGM_CHK;
     }
 
     /* Translate everything to format-1 ccws - the information is the same. */
@@ -805,31 +805,31 @@ static int css_interpret_ccw(SubchDev *sch, hwaddr ccw_addr,
 
     /* Check for invalid command codes. */
     if ((ccw.cmd_code & 0x0f) == 0) {
-        return -EINVAL;
+        return CSS_DO_PGM_CHK;
     }
     if (((ccw.cmd_code & 0x0f) == CCW_CMD_TIC) &&
         ((ccw.cmd_code & 0xf0) != 0)) {
-        return -EINVAL;
+        return CSS_DO_PGM_CHK;
     }
     if (!sch->ccw_fmt_1 && (ccw.count == 0) &&
         (ccw.cmd_code != CCW_CMD_TIC)) {
-        return -EINVAL;
+        return CSS_DO_PGM_CHK;
     }
 
     /* We don't support MIDA. */
     if (ccw.flags & CCW_FLAG_MIDA) {
-        return -EINVAL;
+        return CSS_DO_PGM_CHK;
     }
 
     if (ccw.flags & CCW_FLAG_SUSPEND) {
-        return suspend_allowed ? -EINPROGRESS : -EINVAL;
+        return suspend_allowed ? CSS_DO_SUSPEND : CSS_DO_PGM_CHK;
     }
 
     check_len = !((ccw.flags & CCW_FLAG_SLI) && !(ccw.flags & CCW_FLAG_DC));
 
     if (!ccw.cda) {
         if (sch->ccw_no_data_cnt == 255) {
-            return -EINVAL;
+            return CSS_DO_PGM_CHK;
         }
         sch->ccw_no_data_cnt++;
     }
@@ -838,12 +838,12 @@ static int css_interpret_ccw(SubchDev *sch, hwaddr ccw_addr,
     switch (ccw.cmd_code) {
     case CCW_CMD_NOOP:
         /* Nothing to do. */
-        ret = 0;
+        ret = CSS_DO_SUCCESS;
         break;
     case CCW_CMD_BASIC_SENSE:
         if (check_len) {
             if (ccw.count != sizeof(sch->sense_data)) {
-                ret = -EINVAL;
+                ret = CSS_DO_INCORR_LEN;
                 break;
             }
         }
@@ -851,7 +851,7 @@ static int css_interpret_ccw(SubchDev *sch, hwaddr ccw_addr,
         cpu_physical_memory_write(ccw.cda, sch->sense_data, len);
         sch->curr_status.scsw.count = ccw.count - len;
         memset(sch->sense_data, 0, sizeof(sch->sense_data));
-        ret = 0;
+        ret = CSS_DO_SUCCESS;
         break;
     case CCW_CMD_SENSE_ID:
     {
@@ -861,7 +861,7 @@ static int css_interpret_ccw(SubchDev *sch, hwaddr ccw_addr,
         /* Sense ID information is device specific. */
         if (check_len) {
             if (ccw.count != sizeof(sense_id)) {
-                ret = -EINVAL;
+                ret = CSS_DO_PGM_CHK;
                 break;
             }
         }
@@ -877,37 +877,37 @@ static int css_interpret_ccw(SubchDev *sch, hwaddr ccw_addr,
         }
         cpu_physical_memory_write(ccw.cda, &sense_id, len);
         sch->curr_status.scsw.count = ccw.count - len;
-        ret = 0;
+        ret = CSS_DO_SUCCESS;
         break;
     }
     case CCW_CMD_TIC:
         if (sch->last_cmd_valid && (sch->last_cmd.cmd_code == CCW_CMD_TIC)) {
-            ret = -EINVAL;
+            ret = CSS_DO_PGM_CHK;
             break;
         }
         if (ccw.flags || ccw.count) {
             /* We have already sanitized these if converted from fmt 0. */
-            ret = -EINVAL;
+            ret = CSS_DO_PGM_CHK;
             break;
         }
         sch->channel_prog = ccw.cda;
-        ret = -EAGAIN;
+        ret = CSS_DO_CONT_CHAIN;
         break;
     default:
         if (sch->ccw_cb) {
             /* Handle device specific commands. */
             ret = sch->ccw_cb(sch, ccw);
         } else {
-            ret = -ENOSYS;
+            ret = CSS_DO_UNIT_CHK_REJ;
         }
         break;
     }
     sch->last_cmd = ccw;
     sch->last_cmd_valid = true;
-    if (ret == 0) {
+    if (ret == CSS_DO_SUCCESS) {
         if (ccw.flags & CCW_FLAG_CC) {
             sch->channel_prog += 8;
-            ret = -EAGAIN;
+            ret = CSS_DO_CONT_CHAIN;
         }
     }
 
@@ -920,7 +920,7 @@ static void sch_handle_start_func_virtual(SubchDev *sch)
     PMCW *p = &sch->curr_status.pmcw;
     SCSW *s = &sch->curr_status.scsw;
     int path;
-    int ret;
+    CcwProcStatus ret;
     bool suspend_allowed;
 
     /* Path management: In our simple css, we always choose the only path. */
@@ -954,10 +954,10 @@ static void sch_handle_start_func_virtual(SubchDev *sch)
     do {
         ret = css_interpret_ccw(sch, sch->channel_prog, suspend_allowed);
         switch (ret) {
-        case -EAGAIN:
+        case CSS_DO_CONT_CHAIN:
             /* ccw chain, continue processing */
             break;
-        case 0:
+        case CSS_DO_SUCCESS:
             /* success */
             s->ctrl &= ~SCSW_ACTL_START_PEND;
             s->ctrl &= ~SCSW_CTRL_MASK_STCTL;
@@ -966,10 +966,10 @@ static void sch_handle_start_func_virtual(SubchDev *sch)
             s->dstat = SCSW_DSTAT_CHANNEL_END | SCSW_DSTAT_DEVICE_END;
             s->cpa = sch->channel_prog + 8;
             break;
-        case -EIO:
+        case CSS_E_CUSTOM:
             /* I/O errors, status depends on specific devices */
             break;
-        case -ENOSYS:
+        case CSS_DO_UNIT_CHK_REJ:
             /* unsupported command, generate unit check (command reject) */
             s->ctrl &= ~SCSW_ACTL_START_PEND;
             s->dstat = SCSW_DSTAT_UNIT_CHECK;
@@ -980,12 +980,21 @@ static void sch_handle_start_func_virtual(SubchDev *sch)
                     SCSW_STCTL_ALERT | SCSW_STCTL_STATUS_PEND;
             s->cpa = sch->channel_prog + 8;
             break;
-        case -EINPROGRESS:
+        case CSS_DO_SUSPEND:
             /* channel program has been suspended */
             s->ctrl &= ~SCSW_ACTL_START_PEND;
             s->ctrl |= SCSW_ACTL_SUSP;
             break;
-        default:
+        case CSS_DO_INCORR_LEN:
+            s->ctrl &= ~SCSW_ACTL_START_PEND;
+            s->cstat = SCSW_CSTAT_INCORR_LEN;
+            s->ctrl &= ~SCSW_CTRL_MASK_STCTL;
+            s->ctrl |= SCSW_STCTL_PRIMARY | SCSW_STCTL_SECONDARY |
+                    SCSW_STCTL_ALERT | SCSW_STCTL_STATUS_PEND;
+            s->cpa = sch->channel_prog + 8;
+            break;
+
+        case CSS_DO_PGM_CHK:
             /* error, generate channel program check */
             s->ctrl &= ~SCSW_ACTL_START_PEND;
             s->cstat = SCSW_CSTAT_PROG_CHECK;
@@ -995,7 +1004,7 @@ static void sch_handle_start_func_virtual(SubchDev *sch)
             s->cpa = sch->channel_prog + 8;
             break;
         }
-    } while (ret == -EAGAIN);
+    } while (ret == CSS_DO_CONT_CHAIN);
 
 }
 
diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
index b1976fdd19..3d288b5343 100644
--- a/hw/s390x/virtio-ccw.c
+++ b/hw/s390x/virtio-ccw.c
@@ -207,16 +207,16 @@ static int virtio_ccw_set_vqs(SubchDev *sch, VqInfoBlock *info,
     uint64_t desc = info ? info->desc : linfo->queue;
 
     if (index >= VIRTIO_QUEUE_MAX) {
-        return -EINVAL;
+        return CSS_DO_PGM_CHK;
     }
 
     /* Current code in virtio.c relies on 4K alignment. */
     if (linfo && desc && (linfo->align != 4096)) {
-        return -EINVAL;
+        return CSS_DO_PGM_CHK;
     }
 
     if (!vdev) {
-        return -EINVAL;
+        return CSS_DO_PGM_CHK;
     }
 
     if (info) {
@@ -231,19 +231,19 @@ static int virtio_ccw_set_vqs(SubchDev *sch, VqInfoBlock *info,
             /* virtio-1 allows changing the ring size. */
             if (virtio_queue_get_max_num(vdev, index) < num) {
                 /* Fail if we exceed the maximum number. */
-                return -EINVAL;
+                return CSS_DO_PGM_CHK;
             }
             virtio_queue_set_num(vdev, index, num);
         } else if (virtio_queue_get_num(vdev, index) > num) {
             /* Fail if we don't have a big enough queue. */
-            return -EINVAL;
+            return CSS_DO_PGM_CHK;
         }
         /* We ignore possible increased num for legacy for compatibility. */
         virtio_queue_set_vector(vdev, index, index);
     }
     /* tell notify handler in case of config change */
     vdev->config_vector = VIRTIO_QUEUE_MAX;
-    return 0;
+    return CSS_DO_SUCCESS;
 }
 
 static void virtio_ccw_reset_virtio(VirtioCcwDevice *dev, VirtIODevice *vdev)
@@ -277,14 +277,14 @@ static int virtio_ccw_handle_set_vq(SubchDev *sch, CCW1 ccw, bool check_len,
 
     if (check_len) {
         if (ccw.count != info_len) {
-            return -EINVAL;
+            return CSS_DO_INCORR_LEN;
         }
     } else if (ccw.count < info_len) {
         /* Can't execute command. */
-        return -EINVAL;
+        return CSS_DO_PGM_CHK;
     }
     if (!ccw.cda) {
-        return -EFAULT;
+        return CSS_DO_PGM_CHK;
     }
     if (is_legacy) {
         linfo.queue = address_space_ldq_be(&address_space_memory, ccw.cda,
@@ -336,7 +336,7 @@ static int virtio_ccw_handle_set_vq(SubchDev *sch, CCW1 ccw, bool check_len,
     return ret;
 }
 
-static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
+static CcwProcStatus virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
 {
     int ret;
     VirtioRevInfo revinfo;
@@ -353,7 +353,7 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
     VirtioThinintInfo *thinint;
 
     if (!dev) {
-        return -EINVAL;
+        return CSS_DO_PGM_CHK;
     }
 
     trace_virtio_ccw_interpret_ccw(sch->cssid, sch->ssid, sch->schid,
@@ -366,7 +366,7 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
          * virtio-1 drivers must start with negotiating to a revision >= 1,
          * so post a command reject for all other commands
          */
-        return -ENOSYS;
+        return CSS_DO_UNIT_CHK_REJ;
     }
 
     /* Look at the command. */
@@ -376,21 +376,21 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
         break;
     case CCW_CMD_VDEV_RESET:
         virtio_ccw_reset_virtio(dev, vdev);
-        ret = 0;
+        ret = CSS_DO_SUCCESS;
         break;
     case CCW_CMD_READ_FEAT:
         if (check_len) {
             if (ccw.count != sizeof(features)) {
-                ret = -EINVAL;
+                ret = CSS_DO_INCORR_LEN;
                 break;
             }
         } else if (ccw.count < sizeof(features)) {
             /* Can't execute command. */
-            ret = -EINVAL;
+            ret = CSS_DO_PGM_CHK;
             break;
         }
         if (!ccw.cda) {
-            ret = -EFAULT;
+            ret = CSS_DO_PGM_CHK;
         } else {
             VirtioDeviceClass *vdc = VIRTIO_DEVICE_GET_CLASS(vdev);
 
@@ -421,22 +421,22 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
                                  features.features, MEMTXATTRS_UNSPECIFIED,
                                  NULL);
             sch->curr_status.scsw.count = ccw.count - sizeof(features);
-            ret = 0;
+            ret = CSS_DO_SUCCESS;
         }
         break;
     case CCW_CMD_WRITE_FEAT:
         if (check_len) {
             if (ccw.count != sizeof(features)) {
-                ret = -EINVAL;
+                ret = CSS_DO_INCORR_LEN;
                 break;
             }
         } else if (ccw.count < sizeof(features)) {
             /* Can't execute command. */
-            ret = -EINVAL;
+            ret = CSS_DO_PGM_CHK;
             break;
         }
         if (!ccw.cda) {
-            ret = -EFAULT;
+            ret = CSS_DO_PGM_CHK;
         } else {
             features.index = address_space_ldub(&address_space_memory,
                                                 ccw.cda
@@ -472,42 +472,42 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
                 }
             }
             sch->curr_status.scsw.count = ccw.count - sizeof(features);
-            ret = 0;
+            ret = CSS_DO_SUCCESS;
         }
         break;
     case CCW_CMD_READ_CONF:
         if (check_len) {
             if (ccw.count > vdev->config_len) {
-                ret = -EINVAL;
+                ret = CSS_DO_INCORR_LEN;
                 break;
             }
         }
         len = MIN(ccw.count, vdev->config_len);
         if (!ccw.cda) {
-            ret = -EFAULT;
+            ret = CSS_DO_PGM_CHK;
         } else {
             virtio_bus_get_vdev_config(&dev->bus, vdev->config);
             /* XXX config space endianness */
             cpu_physical_memory_write(ccw.cda, vdev->config, len);
             sch->curr_status.scsw.count = ccw.count - len;
-            ret = 0;
+            ret = CSS_DO_SUCCESS;
         }
         break;
     case CCW_CMD_WRITE_CONF:
         if (check_len) {
             if (ccw.count > vdev->config_len) {
-                ret = -EINVAL;
+                ret = CSS_DO_INCORR_LEN;
                 break;
             }
         }
         len = MIN(ccw.count, vdev->config_len);
         hw_len = len;
         if (!ccw.cda) {
-            ret = -EFAULT;
+            ret = CSS_DO_PGM_CHK;
         } else {
             config = cpu_physical_memory_map(ccw.cda, &hw_len, 0);
             if (!config) {
-                ret = -EFAULT;
+                ret = CSS_DO_PGM_CHK;
             } else {
                 len = hw_len;
                 /* XXX config space endianness */
@@ -515,43 +515,43 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
                 cpu_physical_memory_unmap(config, hw_len, 0, hw_len);
                 virtio_bus_set_vdev_config(&dev->bus, vdev->config);
                 sch->curr_status.scsw.count = ccw.count - len;
-                ret = 0;
+                ret = CSS_DO_SUCCESS;
             }
         }
         break;
     case CCW_CMD_READ_STATUS:
         if (check_len) {
             if (ccw.count != sizeof(status)) {
-                ret = -EINVAL;
+                ret = CSS_DO_INCORR_LEN;
                 break;
             }
         } else if (ccw.count < sizeof(status)) {
             /* Can't execute command. */
-            ret = -EINVAL;
+            ret = CSS_DO_PGM_CHK;
             break;
         }
         if (!ccw.cda) {
-            ret = -EFAULT;
+            ret = CSS_DO_PGM_CHK;
         } else {
             address_space_stb(&address_space_memory, ccw.cda, vdev->status,
                                         MEMTXATTRS_UNSPECIFIED, NULL);
             sch->curr_status.scsw.count = ccw.count - sizeof(vdev->status);;
-            ret = 0;
+            ret = CSS_DO_SUCCESS;
         }
         break;
     case CCW_CMD_WRITE_STATUS:
         if (check_len) {
             if (ccw.count != sizeof(status)) {
-                ret = -EINVAL;
+                ret = CSS_DO_INCORR_LEN;
                 break;
             }
         } else if (ccw.count < sizeof(status)) {
             /* Can't execute command. */
-            ret = -EINVAL;
+            ret = CSS_DO_PGM_CHK;
             break;
         }
         if (!ccw.cda) {
-            ret = -EFAULT;
+            ret = CSS_DO_PGM_CHK;
         } else {
             status = address_space_ldub(&address_space_memory, ccw.cda,
                                         MEMTXATTRS_UNSPECIFIED, NULL);
@@ -566,85 +566,85 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
                     virtio_ccw_start_ioeventfd(dev);
                 }
                 sch->curr_status.scsw.count = ccw.count - sizeof(status);
-                ret = 0;
+                ret = CSS_DO_SUCCESS;
             } else {
                 /* Trigger a command reject. */
-                ret = -ENOSYS;
+                ret = CSS_DO_UNIT_CHK_REJ;
             }
         }
         break;
     case CCW_CMD_SET_IND:
         if (check_len) {
             if (ccw.count != sizeof(indicators)) {
-                ret = -EINVAL;
+                ret = CSS_DO_INCORR_LEN;
                 break;
             }
         } else if (ccw.count < sizeof(indicators)) {
             /* Can't execute command. */
-            ret = -EINVAL;
+            ret = CSS_DO_PGM_CHK;
             break;
         }
         if (sch->thinint_active) {
             /* Trigger a command reject. */
-            ret = -ENOSYS;
+            ret = CSS_DO_UNIT_CHK_REJ;
             break;
         }
         if (virtio_get_num_queues(vdev) > NR_CLASSIC_INDICATOR_BITS) {
             /* More queues than indicator bits --> trigger a reject */
-            ret = -ENOSYS;
+            ret = CSS_DO_UNIT_CHK_REJ;
             break;
         }
         if (!ccw.cda) {
-            ret = -EFAULT;
+            ret = CSS_DO_PGM_CHK;
         } else {
             indicators = address_space_ldq_be(&address_space_memory, ccw.cda,
                                               MEMTXATTRS_UNSPECIFIED, NULL);
             dev->indicators = get_indicator(indicators, sizeof(uint64_t));
             sch->curr_status.scsw.count = ccw.count - sizeof(indicators);
-            ret = 0;
+            ret = CSS_DO_SUCCESS;
         }
         break;
     case CCW_CMD_SET_CONF_IND:
         if (check_len) {
             if (ccw.count != sizeof(indicators)) {
-                ret = -EINVAL;
+                ret = CSS_DO_INCORR_LEN;
                 break;
             }
         } else if (ccw.count < sizeof(indicators)) {
             /* Can't execute command. */
-            ret = -EINVAL;
+            ret = CSS_DO_PGM_CHK;
             break;
         }
         if (!ccw.cda) {
-            ret = -EFAULT;
+            ret = CSS_DO_PGM_CHK;
         } else {
             indicators = address_space_ldq_be(&address_space_memory, ccw.cda,
                                               MEMTXATTRS_UNSPECIFIED, NULL);
             dev->indicators2 = get_indicator(indicators, sizeof(uint64_t));
             sch->curr_status.scsw.count = ccw.count - sizeof(indicators);
-            ret = 0;
+            ret = CSS_DO_SUCCESS;
         }
         break;
     case CCW_CMD_READ_VQ_CONF:
         if (check_len) {
             if (ccw.count != sizeof(vq_config)) {
-                ret = -EINVAL;
+                ret = CSS_DO_INCORR_LEN;
                 break;
             }
         } else if (ccw.count < sizeof(vq_config)) {
             /* Can't execute command. */
-            ret = -EINVAL;
+            ret = CSS_DO_PGM_CHK;
             break;
         }
         if (!ccw.cda) {
-            ret = -EFAULT;
+            ret = CSS_DO_PGM_CHK;
         } else {
             vq_config.index = address_space_lduw_be(&address_space_memory,
                                                     ccw.cda,
                                                     MEMTXATTRS_UNSPECIFIED,
                                                     NULL);
             if (vq_config.index >= VIRTIO_QUEUE_MAX) {
-                ret = -EINVAL;
+                ret = CSS_DO_PGM_CHK;
                 break;
             }
             vq_config.num_max = virtio_queue_get_num(vdev,
@@ -655,31 +655,31 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
                                  MEMTXATTRS_UNSPECIFIED,
                                  NULL);
             sch->curr_status.scsw.count = ccw.count - sizeof(vq_config);
-            ret = 0;
+            ret = CSS_DO_SUCCESS;
         }
         break;
     case CCW_CMD_SET_IND_ADAPTER:
         if (check_len) {
             if (ccw.count != sizeof(*thinint)) {
-                ret = -EINVAL;
+                ret = CSS_DO_INCORR_LEN;
                 break;
             }
         } else if (ccw.count < sizeof(*thinint)) {
             /* Can't execute command. */
-            ret = -EINVAL;
+            ret = CSS_DO_PGM_CHK;
             break;
         }
         len = sizeof(*thinint);
         hw_len = len;
         if (!ccw.cda) {
-            ret = -EFAULT;
+            ret = CSS_DO_PGM_CHK;
         } else if (dev->indicators && !sch->thinint_active) {
             /* Trigger a command reject. */
-            ret = -ENOSYS;
+            ret = CSS_DO_UNIT_CHK_REJ;
         } else {
             thinint = cpu_physical_memory_map(ccw.cda, &hw_len, 0);
             if (!thinint) {
-                ret = -EFAULT;
+                ret = CSS_DO_PGM_CHK;
             } else {
                 uint64_t ind_bit = ldq_be_p(&thinint->ind_bit);
 
@@ -700,18 +700,18 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
                 sch->thinint_active = ((dev->indicators != NULL) &&
                                        (dev->summary_indicator != NULL));
                 sch->curr_status.scsw.count = ccw.count - len;
-                ret = 0;
+                ret = CSS_DO_SUCCESS;
             }
         }
         break;
     case CCW_CMD_SET_VIRTIO_REV:
         len = sizeof(revinfo);
         if (ccw.count < len) {
-            ret = -EINVAL;
+            ret = CSS_DO_PGM_CHK;
             break;
         }
         if (!ccw.cda) {
-            ret = -EFAULT;
+            ret = CSS_DO_PGM_CHK;
             break;
         }
         revinfo.revision =
@@ -723,7 +723,7 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
                                   MEMTXATTRS_UNSPECIFIED, NULL);
         if (ccw.count < len + revinfo.length ||
             (check_len && ccw.count > len + revinfo.length)) {
-            ret = -EINVAL;
+            ret = CSS_DO_PGM_CHK;
             break;
         }
         /*
@@ -733,14 +733,14 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
         if (dev->revision >= 0 ||
             revinfo.revision > virtio_ccw_rev_max(dev) ||
             (dev->force_revision_1 && !revinfo.revision)) {
-            ret = -ENOSYS;
+            ret = CSS_DO_UNIT_CHK_REJ;
             break;
         }
-        ret = 0;
+        ret = CSS_DO_SUCCESS;
         dev->revision = revinfo.revision;
         break;
     default:
-        ret = -ENOSYS;
+        ret = CSS_DO_UNIT_CHK_REJ;
         break;
     }
     return ret;
diff --git a/include/hw/s390x/css.h b/include/hw/s390x/css.h
index 0653d3c9be..da518580be 100644
--- a/include/hw/s390x/css.h
+++ b/include/hw/s390x/css.h
@@ -75,6 +75,17 @@ typedef struct CMBE {
     uint32_t reserved[7];
 } QEMU_PACKED CMBE;
 
+
+typedef enum CcwProcStatus {
+        CSS_DO_SUCCESS = 0, /* requet successful completin */
+        CSS_DO_CONT_CHAIN,  /* request continue ccw-chaning */
+        CSS_DO_SUSPEND,     /* request subchannel suspended */
+        CSS_DO_PGM_CHK,     /* request channel-program check */
+        CSS_DO_UNIT_CHK_REJ,/* request unit check cmd reject */
+        CSS_DO_INCORR_LEN,  /* request incorrect length */
+        CSS_E_CUSTOM        /* SCSW updated by device */
+} CcwProcStatus;
+
 typedef struct SubchDev SubchDev;
 struct SubchDev {
     /* channel-subsystem related things: */
@@ -93,7 +104,7 @@ struct SubchDev {
     uint16_t migrated_schid; /* used for missmatch detection */
     ORB orb;
     /* transport-provided data: */
-    int (*ccw_cb) (SubchDev *, CCW1);
+    CcwProcStatus (*ccw_cb) (SubchDev *, CCW1);
     void (*disable_cb)(SubchDev *);
     int (*do_subchannel_work) (SubchDev *);
     SenseId id;
-- 
2.13.5

Re: [Qemu-devel] [PATCH 1/4] s390x/css: drop data-check in interpretation

[Cornelia Huck]

On Fri,  8 Sep 2017 17:24:43 +0200
Halil Pasic <pasic@linux.vnet.ibm.com> wrote:

> The architecture says that channel-data check is indicating that
> an uncorrected storage (memory) error has been detected in regard
> to the data residing in main storage (memory) that is currently
> used for an I/O operation. The described detection is done using
> the CBC technology.
> 
> The ccw interpretation code is however generating a channel-data check
> effectively when the (device specific) ccw_cb returns -EFAULT.  In case
> of virtio-ccw devices this happens when mapping memory fails, or when a
> NULL pointer is encountered. So this behavior is not architecture
> conform.
> 
> Furthermore the best fit for these situations (null pointer, mapping a
> piece of guest memory fails) from architectural perspective the condition
> described as the channel subsystem refers to a location that is not
> available, which when encountered shall result in a channel-program
> check.
> 
> To fix this, all we have to do is to get rid of the switch case matching
> -EFAULT: the default is generating a channel-program check.
> 
> ---
> 
> Was posted as stand alone patch. See:
> http://patchwork.ozlabs.org/patch/810995/
> 
> If you are going to, *please review there*.

Countered. *Please review here*

(It makes it easier for me.)

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

Your s-o-b should go before the triple-dash delimiter. I can fix that
up.

> ---
>  hw/s390x/css.c | 9 ---------
>  1 file changed, 9 deletions(-)
> 
> diff --git a/hw/s390x/css.c b/hw/s390x/css.c
> index 901dc6a0f3..09f6ba0310 100644
> --- a/hw/s390x/css.c
> +++ b/hw/s390x/css.c
> @@ -980,15 +980,6 @@ static void sch_handle_start_func_virtual(SubchDev *sch)
>                      SCSW_STCTL_ALERT | SCSW_STCTL_STATUS_PEND;
>              s->cpa = sch->channel_prog + 8;
>              break;
> -        case -EFAULT:
> -            /* memory problem, generate channel data check */
> -            s->ctrl &= ~SCSW_ACTL_START_PEND;
> -            s->cstat = SCSW_CSTAT_DATA_CHECK;
> -            s->ctrl &= ~SCSW_CTRL_MASK_STCTL;
> -            s->ctrl |= SCSW_STCTL_PRIMARY | SCSW_STCTL_SECONDARY |
> -                    SCSW_STCTL_ALERT | SCSW_STCTL_STATUS_PEND;
> -            s->cpa = sch->channel_prog + 8;
> -            break;
>          case -EBUSY:
>              /* subchannel busy, generate deferred cc 1 */
>              s->flags &= ~SCSW_FLAGS_MASK_CC;

Looks reasonable. Queued.

Re: [Qemu-devel] [PATCH 2/4] s390x/css: fix NULL handling for CCW addresses

[Cornelia Huck]

On Fri,  8 Sep 2017 17:24:44 +0200
Halil Pasic <pasic@linux.vnet.ibm.com> wrote:

> Back then in the time of df1fe5bb49 ("s390: Virtual channel subsystem
> support.", 2013-01-24) -EIO used to map to a channel-program check (via
> the default label of the switch statement).  Then 2dc95b4cac
> ("s390x/3270: 3270 data stream handling", 2016-04-01) came along
> and that changed dramatically.
> 
> Let us roll back this undesired side effect, and go back to
> channel-program check.
> 
> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
> Fixes: 2dc95b4cac "s390x/3270: 3270 data stream handling"
> ---
> 
> I'm not sure 0 as CCW address  it's strictly illegal. Yes 0 is an
> unlikely address for a CCW but I would appreciate a PoP reference
> clarifying this...

0 is not an address we can fetch from, I think that is enough to
trigger a channel-program check.

> 
> Another reason to not use Unix/POSIX error codes like this.
> ---
>  hw/s390x/css.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/s390x/css.c b/hw/s390x/css.c
> index 09f6ba0310..a44d87ab3e 100644
> --- a/hw/s390x/css.c
> +++ b/hw/s390x/css.c
> @@ -793,7 +793,7 @@ static int css_interpret_ccw(SubchDev *sch, hwaddr ccw_addr,
>      CCW1 ccw;
>  
>      if (!ccw_addr) {
> -        return -EIO;
> +        return -EINVAL; /* channel-program check */
>      }
>      /* Check doubleword aligned and 31 or 24 (fmt 0) bit addressable. */
>      if (ccw_addr & (sch->ccw_fmt_1 ? 0x80000007 : 0xff000007)) {

Thanks, queued.

Re: [Qemu-devel] [PATCH 3/4] s390x/css: remove dubious error handling branch

[Cornelia Huck]

On Fri,  8 Sep 2017 17:24:45 +0200
Halil Pasic <pasic@linux.vnet.ibm.com> wrote:

s/dubious/unused/

> The case in question actually never happens. Let us get rid of the dead
> code.

I had tried to be complete in my initial implementation. With the
current implementation, it can never happen, yes. We can easily
resurrect it from git history should we need it in the future.

> 
> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
> ---
>  hw/s390x/css.c | 7 -------
>  1 file changed, 7 deletions(-)
> 
> diff --git a/hw/s390x/css.c b/hw/s390x/css.c
> index a44d87ab3e..a9cdd54efc 100644
> --- a/hw/s390x/css.c
> +++ b/hw/s390x/css.c
> @@ -980,13 +980,6 @@ static void sch_handle_start_func_virtual(SubchDev *sch)
>                      SCSW_STCTL_ALERT | SCSW_STCTL_STATUS_PEND;
>              s->cpa = sch->channel_prog + 8;
>              break;
> -        case -EBUSY:
> -            /* subchannel busy, generate deferred cc 1 */
> -            s->flags &= ~SCSW_FLAGS_MASK_CC;
> -            s->flags |= (1 << 8);
> -            s->ctrl &= ~SCSW_CTRL_MASK_STCTL;
> -            s->ctrl |= SCSW_STCTL_ALERT | SCSW_STCTL_STATUS_PEND;
> -            break;
>          case -EINPROGRESS:
>              /* channel program has been suspended */
>              s->ctrl &= ~SCSW_ACTL_START_PEND;

Thanks, queued.

Re: [Qemu-devel] [PATCH 4/4] s390x/css: fix incorrect length indication

[Cornelia Huck]

On Fri,  8 Sep 2017 17:24:46 +0200
Halil Pasic <pasic@linux.vnet.ibm.com> wrote:

> We report incorrect length via SCSW program check instead of incorrect
> length check (SCWS word 2 bit 10 instead of bit 9). Since we have there
> is no fitting errno for incorrect length, and since I don't like what we
> do with the errno's, as part of the fix, errnos used for control flow in
> ccw interpretation are replaced with an enum using more speaking names.

I'm not sure whether this is the way to go. I mainly dislike the size
of the patch (and the fact that it mixes a fix and a change of function
signature).

Can we instead choose a mapping for incorrect length, and defer a
possible rework?

(Another idea would be to have the callback prepare the scsw via helper
functions. We'd just keep -EAGAIN to keep processing a chain and 0 to
stop.)

> 
> For virtio, if incorrect length checking is suppressed we keep the
> current behavior (channel-program check).

Confused. If it is suppressed, there should not be an error, no?

> 
> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
> ---
>  hw/s390x/3270-ccw.c    |  24 +++++-----
>  hw/s390x/css.c         |  67 +++++++++++++++-----------
>  hw/s390x/virtio-ccw.c  | 128 ++++++++++++++++++++++++-------------------------
>  include/hw/s390x/css.h |  13 ++++-
>  4 files changed, 127 insertions(+), 105 deletions(-)

Re: [Qemu-devel] [PATCH 4/4] s390x/css: fix incorrect length indication

[Halil Pasic]

On 09/11/2017 12:07 PM, Cornelia Huck wrote:
> On Fri,  8 Sep 2017 17:24:46 +0200
> Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
> 
>> We report incorrect length via SCSW program check instead of incorrect
>> length check (SCWS word 2 bit 10 instead of bit 9). Since we have there
>> is no fitting errno for incorrect length, and since I don't like what we
>> do with the errno's, as part of the fix, errnos used for control flow in
>> ccw interpretation are replaced with an enum using more speaking names.
> 
> I'm not sure whether this is the way to go. I mainly dislike the size
> of the patch (and the fact that it mixes a fix and a change of function
> signature).

Do you agree that we should move away from POSIX errno codes? I think
if we do, this cant' get much smaller.

> 
> Can we instead choose a mapping for incorrect length, and defer a
> possible rework?
> 

In the commit message, I say that I don't have a fitting errno.
If you tell me which one to use, I would be glad to split this up.
I don't like mixing re-factoring and changing behavior myself.

Can I have your position on the re-factoring (that is let us
imagine I did not change handling for incorrect length)?

> (Another idea would be to have the callback prepare the scsw via helper
> functions. We'd just keep -EAGAIN to keep processing a chain and 0 to
> stop.)
> 

That was my first idea how to improve on this. I should still have the
code (patches), but I'm not sure whether it's clean or lumped together
with other experiments.

After pushing the handling down the call chain (caller would use
inline functions to manipulate SCSW), I've realized that it does
not buy us much/anything expect the better names, while we get
the machine code manipulating the SCSW generated in multiple
instead of in one place. I also showed the results to Dong Jia and
he was ambivalent too: said something like it does look better,
but it ain't better enough to make it worthwhile.

This is why I've decided to go with a less intrusive approach:
just change the names so that it's obvious what's happening.

If you like, I look for those patches and if it ain't too
much work post them as an RFC -- so we have both options in
front of our eyes. 

>>
>> For virtio, if incorrect length checking is suppressed we keep the
>> current behavior (channel-program check).
> 
> Confused. If it is suppressed, there should not be an error, no?

No.

>From VIRTIO 1.0 4.3.1.2  Device Requirements: Basic Concepts

"If a driver did suppress length checks for a channel command, the device
MUST present a check condition if the transmitted data does not contain
enough data to process the command."
(http://docs.oasis-open.org/virtio/virtio/v1.0/cs04/virtio-v1.0-cs04.html#x1-1230001)

So for virtio we have to present a check condition. Architecturally it
might look better if the one refusing is the device and not the CSS, but
for that we would have to change the VIRTIO spec. With the given
constraints a program check is IMHO the best fit.

> 
>>
>> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
>> ---
>>  hw/s390x/3270-ccw.c    |  24 +++++-----
>>  hw/s390x/css.c         |  67 +++++++++++++++-----------
>>  hw/s390x/virtio-ccw.c  | 128 ++++++++++++++++++++++++-------------------------
>>  include/hw/s390x/css.h |  13 ++++-
>>  4 files changed, 127 insertions(+), 105 deletions(-)
> 

Re: [Qemu-devel] [PATCH 3/4] s390x/css: remove dubious error handling branch

[Halil Pasic]

On 09/11/2017 11:48 AM, Cornelia Huck wrote:
> On Fri,  8 Sep 2017 17:24:45 +0200
> Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
> 
> s/dubious/unused/
> 

Nod.

>> The case in question actually never happens. Let us get rid of the dead
>> code.
> 
> I had tried to be complete in my initial implementation. With the
> current implementation, it can never happen, yes. We can easily
> resurrect it from git history should we need it in the future.
> 

I've tried to understand the branch but I failed. Weather the subchannel
is busy or not should be already decided, and if the device is busy
that is probably supposed to be indicated via device status (word 2 bit 3).

So I wrote dubious because I could not figure it out. But unused
is certainly nicer.

I think shall the need emerge the preferred way of handling would
be to use the custom handling (-EIO) branch.


>>
>> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
>> ---
>>  hw/s390x/css.c | 7 -------
>>  1 file changed, 7 deletions(-)
>>
>> diff --git a/hw/s390x/css.c b/hw/s390x/css.c
>> index a44d87ab3e..a9cdd54efc 100644
>> --- a/hw/s390x/css.c
>> +++ b/hw/s390x/css.c
>> @@ -980,13 +980,6 @@ static void sch_handle_start_func_virtual(SubchDev *sch)
>>                      SCSW_STCTL_ALERT | SCSW_STCTL_STATUS_PEND;
>>              s->cpa = sch->channel_prog + 8;
>>              break;
>> -        case -EBUSY:
>> -            /* subchannel busy, generate deferred cc 1 */
>> -            s->flags &= ~SCSW_FLAGS_MASK_CC;
>> -            s->flags |= (1 << 8);
>> -            s->ctrl &= ~SCSW_CTRL_MASK_STCTL;
>> -            s->ctrl |= SCSW_STCTL_ALERT | SCSW_STCTL_STATUS_PEND;
>> -            break;
>>          case -EINPROGRESS:
>>              /* channel program has been suspended */
>>              s->ctrl &= ~SCSW_ACTL_START_PEND;
> 
> Thanks, queued.
> 

Thanks!

Re: [Qemu-devel] [PATCH 1/4] s390x/css: drop data-check in interpretation

[Halil Pasic]

On 09/11/2017 11:33 AM, Cornelia Huck wrote:
> On Fri,  8 Sep 2017 17:24:43 +0200
> Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
> 
>> The architecture says that channel-data check is indicating that
>> an uncorrected storage (memory) error has been detected in regard
>> to the data residing in main storage (memory) that is currently
>> used for an I/O operation. The described detection is done using
>> the CBC technology.
>>
>> The ccw interpretation code is however generating a channel-data check
>> effectively when the (device specific) ccw_cb returns -EFAULT.  In case
>> of virtio-ccw devices this happens when mapping memory fails, or when a
>> NULL pointer is encountered. So this behavior is not architecture
>> conform.
>>
>> Furthermore the best fit for these situations (null pointer, mapping a
>> piece of guest memory fails) from architectural perspective the condition
>> described as the channel subsystem refers to a location that is not
>> available, which when encountered shall result in a channel-program
>> check.
>>
>> To fix this, all we have to do is to get rid of the switch case matching
>> -EFAULT: the default is generating a channel-program check.
>>
>> ---
>>
>> Was posted as stand alone patch. See:
>> http://patchwork.ozlabs.org/patch/810995/
>>
>> If you are going to, *please review there*.
> 
> Countered. *Please review here*
> 
> (It makes it easier for me.)
> 
>>
>> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
> 
> Your s-o-b should go before the triple-dash delimiter. I can fix that
> up.

Thanks! Btw. the original (there) was correct in this respect.

> 
>> ---
>>  hw/s390x/css.c | 9 ---------
>>  1 file changed, 9 deletions(-)
>>
>> diff --git a/hw/s390x/css.c b/hw/s390x/css.c
>> index 901dc6a0f3..09f6ba0310 100644
>> --- a/hw/s390x/css.c
>> +++ b/hw/s390x/css.c
>> @@ -980,15 +980,6 @@ static void sch_handle_start_func_virtual(SubchDev *sch)
>>                      SCSW_STCTL_ALERT | SCSW_STCTL_STATUS_PEND;
>>              s->cpa = sch->channel_prog + 8;
>>              break;
>> -        case -EFAULT:
>> -            /* memory problem, generate channel data check */
>> -            s->ctrl &= ~SCSW_ACTL_START_PEND;
>> -            s->cstat = SCSW_CSTAT_DATA_CHECK;
>> -            s->ctrl &= ~SCSW_CTRL_MASK_STCTL;
>> -            s->ctrl |= SCSW_STCTL_PRIMARY | SCSW_STCTL_SECONDARY |
>> -                    SCSW_STCTL_ALERT | SCSW_STCTL_STATUS_PEND;
>> -            s->cpa = sch->channel_prog + 8;
>> -            break;
>>          case -EBUSY:
>>              /* subchannel busy, generate deferred cc 1 */
>>              s->flags &= ~SCSW_FLAGS_MASK_CC;
> 
> Looks reasonable. Queued.
> 

Thanks!

Halil

Re: [Qemu-devel] [PATCH 3/4] s390x/css: remove dubious error handling branch

[Cornelia Huck]

On Mon, 11 Sep 2017 15:08:52 +0200
Halil Pasic <pasic@linux.vnet.ibm.com> wrote:

> On 09/11/2017 11:48 AM, Cornelia Huck wrote:
> > On Fri,  8 Sep 2017 17:24:45 +0200
> > Halil Pasic <pasic@linux.vnet.ibm.com> wrote:

> >> The case in question actually never happens. Let us get rid of the dead
> >> code.  
> > 
> > I had tried to be complete in my initial implementation. With the
> > current implementation, it can never happen, yes. We can easily
> > resurrect it from git history should we need it in the future.
> >   
> 
> I've tried to understand the branch but I failed. Weather the subchannel
> is busy or not should be already decided, and if the device is busy
> that is probably supposed to be indicated via device status (word 2 bit 3).
> 
> So I wrote dubious because I could not figure it out. But unused
> is certainly nicer.
> 
> I think shall the need emerge the preferred way of handling would
> be to use the custom handling (-EIO) branch.

What this is trying to do is deferred cc 1 handling. On a real system,
a ssch may return with cc 0 to indicate that the channel program has
been accepted; but when the channel subsystem actually tries to execute
it, a status is already pending. Basically, it's a race condition:
Therefore the 'deferred' cc 1.

Maybe I should have called this 'deferred subchannel busy'... but
anyway, this cannot happen in our current implementation.

Re: [Qemu-devel] [PATCH 4/4] s390x/css: fix incorrect length indication

[Cornelia Huck]

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

> On 09/11/2017 12:07 PM, Cornelia Huck wrote:
> > On Fri,  8 Sep 2017 17:24:46 +0200
> > Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
> >   
> >> We report incorrect length via SCSW program check instead of incorrect
> >> length check (SCWS word 2 bit 10 instead of bit 9). Since we have there
> >> is no fitting errno for incorrect length, and since I don't like what we
> >> do with the errno's, as part of the fix, errnos used for control flow in
> >> ccw interpretation are replaced with an enum using more speaking names.  
> > 
> > I'm not sure whether this is the way to go. I mainly dislike the size
> > of the patch (and the fact that it mixes a fix and a change of function
> > signature).  
> 
> Do you agree that we should move away from POSIX errno codes? I think
> if we do, this cant' get much smaller.

I'm not really a fan of defining our own return values, tbh.

> 
> > 
> > Can we instead choose a mapping for incorrect length, and defer a
> > possible rework?
> >   
> 
> In the commit message, I say that I don't have a fitting errno.
> If you tell me which one to use, I would be glad to split this up.
> I don't like mixing re-factoring and changing behavior myself.
> 
> Can I have your position on the re-factoring (that is let us
> imagine I did not change handling for incorrect length)?

If there is no return code that can be made to fit, we probably won't
be able to get around some kind of refactoring... but then I'd prefer
to do the refactoring first and the fix second.

> 
> > (Another idea would be to have the callback prepare the scsw via helper
> > functions. We'd just keep -EAGAIN to keep processing a chain and 0 to
> > stop.)
> >   
> 
> That was my first idea how to improve on this. I should still have the
> code (patches), but I'm not sure whether it's clean or lumped together
> with other experiments.
> 
> After pushing the handling down the call chain (caller would use
> inline functions to manipulate SCSW), I've realized that it does
> not buy us much/anything expect the better names, while we get
> the machine code manipulating the SCSW generated in multiple
> instead of in one place. I also showed the results to Dong Jia and
> he was ambivalent too: said something like it does look better,
> but it ain't better enough to make it worthwhile.
> 
> This is why I've decided to go with a less intrusive approach:
> just change the names so that it's obvious what's happening.

Something like return channel_program_check(...); or so would be quite
obvious, I think.

But yes, it will be easier to evaluate this for an actual patch ;)

> >> For virtio, if incorrect length checking is suppressed we keep the
> >> current behavior (channel-program check).  
> > 
> > Confused. If it is suppressed, there should not be an error, no?  
> 
> No.
> 
> From VIRTIO 1.0 4.3.1.2  Device Requirements: Basic Concepts
> 
> "If a driver did suppress length checks for a channel command, the device
> MUST present a check condition if the transmitted data does not contain
> enough data to process the command."
> (http://docs.oasis-open.org/virtio/virtio/v1.0/cs04/virtio-v1.0-cs04.html#x1-1230001)
> 
> So for virtio we have to present a check condition. Architecturally it
> might look better if the one refusing is the device and not the CSS, but
> for that we would have to change the VIRTIO spec. With the given
> constraints a program check is IMHO the best fit.

Ah, but that's not general length checking for virtio-ccw :)

Reword the sentence to use 'short data with incorrect length checking
suppressed' or so?

Re: [Qemu-devel] [PATCH 4/4] s390x/css: fix incorrect length indication

[Halil Pasic]

On 09/12/2017 04:37 PM, Cornelia Huck wrote:
> On Mon, 11 Sep 2017 13:36:29 +0200
> Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
> 
>> On 09/11/2017 12:07 PM, Cornelia Huck wrote:
>>> On Fri,  8 Sep 2017 17:24:46 +0200
>>> Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
>>>   
>>>> We report incorrect length via SCSW program check instead of incorrect
>>>> length check (SCWS word 2 bit 10 instead of bit 9). Since we have there
>>>> is no fitting errno for incorrect length, and since I don't like what we
>>>> do with the errno's, as part of the fix, errnos used for control flow in
>>>> ccw interpretation are replaced with an enum using more speaking names.  
>>>
>>> I'm not sure whether this is the way to go. I mainly dislike the size
>>> of the patch (and the fact that it mixes a fix and a change of function
>>> signature).  
>>
>> Do you agree that we should move away from POSIX errno codes? I think
>> if we do, this cant' get much smaller.
> 
> I'm not really a fan of defining our own return values, tbh.
> 

I've suspected. But your statement, although being useful, does
not answer my question. I think we need to agree on this question
before proceeding.

In my opinion both the EIO bug and this bug are great examples
why the POSIX errno codes are sub-optimal and misleading, but
that's my opinion.

>>
>>>
>>> Can we instead choose a mapping for incorrect length, and defer a
>>> possible rework?
>>>   
>>
>> In the commit message, I say that I don't have a fitting errno.
>> If you tell me which one to use, I would be glad to split this up.
>> I don't like mixing re-factoring and changing behavior myself.
>>
>> Can I have your position on the re-factoring (that is let us
>> imagine I did not change handling for incorrect length)?
> 
> If there is no return code that can be made to fit, we probably won't
> be able to get around some kind of refactoring... but then I'd prefer
> to do the refactoring first and the fix second.
> 

That is a can do. I dislike refactoring known bugs, because fixing
bugs is usually higher priority than making the code nicer, or even
marginally faster. (Btw I found these while trying to refactor.)
This however is a weak principle of mine and can be easily overpowered
by a maintainer request for example.

>>
>>> (Another idea would be to have the callback prepare the scsw via helper
>>> functions. We'd just keep -EAGAIN to keep processing a chain and 0 to
>>> stop.)
>>>   
>>
>> That was my first idea how to improve on this. I should still have the
>> code (patches), but I'm not sure whether it's clean or lumped together
>> with other experiments.
>>
>> After pushing the handling down the call chain (caller would use
>> inline functions to manipulate SCSW), I've realized that it does
>> not buy us much/anything expect the better names, while we get
>> the machine code manipulating the SCSW generated in multiple
>> instead of in one place. I also showed the results to Dong Jia and
>> he was ambivalent too: said something like it does look better,
>> but it ain't better enough to make it worthwhile.
>>
>> This is why I've decided to go with a less intrusive approach:
>> just change the names so that it's obvious what's happening.
> 
> Something like return channel_program_check(...); or so would be quite
> obvious, I think.
> 
> But yes, it will be easier to evaluate this for an actual patch ;)
> 

OK, I will look into this, and probably send an RFC these days.

>>>> For virtio, if incorrect length checking is suppressed we keep the
>>>> current behavior (channel-program check).  
>>>
>>> Confused. If it is suppressed, there should not be an error, no?  
>>
>> No.
>>
>> From VIRTIO 1.0 4.3.1.2  Device Requirements: Basic Concepts
>>
>> "If a driver did suppress length checks for a channel command, the device
>> MUST present a check condition if the transmitted data does not contain
>> enough data to process the command."
>> (http://docs.oasis-open.org/virtio/virtio/v1.0/cs04/virtio-v1.0-cs04.html#x1-1230001)
>>
>> So for virtio we have to present a check condition. Architecturally it
>> might look better if the one refusing is the device and not the CSS, but
>> for that we would have to change the VIRTIO spec. With the given
>> constraints a program check is IMHO the best fit.
> 
> Ah, but that's not general length checking for virtio-ccw :)

What is general length checking for virtio-ccw? Did I say it
was general length checking for virtio-ccw?

> 
> Reword the sentence to use 'short data with incorrect length checking
> suppressed' or so?
> 

Could you provide a whole sentence? I think my original sentence is OK
(purpose: indicate that virtio is special, and that we have to bend the
architecture a bit), but I agree, being a little more verbose may be a
good idea. I just can't come up with a nice sentence.

Halil

Re: [Qemu-devel] [PATCH 4/4] s390x/css: fix incorrect length indication

[Cornelia Huck]

On Tue, 12 Sep 2017 17:43:03 +0200
Halil Pasic <pasic@linux.vnet.ibm.com> wrote:

> On 09/12/2017 04:37 PM, Cornelia Huck wrote:
> > On Mon, 11 Sep 2017 13:36:29 +0200
> > Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
> >   
> >> On 09/11/2017 12:07 PM, Cornelia Huck wrote:  
> >>> On Fri,  8 Sep 2017 17:24:46 +0200
> >>> Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
> >>>     
> >>>> We report incorrect length via SCSW program check instead of incorrect
> >>>> length check (SCWS word 2 bit 10 instead of bit 9). Since we have there
> >>>> is no fitting errno for incorrect length, and since I don't like what we
> >>>> do with the errno's, as part of the fix, errnos used for control flow in
> >>>> ccw interpretation are replaced with an enum using more speaking names.    
> >>>
> >>> I'm not sure whether this is the way to go. I mainly dislike the size
> >>> of the patch (and the fact that it mixes a fix and a change of function
> >>> signature).    
> >>
> >> Do you agree that we should move away from POSIX errno codes? I think
> >> if we do, this cant' get much smaller.  
> > 
> > I'm not really a fan of defining our own return values, tbh.
> >   
> 
> I've suspected. But your statement, although being useful, does
> not answer my question. I think we need to agree on this question
> before proceeding.
> 
> In my opinion both the EIO bug and this bug are great examples
> why the POSIX errno codes are sub-optimal and misleading, but
> that's my opinion.

It depends. I prefer them over home-grown ones.

(And I tend to dislike absolute statements.)

> 
> >>  
> >>>
> >>> Can we instead choose a mapping for incorrect length, and defer a
> >>> possible rework?
> >>>     
> >>
> >> In the commit message, I say that I don't have a fitting errno.
> >> If you tell me which one to use, I would be glad to split this up.
> >> I don't like mixing re-factoring and changing behavior myself.
> >>
> >> Can I have your position on the re-factoring (that is let us
> >> imagine I did not change handling for incorrect length)?  
> > 
> > If there is no return code that can be made to fit, we probably won't
> > be able to get around some kind of refactoring... but then I'd prefer
> > to do the refactoring first and the fix second.
> >   
> 
> That is a can do. I dislike refactoring known bugs, because fixing
> bugs is usually higher priority than making the code nicer, or even
> marginally faster. (Btw I found these while trying to refactor.)
> This however is a weak principle of mine and can be easily overpowered
> by a maintainer request for example.

If a good fix requires refactoring, I'd prefer to do the refactoring
first. I'd prefer an ugly fix first only for serious issues (and I
don't think that one counts as one.)

> >>>> For virtio, if incorrect length checking is suppressed we keep the
> >>>> current behavior (channel-program check).    
> >>>
> >>> Confused. If it is suppressed, there should not be an error, no?    
> >>
> >> No.
> >>
> >> From VIRTIO 1.0 4.3.1.2  Device Requirements: Basic Concepts
> >>
> >> "If a driver did suppress length checks for a channel command, the device
> >> MUST present a check condition if the transmitted data does not contain
> >> enough data to process the command."
> >> (http://docs.oasis-open.org/virtio/virtio/v1.0/cs04/virtio-v1.0-cs04.html#x1-1230001)
> >>
> >> So for virtio we have to present a check condition. Architecturally it
> >> might look better if the one refusing is the device and not the CSS, but
> >> for that we would have to change the VIRTIO spec. With the given
> >> constraints a program check is IMHO the best fit.  
> > 
> > Ah, but that's not general length checking for virtio-ccw :)  
> 
> What is general length checking for virtio-ccw? Did I say it
> was general length checking for virtio-ccw?

Hm? Generally, suppressing is supposed to allow incorrect length
specifications. For virtio-ccw, that only applies to 'too much' and not
'not enough'.

Also, reading the statement in the spec: It only talks about a 'check
condition', not _which_ one - so there's no requirement to keep a
channel-program check (other than possibly confusing guests)?

Re: [Qemu-devel] [PATCH 4/4] s390x/css: fix incorrect length indication

[Halil Pasic]

On 09/12/2017 05:59 PM, Cornelia Huck wrote:
> On Tue, 12 Sep 2017 17:43:03 +0200
> Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
> 
>> On 09/12/2017 04:37 PM, Cornelia Huck wrote:
>>> On Mon, 11 Sep 2017 13:36:29 +0200
>>> Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
>>>   
>>>> On 09/11/2017 12:07 PM, Cornelia Huck wrote:  
>>>>> On Fri,  8 Sep 2017 17:24:46 +0200
>>>>> Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
>>>>>     
>>>>>> We report incorrect length via SCSW program check instead of incorrect
>>>>>> length check (SCWS word 2 bit 10 instead of bit 9). Since we have there
>>>>>> is no fitting errno for incorrect length, and since I don't like what we
>>>>>> do with the errno's, as part of the fix, errnos used for control flow in
>>>>>> ccw interpretation are replaced with an enum using more speaking names.    
>>>>>
>>>>> I'm not sure whether this is the way to go. I mainly dislike the size
>>>>> of the patch (and the fact that it mixes a fix and a change of function
>>>>> signature).    
>>>>
>>>> Do you agree that we should move away from POSIX errno codes? I think
>>>> if we do, this cant' get much smaller.  
>>>
>>> I'm not really a fan of defining our own return values, tbh.
>>>   
>>
>> I've suspected. But your statement, although being useful, does
>> not answer my question. I think we need to agree on this question
>> before proceeding.
>>
>> In my opinion both the EIO bug and this bug are great examples
>> why the POSIX errno codes are sub-optimal and misleading, but
>> that's my opinion.
> 
> It depends. I prefer them over home-grown ones.
> 
> (And I tend to dislike absolute statements.)
> 

Ah, we may have a misunderstanding here. POSIX errno codes are great
if they are used for what they are supposed to. The context was meant
to be implicitly included in the statement limiting it's scope.

Other than spotting a possible misunderstanding (I hope I did
not misunderstand what do you mean by absolute statements myself) this
did not bring me any further.

>>
>>>>  
>>>>>
>>>>> Can we instead choose a mapping for incorrect length, and defer a
>>>>> possible rework?
>>>>>     
>>>>
>>>> In the commit message, I say that I don't have a fitting errno.
>>>> If you tell me which one to use, I would be glad to split this up.
>>>> I don't like mixing re-factoring and changing behavior myself.
>>>>
>>>> Can I have your position on the re-factoring (that is let us
>>>> imagine I did not change handling for incorrect length)?  
>>>
>>> If there is no return code that can be made to fit, we probably won't
>>> be able to get around some kind of refactoring... but then I'd prefer
>>> to do the refactoring first and the fix second.
>>>   
>>
>> That is a can do. I dislike refactoring known bugs, because fixing
>> bugs is usually higher priority than making the code nicer, or even
>> marginally faster. (Btw I found these while trying to refactor.)
>> This however is a weak principle of mine and can be easily overpowered
>> by a maintainer request for example.
> 
> If a good fix requires refactoring, I'd prefer to do the refactoring
> first. I'd prefer an ugly fix first only for serious issues (and I
> don't think that one counts as one.)
> 

Agree, this isn't a serous issue -- I've even asked Viktor M. should
I care about it before doing this patch: along the lines do we care about
adhering to the architecture spec. if our guests are agnostic about the
difference/divergence.

>>>>>> For virtio, if incorrect length checking is suppressed we keep the
>>>>>> current behavior (channel-program check).    
>>>>>
>>>>> Confused. If it is suppressed, there should not be an error, no?    
>>>>
>>>> No.
>>>>
>>>> From VIRTIO 1.0 4.3.1.2  Device Requirements: Basic Concepts
>>>>
>>>> "If a driver did suppress length checks for a channel command, the device
>>>> MUST present a check condition if the transmitted data does not contain
>>>> enough data to process the command."
>>>> (http://docs.oasis-open.org/virtio/virtio/v1.0/cs04/virtio-v1.0-cs04.html#x1-1230001)
>>>>
>>>> So for virtio we have to present a check condition. Architecturally it
>>>> might look better if the one refusing is the device and not the CSS, but
>>>> for that we would have to change the VIRTIO spec. With the given
>>>> constraints a program check is IMHO the best fit.  
>>>
>>> Ah, but that's not general length checking for virtio-ccw :)  
>>
>> What is general length checking for virtio-ccw? Did I say it
>> was general length checking for virtio-ccw?
> 
> Hm? Generally, suppressing is supposed to allow incorrect length
> specifications. For virtio-ccw, that only applies to 'too much' and not
> 'not enough'.
> 
> Also, reading the statement in the spec: It only talks about a 'check
> condition', not _which_ one - so there's no requirement to keep a
> channel-program check (other than possibly confusing guests)?
> 
.
You are right, and I was wrong. We could also present an unit-check
(that's also check  -- and is the only one in device status. The spec
even says the 'device must present', although I device is in virtio sense
and not in PoP sense here, and does not use 'present subchannel status'
as in the previous sentence.  For a unit check I would have expected the
some sense bit specified to though (like it's specified that under
certain conditions we have to do an unit check with a command reject
(that is sense bit 0). What shall we do in your opinion?

Re: [Qemu-devel] [PATCH 4/4] s390x/css: fix incorrect length indication

[Cornelia Huck]

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

> On 09/12/2017 05:59 PM, Cornelia Huck wrote:
> > On Tue, 12 Sep 2017 17:43:03 +0200
> > Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
> >   
> >> On 09/12/2017 04:37 PM, Cornelia Huck wrote:  
> >>> On Mon, 11 Sep 2017 13:36:29 +0200
> >>> Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
> >>>     
> >>>> On 09/11/2017 12:07 PM, Cornelia Huck wrote:    
> >>>>> On Fri,  8 Sep 2017 17:24:46 +0200
> >>>>> Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
> >>>>>       
> >>>>>> We report incorrect length via SCSW program check instead of incorrect
> >>>>>> length check (SCWS word 2 bit 10 instead of bit 9). Since we have there
> >>>>>> is no fitting errno for incorrect length, and since I don't like what we
> >>>>>> do with the errno's, as part of the fix, errnos used for control flow in
> >>>>>> ccw interpretation are replaced with an enum using more speaking names.      
> >>>>>
> >>>>> I'm not sure whether this is the way to go. I mainly dislike the size
> >>>>> of the patch (and the fact that it mixes a fix and a change of function
> >>>>> signature).      
> >>>>
> >>>> Do you agree that we should move away from POSIX errno codes? I think
> >>>> if we do, this cant' get much smaller.    
> >>>
> >>> I'm not really a fan of defining our own return values, tbh.
> >>>     
> >>
> >> I've suspected. But your statement, although being useful, does
> >> not answer my question. I think we need to agree on this question
> >> before proceeding.
> >>
> >> In my opinion both the EIO bug and this bug are great examples
> >> why the POSIX errno codes are sub-optimal and misleading, but
> >> that's my opinion.  
> > 
> > It depends. I prefer them over home-grown ones.
> > 
> > (And I tend to dislike absolute statements.)
> >   
> 
> Ah, we may have a misunderstanding here. POSIX errno codes are great
> if they are used for what they are supposed to. The context was meant
> to be implicitly included in the statement limiting it's scope.
> 
> Other than spotting a possible misunderstanding (I hope I did
> not misunderstand what do you mean by absolute statements myself) this
> did not bring me any further.

As said, I'd probably prefer the alternative approach, as I'm not
really a fan of defining a set of return codes.


> >>>>>> For virtio, if incorrect length checking is suppressed we keep the
> >>>>>> current behavior (channel-program check).      
> >>>>>
> >>>>> Confused. If it is suppressed, there should not be an error, no?      
> >>>>
> >>>> No.
> >>>>
> >>>> From VIRTIO 1.0 4.3.1.2  Device Requirements: Basic Concepts
> >>>>
> >>>> "If a driver did suppress length checks for a channel command, the device
> >>>> MUST present a check condition if the transmitted data does not contain
> >>>> enough data to process the command."
> >>>> (http://docs.oasis-open.org/virtio/virtio/v1.0/cs04/virtio-v1.0-cs04.html#x1-1230001)
> >>>>
> >>>> So for virtio we have to present a check condition. Architecturally it
> >>>> might look better if the one refusing is the device and not the CSS, but
> >>>> for that we would have to change the VIRTIO spec. With the given
> >>>> constraints a program check is IMHO the best fit.    
> >>>
> >>> Ah, but that's not general length checking for virtio-ccw :)    
> >>
> >> What is general length checking for virtio-ccw? Did I say it
> >> was general length checking for virtio-ccw?  
> > 
> > Hm? Generally, suppressing is supposed to allow incorrect length
> > specifications. For virtio-ccw, that only applies to 'too much' and not
> > 'not enough'.
> > 
> > Also, reading the statement in the spec: It only talks about a 'check
> > condition', not _which_ one - so there's no requirement to keep a
> > channel-program check (other than possibly confusing guests)?
> >   
> .
> You are right, and I was wrong. We could also present an unit-check
> (that's also check  -- and is the only one in device status. The spec
> even says the 'device must present', although I device is in virtio sense
> and not in PoP sense here, and does not use 'present subchannel status'
> as in the previous sentence.  For a unit check I would have expected the
> some sense bit specified to though (like it's specified that under
> certain conditions we have to do an unit check with a command reject
> (that is sense bit 0). What shall we do in your opinion?

Whatever matches both the architecture and the qemu code flow best ;)

If we can present an incorrect-length check on short data even with
suppression set, that seems like the most consistent one.

Else I would keep the current behaviour. Defining a sense bit (we'd
need to spec that) seems like overkill to me.