On Tue, 6 Apr 2021 09:44:13 +0200 Pierre Morel <pmo...@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 <pmo...@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.