diff mbox series

[v3,5/5] s390x/css: support ccw IDA

Message ID 20170919182745.90280-6-pasic@linux.vnet.ibm.com
State New
Headers show
Series add CCW indirect data access support | expand

Commit Message

Halil Pasic Sept. 19, 2017, 6:27 p.m. UTC 
Let's add indirect data addressing support for our virtual channel
subsystem. This implementation does not bother with any kind of
prefetching. We simply step through the IDAL on demand.

Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
Signed-off-by: Cornelia Huck <cohuck@redhat.com>
---
 hw/s390x/css.c | 117 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 116 insertions(+), 1 deletion(-)

Comments

Dong Jia Shi Sept. 20, 2017, 7:42 a.m. UTC | #1 
* Halil Pasic <pasic@linux.vnet.ibm.com> [2017-09-19 20:27:45 +0200]:

> Let's add indirect data addressing support for our virtual channel
> subsystem. This implementation does not bother with any kind of
> prefetching. We simply step through the IDAL on demand.
> 
> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
> Signed-off-by: Cornelia Huck <cohuck@redhat.com>
> ---
>  hw/s390x/css.c | 117 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 116 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/s390x/css.c b/hw/s390x/css.c
> index 2d37a9ddde..a3ce6d89b6 100644
> --- a/hw/s390x/css.c
> +++ b/hw/s390x/css.c
> @@ -827,6 +827,121 @@ incr:
>      return 0;
>  }
> 
> +/* returns values between 1 and bsz, where bsz 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)
> +{
> +    if ((flags & CDS_F_C64) && !(flags & CDS_F_I2K)) {
> +        return 1ULL << 12;
> +    }
> +    return 1ULL << 11;
> +}
> +
> +static inline int ida_read_next_idaw(CcwDataStream *cds, bool ccw_fmt1,
> +                                     bool idaw_fmt_2)
> +{
> +    union {uint64_t fmt2; uint32_t fmt1; } idaw;
> +    int ret;
> +    hwaddr idaw_addr;
> +
> +    if (idaw_fmt_2) {
> +        idaw_addr = cds->cda_orig + sizeof(idaw.fmt2) * cds->at_idaw;
> +        if (idaw_addr & 0x07 && cds_ccw_addrs_ok(idaw_addr, 0, ccw_fmt1)) {
> +            return -EINVAL; /* channel program check */
> +        }
> +        ret = address_space_rw(&address_space_memory, idaw_addr,
Ahh, just got one question here:
Do we need to considerate endianess for 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 && 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.

> +    }
> +    ++(cds->at_idaw);
> +    if (ret != MEMTX_OK) {
> +        /* assume inaccessible address */
> +        return -EINVAL; /* channel program check */
> +
Extra line.

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

> +
> +    ret = cds_check_len(cds, len);
> +    if (ret <= 0) {
> +        return ret;
> +    }

[...]
Cornelia Huck Sept. 20, 2017, 8:11 a.m. UTC | #2 
On Tue, 19 Sep 2017 20:27:45 +0200
Halil Pasic <pasic@linux.vnet.ibm.com> wrote:

> Let's add indirect data addressing support for our virtual channel
> subsystem. This implementation does not bother with any kind of
> prefetching. We simply step through the IDAL on demand.
> 
> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
> Signed-off-by: Cornelia Huck <cohuck@redhat.com>

My s-o-b is a bit preliminary ;)

