Patchwork [4/7] s390: sclp event support

login
register
mail settings
Submitter Christian Borntraeger
Date July 24, 2012, 7:37 a.m.
Message ID <1343115430-34285-5-git-send-email-borntraeger@de.ibm.com>
Download mbox | patch
Permalink /patch/172793/
State New
Headers show

Comments

Christian Borntraeger - July 24, 2012, 7:37 a.m.
From: Heinz Graalfs <graalfs@linux.vnet.ibm.com>

Several SCLP features are considered to be events. Those events don't
provide SCLP commands on their own, instead they are all based on
Read Event Data, Write Event Data, Write Event Mask and the service
interrupt. Follow-on patches will provide SCLP's Signal Quiesce (via
system_powerdown) and the ASCII console.
Further down the road the sclp line mode console and configuration
change events (e.g. cpu hotplug) can be implemented.

Signed-off-by: Heinz Graalfs <graalfs@linux.vnet.ibm.com>
Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
---
 hw/s390x/Makefile.objs    |    1 +
 hw/s390x/event-facility.c |  390 +++++++++++++++++++++++++++++++++++++++++++++
 hw/s390x/event-facility.h |  107 ++++++++++++
 hw/s390x/sclp.c           |   48 +++++-
 hw/s390x/sclp.h           |   44 +++++
 5 files changed, 587 insertions(+), 3 deletions(-)
 create mode 100644 hw/s390x/event-facility.c
 create mode 100644 hw/s390x/event-facility.h
Alexander Graf - July 30, 2012, 1:24 p.m.
On 24.07.2012, at 09:37, Christian Borntraeger wrote:

> From: Heinz Graalfs <graalfs@linux.vnet.ibm.com>
> 
> Several SCLP features are considered to be events. Those events don't
> provide SCLP commands on their own, instead they are all based on
> Read Event Data, Write Event Data, Write Event Mask and the service
> interrupt. Follow-on patches will provide SCLP's Signal Quiesce (via
> system_powerdown) and the ASCII console.
> Further down the road the sclp line mode console and configuration
> change events (e.g. cpu hotplug) can be implemented.
> 
> Signed-off-by: Heinz Graalfs <graalfs@linux.vnet.ibm.com>
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> ---
> hw/s390x/Makefile.objs    |    1 +
> hw/s390x/event-facility.c |  390 +++++++++++++++++++++++++++++++++++++++++++++
> hw/s390x/event-facility.h |  107 ++++++++++++
> hw/s390x/sclp.c           |   48 +++++-
> hw/s390x/sclp.h           |   44 +++++
> 5 files changed, 587 insertions(+), 3 deletions(-)
> create mode 100644 hw/s390x/event-facility.c
> create mode 100644 hw/s390x/event-facility.h
> 
> diff --git a/hw/s390x/Makefile.objs b/hw/s390x/Makefile.objs
> index 1c14b96..b32fc52 100644
> --- a/hw/s390x/Makefile.objs
> +++ b/hw/s390x/Makefile.objs
> @@ -2,3 +2,4 @@ obj-y = s390-virtio-bus.o s390-virtio.o
> 
> obj-y := $(addprefix ../,$(obj-y))
> obj-y += sclp.o
> +obj-y += event-facility.o
> diff --git a/hw/s390x/event-facility.c b/hw/s390x/event-facility.c
> new file mode 100644
> index 0000000..74a3514
> --- /dev/null
> +++ b/hw/s390x/event-facility.c
> @@ -0,0 +1,390 @@
> +/*
> + * SCLP
> + *    Event Facility
> + *       handles SCLP event types
> + *          - Signal Quiesce - system power down
> + *          - ASCII Console Data - VT220 read and write
> + *
> + * Copyright IBM, Corp. 2012
> + *
> + * Authors:
> + *  Heinz Graalfs <graalfs@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 "monitor.h"
> +#include "sysemu.h"
> +
> +#include "sclp.h"
> +#include "event-facility.h"
> +
> +typedef struct EventTypes {
> +    BusState qbus;
> +    SCLPEventFacility *event_facility;
> +} EventTypes;
> +
> +struct SCLPEventFacility {
> +    EventTypes sbus;
> +    DeviceState *qdev;
> +    /* guest' receive mask */
> +    unsigned int receive_mask;
> +};
> +
> +/* return true if any child has event pending set */
> +static bool event_pending(SCLPEventFacility *ef)
> +{
> +    BusChild *kid;
> +    SCLPEvent *event;
> +
> +    QTAILQ_FOREACH(kid, &ef->sbus.qbus.children, sibling) {
> +        DeviceState *qdev = kid->child;
> +        event = DO_UPCAST(SCLPEvent, qdev, qdev);
> +        lock(event);

While I like short function names, you're certainly not the only person who wants locking in the tree :). Please name this a bit more explicitly.

In fact, why do you need locking in the first place?

> +        if (event->event_pending) {
> +            unlock(event);
> +            return true;
> +        }
> +        unlock(event);
> +    }
> +    return false;
> +}
> +
> +static unsigned int get_host_send_mask(SCLPEventFacility *ef)
> +{
> +    unsigned int mask;
> +    BusChild *kid;
> +    SCLPEventClass *child;
> +
> +    mask = 0;
> +
> +    QTAILQ_FOREACH(kid, &ef->sbus.qbus.children, sibling) {
> +        DeviceState *qdev = kid->child;
> +        child = SCLP_EVENT_GET_CLASS((SCLPEvent *) qdev);
> +        mask |= child->get_send_mask();
> +    }
> +    return mask;
> +}
> +
> +static unsigned int get_host_receive_mask(SCLPEventFacility *ef)
> +{
> +    unsigned int mask;
> +    BusChild *kid;
> +    SCLPEventClass *child;
> +
> +    mask = 0;
> +
> +    QTAILQ_FOREACH(kid, &ef->sbus.qbus.children, sibling) {
> +        DeviceState *qdev = kid->child;
> +        child = SCLP_EVENT_GET_CLASS((SCLPEvent *) qdev);
> +        mask |= child->get_receive_mask();
> +    }
> +    return mask;
> +}
> +
> +static bool sccb_events_ok(SCCB *sccb)

Please return the failure code as return value here. That'd turn the function into completely read-only, making the big picture easier to understand.

> +{
> +    int slen;
> +    unsigned elen = 0;
> +    EventBufferHeader *event;
> +    WriteEventData *wed = (WriteEventData *) sccb;

Mind to explain why every sccb_event (coming from the function name) is a WriteEventData?

> +
> +    event = (EventBufferHeader *) &wed->ebh;
> +    for (slen = be16_to_cpu(sccb->h.length) - sizeof(sccb->h);
> +         slen > 0; slen -= elen) {
> +        elen = be16_to_cpu(event->length);
> +        if (elen < sizeof(*event) || elen > slen) {
> +            sccb->h.response_code =
> +                    cpu_to_be16(SCLP_RC_EVENT_BUFFER_SYNTAX_ERROR);
> +            return false;
> +        }
> +        event = (void *) event + elen;
> +    }
> +    if (slen) {
> +        sccb->h.response_code = cpu_to_be16(SCLP_RC_INCONSISTENT_LENGTHS);
> +        return false;
> +    }
> +    return true;
> +}
> +
> +static void handle_sccb_write_events(SCLPEventFacility *ef, SCCB *sccb)
> +{
> +    int slen;
> +    unsigned elen = 0;
> +    EventBufferHeader *event_buf;
> +    BusChild *kid;
> +    SCLPEvent *event;
> +    SCLPEventClass *ec;
> +
> +    WriteEventData *wed = (WriteEventData *) sccb;
> +
> +    event_buf = &wed->ebh;
> +
> +    /* loop over all contained event buffers */
> +    for (slen = be16_to_cpu(sccb->h.length) - sizeof(sccb->h);

How about a

static inline int sccb_data_len(SCCB *sccb) {
  return be16_to_cpu(sccb->h.length) - sizeof(sccb->h);
}

in the sclp header, and then

for (slen = sccb_data_len(sccb); slen > 0; slen -= elen) {
...

here and above?

> +         slen > 0; slen -= elen) {
> +        elen = be16_to_cpu(event_buf->length);
> +
> +        /* in case of a previous error mark all trailing buffers
> +         * as not accepted */
> +        if (sccb->h.response_code !=
> +                cpu_to_be16(SCLP_RC_NORMAL_COMPLETION)) {
> +            event_buf->flags &= ~(SCLP_EVENT_BUFFER_ACCEPTED);

When would we hit this?

> +        } else {
> +            sccb->h.response_code = cpu_to_be16(SCLP_RC_INVALID_FUNCTION);

Again, please factor that out to either the parent function or at least the end of this function.

> +            QTAILQ_FOREACH(kid, &ef->sbus.qbus.children, sibling) {
> +                DeviceState *qdev = kid->child;
> +                event = (SCLPEvent *) qdev;
> +                ec = SCLP_EVENT_GET_CLASS(event);
> +
> +                if (ec->write_event_data &&
> +                    ec->event_type() == event_buf->type) {
> +                    sccb->h.response_code = cpu_to_be16(
> +                            ec->write_event_data(event, event_buf));
> +                    break;
> +                }
> +            }
> +        }
> +        event_buf = (void *) event_buf + elen;
> +    }
> +}
> +
> +static int write_event_data(SCLPEventFacility *ef, SCCB *sccb)
> +{
> +    if (sccb->h.function_code != SCLP_FC_NORMAL_WRITE) {
> +        sccb->h.response_code = cpu_to_be16(SCLP_RC_INVALID_FUNCTION);
> +        goto out;
> +    }
> +    if (be16_to_cpu(sccb->h.length) < 8) {
> +        sccb->h.response_code = cpu_to_be16(SCLP_RC_INSUFFICIENT_SCCB_LENGTH);
> +        goto out;
> +    }
> +    /* first check the sum of all events */
> +    if (!sccb_events_ok(sccb)) {
> +        goto out;
> +    }
> +    /* then execute */
> +    sccb->h.response_code = cpu_to_be16(SCLP_RC_NORMAL_COMPLETION);
> +    handle_sccb_write_events(ef, sccb);
> +
> +out:
> +    return 0;

This always returns 0? Let me guess - you originally wanted to pass response_code as return value! ;)

I think you get the pattern by now :)

