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

[Pierre Morel]

On 2019-11-14 11:38, Cornelia Huck wrote:
> On Wed, 13 Nov 2019 20:02:33 +0100
> Pierre Morel <address@hidden> 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 <address@hidden>
> > ---
> >   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 <address@hidden>
> > + *
> > + * 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
IBM Lab Boeblingen