> ---
>  hw/s390x/css.c | 117 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 116 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/s390x/css.c b/hw/s390x/css.c
> index 2d37a9ddde..a3ce6d89b6 100644
> --- a/hw/s390x/css.c
> +++ b/hw/s390x/css.c
> @@ -827,6 +827,121 @@ incr:
>      return 0;
>  }
>  
> +/* returns values between 1 and bsz, where bsz 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)
> +{
> +    if ((flags & CDS_F_C64) && !(flags & CDS_F_I2K)) {
> +        return 1ULL << 12;
> +    }
> +    return 1ULL << 11;
> +}
> +
> +static inline int ida_read_next_idaw(CcwDataStream *cds, bool ccw_fmt1,
> +                                     bool idaw_fmt_2)
> +{
> +    union {uint64_t fmt2; uint32_t fmt1; } idaw;
> +    int ret;
> +    hwaddr idaw_addr;
> +
> +    if (idaw_fmt_2) {
> +        idaw_addr = cds->cda_orig + sizeof(idaw.fmt2) * cds->at_idaw;
> +        if (idaw_addr & 0x07 && cds_ccw_addrs_ok(idaw_addr, 0, ccw_fmt1)) {

So, what is supposed to happen if the idaw_addr is not aligned
correctly _and_ is violating the limits?

> +            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 && 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);
> +    }
> +    ++(cds->at_idaw);
> +    if (ret != MEMTX_OK) {
> +        /* assume inaccessible address */
> +        return -EINVAL; /* channel program check */
> +
> +    }
> +    return 0;
> +}
Cornelia Huck Sept. 20, 2017, 8:33 a.m. UTC | #3 
On Wed, 20 Sep 2017 15:42:38 +0800
Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com> wrote:

> * Halil Pasic <pasic@linux.vnet.ibm.com> [2017-09-19 20:27:45 +0200]:
> 
> > Let's add indirect data addressing support for our virtual channel
> > subsystem. This implementation does not bother with any kind of
> > prefetching. We simply step through the IDAL on demand.
> > 
> > Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
> > Signed-off-by: Cornelia Huck <cohuck@redhat.com>
> > ---
> >  hw/s390x/css.c | 117 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 116 insertions(+), 1 deletion(-)
> > 
> > diff --git a/hw/s390x/css.c b/hw/s390x/css.c
> > index 2d37a9ddde..a3ce6d89b6 100644
> > --- a/hw/s390x/css.c
> > +++ b/hw/s390x/css.c
> > @@ -827,6 +827,121 @@ incr:
> >      return 0;
> >  }
> > 
> > +/* returns values between 1 and bsz, where bsz 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)
> > +{
> > +    if ((flags & CDS_F_C64) && !(flags & CDS_F_I2K)) {
> > +        return 1ULL << 12;
> > +    }
> > +    return 1ULL << 11;
> > +}
> > +
> > +static inline int ida_read_next_idaw(CcwDataStream *cds, bool ccw_fmt1,
> > +                                     bool idaw_fmt_2)
> > +{
> > +    union {uint64_t fmt2; uint32_t fmt1; } idaw;
> > +    int ret;
> > +    hwaddr idaw_addr;
> > +
> > +    if (idaw_fmt_2) {
> > +        idaw_addr = cds->cda_orig + sizeof(idaw.fmt2) * cds->at_idaw;
> > +        if (idaw_addr & 0x07 && cds_ccw_addrs_ok(idaw_addr, 0, ccw_fmt1)) {
> > +            return -EINVAL; /* channel program check */
> > +        }
> > +        ret = address_space_rw(&address_space_memory, idaw_addr,  
> Ahh, just got one question here:
> Do we need to considerate endianess for idaw_addr?

That is taken care of below.

And the previous version worked on my laptop via tcg ;)

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

> 
> > +    }
> > +    ++(cds->at_idaw);
> > +    if (ret != MEMTX_OK) {
> > +        /* assume inaccessible address */
> > +        return -EINVAL; /* channel program check */
> > +  
> Extra line.
> 
> > +    }
> > +    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;
> > +    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.

