qemu-devel
[Top][All Lists]
Advanced
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH v2 4/4] s390x/css: support ccw IDA

From: Pierre Morel
Subject: Re: [Qemu-devel] [PATCH v2 4/4] s390x/css: support ccw IDA
Date: Tue, 19 Sep 2017 15:46:20 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.2.1 
On 19/09/2017 12:57, Cornelia Huck wrote:

On Tue, 19 Sep 2017 12:36:33 +0200
Halil Pasic <address@hidden> wrote:


On 09/19/2017 11:48 AM, Cornelia Huck wrote:

On Tue, 19 Sep 2017 13:50:05 +0800
Dong Jia Shi <address@hidden> wrote:

* Halil Pasic <address@hidden> [2017-09-13 13:50:29 +0200]:

Let's add indirect data addressing support for our virtual channel
subsystem. This implementation does no bother with any kind of
prefetching. We simply step trough the IDAL on demand.

Signed-off-by: Halil Pasic <address@hidden>
---
  hw/s390x/css.c | 109 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
  1 file changed, 108 insertions(+), 1 deletion(-)

diff --git a/hw/s390x/css.c b/hw/s390x/css.c
index 6b0cd8861b..e34b2af4eb 100644
--- a/hw/s390x/css.c
+++ b/hw/s390x/css.c
@@ -819,6 +819,113 @@ incr:
      return 0;
  }

+/* returns values between 1 and bsz, where bs is a power of 2 */
+static inline uint16_t ida_continuous_left(hwaddr cda, uint64_t bsz)
+{
+    return bsz - (cda & (bsz - 1));
+}
+
+static inline uint64_t ccw_ida_block_size(uint8_t flags)
+{
+    return 1ULL << (((flags ^ CDS_F_C64) & (CDS_F_C64 | CDS_F_I2K)) ? 11 : 12);

If CDS_F_C64 is set, (flags ^ CDS_F_C64) will be 0, so (1ULL << 11) will
be the result regardless the I2K flag? The logic seems wrong.


No. If CDS_F_C64 is set then the outcome depends on the fact if
CDS_F_I2K is set or not.
(flags & CDS_F_IK) => ((flags ^ CDS_F_C64) & CDS_F_IK)
"(flags ^ CDS_F_C64) will be 0" is wrong. flags ^ CDS_F_C64
just flips the CDS_F_C64.

OTOH if the CDS_F_C64 was not set we have the corresponding
bit set in flags ^ CDS_F_C64 so then the  CDS_F_I2K bit does
not matter: we have 1ULL << 11.

In my reading the logic is good.


So I'll just leave it...





I've stared at that condition now for a bit, but all it managed was to
get me more confused... probably just need a break.


I2K is meaningful only when C64 is 1, otherwise it is ignored. The logic
here should be:
if ((flags & CDS_F_C64) && !(flags & CDS_F_I2K)) {
     return 1ULL << 12;
}
     return 1ULL << 11;


But I do think your version is more readable... >>>


I won't argue with this.


:)



...and we could change that in a patch on top to avoid future confusion.




