[v1] s390x: kvm-unit-tests: a PONG device for Sub Channels tests

Commit Message

Pierre Morel Nov. 13, 2019, 7:02 p.m. UTC

The PONG device accept two commands: PONG_READ and PONG_WRITE
which allow to read from and write to an internal buffer of
1024 bytes.

The QEMU device is named ccw-pong.

Signed-off-by: Pierre Morel <[e-mail]>
---
 hw/s390x/Makefile.objs  |   1 +
 hw/s390x/ccw-pong.c     | 186 ++++++++++++++++++++++++++++++++++++++++++++++++
 include/hw/s390x/pong.h |  47 ++++++++++++
 3 files changed, 234 insertions(+)
 create mode 100644 hw/s390x/ccw-pong.c
 create mode 100644 include/hw/s390x/pong.h

Comments

Cornelia Huck Nov. 14, 2019, 10:38 a.m. UTC | #1

On Wed, 13 Nov 2019 20:02:33 +0100
Pierre Morel <[e-mail]> wrote:

Minor nit for $SUBJECT: this isn't a kvm-unit-tests patch, that's just
one consumer :)

> The PONG device accept two commands: PONG_READ and PONG_WRITE
> which allow to read from and write to an internal buffer of
> 1024 bytes.
> 
> The QEMU device is named ccw-pong.
> 
> Signed-off-by: Pierre Morel <[e-mail]>
> ---
>  hw/s390x/Makefile.objs  |   1 +
>  hw/s390x/ccw-pong.c     | 186 ++++++++++++++++++++++++++++++++++++++++++++++++
>  include/hw/s390x/pong.h |  47 ++++++++++++
>  3 files changed, 234 insertions(+)
>  create mode 100644 hw/s390x/ccw-pong.c
>  create mode 100644 include/hw/s390x/pong.h
> 
> diff --git a/hw/s390x/Makefile.objs b/hw/s390x/Makefile.objs
> index ee91152..3a83438 100644
> --- a/hw/s390x/Makefile.objs
> +++ b/hw/s390x/Makefile.objs
> @@ -32,6 +32,7 @@ obj-$(CONFIG_KVM) += tod-kvm.o
>  obj-$(CONFIG_KVM) += s390-skeys-kvm.o
>  obj-$(CONFIG_KVM) += s390-stattrib-kvm.o s390-mchk.o
>  obj-y += s390-ccw.o
> +obj-y += ccw-pong.o

Not sure if unconditionally introducing a test device is a good idea.

>  obj-y += ap-device.o
>  obj-y += ap-bridge.o
>  obj-y += s390-sei.o
> diff --git a/hw/s390x/ccw-pong.c b/hw/s390x/ccw-pong.c
> new file mode 100644
> index 0000000..e7439d5
> --- /dev/null
> +++ b/hw/s390x/ccw-pong.c
> @@ -0,0 +1,186 @@
> +/*
> + * CCW PING-PONG
> + *
> + * Copyright 2019 IBM Corp.
> + * Author(s): Pierre Morel <[e-mail]>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or (at
> + * your option) any later version. See the COPYING file in the top-level
> + * directory.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qapi/error.h"
> +#include "qemu/module.h"
> +#include "cpu.h"
> +#include "exec/address-spaces.h"
> +#include "hw/s390x/css.h"
> +#include "hw/s390x/css-bridge.h"
> +#include "hw/qdev-properties.h"
> +#include "hw/s390x/pong.h"
> +
> +#define PONG_BUF_SIZE 0x1000
> +static char buf[PONG_BUF_SIZE] = "Hello world\n";
> +
> +static inline int pong_rw(CCW1 *ccw, char *p, int len, bool dir)
> +{
> +    int ret;
> +
> +    ret = address_space_rw(&address_space_memory, ccw->cda,
> +                           MEMTXATTRS_UNSPECIFIED,
> +                           (unsigned char *)buf, len, dir);
> +
> +    return (ret == MEMTX_OK) ? -EIO : 0;
> +}
> +
> +/* Handle READ ccw commands from guest */
> +static int handle_payload_read(CcwPONGDevice *dev, CCW1 *ccw)
> +{
> +    CcwDevice *ccw_dev = CCW_DEVICE(dev);
> +    int len;
> +
> +    if (!ccw->cda) {
> +        return -EFAULT;
> +    }
> +
> +    if (ccw->count > PONG_BUF_SIZE) {
> +        len = PONG_BUF_SIZE;
> +        ccw_dev->sch->curr_status.scsw.count = ccw->count - PONG_BUF_SIZE;
> +    } else {
> +        len = ccw->count;
> +        ccw_dev->sch->curr_status.scsw.count = 0;
> +    }
> +
> +    return pong_rw(ccw, buf, len, 1);
> +}
> +
> +/*
> + * Handle WRITE ccw commands to write data to client
> + * The SCSW count is set to the number of bytes not transfered.
> + */
> +static int handle_payload_write(CcwPONGDevice *dev, CCW1 *ccw)
> +{
> +    CcwDevice *ccw_dev = CCW_DEVICE(dev);
> +    int len;
> +
> +    if (!ccw->cda) {
> +        ccw_dev->sch->curr_status.scsw.count = ccw->count;
> +        return -EFAULT;
> +    }
> +
> +    if (ccw->count > PONG_BUF_SIZE) {
> +        len = PONG_BUF_SIZE;
> +        ccw_dev->sch->curr_status.scsw.count = ccw->count - PONG_BUF_SIZE;
> +    } else {
> +        len = ccw->count;
> +        ccw_dev->sch->curr_status.scsw.count = 0;
> +    }
> +
> +    return pong_rw(ccw, buf, len, 0);

Can you please use the dstream infrastructure for read/write handling?

You also seem to miss some basic checks e.g. for short reads/writes
with and without SLI set.

> +}
> +
> +static int pong_ccw_cb(SubchDev *sch, CCW1 ccw)
> +{
> +    int rc = 0;
> +    CcwPONGDevice *dev = sch->driver_data;
> +
> +    switch (ccw.cmd_code) {
> +    case PONG_WRITE:
> +        rc = handle_payload_write(dev, &ccw);
> +        break;
> +    case PONG_READ:
> +        rc = handle_payload_read(dev, &ccw);
> +        break;
> +    default:
> +        rc = -ENOSYS;
> +        break;
> +    }
> +
> +    if (rc == -EIO) {
> +        /* I/O error, specific devices generate specific conditions */
> +        SCHIB *schib = &sch->curr_status;
> +
> +        sch->curr_status.scsw.dstat = SCSW_DSTAT_UNIT_CHECK;
> +        sch->sense_data[0] = 0x40;    /* intervention-req */

This is really odd. If it succeeds, you generate a unit check with
intervention required? Confused. At the very least, this requires some
description as to how this device is supposed to interact with the
driver.

> +        schib->scsw.ctrl &= ~SCSW_ACTL_START_PEND;
> +        schib->scsw.ctrl &= ~SCSW_CTRL_MASK_STCTL;
> +        schib->scsw.ctrl |= SCSW_STCTL_PRIMARY | SCSW_STCTL_SECONDARY |
> +                   SCSW_STCTL_ALERT | SCSW_STCTL_STATUS_PEND;
> +    }
> +    return rc;
> +}
> +
> +static void pong_ccw_realize(DeviceState *ds, Error **errp)
> +{
> +    uint16_t chpid;
> +    CcwPONGDevice *dev = CCW_PONG(ds);
> +    CcwDevice *cdev = CCW_DEVICE(ds);
> +    CCWDeviceClass *cdk = CCW_DEVICE_GET_CLASS(cdev);
> +    SubchDev *sch;
> +    Error *err = NULL;
> +
> +    sch = css_create_sch(cdev->devno, errp);
> +    if (!sch) {
> +        return;
> +    }
> +
> +    sch->driver_data = dev;
> +    cdev->sch = sch;
> +    chpid = css_find_free_chpid(sch->cssid);
> +
> +    if (chpid > MAX_CHPID) {
> +        error_setg(&err, "No available chpid to use.");
> +        goto out_err;
> +    }
> +
> +    sch->id.reserved = 0xff;
> +    sch->id.cu_type = CCW_PONG_CU_TYPE;
> +    css_sch_build_virtual_schib(sch, (uint8_t)chpid,
> +                                CCW_PONG_CHPID_TYPE);
> +    sch->do_subchannel_work = do_subchannel_work_virtual;
> +    sch->ccw_cb = pong_ccw_cb;
> +
> +    cdk->realize(cdev, &err);
> +    if (err) {
> +        goto out_err;
> +    }
> +
> +    css_reset_sch(sch);
> +    return;
> +
> +out_err:
> +    error_propagate(errp, err);
> +    css_subch_assign(sch->cssid, sch->ssid, sch->schid, sch->devno, NULL);
> +    cdev->sch = NULL;
> +    g_free(sch);
> +}
> +
> +static Property pong_ccw_properties[] = {
> +    DEFINE_PROP_END_OF_LIST(),
> +};
> +
> +static void pong_ccw_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +
> +    dc->props = pong_ccw_properties;
> +    dc->bus_type = TYPE_VIRTUAL_CSS_BUS;
> +    dc->realize = pong_ccw_realize;
> +    dc->hotpluggable = false;
> +    set_bit(DEVICE_CATEGORY_DISPLAY, dc->categories);

