Re: [Qemu-devel] [PATCH 2/8] s390: Channel I/O basic defintions.

[Cornelia Huck <cornelia.huck@de.ibm.com>]

On Tue, 11 Dec 2012 11:27:05 +0100
Alexander Graf <agraf@suse.de> wrote:

> 
> On 10.12.2012, at 11:18, Cornelia Huck wrote:
> 
> > On Mon, 10 Dec 2012 09:07:57 +0100
> > Alexander Graf <agraf@suse.de> wrote:
> > 
> >> 
> >> On 07.12.2012, at 13:50, Cornelia Huck wrote:
> >> 
> >>> Basic channel I/O structures and helper function.
> >>> 
> >>> Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
> >>> ---
> >>> target-s390x/Makefile.objs |   2 +-
> >>> target-s390x/ioinst.c      |  46 ++++++++++
> >>> target-s390x/ioinst.h      | 207 +++++++++++++++++++++++++++++++++++++++++++++
> >>> 3 files changed, 254 insertions(+), 1 deletion(-)
> >>> create mode 100644 target-s390x/ioinst.c
> >>> create mode 100644 target-s390x/ioinst.h
> >>> 
> >>> diff --git a/target-s390x/Makefile.objs b/target-s390x/Makefile.objs
> >>> index e728abf..3afb0b7 100644
> >>> --- a/target-s390x/Makefile.objs
> >>> +++ b/target-s390x/Makefile.objs
> >>> @@ -1,4 +1,4 @@
> >>> obj-y += translate.o helper.o cpu.o interrupt.o
> >>> obj-y += int_helper.o fpu_helper.o cc_helper.o mem_helper.o misc_helper.o
> >>> -obj-$(CONFIG_SOFTMMU) += machine.o
> >>> +obj-$(CONFIG_SOFTMMU) += machine.o ioinst.o
> >>> obj-$(CONFIG_KVM) += kvm.o
> >>> diff --git a/target-s390x/ioinst.c b/target-s390x/ioinst.c
> >>> new file mode 100644
> >>> index 0000000..8577b2c
> >>> --- /dev/null
> >>> +++ b/target-s390x/ioinst.c
> >>> @@ -0,0 +1,46 @@
> >>> +/*
> >>> + * I/O instructions for S/390
> >>> + *
> >>> + * Copyright 2012 IBM Corp.
> >>> + * Author(s): Cornelia Huck <cornelia.huck@de.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 <sys/types.h>
> >>> +#include <sys/ioctl.h>
> >>> +#include <sys/mman.h>
> >>> +
> >>> +#include "cpu.h"
> >>> +#include "ioinst.h"
> >>> +
> >>> +#ifdef DEBUG_IOINST
> >>> +#define dprintf(fmt, ...) \
> >>> +    do { fprintf(stderr, fmt, ## __VA_ARGS__); } while (0)
> >>> +#else
> >>> +#define dprintf(fmt, ...) \
> >>> +    do { } while (0)
> >>> +#endif
> >>> +
> >>> +int ioinst_disassemble_sch_ident(uint32_t value, int *m, int *cssid, int *ssid,
> >>> +                                 int *schid)
> >>> +{
> >>> +    if (!(value & IOINST_SCHID_ONE)) {
> >>> +        return -EINVAL;
> >>> +    }
> >>> +    if (!(value & IOINST_SCHID_M)) {
> >>> +        if (value & IOINST_SCHID_CSSID) {
> >>> +            return -EINVAL;
> >>> +        }
> >>> +        *cssid = 0;
> >>> +        *m = 0;
> >>> +    } else {
> >>> +        *cssid = (value & IOINST_SCHID_CSSID) >> 24;
> >> 
> >> (value & IOINST_SCHID_CSSID_MASK) >> IOINST_SCHID_CSSID_SHIFT
> > 
> > I think that actually decreases readability.
> 
> I'm fine with doing it Jocelyn-style too:
> 
> #define IOINST_SCHID_CSSID(x) ((x & 0x...) >> 24)
> 
> if you prefer that for readability :)

It's worth a try.