[Qemu-devel] [PATCH 0/5] add CCW indirect data access support

Halil Pasic posted 5 patches 6 years, 6 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20170905111645.18068-1-pasic@linux.vnet.ibm.com
Test checkpatch passed
Test docker passed
Test s390x passed
There is a newer version of this series
.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
[Qemu-devel] [PATCH 0/5] add CCW indirect data access support
Posted by Halil Pasic 6 years, 6 months ago
Abstract 
--------

The objective of this series is introducing CCW IDA (indirect data
access) support to our virtual channel subsystem implementation. Briefly
CCW IDA can be thought of as a kind of a scatter gather support for a
single CCW. If certain flags are set, the cda is to be interpreted as an
address to a list which in turn holds further addresses designating the
actual data.  Thus the scheme which we are currently using for accessing
CCW payload does not work in general case. Currently there is no
immediate need for proper IDA handling (no use case), but since it IDA is
a non-optional part of the architecture, the only way towards AR
compliance is actually implementing IDA.

Testing
-------

Because there is not matching (emulated) QEMU device for the Linux
drivers using IDA testing requires extra effort. The last patch in this
series introduces an emulated CCW device (just for testing) which does
support IDA (using the infrastructure introduced in the previous
patches). The last patch isn't strictly meant for merging. To exercise
this QEMU device an out-of-tree Linux kernel module is provided at the
end of this cover letter (apply to an empty repo using 
git am -c <this_email>).

Halil Pasic (5):
  s390x/css: introduce css data stream
  s390x/css: use ccw data stream
  virtio-ccw: use ccw data stream
  s390x/css: support ccw IDA
  s390x/ccs: add ccw-tester emulated device

 hw/s390x/Makefile.objs |   1 +
 hw/s390x/ccw-tester.c  | 179 +++++++++++++++++++++++++++++++++++++++++++++++++
 hw/s390x/css.c         | 168 +++++++++++++++++++++++++++++++++++++++++++++-
 hw/s390x/virtio-ccw.c  | 158 +++++++++++++------------------------------
 include/hw/s390x/css.h |  67 ++++++++++++++++++
 5 files changed, 461 insertions(+), 112 deletions(-)
 create mode 100644 hw/s390x/ccw-tester.c

-- 
2.13.5

---------------------------8<----------------------------------------
From: Halil Pasic <pasic@linux.vnet.ibm.com>
Date: Fri, 1 Sep 2017 18:59:13 +0200
Subject: [PATCH] ccw-tester: a tester device for ccw I/O

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>
---
 .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 0/5] add CCW indirect data access support
Posted by Halil Pasic 6 years, 6 months ago

On 09/05/2017 01:16 PM, Halil Pasic wrote:
> Abstract 
> --------
> 
> The objective of this series is introducing CCW IDA (indirect data
> access) support to our virtual channel subsystem implementation. Briefly
> CCW IDA can be thought of as a kind of a scatter gather support for a
> single CCW. If certain flags are set, the cda is to be interpreted as an
> address to a list which in turn holds further addresses designating the
> actual data.  Thus the scheme which we are currently using for accessing
> CCW payload does not work in general case. Currently there is no
> immediate need for proper IDA handling (no use case), but since it IDA is
> a non-optional part of the architecture, the only way towards AR
> compliance is actually implementing IDA.
> 

[..]

The discussion seems to have settled down quite a bit. Since there weren't
many complaints, I would like to opt for a v2 fixing the things pointed out
during the review early next week (I was thinking Tuesday maybe some late birds
are going to join in).

@Connie 
========

Does that sound reasonable or would you like more time for v1?

What do you think, would it make more sense to omit or to keep the testing
stuff for v2 (I mean patch 5 and the kernel module in the cover letter)?

You probably haven't found the time to look at have a glance at "s390x/css: drop
data-check in interpretation" (http://patchwork.ozlabs.org/patch/810995/). We
have said it would make some things more straight forward here, and I could
drop that ugly TODO comment. I think it's quite straight-forward, and I would
not mind having a decision on it before v2 or putting it as preparation into
v2. What do you prefer?

Regards,
Halil



Re: [Qemu-devel] [PATCH 0/5] add CCW indirect data access support
Posted by Cornelia Huck 6 years, 6 months ago
On Fri, 8 Sep 2017 12:45:25 +0200
Halil Pasic <pasic@linux.vnet.ibm.com> wrote:

> The discussion seems to have settled down quite a bit. Since there weren't
> many complaints, I would like to opt for a v2 fixing the things pointed out
> during the review early next week (I was thinking Tuesday maybe some late birds
> are going to join in).
> 
> @Connie 
> ========
> 
> Does that sound reasonable or would you like more time for v1?

No, sounds good.

> 
> What do you think, would it make more sense to omit or to keep the testing
> stuff for v2 (I mean patch 5 and the kernel module in the cover letter)?

Can you maybe split this out? It makes it easier if you don't have to
go hunt in a cover letter.

> 
> You probably haven't found the time to look at have a glance at "s390x/css: drop
> data-check in interpretation" (http://patchwork.ozlabs.org/patch/810995/). We
> have said it would make some things more straight forward here, and I could
> drop that ugly TODO comment. I think it's quite straight-forward, and I would
> not mind having a decision on it before v2 or putting it as preparation into
> v2. What do you prefer?

It is marked for my attention. I don't know whether I find time to look
at it today, but probably early next week.

Re: [Qemu-devel] [PATCH 0/5] add CCW indirect data access support
Posted by Halil Pasic 6 years, 6 months ago

On 09/08/2017 12:49 PM, Cornelia Huck wrote:
> On Fri, 8 Sep 2017 12:45:25 +0200
> Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
> 
>> The discussion seems to have settled down quite a bit. Since there weren't
>> many complaints, I would like to opt for a v2 fixing the things pointed out
>> during the review early next week (I was thinking Tuesday maybe some late birds
>> are going to join in).
>>
>> @Connie 
>> ========
>>
>> Does that sound reasonable or would you like more time for v1?
> 
> No, sounds good.
> 

Nod.

>>
>> What do you think, would it make more sense to omit or to keep the testing
>> stuff for v2 (I mean patch 5 and the kernel module in the cover letter)?
> 
> Can you maybe split this out? It makes it easier if you don't have to
> go hunt in a cover letter.
> 

I'm not sure, I know what you mean. Adding an out-of-tree linux kernel module to
the qemu tree does not sound right, so I suppose I should not send it as a patch.

Splitting out the test device patch (#5) does not sound like a good idea either,
because it depends on patches #1 and #4.

TL;DR Yes, I would be glad to if you tell me how.

>>
>> You probably haven't found the time to look at have a glance at "s390x/css: drop
>> data-check in interpretation" (https://urldefense.proofpoint.com/v2/url?u=http-3A__patchwork.ozlabs.org_patch_810995_&d=DwICAg&c=jf_iaSHvJObTbx-siA1ZOg&r=afpWhmOLStQASenyglRLvnb_ajvdRfgp4RlDrLw42F4&m=hshoLebtV7YUijl44CLPl5gP9F1HrXyCbL85tQhvA1w&s=SjTjqdOybbUj1pGpODNHdUfXBZBZU-iav6j10EEWYfQ&e= ). We
>> have said it would make some things more straight forward here, and I could
>> drop that ugly TODO comment. I think it's quite straight-forward, and I would
>> not mind having a decision on it before v2 or putting it as preparation into
>> v2. What do you prefer?
> 
> It is marked for my attention. I don't know whether I find time to look
> at it today, but probably early next week.
> 

OK. Btw, I have a couple of other bug-fixes in the pipe. I think I will just
send out a v1 series to get the discussion started (and for now ignore possible
merge  conflicts with my patches already on the list).

Regards,
Halil


Re: [Qemu-devel] [PATCH 0/5] add CCW indirect data access support
Posted by Cornelia Huck 6 years, 6 months ago
On Fri, 8 Sep 2017 13:03:00 +0200
Halil Pasic <pasic@linux.vnet.ibm.com> wrote:

> On 09/08/2017 12:49 PM, Cornelia Huck wrote:
> > On Fri, 8 Sep 2017 12:45:25 +0200
> > Halil Pasic <pasic@linux.vnet.ibm.com> wrote:

> >> What do you think, would it make more sense to omit or to keep the testing
> >> stuff for v2 (I mean patch 5 and the kernel module in the cover letter)?  
> > 
> > Can you maybe split this out? It makes it easier if you don't have to
> > go hunt in a cover letter.
> >   
> 
> I'm not sure, I know what you mean. Adding an out-of-tree linux kernel module to
> the qemu tree does not sound right, so I suppose I should not send it as a patch.
> 
> Splitting out the test device patch (#5) does not sound like a good idea either,
> because it depends on patches #1 and #4.
> 
> TL;DR Yes, I would be glad to if you tell me how.

I'd do a separate "series" with both the kernel and the qemu part,
stating the dependencies in the cover letter. Patchew will be unhappy,
but I will be happier :)

> 
> >>
> >> You probably haven't found the time to look at have a glance at "s390x/css: drop
> >> data-check in interpretation" (https://urldefense.proofpoint.com/v2/url?u=http-3A__patchwork.ozlabs.org_patch_810995_&d=DwICAg&c=jf_iaSHvJObTbx-siA1ZOg&r=afpWhmOLStQASenyglRLvnb_ajvdRfgp4RlDrLw42F4&m=hshoLebtV7YUijl44CLPl5gP9F1HrXyCbL85tQhvA1w&s=SjTjqdOybbUj1pGpODNHdUfXBZBZU-iav6j10EEWYfQ&e= ). We

Unlikely to be of your doing, but wtf happened here?

> >> have said it would make some things more straight forward here, and I could
> >> drop that ugly TODO comment. I think it's quite straight-forward, and I would
> >> not mind having a decision on it before v2 or putting it as preparation into
> >> v2. What do you prefer?  
> > 
> > It is marked for my attention. I don't know whether I find time to look
> > at it today, but probably early next week.
> >   
> 
> OK. Btw, I have a couple of other bug-fixes in the pipe. I think I will just
> send out a v1 series to get the discussion started (and for now ignore possible
> merge  conflicts with my patches already on the list).

Don't worry about merge conflicts, I need to figure them out myself
anyway :)

Re: [Qemu-devel] [PATCH 0/5] add CCW indirect data access support
Posted by Halil Pasic 6 years, 6 months ago

On 09/08/2017 01:19 PM, Cornelia Huck wrote:
> On Fri, 8 Sep 2017 13:03:00 +0200
> Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
> 
>> On 09/08/2017 12:49 PM, Cornelia Huck wrote:
>>> On Fri, 8 Sep 2017 12:45:25 +0200
>>> Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
> 
>>>> What do you think, would it make more sense to omit or to keep the testing
>>>> stuff for v2 (I mean patch 5 and the kernel module in the cover letter)?  
>>>
>>> Can you maybe split this out? It makes it easier if you don't have to
>>> go hunt in a cover letter.
>>>   
>>
>> I'm not sure, I know what you mean. Adding an out-of-tree linux kernel module to
>> the qemu tree does not sound right, so I suppose I should not send it as a patch.
>>
>> Splitting out the test device patch (#5) does not sound like a good idea either,
>> because it depends on patches #1 and #4.
>>
>> TL;DR Yes, I would be glad to if you tell me how.
> 
> I'd do a separate "series" with both the kernel and the qemu part,
> stating the dependencies in the cover letter. Patchew will be unhappy,
> but I will be happier :)
> 

I can do that, for me you are definitely more important than Patchew.
The kernel module patch won't apply to the qemu tree so I can make
it a two patch series without being too associal.

>>
>>>>
>>>> You probably haven't found the time to look at have a glance at "s390x/css: drop
>>>> data-check in interpretation" (https://urldefense.proofpoint.com/v2/url?u=http-3A__patchwork.ozlabs.org_patch_810995_&d=DwICAg&c=jf_iaSHvJObTbx-siA1ZOg&r=afpWhmOLStQASenyglRLvnb_ajvdRfgp4RlDrLw42F4&m=hshoLebtV7YUijl44CLPl5gP9F1HrXyCbL85tQhvA1w&s=SjTjqdOybbUj1pGpODNHdUfXBZBZU-iav6j10EEWYfQ&e= ). We
> 
> Unlikely to be of your doing, but wtf happened here?
> 

Exactly that. We are fighting it. Corporate cyber-security policies
are not exactly easy to fight.

>>>> have said it would make some things more straight forward here, and I could
>>>> drop that ugly TODO comment. I think it's quite straight-forward, and I would
>>>> not mind having a decision on it before v2 or putting it as preparation into
>>>> v2. What do you prefer?  
>>>
>>> It is marked for my attention. I don't know whether I find time to look
>>> at it today, but probably early next week.
>>>   
>>
>> OK. Btw, I have a couple of other bug-fixes in the pipe. I think I will just
>> send out a v1 series to get the discussion started (and for now ignore possible
>> merge  conflicts with my patches already on the list).
> 
> Don't worry about merge conflicts, I need to figure them out myself
> anyway :)
> 

OK. Many thanks!

Halil


[Qemu-devel] [PATCH 1/5] s390x/css: introduce css data stream
Posted by Halil Pasic 6 years, 6 months ago
This is a preparation for introducing handling for indirect data
addressing and modified indirect data addressing (CCW). Here we introduce
an interface which should make the addressing scheme transparent for the
client code. Here we implement only basic scheme (no IDA or MIDA).

Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
---
 hw/s390x/css.c         | 55 +++++++++++++++++++++++++++++++++++++++++
 include/hw/s390x/css.h | 67 ++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 122 insertions(+)

diff --git a/hw/s390x/css.c b/hw/s390x/css.c
index 1880b1a0ff..87d913f81c 100644
--- a/hw/s390x/css.c
+++ b/hw/s390x/css.c
@@ -783,6 +783,61 @@ static CCW1 copy_ccw_from_guest(hwaddr addr, bool fmt1)
     }
     return ret;
 }
+/**
+ * If out of bounds marks the stream broken. If broken returns -EINVAL,
+ * otherwise the requested  length (may be zero)
+ */
+static inline int cds_check_len(CcwDataStream *cds, int len)
+{
+    if (cds->at_byte + len > cds->count) {
+        cds->flags |= CDS_F_STREAM_BROKEN;
+    }
+    return cds->flags & CDS_F_STREAM_BROKEN ? -EINVAL : len;
+}
+
+static int ccw_dstream_rw_noflags(CcwDataStream *cds, void *buff, int len,
+                                  CcwDataStreamOp op)
+{
+    int ret;
+
+    ret = cds_check_len(cds, len);
+    if (ret <= 0) {
+        return ret;
+    }
+    if (op == CDS_OP_A) {
+        goto incr;
+    }
+    ret = address_space_rw(&address_space_memory, cds->cda,
+                           MEMTXATTRS_UNSPECIFIED, buff, len, op);
+    if (ret != MEMTX_OK) {
+        cds->flags |= CDS_F_STREAM_BROKEN;
+        return -EINVAL;
+    }
+incr:
+    cds->at_byte += len;
+    cds->cda += len;
+    return 0;
+}
+
+void ccw_dstream_init(CcwDataStream *cds, CCW1 const *ccw, ORB const *orb)
+{
+    /*
+     * We don't support MIDA (an optional facility) yet and we
+     * catch this earlier. Just for expressing the precondition.
+     */
+    assert(!(orb->ctrl1 & ORB_CTRL1_MASK_MIDAW));
+    cds->flags = (orb->ctrl0 & ORB_CTRL0_MASK_I2K ? CDS_F_I2K : 0) |
+                 (orb->ctrl0 & ORB_CTRL0_MASK_C64 ? CDS_F_C64 : 0) |
+                 (ccw->flags & CCW_FLAG_IDA ? CDS_F_IDA : 0);
+    cds->count = ccw->count;
+    cds->cda_orig = ccw->cda;
+    ccw_dstream_rewind(cds);
+    if (!(cds->flags & CDS_F_IDA)) {
+        cds->op_handler = ccw_dstream_rw_noflags;
+    } else {
+        assert(false);
+    }
+}
 
 static int css_interpret_ccw(SubchDev *sch, hwaddr ccw_addr,
                              bool suspend_allowed)
diff --git a/include/hw/s390x/css.h b/include/hw/s390x/css.h
index 5c5fe6b202..7b7207f9a2 100644
--- a/include/hw/s390x/css.h
+++ b/include/hw/s390x/css.h
@@ -74,6 +74,29 @@ typedef struct CMBE {
     uint32_t reserved[7];
 } QEMU_PACKED CMBE;
 