Huh? misc seems like a better idea for this :)

> +}
> +
> +static const TypeInfo pong_ccw_info = {
> +    .name = TYPE_CCW_PONG,
> +    .parent = TYPE_CCW_DEVICE,
> +    .instance_size = sizeof(CcwPONGDevice),
> +    .class_init = pong_ccw_class_init,
> +    .class_size = sizeof(CcwPONGClass),
> +};
> +
> +static void pong_ccw_register(void)
> +{
> +    type_register_static(&pong_ccw_info);
> +}
> +
> +type_init(pong_ccw_register)

Some questions regarding this device and its intended usage:

- What are you trying to test? Basic ccw processing, or something more
  specific? Is there any way you can use the kvm-unit-test
  infrastructure to test basic processing with an existing device?
- Who is instantiating this device? Only the kvm-unit-test?
- Can you instantiate multiple instances? Does that make sense? If yes,
  it should probably not request a new chpid every time :)
- What happens if someone instantiates this by hand? The only drawback
  is that it uses up a subchannel and a chpid, right?
- Do you plan to make this hotpluggable later?

Thomas Huth Nov. 14, 2019, 12:33 p.m. UTC | #2

On 14/11/2019 11.38, Cornelia Huck wrote:
> On Wed, 13 Nov 2019 20:02:33 +0100
> Pierre Morel <[e-mail]> wrote:
> 
> Minor nit for $SUBJECT: this isn't a kvm-unit-tests patch, that's just
> one consumer :)
> 
>> The PONG device accept two commands: PONG_READ and PONG_WRITE
>> which allow to read from and write to an internal buffer of
>> 1024 bytes.
>>
>> The QEMU device is named ccw-pong.
>>
>> Signed-off-by: Pierre Morel <[e-mail]>
>> ---
>>  hw/s390x/Makefile.objs  |   1 +
>>  hw/s390x/ccw-pong.c     | 186 ++++++++++++++++++++++++++++++++++++++++++++++++
>>  include/hw/s390x/pong.h |  47 ++++++++++++
>>  3 files changed, 234 insertions(+)
>>  create mode 100644 hw/s390x/ccw-pong.c
>>  create mode 100644 include/hw/s390x/pong.h
>>
>> diff --git a/hw/s390x/Makefile.objs b/hw/s390x/Makefile.objs
>> index ee91152..3a83438 100644
>> --- a/hw/s390x/Makefile.objs
>> +++ b/hw/s390x/Makefile.objs
>> @@ -32,6 +32,7 @@ obj-$(CONFIG_KVM) += tod-kvm.o
>>  obj-$(CONFIG_KVM) += s390-skeys-kvm.o
>>  obj-$(CONFIG_KVM) += s390-stattrib-kvm.o s390-mchk.o
>>  obj-y += s390-ccw.o
>> +obj-y += ccw-pong.o
> 
> Not sure if unconditionally introducing a test device is a good idea.

This definitely needs a CONFIG switch (which can be "y" by default, but
still we should provide a possibility to disable it)

