Re: [Qemu-devel] [PATCH 1/5] s390x/css: introduce css data stream

[Halil Pasic <pasic@linux.vnet.ibm.com>]

On 09/06/2017 02:18 PM, Cornelia Huck wrote:
> On Tue,  5 Sep 2017 13:16:41 +0200
> Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
> 
>> This is a preparation for introducing handling for indirect data
>> addressing and modified indirect data addressing (CCW). Here we introduce
>> an interface which should make the addressing scheme transparent for the
>> client code. Here we implement only basic scheme (no IDA or MIDA).
> 
> s/basic scheme/the basic scheme/
> 

Nod.

>>
>> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
>> ---
>>  hw/s390x/css.c         | 55 +++++++++++++++++++++++++++++++++++++++++
>>  include/hw/s390x/css.h | 67 ++++++++++++++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 122 insertions(+)
>>
>> diff --git a/hw/s390x/css.c b/hw/s390x/css.c
>> index 1880b1a0ff..87d913f81c 100644
>> --- a/hw/s390x/css.c
>> +++ b/hw/s390x/css.c
>> @@ -783,6 +783,61 @@ static CCW1 copy_ccw_from_guest(hwaddr addr, bool fmt1)
>>      }
>>      return ret;
>>  }
>> +/**
>> + * If out of bounds marks the stream broken. If broken returns -EINVAL,
>> + * otherwise the requested  length (may be zero)
> 
> s/requested  length/requested length/
> 

Nod.

>> + */
>> +static inline int cds_check_len(CcwDataStream *cds, int len)
>> +{
>> +    if (cds->at_byte + len > cds->count) {
>> +        cds->flags |= CDS_F_STREAM_BROKEN;
>> +    }
>> +    return cds->flags & CDS_F_STREAM_BROKEN ? -EINVAL : len;
>> +}
>> +
>> +static int ccw_dstream_rw_noflags(CcwDataStream *cds, void *buff, int len,
>> +                                  CcwDataStreamOp op)
>> +{
>> +    int ret;
>> +
>> +    ret = cds_check_len(cds, len);
>> +    if (ret <= 0) {
>> +        return ret;
>> +    }
>> +    if (op == CDS_OP_A) {
>> +        goto incr;
>> +    }
>> +    ret = address_space_rw(&address_space_memory, cds->cda,
>> +                           MEMTXATTRS_UNSPECIFIED, buff, len, op);
>> +    if (ret != MEMTX_OK) {
>> +        cds->flags |= CDS_F_STREAM_BROKEN;
> 
> Do we want to distinguish between different reasons for the stream
> being broken? I.e, is there a case where we want to signal different
> errors back to the caller?
> 

Hm, after I've done the error handling it seems that basically
everything is to be handled with a program check. The stream
records the place where the problem was encountered, so for debug
we would not have to search for long.

There seems to be no need to distinguish.

>> +        return -EINVAL;
>> +    }
>> +incr:
>> +    cds->at_byte += len;
>> +    cds->cda += len;
>> +    return 0;
>> +}
>> +
>> +void ccw_dstream_init(CcwDataStream *cds, CCW1 const *ccw, ORB const *orb)
>> +{
>> +    /*
>> +     * We don't support MIDA (an optional facility) yet and we
>> +     * catch this earlier. Just for expressing the precondition.
>> +     */
>> +    assert(!(orb->ctrl1 & ORB_CTRL1_MASK_MIDAW));
> 
> I don't know, this is infrastructure, should it trust its callers? If
> you keep the assert, please make it g_assert().


Why g_assert? I think g_assert comes form a test framework, this is not
test code.

I feel more comfortable having this assert around.

> 
>> +    cds->flags = (orb->ctrl0 & ORB_CTRL0_MASK_I2K ? CDS_F_I2K : 0) |
>> +                 (orb->ctrl0 & ORB_CTRL0_MASK_C64 ? CDS_F_C64 : 0) |
>> +                 (ccw->flags & CCW_FLAG_IDA ? CDS_F_IDA : 0);
>> +    cds->count = ccw->count;
>> +    cds->cda_orig = ccw->cda;
>> +    ccw_dstream_rewind(cds);
>> +    if (!(cds->flags & CDS_F_IDA)) {
>> +        cds->op_handler = ccw_dstream_rw_noflags;
>> +    } else {
>> +        assert(false);
> 
> Same here.
> 
> Or should we make this return an error and have the callers deal with
> that? (I still need to look at the users.)
> 

This assert is going away soon (patch 4). I'm not sure doing much more here
is justified.

>> +    }
>> +}
>>  
>>  static int css_interpret_ccw(SubchDev *sch, hwaddr ccw_addr,
>>                               bool suspend_allowed)
>