Re: [Qemu-devel] [PATCH v2 4/4] s390x/css: support ccw IDA

[Halil Pasic]

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 <address@hidden> wrote:
>>
>>> On 09/19/2017 11:48 AM, Cornelia Huck wrote:
>>>> On Tue, 19 Sep 2017 13:50:05 +0800
>>>> Dong Jia Shi <address@hidden> wrote:
>>>>   
>>>>> * Halil Pasic <address@hidden> [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 <address@hidden>
>>>>>> ---
>>>>>>   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