[Qemu-devel] [RFC PATCH v2 0/3] tests for CCW IDA

[Qemu-devel] [RFC PATCH v2 0/3] tests for CCW IDA

[Halil Pasic]

I've keept the title althogh the scope shifted a bit: it's
more about introducing ccw-testdev than about IDA. The goal
is to facilitate testing the virtual channel subsystem
implementation, and the ccw interpretation.

The first patch is the interesting one. See it's cover letter
for details. The RFC is about discussing some technical issues
with this patch.

The other two patches are an out of source kernel module which
is basically only there so you can try out the first patch. The
tests there should probably be ported to something else. I don't
know what: maybe kvm-unit-tests, maybe qtest+libqos, or maybe some
bios based test image. We still have to figure out that. 

Halil Pasic (3):
  s390x/ccs: add ccw-testdev emulated device
  ccw-tester: a tester device for ccw I/O
  ccw-tester: add tic test

 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

-- 
2.13.5

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

[Halil Pasic]

Add a fake device meant for testing the correctness of our css emulation.

What we currently have is writing a Fibonacci sequence of uint32_t to the
device via ccw write. The write is going to fail if it ain't a Fibonacci
and indicate a device exception in scsw together with the proper residual
count. With this we can do basic verification of data integrity.

Of course lot's of invalid inputs (besides basic data processing) can be
tested with that as well.

We also have a no-op mode where the device just tells all-good! This
could be useful for fuzzing.

Usage:
1) fire up a qemu with something like -device ccw-testdev,devno=fe.0.0001
   on the command line
2) exercise the device from the guest

Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
---

Introduction
------------

While discussing v1 we (more or less) decided a test device for ccw is a
good idea. This is an RFC because there are some unresolved technical
questions I would like to discuss.

Usage
-----

Build like this:  make CONFIG_CCW_TESTDEV=y

Changelog
---------

v1 -> v2:
- Renamed and moved to hw/misc/
- Changed cu_type to 0xfffe.
- Changed chpid_type to 0x25 (questionable).
- Extensibility: addedd both in-band (ccw) and out-of-band set mode
  mechanism. Currently we have two modes: fib and no-op. The purpose
  of the mode mechanism is to facilitate different behaviors. One can
  both specify and lock a mode on the command line.
- Added read for fib mode.

Things I've figured out and things to figure out
-----------------------------------------------

The zVM folks say they don't have a reserved cu_type reserved for test
(or hypervisor use in general). So I've gone with cu_type  0xfffe because
according to Christian  only numeric digit hex values are used, and Linux
uses x0ffff as extremal value.

The zVM folks say the don't have a chpid_type reserved for test (or
hypervisor use in general. So I took 0x25 for now, which belongs to FC
and should be safe in my opinion.

AFAIR there was some discussion on using a dedicated diag function code.
There are multiple such that could be re-purposed for out-of-band
signaling reserved by zVM. For now I've decided to go with a new subcode
for diag 0x500 as it appears to me that it requires less changes.

I've implemented both in-band and out-of-band signaling for influencing
what the testdev does from the guest. We should probably get rid of one.
I've just implemented both so we can discuss pros and cons with some
code.

I've taken subcode 254, the last one supported at the moment. I'm not
really happy with the way I 'took' it. Maybe all taken subcodes
could go into hw/s390x/s390-virtio-hcall.h either via include or
directly.

I'm not really happy with the side effects of moving it to hw/misc, which
ain't s390x specific. 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.

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 @@
+#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;
+}
+
+
+static unsigned int abs_to_ring(unsigned int i)
+{
+    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");
+            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);
+    }
+    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;
+    }
+}
+
+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;
+
+
+    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 */
+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 @@
+#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
-- 
2.13.5

[Qemu-devel] [RFC PATCH NOT QEMU v2 2/3] ccw-tester: a tester device for ccw I/O

[Halil Pasic]

Let's introduce a device driver for doing ccw I/O tests. The initial
focus is on indirect data access.

The driver is impemented as an out-of-tree Linux kernel module. A module
parameter cu_type is used for matching ccw devices. The parameter
defaults to 0x3831 which is the default cu_type for the qemu
counterpart of this (a fully emulated ccw device just for test).

The current status of the module is means to an end where the end
is testing my IDA implementation.

Usage:

You load the module.  The driver is supposed to auto detect and auto
online the device and provide sysfs atributes for the tests available.
Tests are triggered by writing to the attributes. Reoprting is done via
printk in almost TAP format (could use improvement if more ambitious).
We run one test at a time and do that async to the write. If you try to
start more in parallel you will get -EBUSY.

Currently all you can do something like:
* echo 1 > /sys/bus/ccw/devices/<devno>/w_fib
  To test good old ccw.
* echo 1 > /sys/bus/ccw/devices/<devno>/w_fib_idal
  To test IDA ccw.

These tests are designed to wrok together with the qemu device mentioned
before. The basic idea is that a device is expecting a stream of words
such that the sequence words interpreted as uint32_t is a Fibonacci
sequence (that is for n > 2 a_{n} = a_{n-1} + a{n-2}).

Using his simple scheme one can check that the right bytes are
transferred (with reasonable confidence). If the device detects an
element violating the Fibonacci property the driver expects the device
posts a device exception indicating that element.

Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
---

Do not try to apply this to a QEMU tree. Use an empty repo.
---
 .gitignore    |   8 +
 Makefile      |  10 ++
 ccw-testdev.h |  18 +++
 ccw_tester.c  | 467 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 503 insertions(+)
 create mode 100644 .gitignore
 create mode 100644 Makefile
 create mode 100644 ccw-testdev.h
 create mode 100644 ccw_tester.c

