qemu-s390x
[Top][All Lists]
Advanced

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

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


From: Halil Pasic
Subject: Re: [PATCH v1 1/1] s390x: css: report errors from ccw_dstream_read/write
Date: Wed, 7 Apr 2021 19:47:11 +0200

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.


The return values of ccw_dstream_write/read were intentionally ignored
in commit f57ba05823 ("virtio-ccw: use ccw data stream") to avoid
changes in behavior that wound not contribute to the objective of the
patch-set, like silently doing more error checking and handling.

If we consider the first hunk:


--- a/hw/s390x/virtio-ccw.c
+++ b/hw/s390x/virtio-ccw.c
@@ -289,49 +289,19 @@ static int virtio_ccw_handle_set_vq(SubchDev *sch, CCW1 
ccw, bool check_len,
         return -EFAULT;
     }
     if (is_legacy) {
-        linfo.queue = address_space_ldq_be(&address_space_memory, ccw.cda,
-                                           MEMTXATTRS_UNSPECIFIED, NULL);
-        linfo.align = address_space_ldl_be(&address_space_memory,
-                                           ccw.cda + sizeof(linfo.queue),
-                                           MEMTXATTRS_UNSPECIFIED,
-                                           NULL);
-        linfo.index = address_space_lduw_be(&address_space_memory,
-                                            ccw.cda + sizeof(linfo.queue)
-                                            + sizeof(linfo.align),
-                                            MEMTXATTRS_UNSPECIFIED,
-                                            NULL);
-        linfo.num = address_space_lduw_be(&address_space_memory,
-                                          ccw.cda + sizeof(linfo.queue)
-                                          + sizeof(linfo.align)
-                                          + sizeof(linfo.index),
-                                          MEMTXATTRS_UNSPECIFIED,
-                                          NULL);
+        ccw_dstream_read(&sch->cds, linfo);
+        be64_to_cpus(&linfo.queue);
+        be32_to_cpus(&linfo.align);
+        be16_to_cpus(&linfo.index);
+        be16_to_cpus(&linfo.num);

we can see, that the original code did not contain any error checking regarding
the invalidity of the guest physical address.

What was the behavior there? The last argument of address_space_* where we pass
the NULL is actually an optional pointer to the MemTxResult, which would tell
us if the operation succeeded or failed. Passing NULL there means, we don't 
care.

My guess is that when those loads fail (or the read fails) we will just carry on
with the garbage we found on the stack.

So this begs the question, do we need this fixed for old releases as well?

My answer is yes we do. Conny what do you think?

Regards,
Halil




reply via email to

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