Re: [Qemu-devel] [PATCH v3 5/5] s390x/css: support ccw IDA

[Halil Pasic]

On 09/20/2017 01:18 PM, Cornelia Huck wrote:
> On Wed, 20 Sep 2017 13:13:01 +0200
> Halil Pasic <address@hidden> wrote:
> 
>> On 09/20/2017 10:33 AM, Cornelia Huck wrote:
>>> On Wed, 20 Sep 2017 15:42:38 +0800
>>> Dong Jia Shi <address@hidden> wrote:
>>>   
>>>> * Halil Pasic <address@hidden> [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