> 
> > +
> > +    ret = cds_check_len(cds, len);
> > +    if (ret <= 0) {
> > +        return ret;
> > +    }  
> 
> [...]
>
Halil Pasic Sept. 20, 2017, 11:01 a.m. UTC | #4 
On 09/20/2017 10:11 AM, Cornelia Huck wrote:
> On Tue, 19 Sep 2017 20:27:45 +0200
> Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
> 
>> Let's add indirect data addressing support for our virtual channel
>> subsystem. This implementation does not bother with any kind of
>> prefetching. We simply step through the IDAL on demand.
>>
>> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
>> Signed-off-by: Cornelia Huck <cohuck@redhat.com>
> 
> My s-o-b is a bit preliminary ;)
> 
>> ---
>>  hw/s390x/css.c | 117 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>>  1 file changed, 116 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/s390x/css.c b/hw/s390x/css.c
>> index 2d37a9ddde..a3ce6d89b6 100644
>> --- a/hw/s390x/css.c
>> +++ b/hw/s390x/css.c
>> @@ -827,6 +827,121 @@ incr:
>>      return 0;
>>  }
>>  
>> +/* returns values between 1 and bsz, where bsz 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)
>> +{
>> +    if ((flags & CDS_F_C64) && !(flags & CDS_F_I2K)) {
>> +        return 1ULL << 12;
>> +    }
>> +    return 1ULL << 11;
>> +}
>> +
>> +static inline int ida_read_next_idaw(CcwDataStream *cds, bool ccw_fmt1,
>> +                                     bool idaw_fmt_2)
>> +{
>> +    union {uint64_t fmt2; uint32_t fmt1; } idaw;
>> +    int ret;
>> +    hwaddr idaw_addr;
>> +
>> +    if (idaw_fmt_2) {
>> +        idaw_addr = cds->cda_orig + sizeof(idaw.fmt2) * cds->at_idaw;
>> +        if (idaw_addr & 0x07 && cds_ccw_addrs_ok(idaw_addr, 0, ccw_fmt1)) {
> 
> So, what is supposed to happen if the idaw_addr is not aligned
> correctly _and_ is violating the limits?

You are right this is obviously wrong.

Should have been 
if (idaw_addr & 0x07 || !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.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)) {

Same here.

>> +            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;
>> +}
>
Halil Pasic Sept. 20, 2017, 11:13 a.m. UTC | #5 
On 09/20/2017 10:33 AM, Cornelia Huck wrote:
> On Wed, 20 Sep 2017 15:42:38 +0800
> Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com> wrote:
> 
>> * Halil Pasic <pasic@linux.vnet.ibm.com> [2017-09-19 20:27:45 +0200]:
>>
>>> Let's add indirect data addressing support for our virtual channel
>>> subsystem. This implementation does not bother with any kind of
>>> prefetching. We simply step through the IDAL on demand.
>>>
>>> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
>>> Signed-off-by: Cornelia Huck <cohuck@redhat.com>
>>> ---
>>>  hw/s390x/css.c | 117 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>>>  1 file changed, 116 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/hw/s390x/css.c b/hw/s390x/css.c
>>> index 2d37a9ddde..a3ce6d89b6 100644
>>> --- a/hw/s390x/css.c
>>> +++ b/hw/s390x/css.c
>>> @@ -827,6 +827,121 @@ incr:
>>>      return 0;
>>>  }
>>>
>>> +/* returns values between 1 and bsz, where bsz 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)
>>> +{
>>> +    if ((flags & CDS_F_C64) && !(flags & CDS_F_I2K)) {
>>> +        return 1ULL << 12;
>>> +    }
>>> +    return 1ULL << 11;
>>> +}
>>> +
>>> +static inline int ida_read_next_idaw(CcwDataStream *cds, bool ccw_fmt1,
>>> +                                     bool idaw_fmt_2)
>>> +{
>>> +    union {uint64_t fmt2; uint32_t fmt1; } idaw;
>>> +    int ret;
>>> +    hwaddr idaw_addr;
>>> +
>>> +    if (idaw_fmt_2) {
>>> +        idaw_addr = cds->cda_orig + sizeof(idaw.fmt2) * cds->at_idaw;
>>> +        if (idaw_addr & 0x07 && cds_ccw_addrs_ok(idaw_addr, 0, ccw_fmt1)) {
>>> +            return -EINVAL; /* channel program check */
>>> +        }
>>> +        ret = address_space_rw(&address_space_memory, idaw_addr,  
>> Ahh, just got one question here:
>> Do we need to considerate endianess for idaw_addr?
> 
> That is taken care of below.
> 
> And the previous version worked on my laptop via tcg ;)