diff --git a/.gitignore b/.gitignore
new file mode 100644
index 0000000..1b9eac9
--- /dev/null
+++ b/.gitignore
@@ -0,0 +1,8 @@
+#ignore these
+*.o
+*.cmd
+*.ko
+*.mod.c
+Module.symvers
+modules.order
+.tmp_versions/
diff --git a/Makefile b/Makefile
new file mode 100644
index 0000000..0583456
--- /dev/null
+++ b/Makefile
@@ -0,0 +1,10 @@
+ifneq ($(KERNELRELEASE),)
+obj-m  := ccw_tester.o
+else
+# normal makefile
+KDIR ?= /lib/modules/`uname -r`/build
+
+default:
+	$(MAKE) -C $(KDIR) M=$$PWD
+
+endif
diff --git a/ccw-testdev.h b/ccw-testdev.h
new file mode 100644
index 0000000..f4d4570
--- /dev/null
+++ b/ccw-testdev.h
@@ -0,0 +1,18 @@
+#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
diff --git a/ccw_tester.c b/ccw_tester.c
new file mode 100644
index 0000000..b8e632b
--- /dev/null
+++ b/ccw_tester.c
@@ -0,0 +1,467 @@
+#include <linux/kernel_stat.h>
+#include <linux/init.h>
+#include <linux/bootmem.h>
+#include <linux/err.h>
+#include <linux/slab.h>
+#include <linux/string.h>
+#include <linux/interrupt.h>
+#include <linux/pfn.h>
+#include <linux/async.h>
+#include <linux/wait.h>
+#include <linux/list.h>
+#include <linux/bitops.h>
+#include <linux/module.h>
+#include <linux/moduleparam.h>
+#include <linux/io.h>
+#include <linux/kvm_para.h>
+#include <linux/notifier.h>
+#include <asm/diag.h>
+#include <asm/setup.h>
+#include <asm/irq.h>
+#include <asm/cio.h>
+#include <asm/ccwdev.h>
+#include <asm/isc.h>
+#include <asm/airq.h>
+#include <asm/idals.h>
+#include <ccw-testdev.h>
+
+inline bool _ccw_test_assert(bool expr, const char *loc, int ln,
+			     const char *expl)
+{
+	if (expr)
+		printk(KERN_NOTICE "ok -- %s:%d\n", loc, ln);
+	else
+		printk(KERN_WARNING "not ok  -- %s:%d (%s)\n", loc, ln, expl);
+	return expr;
+}
+
+
+#define ccw_test_assert(_expr, _expl) ({_ccw_test_assert((_expr), \
+			__func__,  __LINE__, (_expl)); })
+
+struct workqueue_struct *work_q;
+
+static __u16 cu_type = 0xfffe;
+module_param(cu_type, ushort, 0444);
+MODULE_PARM_DESC(cu_type, "Use this cu type for matching (default 0x3831)");
+
+
+static struct ccw_device_id ccw_tester_ids[] = {
+	{ CCW_DEVICE(0, 0) }, /* placeholder */
+	{},
+};
+
+struct ccw_test_work {
+	struct work_struct work;
+	struct ccw1 *ccw;
+	__u32  intparm;
+	void *private;
+	void (*setup)(struct ccw_test_work *w);
+	void (*do_test)(struct ccw_test_work *w);
+	void (*teardown)(struct ccw_test_work *w);
+	struct irb irb;
+	int ret;
+	bool doing_io;
+};
+
+struct ccw_tester_device {
+	spinlock_t lock;
+	wait_queue_head_t wait_q;
+	struct ccw_device *cdev;
+	struct ccw_test_work work;
+	bool work_pending;
+};
+
+static struct ccw_tester_device *to_mydev(struct ccw_device *cdev)
+{
+	return dev_get_drvdata(&(cdev->dev));
+}
+
+
+static void ccw_tester_auto_online(void *data, async_cookie_t cookie)
+{
+	struct ccw_device *cdev = data;
+	int ret;
+
+	ret = ccw_device_set_online(cdev);
+	if (ret)
+		dev_warn(&cdev->dev, "Failed to set online: %d\n", ret);
+}
+
+static void do_io_work(struct ccw_tester_device *tdev)
+{
+	struct ccw_test_work *w = &tdev->work;
+	unsigned long flags;
+	int retry = 124;
+
+	do {
+		spin_lock_irqsave(get_ccwdev_lock(tdev->cdev), flags);
+		tdev->work.doing_io = true;
+		w->ret = ccw_device_start(tdev->cdev, w->ccw, w->intparm, 0, 0);
+		spin_unlock_irqrestore(get_ccwdev_lock(tdev->cdev), flags);
+		cpu_relax();
+	} while (w->ret == -EBUSY && --retry > 0);
+	wait_event(tdev->wait_q, w->doing_io == false);
+}
+
+static void do_test_do_io(struct ccw_test_work *w)
+{
+	struct ccw_tester_device *tdev;
+
+	tdev = container_of(w, struct ccw_tester_device, work);
+	do_io_work(tdev);
+}
+
+static int irb_is_error(struct irb *irb)
+{
+	if (scsw_cstat(&irb->scsw) != 0)
+		return 1;
+	if (scsw_dstat(&irb->scsw) & ~(DEV_STAT_CHN_END | DEV_STAT_DEV_END))
+		return 1;
+	if (scsw_cc(&irb->scsw) != 0)
+		return 1;
+	return 0;
+}
+
+static void set_mode_ccw(struct ccw_test_work *w, u32 mode)
+{
+	const char incant[] = CCW_TST_SET_MODE_INCANTATION;
+	char *buf;
+
+	buf = kzalloc(sizeof(incant) + sizeof(mode), GFP_DMA | GFP_KERNEL);
+
+	memcpy(buf, incant, sizeof(incant));
+	memcpy(buf + sizeof(incant), &mode, sizeof(mode));
+	w->ccw->cmd_code = CCW_CMD_CTL_MODE;
+	w->ccw->count = sizeof(incant) + sizeof(mode) ;
+	w->ccw->cda = (__u32)(unsigned long) buf;
+	do_test_do_io(w);
+	w->ret = irb_is_error(&w->irb) ? -EINVAL : 0;
+	kfree(buf);
+}
+
+static long set_mode_diag(struct subchannel_id schid, u32 mode)
+{
+	register unsigned long __nr asm("1") = CCW_TST_DIAG_500_SUB;
+	register struct subchannel_id __schid asm("2") = schid;
+	register unsigned long __mode asm("3") = mode;
+	register long __rc asm("2");
+
+	asm volatile ("diag 2,3,0x500\n"
+		: "=d" (__rc) : "d" (__nr), "d" (__schid), "d" (__mode)
+		: "memory", "cc");
+	return __rc;
+}
+
+static void w_fib_setup(struct ccw_test_work *w)
+{
+	const int test_fib_length = 32;
+	u32 *test_fib;
+	int i;
+
+	set_mode_ccw(w, OP_MODE_FIB);
+	if (w->ret) {
+		printk(KERN_WARNING "w_fib_setup ret = %d\n", w->ret);
+		w->ret = 0;
+		return;
+	}
+
+	test_fib = kcalloc(test_fib_length, sizeof(u32),
+				       GFP_DMA | GFP_KERNEL);
+	if (!test_fib)
+		w->ret = -ENOMEM;
+	w->private = test_fib;
+
+	test_fib[0] = 1;
+	test_fib[1] = 2;
+	for (i = 2; i < test_fib_length; ++i)
+		test_fib[i] = test_fib[i - 1] + test_fib[i - 2];
+
+	w->ccw->cmd_code = CCW_CMD_WRITE;
+	w->ccw->count = sizeof(*test_fib) * test_fib_length;
+	w->ccw->cda = (__u32)(unsigned long) test_fib;
+}
+
+static void basic_teardown(struct ccw_test_work *w)
+{
+	kfree(w->private);
+	w->private = NULL;
+	if (w->ret)
+		printk(KERN_WARNING "w_fib_teardown ret = %d\n", w->ret);
+}
+
+static void ccw_tester_int_handler(struct ccw_device *cdev,
+				   unsigned long intparm,
+				   struct irb *irb)
+{
+	struct ccw_tester_device *tdev = to_mydev(cdev);
+
+	memcpy(&tdev->work.irb, irb, sizeof(*irb));
+	tdev->work.doing_io = false;
+	wake_up(&tdev->wait_q);
+}
+
+static bool expect_is_not_fib(struct irb *irb, int count_expected)
+{
+	if (!(irb_is_error(irb)
+		&& (scsw_dstat(&irb->scsw) & DEV_STAT_UNIT_EXCEP)
+		&& scsw_stctl(&irb->scsw) & SCSW_STCTL_ALERT_STATUS))
+		return false;
+	if (irb->scsw.cmd.count == count_expected)
+		return true;
+	printk(KERN_NOTICE
+		"expected residual count of %d got %d (fib at wrong place)\n",
+		count_expected, irb->scsw.cmd.count);
+	return false;
+}
+
+
+static void w_fib_do_test(struct ccw_test_work *w)
+{
+	u32 *test_fib = w->private;
+
+	do_test_do_io(w);
+	ccw_test_assert(!irb_is_error(&w->irb), "completion expected");
+	test_fib[25] = 0;
+	do_test_do_io(w);
+	ccw_test_assert(expect_is_not_fib(&w->irb,
+		(31-25)*sizeof(u32)), "expected non fib");
+}
+
+
+static int queue_ccw_test_work(struct ccw_tester_device *tdev,
+		void (*setup)(struct ccw_test_work *),
+		void (*do_test)(struct ccw_test_work *),
+		void (*teardown)(struct ccw_test_work *))
+{
+	if (!spin_trylock(&tdev->lock))
+		return -EBUSY;
+	if (tdev->work_pending) {
+		spin_unlock(&tdev->lock);
+		return -EBUSY;
+	}
+	tdev->work_pending = true;
+	tdev->work.setup = setup;
+	tdev->work.do_test = do_test;
+	tdev->work.teardown = teardown;
+	queue_work(work_q, &tdev->work.work);
+	spin_unlock(&tdev->lock);
+	return 0;
+}
+
+
+static ssize_t w_fib_store(struct device *dev, struct device_attribute *attr,
+			     const char *buf, size_t count)
+{
+	struct ccw_tester_device *tdev = to_mydev(to_ccwdev(dev));
+	int ret;
+
+	ret = queue_ccw_test_work(tdev,
+		w_fib_setup, w_fib_do_test, basic_teardown);
+	return ret ? ret : count;
+}
+
+static u32 *u32_arr_in_idal_buf_at(struct idal_buffer const *ib, int i)
+{
+	u64 b = IDA_BLOCK_SIZE/sizeof(u32);
+
+	return  (u32 *)(ib->data[i/b]) + i % b;
+}
+
+#define IDAL_TEST_BYTES  (IDA_BLOCK_SIZE * 3 + IDA_BLOCK_SIZE/2)
+#define IDAL_TEST_ELEMENTS  (IDAL_TEST_BYTES/sizeof(u32))
+
+static void fib_idal_setup(struct ccw_test_work *w)
+{
+	struct ccw_tester_device *tdev;
+	struct idal_buffer *ib = NULL;
+	u32 n, n_1 = 2, n_2 = 1;
+	int i = 0;
+	struct subchannel_id schid;
+
+	tdev = container_of(w, struct ccw_tester_device, work);
+	ccw_device_get_schid(tdev->cdev, &schid);
+	w->ret = set_mode_diag(schid, OP_MODE_FIB);
+	if (w->ret) {
+		printk(KERN_WARNING "w_fib_idal_setup ret = %d\n", w->ret);
+		w->ret = 0;
+		return;
+	}
+	ib = idal_buffer_alloc(IDAL_TEST_BYTES, 0);
+	if (IS_ERR(ib)) {
+		w->ret = PTR_ERR(ib);
+		return;
+	}
+	w->private = ib;
+	*u32_arr_in_idal_buf_at(ib, 0) = n_2;
+	*u32_arr_in_idal_buf_at(ib, 1) = n_1;
+	for (i = 2; i < IDAL_TEST_ELEMENTS; ++i) {
+		n = n_1 + n_2;
+		n_2 = n_1;
+		n_1 = n;
+		*u32_arr_in_idal_buf_at(ib, i) = n;
+	}
+	idal_buffer_set_cda(ib, w->ccw);
+	w->ccw->count = IDAL_TEST_BYTES;
+	w->ccw->cmd_code = CCW_CMD_WRITE;
+}
+
+static void fib_idal_teardown(struct ccw_test_work *w)
+{
+	if (w->private) {
+		idal_buffer_free(w->private);
+		w->private = NULL;
+	}
+	if (w->ret)
+		printk(KERN_WARNING "fib_idal_teardown ret = %d\n", w->ret);
+}
+
+static void do_fib_idal_test(struct ccw_test_work *w)
+{
+	struct idal_buffer *ib = w->private;
+
+	/* we have one already set up, fire it */
+	do_test_do_io(w);
+	ccw_test_assert(!irb_is_error(&w->irb), "completion expected");
+
+	/* let's break fib and check if the device detects it */
+	++(*u32_arr_in_idal_buf_at(ib, IDAL_TEST_ELEMENTS - 5));
+	do_test_do_io(w);
+	ccw_test_assert(expect_is_not_fib(&w->irb,
+			4 * sizeof(u32)), "expected non fib");
+	/* shorten the seq so the broken element is not included */
+	w->ccw->count = IDAL_TEST_BYTES - 5 * sizeof(u32);
+	do_test_do_io(w);
+	ccw_test_assert(!irb_is_error(&w->irb), "completion expected");
+}
+
+static ssize_t w_fib_idal_store(struct device *dev,
+				struct device_attribute *attr,
+				const char *buf, size_t count)
+{
+	int ret;
+	struct ccw_tester_device *tdev = to_mydev(to_ccwdev(dev));
+
+	ret = queue_ccw_test_work(tdev,
+			fib_idal_setup, do_fib_idal_test, fib_idal_teardown);
+	return ret ? ret : count;
+}
+
+static DEVICE_ATTR_WO(w_fib);
+static DEVICE_ATTR_WO(w_fib_idal);
+
+static void do_ccw_test_work(struct work_struct *work)
+{
+
+	struct ccw_test_work *w;
+	struct ccw_tester_device *tdev;
+
+	w = container_of(work, struct ccw_test_work, work);
+	tdev = container_of(w, struct ccw_tester_device, work);
+
+	w->ret = 0;
+	w->setup(w);
+	w->do_test(w);
+	w->teardown(w);
+	spin_lock(&tdev->lock);
+	tdev->work_pending = false;
+	spin_unlock(&tdev->lock);
+	memset(w->ccw, 0, sizeof(*(w->ccw)));
+	memset(&w->irb, 0, sizeof(w->irb));
+}
+
+static int ccw_tester_offline(struct ccw_device *cdev)
+{
+	struct ccw_tester_device *tdev = to_mydev(cdev);
+
+	if (!tdev)
+		return 0;
+	device_remove_file(&(cdev->dev), &dev_attr_w_fib);
+	device_remove_file(&(cdev->dev), &dev_attr_w_fib_idal);
+	spin_lock(&tdev->lock);
+	tdev->work_pending = true;
+	spin_unlock(&tdev->lock);
+	kfree(tdev->work.ccw);
+	tdev->work.ccw = NULL;
+	kfree(tdev);
+	dev_set_drvdata(&cdev->dev, NULL);
+	return 0;
+}
+
+static int ccw_tester_online(struct ccw_device *cdev)
+{
+	int ret;
+	struct ccw_tester_device *tdev;
+
+	tdev = kzalloc(sizeof(*tdev), GFP_KERNEL);
+	if (!tdev) {
+		dev_warn(&cdev->dev, "Could not get memory\n");
+		return -ENOMEM;
+	}
+	init_waitqueue_head(&tdev->wait_q);
+	INIT_WORK(&(tdev->work.work), do_ccw_test_work);
+	spin_lock_init(&tdev->lock);
+	tdev->work.ccw = kzalloc(sizeof(*tdev->work.ccw), GFP_DMA | GFP_KERNEL);
+	if (!tdev) {
+		dev_warn(&cdev->dev, "Could not get memory\n");
+		ret = -ENOMEM;
+		goto out_free;
+	}
+	dev_set_drvdata(&cdev->dev, tdev);
+	tdev->cdev = cdev;
+
+	ret = device_create_file(&(cdev->dev), &dev_attr_w_fib);
+	if (ret)
+		goto out_free;
+	ret = device_create_file(&(cdev->dev), &dev_attr_w_fib_idal);
+	if (ret)
+		goto out_free;
+	return ret;
+out_free:
+	ccw_tester_offline(cdev);
+	return ret;
+}
+
+static void ccw_tester_remove(struct ccw_device *cdev)
+{
+	ccw_device_set_offline(cdev);
+}
+
+static int ccw_tester_probe(struct ccw_device *cdev)
+{
+	cdev->handler = ccw_tester_int_handler;
+	async_schedule(ccw_tester_auto_online, cdev);
+	return 0;
+}
+
+static struct ccw_driver ccw_tester_driver = {
+	.driver = {
+		.owner = THIS_MODULE,
+		.name = "ccw_tester",
+	},
+	.ids = ccw_tester_ids,
+	.probe = ccw_tester_probe,
+	.set_online = ccw_tester_online,
+	.set_offline = ccw_tester_offline,
+	.remove = ccw_tester_remove,
+	.int_class = IRQIO_VIR,
+};
+
+
+static int __init ccw_tester_init(void)
+{
+	work_q = create_singlethread_workqueue("ccw-tester");
+	ccw_tester_ids[0].cu_type = cu_type;
+	return ccw_driver_register(&ccw_tester_driver);
+}
+module_init(ccw_tester_init);
+
+static void __exit ccw_tester_exit(void)
+{
+	ccw_driver_unregister(&ccw_tester_driver);
+}
+module_exit(ccw_tester_exit);
+
+MODULE_DESCRIPTION("ccw test driver -- throw ccws at devices");
+MODULE_LICENSE("GPL v2");
+MODULE_AUTHOR("Halil Pasic <pasic@linux.vnet.ibm.com>");
-- 
2.13.5