>>  obj-y += ap-device.o
>>  obj-y += ap-bridge.o
>>  obj-y += s390-sei.o
>> diff --git a/hw/s390x/ccw-pong.c b/hw/s390x/ccw-pong.c
>> new file mode 100644
>> index 0000000..e7439d5
>> --- /dev/null
>> +++ b/hw/s390x/ccw-pong.c
>> @@ -0,0 +1,186 @@
>> +/*
>> + * CCW PING-PONG

Please add a short description here what this device is all about.

>> + * Copyright 2019 IBM Corp.
>> + * Author(s): Pierre Morel <[e-mail]>
>> + *
>> + * This work is licensed under the terms of the GNU GPL, version 2 or (at
>> + * your option) any later version. See the COPYING file in the top-level
>> + * directory.
>> + */
>> +
>> +#include "qemu/osdep.h"
>> +#include "qapi/error.h"
>> +#include "qemu/module.h"
>> +#include "cpu.h"
>> +#include "exec/address-spaces.h"
>> +#include "hw/s390x/css.h"
>> +#include "hw/s390x/css-bridge.h"
>> +#include "hw/qdev-properties.h"
>> +#include "hw/s390x/pong.h"
>> +
>> +#define PONG_BUF_SIZE 0x1000
>> +static char buf[PONG_BUF_SIZE] = "Hello world\n";
>> +
>> +static inline int pong_rw(CCW1 *ccw, char *p, int len, bool dir)
>> +{
>> +    int ret;
>> +
>> +    ret = address_space_rw(&address_space_memory, ccw->cda,
>> +                           MEMTXATTRS_UNSPECIFIED,
>> +                           (unsigned char *)buf, len, dir);
>> +
>> +    return (ret == MEMTX_OK) ? -EIO : 0;

If return code was OK, then you return an EIO error? That looks weird?

>> +}
>> +
>> +/* Handle READ ccw commands from guest */
>> +static int handle_payload_read(CcwPONGDevice *dev, CCW1 *ccw)
>> +{
>> +    CcwDevice *ccw_dev = CCW_DEVICE(dev);
>> +    int len;
>> +
>> +    if (!ccw->cda) {
>> +        return -EFAULT;
>> +    }
>> +
>> +    if (ccw->count > PONG_BUF_SIZE) {
>> +        len = PONG_BUF_SIZE;
>> +        ccw_dev->sch->curr_status.scsw.count = ccw->count - PONG_BUF_SIZE;
>> +    } else {
>> +        len = ccw->count;
>> +        ccw_dev->sch->curr_status.scsw.count = 0;
>> +    }
>> +
>> +    return pong_rw(ccw, buf, len, 1);
>> +}
>> +
>> +/*
>> + * Handle WRITE ccw commands to write data to client
>> + * The SCSW count is set to the number of bytes not transfered.
>> + */
>> +static int handle_payload_write(CcwPONGDevice *dev, CCW1 *ccw)
>> +{
>> +    CcwDevice *ccw_dev = CCW_DEVICE(dev);
>> +    int len;
>> +
>> +    if (!ccw->cda) {
>> +        ccw_dev->sch->curr_status.scsw.count = ccw->count;
>> +        return -EFAULT;
>> +    }
>> +
>> +    if (ccw->count > PONG_BUF_SIZE) {
>> +        len = PONG_BUF_SIZE;
>> +        ccw_dev->sch->curr_status.scsw.count = ccw->count - PONG_BUF_SIZE;
>> +    } else {
>> +        len = ccw->count;
>> +        ccw_dev->sch->curr_status.scsw.count = 0;
>> +    }
>> +
>> +    return pong_rw(ccw, buf, len, 0);
> 
> Can you please use the dstream infrastructure for read/write handling?
> 
> You also seem to miss some basic checks e.g. for short reads/writes
> with and without SLI set.
> 
>> +}
>> +
>> +static int pong_ccw_cb(SubchDev *sch, CCW1 ccw)
>> +{
>> +    int rc = 0;
>> +    CcwPONGDevice *dev = sch->driver_data;
>> +
>> +    switch (ccw.cmd_code) {
>> +    case PONG_WRITE:
>> +        rc = handle_payload_write(dev, &ccw);
>> +        break;
>> +    case PONG_READ:
>> +        rc = handle_payload_read(dev, &ccw);
>> +        break;
>> +    default:
>> +        rc = -ENOSYS;
>> +        break;
>> +    }
>> +
>> +    if (rc == -EIO) {
>> +        /* I/O error, specific devices generate specific conditions */
>> +        SCHIB *schib = &sch->curr_status;
>> +
>> +        sch->curr_status.scsw.dstat = SCSW_DSTAT_UNIT_CHECK;
>> +        sch->sense_data[0] = 0x40;    /* intervention-req */
> 
> This is really odd. If it succeeds, you generate a unit check with
> intervention required? Confused. At the very least, this requires some
> description as to how this device is supposed to interact with the
> driver.
> 
>> +        schib->scsw.ctrl &= ~SCSW_ACTL_START_PEND;
>> +        schib->scsw.ctrl &= ~SCSW_CTRL_MASK_STCTL;
>> +        schib->scsw.ctrl |= SCSW_STCTL_PRIMARY | SCSW_STCTL_SECONDARY |
>> +                   SCSW_STCTL_ALERT | SCSW_STCTL_STATUS_PEND;
>> +    }
>> +    return rc;
>> +}
[...]
>> +
>> +static Property pong_ccw_properties[] = {
>> +    DEFINE_PROP_END_OF_LIST(),
>> +};
>> +
>> +static void pong_ccw_class_init(ObjectClass *klass, void *data)
>> +{
>> +    DeviceClass *dc = DEVICE_CLASS(klass);
>> +
>> +    dc->props = pong_ccw_properties;

As long as there are no properties, I think you can simply drop that line.

 Thomas

Halil Pasic Nov. 14, 2019, 1:02 p.m. UTC | #3

On Thu, 14 Nov 2019 11:38:23 +0100
Cornelia Huck <[e-mail]> wrote:

> On Wed, 13 Nov 2019 20:02:33 +0100
> Pierre Morel <[e-mail]> wrote:
> 
> Minor nit for $SUBJECT: this isn't a kvm-unit-tests patch, that's just
> one consumer :)

And subchannel is one word in s390-speak.

> 

[..]

> Some questions regarding this device and its intended usage:
> 
> - What are you trying to test? Basic ccw processing, or something more
>   specific? Is there any way you can use the kvm-unit-test
>   infrastructure to test basic processing with an existing device?

I'm also curious about the big picture (what is in scope and what out
of scope). Your design should be evaluated in the light of intended
usage.

BTW have you had a look at this abandoned patch-set of mine:

https://lists.gnu.org/archive/html/qemu-devel/2017-11/msg04220.html

We made some different design decisions, while aiming essentially for the
same. Maybe it's due to different scope, maybe not. For instance one
can't test IDA with PONG, I guess.

Regards,
Halil

> - Who is instantiating this device? Only the kvm-unit-test?
> - Can you instantiate multiple instances? Does that make sense? If yes,
>   it should probably not request a new chpid every time :)
> - What happens if someone instantiates this by hand? The only drawback
>   is that it uses up a subchannel and a chpid, right?
> - Do you plan to make this hotpluggable later?
> 
>

Cornelia Huck Nov. 14, 2019, 1:19 p.m. UTC | #4

On Thu, 14 Nov 2019 14:02:35 +0100
Halil Pasic <[e-mail]> wrote:

> On Thu, 14 Nov 2019 11:38:23 +0100
> Cornelia Huck <[e-mail]> wrote:
> 
> > On Wed, 13 Nov 2019 20:02:33 +0100
> > Pierre Morel <[e-mail]> wrote:
> > 
> > Minor nit for $SUBJECT: this isn't a kvm-unit-tests patch, that's just
> > one consumer :)  
> 
> And subchannel is one word in s390-speak.
> 
> >   
> 
> [..]
> 
> > Some questions regarding this device and its intended usage:
> > 
> > - What are you trying to test? Basic ccw processing, or something more
> >   specific? Is there any way you can use the kvm-unit-test
> >   infrastructure to test basic processing with an existing device?  
> 
> I'm also curious about the big picture (what is in scope and what out
> of scope). Your design should be evaluated in the light of intended
> usage.
> 
> BTW have you had a look at this abandoned patch-set of mine:
> 
> https://lists.gnu.org/archive/html/qemu-devel/2017-11/msg04220.html

Do you recall why it was abandoned? Or did we just forget to follow up
on it?

> 
> We made some different design decisions, while aiming essentially for the
> same. Maybe it's due to different scope, maybe not. For instance one
> can't test IDA with PONG, I guess.

Now that I saw this again, I also recall the discussion of comparing it
with the "testdev" for pci/isa. Anybody knows if these are used by
kvm-unit-tests?

> 
> Regards,
> Halil
> 
> > - Who is instantiating this device? Only the kvm-unit-test?
> > - Can you instantiate multiple instances? Does that make sense? If yes,
> >   it should probably not request a new chpid every time :)
> > - What happens if someone instantiates this by hand? The only drawback
> >   is that it uses up a subchannel and a chpid, right?
> > - Do you plan to make this hotpluggable later?
> > 
> >   
>

Halil Pasic Nov. 14, 2019, 1:42 p.m. UTC | #5

On Thu, 14 Nov 2019 14:19:15 +0100
Cornelia Huck <[e-mail]> wrote:

> On Thu, 14 Nov 2019 14:02:35 +0100
> Halil Pasic <[e-mail]> wrote:
> 
> > On Thu, 14 Nov 2019 11:38:23 +0100
> > Cornelia Huck <[e-mail]> wrote:
> > 
> > > On Wed, 13 Nov 2019 20:02:33 +0100
> > > Pierre Morel <[e-mail]> wrote:
> > > 
> > > Minor nit for $SUBJECT: this isn't a kvm-unit-tests patch, that's just
> > > one consumer :)  
> > 
> > And subchannel is one word in s390-speak.
> > 
> > >   
> > 
> > [..]
> > 
> > > Some questions regarding this device and its intended usage:
> > > 
> > > - What are you trying to test? Basic ccw processing, or something more
> > >   specific? Is there any way you can use the kvm-unit-test
> > >   infrastructure to test basic processing with an existing device?  
> > 
> > I'm also curious about the big picture (what is in scope and what out
> > of scope). Your design should be evaluated in the light of intended
> > usage.
> > 
> > BTW have you had a look at this abandoned patch-set of mine:
> > 
> > https://lists.gnu.org/archive/html/qemu-devel/2017-11/msg04220.html
> 
> Do you recall why it was abandoned? Or did we just forget to follow up
> on it?
> 

I think I do remember. Priorities. Some colleagues were supposed to take
over the job of making proper tests for this device -- the part that is
now intended to be played by kvm-uni-tests. But they never got to
actually start working on it. And my tests for IDA are just a kernel
module -- i.e. nothing sustainable. So without proper exploitation, and
with no time to do a proper test suite myself, I decided to not invest
any more for the time beeing. 

> > 
> > We made some different design decisions, while aiming essentially for the
> > same. Maybe it's due to different scope, maybe not. For instance one
> > can't test IDA with PONG, I guess.
> 
> Now that I saw this again, I also recall the discussion of comparing it
> with the "testdev" for pci/isa. Anybody knows if these are used by
> kvm-unit-tests?
> 

I don't.

Regards,
Halil

> > 
> > Regards,
> > Halil
> > 
> > > - Who is instantiating this device? Only the kvm-unit-test?
> > > - Can you instantiate multiple instances? Does that make sense? If yes,
> > >   it should probably not request a new chpid every time :)
> > > - What happens if someone instantiates this by hand? The only drawback
> > >   is that it uses up a subchannel and a chpid, right?
> > > - Do you plan to make this hotpluggable later?
> > > 
> > >   
> > 
> 
>

Pierre Morel Nov. 14, 2019, 5:11 p.m. UTC | #6

On 2019-11-14 11:38, Cornelia Huck wrote:
> On Wed, 13 Nov 2019 20:02:33 +0100
> Pierre Morel <[e-mail]> wrote:
>
> Minor nit for $SUBJECT: this isn't a kvm-unit-tests patch, that's just
> one consumer :)


yes, right.


>
>> The PONG device accept two commands: PONG_READ and PONG_WRITE
>> which allow to read from and write to an internal buffer of
>> 1024 bytes.
>>
>> The QEMU device is named ccw-pong.
>>
>> Signed-off-by: Pierre Morel <[e-mail]>
>> ---
>>   hw/s390x/Makefile.objs  |   1 +
>>   hw/s390x/ccw-pong.c     | 186 ++++++++++++++++++++++++++++++++++++++++++++++++
>>   include/hw/s390x/pong.h |  47 ++++++++++++
>>   3 files changed, 234 insertions(+)
>>   create mode 100644 hw/s390x/ccw-pong.c
>>   create mode 100644 include/hw/s390x/pong.h
>>
>> diff --git a/hw/s390x/Makefile.objs b/hw/s390x/Makefile.objs
>> index ee91152..3a83438 100644
>> --- a/hw/s390x/Makefile.objs
>> +++ b/hw/s390x/Makefile.objs
>> @@ -32,6 +32,7 @@ obj-$(CONFIG_KVM) += tod-kvm.o
>>   obj-$(CONFIG_KVM) += s390-skeys-kvm.o
>>   obj-$(CONFIG_KVM) += s390-stattrib-kvm.o s390-mchk.o
>>   obj-y += s390-ccw.o
>> +obj-y += ccw-pong.o
> Not sure if unconditionally introducing a test device is a good idea.


sure not.