Nod.

> 
>>
>>> +                               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.
 
>>
>>> +    }
>>> +    ++(cds->at_idaw);
>>> +    if (ret != MEMTX_OK) {
>>> +        /* assume inaccessible address */
>>> +        return -EINVAL; /* channel program check */
>>> +  
>> Extra line.
>>
>>> +    }
>>> +    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;
>>> +    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)?

>>
>>> +
>>> +    ret = cds_check_len(cds, len);
>>> +    if (ret <= 0) {
>>> +        return ret;
>>> +    }  
>>
>> [...]
>>
>
Cornelia Huck Sept. 20, 2017, 11:18 a.m. UTC | #6 
On Wed, 20 Sep 2017 13:13:01 +0200
Halil Pasic <pasic@linux.vnet.ibm.com> wrote:

> On 09/20/2017 10:33 AM, Cornelia Huck wrote:
> > On Wed, 20 Sep 2017 15:42:38 +0800
> > Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com> wrote:
> >   
> >> * Halil Pasic <pasic@linux.vnet.ibm.com> [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.)
Halil Pasic Sept. 20, 2017, 4:46 p.m. UTC | #7 
On 09/20/2017 01:18 PM, Cornelia Huck wrote:
> On Wed, 20 Sep 2017 13:13:01 +0200
> Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
> 
>> On 09/20/2017 10:33 AM, Cornelia Huck wrote:
>>> On Wed, 20 Sep 2017 15:42:38 +0800
>>> Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com> wrote:
>>>   
>>>> * Halil Pasic <pasic@linux.vnet.ibm.com> [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
Dong Jia Shi Sept. 21, 2017, 12:50 a.m. UTC | #8 
* Halil Pasic <pasic@linux.vnet.ibm.com> [2017-09-20 18:46:57 +0200]:

> 
> 
> On 09/20/2017 01:18 PM, Cornelia Huck wrote:
> > On Wed, 20 Sep 2017 13:13:01 +0200
> > Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
> > 
> >> On 09/20/2017 10:33 AM, Cornelia Huck wrote:
> >>> On Wed, 20 Sep 2017 15:42:38 +0800
> >>> Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com> wrote:
> >>>   
> >>>> * Halil Pasic <pasic@linux.vnet.ibm.com> [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?
Agree. That would also do the check in the first place. Sounds better.

> 
> >>>>> +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.
Thanks. I'm coming. :)

> On the other hand waiting more that that will IMHO do us no favor
> either (I think of our storage/memory hierarchy).
> 
> Regards,
> Halil
Dong Jia Shi Sept. 21, 2017, 1:10 a.m. UTC | #9 
* Halil Pasic <pasic@linux.vnet.ibm.com> [2017-09-20 13:13:01 +0200]:

> 
> 
> On 09/20/2017 10:33 AM, Cornelia Huck wrote:
> > On Wed, 20 Sep 2017 15:42:38 +0800
> > Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com> wrote:
> > 
> >> * Halil Pasic <pasic@linux.vnet.ibm.com> [2017-09-19 20:27:45 +0200]:
> >>
> >>> Let's add indirect data addressing support for our virtual channel
> >>> subsystem. This implementation does not bother with any kind of
> >>> prefetching. We simply step through the IDAL on demand.
> >>>
> >>> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
> >>> Signed-off-by: Cornelia Huck <cohuck@redhat.com>
> >>> ---
> >>>  hw/s390x/css.c | 117 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
> >>>  1 file changed, 116 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/hw/s390x/css.c b/hw/s390x/css.c
> >>> index 2d37a9ddde..a3ce6d89b6 100644
> >>> --- a/hw/s390x/css.c
> >>> +++ b/hw/s390x/css.c
> >>> @@ -827,6 +827,121 @@ incr:
> >>>      return 0;
> >>>  }
> >>>
> >>> +/* returns values between 1 and bsz, where bsz 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)
> >>> +{
> >>> +    if ((flags & CDS_F_C64) && !(flags & CDS_F_I2K)) {
> >>> +        return 1ULL << 12;
> >>> +    }
> >>> +    return 1ULL << 11;
> >>> +}
> >>> +
> >>> +static inline int ida_read_next_idaw(CcwDataStream *cds, bool ccw_fmt1,
> >>> +                                     bool idaw_fmt_2)
> >>> +{
> >>> +    union {uint64_t fmt2; uint32_t fmt1; } idaw;
> >>> +    int ret;
> >>> +    hwaddr idaw_addr;
> >>> +
> >>> +    if (idaw_fmt_2) {
> >>> +        idaw_addr = cds->cda_orig + sizeof(idaw.fmt2) * cds->at_idaw;
> >>> +        if (idaw_addr & 0x07 && cds_ccw_addrs_ok(idaw_addr, 0, ccw_fmt1)) {
> >>> +            return -EINVAL; /* channel program check */
> >>> +        }
> >>> +        ret = address_space_rw(&address_space_memory, idaw_addr,  
> >> Ahh, just got one question here:
> >> Do we need to considerate endianess for idaw_addr?
> > 
> > That is taken care of below.
> > 
> > And the previous version worked on my laptop via tcg ;)
> 
> Nod.

