* Halil Pasic <pa...@linux.vnet.ibm.com> [2017-09-20 18:46:57 +0200]:
>
>
> On 09/20/2017 01:18 PM, Cornelia Huck wrote:
> > On Wed, 20 Sep 2017 13:13:01 +0200
> > Halil Pasic <pa...@linux.vnet.ibm.com> wrote:
> >
> >> On 09/20/2017 10:33 AM, Cornelia Huck wrote:
> >>> On Wed, 20 Sep 2017 15:42:38 +0800
> >>> Dong Jia Shi <bjsdj...@linux.vnet.ibm.com> wrote:
> >>>
> >>>> * Halil Pasic <pa...@linux.vnet.ibm.com> [2017-09-19 20:27:45 +0200]:
> >
> >>>>> + 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 && cds_ccw_addrs_ok(idaw_addr, 0,
> >>>>> ccw_fmt1)) {
> >>>>> + 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);
> >>>> Still need to check bit 0x80000000 here I think.
> >>>
> >>> Yes, I think this is 'must be zero' for format-1 idaws, and not covered
> >>> by the ccw-format specific checks above. (Although the PoP can be a bit
> >>> confusing with many similar terms...)
> >>>
> >>
> >> It's taken care of in ccw_dstream_rw_ida before the actual
> >> access happens. Code looks like this:
> >> + if (!idaw_fmt2 && (cds->cda + iter_len) >= (1ULL << 31)) {
> >> + ret = -EINVAL; /* channel program check */
> >> + goto err;
> >> + }
> >>
> >> The idea was to have it similar to the non-indirect case.
> >
> > <looks at patch again>
> >
> > Ah, I was simply looking for the wrong pattern. Looks correct.
> >
> >
>
> Thinking about this some more. Since in case of IDA we are guaranteed
> to never cross a block boundary with a single IDAW we won't ever cross
> block boundary. So we can do the check in ida_read_next_idaw by checking
> bit 0x80000000 on the ccw->cda. So we could keep idaw_fmt2 and ccw_fmt1
> local to ida_read_next_idaw and save one goto err. I think that would
> look a bit nicer than what I have here in v3. Agree?
Agree. That would also do the check in the first place. Sounds better.
>
> >>>>> +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;
> >>>>> + const bool idaw_fmt2 = cds->flags & CDS_F_C64;
> >>>>> + bool ccw_fmt1 = cds->flags & CDS_F_FMT;
> >>>> Use 'const bool' either? Although I doubt the value of using const here.
> >>>> ;)
> >>>
> >>> Both being the same is still a good idea.
> >>>
> >>
> >> Yeah. For which one should I go (with const or without)?
> >
> > For the one you prefer :) (I'm not sure if the const adds value here.)
> >
>
> I think we generally don't care about const-ness in such situations,
> so I think I won't use consts.
>
> I intend to fix the issues we have found and do a v4 tomorrow, unless
> somebody screams -- could do it today but I would like to give Dong
> Jia an opportunity to react.
Thanks. I'm coming. :)
> On the other hand waiting more that that will IMHO do us no favor
> either (I think of our storage/memory hierarchy).
>
> Regards,
> Halil
--
Dong Jia Shi
