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