+typedef enum CcwDataStreamOp {
+    CDS_OP_R = 0,
+    CDS_OP_W = 1,
+    CDS_OP_A = 2
+} CcwDataStreamOp;
+
+/** normal usage is via SuchchDev.cds instead of instantiating */
+typedef struct CcwDataStream {
+#define CDS_F_IDA   0x01
+#define CDS_F_MIDA  0x02
+#define CDS_F_I2K   0x04
+#define CDS_F_C64   0x08
+#define CDS_F_STREAM_BROKEN  0x80
+    uint8_t flags;
+    uint8_t at_idaw;
+    uint16_t at_byte;
+    uint16_t count;
+    uint32_t cda_orig;
+    int (*op_handler)(struct CcwDataStream *cds, void *buff, int len,
+                      CcwDataStreamOp op);
+    hwaddr cda;
+} CcwDataStream;
+
 typedef struct SubchDev SubchDev;
 struct SubchDev {
     /* channel-subsystem related things: */
@@ -91,6 +114,7 @@ struct SubchDev {
     uint8_t ccw_no_data_cnt;
     uint16_t migrated_schid; /* used for missmatch detection */
     ORB orb;
+    CcwDataStream cds;
     /* transport-provided data: */
     int (*ccw_cb) (SubchDev *, CCW1);
     void (*disable_cb)(SubchDev *);
@@ -238,4 +262,47 @@ SubchDev *css_create_sch(CssDevId bus_id, bool is_virtual, bool squash_mcss,
 /** Turn on css migration */
 void css_register_vmstate(void);
 
+
+void ccw_dstream_init(CcwDataStream *cds, CCW1 const *ccw, ORB const *orb);
+
+static inline void ccw_dstream_rewind(CcwDataStream *cds)
+{
+    cds->at_byte = 0;
+    cds->at_idaw = 0;
+    cds->cda = cds->cda_orig;
+}
+
+static inline bool ccw_dstream_good(CcwDataStream *cds)
+{
+    return !(cds->flags & CDS_F_STREAM_BROKEN);
+}
+
+static inline uint16_t ccw_dstream_residual_count(CcwDataStream *cds)
+{
+    return cds->count - cds->at_byte;
+}
+
+static inline uint16_t ccw_dstream_avail(CcwDataStream *cds)
+{
+    return ccw_dstream_good(cds) ?  ccw_dstream_residual_count(cds) : 0;
+}
+
+static inline int ccw_dstream_advance(CcwDataStream *cds, int len)
+{
+    return cds->op_handler(cds, NULL, len, CDS_OP_A);
+}
+
+static inline int ccw_dstream_write_buf(CcwDataStream *cds, void *buff, int len)
+{
+    return cds->op_handler(cds, buff, len, CDS_OP_W);
+}
+
+static inline int ccw_dstream_read_buf(CcwDataStream *cds, void *buff, int len)
+{
+    return cds->op_handler(cds, buff, len, CDS_OP_R);
+}
+
+#define ccw_dstream_read(cds, v) ccw_dstream_read_buf((cds), &(v), sizeof(v))
+#define ccw_dstream_write(cds, v) ccw_dstream_write_buf((cds), &(v), sizeof(v))
+
 #endif
-- 
2.13.5


Re: [Qemu-devel] [PATCH 1/5] s390x/css: introduce css data stream
Posted by Cornelia Huck 6 years, 6 months ago
On Tue,  5 Sep 2017 13:16:41 +0200
Halil Pasic <pasic@linux.vnet.ibm.com> wrote:

> This is a preparation for introducing handling for indirect data
> addressing and modified indirect data addressing (CCW). Here we introduce
> an interface which should make the addressing scheme transparent for the
> client code. Here we implement only basic scheme (no IDA or MIDA).

s/basic scheme/the basic scheme/

> 
> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
> ---
>  hw/s390x/css.c         | 55 +++++++++++++++++++++++++++++++++++++++++
>  include/hw/s390x/css.h | 67 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 122 insertions(+)
> 
> diff --git a/hw/s390x/css.c b/hw/s390x/css.c
> index 1880b1a0ff..87d913f81c 100644
> --- a/hw/s390x/css.c
> +++ b/hw/s390x/css.c
> @@ -783,6 +783,61 @@ static CCW1 copy_ccw_from_guest(hwaddr addr, bool fmt1)
>      }
>      return ret;
>  }
> +/**
> + * If out of bounds marks the stream broken. If broken returns -EINVAL,
> + * otherwise the requested  length (may be zero)

s/requested  length/requested length/

> + */
> +static inline int cds_check_len(CcwDataStream *cds, int len)
> +{
> +    if (cds->at_byte + len > cds->count) {
> +        cds->flags |= CDS_F_STREAM_BROKEN;
> +    }
> +    return cds->flags & CDS_F_STREAM_BROKEN ? -EINVAL : len;
> +}
> +
> +static int ccw_dstream_rw_noflags(CcwDataStream *cds, void *buff, int len,
> +                                  CcwDataStreamOp op)
> +{
> +    int ret;
> +
> +    ret = cds_check_len(cds, len);
> +    if (ret <= 0) {
> +        return ret;
> +    }
> +    if (op == CDS_OP_A) {
> +        goto incr;
> +    }
> +    ret = address_space_rw(&address_space_memory, cds->cda,
> +                           MEMTXATTRS_UNSPECIFIED, buff, len, op);
> +    if (ret != MEMTX_OK) {
> +        cds->flags |= CDS_F_STREAM_BROKEN;

Do we want to distinguish between different reasons for the stream
being broken? I.e, is there a case where we want to signal different
errors back to the caller?

> +        return -EINVAL;
> +    }
> +incr:
> +    cds->at_byte += len;
> +    cds->cda += len;
> +    return 0;
> +}
> +
> +void ccw_dstream_init(CcwDataStream *cds, CCW1 const *ccw, ORB const *orb)
> +{
> +    /*
> +     * We don't support MIDA (an optional facility) yet and we
> +     * catch this earlier. Just for expressing the precondition.
> +     */
> +    assert(!(orb->ctrl1 & ORB_CTRL1_MASK_MIDAW));

I don't know, this is infrastructure, should it trust its callers? If
you keep the assert, please make it g_assert().

> +    cds->flags = (orb->ctrl0 & ORB_CTRL0_MASK_I2K ? CDS_F_I2K : 0) |
> +                 (orb->ctrl0 & ORB_CTRL0_MASK_C64 ? CDS_F_C64 : 0) |
> +                 (ccw->flags & CCW_FLAG_IDA ? CDS_F_IDA : 0);
> +    cds->count = ccw->count;
> +    cds->cda_orig = ccw->cda;
> +    ccw_dstream_rewind(cds);
> +    if (!(cds->flags & CDS_F_IDA)) {
> +        cds->op_handler = ccw_dstream_rw_noflags;
> +    } else {
> +        assert(false);

Same here.

Or should we make this return an error and have the callers deal with
that? (I still need to look at the users.)

> +    }
> +}
>  
>  static int css_interpret_ccw(SubchDev *sch, hwaddr ccw_addr,
>                               bool suspend_allowed)

Re: [Qemu-devel] [PATCH 1/5] s390x/css: introduce css data stream
Posted by Halil Pasic 6 years, 6 months ago

On 09/06/2017 02:18 PM, Cornelia Huck wrote:
> On Tue,  5 Sep 2017 13:16:41 +0200
> Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
> 
>> This is a preparation for introducing handling for indirect data
>> addressing and modified indirect data addressing (CCW). Here we introduce
>> an interface which should make the addressing scheme transparent for the
>> client code. Here we implement only basic scheme (no IDA or MIDA).
> 
> s/basic scheme/the basic scheme/
> 

Nod.

>>
>> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
>> ---
>>  hw/s390x/css.c         | 55 +++++++++++++++++++++++++++++++++++++++++
>>  include/hw/s390x/css.h | 67 ++++++++++++++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 122 insertions(+)
>>
>> diff --git a/hw/s390x/css.c b/hw/s390x/css.c
>> index 1880b1a0ff..87d913f81c 100644
>> --- a/hw/s390x/css.c
>> +++ b/hw/s390x/css.c
>> @@ -783,6 +783,61 @@ static CCW1 copy_ccw_from_guest(hwaddr addr, bool fmt1)
>>      }
>>      return ret;
>>  }
>> +/**
>> + * If out of bounds marks the stream broken. If broken returns -EINVAL,
>> + * otherwise the requested  length (may be zero)
> 
> s/requested  length/requested length/
> 

Nod.

>> + */
>> +static inline int cds_check_len(CcwDataStream *cds, int len)
>> +{
>> +    if (cds->at_byte + len > cds->count) {
>> +        cds->flags |= CDS_F_STREAM_BROKEN;
>> +    }
>> +    return cds->flags & CDS_F_STREAM_BROKEN ? -EINVAL : len;
>> +}
>> +
>> +static int ccw_dstream_rw_noflags(CcwDataStream *cds, void *buff, int len,
>> +                                  CcwDataStreamOp op)
>> +{
>> +    int ret;
>> +
>> +    ret = cds_check_len(cds, len);
>> +    if (ret <= 0) {
>> +        return ret;
>> +    }
>> +    if (op == CDS_OP_A) {
>> +        goto incr;
>> +    }
>> +    ret = address_space_rw(&address_space_memory, cds->cda,
>> +                           MEMTXATTRS_UNSPECIFIED, buff, len, op);
>> +    if (ret != MEMTX_OK) {
>> +        cds->flags |= CDS_F_STREAM_BROKEN;
> 
> Do we want to distinguish between different reasons for the stream
> being broken? I.e, is there a case where we want to signal different
> errors back to the caller?
> 

Hm, after I've done the error handling it seems that basically
everything is to be handled with a program check. The stream
records the place where the problem was encountered, so for debug
we would not have to search for long.

There seems to be no need to distinguish.

>> +        return -EINVAL;
>> +    }
>> +incr:
>> +    cds->at_byte += len;
>> +    cds->cda += len;
>> +    return 0;
>> +}
>> +
>> +void ccw_dstream_init(CcwDataStream *cds, CCW1 const *ccw, ORB const *orb)
>> +{
>> +    /*
>> +     * We don't support MIDA (an optional facility) yet and we
>> +     * catch this earlier. Just for expressing the precondition.
>> +     */
>> +    assert(!(orb->ctrl1 & ORB_CTRL1_MASK_MIDAW));
> 
> I don't know, this is infrastructure, should it trust its callers? If
> you keep the assert, please make it g_assert().


Why g_assert? I think g_assert comes form a test framework, this is not
test code.

I feel more comfortable having this assert around.

> 
>> +    cds->flags = (orb->ctrl0 & ORB_CTRL0_MASK_I2K ? CDS_F_I2K : 0) |
>> +                 (orb->ctrl0 & ORB_CTRL0_MASK_C64 ? CDS_F_C64 : 0) |
>> +                 (ccw->flags & CCW_FLAG_IDA ? CDS_F_IDA : 0);
>> +    cds->count = ccw->count;
>> +    cds->cda_orig = ccw->cda;
>> +    ccw_dstream_rewind(cds);
>> +    if (!(cds->flags & CDS_F_IDA)) {
>> +        cds->op_handler = ccw_dstream_rw_noflags;
>> +    } else {
>> +        assert(false);
> 
> Same here.
> 
> Or should we make this return an error and have the callers deal with
> that? (I still need to look at the users.)
> 

This assert is going away soon (patch 4). I'm not sure doing much more here
is justified.

>> +    }
>> +}
>>  
>>  static int css_interpret_ccw(SubchDev *sch, hwaddr ccw_addr,
>>                               bool suspend_allowed)
> 


Re: [Qemu-devel] [PATCH 1/5] s390x/css: introduce css data stream
Posted by Cornelia Huck 6 years, 6 months ago
On Wed, 6 Sep 2017 14:40:48 +0200
Halil Pasic <pasic@linux.vnet.ibm.com> wrote:

> On 09/06/2017 02:18 PM, Cornelia Huck wrote:
> > On Tue,  5 Sep 2017 13:16:41 +0200
> > Halil Pasic <pasic@linux.vnet.ibm.com> wrote:

> >> +static int ccw_dstream_rw_noflags(CcwDataStream *cds, void *buff, int len,
> >> +                                  CcwDataStreamOp op)
> >> +{
> >> +    int ret;
> >> +
> >> +    ret = cds_check_len(cds, len);
> >> +    if (ret <= 0) {
> >> +        return ret;
> >> +    }
> >> +    if (op == CDS_OP_A) {
> >> +        goto incr;
> >> +    }
> >> +    ret = address_space_rw(&address_space_memory, cds->cda,
> >> +                           MEMTXATTRS_UNSPECIFIED, buff, len, op);
> >> +    if (ret != MEMTX_OK) {
> >> +        cds->flags |= CDS_F_STREAM_BROKEN;  
> > 
> > Do we want to distinguish between different reasons for the stream
> > being broken? I.e, is there a case where we want to signal different
> > errors back to the caller?
> >   
> 
> Hm, after I've done the error handling it seems that basically
> everything is to be handled with a program check. The stream
> records the place where the problem was encountered, so for debug
> we would not have to search for long.
> 
> There seems to be no need to distinguish.

OK, makes sense. Let's keep it as it is.

> 
> >> +        return -EINVAL;
> >> +    }
> >> +incr:
> >> +    cds->at_byte += len;
> >> +    cds->cda += len;
> >> +    return 0;
> >> +}
> >> +
> >> +void ccw_dstream_init(CcwDataStream *cds, CCW1 const *ccw, ORB const *orb)
> >> +{
> >> +    /*
> >> +     * We don't support MIDA (an optional facility) yet and we
> >> +     * catch this earlier. Just for expressing the precondition.
> >> +     */
> >> +    assert(!(orb->ctrl1 & ORB_CTRL1_MASK_MIDAW));  
> > 
> > I don't know, this is infrastructure, should it trust its callers? If
> > you keep the assert, please make it g_assert().  
> 
> 
> Why g_assert? I think g_assert comes form a test framework, this is not
> test code.

g_assert() is glib, no?

> 
> I feel more comfortable having this assert around.

Let's revisit that when we add mida support :) I don't really object to
it.

> 
> >   
> >> +    cds->flags = (orb->ctrl0 & ORB_CTRL0_MASK_I2K ? CDS_F_I2K : 0) |
> >> +                 (orb->ctrl0 & ORB_CTRL0_MASK_C64 ? CDS_F_C64 : 0) |
> >> +                 (ccw->flags & CCW_FLAG_IDA ? CDS_F_IDA : 0);
> >> +    cds->count = ccw->count;
> >> +    cds->cda_orig = ccw->cda;
> >> +    ccw_dstream_rewind(cds);
> >> +    if (!(cds->flags & CDS_F_IDA)) {
> >> +        cds->op_handler = ccw_dstream_rw_noflags;
> >> +    } else {
> >> +        assert(false);  
> > 
> > Same here.
> > 
> > Or should we make this return an error and have the callers deal with
> > that? (I still need to look at the users.)
> >   
> 
> This assert is going away soon (patch 4). I'm not sure doing much more here
> is justified.

OK, if it is transient anyway...

Re: [Qemu-devel] [PATCH 1/5] s390x/css: introduce css data stream
Posted by Halil Pasic 6 years, 6 months ago

On 09/06/2017 02:51 PM, Cornelia Huck wrote:
>>>> +void ccw_dstream_init(CcwDataStream *cds, CCW1 const *ccw, ORB const *orb)
>>>> +{
>>>> +    /*
>>>> +     * We don't support MIDA (an optional facility) yet and we
>>>> +     * catch this earlier. Just for expressing the precondition.
>>>> +     */
>>>> +    assert(!(orb->ctrl1 & ORB_CTRL1_MASK_MIDAW));  
>>> I don't know, this is infrastructure, should it trust its callers? If
>>> you keep the assert, please make it g_assert().  
>>
>> Why g_assert? I think g_assert comes form a test framework, this is not
>> test code.
> g_assert() is glib, no?
> 

It lives in GLib > GLib Utilities > Testing:
https://developer.gnome.org/glib/stable/glib-Testing.html

The description of "Testing" starts like this: "GLib provides a framework
for writing and maintaining unit tests in parallel to the code they are
testing. The API is designed according to established concepts found in
the other test frameworks (JUnit, NUnit, RUnit), which in turn is based
on smalltalk unit testing concepts."

So yes, it's both glib and testing framework. This is why I
ask why should one use g_assert in not-unit-test code.

Halil


Re: [Qemu-devel] [PATCH 1/5] s390x/css: introduce css data stream
Posted by Cornelia Huck 6 years, 6 months ago
On Mon, 11 Sep 2017 18:36:00 +0200
Halil Pasic <pasic@linux.vnet.ibm.com> wrote:

> On 09/06/2017 02:51 PM, Cornelia Huck wrote:
> >>>> +void ccw_dstream_init(CcwDataStream *cds, CCW1 const *ccw, ORB const *orb)
> >>>> +{
> >>>> +    /*
> >>>> +     * We don't support MIDA (an optional facility) yet and we
> >>>> +     * catch this earlier. Just for expressing the precondition.
> >>>> +     */
> >>>> +    assert(!(orb->ctrl1 & ORB_CTRL1_MASK_MIDAW));    
> >>> I don't know, this is infrastructure, should it trust its callers? If
> >>> you keep the assert, please make it g_assert().    
> >>
> >> Why g_assert? I think g_assert comes form a test framework, this is not
> >> test code.  
> > g_assert() is glib, no?
> >   
> 
> It lives in GLib > GLib Utilities > Testing:
> https://developer.gnome.org/glib/stable/glib-Testing.html
> 
> The description of "Testing" starts like this: "GLib provides a framework
> for writing and maintaining unit tests in parallel to the code they are
> testing. The API is designed according to established concepts found in
> the other test frameworks (JUnit, NUnit, RUnit), which in turn is based
> on smalltalk unit testing concepts."
> 
> So yes, it's both glib and testing framework. This is why I
> ask why should one use g_assert in not-unit-test code.

I have searched the archives, but unfortunately was not able to come to
a conclusion. Checkpatch advises against using anything but g_assert or
g_assert_not_reached in anything but test code, but that is because
those other g_asserts can be made non-fatal. g_assert_not_reached does
not seem to have a non-glib equivalent.

I have it somewhere in the back of my mind that g_assert should be
preferred...

Re: [Qemu-devel] [PATCH 1/5] s390x/css: introduce css data stream
Posted by Halil Pasic 6 years, 6 months ago

On 09/13/2017 11:53 AM, Cornelia Huck wrote:
> On Mon, 11 Sep 2017 18:36:00 +0200
> Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
> 
>> On 09/06/2017 02:51 PM, Cornelia Huck wrote:
>>>>>> +void ccw_dstream_init(CcwDataStream *cds, CCW1 const *ccw, ORB const *orb)
>>>>>> +{
>>>>>> +    /*
>>>>>> +     * We don't support MIDA (an optional facility) yet and we
>>>>>> +     * catch this earlier. Just for expressing the precondition.
>>>>>> +     */
>>>>>> +    assert(!(orb->ctrl1 & ORB_CTRL1_MASK_MIDAW));    
>>>>> I don't know, this is infrastructure, should it trust its callers? If
>>>>> you keep the assert, please make it g_assert().    
>>>>
>>>> Why g_assert? I think g_assert comes form a test framework, this is not
>>>> test code.  
>>> g_assert() is glib, no?
>>>   
>>
>> It lives in GLib > GLib Utilities > Testing:
>> https://developer.gnome.org/glib/stable/glib-Testing.html
>>
>> The description of "Testing" starts like this: "GLib provides a framework
>> for writing and maintaining unit tests in parallel to the code they are
>> testing. The API is designed according to established concepts found in
>> the other test frameworks (JUnit, NUnit, RUnit), which in turn is based
>> on smalltalk unit testing concepts."
>>
>> So yes, it's both glib and testing framework. This is why I
>> ask why should one use g_assert in not-unit-test code.
> 
> I have searched the archives, but unfortunately was not able to come to
> a conclusion. Checkpatch advises against using anything but g_assert or
> g_assert_not_reached in anything but test code, but that is because
> those other g_asserts can be made non-fatal. g_assert_not_reached does
> not seem to have a non-glib equivalent.
> 

I think the standard library equivalent of g_assert_not_reached is
assert(false).

Yeah, checkpatch does not advise against using g_assert and
g_assert_not_reached in non-test code, but it does not advise against
using standard lib assert.

> I have it somewhere in the back of my mind that g_assert should be
> preferred...
> 

OK, I will use g_assert.


[Qemu-devel] [PATCH 2/5] s390x/css: use ccw data stream
Posted by Halil Pasic 6 years, 6 months ago
Replace direct access which implicitly assumes no IDA
or MIDA with the new ccw data stream interface which should
cope with these transparently in the future.

Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
---
 hw/s390x/css.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/hw/s390x/css.c b/hw/s390x/css.c
index 87d913f81c..c1bc9944e6 100644
--- a/hw/s390x/css.c
+++ b/hw/s390x/css.c
@@ -890,6 +890,7 @@ static int css_interpret_ccw(SubchDev *sch, hwaddr ccw_addr,
     }
 
     /* Look at the command. */
+    ccw_dstream_init(&sch->cds, &ccw, &(sch->orb));
     switch (ccw.cmd_code) {
     case CCW_CMD_NOOP:
         /* Nothing to do. */
@@ -903,7 +904,7 @@ static int css_interpret_ccw(SubchDev *sch, hwaddr ccw_addr,
             }
         }
         len = MIN(ccw.count, sizeof(sch->sense_data));
-        cpu_physical_memory_write(ccw.cda, sch->sense_data, len);
+        ccw_dstream_write_buf(&sch->cds, sch->sense_data, len);
         sch->curr_status.scsw.count = ccw.count - len;
         memset(sch->sense_data, 0, sizeof(sch->sense_data));
         ret = 0;
@@ -930,7 +931,7 @@ static int css_interpret_ccw(SubchDev *sch, hwaddr ccw_addr,
         } else {
             sense_id.reserved = 0;
         }
-        cpu_physical_memory_write(ccw.cda, &sense_id, len);
+        ccw_dstream_write_buf(&sch->cds, &sense_id, len);
         sch->curr_status.scsw.count = ccw.count - len;
         ret = 0;
         break;
-- 
2.13.5


Re: [Qemu-devel] [PATCH 2/5] s390x/css: use ccw data stream
Posted by Cornelia Huck 6 years, 6 months ago
On Tue,  5 Sep 2017 13:16:42 +0200
Halil Pasic <pasic@linux.vnet.ibm.com> wrote:

> Replace direct access which implicitly assumes no IDA
> or MIDA with the new ccw data stream interface which should
> cope with these transparently in the future.
> 
> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
> ---
>  hw/s390x/css.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/s390x/css.c b/hw/s390x/css.c
> index 87d913f81c..c1bc9944e6 100644
> --- a/hw/s390x/css.c
> +++ b/hw/s390x/css.c
> @@ -890,6 +890,7 @@ static int css_interpret_ccw(SubchDev *sch, hwaddr ccw_addr,
>      }
>  
>      /* Look at the command. */
> +    ccw_dstream_init(&sch->cds, &ccw, &(sch->orb));
>      switch (ccw.cmd_code) {
>      case CCW_CMD_NOOP:
>          /* Nothing to do. */
> @@ -903,7 +904,7 @@ static int css_interpret_ccw(SubchDev *sch, hwaddr ccw_addr,
>              }
>          }
>          len = MIN(ccw.count, sizeof(sch->sense_data));
> -        cpu_physical_memory_write(ccw.cda, sch->sense_data, len);
> +        ccw_dstream_write_buf(&sch->cds, sch->sense_data, len);
>          sch->curr_status.scsw.count = ccw.count - len;

ccw_dstream_residual_count()?

>          memset(sch->sense_data, 0, sizeof(sch->sense_data));
>          ret = 0;
> @@ -930,7 +931,7 @@ static int css_interpret_ccw(SubchDev *sch, hwaddr ccw_addr,
>          } else {
>              sense_id.reserved = 0;
>          }
> -        cpu_physical_memory_write(ccw.cda, &sense_id, len);
> +        ccw_dstream_write_buf(&sch->cds, &sense_id, len);
>          sch->curr_status.scsw.count = ccw.count - len;

dito

>          ret = 0;
>          break;


Re: [Qemu-devel] [PATCH 2/5] s390x/css: use ccw data stream
Posted by Halil Pasic 6 years, 6 months ago

On 09/06/2017 02:32 PM, Cornelia Huck wrote:
> On Tue,  5 Sep 2017 13:16:42 +0200
> Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
> 
>> Replace direct access which implicitly assumes no IDA
>> or MIDA with the new ccw data stream interface which should
>> cope with these transparently in the future.
>>
>> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
>> ---
>>  hw/s390x/css.c | 5 +++--
>>  1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/s390x/css.c b/hw/s390x/css.c
>> index 87d913f81c..c1bc9944e6 100644
>> --- a/hw/s390x/css.c
>> +++ b/hw/s390x/css.c
>> @@ -890,6 +890,7 @@ static int css_interpret_ccw(SubchDev *sch, hwaddr ccw_addr,
>>      }
>>  
>>      /* Look at the command. */
>> +    ccw_dstream_init(&sch->cds, &ccw, &(sch->orb));
>>      switch (ccw.cmd_code) {
>>      case CCW_CMD_NOOP:
>>          /* Nothing to do. */
>> @@ -903,7 +904,7 @@ static int css_interpret_ccw(SubchDev *sch, hwaddr ccw_addr,
>>              }
>>          }
>>          len = MIN(ccw.count, sizeof(sch->sense_data));
>> -        cpu_physical_memory_write(ccw.cda, sch->sense_data, len);
>> +        ccw_dstream_write_buf(&sch->cds, sch->sense_data, len);
>>          sch->curr_status.scsw.count = ccw.count - len;
> 
> ccw_dstream_residual_count()?

Yeah, I've introduced that function later. Will do.

> 
>>          memset(sch->sense_data, 0, sizeof(sch->sense_data));
>>          ret = 0;
>> @@ -930,7 +931,7 @@ static int css_interpret_ccw(SubchDev *sch, hwaddr ccw_addr,
>>          } else {
>>              sense_id.reserved = 0;
>>          }
>> -        cpu_physical_memory_write(ccw.cda, &sense_id, len);
>> +        ccw_dstream_write_buf(&sch->cds, &sense_id, len);
>>          sch->curr_status.scsw.count = ccw.count - len;
> 
> dito
> 
>>          ret = 0;
>>          break;
> 


Re: [Qemu-devel] [PATCH 2/5] s390x/css: use ccw data stream
Posted by Pierre Morel 6 years, 6 months ago
On 05/09/2017 13:16, Halil Pasic wrote:
> Replace direct access which implicitly assumes no IDA
> or MIDA with the new ccw data stream interface which should
> cope with these transparently in the future.
> 
> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
> ---
>   hw/s390x/css.c | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/s390x/css.c b/hw/s390x/css.c
> index 87d913f81c..c1bc9944e6 100644
> --- a/hw/s390x/css.c
> +++ b/hw/s390x/css.c
> @@ -890,6 +890,7 @@ static int css_interpret_ccw(SubchDev *sch, hwaddr ccw_addr,
>       }
> 
>       /* Look at the command. */
> +    ccw_dstream_init(&sch->cds, &ccw, &(sch->orb));
>       switch (ccw.cmd_code) {
>       case CCW_CMD_NOOP:
>           /* Nothing to do. */
> @@ -903,7 +904,7 @@ static int css_interpret_ccw(SubchDev *sch, hwaddr ccw_addr,
>               }
>           }
>           len = MIN(ccw.count, sizeof(sch->sense_data));
> -        cpu_physical_memory_write(ccw.cda, sch->sense_data, len);
> +        ccw_dstream_write_buf(&sch->cds, sch->sense_data, len);

Don't you need to check the return value ?

>           sch->curr_status.scsw.count = ccw.count - len;
>           memset(sch->sense_data, 0, sizeof(sch->sense_data));
>           ret = 0;
> @@ -930,7 +931,7 @@ static int css_interpret_ccw(SubchDev *sch, hwaddr ccw_addr,
>           } else {
>               sense_id.reserved = 0;
>           }
> -        cpu_physical_memory_write(ccw.cda, &sense_id, len);
> +        ccw_dstream_write_buf(&sch->cds, &sense_id, len);

here too ?

>           sch->curr_status.scsw.count = ccw.count - len;
>           ret = 0;
>           break;
> 


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


Re: [Qemu-devel] [PATCH 2/5] s390x/css: use ccw data stream
Posted by Pierre Morel 6 years, 6 months ago
On 21/09/2017 11:33, Pierre Morel wrote:
> On 05/09/2017 13:16, Halil Pasic wrote:
>> Replace direct access which implicitly assumes no IDA
>> or MIDA with the new ccw data stream interface which should
>> cope with these transparently in the future.
>>
>> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
>> ---
>>   hw/s390x/css.c | 5 +++--
>>   1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/s390x/css.c b/hw/s390x/css.c
>> index 87d913f81c..c1bc9944e6 100644
>> --- a/hw/s390x/css.c
>> +++ b/hw/s390x/css.c
>> @@ -890,6 +890,7 @@ static int css_interpret_ccw(SubchDev *sch, hwaddr 
>> ccw_addr,
>>       }
>>
>>       /* Look at the command. */
>> +    ccw_dstream_init(&sch->cds, &ccw, &(sch->orb));
>>       switch (ccw.cmd_code) {
>>       case CCW_CMD_NOOP:
>>           /* Nothing to do. */
>> @@ -903,7 +904,7 @@ static int css_interpret_ccw(SubchDev *sch, hwaddr 
>> ccw_addr,
>>               }
>>           }
>>           len = MIN(ccw.count, sizeof(sch->sense_data));
>> -        cpu_physical_memory_write(ccw.cda, sch->sense_data, len);
>> +        ccw_dstream_write_buf(&sch->cds, sch->sense_data, len);
> 
> Don't you need to check the return value ?
> 
>>           sch->curr_status.scsw.count = ccw.count - len;
>>           memset(sch->sense_data, 0, sizeof(sch->sense_data));
>>           ret = 0;
>> @@ -930,7 +931,7 @@ static int css_interpret_ccw(SubchDev *sch, hwaddr 
>> ccw_addr,
>>           } else {
>>               sense_id.reserved = 0;
>>           }
>> -        cpu_physical_memory_write(ccw.cda, &sense_id, len);
>> +        ccw_dstream_write_buf(&sch->cds, &sense_id, len);
> 
> here too ?
> 
>>           sch->curr_status.scsw.count = ccw.count - len;
>>           ret = 0;
>>           break;
>>
> 
> 
Forget it I took the wrong serie.
Sorry


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

Re: [Qemu-devel] [PATCH 2/5] s390x/css: use ccw data stream
Posted by Cornelia Huck 6 years, 6 months ago
On Thu, 21 Sep 2017 11:33:03 +0200
Pierre Morel <pmorel@linux.vnet.ibm.com> wrote:

> On 05/09/2017 13:16, Halil Pasic wrote:
> > Replace direct access which implicitly assumes no IDA
> > or MIDA with the new ccw data stream interface which should
> > cope with these transparently in the future.
> > 
> > Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
> > ---
> >   hw/s390x/css.c | 5 +++--
> >   1 file changed, 3 insertions(+), 2 deletions(-)
> > 
> > diff --git a/hw/s390x/css.c b/hw/s390x/css.c
> > index 87d913f81c..c1bc9944e6 100644
> > --- a/hw/s390x/css.c
> > +++ b/hw/s390x/css.c
> > @@ -890,6 +890,7 @@ static int css_interpret_ccw(SubchDev *sch, hwaddr ccw_addr,
> >       }
> > 
> >       /* Look at the command. */
> > +    ccw_dstream_init(&sch->cds, &ccw, &(sch->orb));
> >       switch (ccw.cmd_code) {
> >       case CCW_CMD_NOOP:
> >           /* Nothing to do. */
> > @@ -903,7 +904,7 @@ static int css_interpret_ccw(SubchDev *sch, hwaddr ccw_addr,
> >               }
> >           }
> >           len = MIN(ccw.count, sizeof(sch->sense_data));
> > -        cpu_physical_memory_write(ccw.cda, sch->sense_data, len);
> > +        ccw_dstream_write_buf(&sch->cds, sch->sense_data, len);  
> 
> Don't you need to check the return value ?
> 
> >           sch->curr_status.scsw.count = ccw.count - len;
> >           memset(sch->sense_data, 0, sizeof(sch->sense_data));
> >           ret = 0;
> > @@ -930,7 +931,7 @@ static int css_interpret_ccw(SubchDev *sch, hwaddr ccw_addr,
> >           } else {
> >               sense_id.reserved = 0;
> >           }
> > -        cpu_physical_memory_write(ccw.cda, &sense_id, len);
> > +        ccw_dstream_write_buf(&sch->cds, &sense_id, len);  
> 
> here too ?
> 
> >           sch->curr_status.scsw.count = ccw.count - len;
> >           ret = 0;
> >           break;
> >   
> 
> 

I think Halil wanted to introduce error checking in a patch on top, to
keep changes minimal for now (at least one of the patch submissions
said so).

It isn't worse than before, so putting it into a separate patch is fine
by me.

[Qemu-devel] [PATCH 3/5] virtio-ccw: use ccw data stream
Posted by Halil Pasic 6 years, 6 months ago
Replace direct access which implicitly assumes no IDA
or MIDA with the new ccw data stream interface which should
cope with these transparently in the future.

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

---

Error handling: At the moment we ignore errors reported
by stream ops to keep the change minimal. It might make sense
to change this.
---
 hw/s390x/virtio-ccw.c | 158 +++++++++++++++-----------------------------------
 1 file changed, 48 insertions(+), 110 deletions(-)

diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
index b1976fdd19..72dd129a15 100644
--- a/hw/s390x/virtio-ccw.c
+++ b/hw/s390x/virtio-ccw.c
@@ -287,49 +287,19 @@ static int virtio_ccw_handle_set_vq(SubchDev *sch, CCW1 ccw, bool check_len,
         return -EFAULT;
     }
     if (is_legacy) {
-        linfo.queue = address_space_ldq_be(&address_space_memory, ccw.cda,
-                                           MEMTXATTRS_UNSPECIFIED, NULL);
-        linfo.align = address_space_ldl_be(&address_space_memory,
-                                           ccw.cda + sizeof(linfo.queue),
-                                           MEMTXATTRS_UNSPECIFIED,
-                                           NULL);
-        linfo.index = address_space_lduw_be(&address_space_memory,
-                                            ccw.cda + sizeof(linfo.queue)
-                                            + sizeof(linfo.align),
-                                            MEMTXATTRS_UNSPECIFIED,
-                                            NULL);
-        linfo.num = address_space_lduw_be(&address_space_memory,
-                                          ccw.cda + sizeof(linfo.queue)
-                                          + sizeof(linfo.align)
-                                          + sizeof(linfo.index),
-                                          MEMTXATTRS_UNSPECIFIED,
-                                          NULL);
+        ccw_dstream_read(&sch->cds, linfo);
+        be64_to_cpus(&linfo.queue);
+        be32_to_cpus(&linfo.align);
+        be16_to_cpus(&linfo.index);
+        be16_to_cpus(&linfo.num);
         ret = virtio_ccw_set_vqs(sch, NULL, &linfo);
     } else {
-        info.desc = address_space_ldq_be(&address_space_memory, ccw.cda,
-                                           MEMTXATTRS_UNSPECIFIED, NULL);
-        info.index = address_space_lduw_be(&address_space_memory,
-                                           ccw.cda + sizeof(info.desc)
-                                           + sizeof(info.res0),
-                                           MEMTXATTRS_UNSPECIFIED, NULL);
-        info.num = address_space_lduw_be(&address_space_memory,
-                                         ccw.cda + sizeof(info.desc)
-                                         + sizeof(info.res0)
-                                         + sizeof(info.index),
-                                         MEMTXATTRS_UNSPECIFIED, NULL);
-        info.avail = address_space_ldq_be(&address_space_memory,
-                                          ccw.cda + sizeof(info.desc)
-                                          + sizeof(info.res0)
-                                          + sizeof(info.index)
-                                          + sizeof(info.num),
-                                          MEMTXATTRS_UNSPECIFIED, NULL);
-        info.used = address_space_ldq_be(&address_space_memory,
-                                         ccw.cda + sizeof(info.desc)
-                                         + sizeof(info.res0)
-                                         + sizeof(info.index)
-                                         + sizeof(info.num)
-                                         + sizeof(info.avail),
-                                         MEMTXATTRS_UNSPECIFIED, NULL);
+        ccw_dstream_read(&sch->cds, info);
+        be64_to_cpus(&info.desc);
+        be16_to_cpus(&info.index);
+        be16_to_cpus(&info.num);
+        be64_to_cpus(&info.avail);
+        be64_to_cpus(&info.used);
         ret = virtio_ccw_set_vqs(sch, &info, NULL);
     }
     sch->curr_status.scsw.count = 0;
@@ -342,15 +312,13 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
     VirtioRevInfo revinfo;
     uint8_t status;
     VirtioFeatDesc features;
-    void *config;
     hwaddr indicators;
     VqConfigBlock vq_config;
     VirtioCcwDevice *dev = sch->driver_data;
     VirtIODevice *vdev = virtio_ccw_get_vdev(sch);
     bool check_len;
     int len;
-    hwaddr hw_len;
-    VirtioThinintInfo *thinint;
+    VirtioThinintInfo thinint;
 
     if (!dev) {
         return -EINVAL;
@@ -394,11 +362,8 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
         } else {
             VirtioDeviceClass *vdc = VIRTIO_DEVICE_GET_CLASS(vdev);
 
-            features.index = address_space_ldub(&address_space_memory,
-                                                ccw.cda
-                                                + sizeof(features.features),
-                                                MEMTXATTRS_UNSPECIFIED,
-                                                NULL);
+            ccw_dstream_advance(&sch->cds, sizeof(features.features));
+            ccw_dstream_read(&sch->cds, features.index);
             if (features.index == 0) {
                 if (dev->revision >= 1) {
                     /* Don't offer legacy features for modern devices. */
@@ -417,9 +382,9 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
                 /* Return zeroes if the guest supports more feature bits. */
                 features.features = 0;
             }
-            address_space_stl_le(&address_space_memory, ccw.cda,
-                                 features.features, MEMTXATTRS_UNSPECIFIED,
-                                 NULL);
+            ccw_dstream_rewind(&sch->cds);
+            cpu_to_le32s(&features.features);
+            ccw_dstream_write(&sch->cds, features.features);
             sch->curr_status.scsw.count = ccw.count - sizeof(features);
             ret = 0;
         }
@@ -438,15 +403,8 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
         if (!ccw.cda) {
             ret = -EFAULT;
         } else {
-            features.index = address_space_ldub(&address_space_memory,
-                                                ccw.cda
-                                                + sizeof(features.features),
-                                                MEMTXATTRS_UNSPECIFIED,
-                                                NULL);
-            features.features = address_space_ldl_le(&address_space_memory,
-                                                     ccw.cda,
-                                                     MEMTXATTRS_UNSPECIFIED,
-                                                     NULL);
+            ccw_dstream_read(&sch->cds, features);
+            le32_to_cpus(&features.features);
             if (features.index == 0) {
                 virtio_set_features(vdev,
                                     (vdev->guest_features & 0xffffffff00000000ULL) |
@@ -488,7 +446,8 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
         } else {
             virtio_bus_get_vdev_config(&dev->bus, vdev->config);
             /* XXX config space endianness */
-            cpu_physical_memory_write(ccw.cda, vdev->config, len);
+            /* TODO we may have made -EINVAL out of -EFAULT */
+            ccw_dstream_write_buf(&sch->cds, vdev->config, len);
             sch->curr_status.scsw.count = ccw.count - len;
             ret = 0;
         }
@@ -501,21 +460,14 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
             }
         }
         len = MIN(ccw.count, vdev->config_len);
-        hw_len = len;
         if (!ccw.cda) {
             ret = -EFAULT;
         } else {
-            config = cpu_physical_memory_map(ccw.cda, &hw_len, 0);
-            if (!config) {
-                ret = -EFAULT;
-            } else {
-                len = hw_len;
-                /* XXX config space endianness */
-                memcpy(vdev->config, config, len);
-                cpu_physical_memory_unmap(config, hw_len, 0, hw_len);
+            /* TODO we may have made -EINVAL out of -EFAULT */
+            ret = ccw_dstream_read_buf(&sch->cds, vdev->config, len);
+            if (!ret) {
                 virtio_bus_set_vdev_config(&dev->bus, vdev->config);
                 sch->curr_status.scsw.count = ccw.count - len;
-                ret = 0;
             }
         }
         break;
@@ -553,8 +505,7 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
         if (!ccw.cda) {
             ret = -EFAULT;
         } else {
-            status = address_space_ldub(&address_space_memory, ccw.cda,
-                                        MEMTXATTRS_UNSPECIFIED, NULL);
+            ccw_dstream_read(&sch->cds, status);
             if (!(status & VIRTIO_CONFIG_S_DRIVER_OK)) {
                 virtio_ccw_stop_ioeventfd(dev);
             }
@@ -597,8 +548,8 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
         if (!ccw.cda) {
             ret = -EFAULT;
         } else {
-            indicators = address_space_ldq_be(&address_space_memory, ccw.cda,
-                                              MEMTXATTRS_UNSPECIFIED, NULL);
+            ccw_dstream_read(&sch->cds, indicators);
+            be64_to_cpus(&indicators);
             dev->indicators = get_indicator(indicators, sizeof(uint64_t));
             sch->curr_status.scsw.count = ccw.count - sizeof(indicators);
             ret = 0;
@@ -618,8 +569,8 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
         if (!ccw.cda) {
             ret = -EFAULT;
         } else {
-            indicators = address_space_ldq_be(&address_space_memory, ccw.cda,
-                                              MEMTXATTRS_UNSPECIFIED, NULL);
+            ccw_dstream_read(&sch->cds, indicators);
+            be64_to_cpus(&indicators);
             dev->indicators2 = get_indicator(indicators, sizeof(uint64_t));
             sch->curr_status.scsw.count = ccw.count - sizeof(indicators);
             ret = 0;
@@ -639,67 +590,58 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
         if (!ccw.cda) {
             ret = -EFAULT;
         } else {
-            vq_config.index = address_space_lduw_be(&address_space_memory,
-                                                    ccw.cda,
-                                                    MEMTXATTRS_UNSPECIFIED,
-                                                    NULL);
+            ccw_dstream_read(&sch->cds, vq_config.index);
+            be16_to_cpus(&vq_config.index);
             if (vq_config.index >= VIRTIO_QUEUE_MAX) {
                 ret = -EINVAL;
                 break;
             }
             vq_config.num_max = virtio_queue_get_num(vdev,
                                                      vq_config.index);
-            address_space_stw_be(&address_space_memory,
-                                 ccw.cda + sizeof(vq_config.index),
-                                 vq_config.num_max,
-                                 MEMTXATTRS_UNSPECIFIED,
-                                 NULL);
+            cpu_to_be16s(&vq_config.num_max);
+            ccw_dstream_write(&sch->cds, vq_config.num_max);
             sch->curr_status.scsw.count = ccw.count - sizeof(vq_config);
             ret = 0;
         }
         break;
     case CCW_CMD_SET_IND_ADAPTER:
         if (check_len) {
-            if (ccw.count != sizeof(*thinint)) {
+            if (ccw.count != sizeof(thinint)) {
                 ret = -EINVAL;
                 break;
             }
-        } else if (ccw.count < sizeof(*thinint)) {
+        } else if (ccw.count < sizeof(thinint)) {
             /* Can't execute command. */
             ret = -EINVAL;
             break;
         }
-        len = sizeof(*thinint);
-        hw_len = len;
         if (!ccw.cda) {
             ret = -EFAULT;
         } else if (dev->indicators && !sch->thinint_active) {
             /* Trigger a command reject. */
             ret = -ENOSYS;
         } else {
-            thinint = cpu_physical_memory_map(ccw.cda, &hw_len, 0);
-            if (!thinint) {
+            if (ccw_dstream_read(&sch->cds, thinint)) {
                 ret = -EFAULT;
             } else {
-                uint64_t ind_bit = ldq_be_p(&thinint->ind_bit);
+                be64_to_cpus(&thinint.ind_bit);
+                be64_to_cpus(&thinint.summary_indicator);
+                be64_to_cpus(&thinint.device_indicator);
 
-                len = hw_len;
                 dev->summary_indicator =
-                    get_indicator(ldq_be_p(&thinint->summary_indicator),
-                                  sizeof(uint8_t));
+                    get_indicator(thinint.summary_indicator, sizeof(uint8_t));
                 dev->indicators =
-                    get_indicator(ldq_be_p(&thinint->device_indicator),
-                                  ind_bit / 8 + 1);
-                dev->thinint_isc = thinint->isc;
-                dev->routes.adapter.ind_offset = ind_bit;
+                    get_indicator(thinint.device_indicator,
+                                  thinint.ind_bit / 8 + 1);
+                dev->thinint_isc = thinint.isc;
+                dev->routes.adapter.ind_offset = thinint.ind_bit;
                 dev->routes.adapter.summary_offset = 7;
-                cpu_physical_memory_unmap(thinint, hw_len, 0, hw_len);
                 dev->routes.adapter.adapter_id = css_get_adapter_id(
                                                  CSS_IO_ADAPTER_VIRTIO,
                                                  dev->thinint_isc);
                 sch->thinint_active = ((dev->indicators != NULL) &&
                                        (dev->summary_indicator != NULL));
-                sch->curr_status.scsw.count = ccw.count - len;
+                sch->curr_status.scsw.count = ccw.count - sizeof(thinint);
                 ret = 0;
             }
         }
@@ -714,13 +656,9 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
             ret = -EFAULT;
             break;
         }
-        revinfo.revision =
-            address_space_lduw_be(&address_space_memory, ccw.cda,
-                                  MEMTXATTRS_UNSPECIFIED, NULL);
-        revinfo.length =
-            address_space_lduw_be(&address_space_memory,
-                                  ccw.cda + sizeof(revinfo.revision),
-                                  MEMTXATTRS_UNSPECIFIED, NULL);
+        ccw_dstream_read_buf(&sch->cds, &revinfo, 4);
+        be16_to_cpus(&revinfo.revision);
+        be16_to_cpus(&revinfo.length);
         if (ccw.count < len + revinfo.length ||
             (check_len && ccw.count > len + revinfo.length)) {
             ret = -EINVAL;
-- 
2.13.5


Re: [Qemu-devel] [PATCH 3/5] virtio-ccw: use ccw data stream
Posted by Cornelia Huck 6 years, 6 months ago
On Tue,  5 Sep 2017 13:16:43 +0200
Halil Pasic <pasic@linux.vnet.ibm.com> wrote:

> Replace direct access which implicitly assumes no IDA
> or MIDA with the new ccw data stream interface which should
> cope with these transparently in the future.
> 
> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
> 
> ---
> 
> Error handling: At the moment we ignore errors reported
> by stream ops to keep the change minimal. It might make sense
> to change this.

Do that as an add-on patch?

> ---
>  hw/s390x/virtio-ccw.c | 158 +++++++++++++++-----------------------------------
>  1 file changed, 48 insertions(+), 110 deletions(-)
> 
> diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
> index b1976fdd19..72dd129a15 100644
> --- a/hw/s390x/virtio-ccw.c
> +++ b/hw/s390x/virtio-ccw.c
> @@ -287,49 +287,19 @@ static int virtio_ccw_handle_set_vq(SubchDev *sch, CCW1 ccw, bool check_len,
>          return -EFAULT;
>      }
>      if (is_legacy) {
> -        linfo.queue = address_space_ldq_be(&address_space_memory, ccw.cda,
> -                                           MEMTXATTRS_UNSPECIFIED, NULL);
> -        linfo.align = address_space_ldl_be(&address_space_memory,
> -                                           ccw.cda + sizeof(linfo.queue),
> -                                           MEMTXATTRS_UNSPECIFIED,
> -                                           NULL);
> -        linfo.index = address_space_lduw_be(&address_space_memory,
> -                                            ccw.cda + sizeof(linfo.queue)
> -                                            + sizeof(linfo.align),
> -                                            MEMTXATTRS_UNSPECIFIED,
> -                                            NULL);
> -        linfo.num = address_space_lduw_be(&address_space_memory,
> -                                          ccw.cda + sizeof(linfo.queue)
> -                                          + sizeof(linfo.align)
> -                                          + sizeof(linfo.index),
> -                                          MEMTXATTRS_UNSPECIFIED,
> -                                          NULL);
> +        ccw_dstream_read(&sch->cds, linfo);
> +        be64_to_cpus(&linfo.queue);
> +        be32_to_cpus(&linfo.align);
> +        be16_to_cpus(&linfo.index);
> +        be16_to_cpus(&linfo.num);

That's a very nice contraction :)

>          ret = virtio_ccw_set_vqs(sch, NULL, &linfo);
>      } else {
> -        info.desc = address_space_ldq_be(&address_space_memory, ccw.cda,
> -                                           MEMTXATTRS_UNSPECIFIED, NULL);
> -        info.index = address_space_lduw_be(&address_space_memory,
> -                                           ccw.cda + sizeof(info.desc)
> -                                           + sizeof(info.res0),
> -                                           MEMTXATTRS_UNSPECIFIED, NULL);
> -        info.num = address_space_lduw_be(&address_space_memory,
> -                                         ccw.cda + sizeof(info.desc)
> -                                         + sizeof(info.res0)
> -                                         + sizeof(info.index),
> -                                         MEMTXATTRS_UNSPECIFIED, NULL);
> -        info.avail = address_space_ldq_be(&address_space_memory,
> -                                          ccw.cda + sizeof(info.desc)
> -                                          + sizeof(info.res0)
> -                                          + sizeof(info.index)
> -                                          + sizeof(info.num),
> -                                          MEMTXATTRS_UNSPECIFIED, NULL);
> -        info.used = address_space_ldq_be(&address_space_memory,
> -                                         ccw.cda + sizeof(info.desc)
> -                                         + sizeof(info.res0)
> -                                         + sizeof(info.index)
> -                                         + sizeof(info.num)
> -                                         + sizeof(info.avail),
> -                                         MEMTXATTRS_UNSPECIFIED, NULL);
> +        ccw_dstream_read(&sch->cds, info);
> +        be64_to_cpus(&info.desc);
> +        be16_to_cpus(&info.index);
> +        be16_to_cpus(&info.num);
> +        be64_to_cpus(&info.avail);
> +        be64_to_cpus(&info.used);
>          ret = virtio_ccw_set_vqs(sch, &info, NULL);
>      }
>      sch->curr_status.scsw.count = 0;

(...)

> @@ -488,7 +446,8 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
>          } else {
>              virtio_bus_get_vdev_config(&dev->bus, vdev->config);
>              /* XXX config space endianness */

Unrelated: That should be fine, I guess?

> -            cpu_physical_memory_write(ccw.cda, vdev->config, len);
> +            /* TODO we may have made -EINVAL out of -EFAULT */

Eek.

> +            ccw_dstream_write_buf(&sch->cds, vdev->config, len);
>              sch->curr_status.scsw.count = ccw.count - len;
>              ret = 0;
>          }

Looks fine in general.

Re: [Qemu-devel] [PATCH 3/5] virtio-ccw: use ccw data stream
Posted by Halil Pasic 6 years, 6 months ago

On 09/06/2017 02:42 PM, Cornelia Huck wrote:
> On Tue,  5 Sep 2017 13:16:43 +0200
> Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
> 
>> Replace direct access which implicitly assumes no IDA
>> or MIDA with the new ccw data stream interface which should
>> cope with these transparently in the future.
>>
>> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
>>
>> ---
>>
>> Error handling: At the moment we ignore errors reported
>> by stream ops to keep the change minimal. It might make sense
>> to change this.
> 
> Do that as an add-on patch?

That was the idea.

> 
>> ---
>>  hw/s390x/virtio-ccw.c | 158 +++++++++++++++-----------------------------------
>>  1 file changed, 48 insertions(+), 110 deletions(-)
>>
>> diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
>> index b1976fdd19..72dd129a15 100644
>> --- a/hw/s390x/virtio-ccw.c
>> +++ b/hw/s390x/virtio-ccw.c
>> @@ -287,49 +287,19 @@ static int virtio_ccw_handle_set_vq(SubchDev *sch, CCW1 ccw, bool check_len,
>>          return -EFAULT;
>>      }
>>      if (is_legacy) {
>> -        linfo.queue = address_space_ldq_be(&address_space_memory, ccw.cda,
>> -                                           MEMTXATTRS_UNSPECIFIED, NULL);
>> -        linfo.align = address_space_ldl_be(&address_space_memory,
>> -                                           ccw.cda + sizeof(linfo.queue),
>> -                                           MEMTXATTRS_UNSPECIFIED,
>> -                                           NULL);
>> -        linfo.index = address_space_lduw_be(&address_space_memory,
>> -                                            ccw.cda + sizeof(linfo.queue)
>> -                                            + sizeof(linfo.align),
>> -                                            MEMTXATTRS_UNSPECIFIED,
>> -                                            NULL);
>> -        linfo.num = address_space_lduw_be(&address_space_memory,
>> -                                          ccw.cda + sizeof(linfo.queue)
>> -                                          + sizeof(linfo.align)
>> -                                          + sizeof(linfo.index),
>> -                                          MEMTXATTRS_UNSPECIFIED,
>> -                                          NULL);
>> +        ccw_dstream_read(&sch->cds, linfo);
>> +        be64_to_cpus(&linfo.queue);
>> +        be32_to_cpus(&linfo.align);
>> +        be16_to_cpus(&linfo.index);
>> +        be16_to_cpus(&linfo.num);
> 
> That's a very nice contraction :)

Thanks.

> 
>>          ret = virtio_ccw_set_vqs(sch, NULL, &linfo);
>>      } else {
>> -        info.desc = address_space_ldq_be(&address_space_memory, ccw.cda,
>> -                                           MEMTXATTRS_UNSPECIFIED, NULL);
>> -        info.index = address_space_lduw_be(&address_space_memory,
>> -                                           ccw.cda + sizeof(info.desc)
>> -                                           + sizeof(info.res0),
>> -                                           MEMTXATTRS_UNSPECIFIED, NULL);
>> -        info.num = address_space_lduw_be(&address_space_memory,
>> -                                         ccw.cda + sizeof(info.desc)
>> -                                         + sizeof(info.res0)
>> -                                         + sizeof(info.index),
>> -                                         MEMTXATTRS_UNSPECIFIED, NULL);
>> -        info.avail = address_space_ldq_be(&address_space_memory,
>> -                                          ccw.cda + sizeof(info.desc)
>> -                                          + sizeof(info.res0)
>> -                                          + sizeof(info.index)
>> -                                          + sizeof(info.num),
>> -                                          MEMTXATTRS_UNSPECIFIED, NULL);
>> -        info.used = address_space_ldq_be(&address_space_memory,
>> -                                         ccw.cda + sizeof(info.desc)
>> -                                         + sizeof(info.res0)
>> -                                         + sizeof(info.index)
>> -                                         + sizeof(info.num)
>> -                                         + sizeof(info.avail),
>> -                                         MEMTXATTRS_UNSPECIFIED, NULL);
>> +        ccw_dstream_read(&sch->cds, info);
>> +        be64_to_cpus(&info.desc);
>> +        be16_to_cpus(&info.index);
>> +        be16_to_cpus(&info.num);
>> +        be64_to_cpus(&info.avail);
>> +        be64_to_cpus(&info.used);
>>          ret = virtio_ccw_set_vqs(sch, &info, NULL);
>>      }
>>      sch->curr_status.scsw.count = 0;
> 
> (...)
> 
>> @@ -488,7 +446,8 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
>>          } else {
>>              virtio_bus_get_vdev_config(&dev->bus, vdev->config);
>>              /* XXX config space endianness */
> 
> Unrelated: That should be fine, I guess?
> 
>> -            cpu_physical_memory_write(ccw.cda, vdev->config, len);
>> +            /* TODO we may have made -EINVAL out of -EFAULT */
> 
> Eek.
> 

Actually I wanted to send a patch which gets rid of the -EFAULT case
in sch_handle_start_func_virtual.

IMHO a program check is more appropriate here. We did the EFAULT if
cpu_physical_memory_map failed, but we don't have that any more.

>> +            ccw_dstream_write_buf(&sch->cds, vdev->config, len);
>>              sch->curr_status.scsw.count = ccw.count - len;
>>              ret = 0;
>>          }
> 
> Looks fine in general.
> 

Thanks!


Re: [Qemu-devel] [PATCH 3/5] virtio-ccw: use ccw data stream
Posted by Cornelia Huck 6 years, 6 months ago
On Wed, 6 Sep 2017 14:49:52 +0200
Halil Pasic <pasic@linux.vnet.ibm.com> wrote:

> On 09/06/2017 02:42 PM, Cornelia Huck wrote:
> > On Tue,  5 Sep 2017 13:16:43 +0200
> > Halil Pasic <pasic@linux.vnet.ibm.com> wrote:

> >> @@ -488,7 +446,8 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
> >>          } else {
> >>              virtio_bus_get_vdev_config(&dev->bus, vdev->config);
> >>              /* XXX config space endianness */  
> > 
> > Unrelated: That should be fine, I guess?
> >   
> >> -            cpu_physical_memory_write(ccw.cda, vdev->config, len);
> >> +            /* TODO we may have made -EINVAL out of -EFAULT */  
> > 
> > Eek.
> >   
> 
> Actually I wanted to send a patch which gets rid of the -EFAULT case
> in sch_handle_start_func_virtual.
> 
> IMHO a program check is more appropriate here. We did the EFAULT if
> cpu_physical_memory_map failed, but we don't have that any more.

OK, such a patch would make this easier to figure out.

Re: [Qemu-devel] [PATCH 3/5] virtio-ccw: use ccw data stream
Posted by Halil Pasic 6 years, 6 months ago

On 09/06/2017 02:42 PM, Cornelia Huck wrote:
>> @@ -488,7 +446,8 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
>>          } else {
>>              virtio_bus_get_vdev_config(&dev->bus, vdev->config);
>>              /* XXX config space endianness */
> Unrelated: That should be fine, I guess?
> 

Would be pretty bad if it weren't. It's the responsibility of the
virtio device to give us the right endianness. We don't know the layout
so we could not fix it even if we had to. I guess this comment should be
removed.

Regards,
Halil


Re: [Qemu-devel] [PATCH 3/5] virtio-ccw: use ccw data stream
Posted by Cornelia Huck 6 years, 6 months ago
On Mon, 11 Sep 2017 20:14:59 +0200
Halil Pasic <pasic@linux.vnet.ibm.com> wrote:

> On 09/06/2017 02:42 PM, Cornelia Huck wrote:
> >> @@ -488,7 +446,8 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
> >>          } else {
> >>              virtio_bus_get_vdev_config(&dev->bus, vdev->config);
> >>              /* XXX config space endianness */  
> > Unrelated: That should be fine, I guess?
> >   
> 
> Would be pretty bad if it weren't. It's the responsibility of the
> virtio device to give us the right endianness. We don't know the layout
> so we could not fix it even if we had to. I guess this comment should be
> removed.

Yes, please do so.

Re: [Qemu-devel] [PATCH 3/5] virtio-ccw: use ccw data stream
Posted by Halil Pasic 6 years, 6 months ago

On 09/13/2017 11:58 AM, Cornelia Huck wrote:
> On Mon, 11 Sep 2017 20:14:59 +0200
> Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
> 
>> On 09/06/2017 02:42 PM, Cornelia Huck wrote:
>>>> @@ -488,7 +446,8 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
>>>>          } else {
>>>>              virtio_bus_get_vdev_config(&dev->bus, vdev->config);
>>>>              /* XXX config space endianness */  
>>> Unrelated: That should be fine, I guess?
>>>   
>>
>> Would be pretty bad if it weren't. It's the responsibility of the
>> virtio device to give us the right endianness. We don't know the layout
>> so we could not fix it even if we had to. I guess this comment should be
>> removed.
> 
> Yes, please do so.
> 

Will do as a stand-alone patch.


[Qemu-devel] [PATCH 4/5] s390x/css: support ccw IDA
Posted by Halil Pasic 6 years, 6 months ago
Let's add indirect data addressing support for our virtual channel
subsystem. This implementation does no prefetching, but steps
trough the idal as we go.

Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
---
 hw/s390x/css.c | 110 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 109 insertions(+), 1 deletion(-)

diff --git a/hw/s390x/css.c b/hw/s390x/css.c
index c1bc9944e6..9d8f9fd7ea 100644
--- a/hw/s390x/css.c
+++ b/hw/s390x/css.c
@@ -819,6 +819,114 @@ incr:
     return 0;
 }
 
+/* retuns values between 1 and bs, where bs is a power of 2 */
+static inline uint16_t ida_continuous_left(hwaddr cda, uint64_t bs)
+{
+    return bs - (cda & (bs - 1));
+}
+
+static inline uint64_t ccw_ida_block_size(uint8_t flags)
+{
+    return 1ULL << (((flags ^ CDS_F_C64) & (CDS_F_C64 | CDS_F_I2K)) ? 11 : 12);
+}
+
+static inline int ida_read_next_idaw(CcwDataStream *cds)
+{
+    union {uint64_t fmt2; uint32_t fmt1; } idaw;
+    bool is_fmt2 = cds->flags & CDS_F_C64;
+    int ret;
+    hwaddr idaw_addr;
+
+    if (is_fmt2) {
+        idaw_addr = cds->cda_orig + sizeof(idaw.fmt2) * cds->at_idaw;
+        if (idaw_addr & 0x07) {
+            return -EINVAL; /* chanel program check */
+        }
+        ret = address_space_rw(&address_space_memory, idaw_addr,
+                               MEMTXATTRS_UNSPECIFIED, (void *) &idaw.fmt2,
+                               sizeof(idaw.fmt2), false);
+        cds->cda = be64_to_cpu(idaw.fmt2);
+    } else {
+        idaw_addr = cds->cda_orig + sizeof(idaw.fmt1) * cds->at_idaw;
+        if (idaw_addr & 0x03) {
+            return -EINVAL; /* chanel program check */
+
+        }
+        ret = address_space_rw(&address_space_memory, idaw_addr,
+                               MEMTXATTRS_UNSPECIFIED, (void *) &idaw.fmt1,
+                               sizeof(idaw.fmt1), false);
+        cds->cda = be64_to_cpu(idaw.fmt1);
+    }
+    ++(cds->at_idaw);
+    if (ret != MEMTX_OK) {
+        /* assume inaccessible address */
+        return -EINVAL; /* chanel program check */
+
+    }
+    return 0;
+}
+
+static int ccw_dstream_rw_ida(CcwDataStream *cds, void *buff, int len,
+                              CcwDataStreamOp op)
+{
+    uint64_t bs = ccw_ida_block_size(cds->flags);
+    int ret = 0;
+    uint16_t cont_left, iter_len;
+
+    ret = cds_check_len(cds, len);
+    if (ret <= 0) {
+        return ret;
+    }
+    if (!cds->at_idaw) {
+        /* read first idaw */
+        ret = ida_read_next_idaw(cds);
+        if (ret) {
+            goto err;
+        }
+        cont_left = ida_continuous_left(cds->cda, bs);
+    } else {
+        cont_left = ida_continuous_left(cds->cda, bs);
+        if (cont_left == bs) {
+            ret = ida_read_next_idaw(cds);
+            if (ret) {
+                goto err;
+            }
+            if (cds->cda & (bs - 1)) {
+                ret = -EINVAL; /* chanel program check */
+                goto err;
+            }
+        }
+    }
+do_iter_len:
+    iter_len = MIN(len, cont_left);
+    if (op == CDS_OP_A) {
+        goto incr;
+    }
+    ret = address_space_rw(&address_space_memory, cds->cda,
+                           MEMTXATTRS_UNSPECIFIED, buff, iter_len, op);
+    if (ret != MEMTX_OK) {
+        /* assume inaccessible address */
+        ret = -EINVAL; /* chanel program check */
+        goto err;
+    }
+incr:
+    cds->at_byte += iter_len;
+    cds->cda += iter_len;
+    len -= iter_len;
+    if (len) {
+        ret = ida_read_next_idaw(cds);
+        if (ret) {
+            goto err;
+        }
+        cont_left = bs;
+        goto do_iter_len;
+    }
+    return ret;
+err:
+    cds->flags |= CDS_F_STREAM_BROKEN;
+    return ret;
+}
+
 void ccw_dstream_init(CcwDataStream *cds, CCW1 const *ccw, ORB const *orb)
 {
     /*
@@ -835,7 +943,7 @@ void ccw_dstream_init(CcwDataStream *cds, CCW1 const *ccw, ORB const *orb)
     if (!(cds->flags & CDS_F_IDA)) {
         cds->op_handler = ccw_dstream_rw_noflags;
     } else {
-        assert(false);
+        cds->op_handler = ccw_dstream_rw_ida;
     }
 }
 
-- 
2.13.5


Re: [Qemu-devel] [PATCH 4/5] s390x/css: support ccw IDA
Posted by Cornelia Huck 6 years, 6 months ago
On Tue,  5 Sep 2017 13:16:44 +0200
Halil Pasic <pasic@linux.vnet.ibm.com> wrote:

> Let's add indirect data addressing support for our virtual channel
> subsystem. This implementation does no prefetching, but steps
> trough the idal as we go.
> 
> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
> ---
>  hw/s390x/css.c | 110 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 109 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/s390x/css.c b/hw/s390x/css.c
> index c1bc9944e6..9d8f9fd7ea 100644
> --- a/hw/s390x/css.c
> +++ b/hw/s390x/css.c
> @@ -819,6 +819,114 @@ incr:
>      return 0;
>  }
>  
> +/* retuns values between 1 and bs, where bs is a power of 2 */

'bs' is 'block size'?

> +static inline uint16_t ida_continuous_left(hwaddr cda, uint64_t bs)
> +{
> +    return bs - (cda & (bs - 1));
> +}
> +
> +static inline uint64_t ccw_ida_block_size(uint8_t flags)
> +{
> +    return 1ULL << (((flags ^ CDS_F_C64) & (CDS_F_C64 | CDS_F_I2K)) ? 11 : 12);
> +}
> +
> +static inline int ida_read_next_idaw(CcwDataStream *cds)
> +{
> +    union {uint64_t fmt2; uint32_t fmt1; } idaw;
> +    bool is_fmt2 = cds->flags & CDS_F_C64;
> +    int ret;
> +    hwaddr idaw_addr;
> +
> +    if (is_fmt2) {
> +        idaw_addr = cds->cda_orig + sizeof(idaw.fmt2) * cds->at_idaw;
> +        if (idaw_addr & 0x07) {
> +            return -EINVAL; /* chanel program check */

How fashionable ;)

[s/chanel/channel/ :D]

> +        }
> +        ret = address_space_rw(&address_space_memory, idaw_addr,
> +                               MEMTXATTRS_UNSPECIFIED, (void *) &idaw.fmt2,
> +                               sizeof(idaw.fmt2), false);
> +        cds->cda = be64_to_cpu(idaw.fmt2);
> +    } else {
> +        idaw_addr = cds->cda_orig + sizeof(idaw.fmt1) * cds->at_idaw;
> +        if (idaw_addr & 0x03) {
> +            return -EINVAL; /* chanel program check */
> +
> +        }
> +        ret = address_space_rw(&address_space_memory, idaw_addr,
> +                               MEMTXATTRS_UNSPECIFIED, (void *) &idaw.fmt1,
> +                               sizeof(idaw.fmt1), false);
> +        cds->cda = be64_to_cpu(idaw.fmt1);
> +    }
> +    ++(cds->at_idaw);
> +    if (ret != MEMTX_OK) {
> +        /* assume inaccessible address */
> +        return -EINVAL; /* chanel program check */
> +
> +    }
> +    return 0;
> +}
> +
> +static int ccw_dstream_rw_ida(CcwDataStream *cds, void *buff, int len,
> +                              CcwDataStreamOp op)
> +{
> +    uint64_t bs = ccw_ida_block_size(cds->flags);
> +    int ret = 0;
> +    uint16_t cont_left, iter_len;
> +
> +    ret = cds_check_len(cds, len);
> +    if (ret <= 0) {
> +        return ret;
> +    }
> +    if (!cds->at_idaw) {
> +        /* read first idaw */
> +        ret = ida_read_next_idaw(cds);
> +        if (ret) {
> +            goto err;
> +        }
> +        cont_left = ida_continuous_left(cds->cda, bs);
> +    } else {
> +        cont_left = ida_continuous_left(cds->cda, bs);
> +        if (cont_left == bs) {
> +            ret = ida_read_next_idaw(cds);
> +            if (ret) {
> +                goto err;
> +            }
> +            if (cds->cda & (bs - 1)) {
> +                ret = -EINVAL; /* chanel program check */
> +                goto err;
> +            }
> +        }
> +    }
> +do_iter_len:
> +    iter_len = MIN(len, cont_left);
> +    if (op == CDS_OP_A) {
> +        goto incr;
> +    }
> +    ret = address_space_rw(&address_space_memory, cds->cda,
> +                           MEMTXATTRS_UNSPECIFIED, buff, iter_len, op);
> +    if (ret != MEMTX_OK) {
> +        /* assume inaccessible address */
> +        ret = -EINVAL; /* chanel program check */
> +        goto err;
> +    }
> +incr:
> +    cds->at_byte += iter_len;
> +    cds->cda += iter_len;
> +    len -= iter_len;
> +    if (len) {
> +        ret = ida_read_next_idaw(cds);
> +        if (ret) {
> +            goto err;
> +        }
> +        cont_left = bs;
> +        goto do_iter_len;
> +    }
> +    return ret;
> +err:
> +    cds->flags |= CDS_F_STREAM_BROKEN;
> +    return ret;
> +}

I'm not so happy about the many gotos (excepting the err ones). Any
chance to get around these?

> +
>  void ccw_dstream_init(CcwDataStream *cds, CCW1 const *ccw, ORB const *orb)
>  {
>      /*
> @@ -835,7 +943,7 @@ void ccw_dstream_init(CcwDataStream *cds, CCW1 const *ccw, ORB const *orb)
>      if (!(cds->flags & CDS_F_IDA)) {
>          cds->op_handler = ccw_dstream_rw_noflags;
>      } else {
> -        assert(false);
> +        cds->op_handler = ccw_dstream_rw_ida;
>      }
>  }
>  


Re: [Qemu-devel] [PATCH 4/5] s390x/css: support ccw IDA
Posted by Halil Pasic 6 years, 6 months ago

On 09/06/2017 03:10 PM, Cornelia Huck wrote:
> On Tue,  5 Sep 2017 13:16:44 +0200
> Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
> 
>> Let's add indirect data addressing support for our virtual channel
>> subsystem. This implementation does no prefetching, but steps
>> trough the idal as we go.
>>
>> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
>> ---
>>  hw/s390x/css.c | 110 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>>  1 file changed, 109 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/s390x/css.c b/hw/s390x/css.c
>> index c1bc9944e6..9d8f9fd7ea 100644
>> --- a/hw/s390x/css.c
>> +++ b/hw/s390x/css.c
>> @@ -819,6 +819,114 @@ incr:
>>      return 0;
>>  }
>>  
>> +/* retuns values between 1 and bs, where bs is a power of 2 */
> 
> 'bs' is 'block size'?

Yes.

> 
>> +static inline uint16_t ida_continuous_left(hwaddr cda, uint64_t bs)
>> +{
>> +    return bs - (cda & (bs - 1));
>> +}
>> +
>> +static inline uint64_t ccw_ida_block_size(uint8_t flags)
>> +{
>> +    return 1ULL << (((flags ^ CDS_F_C64) & (CDS_F_C64 | CDS_F_I2K)) ? 11 : 12);
>> +}
>> +
>> +static inline int ida_read_next_idaw(CcwDataStream *cds)
>> +{
>> +    union {uint64_t fmt2; uint32_t fmt1; } idaw;
>> +    bool is_fmt2 = cds->flags & CDS_F_C64;
>> +    int ret;
>> +    hwaddr idaw_addr;
>> +
>> +    if (is_fmt2) {
>> +        idaw_addr = cds->cda_orig + sizeof(idaw.fmt2) * cds->at_idaw;
>> +        if (idaw_addr & 0x07) {
>> +            return -EINVAL; /* chanel program check */
> 
> How fashionable ;)
> 

Learned something new.

> [s/chanel/channel/ :D]
> 
>> +        }
>> +        ret = address_space_rw(&address_space_memory, idaw_addr,
>> +                               MEMTXATTRS_UNSPECIFIED, (void *) &idaw.fmt2,
>> +                               sizeof(idaw.fmt2), false);
>> +        cds->cda = be64_to_cpu(idaw.fmt2);
>> +    } else {
>> +        idaw_addr = cds->cda_orig + sizeof(idaw.fmt1) * cds->at_idaw;
>> +        if (idaw_addr & 0x03) {
>> +            return -EINVAL; /* chanel program check */
>> +
>> +        }
>> +        ret = address_space_rw(&address_space_memory, idaw_addr,
>> +                               MEMTXATTRS_UNSPECIFIED, (void *) &idaw.fmt1,
>> +                               sizeof(idaw.fmt1), false);
>> +        cds->cda = be64_to_cpu(idaw.fmt1);
>> +    }
>> +    ++(cds->at_idaw);
>> +    if (ret != MEMTX_OK) {
>> +        /* assume inaccessible address */
>> +        return -EINVAL; /* chanel program check */
>> +
>> +    }
>> +    return 0;
>> +}
>> +
>> +static int ccw_dstream_rw_ida(CcwDataStream *cds, void *buff, int len,
>> +                              CcwDataStreamOp op)
>> +{
>> +    uint64_t bs = ccw_ida_block_size(cds->flags);
>> +    int ret = 0;
>> +    uint16_t cont_left, iter_len;
>> +
>> +    ret = cds_check_len(cds, len);
>> +    if (ret <= 0) {
>> +        return ret;
>> +    }
>> +    if (!cds->at_idaw) {
>> +        /* read first idaw */
>> +        ret = ida_read_next_idaw(cds);
>> +        if (ret) {
>> +            goto err;
>> +        }
>> +        cont_left = ida_continuous_left(cds->cda, bs);
>> +    } else {
>> +        cont_left = ida_continuous_left(cds->cda, bs);
>> +        if (cont_left == bs) {
>> +            ret = ida_read_next_idaw(cds);
>> +            if (ret) {
>> +                goto err;
>> +            }
>> +            if (cds->cda & (bs - 1)) {
>> +                ret = -EINVAL; /* chanel program check */
>> +                goto err;
>> +            }
>> +        }
>> +    }
>> +do_iter_len:
>> +    iter_len = MIN(len, cont_left);
>> +    if (op == CDS_OP_A) {
>> +        goto incr;
>> +    }
>> +    ret = address_space_rw(&address_space_memory, cds->cda,
>> +                           MEMTXATTRS_UNSPECIFIED, buff, iter_len, op);
>> +    if (ret != MEMTX_OK) {
>> +        /* assume inaccessible address */
>> +        ret = -EINVAL; /* chanel program check */
>> +        goto err;
>> +    }
>> +incr:
>> +    cds->at_byte += iter_len;
>> +    cds->cda += iter_len;
>> +    len -= iter_len;
>> +    if (len) {
>> +        ret = ida_read_next_idaw(cds);
>> +        if (ret) {
>> +            goto err;
>> +        }
>> +        cont_left = bs;
>> +        goto do_iter_len;
>> +    }
>> +    return ret;
>> +err:
>> +    cds->flags |= CDS_F_STREAM_BROKEN;
>> +    return ret;
>> +}
> 
> I'm not so happy about the many gotos (excepting the err ones). Any
> chance to get around these?
> 
Certainly possible:

static int ccw_dstream_rw_ida(CcwDataStream *cds, void *buff, int len,          
                              CcwDataStreamOp op)                               
{                                                                               
    uint64_t bs = ccw_ida_block_size(cds->flags);                               
    int ret = 0;                                                                
    uint16_t cont_left, iter_len;                                               
                                                                                
    ret = cds_check_len(cds, len);                                              
    if (ret <= 0) {                                                             
        return ret;                                                             
    }                                                                           
    if (!cds->at_idaw) {                                                        
        /* read first idaw */                                                   
        ret = ida_read_next_idaw(cds);                                          
        if (ret) {                                                              
            goto err;                                                           
        }                                                                       
        cont_left = ida_continuous_left(cds->cda, bs);                          
    } else {                                                                    
        cont_left = ida_continuous_left(cds->cda, bs);                          
        if (cont_left == bs) {                                                  
            ret = ida_read_next_idaw(cds);                                      
            if (ret) {                                                          
                goto err;                                                       
            }                                                                   
            if (cds->cda & (bs - 1)) {                                          
                ret = -EINVAL; /* channel program check */                      
                goto err;                                                       
            }                                                                   
        }                                                                       
    }                                                                           
    do {                                                                        
        iter_len = MIN(len, cont_left);                                         
        if (op != CDS_OP_A) {                                                   
            ret = address_space_rw(&address_space_memory, cds->cda,             
                                   MEMTXATTRS_UNSPECIFIED, buff, iter_len, op); 
            if (ret != MEMTX_OK) {                                              
                /* assume inaccessible address */                               
                ret = -EINVAL; /* channel program check */                      
                goto err;                                                       
            }                                                                   
        }                                                                       
        cds->at_byte += iter_len;                                               
        cds->cda += iter_len;                                                   
        len -= iter_len;                                                        
        if (!len) {                                                             
            break;                                                              
        }                                                                       
        ret = ida_read_next_idaw(cds);                                          
        if (ret) {                                                              
            goto err;                                                           
        }                                                                       
        cont_left = bs;                                                         
    } while (true);                                                             
    return ret;                                                                 
err:                                                                            
    cds->flags |= CDS_F_STREAM_BROKEN;                                          
    return ret;                                                                 
}

My idea was decrease indentation level and at the same time make
labels tell us something about the purpose of code pieces. Which
one do you prefer?

Sorry I've forgot about this comment. I would not have negotiated
v2 before clarifying this.

Halil

>> +
>>  void ccw_dstream_init(CcwDataStream *cds, CCW1 const *ccw, ORB const *orb)
>>  {
>>      /*
>> @@ -835,7 +943,7 @@ void ccw_dstream_init(CcwDataStream *cds, CCW1 const *ccw, ORB const *orb)
>>      if (!(cds->flags & CDS_F_IDA)) {
>>          cds->op_handler = ccw_dstream_rw_noflags;
>>      } else {
>> -        assert(false);
>> +        cds->op_handler = ccw_dstream_rw_ida;
>>      }
>>  }
>>  
> 
> 


Re: [Qemu-devel] [PATCH 4/5] s390x/css: support ccw IDA
Posted by Cornelia Huck 6 years, 6 months ago
On Mon, 11 Sep 2017 20:08:16 +0200
Halil Pasic <pasic@linux.vnet.ibm.com> wrote:

> On 09/06/2017 03:10 PM, Cornelia Huck wrote:
> > On Tue,  5 Sep 2017 13:16:44 +0200
> > Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
> >   
> >> Let's add indirect data addressing support for our virtual channel
> >> subsystem. This implementation does no prefetching, but steps
> >> trough the idal as we go.
> >>
> >> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
> >> ---
> >>  hw/s390x/css.c | 110 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
> >>  1 file changed, 109 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/hw/s390x/css.c b/hw/s390x/css.c
> >> index c1bc9944e6..9d8f9fd7ea 100644
> >> --- a/hw/s390x/css.c
> >> +++ b/hw/s390x/css.c
> >> @@ -819,6 +819,114 @@ incr:
> >>      return 0;
> >>  }
> >>  
> >> +/* retuns values between 1 and bs, where bs is a power of 2 */  
> > 
> > 'bs' is 'block size'?  
> 
> Yes.

The first thing that popped up in my head was something else, that's
why I asked :) Maybe bsz would be better.

In any case, s/retuns/returns/

> 
> >   
> >> +static inline uint16_t ida_continuous_left(hwaddr cda, uint64_t bs)
> >> +{
> >> +    return bs - (cda & (bs - 1));
> >> +}

> >> +static int ccw_dstream_rw_ida(CcwDataStream *cds, void *buff, int len,
> >> +                              CcwDataStreamOp op)
> >> +{
> >> +    uint64_t bs = ccw_ida_block_size(cds->flags);
> >> +    int ret = 0;
> >> +    uint16_t cont_left, iter_len;
> >> +
> >> +    ret = cds_check_len(cds, len);
> >> +    if (ret <= 0) {
> >> +        return ret;
> >> +    }
> >> +    if (!cds->at_idaw) {
> >> +        /* read first idaw */
> >> +        ret = ida_read_next_idaw(cds);
> >> +        if (ret) {
> >> +            goto err;
> >> +        }
> >> +        cont_left = ida_continuous_left(cds->cda, bs);
> >> +    } else {
> >> +        cont_left = ida_continuous_left(cds->cda, bs);
> >> +        if (cont_left == bs) {
> >> +            ret = ida_read_next_idaw(cds);
> >> +            if (ret) {
> >> +                goto err;
> >> +            }
> >> +            if (cds->cda & (bs - 1)) {
> >> +                ret = -EINVAL; /* chanel program check */
> >> +                goto err;
> >> +            }
> >> +        }
> >> +    }
> >> +do_iter_len:
> >> +    iter_len = MIN(len, cont_left);
> >> +    if (op == CDS_OP_A) {
> >> +        goto incr;
> >> +    }
> >> +    ret = address_space_rw(&address_space_memory, cds->cda,
> >> +                           MEMTXATTRS_UNSPECIFIED, buff, iter_len, op);
> >> +    if (ret != MEMTX_OK) {
> >> +        /* assume inaccessible address */
> >> +        ret = -EINVAL; /* chanel program check */
> >> +        goto err;
> >> +    }
> >> +incr:
> >> +    cds->at_byte += iter_len;
> >> +    cds->cda += iter_len;
> >> +    len -= iter_len;
> >> +    if (len) {
> >> +        ret = ida_read_next_idaw(cds);
> >> +        if (ret) {
> >> +            goto err;
> >> +        }
> >> +        cont_left = bs;
> >> +        goto do_iter_len;
> >> +    }
> >> +    return ret;
> >> +err:
> >> +    cds->flags |= CDS_F_STREAM_BROKEN;
> >> +    return ret;
> >> +}  
> > 
> > I'm not so happy about the many gotos (excepting the err ones). Any
> > chance to get around these?
> >   
> Certainly possible:
> 
> static int ccw_dstream_rw_ida(CcwDataStream *cds, void *buff, int len,          
>                               CcwDataStreamOp op)                               
> {                                                                               
>     uint64_t bs = ccw_ida_block_size(cds->flags);                               
>     int ret = 0;                                                                
>     uint16_t cont_left, iter_len;                                               
>                                                                                 
>     ret = cds_check_len(cds, len);                                              
>     if (ret <= 0) {                                                             
>         return ret;                                                             
>     }                                                                           
>     if (!cds->at_idaw) {                                                        
>         /* read first idaw */                                                   
>         ret = ida_read_next_idaw(cds);                                          
>         if (ret) {                                                              
>             goto err;                                                           
>         }                                                                       
>         cont_left = ida_continuous_left(cds->cda, bs);                          
>     } else {                                                                    
>         cont_left = ida_continuous_left(cds->cda, bs);                          
>         if (cont_left == bs) {                                                  
>             ret = ida_read_next_idaw(cds);                                      
>             if (ret) {                                                          
>                 goto err;                                                       
>             }                                                                   
>             if (cds->cda & (bs - 1)) {                                          
>                 ret = -EINVAL; /* channel program check */                      
>                 goto err;                                                       
>             }                                                                   
>         }                                                                       
>     }                                                                           
>     do {                                                                        
>         iter_len = MIN(len, cont_left);                                         
>         if (op != CDS_OP_A) {                                                   
>             ret = address_space_rw(&address_space_memory, cds->cda,             
>                                    MEMTXATTRS_UNSPECIFIED, buff, iter_len, op); 
>             if (ret != MEMTX_OK) {                                              
>                 /* assume inaccessible address */                               
>                 ret = -EINVAL; /* channel program check */                      
>                 goto err;                                                       
>             }                                                                   
>         }                                                                       
>         cds->at_byte += iter_len;                                               
>         cds->cda += iter_len;                                                   
>         len -= iter_len;                                                        
>         if (!len) {                                                             
>             break;                                                              
>         }                                                                       
>         ret = ida_read_next_idaw(cds);                                          
>         if (ret) {                                                              
>             goto err;                                                           
>         }                                                                       
>         cont_left = bs;                                                         
>     } while (true);                                                             
>     return ret;                                                                 
> err:                                                                            
>     cds->flags |= CDS_F_STREAM_BROKEN;                                          
>     return ret;                                                                 
> }
> 
> My idea was decrease indentation level and at the same time make
> labels tell us something about the purpose of code pieces. Which
> one do you prefer?

I do not really like either much :( Any chance to make this better via
some kind of helper functions?

Re: [Qemu-devel] [PATCH 4/5] s390x/css: support ccw IDA
Posted by Halil Pasic 6 years, 6 months ago

On 09/13/2017 11:58 AM, Cornelia Huck wrote:
> On Mon, 11 Sep 2017 20:08:16 +0200
> Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
> 
>> On 09/06/2017 03:10 PM, Cornelia Huck wrote:
>>> On Tue,  5 Sep 2017 13:16:44 +0200
>>> Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
>>>   
>>>> Let's add indirect data addressing support for our virtual channel
>>>> subsystem. This implementation does no prefetching, but steps
>>>> trough the idal as we go.
>>>>
>>>> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
>>>> ---
>>>>  hw/s390x/css.c | 110 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>>>>  1 file changed, 109 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/hw/s390x/css.c b/hw/s390x/css.c
>>>> index c1bc9944e6..9d8f9fd7ea 100644
>>>> --- a/hw/s390x/css.c
>>>> +++ b/hw/s390x/css.c
>>>> @@ -819,6 +819,114 @@ incr:
>>>>      return 0;
>>>>  }
>>>>  
>>>> +/* retuns values between 1 and bs, where bs is a power of 2 */  
>>>
>>> 'bs' is 'block size'?  
>>
>> Yes.
> 
> The first thing that popped up in my head was something else, that's
> why I asked :) Maybe bsz would be better.
> 

Will change to bsz.

> In any case, s/retuns/returns/
> 

I think I've already fixed that :).

>>
>>>   
>>>> +static inline uint16_t ida_continuous_left(hwaddr cda, uint64_t bs)
>>>> +{
>>>> +    return bs - (cda & (bs - 1));
>>>> +}
> 
>>>> +static int ccw_dstream_rw_ida(CcwDataStream *cds, void *buff, int len,
>>>> +                              CcwDataStreamOp op)
>>>> +{
>>>> +    uint64_t bs = ccw_ida_block_size(cds->flags);
>>>> +    int ret = 0;
>>>> +    uint16_t cont_left, iter_len;
>>>> +
>>>> +    ret = cds_check_len(cds, len);
>>>> +    if (ret <= 0) {
>>>> +        return ret;
>>>> +    }
>>>> +    if (!cds->at_idaw) {
>>>> +        /* read first idaw */
>>>> +        ret = ida_read_next_idaw(cds);
>>>> +        if (ret) {
>>>> +            goto err;
>>>> +        }
>>>> +        cont_left = ida_continuous_left(cds->cda, bs);
>>>> +    } else {
>>>> +        cont_left = ida_continuous_left(cds->cda, bs);
>>>> +        if (cont_left == bs) {
>>>> +            ret = ida_read_next_idaw(cds);
>>>> +            if (ret) {
>>>> +                goto err;
>>>> +            }
>>>> +            if (cds->cda & (bs - 1)) {
>>>> +                ret = -EINVAL; /* chanel program check */
>>>> +                goto err;
>>>> +            }
>>>> +        }
>>>> +    }
>>>> +do_iter_len:
>>>> +    iter_len = MIN(len, cont_left);
>>>> +    if (op == CDS_OP_A) {
>>>> +        goto incr;
>>>> +    }
>>>> +    ret = address_space_rw(&address_space_memory, cds->cda,
>>>> +                           MEMTXATTRS_UNSPECIFIED, buff, iter_len, op);
>>>> +    if (ret != MEMTX_OK) {
>>>> +        /* assume inaccessible address */
>>>> +        ret = -EINVAL; /* chanel program check */
>>>> +        goto err;
>>>> +    }
>>>> +incr:
>>>> +    cds->at_byte += iter_len;
>>>> +    cds->cda += iter_len;
>>>> +    len -= iter_len;
>>>> +    if (len) {
>>>> +        ret = ida_read_next_idaw(cds);
>>>> +        if (ret) {
>>>> +            goto err;
>>>> +        }
>>>> +        cont_left = bs;
>>>> +        goto do_iter_len;
>>>> +    }
>>>> +    return ret;
>>>> +err:
>>>> +    cds->flags |= CDS_F_STREAM_BROKEN;
>>>> +    return ret;
>>>> +}  
>>>
>>> I'm not so happy about the many gotos (excepting the err ones). Any
>>> chance to get around these?
>>>   
>> Certainly possible:
>>
>> static int ccw_dstream_rw_ida(CcwDataStream *cds, void *buff, int len,          
>>                               CcwDataStreamOp op)                               
>> {                                                                               
>>     uint64_t bs = ccw_ida_block_size(cds->flags);                               
>>     int ret = 0;                                                                
>>     uint16_t cont_left, iter_len;                                               
>>                                                                                 
>>     ret = cds_check_len(cds, len);                                              
>>     if (ret <= 0) {                                                             
>>         return ret;                                                             
>>     }                                                                           
>>     if (!cds->at_idaw) {                                                        
>>         /* read first idaw */                                                   
>>         ret = ida_read_next_idaw(cds);                                          
>>         if (ret) {                                                              
>>             goto err;                                                           
>>         }                                                                       
>>         cont_left = ida_continuous_left(cds->cda, bs);                          
>>     } else {                                                                    
>>         cont_left = ida_continuous_left(cds->cda, bs);                          
>>         if (cont_left == bs) {                                                  
>>             ret = ida_read_next_idaw(cds);                                      
>>             if (ret) {                                                          
>>                 goto err;                                                       
>>             }                                                                   
>>             if (cds->cda & (bs - 1)) {                                          
>>                 ret = -EINVAL; /* channel program check */                      
>>                 goto err;                                                       
>>             }                                                                   
>>         }                                                                       
>>     }                                                                           
>>     do {                                                                        
>>         iter_len = MIN(len, cont_left);                                         
>>         if (op != CDS_OP_A) {                                                   
>>             ret = address_space_rw(&address_space_memory, cds->cda,             
>>                                    MEMTXATTRS_UNSPECIFIED, buff, iter_len, op); 
>>             if (ret != MEMTX_OK) {                                              
>>                 /* assume inaccessible address */                               
>>                 ret = -EINVAL; /* channel program check */                      
>>                 goto err;                                                       
>>             }                                                                   
>>         }                                                                       
>>         cds->at_byte += iter_len;                                               
>>         cds->cda += iter_len;                                                   
>>         len -= iter_len;                                                        
>>         if (!len) {                                                             
>>             break;                                                              
>>         }                                                                       
>>         ret = ida_read_next_idaw(cds);                                          
>>         if (ret) {                                                              
>>             goto err;                                                           
>>         }                                                                       
>>         cont_left = bs;                                                         
>>     } while (true);                                                             
>>     return ret;                                                                 
>> err:                                                                            
>>     cds->flags |= CDS_F_STREAM_BROKEN;                                          
>>     return ret;                                                                 
>> }
>>
>> My idea was decrease indentation level and at the same time make
>> labels tell us something about the purpose of code pieces. Which
>> one do you prefer?
> 
> I do not really like either much :( Any chance to make this better via
> some kind of helper functions?
> 

I already use helper functions (ida_read_next_idaw, ida_continuous_left,
ccw_ida_block_size, cds_check_len). The function is flow control and
error handling heavy. I have no further ideas for helper functions,
or other simplifications.

IMHO if there are no non-aesthetics issues we can defer the non-trivial
aesthetics issues until somebody/anybody has an idea and the time to
sort them out.

I think, in absence of further complaints or concrete ideas I will go
with the more structured variant (second one) for v2. I would like
to post a v2 soon to avoid juggling to much stuff in head.

Regards,
Halil


Re: [Qemu-devel] [PATCH 4/5] s390x/css: support ccw IDA
Posted by Cornelia Huck 6 years, 6 months ago
On Wed, 13 Sep 2017 12:31:20 +0200
Halil Pasic <pasic@linux.vnet.ibm.com> wrote:

> I think, in absence of further complaints or concrete ideas I will go
> with the more structured variant (second one) for v2. I would like
> to post a v2 soon to avoid juggling to much stuff in head.

Yeah, let's see v2. Maybe someone else can chime in as well.

[Qemu-devel] [PATCH 5/5] s390x/ccs: add ccw-tester emulated device
Posted by Halil Pasic 6 years, 6 months ago
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>
---

It may not make sense to merge this work in the current form, as it is
solely for test purposes.
---
 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 b2aade2466..44a07431da 100644
--- a/hw/s390x/Makefile.objs
+++ b/hw/s390x/Makefile.objs
@@ -17,3 +17,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


Re: [Qemu-devel] [PATCH 5/5] s390x/ccs: add ccw-tester emulated device
Posted by Cornelia Huck 6 years, 6 months ago
On Tue,  5 Sep 2017 13:16:45 +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>
> ---
> 
> It may not make sense to merge this work in the current form, as it is
> solely for test purposes.
> ---
>  hw/s390x/Makefile.objs |   1 +
>  hw/s390x/ccw-tester.c  | 179 +++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 180 insertions(+)
>  create mode 100644 hw/s390x/ccw-tester.c

The main problem here is that you want to exercise a middle layer (the
css code) and need to write boilerplate code on both host and guest
side in order to be able to do so.

In general, a device that accepts arbitrary channel programs looks
useful for testing purposes. I would split out processing of expected
responses out, though, so that it can be more easily reused for
different use cases.

(I dimly recall other test devices...)

For the guest tester: Can that be done via the qtest infrastructure
somehow?

Re: [Qemu-devel] [PATCH 5/5] s390x/ccs: add ccw-tester emulated device
Posted by Halil Pasic 6 years, 6 months ago

On 09/06/2017 03:18 PM, Cornelia Huck wrote:
> On Tue,  5 Sep 2017 13:16:45 +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>
>> ---
>>
>> It may not make sense to merge this work in the current form, as it is
>> solely for test purposes.
>> ---
>>  hw/s390x/Makefile.objs |   1 +
>>  hw/s390x/ccw-tester.c  | 179 +++++++++++++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 180 insertions(+)
>>  create mode 100644 hw/s390x/ccw-tester.c
> 
> The main problem here is that you want to exercise a middle layer (the
> css code) and need to write boilerplate code on both host and guest
> side in order to be able to do so.
> 

Nod.

> In general, a device that accepts arbitrary channel programs looks
> useful for testing purposes. I would split out processing of expected
> responses out, though, so that it can be more easily reused for
> different use cases.
> 

I'm not sure what do you mean here. Could you clarify please?

> (I dimly recall other test devices...)
> 

What's on your mind? How do these relate to our problem? Can you
give me some pointers?

> For the guest tester: Can that be done via the qtest infrastructure
> somehow?
> 

Well, for now I have the out-of-tree Linux kernel module provided in the
cover letter of the series (you did not comment on that one yet).

I think for building trust regarding my IDA implementation it should be
able to do the job. Don't you agree?

Just a couple of hours ago Janosch (cc-ing Janosch) came to my office,
and told be that he is working on CCW-tests for zonk (a minimal kernel
for testing -- internal tool AFAIR).

By qtest you mean libqtest/libqos? I'm not familiar with that and have no
idea what do we have for s390x. I see on libqos-s390x file in test/libqos
for starters.

Regards,
Halil


Re: [Qemu-devel] [PATCH 5/5] s390x/ccs: add ccw-tester emulated device
Posted by Cornelia Huck 6 years, 6 months ago
On Wed, 6 Sep 2017 16:24:13 +0200
Halil Pasic <pasic@linux.vnet.ibm.com> wrote:

> On 09/06/2017 03:18 PM, Cornelia Huck wrote:
> > On Tue,  5 Sep 2017 13:16:45 +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>
> >> ---
> >>
> >> It may not make sense to merge this work in the current form, as it is
> >> solely for test purposes.
> >> ---
> >>  hw/s390x/Makefile.objs |   1 +
> >>  hw/s390x/ccw-tester.c  | 179 +++++++++++++++++++++++++++++++++++++++++++++++++
> >>  2 files changed, 180 insertions(+)
> >>  create mode 100644 hw/s390x/ccw-tester.c  
> > 
> > The main problem here is that you want to exercise a middle layer (the
> > css code) and need to write boilerplate code on both host and guest
> > side in order to be able to do so.
> >   
> 
> Nod.
> 
> > In general, a device that accepts arbitrary channel programs looks
> > useful for testing purposes. I would split out processing of expected
> > responses out, though, so that it can be more easily reused for
> > different use cases.
> >   
> 
> I'm not sure what do you mean here. Could you clarify please?

Maybe doing things like logging received ccws and responses etc. and
and then comparing against an expected outcome. You should be able to
write test cases for different sets of things to be tested. I know this
is awfully vague :)

> 
> > (I dimly recall other test devices...)
> >   
> 
> What's on your mind? How do these relate to our problem? Can you
> give me some pointers?

I was thinking of precedent for test devices... but as said, I only
recall this very dimly.

> 
> > For the guest tester: Can that be done via the qtest infrastructure
> > somehow?
> >   
> 
> Well, for now I have the out-of-tree Linux kernel module provided in the
> cover letter of the series (you did not comment on that one yet).

Yes, I saw that, but have not yet looked at it. If there is a way to do
it without too many extra parts, I'd prefer that.

> 
> I think for building trust regarding my IDA implementation it should be
> able to do the job. Don't you agree?

There's nothing wrong with your test. But we really should try to move
to some kind of unified testing that is hopefully self-contained so
that interested people can just play with it within the context of qemu.

> 
> Just a couple of hours ago Janosch (cc-ing Janosch) came to my office,
> and told be that he is working on CCW-tests for zonk (a minimal kernel
> for testing -- internal tool AFAIR).
> 
> By qtest you mean libqtest/libqos? I'm not familiar with that and have no
> idea what do we have for s390x. I see on libqos-s390x file in test/libqos
> for starters.

Yes, that was what I was thinking about. Integrating into what is
already there is what we should aim for.

[For things that are done via kvm, kvm unit tests are good. But I think
we can do css tests completely within qemu.]

Re: [Qemu-devel] [PATCH 5/5] s390x/ccs: add ccw-tester emulated device
Posted by Halil Pasic 6 years, 6 months ago

On 09/06/2017 05:20 PM, Cornelia Huck wrote:
> On Wed, 6 Sep 2017 16:24:13 +0200
> Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
> 
>> On 09/06/2017 03:18 PM, Cornelia Huck wrote:
>>> On Tue,  5 Sep 2017 13:16:45 +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>
>>>> ---
>>>>
>>>> It may not make sense to merge this work in the current form, as it is
>>>> solely for test purposes.
>>>> ---
>>>>  hw/s390x/Makefile.objs |   1 +
>>>>  hw/s390x/ccw-tester.c  | 179 +++++++++++++++++++++++++++++++++++++++++++++++++
>>>>  2 files changed, 180 insertions(+)
>>>>  create mode 100644 hw/s390x/ccw-tester.c  
>>>
>>> The main problem here is that you want to exercise a middle layer (the
>>> css code) and need to write boilerplate code on both host and guest
>>> side in order to be able to do so.
>>>   
>>
>> Nod.
>>
>>> In general, a device that accepts arbitrary channel programs looks
>>> useful for testing purposes. I would split out processing of expected
>>> responses out, though, so that it can be more easily reused for
>>> different use cases.
>>>   
>>
>> I'm not sure what do you mean here. Could you clarify please?
> 
> Maybe doing things like logging received ccws and responses etc. and
> and then comparing against an expected outcome. You should be able to
> write test cases for different sets of things to be tested. I know this
> is awfully vague :)

Yes it does sound awfully vague :). In my case different tests are
actually done in the kernel module implementing a driver for this
device. I compare the expected outcome and the actual outcome there.
This device is just a back-end lending itself to experimentation.

> 
>>
>>> (I dimly recall other test devices...)
>>>   
>>
>> What's on your mind? How do these relate to our problem? Can you
>> give me some pointers?
> 
> I was thinking of precedent for test devices... but as said, I only
> recall this very dimly.
> 
>>
>>> For the guest tester: Can that be done via the qtest infrastructure
>>> somehow?
>>>   
>>
>> Well, for now I have the out-of-tree Linux kernel module provided in the
>> cover letter of the series (you did not comment on that one yet).
> 
> Yes, I saw that, but have not yet looked at it. If there is a way to do
> it without too many extra parts, I'd prefer that.
> 

Well, I think the module is almost as economical with extra parts as it
can get: it uses the kernel infrastructure and does not do a whole lot
of things on top.

I think it's worth a look. I hope it will also give answers to some
of the implicit questions I see above. Yes, tests done in the driver
are currently very few. Both with and without indirect data access
we first let a device consume a Fibonacci sequence and verify that
the IO has succeeded. Then we deliberately change an element in the
sequence so it ain't the next Fibonacci number. And check for unit
exception and proper residual count. With that we are sure that
the data was properly consumed up until the given element. For IDA
we the shorten the sequence so it does not contain the 'broken'
element, and expect completion again. As you see this is a sufficient
test for the good path for single CCW programs.

Extending to testing chaining (different concern), or responses
to broken channel programs (e.g. ORB flags, bad addresses, tic
rules) should be quite straightforward.

>>
>> I think for building trust regarding my IDA implementation it should be
>> able to do the job. Don't you agree?
> 
> There's nothing wrong with your test. But we really should try to move
> to some kind of unified testing that is hopefully self-contained so
> that interested people can just play with it within the context of qemu.
> 
>>
>> Just a couple of hours ago Janosch (cc-ing Janosch) came to my office,
>> and told be that he is working on CCW-tests for zonk (a minimal kernel
>> for testing -- internal tool AFAIR).
>>
>> By qtest you mean libqtest/libqos? I'm not familiar with that and have no
>> idea what do we have for s390x. I see on libqos-s390x file in test/libqos
>> for starters.
> 
> Yes, that was what I was thinking about. Integrating into what is
> already there is what we should aim for.
> 
> [For things that are done via kvm, kvm unit tests are good. But I think
> we can do css tests completely within qemu.]
> 

I agree fully, the point is somebody needs to make it happen. I would
be very sad if forced to make it happen for the sake of this patch set.

That said. I could not identify any required actions on this front (testing
IDA) . If I made a mistake please point it out. I'm gonna try and speak
with the folks here about our testing strategy. IMHO libqtest/libqos could
very well be a part of it, but I can't tell anything for sure for now.

If somebody volunteers to transform what I have in this patch and the
cover letter to something better integrated, it will make me more than
happy.

Regards,
Halil


Re: [Qemu-devel] [PATCH 5/5] s390x/ccs: add ccw-tester emulated device
Posted by Cornelia Huck 6 years, 6 months ago
On Wed, 6 Sep 2017 18:16:29 +0200
Halil Pasic <pasic@linux.vnet.ibm.com> wrote:

> On 09/06/2017 05:20 PM, Cornelia Huck wrote:
> > On Wed, 6 Sep 2017 16:24:13 +0200
> > Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
> >   
> >> On 09/06/2017 03:18 PM, Cornelia Huck wrote:  
> >>> On Tue,  5 Sep 2017 13:16:45 +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>
> >>>> ---
> >>>>
> >>>> It may not make sense to merge this work in the current form, as it is
> >>>> solely for test purposes.
> >>>> ---
> >>>>  hw/s390x/Makefile.objs |   1 +
> >>>>  hw/s390x/ccw-tester.c  | 179 +++++++++++++++++++++++++++++++++++++++++++++++++
> >>>>  2 files changed, 180 insertions(+)
> >>>>  create mode 100644 hw/s390x/ccw-tester.c    
> >>>
> >>> The main problem here is that you want to exercise a middle layer (the
> >>> css code) and need to write boilerplate code on both host and guest
> >>> side in order to be able to do so.
> >>>     
> >>
> >> Nod.
> >>  
> >>> In general, a device that accepts arbitrary channel programs looks
> >>> useful for testing purposes. I would split out processing of expected
> >>> responses out, though, so that it can be more easily reused for
> >>> different use cases.
> >>>     
> >>
> >> I'm not sure what do you mean here. Could you clarify please?  
> > 
> > Maybe doing things like logging received ccws and responses etc. and
> > and then comparing against an expected outcome. You should be able to
> > write test cases for different sets of things to be tested. I know this
> > is awfully vague :)  
> 
> Yes it does sound awfully vague :). In my case different tests are
> actually done in the kernel module implementing a driver for this
> device. I compare the expected outcome and the actual outcome there.
> This device is just a back-end lending itself to experimentation.

I think we want both guest-side and host-side checking. I think we
should aim for a guest-side checker that does not require a kernel
module (a simple guest makes it easier to run as an automated test.)

> >>> For the guest tester: Can that be done via the qtest infrastructure
> >>> somehow?
> >>>     
> >>
> >> Well, for now I have the out-of-tree Linux kernel module provided in the
> >> cover letter of the series (you did not comment on that one yet).  
> > 
> > Yes, I saw that, but have not yet looked at it. If there is a way to do
> > it without too many extra parts, I'd prefer that.
> >   
> 
> Well, I think the module is almost as economical with extra parts as it
> can get: it uses the kernel infrastructure and does not do a whole lot
> of things on top.

And that's also the problem: You drag the whole kernel infrastructure
along with it.

> 
> I think it's worth a look.

It certainly is, but you know how it is with resources...

> I hope it will also give answers to some
> of the implicit questions I see above. Yes, tests done in the driver
> are currently very few. Both with and without indirect data access
> we first let a device consume a Fibonacci sequence and verify that
> the IO has succeeded. Then we deliberately change an element in the
> sequence so it ain't the next Fibonacci number. And check for unit
> exception and proper residual count. With that we are sure that
> the data was properly consumed up until the given element. For IDA
> we the shorten the sequence so it does not contain the 'broken'
> element, and expect completion again. As you see this is a sufficient
> test for the good path for single CCW programs.
> 
> Extending to testing chaining (different concern), or responses
> to broken channel programs (e.g. ORB flags, bad addresses, tic
> rules) should be quite straightforward.

Yes, that all sounds very nice. We just disagree about the means :)

> 
> >>
> >> I think for building trust regarding my IDA implementation it should be
> >> able to do the job. Don't you agree?  
> > 
> > There's nothing wrong with your test. But we really should try to move
> > to some kind of unified testing that is hopefully self-contained so
> > that interested people can just play with it within the context of qemu.
> >   
> >>
> >> Just a couple of hours ago Janosch (cc-ing Janosch) came to my office,
> >> and told be that he is working on CCW-tests for zonk (a minimal kernel
> >> for testing -- internal tool AFAIR).
> >>
> >> By qtest you mean libqtest/libqos? I'm not familiar with that and have no
> >> idea what do we have for s390x. I see on libqos-s390x file in test/libqos
> >> for starters.  
> > 
> > Yes, that was what I was thinking about. Integrating into what is
> > already there is what we should aim for.
> > 
> > [For things that are done via kvm, kvm unit tests are good. But I think
> > we can do css tests completely within qemu.]
> >   
> 
> I agree fully, the point is somebody needs to make it happen. I would
> be very sad if forced to make it happen for the sake of this patch set.

You must have misunderstood me: This is not a requirement for this
patch set...

> 
> That said. I could not identify any required actions on this front (testing
> IDA) . If I made a mistake please point it out. I'm gonna try and speak
> with the folks here about our testing strategy. IMHO libqtest/libqos could
> very well be a part of it, but I can't tell anything for sure for now.

Yes, it would be great if we could get something that benefits
everyone. If we have tests that integrate into existing infrastructure,
it is more likely that they will actually be run :)

> 
> If somebody volunteers to transform what I have in this patch and the
> cover letter to something better integrated, it will make me more than
> happy.

Me too :)

Re: [Qemu-devel] [PATCH 5/5] s390x/ccs: add ccw-tester emulated device
Posted by Janosch Frank 6 years, 6 months ago
[...]
>>>>> The main problem here is that you want to exercise a middle layer (the
>>>>> css code) and need to write boilerplate code on both host and guest
>>>>> side in order to be able to do so.
>>>>>     
>>>>
>>>> Nod.
>>>>  
>>>>> In general, a device that accepts arbitrary channel programs looks
>>>>> useful for testing purposes. I would split out processing of expected
>>>>> responses out, though, so that it can be more easily reused for
>>>>> different use cases.
>>>>>     
>>>>
>>>> I'm not sure what do you mean here. Could you clarify please?  
>>>
>>> Maybe doing things like logging received ccws and responses etc. and
>>> and then comparing against an expected outcome. You should be able to
>>> write test cases for different sets of things to be tested. I know this
>>> is awfully vague :)  
>>
>> Yes it does sound awfully vague :). In my case different tests are
>> actually done in the kernel module implementing a driver for this
>> device. I compare the expected outcome and the actual outcome there.
>> This device is just a back-end lending itself to experimentation.
> 
> I think we want both guest-side and host-side checking. I think we
> should aim for a guest-side checker that does not require a kernel
> module (a simple guest makes it easier to run as an automated test.)

