Re: [qemu-s390x] [Qemu-devel] [RFC PATCH v2 1/3] s390x/ccs: add ccw-testdev emulated device

[Thomas Huth]

 Hi Halil,

just a high-level review since I'm not a CSS expert...

On 08.11.2017 17:54, Halil Pasic wrote:
[...]
> I'm not really happy with the side effects of moving it to hw/misc, which
> ain't s390x specific.

Sorry, I'm missing the context - why can't this go into hw/s390x/ ?

> I've pretty much bounced off the build system, so
> I would really appreciate some help here. Currently you have to say you
> want it when you do make or it won't get compiled into your qemu. IMHO
> this device only makes sense for testing and should not be rutinely
> shipped in production builds. That is why I did not touch
> default-configs/s390x-softmmu.mak.

You could at least add a CONFIG_CCW_TESTDEV=n there, but I think the
normal QEMU policy is to enable everything by default to avoid that code
is bit-rotting, so I think "=y" is also OK there (distros can then still
disable it in downstream if they want).

> I think, I have the most problematic places marked with a  TODO
> comment in the code.
> 
> Happy reviewing and looking forward to your comments.
> ---
>  hw/misc/Makefile.objs |   1 +
>  hw/misc/ccw-testdev.c | 284 
> ++++++++++++++++++++++++++++++++++++++++++++++++++
>  hw/misc/ccw-testdev.h |  18 ++++
>  3 files changed, 303 insertions(+)
>  create mode 100644 hw/misc/ccw-testdev.c
>  create mode 100644 hw/misc/ccw-testdev.h
> 
> diff --git a/hw/misc/Makefile.objs b/hw/misc/Makefile.objs
> index 19202d90cf..b41314d096 100644
> --- a/hw/misc/Makefile.objs
> +++ b/hw/misc/Makefile.objs
> @@ -61,3 +61,4 @@ obj-$(CONFIG_AUX) += auxbus.o
>  obj-$(CONFIG_ASPEED_SOC) += aspeed_scu.o aspeed_sdmc.o
>  obj-y += mmio_interface.o
>  obj-$(CONFIG_MSF2) += msf2-sysreg.o
> +obj-$(CONFIG_CCW_TESTDEV) += ccw-testdev.o
> diff --git a/hw/misc/ccw-testdev.c b/hw/misc/ccw-testdev.c
> new file mode 100644
> index 0000000000..39cf46e90d
> --- /dev/null
> +++ b/hw/misc/ccw-testdev.c
> @@ -0,0 +1,284 @@

Please add a short description of the device in a comment here.

And don't you also want to add some license statement and/or author
information here?

> +#include "qemu/osdep.h"
> +#include "qapi/error.h"
> +#include "qemu/module.h"
> +#include "ccw-testdev.h"
> +#include "hw/s390x/css.h"
> +#include "hw/s390x/css-bridge.h"
> +#include "hw/s390x/3270-ccw.h"
> +#include "exec/address-spaces.h"
> +#include "hw/s390x/s390-virtio-hcall.h"
> +#include <error.h>
> +
> +typedef struct CcwTestDevDevice {
> +    CcwDevice parent_obj;
> +    uint16_t cu_type;
> +    uint8_t chpid_type;
> +    uint32_t op_mode;
> +    bool op_mode_locked;
> +    struct {
> +        uint32_t ring[4];
> +        unsigned int next;
> +    } fib;
> +} CcwTestDevDevice;
> +
> +typedef struct CcwTestDevClass {
> +    CCWDeviceClass parent_class;
> +    DeviceRealize parent_realize;
> +} CcwTestDevClass;
> +
> +#define TYPE_CCW_TESTDEV "ccw-testdev"
> +
> +#define CCW_TESTDEV(obj) \
> +     OBJECT_CHECK(CcwTestDevDevice, (obj), TYPE_CCW_TESTDEV)
> +#define CCW_TESTDEV_CLASS(klass) \
> +     OBJECT_CLASS_CHECK(CcwTestDevClass, (klass), TYPE_CCW_TESTDEV)
> +#define CCW_TESTDEV_GET_CLASS(obj) \
> +     OBJECT_GET_CLASS(CcwTestDevClass, (obj), TYPE_CCW_TESTDEV)
> +
> +typedef int (*ccw_cb_t)(SubchDev *, CCW1);
> +static ccw_cb_t get_ccw_cb(CcwTestDevOpMode op_mode);
> +
> +/* TODO This is the in-band set mode. We may want to get rid of it. */
> +static int set_mode_ccw(SubchDev *sch)
> +{
> +    CcwTestDevDevice *d = sch->driver_data;
> +    const char pattern[] = CCW_TST_SET_MODE_INCANTATION;
> +    char buf[sizeof(pattern)];
> +    int ret;
> +    uint32_t tmp;
> +
> +    if (d->op_mode_locked) {
> +        return -EINVAL;
> +    }
> +
> +    ret = ccw_dstream_read(&sch->cds, buf);
> +    if (ret) {
> +        return ret;
> +    }
> +    ret = strncmp(buf, pattern, sizeof(pattern));
> +    if (ret) {
> +        return 0; /* ignore malformed request -- maybe fuzzing */
> +    }
> +    ret = ccw_dstream_read(&sch->cds, tmp);
> +    if (ret) {
> +        return ret;
> +    }
> +    be32_to_cpus(&tmp);
> +    if (tmp >= OP_MODE_MAX) {
> +        return -EINVAL;
> +    }
> +    d->op_mode = tmp;
> +    sch->ccw_cb = get_ccw_cb(d->op_mode);
> +    return ret;
> +}
> +
> +

