[Qemu-devel] [PATCH 0/2] tests for CCW IDA

[Qemu-devel] [PATCH 0/2] tests for CCW IDA

[Halil Pasic]

Because there is not matching (emulated) QEMU device for the Linux
drivers using IDA, testing IDA requires extra effort. 

The last patch of this  series is a QEMU patch which introduces an
emulated CCW device,  which does support IDA. This patch depends on my
IDA work and is intended to be used with the following patch set:
https://lists.nongnu.org/archive/html/qemu-devel/2017-09/msg03434.html

The second patch is an out-of-tree Linux kernel module, meant to exercise
the QEMU device introduced by the first patch. The idea is to apply this
patch to an empty git repository.

Both of the patches used to be included in my IDA series, but I've split
them out on maintainer request.

Halil Pasic (1):
  s390x/ccs: add ccw-tester emulated device

 hw/s390x/Makefile.objs |   1 +
 hw/s390x/ccw-tester.c  | 179 +++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 180 insertions(+)
 create mode 100644 hw/s390x/ccw-tester.c

-- 
2.13.5

[Qemu-devel] [PATCH 1/2] s390x/ccs: add ccw-tester 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.

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

Usage:
1) fire up a qemu with something like -device ccw-tester,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>
---

Depends on the series 'add CCW indirect data access support'

---
 hw/s390x/Makefile.objs |   1 +
 hw/s390x/ccw-tester.c  | 179 +++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 180 insertions(+)
 create mode 100644 hw/s390x/ccw-tester.c

diff --git a/hw/s390x/Makefile.objs b/hw/s390x/Makefile.objs
index 7ee19d3abc..28eb58a3cb 100644
--- a/hw/s390x/Makefile.objs
+++ b/hw/s390x/Makefile.objs
@@ -18,3 +18,4 @@ obj-y += s390-stattrib.o
 obj-$(CONFIG_KVM) += s390-skeys-kvm.o
 obj-$(CONFIG_KVM) += s390-stattrib-kvm.o
 obj-y += s390-ccw.o
