On Tue, 5 Sep 2017 13:16:41 +0200
Halil Pasic <pa...@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 <pa...@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)
