On 14/11/2019 11.38, Cornelia Huck wrote:
> On Wed, 13 Nov 2019 20:02:33 +0100
> Pierre Morel <pmo...@linux.ibm.com> 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 <pmo...@linux.ibm.com>
>> ---
>> 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 <pmo...@linux.ibm.com>
>> + *
>> + * 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
