Patchwork [2/6] s390: sclp base support

login
register
mail settings
Submitter Christian Borntraeger
Date July 13, 2012, 10:52 a.m.
Message ID <1342176724-43776-3-git-send-email-borntraeger@de.ibm.com>
Download mbox | patch
Permalink /patch/170851/
State New
Headers show

Comments

Christian Borntraeger - July 13, 2012, 10:52 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-sclp.c           |  148 ++++++++++++++++++++++++++++++++++++++++++++++
 hw/s390-sclp.h           |   80 +++++++++++++++++++++++++
 hw/s390-virtio.c         |    3 +
 hw/s390x/Makefile.objs   |    1 +
 target-s390x/cpu.c       |   17 ++++++
 target-s390x/cpu.h       |   18 ++----
 target-s390x/kvm.c       |    5 +-
 target-s390x/op_helper.c |   45 ++------------
 8 files changed, 261 insertions(+), 56 deletions(-)
 create mode 100644 hw/s390-sclp.c
 create mode 100644 hw/s390-sclp.h
Blue Swirl - July 13, 2012, 3:08 p.m.
On Fri, Jul 13, 2012 at 10:52 AM, Christian Borntraeger
<borntraeger@de.ibm.com> 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-sclp.c           |  148 ++++++++++++++++++++++++++++++++++++++++++++++
>  hw/s390-sclp.h           |   80 +++++++++++++++++++++++++
>  hw/s390-virtio.c         |    3 +
>  hw/s390x/Makefile.objs   |    1 +
>  target-s390x/cpu.c       |   17 ++++++
>  target-s390x/cpu.h       |   18 ++----
>  target-s390x/kvm.c       |    5 +-
>  target-s390x/op_helper.c |   45 ++------------
>  8 files changed, 261 insertions(+), 56 deletions(-)
>  create mode 100644 hw/s390-sclp.c
>  create mode 100644 hw/s390-sclp.h
>
> diff --git a/hw/s390-sclp.c b/hw/s390-sclp.c
> new file mode 100644
> index 0000000..74a3e66
> --- /dev/null
> +++ b/hw/s390-sclp.c
> @@ -0,0 +1,148 @@
> +/*
> + * SCLP Support
> + *
> + * Copyright IBM, Corp. 2007, 2012

2007, really?

> + *
> + * 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.  See

Why GPLv2only, can't this be licensed under later versions?

> + * the COPYING file in the top-level directory.
> + *
> + */
> +
> +#include "cpu.h"
> +#include "kvm.h"
> +#include "sysbus.h"
> +
> +#include "s390-sclp.h"
> +
> +/* 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:
> +#ifdef DEBUG_HELPER
> +        printf("KVM: invalid sclp call 0x%x / 0x%" PRIx64 "x\n", sccb, code);
> +#endif
> +        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)

sccb could be target_phys_addr_t.

> +{
> +    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));

Perhaps the DMA helpers should be used instead.

> +    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 */
> +
> +#define S390_SCLP_BUS(obj) OBJECT_CHECK(SCLPS390Bus, (obj), TYPE_S390_SCLP_BUS)
> +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)
> +{
> +    SCLPS390Bus *bus;
> +    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, "s390-sclp-bus");
> +    bus_state->allow_hotplug = 0;
> +
> +    bus = DO_UPCAST(SCLPS390Bus, bus, bus_state);
> +    return 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(&s390_sclp_bridge_info);
> +    type_register_static(&s390_sclp_bus_info);
> +}
> +type_init(s390_sclp_register_types)
> diff --git a/hw/s390-sclp.h b/hw/s390-sclp.h
> new file mode 100644
> index 0000000..f7bf140
> --- /dev/null
> +++ b/hw/s390-sclp.h
> @@ -0,0 +1,80 @@
> +/*
> + * SCLP Support
> + *
> + * Copyright IBM, Corp. 2007, 2012
> + *
> + * Authors:
> + *  Christian Borntraeger <borntraeger@de.ibm.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2.  See
> + * the COPYING file in the top-level directory.
> + *
> + */
> +
> +#ifndef _QEMU_S390_SCLP_H
> +#define _QEMU_S390_SCLP_H