Sure, that would be great.

The thing is that I want to test the subchannel and not the device and
therefore I really do not want to have to issue control commands to a
device in order to get data. Having a device that reads and writes
without a lot of overhead (like this) is therefore my main target.

Where this device lives doesn't concern me and as I'm new to this
wonderful IO system take my statements with some salt. :)


>>>>> For the guest tester: Can that be done via the qtest infrastructure
>>>>> somehow?
>>>>>     
>>>>
>>>> Well, for now I have the out-of-tree Linux kernel module provided in the
>>>> cover letter of the series (you did not comment on that one yet).  
>>>
>>> Yes, I saw that, but have not yet looked at it. If there is a way to do
>>> it without too many extra parts, I'd prefer that.
>>>   
>>
>> Well, I think the module is almost as economical with extra parts as it
>> can get: it uses the kernel infrastructure and does not do a whole lot
>> of things on top.
> 
> And that's also the problem: You drag the whole kernel infrastructure
> along with i
> 
>>
>> I think it's worth a look.
> 
> It certainly is, but you know how it is with resources...

Yes it is and we certainly want to be integrated in as much external
testing as possible. It seems like a few people began to run into the
same direction but chose different approaches. My zonk test intentions
are mainly to get a understanding how this all works without having to
use the Linux devices but getting my hands dirty with the instructions
and structures.

