add CCW indirect data access support
cover add CCW indirect data access support
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
Signature · 52 lines
2.13.5 ---------------------------8<---------------------------------------- From: Halil Pasic <[email protected]> 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 <[email protected]> --- .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
.gitignore new file +8 -0
@@ -0,0 +1,8 @@ +#ignore these +*.o +*.cmd +*.ko +*.mod.c +Module.symvers +modules.order +.tmp_versions/
Makefile new file +10 -0
@@ -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
ccw_tester.c new file +420 -0
420 lines not on this page · raw message
Signature · 1 line
2.13.5
PATCH 1/5 s390x/css: introduce css data stream
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).
--- hw/s390x/css.c | 55 +++++++++++++++++++++++++++++++++++++++++ include/hw/s390x/css.h | 67 ++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 122 insertions(+)
hw/s390x/css.c +55 -0
@@ -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)
include/hw/s390x/css.h +67 -0
83 lines not on this page · raw message
Signature · 1 line
2.13.5
Re: [Qemu-devel] [PATCH 1/5] s390x/css: introduce css data stream
Quoted 4 lines · On Tue, 5 Sep 2017 13:16:41 +0200 Halil Pasic <[email protected]> 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/
Quoted 12 lines
Signed-off-by Halil Pasic <[email protected]>--- hw/s390x/css.c | 55 +++++++++++++++++++++++++++++++++++++++++ include/hw/s390x/css.h | 67 ++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 122 insertions(+)hw/s390x/css.c +0
-0
hw/s390x/css.c static CCW1 copy_ccw_from_guest(hwaddr addr, bool fmt1) · line 788
Quoted 6 lines
hw/s390x/css.c +3
-0@@ -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/
Quoted 15 lines
hw/s390x/css.c +15
-0+*/ +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);
hw/s390x/css.c static CCW1 copy_ccw_from_guest(hwaddr addr, bool fmt1) · line 813
Quoted 10 lines
hw/s390x/css.c +10
-0+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?
Quoted 5 lines
hw/s390x/css.c +5
-0+return -EINVAL ; + } +incr: + cds->at_byte += len; + cds->cda += len;
hw/s390x/css.c static CCW1 copy_ccw_from_guest(hwaddr addr, bool fmt1) · line 828
Quoted 10 lines
hw/s390x/css.c +10
-0+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().
hw/s390x/css.c static CCW1 copy_ccw_from_guest(hwaddr addr, bool fmt1) · line 838
Quoted 10 lines
hw/s390x/css.c +10
-0+ 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.)
Quoted 5 lines
hw/s390x/css.c +2
-0+ } +}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
Quoted 10 lines · Cornelia Huck
Quoted 4 lines · On Tue, 5 Sep 2017 13:16:41 +0200 Halil Pasic <[email protected]> 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.
Quoted 7 lines
Quoted 7 lines
Signed-off-by Halil Pasic <[email protected]>--- hw/s390x/css.c | 55 +++++++++++++++++++++++++++++++++++++++++ include/hw/s390x/css.h | 67 ++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 122 insertions(+)
Quoted 14 lines
hw/s390x/css.c static CCW1 copy_ccw_from_guest(hwaddr addr, bool fmt1) · line 788
Quoted 11 lines
hw/s390x/css.c +3
-0@@ -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.
Quoted 21 lines
Quoted 21 lines
hw/s390x/css.c +21
-0+*/ +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; + }
Quoted 9 lines
hw/s390x/css.c static CCW1 copy_ccw_from_guest(hwaddr addr, bool fmt1) · line 813
Quoted 4 lines
hw/s390x/css.c +4
-0+ 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.
Quoted 10 lines
Quoted 10 lines
hw/s390x/css.c +10
-0+return -EINVAL ; + } +incr: + cds->at_byte += len; + cds->cda += len; +return 0 ; +} + +void ccw_dstream_init (CcwDataStream *cds,CCW1 const *ccw,ORB const *orb) +{
Quoted 8 lines
hw/s390x/css.c static CCW1 copy_ccw_from_guest(hwaddr addr, bool fmt1) · line 828
Quoted 5 lines
hw/s390x/css.c +5
-0+/* +* 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.
Quoted 12 lines
hw/s390x/css.c static CCW1 copy_ccw_from_guest(hwaddr addr, bool fmt1) · line 838
Quoted 11 lines
hw/s390x/css.c +10
-0+ 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);
Quoted 5 lines
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.
Quoted 6 lines
Quoted 5 lines
hw/s390x/css.c +2
-0+ } +}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
Quoted 26 lines · On Wed, 6 Sep 2017 14:40:48 +0200 Halil Pasic <[email protected]> wrote:
Quoted 15 lines · Cornelia Huck
Quoted 12 lines · On Tue, 5 Sep 2017 13:16:41 +0200 Halil Pasic <[email protected]> wrote:
hw/s390x/css.c +12
-0+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; + }Quoted 9 lines
hw/s390x/css.c static CCW1 copy_ccw_from_guest(hwaddr addr, bool fmt1) · line 813
Quoted 4 lines
hw/s390x/css.c +4
-0+ 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?
Quoted 6 lines
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.
Quoted 21 lines
Quoted 11 lines
Quoted 11 lines
hw/s390x/css.c +10
-0+return -EINVAL ; + } +incr: + cds->at_byte += len; + cds->cda += len; +return 0 ; +} + +void ccw_dstream_init (CcwDataStream *cds,CCW1 const *ccw,ORB const *orb) +{Quoted 8 lines
hw/s390x/css.c static CCW1 copy_ccw_from_guest(hwaddr addr, bool fmt1) · line 828
Quoted 5 lines
hw/s390x/css.c +5
-0+/* +* 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().
Quoted 2 lines
Why
g_assert? I thinkg_assertcomes form a test framework, this is not test code.
g_assert() is glib, no?
Quoted 2 lines
I feel more comfortable having this assert around.
Let's revisit that when we add mida support :) I don't really object to it.
Quoted 13 lines
Quoted 13 lines
Quoted 12 lines
hw/s390x/css.c +10
-0+ 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);
Quoted 8 lines
Quoted 5 lines
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
Quoted 10 lines · Cornelia Huck
Quoted 10 lines
Quoted 9 lines
hw/s390x/css.c static CCW1 copy_ccw_from_guest(hwaddr addr, bool fmt1) · line 828
Quoted 7 lines
hw/s390x/css.c +7
-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().
Quoted 4 lines
Quoted 2 lines
Why
g_assert? I thinkg_assertcomes 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
Quoted 19 lines · On Mon, 11 Sep 2017 18:36:00 +0200 Halil Pasic <[email protected]> wrote:
Quoted 10 lines · Cornelia Huck
Quoted 10 lines
Quoted 9 lines
hw/s390x/css.c static CCW1 copy_ccw_from_guest(hwaddr addr, bool fmt1) · line 828
Quoted 7 lines
hw/s390x/css.c +7
-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().Quoted 4 lines
Quoted 2 lines
Why
g_assert? I thinkg_assertcomes 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
Quoted 8 lines
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_assertin 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
Quoted 28 lines · Cornelia Huck
Quoted 25 lines · On Mon, 11 Sep 2017 18:36:00 +0200 Halil Pasic <[email protected]> wrote:
Quoted 10 lines · Cornelia Huck
Quoted 10 lines
Quoted 9 lines
hw/s390x/css.c static CCW1 copy_ccw_from_guest(hwaddr addr, bool fmt1) · line 828
Quoted 7 lines
hw/s390x/css.c +7
-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().Quoted 4 lines
Quoted 2 lines
Why
g_assert? I thinkg_assertcomes 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.htmlThe 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."
Quoted 9 lines
Quoted 2 lines
So yes, it's both glib and testing framework. This is why I ask why should one use
g_assertin 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_assertorg_assert_not_reachedin anything but test code, but that is because those otherg_assertscan be made non-fatal.g_assert_not_reacheddoes 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.
Quoted 3 lines
I have it somewhere in the back of my mind that
g_assertshould be preferred...
OK, I will use g_assert.
PATCH 2/5 s390x/css: use ccw data stream
In reply to Halil PasicReplace 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.
--- hw/s390x/css.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
hw/s390x/css.c +3 -2
@@ -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 ;
Signature · 1 line
2.13.5
Re: [Qemu-devel] [PATCH 2/5] s390x/css: use ccw data stream
Quoted 22 lines · On Tue, 5 Sep 2017 13:16:42 +0200 Halil Pasic <[email protected]> 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 <[email protected]>--- hw/s390x/css.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)hw/s390x/css.c +1
-0@@ -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. */
hw/s390x/css.c static int css_interpret_ccw(SubchDev *sch, hwaddr ccw_addr, · line 908
Quoted 6 lines
hw/s390x/css.c +1
-1@@ -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()?
hw/s390x/css.c static int css_interpret_ccw(SubchDev *sch, hwaddr ccw_addr, · line 935
Quoted 9 lines
hw/s390x/css.c +1
-1memset (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
Quoted 2 lines
hw/s390x/css.c +0
-0ret =0 ;break ;
Re: [Qemu-devel] [PATCH 2/5] s390x/css: use ccw data stream
Quoted 19 lines · Cornelia Huck
Quoted 16 lines · On Tue, 5 Sep 2017 13:16:42 +0200 Halil Pasic <[email protected]> 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 <[email protected]>--- hw/s390x/css.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)hw/s390x/css.c +0
-0@@ -890,6 +890,7 @@ static int css_interpret_ccw(SubchDev *sch, hwaddr ccw_addr, }
Quoted 14 lines
hw/s390x/css.c static int css_interpret_ccw(SubchDev *sch, hwaddr ccw_addr, · line 908
Quoted 12 lines
hw/s390x/css.c +2
-1/* 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.
Quoted 16 lines
hw/s390x/css.c static int css_interpret_ccw(SubchDev *sch, hwaddr ccw_addr, · line 935
Quoted 10 lines
hw/s390x/css.c +1
-1memset (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
Quoted 2 lines
hw/s390x/css.c +0
-0ret =0 ;break ;
Re: [Qemu-devel] [PATCH 2/5] s390x/css: use ccw data stream
In reply to Halil PasicQuoted 22 lines · Halil Pasic
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 <[email protected]>--- hw/s390x/css.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)hw/s390x/css.c +1
-0@@ -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. */
hw/s390x/css.c static int css_interpret_ccw(SubchDev *sch, hwaddr ccw_addr, · line 907
Quoted 5 lines
hw/s390x/css.c +1
-1@@ -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 ?
hw/s390x/css.c static int css_interpret_ccw(SubchDev *sch, hwaddr ccw_addr, · line 934
Quoted 9 lines
hw/s390x/css.c +1
-1sch->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 ?
Quoted 4 lines
hw/s390x/css.c +0
-0sch->curr_status.scsw.count = ccw.count - len; ret =0 ;break ;
Signature · 2 lines
Pierre Morel Linux/KVM/QEMU in Böblingen - Germany
Re: [Qemu-devel] [PATCH 2/5] s390x/css: use ccw data stream
Quoted 33 lines · Pierre Morel
Quoted 23 lines · Halil Pasic
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 <[email protected]>---
hw/s390x/css.c| 5 +++--1 file changed, 3 insertions(+), 2 deletions(-)hw/s390x/css.c +1
-0@@ -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. */hw/s390x/css.c static int css_interpret_ccw(SubchDev *sch, hwaddr · line 908
Quoted 6 lines
hw/s390x/css.c +1
-1@@ -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 ?
Quoted 19 lines
hw/s390x/css.c static int css_interpret_ccw(SubchDev *sch, hwaddr · line 935
Quoted 10 lines
hw/s390x/css.c +0
-0sch->curr_status.scsw.count = ccw.count - len; memset(sch->sense_data, 0, sizeof(sch->sense_data));ret = 0;
hw/s390x/css.c +1
-1@@ -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 ?
Quoted 4 lines
hw/s390x/css.c +0
-0sch->curr_status.scsw.count = ccw.count - len; ret = 0;break;
Forget it I took the wrong serie.
Sorry
Signature · 2 lines
Pierre Morel Linux/KVM/QEMU in Böblingen - Germany
Re: [Qemu-devel] [PATCH 2/5] s390x/css: use ccw data stream
In reply to Pierre MorelQuoted 31 lines · On Thu, 21 Sep 2017 11:33:03 +0200 Pierre Morel <[email protected]>…
Quoted 22 lines · Halil Pasic
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 <[email protected]>--- hw/s390x/css.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)hw/s390x/css.c +1
-0@@ -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. */ hw/s390x/css.c static int css_interpret_ccw(SubchDev *sch, hwaddr ccw_addr, · line 907
Quoted 5 lines
hw/s390x/css.c +1
-1@@ -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 ?
Quoted 18 lines
hw/s390x/css.c static int css_interpret_ccw(SubchDev *sch, hwaddr ccw_addr, · line 934
Quoted 9 lines
hw/s390x/css.c +1
-1sch->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 ?
Quoted 4 lines
hw/s390x/css.c +0
-0sch->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.
PATCH 3/5 virtio-ccw: use ccw data stream
In reply to Halil PasicReplace 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.
---
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(-)
hw/s390x/virtio-ccw.c +48 -110
@@ -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 ;
Signature · 1 line
2.13.5
Re: [Qemu-devel] [PATCH 3/5] virtio-ccw: use ccw data stream
Quoted 11 lines · On Tue, 5 Sep 2017 13:16:43 +0200 Halil Pasic <[email protected]> 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 <[email protected]>---
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?
Quoted 24 lines
--- hw/s390x/virtio-ccw.c | 158 +++++++++++++++----------------------------------- 1 file changed, 48 insertions(+), 110 deletions(-)hw/s390x/virtio-ccw.c +0
-12@@ -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,
hw/s390x/virtio-ccw.c static int virtio_ccw_handle_set_vq(SubchDev *sch, CCW1 ccw, bool check_len, · line 294
Quoted 10 lines
hw/s390x/virtio-ccw.c +5
-5- 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 :)
Quoted 35 lines
hw/s390x/virtio-ccw.c +6
-24ret =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 ;
(...)
hw/s390x/virtio-ccw.c static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw) · line 448
Quoted 4 lines
hw/s390x/virtio-ccw.c +0
-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?
hw/s390x/virtio-ccw.c static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw) · line 449
Quoted 2 lines
hw/s390x/virtio-ccw.c +1
-1-cpu_physical_memory_write (ccw.cda, vdev->config, len); +/* TODO we may have made -EINVAL out of -EFAULT */
Eek.
hw/s390x/virtio-ccw.c static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw) · line 453
Quoted 4 lines
hw/s390x/virtio-ccw.c +1
-0+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
Quoted 9 lines · Cornelia Huck
Quoted 6 lines · On Tue, 5 Sep 2017 13:16:43 +0200 Halil Pasic <[email protected]> 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 <[email protected]>
Quoted 7 lines
Quoted 5 lines
---
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.
Quoted 29 lines
Quoted 29 lines
--- hw/s390x/virtio-ccw.c | 158 +++++++++++++++----------------------------------- 1 file changed, 48 insertions(+), 110 deletions(-)hw/s390x/virtio-ccw.c +0
-16@@ -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,
Quoted 8 lines
hw/s390x/virtio-ccw.c static int virtio_ccw_handle_set_vq(SubchDev *sch, CCW1 ccw, bool check_len, · line 294
Quoted 6 lines
hw/s390x/virtio-ccw.c +5
-1- 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.
Quoted 46 lines
Quoted 36 lines
hw/s390x/virtio-ccw.c +6
-24ret =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 ;(...)
hw/s390x/virtio-ccw.c static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw) · line 448
Quoted 4 lines
hw/s390x/virtio-ccw.c +0
-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?
Quoted 5 lines
hw/s390x/virtio-ccw.c static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw) · line 449
Quoted 2 lines
hw/s390x/virtio-ccw.c +1
-1-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.
Quoted 7 lines
hw/s390x/virtio-ccw.c static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw) · line 453
Quoted 4 lines
hw/s390x/virtio-ccw.c +1
-0+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
Quoted 14 lines · On Wed, 6 Sep 2017 14:49:52 +0200 Halil Pasic <[email protected]> wrote:
Quoted 13 lines · Cornelia Huck
hw/s390x/virtio-ccw.c static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw) · line 448
Quoted 4 lines · On Tue, 5 Sep 2017 13:16:43 +0200 Halil Pasic <[email protected]> wrote:
hw/s390x/virtio-ccw.c +0
-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?
Quoted 2 lines
hw/s390x/virtio-ccw.c +1
-1-cpu_physical_memory_write (ccw.cda, vdev->config, len); +/* TODO we may have made -EINVAL out of -EFAULT */
Quoted 8 lines
Quoted 2 lines
Eek.
Actually I wanted to send a patch which gets rid of the
-EFAULTcase insch_handle_start_func_virtual.IMHO a program check is more appropriate here. We did the EFAULT if
cpu_physical_memory_mapfailed, 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
In reply to Cornelia HuckQuoted 6 lines · Cornelia Huck
hw/s390x/virtio-ccw.c static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw) · line 448
Quoted 4 lines
hw/s390x/virtio-ccw.c +0
-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?
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
Quoted 8 lines · On Mon, 11 Sep 2017 20:14:59 +0200 Halil Pasic <[email protected]> wrote:
Quoted 6 lines · Cornelia Huck
hw/s390x/virtio-ccw.c static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw) · line 448
Quoted 4 lines
hw/s390x/virtio-ccw.c +0
-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?
Quoted 4 lines
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
Quoted 11 lines · Cornelia Huck
Quoted 8 lines · On Mon, 11 Sep 2017 20:14:59 +0200 Halil Pasic <[email protected]> wrote:
Quoted 6 lines · Cornelia Huck
hw/s390x/virtio-ccw.c static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw) · line 448
Quoted 4 lines
hw/s390x/virtio-ccw.c +0
-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?
Quoted 7 lines
Quoted 4 lines
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.
PATCH 4/5 s390x/css: support ccw IDA
In reply to Halil PasicLet's add indirect data addressing support for our virtual channel subsystem. This implementation does no prefetching, but steps trough the idal as we go.
--- hw/s390x/css.c | 110 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 109 insertions(+), 1 deletion(-)
hw/s390x/css.c +109 -1
@@ -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; } }
Signature · 1 line
2.13.5
Re: [Qemu-devel] [PATCH 4/5] s390x/css: support ccw IDA
Quoted 14 lines · On Tue, 5 Sep 2017 13:16:44 +0200 Halil Pasic <[email protected]> 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 <[email protected]>--- hw/s390x/css.c | 110 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 109 insertions(+), 1 deletion(-)hw/s390x/css.c +0
-0
hw/s390x/css.c incr: · line 822
Quoted 4 lines
hw/s390x/css.c +1
-0@@ -819,6 +819,114 @@ incr:return 0 ; } +/* retuns values between 1 and bs, where bs is a power of 2 */
'bs' is 'block size'?
Quoted 11 lines
hw/s390x/css.c +11
-0+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)
hw/s390x/css.c incr: · line 843
Quoted 10 lines
hw/s390x/css.c +10
-0+{ +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]
Quoted 75 lines
hw/s390x/css.c +75
-0+ } + 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) {
hw/s390x/css.c incr: · line 928
Quoted 10 lines
hw/s390x/css.c +10
-0+ 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?
Quoted 13 lines
hw/s390x/css.c +2
-1+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
Quoted 20 lines · Cornelia Huck
Quoted 17 lines · On Tue, 5 Sep 2017 13:16:44 +0200 Halil Pasic <[email protected]> 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 <[email protected]>--- hw/s390x/css.c | 110 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 109 insertions(+), 1 deletion(-)hw/s390x/css.c +0
-0@@ -819,6 +819,114 @@ incr:return 0 ; }
Quoted 3 lines
hw/s390x/css.c incr: · line 822
Quoted 1 line
hw/s390x/css.c +1
-0+/* retuns values between 1 and bs, where bs is a power of 2 */ 'bs' is 'block size'?
Yes.
Quoted 16 lines
Quoted 16 lines
hw/s390x/css.c +15
-0+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;
Quoted 9 lines
hw/s390x/css.c incr: · line 843
Quoted 6 lines
hw/s390x/css.c +6
-0+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.
Quoted 82 lines
[s/chanel/channel/ :D]
Quoted 80 lines
hw/s390x/css.c +80
-0+ } + 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; + }
Quoted 9 lines
hw/s390x/css.c incr: · line 928
Quoted 5 lines
hw/s390x/css.c +5
-0+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
Quoted 15 lines
Quoted 13 lines
hw/s390x/css.c +2
-1+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
Quoted 21 lines · On Mon, 11 Sep 2017 20:08:16 +0200 Halil Pasic <[email protected]> wrote:
Quoted 20 lines · Cornelia Huck
Quoted 17 lines · On Tue, 5 Sep 2017 13:16:44 +0200 Halil Pasic <[email protected]> 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 <[email protected]>--- hw/s390x/css.c | 110 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 109 insertions(+), 1 deletion(-)hw/s390x/css.c +0
-0@@ -819,6 +819,114 @@ incr:return 0 ; }
Quoted 5 lines
Quoted 3 lines
hw/s390x/css.c incr: · line 822
Quoted 1 line
hw/s390x/css.c +1
-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/
Quoted 133 lines
Quoted 62 lines
Quoted 62 lines
hw/s390x/css.c +59
-0+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; + }Quoted 9 lines
hw/s390x/css.c incr: · line 928
Quoted 5 lines
hw/s390x/css.c +5
-0+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; }
Quoted 3 lines
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
Quoted 24 lines · Cornelia Huck
Quoted 21 lines · On Mon, 11 Sep 2017 20:08:16 +0200 Halil Pasic <[email protected]> wrote:
Quoted 20 lines · Cornelia Huck
Quoted 17 lines · On Tue, 5 Sep 2017 13:16:44 +0200 Halil Pasic <[email protected]> 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 <[email protected]>--- hw/s390x/css.c | 110 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 109 insertions(+), 1 deletion(-)hw/s390x/css.c +0
-0@@ -819,6 +819,114 @@ incr:return 0 ; }
Quoted 9 lines
Quoted 5 lines
Quoted 3 lines
hw/s390x/css.c incr: · line 822
Quoted 1 line
hw/s390x/css.c +1
-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.
Quoted 2 lines
In any case, s/retuns/returns/
I think I've already fixed that :).
Quoted 133 lines
Quoted 133 lines
Quoted 62 lines
Quoted 62 lines
hw/s390x/css.c +59
-0+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; + }Quoted 9 lines
hw/s390x/css.c incr: · line 928
Quoted 5 lines
hw/s390x/css.c +5
-0+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; }
Quoted 7 lines
Quoted 3 lines
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
Quoted 3 lines · On Wed, 13 Sep 2017 12:31:20 +0200 Halil Pasic <[email protected]> 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.
PATCH 5/5 s390x/ccs: add ccw-tester emulated device
In reply to Halil PasicAdd 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
---
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
hw/s390x/Makefile.objs +1 -0
@@ -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
hw/s390x/ccw-tester.c new file +179 -0
179 lines not on this page · raw message
Signature · 1 line
2.13.5
Re: [Qemu-devel] [PATCH 5/5] s390x/ccs: add ccw-tester emulated device
Quoted 18 lines · On Tue, 5 Sep 2017 13:16:45 +0200 Halil Pasic <[email protected]> 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_tto 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 guestSigned-off-by Halil Pasic <[email protected]>---
Quoted 7 lines
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
Quoted 29 lines · Cornelia Huck
Quoted 18 lines · On Tue, 5 Sep 2017 13:16:45 +0200 Halil Pasic <[email protected]> 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_tto 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 guestSigned-off-by Halil Pasic <[email protected]>---
Quoted 7 lines
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
Quoted 4 lines
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.
Quoted 5 lines
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?
Quoted 2 lines
(I dimly recall other test devices...)
What's on your mind? How do these relate to our problem? Can you give me some pointers?
Quoted 3 lines
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
Quoted 37 lines · On Wed, 6 Sep 2017 16:24:13 +0200 Halil Pasic <[email protected]> wrote:
Quoted 29 lines · Cornelia Huck
Quoted 18 lines · On Tue, 5 Sep 2017 13:16:45 +0200 Halil Pasic <[email protected]> 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_tto 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 guestSigned-off-by Halil Pasic <[email protected]>---
Quoted 7 lines
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.cQuoted 4 lines
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.
Quoted 7 lines
Quoted 5 lines
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 :)
Quoted 6 lines
Quoted 3 lines
(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.
Quoted 7 lines
Quoted 4 lines
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.
Quoted 3 lines
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.
Quoted 8 lines
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
Quoted 46 lines · Cornelia Huck
Quoted 43 lines · On Wed, 6 Sep 2017 16:24:13 +0200 Halil Pasic <[email protected]> wrote:
Quoted 29 lines · Cornelia Huck
Quoted 18 lines · On Tue, 5 Sep 2017 13:16:45 +0200 Halil Pasic <[email protected]> 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_tto 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 guestSigned-off-by Halil Pasic <[email protected]>---
Quoted 7 lines
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.cQuoted 4 lines
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.
Quoted 5 lines
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.
Quoted 6 lines
Quoted 1 line
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.
Quoted 16 lines
Quoted 7 lines
Quoted 4 lines
(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.
Quoted 5 lines
Quoted 4 lines
For the guest tester: Can that be done via the qtest infrastructure somehow?
Quoted 6 lines
Quoted 2 lines
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.
Quoted 17 lines
Quoted 3 lines
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.
Quoted 8 lines
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.
Quoted 6 lines
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
Quoted 54 lines · On Wed, 6 Sep 2017 18:16:29 +0200 Halil Pasic <[email protected]> wrote:
Quoted 46 lines · Cornelia Huck
Quoted 43 lines · On Wed, 6 Sep 2017 16:24:13 +0200 Halil Pasic <[email protected]> wrote:
Quoted 29 lines · Cornelia Huck
Quoted 18 lines · On Tue, 5 Sep 2017 13:16:45 +0200 Halil Pasic <[email protected]> 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_tto 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 guestSigned-off-by Halil Pasic <[email protected]>---
Quoted 7 lines
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.cQuoted 4 lines
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.
Quoted 5 lines
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.
Quoted 6 lines
Quoted 1 line
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 :)
Quoted 4 lines
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.)
Quoted 7 lines
Quoted 7 lines
Quoted 6 lines
Quoted 3 lines
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).
Quoted 7 lines
Quoted 3 lines
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.
Quoted 2 lines
I think it's worth a look.
It certainly is, but you know how it is with resources...
Quoted 12 lines
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.
Quoted 3 lines
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 :)
Quoted 21 lines
Quoted 21 lines
Quoted 4 lines
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.
Quoted 8 lines
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.
Quoted 6 lines
Quoted 3 lines
[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...
Quoted 5 lines
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 :)
Quoted 4 lines
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
[...]
Quoted 20 lines
Quoted 20 lines
Quoted 19 lines
Quoted 7 lines
Quoted 4 lines
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.
Quoted 7 lines
Quoted 5 lines
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 :)
Quoted 8 lines
Quoted 4 lines
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. :)
Quoted 15 lines
Quoted 7 lines
Quoted 7 lines
Quoted 6 lines
Quoted 3 lines
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).
Quoted 7 lines
Quoted 3 lines
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.
Quoted 7 lines
And that's also the problem: You drag the whole kernel infrastructure along with i
Quoted 2 lines
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
Quoted 4 lines · On Thu, 7 Sep 2017 11:10:17 +0200 Janosch Frank <[email protected]>…
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 :)
Quoted 3 lines
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)
Quoted 6 lines
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.
Quoted 3 lines
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
In reply to Cornelia HuckQuoted 38 lines · Cornelia Huck
Quoted 18 lines · On Tue, 5 Sep 2017 13:16:45 +0200 Halil Pasic <[email protected]> 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_tto 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 guestSigned-off-by Halil Pasic <[email protected]>---
Quoted 7 lines
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.cThe 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.
Quoted 5 lines
(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?
Signature · 1 line
Dong Jia Shi
Re: [Qemu-devel] [PATCH 5/5] s390x/ccs: add ccw-tester emulated device
Quoted 56 lines · On Thu, 7 Sep 2017 15:31:09 +0800 Dong Jia Shi <[email protected]>…
Quoted 38 lines · Cornelia Huck
Quoted 18 lines · On Tue, 5 Sep 2017 13:16:45 +0200 Halil Pasic <[email protected]> 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_tto 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 guestSigned-off-by Halil Pasic <[email protected]>---
Quoted 7 lines
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.cThe 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.
Quoted 5 lines
(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...
Quoted 7 lines
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
Quoted 63 lines · Cornelia Huck
Quoted 60 lines · On Thu, 7 Sep 2017 15:31:09 +0800 Dong Jia Shi <[email protected]>…
Quoted 38 lines · Cornelia Huck
Quoted 18 lines · On Tue, 5 Sep 2017 13:16:45 +0200 Halil Pasic <[email protected]> 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_tto 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 guestSigned-off-by Halil Pasic <[email protected]>---
Quoted 7 lines
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.cThe 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.
Quoted 5 lines
(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.
Quoted 6 lines
Quoted 3 lines
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
Quoted 25 lines · On Thu, 7 Sep 2017 12:21:50 +0200 Halil Pasic <[email protected]> wrote:
Quoted 17 lines · Cornelia Huck
Quoted 14 lines · On Thu, 7 Sep 2017 15:31:09 +0800 Dong Jia Shi <[email protected]>…
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.
Quoted 6 lines
Quoted 3 lines
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.
Quoted 6 lines
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
Quoted 28 lines · Cornelia Huck
Quoted 25 lines · On Thu, 7 Sep 2017 12:21:50 +0200 Halil Pasic <[email protected]> wrote:
Quoted 23 lines · Cornelia Huck
Quoted 10 lines · On Thu, 7 Sep 2017 15:31:09 +0800 Dong Jia Shi <[email protected]>…
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...
Quoted 7 lines
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.
Quoted 9 lines
Quoted 6 lines
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. :()
Signature · 1 line
Dong Jia Shi
Re: [Qemu-devel] [PATCH 5/5] s390x/ccs: add ccw-tester emulated device
Quoted 37 lines · Dong Jia Shi
Quoted 35 lines · Cornelia Huck
Quoted 31 lines · On Thu, 7 Sep 2017 12:21:50 +0200 Halil Pasic <[email protected]> wrote:
Quoted 17 lines · Cornelia Huck
Quoted 14 lines · On Thu, 7 Sep 2017 15:31:09 +0800 Dong Jia Shi <[email protected]>…
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.
Quoted 6 lines
Quoted 3 lines
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).
Quoted 7 lines
Quoted 2 lines
But the vfio-ccw code uses idals... no need to touch the 3270 emulation.
What Halil pointed out is that the
ccw_cbimplementation 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
* Halil Pasic <[email protected]> [2017-09-08 12:28:03 +0200]:
[snip]
Quoted 10 lines
Quoted 5 lines
What Halil pointed out is that the
ccw_cbimplementation 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. ;)
Signature · 1 line
Dong Jia Shi
Re: [Qemu-devel] [PATCH 5/5] s390x/ccs: add ccw-tester emulated device
In reply to Cornelia Huck* Cornelia Huck <[email protected]> [2017-09-07 10:08:17 +0200]:
[...]
Quoted 14 lines
Quoted 14 lines
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.
Quoted 6 lines
Quoted 3 lines
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.
Signature · 1 line
Dong Jia Shi
Re: [Qemu-devel] [PATCH 5/5] s390x/ccs: add ccw-tester emulated device
Quoted 45 lines · On Thu, 21 Sep 2017 16:45:47 +0800 Dong Jia Shi <[email protected]>…
* Cornelia Huck <[email protected]> [2017-09-07 10:08:17 +0200]:
[...]
Quoted 14 lines
Quoted 14 lines
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.
Quoted 6 lines
Quoted 3 lines
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:(quoted diff) +0
-2-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 thepwrite().
Quoted 3 lines
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
Quoted 48 lines · Cornelia Huck
Quoted 45 lines · On Thu, 21 Sep 2017 16:45:47 +0800 Dong Jia Shi <[email protected]>…
* Cornelia Huck <[email protected]> [2017-09-07 10:08:17 +0200]:
[...]
Quoted 14 lines
Quoted 14 lines
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.
Quoted 6 lines
Quoted 3 lines
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:(quoted diff) +0
-2-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 thepwrite().
Quoted 6 lines
Quoted 3 lines
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.
Quoted 2 lines
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>] 0x1046404e6The 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. ;)
Quoted 2 lines
(I'm also wondering how diag is handled?)
Not looking into this yet. :-/
Quoted 1 line
Signature · 1 line
Dong Jia Shi
Re: [Qemu-devel] [PATCH 5/5] s390x/ccs: add ccw-tester emulated device
* Dong Jia Shi <[email protected]> [2017-09-26 15:48:56 +0800]:
[...]
Quoted 83 lines
Quoted 21 lines
Quoted 21 lines
Tried to test with the following method:
1. Start g1 (first level guest on kvm a host) with a virtio blk device defined:(quoted diff) +0
-2-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 thepwrite().Quoted 6 lines
Quoted 3 lines
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_ID0xe4[OK]
2.CCW_CMD_NOOP0x03[OK]
3.CCW_CMD_SET_VIRTIO_REV0x83[OK]
4.CCW_CMD_VDEV_RESET0x33[FAILED]So this is still in the phase of setting up the device.
Quoted 2 lines
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()
callspfn_array_alloc_pin()with a zero @len parameter.
So it results in a-EINVALreturn.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:000ce330b0b00000TEID:000ce330b0b00800
Fault in primary space mode while using user ASCE.
AS:000000003b6cc1c7 R3:0000000000000024
Segmentation faultdmesg 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
Quoted 3 lines
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:6666 ../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:71PC not saved
Googled lock elision for a while, and I still have no idea on this problem. Any suggestions on this?
Signature · 1 line
Dong Jia Shi
Quoted 14 lines · Halil Pasic
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
Quoted 9 lines · On Fri, 8 Sep 2017 12:45:25 +0200 Halil Pasic <[email protected]> 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.
Quoted 3 lines
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.
Quoted 7 lines
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.
Quoted 8 lines · Cornelia Huck
Quoted 5 lines · On Fri, 8 Sep 2017 12:45:25 +0200 Halil Pasic <[email protected]> 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).
Quoted 7 lines
Quoted 4 lines
@Connie
========Does that sound reasonable or would you like more time for v1?
No, sounds good.
Nod.
Quoted 7 lines
Quoted 3 lines
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.
Quoted 11 lines
Quoted 7 lines
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
Quoted 11 lines · On Fri, 8 Sep 2017 13:03:00 +0200 Halil Pasic <[email protected]> wrote:
Quoted 9 lines · Cornelia Huck
Quoted 2 lines · On Fri, 8 Sep 2017 12:45:25 +0200 Halil Pasic <[email protected]> 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.
Quoted 7 lines
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 :)
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
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?
Quoted 5 lines
Quoted 5 lines
Quoted 4 lines
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?
Quoted 7 lines
Quoted 3 lines
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 :)
Quoted 17 lines · Cornelia Huck
Quoted 14 lines · On Fri, 8 Sep 2017 13:03:00 +0200 Halil Pasic <[email protected]> wrote:
Quoted 9 lines · Cornelia Huck
Quoted 2 lines · On Fri, 8 Sep 2017 12:45:25 +0200 Halil Pasic <[email protected]> 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.
Quoted 9 lines
Quoted 4 lines
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.
Quoted 7 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
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= ). WeUnlikely 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.
Quoted 9 lines
Quoted 9 lines
Quoted 8 lines
Quoted 4 lines
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.
Quoted 7 lines
Quoted 3 lines
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