HW_S390_SCLP_H

> +
> +#include "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;
> +} __attribute__((packed)) SCCBHeader;

QEMU_PACKED

> +
> +#define SCCB_DATA_LEN (SCCB_SIZE - sizeof(SCCBHeader))
> +
> +typedef struct ReadInfo {
> +    SCCBHeader h;
> +    uint16_t rnmax;
> +    uint8_t rnsize;
> +} __attribute__((packed)) ReadInfo;
> +
> +typedef struct SCCB {
> +    SCCBHeader h;
> +    char data[SCCB_DATA_LEN];
> + } __attribute__((packed)) SCCB;
> +
> +#define TYPE_S390_SCLP_BUS "s390-sclp-bus"
> +
> +typedef struct S390SCLPDevice {
> +    DeviceState qdev;
> +} S390SCLPDevice;
> +
> +typedef struct SCLPS390Bus {
> +    BusState bus;
> +} SCLPS390Bus;
> +
> +extern SCLPS390Bus *sclp_bus;

Global state is suspicious, usually it can be avoided with parameter passing.

> +
> +SCLPS390Bus *s390_sclp_bus_init(void);
> +
> +void sclp_service_interrupt(uint32_t sccb);
> +
> +#endif
> diff --git a/hw/s390-virtio.c b/hw/s390-virtio.c
> index 47eed35..577fcee 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
>
> @@ -61,6 +62,7 @@
>  #define MAX_BLK_DEVS                    10
>
>  static VirtIOS390Bus *s390_bus;
> +SCLPS390Bus *sclp_bus;
>  static S390CPU **ipi_states;
>
>  S390CPU *s390_cpu_addr2state(uint16_t cpu_addr)
> @@ -183,6 +185,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);
> diff --git a/hw/s390x/Makefile.objs b/hw/s390x/Makefile.objs
> index dcdcac8..b2d577b 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 += s390-sclp.o
>
>  obj-y := $(addprefix ../,$(obj-y))
> diff --git a/target-s390x/cpu.c b/target-s390x/cpu.c
> index 619b202..8900872 100644
> --- a/target-s390x/cpu.c
> +++ b/target-s390x/cpu.c
> @@ -21,9 +21,26 @@
>   */
>
>  #include "cpu.h"
> +#include "kvm.h"
>  #include "qemu-common.h"
>  #include "qemu-timer.h"
>
> +/* service interrupts are floating therefore we must not pass an cpustate */
> +void s390_sclp_extint(uint32_t parm)
> +{
> +    S390CPU *dummy_cpu = s390_cpu_addr2state(0);
> +    CPUS390XState *env = &dummy_cpu->env;
> +
> +    if (kvm_enabled()) {
> +#ifdef CONFIG_KVM
> +        kvm_s390_interrupt_internal(env, KVM_S390_INT_SERVICE, parm, 0, 1);
> +#endif
> +    } else {
> +        env->psw.addr += 4;
> +        cpu_inject_ext(env, EXT_SERVICE, parm, 0);
> +    }
> +}
> +
>
>  /* CPUClass::reset() */
>  static void s390_cpu_reset(CPUState *s)
> diff --git a/target-s390x/cpu.h b/target-s390x/cpu.h
> index c30ac3a..abdd838 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);
> @@ -315,11 +316,15 @@ static inline void kvm_s390_interrupt_internal(CPUS390XState *env, int type,
>                                                 int vm)
>  {
>  }
> +
>  #endif
>  S390CPU *s390_cpu_addr2state(uint16_t cpu_addr);
>  void s390_add_running_cpu(CPUS390XState *env);
>  unsigned s390_del_running_cpu(CPUS390XState *env);
>
> +/* service interrupts are floating therefore we must not pass an cpustate */
> +void s390_sclp_extint(uint32_t parm);
> +
>  /* from s390-virtio-bus */
>  extern const target_phys_addr_t virtio_size;
>
> @@ -593,17 +598,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 */
> @@ -952,7 +946,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 654f87d..68c4f3e 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
>  };
> @@ -237,7 +234,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 91dd8dc..e7bb980 100644
> --- a/target-s390x/op_helper.c
> +++ b/target-s390x/op_helper.c
> @@ -2362,20 +2362,12 @@ static void program_interrupt(CPUS390XState *env, uint32_t code, int ilc)
>      }
>  }
>
> -static void ext_interrupt(CPUS390XState *env, int type, uint32_t param,
> -                          uint64_t param64)
> -{
> -    cpu_inject_ext(env, type, param, param64);
> -}
> -
>  /*
> + * 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
> @@ -2388,35 +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);
> -
> -            if (kvm_enabled()) {
> -#ifdef CONFIG_KVM
> -                kvm_s390_interrupt_internal(env, KVM_S390_INT_SERVICE,
> -                                            sccb & ~3, 0, 1);
> -#endif
> -            } else {
> -                env->psw.addr += 4;
> -                ext_interrupt(env, EXT_SERVICE, sccb & ~3, 0);
> -            }
> -            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 */
> @@ -2424,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;
> --
> 1.7.10.5
>
>
Christian Borntraeger - July 13, 2012, 4:44 p.m.
Thanks fpr the review,