[Qemu-devel] [RFC PATCH NOT QEMU v2 3/3] ccw-tester: add tic test

[Halil Pasic]

Let's add a test verifying that the channel subsystlem responds to a
format 1 transfer in channel ccw with non-zero count properly -- with a
channel program check.

Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
---
 ccw_tester.c | 69 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 69 insertions(+)

diff --git a/ccw_tester.c b/ccw_tester.c
index b8e632b..6253ae7 100644
--- a/ccw_tester.c
+++ b/ccw_tester.c
@@ -347,8 +347,73 @@ static ssize_t w_fib_idal_store(struct device *dev,
 	return ret ? ret : count;
 }
 
+static void tic_setup(struct ccw_test_work *w)
+{
+	struct ccw_tester_device *tdev;
+	struct subchannel_id schid;
+
+	tdev = container_of(w, struct ccw_tester_device, work);
+	ccw_device_get_schid(tdev->cdev, &schid);
+	w->ret = set_mode_diag(schid, OP_MODE_NOP);
+	if (w->ret) {
+		printk(KERN_WARNING "tic_setup ret = %d\n", w->ret);
+		w->ret = 0;
+		return;
+	}
+	w->private = kzalloc(sizeof(*w->ccw), GFP_DMA | GFP_KERNEL);
+
+	w->ccw->count = 0x0666; /* we are evil */
+	w->ccw->cmd_code = CCW_CMD_TIC;
+	/* hope this won't get used */
+	w->ccw->cda = (__u32)(unsigned long) w->private;
+}
+
+static void tic_teardown(struct ccw_test_work *w)
+{
+	if (w->private) {
+		idal_buffer_free(w->private);
+		w->private = NULL;
+	}
+	if (w->ret)
+		printk(KERN_WARNING "tic_teardown ret = %d\n", w->ret);
+}
+
+static bool expect_pgm_chk(struct irb *irb)
+{
+	if (irb_is_error(irb)
+		&& (scsw_cstat(&irb->scsw) & SCHN_STAT_PROG_CHECK)
+		&& scsw_stctl(&irb->scsw) & SCSW_STCTL_ALERT_STATUS)
+		return true;
+	printk(KERN_NOTICE
+		"expected program check but got none (is_error == %d)\n",
+		 irb_is_error(irb));
+	return false;
+}
+
+static void do_tic_test(struct ccw_test_work *w)
+{
+	/* we have one already set up, fire it */
+	do_test_do_io(w);
+	/* TODO: check for pgm-check */
+	ccw_test_assert(expect_pgm_chk(&w->irb),
+			"expected pgm check for tic with count != 0 fib");
+}
+
+static ssize_t w_tic_store(struct device *dev,
+				struct device_attribute *attr,
+				const char *buf, size_t count)
+{
+	int ret;
+	struct ccw_tester_device *tdev = to_mydev(to_ccwdev(dev));
+
+	ret = queue_ccw_test_work(tdev,
+			tic_setup, do_tic_test, tic_teardown);
+	return ret ? ret : count;
+}
+
 static DEVICE_ATTR_WO(w_fib);
 static DEVICE_ATTR_WO(w_fib_idal);
