[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 4/7] s390: sclp event support
|
From: |
Heinz Graalfs |
|
Subject: |
Re: [Qemu-devel] [PATCH 4/7] s390: sclp event support |
|
Date: |
Mon, 20 Aug 2012 14:15:18 +0200 |
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 <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?
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 <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).
>
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
> >
>