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.