+}
+
+static inline int ida_read_next_idaw(CcwDataStream *cds)
+{
+    union {uint64_t fmt2; uint32_t fmt1; } idaw;

                                            ^
Nit.


Maybe checkpatch wanted it this way. My memories are blurry.



I'd just leave it like that, tbh.




+    bool is_fmt2 = cds->flags & CDS_F_C64;
+    int ret;
+    hwaddr idaw_addr;
+
+    if (is_fmt2) {
+        idaw_addr = cds->cda_orig + sizeof(idaw.fmt2) * cds->at_idaw;
+        if (idaw_addr & 0x07) {
+            return -EINVAL; /* channel program check */
+        }
+        ret = address_space_rw(&address_space_memory, idaw_addr,
+                               MEMTXATTRS_UNSPECIFIED, (void *) &idaw.fmt2,
+                               sizeof(idaw.fmt2), false);
+        cds->cda = be64_to_cpu(idaw.fmt2);
+    } else {
+        idaw_addr = cds->cda_orig + sizeof(idaw.fmt1) * cds->at_idaw;
+        if (idaw_addr & 0x03) {

?:
(idaw_addr & 0x80000003)


Yes.


I will double check this. Does not seem unreasonable but
double-checking is better.


Please let me know. I think the architecture says that the bit must be
zero, and that we may (...) generate a channel program check.



Right (0x80000003) !=0 implies program check .
It is what is done , except for the bit 0 that was forgotten.




+            return -EINVAL; /* channel program check */
+
+        }
+        ret = address_space_rw(&address_space_memory, idaw_addr,
+                               MEMTXATTRS_UNSPECIFIED, (void *) &idaw.fmt1,
+                               sizeof(idaw.fmt1), false);
+        cds->cda = be64_to_cpu(idaw.fmt1);
+    }
+    ++(cds->at_idaw);
+    if (ret != MEMTX_OK) {
+        /* assume inaccessible address */
+        return -EINVAL; /* channel program check */
+
+    }
+    return 0;
+}
+
+static int ccw_dstream_rw_ida(CcwDataStream *cds, void *buff, int len,
+                              CcwDataStreamOp op)
+{
+    uint64_t bsz = ccw_ida_block_size(cds->flags);
+    int ret = 0;
+    uint16_t cont_left, iter_len;
+
+    ret = cds_check_len(cds, len);
+    if (ret <= 0) {
+        return ret;
+    }
+    if (!cds->at_idaw) {
+        /* read first idaw */
+        ret = ida_read_next_idaw(cds);
+        if (ret) {
+            goto err;
+        }
+        cont_left = ida_continuous_left(cds->cda, bsz);
+    } else {
+        cont_left = ida_continuous_left(cds->cda, bsz);
+        if (cont_left == bsz) {
+            ret = ida_read_next_idaw(cds);
+            if (ret) {
+                goto err;
+            }
+            if (cds->cda & (bsz - 1)) {

Could move this check into ida_read_next_idaw?


I'd like to avoid further code movement...


The first idaw is special. I don't think moving is possible.


So, the code is correct and I'll just leave it like that.


hum.
It seems to me that the handling of the first IDAW is indeed a little bit different. It is accessed directly from the CCW->CDA and generated dedicated status but I do not see the handling.
The channel status must be made pending with primary, secondary and alert status.
I do not find this code, may be it is somewhere before, I searched but did not find it. Also, I do not find in the documentation that we have a program check for this case. 






+                ret = -EINVAL; /* channel program check */
+                goto err;
+            }
+        }
+    }
+    do {
+        iter_len = MIN(len, cont_left);
+        if (op != CDS_OP_A) {
+            ret = address_space_rw(&address_space_memory, cds->cda,
+                                   MEMTXATTRS_UNSPECIFIED, buff, iter_len, op);

Ahh, now I recall that explictly defining CDS_OP_R to 0 and CDS_OP_W to
1 in 'struct CcwDataStreamOp' do have a meaning. Does it make sense to
make it more obvious by adding some comment there?


Would you have a good text for that?


I'm fine with clarifications.


Let's do it as a patch on top.




+            if (ret != MEMTX_OK) {
+                /* assume inaccessible address */
+                ret = -EINVAL; /* channel program check */
+                goto err;
+            }
+        }
+        cds->at_byte += iter_len;
+        cds->cda += iter_len;
+        len -= iter_len;
+        if (!len) {
+            break;
+        }
+        ret = ida_read_next_idaw(cds);
+        if (ret) {
+            goto err;
+        }
+        cont_left = bsz;
+    } while (true);
+    return ret;
+err:
+    cds->flags |= CDS_F_STREAM_BROKEN;
+    return ret;
+}
+
  void ccw_dstream_init(CcwDataStream *cds, CCW1 const *ccw, ORB const *orb)
  {
      /*
@@ -835,7 +942,7 @@ void ccw_dstream_init(CcwDataStream *cds, CCW1 const *ccw, 
ORB const *orb)
      if (!(cds->flags & CDS_F_IDA)) {
          cds->op_handler = ccw_dstream_rw_noflags;
      } else {
-        assert(false);
+        cds->op_handler = ccw_dstream_rw_ida;
      }
  }

--
2.13.5


Generally, the logic looks fine to me.


It did pass Halil's test; but that can only test fmt-2 + 4k blocks, as
this is what the kernel infrastructure provides.


Nod.



Halil, do you have some more comments?


Just a question. Do I have to respin?


I don't think so. If you could confirm the check for format-1, I'll
just fixup that one and get the queued patches out of the door.
generally LGTM but in my opinion the check for format-1 and the handling of the error status for the first IDA for format 1 and 2 must be cleared. 




We can do more changes on top; it's not like I don't have more patches
waiting...




--
Pierre Morel
Linux/KVM/QEMU in Böblingen - Germany
reply via email to
[Prev in Thread] Current Thread [Next in Thread]