qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
From: Pierre Morel <pmorel@linux.ibm.com>
To: Cornelia Huck <cohuck@redhat.com>
Cc: thuth@redhat.com, frankja@linux.ibm.com, david@redhat.com,
	mst@redhat.com, richard.henderson@linaro.org,
	qemu-devel@nongnu.org, pasic@linux.ibm.com,
	borntraeger@de.ibm.com, qemu-s390x@nongnu.org,
	pbonzini@redhat.com, marcandre.lureau@redhat.com,
	imbrenda@linux.ibm.com
Subject: Re: [PATCH v1 1/1] s390x: css: report errors from ccw_dstream_read/write
Date: Wed, 7 Apr 2021 13:41:57 +0200	[thread overview]
Message-ID: <c541aacd-34cb-3ed5-0016-cca1064c67e6@linux.ibm.com> (raw)
In-Reply-To: <20210406170306.1be374c2.cohuck@redhat.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


  reply	other threads:[~2021-04-07 11:44 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-04-06  7:44 [PATCH v1 0/1] " 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 [this message]
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

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=c541aacd-34cb-3ed5-0016-cca1064c67e6@linux.ibm.com \
    --to=pmorel@linux.ibm.com \
    --cc=borntraeger@de.ibm.com \
    --cc=cohuck@redhat.com \
    --cc=david@redhat.com \
    --cc=frankja@linux.ibm.com \
    --cc=imbrenda@linux.ibm.com \
    --cc=marcandre.lureau@redhat.com \
    --cc=mst@redhat.com \
    --cc=pasic@linux.ibm.com \
    --cc=pbonzini@redhat.com \
    --cc=qemu-devel@nongnu.org \
    --cc=qemu-s390x@nongnu.org \
    --cc=richard.henderson@linaro.org \
    --cc=thuth@redhat.com \
    --subject='Re: [PATCH v1 1/1] s390x: css: report errors from ccw_dstream_read/write' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

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).