Patchwork [3/7] s390: sclp base support

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

Comments

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

This adds a more generic infrastructure for handling Service-Call
requests on s390. Currently we only support a small subset of Read
SCP Info directly in target-s390x. This patch provides the base
infrastructure for supporting more commands and moves Read SCP
Info.
In the future we could add additional commands for hotplug, call
home and event handling.

Signed-off-by: Heinz Graalfs <graalfs@linux.vnet.ibm.com>
Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
---
 hw/s390-virtio.c         |    2 +
 hw/s390x/Makefile.objs   |    1 +
 hw/s390x/sclp.c          |  145 ++++++++++++++++++++++++++++++++++++++++++++++
 hw/s390x/sclp.h          |   79 +++++++++++++++++++++++++
 target-s390x/cpu.h       |   14 +----
 target-s390x/kvm.c       |    5 +-
 target-s390x/op_helper.c |   31 ++--------
 7 files changed, 235 insertions(+), 42 deletions(-)
 create mode 100644 hw/s390x/sclp.c
 create mode 100644 hw/s390x/sclp.h
Alexander Graf - July 30, 2012, 12:38 p.m.
On 24.07.2012, at 09:37, Christian Borntraeger wrote:

> From: Heinz Graalfs <graalfs@linux.vnet.ibm.com>
> 
> This adds a more generic infrastructure for handling Service-Call
> requests on s390. Currently we only support a small subset of Read
> SCP Info directly in target-s390x. This patch provides the base
> infrastructure for supporting more commands and moves Read SCP
> Info.
> In the future we could add additional commands for hotplug, call
> home and event handling.
> 
> Signed-off-by: Heinz Graalfs <graalfs@linux.vnet.ibm.com>
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> ---
> hw/s390-virtio.c         |    2 +
> hw/s390x/Makefile.objs   |    1 +
> hw/s390x/sclp.c          |  145 ++++++++++++++++++++++++++++++++++++++++++++++
> hw/s390x/sclp.h          |   79 +++++++++++++++++++++++++
> target-s390x/cpu.h       |   14 +----
> target-s390x/kvm.c       |    5 +-
> target-s390x/op_helper.c |   31 ++--------
> 7 files changed, 235 insertions(+), 42 deletions(-)
> create mode 100644 hw/s390x/sclp.c
> create mode 100644 hw/s390x/sclp.h
> 
> diff --git a/hw/s390-virtio.c b/hw/s390-virtio.c
> index 47eed35..28e320d 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/s390x/sclp.h"
> 
> //#define DEBUG_S390
> 
> @@ -183,6 +184,7 @@ static void s390_init(ram_addr_t my_ram_size,
> 
>     /* get a BUS */
>     s390_bus = s390_virtio_bus_init(&my_ram_size);
> +    s390_sclp_bus_init();
> 
>     /* allocate RAM */
>     memory_region_init_ram(ram, "s390.ram", my_ram_size);
> diff --git a/hw/s390x/Makefile.objs b/hw/s390x/Makefile.objs
> index dcdcac8..1c14b96 100644
> --- a/hw/s390x/Makefile.objs
> +++ b/hw/s390x/Makefile.objs
> @@ -1,3 +1,4 @@
> obj-y = s390-virtio-bus.o s390-virtio.o
> 
> obj-y := $(addprefix ../,$(obj-y))
> +obj-y += sclp.o
> diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
> new file mode 100644
> index 0000000..4095ba6
> --- /dev/null
> +++ b/hw/s390x/sclp.c
> @@ -0,0 +1,145 @@
> +/*
> + * SCLP Support
> + *
> + * Copyright IBM, Corp. 2012
> + *
> + * Authors:
> + *  Christian Borntraeger <borntraeger@de.ibm.com>
> + *  Heinz Graalfs <graalfs@linux.vnet.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 "cpu.h"
> +#include "kvm.h"
> +#include <hw/sysbus.h>
> +#include "sclp.h"
> +
> +/* There is one SCLP bus per machine */
> +static SCLPS390Bus *sbus;

... but there isn't necessarily one machine per qemu instance. Today there is, but we shouldn't rely on that fact. Please move the bus variable into a machine struct that only the machine knows about.

> +
> +/* Provide information about the configuration, CPUs and storage */
> +static int read_SCP_info(SCCB *sccb)
> +{
> +    ReadInfo *read_info = (ReadInfo *) sccb;
> +    int shift = 0;
> +
> +    while ((ram_size >> (20 + shift)) > 65535) {
> +        shift++;
> +    }
> +    read_info->rnmax = cpu_to_be16(ram_size >> (20 + shift));
> +    read_info->rnsize = 1 << shift;
> +    sccb->h.response_code = cpu_to_be16(SCLP_RC_NORMAL_READ_COMPLETION);
> +
> +    return 0;
> +}
> +
> +static int sclp_execute(SCCB *sccb, uint64_t code)
> +{
> +    int r = 0;
> +
> +    switch (code) {
> +    case SCLP_CMDW_READ_SCP_INFO:
> +    case SCLP_CMDW_READ_SCP_INFO_FORCED:
> +        r = read_SCP_info(sccb);
> +        break;
> +    default:
> +        sccb->h.response_code = cpu_to_be16(SCLP_RC_INVALID_SCLP_COMMAND);
> +        break;
> +    }
> +    return r;
> +}
> +
> +int do_sclp_service_call(uint32_t sccb, uint64_t code)
> +{
> +    int r = 0;
> +    SCCB work_sccb;
> +
> +    target_phys_addr_t sccb_len = sizeof(SCCB);
> +
> +    /*
> +     * we want to work on a private copy of the sccb, to prevent guests
> +     * from playing dirty tricks by modifying the memory content after
> +     * the host has checked the values
> +     */
> +    cpu_physical_memory_read(sccb, &work_sccb, sccb_len);
> +
> +    /* Valid sccb sizes */
> +    if (be16_to_cpu(work_sccb.h.length) < 8 ||
> +        be16_to_cpu(work_sccb.h.length) > 4096) {
> +        r = -PGM_SPECIFICATION;
> +        goto out;
> +    }
> +
> +    r = sclp_execute((SCCB *)&work_sccb, code);
> +
> +    cpu_physical_memory_write(sccb, &work_sccb,
> +                              be16_to_cpu(work_sccb.h.length));
> +    if (!r) {
> +        sclp_service_interrupt(sccb);
> +    }
> +
> +out:
> +    return r;
> +}
> +
> +void sclp_service_interrupt(uint32_t sccb)

Does this have to be globally visible? If all the sclp source ends up in this file, it should only be necessary internal to it, right?

> +{
> +    if (!sccb) {

Please add a comment when this would trigger. In fact, I'm not sure I fully understand the reason for this function :).


Alex
Christian Borntraeger - July 30, 2012, 2:34 p.m.
On 30/07/12 14:38, Alexander Graf wrote:

>> +/* There is one SCLP bus per machine */
>> +static SCLPS390Bus *sbus;
> 
> ... but there isn't necessarily one machine per qemu instance. Today there is, 
> but we shouldn't rely on that fact. Please move the bus variable into a machine
> struct that only the machine knows about.

Could do. However, we have the same issue with the virtio bus. Would it make sense to leave it
here as is and the do a cleanup later on?

[...]
>> +void sclp_service_interrupt(uint32_t sccb)
> 
> Does this have to be globally visible? If all the sclp source ends up in this file, it should only be necessary internal to it, right?

It will be called by the console and quiesce code

> 
>> +{
>> +    if (!sccb) {
> 
> Please add a comment when this would trigger. In fact, I'm not sure I fully understand the reason for this function :).

It will be enhanced by patch 4. Will move that hunk completely there. See also the explanation for patch 4.
Andreas Färber - Aug. 3, 2012, 3:23 p.m.
Am 24.07.2012 09:37, schrieb Christian Borntraeger:
> diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
> new file mode 100644
> index 0000000..4095ba6
> --- /dev/null
> +++ b/hw/s390x/sclp.c
[...]
> +
> +static TypeInfo s390_sclp_bridge_info = {

Two minor comments:

static const please.

> +    .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(&s390_sclp_bridge_info);
> +    type_register_static(&s390_sclp_bus_info);
> +}
> +type_init(s390_sclp_register_types)

Please insert a white line between the function and type_init().
Both apply to virtually all following patches as well.

Andreas
Heinz Graalfs - Aug. 20, 2012, 12:15 p.m.
Hi Alex,

there was one leftover ...

On Mon, 2012-07-30 at 14:38 +0200, Alexander Graf wrote: 
> On 24.07.2012, at 09:37, Christian Borntraeger wrote:
> 
> > From: Heinz Graalfs <graalfs@linux.vnet.ibm.com>
> > 
> > This adds a more generic infrastructure for handling Service-Call
> > requests on s390. Currently we only support a small subset of Read
> > SCP Info directly in target-s390x. This patch provides the base
> > infrastructure for supporting more commands and moves Read SCP
> > Info.
> > In the future we could add additional commands for hotplug, call
> > home and event handling.
> > 
> > Signed-off-by: Heinz Graalfs <graalfs@linux.vnet.ibm.com>
> > Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> > ---
> > hw/s390-virtio.c         |    2 +
> > hw/s390x/Makefile.objs   |    1 +
> > hw/s390x/sclp.c          |  145 ++++++++++++++++++++++++++++++++++++++++++++++
> > hw/s390x/sclp.h          |   79 +++++++++++++++++++++++++
> > target-s390x/cpu.h       |   14 +----
> > target-s390x/kvm.c       |    5 +-
> > target-s390x/op_helper.c |   31 ++--------
> > 7 files changed, 235 insertions(+), 42 deletions(-)
> > create mode 100644 hw/s390x/sclp.c
> > create mode 100644 hw/s390x/sclp.h
> > 
> > diff --git a/hw/s390-virtio.c b/hw/s390-virtio.c
> > index 47eed35..28e320d 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/s390x/sclp.h"
> > 
> > //#define DEBUG_S390
> > 
> > @@ -183,6 +184,7 @@ static void s390_init(ram_addr_t my_ram_size,
> > 
> >     /* get a BUS */
> >     s390_bus = s390_virtio_bus_init(&my_ram_size);
> > +    s390_sclp_bus_init();
> > 
> >     /* allocate RAM */
> >     memory_region_init_ram(ram, "s390.ram", my_ram_size);
> > diff --git a/hw/s390x/Makefile.objs b/hw/s390x/Makefile.objs
> > index dcdcac8..1c14b96 100644
> > --- a/hw/s390x/Makefile.objs
> > +++ b/hw/s390x/Makefile.objs
> > @@ -1,3 +1,4 @@
> > obj-y = s390-virtio-bus.o s390-virtio.o
> > 
> > obj-y := $(addprefix ../,$(obj-y))
> > +obj-y += sclp.o
> > diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
> > new file mode 100644
> > index 0000000..4095ba6
> > --- /dev/null
> > +++ b/hw/s390x/sclp.c
> > @@ -0,0 +1,145 @@
> > +/*
> > + * SCLP Support
> > + *
> > + * Copyright IBM, Corp. 2012
> > + *
> > + * Authors:
> > + *  Christian Borntraeger <borntraeger@de.ibm.com>
> > + *  Heinz Graalfs <graalfs@linux.vnet.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 "cpu.h"
> > +#include "kvm.h"
> > +#include <hw/sysbus.h>
> > +#include "sclp.h"
> > +
> > +/* There is one SCLP bus per machine */
> > +static SCLPS390Bus *sbus;
> 
> ... but there isn't necessarily one machine per qemu instance. Today there is, but we shouldn't rely on that fact. Please move the bus variable into a machine struct that only the machine knows about.
> 

we removed the complete sclp bus now and keep the event_facility pointer
within the current machines global property-list as opaque pointer

> > +
> > +/* Provide information about the configuration, CPUs and storage */
> > +static int read_SCP_info(SCCB *sccb)
> > +{
> > +    ReadInfo *read_info = (ReadInfo *) sccb;
> > +    int shift = 0;
> > +
> > +    while ((ram_size >> (20 + shift)) > 65535) {
> > +        shift++;
> > +    }
> > +    read_info->rnmax = cpu_to_be16(ram_size >> (20 + shift));
> > +    read_info->rnsize = 1 << shift;
> > +    sccb->h.response_code = cpu_to_be16(SCLP_RC_NORMAL_READ_COMPLETION);
> > +
> > +    return 0;
> > +}
> > +
> > +static int sclp_execute(SCCB *sccb, uint64_t code)
> > +{
> > +    int r = 0;
> > +
> > +    switch (code) {
> > +    case SCLP_CMDW_READ_SCP_INFO:
> > +    case SCLP_CMDW_READ_SCP_INFO_FORCED:
> > +        r = read_SCP_info(sccb);
> > +        break;
> > +    default:
> > +        sccb->h.response_code = cpu_to_be16(SCLP_RC_INVALID_SCLP_COMMAND);
> > +        break;
> > +    }
> > +    return r;
> > +}
> > +
> > +int do_sclp_service_call(uint32_t sccb, uint64_t code)
> > +{
> > +    int r = 0;
> > +    SCCB work_sccb;
> > +
> > +    target_phys_addr_t sccb_len = sizeof(SCCB);
> > +
> > +    /*
> > +     * we want to work on a private copy of the sccb, to prevent guests
> > +     * from playing dirty tricks by modifying the memory content after
> > +     * the host has checked the values
> > +     */
> > +    cpu_physical_memory_read(sccb, &work_sccb, sccb_len);
> > +
> > +    /* Valid sccb sizes */
> > +    if (be16_to_cpu(work_sccb.h.length) < 8 ||
> > +        be16_to_cpu(work_sccb.h.length) > 4096) {
> > +        r = -PGM_SPECIFICATION;
> > +        goto out;
> > +    }
> > +
> > +    r = sclp_execute((SCCB *)&work_sccb, code);
> > +
> > +    cpu_physical_memory_write(sccb, &work_sccb,
> > +                              be16_to_cpu(work_sccb.h.length));
> > +    if (!r) {
> > +        sclp_service_interrupt(sccb);
> > +    }
> > +
> > +out:
> > +    return r;
> > +}
> > +
> > +void sclp_service_interrupt(uint32_t sccb)
> 
> Does this have to be globally visible? If all the sclp source ends up in this file, it should only be necessary internal to it, right?
> 
> > +{
> > +    if (!sccb) {
> 
> Please add a comment when this would trigger. In fact, I'm not sure I fully understand the reason for this function :).
> 
> 
> Alex
>

Patch

diff --git a/hw/s390-virtio.c b/hw/s390-virtio.c
index 47eed35..28e320d 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/s390x/sclp.h"
 
 //#define DEBUG_S390
 
@@ -183,6 +184,7 @@  static void s390_init(ram_addr_t my_ram_size,
 
     /* get a BUS */
     s390_bus = s390_virtio_bus_init(&my_ram_size);
+    s390_sclp_bus_init();
 
     /* allocate RAM */
     memory_region_init_ram(ram, "s390.ram", my_ram_size);
diff --git a/hw/s390x/Makefile.objs b/hw/s390x/Makefile.objs
index dcdcac8..1c14b96 100644
--- a/hw/s390x/Makefile.objs
+++ b/hw/s390x/Makefile.objs
@@ -1,3 +1,4 @@ 
 obj-y = s390-virtio-bus.o s390-virtio.o
 
 obj-y := $(addprefix ../,$(obj-y))
+obj-y += sclp.o
diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
new file mode 100644
index 0000000..4095ba6
--- /dev/null
+++ b/hw/s390x/sclp.c
@@ -0,0 +1,145 @@ 
+/*
+ * SCLP Support
+ *
+ * Copyright IBM, Corp. 2012
+ *
+ * Authors:
+ *  Christian Borntraeger <borntraeger@de.ibm.com>
+ *  Heinz Graalfs <graalfs@linux.vnet.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 "cpu.h"
+#include "kvm.h"
+#include <hw/sysbus.h>
+#include "sclp.h"
+
+/* There is one SCLP bus per machine */
+static SCLPS390Bus *sbus;
+
+/* Provide information about the configuration, CPUs and storage */
+static int read_SCP_info(SCCB *sccb)
+{
+    ReadInfo *read_info = (ReadInfo *) sccb;
+    int shift = 0;
+
+    while ((ram_size >> (20 + shift)) > 65535) {
+        shift++;
+    }
+    read_info->rnmax = cpu_to_be16(ram_size >> (20 + shift));
+    read_info->rnsize = 1 << shift;
+    sccb->h.response_code = cpu_to_be16(SCLP_RC_NORMAL_READ_COMPLETION);
+
+    return 0;
+}
+
+static int sclp_execute(SCCB *sccb, uint64_t code)
+{
+    int r = 0;
+
+    switch (code) {
+    case SCLP_CMDW_READ_SCP_INFO:
+    case SCLP_CMDW_READ_SCP_INFO_FORCED:
+        r = read_SCP_info(sccb);
+        break;
+    default:
+        sccb->h.response_code = cpu_to_be16(SCLP_RC_INVALID_SCLP_COMMAND);
+        break;
+    }
+    return r;
+}
+
+int do_sclp_service_call(uint32_t sccb, uint64_t code)
+{
+    int r = 0;
+    SCCB work_sccb;
+
+    target_phys_addr_t sccb_len = sizeof(SCCB);
+
+    /*
+     * we want to work on a private copy of the sccb, to prevent guests
+     * from playing dirty tricks by modifying the memory content after
+     * the host has checked the values
+     */
+    cpu_physical_memory_read(sccb, &work_sccb, sccb_len);
+
+    /* Valid sccb sizes */
+    if (be16_to_cpu(work_sccb.h.length) < 8 ||
+        be16_to_cpu(work_sccb.h.length) > 4096) {
+        r = -PGM_SPECIFICATION;
+        goto out;
+    }
+
+    r = sclp_execute((SCCB *)&work_sccb, code);
+
+    cpu_physical_memory_write(sccb, &work_sccb,
+                              be16_to_cpu(work_sccb.h.length));
+    if (!r) {
+        sclp_service_interrupt(sccb);
+    }
+
+out:
+    return r;
+}
+
+void sclp_service_interrupt(uint32_t sccb)
+{
+    if (!sccb) {
+        return;
+    }
+    s390_sclp_extint(sccb & ~3);
+}
+
+/* qemu object creation and initialization functions */
+
+static const TypeInfo s390_sclp_bus_info = {
+    .name = TYPE_S390_SCLP_BUS,
+    .parent = TYPE_BUS,
+    .instance_size = sizeof(SCLPS390Bus),
+ };
+
+SCLPS390Bus *s390_sclp_bus_init(void)
+{
+    BusState *bus_state;
+    DeviceState *dev;
+
+    dev = qdev_create(NULL, "s390-sclp-bridge");
+    qdev_init_nofail(dev);
+
+    bus_state = qbus_create(TYPE_S390_SCLP_BUS, dev, NULL);
+    bus_state->allow_hotplug = 0;
+
+    sbus = DO_UPCAST(SCLPS390Bus, bus, bus_state);
+    return sbus;
+}
+
+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(&s390_sclp_bridge_info);
+    type_register_static(&s390_sclp_bus_info);
+}
+type_init(s390_sclp_register_types)
diff --git a/hw/s390x/sclp.h b/hw/s390x/sclp.h
new file mode 100644
index 0000000..4d5a946
--- /dev/null
+++ b/hw/s390x/sclp.h
@@ -0,0 +1,79 @@ 
+/*
+ * SCLP Support
+ *
+ * Copyright IBM, Corp. 2012
+ *
+ * Authors:
+ *  Christian Borntraeger <borntraeger@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_H
+#define HW_S390_SCLP_H
+
+#include <hw/qdev.h>
+
+/* SCLP command codes */
+#define SCLP_CMDW_READ_SCP_INFO                 0x00020001
+#define SCLP_CMDW_READ_SCP_INFO_FORCED          0x00120001
+
+/* SCLP response codes */
+#define SCLP_RC_NORMAL_READ_COMPLETION          0x0010
+#define SCLP_RC_INVALID_SCLP_COMMAND            0x01f0
+
+/* Service Call Control Block (SCCB) and its elements */
+
+#define SCCB_SIZE 4096
+
+/*
+ * 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
+ * reasons, though:
+ * - some of the embedded structures below the SCCB can appear multiple times
+ *   at different locations, so there is no fixed offset
+ * - we work on a private copy of the SCCB, since there are several length
+ *   fields, that would cause a security nightmare if we allow the guest to
+ *   alter the structure while we parse it. We cannot use ldl_p and friends
+ *   either without doing pointer arithmetics
+ * So we have to double check that all users of sclp data structures use the
+ * right endianess wrappers.
+ */
+typedef struct SCCBHeader {
+    uint16_t length;
+    uint8_t function_code;
+    uint8_t control_mask[3];
+    uint16_t response_code;
+} QEMU_PACKED SCCBHeader;
+
+#define SCCB_DATA_LEN (SCCB_SIZE - sizeof(SCCBHeader))
+
+typedef struct ReadInfo {
+    SCCBHeader h;
+    uint16_t rnmax;
+    uint8_t rnsize;
+} QEMU_PACKED ReadInfo;
+
+typedef struct SCCB {
+    SCCBHeader h;
+    char data[SCCB_DATA_LEN];
+ } QEMU_PACKED SCCB;
+
+#define TYPE_S390_SCLP_BUS "s390-sclp-bus"
+#define S390_SCLP_BUS(obj) OBJECT_CHECK(SCLPS390Bus, (obj), TYPE_S390_SCLP_BUS)
+
+typedef struct S390SCLPDevice {
+    DeviceState qdev;
+} S390SCLPDevice;
+
+typedef struct SCLPS390Bus {
+    BusState bus;
+} SCLPS390Bus;
+
+SCLPS390Bus *s390_sclp_bus_init(void);
+
+void sclp_service_interrupt(uint32_t sccb);
+
+#endif
diff --git a/target-s390x/cpu.h b/target-s390x/cpu.h
index 18ac6e3..efd2cda 100644
--- a/target-s390x/cpu.h
+++ b/target-s390x/cpu.h
@@ -294,6 +294,7 @@  void s390x_tod_timer(void *opaque);
 void s390x_cpu_timer(void *opaque);
 
 int s390_virtio_hypercall(CPUS390XState *env, uint64_t mem, uint64_t hypercall);
+int do_sclp_service_call(uint32_t sccb, uint64_t code);
 
 #ifdef CONFIG_KVM
 void kvm_s390_interrupt(CPUS390XState *env, int type, uint32_t code);
@@ -596,17 +597,6 @@  static inline const char *cc_name(int cc_op)
     return cc_names[cc_op];
 }
 
-/* SCLP PV interface defines */
-#define SCLP_CMDW_READ_SCP_INFO         0x00020001
-#define SCLP_CMDW_READ_SCP_INFO_FORCED  0x00120001
-
-#define SCP_LENGTH                      0x00
-#define SCP_FUNCTION_CODE               0x02
-#define SCP_CONTROL_MASK                0x03
-#define SCP_RESPONSE_CODE               0x06
-#define SCP_MEM_CODE                    0x08
-#define SCP_INCREMENT                   0x0a
-
 typedef struct LowCore
 {
     /* prefix area: defined by architecture */
@@ -955,7 +945,7 @@  static inline void ebcdic_put(uint8_t *p, const char *ascii, int len)
 void load_psw(CPUS390XState *env, uint64_t mask, uint64_t addr);
 int mmu_translate(CPUS390XState *env, target_ulong vaddr, int rw, uint64_t asc,
                   target_ulong *raddr, int *flags);
-int sclp_service_call(CPUS390XState *env, uint32_t sccb, uint64_t code);
+int sclp_service_call(uint32_t sccb, uint64_t code);
 uint32_t calc_cc(CPUS390XState *env, uint32_t cc_op, uint64_t src, uint64_t dst,
                  uint64_t vr);
 
diff --git a/target-s390x/kvm.c b/target-s390x/kvm.c
index 07edf93..bcb6b2e 100644
--- a/target-s390x/kvm.c
+++ b/target-s390x/kvm.c
@@ -60,9 +60,6 @@ 
 #define SIGP_STORE_STATUS_ADDR          0x0e
 #define SIGP_SET_ARCH                   0x12
 
-#define SCLP_CMDW_READ_SCP_INFO         0x00020001
-#define SCLP_CMDW_READ_SCP_INFO_FORCED  0x00120001
-
 const KVMCapabilityInfo kvm_arch_required_capabilities[] = {
     KVM_CAP_LAST_INFO
 };
@@ -272,7 +269,7 @@  static int kvm_sclp_service_call(CPUS390XState *env, struct kvm_run *run,
     sccb = env->regs[ipbh0 & 0xf];
     code = env->regs[(ipbh0 & 0xf0) >> 4];
 
-    r = sclp_service_call(env, sccb, code);
+    r = sclp_service_call(sccb, code);
     if (r < 0) {
         enter_pgmcheck(env, -r);
     }
diff --git a/target-s390x/op_helper.c b/target-s390x/op_helper.c
index abc35dd..e7bb980 100644
--- a/target-s390x/op_helper.c
+++ b/target-s390x/op_helper.c
@@ -2363,13 +2363,11 @@  static void program_interrupt(CPUS390XState *env, uint32_t code, int ilc)
 }
 
 /*
+ * we handle here the part that belongs to the cpu, e.g. program checks
  * ret < 0 indicates program check, ret = 0,1,2,3 -> cc
  */
-int sclp_service_call(CPUS390XState *env, uint32_t sccb, uint64_t code)
+int sclp_service_call(uint32_t sccb, uint64_t code)
 {
-    int r = 0;
-    int shift = 0;
-
 #ifdef DEBUG_HELPER
     printf("sclp(0x%x, 0x%" PRIx64 ")\n", sccb, code);
 #endif
@@ -2382,27 +2380,8 @@  int sclp_service_call(CPUS390XState *env, uint32_t sccb, uint64_t code)
         return -PGM_SPECIFICATION;
     }
 
-    switch(code) {
-        case SCLP_CMDW_READ_SCP_INFO:
-        case SCLP_CMDW_READ_SCP_INFO_FORCED:
-            while ((ram_size >> (20 + shift)) > 65535) {
-                shift++;
-            }
-            stw_phys(sccb + SCP_MEM_CODE, ram_size >> (20 + shift));
-            stb_phys(sccb + SCP_INCREMENT, 1 << shift);
-            stw_phys(sccb + SCP_RESPONSE_CODE, 0x10);
-
-            s390_sclp_extint(sccb & ~3);
-            break;
-        default:
-#ifdef DEBUG_HELPER
-            printf("KVM: invalid sclp call 0x%x / 0x%" PRIx64 "x\n", sccb, code);
-#endif
-            r = 3;
-            break;
-    }
-
-    return r;
+    /* the complex part is handled by external components */
+    return do_sclp_service_call(sccb, code);
 }
 
 /* SCLP service call */
@@ -2410,7 +2389,7 @@  uint32_t HELPER(servc)(uint32_t r1, uint64_t r2)
 {
     int r;
 
-    r = sclp_service_call(env, r1, r2);
+    r = sclp_service_call(r1, r2);
     if (r < 0) {
         program_interrupt(env, -r, 4);
         return 0;