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?
>>>>> +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. On the other hand waiting more that that
will IMHO do us no favor either (I think of our storage/memory hierarchy).
Regards,
Halil