>
>>   obj-y += ap-device.o
>>   obj-y += ap-bridge.o
>>   obj-y += s390-sei.o
>> diff --git a/hw/s390x/ccw-pong.c b/hw/s390x/ccw-pong.c
>> new file mode 100644
>> index 0000000..e7439d5
>> --- /dev/null
>> +++ b/hw/s390x/ccw-pong.c
>> @@ -0,0 +1,186 @@
>> +/*
>> + * CCW PING-PONG
>> + *
>> + * Copyright 2019 IBM Corp.
>> + * Author(s): Pierre Morel <[e-mail]>
>> + *
>> + * This work is licensed under the terms of the GNU GPL, version 2 or (at
>> + * your option) any later version. See the COPYING file in the top-level
>> + * directory.
>> + */
>> +
>> +#include "qemu/osdep.h"
>> +#include "qapi/error.h"
>> +#include "qemu/module.h"
>> +#include "cpu.h"
>> +#include "exec/address-spaces.h"
>> +#include "hw/s390x/css.h"
>> +#include "hw/s390x/css-bridge.h"
>> +#include "hw/qdev-properties.h"
>> +#include "hw/s390x/pong.h"
>> +
>> +#define PONG_BUF_SIZE 0x1000
>> +static char buf[PONG_BUF_SIZE] = "Hello world\n";
>> +
>> +static inline int pong_rw(CCW1 *ccw, char *p, int len, bool dir)
>> +{
>> +    int ret;
>> +
>> +    ret = address_space_rw(&address_space_memory, ccw->cda,
>> +                           MEMTXATTRS_UNSPECIFIED,
>> +                           (unsigned char *)buf, len, dir);
>> +
>> +    return (ret == MEMTX_OK) ? -EIO : 0;
>> +}
>> +
>> +/* Handle READ ccw commands from guest */
>> +static int handle_payload_read(CcwPONGDevice *dev, CCW1 *ccw)
>> +{
>> +    CcwDevice *ccw_dev = CCW_DEVICE(dev);
>> +    int len;
>> +
>> +    if (!ccw->cda) {
>> +        return -EFAULT;
>> +    }
>> +
>> +    if (ccw->count > PONG_BUF_SIZE) {
>> +        len = PONG_BUF_SIZE;
>> +        ccw_dev->sch->curr_status.scsw.count = ccw->count - PONG_BUF_SIZE;
>> +    } else {
>> +        len = ccw->count;
>> +        ccw_dev->sch->curr_status.scsw.count = 0;
>> +    }
>> +
>> +    return pong_rw(ccw, buf, len, 1);
>> +}
>> +
>> +/*
>> + * Handle WRITE ccw commands to write data to client
>> + * The SCSW count is set to the number of bytes not transfered.
>> + */
>> +static int handle_payload_write(CcwPONGDevice *dev, CCW1 *ccw)
>> +{
>> +    CcwDevice *ccw_dev = CCW_DEVICE(dev);
>> +    int len;
>> +
>> +    if (!ccw->cda) {
>> +        ccw_dev->sch->curr_status.scsw.count = ccw->count;
>> +        return -EFAULT;
>> +    }
>> +
>> +    if (ccw->count > PONG_BUF_SIZE) {
>> +        len = PONG_BUF_SIZE;
>> +        ccw_dev->sch->curr_status.scsw.count = ccw->count - PONG_BUF_SIZE;
>> +    } else {
>> +        len = ccw->count;
>> +        ccw_dev->sch->curr_status.scsw.count = 0;
>> +    }
>> +
>> +    return pong_rw(ccw, buf, len, 0);
> Can you please use the dstream infrastructure for read/write handling?
>
> You also seem to miss some basic checks e.g. for short reads/writes
> with and without SLI set.

OK,


>
>> +}
>> +
>> +static int pong_ccw_cb(SubchDev *sch, CCW1 ccw)
>> +{
>> +    int rc = 0;
>> +    CcwPONGDevice *dev = sch->driver_data;
>> +
>> +    switch (ccw.cmd_code) {
>> +    case PONG_WRITE:
>> +        rc = handle_payload_write(dev, &ccw);
>> +        break;
>> +    case PONG_READ:
>> +        rc = handle_payload_read(dev, &ccw);
>> +        break;
>> +    default:
>> +        rc = -ENOSYS;
>> +        break;
>> +    }
>> +
>> +    if (rc == -EIO) {
>> +        /* I/O error, specific devices generate specific conditions */
>> +        SCHIB *schib = &sch->curr_status;
>> +
>> +        sch->curr_status.scsw.dstat = SCSW_DSTAT_UNIT_CHECK;
>> +        sch->sense_data[0] = 0x40;    /* intervention-req */
> This is really odd. If it succeeds, you generate a unit check with
> intervention required? Confused. At the very least, this requires some
> description as to how this device is supposed to interact with the
> driver.


The unit check is only done in case of I/O error.

I thought it was right, since it should never happen.

Yes it needs some documentation.


>
>> +        schib->scsw.ctrl &= ~SCSW_ACTL_START_PEND;
>> +        schib->scsw.ctrl &= ~SCSW_CTRL_MASK_STCTL;
>> +        schib->scsw.ctrl |= SCSW_STCTL_PRIMARY | SCSW_STCTL_SECONDARY |
>> +                   SCSW_STCTL_ALERT | SCSW_STCTL_STATUS_PEND;
>> +    }
>> +    return rc;
>> +}
>> +
>> +static void pong_ccw_realize(DeviceState *ds, Error **errp)
>> +{
>> +    uint16_t chpid;
>> +    CcwPONGDevice *dev = CCW_PONG(ds);
>> +    CcwDevice *cdev = CCW_DEVICE(ds);
>> +    CCWDeviceClass *cdk = CCW_DEVICE_GET_CLASS(cdev);
>> +    SubchDev *sch;
>> +    Error *err = NULL;
>> +
>> +    sch = css_create_sch(cdev->devno, errp);
>> +    if (!sch) {
>> +        return;
>> +    }
>> +
>> +    sch->driver_data = dev;
>> +    cdev->sch = sch;
>> +    chpid = css_find_free_chpid(sch->cssid);
>> +
>> +    if (chpid > MAX_CHPID) {
>> +        error_setg(&err, "No available chpid to use.");
>> +        goto out_err;
>> +    }
>> +
>> +    sch->id.reserved = 0xff;
>> +    sch->id.cu_type = CCW_PONG_CU_TYPE;
>> +    css_sch_build_virtual_schib(sch, (uint8_t)chpid,
>> +                                CCW_PONG_CHPID_TYPE);
>> +    sch->do_subchannel_work = do_subchannel_work_virtual;
>> +    sch->ccw_cb = pong_ccw_cb;
>> +
>> +    cdk->realize(cdev, &err);
>> +    if (err) {
>> +        goto out_err;
>> +    }
>> +
>> +    css_reset_sch(sch);
>> +    return;
>> +
>> +out_err:
>> +    error_propagate(errp, err);
>> +    css_subch_assign(sch->cssid, sch->ssid, sch->schid, sch->devno, NULL);
>> +    cdev->sch = NULL;
>> +    g_free(sch);
>> +}
>> +
>> +static Property pong_ccw_properties[] = {
>> +    DEFINE_PROP_END_OF_LIST(),
>> +};
>> +
>> +static void pong_ccw_class_init(ObjectClass *klass, void *data)
>> +{
>> +    DeviceClass *dc = DEVICE_CLASS(klass);
>> +
>> +    dc->props = pong_ccw_properties;
>> +    dc->bus_type = TYPE_VIRTUAL_CSS_BUS;
>> +    dc->realize = pong_ccw_realize;
>> +    dc->hotpluggable = false;
>> +    set_bit(DEVICE_CATEGORY_DISPLAY, dc->categories);
> Huh? misc seems like a better idea for this :)


Sure.


>
>> +}
>> +
>> +static const TypeInfo pong_ccw_info = {
>> +    .name = TYPE_CCW_PONG,
>> +    .parent = TYPE_CCW_DEVICE,
>> +    .instance_size = sizeof(CcwPONGDevice),
>> +    .class_init = pong_ccw_class_init,
>> +    .class_size = sizeof(CcwPONGClass),
>> +};
>> +
>> +static void pong_ccw_register(void)
>> +{
>> +    type_register_static(&pong_ccw_info);
>> +}
>> +
>> +type_init(pong_ccw_register)
> Some questions regarding this device and its intended usage:
>
> - What are you trying to test? Basic ccw processing, or something more
>    specific?

The original purpose is to serve as a device for the kv-unit-test 
development.

Testing the QEMU CCW from inside Secure Execution (next step).

>   Is there any way you can use the kvm-unit-test
>    infrastructure to test basic processing with an existing device?

At first we will use it inside SE so that real devices will not be the 
purpose.

The goal of the css test in the kvm-unit-test development for SE is to 
test I/O through VIRTIO-CCW


> - Who is instantiating this device? Only the kvm-unit-test?

yes.


> - Can you instantiate multiple instances?
Not yet
>   Does that make sense?
Probably not
>   If yes,
>    it should probably not request a new chpid every time :)
Yes, the development was quick.
> - What happens if someone instantiates this by hand?
this needs some work.
>   The only drawback
>    is that it uses up a subchannel and a chpid, right?


The development was very quick, I re-used something I developped 4 years 
ago.

It definitively needs more thinking.


> - Do you plan to make this hotpluggable later?

No, unless there is a good reason to.


Thanks a lot for the comments.

Regards,

Pierre


>
>

Pierre Morel Nov. 14, 2019, 5:17 p.m. UTC | #7

On 2019-11-14 13:33, Thomas Huth wrote:
> On 14/11/2019 11.38, Cornelia Huck wrote:
>> On Wed, 13 Nov 2019 20:02:33 +0100
>> Pierre Morel <[e-mail]> wrote:
>>
>> Minor nit for $SUBJECT: this isn't a kvm-unit-tests patch, that's just
>> one consumer :)
>>
>>> The PONG device accept two commands: PONG_READ and PONG_WRITE
>>> which allow to read from and write to an internal buffer of
>>> 1024 bytes.
>>>
>>> The QEMU device is named ccw-pong.
>>>
>>> Signed-off-by: Pierre Morel <[e-mail]>
>>> ---
>>>   hw/s390x/Makefile.objs  |   1 +
>>>   hw/s390x/ccw-pong.c     | 186 ++++++++++++++++++++++++++++++++++++++++++++++++
>>>   include/hw/s390x/pong.h |  47 ++++++++++++
>>>   3 files changed, 234 insertions(+)
>>>   create mode 100644 hw/s390x/ccw-pong.c
>>>   create mode 100644 include/hw/s390x/pong.h
>>>
>>> diff --git a/hw/s390x/Makefile.objs b/hw/s390x/Makefile.objs
>>> index ee91152..3a83438 100644
>>> --- a/hw/s390x/Makefile.objs
>>> +++ b/hw/s390x/Makefile.objs
>>> @@ -32,6 +32,7 @@ obj-$(CONFIG_KVM) += tod-kvm.o
>>>   obj-$(CONFIG_KVM) += s390-skeys-kvm.o
>>>   obj-$(CONFIG_KVM) += s390-stattrib-kvm.o s390-mchk.o
>>>   obj-y += s390-ccw.o
>>> +obj-y += ccw-pong.o
>> Not sure if unconditionally introducing a test device is a good idea.
> This definitely needs a CONFIG switch (which can be "y" by default, but
> still we should provide a possibility to disable it)