I see your arguments and we'll look into it and discuss consolidating
our efforts.


Re: [Qemu-devel] [PATCH 5/5] s390x/ccs: add ccw-tester emulated device
Posted by Cornelia Huck 6 years, 6 months ago
On Thu, 7 Sep 2017 11:10:17 +0200
Janosch Frank <frankja@linux.vnet.ibm.com> wrote:

> The thing is that I want to test the subchannel and not the device and
> therefore I really do not want to have to issue control commands to a
> device in order to get data. Having a device that reads and writes
> without a lot of overhead (like this) is therefore my main target.

Yes, a simple device backend for the channel subsystem is useful.

What could also be useful is a way to inject errors, e.g. randomly
failing things and then verifying that a sensible error reaches the
guest. These paths are part of the css and not the device, so it makes
sense to verify that these are working as expected. (Especially as they
aren't usually seen in the wild.)

But let's defer that one :)

> 
> Where this device lives doesn't concern me and as I'm new to this
> wonderful IO system take my statements with some salt. :)

You'll learn to love it 8)

> Yes it is and we certainly want to be integrated in as much external
> testing as possible. It seems like a few people began to run into the
> same direction but chose different approaches. My zonk test intentions
> are mainly to get a understanding how this all works without having to
> use the Linux devices but getting my hands dirty with the instructions
> and structures.