On 13/07/12 17:08, Blue Swirl wrote:

>> + * Copyright IBM, Corp. 2007, 2012
> 
> 2007, really?

Well, yes and no. The first userspace for kvm on s390 was kuli and some of that
code was used for bringup. But it looks pretty different now, so we can
change that to be 2012 as the first release.

[...]
>> + * This work is licensed under the terms of the GNU GPL, version 2.  See
> 
> Why GPLv2only, can't this be licensed under later versions?

Was simply copied from other qemu files. We can certainly change that to 2 and later.


[...]

>> +int do_sclp_service_call(uint32_t sccb, uint64_t code)
> 
> sccb could be target_phys_addr_t.

the architecture explicitely requires to have the 32 msbs to be 0. target_phys_addr_t
would certainly work as well, but 

[...]
>> +    cpu_physical_memory_write(sccb, &work_sccb,
>> +                              be16_to_cpu(work_sccb.h.length));
> 
> Perhaps the DMA helpers should be used instead.
> 

Is there any rule what to use under which circumstances.


[...]
>> +#ifndef _QEMU_S390_SCLP_H
>> +#define _QEMU_S390_SCLP_H
> 
> HW_S390_SCLP_H

Ok

[...]
>> +} __attribute__((packed)) SCCBHeader;
> 
> QEMU_PACKED

Ok

[...]
>> +extern SCLPS390Bus *sclp_bus;
> 
> Global state is suspicious, usually it can be avoided with parameter passing.
[...]

Will check if we can make that go away.
Blue Swirl - July 14, 2012, 8:48 a.m.
On Fri, Jul 13, 2012 at 4:44 PM, Christian Borntraeger
<borntraeger@de.ibm.com> wrote:
> Thanks fpr the review,
>
>
> On 13/07/12 17:08, Blue Swirl wrote:
>
>>> + * Copyright IBM, Corp. 2007, 2012
>>
>> 2007, really?
>
> Well, yes and no. The first userspace for kvm on s390 was kuli and some of that
> code was used for bringup. But it looks pretty different now, so we can
> change that to be 2012 as the first release.
>
> [...]
>>> + * This work is licensed under the terms of the GNU GPL, version 2.  See
>>
>> Why GPLv2only, can't this be licensed under later versions?
>
> Was simply copied from other qemu files. We can certainly change that to 2 and later.

Great!

>
>
> [...]
>
>>> +int do_sclp_service_call(uint32_t sccb, uint64_t code)
>>
>> sccb could be target_phys_addr_t.
>
> the architecture explicitely requires to have the 32 msbs to be 0. target_phys_addr_t
> would certainly work as well, but
>
> [...]
>>> +    cpu_physical_memory_write(sccb, &work_sccb,
>>> +                              be16_to_cpu(work_sccb.h.length));
>>
>> Perhaps the DMA helpers should be used instead.
>>
>
> Is there any rule what to use under which circumstances.

