Re: [PATCH v1] s390x: kvm-unit-tests: a PONG device for Sub Channels tests

[Cornelia Huck <cohuck@redhat.com>]

On Wed, 13 Nov 2019 20:02:33 +0100
Pierre Morel <pmorel@linux.ibm.com> wrote:

Minor nit for $SUBJECT: this isn't a kvm-unit-tests patch, that's just
one consumer :)

> The PONG device accept two commands: PONG_READ and PONG_WRITE
> which allow to read from and write to an internal buffer of
> 1024 bytes.
> 
> The QEMU device is named ccw-pong.
> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>  hw/s390x/Makefile.objs  |   1 +
>  hw/s390x/ccw-pong.c     | 186 ++++++++++++++++++++++++++++++++++++++++++++++++
>  include/hw/s390x/pong.h |  47 ++++++++++++
>  3 files changed, 234 insertions(+)
>  create mode 100644 hw/s390x/ccw-pong.c
>  create mode 100644 include/hw/s390x/pong.h
> 
> diff --git a/hw/s390x/Makefile.objs b/hw/s390x/Makefile.objs
> index ee91152..3a83438 100644
> --- a/hw/s390x/Makefile.objs
> +++ b/hw/s390x/Makefile.objs
> @@ -32,6 +32,7 @@ obj-$(CONFIG_KVM) += tod-kvm.o
>  obj-$(CONFIG_KVM) += s390-skeys-kvm.o
>  obj-$(CONFIG_KVM) += s390-stattrib-kvm.o s390-mchk.o
>  obj-y += s390-ccw.o
> +obj-y += ccw-pong.o

Not sure if unconditionally introducing a test device is a good idea.

