qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v1 0/1] s390x: css: report errors from ccw_dstream_read/write
@ 2021-04-06  7:44 Pierre Morel
  2021-04-06  7:44 ` [PATCH v1 1/1] " Pierre Morel
  0 siblings, 1 reply; 13+ messages in thread
From: Pierre Morel @ 2021-04-06  7:44 UTC (permalink / raw)
  To: qemu-devel
  Cc: thuth, frankja, david, cohuck, richard.henderson, pasic,
	borntraeger, qemu-s390x, mst, pbonzini, marcandre.lureau,
	imbrenda

By checking the results of errors on SSCH in the kvm-unit-tests
We noticed that no error was reported when a SSCH is started
to access addresses not existing in the guest.
For exemple accessing 3G on a guest with 1G memory.

If we look at QEMU ccw_dstream_write/write functions we see that they
are often not checked for error in various places.
  
It follows that accessing an invalid address does not trigger a
subchannel status program check to the guest as it should.

Regards,
Pierre
 

Pierre Morel (1):
  s390x: css: report errors from ccw_dstream_read/write

 hw/char/terminal3270.c | 11 +++++--
 hw/s390x/3270-ccw.c    |  3 ++
 hw/s390x/css.c         | 16 +++++-----
 hw/s390x/virtio-ccw.c  | 66 ++++++++++++++++++++++++++++++------------
 4 files changed, 69 insertions(+), 27 deletions(-)

-- 
2.17.1



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

* [PATCH v1 1/1] s390x: css: report errors from ccw_dstream_read/write
  2021-04-06  7:44 [PATCH v1 0/1] s390x: css: report errors from ccw_dstream_read/write Pierre Morel
@ 2021-04-06  7:44 ` Pierre Morel
  2021-04-06 15:03   ` Cornelia Huck
  2021-04-07 17:47   ` Halil Pasic
  0 siblings, 2 replies; 13+ messages in thread
From: Pierre Morel @ 2021-04-06  7:44 UTC (permalink / raw)
  To: qemu-devel
  Cc: thuth, frankja, david, cohuck, richard.henderson, pasic,
	borntraeger, qemu-s390x, mst, pbonzini, marcandre.lureau,
	imbrenda

ccw_dstream_read/write functions returned values are sometime
not taking into account and reported back to the upper level
of interpretation of CCW instructions.

It follows that accessing an invalid address does not trigger
a subchannel status program check to the guest as it should.

Let's test the return values of ccw_dstream_write[_buf] and
ccw_dstream_read[_buf] and report it to the caller.

Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
---
 hw/char/terminal3270.c | 11 +++++--
 hw/s390x/3270-ccw.c    |  3 ++
 hw/s390x/css.c         | 16 +++++-----
 hw/s390x/virtio-ccw.c  | 66 ++++++++++++++++++++++++++++++------------
 4 files changed, 69 insertions(+), 27 deletions(-)

diff --git a/hw/char/terminal3270.c b/hw/char/terminal3270.c
index a9a46c8ed3..82e85fac2e 100644
--- a/hw/char/terminal3270.c
+++ b/hw/char/terminal3270.c
@@ -200,9 +200,13 @@ static int read_payload_3270(EmulatedCcw3270Device *dev)
 {
     Terminal3270 *t = TERMINAL_3270(dev);
     int len;
+    int ret;
 
     len = MIN(ccw_dstream_avail(get_cds(t)), t->in_len);
-    ccw_dstream_write_buf(get_cds(t), t->inv, len);
+    ret = ccw_dstream_write_buf(get_cds(t), t->inv, len);
+    if (ret < 0) {
+        return ret;
+    }
     t->in_len -= len;
 
     return len;
@@ -260,7 +264,10 @@ static int write_payload_3270(EmulatedCcw3270Device *dev, uint8_t cmd)
 
     t->outv[out_len++] = cmd;
     do {
-        ccw_dstream_read_buf(get_cds(t), &t->outv[out_len], len);
+        retval = ccw_dstream_read_buf(get_cds(t), &t->outv[out_len], len);
+        if (retval < 0) {
+            return retval;
+        }
         count = ccw_dstream_avail(get_cds(t));
         out_len += len;
 
diff --git a/hw/s390x/3270-ccw.c b/hw/s390x/3270-ccw.c
index 821319eee6..cc1371f01c 100644
--- a/hw/s390x/3270-ccw.c
+++ b/hw/s390x/3270-ccw.c
@@ -31,6 +31,9 @@ static int handle_payload_3270_read(EmulatedCcw3270Device *dev, CCW1 *ccw)
     }
 
     len = ck->read_payload_3270(dev);
+    if (len < 0) {
+        return len;
+    }
     ccw_dev->sch->curr_status.scsw.count = ccw->count - len;
 
     return 0;
diff --git a/hw/s390x/css.c b/hw/s390x/css.c
index fe47751df4..99e476f193 100644
--- a/hw/s390x/css.c
+++ b/hw/s390x/css.c
@@ -1055,10 +1055,11 @@ static int css_interpret_ccw(SubchDev *sch, hwaddr ccw_addr,
             }
         }
         len = MIN(ccw.count, sizeof(sch->sense_data));
