On Tue, 19 Sep 2017 13:50:05 +0800
Dong Jia Shi <bjsdj...@linux.vnet.ibm.com> wrote:
> * Halil Pasic <pa...@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 <pa...@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?