No, it's a recent addition. Devices (at least those which may be used
in IOMMU environment) should use DMA helpers for their memory accesses
in general but virtio may be an exception. I'm not sure which category
this falls in.

>
>
> [...]
>>> +#ifndef _QEMU_S390_SCLP_H
>>> +#define _QEMU_S390_SCLP_H
>>
>> HW_S390_SCLP_H
>
> Ok
>
> [...]
>>> +} __attribute__((packed)) SCCBHeader;
>>
>> QEMU_PACKED
>
> Ok
>
> [...]
>>> +extern SCLPS390Bus *sclp_bus;
>>
>> Global state is suspicious, usually it can be avoided with parameter passing.
> [...]
>
> Will check if we can make that go away.
>
Christian Borntraeger - July 16, 2012, 7:04 a.m.
On 14/07/12 10:48, Blue Swirl wrote:
>>>> +    cpu_physical_memory_write(sccb, &work_sccb,
>>>> +                              be16_to_cpu(work_sccb.h.length));
>>>
>>> Perhaps the DMA helpers should be used instead.
>>>
>>
>> Is there any rule what to use under which circumstances.
> 
> No, it's a recent addition. Devices (at least those which may be used
> in IOMMU environment) should use DMA helpers for their memory accesses
> in general but virtio may be an exception. I'm not sure which category
> this falls in.

Ok, I read this as, if we need to handle DMA into virtual addresses at some
point in time dma helpers are the way to go.

With SCLP we access real storage (no translation) so I think 
cpu_physical_memory_write is fine here. 

Christian
Andreas Färber - July 20, 2012, 2:06 p.m.
Am 13.07.2012 12:52, schrieb Christian Borntraeger:
> 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-sclp.c           |  148 ++++++++++++++++++++++++++++++++++++++++++++++
>  hw/s390-sclp.h           |   80 +++++++++++++++++++++++++
>  hw/s390-virtio.c         |    3 +
>  hw/s390x/Makefile.objs   |    1 +
>  target-s390x/cpu.c       |   17 ++++++
>  target-s390x/cpu.h       |   18 ++----
>  target-s390x/kvm.c       |    5 +-
>  target-s390x/op_helper.c |   45 ++------------
>  8 files changed, 261 insertions(+), 56 deletions(-)
>  create mode 100644 hw/s390-sclp.c
>  create mode 100644 hw/s390-sclp.h
> 
> diff --git a/hw/s390-sclp.c b/hw/s390-sclp.c
> new file mode 100644
> index 0000000..74a3e66
> --- /dev/null
> +++ b/hw/s390-sclp.c
> @@ -0,0 +1,148 @@
> +/*
> + * SCLP Support
> + *
> + * Copyright IBM, Corp. 2007, 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.  See
> + * the COPYING file in the top-level directory.
> + *
> + */
> +
> +#include "cpu.h"
> +#include "kvm.h"
> +#include "sysbus.h"
> +
> +#include "s390-sclp.h"
> +
> +/* 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:
> +#ifdef DEBUG_HELPER
> +        printf("KVM: invalid sclp call 0x%x / 0x%" PRIx64 "x\n", sccb, code);

sccb is a pointer so %x seems wrong for 64-bit host. %p?

> +#endif
> +        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 */
> +
> +#define S390_SCLP_BUS(obj) OBJECT_CHECK(SCLPS390Bus, (obj), TYPE_S390_SCLP_BUS)

This should probably be in the header alongside TYPE_S390_SCLP_BUS?
Otherwise a separating white line would be nice.

> +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)
> +{
> +    SCLPS390Bus *bus;
> +    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, "s390-sclp-bus");

Use TYPE_S390_SCLP_BUS constant?