> +}
> +
> +static void handle_sccb_read_events(SCLPEventFacility *ef, SCCB *sccb,
> +                                    unsigned int mask)
> +{
> +    int slen;
> +    unsigned elen = 0;
> +    BusChild *kid;
> +    SCLPEvent *event;
> +    SCLPEventClass *ec;
> +    EventBufferHeader *event_buf;
> +    ReadEventData *red = (ReadEventData *) sccb;
> +
> +    event_buf = &red->ebh;
> +    event_buf->length = 0;
> +    slen = sizeof(sccb->data);
> +    QTAILQ_FOREACH(kid, &ef->sbus.qbus.children, sibling) {
> +        DeviceState *qdev = kid->child;
> +        event = (SCLPEvent *) qdev;
> +        ec = SCLP_EVENT_GET_CLASS(event);
> +
> +        if (mask & ec->get_send_mask()) {
> +            if (ec->read_event_data(event, event_buf, &slen)) {
> +                sccb->h.response_code = cpu_to_be16(SCLP_RC_NORMAL_COMPLETION);

What is the response code when we don't have any data?

> +            }
> +        }
> +        elen = be16_to_cpu(event_buf->length);
> +        event_buf = (void *) event_buf + elen;
> +    }
> +
> +    if (sccb->h.control_mask[2] & SCLP_VARIABLE_LENGTH_RESPONSE) {

This block deserves a comment.

> +        sccb->h.control_mask[2] &= ~SCLP_VARIABLE_LENGTH_RESPONSE;
> +        sccb->h.length = cpu_to_be16(SCCB_SIZE - slen);
> +    }
> +}
> +
> +static int read_event_data(SCLPEventFacility *ef, SCCB *sccb)
> +{
> +    unsigned int sclp_active_selection_mask;
> +    unsigned int sclp_cp_receive_mask;
> +
> +    ReadEventData *red = (ReadEventData *) sccb;
> +
> +    if (be16_to_cpu(sccb->h.length) != SCCB_SIZE) {
> +        sccb->h.response_code = cpu_to_be16(SCLP_RC_INSUFFICIENT_SCCB_LENGTH);
> +        goto out;
> +    }
> +
> +    sclp_cp_receive_mask = ef->receive_mask;
> +
> +    /* get active selection mask */
> +    switch (sccb->h.function_code) {
> +    case SCLP_UNCONDITIONAL_READ:
> +        sclp_active_selection_mask = sclp_cp_receive_mask;
> +        break;
> +    case SCLP_SELECTIVE_READ:
> +        if (!(sclp_cp_receive_mask & be32_to_cpu(red->mask))) {
> +            sccb->h.response_code =
> +                    cpu_to_be16(SCLP_RC_INVALID_SELECTION_MASK);
> +            goto out;
> +        }
> +        sclp_active_selection_mask = be32_to_cpu(red->mask);
> +        break;
> +    default:
> +        sccb->h.response_code = cpu_to_be16(SCLP_RC_INVALID_FUNCTION);
> +        goto out;
> +    }
> +
> +    sccb->h.response_code = cpu_to_be16(SCLP_RC_NO_EVENT_BUFFERS_STORED);
> +    handle_sccb_read_events(ef, sccb, sclp_active_selection_mask);
> +
> +out:
> +    return 0;
> +}
> +
> +static int write_event_mask(SCLPEventFacility *ef, SCCB *sccb)
> +{
> +    WriteEventMask *we_mask = (WriteEventMask *) sccb;
> +
> +    /* Attention: We assume that Linux uses 4-byte masks, what it actually
> +       does. Architecture allows for masks of variable size, though */
> +    if (be16_to_cpu(we_mask->mask_length) != 4) {
> +        sccb->h.response_code = cpu_to_be16(SCLP_RC_INVALID_MASK_LENGTH);
> +        goto out;
> +    }
> +
> +    /* keep track of the guest's capability masks */
> +    ef->receive_mask = be32_to_cpu(we_mask->cp_receive_mask);
> +
> +    /* return the SCLP's capability masks to the guest */
> +    we_mask->send_mask = cpu_to_be32(get_host_send_mask(ef));
> +    we_mask->receive_mask = cpu_to_be32(get_host_receive_mask(ef));
> +
> +    sccb->h.response_code = cpu_to_be16(SCLP_RC_NORMAL_COMPLETION);
> +
> +out:
> +    return 0;
> +}
> +
> +/* qemu object creation and initialization functions */
> +
> +#define TYPE_SCLP_EVENTS_BUS "s390-sclp-events-bus"
> +#define SCLP_EVENTS_BUS(obj) OBJECT_CHECK(SCLPS390Bus, (obj),\
> +                             TYPE_SCLP_EVENTS_BUS)
> +
> +static void sclp_events_bus_class_init(ObjectClass *klass, void *data)
> +{
> +}
> +
> +static const TypeInfo s390_sclp_events_bus_info = {
> +    .name = TYPE_SCLP_EVENTS_BUS,
> +    .parent = TYPE_BUS,
> +    .instance_size = sizeof(SCLPS390Bus),
> +    .class_init = sclp_events_bus_class_init,
> +};
> +
> +static int command_handler(SCLPEventFacility *ef, SCCB *sccb, uint64_t code)
> +{
> +    int r = 0;
> +
> +    switch (code) {
> +    case SCLP_CMD_READ_EVENT_DATA:
> +        r = read_event_data(ef, sccb);
> +        break;
> +    case SCLP_CMD_WRITE_EVENT_DATA:
> +        r = write_event_data(ef, sccb);
> +        break;
> +    case SCLP_CMD_WRITE_EVENT_MASK:
> +        r = write_event_mask(ef, sccb);
> +        break;
> +    default:
> +#ifdef DEBUG_HELPER

Why DEBUG_HELPER?

> +        printf("KVM: unhandled sclp code 0x%" PRIx64 "x\n", code);
> +#endif
> +        sccb->h.response_code = cpu_to_be16(SCLP_RC_INVALID_SCLP_COMMAND);
> +        break;
> +    }
> +    return r;
> +}
> +
> +static int init_event_facility(S390SCLPDevice *sdev)
> +{
> +    SCLPEventFacility *event_facility;
> +
> +    event_facility = g_malloc0(sizeof(SCLPEventFacility));
> +    sdev->instance = event_facility;
> +    sdev->sclp_command_handler = command_handler;
> +    sdev->event_pending = event_pending;
> +
> +    /* Spawn a new sclp-events facility */
> +    qbus_create_inplace(&event_facility->sbus.qbus,
> +                        TYPE_SCLP_EVENTS_BUS, (DeviceState *)sdev, NULL);
> +    event_facility->sbus.qbus.allow_hotplug = 0;
> +    event_facility->sbus.event_facility = event_facility;
> +    event_facility->qdev = (DeviceState *) sdev;
> +
> +    return 0;
> +}
> +
> +static void init_event_facility_class(ObjectClass *klass, void *data)
> +{
> +    S390SCLPDeviceClass *k = SCLP_S390_DEVICE_CLASS(klass);
> +
> +    k->init = init_event_facility;
> +}
> +
> +static TypeInfo s390_sclp_event_facility_info = {
> +    .name          = "s390-sclp-event-facility",
> +    .parent        = TYPE_DEVICE_S390_SCLP,
> +    .instance_size = sizeof(S390SCLPDevice),
> +    .class_init    = init_event_facility_class,
> +};
> +
> +static int event_qdev_init(DeviceState *qdev)
> +{
> +    SCLPEvent *event = DO_UPCAST(SCLPEvent, qdev, qdev);
> +    SCLPEventClass *child = SCLP_EVENT_GET_CLASS(event);
> +
> +    return child->init(event);
> +}
> +
> +static int event_qdev_exit(DeviceState *qdev)
> +{
> +    SCLPEvent *event = DO_UPCAST(SCLPEvent, qdev, qdev);
> +    SCLPEventClass *child = SCLP_EVENT_GET_CLASS(event);
> +    if (child->exit) {
> +        child->exit(event);
> +    }
> +    return 0;
> +}
> +
> +static void event_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +
> +    dc->bus_type = TYPE_SCLP_EVENTS_BUS;
> +    dc->unplug = qdev_simple_unplug_cb;
> +    dc->init = event_qdev_init;
> +    dc->exit = event_qdev_exit;
> +}
> +
> +static TypeInfo s390_sclp_event_type_info = {
> +    .name = TYPE_SCLP_EVENT,
> +    .parent = TYPE_DEVICE,
> +    .instance_size = sizeof(SCLPEvent),
> +    .class_init = event_class_init,
> +    .class_size = sizeof(SCLPEventClass),
> +    .abstract = true,
> +};
> +
> +static void register_types(void)
> +{
> +    type_register_static(&s390_sclp_events_bus_info);
> +    type_register_static(&s390_sclp_event_facility_info);
> +    type_register_static(&s390_sclp_event_type_info);
> +}
> +type_init(register_types)
> diff --git a/hw/s390x/event-facility.h b/hw/s390x/event-facility.h
> new file mode 100644
> index 0000000..1e022a3
> --- /dev/null
> +++ b/hw/s390x/event-facility.h
> @@ -0,0 +1,107 @@
> +/*
> + * SCLP
> + *    Event Facility definitions
> + *
> + * Copyright IBM, Corp. 2012
> + *
> + * Authors:
> + *  Heinz Graalfs <graalfs@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.
> + *
> + */
> +
> +#ifndef HW_S390_SCLP_EVENT_FACILITY_H
> +#define HW_S390_SCLP_EVENT_FACILITY_H
> +
> +#include <hw/qdev.h>
> +#include "qemu-thread.h"
> +
> +/* SCLP event types */
> +#define SCLP_EVENT_ASCII_CONSOLE_DATA           0x1a
> +#define SCLP_EVENT_SIGNAL_QUIESCE               0x1d
> +
> +/* SCLP event masks */
> +#define SCLP_EVENT_MASK_SIGNAL_QUIESCE          0x00000008
> +#define SCLP_EVENT_MASK_MSG_ASCII               0x00000040
> +
> +#define SCLP_UNCONDITIONAL_READ                 0x00
> +#define SCLP_SELECTIVE_READ                     0x01
> +
> +#define TYPE_SCLP_EVENT "s390-sclp-event-type"
> +#define SCLP_EVENT(obj) \
> +     OBJECT_CHECK(SCLPEvent, (obj), TYPE_SCLP_EVENT)
> +#define SCLP_EVENT_CLASS(klass) \
> +     OBJECT_CLASS_CHECK(SCLPEventClass, (klass), TYPE_SCLP_EVENT)
> +#define SCLP_EVENT_GET_CLASS(obj) \
> +     OBJECT_GET_CLASS(SCLPEventClass, (obj), TYPE_SCLP_EVENT)
> +
> +typedef struct WriteEventMask {
> +    SCCBHeader h;
> +    uint16_t _reserved;
> +    uint16_t mask_length;
> +    uint32_t cp_receive_mask;
> +    uint32_t cp_send_mask;
> +    uint32_t send_mask;
> +    uint32_t receive_mask;
> +} QEMU_PACKED WriteEventMask;
> +
> +typedef struct EventBufferHeader {
> +    uint16_t length;
> +    uint8_t  type;
> +    uint8_t  flags;
> +    uint16_t _reserved;
> +} QEMU_PACKED EventBufferHeader;
> +
> +typedef struct WriteEventData {
> +    SCCBHeader h;
> +    EventBufferHeader ebh;
> +} QEMU_PACKED WriteEventData;
> +
> +typedef struct ReadEventData {
> +    SCCBHeader h;
> +    EventBufferHeader ebh;
> +    uint32_t mask;
> +} QEMU_PACKED ReadEventData;
> +
> +typedef struct SCLPEvent {
> +    DeviceState qdev;
> +    QemuMutex lock;
> +    bool event_pending;
> +    uint32_t event_type;
> +    char *name;
> +} SCLPEvent;
> +
> +typedef struct SCLPEventClass {
> +    DeviceClass parent_class;
> +    int (*init)(SCLPEvent *event);
> +    int (*exit)(SCLPEvent *event);
> +
> +    /* get SCLP's send mask */
> +    unsigned int (*get_send_mask)(void);
> +
> +    /* get SCLP's receive mask */
> +    unsigned int (*get_receive_mask)(void);
> +
> +    int (*read_event_data)(SCLPEvent *event, EventBufferHeader *evt_buf_hdr,
> +                           int *slen);
> +
> +    int (*write_event_data)(SCLPEvent *event, EventBufferHeader *evt_buf_hdr);
> +
> +    /* returns the supported event type */
> +    int (*event_type)(void);
> +
> +} SCLPEventClass;
> +
> +static inline void lock(SCLPEvent *event)
> +{
> +    qemu_mutex_lock(&event->lock);

Yeah, I'm fairly sure you don't need the locks :). It might be nice to keep them in as documentation, but make them noops (and name them properly).

