|
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.5Generally, 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
[Prev in Thread] | Current Thread | [Next in Thread] |