> +    bus_state->allow_hotplug = 0;
> +
> +    bus = DO_UPCAST(SCLPS390Bus, bus, bus_state);
> +    return 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(&s390_sclp_bridge_info);
> +    type_register_static(&s390_sclp_bus_info);
> +}
> +type_init(s390_sclp_register_types)
> diff --git a/hw/s390-sclp.h b/hw/s390-sclp.h
> new file mode 100644
> index 0000000..f7bf140
> --- /dev/null
> +++ b/hw/s390-sclp.h
> @@ -0,0 +1,80 @@
> +/*
> + * SCLP Support
> + *
> + * Copyright IBM, Corp. 2007, 2012
> + *
> + * Authors:
> + *  Christian Borntraeger <borntraeger@de.ibm.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2.  See
> + * the COPYING file in the top-level directory.
> + *
> + */
> +
> +#ifndef _QEMU_S390_SCLP_H
> +#define _QEMU_S390_SCLP_H
> +
> +#include "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;
> +} __attribute__((packed)) SCCBHeader;
> +
> +#define SCCB_DATA_LEN (SCCB_SIZE - sizeof(SCCBHeader))
> +
> +typedef struct ReadInfo {
> +    SCCBHeader h;
> +    uint16_t rnmax;
> +    uint8_t rnsize;
> +} __attribute__((packed)) ReadInfo;
> +
> +typedef struct SCCB {
> +    SCCBHeader h;
> +    char data[SCCB_DATA_LEN];
> + } __attribute__((packed)) SCCB;
> +
> +#define TYPE_S390_SCLP_BUS "s390-sclp-bus"
> +
> +typedef struct S390SCLPDevice {
> +    DeviceState qdev;

I note that this is probably the first device directly derived from
DeviceState. This should be possible now after having merged the QOM
QBus series. Test case to check is "info qdm" from the monitor.

> +} S390SCLPDevice;
> +
> +typedef struct SCLPS390Bus {
> +    BusState bus;
> +} SCLPS390Bus;
> +
> +extern SCLPS390Bus *sclp_bus;
> +
> +SCLPS390Bus *s390_sclp_bus_init(void);
> +
> +void sclp_service_interrupt(uint32_t sccb);
> +
> +#endif
> diff --git a/hw/s390-virtio.c b/hw/s390-virtio.c
> index 47eed35..577fcee 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
>  
> @@ -61,6 +62,7 @@
>  #define MAX_BLK_DEVS                    10
>  
>  static VirtIOS390Bus *s390_bus;
> +SCLPS390Bus *sclp_bus;

I don't like this so much. We shouldn't be following that example and
adding global state for each bus there. Can't we add a child property to
the machine for the hosting device so that we can obtain that via QOM
path and access the bus from there?

>  static S390CPU **ipi_states;
>  
>  S390CPU *s390_cpu_addr2state(uint16_t cpu_addr)
> @@ -183,6 +185,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);
> diff --git a/hw/s390x/Makefile.objs b/hw/s390x/Makefile.objs
> index dcdcac8..b2d577b 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 += s390-sclp.o
>  
>  obj-y := $(addprefix ../,$(obj-y))

IIUC we have a dependency on the CPU inside the device code. If so,
please move the s390-sclp.c file into hw/s390x/ and move the obj-y +=
line below the addprefix line.

> diff --git a/target-s390x/cpu.c b/target-s390x/cpu.c
> index 619b202..8900872 100644
> --- a/target-s390x/cpu.c
> +++ b/target-s390x/cpu.c
> @@ -21,9 +21,26 @@
>   */
>  
>  #include "cpu.h"
> +#include "kvm.h"
>  #include "qemu-common.h"
>  #include "qemu-timer.h"
>  
> +/* service interrupts are floating therefore we must not pass an cpustate */
> +void s390_sclp_extint(uint32_t parm)
> +{
> +    S390CPU *dummy_cpu = s390_cpu_addr2state(0);
> +    CPUS390XState *env = &dummy_cpu->env;
> +
> +    if (kvm_enabled()) {
> +#ifdef CONFIG_KVM
> +        kvm_s390_interrupt_internal(env, KVM_S390_INT_SERVICE, parm, 0, 1);
> +#endif
> +    } else {
> +        env->psw.addr += 4;
> +        cpu_inject_ext(env, EXT_SERVICE, parm, 0);
> +    }
> +}