-        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;
+        ret = ccw_dstream_write_buf(&sch->cds, sch->sense_data, len);
+        if (!ret) {
+            sch->curr_status.scsw.count = ccw_dstream_residual_count(&sch->cds);
+            memset(sch->sense_data, 0, sizeof(sch->sense_data));
+        }
         break;
     case CCW_CMD_SENSE_ID:
     {
@@ -1083,9 +1084,10 @@ static int css_interpret_ccw(SubchDev *sch, hwaddr ccw_addr,
         } else {
             sense_id[0] = 0;
         }
-        ccw_dstream_write_buf(&sch->cds, sense_id, len);
-        sch->curr_status.scsw.count = ccw_dstream_residual_count(&sch->cds);
-        ret = 0;
+        ret = ccw_dstream_write_buf(&sch->cds, sense_id, len);
+        if (!ret) {
+            sch->curr_status.scsw.count = ccw_dstream_residual_count(&sch->cds);
+        }
         break;
     }
     case CCW_CMD_TIC:
diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
index 314ed7b245..8195f3546e 100644
--- a/hw/s390x/virtio-ccw.c
+++ b/hw/s390x/virtio-ccw.c
@@ -288,14 +288,20 @@ static int virtio_ccw_handle_set_vq(SubchDev *sch, CCW1 ccw, bool check_len,
         return -EFAULT;
     }
     if (is_legacy) {
-        ccw_dstream_read(&sch->cds, linfo);
+        ret = ccw_dstream_read(&sch->cds, linfo);
+        if (ret) {
+            return ret;
+        }
         linfo.queue = be64_to_cpu(linfo.queue);
         linfo.align = be32_to_cpu(linfo.align);
         linfo.index = be16_to_cpu(linfo.index);
         linfo.num = be16_to_cpu(linfo.num);
         ret = virtio_ccw_set_vqs(sch, NULL, &linfo);
     } else {
-        ccw_dstream_read(&sch->cds, info);
+        ret = ccw_dstream_read(&sch->cds, info);
+        if (ret) {
+            return ret;
+        }
         info.desc = be64_to_cpu(info.desc);
         info.index = be16_to_cpu(info.index);
         info.num = be16_to_cpu(info.num);
@@ -371,7 +377,10 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
             VirtioDeviceClass *vdc = VIRTIO_DEVICE_GET_CLASS(vdev);
 
             ccw_dstream_advance(&sch->cds, sizeof(features.features));
-            ccw_dstream_read(&sch->cds, features.index);
+            ret = ccw_dstream_read(&sch->cds, features.index);
+            if (ret) {
+                break;
+            }
             if (features.index == 0) {
                 if (dev->revision >= 1) {
                     /* Don't offer legacy features for modern devices. */
@@ -392,9 +401,10 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
             }
             ccw_dstream_rewind(&sch->cds);
             features.features = cpu_to_le32(features.features);
-            ccw_dstream_write(&sch->cds, features.features);
-            sch->curr_status.scsw.count = ccw.count - sizeof(features);
-            ret = 0;
+            ret = ccw_dstream_write(&sch->cds, features.features);
+            if (!ret) {
+                sch->curr_status.scsw.count = ccw.count - sizeof(features);
+            }
         }
         break;
     case CCW_CMD_WRITE_FEAT:
@@ -411,7 +421,10 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
         if (!ccw.cda) {
             ret = -EFAULT;
         } else {
-            ccw_dstream_read(&sch->cds, features);
+            ret = ccw_dstream_read(&sch->cds, features);
+            if (ret) {
+                break;
+            }
             features.features = le32_to_cpu(features.features);
             if (features.index == 0) {
                 virtio_set_features(vdev,
@@ -454,9 +467,10 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
             ret = -EFAULT;
         } else {
             virtio_bus_get_vdev_config(&dev->bus, vdev->config);
-            ccw_dstream_write_buf(&sch->cds, vdev->config, len);
-            sch->curr_status.scsw.count = ccw.count - len;
-            ret = 0;
+            ret = ccw_dstream_write_buf(&sch->cds, vdev->config, len);
+            if (ret) {
+                sch->curr_status.scsw.count = ccw.count - len;
+            }
         }
         break;
     case CCW_CMD_WRITE_CONF:
@@ -511,7 +525,10 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
         if (!ccw.cda) {
             ret = -EFAULT;
         } else {
-            ccw_dstream_read(&sch->cds, status);
+            ret = ccw_dstream_read(&sch->cds, status);
+            if (ret) {
+                break;
+            }
             if (!(status & VIRTIO_CONFIG_S_DRIVER_OK)) {
                 virtio_ccw_stop_ioeventfd(dev);
             }
@@ -554,7 +571,10 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
         if (!ccw.cda) {
             ret = -EFAULT;
         } else {
-            ccw_dstream_read(&sch->cds, indicators);
+            ret = ccw_dstream_read(&sch->cds, indicators);
+            if (ret) {
+                break;
+            }
             indicators = be64_to_cpu(indicators);
             dev->indicators = get_indicator(indicators, sizeof(uint64_t));
             sch->curr_status.scsw.count = ccw.count - sizeof(indicators);
@@ -575,7 +595,10 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
         if (!ccw.cda) {
             ret = -EFAULT;
         } else {
-            ccw_dstream_read(&sch->cds, indicators);
+            ret = ccw_dstream_read(&sch->cds, indicators);
+            if (ret) {
+                break;
+            }
             indicators = be64_to_cpu(indicators);
             dev->indicators2 = get_indicator(indicators, sizeof(uint64_t));
             sch->curr_status.scsw.count = ccw.count - sizeof(indicators);
@@ -596,7 +619,10 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
         if (!ccw.cda) {
             ret = -EFAULT;
         } else {
-            ccw_dstream_read(&sch->cds, vq_config.index);
+            ret = ccw_dstream_read(&sch->cds, vq_config.index);
+            if (ret) {
+                break;
+            }
             vq_config.index = be16_to_cpu(vq_config.index);
             if (vq_config.index >= VIRTIO_QUEUE_MAX) {
                 ret = -EINVAL;
@@ -605,9 +631,10 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
             vq_config.num_max = virtio_queue_get_num(vdev,
                                                      vq_config.index);
             vq_config.num_max = cpu_to_be16(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;
+            ret = ccw_dstream_write(&sch->cds, vq_config.num_max);
+            if (!ret) {
+                sch->curr_status.scsw.count = ccw.count - sizeof(vq_config);
+            }
         }
         break;
     case CCW_CMD_SET_IND_ADAPTER:
@@ -664,7 +691,10 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
             ret = -EFAULT;
             break;
         }
-        ccw_dstream_read_buf(&sch->cds, &revinfo, 4);
+        ret = ccw_dstream_read_buf(&sch->cds, &revinfo, 4);
+        if (ret < 0) {
+            break;
+        }
         revinfo.revision = be16_to_cpu(revinfo.revision);
         revinfo.length = be16_to_cpu(revinfo.length);
         if (ccw.count < len + revinfo.length ||
-- 
2.17.1



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

* Re: [PATCH v1 1/1] s390x: css: report errors from ccw_dstream_read/write
  2021-04-06  7:44 ` [PATCH v1 1/1] " Pierre Morel
@ 2021-04-06 15:03   ` Cornelia Huck
  2021-04-07 11:41     ` Pierre Morel
  2021-04-07 17:47   ` Halil Pasic
  1 sibling, 1 reply; 13+ messages in thread
From: Cornelia Huck @ 2021-04-06 15:03 UTC (permalink / raw)
  To: Pierre Morel
  Cc: thuth, frankja, david, mst, richard.henderson, qemu-devel, pasic,
	borntraeger, qemu-s390x, pbonzini, marcandre.lureau, imbrenda

On Tue,  6 Apr 2021 09:44:13 +0200
Pierre Morel <pmorel@linux.ibm.com> wrote:

> ccw_dstream_read/write functions returned values are sometime
> not taking into account and reported back to the upper level
> of interpretation of CCW instructions.
> 
> It follows that accessing an invalid address does not trigger
> a subchannel status program check to the guest as it should.
> 
> Let's test the return values of ccw_dstream_write[_buf] and
> ccw_dstream_read[_buf] and report it to the caller.

Yes, checking/propagating the return code is something that was missing
in several places.

> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>  hw/char/terminal3270.c | 11 +++++--
>  hw/s390x/3270-ccw.c    |  3 ++
>  hw/s390x/css.c         | 16 +++++-----
>  hw/s390x/virtio-ccw.c  | 66 ++++++++++++++++++++++++++++++------------
>  4 files changed, 69 insertions(+), 27 deletions(-)
> 
> diff --git a/hw/char/terminal3270.c b/hw/char/terminal3270.c
> index a9a46c8ed3..82e85fac2e 100644
> --- a/hw/char/terminal3270.c
> +++ b/hw/char/terminal3270.c
> @@ -200,9 +200,13 @@ static int read_payload_3270(EmulatedCcw3270Device *dev)
>  {
>      Terminal3270 *t = TERMINAL_3270(dev);
>      int len;
> +    int ret;
>  
>      len = MIN(ccw_dstream_avail(get_cds(t)), t->in_len);
> -    ccw_dstream_write_buf(get_cds(t), t->inv, len);
> +    ret = ccw_dstream_write_buf(get_cds(t), t->inv, len);
> +    if (ret < 0) {
> +        return ret;
> +    }

This looks correct: together with the change below, you end up
propagating a negative error value to the ccw callback handling.

>      t->in_len -= len;
>  
>      return len;
> @@ -260,7 +264,10 @@ static int write_payload_3270(EmulatedCcw3270Device *dev, uint8_t cmd)
>  
>      t->outv[out_len++] = cmd;
>      do {
> -        ccw_dstream_read_buf(get_cds(t), &t->outv[out_len], len);
> +        retval = ccw_dstream_read_buf(get_cds(t), &t->outv[out_len], len);
> +        if (retval < 0) {
> +            return retval;

Here, however, I'm not sure. Returning a negative error here is fine,
but handle_payload_3270_write (not changed in this patch) seems to
match everything to -EIO. Shouldn't it just be propagated, and maybe 0
mapped to -EIO only? If I'm not confused, we'll end up mapping every
error to intervention required.

> +        }
>          count = ccw_dstream_avail(get_cds(t));
>          out_len += len;
>  
> diff --git a/hw/s390x/3270-ccw.c b/hw/s390x/3270-ccw.c
> index 821319eee6..cc1371f01c 100644
> --- a/hw/s390x/3270-ccw.c
> +++ b/hw/s390x/3270-ccw.c
> @@ -31,6 +31,9 @@ static int handle_payload_3270_read(EmulatedCcw3270Device *dev, CCW1 *ccw)
>      }
>  
>      len = ck->read_payload_3270(dev);
> +    if (len < 0) {
> +        return len;
> +    }
>      ccw_dev->sch->curr_status.scsw.count = ccw->count - len;
>  
>      return 0;
> diff --git a/hw/s390x/css.c b/hw/s390x/css.c
> index fe47751df4..99e476f193 100644
> --- a/hw/s390x/css.c
> +++ b/hw/s390x/css.c
> @@ -1055,10 +1055,11 @@ static int css_interpret_ccw(SubchDev *sch, hwaddr ccw_addr,
>              }
>          }
>          len = MIN(ccw.count, sizeof(sch->sense_data));
> -        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;
> +        ret = ccw_dstream_write_buf(&sch->cds, sch->sense_data, len);
> +        if (!ret) {
> +            sch->curr_status.scsw.count = ccw_dstream_residual_count(&sch->cds);

I'm wondering about the residual count here. Table 16-7 "Contents of
Count Field in SCSW" in the PoP looks a bit more complicated for some
error conditions, especially if IDALs are involved. Not sure if we
should attempt to write something to count in those conditions; but on
the other hand, I don't think our error conditions are as complex
anyway, and we can make this simplification.

> +            memset(sch->sense_data, 0, sizeof(sch->sense_data));
> +        }
>          break;
>      case CCW_CMD_SENSE_ID:
>      {
> @@ -1083,9 +1084,10 @@ static int css_interpret_ccw(SubchDev *sch, hwaddr ccw_addr,
>          } else {
>              sense_id[0] = 0;
>          }
> -        ccw_dstream_write_buf(&sch->cds, sense_id, len);
> -        sch->curr_status.scsw.count = ccw_dstream_residual_count(&sch->cds);
> -        ret = 0;
> +        ret = ccw_dstream_write_buf(&sch->cds, sense_id, len);
> +        if (!ret) {
> +            sch->curr_status.scsw.count = ccw_dstream_residual_count(&sch->cds);
> +        }
>          break;
>      }
>      case CCW_CMD_TIC:

(...)

Otherwise, looks good.



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

* Re: [PATCH v1 1/1] s390x: css: report errors from ccw_dstream_read/write
  2021-04-06 15:03   ` Cornelia Huck
@ 2021-04-07 11:41     ` Pierre Morel
  2021-04-07 16:54       ` Halil Pasic
  0 siblings, 1 reply; 13+ messages in thread
From: Pierre Morel @ 2021-04-07 11:41 UTC (permalink / raw)
  To: Cornelia Huck
  Cc: thuth, frankja, david, mst, richard.henderson, qemu-devel, pasic,
	borntraeger, qemu-s390x, pbonzini, marcandre.lureau, imbrenda



On 4/6/21 5:03 PM, Cornelia Huck wrote:
> On Tue,  6 Apr 2021 09:44:13 +0200
> Pierre Morel <pmorel@linux.ibm.com> wrote:
> 
>> ccw_dstream_read/write functions returned values are sometime
>> not taking into account and reported back to the upper level
>> of interpretation of CCW instructions.
>>
>> It follows that accessing an invalid address does not trigger
>> a subchannel status program check to the guest as it should.
>>
>> Let's test the return values of ccw_dstream_write[_buf] and
>> ccw_dstream_read[_buf] and report it to the caller.
> 
> Yes, checking/propagating the return code is something that was missing
> in several places.
> 
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> ---
>>   hw/char/terminal3270.c | 11 +++++--
>>   hw/s390x/3270-ccw.c    |  3 ++
>>   hw/s390x/css.c         | 16 +++++-----
>>   hw/s390x/virtio-ccw.c  | 66 ++++++++++++++++++++++++++++++------------
>>   4 files changed, 69 insertions(+), 27 deletions(-)
>>
>> diff --git a/hw/char/terminal3270.c b/hw/char/terminal3270.c
>> index a9a46c8ed3..82e85fac2e 100644
>> --- a/hw/char/terminal3270.c
>> +++ b/hw/char/terminal3270.c
>> @@ -200,9 +200,13 @@ static int read_payload_3270(EmulatedCcw3270Device *dev)
>>   {
>>       Terminal3270 *t = TERMINAL_3270(dev);
>>       int len;
>> +    int ret;
>>   
>>       len = MIN(ccw_dstream_avail(get_cds(t)), t->in_len);
>> -    ccw_dstream_write_buf(get_cds(t), t->inv, len);
>> +    ret = ccw_dstream_write_buf(get_cds(t), t->inv, len);
>> +    if (ret < 0) {
>> +        return ret;
>> +    }
> 
> This looks correct: together with the change below, you end up
> propagating a negative error value to the ccw callback handling.
> 
>>       t->in_len -= len;
>>   
>>       return len;
>> @@ -260,7 +264,10 @@ static int write_payload_3270(EmulatedCcw3270Device *dev, uint8_t cmd)
>>   
>>       t->outv[out_len++] = cmd;
>>       do {
>> -        ccw_dstream_read_buf(get_cds(t), &t->outv[out_len], len);
>> +        retval = ccw_dstream_read_buf(get_cds(t), &t->outv[out_len], len);
>> +        if (retval < 0) {
>> +            return retval;
> 
> Here, however, I'm not sure. Returning a negative error here is fine,
> but handle_payload_3270_write (not changed in this patch) seems to
> match everything to -EIO. Shouldn't it just be propagated, and maybe 0
> mapped to -EIO only? If I'm not confused, we'll end up mapping every
> error to intervention required.

I know very little about the 3270 but in my opinion accessing memory is 
not the problem of the device but of the subchannel.
So we should certainly propagate the error instead of returning -EIO for 
any error.

---> what about

diff --git a/hw/s390x/3270-ccw.c b/hw/s390x/3270-ccw.c
index cc1371f01c..f3e7342b1e 100644
--- a/hw/s390x/3270-ccw.c
+++ b/hw/s390x/3270-ccw.c
@@ -53,7 +53,7 @@ static int 
handle_payload_3270_write(EmulatedCcw3270Device *dev, CCW1 *ccw)
      len = ck->write_payload_3270(dev, ccw->cmd_code);

      if (len <= 0) {
-        return -EIO;
+        return len ? len : -EIO;
      }

      ccw_dev->sch->curr_status.scsw.count = ccw->count - len;

-----------------

> 
>> +        }
>>           count = ccw_dstream_avail(get_cds(t));
>>           out_len += len;
>>   
>> diff --git a/hw/s390x/3270-ccw.c b/hw/s390x/3270-ccw.c
>> index 821319eee6..cc1371f01c 100644
>> --- a/hw/s390x/3270-ccw.c
>> +++ b/hw/s390x/3270-ccw.c
>> @@ -31,6 +31,9 @@ static int handle_payload_3270_read(EmulatedCcw3270Device *dev, CCW1 *ccw)
>>       }
>>   
>>       len = ck->read_payload_3270(dev);
>> +    if (len < 0) {
>> +        return len;
>> +    }
>>       ccw_dev->sch->curr_status.scsw.count = ccw->count - len;
>>   
>>       return 0;
>> diff --git a/hw/s390x/css.c b/hw/s390x/css.c
>> index fe47751df4..99e476f193 100644
>> --- a/hw/s390x/css.c
>> +++ b/hw/s390x/css.c
>> @@ -1055,10 +1055,11 @@ static int css_interpret_ccw(SubchDev *sch, hwaddr ccw_addr,
>>               }
>>           }
>>           len = MIN(ccw.count, sizeof(sch->sense_data));
>> -        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;
>> +        ret = ccw_dstream_write_buf(&sch->cds, sch->sense_data, len);
>> +        if (!ret) {
>> +            sch->curr_status.scsw.count = ccw_dstream_residual_count(&sch->cds);
> 
> I'm wondering about the residual count here. Table 16-7 "Contents of
> Count Field in SCSW" in the PoP looks a bit more complicated for some
> error conditions, especially if IDALs are involved. Not sure if we
> should attempt to write something to count in those conditions; but on
> the other hand, I don't think our error conditions are as complex
> anyway, and we can make this simplification.

I think you are right.
ccw_dstream_residual_count() should know what to do return there and we 
should not ignore it.

On the other hand erasing should only be done if all went OK because a 
following sense, with the right parameters, may succeed.
What do you think?


> 
>> +            memset(sch->sense_data, 0, sizeof(sch->sense_data));
>> +        }
>>           break;
>>       case CCW_CMD_SENSE_ID:
>>       {
>> @@ -1083,9 +1084,10 @@ static int css_interpret_ccw(SubchDev *sch, hwaddr ccw_addr,
>>           } else {
>>               sense_id[0] = 0;
>>           }
>> -        ccw_dstream_write_buf(&sch->cds, sense_id, len);
>> -        sch->curr_status.scsw.count = ccw_dstream_residual_count(&sch->cds);
>> -        ret = 0;
>> +        ret = ccw_dstream_write_buf(&sch->cds, sense_id, len);
>> +        if (!ret) {
>> +            sch->curr_status.scsw.count = ccw_dstream_residual_count(&sch->cds);
>> +        }
>>           break;
>>       }
>>       case CCW_CMD_TIC:
> 
> (...)
> 
> Otherwise, looks good.
> 

Thanks,
Pierre


-- 
Pierre Morel
IBM Lab Boeblingen


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

* Re: [PATCH v1 1/1] s390x: css: report errors from ccw_dstream_read/write
  2021-04-07 11:41     ` Pierre Morel
@ 2021-04-07 16:54       ` Halil Pasic
  2021-04-08  8:53         ` Cornelia Huck
  0 siblings, 1 reply; 13+ messages in thread
From: Halil Pasic @ 2021-04-07 16:54 UTC (permalink / raw)
  To: Pierre Morel
  Cc: thuth, frankja, mst, Cornelia Huck, richard.henderson,
	qemu-devel, borntraeger, qemu-s390x, marcandre.lureau, pbonzini,
	david, imbrenda

On Wed, 7 Apr 2021 13:41:57 +0200
Pierre Morel <pmorel@linux.ibm.com> wrote:

> > Here, however, I'm not sure. Returning a negative error here is fine,
> > but handle_payload_3270_write (not changed in this patch) seems to
> > match everything to -EIO. Shouldn't it just be propagated, and maybe 0
> > mapped to -EIO only? If I'm not confused, we'll end up mapping every
> > error to intervention required.  
> 
> I know very little about the 3270 but in my opinion accessing memory is 
> not the problem of the device but of the subchannel.
> So we should certainly propagate the error instead of returning -EIO for 
> any error.

I agree, what condition needs to be indicated when we encounter an
invalid CCW, IDAW or MIDAW address is clear. In the PoP this is
described under Chapter 16. I/O Interruptions > Subchannel-Status Word >
Subchannel-Status Field > Program Check in the paragraphs on "Invalid
IDAW or MIDAW Address and "Invalid CCW Address".

My guess about handle_payload_3270_write() is that IMHO the initial 3270
emulation code was, let's say of mixed quality in the first place, so
wouldn't seek some hidden meaning/sense, behind the -EIO. IMHO first
mapping architectural conditions to "errno-style" error codes, passing
these around, and then mapping them back is a litle non-intuitive. If one
looks at the purpose of handle_payload_3270_write(), it is basically
emulating an IO data transfer from the device into the main storage. If
that sort of operation. So with -EIO the original author probably wanted
to say: hey there was an input/output error, which ain't that wrong. The
problem is that somewhere up the call stack EIO gets interpreted in a
very peculiar way, and along with other errnos mapped to CIO
architecture stuff like SCSW bits.

Regards,
Halil


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

* Re: [PATCH v1 1/1] s390x: css: report errors from ccw_dstream_read/write
  2021-04-06  7:44 ` [PATCH v1 1/1] " Pierre Morel
  2021-04-06 15:03   ` Cornelia Huck
@ 2021-04-07 17:47   ` Halil Pasic
  2021-04-08  9:02     ` Cornelia Huck
  1 sibling, 1 reply; 13+ messages in thread
From: Halil Pasic @ 2021-04-07 17:47 UTC (permalink / raw)
  To: Pierre Morel
  Cc: thuth, frankja, david, cohuck, richard.henderson, qemu-devel,
	borntraeger, qemu-s390x, mst, pbonzini, marcandre.lureau,
	imbrenda

On Tue,  6 Apr 2021 09:44:13 +0200
Pierre Morel <pmorel@linux.ibm.com> wrote:

> ccw_dstream_read/write functions returned values are sometime
> not taking into account and reported back to the upper level
> of interpretation of CCW instructions.


The return values of ccw_dstream_write/read were intentionally ignored
in commit f57ba05823 ("virtio-ccw: use ccw data stream") to avoid
changes in behavior that wound not contribute to the objective of the
patch-set, like silently doing more error checking and handling.

If we consider the first hunk:


--- a/hw/s390x/virtio-ccw.c
+++ b/hw/s390x/virtio-ccw.c
@@ -289,49 +289,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);

we can see, that the original code did not contain any error checking regarding
the invalidity of the guest physical address.

What was the behavior there? The last argument of address_space_* where we pass
the NULL is actually an optional pointer to the MemTxResult, which would tell
us if the operation succeeded or failed. Passing NULL there means, we don't care.

My guess is that when those loads fail (or the read fails) we will just carry on
with the garbage we found on the stack.

So this begs the question, do we need this fixed for old releases as well?

My answer is yes we do. Conny what do you think?

Regards,
Halil



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

* Re: [PATCH v1 1/1] s390x: css: report errors from ccw_dstream_read/write
  2021-04-07 16:54       ` Halil Pasic
@ 2021-04-08  8:53         ` Cornelia Huck
  0 siblings, 0 replies; 13+ messages in thread
From: Cornelia Huck @ 2021-04-08  8:53 UTC (permalink / raw)
  To: Halil Pasic
  Cc: thuth, frankja, Pierre Morel, mst, david, richard.henderson,
	qemu-devel, borntraeger, qemu-s390x, marcandre.lureau, pbonzini,
	imbrenda

On Wed, 7 Apr 2021 18:54:26 +0200
Halil Pasic <pasic@linux.ibm.com> wrote:

> On Wed, 7 Apr 2021 13:41:57 +0200
> Pierre Morel <pmorel@linux.ibm.com> wrote:
> 
> > > Here, however, I'm not sure. Returning a negative error here is fine,
> > > but handle_payload_3270_write (not changed in this patch) seems to
> > > match everything to -EIO. Shouldn't it just be propagated, and maybe 0
> > > mapped to -EIO only? If I'm not confused, we'll end up mapping every
> > > error to intervention required.    
> > 
> > I know very little about the 3270 but in my opinion accessing memory is 
> > not the problem of the device but of the subchannel.
> > So we should certainly propagate the error instead of returning -EIO for 
> > any error.  
> 
> I agree, what condition needs to be indicated when we encounter an
> invalid CCW, IDAW or MIDAW address is clear. In the PoP this is
> described under Chapter 16. I/O Interruptions > Subchannel-Status Word >
> Subchannel-Status Field > Program Check in the paragraphs on "Invalid
> IDAW or MIDAW Address and "Invalid CCW Address".
> 
> My guess about handle_payload_3270_write() is that IMHO the initial 3270
> emulation code was, let's say of mixed quality in the first place, so
> wouldn't seek some hidden meaning/sense, behind the -EIO. IMHO first
> mapping architectural conditions to "errno-style" error codes, passing
> these around, and then mapping them back is a litle non-intuitive. If one
> looks at the purpose of handle_payload_3270_write(), it is basically
> emulating an IO data transfer from the device into the main storage. If
> that sort of operation. So with -EIO the original author probably wanted
> to say: hey there was an input/output error, which ain't that wrong. The
> problem is that somewhere up the call stack EIO gets interpreted in a
> very peculiar way, and along with other errnos mapped to CIO
> architecture stuff like SCSW bits.

It's not quite clear to me what the 3270 is supposed to do (I remember
some requirements for the aforementioned intervention required, but
posting unit checks does not look like the right thing to do for all of
the possible errors.)

Let's just loop the error through? I doubt that this is the last
problem in the 3270 code :) Fortunately, it is a rather obscure device
to be used with QEMU.



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

* Re: [PATCH v1 1/1] s390x: css: report errors from ccw_dstream_read/write
  2021-04-07 17:47   ` Halil Pasic
@ 2021-04-08  9:02     ` Cornelia Huck
  2021-04-08 12:32       ` Pierre Morel
  2021-04-08 12:39       ` Halil Pasic
  0 siblings, 2 replies; 13+ messages in thread
From: Cornelia Huck @ 2021-04-08  9:02 UTC (permalink / raw)
  To: Halil Pasic
  Cc: thuth, frankja, Pierre Morel, david, mst, richard.henderson,
	qemu-devel, borntraeger, qemu-s390x, pbonzini, marcandre.lureau,
	imbrenda

On Wed, 7 Apr 2021 19:47:11 +0200
Halil Pasic <pasic@linux.ibm.com> wrote:

> So this begs the question, do we need this fixed for old releases as well?
> 
> My answer is yes we do. Conny what do you think?

What do you mean with "old releases"? The dstream rework was in 2.11,
and I doubt that anyone is using anything older, or a downstream
release that is based on pre-2.11.

If you mean "include in stable", then yes, we can do that; if we want
the commit in 6.0, I need the final version soon.



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

* Re: [PATCH v1 1/1] s390x: css: report errors from ccw_dstream_read/write
  2021-04-08  9:02     ` Cornelia Huck
@ 2021-04-08 12:32       ` Pierre Morel
  2021-04-08 13:23         ` Cornelia Huck
  2021-04-08 12:39       ` Halil Pasic
  1 sibling, 1 reply; 13+ messages in thread
From: Pierre Morel @ 2021-04-08 12:32 UTC (permalink / raw)
  To: Cornelia Huck, Halil Pasic
  Cc: thuth, frankja, mst, david, richard.henderson, qemu-devel,
	borntraeger, qemu-s390x, marcandre.lureau, pbonzini, imbrenda



On 4/8/21 11:02 AM, Cornelia Huck wrote:
> On Wed, 7 Apr 2021 19:47:11 +0200
> Halil Pasic <pasic@linux.ibm.com> wrote:
> 
>> So this begs the question, do we need this fixed for old releases as well?
>>
>> My answer is yes we do. Conny what do you think?
> 
> What do you mean with "old releases"? The dstream rework was in 2.11,
> and I doubt that anyone is using anything older, or a downstream
> release that is based on pre-2.11.
> 
> If you mean "include in stable", then yes, we can do that; if we want
> the commit in 6.0, I need the final version soon.
> 
> 

OK, are you OK with the two change propositions I sent?

1) let the 3270 decide for internal errors (-EIO) but return the error 
for CSS errors in handle_payload_3270_write()

2) for senseid, always ask CSS to update the residual count
but only erase the senseid if the write succeeded


-- 
Pierre Morel
IBM Lab Boeblingen


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

* Re: [PATCH v1 1/1] s390x: css: report errors from ccw_dstream_read/write
  2021-04-08  9:02     ` Cornelia Huck
  2021-04-08 12:32       ` Pierre Morel
@ 2021-04-08 12:39       ` Halil Pasic
  2021-04-08 13:26         ` Cornelia Huck
  1 sibling, 1 reply; 13+ messages in thread
From: Halil Pasic @ 2021-04-08 12:39 UTC (permalink / raw)
  To: Cornelia Huck
  Cc: thuth, frankja, Pierre Morel, david, mst, richard.henderson,
	qemu-devel, borntraeger, qemu-s390x, pbonzini, marcandre.lureau,
	imbrenda

On Thu, 8 Apr 2021 11:02:32 +0200
Cornelia Huck <cohuck@redhat.com> wrote:

> On Wed, 7 Apr 2021 19:47:11 +0200
> Halil Pasic <pasic@linux.ibm.com> wrote:
> 
> > So this begs the question, do we need this fixed for old releases as well?
> > 
> > My answer is yes we do. Conny what do you think?  
> 
> What do you mean with "old releases"? The dstream rework was in 2.11,
> and I doubt that anyone is using anything older, or a downstream
> release that is based on pre-2.11.
> 
> If you mean "include in stable", then yes, we can do that; if we want
> the commit in 6.0, I need the final version soon.

With old releases, I wanted to say any QEMU that is still supported by
us ;). For upstream it is backport to the stable versions currently in
support.

The commit message does not tell us if this is an enhancement or a
bugfix, stable is not mentioned, and neither do we get the information
since when is this problem existent. I simply wanted to have that
discussion.

Would it make sense to split this up into a virtio-ccw a css and a 3270
patch? That way if there was a problem with let's say 3270, we could
still keep the other two?

Regards,
Halil


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

* Re: [PATCH v1 1/1] s390x: css: report errors from ccw_dstream_read/write
  2021-04-08 12:32       ` Pierre Morel
@ 2021-04-08 13:23         ` Cornelia Huck
  2021-04-08 16:18           ` Pierre Morel
  0 siblings, 1 reply; 13+ messages in thread
From: Cornelia Huck @ 2021-04-08 13:23 UTC (permalink / raw)
  To: Pierre Morel
  Cc: thuth, frankja, mst, david, richard.henderson, qemu-devel,
	Halil Pasic, borntraeger, qemu-s390x, marcandre.lureau, pbonzini,
	imbrenda

On Thu, 8 Apr 2021 14:32:11 +0200
Pierre Morel <pmorel@linux.ibm.com> wrote:

> On 4/8/21 11:02 AM, Cornelia Huck wrote:
> > On Wed, 7 Apr 2021 19:47:11 +0200
> > Halil Pasic <pasic@linux.ibm.com> wrote:
> >   
> >> So this begs the question, do we need this fixed for old releases as well?
> >>
> >> My answer is yes we do. Conny what do you think?  
> > 
> > What do you mean with "old releases"? The dstream rework was in 2.11,
> > and I doubt that anyone is using anything older, or a downstream
> > release that is based on pre-2.11.
> > 
> > If you mean "include in stable", then yes, we can do that; if we want
> > the commit in 6.0, I need the final version soon.
> > 
> >   
> 
> OK, are you OK with the two change propositions I sent?
> 
> 1) let the 3270 decide for internal errors (-EIO) but return the error 
> for CSS errors in handle_payload_3270_write()
> 
> 2) for senseid, always ask CSS to update the residual count
> but only erase the senseid if the write succeeded
> 
> 

I think both make sense.



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

* Re: [PATCH v1 1/1] s390x: css: report errors from ccw_dstream_read/write
  2021-04-08 12:39       ` Halil Pasic
@ 2021-04-08 13:26         ` Cornelia Huck
  0 siblings, 0 replies; 13+ messages in thread
From: Cornelia Huck @ 2021-04-08 13:26 UTC (permalink / raw)
  To: Halil Pasic
  Cc: thuth, frankja, Pierre Morel, david, mst, richard.henderson,
	qemu-devel, borntraeger, qemu-s390x, pbonzini, marcandre.lureau,
	imbrenda

On Thu, 8 Apr 2021 14:39:59 +0200
Halil Pasic <pasic@linux.ibm.com> wrote:

> On Thu, 8 Apr 2021 11:02:32 +0200
> Cornelia Huck <cohuck@redhat.com> wrote:
> 
> > On Wed, 7 Apr 2021 19:47:11 +0200
> > Halil Pasic <pasic@linux.ibm.com> wrote:
> >   
> > > So this begs the question, do we need this fixed for old releases as well?
> > > 
> > > My answer is yes we do. Conny what do you think?    
> > 
> > What do you mean with "old releases"? The dstream rework was in 2.11,
> > and I doubt that anyone is using anything older, or a downstream
> > release that is based on pre-2.11.
> > 
> > If you mean "include in stable", then yes, we can do that; if we want
> > the commit in 6.0, I need the final version soon.  
> 
> With old releases, I wanted to say any QEMU that is still supported by
> us ;). For upstream it is backport to the stable versions currently in
> support.
> 
> The commit message does not tell us if this is an enhancement or a
> bugfix, stable is not mentioned, and neither do we get the information
> since when is this problem existent. I simply wanted to have that
> discussion.
> 
> Would it make sense to split this up into a virtio-ccw a css and a 3270
> patch? That way if there was a problem with let's say 3270, we could
> still keep the other two?

I'm not sure that makes sense; it's not too complicated.



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

* Re: [PATCH v1 1/1] s390x: css: report errors from ccw_dstream_read/write
  2021-04-08 13:23         ` Cornelia Huck
@ 2021-04-08 16:18           ` Pierre Morel
  0 siblings, 0 replies; 13+ messages in thread
From: Pierre Morel @ 2021-04-08 16:18 UTC (permalink / raw)
  To: Cornelia Huck
  Cc: thuth, frankja, mst, david, richard.henderson, qemu-devel,
	Halil Pasic, borntraeger, qemu-s390x, marcandre.lureau, pbonzini,
	imbrenda



On 4/8/21 3:23 PM, Cornelia Huck wrote:
> On Thu, 8 Apr 2021 14:32:11 +0200
> Pierre Morel <pmorel@linux.ibm.com> wrote:
> 
>> On 4/8/21 11:02 AM, Cornelia Huck wrote:
>>> On Wed, 7 Apr 2021 19:47:11 +0200
>>> Halil Pasic <pasic@linux.ibm.com> wrote:
>>>    
>>>> So this begs the question, do we need this fixed for old releases as well?
>>>>
>>>> My answer is yes we do. Conny what do you think?
>>>
>>> What do you mean with "old releases"? The dstream rework was in 2.11,
>>> and I doubt that anyone is using anything older, or a downstream
>>> release that is based on pre-2.11.
>>>
>>> If you mean "include in stable", then yes, we can do that; if we want
>>> the commit in 6.0, I need the final version soon.
>>>
>>>    
>>
>> OK, are you OK with the two change propositions I sent?
>>
>> 1) let the 3270 decide for internal errors (-EIO) but return the error
>> for CSS errors in handle_payload_3270_write()
>>
>> 2) for senseid, always ask CSS to update the residual count
>> but only erase the senseid if the write succeeded
>>
>>
> 
> I think both make sense.
> 

OK I send a v2
Thanks

Pierre


-- 
Pierre Morel
IBM Lab Boeblingen


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

end of thread, other threads:[~2021-04-08 16:37 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-06  7:44 [PATCH v1 0/1] s390x: css: report errors from ccw_dstream_read/write Pierre Morel
2021-04-06  7:44 ` [PATCH v1 1/1] " Pierre Morel
2021-04-06 15:03   ` Cornelia Huck
2021-04-07 11:41     ` Pierre Morel
2021-04-07 16:54       ` Halil Pasic
2021-04-08  8:53         ` Cornelia Huck
2021-04-07 17:47   ` Halil Pasic
2021-04-08  9:02     ` Cornelia Huck
2021-04-08 12:32       ` Pierre Morel
2021-04-08 13:23         ` Cornelia Huck
2021-04-08 16:18           ` Pierre Morel
2021-04-08 12:39       ` Halil Pasic
2021-04-08 13:26         ` Cornelia Huck

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).