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

[Halil Pasic]

On 09/13/2017 11:58 AM, Cornelia Huck wrote:
> On Mon, 11 Sep 2017 20:08:16 +0200
> Halil Pasic <address@hidden> wrote:
> 
>> On 09/06/2017 03:10 PM, Cornelia Huck wrote:
>>> On Tue,  5 Sep 2017 13:16:44 +0200
>>> Halil Pasic <address@hidden> wrote:
>>>   
>>>> Let's add indirect data addressing support for our virtual channel
>>>> subsystem. This implementation does no prefetching, but steps
>>>> trough the idal as we go.
>>>>
>>>> Signed-off-by: Halil Pasic <address@hidden>
>>>> ---
>>>>  hw/s390x/css.c | 110 
>>>> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>>>>  1 file changed, 109 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/hw/s390x/css.c b/hw/s390x/css.c
>>>> index c1bc9944e6..9d8f9fd7ea 100644
>>>> --- a/hw/s390x/css.c
>>>> +++ b/hw/s390x/css.c
>>>> @@ -819,6 +819,114 @@ incr:
>>>>      return 0;
>>>>  }
>>>>  
>>>> +/* retuns values between 1 and bs, where bs is a power of 2 */  
>>>
>>> 'bs' is 'block size'?  
>>
>> Yes.
> 
> The first thing that popped up in my head was something else, that's
> why I asked :) Maybe bsz would be better.
> 

Will change to bsz.

> In any case, s/retuns/returns/
> 

I think I've already fixed that :).

>>
>>>   
>>>> +static inline uint16_t ida_continuous_left(hwaddr cda, uint64_t bs)
>>>> +{
>>>> +    return bs - (cda & (bs - 1));
>>>> +}
> 
>>>> +static int ccw_dstream_rw_ida(CcwDataStream *cds, void *buff, int len,
>>>> +                              CcwDataStreamOp op)
>>>> +{
>>>> +    uint64_t bs = 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, bs);
>>>> +    } else {
>>>> +        cont_left = ida_continuous_left(cds->cda, bs);
>>>> +        if (cont_left == bs) {
>>>> +            ret = ida_read_next_idaw(cds);
>>>> +            if (ret) {
>>>> +                goto err;
>>>> +            }
>>>> +            if (cds->cda & (bs - 1)) {
>>>> +                ret = -EINVAL; /* chanel program check */
>>>> +                goto err;
>>>> +            }
>>>> +        }
>>>> +    }
>>>> +do_iter_len:
>>>> +    iter_len = MIN(len, cont_left);
>>>> +    if (op == CDS_OP_A) {
>>>> +        goto incr;
>>>> +    }
>>>> +    ret = address_space_rw(&address_space_memory, cds->cda,
>>>> +                           MEMTXATTRS_UNSPECIFIED, buff, iter_len, op);
>>>> +    if (ret != MEMTX_OK) {
>>>> +        /* assume inaccessible address */
>>>> +        ret = -EINVAL; /* chanel program check */
>>>> +        goto err;
>>>> +    }
>>>> +incr:
>>>> +    cds->at_byte += iter_len;
>>>> +    cds->cda += iter_len;
>>>> +    len -= iter_len;
>>>> +    if (len) {
>>>> +        ret = ida_read_next_idaw(cds);
>>>> +        if (ret) {
>>>> +            goto err;
>>>> +        }
>>>> +        cont_left = bs;
>>>> +        goto do_iter_len;
>>>> +    }
>>>> +    return ret;
>>>> +err:
>>>> +    cds->flags |= CDS_F_STREAM_BROKEN;
>>>> +    return ret;
>>>> +}  
>>>
>>> I'm not so happy about the many gotos (excepting the err ones). Any
>>> chance to get around these?
>>>   
>> Certainly possible:
>>
>> static int ccw_dstream_rw_ida(CcwDataStream *cds, void *buff, int len,       
>>    
>>                               CcwDataStreamOp op)                            
>>    
>> {                                                                            
>>    
>>     uint64_t bs = 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, bs);                       
>>    
>>     } else {                                                                 
>>    
>>         cont_left = ida_continuous_left(cds->cda, bs);                       
>>    
>>         if (cont_left == bs) {                                               
>>    
>>             ret = ida_read_next_idaw(cds);                                   
>>    
>>             if (ret) {                                                       
>>    
>>                 goto err;                                                    
>>    
>>             }                                                                
>>    
>>             if (cds->cda & (bs - 1)) {                                       
>>    
>>                 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); 
>>             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 = bs;                                                      
>>    
>>     } while (true);                                                          
>>    
>>     return ret;                                                              
>>    
>> err:                                                                         
>>    
>>     cds->flags |= CDS_F_STREAM_BROKEN;                                       
>>    
>>     return ret;                                                              
>>    
>> }
>>
>> My idea was decrease indentation level and at the same time make
>> labels tell us something about the purpose of code pieces. Which
>> one do you prefer?
> 
> I do not really like either much :( Any chance to make this better via
> some kind of helper functions?
> 

I already use helper functions (ida_read_next_idaw, ida_continuous_left,
ccw_ida_block_size, cds_check_len). The function is flow control and
error handling heavy. I have no further ideas for helper functions,
or other simplifications.

IMHO if there are no non-aesthetics issues we can defer the non-trivial
aesthetics issues until somebody/anybody has an idea and the time to
sort them out.

I think, in absence of further complaints or concrete ideas I will go
with the more structured variant (second one) for v2. I would like
to post a v2 soon to avoid juggling to much stuff in head.

Regards,
Halil