>  obj-y += ap-device.o
>  obj-y += ap-bridge.o
>  obj-y += s390-sei.o
> diff --git a/hw/s390x/ccw-pong.c b/hw/s390x/ccw-pong.c
> new file mode 100644
> index 0000000..e7439d5
> --- /dev/null
> +++ b/hw/s390x/ccw-pong.c
> @@ -0,0 +1,186 @@
> +/*
> + * CCW PING-PONG
> + *
> + * Copyright 2019 IBM Corp.
> + * Author(s): Pierre Morel <pmorel@linux.ibm.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or (at
> + * your option) any later version. See the COPYING file in the top-level
> + * directory.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qapi/error.h"
> +#include "qemu/module.h"
> +#include "cpu.h"
> +#include "exec/address-spaces.h"
> +#include "hw/s390x/css.h"
> +#include "hw/s390x/css-bridge.h"
> +#include "hw/qdev-properties.h"
> +#include "hw/s390x/pong.h"
> +
> +#define PONG_BUF_SIZE 0x1000
> +static char buf[PONG_BUF_SIZE] = "Hello world\n";
> +
> +static inline int pong_rw(CCW1 *ccw, char *p, int len, bool dir)
> +{
> +    int ret;
> +
> +    ret = address_space_rw(&address_space_memory, ccw->cda,
> +                           MEMTXATTRS_UNSPECIFIED,
> +                           (unsigned char *)buf, len, dir);
> +
> +    return (ret == MEMTX_OK) ? -EIO : 0;
> +}
> +
> +/* Handle READ ccw commands from guest */
> +static int handle_payload_read(CcwPONGDevice *dev, CCW1 *ccw)
> +{
> +    CcwDevice *ccw_dev = CCW_DEVICE(dev);
> +    int len;
> +
> +    if (!ccw->cda) {
> +        return -EFAULT;
> +    }
> +
> +    if (ccw->count > PONG_BUF_SIZE) {
> +        len = PONG_BUF_SIZE;
> +        ccw_dev->sch->curr_status.scsw.count = ccw->count - PONG_BUF_SIZE;
> +    } else {
> +        len = ccw->count;
> +        ccw_dev->sch->curr_status.scsw.count = 0;
> +    }
> +
> +    return pong_rw(ccw, buf, len, 1);
> +}
> +
> +/*
> + * Handle WRITE ccw commands to write data to client
> + * The SCSW count is set to the number of bytes not transfered.
> + */
> +static int handle_payload_write(CcwPONGDevice *dev, CCW1 *ccw)
> +{
> +    CcwDevice *ccw_dev = CCW_DEVICE(dev);
> +    int len;
> +
> +    if (!ccw->cda) {
> +        ccw_dev->sch->curr_status.scsw.count = ccw->count;
> +        return -EFAULT;
> +    }
> +
> +    if (ccw->count > PONG_BUF_SIZE) {
> +        len = PONG_BUF_SIZE;
> +        ccw_dev->sch->curr_status.scsw.count = ccw->count - PONG_BUF_SIZE;
> +    } else {
> +        len = ccw->count;
> +        ccw_dev->sch->curr_status.scsw.count = 0;
> +    }
> +
> +    return pong_rw(ccw, buf, len, 0);

Can you please use the dstream infrastructure for read/write handling?

You also seem to miss some basic checks e.g. for short reads/writes
with and without SLI set.

> +}
> +
> +static int pong_ccw_cb(SubchDev *sch, CCW1 ccw)
> +{
> +    int rc = 0;
> +    CcwPONGDevice *dev = sch->driver_data;
> +
> +    switch (ccw.cmd_code) {
> +    case PONG_WRITE:
> +        rc = handle_payload_write(dev, &ccw);
> +        break;
> +    case PONG_READ:
> +        rc = handle_payload_read(dev, &ccw);
> +        break;
> +    default:
> +        rc = -ENOSYS;
> +        break;
> +    }
> +
> +    if (rc == -EIO) {
> +        /* I/O error, specific devices generate specific conditions */
> +        SCHIB *schib = &sch->curr_status;
> +
> +        sch->curr_status.scsw.dstat = SCSW_DSTAT_UNIT_CHECK;
> +        sch->sense_data[0] = 0x40;    /* intervention-req */

This is really odd. If it succeeds, you generate a unit check with
intervention required? Confused. At the very least, this requires some
description as to how this device is supposed to interact with the
driver.

> +        schib->scsw.ctrl &= ~SCSW_ACTL_START_PEND;
> +        schib->scsw.ctrl &= ~SCSW_CTRL_MASK_STCTL;
> +        schib->scsw.ctrl |= SCSW_STCTL_PRIMARY | SCSW_STCTL_SECONDARY |
> +                   SCSW_STCTL_ALERT | SCSW_STCTL_STATUS_PEND;
> +    }
> +    return rc;
> +}
> +
> +static void pong_ccw_realize(DeviceState *ds, Error **errp)
> +{
> +    uint16_t chpid;
> +    CcwPONGDevice *dev = CCW_PONG(ds);
> +    CcwDevice *cdev = CCW_DEVICE(ds);
> +    CCWDeviceClass *cdk = CCW_DEVICE_GET_CLASS(cdev);
> +    SubchDev *sch;
> +    Error *err = NULL;
> +
> +    sch = css_create_sch(cdev->devno, errp);
> +    if (!sch) {
> +        return;
> +    }
> +
> +    sch->driver_data = dev;
> +    cdev->sch = sch;
> +    chpid = css_find_free_chpid(sch->cssid);
> +
> +    if (chpid > MAX_CHPID) {
> +        error_setg(&err, "No available chpid to use.");
> +        goto out_err;
> +    }
> +
> +    sch->id.reserved = 0xff;
> +    sch->id.cu_type = CCW_PONG_CU_TYPE;
> +    css_sch_build_virtual_schib(sch, (uint8_t)chpid,
> +                                CCW_PONG_CHPID_TYPE);
> +    sch->do_subchannel_work = do_subchannel_work_virtual;
> +    sch->ccw_cb = pong_ccw_cb;
> +
> +    cdk->realize(cdev, &err);
> +    if (err) {
> +        goto out_err;
> +    }
> +
> +    css_reset_sch(sch);
> +    return;
> +
> +out_err:
> +    error_propagate(errp, err);
> +    css_subch_assign(sch->cssid, sch->ssid, sch->schid, sch->devno, NULL);
> +    cdev->sch = NULL;
> +    g_free(sch);
> +}
> +
> +static Property pong_ccw_properties[] = {
> +    DEFINE_PROP_END_OF_LIST(),
> +};
> +
> +static void pong_ccw_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +
> +    dc->props = pong_ccw_properties;
> +    dc->bus_type = TYPE_VIRTUAL_CSS_BUS;
> +    dc->realize = pong_ccw_realize;
> +    dc->hotpluggable = false;
> +    set_bit(DEVICE_CATEGORY_DISPLAY, dc->categories);

Huh? misc seems like a better idea for this :)

> +}
> +
> +static const TypeInfo pong_ccw_info = {
> +    .name = TYPE_CCW_PONG,
> +    .parent = TYPE_CCW_DEVICE,
> +    .instance_size = sizeof(CcwPONGDevice),
> +    .class_init = pong_ccw_class_init,
> +    .class_size = sizeof(CcwPONGClass),
> +};
> +
> +static void pong_ccw_register(void)
> +{
> +    type_register_static(&pong_ccw_info);
> +}
> +
> +type_init(pong_ccw_register)

Some questions regarding this device and its intended usage:

- What are you trying to test? Basic ccw processing, or something more
  specific? Is there any way you can use the kvm-unit-test
  infrastructure to test basic processing with an existing device?
- Who is instantiating this device? Only the kvm-unit-test?
- Can you instantiate multiple instances? Does that make sense? If yes,
  it should probably not request a new chpid every time :)
- What happens if someone instantiates this by hand? The only drawback
  is that it uses up a subchannel and a chpid, right?
- Do you plan to make this hotpluggable later?