+obj-y += ccw-tester.o
diff --git a/hw/s390x/ccw-tester.c b/hw/s390x/ccw-tester.c
new file mode 100644
index 0000000000..c8017818c4
--- /dev/null
+++ b/hw/s390x/ccw-tester.c
@@ -0,0 +1,179 @@
+#include "qemu/osdep.h"
+#include "qapi/error.h"
+#include "qemu/module.h"
+#include "cpu.h"
+#include "hw/s390x/css.h"
+#include "hw/s390x/css-bridge.h"
+#include "hw/s390x/3270-ccw.h"
+#include "exec/address-spaces.h"
+#include <error.h>
+
+typedef struct CcwTesterDevice {
+    CcwDevice parent_obj;
+    uint16_t cu_type;
+    uint8_t chpid_type;
+    struct {
+        uint32_t ring[4];
+        unsigned int next;
+    } fib;
+} CcwTesterDevice;
+
+
+typedef struct CcwTesterClass {
+    CCWDeviceClass parent_class;
+    DeviceRealize parent_realize;
+} CcwTesterClass;
+
+#define TYPE_CCW_TESTER "ccw-tester"
+
+#define CCW_TESTER(obj) \
+     OBJECT_CHECK(CcwTesterDevice, (obj), TYPE_CCW_TESTER)
+#define CCW_TESTER_CLASS(klass) \
+     OBJECT_CLASS_CHECK(CcwTesterClass, (klass), TYPE_CCW_TESTER)
+#define CCW_TESTER_GET_CLASS(obj) \
+     OBJECT_GET_CLASS(CcwTesterClass, (obj), TYPE_CCW_TESTER)
+
+#define CCW_CMD_READ 0x01U
+#define CCW_CMD_WRITE 0x02U
+
+static unsigned int abs_to_ring(unsigned int i)
+{
+    return i & 0x3;
+}
+
+static int  ccw_tester_write_fib(SubchDev *sch, CCW1 ccw)
+{
+    CcwTesterDevice *d = sch->driver_data;
+    bool is_fib = true;
+    uint32_t sum;
+    int ret = 0;
+
+    ccw_dstream_init(&sch->cds, &ccw, &sch->orb);
+    d->fib.next = 0;
+    while (ccw_dstream_avail(&sch->cds) > 0) {
+        ret = ccw_dstream_read(&sch->cds,
+                               d->fib.ring[abs_to_ring(d->fib.next)]);
+        if (ret) {
+            error(0, -ret, "fib");
+            break;
+        }
+        if (d->fib.next > 2) {
+            sum = (d->fib.ring[abs_to_ring(d->fib.next - 1)]
+                  + d->fib.ring[abs_to_ring(d->fib.next - 2)]);
+            is_fib = sum ==  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_tester_ccw_cb_impl(SubchDev *sch, CCW1 ccw)
+{
+    switch (ccw.cmd_code) {
+    case CCW_CMD_READ:
+        break;
+    case CCW_CMD_WRITE:
+        return ccw_tester_write_fib(sch, ccw);
+    default:
+        return -EINVAL;
+    }
+    return 0;
+}
+
+static void ccw_tester_realize(DeviceState *ds, Error **errp)
+{
+    uint16_t chpid;
+    CcwTesterDevice *dev = CCW_TESTER(ds);
+    CcwTesterClass *ctc = CCW_TESTER_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 = ccw_tester_ccw_cb_impl;
+    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_tester_properties[] = {
+    DEFINE_PROP_UINT16("cu_type", CcwTesterDevice, cu_type,
+                        0x3831),
+    DEFINE_PROP_UINT8("chpid_type", CcwTesterDevice, chpid_type,
+                       0x98),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
+static void ccw_tester_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+    CcwTesterClass *ctc = CCW_TESTER_CLASS(klass);
+
+    dc->props = ccw_tester_properties;
+    dc->bus_type = TYPE_VIRTUAL_CSS_BUS;
+    ctc->parent_realize = dc->realize;
+    dc->realize = ccw_tester_realize;
+    dc->hotpluggable = false;
+}
+
+static const TypeInfo ccw_tester_info = {
+    .name = TYPE_CCW_TESTER,
+    .parent = TYPE_CCW_DEVICE,
+    .instance_size = sizeof(CcwTesterDevice),
+    .class_init = ccw_tester_class_init,
+    .class_size = sizeof(CcwTesterClass),
+};
+
+static void ccw_tester_register(void)
+{
+    type_register_static(&ccw_tester_info);
+}
+
+type_init(ccw_tester_register)
-- 
2.13.5

[Qemu-devel] [PATCH 2/2 NOT QEMU] 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_tester.c | 420 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 438 insertions(+)
 create mode 100644 .gitignore
 create mode 100644 Makefile
 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_tester.c b/ccw_tester.c
new file mode 100644
index 0000000..320486a
--- /dev/null
+++ b/ccw_tester.c
@@ -0,0 +1,420 @@
+#include <linux/kernel_stat.h>
+#include <linux/init.h>
+#include <linux/bootmem.h>
+#include <linux/err.h>
+#include <linux/slab.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>
+
+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 = 0x3831;
+module_param(cu_type, ushort, 0000);
+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 w_fib_w_setup(struct ccw_test_work *w)
+{
+	const int test_fib_length = 32;
+	u32 *test_fib;
+	int i;
+
+	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 = 0x02;
+	w->ccw->count = sizeof(*test_fib) * test_fib_length;
+	w->ccw->cda = (__u32)(unsigned long) test_fib;
+}
+
+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 void basic_teardown(struct ccw_test_work *w)
+{
+	kfree(w->private);
+	w->private = NULL;
+	if (w->ret)
+		printk(KERN_WARNING "w_fib_w_teardown ret = %d\n", w->ret);
+}
+
+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 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_w_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 w_fib_idal_setup(struct ccw_test_work *w)
+{
+	struct idal_buffer *ib = NULL;
+	u32 n, n_1 = 2, n_2 = 1;
+	int i = 0;
+
+	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 = 0x02;
+}
+
+static void idal_teardown(struct ccw_test_work *w)
+{
+	if (w->private) {
+		idal_buffer_free(w->private);
+		w->private = NULL;
+	}
+	if (w->ret)
+		printk(KERN_WARNING "w_fib_w_teardown ret = %d\n", w->ret);
+}
+
+static void w_fib_do_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,
+			w_fib_idal_setup, w_fib_do_idal_test, 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

Re: [Qemu-devel] [PATCH 1/2] s390x/ccs: add ccw-tester emulated device

[Cornelia Huck]

On Wed, 13 Sep 2017 15:27:51 +0200
Halil Pasic <pasic@linux.vnet.ibm.com> wrote:

> 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.
> 
> Of course lot's of invalid inputs (besides basic data processing) can be
> tested with that as well.
> 
> Usage:
> 1) fire up a qemu with something like -device ccw-tester,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>
> ---
> 
> Depends on the series 'add CCW indirect data access support'
> 
> ---
>  hw/s390x/Makefile.objs |   1 +
>  hw/s390x/ccw-tester.c  | 179 +++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 180 insertions(+)
>  create mode 100644 hw/s390x/ccw-tester.c
> 

> +static int  ccw_tester_write_fib(SubchDev *sch, CCW1 ccw)
> +{
> +    CcwTesterDevice *d = sch->driver_data;
> +    bool is_fib = true;
> +    uint32_t sum;
> +    int ret = 0;
> +
> +    ccw_dstream_init(&sch->cds, &ccw, &sch->orb);
> +    d->fib.next = 0;
> +    while (ccw_dstream_avail(&sch->cds) > 0) {
> +        ret = ccw_dstream_read(&sch->cds,
> +                               d->fib.ring[abs_to_ring(d->fib.next)]);
> +        if (ret) {
> +            error(0, -ret, "fib");
> +            break;
> +        }
> +        if (d->fib.next > 2) {
> +            sum = (d->fib.ring[abs_to_ring(d->fib.next - 1)]
> +                  + d->fib.ring[abs_to_ring(d->fib.next - 2)]);
> +            is_fib = sum ==  d->fib.ring[abs_to_ring(d->fib.next)];

This is not endian-safe (noticed while testing on my laptop). Trivial
to fix:

diff --git a/hw/s390x/ccw-tester.c b/hw/s390x/ccw-tester.c
index c8017818c4..a425daaa34 100644
--- a/hw/s390x/ccw-tester.c
+++ b/hw/s390x/ccw-tester.c
@@ -58,9 +58,9 @@ static int  ccw_tester_write_fib(SubchDev *sch, CCW1 ccw)
             break;
         }
         if (d->fib.next > 2) {
-            sum = (d->fib.ring[abs_to_ring(d->fib.next - 1)]
-                  + d->fib.ring[abs_to_ring(d->fib.next - 2)]);
-            is_fib = sum ==  d->fib.ring[abs_to_ring(d->fib.next)];
+            sum = be32_to_cpu(d->fib.ring[abs_to_ring(d->fib.next - 1)])
+                + be32_to_cpu(d->fib.ring[abs_to_ring(d->fib.next - 2)]);
+            is_fib = sum == be32_to_cpu(d->fib.ring[abs_to_ring(d->fib.next)]);
             if (!is_fib) {
                 break;
             }

> +            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 Property ccw_tester_properties[] = {
> +    DEFINE_PROP_UINT16("cu_type", CcwTesterDevice, cu_type,
> +                        0x3831),

0x4711 would be nice :)

If we want to follow up on that testdev idea (and I think we should),
it might make sense to have a proper type reserve to prevent accidental
clashes.

(Or is there already something reserved for "hypervisor use" or
whatever?)

> +    DEFINE_PROP_UINT8("chpid_type", CcwTesterDevice, chpid_type,
> +                       0x98),
> +    DEFINE_PROP_END_OF_LIST(),
> +};

IIUC, pci-testdev provides some unit tests to testers (like kvm-tests)
itself. This might be an idea to follow up on for ccw as well.

There's quite some potential in this. We may want to make this a
permanent addition.

Re: [Qemu-devel] [PATCH 1/2] s390x/ccs: add ccw-tester emulated device

[Halil Pasic]

On 09/14/2017 04:26 PM, Cornelia Huck wrote:
> On Wed, 13 Sep 2017 15:27:51 +0200
> Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
> 
>> 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.
>>
>> Of course lot's of invalid inputs (besides basic data processing) can be
>> tested with that as well.
>>
>> Usage:
>> 1) fire up a qemu with something like -device ccw-tester,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>
>> ---
>>
>> Depends on the series 'add CCW indirect data access support'
>>
>> ---
>>  hw/s390x/Makefile.objs |   1 +
>>  hw/s390x/ccw-tester.c  | 179 +++++++++++++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 180 insertions(+)
>>  create mode 100644 hw/s390x/ccw-tester.c
>>
> 
>> +static int  ccw_tester_write_fib(SubchDev *sch, CCW1 ccw)
>> +{
>> +    CcwTesterDevice *d = sch->driver_data;
>> +    bool is_fib = true;
>> +    uint32_t sum;
>> +    int ret = 0;
>> +
>> +    ccw_dstream_init(&sch->cds, &ccw, &sch->orb);
>> +    d->fib.next = 0;
>> +    while (ccw_dstream_avail(&sch->cds) > 0) {
>> +        ret = ccw_dstream_read(&sch->cds,
>> +                               d->fib.ring[abs_to_ring(d->fib.next)]);
>> +        if (ret) {
>> +            error(0, -ret, "fib");
>> +            break;
>> +        }
>> +        if (d->fib.next > 2) {
>> +            sum = (d->fib.ring[abs_to_ring(d->fib.next - 1)]
>> +                  + d->fib.ring[abs_to_ring(d->fib.next - 2)]);
>> +            is_fib = sum ==  d->fib.ring[abs_to_ring(d->fib.next)];
> 
> This is not endian-safe (noticed while testing on my laptop). Trivial
> to fix:
> 
> diff --git a/hw/s390x/ccw-tester.c b/hw/s390x/ccw-tester.c
> index c8017818c4..a425daaa34 100644
> --- a/hw/s390x/ccw-tester.c
> +++ b/hw/s390x/ccw-tester.c
> @@ -58,9 +58,9 @@ static int  ccw_tester_write_fib(SubchDev *sch, CCW1 ccw)
>              break;
>          }
>          if (d->fib.next > 2) {
> -            sum = (d->fib.ring[abs_to_ring(d->fib.next - 1)]
> -                  + d->fib.ring[abs_to_ring(d->fib.next - 2)]);
> -            is_fib = sum ==  d->fib.ring[abs_to_ring(d->fib.next)];
> +            sum = be32_to_cpu(d->fib.ring[abs_to_ring(d->fib.next - 1)])
> +                + be32_to_cpu(d->fib.ring[abs_to_ring(d->fib.next - 2)]);
> +            is_fib = sum == be32_to_cpu(d->fib.ring[abs_to_ring(d->fib.next)]);
>              if (!is_fib) {
>                  break;
>              }
> 

Nod. 

>> +            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 Property ccw_tester_properties[] = {
>> +    DEFINE_PROP_UINT16("cu_type", CcwTesterDevice, cu_type,
>> +                        0x3831),
> 
> 0x4711 would be nice :)

I don't understand the joke/pun/whatever if there is one,
but I'm fine with changing this too.

> 
> If we want to follow up on that testdev idea (and I think we should),
> it might make sense to have a proper type reserve to prevent accidental
> clashes.

I agree. Although I would still keep the cu_type configurable,
because it might make sense to test a particular 'real' driver
(and not a test driver like here). I haven't really thought
this through, but it was an idea I had while agonizing over
not having a proper type reserved.

I suppose you did something like that for virtio, or? I'm in dark
when it comes to the question what process do we/I have to go to
get a type,for example 0x4711, reserved.

> 
> (Or is there already something reserved for "hypervisor use" or
> whatever?)

Not that I know. I can't recall encountering a list of reserved
types. Honestly I've hoped to leverage your experience (again
because of virtio-ccw).

> 
>> +    DEFINE_PROP_UINT8("chpid_type", CcwTesterDevice, chpid_type,
>> +                       0x98),
>> +    DEFINE_PROP_END_OF_LIST(),
>> +};
> 
> IIUC, pci-testdev provides some unit tests to testers (like kvm-tests)
> itself. This might be an idea to follow up on for ccw as well.
> 

I've just had a first look at pci-testdev, and it does appear to be a similar
concept. 

> There's quite some potential in this. We may want to make this a
> permanent addition.
> 

I'm happy to contribute! I'm not sure how shall we proceed though.
Maybe with making a todo list?

Re: [Qemu-devel] [PATCH 1/2] s390x/ccs: add ccw-tester emulated device

[Cornelia Huck]

On Thu, 14 Sep 2017 18:50:29 +0200
Halil Pasic <pasic@linux.vnet.ibm.com> wrote:

> On 09/14/2017 04:26 PM, Cornelia Huck wrote:
> > On Wed, 13 Sep 2017 15:27:51 +0200
> > Halil Pasic <pasic@linux.vnet.ibm.com> wrote:

> >> +static Property ccw_tester_properties[] = {
> >> +    DEFINE_PROP_UINT16("cu_type", CcwTesterDevice, cu_type,
> >> +                        0x3831),  
> > 
> > 0x4711 would be nice :)  
> 
> I don't understand the joke/pun/whatever if there is one,
> but I'm fine with changing this too.

https://en.wikipedia.org/wiki/4711

That's my default if I need a four-digit number :)

> 
> > 
> > If we want to follow up on that testdev idea (and I think we should),
> > it might make sense to have a proper type reserve to prevent accidental
> > clashes.  
> 
> I agree. Although I would still keep the cu_type configurable,
> because it might make sense to test a particular 'real' driver
> (and not a test driver like here). I haven't really thought
> this through, but it was an idea I had while agonizing over
> not having a proper type reserved.
> 
> I suppose you did something like that for virtio, or? I'm in dark
> when it comes to the question what process do we/I have to go to
> get a type,for example 0x4711, reserved.

4711 is more a joke :) It might be worth trying the same channels as
for virtio-ccw.

Christian should know more about that.

> 
> > 
> > (Or is there already something reserved for "hypervisor use" or
> > whatever?)  
> 
> Not that I know. I can't recall encountering a list of reserved
> types. Honestly I've hoped to leverage your experience (again
> because of virtio-ccw).

My thought was that the z/VM folks already might have something
(type-wise) that we could use as well. There's a surprising amount of
values that are reserved for one use or the other. But obviously, I
can't find out about that.

> 
> >   
> >> +    DEFINE_PROP_UINT8("chpid_type", CcwTesterDevice, chpid_type,
> >> +                       0x98),

This might also need re-evaluation - we should not really need a new
chpid type.

> >> +    DEFINE_PROP_END_OF_LIST(),
> >> +};  
> > 
> > IIUC, pci-testdev provides some unit tests to testers (like kvm-tests)
> > itself. This might be an idea to follow up on for ccw as well.
> >   
> 
> I've just had a first look at pci-testdev, and it does appear to be a similar
> concept. 
> 
> > There's quite some potential in this. We may want to make this a
> > permanent addition.
> >   
> 
> I'm happy to contribute! I'm not sure how shall we proceed though.
> Maybe with making a todo list?

I think the first step would be to figure out the ids so we don't step
on anyone's toes. Then maybe refactor a bit so that other testers can
be added easily.

For ideas about things to be tested, maybe put a list into the wiki?

Re: [Qemu-devel] [PATCH 1/2] s390x/ccs: add ccw-tester emulated device

[Cornelia Huck]

On Fri, 15 Sep 2017 09:27:58 +0200
Cornelia Huck <cohuck@redhat.com> wrote:

> On Thu, 14 Sep 2017 18:50:29 +0200
> Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
> 
> > On 09/14/2017 04:26 PM, Cornelia Huck wrote:  

> > > There's quite some potential in this. We may want to make this a
> > > permanent addition.
> > >     
> > 
> > I'm happy to contribute! I'm not sure how shall we proceed though.
> > Maybe with making a todo list?  
> 
> I think the first step would be to figure out the ids so we don't step
> on anyone's toes. Then maybe refactor a bit so that other testers can
> be added easily.
> 
> For ideas about things to be tested, maybe put a list into the wiki?

OK, I've created https://wiki.qemu.org/Testing/CCWTestDevice - let me
know if you need an account.

Re: [Qemu-devel] [PATCH 1/2] s390x/ccs: add ccw-tester emulated device

[Christian Borntraeger]

On 09/15/2017 09:27 AM, Cornelia Huck wrote:
> On Thu, 14 Sep 2017 18:50:29 +0200
> Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
> 
>> On 09/14/2017 04:26 PM, Cornelia Huck wrote:
>>> On Wed, 13 Sep 2017 15:27:51 +0200
>>> Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
> 
>>>> +static Property ccw_tester_properties[] = {
>>>> +    DEFINE_PROP_UINT16("cu_type", CcwTesterDevice, cu_type,
>>>> +                        0x3831),  
>>>
>>> 0x4711 would be nice :)  
>>
>> I don't understand the joke/pun/whatever if there is one,
>> but I'm fine with changing this too.
> 
> https://en.wikipedia.org/wiki/4711
> 
> That's my default if I need a four-digit number :)
> 
>>
>>>
>>> If we want to follow up on that testdev idea (and I think we should),
>>> it might make sense to have a proper type reserve to prevent accidental
>>> clashes.  
>>
>> I agree. Although I would still keep the cu_type configurable,
>> because it might make sense to test a particular 'real' driver
>> (and not a test driver like here). I haven't really thought
>> this through, but it was an idea I had while agonizing over
>> not having a proper type reserved.
>>
>> I suppose you did something like that for virtio, or? I'm in dark
>> when it comes to the question what process do we/I have to go to
>> get a type,for example 0x4711, reserved.
> 
> 4711 is more a joke :) It might be worth trying the same channels as
> for virtio-ccw.
> 
> Christian should know more about that.

Getting a new number was very easy (because it is attached to a machine type
number). I I remember correctly, only numerical values are uses, so maybe
we can use ffff as there will never be such a real value?

Re: [Qemu-devel] [PATCH 1/2] s390x/ccs: add ccw-tester emulated device

[Cornelia Huck]

On Mon, 18 Sep 2017 10:30:32 +0200
Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> On 09/15/2017 09:27 AM, Cornelia Huck wrote:
> > On Thu, 14 Sep 2017 18:50:29 +0200
> > Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
> >   
> >> On 09/14/2017 04:26 PM, Cornelia Huck wrote:  
> >>> On Wed, 13 Sep 2017 15:27:51 +0200
> >>> Halil Pasic <pasic@linux.vnet.ibm.com> wrote:  
> >   
> >>>> +static Property ccw_tester_properties[] = {
> >>>> +    DEFINE_PROP_UINT16("cu_type", CcwTesterDevice, cu_type,
> >>>> +                        0x3831),    
> >>>
> >>> 0x4711 would be nice :)    
> >>
> >> I don't understand the joke/pun/whatever if there is one,
> >> but I'm fine with changing this too.  
> > 
> > https://en.wikipedia.org/wiki/4711
> > 
> > That's my default if I need a four-digit number :)
> >   
> >>  
> >>>
> >>> If we want to follow up on that testdev idea (and I think we should),
> >>> it might make sense to have a proper type reserve to prevent accidental
> >>> clashes.    
> >>
> >> I agree. Although I would still keep the cu_type configurable,
> >> because it might make sense to test a particular 'real' driver
> >> (and not a test driver like here). I haven't really thought
> >> this through, but it was an idea I had while agonizing over
> >> not having a proper type reserved.
> >>
> >> I suppose you did something like that for virtio, or? I'm in dark
> >> when it comes to the question what process do we/I have to go to
> >> get a type,for example 0x4711, reserved.  
> > 
> > 4711 is more a joke :) It might be worth trying the same channels as
> > for virtio-ccw.
> > 
> > Christian should know more about that.  
> 
> Getting a new number was very easy (because it is attached to a machine type
> number). I I remember correctly, only numerical values are uses, so maybe
> we can use ffff as there will never be such a real value?
> 