yes, clearly


>
>>>   obj-y += ap-device.o
>>>   obj-y += ap-bridge.o
>>>   obj-y += s390-sei.o
>>> diff --git a/hw/s390x/ccw-pong.c b/hw/s390x/ccw-pong.c
>>> new file mode 100644
>>> index 0000000..e7439d5
>>> --- /dev/null
>>> +++ b/hw/s390x/ccw-pong.c
>>> @@ -0,0 +1,186 @@
>>> +/*
>>> + * CCW PING-PONG
> Please add a short description here what this device is all about.


yes


>
>>> + * Copyright 2019 IBM Corp.
>>> + * Author(s): Pierre Morel <[e-mail]>
>>> + *
>>> + * This work is licensed under the terms of the GNU GPL, version 2 or (at
>>> + * your option) any later version. See the COPYING file in the top-level
>>> + * directory.
>>> + */
>>> +
>>> +#include "qemu/osdep.h"
>>> +#include "qapi/error.h"
>>> +#include "qemu/module.h"
>>> +#include "cpu.h"
>>> +#include "exec/address-spaces.h"
>>> +#include "hw/s390x/css.h"
>>> +#include "hw/s390x/css-bridge.h"
>>> +#include "hw/qdev-properties.h"
>>> +#include "hw/s390x/pong.h"
>>> +
>>> +#define PONG_BUF_SIZE 0x1000
>>> +static char buf[PONG_BUF_SIZE] = "Hello world\n";
>>> +
>>> +static inline int pong_rw(CCW1 *ccw, char *p, int len, bool dir)
>>> +{
>>> +    int ret;
>>> +
>>> +    ret = address_space_rw(&address_space_memory, ccw->cda,
>>> +                           MEMTXATTRS_UNSPECIFIED,
>>> +                           (unsigned char *)buf, len, dir);
>>> +
>>> +    return (ret == MEMTX_OK) ? -EIO : 0;
> If return code was OK, then you return an EIO error? That looks weird?

Totally weird. it is of course the oposite.

This explain the comment from Connie on the unit check.


>
>>> +}
>>> +
>>> +/* Handle READ ccw commands from guest */
...snip...
>>> +
>>> +static Property pong_ccw_properties[] = {
>>> +    DEFINE_PROP_END_OF_LIST(),
>>> +};
>>> +
>>> +static void pong_ccw_class_init(ObjectClass *klass, void *data)
>>> +{
>>> +    DeviceClass *dc = DEVICE_CLASS(klass);
>>> +
>>> +    dc->props = pong_ccw_properties;
> As long as there are no properties, I think you can simply drop that line.
>
>   Thomas

Yes, right.


Thanks for the comments,

Regards,

Pierre

Pierre Morel Nov. 14, 2019, 5:37 p.m. UTC | #8

On 2019-11-14 14:02, Halil Pasic wrote:
> On Thu, 14 Nov 2019 11:38:23 +0100
> Cornelia Huck <[e-mail]> wrote:
>
>> On Wed, 13 Nov 2019 20:02:33 +0100
>> Pierre Morel <[e-mail]> wrote:
>>
>> Minor nit for $SUBJECT: this isn't a kvm-unit-tests patch, that's just
>> one consumer :)
> And subchannel is one word in s390-speak.

OK, surely better for grep


>
> [..]
>
>> Some questions regarding this device and its intended usage:
>>
>> - What are you trying to test? Basic ccw processing, or something more
>>    specific? Is there any way you can use the kvm-unit-test
>>    infrastructure to test basic processing with an existing device?
> I'm also curious about the big picture (what is in scope and what out
> of scope). Your design should be evaluated in the light of intended
> usage.
>
> BTW have you had a look at this abandoned patch-set of mine:
>
> https://lists.gnu.org/archive/html/qemu-devel/2017-11/msg04220.html


No, but now yes.

Yes, it is something similar, I can remember as you did it.


>
> We made some different design decisions, while aiming essentially for the
> same. Maybe it's due to different scope, maybe not. For instance one
> can't test IDA with PONG, I guess.

No not now.


Regards,

Pierre

Pierre Morel Nov. 14, 2019, 5:42 p.m. UTC | #9

On 2019-11-14 14:19, Cornelia Huck wrote:
> On Thu, 14 Nov 2019 14:02:35 +0100
> Halil Pasic <[e-mail]> wrote:
>
>> On Thu, 14 Nov 2019 11:38:23 +0100
>> Cornelia Huck <[e-mail]> wrote:
>>
>>> On Wed, 13 Nov 2019 20:02:33 +0100
>>> Pierre Morel <[e-mail]> wrote:
>>>
...snip...
>> We made some different design decisions, while aiming essentially for the
>> same. Maybe it's due to different scope, maybe not. For instance one
>> can't test IDA with PONG, I guess.
> Now that I saw this again, I also recall the discussion of comparing it
> with the "testdev" for pci/isa. Anybody knows if these are used by
> kvm-unit-tests?

Only by X.

Regards,

Pierre

Cornelia Huck Nov. 15, 2019, 10:35 a.m. UTC | #10

On Thu, 14 Nov 2019 18:42:35 +0100
Pierre Morel <[e-mail]> wrote:

> On 2019-11-14 14:19, Cornelia Huck wrote:
> > On Thu, 14 Nov 2019 14:02:35 +0100
> > Halil Pasic <[e-mail]> wrote:
> >  
> >> On Thu, 14 Nov 2019 11:38:23 +0100
> >> Cornelia Huck <[e-mail]> wrote:
> >>  
> >>> On Wed, 13 Nov 2019 20:02:33 +0100
> >>> Pierre Morel <[e-mail]> wrote:
> >>>  
> ...snip...
> >> We made some different design decisions, while aiming essentially for the
> >> same. Maybe it's due to different scope, maybe not. For instance one
> >> can't test IDA with PONG, I guess.  
> > Now that I saw this again, I also recall the discussion of comparing it
> > with the "testdev" for pci/isa. Anybody knows if these are used by
> > kvm-unit-tests?  
> 
> Only by X.

If they use it, it might make sense for s390 to use a comparable
approach.

Btw, I created https://wiki.qemu.org/Testing/CCWTestDevice back then;
might make sense to collect ideas there?

> 
> Regards,
> 
> Pierre
> 
>

Thomas Huth Nov. 15, 2019, 2:22 p.m. UTC | #11