> +}
> +
> +static inline void unlock(SCLPEvent *event)
> +{
> +    qemu_mutex_unlock(&event->lock);
> +}
> +
> +#endif
> diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
> index 4095ba6..cfc41ea 100644
> --- a/hw/s390x/sclp.c
> +++ b/hw/s390x/sclp.c
> @@ -39,6 +39,7 @@ static int read_SCP_info(SCCB *sccb)
> static int sclp_execute(SCCB *sccb, uint64_t code)
> {
>     int r = 0;
> +    SCLPEventFacility *ef = sbus->event_facility->instance;
> 
>     switch (code) {
>     case SCLP_CMDW_READ_SCP_INFO:
> @@ -46,7 +47,7 @@ static int sclp_execute(SCCB *sccb, uint64_t code)
>         r = read_SCP_info(sccb);
>         break;
>     default:
> -        sccb->h.response_code = cpu_to_be16(SCLP_RC_INVALID_SCLP_COMMAND);
> +        r = sbus->event_facility->sclp_command_handler(ef, sccb, code);
>         break;
>     }
>     return r;
> @@ -87,10 +88,13 @@ out:
> 
> void sclp_service_interrupt(uint32_t sccb)
> {
> -    if (!sccb) {
> +    SCLPEventFacility *ef = sbus->event_facility->instance;
> +    int event_pending = sbus->event_facility->event_pending(ef);
> +
> +    if (!event_pending && !sccb) {

Uh. So when there's no event pending no sclp call may trigger an interrupt?

>         return;
>     }
> -    s390_sclp_extint(sccb & ~3);
> +    s390_sclp_extint((sccb & ~3) + event_pending);

event_pending returns a bool, right? Please make this code a bit less magical :).



Alex

> }
> 
> /* qemu object creation and initialization functions */
> @@ -112,6 +116,9 @@ SCLPS390Bus *s390_sclp_bus_init(void)
>     bus_state = qbus_create(TYPE_S390_SCLP_BUS, dev, NULL);
>     bus_state->allow_hotplug = 0;
> 
> +    dev = qdev_create(bus_state, "s390-sclp-event-facility");
> +    qdev_init_nofail(dev);
> +
>     sbus = DO_UPCAST(SCLPS390Bus, bus, bus_state);
>     return sbus;
> }
> @@ -137,9 +144,44 @@ static TypeInfo s390_sclp_bridge_info = {
>     .class_init    = s390_sclp_bridge_class_init,
> };
> 
> +static int s390_sclp_busdev_init(DeviceState *dev)
> +{
> +    int r;
> +    S390SCLPDevice *sdev = (S390SCLPDevice *)dev;
> +    S390SCLPDeviceClass *sclp = SCLP_S390_DEVICE_GET_CLASS(dev);
> +    SCLPS390Bus *bus = DO_UPCAST(SCLPS390Bus, bus, sdev->qdev.parent_bus);
> +
> +    r = sclp->init(sdev);
> +    if (!r) {
> +        assert(sdev->event_pending);
> +        assert(sdev->sclp_command_handler);
> +    }
> +    bus->event_facility = sdev;
> +
> +    return r;
> +}
> +
> +static void s390_sclp_device_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +
> +    dc->init = s390_sclp_busdev_init;
> +    dc->bus_type = TYPE_S390_SCLP_BUS;
> +}
> +
> +static TypeInfo s390_sclp_device_info = {
> +    .name = TYPE_DEVICE_S390_SCLP,
> +    .parent = TYPE_DEVICE,
> +    .instance_size = sizeof(S390SCLPDevice),
> +    .class_init = s390_sclp_device_class_init,
> +    .class_size = sizeof(S390SCLPDeviceClass),
> +    .abstract = true,
> +};
> +
> static void s390_sclp_register_types(void)
> {
>     type_register_static(&s390_sclp_bridge_info);
>     type_register_static(&s390_sclp_bus_info);
> +    type_register_static(&s390_sclp_device_info);
> }
> type_init(s390_sclp_register_types)
> diff --git a/hw/s390x/sclp.h b/hw/s390x/sclp.h
> index 4d5a946..d071ccd 100644
> --- a/hw/s390x/sclp.h
> +++ b/hw/s390x/sclp.h
> @@ -19,15 +19,35 @@
> /* SCLP command codes */
> #define SCLP_CMDW_READ_SCP_INFO                 0x00020001
> #define SCLP_CMDW_READ_SCP_INFO_FORCED          0x00120001
> +#define SCLP_CMD_READ_EVENT_DATA                0x00770005
> +#define SCLP_CMD_WRITE_EVENT_DATA               0x00760005
> +#define SCLP_CMD_READ_EVENT_DATA                0x00770005
> +#define SCLP_CMD_WRITE_EVENT_DATA               0x00760005
> +#define SCLP_CMD_WRITE_EVENT_MASK               0x00780005
> 
> /* SCLP response codes */
> #define SCLP_RC_NORMAL_READ_COMPLETION          0x0010
> +#define SCLP_RC_NORMAL_COMPLETION               0x0020
> #define SCLP_RC_INVALID_SCLP_COMMAND            0x01f0
> +#define SCLP_RC_CONTAINED_EQUIPMENT_CHECK       0x0340
> +#define SCLP_RC_INSUFFICIENT_SCCB_LENGTH        0x0300
> +#define SCLP_RC_INVALID_FUNCTION                0x40f0
> +#define SCLP_RC_NO_EVENT_BUFFERS_STORED         0x60f0
> +#define SCLP_RC_INVALID_SELECTION_MASK          0x70f0
> +#define SCLP_RC_INCONSISTENT_LENGTHS            0x72f0
> +#define SCLP_RC_EVENT_BUFFER_SYNTAX_ERROR       0x73f0
> +#define SCLP_RC_INVALID_MASK_LENGTH             0x74f0
> +
> 
> /* Service Call Control Block (SCCB) and its elements */
> 
> #define SCCB_SIZE 4096
> 
> +#define SCLP_VARIABLE_LENGTH_RESPONSE           0x80
> +#define SCLP_EVENT_BUFFER_ACCEPTED              0x80
> +
> +#define SCLP_FC_NORMAL_WRITE                    0
> +
> /*
>  * Normally packed structures are not the right thing to do, since all code
>  * must take care of endianess. We cant use ldl_phys and friends for two
> @@ -64,14 +84,38 @@ typedef struct SCCB {
> #define TYPE_S390_SCLP_BUS "s390-sclp-bus"
> #define S390_SCLP_BUS(obj) OBJECT_CHECK(SCLPS390Bus, (obj), TYPE_S390_SCLP_BUS)
> 
> +#define TYPE_DEVICE_S390_SCLP "s390-sclp-device"
> +#define SCLP_S390_DEVIVE(obj) \
> +     OBJECT_CHECK(S390SCLPDevice, (obj), TYPE_DEVICE_S390_SCLP)
> +#define SCLP_S390_DEVICE_CLASS(klass) \
> +     OBJECT_CLASS_CHECK(S390SCLPDeviceClass, (klass), \
> +             TYPE_DEVICE_S390_SCLP)
> +#define SCLP_S390_DEVICE_GET_CLASS(obj) \
> +     OBJECT_GET_CLASS(S390SCLPDeviceClass, (obj), \
> +             TYPE_DEVICE_S390_SCLP)
> +
> +typedef struct SCLPEventFacility SCLPEventFacility;
> +
> typedef struct S390SCLPDevice {
>     DeviceState qdev;
> +    SCLPEventFacility *instance;
> +    int (*sclp_command_handler)(SCLPEventFacility *ef, SCCB *sccb,
> +                                uint64_t code);
> +    bool (*event_pending)(SCLPEventFacility *ef);
> } S390SCLPDevice;
> 
> typedef struct SCLPS390Bus {
>     BusState bus;
> +    S390SCLPDevice *event_facility;
> } SCLPS390Bus;
> 
> +typedef struct S390SCLPDeviceClass {
> +    DeviceClass qdev;
> +
> +    int (*init)(S390SCLPDevice *sdev);
> +
> +} S390SCLPDeviceClass;
> +
> SCLPS390Bus *s390_sclp_bus_init(void);
> 
> void sclp_service_interrupt(uint32_t sccb);
> -- 
> 1.7.0.1
>
Christian Borntraeger - July 30, 2012, 2:46 p.m.
On 30/07/12 15:24, Alexander Graf wrote:

Thanks for the review.

Here is just a short answer regarding the interrupt, we will adress the other comments in a later
mail.

>> void sclp_service_interrupt(uint32_t sccb)
>> {
>> -    if (!sccb) {
>> +    SCLPEventFacility *ef = sbus->event_facility->instance;
>> +    int event_pending = sbus->event_facility->event_pending(ef);
>> +
>> +    if (!event_pending && !sccb) {
>>         return;
>>     }
> 
> Uh. So when there's no event pending no sclp call may trigger an interrupt?

No. If there is no event pending AND no sccb was specified, then we have nothing to report
--> no interrupt.

An service interrupt has a 32bit value as payload. Bits 0-28 (in IBM speak, 31-3 in Linux speak)
contain the sccb address (if any), the other bits are special. The LSB describes if there are still
events pending.

So an unsolicited interrupt will only set the LSB.
An answer to a service call will always contain the sccb and might have the lsb set if there are
events pending.


>> -    s390_sclp_extint(sccb & ~3);
>> +    s390_sclp_extint((sccb & ~3) + event_pending);
> 
> event_pending returns a bool, right? Please make this code a bit less magical :).

Something like "event_pending?1:0" ?
Alexander Graf - July 30, 2012, 2:57 p.m.
On 30.07.2012, at 16:46, Christian Borntraeger wrote:

> On 30/07/12 15:24, Alexander Graf wrote:
> 
> Thanks for the review.
> 
> Here is just a short answer regarding the interrupt, we will adress the other comments in a later
> mail.
> 
>>> void sclp_service_interrupt(uint32_t sccb)
>>> {
>>> -    if (!sccb) {
>>> +    SCLPEventFacility *ef = sbus->event_facility->instance;
>>> +    int event_pending = sbus->event_facility->event_pending(ef);
>>> +
>>> +    if (!event_pending && !sccb) {
>>>        return;
>>>    }
>> 
>> Uh. So when there's no event pending no sclp call may trigger an interrupt?
> 
> No. If there is no event pending AND no sccb was specified, then we have nothing to report
> --> no interrupt.

Oh. My bad :).

> 
> An service interrupt has a 32bit value as payload. Bits 0-28 (in IBM speak, 31-3 in Linux speak)
> contain the sccb address (if any), the other bits are special. The LSB describes if there are still
> events pending.
> 
> So an unsolicited interrupt will only set the LSB.
> An answer to a service call will always contain the sccb and might have the lsb set if there are
> events pending.

Ok, so this really is a bit we're trying to set then, not an integer we want to add.

> 
> 
>>> -    s390_sclp_extint(sccb & ~3);
>>> +    s390_sclp_extint((sccb & ~3) + event_pending);
>> 
>> event_pending returns a bool, right? Please make this code a bit less magical :).
> 
> Something like "event_pending?1:0" ?

Something like

  param = sccb & ~3;
  /* Indicate whether an event is still pending */
  param |= event_pending ? 1 : 0;

  if (!param) {
    /* No need to send an interrupt, there's nothing to be notified about */
    return;
  }
  s390_sclp_extint(param);


Alex
Andreas Färber - July 31, 2012, 12:59 p.m.
Am 24.07.2012 09:37, schrieb Christian Borntraeger:
> From: Heinz Graalfs <graalfs@linux.vnet.ibm.com>
> 
> Several SCLP features are considered to be events. Those events don't
> provide SCLP commands on their own, instead they are all based on
> Read Event Data, Write Event Data, Write Event Mask and the service
> interrupt. Follow-on patches will provide SCLP's Signal Quiesce (via
> system_powerdown) and the ASCII console.
> Further down the road the sclp line mode console and configuration
> change events (e.g. cpu hotplug) can be implemented.
> 
> Signed-off-by: Heinz Graalfs <graalfs@linux.vnet.ibm.com>
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> ---
>  hw/s390x/Makefile.objs    |    1 +
>  hw/s390x/event-facility.c |  390 +++++++++++++++++++++++++++++++++++++++++++++
>  hw/s390x/event-facility.h |  107 ++++++++++++
>  hw/s390x/sclp.c           |   48 +++++-
>  hw/s390x/sclp.h           |   44 +++++
>  5 files changed, 587 insertions(+), 3 deletions(-)
>  create mode 100644 hw/s390x/event-facility.c
>  create mode 100644 hw/s390x/event-facility.h
> 
> diff --git a/hw/s390x/Makefile.objs b/hw/s390x/Makefile.objs
> index 1c14b96..b32fc52 100644
> --- a/hw/s390x/Makefile.objs
> +++ b/hw/s390x/Makefile.objs
> @@ -2,3 +2,4 @@ obj-y = s390-virtio-bus.o s390-virtio.o
>  
>  obj-y := $(addprefix ../,$(obj-y))
>  obj-y += sclp.o
> +obj-y += event-facility.o
> diff --git a/hw/s390x/event-facility.c b/hw/s390x/event-facility.c
> new file mode 100644
> index 0000000..74a3514
> --- /dev/null
> +++ b/hw/s390x/event-facility.c
> @@ -0,0 +1,390 @@
> +/*
> + * SCLP
> + *    Event Facility
> + *       handles SCLP event types
> + *          - Signal Quiesce - system power down
> + *          - ASCII Console Data - VT220 read and write
> + *
> + * Copyright IBM, Corp. 2012
> + *
> + * Authors:
> + *  Heinz Graalfs <graalfs@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 "monitor.h"
> +#include "sysemu.h"
> +
> +#include "sclp.h"
> +#include "event-facility.h"
> +
> +typedef struct EventTypes {
> +    BusState qbus;
> +    SCLPEventFacility *event_facility;
> +} EventTypes;
> +
> +struct SCLPEventFacility {
> +    EventTypes sbus;
> +    DeviceState *qdev;
> +    /* guest' receive mask */
> +    unsigned int receive_mask;
> +};

The naming here strikes me as particularly odd...

IIUC this Event Facility is a device sitting on the SCLP bus.

But why does it expose a bus named "EventTypes"? Busses are usually
named ...Bus (PCIBus, IDEBus, etc.). So is this actually a bus? If not,
and if all you need is an SCLP-specific list API, maybe compare the
sPAPR hypercall registration API.

Regards,
Andreas
Christian Borntraeger - Aug. 3, 2012, 12:31 p.m.
On 31/07/12 14:59, Andreas Färber wrote:
>> +typedef struct EventTypes {
>> +    BusState qbus;
>> +    SCLPEventFacility *event_facility;
>> +} EventTypes;
>> +
>> +struct SCLPEventFacility {
>> +    EventTypes sbus;
>> +    DeviceState *qdev;
>> +    /* guest' receive mask */
>> +    unsigned int receive_mask;
>> +};
> 
> The naming here strikes me as particularly odd...
> 
> IIUC this Event Facility is a device sitting on the SCLP bus.
> 
> But why does it expose a bus named "EventTypes"? Busses are usually
> named ...Bus (PCIBus, IDEBus, etc.). So is this actually a bus? If not,
> and if all you need is an SCLP-specific list API, maybe compare the
> sPAPR hypercall registration API.