Please remove one empty line above.

> +static unsigned int abs_to_ring(unsigned int i)

IMHO a short comment above that function would be nice. If I just read
"abs_to_ring(unsigned int i)" I have a hard time to figure out what this
means.

> +{
> +    return i & 0x3;
> +}
> +
> +static int  ccw_testdev_write_fib(SubchDev *sch)
> +{
> +    CcwTestDevDevice *d = sch->driver_data;
> +    bool is_fib = true;
> +    uint32_t tmp;
> +    int ret = 0;
> +
> +    d->fib.next = 0;
> +    while (ccw_dstream_avail(&sch->cds) > 0) {
> +        ret = ccw_dstream_read(&sch->cds, tmp);
> +        if (ret) {
> +            error(0, -ret, "fib");

Where does this error() function come from? I failed to spot its location...

> +            break;
> +        }
> +        d->fib.ring[abs_to_ring(d->fib.next)] = cpu_to_be32(tmp);
> +        if (d->fib.next > 2) {
> +            tmp = (d->fib.ring[abs_to_ring(d->fib.next - 1)]
> +                  + d->fib.ring[abs_to_ring(d->fib.next - 2)]);
> +            is_fib = tmp ==  d->fib.ring[abs_to_ring(d->fib.next)];
> +            if (!is_fib) {
> +                break;
> +            }
> +        }
> +        ++(d->fib.next);

I'd prefer to do this without braces, e.g.:

           d->fib.next++;

> +    }
> +    if (!is_fib) {
> +        sch->curr_status.scsw.ctrl &= ~SCSW_ACTL_START_PEND;
> +        sch->curr_status.scsw.ctrl |= SCSW_STCTL_PRIMARY |
> +                                      SCSW_STCTL_SECONDARY |
> +                                      SCSW_STCTL_ALERT |
> +                                      SCSW_STCTL_STATUS_PEND;
> +        sch->curr_status.scsw.count = ccw_dstream_residual_count(&sch->cds);
> +        sch->curr_status.scsw.cpa = sch->channel_prog + 8;
> +        sch->curr_status.scsw.dstat =  SCSW_DSTAT_UNIT_EXCEP;
> +        return -EIO;
> +    }
> +    return ret;
> +}
> +
> +static int  ccw_testdev_read_fib(SubchDev *sch)
> +{
> +    uint32_t l = 0, m = 1, n = 0;
> +    int ret = 0;
> +
> +    while (ccw_dstream_avail(&sch->cds) > 0) {
> +        n = m + l;
> +        l = m;
> +        m = n;
> +        ret = ccw_dstream_read(&sch->cds, n);
> +    }
> +    return ret;
> +}
> +
> +static int ccw_testdev_ccw_cb_mode_fib(SubchDev *sch, CCW1 ccw)
> +{
> +    switch (ccw.cmd_code) {
> +    case CCW_CMD_READ:
> +        ccw_testdev_read_fib(sch);
> +        break;
> +    case CCW_CMD_WRITE:
> +        return ccw_testdev_write_fib(sch);
> +    case CCW_CMD_CTL_MODE:
> +        return set_mode_ccw(sch);
> +    default:
> +        return -EINVAL;
> +    }
> +    return 0;
> +}
> +
> +static int ccw_testdev_ccw_cb_mode_nop(SubchDev *sch, CCW1 ccw)
> +{
> +    CcwTestDevDevice *d = sch->driver_data;
> +
> +    if (!d->op_mode_locked && ccw.cmd_code == CCW_CMD_CTL_MODE) {
> +        return set_mode_ccw(sch);
> +    }
> +    return 0;
> +}
> +
> +static ccw_cb_t get_ccw_cb(CcwTestDevOpMode op_mode)
> +{
> +    switch (op_mode) {
> +    case OP_MODE_FIB:
> +        return ccw_testdev_ccw_cb_mode_fib;
> +    case OP_MODE_NOP:
> +    default:
> +        return ccw_testdev_ccw_cb_mode_nop;

Do we really want to use "nop" for unknown modes? Or should there rather
be a ccw_testdev_ccw_cb_mode_error instead, too?

> +    }
> +}
> +
> +static void ccw_testdev_realize(DeviceState *ds, Error **errp)
> +{
> +    uint16_t chpid;
> +    CcwTestDevDevice *dev = CCW_TESTDEV(ds);
> +    CcwTestDevClass *ctc = CCW_TESTDEV_GET_CLASS(dev);
> +    CcwDevice *cdev = CCW_DEVICE(ds);
> +    BusState *qbus = qdev_get_parent_bus(ds);
> +    VirtualCssBus *cbus = VIRTUAL_CSS_BUS(qbus);
> +    SubchDev *sch;
> +    Error *err = NULL;
> +
> +    sch = css_create_sch(cdev->devno, true, cbus->squash_mcss, 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 = dev->cu_type;
> +    css_sch_build_virtual_schib(sch, (uint8_t)chpid,
> +                                dev->chpid_type);
> +    sch->ccw_cb = get_ccw_cb(dev->op_mode);
> +    sch->do_subchannel_work = do_subchannel_work_virtual;
> +
> +

Please remove superfluous empty line.

> +    ctc->parent_realize(ds, &err);
> +    if (err) {
> +        goto out_err;
> +    }
> +
> +    css_generate_sch_crws(sch->cssid, sch->ssid, sch->schid,
> +                          ds->hotplugged, 1);
> +    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 ccw_testdev_properties[] = {
> +    DEFINE_PROP_UINT16("cu_type", CcwTestDevDevice, cu_type,
> +                        0xfffe), /* only numbers used for real HW */
> +    DEFINE_PROP_UINT8("chpid_type", CcwTestDevDevice, chpid_type,
> +                       0x25), /* took FC, TODO discuss */
> +    DEFINE_PROP_UINT32("op_mode", CcwTestDevDevice, op_mode,
> +                       0), /* TODO discuss */
> +    DEFINE_PROP_BOOL("op_mode_locked", CcwTestDevDevice, op_mode_locked,
> +                       false), /* TODO discuss */
> +    DEFINE_PROP_END_OF_LIST(),
> +};
> +
> +/* TODO This is the out-of-band variant. We may want to get rid of it */

I agree, this should rather go away in the final version.

> +static int set_mode_diag(const uint64_t *args)
> +{
> +    uint64_t subch_id = args[0];
> +    uint64_t op_mode = args[1];
> +    SubchDev *sch;
> +    CcwTestDevDevice *dev;
> +    int cssid, ssid, schid, m;
> +
> +    if (ioinst_disassemble_sch_ident(subch_id, &m, &cssid, &ssid, &schid)) {
> +        return -EINVAL;
> +    }
> +    sch = css_find_subch(m, cssid, ssid, schid);
> +    if (!sch || !css_subch_visible(sch)) {
> +        return -EINVAL;
> +    }
> +    dev = CCW_TESTDEV(sch->driver_data);
> +    if (dev->op_mode_locked) {
> +        return op_mode == dev->op_mode ? 0 : -EINVAL;
> +    }
> +    dev->op_mode = op_mode;
> +    sch->ccw_cb = get_ccw_cb(dev->op_mode);
> +    return 0;
> +}
> +
> +static void ccw_testdev_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +    CcwTestDevClass *ctc = CCW_TESTDEV_CLASS(klass);
> +
> +    dc->props = ccw_testdev_properties;
> +    dc->bus_type = TYPE_VIRTUAL_CSS_BUS;
> +    ctc->parent_realize = dc->realize;
> +    dc->realize = ccw_testdev_realize;
> +    dc->hotpluggable = false;
> +
> +    s390_register_virtio_hypercall(CCW_TST_DIAG_500_SUB, set_mode_diag);
> +}
> +
> +static const TypeInfo ccw_testdev_info = {
> +    .name = TYPE_CCW_TESTDEV,
> +    .parent = TYPE_CCW_DEVICE,
> +    .instance_size = sizeof(CcwTestDevDevice),
> +    .class_init = ccw_testdev_class_init,
> +    .class_size = sizeof(CcwTestDevClass),
> +};
> +
> +static void ccw_testdev_register(void)
> +{
> +    type_register_static(&ccw_testdev_info);
> +}
> +
> +type_init(ccw_testdev_register)
> diff --git a/hw/misc/ccw-testdev.h b/hw/misc/ccw-testdev.h
> new file mode 100644
> index 0000000000..f4d4570f5e
> --- /dev/null
> +++ b/hw/misc/ccw-testdev.h
> @@ -0,0 +1,18 @@

Add some license statement and/or author information here?

> +#ifndef HW_s390X_CCW_TESTDEV_H
> +#define HW_s390X_CCW_TESTDEV_H
> +
> +typedef enum CcwTestDevOpMode {
> +    OP_MODE_NOP = 0,
> +    OP_MODE_FIB = 1,
> +    OP_MODE_MAX /* extremal element */
> +} CcwTestDevOpMode;
> +
> +#define CCW_CMD_READ 0x01U
> +#define CCW_CMD_WRITE 0x02U
> +
> +#define CCW_CMD_CTL_MODE 0x07U
> +#define CCW_TST_SET_MODE_INCANTATION "SET MODE="
> +/* Subcode for diagnose 500 (virtio hypercall). */
> +#define CCW_TST_DIAG_500_SUB 254
> +
> +#endif

 Thomas