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

[Pierre Morel]

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

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

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
IBM Lab Boeblingen