qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Cornelia Huck
Subject: Re: [Qemu-devel] [PATCH v3 5/5] s390x/css: support ccw IDA
Date: Wed, 20 Sep 2017 13:18:44 +0200

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.


> >>> +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.)



reply via email to

[Prev in Thread] Current Thread [Next in Thread]