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

[Thomas Huth]

On 14/11/2019 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 :)
> 
>> 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.

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 <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;

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