+static DEVICE_ATTR_WO(w_tic);
 
 static void do_ccw_test_work(struct work_struct *work)
 {
@@ -378,6 +443,7 @@ static int ccw_tester_offline(struct ccw_device *cdev)
 		return 0;
 	device_remove_file(&(cdev->dev), &dev_attr_w_fib);
 	device_remove_file(&(cdev->dev), &dev_attr_w_fib_idal);
+	device_remove_file(&(cdev->dev), &dev_attr_w_tic);
 	spin_lock(&tdev->lock);
 	tdev->work_pending = true;
 	spin_unlock(&tdev->lock);
@@ -416,6 +482,9 @@ static int ccw_tester_online(struct ccw_device *cdev)
 	ret = device_create_file(&(cdev->dev), &dev_attr_w_fib_idal);
 	if (ret)
 		goto out_free;
+	ret = device_create_file(&(cdev->dev), &dev_attr_w_tic);
+	if (ret)
+		goto out_free;
 	return ret;
 out_free:
 	ccw_tester_offline(cdev);
-- 
2.13.5

Re: [Qemu-devel] [RFC PATCH v2 0/3] tests for CCW IDA

[Halil Pasic]

ping

On 11/08/2017 05:54 PM, Halil Pasic wrote:
> I've keept the title althogh the scope shifted a bit: it's
> more about introducing ccw-testdev than about IDA. The goal
> is to facilitate testing the virtual channel subsystem
> implementation, and the ccw interpretation.
> 
> The first patch is the interesting one. See it's cover letter
> for details. The RFC is about discussing some technical issues
> with this patch.
> 
> The other two patches are an out of source kernel module which
> is basically only there so you can try out the first patch. The
> tests there should probably be ported to something else. I don't
> know what: maybe kvm-unit-tests, maybe qtest+libqos, or maybe some
> bios based test image. We still have to figure out that. 
> 
> Halil Pasic (3):
>   s390x/ccs: add ccw-testdev emulated device
>   ccw-tester: a tester device for ccw I/O
>   ccw-tester: add tic test
> 
>  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
> 

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

[Cornelia Huck]

On Wed,  8 Nov 2017 17:54:20 +0100
Halil Pasic <pasic@linux.vnet.ibm.com> wrote:

Subject: s/ccs/css/

> Add a fake device meant for testing the correctness of our css emulation.
> 
> What we currently have is writing a Fibonacci sequence of uint32_t to the
> device via ccw write. The write is going to fail if it ain't a Fibonacci
> and indicate a device exception in scsw together with the proper residual
> count. With this we can do basic verification of data integrity.
> 
> Of course lot's of invalid inputs (besides basic data processing) can be
> tested with that as well.
> 
> We also have a no-op mode where the device just tells all-good! This
> could be useful for fuzzing.
> 
> Usage:
> 1) fire up a qemu with something like -device ccw-testdev,devno=fe.0.0001
>    on the command line
> 2) exercise the device from the guest
> 
> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
> ---
> 
> Introduction
> ------------
> 
> While discussing v1 we (more or less) decided a test device for ccw is a
> good idea. This is an RFC because there are some unresolved technical
> questions I would like to discuss.
> 
> Usage
> -----
> 
> Build like this:  make CONFIG_CCW_TESTDEV=y
> 
> Changelog
> ---------
> 
> v1 -> v2:
> - Renamed and moved to hw/misc/
> - Changed cu_type to 0xfffe.
> - Changed chpid_type to 0x25 (questionable).
> - Extensibility: addedd both in-band (ccw) and out-of-band set mode
>   mechanism. Currently we have two modes: fib and no-op. The purpose
>   of the mode mechanism is to facilitate different behaviors. One can
>   both specify and lock a mode on the command line.
> - Added read for fib mode.
> 
> Things I've figured out and things to figure out
> -----------------------------------------------
> 
> The zVM folks say they don't have a reserved cu_type reserved for test
> (or hypervisor use in general). So I've gone with cu_type  0xfffe because
> according to Christian  only numeric digit hex values are used, and Linux
> uses x0ffff as extremal value.