Not so happy about this placement, it is not called s390_cpu_, the
prototype is in cpu.h not cpu-qom.h and it operates only indirectly on
the CPU. Is there another place for it?

> +
>  
>  /* CPUClass::reset() */
>  static void s390_cpu_reset(CPUState *s)
> diff --git a/target-s390x/cpu.h b/target-s390x/cpu.h
> index c30ac3a..abdd838 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);
> @@ -315,11 +316,15 @@ static inline void kvm_s390_interrupt_internal(CPUS390XState *env, int type,
>                                                 int vm)
>  {
>  }
> +
>  #endif

Stray whitespace change? If we want to do it, we might want to add one
after #endif as well and after #ifdef in the preceding hunk.

Regards,
Andreas

>  S390CPU *s390_cpu_addr2state(uint16_t cpu_addr);
>  void s390_add_running_cpu(CPUS390XState *env);
>  unsigned s390_del_running_cpu(CPUS390XState *env);
>  
> +/* service interrupts are floating therefore we must not pass an cpustate */
> +void s390_sclp_extint(uint32_t parm);
> +
>  /* from s390-virtio-bus */
>  extern const target_phys_addr_t virtio_size;
>  
> @@ -593,17 +598,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 */
> @@ -952,7 +946,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 654f87d..68c4f3e 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
>  };
> @@ -237,7 +234,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 91dd8dc..e7bb980 100644
> --- a/target-s390x/op_helper.c
> +++ b/target-s390x/op_helper.c
> @@ -2362,20 +2362,12 @@ static void program_interrupt(CPUS390XState *env, uint32_t code, int ilc)
>      }
>  }
>  
> -static void ext_interrupt(CPUS390XState *env, int type, uint32_t param,
> -                          uint64_t param64)
> -{
> -    cpu_inject_ext(env, type, param, param64);
> -}
> -
>  /*
> + * 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
> @@ -2388,35 +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);
> -
> -            if (kvm_enabled()) {
> -#ifdef CONFIG_KVM
> -                kvm_s390_interrupt_internal(env, KVM_S390_INT_SERVICE,
> -                                            sccb & ~3, 0, 1);
> -#endif
> -            } else {
> -                env->psw.addr += 4;
> -                ext_interrupt(env, EXT_SERVICE, sccb & ~3, 0);
> -            }
> -            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 */
> @@ -2424,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;
>
Christian Borntraeger - July 23, 2012, 11:05 a.m.
Andreas,

thanks for having a look. I no other comments arrive today I will resubmit the patch 
set with all comments addressed.

On 20/07/12 16:06, Andreas Färber wrote:
>> +#ifdef DEBUG_HELPER
>> +        printf("KVM: invalid sclp call 0x%x / 0x%" PRIx64 "x\n", sccb, code);
> 
> sccb is a pointer so %x seems wrong for 64-bit host. %p?

yes its broken.
We will probably just remove that, since it will be changed by followon patch.