Yes, that sounds like the easiest way to do that.

Re: [Qemu-devel] [PATCH 1/2] s390x/ccs: add ccw-tester emulated device

[Pierre Morel]

On 15/09/2017 19:01, Cornelia Huck wrote:
> On Fri, 15 Sep 2017 09:27:58 +0200
> Cornelia Huck <cohuck@redhat.com> wrote:
> 
>> On Thu, 14 Sep 2017 18:50:29 +0200
>> Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
>>
>>> On 09/14/2017 04:26 PM, Cornelia Huck wrote:
> 
>>>> There's quite some potential in this. We may want to make this a
>>>> permanent addition.
>>>>      
>>>
>>> I'm happy to contribute! I'm not sure how shall we proceed though.
>>> Maybe with making a todo list?
>>
>> I think the first step would be to figure out the ids so we don't step
>> on anyone's toes. Then maybe refactor a bit so that other testers can
>> be added easily.
>>
>> For ideas about things to be tested, maybe put a list into the wiki?
> 
> OK, I've created https://wiki.qemu.org/Testing/CCWTestDevice - let me
> know if you need an account.
> 

Great!
I just saw your email, it was waiting in my inbox.
strange things happens some time. A good serie. strange things.
neverless...

Seriously.
Yes, I definitively need an account!


