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.
> 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 <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;
> +}
> +
> +/* 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?