ack

> 
> The zVM folks say the don't have a chpid_type reserved for test (or
> hypervisor use in general. So I took 0x25 for now, which belongs to FC
> and should be safe in my opinion.

That's fine, I think.

> 
> AFAIR there was some discussion on using a dedicated diag function code.
> There are multiple such that could be re-purposed for out-of-band
> signaling reserved by zVM. For now I've decided to go with a new subcode
> for diag 0x500 as it appears to me that it requires less changes.

It means that we don't have to synchronize with anyone else, yes.

> 
> I've implemented both in-band and out-of-band signaling for influencing
> what the testdev does from the guest. We should probably get rid of one.
> I've just implemented both so we can discuss pros and cons with some
> code.
> 
> I've taken subcode 254, the last one supported at the moment. I'm not
> really happy with the way I 'took' it. Maybe all taken subcodes
> could go into hw/s390x/s390-virtio-hcall.h either via include or
> directly.

For virtio-ccw, we're going the way via the standard headers (as for
other virtio things). The last used subcode for the old virtio
transport is already in this file (in s390-next).

We currently have Documentation/virtual/kvm/s390-diag.txt in the
kernel. Not sure whether that would be a good place to document the
testdev subcode.

> 
> I'm not really happy with the side effects of moving it to hw/misc, which
> ain't s390x specific. 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.

The pci and isa testdevs seem to be included on all platforms that
support isa/pci (except for the pci testdev on s390x). They do have
their own config symbols, so they can be manually disabled should it be
desired. This seems like a good way to go for us as well.

> 
> I think, I have the most problematic places marked with a  TODO
> comment in the code.

I'll save looking at the code for another day.

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

[Dong Jia Shi]

* Halil Pasic <pasic@linux.vnet.ibm.com> [2017-11-08 17:54:20 +0100]:

Hi Halil,

> Add a fake device meant for testing the correctness of our css emulation.
> 
> What we currently have is writing a Fibonacci sequence of uint32_t to the
> device via ccw write. The write is going to fail if it ain't a Fibonacci
> and indicate a device exception in scsw together with the proper residual
> count. With this we can do basic verification of data integrity.
> 
> Of course lot's of invalid inputs (besides basic data processing) can be
> tested with that as well.
> 
> We also have a no-op mode where the device just tells all-good! This
> could be useful for fuzzing.
> 
> Usage:
> 1) fire up a qemu with something like -device ccw-testdev,devno=fe.0.0001
>    on the command line
> 2) exercise the device from the guest
> 
> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
> ---
> 
> Introduction
> ------------
> 
> While discussing v1 we (more or less) decided a test device for ccw is a
> good idea. This is an RFC because there are some unresolved technical
> questions I would like to discuss.
> 
Regarding to test coverage, this mainly covers the cds_read. What we
want would be surely more than this. So to extend the coverage, we need
to design more modes (aka, test cases), right?