>> +typedef struct S390SCLPDevice {
>> +    DeviceState qdev;
> 
> I note that this is probably the first device directly derived from
> DeviceState. This should be possible now after having merged the QOM
> QBus series. Test case to check is "info qdm" from the monitor.

info qdm seems to work. what test did you have in mind?

[...]
>>  //#define DEBUG_S390
>>  
>> @@ -61,6 +62,7 @@
>>  #define MAX_BLK_DEVS                    10
>>  
>>  static VirtIOS390Bus *s390_bus;
>> +SCLPS390Bus *sclp_bus;
> 
> I don't like this so much. We shouldn't be following that example and
> adding global state for each bus there. Can't we add a child property to
> the machine for the hosting device so that we can obtain that via QOM
> path and access the bus from there?

Since Blue already compained Heinz already moved that bus into s390-sclp.c as a
static variable without any export since we only touch that there. We also added
an comment that by architecture definition there is only one sclp per machine, so 
this should be fine.


>> --- 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 += s390-sclp.o
>>  
>>  obj-y := $(addprefix ../,$(obj-y))
> 
> IIUC we have a dependency on the CPU inside the device code. If so,
> please move the s390-sclp.c file into hw/s390x/ and move the obj-y +=
> line below the addprefix line.

Ok.

>> diff --git a/target-s390x/cpu.c b/target-s390x/cpu.c
>> index 619b202..8900872 100644
>> --- a/target-s390x/cpu.c
>> +++ b/target-s390x/cpu.c
>> @@ -21,9 +21,26 @@
>>   */
>>  
>>  #include "cpu.h"
>> +#include "kvm.h"
>>  #include "qemu-common.h"
>>  #include "qemu-timer.h"
>>  
>> +/* service interrupts are floating therefore we must not pass an cpustate */
>> +void s390_sclp_extint(uint32_t parm)
>> +{
>> +    S390CPU *dummy_cpu = s390_cpu_addr2state(0);
>> +    CPUS390XState *env = &dummy_cpu->env;
>> +
>> +    if (kvm_enabled()) {
>> +#ifdef CONFIG_KVM
>> +        kvm_s390_interrupt_internal(env, KVM_S390_INT_SERVICE, parm, 0, 1);
>> +#endif
>> +    } else {
>> +        env->psw.addr += 4;
>> +        cpu_inject_ext(env, EXT_SERVICE, parm, 0);
>> +    }
>> +}
> 
> Not so happy about this placement, it is not called s390_cpu_, the
> prototype is in cpu.h not cpu-qom.h and it operates only indirectly on
> the CPU. Is there another place for it?

Not yet. I will try to introduce a new one, e.g. target-s390x/interrupt.c
We can then move other code there as well.

Patch

diff --git a/hw/s390-sclp.c b/hw/s390-sclp.c
new file mode 100644
index 0000000..74a3e66
--- /dev/null
+++ b/hw/s390-sclp.c
@@ -0,0 +1,148 @@ 
+/*
+ * SCLP Support
+ *
+ * Copyright IBM, Corp. 2007, 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.  See
+ * the COPYING file in the top-level directory.
+ *
+ */
+
+#include "cpu.h"
+#include "kvm.h"
+#include "sysbus.h"
+
+#include "s390-sclp.h"
+
+/* 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:
+#ifdef DEBUG_HELPER
+        printf("KVM: invalid sclp call 0x%x / 0x%" PRIx64 "x\n", sccb, code);
+#endif
+        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 */
+
+#define S390_SCLP_BUS(obj) OBJECT_CHECK(SCLPS390Bus, (obj), TYPE_S390_SCLP_BUS)
+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)
+{
+    SCLPS390Bus *bus;
+    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, "s390-sclp-bus");
+    bus_state->allow_hotplug = 0;
+
+    bus = DO_UPCAST(SCLPS390Bus, bus, bus_state);
+    return 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(&s390_sclp_bridge_info);
+    type_register_static(&s390_sclp_bus_info);
+}
+type_init(s390_sclp_register_types)
diff --git a/hw/s390-sclp.h b/hw/s390-sclp.h
new file mode 100644
index 0000000..f7bf140
--- /dev/null
+++ b/hw/s390-sclp.h
@@ -0,0 +1,80 @@ 
+/*
+ * SCLP Support
+ *
+ * Copyright IBM, Corp. 2007, 2012
+ *
+ * Authors:
+ *  Christian Borntraeger <borntraeger@de.ibm.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2.  See
+ * the COPYING file in the top-level directory.
+ *
+ */
+
+#ifndef _QEMU_S390_SCLP_H
+#define _QEMU_S390_SCLP_H
+
+#include "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;
+} __attribute__((packed)) SCCBHeader;
+
+#define SCCB_DATA_LEN (SCCB_SIZE - sizeof(SCCBHeader))
+
+typedef struct ReadInfo {
+    SCCBHeader h;
+    uint16_t rnmax;
+    uint8_t rnsize;
+} __attribute__((packed)) ReadInfo;
+
+typedef struct SCCB {
+    SCCBHeader h;
+    char data[SCCB_DATA_LEN];
+ } __attribute__((packed)) SCCB;
+
+#define TYPE_S390_SCLP_BUS "s390-sclp-bus"
+
+typedef struct S390SCLPDevice {
+    DeviceState qdev;
+} S390SCLPDevice;
+
+typedef struct SCLPS390Bus {
+    BusState bus;
+} SCLPS390Bus;
+
+extern SCLPS390Bus *sclp_bus;
+
+SCLPS390Bus *s390_sclp_bus_init(void);
+
+void sclp_service_interrupt(uint32_t sccb);
+
+#endif
diff --git a/hw/s390-virtio.c b/hw/s390-virtio.c
index 47eed35..577fcee 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
 
