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

[Halil Pasic]

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]:
>>
>>> 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 <address@hidden>
>>> Signed-off-by: Cornelia Huck <address@hidden>
>>> ---
>>>  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;
>>> +    }  
>>
>> [...]
>>
>