If nobody disagrees with the basic idea of this series, I can start a
review then. ;)

[...]

-- 
Dong Jia Shi

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

[Halil Pasic]

On 11/23/2017 09:20 AM, Dong Jia Shi wrote:
> * Halil Pasic <pasic@linux.vnet.ibm.com> [2017-11-08 17:54:20 +0100]:
> 
> Hi Halil,
> 
>> Add a fake device meant for testing the correctness of our css emulation.
>>
>> What we currently have is writing a Fibonacci sequence of uint32_t to the
>> device via ccw write. The write is going to fail if it ain't a Fibonacci
>> and indicate a device exception in scsw together with the proper residual
>> count. With this we can do basic verification of data integrity.
>>
>> Of course lot's of invalid inputs (besides basic data processing) can be
>> tested with that as well.
>>
>> We also have a no-op mode where the device just tells all-good! This
>> could be useful for fuzzing.
>>
>> Usage:
>> 1) fire up a qemu with something like -device ccw-testdev,devno=fe.0.0001
>>    on the command line
>> 2) exercise the device from the guest
>>
>> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
>> ---
>>
>> Introduction
>> ------------
>>
>> While discussing v1 we (more or less) decided a test device for ccw is a
>> good idea. This is an RFC because there are some unresolved technical
>> questions I would like to discuss.
>>
> Regarding to test coverage, this mainly covers the cds_read. 

This series has only a linux module with some texts and is more
of a PoC than the way it should be done. I think I wrote somewhere
about this.

The ccw-testdev being mainly about cds_read is not correct. See
the TIC must have zero count test in patch 3. The cds things only
become relevant if data matters for the test. The ccw-testdev is
about making it possible to check how does the channel subsystem respond
to certain stimuli without actually having to care about a real device.

> What we
> want would be surely more than this. So to extend the coverage, we need
> to design more modes (aka, test cases), right?

Modes and testcases are not the same. See patches 2 and 3. Modes are
mostly for being extensible in a sense of expect the unexpected. I don't
have too may ideas for modes (one more I can think of could be some tee
like proxy for doing whatever kind of assertions/analysis on what some
driver does).  In the previous round we said we want this extensible, and
it make sense.


> 
> If nobody disagrees with the basic idea of this series, I can start a
> review then. ;)
> 
> [...]
> 

Re: [Qemu-devel] [RFC PATCH v2 0/3] tests for CCW IDA

[Halil Pasic]

On 11/22/2017 03:19 PM, Halil Pasic wrote:
> ping
> 

ping^2

> On 11/08/2017 05:54 PM, Halil Pasic wrote:
>> I've keept the title althogh the scope shifted a bit: it's
>> more about introducing ccw-testdev than about IDA. The goal
>> is to facilitate testing the virtual channel subsystem
>> implementation, and the ccw interpretation.
>>
>> The first patch is the interesting one. See it's cover letter
>> for details. The RFC is about discussing some technical issues
>> with this patch.
>>
>> The other two patches are an out of source kernel module which
>> is basically only there so you can try out the first patch. The
>> tests there should probably be ported to something else. I don't
>> know what: maybe kvm-unit-tests, maybe qtest+libqos, or maybe some
>> bios based test image. We still have to figure out that. 
>>
>> Halil Pasic (3):
>>   s390x/ccs: add ccw-testdev emulated device
>>   ccw-tester: a tester device for ccw I/O
>>   ccw-tester: add tic test
>>
>>  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
>>
> 
> 

Re: [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

Re: [Qemu-devel] [qemu-s390x] [RFC PATCH v2 0/3] tests for CCW IDA

[Thomas Huth]

On 08.11.2017 17:54, Halil Pasic wrote:
> I've keept the title althogh the scope shifted a bit: it's
> more about introducing ccw-testdev than about IDA. The goal
> is to facilitate testing the virtual channel subsystem
> implementation, and the ccw interpretation.
> 
> The first patch is the interesting one. See it's cover letter
> for details. The RFC is about discussing some technical issues
> with this patch.
> 
> The other two patches are an out of source kernel module which
> is basically only there so you can try out the first patch. The
> tests there should probably be ported to something else. I don't
> know what: maybe kvm-unit-tests, maybe qtest+libqos, or maybe some
> bios based test image. We still have to figure out that. 

I think both, kvm-unit-tests or qtest+libqos would be good candidates.
Please don't invent a new bios base test image, since kvm-unit-tests
should be very similar already and we really don't need to duplicate
work here.

Anyway, you'd need to add some CSS infracture there first (in both
kvm-unit-tests and the qtest environments), so it's likely a similar
amount of work. qtest has the advantage that it gets checked
automatically during "make check" each time, so I'd have a weak
preference for that one.

 Thomas

Re: [Qemu-devel] [qemu-s390x] [RFC PATCH v2 0/3] tests for CCW IDA

[Thomas Huth]

On 07.12.2017 07:38, Thomas Huth wrote:
> On 08.11.2017 17:54, Halil Pasic wrote:
>> I've keept the title althogh the scope shifted a bit: it's
>> more about introducing ccw-testdev than about IDA. The goal
>> is to facilitate testing the virtual channel subsystem
>> implementation, and the ccw interpretation.
>>
>> The first patch is the interesting one. See it's cover letter
>> for details. The RFC is about discussing some technical issues
>> with this patch.
>>
>> The other two patches are an out of source kernel module which
>> is basically only there so you can try out the first patch. The
>> tests there should probably be ported to something else. I don't
>> know what: maybe kvm-unit-tests, maybe qtest+libqos, or maybe some
>> bios based test image. We still have to figure out that. 
> 
> I think both, kvm-unit-tests or qtest+libqos would be good candidates.
> Please don't invent a new bios base test image, since kvm-unit-tests
> should be very similar already and we really don't need to duplicate
> work here.
> 
> Anyway, you'd need to add some CSS infracture there first (in both
> kvm-unit-tests and the qtest environments), so it's likely a similar
> amount of work. qtest has the advantage that it gets checked
> automatically during "make check" each time, so I'd have a weak
> preference for that one.

Another thought: I'd also like to see the more complex virtio device
qtests enabled for virtio-ccw one day (e.g. tests/virtio-blk-test.c), so
I think we sooner or later should have some CSS infrastructure in the
qtests anyway ==> May I suggest that you have a try with the qtest approach?

 Thanks,
  Thomas