@@ -61,6 +62,7 @@ 
 #define MAX_BLK_DEVS                    10
 
 static VirtIOS390Bus *s390_bus;
+SCLPS390Bus *sclp_bus;
 static S390CPU **ipi_states;
 
 S390CPU *s390_cpu_addr2state(uint16_t cpu_addr)
@@ -183,6 +185,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);
diff --git a/hw/s390x/Makefile.objs b/hw/s390x/Makefile.objs
index dcdcac8..b2d577b 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 += s390-sclp.o
 
 obj-y := $(addprefix ../,$(obj-y))
diff --git a/target-s390x/cpu.c b/target-s390x/cpu.c
index 619b202..8900872 100644
--- a/target-s390x/cpu.c
+++ b/target-s390x/cpu.c
@@ -21,9 +21,26 @@ 
  */
 
 #include "cpu.h"
+#include "kvm.h"
 #include "qemu-common.h"
 #include "qemu-timer.h"
 
+/* service interrupts are floating therefore we must not pass an cpustate */
+void s390_sclp_extint(uint32_t parm)
+{
+    S390CPU *dummy_cpu = s390_cpu_addr2state(0);
+    CPUS390XState *env = &dummy_cpu->env;
+
+    if (kvm_enabled()) {
+#ifdef CONFIG_KVM
+        kvm_s390_interrupt_internal(env, KVM_S390_INT_SERVICE, parm, 0, 1);
+#endif
+    } else {
+        env->psw.addr += 4;
+        cpu_inject_ext(env, EXT_SERVICE, parm, 0);
+    }
+}
+
 
 /* CPUClass::reset() */
 static void s390_cpu_reset(CPUState *s)
diff --git a/target-s390x/cpu.h b/target-s390x/cpu.h
index c30ac3a..abdd838 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);
@@ -315,11 +316,15 @@  static inline void kvm_s390_interrupt_internal(CPUS390XState *env, int type,
                                                int vm)
 {
 }
+
 #endif
 S390CPU *s390_cpu_addr2state(uint16_t cpu_addr);
 void s390_add_running_cpu(CPUS390XState *env);
 unsigned s390_del_running_cpu(CPUS390XState *env);
 
+/* service interrupts are floating therefore we must not pass an cpustate */
+void s390_sclp_extint(uint32_t parm);
+
 /* from s390-virtio-bus */
 extern const target_phys_addr_t virtio_size;
 
@@ -593,17 +598,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 */
@@ -952,7 +946,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 654f87d..68c4f3e 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
 };
@@ -237,7 +234,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 91dd8dc..e7bb980 100644
--- a/target-s390x/op_helper.c
+++ b/target-s390x/op_helper.c
@@ -2362,20 +2362,12 @@  static void program_interrupt(CPUS390XState *env, uint32_t code, int ilc)
     }
 }
 
-static void ext_interrupt(CPUS390XState *env, int type, uint32_t param,
-                          uint64_t param64)
-{
-    cpu_inject_ext(env, type, param, param64);
-}
-
 /*
+ * 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
@@ -2388,35 +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);
-
-            if (kvm_enabled()) {
-#ifdef CONFIG_KVM
-                kvm_s390_interrupt_internal(env, KVM_S390_INT_SERVICE,
-                                            sccb & ~3, 0, 1);
-#endif
-            } else {
-                env->psw.addr += 4;
-                ext_interrupt(env, EXT_SERVICE, sccb & ~3, 0);
-            }
-            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 */
@@ -2424,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;