Re: [PATCH v1 1/1] s390x: css: report errors from ccw_dstream_read/write

[Pierre Morel <pmorel@linux.ibm.com>]

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