Re: [Qemu-devel] [qemu-s390x] [RFC PATCH v2 0/3] tests for CCW IDA

[Cornelia Huck]

On Thu, 7 Dec 2017 10:01:35 +0100
Thomas Huth <thuth@redhat.com> wrote:

> On 07.12.2017 07:38, Thomas Huth wrote:
> > On 08.11.2017 17:54, Halil Pasic wrote:  
> >> I've keept the title althogh the scope shifted a bit: it's
> >> more about introducing ccw-testdev than about IDA. The goal
> >> is to facilitate testing the virtual channel subsystem
> >> implementation, and the ccw interpretation.
> >>
> >> The first patch is the interesting one. See it's cover letter
> >> for details. The RFC is about discussing some technical issues
> >> with this patch.
> >>
> >> The other two patches are an out of source kernel module which
> >> is basically only there so you can try out the first patch. The
> >> tests there should probably be ported to something else. I don't
> >> know what: maybe kvm-unit-tests, maybe qtest+libqos, or maybe some
> >> bios based test image. We still have to figure out that.   
> > 
> > I think both, kvm-unit-tests or qtest+libqos would be good candidates.
> > Please don't invent a new bios base test image, since kvm-unit-tests
> > should be very similar already and we really don't need to duplicate
> > work here.
> > 
> > Anyway, you'd need to add some CSS infracture there first (in both
> > kvm-unit-tests and the qtest environments), so it's likely a similar
> > amount of work. qtest has the advantage that it gets checked
> > automatically during "make check" each time, so I'd have a weak
> > preference for that one.  
> 
> Another thought: I'd also like to see the more complex virtio device
> qtests enabled for virtio-ccw one day (e.g. tests/virtio-blk-test.c), so
> I think we sooner or later should have some CSS infrastructure in the
> qtests anyway ==> May I suggest that you have a try with the qtest approach?

Agreed, this would be helpful to get more ccw coverage in general.

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

[Cornelia Huck]

On Thu, 7 Dec 2017 07:33:19 +0100
Thomas Huth <thuth@redhat.com> wrote:

> On 08.11.2017 17:54, Halil Pasic wrote:

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

I like the idea of an error mode.

Related: Should the device have a mechanism to report the supported
modes?

> 
> > +    }
> > +}

(...)

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

I'm not sure that the in-band variant with its magic buffer value is
superior to this version using a dedicated hypercall.

> 
> > +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;

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

[Halil Pasic]

On 11/08/2017 05:54 PM, Halil Pasic wrote:
> +/* TODO This is the out-of-band variant. We may want to get rid of it */
> +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;
> +    }

Should probably do validation on op_mode here.

> +    dev->op_mode = op_mode;
> +    sch->ccw_cb = get_ccw_cb(dev->op_mode);
> +    return 0;
> +}
> +

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

[Halil Pasic]

On 12/07/2017 12:59 PM, Cornelia Huck wrote:
> On Thu, 7 Dec 2017 07:33:19 +0100
> Thomas Huth <thuth@redhat.com> wrote:
> 
>> On 08.11.2017 17:54, Halil Pasic wrote:
> 
>>> +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?
> 
> I like the idea of an error mode.

What would be the benefit of the error mode? The idea is that
the tester in the guest has a certain set of tests implemented
each requiring certain behavior from the device. This behavior
is represented by the mode.

If the device does not support the mode, the set of tests can't
be executed meaningfully. The only option is either ignore them
(and preferably report them as ignored), or fail them (not soo
good in my opinion).

The in guest tester should simply iterate over it test sets
and try to select the mode the test set (or suite in other
terminology) requires. If selecting the mode fails, than
means you are working with an old ccw-testdev.

> 
> Related: Should the device have a mechanism to report the supported
> modes?
> 

I don't see the value. See above. I think the set mode operation
reporting failure is sufficient.

But if you have something in mind, please do tell. I'm open.