So let me explain it and then we can see what is the right thing to do:

The sclp itself is a service processor that will handle service calls and
can send service interrupts. Most service calls deal with one specific thing:
reading info, cpu hotplug, memory hotplug etc.

Some of the service calls are special, because they form an event subsystem:
Events are features that can notify the guest asynchronously. 
(e.g. system_powerdown is wired to signal quiesce which will be seen as
ctrl-alt-del in the guest, or several console types where the input is sent
as an event). 
The service calls are read_event_data, write_event_data and write_event_mask.

write_event_mask is used by the guest to notify the host
about its event capabilities and to the query the host events.

read_event_data allows a guest to get event data - iow host2guest data.

guest2host traffic also goes via the event mechanism, e.g. console output
is send via write_event_data.

So each event implements several callbacks: event_pending to tell that 
this event is pending, read event data to fill in the buffer with the event data,
write_event_data if the event allows guest2host traffic. There are also two bits
per event that tell if that specific event will allow read/write event data.

Since some of the events implement complex things (console) a bus seemed appropriate,
but there are of course other ways of implementing.
Comments?

Christian
Heinz Graalfs - Aug. 20, 2012, 12:14 p.m.
On Tue, 2012-07-31 at 14:59 +0200, Andreas Färber wrote: 
> Am 24.07.2012 09:37, schrieb Christian Borntraeger:
> > From: Heinz Graalfs <graalfs@linux.vnet.ibm.com>
> > 
> > Several SCLP features are considered to be events. Those events don't
> > provide SCLP commands on their own, instead they are all based on
> > Read Event Data, Write Event Data, Write Event Mask and the service
> > interrupt. Follow-on patches will provide SCLP's Signal Quiesce (via
> > system_powerdown) and the ASCII console.
> > Further down the road the sclp line mode console and configuration
> > change events (e.g. cpu hotplug) can be implemented.
> > 
> > Signed-off-by: Heinz Graalfs <graalfs@linux.vnet.ibm.com>
> > Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> > ---
> >  hw/s390x/Makefile.objs    |    1 +
> >  hw/s390x/event-facility.c |  390 +++++++++++++++++++++++++++++++++++++++++++++
> >  hw/s390x/event-facility.h |  107 ++++++++++++
> >  hw/s390x/sclp.c           |   48 +++++-
> >  hw/s390x/sclp.h           |   44 +++++
> >  5 files changed, 587 insertions(+), 3 deletions(-)
> >  create mode 100644 hw/s390x/event-facility.c
> >  create mode 100644 hw/s390x/event-facility.h
> > 
> > diff --git a/hw/s390x/Makefile.objs b/hw/s390x/Makefile.objs
> > index 1c14b96..b32fc52 100644
> > --- a/hw/s390x/Makefile.objs
> > +++ b/hw/s390x/Makefile.objs
> > @@ -2,3 +2,4 @@ obj-y = s390-virtio-bus.o s390-virtio.o
> >  
> >  obj-y := $(addprefix ../,$(obj-y))
> >  obj-y += sclp.o
> > +obj-y += event-facility.o
> > diff --git a/hw/s390x/event-facility.c b/hw/s390x/event-facility.c
> > new file mode 100644
> > index 0000000..74a3514
> > --- /dev/null
> > +++ b/hw/s390x/event-facility.c
> > @@ -0,0 +1,390 @@
> > +/*
> > + * SCLP
> > + *    Event Facility
> > + *       handles SCLP event types
> > + *          - Signal Quiesce - system power down
> > + *          - ASCII Console Data - VT220 read and write
> > + *
> > + * Copyright IBM, Corp. 2012
> > + *
> > + * Authors:
> > + *  Heinz Graalfs <graalfs@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 "monitor.h"
> > +#include "sysemu.h"
> > +
> > +#include "sclp.h"
> > +#include "event-facility.h"
> > +
> > +typedef struct EventTypes {
> > +    BusState qbus;
> > +    SCLPEventFacility *event_facility;
> > +} EventTypes;
> > +
> > +struct SCLPEventFacility {
> > +    EventTypes sbus;
> > +    DeviceState *qdev;
> > +    /* guest' receive mask */
> > +    unsigned int receive_mask;
> > +};
> 
> The naming here strikes me as particularly odd...
> 
> IIUC this Event Facility is a device sitting on the SCLP bus.
> 
> But why does it expose a bus named "EventTypes"? Busses are usually
> named ...Bus (PCIBus, IDEBus, etc.). So is this actually a bus? If not,
> and if all you need is an SCLP-specific list API, maybe compare the
> sPAPR hypercall registration API.
> 

ok, the Event Facility is now on the main system bus.
We keep the renamed bus EventTypesBus for the sclpquiesce and
sclpconsole (...) items

> Regards,
> Andreas
>
Heinz Graalfs - Aug. 20, 2012, 12:15 p.m.
Alex, some more answers for a couple of leftovers.

Heinz

On Mon, 2012-07-30 at 15:24 +0200, Alexander Graf wrote: 
> On 24.07.2012, at 09:37, Christian Borntraeger wrote:
> 
> > From: Heinz Graalfs <graalfs@linux.vnet.ibm.com>
> > 
> > Several SCLP features are considered to be events. Those events don't
> > provide SCLP commands on their own, instead they are all based on
> > Read Event Data, Write Event Data, Write Event Mask and the service
> > interrupt. Follow-on patches will provide SCLP's Signal Quiesce (via
> > system_powerdown) and the ASCII console.
> > Further down the road the sclp line mode console and configuration
> > change events (e.g. cpu hotplug) can be implemented.
> > 
> > Signed-off-by: Heinz Graalfs <graalfs@linux.vnet.ibm.com>
> > Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> > ---
> > hw/s390x/Makefile.objs    |    1 +
> > hw/s390x/event-facility.c |  390 +++++++++++++++++++++++++++++++++++++++++++++
> > hw/s390x/event-facility.h |  107 ++++++++++++
> > hw/s390x/sclp.c           |   48 +++++-
> > hw/s390x/sclp.h           |   44 +++++
> > 5 files changed, 587 insertions(+), 3 deletions(-)
> > create mode 100644 hw/s390x/event-facility.c
> > create mode 100644 hw/s390x/event-facility.h
> > 
> > diff --git a/hw/s390x/Makefile.objs b/hw/s390x/Makefile.objs
> > index 1c14b96..b32fc52 100644
> > --- a/hw/s390x/Makefile.objs
> > +++ b/hw/s390x/Makefile.objs
> > @@ -2,3 +2,4 @@ obj-y = s390-virtio-bus.o s390-virtio.o
> > 
> > obj-y := $(addprefix ../,$(obj-y))
> > obj-y += sclp.o
> > +obj-y += event-facility.o
> > diff --git a/hw/s390x/event-facility.c b/hw/s390x/event-facility.c
> > new file mode 100644
> > index 0000000..74a3514
> > --- /dev/null
> > +++ b/hw/s390x/event-facility.c
> > @@ -0,0 +1,390 @@
> > +/*
> > + * SCLP
> > + *    Event Facility
> > + *       handles SCLP event types
> > + *          - Signal Quiesce - system power down
> > + *          - ASCII Console Data - VT220 read and write
> > + *
> > + * Copyright IBM, Corp. 2012
> > + *
> > + * Authors:
> > + *  Heinz Graalfs <graalfs@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 "monitor.h"
> > +#include "sysemu.h"
> > +
> > +#include "sclp.h"
> > +#include "event-facility.h"
> > +
> > +typedef struct EventTypes {
> > +    BusState qbus;
> > +    SCLPEventFacility *event_facility;
> > +} EventTypes;
> > +
> > +struct SCLPEventFacility {
> > +    EventTypes sbus;
> > +    DeviceState *qdev;
> > +    /* guest' receive mask */
> > +    unsigned int receive_mask;
> > +};
> > +
> > +/* return true if any child has event pending set */
> > +static bool event_pending(SCLPEventFacility *ef)
> > +{
> > +    BusChild *kid;
> > +    SCLPEvent *event;
> > +
> > +    QTAILQ_FOREACH(kid, &ef->sbus.qbus.children, sibling) {
> > +        DeviceState *qdev = kid->child;
> > +        event = DO_UPCAST(SCLPEvent, qdev, qdev);
> > +        lock(event);
> 
> While I like short function names, you're certainly not the only person who wants locking in the tree :). Please name this a bit more explicitly.
> 
> In fact, why do you need locking in the first place?

Ok, we verified with gdb that character layer always holds the
qemu_global_mutex when calling us and the sclp calls from the cpu
threads also hold the qemu_global_mutex. So I just removed these locks
and verified with valgrinds drd. valgrind complains about the aio code,
but the sclp stuff is fine. 

> 
> > +        if (event->event_pending) {
> > +            unlock(event);
> > +            return true;
> > +        }
> > +        unlock(event);
> > +    }
> > +    return false;
> > +}
> > +
> > +static unsigned int get_host_send_mask(SCLPEventFacility *ef)
> > +{
> > +    unsigned int mask;
> > +    BusChild *kid;
> > +    SCLPEventClass *child;
> > +
> > +    mask = 0;
> > +
> > +    QTAILQ_FOREACH(kid, &ef->sbus.qbus.children, sibling) {
> > +        DeviceState *qdev = kid->child;
> > +        child = SCLP_EVENT_GET_CLASS((SCLPEvent *) qdev);
> > +        mask |= child->get_send_mask();
> > +    }
> > +    return mask;
> > +}
> > +
> > +static unsigned int get_host_receive_mask(SCLPEventFacility *ef)
> > +{
> > +    unsigned int mask;
> > +    BusChild *kid;
> > +    SCLPEventClass *child;
> > +
> > +    mask = 0;
> > +
> > +    QTAILQ_FOREACH(kid, &ef->sbus.qbus.children, sibling) {
> > +        DeviceState *qdev = kid->child;
> > +        child = SCLP_EVENT_GET_CLASS((SCLPEvent *) qdev);
> > +        mask |= child->get_receive_mask();
> > +    }
> > +    return mask;
> > +}
> > +
> > +static bool sccb_events_ok(SCCB *sccb)
> 
> Please return the failure code as return value here. That'd turn the function into completely read-only, making the big picture easier to understand.
> 

OK...

> > +{
> > +    int slen;
> > +    unsigned elen = 0;
> > +    EventBufferHeader *event;
> > +    WriteEventData *wed = (WriteEventData *) sccb;
> 
> Mind to explain why every sccb_event (coming from the function name) is a WriteEventData?
> 

... and function renamed to write_event_length_check

> > +
> > +    event = (EventBufferHeader *) &wed->ebh;
> > +    for (slen = be16_to_cpu(sccb->h.length) - sizeof(sccb->h);
> > +         slen > 0; slen -= elen) {
> > +        elen = be16_to_cpu(event->length);
> > +        if (elen < sizeof(*event) || elen > slen) {
> > +            sccb->h.response_code =
> > +                    cpu_to_be16(SCLP_RC_EVENT_BUFFER_SYNTAX_ERROR);
> > +            return false;
> > +        }
> > +        event = (void *) event + elen;
> > +    }
> > +    if (slen) {
> > +        sccb->h.response_code = cpu_to_be16(SCLP_RC_INCONSISTENT_LENGTHS);
> > +        return false;
> > +    }
> > +    return true;
> > +}
> > +
> > +static void handle_sccb_write_events(SCLPEventFacility *ef, SCCB *sccb)
> > +{
> > +    int slen;
> > +    unsigned elen = 0;
> > +    EventBufferHeader *event_buf;
> > +    BusChild *kid;
> > +    SCLPEvent *event;
> > +    SCLPEventClass *ec;
> > +
> > +    WriteEventData *wed = (WriteEventData *) sccb;
> > +
> > +    event_buf = &wed->ebh;
> > +
> > +    /* loop over all contained event buffers */
> > +    for (slen = be16_to_cpu(sccb->h.length) - sizeof(sccb->h);
> 
> How about a
> 
> static inline int sccb_data_len(SCCB *sccb) {
>   return be16_to_cpu(sccb->h.length) - sizeof(sccb->h);
> }
> 
> in the sclp header, and then
> 
> for (slen = sccb_data_len(sccb); slen > 0; slen -= elen) {
> ...
> 
> here and above?

ok

> 
> > +         slen > 0; slen -= elen) {
> > +        elen = be16_to_cpu(event_buf->length);
> > +
> > +        /* in case of a previous error mark all trailing buffers
> > +         * as not accepted */
> > +        if (sccb->h.response_code !=
> > +                cpu_to_be16(SCLP_RC_NORMAL_COMPLETION)) {
> > +            event_buf->flags &= ~(SCLP_EVENT_BUFFER_ACCEPTED);
> 
> When would we hit this?

in case of a write failed

> 
> > +        } else {
> > +            sccb->h.response_code = cpu_to_be16(SCLP_RC_INVALID_FUNCTION);
> 
> Again, please factor that out to either the parent function or at least the end of this function.
> 

ok, end of this function moved to new function

> > +            QTAILQ_FOREACH(kid, &ef->sbus.qbus.children, sibling) {
> > +                DeviceState *qdev = kid->child;
> > +                event = (SCLPEvent *) qdev;
> > +                ec = SCLP_EVENT_GET_CLASS(event);
> > +
> > +                if (ec->write_event_data &&
> > +                    ec->event_type() == event_buf->type) {
> > +                    sccb->h.response_code = cpu_to_be16(
> > +                            ec->write_event_data(event, event_buf));
> > +                    break;
> > +                }
> > +            }
> > +        }
> > +        event_buf = (void *) event_buf + elen;
> > +    }
> > +}
> > +
> > +static int write_event_data(SCLPEventFacility *ef, SCCB *sccb)
> > +{
> > +    if (sccb->h.function_code != SCLP_FC_NORMAL_WRITE) {
> > +        sccb->h.response_code = cpu_to_be16(SCLP_RC_INVALID_FUNCTION);
> > +        goto out;
> > +    }
> > +    if (be16_to_cpu(sccb->h.length) < 8) {
> > +        sccb->h.response_code = cpu_to_be16(SCLP_RC_INSUFFICIENT_SCCB_LENGTH);
> > +        goto out;
> > +    }
> > +    /* first check the sum of all events */
> > +    if (!sccb_events_ok(sccb)) {
> > +        goto out;
> > +    }
> > +    /* then execute */
> > +    sccb->h.response_code = cpu_to_be16(SCLP_RC_NORMAL_COMPLETION);
> > +    handle_sccb_write_events(ef, sccb);
> > +
> > +out:
> > +    return 0;
> 
> This always returns 0? Let me guess - you originally wanted to pass response_code as return value! ;)
> 
> I think you get the pattern by now :)
> 