Pierre

-- 
Pierre Morel
Linux/KVM/QEMU in Böblingen - Germany

Re: [Qemu-devel] [PATCH 1/2] s390x/ccs: add ccw-tester emulated device

[Pierre Morel]

On 14/09/2017 16:26, Cornelia Huck wrote:
> On Wed, 13 Sep 2017 15:27:51 +0200
> Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
> 
>> 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.
>>
>> Of course lot's of invalid inputs (besides basic data processing) can be
>> tested with that as well.
>>
>> Usage:
>> 1) fire up a qemu with something like -device ccw-tester,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>
>> ---
>>
>> Depends on the series 'add CCW indirect data access support'
>>
>> ---
>>   hw/s390x/Makefile.objs |   1 +
>>   hw/s390x/ccw-tester.c  | 179 +++++++++++++++++++++++++++++++++++++++++++++++++
>>   2 files changed, 180 insertions(+)
>>   create mode 100644 hw/s390x/ccw-tester.c
>>
> 
>> +static int  ccw_tester_write_fib(SubchDev *sch, CCW1 ccw)
>> +{
>> +    CcwTesterDevice *d = sch->driver_data;
>> +    bool is_fib = true;
>> +    uint32_t sum;
>> +    int ret = 0;
>> +
>> +    ccw_dstream_init(&sch->cds, &ccw, &sch->orb);
>> +    d->fib.next = 0;
>> +    while (ccw_dstream_avail(&sch->cds) > 0) {
>> +        ret = ccw_dstream_read(&sch->cds,
>> +                               d->fib.ring[abs_to_ring(d->fib.next)]);
>> +        if (ret) {
>> +            error(0, -ret, "fib");
>> +            break;
>> +        }
>> +        if (d->fib.next > 2) {
>> +            sum = (d->fib.ring[abs_to_ring(d->fib.next - 1)]
>> +                  + d->fib.ring[abs_to_ring(d->fib.next - 2)]);
>> +            is_fib = sum ==  d->fib.ring[abs_to_ring(d->fib.next)];
> 
> This is not endian-safe (noticed while testing on my laptop). Trivial
> to fix:
> 
> diff --git a/hw/s390x/ccw-tester.c b/hw/s390x/ccw-tester.c
> index c8017818c4..a425daaa34 100644
> --- a/hw/s390x/ccw-tester.c
> +++ b/hw/s390x/ccw-tester.c
> @@ -58,9 +58,9 @@ static int  ccw_tester_write_fib(SubchDev *sch, CCW1 ccw)
>               break;
>           }
>           if (d->fib.next > 2) {
> -            sum = (d->fib.ring[abs_to_ring(d->fib.next - 1)]
> -                  + d->fib.ring[abs_to_ring(d->fib.next - 2)]);
> -            is_fib = sum ==  d->fib.ring[abs_to_ring(d->fib.next)];
> +            sum = be32_to_cpu(d->fib.ring[abs_to_ring(d->fib.next - 1)])
> +                + be32_to_cpu(d->fib.ring[abs_to_ring(d->fib.next - 2)]);
> +            is_fib = sum == be32_to_cpu(d->fib.ring[abs_to_ring(d->fib.next)]);
>               if (!is_fib) {
>                   break;
>               }
> 
>> +            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 Property ccw_tester_properties[] = {
>> +    DEFINE_PROP_UINT16("cu_type", CcwTesterDevice, cu_type,
>> +                        0x3831),
> 
> 0x4711 would be nice :)

The C0C0 channel would be nice too.
Sorry.

> 
> If we want to follow up on that testdev idea (and I think we should),
> it might make sense to have a proper type reserve to prevent accidental
> clashes.
> 
> (Or is there already something reserved for "hypervisor use" or
> whatever?)
> 
>> +    DEFINE_PROP_UINT8("chpid_type", CcwTesterDevice, chpid_type,
>> +                       0x98),
>> +    DEFINE_PROP_END_OF_LIST(),
>> +};
> 
> IIUC, pci-testdev provides some unit tests to testers (like kvm-tests)
> itself. This might be an idea to follow up on for ccw as well.
> 
> There's quite some potential in this. We may want to make this a
> permanent addition.
> 


-- 
Pierre Morel
Linux/KVM/QEMU in Böblingen - Germany

Re: [Qemu-devel] [PATCH 1/2] s390x/ccs: add ccw-tester emulated device

[Halil Pasic]

On 09/19/2017 04:20 PM, Pierre Morel wrote:
> On 15/09/2017 19:01, Cornelia Huck wrote:
>> On Fri, 15 Sep 2017 09:27:58 +0200
>> Cornelia Huck <cohuck@redhat.com> wrote:
>>
>>> On Thu, 14 Sep 2017 18:50:29 +0200
>>> Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
>>>
>>>> On 09/14/2017 04:26 PM, Cornelia Huck wrote:
>>
>>>>> There's quite some potential in this. We may want to make this a
>>>>> permanent addition.
>>>>>      
>>>>
>>>> I'm happy to contribute! I'm not sure how shall we proceed though.
>>>> Maybe with making a todo list?
>>>
>>> I think the first step would be to figure out the ids so we don't step
>>> on anyone's toes. Then maybe refactor a bit so that other testers can
>>> be added easily.
>>>
>>> For ideas about things to be tested, maybe put a list into the wiki?
>>
>> OK, I've created https://wiki.qemu.org/Testing/CCWTestDevice - let me
>> know if you need an account.
>>
> 
> Great!
> I just saw your email, it was waiting in my inbox.
> strange things happens some time. A good serie. strange things.
> neverless...
> 
> Seriously.
> Yes, I definitively need an account!
> 
> 
> Pierre
> 

@Connie
I would also like to have an account. I would also like to do a
v2 of this somewhere in the not too distant future. I intend
to address the issues pointed out by you here (chpid_type,
cu_type). Another question is whether this should go under
hw/misc/ like the pci-testdev? ccw-testdev would also probably
be a better file name (that ccw-tester) regardless of in which
folder does this belong. One more thing I am considering for
v2 is this make it extensible argument. I had something like
mode on my mind from the very beginning (fib would be one mode).
The idea was to provide a control ccw for setting/getting the mode
(something like virtio) as well as a device property for setting
the initial mode (so that guest does not have to know about it).

Regards,
Halil

Re: [Qemu-devel] [PATCH 1/2] s390x/ccs: add ccw-tester emulated device

[Cornelia Huck]

On Mon, 25 Sep 2017 17:06:01 +0200
Halil Pasic <pasic@linux.vnet.ibm.com> wrote:

> @Connie
> I would also like to have an account. 

Done (for both of you).

> I would also like to do a
> v2 of this somewhere in the not too distant future. I intend
> to address the issues pointed out by you here (chpid_type,
> cu_type). Another question is whether this should go under
> hw/misc/ like the pci-testdev? ccw-testdev would also probably
> be a better file name (that ccw-tester) regardless of in which
> folder does this belong. 

Probably not a bad idea to follow pci here.

> One more thing I am considering for
> v2 is this make it extensible argument. I had something like
> mode on my mind from the very beginning (fib would be one mode).
> The idea was to provide a control ccw for setting/getting the mode
> (something like virtio) as well as a device property for setting
> the initial mode (so that guest does not have to know about it).

Yes, a control ccw sounds nice, we should just make sure that it
doesn't collide with future tests (e.g. if we want to do some channel
program fuzzing). Alternatively, this could be controled by a diagnose,
which would be out of band (we can use a high function code for diag
500 that is unlikely to collide with future extensions).

Re: [Qemu-devel] [PATCH 1/2] s390x/ccs: add ccw-tester emulated device

[Halil Pasic]

On 09/15/2017 09:27 AM, Cornelia Huck wrote:
>>>   
>>>> +    DEFINE_PROP_UINT8("chpid_type", CcwTesterDevice, chpid_type,
>>>> +                       0x98),
> This might also need re-evaluation - we should not really need a new
> chpid type.
> 

I'm back at ccw-tester again (for v2). I've realized we did not agree
on what to use here (chpid_type). Shall I use 0x25 (Fibre Channel) or
EMULATED_CCW_3270_CHPID_TYPE, or even 0x32 (virtio-ccw) as a default
value? And should EMULATED_CCW_3270_CHPID_TYPE be called like that
(is it really supposed to be specific to 3270?

Sorry I did not notice sooner.

Regards,
Halil

Re: [Qemu-devel] [PATCH 1/2] s390x/ccs: add ccw-tester emulated device

[Cornelia Huck]

On Thu, 19 Oct 2017 18:39:37 +0200
Halil Pasic <pasic@linux.vnet.ibm.com> wrote:

> On 09/15/2017 09:27 AM, Cornelia Huck wrote:
> >>>     
> >>>> +    DEFINE_PROP_UINT8("chpid_type", CcwTesterDevice, chpid_type,
> >>>> +                       0x98),  
> > This might also need re-evaluation - we should not really need a new
> > chpid type.
> >   
> 
> I'm back at ccw-tester again (for v2). I've realized we did not agree
> on what to use here (chpid_type). Shall I use 0x25 (Fibre Channel) or
> EMULATED_CCW_3270_CHPID_TYPE, or even 0x32 (virtio-ccw) as a default
> value? And should EMULATED_CCW_3270_CHPID_TYPE be called like that
> (is it really supposed to be specific to 3270?
> 
> Sorry I did not notice sooner.

It might make sense to pick whatever z/VM commonly uses for emulated
devices. (Or ask them if they reserved something explicitly for testing
-- that would be an even better match.)

Re: [Qemu-devel] [PATCH 1/2] s390x/ccs: add ccw-tester emulated device

[Halil Pasic]

On 10/20/2017 03:00 PM, Cornelia Huck wrote:
> On Thu, 19 Oct 2017 18:39:37 +0200
> Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
> 
>> On 09/15/2017 09:27 AM, Cornelia Huck wrote:
>>>>>     
>>>>>> +    DEFINE_PROP_UINT8("chpid_type", CcwTesterDevice, chpid_type,
>>>>>> +                       0x98),  
>>> This might also need re-evaluation - we should not really need a new
>>> chpid type.
>>>   
>>
>> I'm back at ccw-tester again (for v2). I've realized we did not agree
>> on what to use here (chpid_type). Shall I use 0x25 (Fibre Channel) or
>> EMULATED_CCW_3270_CHPID_TYPE, or even 0x32 (virtio-ccw) as a default
>> value? And should EMULATED_CCW_3270_CHPID_TYPE be called like that
>> (is it really supposed to be specific to 3270?
>>
>> Sorry I did not notice sooner.
> 
> It might make sense to pick whatever z/VM commonly uses for emulated
> devices. (Or ask them if they reserved something explicitly for testing
> -- that would be an even better match.)
> 

OK, I will approach the z/VM guys. About EMULATED_CCW_3270_CHPID_TYPE,
does it really make sense to have a separate chpid type for 3270? (You
have missed that question, so I'm asking it again).

Halil

Re: [Qemu-devel] [PATCH 1/2] s390x/ccs: add ccw-tester emulated device

[Cornelia Huck]

On Fri, 20 Oct 2017 16:33:47 +0200
Halil Pasic <pasic@linux.vnet.ibm.com> wrote:

> OK, I will approach the z/VM guys. About EMULATED_CCW_3270_CHPID_TYPE,
> does it really make sense to have a separate chpid type for 3270? (You
> have missed that question, so I'm asking it again).

I had not. But my resources are depleted.