Something zonk-like would probably be a good thing for a test setup
that can be run via make check or something like that.

> 
> I see your arguments and we'll look into it and discuss consolidating
> our efforts.

Great, thanks for doing that!

Re: [Qemu-devel] [PATCH 5/5] s390x/ccs: add ccw-tester emulated device
Posted by Dong Jia Shi 6 years, 6 months ago
* Cornelia Huck <cohuck@redhat.com> [2017-09-06 15:18:21 +0200]:

> On Tue,  5 Sep 2017 13:16:45 +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>
> > ---
> > 
> > It may not make sense to merge this work in the current form, as it is
> > solely for test purposes.
> > ---
> >  hw/s390x/Makefile.objs |   1 +
> >  hw/s390x/ccw-tester.c  | 179 +++++++++++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 180 insertions(+)
> >  create mode 100644 hw/s390x/ccw-tester.c
> 
> The main problem here is that you want to exercise a middle layer (the
> css code) and need to write boilerplate code on both host and guest
> side in order to be able to do so.
> 
> In general, a device that accepts arbitrary channel programs looks
> useful for testing purposes. I would split out processing of expected
> responses out, though, so that it can be more easily reused for
> different use cases.
> 
> (I dimly recall other test devices...)
> 
> For the guest tester: Can that be done via the qtest infrastructure
> somehow?
> 