On 14/11/2019 18.17, Pierre Morel wrote:
> 
> On 2019-11-14 13:33, Thomas Huth wrote:
>> On 14/11/2019 11.38, Cornelia Huck wrote:
>>> On Wed, 13 Nov 2019 20:02:33 +0100
>>> Pierre Morel <[e-mail]> wrote:
>>>
>>> Minor nit for $SUBJECT: this isn't a kvm-unit-tests patch, that's just
>>> one consumer :)
>>>
>>>> The PONG device accept two commands: PONG_READ and PONG_WRITE
>>>> which allow to read from and write to an internal buffer of
>>>> 1024 bytes.
>>>>
>>>> The QEMU device is named ccw-pong.
>>>>
>>>> Signed-off-by: Pierre Morel <[e-mail]>
>>>> ---
>>>>   hw/s390x/Makefile.objs  |   1 +
>>>>   hw/s390x/ccw-pong.c     | 186
>>>> ++++++++++++++++++++++++++++++++++++++++++++++++
>>>>   include/hw/s390x/pong.h |  47 ++++++++++++
>>>>   3 files changed, 234 insertions(+)
>>>>   create mode 100644 hw/s390x/ccw-pong.c
>>>>   create mode 100644 include/hw/s390x/pong.h
>>>>
>>>> diff --git a/hw/s390x/Makefile.objs b/hw/s390x/Makefile.objs
>>>> index ee91152..3a83438 100644
>>>> --- a/hw/s390x/Makefile.objs
>>>> +++ b/hw/s390x/Makefile.objs
>>>> @@ -32,6 +32,7 @@ obj-$(CONFIG_KVM) += tod-kvm.o
>>>>   obj-$(CONFIG_KVM) += s390-skeys-kvm.o
>>>>   obj-$(CONFIG_KVM) += s390-stattrib-kvm.o s390-mchk.o
>>>>   obj-y += s390-ccw.o
>>>> +obj-y += ccw-pong.o
>>> Not sure if unconditionally introducing a test device is a good idea.
>> This definitely needs a CONFIG switch (which can be "y" by default, but
>> still we should provide a possibility to disable it)
> 
> yes, clearly

I just noticed that we already have a global config switch for such
devices, so you could simply do:

obj-$(CONFIG_TEST_DEVICE) += ccw-pong.o

 Thomas

Pierre Morel Nov. 15, 2019, 3:15 p.m. UTC | #12

On 2019-11-15 11:35, Cornelia Huck wrote:
> On Thu, 14 Nov 2019 18:42:35 +0100
> Pierre Morel <[e-mail]> wrote:
>
>> On 2019-11-14 14:19, Cornelia Huck wrote:
>>> On Thu, 14 Nov 2019 14:02:35 +0100
>>> Halil Pasic <[e-mail]> wrote:
>>>   
>>>> On Thu, 14 Nov 2019 11:38:23 +0100
>>>> Cornelia Huck <[e-mail]> wrote:
>>>>   
>>>>> On Wed, 13 Nov 2019 20:02:33 +0100
>>>>> Pierre Morel <[e-mail]> wrote:
>>>>>   
>> ...snip...
>>>> We made some different design decisions, while aiming essentially for the
>>>> same. Maybe it's due to different scope, maybe not. For instance one
>>>> can't test IDA with PONG, I guess.
>>> Now that I saw this again, I also recall the discussion of comparing it
>>> with the "testdev" for pci/isa. Anybody knows if these are used by
>>> kvm-unit-tests?
>> Only by X.
> If they use it, it might make sense for s390 to use a comparable
> approach.

yes it does.

>
> Btw, I created https://wiki.qemu.org/Testing/CCWTestDevice back then;
> might make sense to collect ideas there?

right, I forgot this too.

OK, will use it :)

Regards,

Pierre

Pierre Morel Nov. 15, 2019, 3:23 p.m. UTC | #13

On 2019-11-15 15:22, Thomas Huth wrote:
> On 14/11/2019 18.17, Pierre Morel wrote:
>> On 2019-11-14 13:33, Thomas Huth wrote:
>>> On 14/11/2019 11.38, Cornelia Huck wrote:
>>>> On Wed, 13 Nov 2019 20:02:33 +0100
>>>> Pierre Morel <[e-mail]> wrote:
>>>>
>>>> Minor nit for $SUBJECT: this isn't a kvm-unit-tests patch, that's just
>>>> one consumer :)
>>>>
>>>>> The PONG device accept two commands: PONG_READ and PONG_WRITE
>>>>> which allow to read from and write to an internal buffer of
>>>>> 1024 bytes.
>>>>>
>>>>> The QEMU device is named ccw-pong.
>>>>>
>>>>> Signed-off-by: Pierre Morel <[e-mail]>
>>>>> ---
>>>>>    hw/s390x/Makefile.objs  |   1 +
>>>>>    hw/s390x/ccw-pong.c     | 186
>>>>> ++++++++++++++++++++++++++++++++++++++++++++++++
>>>>>    include/hw/s390x/pong.h |  47 ++++++++++++
>>>>>    3 files changed, 234 insertions(+)
>>>>>    create mode 100644 hw/s390x/ccw-pong.c
>>>>>    create mode 100644 include/hw/s390x/pong.h
>>>>>
>>>>> diff --git a/hw/s390x/Makefile.objs b/hw/s390x/Makefile.objs
>>>>> index ee91152..3a83438 100644
>>>>> --- a/hw/s390x/Makefile.objs
>>>>> +++ b/hw/s390x/Makefile.objs
>>>>> @@ -32,6 +32,7 @@ obj-$(CONFIG_KVM) += tod-kvm.o
>>>>>    obj-$(CONFIG_KVM) += s390-skeys-kvm.o
>>>>>    obj-$(CONFIG_KVM) += s390-stattrib-kvm.o s390-mchk.o
>>>>>    obj-y += s390-ccw.o
>>>>> +obj-y += ccw-pong.o
>>>> Not sure if unconditionally introducing a test device is a good idea.
>>> This definitely needs a CONFIG switch (which can be "y" by default, but
>>> still we should provide a possibility to disable it)
>> yes, clearly
> I just noticed that we already have a global config switch for such
> devices, so you could simply do:
>
> obj-$(CONFIG_TEST_DEVICE) += ccw-pong.o
>
>   Thomas


Thanks, will do! :)

regards,

Pierre

Patch

diff --git a/hw/s390x/Makefile.objs b/hw/s390x/Makefile.objs
index ee91152..3a83438 100644
--- a/hw/s390x/Makefile.objs
+++ b/hw/s390x/Makefile.objs
@@ -32,6 +32,7 @@  obj-$(CONFIG_KVM) += tod-kvm.o
 obj-$(CONFIG_KVM) += s390-skeys-kvm.o
 obj-$(CONFIG_KVM) += s390-stattrib-kvm.o s390-mchk.o
 obj-y += s390-ccw.o
+obj-y += ccw-pong.o
 obj-y += ap-device.o
 obj-y += ap-bridge.o
 obj-y += s390-sei.o