OK

> > +}
> > +
> > +static void handle_sccb_read_events(SCLPEventFacility *ef, SCCB *sccb,
> > +                                    unsigned int mask)
> > +{
> > +    int slen;
> > +    unsigned elen = 0;
> > +    BusChild *kid;
> > +    SCLPEvent *event;
> > +    SCLPEventClass *ec;
> > +    EventBufferHeader *event_buf;
> > +    ReadEventData *red = (ReadEventData *) sccb;
> > +
> > +    event_buf = &red->ebh;
> > +    event_buf->length = 0;
> > +    slen = sizeof(sccb->data);
> > +    QTAILQ_FOREACH(kid, &ef->sbus.qbus.children, sibling) {
> > +        DeviceState *qdev = kid->child;
> > +        event = (SCLPEvent *) qdev;
> > +        ec = SCLP_EVENT_GET_CLASS(event);
> > +
> > +        if (mask & ec->get_send_mask()) {
> > +            if (ec->read_event_data(event, event_buf, &slen)) {
> > +                sccb->h.response_code = cpu_to_be16(SCLP_RC_NORMAL_COMPLETION);
> 
> What is the response code when we don't have any data?
> 

SCLP_RC_NO_EVENT_BUFFERS_STORED (moved now from parent to this function)

> > +            }
> > +        }
> > +        elen = be16_to_cpu(event_buf->length);
> > +        event_buf = (void *) event_buf + elen;
> > +    }
> > +
> > +    if (sccb->h.control_mask[2] & SCLP_VARIABLE_LENGTH_RESPONSE) {
> 
> This block deserves a comment.
> 

OK

> > +        sccb->h.control_mask[2] &= ~SCLP_VARIABLE_LENGTH_RESPONSE;
> > +        sccb->h.length = cpu_to_be16(SCCB_SIZE - slen);
> > +    }
> > +}
> > +
> > +static int read_event_data(SCLPEventFacility *ef, SCCB *sccb)
> > +{
> > +    unsigned int sclp_active_selection_mask;
> > +    unsigned int sclp_cp_receive_mask;
> > +
> > +    ReadEventData *red = (ReadEventData *) sccb;
> > +
> > +    if (be16_to_cpu(sccb->h.length) != SCCB_SIZE) {
> > +        sccb->h.response_code = cpu_to_be16(SCLP_RC_INSUFFICIENT_SCCB_LENGTH);
> > +        goto out;
> > +    }
> > +
> > +    sclp_cp_receive_mask = ef->receive_mask;
> > +
> > +    /* get active selection mask */
> > +    switch (sccb->h.function_code) {
> > +    case SCLP_UNCONDITIONAL_READ:
> > +        sclp_active_selection_mask = sclp_cp_receive_mask;
> > +        break;
> > +    case SCLP_SELECTIVE_READ:
> > +        if (!(sclp_cp_receive_mask & be32_to_cpu(red->mask))) {
> > +            sccb->h.response_code =
> > +                    cpu_to_be16(SCLP_RC_INVALID_SELECTION_MASK);
> > +            goto out;
> > +        }
> > +        sclp_active_selection_mask = be32_to_cpu(red->mask);
> > +        break;
> > +    default:
> > +        sccb->h.response_code = cpu_to_be16(SCLP_RC_INVALID_FUNCTION);
> > +        goto out;
> > +    }
> > +
> > +    sccb->h.response_code = cpu_to_be16(SCLP_RC_NO_EVENT_BUFFERS_STORED);
> > +    handle_sccb_read_events(ef, sccb, sclp_active_selection_mask);
> > +
> > +out:
> > +    return 0;
> > +}
> > +
> > +static int write_event_mask(SCLPEventFacility *ef, SCCB *sccb)
> > +{
> > +    WriteEventMask *we_mask = (WriteEventMask *) sccb;
> > +
> > +    /* Attention: We assume that Linux uses 4-byte masks, what it actually
> > +       does. Architecture allows for masks of variable size, though */
> > +    if (be16_to_cpu(we_mask->mask_length) != 4) {
> > +        sccb->h.response_code = cpu_to_be16(SCLP_RC_INVALID_MASK_LENGTH);
> > +        goto out;
> > +    }
> > +
> > +    /* keep track of the guest's capability masks */
> > +    ef->receive_mask = be32_to_cpu(we_mask->cp_receive_mask);
> > +
> > +    /* return the SCLP's capability masks to the guest */
> > +    we_mask->send_mask = cpu_to_be32(get_host_send_mask(ef));
> > +    we_mask->receive_mask = cpu_to_be32(get_host_receive_mask(ef));
> > +
> > +    sccb->h.response_code = cpu_to_be16(SCLP_RC_NORMAL_COMPLETION);
> > +
> > +out:
> > +    return 0;
> > +}
> > +
> > +/* qemu object creation and initialization functions */
> > +
> > +#define TYPE_SCLP_EVENTS_BUS "s390-sclp-events-bus"
> > +#define SCLP_EVENTS_BUS(obj) OBJECT_CHECK(SCLPS390Bus, (obj),\
> > +                             TYPE_SCLP_EVENTS_BUS)
> > +
> > +static void sclp_events_bus_class_init(ObjectClass *klass, void *data)
> > +{
> > +}
> > +
> > +static const TypeInfo s390_sclp_events_bus_info = {
> > +    .name = TYPE_SCLP_EVENTS_BUS,
> > +    .parent = TYPE_BUS,
> > +    .instance_size = sizeof(SCLPS390Bus),
> > +    .class_init = sclp_events_bus_class_init,
> > +};
> > +
> > +static int command_handler(SCLPEventFacility *ef, SCCB *sccb, uint64_t code)
> > +{
> > +    int r = 0;
> > +
> > +    switch (code) {
> > +    case SCLP_CMD_READ_EVENT_DATA:
> > +        r = read_event_data(ef, sccb);
> > +        break;
> > +    case SCLP_CMD_WRITE_EVENT_DATA:
> > +        r = write_event_data(ef, sccb);
> > +        break;
> > +    case SCLP_CMD_WRITE_EVENT_MASK:
> > +        r = write_event_mask(ef, sccb);
> > +        break;
> > +    default:
> > +#ifdef DEBUG_HELPER
> 
> Why DEBUG_HELPER?

OK, removed

> 
> > +        printf("KVM: unhandled sclp code 0x%" PRIx64 "x\n", code);
> > +#endif
> > +        sccb->h.response_code = cpu_to_be16(SCLP_RC_INVALID_SCLP_COMMAND);
> > +        break;
> > +    }
> > +    return r;
> > +}
> > +
> > +static int init_event_facility(S390SCLPDevice *sdev)
> > +{
> > +    SCLPEventFacility *event_facility;
> > +
> > +    event_facility = g_malloc0(sizeof(SCLPEventFacility));
> > +    sdev->instance = event_facility;
> > +    sdev->sclp_command_handler = command_handler;
> > +    sdev->event_pending = event_pending;
> > +
> > +    /* Spawn a new sclp-events facility */
> > +    qbus_create_inplace(&event_facility->sbus.qbus,
> > +                        TYPE_SCLP_EVENTS_BUS, (DeviceState *)sdev, NULL);
> > +    event_facility->sbus.qbus.allow_hotplug = 0;
> > +    event_facility->sbus.event_facility = event_facility;
> > +    event_facility->qdev = (DeviceState *) sdev;
> > +
> > +    return 0;
> > +}
> > +
> > +static void init_event_facility_class(ObjectClass *klass, void *data)
> > +{
> > +    S390SCLPDeviceClass *k = SCLP_S390_DEVICE_CLASS(klass);
> > +
> > +    k->init = init_event_facility;
> > +}
> > +
> > +static TypeInfo s390_sclp_event_facility_info = {
> > +    .name          = "s390-sclp-event-facility",
> > +    .parent        = TYPE_DEVICE_S390_SCLP,
> > +    .instance_size = sizeof(S390SCLPDevice),
> > +    .class_init    = init_event_facility_class,
> > +};
> > +
> > +static int event_qdev_init(DeviceState *qdev)
> > +{
> > +    SCLPEvent *event = DO_UPCAST(SCLPEvent, qdev, qdev);
> > +    SCLPEventClass *child = SCLP_EVENT_GET_CLASS(event);
> > +
> > +    return child->init(event);
> > +}
> > +
> > +static int event_qdev_exit(DeviceState *qdev)
> > +{
> > +    SCLPEvent *event = DO_UPCAST(SCLPEvent, qdev, qdev);
> > +    SCLPEventClass *child = SCLP_EVENT_GET_CLASS(event);
> > +    if (child->exit) {
> > +        child->exit(event);
> > +    }
> > +    return 0;
> > +}
> > +
> > +static void event_class_init(ObjectClass *klass, void *data)
> > +{
> > +    DeviceClass *dc = DEVICE_CLASS(klass);
> > +
> > +    dc->bus_type = TYPE_SCLP_EVENTS_BUS;
> > +    dc->unplug = qdev_simple_unplug_cb;
> > +    dc->init = event_qdev_init;
> > +    dc->exit = event_qdev_exit;
> > +}
> > +
> > +static TypeInfo s390_sclp_event_type_info = {
> > +    .name = TYPE_SCLP_EVENT,
> > +    .parent = TYPE_DEVICE,
> > +    .instance_size = sizeof(SCLPEvent),
> > +    .class_init = event_class_init,
> > +    .class_size = sizeof(SCLPEventClass),
> > +    .abstract = true,
> > +};
> > +
> > +static void register_types(void)
> > +{
> > +    type_register_static(&s390_sclp_events_bus_info);
> > +    type_register_static(&s390_sclp_event_facility_info);
> > +    type_register_static(&s390_sclp_event_type_info);
> > +}
> > +type_init(register_types)
> > diff --git a/hw/s390x/event-facility.h b/hw/s390x/event-facility.h
> > new file mode 100644
> > index 0000000..1e022a3
> > --- /dev/null
> > +++ b/hw/s390x/event-facility.h
> > @@ -0,0 +1,107 @@
> > +/*
> > + * SCLP
> > + *    Event Facility definitions
> > + *
> > + * Copyright IBM, Corp. 2012
> > + *
> > + * Authors:
> > + *  Heinz Graalfs <graalfs@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.
> > + *
> > + */
> > +
> > +#ifndef HW_S390_SCLP_EVENT_FACILITY_H
> > +#define HW_S390_SCLP_EVENT_FACILITY_H
> > +
> > +#include <hw/qdev.h>
> > +#include "qemu-thread.h"
> > +
> > +/* SCLP event types */
> > +#define SCLP_EVENT_ASCII_CONSOLE_DATA           0x1a
> > +#define SCLP_EVENT_SIGNAL_QUIESCE               0x1d
> > +
> > +/* SCLP event masks */
> > +#define SCLP_EVENT_MASK_SIGNAL_QUIESCE          0x00000008
> > +#define SCLP_EVENT_MASK_MSG_ASCII               0x00000040
> > +
> > +#define SCLP_UNCONDITIONAL_READ                 0x00
> > +#define SCLP_SELECTIVE_READ                     0x01
> > +
> > +#define TYPE_SCLP_EVENT "s390-sclp-event-type"
> > +#define SCLP_EVENT(obj) \
> > +     OBJECT_CHECK(SCLPEvent, (obj), TYPE_SCLP_EVENT)
> > +#define SCLP_EVENT_CLASS(klass) \
> > +     OBJECT_CLASS_CHECK(SCLPEventClass, (klass), TYPE_SCLP_EVENT)
> > +#define SCLP_EVENT_GET_CLASS(obj) \
> > +     OBJECT_GET_CLASS(SCLPEventClass, (obj), TYPE_SCLP_EVENT)
> > +
> > +typedef struct WriteEventMask {
> > +    SCCBHeader h;
> > +    uint16_t _reserved;
> > +    uint16_t mask_length;
> > +    uint32_t cp_receive_mask;
> > +    uint32_t cp_send_mask;
> > +    uint32_t send_mask;
> > +    uint32_t receive_mask;
> > +} QEMU_PACKED WriteEventMask;
> > +
> > +typedef struct EventBufferHeader {
> > +    uint16_t length;
> > +    uint8_t  type;
> > +    uint8_t  flags;
> > +    uint16_t _reserved;
> > +} QEMU_PACKED EventBufferHeader;
> > +
> > +typedef struct WriteEventData {
> > +    SCCBHeader h;
> > +    EventBufferHeader ebh;
> > +} QEMU_PACKED WriteEventData;
> > +
> > +typedef struct ReadEventData {
> > +    SCCBHeader h;
> > +    EventBufferHeader ebh;
> > +    uint32_t mask;
> > +} QEMU_PACKED ReadEventData;
> > +
> > +typedef struct SCLPEvent {
> > +    DeviceState qdev;
> > +    QemuMutex lock;
> > +    bool event_pending;
> > +    uint32_t event_type;
> > +    char *name;
> > +} SCLPEvent;
> > +
> > +typedef struct SCLPEventClass {
> > +    DeviceClass parent_class;
> > +    int (*init)(SCLPEvent *event);
> > +    int (*exit)(SCLPEvent *event);
> > +
> > +    /* get SCLP's send mask */
> > +    unsigned int (*get_send_mask)(void);
> > +
> > +    /* get SCLP's receive mask */
> > +    unsigned int (*get_receive_mask)(void);
> > +
> > +    int (*read_event_data)(SCLPEvent *event, EventBufferHeader *evt_buf_hdr,
> > +                           int *slen);
> > +
> > +    int (*write_event_data)(SCLPEvent *event, EventBufferHeader *evt_buf_hdr);
> > +
> > +    /* returns the supported event type */
> > +    int (*event_type)(void);
> > +
> > +} SCLPEventClass;
> > +
> > +static inline void lock(SCLPEvent *event)
> > +{
> > +    qemu_mutex_lock(&event->lock);
> 
> Yeah, I'm fairly sure you don't need the locks :). It might be nice to keep them in as documentation, but make them noops (and name them properly).
> 