My fault!

I was thinking of the idaw_addr itself, not the content of it. Now I
realized that, since we already converted (cds->cda_orig) in
copy_ccw_from_guest(), there is no need to convert (idaw_addr +
idaw_size * idaw_index) anymore.

Please ingnore my noise. ;P

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

[...]
Cornelia Huck Sept. 21, 2017, 7:31 a.m. UTC | #10 
On Thu, 21 Sep 2017 08:50:36 +0800
Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com> wrote:

> * Halil Pasic <pasic@linux.vnet.ibm.com> [2017-09-20 18:46:57 +0200]:

> > 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?  
> Agree. That would also do the check in the first place. Sounds better.

Can't argue with nicer code, either :) Looking forward to the next
version.
diff mbox series

Patch

diff --git a/hw/s390x/css.c b/hw/s390x/css.c
index 2d37a9ddde..a3ce6d89b6 100644
--- a/hw/s390x/css.c
+++ b/hw/s390x/css.c
@@ -827,6 +827,121 @@  incr:
     return 0;
 }
 
+/* returns values between 1 and bsz, where bsz 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)
+{
+    if ((flags & CDS_F_C64) && !(flags & CDS_F_I2K)) {
+        return 1ULL << 12;
+    }
+    return 1ULL << 11;
+}
+
+static inline int ida_read_next_idaw(CcwDataStream *cds, bool ccw_fmt1,
+                                     bool idaw_fmt_2)
+{
+    union {uint64_t fmt2; uint32_t fmt1; } idaw;
+    int ret;
+    hwaddr idaw_addr;
+
+    if (idaw_fmt_2) {
+        idaw_addr = cds->cda_orig + sizeof(idaw.fmt2) * cds->at_idaw;
+        if (idaw_addr & 0x07 && 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.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);
+    }
+    ++(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;
+    const bool idaw_fmt2 = cds->flags & CDS_F_C64;
+    bool ccw_fmt1 = cds->flags & CDS_F_FMT;
+
+    ret = cds_check_len(cds, len);
+    if (ret <= 0) {
+        return ret;
+    }
+    if (!cds->at_idaw) {
+        /* read first idaw */
+        ret = ida_read_next_idaw(cds, ccw_fmt1, idaw_fmt2);
+        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, ccw_fmt1, idaw_fmt2);
+            if (ret) {
+                goto err;
+            }
+            if (cds->cda & (bsz - 1)) {
+                ret = -EINVAL; /* channel program check */
+                goto err;
+            }
+        }
+    }
+    do {
+        iter_len = MIN(len, cont_left);
+        if (!idaw_fmt2 && (cds->cda + iter_len) >= (1ULL << 31)) {
+                ret = -EINVAL; /* channel program check */
+                goto err;
+        }
+        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, ccw_fmt1, idaw_fmt2);
+        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)
 {
     /*
@@ -845,7 +960,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;
     }
 }