* [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 related [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 related [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-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-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 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 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 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
* 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: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
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).