Re: [Qemu-devel] [PATCH 4/7] s390: sclp event support

[Alexander Graf]

On 24.07.2012, at 09:37, Christian Borntraeger wrote:

> From: Heinz Graalfs <address@hidden>
> 
> 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 <address@hidden>
> Signed-off-by: Christian Borntraeger <address@hidden>
> ---
> 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 <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 "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 <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.
> + *
> + */
> +
> +#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
>