| Message ID | 20170913115029.47626-5-pasic@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, 13 Sep 2017 13:50:29 +0200 Halil Pasic <pasic@linux.vnet.ibm.com> wrote: > Let's add indirect data addressing support for our virtual channel > subsystem. This implementation does no bother with any kind of s/no/not/ > prefetching. We simply step trough the IDAL on demand. s/trough/through/ > > Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com> > --- > 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 */ s/bz/bsz/ (missed the second one) I can fix the typos while applying.
* Halil Pasic <pasic@linux.vnet.ibm.com> [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 <pasic@linux.vnet.ibm.com> > --- > 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. 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; > +} > + > +static inline int ida_read_next_idaw(CcwDataStream *cds) > +{ > + union {uint64_t fmt2; uint32_t fmt1; } idaw; ^ Nit. > + 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) > + 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? > + 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? > + 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.
On Tue, 19 Sep 2017 13:50:05 +0800 Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com> wrote: > * Halil Pasic <pasic@linux.vnet.ibm.com> [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 <pasic@linux.vnet.ibm.com> > > --- > > 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. 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... > > > +} > > + > > +static inline int ida_read_next_idaw(CcwDataStream *cds) > > +{ > > + union {uint64_t fmt2; uint32_t fmt1; } idaw; > ^ > Nit. > > > + 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. > > > + 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... > > > + 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? > > > + 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. Halil, do you have some more comments?
On 09/19/2017 11:48 AM, Cornelia Huck wrote: > On Tue, 19 Sep 2017 13:50:05 +0800 > Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com> wrote: > >> * Halil Pasic <pasic@linux.vnet.ibm.com> [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 <pasic@linux.vnet.ibm.com> >>> --- >>> 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. > > 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. >> >>> +} >>> + >>> +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. >>> + 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. >> >>> + 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. >> >>> + 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. >> >>> + 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? Halil
On Tue, 19 Sep 2017 12:36:33 +0200 Halil Pasic <pasic@linux.vnet.ibm.com> wrote: > On 09/19/2017 11:48 AM, Cornelia Huck wrote: > > On Tue, 19 Sep 2017 13:50:05 +0800 > > Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com> wrote: > > > >> * Halil Pasic <pasic@linux.vnet.ibm.com> [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 <pasic@linux.vnet.ibm.com> > >>> --- > >>> 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. > > >> > >>> + 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. > > >> > >>> + 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. We can do more changes on top; it's not like I don't have more patches waiting...
On 09/19/2017 12:57 PM, Cornelia Huck wrote: >>>>> +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. > Not exactly. The more significant bits part of the check depend on the ccw format. This needs to be done for both idaw formats. I would need to introduce a new flag, or access the SubchDev to do this properly. Architecturally we also need to check the data addresses from which we read so we have nothing bigger than (1 << 31) - 1 if we are working with format-1 idaws. I also think we did not take proper care of proper maximum data address checks prior CwwDataStream which also depend on the ccw format (in absence of IDAW or MIDAW). The ccw format dependent maximum address checks are (1 << 24) - 1 and (1 << 31) - 1 respectively for format-0 and format-1 (on the first indirection level that is for non-IDA for the data, and for (M)IDA for the (M)IDAWs). Reference: PoP pages 16-25 and 16-26 "Invalid IDAW or MIDAW Addre" and "Invalid Data Address". How shall we proceed? Halil >>>> >>>>> + 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; >>>>> +}
On Tue, 19 Sep 2017 14:04:03 +0200 Halil Pasic <pasic@linux.vnet.ibm.com> wrote: > On 09/19/2017 12:57 PM, Cornelia Huck wrote: > >>>>> +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. > > > > Not exactly. The more significant bits part of the check > depend on the ccw format. This needs to be done for both > idaw formats. I would need to introduce a new flag, or > access the SubchDev to do this properly. > > Architecturally we also need to check the data addresses > from which we read so we have nothing bigger than > (1 << 31) - 1 if we are working with format-1 idaws. > > I also think we did not take proper care of proper > maximum data address checks prior CwwDataStream which also > depend on the ccw format (in absence of IDAW or MIDAW). > > The ccw format dependent maximum address checks are (1 << 24) - 1 > and (1 << 31) - 1 respectively for format-0 and format-1 (on > the first indirection level that is for non-IDA for the data, > and for (M)IDA for the (M)IDAWs). > > Reference: > PoP pages 16-25 and 16-26 "Invalid IDAW or MIDAW Addre" and > "Invalid Data Address". Sigh, when are things ever easy :( I'll need some time to review this. But more urgently, I need a break. > > How shall we proceed? Given the discussion about this patch in particular, I'll probably dequeue it and send it with the next batch of patches. We can also include the other improvements, then. Not sure whether I should just dequeue this one or the whole series (I really want to get the rest of the patches out...) More after the break :)
On 09/19/2017 02:23 PM, Cornelia Huck wrote: > On Tue, 19 Sep 2017 14:04:03 +0200 > Halil Pasic <pasic@linux.vnet.ibm.com> wrote: > >> On 09/19/2017 12:57 PM, Cornelia Huck wrote: >>>>>>> +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. >>> >> >> Not exactly. The more significant bits part of the check >> depend on the ccw format. This needs to be done for both >> idaw formats. I would need to introduce a new flag, or >> access the SubchDev to do this properly. >> >> Architecturally we also need to check the data addresses >> from which we read so we have nothing bigger than >> (1 << 31) - 1 if we are working with format-1 idaws. >> >> I also think we did not take proper care of proper >> maximum data address checks prior CwwDataStream which also >> depend on the ccw format (in absence of IDAW or MIDAW). >> >> The ccw format dependent maximum address checks are (1 << 24) - 1 >> and (1 << 31) - 1 respectively for format-0 and format-1 (on >> the first indirection level that is for non-IDA for the data, >> and for (M)IDA for the (M)IDAWs). >> >> Reference: >> PoP pages 16-25 and 16-26 "Invalid IDAW or MIDAW Addre" and >> "Invalid Data Address". > > Sigh, when are things ever easy :( > > I'll need some time to review this. But more urgently, I need a break. > >> >> How shall we proceed? > > Given the discussion about this patch in particular, I'll probably > dequeue it and send it with the next batch of patches. We can also > include the other improvements, then. Not sure whether I should just > dequeue this one or the whole series (I really want to get the rest of > the patches out...) > > More after the break :) > I think I can fix this pretty fast, and fixing this later is an option too in my opinion as the patch is consistent with our prior art (we ignored this maximum address checking requirement, and we keep ignoring it). I would prefer not dequeue-ing the whole series because a 3270 fix of mine (which just made the internal review) depends on this. IMHO, it's not like the series is fundamentally broken, not ain't an improvement at all. It is not perfect, but it's IMHO certainly an improvement over what we have. Regards, Halil
On 19/09/2017 12:57, Cornelia Huck wrote: > On Tue, 19 Sep 2017 12:36:33 +0200 > Halil Pasic <pasic@linux.vnet.ibm.com> wrote: > >> On 09/19/2017 11:48 AM, Cornelia Huck wrote: >>> On Tue, 19 Sep 2017 13:50:05 +0800 >>> Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com> wrote: >>> >>>> * Halil Pasic <pasic@linux.vnet.ibm.com> [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 <pasic@linux.vnet.ibm.com> >>>>> --- >>>>> 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... >
On Tue, 19 Sep 2017 14:32:33 +0200 Halil Pasic <pasic@linux.vnet.ibm.com> wrote: > On 09/19/2017 02:23 PM, Cornelia Huck wrote: > > On Tue, 19 Sep 2017 14:04:03 +0200 > > Halil Pasic <pasic@linux.vnet.ibm.com> wrote: > > > >> On 09/19/2017 12:57 PM, Cornelia Huck wrote: > >>>>>>> +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. > >>> > >> > >> Not exactly. The more significant bits part of the check > >> depend on the ccw format. This needs to be done for both > >> idaw formats. I would need to introduce a new flag, or > >> access the SubchDev to do this properly. > >> > >> Architecturally we also need to check the data addresses > >> from which we read so we have nothing bigger than > >> (1 << 31) - 1 if we are working with format-1 idaws. > >> > >> I also think we did not take proper care of proper > >> maximum data address checks prior CwwDataStream which also > >> depend on the ccw format (in absence of IDAW or MIDAW). > >> > >> The ccw format dependent maximum address checks are (1 << 24) - 1 > >> and (1 << 31) - 1 respectively for format-0 and format-1 (on > >> the first indirection level that is for non-IDA for the data, > >> and for (M)IDA for the (M)IDAWs). > >> > >> Reference: > >> PoP pages 16-25 and 16-26 "Invalid IDAW or MIDAW Addre" and > >> "Invalid Data Address". > > > > Sigh, when are things ever easy :( > > > > I'll need some time to review this. But more urgently, I need a break. > > > >> > >> How shall we proceed? > > > > Given the discussion about this patch in particular, I'll probably > > dequeue it and send it with the next batch of patches. We can also > > include the other improvements, then. Not sure whether I should just > > dequeue this one or the whole series (I really want to get the rest of > > the patches out...) > > > > More after the break :) > > > > I think I can fix this pretty fast, and fixing this later is > an option too in my opinion as the patch is consistent with our > prior art (we ignored this maximum address checking requirement, > and we keep ignoring it). > > I would prefer not dequeue-ing the whole series because a > 3270 fix of mine (which just made the internal review) depends > on this. IMHO, it's not like the series is fundamentally broken, > not ain't an improvement at all. It is not perfect, but it's > IMHO certainly an improvement over what we have. The issue is not that the series is problematic, but that I need to put a stop on changes at some point in time and just flush my queue - otherwise this is not workable. Therefore, I dequeued the series for now. I'll probably do another pull request in the next weeks anyway - so there's unlikely to be a long delay, and we can hash out everything without pressure. It would be best to just think this through; then you can send a v3 with the feedback addressed and your 3270 patch on top.
On 09/19/2017 03:46 PM, Pierre Morel wrote: > On 19/09/2017 12:57, Cornelia Huck wrote: >> On Tue, 19 Sep 2017 12:36:33 +0200 >> Halil Pasic <pasic@linux.vnet.ibm.com> wrote: >> >>> On 09/19/2017 11:48 AM, Cornelia Huck wrote: >>>> On Tue, 19 Sep 2017 13:50:05 +0800 >>>> Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com> wrote: >>>> >>>>> * Halil Pasic <pasic@linux.vnet.ibm.com> [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 <pasic@linux.vnet.ibm.com> >>>>>> --- >>>>>> 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. Reference? I've just explained something different above (depends on the ccw format and yes in case of format 1 I guess the check can be done like that). If you are arguing with that, please be more verbose. > >>> >>>>> >>>>>> + 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. > Reference? > 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. > I don't understand a thing. Aren't you confusing this with some internal discussion? Sorry, but I think, I will ignore the comment for now. >> >>> >>>>> >>>>>> + 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. OK, I'm not adding any r-b or ack for v3 unless told otherwise. Thanks for your review! Halil
On 09/19/2017 02:23 PM, Cornelia Huck wrote: >>>>> +{ >>>>> + 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. Yes, if I remove the space before the } checkpatch.pl complains. Going to keep it as is for v3. Halil
* Halil Pasic <pasic@linux.vnet.ibm.com> [2017-09-19 20:05:48 +0200]: > > > On 09/19/2017 02:23 PM, Cornelia Huck wrote: > >>>>> +{ > >>>>> + 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. > > Yes, if I remove the space before the } checkpatch.pl complains. > > Going to keep it as is for v3. My bad. :-/ > > Halil
* Halil Pasic <pasic@linux.vnet.ibm.com> [2017-09-19 14:04:03 +0200]: I have no problem with the rest parts of the discussion in this thread. > > > On 09/19/2017 12:57 PM, Cornelia Huck wrote: > >>>>> +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. > > My fault... This is the address of an IDAW, not the content (data address) in an IDAW. So what Halil pointed out is the right direction to go I think. I will review in the thread of the new version (v3). > > Not exactly. The more significant bits part of the check > depend on the ccw format. This needs to be done for both > idaw formats. I would need to introduce a new flag, or > access the SubchDev to do this properly. > > Architecturally we also need to check the data addresses > from which we read so we have nothing bigger than > (1 << 31) - 1 if we are working with format-1 idaws. Right. This is what I actually wanted to say. > > I also think we did not take proper care of proper > maximum data address checks prior CwwDataStream which also > depend on the ccw format (in absence of IDAW or MIDAW). > > The ccw format dependent maximum address checks are (1 << 24) - 1 > and (1 << 31) - 1 respectively for format-0 and format-1 (on > the first indirection level that is for non-IDA for the data, > and for (M)IDA for the (M)IDAWs). > > Reference: > PoP pages 16-25 and 16-26 "Invalid IDAW or MIDAW Addre" and > "Invalid Data Address". > > How shall we proceed? > > Halil > > >>>> > >>>>> + 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; > >>>>> +}
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); +} + +static inline int ida_read_next_idaw(CcwDataStream *cds) +{ + union {uint64_t fmt2; uint32_t fmt1; } idaw; + 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) { + 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)) { + 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); + 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; } }
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 <pasic@linux.vnet.ibm.com> --- hw/s390x/css.c | 109 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 108 insertions(+), 1 deletion(-)