qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Pierre Morel
Subject: Re: [PATCH v2 1/1] s390x: css: report errors from ccw_dstream_read/write
Date: Fri, 9 Apr 2021 11:55:56 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.9.0



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



reply via email to

[Prev in Thread] Current Thread [Next in Thread]