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

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

On 4/9/21 10:49 AM, Cornelia Huck wrote:
> On Fri, 9 Apr 2021 10:38:37 +0200
> Halil Pasic <pasic@linux.ibm.com> wrote:
> 
>> On Thu,  8 Apr 2021 18:32:09 +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.
>>>
>>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>>
>> Acked-by: Halil Pasic <pasic@linux.ibm.com>
>>
>> I did not look into the whole scsw.count stuff or into wether
>> your changes to 3270 (look form <mark></mark> in the diff part) affect
>> more than just ccw_dstream_*.
>>
>> I would have preferred this patch split up based on the intended effect
>> and thus also subsystem (css, virtio-ccw, 3270), but I've alluded to
>> that before, and since we are in a hurry I can live with it as is.
>>
>> Regards,
>> Halil
>>
>>> ---
>>>   hw/char/terminal3270.c | 11 +++++--
>>>   hw/s390x/3270-ccw.c    |  5 +++-
>>>   hw/s390x/css.c         | 14 +++++----
>>>   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..f3e7342b1e 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);
>>
>> <mark>
>>
>>> +    if (len < 0) {
>>> +        return len;
>>> +    }
>>>       ccw_dev->sch->curr_status.scsw.count = ccw->count - len;
>>>     
>>
>> </mark>
>>
>> Do we eventually update scsw.count?
> 
> I think we can consider the contents of scsw.count 'unpredictable', no?

I think so, the (len < 0) here will trigger a program check and the POP 
specifies the count as "not meaningful" in case of a program check.


> 
>>
>>>       return 0;
>>> @@ -50,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) {
>>
>> <mark>
>>> -        return -EIO;
>>> +        return len ? len : -EIO;
>>
>> </mark>

Here we do not change the previous behavior.
This problem, if it is one, is not related to not checking the dstream 
read/write functions.

>>
>>>       }
>>>   
>>>       ccw_dev->sch->curr_status.scsw.count = ccw->count - len;
>>> diff --git a/hw/s390x/css.c b/hw/s390x/css.c
>>> index fe47751df4..4149b8e5a7 100644
>>
> 

-- 
Pierre Morel
IBM Lab Boeblingen