I'm thinking of a method these days:
Could passing through an fully emulated ccw device (e.g. 3270), or a
virtio ccw device, in the level 1 kvm guest to a level 2 guest be a test
method for this?

All of the CCWs will be translated to IDAL CCWs by vfio-ccw in the level
1 guest (which is the level 2 kvm host) and issued to the level 1 kvm
host. So, those IDALs will eventually be handled by the emulated device,
or the virtio ccw device, on the level 1 kvm host...

Some days ago, one of my colleague tried the emulated 3270 passing
through. She stucked with the problem that the level 1 kvm host handling
a senseid IDAL ccw as a Direct ccw.

Maybe I could try to pass through a virtio ccw device. I don't think of
any obvious problem that would lead to fail. Any comment?

-- 
Dong Jia Shi


Re: [Qemu-devel] [PATCH 5/5] s390x/ccs: add ccw-tester emulated device
Posted by Cornelia Huck 6 years, 6 months ago
On Thu, 7 Sep 2017 15:31:09 +0800
Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com> wrote:

> * Cornelia Huck <cohuck@redhat.com> [2017-09-06 15:18:21 +0200]:
> 
> > On Tue,  5 Sep 2017 13:16:45 +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>
> > > ---
> > > 
> > > It may not make sense to merge this work in the current form, as it is
> > > solely for test purposes.
> > > ---
> > >  hw/s390x/Makefile.objs |   1 +
> > >  hw/s390x/ccw-tester.c  | 179 +++++++++++++++++++++++++++++++++++++++++++++++++
> > >  2 files changed, 180 insertions(+)
> > >  create mode 100644 hw/s390x/ccw-tester.c  
> > 
> > The main problem here is that you want to exercise a middle layer (the
> > css code) and need to write boilerplate code on both host and guest
> > side in order to be able to do so.
> > 
> > In general, a device that accepts arbitrary channel programs looks
> > useful for testing purposes. I would split out processing of expected
> > responses out, though, so that it can be more easily reused for
> > different use cases.
> > 
> > (I dimly recall other test devices...)
> > 
> > For the guest tester: Can that be done via the qtest infrastructure
> > somehow?
> >   
> 
> I'm thinking of a method these days:
> Could passing through an fully emulated ccw device (e.g. 3270), or a
> virtio ccw device, in the level 1 kvm guest to a level 2 guest be a test
> method for this?
> 
> All of the CCWs will be translated to IDAL CCWs by vfio-ccw in the level
> 1 guest (which is the level 2 kvm host) and issued to the level 1 kvm
> host. So, those IDALs will eventually be handled by the emulated device,
> or the virtio ccw device, on the level 1 kvm host...
> 
> Some days ago, one of my colleague tried the emulated 3270 passing
> through. She stucked with the problem that the level 1 kvm host handling
> a senseid IDAL ccw as a Direct ccw.
> 
> Maybe I could try to pass through a virtio ccw device. I don't think of
> any obvious problem that would lead to fail. Any comment?
> 

That actually looks like a good thing to try! Cool idea.

Re: [Qemu-devel] [PATCH 5/5] s390x/ccs: add ccw-tester emulated device
Posted by Halil Pasic 6 years, 6 months ago

On 09/07/2017 10:08 AM, Cornelia Huck wrote:
> On Thu, 7 Sep 2017 15:31:09 +0800
> Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com> wrote:
> 
>> * Cornelia Huck <cohuck@redhat.com> [2017-09-06 15:18:21 +0200]:
>>
>>> On Tue,  5 Sep 2017 13:16:45 +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>
>>>> ---
>>>>
>>>> It may not make sense to merge this work in the current form, as it is
>>>> solely for test purposes.
>>>> ---
>>>>  hw/s390x/Makefile.objs |   1 +
>>>>  hw/s390x/ccw-tester.c  | 179 +++++++++++++++++++++++++++++++++++++++++++++++++
>>>>  2 files changed, 180 insertions(+)
>>>>  create mode 100644 hw/s390x/ccw-tester.c  
>>>
>>> The main problem here is that you want to exercise a middle layer (the
>>> css code) and need to write boilerplate code on both host and guest
>>> side in order to be able to do so.
>>>
>>> In general, a device that accepts arbitrary channel programs looks
>>> useful for testing purposes. I would split out processing of expected
>>> responses out, though, so that it can be more easily reused for
>>> different use cases.
>>>
>>> (I dimly recall other test devices...)
>>>
>>> For the guest tester: Can that be done via the qtest infrastructure
>>> somehow?
>>>   
>>
>> I'm thinking of a method these days:
>> Could passing through an fully emulated ccw device (e.g. 3270), or a
>> virtio ccw device, in the level 1 kvm guest to a level 2 guest be a test
>> method for this?
>>
>> All of the CCWs will be translated to IDAL CCWs by vfio-ccw in the level
>> 1 guest (which is the level 2 kvm host) and issued to the level 1 kvm
>> host. So, those IDALs will eventually be handled by the emulated device,
>> or the virtio ccw device, on the level 1 kvm host...
>>
>> Some days ago, one of my colleague tried the emulated 3270 passing
>> through. She stucked with the problem that the level 1 kvm host handling
>> a senseid IDAL ccw as a Direct ccw.
>>
>> Maybe I could try to pass through a virtio ccw device. I don't think of
>> any obvious problem that would lead to fail. Any comment?
>>
> 
> That actually looks like a good thing to try! Cool idea.
> 

I'm afraid that it would not work without some extra work.
AFAIR Connie we said that the 3270 does not use any IDA, so
I did not touch the 3270 emulation code in QEMU. To make
the scenario viable one should convert the 3270 emulation
to ccw data stream (unless the original implementation
already took care of IDA, which I doubt).

Halil


Re: [Qemu-devel] [PATCH 5/5] s390x/ccs: add ccw-tester emulated device
Posted by Cornelia Huck 6 years, 6 months ago
On Thu, 7 Sep 2017 12:21:50 +0200
Halil Pasic <pasic@linux.vnet.ibm.com> wrote:

> On 09/07/2017 10:08 AM, Cornelia Huck wrote:
> > On Thu, 7 Sep 2017 15:31:09 +0800
> > Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com> wrote:

> >> I'm thinking of a method these days:
> >> Could passing through an fully emulated ccw device (e.g. 3270), or a
> >> virtio ccw device, in the level 1 kvm guest to a level 2 guest be a test
> >> method for this?
> >>
> >> All of the CCWs will be translated to IDAL CCWs by vfio-ccw in the level
> >> 1 guest (which is the level 2 kvm host) and issued to the level 1 kvm
> >> host. So, those IDALs will eventually be handled by the emulated device,
> >> or the virtio ccw device, on the level 1 kvm host...
> >>
> >> Some days ago, one of my colleague tried the emulated 3270 passing
> >> through. She stucked with the problem that the level 1 kvm host handling
> >> a senseid IDAL ccw as a Direct ccw.
> >>
> >> Maybe I could try to pass through a virtio ccw device. I don't think of
> >> any obvious problem that would lead to fail. Any comment?
> >>  
> > 
> > That actually looks like a good thing to try! Cool idea.
> >   
> 
> I'm afraid that it would not work without some extra work.
> AFAIR Connie we said that the 3270 does not use any IDA, so
> I did not touch the 3270 emulation code in QEMU. To make
> the scenario viable one should convert the 3270 emulation
> to ccw data stream (unless the original implementation
> already took care of IDA, which I doubt).

But the vfio-ccw code uses idals... no need to touch the 3270 emulation.

Re: [Qemu-devel] [PATCH 5/5] s390x/ccs: add ccw-tester emulated device
Posted by Dong Jia Shi 6 years, 6 months ago
* Cornelia Huck <cohuck@redhat.com> [2017-09-07 12:52:20 +0200]:

> On Thu, 7 Sep 2017 12:21:50 +0200
> Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
> 
> > On 09/07/2017 10:08 AM, Cornelia Huck wrote:
> > > On Thu, 7 Sep 2017 15:31:09 +0800
> > > Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com> wrote:
> 
> > >> I'm thinking of a method these days:
> > >> Could passing through an fully emulated ccw device (e.g. 3270), or a
> > >> virtio ccw device, in the level 1 kvm guest to a level 2 guest be a test
> > >> method for this?
> > >>
> > >> All of the CCWs will be translated to IDAL CCWs by vfio-ccw in the level
> > >> 1 guest (which is the level 2 kvm host) and issued to the level 1 kvm
> > >> host. So, those IDALs will eventually be handled by the emulated device,
> > >> or the virtio ccw device, on the level 1 kvm host...
> > >>
> > >> Some days ago, one of my colleague tried the emulated 3270 passing
> > >> through. She stucked with the problem that the level 1 kvm host handling
> > >> a senseid IDAL ccw as a Direct ccw.
> > >>
> > >> Maybe I could try to pass through a virtio ccw device. I don't think of
> > >> any obvious problem that would lead to fail. Any comment?
> > >>  
> > > 
> > > That actually looks like a good thing to try! Cool idea.
> > >   
> > 
> > I'm afraid that it would not work without some extra work.
> > AFAIR Connie we said that the 3270 does not use any IDA, so
> > I did not touch the 3270 emulation code in QEMU. To make
> > the scenario viable one should convert the 3270 emulation
> > to ccw data stream (unless the original implementation
> > already took care of IDA, which I doubt).
> 
> But the vfio-ccw code uses idals... no need to touch the 3270 emulation.
> 
What Halil pointed out is that the ccw_cb implementation of 3270
emulation does not take care of IDALs. This is true.