ok, locking removed

> > +}
> > +
> > +static inline void unlock(SCLPEvent *event)
> > +{
> > +    qemu_mutex_unlock(&event->lock);
> > +}
> > +
> > +#endif
> > diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
> > index 4095ba6..cfc41ea 100644
> > --- a/hw/s390x/sclp.c
> > +++ b/hw/s390x/sclp.c
> > @@ -39,6 +39,7 @@ static int read_SCP_info(SCCB *sccb)
> > static int sclp_execute(SCCB *sccb, uint64_t code)
> > {
> >     int r = 0;
> > +    SCLPEventFacility *ef = sbus->event_facility->instance;
> > 
> >     switch (code) {
> >     case SCLP_CMDW_READ_SCP_INFO:
> > @@ -46,7 +47,7 @@ static int sclp_execute(SCCB *sccb, uint64_t code)
> >         r = read_SCP_info(sccb);
> >         break;
> >     default:
> > -        sccb->h.response_code = cpu_to_be16(SCLP_RC_INVALID_SCLP_COMMAND);
> > +        r = sbus->event_facility->sclp_command_handler(ef, sccb, code);
> >         break;
> >     }
> >     return r;
> > @@ -87,10 +88,13 @@ out:
> > 
> > void sclp_service_interrupt(uint32_t sccb)
> > {
> > -    if (!sccb) {
> > +    SCLPEventFacility *ef = sbus->event_facility->instance;
> > +    int event_pending = sbus->event_facility->event_pending(ef);
> > +
> > +    if (!event_pending && !sccb) {
> 
> Uh. So when there's no event pending no sclp call may trigger an interrupt?
> 
> >         return;
> >     }
> > -    s390_sclp_extint(sccb & ~3);
> > +    s390_sclp_extint((sccb & ~3) + event_pending);
> 
> event_pending returns a bool, right? Please make this code a bit less magical :).
> 
> 
> 
> Alex
> 
> > }
> > 
> > /* qemu object creation and initialization functions */
> > @@ -112,6 +116,9 @@ SCLPS390Bus *s390_sclp_bus_init(void)
> >     bus_state = qbus_create(TYPE_S390_SCLP_BUS, dev, NULL);
> >     bus_state->allow_hotplug = 0;
> > 
> > +    dev = qdev_create(bus_state, "s390-sclp-event-facility");
> > +    qdev_init_nofail(dev);
> > +
> >     sbus = DO_UPCAST(SCLPS390Bus, bus, bus_state);
> >     return sbus;
> > }
> > @@ -137,9 +144,44 @@ static TypeInfo s390_sclp_bridge_info = {
> >     .class_init    = s390_sclp_bridge_class_init,
> > };
> > 
> > +static int s390_sclp_busdev_init(DeviceState *dev)
> > +{
> > +    int r;
> > +    S390SCLPDevice *sdev = (S390SCLPDevice *)dev;
> > +    S390SCLPDeviceClass *sclp = SCLP_S390_DEVICE_GET_CLASS(dev);
> > +    SCLPS390Bus *bus = DO_UPCAST(SCLPS390Bus, bus, sdev->qdev.parent_bus);
> > +
> > +    r = sclp->init(sdev);
> > +    if (!r) {
> > +        assert(sdev->event_pending);
> > +        assert(sdev->sclp_command_handler);
> > +    }
> > +    bus->event_facility = sdev;
> > +
> > +    return r;
> > +}
> > +
> > +static void s390_sclp_device_class_init(ObjectClass *klass, void *data)
> > +{
> > +    DeviceClass *dc = DEVICE_CLASS(klass);
> > +
> > +    dc->init = s390_sclp_busdev_init;
> > +    dc->bus_type = TYPE_S390_SCLP_BUS;
> > +}
> > +
> > +static TypeInfo s390_sclp_device_info = {
> > +    .name = TYPE_DEVICE_S390_SCLP,
> > +    .parent = TYPE_DEVICE,
> > +    .instance_size = sizeof(S390SCLPDevice),
> > +    .class_init = s390_sclp_device_class_init,
> > +    .class_size = sizeof(S390SCLPDeviceClass),
> > +    .abstract = true,
> > +};
> > +
> > static void s390_sclp_register_types(void)
> > {
> >     type_register_static(&s390_sclp_bridge_info);
> >     type_register_static(&s390_sclp_bus_info);
> > +    type_register_static(&s390_sclp_device_info);
> > }
> > type_init(s390_sclp_register_types)
> > diff --git a/hw/s390x/sclp.h b/hw/s390x/sclp.h
> > index 4d5a946..d071ccd 100644
> > --- a/hw/s390x/sclp.h
> > +++ b/hw/s390x/sclp.h
> > @@ -19,15 +19,35 @@
> > /* SCLP command codes */
> > #define SCLP_CMDW_READ_SCP_INFO                 0x00020001
> > #define SCLP_CMDW_READ_SCP_INFO_FORCED          0x00120001
> > +#define SCLP_CMD_READ_EVENT_DATA                0x00770005
> > +#define SCLP_CMD_WRITE_EVENT_DATA               0x00760005
> > +#define SCLP_CMD_READ_EVENT_DATA                0x00770005
> > +#define SCLP_CMD_WRITE_EVENT_DATA               0x00760005
> > +#define SCLP_CMD_WRITE_EVENT_MASK               0x00780005
> > 
> > /* SCLP response codes */
> > #define SCLP_RC_NORMAL_READ_COMPLETION          0x0010
> > +#define SCLP_RC_NORMAL_COMPLETION               0x0020
> > #define SCLP_RC_INVALID_SCLP_COMMAND            0x01f0
> > +#define SCLP_RC_CONTAINED_EQUIPMENT_CHECK       0x0340
> > +#define SCLP_RC_INSUFFICIENT_SCCB_LENGTH        0x0300
> > +#define SCLP_RC_INVALID_FUNCTION                0x40f0
> > +#define SCLP_RC_NO_EVENT_BUFFERS_STORED         0x60f0
> > +#define SCLP_RC_INVALID_SELECTION_MASK          0x70f0
> > +#define SCLP_RC_INCONSISTENT_LENGTHS            0x72f0
> > +#define SCLP_RC_EVENT_BUFFER_SYNTAX_ERROR       0x73f0
> > +#define SCLP_RC_INVALID_MASK_LENGTH             0x74f0
> > +
> > 
> > /* Service Call Control Block (SCCB) and its elements */
> > 
> > #define SCCB_SIZE 4096
> > 
> > +#define SCLP_VARIABLE_LENGTH_RESPONSE           0x80
> > +#define SCLP_EVENT_BUFFER_ACCEPTED              0x80
> > +
> > +#define SCLP_FC_NORMAL_WRITE                    0
> > +
> > /*
> >  * Normally packed structures are not the right thing to do, since all code
> >  * must take care of endianess. We cant use ldl_phys and friends for two
> > @@ -64,14 +84,38 @@ typedef struct SCCB {
> > #define TYPE_S390_SCLP_BUS "s390-sclp-bus"
> > #define S390_SCLP_BUS(obj) OBJECT_CHECK(SCLPS390Bus, (obj), TYPE_S390_SCLP_BUS)
> > 
> > +#define TYPE_DEVICE_S390_SCLP "s390-sclp-device"
> > +#define SCLP_S390_DEVIVE(obj) \
> > +     OBJECT_CHECK(S390SCLPDevice, (obj), TYPE_DEVICE_S390_SCLP)
> > +#define SCLP_S390_DEVICE_CLASS(klass) \
> > +     OBJECT_CLASS_CHECK(S390SCLPDeviceClass, (klass), \
> > +             TYPE_DEVICE_S390_SCLP)
> > +#define SCLP_S390_DEVICE_GET_CLASS(obj) \
> > +     OBJECT_GET_CLASS(S390SCLPDeviceClass, (obj), \
> > +             TYPE_DEVICE_S390_SCLP)
> > +
> > +typedef struct SCLPEventFacility SCLPEventFacility;
> > +
> > typedef struct S390SCLPDevice {
> >     DeviceState qdev;
> > +    SCLPEventFacility *instance;
> > +    int (*sclp_command_handler)(SCLPEventFacility *ef, SCCB *sccb,
> > +                                uint64_t code);
> > +    bool (*event_pending)(SCLPEventFacility *ef);
> > } S390SCLPDevice;
> > 
> > typedef struct SCLPS390Bus {
> >     BusState bus;
> > +    S390SCLPDevice *event_facility;
> > } SCLPS390Bus;
> > 
> > +typedef struct S390SCLPDeviceClass {
> > +    DeviceClass qdev;
> > +
> > +    int (*init)(S390SCLPDevice *sdev);
> > +
> > +} S390SCLPDeviceClass;
> > +
> > SCLPS390Bus *s390_sclp_bus_init(void);
> > 
> > void sclp_service_interrupt(uint32_t sccb);
> > -- 
> > 1.7.0.1
> > 
>

Patch

diff --git a/hw/s390x/Makefile.objs b/hw/s390x/Makefile.objs
index 1c14b96..b32fc52 100644
--- a/hw/s390x/Makefile.objs
+++ b/hw/s390x/Makefile.objs
@@ -2,3 +2,4 @@  obj-y = s390-virtio-bus.o s390-virtio.o
 
 obj-y := $(addprefix ../,$(obj-y))
 obj-y += sclp.o
+obj-y += event-facility.o
diff --git a/hw/s390x/event-facility.c b/hw/s390x/event-facility.c
new file mode 100644
index 0000000..74a3514
--- /dev/null
+++ b/hw/s390x/event-facility.c
@@ -0,0 +1,390 @@ 
+/*
+ * SCLP
+ *    Event Facility
+ *       handles SCLP event types
+ *          - Signal Quiesce - system power down
+ *          - ASCII Console Data - VT220 read and write
+ *
+ * Copyright IBM, Corp. 2012
+ *
+ * Authors:
+ *  Heinz Graalfs <graalfs@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 "monitor.h"
+#include "sysemu.h"
+
+#include "sclp.h"
+#include "event-facility.h"
+
+typedef struct EventTypes {
+    BusState qbus;
+    SCLPEventFacility *event_facility;
+} EventTypes;
+
+struct SCLPEventFacility {
+    EventTypes sbus;
+    DeviceState *qdev;
+    /* guest' receive mask */
+    unsigned int receive_mask;
+};
+
+/* return true if any child has event pending set */
+static bool event_pending(SCLPEventFacility *ef)
+{
+    BusChild *kid;
+    SCLPEvent *event;
+
+    QTAILQ_FOREACH(kid, &ef->sbus.qbus.children, sibling) {
+        DeviceState *qdev = kid->child;
+        event = DO_UPCAST(SCLPEvent, qdev, qdev);
+        lock(event);
+        if (event->event_pending) {
+            unlock(event);
+            return true;
+        }
+        unlock(event);
+    }
+    return false;
+}
+
+static unsigned int get_host_send_mask(SCLPEventFacility *ef)
+{
+    unsigned int mask;
+    BusChild *kid;
+    SCLPEventClass *child;
+
+    mask = 0;
+
+    QTAILQ_FOREACH(kid, &ef->sbus.qbus.children, sibling) {
+        DeviceState *qdev = kid->child;
+        child = SCLP_EVENT_GET_CLASS((SCLPEvent *) qdev);
+        mask |= child->get_send_mask();
+    }
+    return mask;
+}
+
+static unsigned int get_host_receive_mask(SCLPEventFacility *ef)
+{
+    unsigned int mask;
+    BusChild *kid;
+    SCLPEventClass *child;
+
+    mask = 0;
+
+    QTAILQ_FOREACH(kid, &ef->sbus.qbus.children, sibling) {
+        DeviceState *qdev = kid->child;
+        child = SCLP_EVENT_GET_CLASS((SCLPEvent *) qdev);
+        mask |= child->get_receive_mask();
+    }
+    return mask;
+}
+
+static bool sccb_events_ok(SCCB *sccb)
+{
+    int slen;
+    unsigned elen = 0;
+    EventBufferHeader *event;
+    WriteEventData *wed = (WriteEventData *) sccb;
+
+    event = (EventBufferHeader *) &wed->ebh;
+    for (slen = be16_to_cpu(sccb->h.length) - sizeof(sccb->h);
+         slen > 0; slen -= elen) {
+        elen = be16_to_cpu(event->length);
+        if (elen < sizeof(*event) || elen > slen) {
+            sccb->h.response_code =
+                    cpu_to_be16(SCLP_RC_EVENT_BUFFER_SYNTAX_ERROR);
+            return false;
+        }
+        event = (void *) event + elen;
+    }
+    if (slen) {
+        sccb->h.response_code = cpu_to_be16(SCLP_RC_INCONSISTENT_LENGTHS);
+        return false;
+    }
+    return true;
+}
+
+static void handle_sccb_write_events(SCLPEventFacility *ef, SCCB *sccb)
+{
+    int slen;
+    unsigned elen = 0;
+    EventBufferHeader *event_buf;
+    BusChild *kid;
+    SCLPEvent *event;
+    SCLPEventClass *ec;
+
+    WriteEventData *wed = (WriteEventData *) sccb;
+
+    event_buf = &wed->ebh;
+
+    /* loop over all contained event buffers */
+    for (slen = be16_to_cpu(sccb->h.length) - sizeof(sccb->h);
+         slen > 0; slen -= elen) {
+        elen = be16_to_cpu(event_buf->length);
+
+        /* in case of a previous error mark all trailing buffers
+         * as not accepted */
+        if (sccb->h.response_code !=
+                cpu_to_be16(SCLP_RC_NORMAL_COMPLETION)) {
+            event_buf->flags &= ~(SCLP_EVENT_BUFFER_ACCEPTED);
+        } else {
+            sccb->h.response_code = cpu_to_be16(SCLP_RC_INVALID_FUNCTION);
+            QTAILQ_FOREACH(kid, &ef->sbus.qbus.children, sibling) {
+                DeviceState *qdev = kid->child;
+                event = (SCLPEvent *) qdev;
+                ec = SCLP_EVENT_GET_CLASS(event);
+
+                if (ec->write_event_data &&
+                    ec->event_type() == event_buf->type) {
+                    sccb->h.response_code = cpu_to_be16(
+                            ec->write_event_data(event, event_buf));
+                    break;
+                }
+            }
+        }
+        event_buf = (void *) event_buf + elen;
+    }
+}
+
+static int write_event_data(SCLPEventFacility *ef, SCCB *sccb)
+{
+    if (sccb->h.function_code != SCLP_FC_NORMAL_WRITE) {
+        sccb->h.response_code = cpu_to_be16(SCLP_RC_INVALID_FUNCTION);
+        goto out;
+    }
+    if (be16_to_cpu(sccb->h.length) < 8) {
+        sccb->h.response_code = cpu_to_be16(SCLP_RC_INSUFFICIENT_SCCB_LENGTH);
+        goto out;
+    }
+    /* first check the sum of all events */
+    if (!sccb_events_ok(sccb)) {
+        goto out;
+    }
+    /* then execute */
+    sccb->h.response_code = cpu_to_be16(SCLP_RC_NORMAL_COMPLETION);
+    handle_sccb_write_events(ef, sccb);
+
+out:
+    return 0;
+}
+
+static void handle_sccb_read_events(SCLPEventFacility *ef, SCCB *sccb,
+                                    unsigned int mask)
+{
+    int slen;
+    unsigned elen = 0;
+    BusChild *kid;
+    SCLPEvent *event;
+    SCLPEventClass *ec;
+    EventBufferHeader *event_buf;
+    ReadEventData *red = (ReadEventData *) sccb;
+
+    event_buf = &red->ebh;
+    event_buf->length = 0;
+    slen = sizeof(sccb->data);
+    QTAILQ_FOREACH(kid, &ef->sbus.qbus.children, sibling) {
+        DeviceState *qdev = kid->child;
+        event = (SCLPEvent *) qdev;
+        ec = SCLP_EVENT_GET_CLASS(event);
+
+        if (mask & ec->get_send_mask()) {
+            if (ec->read_event_data(event, event_buf, &slen)) {
+                sccb->h.response_code = cpu_to_be16(SCLP_RC_NORMAL_COMPLETION);
+            }
+        }
+        elen = be16_to_cpu(event_buf->length);
+        event_buf = (void *) event_buf + elen;
+    }
+
+    if (sccb->h.control_mask[2] & SCLP_VARIABLE_LENGTH_RESPONSE) {
+        sccb->h.control_mask[2] &= ~SCLP_VARIABLE_LENGTH_RESPONSE;
+        sccb->h.length = cpu_to_be16(SCCB_SIZE - slen);
+    }
+}
+
+static int read_event_data(SCLPEventFacility *ef, SCCB *sccb)
+{
+    unsigned int sclp_active_selection_mask;
+    unsigned int sclp_cp_receive_mask;
+
+    ReadEventData *red = (ReadEventData *) sccb;
+
+    if (be16_to_cpu(sccb->h.length) != SCCB_SIZE) {
+        sccb->h.response_code = cpu_to_be16(SCLP_RC_INSUFFICIENT_SCCB_LENGTH);
+        goto out;
+    }
+
+    sclp_cp_receive_mask = ef->receive_mask;
+
+    /* get active selection mask */
+    switch (sccb->h.function_code) {
+    case SCLP_UNCONDITIONAL_READ:
+        sclp_active_selection_mask = sclp_cp_receive_mask;
+        break;
+    case SCLP_SELECTIVE_READ:
+        if (!(sclp_cp_receive_mask & be32_to_cpu(red->mask))) {
+            sccb->h.response_code =
+                    cpu_to_be16(SCLP_RC_INVALID_SELECTION_MASK);
+            goto out;
+        }
+        sclp_active_selection_mask = be32_to_cpu(red->mask);
+        break;
+    default:
+        sccb->h.response_code = cpu_to_be16(SCLP_RC_INVALID_FUNCTION);
+        goto out;
+    }
+
+    sccb->h.response_code = cpu_to_be16(SCLP_RC_NO_EVENT_BUFFERS_STORED);
+    handle_sccb_read_events(ef, sccb, sclp_active_selection_mask);
+
+out:
+    return 0;
+}
+
+static int write_event_mask(SCLPEventFacility *ef, SCCB *sccb)
+{
+    WriteEventMask *we_mask = (WriteEventMask *) sccb;
+
+    /* Attention: We assume that Linux uses 4-byte masks, what it actually
+       does. Architecture allows for masks of variable size, though */
+    if (be16_to_cpu(we_mask->mask_length) != 4) {
+        sccb->h.response_code = cpu_to_be16(SCLP_RC_INVALID_MASK_LENGTH);
+        goto out;
+    }
+
+    /* keep track of the guest's capability masks */
+    ef->receive_mask = be32_to_cpu(we_mask->cp_receive_mask);
+
+    /* return the SCLP's capability masks to the guest */
+    we_mask->send_mask = cpu_to_be32(get_host_send_mask(ef));
+    we_mask->receive_mask = cpu_to_be32(get_host_receive_mask(ef));
+
+    sccb->h.response_code = cpu_to_be16(SCLP_RC_NORMAL_COMPLETION);
+
+out:
+    return 0;
+}
+
+/* qemu object creation and initialization functions */
+
+#define TYPE_SCLP_EVENTS_BUS "s390-sclp-events-bus"
+#define SCLP_EVENTS_BUS(obj) OBJECT_CHECK(SCLPS390Bus, (obj),\
+                             TYPE_SCLP_EVENTS_BUS)
+
+static void sclp_events_bus_class_init(ObjectClass *klass, void *data)
+{
+}
+
+static const TypeInfo s390_sclp_events_bus_info = {
+    .name = TYPE_SCLP_EVENTS_BUS,
+    .parent = TYPE_BUS,
+    .instance_size = sizeof(SCLPS390Bus),
+    .class_init = sclp_events_bus_class_init,
+};
+
+static int command_handler(SCLPEventFacility *ef, SCCB *sccb, uint64_t code)
+{
+    int r = 0;
+
+    switch (code) {
+    case SCLP_CMD_READ_EVENT_DATA:
+        r = read_event_data(ef, sccb);
+        break;
+    case SCLP_CMD_WRITE_EVENT_DATA:
+        r = write_event_data(ef, sccb);
+        break;
+    case SCLP_CMD_WRITE_EVENT_MASK:
+        r = write_event_mask(ef, sccb);
+        break;
+    default:
+#ifdef DEBUG_HELPER
+        printf("KVM: unhandled sclp code 0x%" PRIx64 "x\n", code);
+#endif
+        sccb->h.response_code = cpu_to_be16(SCLP_RC_INVALID_SCLP_COMMAND);
+        break;
+    }
+    return r;
+}
+
+static int init_event_facility(S390SCLPDevice *sdev)
+{
+    SCLPEventFacility *event_facility;
+
+    event_facility = g_malloc0(sizeof(SCLPEventFacility));
+    sdev->instance = event_facility;
+    sdev->sclp_command_handler = command_handler;
+    sdev->event_pending = event_pending;
+
+    /* Spawn a new sclp-events facility */
+    qbus_create_inplace(&event_facility->sbus.qbus,
+                        TYPE_SCLP_EVENTS_BUS, (DeviceState *)sdev, NULL);
+    event_facility->sbus.qbus.allow_hotplug = 0;
+    event_facility->sbus.event_facility = event_facility;
+    event_facility->qdev = (DeviceState *) sdev;
+
+    return 0;
+}
+
+static void init_event_facility_class(ObjectClass *klass, void *data)
+{
+    S390SCLPDeviceClass *k = SCLP_S390_DEVICE_CLASS(klass);
+
+    k->init = init_event_facility;
+}
+
+static TypeInfo s390_sclp_event_facility_info = {
+    .name          = "s390-sclp-event-facility",
+    .parent        = TYPE_DEVICE_S390_SCLP,
+    .instance_size = sizeof(S390SCLPDevice),
+    .class_init    = init_event_facility_class,
+};
+
+static int event_qdev_init(DeviceState *qdev)
+{
+    SCLPEvent *event = DO_UPCAST(SCLPEvent, qdev, qdev);
+    SCLPEventClass *child = SCLP_EVENT_GET_CLASS(event);
+
+    return child->init(event);
+}
+
+static int event_qdev_exit(DeviceState *qdev)
+{
+    SCLPEvent *event = DO_UPCAST(SCLPEvent, qdev, qdev);
+    SCLPEventClass *child = SCLP_EVENT_GET_CLASS(event);
+    if (child->exit) {
+        child->exit(event);
+    }
+    return 0;
+}
+
+static void event_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+
+    dc->bus_type = TYPE_SCLP_EVENTS_BUS;
+    dc->unplug = qdev_simple_unplug_cb;
+    dc->init = event_qdev_init;
+    dc->exit = event_qdev_exit;
+}
+
+static TypeInfo s390_sclp_event_type_info = {
+    .name = TYPE_SCLP_EVENT,
+    .parent = TYPE_DEVICE,
+    .instance_size = sizeof(SCLPEvent),
+    .class_init = event_class_init,
+    .class_size = sizeof(SCLPEventClass),
+    .abstract = true,
+};
+
+static void register_types(void)
+{
+    type_register_static(&s390_sclp_events_bus_info);
+    type_register_static(&s390_sclp_event_facility_info);
+    type_register_static(&s390_sclp_event_type_info);
+}
+type_init(register_types)
diff --git a/hw/s390x/event-facility.h b/hw/s390x/event-facility.h
new file mode 100644
index 0000000..1e022a3
--- /dev/null
+++ b/hw/s390x/event-facility.h
@@ -0,0 +1,107 @@ 
+/*
+ * SCLP
+ *    Event Facility definitions
+ *
+ * Copyright IBM, Corp. 2012
+ *
+ * Authors:
+ *  Heinz Graalfs <graalfs@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.
+ *
+ */
+
+#ifndef HW_S390_SCLP_EVENT_FACILITY_H
+#define HW_S390_SCLP_EVENT_FACILITY_H
+
+#include <hw/qdev.h>
+#include "qemu-thread.h"
+
+/* SCLP event types */
+#define SCLP_EVENT_ASCII_CONSOLE_DATA           0x1a
+#define SCLP_EVENT_SIGNAL_QUIESCE               0x1d
+
+/* SCLP event masks */
+#define SCLP_EVENT_MASK_SIGNAL_QUIESCE          0x00000008
+#define SCLP_EVENT_MASK_MSG_ASCII               0x00000040
+
+#define SCLP_UNCONDITIONAL_READ                 0x00
+#define SCLP_SELECTIVE_READ                     0x01
+
+#define TYPE_SCLP_EVENT "s390-sclp-event-type"
+#define SCLP_EVENT(obj) \
+     OBJECT_CHECK(SCLPEvent, (obj), TYPE_SCLP_EVENT)
+#define SCLP_EVENT_CLASS(klass) \
+     OBJECT_CLASS_CHECK(SCLPEventClass, (klass), TYPE_SCLP_EVENT)
+#define SCLP_EVENT_GET_CLASS(obj) \
+     OBJECT_GET_CLASS(SCLPEventClass, (obj), TYPE_SCLP_EVENT)
+
+typedef struct WriteEventMask {
+    SCCBHeader h;
+    uint16_t _reserved;
+    uint16_t mask_length;
+    uint32_t cp_receive_mask;
+    uint32_t cp_send_mask;
+    uint32_t send_mask;
+    uint32_t receive_mask;
+} QEMU_PACKED WriteEventMask;
+
+typedef struct EventBufferHeader {
+    uint16_t length;
+    uint8_t  type;
+    uint8_t  flags;
+    uint16_t _reserved;
+} QEMU_PACKED EventBufferHeader;
+
+typedef struct WriteEventData {
+    SCCBHeader h;
+    EventBufferHeader ebh;
+} QEMU_PACKED WriteEventData;
+
+typedef struct ReadEventData {
+    SCCBHeader h;
+    EventBufferHeader ebh;
+    uint32_t mask;
+} QEMU_PACKED ReadEventData;
+
+typedef struct SCLPEvent {
+    DeviceState qdev;
+    QemuMutex lock;
+    bool event_pending;
+    uint32_t event_type;
+    char *name;
+} SCLPEvent;
+
+typedef struct SCLPEventClass {
+    DeviceClass parent_class;
+    int (*init)(SCLPEvent *event);
+    int (*exit)(SCLPEvent *event);
+
+    /* get SCLP's send mask */
+    unsigned int (*get_send_mask)(void);
+
+    /* get SCLP's receive mask */
+    unsigned int (*get_receive_mask)(void);
+
+    int (*read_event_data)(SCLPEvent *event, EventBufferHeader *evt_buf_hdr,
+                           int *slen);
+
+    int (*write_event_data)(SCLPEvent *event, EventBufferHeader *evt_buf_hdr);
+
+    /* returns the supported event type */
+    int (*event_type)(void);
+
+} SCLPEventClass;
+
+static inline void lock(SCLPEvent *event)
+{
+    qemu_mutex_lock(&event->lock);
+}
+
+static inline void unlock(SCLPEvent *event)
+{
+    qemu_mutex_unlock(&event->lock);
+}
+
+#endif
diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
index 4095ba6..cfc41ea 100644
--- a/hw/s390x/sclp.c
+++ b/hw/s390x/sclp.c
@@ -39,6 +39,7 @@  static int read_SCP_info(SCCB *sccb)
 static int sclp_execute(SCCB *sccb, uint64_t code)
 {
     int r = 0;
+    SCLPEventFacility *ef = sbus->event_facility->instance;
 
     switch (code) {
     case SCLP_CMDW_READ_SCP_INFO:
@@ -46,7 +47,7 @@  static int sclp_execute(SCCB *sccb, uint64_t code)
         r = read_SCP_info(sccb);
         break;
     default:
-        sccb->h.response_code = cpu_to_be16(SCLP_RC_INVALID_SCLP_COMMAND);
+        r = sbus->event_facility->sclp_command_handler(ef, sccb, code);
         break;
     }
     return r;
@@ -87,10 +88,13 @@  out:
 
 void sclp_service_interrupt(uint32_t sccb)
 {
-    if (!sccb) {
+    SCLPEventFacility *ef = sbus->event_facility->instance;
+    int event_pending = sbus->event_facility->event_pending(ef);
+
+    if (!event_pending && !sccb) {
         return;
     }
-    s390_sclp_extint(sccb & ~3);
+    s390_sclp_extint((sccb & ~3) + event_pending);
 }
 
 /* qemu object creation and initialization functions */
@@ -112,6 +116,9 @@  SCLPS390Bus *s390_sclp_bus_init(void)
     bus_state = qbus_create(TYPE_S390_SCLP_BUS, dev, NULL);
     bus_state->allow_hotplug = 0;
 
+    dev = qdev_create(bus_state, "s390-sclp-event-facility");
+    qdev_init_nofail(dev);
+
     sbus = DO_UPCAST(SCLPS390Bus, bus, bus_state);
     return sbus;
 }
@@ -137,9 +144,44 @@  static TypeInfo s390_sclp_bridge_info = {
     .class_init    = s390_sclp_bridge_class_init,
 };
 
+static int s390_sclp_busdev_init(DeviceState *dev)
+{
+    int r;
+    S390SCLPDevice *sdev = (S390SCLPDevice *)dev;
+    S390SCLPDeviceClass *sclp = SCLP_S390_DEVICE_GET_CLASS(dev);
+    SCLPS390Bus *bus = DO_UPCAST(SCLPS390Bus, bus, sdev->qdev.parent_bus);
+
+    r = sclp->init(sdev);
+    if (!r) {
+        assert(sdev->event_pending);
+        assert(sdev->sclp_command_handler);
+    }
+    bus->event_facility = sdev;
+
+    return r;
+}
+
+static void s390_sclp_device_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+
+    dc->init = s390_sclp_busdev_init;
+    dc->bus_type = TYPE_S390_SCLP_BUS;
+}
+
+static TypeInfo s390_sclp_device_info = {
+    .name = TYPE_DEVICE_S390_SCLP,
+    .parent = TYPE_DEVICE,
+    .instance_size = sizeof(S390SCLPDevice),
+    .class_init = s390_sclp_device_class_init,
+    .class_size = sizeof(S390SCLPDeviceClass),
+    .abstract = true,
+};
+
 static void s390_sclp_register_types(void)
 {
     type_register_static(&s390_sclp_bridge_info);
     type_register_static(&s390_sclp_bus_info);
+    type_register_static(&s390_sclp_device_info);
 }
 type_init(s390_sclp_register_types)