diff --git a/hw/s390x/ccw-pong.c b/hw/s390x/ccw-pong.c
new file mode 100644
index 0000000..e7439d5
--- /dev/null
+++ b/hw/s390x/ccw-pong.c
@@ -0,0 +1,186 @@ 
+/*
+ * CCW PING-PONG
+ *
+ * Copyright 2019 IBM Corp.
+ * Author(s): Pierre Morel <[e-mail]>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or (at
+ * your option) any later version. See the COPYING file in the top-level
+ * directory.
+ */
+
+#include "qemu/osdep.h"
+#include "qapi/error.h"
+#include "qemu/module.h"
+#include "cpu.h"
+#include "exec/address-spaces.h"
+#include "hw/s390x/css.h"
+#include "hw/s390x/css-bridge.h"
+#include "hw/qdev-properties.h"
+#include "hw/s390x/pong.h"
+
+#define PONG_BUF_SIZE 0x1000
+static char buf[PONG_BUF_SIZE] = "Hello world\n";
+
+static inline int pong_rw(CCW1 *ccw, char *p, int len, bool dir)
+{
+    int ret;
+
+    ret = address_space_rw(&address_space_memory, ccw->cda,
+                           MEMTXATTRS_UNSPECIFIED,
+                           (unsigned char *)buf, len, dir);
+
+    return (ret == MEMTX_OK) ? -EIO : 0;
+}
+
+/* Handle READ ccw commands from guest */
+static int handle_payload_read(CcwPONGDevice *dev, CCW1 *ccw)
+{
+    CcwDevice *ccw_dev = CCW_DEVICE(dev);
+    int len;
+
+    if (!ccw->cda) {
+        return -EFAULT;
+    }
+
+    if (ccw->count > PONG_BUF_SIZE) {
+        len = PONG_BUF_SIZE;
+        ccw_dev->sch->curr_status.scsw.count = ccw->count - PONG_BUF_SIZE;
+    } else {
+        len = ccw->count;
+        ccw_dev->sch->curr_status.scsw.count = 0;
+    }
+
+    return pong_rw(ccw, buf, len, 1);
+}
+
+/*
+ * Handle WRITE ccw commands to write data to client
+ * The SCSW count is set to the number of bytes not transfered.
+ */
+static int handle_payload_write(CcwPONGDevice *dev, CCW1 *ccw)
+{
+    CcwDevice *ccw_dev = CCW_DEVICE(dev);
+    int len;
+
+    if (!ccw->cda) {
+        ccw_dev->sch->curr_status.scsw.count = ccw->count;
+        return -EFAULT;
+    }
+
+    if (ccw->count > PONG_BUF_SIZE) {
+        len = PONG_BUF_SIZE;
+        ccw_dev->sch->curr_status.scsw.count = ccw->count - PONG_BUF_SIZE;
+    } else {
+        len = ccw->count;
+        ccw_dev->sch->curr_status.scsw.count = 0;
+    }
+
+    return pong_rw(ccw, buf, len, 0);
+}
+
+static int pong_ccw_cb(SubchDev *sch, CCW1 ccw)
+{
+    int rc = 0;
+    CcwPONGDevice *dev = sch->driver_data;
+
+    switch (ccw.cmd_code) {
+    case PONG_WRITE:
+        rc = handle_payload_write(dev, &ccw);
+        break;
+    case PONG_READ:
+        rc = handle_payload_read(dev, &ccw);
+        break;
+    default:
+        rc = -ENOSYS;
+        break;
+    }
+
+    if (rc == -EIO) {
+        /* I/O error, specific devices generate specific conditions */
+        SCHIB *schib = &sch->curr_status;
+
+        sch->curr_status.scsw.dstat = SCSW_DSTAT_UNIT_CHECK;
+        sch->sense_data[0] = 0x40;    /* intervention-req */
+        schib->scsw.ctrl &= ~SCSW_ACTL_START_PEND;
+        schib->scsw.ctrl &= ~SCSW_CTRL_MASK_STCTL;
+        schib->scsw.ctrl |= SCSW_STCTL_PRIMARY | SCSW_STCTL_SECONDARY |
+                   SCSW_STCTL_ALERT | SCSW_STCTL_STATUS_PEND;
+    }
+    return rc;
+}
+
+static void pong_ccw_realize(DeviceState *ds, Error **errp)
+{
+    uint16_t chpid;
+    CcwPONGDevice *dev = CCW_PONG(ds);
+    CcwDevice *cdev = CCW_DEVICE(ds);
+    CCWDeviceClass *cdk = CCW_DEVICE_GET_CLASS(cdev);
+    SubchDev *sch;
+    Error *err = NULL;
+
+    sch = css_create_sch(cdev->devno, errp);
+    if (!sch) {
+        return;
+    }
+
+    sch->driver_data = dev;
+    cdev->sch = sch;
+    chpid = css_find_free_chpid(sch->cssid);
+
+    if (chpid > MAX_CHPID) {
+        error_setg(&err, "No available chpid to use.");
+        goto out_err;
+    }
+
+    sch->id.reserved = 0xff;
+    sch->id.cu_type = CCW_PONG_CU_TYPE;
+    css_sch_build_virtual_schib(sch, (uint8_t)chpid,
+                                CCW_PONG_CHPID_TYPE);
+    sch->do_subchannel_work = do_subchannel_work_virtual;
+    sch->ccw_cb = pong_ccw_cb;
+
+    cdk->realize(cdev, &err);
+    if (err) {
+        goto out_err;
+    }
+
+    css_reset_sch(sch);
+    return;
+
+out_err:
+    error_propagate(errp, err);
+    css_subch_assign(sch->cssid, sch->ssid, sch->schid, sch->devno, NULL);
+    cdev->sch = NULL;
+    g_free(sch);
+}
+
+static Property pong_ccw_properties[] = {
+    DEFINE_PROP_END_OF_LIST(),
+};
+
+static void pong_ccw_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+
+    dc->props = pong_ccw_properties;
+    dc->bus_type = TYPE_VIRTUAL_CSS_BUS;
+    dc->realize = pong_ccw_realize;
+    dc->hotpluggable = false;
+    set_bit(DEVICE_CATEGORY_DISPLAY, dc->categories);
+}
+
+static const TypeInfo pong_ccw_info = {
+    .name = TYPE_CCW_PONG,
+    .parent = TYPE_CCW_DEVICE,
+    .instance_size = sizeof(CcwPONGDevice),
+    .class_init = pong_ccw_class_init,
+    .class_size = sizeof(CcwPONGClass),
+};
+
+static void pong_ccw_register(void)
+{
+    type_register_static(&pong_ccw_info);
+}
+
+type_init(pong_ccw_register)
diff --git a/include/hw/s390x/pong.h b/include/hw/s390x/pong.h
new file mode 100644
index 0000000..6358e2d
--- /dev/null
+++ b/include/hw/s390x/pong.h
@@ -0,0 +1,47 @@ 
+/*
+ *  ccw-attached PONG definitions
+ *
+ * Copyright 2019 IBM Corp.
+ * Author(s): Pierre Morel <[e-mail]>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or (at
+ * your option) any later version. See the COPYING file in the top-level
+ * directory.
+ */
+
+#ifndef HW_S390X_PONG_CCW_H
+#define HW_S390X_PONG_CCW_H
+
+#include "hw/sysbus.h"
+#include "hw/s390x/css.h"
+#include "hw/s390x/ccw-device.h"
+
+#define CCW_PONG_CU_TYPE 0x0007
+#define CCW_PONG_CHPID_TYPE 0x09
+
+#define TYPE_CCW_PONG "ccw-pong"
+
+/* Local Channel Commands */
+#define PONG_WRITE 0x21         /* Write */
+#define PONG_READ  0x22         /* Read buffer */
+
+#define CCW_PONG(obj) \
+     OBJECT_CHECK(CcwPONGDevice, (obj), TYPE_CCW_PONG)
+#define CCW_PONG_CLASS(klass) \
+     OBJECT_CLASS_CHECK(CcwPONGClass, (klass), TYPE_CCW_PONG)
+#define CCW_PONG_GET_CLASS(obj) \
+     OBJECT_GET_CLASS(CcwPONGClass, (obj), TYPE_CCW_PONG)
+
+typedef struct CcwPONGDevice {
+    CcwDevice parent_obj;
+} CcwPONGDevice;
+
+typedef struct CcwPONGClass {
+    CCWDeviceClass parent_class;
+
+    void (*init)(CcwPONGDevice *, Error **);
+    int (*read_payload)(CcwPONGDevice *);
+    int (*write_payload)(CcwPONGDevice *, uint8_t);
+} CcwPONGClass;
+
+#endif