I can also do that right after this series, if Halil agrees.
(The 3270 emulation authors are busy of other stuff these days. :()

-- 
Dong Jia Shi


Re: [Qemu-devel] [PATCH 5/5] s390x/ccs: add ccw-tester emulated device
Posted by Halil Pasic 6 years, 6 months ago

On 09/08/2017 04:01 AM, Dong Jia Shi wrote:
> * Cornelia Huck <cohuck@redhat.com> [2017-09-07 12:52:20 +0200]:
> 
>> On Thu, 7 Sep 2017 12:21:50 +0200
>> Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
>>
>>> On 09/07/2017 10:08 AM, Cornelia Huck wrote:
>>>> On Thu, 7 Sep 2017 15:31:09 +0800
>>>> Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com> wrote:
>>
>>>>> I'm thinking of a method these days:
>>>>> Could passing through an fully emulated ccw device (e.g. 3270), or a
>>>>> virtio ccw device, in the level 1 kvm guest to a level 2 guest be a test
>>>>> method for this?
>>>>>
>>>>> All of the CCWs will be translated to IDAL CCWs by vfio-ccw in the level
>>>>> 1 guest (which is the level 2 kvm host) and issued to the level 1 kvm
>>>>> host. So, those IDALs will eventually be handled by the emulated device,
>>>>> or the virtio ccw device, on the level 1 kvm host...
>>>>>
>>>>> Some days ago, one of my colleague tried the emulated 3270 passing
>>>>> through. She stucked with the problem that the level 1 kvm host handling
>>>>> a senseid IDAL ccw as a Direct ccw.
>>>>>
>>>>> Maybe I could try to pass through a virtio ccw device. I don't think of
>>>>> any obvious problem that would lead to fail. Any comment?
>>>>>  
>>>>
>>>> That actually looks like a good thing to try! Cool idea.
>>>>   
>>>
>>> I'm afraid that it would not work without some extra work.
>>> AFAIR Connie we said that the 3270 does not use any IDA, so
>>> I did not touch the 3270 emulation code in QEMU. To make
>>> the scenario viable one should convert the 3270 emulation
>>> to ccw data stream (unless the original implementation
>>> already took care of IDA, which I doubt).
>>
>> But the vfio-ccw code uses idals... no need to touch the 3270 emulation.
>>
> What Halil pointed out is that the ccw_cb implementation of 3270
> emulation does not take care of IDALs. This is true.
> 
> I can also do that right after this series, if Halil agrees.
> (The 3270 emulation authors are busy of other stuff these days. :()

Generally, yes I agree. If it's trivial I will probably do it myself
for v2. I need to do a v2 anyway.

Halil


Re: [Qemu-devel] [PATCH 5/5] s390x/ccs: add ccw-tester emulated device
Posted by Dong Jia Shi 6 years, 6 months ago
* Halil Pasic <pasic@linux.vnet.ibm.com> [2017-09-08 12:28:03 +0200]:

[snip]

> > What Halil pointed out is that the ccw_cb implementation of 3270
> > emulation does not take care of IDALs. This is true.
> > 
> > I can also do that right after this series, if Halil agrees.
> > (The 3270 emulation authors are busy of other stuff these days. :()
> 
> Generally, yes I agree. If it's trivial I will probably do it myself
> for v2. I need to do a v2 anyway.
> 
> Halil

I saw you are working on this. So leave it to you. ;)

-- 
Dong Jia Shi


Re: [Qemu-devel] [PATCH 5/5] s390x/ccs: add ccw-tester emulated device
Posted by Dong Jia Shi 6 years, 6 months ago
* Cornelia Huck <cohuck@redhat.com> [2017-09-07 10:08:17 +0200]:

[...]

> > I'm thinking of a method these days:
> > Could passing through an fully emulated ccw device (e.g. 3270), or a
> > virtio ccw device, in the level 1 kvm guest to a level 2 guest be a test
> > method for this?
> > 
> > All of the CCWs will be translated to IDAL CCWs by vfio-ccw in the level
> > 1 guest (which is the level 2 kvm host) and issued to the level 1 kvm
> > host. So, those IDALs will eventually be handled by the emulated device,
> > or the virtio ccw device, on the level 1 kvm host...
> > 
> > Some days ago, one of my colleague tried the emulated 3270 passing
> > through. She stucked with the problem that the level 1 kvm host handling
> > a senseid IDAL ccw as a Direct ccw.
> > 
> > Maybe I could try to pass through a virtio ccw device. I don't think of
> > any obvious problem that would lead to fail. Any comment?
> > 
> 
> That actually looks like a good thing to try! Cool idea.
> 

Tried to test with the following method:
1. Start g1 (first level guest on kvm a host) with a virtio blk device
   defined:
-drive file=/dev/disk/by-path/ccw-0.0.3f3e,if=none,id=drive-virtio-disk1,format=raw \
-device virtio-blk-ccw,devno=fe.0.2222,scsi=off,drive=drive-virtio-disk1,id=virtio-disk1 \
2. Login g1, and bind the subchannel of ccw device 0.0.2222 with
   vfio-ccw drvier.
3. Create a mdev on the above subchannel.
4. Passthrough the mdev to g2, and try to start g2.

The 4th step failed with the following message and hang:
qemu-system-s390x: vfio-ccw: wirte I/O region: errno=4
(BTW, 4 is EINTR.)

I roughly guess this might be caused by:
On the kvm host, virtio callback injects the I/O interrupt in a
synchronzing manner. And this causes g1's I/O interrupt handler getting
the interrupt and then signaling the Qemu instance on g1 with the I/O
result, even before return of the pwrite().

But, using gdb on the kvm host, I do see several ssch successfully
executed. I will dig the root reason, and see if there is some way to
fix the issue.

-- 
Dong Jia Shi


Re: [Qemu-devel] [PATCH 5/5] s390x/ccs: add ccw-tester emulated device
Posted by Cornelia Huck 6 years, 6 months ago
On Thu, 21 Sep 2017 16:45:47 +0800
Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com> wrote:

> * Cornelia Huck <cohuck@redhat.com> [2017-09-07 10:08:17 +0200]:
> 
> [...]
> 
> > > I'm thinking of a method these days:
> > > Could passing through an fully emulated ccw device (e.g. 3270), or a
> > > virtio ccw device, in the level 1 kvm guest to a level 2 guest be a test
> > > method for this?
> > > 
> > > All of the CCWs will be translated to IDAL CCWs by vfio-ccw in the level
> > > 1 guest (which is the level 2 kvm host) and issued to the level 1 kvm
> > > host. So, those IDALs will eventually be handled by the emulated device,
> > > or the virtio ccw device, on the level 1 kvm host...
> > > 
> > > Some days ago, one of my colleague tried the emulated 3270 passing
> > > through. She stucked with the problem that the level 1 kvm host handling
> > > a senseid IDAL ccw as a Direct ccw.
> > > 
> > > Maybe I could try to pass through a virtio ccw device. I don't think of
> > > any obvious problem that would lead to fail. Any comment?
> > >   
> > 
> > That actually looks like a good thing to try! Cool idea.
> >   
> 
> Tried to test with the following method:
> 1. Start g1 (first level guest on kvm a host) with a virtio blk device
>    defined:
> -drive file=/dev/disk/by-path/ccw-0.0.3f3e,if=none,id=drive-virtio-disk1,format=raw \
> -device virtio-blk-ccw,devno=fe.0.2222,scsi=off,drive=drive-virtio-disk1,id=virtio-disk1 \
> 2. Login g1, and bind the subchannel of ccw device 0.0.2222 with
>    vfio-ccw drvier.
> 3. Create a mdev on the above subchannel.
> 4. Passthrough the mdev to g2, and try to start g2.
> 
> The 4th step failed with the following message and hang:
> qemu-system-s390x: vfio-ccw: wirte I/O region: errno=4
> (BTW, 4 is EINTR.)
> 
> I roughly guess this might be caused by:
> On the kvm host, virtio callback injects the I/O interrupt in a
> synchronzing manner. And this causes g1's I/O interrupt handler getting
> the interrupt and then signaling the Qemu instance on g1 with the I/O
> result, even before return of the pwrite().
> 
> But, using gdb on the kvm host, I do see several ssch successfully
> executed. I will dig the root reason, and see if there is some way to
> fix the issue.

Hm... would that be the ccws used for setting up a virtio device, and
the problems start once adapter interrupts become active? Does it work
if you modify the nested guest to use the old per-subchannel indicators
mechanism?

(I'm also wondering how diag is handled?)

Re: [Qemu-devel] [PATCH 5/5] s390x/ccs: add ccw-tester emulated device
Posted by Dong Jia Shi 6 years, 6 months ago
* Cornelia Huck <cohuck@redhat.com> [2017-09-21 10:54:02 +0200]:

> On Thu, 21 Sep 2017 16:45:47 +0800
> Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com> wrote:
> 
> > * Cornelia Huck <cohuck@redhat.com> [2017-09-07 10:08:17 +0200]:
> > 
> > [...]
> > 
> > > > I'm thinking of a method these days:
> > > > Could passing through an fully emulated ccw device (e.g. 3270), or a
> > > > virtio ccw device, in the level 1 kvm guest to a level 2 guest be a test
> > > > method for this?
> > > > 
> > > > All of the CCWs will be translated to IDAL CCWs by vfio-ccw in the level
> > > > 1 guest (which is the level 2 kvm host) and issued to the level 1 kvm
> > > > host. So, those IDALs will eventually be handled by the emulated device,
> > > > or the virtio ccw device, on the level 1 kvm host...
> > > > 
> > > > Some days ago, one of my colleague tried the emulated 3270 passing
> > > > through. She stucked with the problem that the level 1 kvm host handling
> > > > a senseid IDAL ccw as a Direct ccw.
> > > > 
> > > > Maybe I could try to pass through a virtio ccw device. I don't think of
> > > > any obvious problem that would lead to fail. Any comment?
> > > >   
> > > 
> > > That actually looks like a good thing to try! Cool idea.
> > >   
> > 
> > Tried to test with the following method:
> > 1. Start g1 (first level guest on kvm a host) with a virtio blk device
> >    defined:
> > -drive file=/dev/disk/by-path/ccw-0.0.3f3e,if=none,id=drive-virtio-disk1,format=raw \
> > -device virtio-blk-ccw,devno=fe.0.2222,scsi=off,drive=drive-virtio-disk1,id=virtio-disk1 \
> > 2. Login g1, and bind the subchannel of ccw device 0.0.2222 with
> >    vfio-ccw drvier.
> > 3. Create a mdev on the above subchannel.
> > 4. Passthrough the mdev to g2, and try to start g2.
> > 
> > The 4th step failed with the following message and hang:
> > qemu-system-s390x: vfio-ccw: wirte I/O region: errno=4
> > (BTW, 4 is EINTR.)
> > 
> > I roughly guess this might be caused by:
> > On the kvm host, virtio callback injects the I/O interrupt in a
> > synchronzing manner. And this causes g1's I/O interrupt handler getting
> > the interrupt and then signaling the Qemu instance on g1 with the I/O
> > result, even before return of the pwrite().
> > 
> > But, using gdb on the kvm host, I do see several ssch successfully
> > executed. I will dig the root reason, and see if there is some way to
> > fix the issue.
> 
> Hm... would that be the ccws used for setting up a virtio device, and
> the problems start once adapter interrupts become active?
After a debugging, when starting g2, I got the following ccw sequence:
1. CCW_CMD_SENSE_ID		0xe4 [OK]
2. CCW_CMD_NOOP			0x03 [OK]
3. CCW_CMD_SET_VIRTIO_REV	0x83 [OK]
4. CCW_CMD_VDEV_RESET		0x33 [FAILED]

So this is still in the phase of setting up the device.

> Does it work if you modify the nested guest to use the old
> per-subchannel indicators mechanism?
It turns out the root reason for the pwrite failure is caused by a bug
in the vfio-ccw driver:
drivers/s390/cio/vfio_ccw_cp.c: ccwchain_fetch_direct()
    calls pfn_array_alloc_pin() with a zero @len parameter.
So it results in a -EINVAL return.

The current code assumes that a valid direct ccw always has its count
value not equal to zero. However this is not true at least for the
CCW_CMD_VDEV_RESET (0x33) command:
(gdb) p/x ccw
 $5 = {cmd_code = 0x33, flags = 0x4, count = 0x0, cda = 0x0}

With a temp fix on this problem, more ccws (e.g. 0x11, 0x12, 0x31, 0x72
...) could be translated and executed well. But finnaly the qemu process
on g1 got a segmentation fault:
User process fault: interruption code 0238 ilc:3 in libpthread-2.24.so[3ff84f80000+1b000]
Failing address: 000ce330b0b00000 TEID: 000ce330b0b00800
Fault in primary space mode while using user ASCE.
AS:000000003b6cc1c7 R3:0000000000000024 
Segmentation fault

dmesg on g1:
[   18.160413] User process fault: interruption code 0238 ilc:3 in libpthread-2.24.so[3ff84f80000+1b000]
[   18.160462] Failing address: 000ce330b0b00000 TEID: 000ce330b0b00800
[   18.160463] Fault in primary space mode while using user ASCE.
[   18.160470] AS:000000003b6cc1c7 R3:0000000000000024 
[   18.160476] CPU: 1 PID: 2095 Comm: qemu-system-s39 Not tainted 4.13.0-01250-g6baa298-dirty #58
[   18.160477] Hardware name: IBM 2964 NC9 704 (KVM/Linux)
[   18.160479] task: 0000000038ac8000 task.stack: 0000000038e4c000
[   18.160480] User PSW : 0705200180000000 000003ff84f93b8a
[   18.160483]            R:0 T:1 IO:1 EX:1 Key:0 M:1 W:0 P:1 AS:0 CC:2 PM:0 RI:0 EA:3
[   18.160486] User GPRS: 0000000000000001 000003ff00000003 0000000104be86b0 0000000104be86c6
[   18.160487]            0000000000000000 0000000100000001 00000001049efb22 000003ffc5dfe13f
[   18.160489]            000003ff643fee60 0000000000000000 000003ffc5dfe258 000003ff643fe8c8
[   18.160490]            000003ff855a5000 00000001049cc320 000003ff643fe888 000003ff643fe7e8
[   18.160503] User Code: 000003ff84f93b7a: c0e5ffffe7cb        brasl %r14,3ff84f90b10
                          000003ff84f93b80: a7f4ffc4            brc 15,3ff84f93b08
                         #000003ff84f93b84: e5600000ff0c        tbegin 0,65292
                         >000003ff84f93b8a: b2220050            ipm >%r5
                          000003ff84f93b8e: 8850001c            srl %r5,28
                          000003ff84f93b92: a774001c            brc 7,3ff84f93bca
                          000003ff84f93b96: e30020000012        lt %r0,0(%r2)
                          000003ff84f93b9c: a784ffb6            brc 8,3ff84f93b08
[   18.160520] Last Breaking-Event-Address:
[   18.160524]  [<00000001046404e6>] 0x1046404e6

The above fault is not caused by vfio-ccw directly I think. So now I
need to install gdb stuff on g1, and continuing debugging. But ideas on
this are welcomed. ;)

> 
> (I'm also wondering how diag is handled?)
Not looking into this yet. :-/

> 

-- 
Dong Jia Shi


Re: [Qemu-devel] [PATCH 5/5] s390x/ccs: add ccw-tester emulated device
Posted by Dong Jia Shi 6 years, 6 months ago
* Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com> [2017-09-26 15:48:56 +0800]:

[...]

> > > 
> > > Tried to test with the following method:
> > > 1. Start g1 (first level guest on kvm a host) with a virtio blk device
> > >    defined:
> > > -drive file=/dev/disk/by-path/ccw-0.0.3f3e,if=none,id=drive-virtio-disk1,format=raw \
> > > -device virtio-blk-ccw,devno=fe.0.2222,scsi=off,drive=drive-virtio-disk1,id=virtio-disk1 \
> > > 2. Login g1, and bind the subchannel of ccw device 0.0.2222 with
> > >    vfio-ccw drvier.
> > > 3. Create a mdev on the above subchannel.
> > > 4. Passthrough the mdev to g2, and try to start g2.
> > > 
> > > The 4th step failed with the following message and hang:
> > > qemu-system-s390x: vfio-ccw: wirte I/O region: errno=4
> > > (BTW, 4 is EINTR.)
> > > 
> > > I roughly guess this might be caused by:
> > > On the kvm host, virtio callback injects the I/O interrupt in a
> > > synchronzing manner. And this causes g1's I/O interrupt handler getting
> > > the interrupt and then signaling the Qemu instance on g1 with the I/O
> > > result, even before return of the pwrite().
> > > 
> > > But, using gdb on the kvm host, I do see several ssch successfully
> > > executed. I will dig the root reason, and see if there is some way to
> > > fix the issue.
> > 
> > Hm... would that be the ccws used for setting up a virtio device, and
> > the problems start once adapter interrupts become active?
> After a debugging, when starting g2, I got the following ccw sequence:
> 1. CCW_CMD_SENSE_ID		0xe4 [OK]
> 2. CCW_CMD_NOOP			0x03 [OK]
> 3. CCW_CMD_SET_VIRTIO_REV	0x83 [OK]
> 4. CCW_CMD_VDEV_RESET		0x33 [FAILED]
> 
> So this is still in the phase of setting up the device.
> 
> > Does it work if you modify the nested guest to use the old
> > per-subchannel indicators mechanism?
> It turns out the root reason for the pwrite failure is caused by a bug
> in the vfio-ccw driver:
> drivers/s390/cio/vfio_ccw_cp.c: ccwchain_fetch_direct()
>     calls pfn_array_alloc_pin() with a zero @len parameter.
> So it results in a -EINVAL return.
> 
> The current code assumes that a valid direct ccw always has its count
> value not equal to zero. However this is not true at least for the
> CCW_CMD_VDEV_RESET (0x33) command:
> (gdb) p/x ccw
>  $5 = {cmd_code = 0x33, flags = 0x4, count = 0x0, cda = 0x0}
> 
> With a temp fix on this problem, more ccws (e.g. 0x11, 0x12, 0x31, 0x72
> ...) could be translated and executed well. But finnaly the qemu process
> on g1 got a segmentation fault:
> User process fault: interruption code 0238 ilc:3 in libpthread-2.24.so[3ff84f80000+1b000]
> Failing address: 000ce330b0b00000 TEID: 000ce330b0b00800
> Fault in primary space mode while using user ASCE.
> AS:000000003b6cc1c7 R3:0000000000000024 
> Segmentation fault
> 
> dmesg on g1:
> [   18.160413] User process fault: interruption code 0238 ilc:3 in libpthread-2.24.so[3ff84f80000+1b000]
> [   18.160462] Failing address: 000ce330b0b00000 TEID: 000ce330b0b00800
> [   18.160463] Fault in primary space mode while using user ASCE.
> [   18.160470] AS:000000003b6cc1c7 R3:0000000000000024 
> [   18.160476] CPU: 1 PID: 2095 Comm: qemu-system-s39 Not tainted 4.13.0-01250-g6baa298-dirty #58
> [   18.160477] Hardware name: IBM 2964 NC9 704 (KVM/Linux)
> [   18.160479] task: 0000000038ac8000 task.stack: 0000000038e4c000
> [   18.160480] User PSW : 0705200180000000 000003ff84f93b8a
> [   18.160483]            R:0 T:1 IO:1 EX:1 Key:0 M:1 W:0 P:1 AS:0 CC:2 PM:0 RI:0 EA:3
> [   18.160486] User GPRS: 0000000000000001 000003ff00000003 0000000104be86b0 0000000104be86c6
> [   18.160487]            0000000000000000 0000000100000001 00000001049efb22 000003ffc5dfe13f
> [   18.160489]            000003ff643fee60 0000000000000000 000003ffc5dfe258 000003ff643fe8c8
> [   18.160490]            000003ff855a5000 00000001049cc320 000003ff643fe888 000003ff643fe7e8
> [   18.160503] User Code: 000003ff84f93b7a: c0e5ffffe7cb        brasl %r14,3ff84f90b10
>                           000003ff84f93b80: a7f4ffc4            brc 15,3ff84f93b08
>                          #000003ff84f93b84: e5600000ff0c        tbegin 0,65292
>                          >000003ff84f93b8a: b2220050            ipm >%r5
>                           000003ff84f93b8e: 8850001c            srl %r5,28
>                           000003ff84f93b92: a774001c            brc 7,3ff84f93bca
>                           000003ff84f93b96: e30020000012        lt %r0,0(%r2)
>                           000003ff84f93b9c: a784ffb6            brc 8,3ff84f93b08
> [   18.160520] Last Breaking-Event-Address:
> [   18.160524]  [<00000001046404e6>] 0x1046404e6
> 
> The above fault is not caused by vfio-ccw directly I think. So now I
> need to install gdb stuff on g1, and continuing debugging. But ideas on
> this are welcomed. ;)

Using gdb with Qemu on g1, I got the following information:

Thread 3 "qemu-system-s39" received signal SIGSEGV, Segmentation fault.
[Switching to Thread 0x3ffdcdff910 (LWP 2095)]
__lll_lock_elision (futex=0x1007686b0 <qemu_global_mutex>, 
    adapt_count=0x1007686c6 <qemu_global_mutex+22>, private=0)
    at ../sysdeps/unix/sysv/linux/s390/elision-lock.c:66
66      ../sysdeps/unix/sysv/linux/s390/elision-lock.c: No such file or directory.

(gdb) bt
#0  __lll_lock_elision (futex=0x1007686b0 <qemu_global_mutex>, 
    adapt_count=0x1007686c6 <qemu_global_mutex+22>, private=0)
    at ../sysdeps/unix/sysv/linux/s390/elision-lock.c:66
#1  0x000003fffd98a1f4 in __GI___pthread_mutex_lock (mutex=<optimized out>)
    at ../nptl/pthread_mutex_lock.c:92
#2  0x0000000100515326 in qemu_mutex_lock (
    mutex=0x1007686b0 <qemu_global_mutex>) at util/qemu-thread-posix.c:65
#3  0x00000001000f2dec in qemu_mutex_lock_iothread () at /root/qemu/cpus.c:1581
#4  0x000000010022827e in kvm_arch_handle_exit (cs=0x100c30ce0, 
    run=0x3fffce80000) at /root/qemu/target/s390x/kvm.c:2193
#5  0x0000000100131c40 in kvm_cpu_exec (cpu=0x100c30ce0)
    at /root/qemu/accel/kvm/kvm-all.c:2094
#6  0x00000001000f1d2a in qemu_kvm_cpu_thread_fn (arg=0x100c30ce0)
    at /root/qemu/cpus.c:1128
#7  0x000003fffd9879d4 in start_thread (arg=0x3ffdcdff910)
    at pthread_create.c:335
#8  0x000003fffd8736ae in thread_start ()
    at ../sysdeps/unix/sysv/linux/s390/s390-64/clone.S:71
PC not saved

Googled lock elision for a while, and I still have no idea on this
problem. Any suggestions on this?

-- 
Dong Jia Shi