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

[Cornelia Huck]

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.

In any case, s/retuns/returns/

> 
> >   
> >> +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?