diff --git a/hw/s390x/sclp.h b/hw/s390x/sclp.h
index 4d5a946..d071ccd 100644
--- a/hw/s390x/sclp.h
+++ b/hw/s390x/sclp.h
@@ -19,15 +19,35 @@ 
 /* SCLP command codes */
 #define SCLP_CMDW_READ_SCP_INFO                 0x00020001
 #define SCLP_CMDW_READ_SCP_INFO_FORCED          0x00120001
+#define SCLP_CMD_READ_EVENT_DATA                0x00770005
+#define SCLP_CMD_WRITE_EVENT_DATA               0x00760005
+#define SCLP_CMD_READ_EVENT_DATA                0x00770005
+#define SCLP_CMD_WRITE_EVENT_DATA               0x00760005
+#define SCLP_CMD_WRITE_EVENT_MASK               0x00780005
 
 /* SCLP response codes */
 #define SCLP_RC_NORMAL_READ_COMPLETION          0x0010
+#define SCLP_RC_NORMAL_COMPLETION               0x0020
 #define SCLP_RC_INVALID_SCLP_COMMAND            0x01f0
+#define SCLP_RC_CONTAINED_EQUIPMENT_CHECK       0x0340
+#define SCLP_RC_INSUFFICIENT_SCCB_LENGTH        0x0300
+#define SCLP_RC_INVALID_FUNCTION                0x40f0
+#define SCLP_RC_NO_EVENT_BUFFERS_STORED         0x60f0
+#define SCLP_RC_INVALID_SELECTION_MASK          0x70f0
+#define SCLP_RC_INCONSISTENT_LENGTHS            0x72f0
+#define SCLP_RC_EVENT_BUFFER_SYNTAX_ERROR       0x73f0
+#define SCLP_RC_INVALID_MASK_LENGTH             0x74f0
+
 
 /* Service Call Control Block (SCCB) and its elements */
 
 #define SCCB_SIZE 4096
 
