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

[Cornelia Huck <cohuck@redhat.com>]

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/

> 
> 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/

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

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

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

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