>>
>>> +    }
>>> +}
> 
> (...)
> 
>>> +/* 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.
> 
> I'm not sure that the in-band variant with its magic buffer value is
> superior to this version using a dedicated hypercall.
> 

I have a similar todo comment for the in-band variant. It is basically
up to us to decide which one do we want to keep. It's been a while since
I've looked at this, but AFAIR the out-of-band appears cleaner.

>>
>>> +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;
> 

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

[Cornelia Huck]

On Thu, 7 Dec 2017 13:38:22 +0100
Halil Pasic <pasic@linux.vnet.ibm.com> wrote:

> On 12/07/2017 12:59 PM, Cornelia Huck wrote:
> > On Thu, 7 Dec 2017 07:33:19 +0100
> > Thomas Huth <thuth@redhat.com> wrote:
> >   
> >> On 08.11.2017 17:54, Halil Pasic wrote:  
> >   
> >>> +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?  
> > 
> > I like the idea of an error mode.  
> 
> What would be the benefit of the error mode? The idea is that
> the tester in the guest has a certain set of tests implemented
> each requiring certain behavior from the device. This behavior
> is represented by the mode.
> 
> If the device does not support the mode, the set of tests can't
> be executed meaningfully. The only option is either ignore them
> (and preferably report them as ignored), or fail them (not soo
> good in my opinion).

Failing it sounds superior to me: You really want to know if something
does not work as expected.

> 
> The in guest tester should simply iterate over it test sets
> and try to select the mode the test set (or suite in other
> terminology) requires. If selecting the mode fails, than
> means you are working with an old ccw-testdev.
> 
> > 
> > Related: Should the device have a mechanism to report the supported
> > modes?
> >   
> 
> I don't see the value. See above. I think the set mode operation
> reporting failure is sufficient.
> 
> But if you have something in mind, please do tell. I'm open.

If we keep guest/host in lockstep, we probably don't need this. But if
not, self-reporting looks like a reasonable feature as it gives more
flexibility.

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

[Halil Pasic]

On 12/07/2017 01:57 PM, Cornelia Huck wrote:
> On Thu, 7 Dec 2017 13:38:22 +0100
> Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
> 
>> On 12/07/2017 12:59 PM, Cornelia Huck wrote:
>>> On Thu, 7 Dec 2017 07:33:19 +0100
>>> Thomas Huth <thuth@redhat.com> wrote:
>>>   
>>>> On 08.11.2017 17:54, Halil Pasic wrote:  
>>>   
>>>>> +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?  
>>>
>>> I like the idea of an error mode.  
>>
>> What would be the benefit of the error mode? The idea is that
>> the tester in the guest has a certain set of tests implemented
>> each requiring certain behavior from the device. This behavior
>> is represented by the mode.
>>
>> If the device does not support the mode, the set of tests can't
>> be executed meaningfully. The only option is either ignore them
>> (and preferably report them as ignored), or fail them (not soo
>> good in my opinion).
> 
> Failing it sounds superior to me: You really want to know if something
> does not work as expected.
> 

When doing unit tests it's usual to differentiate between:
* success, which ideally should give you guarantees about the unit
not misbehaving in production if client code respects the contract
* failure, which ideally should pretty much narrow down what
went wrong
* ignored, either due to user explicitly disabled a test, or
a precondition for executing the was not met.

If you think traffic light its green, red and yellow respectively.

For instance qemu-iotests also skips tests where the environment
does not provide a preconditions necessary to execute it.

Of course a quality gate for a release should really be the suite
executed successfully (green), and not some tests were skipped.

A quality gate for let's day sending a series to the list could
also be defined less rigorous. 

>>
>> The in guest tester should simply iterate over it test sets
>> and try to select the mode the test set (or suite in other
>> terminology) requires. If selecting the mode fails, than
>> means you are working with an old ccw-testdev.
>>
>>>
>>> Related: Should the device have a mechanism to report the supported
>>> modes?
>>>   
>>
>> I don't see the value. See above. I think the set mode operation
>> reporting failure is sufficient.
>>
>> But if you have something in mind, please do tell. I'm open.
> 
> If we keep guest/host in lockstep, we probably don't need this. But if
> not, self-reporting looks like a reasonable feature as it gives more
> flexibility.
> 
I would prefer adding this should the need arise instead of
speculatively. Would that be fine for you.

BTW if interface stability is a concern maybe making the device
experimental (at least for a while) is a good idea.

Regards,
Halil

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

[Cornelia Huck]

On Thu, 7 Dec 2017 17:35:26 +0100
Halil Pasic <pasic@linux.vnet.ibm.com> wrote:

> On 12/07/2017 01:57 PM, Cornelia Huck wrote:
> > On Thu, 7 Dec 2017 13:38:22 +0100
> > Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
> >   
> >> On 12/07/2017 12:59 PM, Cornelia Huck wrote:  
> >>> On Thu, 7 Dec 2017 07:33:19 +0100
> >>> Thomas Huth <thuth@redhat.com> wrote:
> >>>     
> >>>> On 08.11.2017 17:54, Halil Pasic wrote:    
> >>>     
> >>>>> +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?    
> >>>
> >>> I like the idea of an error mode.    
> >>
> >> What would be the benefit of the error mode? The idea is that
> >> the tester in the guest has a certain set of tests implemented
> >> each requiring certain behavior from the device. This behavior
> >> is represented by the mode.
> >>
> >> If the device does not support the mode, the set of tests can't
> >> be executed meaningfully. The only option is either ignore them
> >> (and preferably report them as ignored), or fail them (not soo
> >> good in my opinion).  
> > 
> > Failing it sounds superior to me: You really want to know if something
> > does not work as expected.
> >   
> 
> When doing unit tests it's usual to differentiate between:
> * success, which ideally should give you guarantees about the unit
> not misbehaving in production if client code respects the contract
> * failure, which ideally should pretty much narrow down what
> went wrong
> * ignored, either due to user explicitly disabled a test, or
> a precondition for executing the was not met.
> 
> If you think traffic light its green, red and yellow respectively.
> 
> For instance qemu-iotests also skips tests where the environment
> does not provide a preconditions necessary to execute it.
> 
> Of course a quality gate for a release should really be the suite
> executed successfully (green), and not some tests were skipped.
> 
> A quality gate for let's day sending a series to the list could
> also be defined less rigorous. 
> 
> >>
> >> The in guest tester should simply iterate over it test sets
> >> and try to select the mode the test set (or suite in other
> >> terminology) requires. If selecting the mode fails, than
> >> means you are working with an old ccw-testdev.
> >>  
> >>>
> >>> Related: Should the device have a mechanism to report the supported
> >>> modes?
> >>>     
> >>
> >> I don't see the value. See above. I think the set mode operation
> >> reporting failure is sufficient.
> >>
> >> But if you have something in mind, please do tell. I'm open.  
> > 
> > If we keep guest/host in lockstep, we probably don't need this. But if
> > not, self-reporting looks like a reasonable feature as it gives more
> > flexibility.
> >   
> I would prefer adding this should the need arise instead of
> speculatively. Would that be fine for you.
> 
> BTW if interface stability is a concern maybe making the device
> experimental (at least for a while) is a good idea.

Well, if we used qtests, we would be fine without an error state, as
both sides have the same features.

Re: [Qemu-devel] [qemu-s390x] [RFC PATCH v2 0/3] tests for CCW IDA

[Halil Pasic]

On 12/07/2017 12:53 PM, Cornelia Huck wrote:
> On Thu, 7 Dec 2017 10:01:35 +0100
> Thomas Huth <thuth@redhat.com> wrote:
> 
>> On 07.12.2017 07:38, Thomas Huth wrote:
>>> On 08.11.2017 17:54, Halil Pasic wrote:  
>>>> I've keept the title althogh the scope shifted a bit: it's
>>>> more about introducing ccw-testdev than about IDA. The goal
>>>> is to facilitate testing the virtual channel subsystem
>>>> implementation, and the ccw interpretation.
>>>>
>>>> The first patch is the interesting one. See it's cover letter
>>>> for details. The RFC is about discussing some technical issues
>>>> with this patch.
>>>>
>>>> The other two patches are an out of source kernel module which
>>>> is basically only there so you can try out the first patch. The
>>>> tests there should probably be ported to something else. I don't
>>>> know what: maybe kvm-unit-tests, maybe qtest+libqos, or maybe some
>>>> bios based test image. We still have to figure out that.   
>>>
>>> I think both, kvm-unit-tests or qtest+libqos would be good candidates.
>>> Please don't invent a new bios base test image, since kvm-unit-tests
>>> should be very similar already and we really don't need to duplicate
>>> work here.
>>>
>>> Anyway, you'd need to add some CSS infracture there first (in both
>>> kvm-unit-tests and the qtest environments), so it's likely a similar
>>> amount of work. qtest has the advantage that it gets checked
>>> automatically during "make check" each time, so I'd have a weak
>>> preference for that one.  
>>
>> Another thought: I'd also like to see the more complex virtio device
>> qtests enabled for virtio-ccw one day (e.g. tests/virtio-blk-test.c), so
>> I think we sooner or later should have some CSS infrastructure in the
>> qtests anyway ==> May I suggest that you have a try with the qtest approach?
> 
> Agreed, this would be helpful to get more ccw coverage in general.
> 

Yeah qtest+libqos does seem like the most likely candidate. We
are likely to go down this path. I say we, because it seems likely
that the guest counterpart with the unit test suite is going to
be done by somebody having more time to invest into this.

Regards,
Halil