Re: [Qemu-devel] [PATCH 6/8] s390: sclp event facility and signal quiesce support via system_powerdown

[Alexander Graf <agraf@suse.de>]

On 06/06/2012 02:05 PM, Jens Freimann wrote:
> From: Heinz Graalfs<graalfs@linux.vnet.ibm.com>
>
> This patch implements a subset of the sclp event facility as well
> as the signal quiesce event. This allows to gracefully shutdown
> a guest by using system_powerdown. This sends a signal quiesce
> signal that is interpreted by linux guests as ctrl-alt-del.
> In addition the event facility is modeled using the QOM.
>
> Signed-off-by: Heinz Graalfs<graalfs@linux.vnet.ibm.com>
> Signed-off-by: Christian Borntraeger<borntraeger@de.ibm.com>
> Signed-off-by: Jens Freimann<jfrei@de.ibm.com>

Andreas, I'm always getting headaches reviewing qdev and/or qom patches. 
Could you please check this for layering violations?

> ---
>   Makefile.target          |    1 +
>   hw/s390-event-facility.c |  232 +++++++++++++++++++++++++++++++++++++++++
>   hw/s390-event-facility.h |   54 ++++++++++
>   hw/s390-sclp.c           |  256 +++++++++++++++++++++++++++++++++++++++++++++-
>   hw/s390-sclp.h           |   98 +++++++++++++++++-
>   hw/s390-virtio.c         |   11 +-
>   target-s390x/op_helper.c |    3 +
>   7 files changed, 649 insertions(+), 6 deletions(-)
>   create mode 100644 hw/s390-event-facility.c
>   create mode 100644 hw/s390-event-facility.h
>
> diff --git a/Makefile.target b/Makefile.target
> index fed2d72..f939c3a 100644
> --- a/Makefile.target
> +++ b/Makefile.target
> @@ -375,6 +375,7 @@ obj-m68k-y = an5206.o mcf5206.o mcf_uart.o mcf_intc.o mcf5208.o mcf_fec.o
>   obj-m68k-y += m68k-semi.o dummy_m68k.o
>
>   obj-s390x-y = s390-virtio-bus.o s390-virtio.o s390-sclp.o
> +obj-s390x-y += s390-event-facility.o
>
>   obj-alpha-y = mc146818rtc.o
>   obj-alpha-y += alpha_pci.o alpha_dp264.o alpha_typhoon.o
> diff --git a/hw/s390-event-facility.c b/hw/s390-event-facility.c
> new file mode 100644
> index 0000000..b8106a6
> --- /dev/null
> +++ b/hw/s390-event-facility.c
> @@ -0,0 +1,232 @@
> +/*
> + * SCLP Event Facility
> + *
> + * Copyright IBM Corp. 2007,2012
> + * Author: Heinz Graalfs<graalfs@de.ibm.com>
> + *
> + * This file is licensed under the terms of the GNU General Public License(GPL)
> + */
> +
> +#include "iov.h"
> +#include "monitor.h"
> +#include "qemu-queue.h"
> +#include "sysbus.h"
> +#include "sysemu.h"
> +
> +#include "s390-sclp.h"
> +#include "s390-event-facility.h"
> +
> +struct SCLPDevice {
> +    const char *name;
> +    bool vm_running;
> +};
> +
> +struct SCLP {
> +    BusState qbus;
> +    SCLPEventFacility *event_facility;
> +};
> +
> +struct SCLPEventFacility {
> +    SCLPDevice sdev;
> +    SCLP sbus;
> +    DeviceState *qdev;
> +    void *opaque;
> +};
> +
> +typedef struct SCLPConsole {
> +    SCLPEvent event;
> +    CharDriverState *chr;
> +} SCLPConsole;
> +
> +static void sclpef_bus_dev_print(Monitor *mon, DeviceState *qdev, int indent)
> +{
> +    SCLPEvent *event = DO_UPCAST(SCLPEvent, dev, qdev);
> +
> +    monitor_printf(mon, "%*sid %d\n", indent, "", event->id);
> +}
> +
> +static unsigned int send_mask_quiesce(void)
> +{
> +    return SCLP_EVENT_MASK_SIGNAL_QUIESCE;
> +}
> +
> +static unsigned int receive_mask_quiesce(void)
> +{
> +    return 0;
> +}
> +
> +static void s390_signal_quiesce(void *opaque, int n, int level)
> +{
> +    sclp_enable_signal_quiesce();
> +    sclp_service_interrupt(opaque, 0);
> +}
> +
> +unsigned int sclpef_send_mask(SCLPDevice *sdev)
> +{
> +    unsigned int mask;
> +    DeviceState *dev;
> +    SCLPEventFacility *event_facility;
> +    SCLPEventClass *cons;
> +
> +    event_facility = DO_UPCAST(SCLPEventFacility, sdev, sdev);
> +
> +    mask = 0;
> +
> +    QTAILQ_FOREACH(dev,&event_facility->sbus.qbus.children, sibling) {
> +        cons = SCLP_EVENT_GET_CLASS((SCLPEvent *) dev);
> +        mask |= cons->get_send_mask();
> +    }
> +    return mask;
> +}
> +
> +unsigned int sclpef_receive_mask(SCLPDevice *sdev)
> +{
> +    unsigned int mask;
> +    DeviceState *dev;
> +    SCLPEventClass *cons;
> +    SCLPEventFacility *event_facility;
> +
> +    event_facility = DO_UPCAST(SCLPEventFacility, sdev, sdev);
> +
> +    mask = 0;
> +
> +    QTAILQ_FOREACH(dev,&event_facility->sbus.qbus.children, sibling) {
> +        cons = SCLP_EVENT_GET_CLASS((SCLPEvent *) dev);
> +        mask |= cons->get_receive_mask();
> +    }
> +    return mask;
> +}
> +
> +static struct BusInfo sclp_bus_info = {
> +    .name      = "s390-sclp-bus",
> +    .size      = sizeof(SCLPS390Bus),
> +    .print_dev = sclpef_bus_dev_print,
> +    .props      = (Property[]) {
> +        DEFINE_PROP_STRING("name", SCLPEvent, name),
> +        DEFINE_PROP_END_OF_LIST()
> +    }
> +};
> +
> +static int sclpef_qdev_init(DeviceState *qdev)
> +{
> +    int rc;
> +    SCLPEvent *event = DO_UPCAST(SCLPEvent, dev, qdev);
> +    SCLPEventClass *cons = SCLP_EVENT_GET_CLASS(event);
> +    SCLP *bus = DO_UPCAST(SCLP, qbus, qdev->parent_bus);
> +
> +    event->evt_fac = bus->event_facility;
> +    rc = cons->init(event);
> +
> +    return rc;
> +}
> +
> +static int sclpef_qdev_exit(DeviceState *qdev)
> +{
> +    SCLPEvent *event = DO_UPCAST(SCLPEvent, dev, qdev);
> +    SCLPEventClass *cons = SCLP_EVENT_GET_CLASS(event);
> +    if (cons->exit) {
> +        cons->exit(event);
> +    }
> +    return 0;
> +}
> +
> +static SCLPDevice *sclpef_common_init(const char *name, size_t struct_size)
> +{
> +    SCLPDevice *sdev;
> +
> +    sdev = malloc(struct_size);

g_malloc please. I suppose even g_malloc0?

> +    if (!sdev) {
> +        return NULL;
> +    }
> +    sdev->vm_running = runstate_is_running();
> +    sdev->name = name;
> +
> +    return sdev;
> +}
> +
> +void sclpef_enable_irqs(SCLPDevice *sdev, void *opaque)
> +{
> +    SCLPEventFacility *event_facility;
> +
> +    event_facility = DO_UPCAST(SCLPEventFacility, sdev, sdev);
> +    qemu_system_powerdown = *qemu_allocate_irqs(s390_signal_quiesce,
> +                                                opaque, 1);
> +    event_facility->opaque = opaque;
> +}
> +
> +SCLPDevice *sclpef_init_event_facility(DeviceState *dev)
> +{
> +    DeviceState *quiesce;
> +    SCLPDevice *sdev;
> +    SCLPEventFacility *event_facility;
> +
> +    sdev = sclpef_common_init("sclp-event-facility",
> +            sizeof(SCLPEventFacility));
> +
> +    if (!sdev) {
> +        return NULL;
> +    }
> +    event_facility = DO_UPCAST(SCLPEventFacility, sdev, sdev);
> +
> +    /* Spawn a new sclp-facility */
> +    qbus_create_inplace(&event_facility->sbus.qbus,&sclp_bus_info, dev, NULL);
> +    event_facility->sbus.qbus.allow_hotplug = 0;
> +    event_facility->sbus.event_facility = event_facility;
> +    event_facility->qdev = dev;
> +    event_facility->opaque = NULL;
> +    quiesce = qdev_create(&event_facility->sbus.qbus, "sclpquiesce");
> +
> +    if (!quiesce) {
> +        return NULL;
> +    }
> +
> +    return sdev;
> +}
> +
> +static void sclpef_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +    dc->init = sclpef_qdev_init;
> +    dc->bus_info =&sclp_bus_info;
> +    dc->exit = sclpef_qdev_exit;
> +    dc->unplug = qdev_simple_unplug_cb;
> +}
> +
> +static TypeInfo sclp_event_facility_type_info = {
> +    .name = TYPE_SCLP_EVENT,
> +    .parent = TYPE_DEVICE,
> +    .instance_size = sizeof(SCLPEvent),
> +    .class_size = sizeof(SCLPEventClass),
> +    .class_init = sclpef_class_init,
> +    .abstract = true,
> +};
> +
> +static int sclpquiesce_initfn(SCLPEvent *event)
> +{
> +    event->id = ID_QUIESCE;
> +
> +    return 0;
> +}
> +
> +static void sclpef_quiesce_class_init(ObjectClass *klass, void *data)
> +{
> +    SCLPEventClass *k = SCLP_EVENT_CLASS(klass);
> +
> +    k->init = sclpquiesce_initfn;
> +    k->get_send_mask = send_mask_quiesce;
> +    k->get_receive_mask = receive_mask_quiesce;
> +}
> +
> +static TypeInfo sclp_quiesce_info = {
> +    .name          = "sclpquiesce",
> +    .parent        = TYPE_SCLP_EVENT,
> +    .instance_size = sizeof(SCLPEvent),
> +    .class_init    = sclpef_quiesce_class_init,
> +};
> +
> +static void sclpef_register_types(void)
> +{
> +    type_register_static(&sclp_event_facility_type_info);
> +    type_register_static(&sclp_quiesce_info);
> +}
> +type_init(sclpef_register_types)
> diff --git a/hw/s390-event-facility.h b/hw/s390-event-facility.h
> new file mode 100644
> index 0000000..40d4049
> --- /dev/null
> +++ b/hw/s390-event-facility.h
> @@ -0,0 +1,54 @@
> +/*
> + * SCLP definitions
> + *
> + * Copyright IBM Corp. 2007,2012
> + * Author: Heinz Graalfs<graalfs@de.ibm.com>
> + *
> + * This file is licensed under the terms of the GNU General Public License(GPL)
> + */
> +
> +#ifndef _QEMU_SCLP_EVENT_H
> +#define _QEMU_SCLP_EVENT_H
> +
> +#include "qdev.h"
> +#include "qemu-common.h"
> +
> +#define ID_QUIESCE SCLP_EVENT_SIGNAL_QUIESCE
> +
> +#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 SCLP SCLP;
> +typedef struct SCLPEventFacility SCLPEventFacility;
> +typedef struct SCLPEvent SCLPEvent;
> +typedef struct SCLPDevice SCLPDevice;
> +
> +typedef struct SCLPEventClass {
> +    DeviceClass parent_class;
> +    int (*init)(SCLPEvent *event);
> +    int (*exit)(SCLPEvent *event);
> +    unsigned int (*get_send_mask)(void);
> +    unsigned int (*get_receive_mask)(void);
> +} SCLPEventClass;
> +
> +struct SCLPEvent {
> +    DeviceState dev;
> +    SCLPEventFacility *evt_fac;
> +    char *name;
> +    uint32_t id;
> +};
> +
> +SCLPDevice *sclpef_init_event_facility(DeviceState *dev);
> +
> +void sclpef_enable_irqs(SCLPDevice *sdev, void *opaque);
> +
> +void sclpef_set_masks(void);
> +unsigned int sclpef_send_mask(SCLPDevice *sdev);
> +unsigned int sclpef_receive_mask(SCLPDevice *sdev);
> +
> +#endif
> diff --git a/hw/s390-sclp.c b/hw/s390-sclp.c
> index c046441..683a709 100644
> --- a/hw/s390-sclp.c
> +++ b/hw/s390-sclp.c
> @@ -7,7 +7,20 @@
>
>   #include "cpu.h"
>   #include "kvm.h"
> +#include "hw/sysbus.h"
>   #include "hw/s390-sclp.h"
> +#include "hw/s390-event-facility.h"
> +
> +/* Host capabilites */
> +static unsigned int sclp_send_mask;
> +static unsigned int sclp_receive_mask;
> +
> +/* Guest capabilities */
> +static unsigned int sclp_cp_send_mask;
> +static unsigned int sclp_cp_receive_mask;
> +
> +static int quiesce;
> +static int event_pending;