+#define SCLP_VARIABLE_LENGTH_RESPONSE           0x80
+#define SCLP_EVENT_BUFFER_ACCEPTED              0x80
+
+#define SCLP_FC_NORMAL_WRITE                    0
+
 /*
  * Normally packed structures are not the right thing to do, since all code
  * must take care of endianess. We cant use ldl_phys and friends for two
@@ -64,14 +84,38 @@  typedef struct SCCB {
 #define TYPE_S390_SCLP_BUS "s390-sclp-bus"
 #define S390_SCLP_BUS(obj) OBJECT_CHECK(SCLPS390Bus, (obj), TYPE_S390_SCLP_BUS)
 
+#define TYPE_DEVICE_S390_SCLP "s390-sclp-device"
+#define SCLP_S390_DEVIVE(obj) \
+     OBJECT_CHECK(S390SCLPDevice, (obj), TYPE_DEVICE_S390_SCLP)
+#define SCLP_S390_DEVICE_CLASS(klass) \
+     OBJECT_CLASS_CHECK(S390SCLPDeviceClass, (klass), \
+             TYPE_DEVICE_S390_SCLP)
+#define SCLP_S390_DEVICE_GET_CLASS(obj) \
+     OBJECT_GET_CLASS(S390SCLPDeviceClass, (obj), \
+             TYPE_DEVICE_S390_SCLP)
+
+typedef struct SCLPEventFacility SCLPEventFacility;
+
 typedef struct S390SCLPDevice {
     DeviceState qdev;
+    SCLPEventFacility *instance;
+    int (*sclp_command_handler)(SCLPEventFacility *ef, SCCB *sccb,
+                                uint64_t code);
+    bool (*event_pending)(SCLPEventFacility *ef);
 } S390SCLPDevice;
 
 typedef struct SCLPS390Bus {
     BusState bus;
+    S390SCLPDevice *event_facility;
 } SCLPS390Bus;
 
+typedef struct S390SCLPDeviceClass {
+    DeviceClass qdev;
+
+    int (*init)(S390SCLPDevice *sdev);
+
+} S390SCLPDeviceClass;
+
 SCLPS390Bus *s390_sclp_bus_init(void);
 
 void sclp_service_interrupt(uint32_t sccb);