Global variables? Bad idea :). These should be properties of your 
quiesce device.

>
>   int sclp_read_info(CPUS390XState *env, struct sccb *sccb)
>   {
> @@ -23,20 +36,257 @@ int sclp_read_info(CPUS390XState *env, struct sccb *sccb)
>       return 0;
>   }
>
> +static int signal_quiesce_event(struct sccb *sccb, int *slen)
> +{
> +    if (!quiesce) {
> +        return 0;
> +    }
> +
> +    if (*slen<  sizeof(struct signal_quiesce)) {
> +        event_pending = 1;
> +        return 0;
> +    }
> +
> +    sccb->c.read.quiesce.h.length = cpu_to_be16(sizeof(struct signal_quiesce));
> +    sccb->c.read.quiesce.h.type = SCLP_EVENT_SIGNAL_QUIESCE;
> +    sccb->c.read.quiesce.h.flags&= ~SCLP_EVENT_BUFFER_ACCEPTED;
> +    /*
> +     * system_powerdown does not have a timeout. Fortunately the
> +     * timeout value is curently ignored by Linux, anyway
> +     */
> +    sccb->c.read.quiesce.timeout = cpu_to_be16(0);
> +    sccb->c.read.quiesce.unit = cpu_to_be16(0);
> +
> +    quiesce = 0;
> +    *slen -= sizeof(struct signal_quiesce);
> +    return 1;
> +}
> +
> +void sclp_enable_signal_quiesce(void)
> +{
> +    quiesce = 1;
> +    event_pending = 1;
> +}
> +
> +static void sclp_set_masks(void)
> +{
> +    SCLPS390EventFacility *evt_fac;
> +
> +    if (!sclp_bus) {
> +        return;
> +    }
> +    evt_fac = sclp_bus->event_facility;
> +
> +    assert(evt_fac);
> +
> +    sclp_send_mask = sclpef_send_mask(evt_fac->sdev);
> +    sclp_receive_mask = sclpef_receive_mask(evt_fac->sdev);
> + }
> +
> +int sclp_read_event_data(CPUS390XState *env, struct sccb *sccb)
> +{
> +    unsigned int sclp_active_selection_mask;
> +    int slen;
> +
> +    if (be16_to_cpu(sccb->h.length) != SCCB_SIZE) {
> +        sccb->h.response_code = cpu_to_be16(SCLP_RC_INSUFFICIENT_SCCB_LENGTH);
> +        goto out;
> +    }
> +
> +    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(sccb->c.read.mask))) {
> +            sccb->h.response_code = cpu_to_be16(SCLP_RC_INVALID_SELECTION_MASK);
> +            goto out;
> +        }
> +        sclp_active_selection_mask = be32_to_cpu(sccb->c.read.mask);
> +        break;
> +    default:
> +        sccb->h.response_code = cpu_to_be16(SCLP_RC_INVALID_FUNCTION);
> +        goto out;
> +    }
> +
> +    slen = sizeof(sccb->c.data);
> +    sccb->h.response_code = cpu_to_be16(SCLP_RC_NO_EVENT_BUFFERS_STORED);
> +
> +    if (sclp_active_selection_mask&  SCLP_EVENT_MASK_SIGNAL_QUIESCE) {
> +        if (signal_quiesce_event(sccb,&slen)) {
> +            sccb->h.response_code = cpu_to_be16(SCLP_RC_NORMAL_COMPLETION);
> +        }
> +    }
> +
> +    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);
> +    }
> +
> +out:
> +    return 0;
> +}
> +
> +int sclp_write_event_mask(CPUS390XState *env, struct sccb *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(sccb->c.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 */
> +    sclp_cp_send_mask = be32_to_cpu(sccb->c.we_mask.cp_send_mask);
> +    sclp_cp_receive_mask = be32_to_cpu(sccb->c.we_mask.cp_receive_mask);
> +
> +    sclp_set_masks();
> +
> +    /* return the SCLP's capability masks to the guest */
> +    sccb->c.we_mask.send_mask = cpu_to_be32(sclp_send_mask);
> +    sccb->c.we_mask.receive_mask = cpu_to_be32(sclp_receive_mask);
> +
> +    sccb->h.response_code = cpu_to_be32(SCLP_RC_NORMAL_COMPLETION);
> +
> +out:
> +    return 0;
> +}
> +
>   void sclp_service_interrupt(CPUS390XState *env, uint32_t sccb)
>   {
> -    if (!sccb) {
> +    if (!event_pending&&  !sccb) {
>           return;
>       }
>
>       if (kvm_enabled()) {
>   #ifdef CONFIG_KVM
>           kvm_s390_interrupt_internal(env, KVM_S390_INT_SERVICE,
> -                                    (sccb&  ~3), 0, 1);
> +                                    (sccb&  ~3) + event_pending, 0, 1);
>   #endif
>       } else {
>           env->psw.addr += 4;
> -        cpu_inject_ext(env, EXT_SERVICE, (sccb&  ~3), 0);
> +        cpu_inject_ext(env, EXT_SERVICE, (sccb&  ~3) + event_pending, 0);
> +    }

Please fix the above piece of code to use the same mechanism regarless 
of TCG or KVM, so that SCLP code doesn't have to be aware of KVM.

> +    event_pending = 0;
> +}
> +
> +struct BusInfo s390_sclp_bus_info = {
> +    .name       = "s390-sclp",
> +    .size       = sizeof(SCLPS390Bus),
> +};
> +
> +SCLPS390Bus *s390_sclp_bus_init(void)
> +{
> +    SCLPS390Bus *bus;
> +    BusState *bus_state;
> +    DeviceState *dev;
> +
> +    dev = qdev_create(NULL, "s390-sclp-bridge");
> +    qdev_init_nofail(dev);
> +    bus_state = qbus_create(&s390_sclp_bus_info, dev, "s390-sclp-bus");
> +    bus = DO_UPCAST(SCLPS390Bus, bus, bus_state);
> +
> +    bus_state->allow_hotplug = 0;
> +
> +    return bus;
> +}
> +
> +static int s390_sclp_device_init(SCLPS390EventFacility *dev, SCLPDevice *sdev)
> +{
> +    dev->sdev = sdev;
> +
> +    return 0;
> +}
> +
> +static int s390_sclp_init(SCLPS390EventFacility *dev)
> +{
> +    int rc;
> +    SCLPS390Bus *bus;
> +    SCLPDevice *sdev;
> +
> +    bus = DO_UPCAST(SCLPS390Bus, bus, dev->qdev.parent_bus);
> +
> +    sdev = sclpef_init_event_facility((DeviceState *)dev);
> +    if (!sdev) {
> +        return -1;
>       }
> +
> +    rc = s390_sclp_device_init(dev, sdev);
> +    if (!rc) {
> +        bus->event_facility = dev;
> +    }
> +
> +    return rc;
> +}
> +
> +static void s390_sclp_class_init(ObjectClass *klass, void *data)
> +{
> +    SCLPS390EventFacilityClass *k = SCLP_S390_EVENT_FACILITY_CLASS(klass);
> +
> +    k->init = s390_sclp_init;
> +}
> +
> +static TypeInfo s390_sclp_event_facility = {
> +    .name          = "sclp-event-facility",
> +    .parent        = TYPE_SCLP_S390_EVENT_FACILITY,
> +    .instance_size = sizeof(SCLPS390EventFacility),
> +    .class_init    = s390_sclp_class_init,
> +};
> +
> +static int s390_sclp_busdev_init(DeviceState *dev)
> +{
> +    SCLPS390EventFacility *_dev = (SCLPS390EventFacility *)dev;
> +    SCLPS390EventFacilityClass *evt_fac =
> +        SCLP_S390_EVENT_FACILITY_GET_CLASS(dev);
> +
> +    return evt_fac->init(_dev);
>   }
>
> +static void sclp_s390_device_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +
> +    dc->init = s390_sclp_busdev_init;
> +    dc->bus_info =&s390_sclp_bus_info;
> +}
> +
> +static TypeInfo sclp_s390_device_info = {
> +    .name = TYPE_SCLP_S390_EVENT_FACILITY,
> +    .parent = TYPE_DEVICE,
> +    .instance_size = sizeof(SCLPS390EventFacility),
> +    .class_init = sclp_s390_device_class_init,
> +    .class_size = sizeof(SCLPS390EventFacilityClass),
> +    .abstract = true,
> +};
> +
> +/***************** S390 SCLP Bus Bridge Device *******************/
> +/* Only required to have the sclp bus as child in the system bus */
> +
> +static int s390_sclp_bridge_init(SysBusDevice *dev)
> +{
> +    return 0;
> +}
> +
> +static void s390_sclp_bridge_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +    SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
> +
> +    k->init = s390_sclp_bridge_init;
> +    dc->no_user = 1;
> +}
> +
> +static TypeInfo s390_sclp_bridge_info = {
> +    .name          = "s390-sclp-bridge",
> +    .parent        = TYPE_SYS_BUS_DEVICE,
> +    .instance_size = sizeof(SysBusDevice),
> +    .class_init    = s390_sclp_bridge_class_init,
> +};
> +
> +static void s390_sclp_register_types(void)
> +{
> +    type_register_static(&sclp_s390_device_info);
> +    type_register_static(&s390_sclp_event_facility);
> +    type_register_static(&s390_sclp_bridge_info);
> +}
> +type_init(s390_sclp_register_types)
> diff --git a/hw/s390-sclp.h b/hw/s390-sclp.h
> index e335b21..f61421b 100644
> --- a/hw/s390-sclp.h
> +++ b/hw/s390-sclp.h
> @@ -1,13 +1,69 @@
>   #include<stdint.h>
>   #include<qemu-common.h>
>
> +#include "sysbus.h"
> +#include "hw/s390-event-facility.h"
>
>   /* SCLP command codes */
>   #define SCLP_CMDW_READ_SCP_INFO                 0x00020001
>   #define SCLP_CMDW_READ_SCP_INFO_FORCED          0x00120001
> +#define SCLP_CMD_WRITE_EVENT_MASK               0x00780005
>
>   /* SCLP response codes */
> -#define SCLP_RC_SCCB_RESOURCE_INSUFFICENT       0x07f0
> +#define SCLP_RC_NORMAL_COMPLETION               0x0020
> +#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
> +
> +/* SCLP event types */
> +#define SCLP_EVENT_SIGNAL_QUIESCE               0x1d
> +
> +/* SCLP event masks */
> +#define SCLP_EVENT_MASK_SIGNAL_QUIESCE          0x00000008
> +#define SCLP_EVENT_MASK_MSG                     0x40000000
> +
> +#define SCLP_UNCONDITIONAL_READ                 0x00
> +#define SCLP_SELECTIVE_READ                     0x01
> +
> +#define SCLP_VARIABLE_LENGTH_RESPONSE           0x80
> +
> +#define SCCB_SIZE 4096
> +
> +/* Service Call Control Block (SCCB) and its elements */
> +
> +struct write_event_mask {
> +    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;
> +} __attribute__((packed));
> +
> +struct event_buffer_header {
> +    uint16_t length;
> +    uint8_t  type;
> +#define SCLP_EVENT_BUFFER_ACCEPTED              0x80
> +    uint8_t  flags;
> +    uint16_t _reserved;
> +} __attribute__((packed));
> +
> +struct signal_quiesce {
> +    struct event_buffer_header h;
> +    uint16_t timeout;
> +    uint8_t unit;
> +} __attribute__((packed));
> +
> +struct read_event_data {
> +    union {
> +        struct signal_quiesce quiesce;
> +        uint32_t mask;
> +    };
> +} __attribute__((packed));
>
>   struct sccb_header {
>       uint16_t length;
> @@ -22,13 +78,51 @@ struct read_info_sccb {
>       uint8_t rnsize;
>   } __attribute__((packed));
>
> +#define SCCB_DATA_LEN 4088
> +
>   struct sccb {
>       struct sccb_header h;
>       union {
>           struct read_info_sccb read_info;
> -        char data[4088];
> +        struct read_event_data read;
> +        struct write_event_mask we_mask;
> +        char data[SCCB_DATA_LEN];
>       } c;
>    } __attribute__((packed));
>
> +void sclp_enable_signal_quiesce(void);
>   int sclp_read_info(CPUS390XState *env, struct sccb *sccb);
> +int sclp_read_event_data(CPUS390XState *env, struct sccb *sccb);
> +int sclp_write_event_mask(CPUS390XState *env, struct sccb *sccb);
>   void sclp_service_interrupt(CPUS390XState *env, uint32_t sccb);
> +
> +#define TYPE_SCLP_S390_EVENT_FACILITY "s390-sclp-event-facility"
> +#define SCLP_S390_EVENT_FACILITY(obj) \
> +     OBJECT_CHECK(SCLPS390EventFacility, (obj), TYPE_SCLP_S390_EVENT_FACILITY)
> +#define SCLP_S390_EVENT_FACILITY_CLASS(klass) \
> +     OBJECT_CLASS_CHECK(SCLPS390EventFacilityClass, (klass), \
> +             TYPE_SCLP_S390_EVENT_FACILITY)
> +#define SCLP_S390_EVENT_FACILITY_GET_CLASS(obj) \
> +     OBJECT_GET_CLASS(SCLPS390EventFacilityClass, (obj), \
> +             TYPE_SCLP_S390_EVENT_FACILITY)
> +
> +typedef struct SCLPS390EventFacility SCLPS390EventFacility;
> +
> +typedef struct SCLPS390EventFacilityClass {
> +    DeviceClass qdev;
> +    int (*init)(SCLPS390EventFacility *dev);
> +} SCLPS390EventFacilityClass;
> +
> +struct SCLPS390EventFacility {
> +    DeviceState qdev;
> +    SCLPDevice *sdev;
> +};
> +
> +typedef struct SCLPS390Bus {
> +    BusState bus;
> +    SCLPS390EventFacility *event_facility;
> +} SCLPS390Bus;
> +
> +extern SCLPS390Bus *sclp_bus;
> +
> +SCLPS390Bus *s390_sclp_bus_init(void);
> diff --git a/hw/s390-virtio.c b/hw/s390-virtio.c
> index c0e19fd..0babf27 100644
> --- a/hw/s390-virtio.c
> +++ b/hw/s390-virtio.c
> @@ -32,6 +32,7 @@
>   #include "exec-memory.h"
>
>   #include "hw/s390-virtio-bus.h"
> +#include "hw/s390-sclp.h"
>
>   //#define DEBUG_S390
>
> @@ -60,7 +61,9 @@
>
>   #define MAX_BLK_DEVS                    10
>
> -static VirtIOS390Bus *s390_bus;
> +VirtIOS390Bus *s390_bus;
> +SCLPS390Bus *sclp_bus;
> +
>   static CPUS390XState **ipi_states;
>
>   CPUS390XState *s390_cpu_addr2state(uint16_t cpu_addr)
> @@ -170,6 +173,7 @@ static void s390_init(ram_addr_t my_ram_size,
>       target_phys_addr_t virtio_region_len;
>       target_phys_addr_t virtio_region_start;
>       int i;
> +    DeviceState *dev;
>
>       /* s390x ram size detection needs a 16bit multiplier + an increment. So
>          guests>  64GB can be specified in 2MB steps etc. */
> @@ -183,6 +187,7 @@ static void s390_init(ram_addr_t my_ram_size,
>
>       /* get a BUS */
>       s390_bus = s390_virtio_bus_init(&my_ram_size);
> +    sclp_bus = s390_sclp_bus_init();
>
>       /* allocate RAM */
>       memory_region_init_ram(ram, "s390.ram", my_ram_size);
> @@ -325,6 +330,10 @@ static void s390_init(ram_addr_t my_ram_size,
>           qdev_prop_set_drive_nofail(dev, "drive", dinfo->bdrv);
>           qdev_init_nofail(dev);
>       }
> +
> +    dev = qdev_create((BusState *)sclp_bus, "sclp-event-facility");
> +    qdev_init_nofail(dev);
> +    sclpef_enable_irqs(sclp_bus->event_facility->sdev, ipi_states[0]);
>   }
>
>   static QEMUMachine s390_machine = {
> diff --git a/target-s390x/op_helper.c b/target-s390x/op_helper.c
> index 74bd9ad..3e5eff4 100644
> --- a/target-s390x/op_helper.c
> +++ b/target-s390x/op_helper.c
> @@ -2391,6 +2391,9 @@ int sclp_service_call(CPUS390XState *env, uint32_t sccb, uint64_t code)
>           case SCLP_CMDW_READ_SCP_INFO_FORCED:
>               r = sclp_read_info(env,&work_sccb);
>               break;
> +        case SCLP_CMD_WRITE_EVENT_MASK:
> +            r = sclp_write_event_mask(env,&work_sccb);
> +            break;
>           default:

I don't understand most of the patch tbh. It does a lot of things at the 
same time, but none of them really thorough. Please split this off into 
separate patches and make them actually do something small, but 
constistently.

Things I saw while looking over this:

   - you create a bus to plug in sclp users. This needs to be separate. 
Don't introduce users of a framework when introducing the framework 
itself, unless really neccessary (or if the patch only becomes 5 lines 
longer)
   - you create objects for the new event parser, but not all the other 
sclp users. There's also no generic distribution mechanism in place
   - the event parser itself uses globals - that's nowhere near object 
oriented :)
   - I don't see the header inclusions i commented on in the last patch 
remedied here, please think your model over. In general, moving sclp to 
hw/ is probably a good idea, but then please do it in a properly 
abstracted way.
   - A lot of the above code could use some comments, so people reading 
it would actually understand what's going on :)

Alex