Patchwork [04/11] Add KVM support for S390x

login
register
mail settings
Submitter Alexander Graf
Date Nov. 26, 2009, 1:23 p.m.
Message ID <1259241800-2810-5-git-send-email-agraf@suse.de>
Download mbox | patch
Permalink /patch/39510/
State New
Headers show

Comments

Alexander Graf - Nov. 26, 2009, 1:23 p.m.
S390x was one of the first platforms that received support for KVM back in the
day. Unfortunately until now there hasn't been a qemu implementation that would
enable users to actually run guests.

So let's include support for KVM S390x in qemu!

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 configure          |    4 +-
 target-s390x/kvm.c |  463 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 466 insertions(+), 1 deletions(-)
 create mode 100644 target-s390x/kvm.c
Aurelien Jarno - Nov. 30, 2009, 6:18 p.m.
On Thu, Nov 26, 2009 at 02:23:13PM +0100, Alexander Graf wrote:
> S390x was one of the first platforms that received support for KVM back in the
> day. Unfortunately until now there hasn't been a qemu implementation that would
> enable users to actually run guests.
> 
> So let's include support for KVM S390x in qemu!

Please find the comments below.

> Signed-off-by: Alexander Graf <agraf@suse.de>
> ---
>  configure          |    4 +-
>  target-s390x/kvm.c |  463 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 466 insertions(+), 1 deletions(-)
>  create mode 100644 target-s390x/kvm.c
> 
> diff --git a/configure b/configure
> index b616e1a..cf466ec 100755
> --- a/configure
> +++ b/configure
> @@ -1348,6 +1348,8 @@ EOF
>              kvm_cflags="$kvm_cflags -I$kerneldir/arch/x86/include"
>  	elif test "$cpu" = "ppc" -a -d "$kerneldir/arch/powerpc/include" ; then
>  	    kvm_cflags="$kvm_cflags -I$kerneldir/arch/powerpc/include"
> +	elif test "$cpu" = "s390x" -a -d "$kerneldir/arch/s390/include" ; then
> +	    kvm_cflags="$kvm_cflags -I$kerneldir/arch/s390/include"
>          elif test -d "$kerneldir/arch/$cpu/include" ; then
>              kvm_cflags="$kvm_cflags -I$kerneldir/arch/$cpu/include"
>        fi
> @@ -2360,7 +2362,7 @@ case "$target_arch2" in
>      fi
>  esac
>  case "$target_arch2" in
> -  i386|x86_64|ppcemb|ppc|ppc64)
> +  i386|x86_64|ppcemb|ppc|ppc64|s390x)
>      # Make sure the target and host cpus are compatible
>      if test "$kvm" = "yes" -a "$target_softmmu" = "yes" -a \
>        \( "$target_arch2" = "$cpu" -o \
> diff --git a/target-s390x/kvm.c b/target-s390x/kvm.c
> new file mode 100644
> index 0000000..d477664
> --- /dev/null
> +++ b/target-s390x/kvm.c
> @@ -0,0 +1,463 @@
> +/*
> + * QEMU S390x KVM implementation
> + *
> + * Copyright (c) 2009 Alexander Graf <agraf@suse.de>
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library; if not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include <sys/types.h>
> +#include <sys/ioctl.h>
> +#include <sys/mman.h>
> +
> +#include <linux/kvm.h>
> +#include <asm/ptrace.h>
> +
> +#include "qemu-common.h"
> +#include "qemu-timer.h"
> +#include "sysemu.h"
> +#include "kvm.h"
> +#include "cpu.h"
> +#include "device_tree.h"
> +
> +/* #define DEBUG_KVM */
> +
> +#ifdef DEBUG_KVM
> +#define dprintf(fmt, ...) \
> +    do { fprintf(stderr, fmt, ## __VA_ARGS__); } while (0)
> +#else
> +#define dprintf(fmt, ...) \
> +    do { } while (0)
> +#endif
> +
> +#define IPA0_DIAG               0x8300
> +#define IPA0_SIGP               0xae00
> +#define IPA0_PRIV               0xb200
> +
> +#define PRIV_SCLP_CALL          0x20
> +#define DIAG_KVM_HYPERCALL      0x500
> +#define DIAG_KVM_BREAKPOINT     0x501
> +
> +#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
> +
> +#define ICPT_INSTRUCTION        0x04
> +#define ICPT_WAITPSW            0x1c
> +#define ICPT_SOFT_INTERCEPT     0x24
> +#define ICPT_CPU_STOP           0x28
> +#define ICPT_IO                 0x40
> +
> +#define SIGP_RESTART            0x06
> +#define SIGP_INITIAL_CPU_RESET  0x0b
> +#define SIGP_STORE_STATUS_ADDR  0x0e
> +#define SIGP_SET_ARCH           0x12
> +
> +
> +int kvm_arch_init(KVMState *s, int smp_cpus)
> +{
> +    return 0;
> +}
> +
> +int kvm_arch_init_vcpu(CPUState *env)
> +{
> +    int ret = 0;
> +
> +    if (kvm_vcpu_ioctl(env, KVM_S390_INITIAL_RESET, NULL) < 0)
> +        perror("cannot init reset vcpu");

coding style.

> +
> +    return ret;
> +}
> +
> +void kvm_arch_reset_vcpu(CPUState *env)
> +{
> +}

Is there really nothing to do? If some code has to be added later, it
may be worth adding a comment.

> +int kvm_arch_put_registers(CPUState *env)
> +{
> +    struct kvm_regs regs;
> +    int ret;
> +    int i;
> +
> +    ret = kvm_vcpu_ioctl(env, KVM_GET_REGS, &regs);
> +    if (ret < 0)
> +        return ret;

coding style.

> +    for (i = 0; i < 16; i++)
> +        regs.gprs[i] = env->regs[i];

coding style.

> +
> +    ret = kvm_vcpu_ioctl(env, KVM_SET_REGS, &regs);
> +    if (ret < 0)
> +        return ret;

coding style.

> +
> +    env->kvm_run->psw_addr = env->psw.addr;
> +    env->kvm_run->psw_mask = env->psw.mask;
> +
> +    return ret;
> +}
> +
> +int kvm_arch_get_registers(CPUState *env)
> +{
> +    uint32_t ret;
> +    struct kvm_regs regs;
> +    int i;
> +
> +    ret = kvm_vcpu_ioctl(env, KVM_GET_REGS, &regs);
> +    if (ret < 0)
> +        return ret;

coding style.

> +
> +    for (i = 0; i < 16; i++)
> +        env->regs[i] = regs.gprs[i];

coding style.

> +
> +    env->psw.addr = env->kvm_run->psw_addr;
> +    env->psw.mask = env->kvm_run->psw_mask;
> +
> +    return 0;
> +}
> +
> +int kvm_arch_insert_sw_breakpoint(CPUState *env, struct kvm_sw_breakpoint *bp)
> +{
> +    static const uint8_t diag_501[] = {0x83, 0x24, 0x05, 0x01};
> +
> +    if (cpu_memory_rw_debug(env, bp->pc, (uint8_t *)&bp->saved_insn, 4, 0) ||
> +        cpu_memory_rw_debug(env, bp->pc, (uint8_t *)diag_501, 4, 1))
> +        return -EINVAL;

coding style.

> +    return 0;
> +}
> +
> +int kvm_arch_remove_sw_breakpoint(CPUState *env, struct kvm_sw_breakpoint *bp)
> +{
> +    uint8_t t[4];
> +    static const uint8_t diag_501[] = {0x83, 0x24, 0x05, 0x01};
> +
> +    if (cpu_memory_rw_debug(env, bp->pc, t, 4, 0))
> +        return -EINVAL;
> +    if (memcmp(t, diag_501, 4))
> +        return -EINVAL;
> +    if (cpu_memory_rw_debug(env, bp->pc, (uint8_t *)&bp->saved_insn, 1, 1))
> +        return -EINVAL;

coding style.

> +
> +    return 0;
> +}
> +
> +int kvm_arch_pre_run(CPUState *env, struct kvm_run *run)
> +{
> +    return 0;
> +}
> +
> +int kvm_arch_post_run(CPUState *env, struct kvm_run *run)
> +{
> +    return 0;
> +}
> +
> +static void _kvm_s390_interrupt(CPUState *env, int type, uint32_t parm, uint64_t parm64, int vm)
> +{

Why such a name starting with an underscore?

> +    struct kvm_s390_interrupt kvmint;
> +    int r;
> +
> +    if (!env->kvm_state)
> +        return;

coding style.

> +
> +    env->halted = 0;
> +    env->exception_index = 0;
> +
> +    kvmint.type = type;
> +    kvmint.parm = parm;
> +    kvmint.parm64 = parm64;
> +
> +    if (vm)
> +        r = kvm_vm_ioctl(env->kvm_state, KVM_S390_INTERRUPT, &kvmint);
> +    else 
> +        r = kvm_vcpu_ioctl(env, KVM_S390_INTERRUPT, &kvmint);

coding style.

> +
> +    if (r < 0) {
> +        fprintf(stderr, "KVM failed to inject interrupt\n");
> +        exit(1);
> +    }
> +}
> +
> +void kvm_s390_virtio_irq(CPUState *env, int config_change, uint64_t token)
> +{
> +    _kvm_s390_interrupt(env, KVM_S390_INT_VIRTIO, config_change, token, 1);
> +}
> +
> +static void kvm_s390_interrupt(CPUState *env, int type, uint32_t code)
> +{
> +    _kvm_s390_interrupt(env, type, code, 0, 0);
> +}
> +
> +static void enter_pgmcheck(CPUState *env, uint16_t code)
> +{
> +    kvm_s390_interrupt(env, KVM_S390_PROGRAM_INT, code);
> +}
> +
> +static void setcc(CPUState *env, uint64_t cc)
> +{
> +    env->kvm_run->psw_mask &= ~(3ul << 44);
> +    env->kvm_run->psw_mask |= (cc & 3) << 44;
> +
> +    env->psw.mask &= ~(3ul << 44);
> +    env->psw.mask |= (cc & 3) << 44;
> +}
> +
> +static int sclp_service_call(CPUState *env, struct kvm_run *run, uint16_t ipbh0)
> +{
> +    uint32_t sccb;
> +    uint64_t code;
> +    int r = 0;
> +
> +    cpu_synchronize_state(env);
> +    sccb = env->regs[ipbh0 & 0xf];
> +    code = env->regs[(ipbh0 & 0xf0) >> 4];
> +
> +    dprintf("sclp(0x%x, 0x%lx)\n", sccb, code);
> +
> +    if (sccb & ~0x7ffffff8ul) {
> +        fprintf(stderr, "KVM: invalid sccb address 0x%x\n", sccb);
> +        r = -1;
> +        goto out;
> +    }
> +
> +    switch(code) {
> +        case 0x00020001:
> +        case 0x00120001:

What are those constants?

> +            stw_phys(sccb + SCP_MEM_CODE, ram_size >> 20);
> +            stb_phys(sccb + SCP_INCREMENT, 1);
> +            stw_phys(sccb + SCP_RESPONSE_CODE, 0x10);
> +            setcc(env, 0);
> +
> +            _kvm_s390_interrupt(env, KVM_S390_INT_SERVICE, sccb & ~3, 0, 1);
> +            break;
> +        default:
> +            dprintf("KVM: invalid sclp call 0x%x / 0x%lx\n", sccb, code);
> +            r = -1;
> +            break;
> +    }
> +
> +out:
> +    if (r < 0)
> +        setcc(env, 3);

coding style.

> +    return 0;
> +}
> +
> +static int handle_priv(CPUState *env, struct kvm_run *run, uint8_t ipa1)
> +{
> +    int r = 0;
> +    uint16_t ipbh0 = (run->s390_sieic.ipb & 0xffff0000) >> 16;
> +
> +    dprintf("KVM: PRIV: %d\n", ipa1);
> +    switch (ipa1) {
> +        case PRIV_SCLP_CALL:
> +            r = sclp_service_call(env, run, ipbh0);
> +            break;
> +        default:
> +            dprintf("KVM: unknown PRIV: 0x%x\n", ipa1);
> +            r = -1;
> +            break;
> +    }
> +
> +    return r;
> +}
> +
> +static int handle_hypercall(CPUState *env, struct kvm_run *run)
> +{
> +    int r;
> +
> +    cpu_synchronize_state(env);
> +    r = s390_virtio_hypercall(env);
> +    kvm_arch_put_registers(env);
> +
> +    return r;
> +}
> +
> +static int handle_diag(CPUState *env, struct kvm_run *run, int ipb_code)
> +{
> +    int r = 0;
> +
> +    switch (ipb_code) {
> +        case DIAG_KVM_HYPERCALL:
> +            r = handle_hypercall(env, run);
> +            break;
> +        case DIAG_KVM_BREAKPOINT:
> +            sleep(10);
> +            break;
> +        default:
> +            dprintf("KVM: unknown DIAG: 0x%x\n", ipb_code);
> +            r = -1;
> +            break;
> +    }
> +
> +    return r;
> +}
> +
> +static int s390_cpu_restart(CPUState *env)
> +{
> +    kvm_s390_interrupt(env, KVM_S390_RESTART, 0);
> +    env->halted = 0;
> +    env->exception_index = 0;
> +    qemu_cpu_kick(env);
> +    fprintf(stderr, "DONE: SIGP cpu restart: %p\n", env);
> +    return 0;
> +}
> +
> +static int s390_store_status(CPUState *env, uint32_t parameter)
> +{
> +    /* XXX */
> +    fprintf(stderr, "XXX SIGP store status\n");
> +    return -1;
> +}
> +
> +static int s390_cpu_initial_reset(CPUState *env)
> +{
> +    /* XXX */
> +    fprintf(stderr, "XXX SIGP init\n");
> +    return -1;
> +}
> +
> +static int handle_sigp(CPUState *env, struct kvm_run *run, uint8_t ipa1)
> +{
> +    uint8_t order_code;
> +    uint32_t parameter;
> +    uint16_t cpu_addr;
> +    uint8_t t;
> +    int r = -1;
> +    CPUState *target_env;
> +
> +    cpu_synchronize_state(env);
> +
> +    /* get order code */
> +    order_code = run->s390_sieic.ipb >> 28;
> +    if (order_code > 0)
> +        order_code = env->regs[order_code];

coding style.

> +    order_code += (run->s390_sieic.ipb & 0x0fff0000) >> 16;
> +
> +    /* get parameters */
> +    t = (ipa1 & 0xf0) >> 4;
> +    if (!(t % 2))
> +        t++;

coding style.

> +
> +    parameter = env->regs[t] & 0x7ffffe00;
> +    cpu_addr = env->regs[ipa1 & 0x0f];
> +
> +    target_env = s390_cpu_addr2state(cpu_addr);
> +    if (!target_env)
> +        goto out;

coding style.

> +
> +    switch (order_code) {
> +        case SIGP_RESTART:
> +            r = s390_cpu_restart(target_env);
> +            break;
> +        case SIGP_STORE_STATUS_ADDR:
> +            r = s390_store_status(target_env, parameter);
> +            break;
> +        case SIGP_SET_ARCH:
> +            /* make the caller panic */
> +            return -1;
> +        case SIGP_INITIAL_CPU_RESET:
> +            r = s390_cpu_initial_reset(target_env);
> +            break;
> +        default:
> +            fprintf(stderr, "KVM: unknown SIGP: 0x%x\n", ipa1);
> +            break;
> +    }
> +
> +out:
> +    setcc(env, r ? 3 : 0);
> +    return 0;
> +}
> +
> +static int handle_instruction(CPUState *env, struct kvm_run *run)
> +{
> +    unsigned int ipa0 = (run->s390_sieic.ipa & 0xff00);
> +    uint8_t ipa1 = run->s390_sieic.ipa & 0x00ff;
> +    int ipb_code = (run->s390_sieic.ipb & 0x0fff0000) >> 16;
> +    int r = 0;
> +
> +    dprintf("handle_instruction 0x%x 0x%x\n", run->s390_sieic.ipa, run->s390_sieic.ipb);
> +    switch (ipa0) {
> +        case IPA0_PRIV:
> +            r = handle_priv(env, run, ipa1);
> +            break;
> +        case IPA0_DIAG:
> +            r = handle_diag(env, run, ipb_code);
> +            break;
> +        case IPA0_SIGP:
> +            r = handle_sigp(env, run, ipa1);
> +            break;
> +    }
> +
> +    if (r < 0) {
> +        enter_pgmcheck(env, 0x0001);
> +    }
> +    return r;
> +}
> +
> +static int handle_intercept(CPUState *env)
> +{
> +    struct kvm_run *run = env->kvm_run;
> +    int icpt_code = run->s390_sieic.icptcode;
> +    int r = 0;
> +
> +    dprintf("intercept: 0x%x (at 0x%lx)\n", icpt_code, env->kvm_run->psw_addr);
> +    switch (icpt_code) {
> +        case ICPT_INSTRUCTION:
> +            r = handle_instruction(env, run);
> +            break;
> +        case ICPT_WAITPSW:
> +            /* XXX What to do on system shutdown? */
> +            env->halted = 1;
> +            env->exception_index = EXCP_HLT;
> +            break;
> +        case ICPT_SOFT_INTERCEPT:
> +            fprintf(stderr, "KVM unimplemented icpt SOFT\n");
> +            exit(1);
> +            break;
> +        case ICPT_CPU_STOP:
> +            qemu_system_shutdown_request();
> +            break;
> +        case ICPT_IO:
> +            fprintf(stderr, "KVM unimplemented icpt IO\n");
> +            exit(1);
> +            break;
> +        default:
> +            fprintf(stderr, "Unknown intercept code: %d\n", icpt_code);
> +            exit(1);
> +            break;
> +    }
> +
> +    return r;
> +}
> +
> +int kvm_arch_handle_exit(CPUState *env, struct kvm_run *run)
> +{
> +    int ret = 0;
> +
> +    switch (run->exit_reason) {
> +        case KVM_EXIT_S390_SIEIC:
> +            ret = handle_intercept(env);
> +            break;
> +        case KVM_EXIT_S390_RESET:
> +            fprintf(stderr, "RESET not implemented\n");
> +            exit(1);
> +            break;
> +        default:
> +            fprintf(stderr, "Unknown KVM exit: %d\n", run->exit_reason);
> +            break;
> +    }
> +
> +    return ret;
> +}
> -- 
> 1.6.0.2
> 
> 
> 
>
Alexander Graf - Nov. 30, 2009, 10:25 p.m.
On 30.11.2009, at 19:18, Aurelien Jarno wrote:

> On Thu, Nov 26, 2009 at 02:23:13PM +0100, Alexander Graf wrote:
>> S390x was one of the first platforms that received support for KVM back in the
>> day. Unfortunately until now there hasn't been a qemu implementation that would
>> enable users to actually run guests.
>> 
>> So let's include support for KVM S390x in qemu!
> 
> Please find the comments below.
> 
>> Signed-off-by: Alexander Graf <agraf@suse.de>
>> ---
>> configure          |    4 +-
>> target-s390x/kvm.c |  463 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>> 2 files changed, 466 insertions(+), 1 deletions(-)
>> create mode 100644 target-s390x/kvm.c
>> 
>> diff --git a/configure b/configure
>> index b616e1a..cf466ec 100755
>> --- a/configure
>> +++ b/configure
>> @@ -1348,6 +1348,8 @@ EOF
>>             kvm_cflags="$kvm_cflags -I$kerneldir/arch/x86/include"
>> 	elif test "$cpu" = "ppc" -a -d "$kerneldir/arch/powerpc/include" ; then
>> 	    kvm_cflags="$kvm_cflags -I$kerneldir/arch/powerpc/include"
>> +	elif test "$cpu" = "s390x" -a -d "$kerneldir/arch/s390/include" ; then
>> +	    kvm_cflags="$kvm_cflags -I$kerneldir/arch/s390/include"
>>         elif test -d "$kerneldir/arch/$cpu/include" ; then
>>             kvm_cflags="$kvm_cflags -I$kerneldir/arch/$cpu/include"
>>       fi
>> @@ -2360,7 +2362,7 @@ case "$target_arch2" in
>>     fi
>> esac
>> case "$target_arch2" in
>> -  i386|x86_64|ppcemb|ppc|ppc64)
>> +  i386|x86_64|ppcemb|ppc|ppc64|s390x)
>>     # Make sure the target and host cpus are compatible
>>     if test "$kvm" = "yes" -a "$target_softmmu" = "yes" -a \
>>       \( "$target_arch2" = "$cpu" -o \
>> diff --git a/target-s390x/kvm.c b/target-s390x/kvm.c
>> new file mode 100644
>> index 0000000..d477664
>> --- /dev/null
>> +++ b/target-s390x/kvm.c
>> @@ -0,0 +1,463 @@
>> +/*
>> + * QEMU S390x KVM implementation
>> + *
>> + * Copyright (c) 2009 Alexander Graf <agraf@suse.de>
>> + *
>> + * This library is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU Lesser General Public
>> + * License as published by the Free Software Foundation; either
>> + * version 2 of the License, or (at your option) any later version.
>> + *
>> + * This library is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
>> + * Lesser General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU Lesser General Public
>> + * License along with this library; if not, see <http://www.gnu.org/licenses/>.
>> + */
>> +
>> +#include <sys/types.h>
>> +#include <sys/ioctl.h>
>> +#include <sys/mman.h>
>> +
>> +#include <linux/kvm.h>
>> +#include <asm/ptrace.h>
>> +
>> +#include "qemu-common.h"
>> +#include "qemu-timer.h"
>> +#include "sysemu.h"
>> +#include "kvm.h"
>> +#include "cpu.h"
>> +#include "device_tree.h"
>> +
>> +/* #define DEBUG_KVM */
>> +
>> +#ifdef DEBUG_KVM
>> +#define dprintf(fmt, ...) \
>> +    do { fprintf(stderr, fmt, ## __VA_ARGS__); } while (0)
>> +#else
>> +#define dprintf(fmt, ...) \
>> +    do { } while (0)
>> +#endif
>> +
>> +#define IPA0_DIAG               0x8300
>> +#define IPA0_SIGP               0xae00
>> +#define IPA0_PRIV               0xb200
>> +
>> +#define PRIV_SCLP_CALL          0x20
>> +#define DIAG_KVM_HYPERCALL      0x500
>> +#define DIAG_KVM_BREAKPOINT     0x501
>> +
>> +#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
>> +
>> +#define ICPT_INSTRUCTION        0x04
>> +#define ICPT_WAITPSW            0x1c
>> +#define ICPT_SOFT_INTERCEPT     0x24
>> +#define ICPT_CPU_STOP           0x28
>> +#define ICPT_IO                 0x40
>> +
>> +#define SIGP_RESTART            0x06
>> +#define SIGP_INITIAL_CPU_RESET  0x0b
>> +#define SIGP_STORE_STATUS_ADDR  0x0e
>> +#define SIGP_SET_ARCH           0x12
>> +
>> +
>> +int kvm_arch_init(KVMState *s, int smp_cpus)
>> +{
>> +    return 0;
>> +}
>> +
>> +int kvm_arch_init_vcpu(CPUState *env)
>> +{
>> +    int ret = 0;
>> +
>> +    if (kvm_vcpu_ioctl(env, KVM_S390_INITIAL_RESET, NULL) < 0)
>> +        perror("cannot init reset vcpu");
> 
> coding style.
> 
>> +
>> +    return ret;
>> +}
>> +
>> +void kvm_arch_reset_vcpu(CPUState *env)
>> +{
>> +}
> 
> Is there really nothing to do? If some code has to be added later, it
> may be worth adding a comment.

As you have probably realized by now, all reset code is missing. In fact, we don't even ever call the qemu reset functions to actually do a reset.

>> +
>> +static void _kvm_s390_interrupt(CPUState *env, int type, uint32_t parm, uint64_t parm64, int vm)
>> +{
> 
> Why such a name starting with an underscore?

Because that's the internal function that gets used by the exported, properly named ones. Are there any conventions on how to declare private functions?

>> +static int sclp_service_call(CPUState *env, struct kvm_run *run, uint16_t ipbh0)
>> +{
>> +    uint32_t sccb;
>> +    uint64_t code;
>> +    int r = 0;
>> +
>> +    cpu_synchronize_state(env);
>> +    sccb = env->regs[ipbh0 & 0xf];
>> +    code = env->regs[(ipbh0 & 0xf0) >> 4];
>> +
>> +    dprintf("sclp(0x%x, 0x%lx)\n", sccb, code);
>> +
>> +    if (sccb & ~0x7ffffff8ul) {
>> +        fprintf(stderr, "KVM: invalid sccb address 0x%x\n", sccb);
>> +        r = -1;
>> +        goto out;
>> +    }
>> +
>> +    switch(code) {
>> +        case 0x00020001:
>> +        case 0x00120001:
> 
> What are those constants?

If I knew I'd have defined some more verbose constants. I just took them 1:1 from kuli.
Aurelien Jarno - Dec. 2, 2009, 8:12 a.m.
On Mon, Nov 30, 2009 at 11:25:03PM +0100, Alexander Graf wrote:
> 
> On 30.11.2009, at 19:18, Aurelien Jarno wrote:
> 
> > On Thu, Nov 26, 2009 at 02:23:13PM +0100, Alexander Graf wrote:
> >> S390x was one of the first platforms that received support for KVM back in the
> >> day. Unfortunately until now there hasn't been a qemu implementation that would
> >> enable users to actually run guests.
> >> 
> >> So let's include support for KVM S390x in qemu!
> > 
> > Please find the comments below.
> > 
> >> Signed-off-by: Alexander Graf <agraf@suse.de>
> >> ---
> >> configure          |    4 +-
> >> target-s390x/kvm.c |  463 ++++++++++++++++++++++++++++++++++++++++++++++++++++
> >> 2 files changed, 466 insertions(+), 1 deletions(-)
> >> create mode 100644 target-s390x/kvm.c
> >> 
> >> diff --git a/configure b/configure
> >> index b616e1a..cf466ec 100755
> >> --- a/configure
> >> +++ b/configure
> >> @@ -1348,6 +1348,8 @@ EOF
> >>             kvm_cflags="$kvm_cflags -I$kerneldir/arch/x86/include"
> >> 	elif test "$cpu" = "ppc" -a -d "$kerneldir/arch/powerpc/include" ; then
> >> 	    kvm_cflags="$kvm_cflags -I$kerneldir/arch/powerpc/include"
> >> +	elif test "$cpu" = "s390x" -a -d "$kerneldir/arch/s390/include" ; then
> >> +	    kvm_cflags="$kvm_cflags -I$kerneldir/arch/s390/include"
> >>         elif test -d "$kerneldir/arch/$cpu/include" ; then
> >>             kvm_cflags="$kvm_cflags -I$kerneldir/arch/$cpu/include"
> >>       fi
> >> @@ -2360,7 +2362,7 @@ case "$target_arch2" in
> >>     fi
> >> esac
> >> case "$target_arch2" in
> >> -  i386|x86_64|ppcemb|ppc|ppc64)
> >> +  i386|x86_64|ppcemb|ppc|ppc64|s390x)
> >>     # Make sure the target and host cpus are compatible
> >>     if test "$kvm" = "yes" -a "$target_softmmu" = "yes" -a \
> >>       \( "$target_arch2" = "$cpu" -o \
> >> diff --git a/target-s390x/kvm.c b/target-s390x/kvm.c
> >> new file mode 100644
> >> index 0000000..d477664
> >> --- /dev/null
> >> +++ b/target-s390x/kvm.c
> >> @@ -0,0 +1,463 @@
> >> +/*
> >> + * QEMU S390x KVM implementation
> >> + *
> >> + * Copyright (c) 2009 Alexander Graf <agraf@suse.de>
> >> + *
> >> + * This library is free software; you can redistribute it and/or
> >> + * modify it under the terms of the GNU Lesser General Public
> >> + * License as published by the Free Software Foundation; either
> >> + * version 2 of the License, or (at your option) any later version.
> >> + *
> >> + * This library is distributed in the hope that it will be useful,
> >> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> >> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> >> + * Lesser General Public License for more details.
> >> + *
> >> + * You should have received a copy of the GNU Lesser General Public
> >> + * License along with this library; if not, see <http://www.gnu.org/licenses/>.
> >> + */
> >> +
> >> +#include <sys/types.h>
> >> +#include <sys/ioctl.h>
> >> +#include <sys/mman.h>
> >> +
> >> +#include <linux/kvm.h>
> >> +#include <asm/ptrace.h>
> >> +
> >> +#include "qemu-common.h"
> >> +#include "qemu-timer.h"
> >> +#include "sysemu.h"
> >> +#include "kvm.h"
> >> +#include "cpu.h"
> >> +#include "device_tree.h"
> >> +
> >> +/* #define DEBUG_KVM */
> >> +
> >> +#ifdef DEBUG_KVM
> >> +#define dprintf(fmt, ...) \
> >> +    do { fprintf(stderr, fmt, ## __VA_ARGS__); } while (0)
> >> +#else
> >> +#define dprintf(fmt, ...) \
> >> +    do { } while (0)
> >> +#endif
> >> +
> >> +#define IPA0_DIAG               0x8300
> >> +#define IPA0_SIGP               0xae00
> >> +#define IPA0_PRIV               0xb200
> >> +
> >> +#define PRIV_SCLP_CALL          0x20
> >> +#define DIAG_KVM_HYPERCALL      0x500
> >> +#define DIAG_KVM_BREAKPOINT     0x501
> >> +
> >> +#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
> >> +
> >> +#define ICPT_INSTRUCTION        0x04
> >> +#define ICPT_WAITPSW            0x1c
> >> +#define ICPT_SOFT_INTERCEPT     0x24
> >> +#define ICPT_CPU_STOP           0x28
> >> +#define ICPT_IO                 0x40
> >> +
> >> +#define SIGP_RESTART            0x06
> >> +#define SIGP_INITIAL_CPU_RESET  0x0b
> >> +#define SIGP_STORE_STATUS_ADDR  0x0e
> >> +#define SIGP_SET_ARCH           0x12
> >> +
> >> +
> >> +int kvm_arch_init(KVMState *s, int smp_cpus)
> >> +{
> >> +    return 0;
> >> +}
> >> +
> >> +int kvm_arch_init_vcpu(CPUState *env)
> >> +{
> >> +    int ret = 0;
> >> +
> >> +    if (kvm_vcpu_ioctl(env, KVM_S390_INITIAL_RESET, NULL) < 0)
> >> +        perror("cannot init reset vcpu");
> > 
> > coding style.
> > 
> >> +
> >> +    return ret;
> >> +}
> >> +
> >> +void kvm_arch_reset_vcpu(CPUState *env)
> >> +{
> >> +}
> > 
> > Is there really nothing to do? If some code has to be added later, it
> > may be worth adding a comment.
> 
> As you have probably realized by now, all reset code is missing. In fact, we don't even ever call the qemu reset functions to actually do a reset.

Can you make it clean with an abort?

> >> +
> >> +static void _kvm_s390_interrupt(CPUState *env, int type, uint32_t parm, uint64_t parm64, int vm)
> >> +{
> > 
> > Why such a name starting with an underscore?
> 
> Because that's the internal function that gets used by the exported, properly named ones. Are there any conventions on how to declare private functions?

I don't think there is any convention, but I know malc always complains
about not introducing names starting with an underscore.

> >> +static int sclp_service_call(CPUState *env, struct kvm_run *run, uint16_t ipbh0)
> >> +{
> >> +    uint32_t sccb;
> >> +    uint64_t code;
> >> +    int r = 0;
> >> +
> >> +    cpu_synchronize_state(env);
> >> +    sccb = env->regs[ipbh0 & 0xf];
> >> +    code = env->regs[(ipbh0 & 0xf0) >> 4];
> >> +
> >> +    dprintf("sclp(0x%x, 0x%lx)\n", sccb, code);
> >> +
> >> +    if (sccb & ~0x7ffffff8ul) {
> >> +        fprintf(stderr, "KVM: invalid sccb address 0x%x\n", sccb);
> >> +        r = -1;
> >> +        goto out;
> >> +    }
> >> +
> >> +    switch(code) {
> >> +        case 0x00020001:
> >> +        case 0x00120001:
> > 
> > What are those constants?
> 
> If I knew I'd have defined some more verbose constants. I just took them 1:1 from kuli.
Alexander Graf - Dec. 2, 2009, 8:28 a.m.
On 02.12.2009, at 09:12, Aurelien Jarno wrote:

> On Mon, Nov 30, 2009 at 11:25:03PM +0100, Alexander Graf wrote:
>> 
>> On 30.11.2009, at 19:18, Aurelien Jarno wrote:
>> 
>>> On Thu, Nov 26, 2009 at 02:23:13PM +0100, Alexander Graf wrote:
>>>> S390x was one of the first platforms that received support for KVM back in the
>>>> day. Unfortunately until now there hasn't been a qemu implementation that would
>>>> enable users to actually run guests.
>>>> 
>>>> So let's include support for KVM S390x in qemu!
>>> 
>>> Please find the comments below.
>>> 
>>>> Signed-off-by: Alexander Graf <agraf@suse.de>
>>>> ---
>>>> configure          |    4 +-
>>>> target-s390x/kvm.c |  463 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>> 2 files changed, 466 insertions(+), 1 deletions(-)
>>>> create mode 100644 target-s390x/kvm.c
>>>> 
>>>> diff --git a/configure b/configure
>>>> index b616e1a..cf466ec 100755
>>>> --- a/configure
>>>> +++ b/configure
>>>> @@ -1348,6 +1348,8 @@ EOF
>>>>            kvm_cflags="$kvm_cflags -I$kerneldir/arch/x86/include"
>>>> 	elif test "$cpu" = "ppc" -a -d "$kerneldir/arch/powerpc/include" ; then
>>>> 	    kvm_cflags="$kvm_cflags -I$kerneldir/arch/powerpc/include"
>>>> +	elif test "$cpu" = "s390x" -a -d "$kerneldir/arch/s390/include" ; then
>>>> +	    kvm_cflags="$kvm_cflags -I$kerneldir/arch/s390/include"
>>>>        elif test -d "$kerneldir/arch/$cpu/include" ; then
>>>>            kvm_cflags="$kvm_cflags -I$kerneldir/arch/$cpu/include"
>>>>      fi
>>>> @@ -2360,7 +2362,7 @@ case "$target_arch2" in
>>>>    fi
>>>> esac
>>>> case "$target_arch2" in
>>>> -  i386|x86_64|ppcemb|ppc|ppc64)
>>>> +  i386|x86_64|ppcemb|ppc|ppc64|s390x)
>>>>    # Make sure the target and host cpus are compatible
>>>>    if test "$kvm" = "yes" -a "$target_softmmu" = "yes" -a \
>>>>      \( "$target_arch2" = "$cpu" -o \
>>>> diff --git a/target-s390x/kvm.c b/target-s390x/kvm.c
>>>> new file mode 100644
>>>> index 0000000..d477664
>>>> --- /dev/null
>>>> +++ b/target-s390x/kvm.c
>>>> @@ -0,0 +1,463 @@
>>>> +/*
>>>> + * QEMU S390x KVM implementation
>>>> + *
>>>> + * Copyright (c) 2009 Alexander Graf <agraf@suse.de>
>>>> + *
>>>> + * This library is free software; you can redistribute it and/or
>>>> + * modify it under the terms of the GNU Lesser General Public
>>>> + * License as published by the Free Software Foundation; either
>>>> + * version 2 of the License, or (at your option) any later version.
>>>> + *
>>>> + * This library is distributed in the hope that it will be useful,
>>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
>>>> + * Lesser General Public License for more details.
>>>> + *
>>>> + * You should have received a copy of the GNU Lesser General Public
>>>> + * License along with this library; if not, see <http://www.gnu.org/licenses/>.
>>>> + */
>>>> +
>>>> +#include <sys/types.h>
>>>> +#include <sys/ioctl.h>
>>>> +#include <sys/mman.h>
>>>> +
>>>> +#include <linux/kvm.h>
>>>> +#include <asm/ptrace.h>
>>>> +
>>>> +#include "qemu-common.h"
>>>> +#include "qemu-timer.h"
>>>> +#include "sysemu.h"
>>>> +#include "kvm.h"
>>>> +#include "cpu.h"
>>>> +#include "device_tree.h"
>>>> +
>>>> +/* #define DEBUG_KVM */
>>>> +
>>>> +#ifdef DEBUG_KVM
>>>> +#define dprintf(fmt, ...) \
>>>> +    do { fprintf(stderr, fmt, ## __VA_ARGS__); } while (0)
>>>> +#else
>>>> +#define dprintf(fmt, ...) \
>>>> +    do { } while (0)
>>>> +#endif
>>>> +
>>>> +#define IPA0_DIAG               0x8300
>>>> +#define IPA0_SIGP               0xae00
>>>> +#define IPA0_PRIV               0xb200
>>>> +
>>>> +#define PRIV_SCLP_CALL          0x20
>>>> +#define DIAG_KVM_HYPERCALL      0x500
>>>> +#define DIAG_KVM_BREAKPOINT     0x501
>>>> +
>>>> +#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
>>>> +
>>>> +#define ICPT_INSTRUCTION        0x04
>>>> +#define ICPT_WAITPSW            0x1c
>>>> +#define ICPT_SOFT_INTERCEPT     0x24
>>>> +#define ICPT_CPU_STOP           0x28
>>>> +#define ICPT_IO                 0x40
>>>> +
>>>> +#define SIGP_RESTART            0x06
>>>> +#define SIGP_INITIAL_CPU_RESET  0x0b
>>>> +#define SIGP_STORE_STATUS_ADDR  0x0e
>>>> +#define SIGP_SET_ARCH           0x12
>>>> +
>>>> +
>>>> +int kvm_arch_init(KVMState *s, int smp_cpus)
>>>> +{
>>>> +    return 0;
>>>> +}
>>>> +
>>>> +int kvm_arch_init_vcpu(CPUState *env)
>>>> +{
>>>> +    int ret = 0;
>>>> +
>>>> +    if (kvm_vcpu_ioctl(env, KVM_S390_INITIAL_RESET, NULL) < 0)
>>>> +        perror("cannot init reset vcpu");
>>> 
>>> coding style.
>>> 
>>>> +
>>>> +    return ret;
>>>> +}
>>>> +
>>>> +void kvm_arch_reset_vcpu(CPUState *env)
>>>> +{
>>>> +}
>>> 
>>> Is there really nothing to do? If some code has to be added later, it
>>> may be worth adding a comment.
>> 
>> As you have probably realized by now, all reset code is missing. In fact, we don't even ever call the qemu reset functions to actually do a reset.
> 
> Can you make it clean with an abort?
> 
>>>> +
>>>> +static void _kvm_s390_interrupt(CPUState *env, int type, uint32_t parm, uint64_t parm64, int vm)
>>>> +{
>>> 
>>> Why such a name starting with an underscore?
>> 
>> Because that's the internal function that gets used by the exported, properly named ones. Are there any conventions on how to declare private functions?
> 
> I don't think there is any convention, but I know malc always complains
> about not introducing names starting with an underscore.

Hm - I just wanted to clearly show that this is an internal API, nobody should really have to call directly. But I'm open for other naming suggestions.

Alex
malc - Dec. 2, 2009, 8:42 a.m.
On Wed, 2 Dec 2009, Alexander Graf wrote:

> 
> On 02.12.2009, at 09:12, Aurelien Jarno wrote:
> 
> > On Mon, Nov 30, 2009 at 11:25:03PM +0100, Alexander Graf wrote:
> >> 
> >> On 30.11.2009, at 19:18, Aurelien Jarno wrote:
> >> 
> >>> On Thu, Nov 26, 2009 at 02:23:13PM +0100, Alexander Graf wrote:
> >>>> S390x was one of the first platforms that received support for KVM back in the
> >>>> day. Unfortunately until now there hasn't been a qemu implementation that would
> >>>> enable users to actually run guests.
> >>>> 
> >>>> So let's include support for KVM S390x in qemu!
> >>> 
> >>> Please find the comments below.
> >>> 
> >>>> Signed-off-by: Alexander Graf <agraf@suse.de>
> >>>> ---
> >>>> configure          |    4 +-
> >>>> target-s390x/kvm.c |  463 ++++++++++++++++++++++++++++++++++++++++++++++++++++
> >>>> 2 files changed, 466 insertions(+), 1 deletions(-)
> >>>> create mode 100644 target-s390x/kvm.c
> >>>> 
> >>>> diff --git a/configure b/configure
> >>>> index b616e1a..cf466ec 100755
> >>>> --- a/configure
> >>>> +++ b/configure
> >>>> @@ -1348,6 +1348,8 @@ EOF
> >>>>            kvm_cflags="$kvm_cflags -I$kerneldir/arch/x86/include"
> >>>> 	elif test "$cpu" = "ppc" -a -d "$kerneldir/arch/powerpc/include" ; then
> >>>> 	    kvm_cflags="$kvm_cflags -I$kerneldir/arch/powerpc/include"
> >>>> +	elif test "$cpu" = "s390x" -a -d "$kerneldir/arch/s390/include" ; then
> >>>> +	    kvm_cflags="$kvm_cflags -I$kerneldir/arch/s390/include"
> >>>>        elif test -d "$kerneldir/arch/$cpu/include" ; then
> >>>>            kvm_cflags="$kvm_cflags -I$kerneldir/arch/$cpu/include"
> >>>>      fi
> >>>> @@ -2360,7 +2362,7 @@ case "$target_arch2" in
> >>>>    fi
> >>>> esac
> >>>> case "$target_arch2" in
> >>>> -  i386|x86_64|ppcemb|ppc|ppc64)
> >>>> +  i386|x86_64|ppcemb|ppc|ppc64|s390x)
> >>>>    # Make sure the target and host cpus are compatible
> >>>>    if test "$kvm" = "yes" -a "$target_softmmu" = "yes" -a \
> >>>>      \( "$target_arch2" = "$cpu" -o \
> >>>> diff --git a/target-s390x/kvm.c b/target-s390x/kvm.c
> >>>> new file mode 100644
> >>>> index 0000000..d477664
> >>>> --- /dev/null
> >>>> +++ b/target-s390x/kvm.c
> >>>> @@ -0,0 +1,463 @@
> >>>> +/*
> >>>> + * QEMU S390x KVM implementation
> >>>> + *
> >>>> + * Copyright (c) 2009 Alexander Graf <agraf@suse.de>
> >>>> + *
> >>>> + * This library is free software; you can redistribute it and/or
> >>>> + * modify it under the terms of the GNU Lesser General Public
> >>>> + * License as published by the Free Software Foundation; either
> >>>> + * version 2 of the License, or (at your option) any later version.
> >>>> + *
> >>>> + * This library is distributed in the hope that it will be useful,
> >>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> >>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> >>>> + * Lesser General Public License for more details.
> >>>> + *
> >>>> + * You should have received a copy of the GNU Lesser General Public
> >>>> + * License along with this library; if not, see <http://www.gnu.org/licenses/>.
> >>>> + */
> >>>> +
> >>>> +#include <sys/types.h>
> >>>> +#include <sys/ioctl.h>
> >>>> +#include <sys/mman.h>
> >>>> +
> >>>> +#include <linux/kvm.h>
> >>>> +#include <asm/ptrace.h>
> >>>> +
> >>>> +#include "qemu-common.h"
> >>>> +#include "qemu-timer.h"
> >>>> +#include "sysemu.h"
> >>>> +#include "kvm.h"
> >>>> +#include "cpu.h"
> >>>> +#include "device_tree.h"
> >>>> +
> >>>> +/* #define DEBUG_KVM */
> >>>> +
> >>>> +#ifdef DEBUG_KVM
> >>>> +#define dprintf(fmt, ...) \
> >>>> +    do { fprintf(stderr, fmt, ## __VA_ARGS__); } while (0)
> >>>> +#else
> >>>> +#define dprintf(fmt, ...) \
> >>>> +    do { } while (0)
> >>>> +#endif
> >>>> +
> >>>> +#define IPA0_DIAG               0x8300
> >>>> +#define IPA0_SIGP               0xae00
> >>>> +#define IPA0_PRIV               0xb200
> >>>> +
> >>>> +#define PRIV_SCLP_CALL          0x20
> >>>> +#define DIAG_KVM_HYPERCALL      0x500
> >>>> +#define DIAG_KVM_BREAKPOINT     0x501
> >>>> +
> >>>> +#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
> >>>> +
> >>>> +#define ICPT_INSTRUCTION        0x04
> >>>> +#define ICPT_WAITPSW            0x1c
> >>>> +#define ICPT_SOFT_INTERCEPT     0x24
> >>>> +#define ICPT_CPU_STOP           0x28
> >>>> +#define ICPT_IO                 0x40
> >>>> +
> >>>> +#define SIGP_RESTART            0x06
> >>>> +#define SIGP_INITIAL_CPU_RESET  0x0b
> >>>> +#define SIGP_STORE_STATUS_ADDR  0x0e
> >>>> +#define SIGP_SET_ARCH           0x12
> >>>> +
> >>>> +
> >>>> +int kvm_arch_init(KVMState *s, int smp_cpus)
> >>>> +{
> >>>> +    return 0;
> >>>> +}
> >>>> +
> >>>> +int kvm_arch_init_vcpu(CPUState *env)
> >>>> +{
> >>>> +    int ret = 0;
> >>>> +
> >>>> +    if (kvm_vcpu_ioctl(env, KVM_S390_INITIAL_RESET, NULL) < 0)
> >>>> +        perror("cannot init reset vcpu");
> >>> 
> >>> coding style.
> >>> 
> >>>> +
> >>>> +    return ret;
> >>>> +}
> >>>> +
> >>>> +void kvm_arch_reset_vcpu(CPUState *env)
> >>>> +{
> >>>> +}
> >>> 
> >>> Is there really nothing to do? If some code has to be added later, it
> >>> may be worth adding a comment.
> >> 
> >> As you have probably realized by now, all reset code is missing. In fact, we don't even ever call the qemu reset functions to actually do a reset.
> > 
> > Can you make it clean with an abort?
> > 
> >>>> +
> >>>> +static void _kvm_s390_interrupt(CPUState *env, int type, uint32_t parm, uint64_t parm64, int vm)
> >>>> +{
> >>> 
> >>> Why such a name starting with an underscore?
> >> 
> >> Because that's the internal function that gets used by the exported, properly named ones. Are there any conventions on how to declare private functions?
> > 
> > I don't think there is any convention, but I know malc always complains
> > about not introducing names starting with an underscore.

Yeah he does.

> 
> Hm - I just wanted to clearly show that this is an internal API, nobody 
> should really have to call directly. But I'm open for other naming 
> suggestions.

Thing is, in 7.1.3#1 standard says (after explicitly reserving __ _[A-Z]
for any use):
         -- All  identifiers  that  begin  with  an  underscore are
            always reserved for use as identifiers with file  scope
            in both the ordinary and tag name spaces.

And i could never really understand (or recall/comprehend when asked
and being given an answer) what this entails. (Anyone?)

So i would go with something imaginative like internal_do_not_use_kvm*,
but that's just me. You can go wild here, leading underscore doesn't look
attractive though.
Alexander Graf - Dec. 2, 2009, 8:47 a.m.
On 02.12.2009, at 09:42, malc wrote:

> On Wed, 2 Dec 2009, Alexander Graf wrote:
> 
>> 
>> On 02.12.2009, at 09:12, Aurelien Jarno wrote:
>> 
>>> On Mon, Nov 30, 2009 at 11:25:03PM +0100, Alexander Graf wrote:
>>>> 
>>>> On 30.11.2009, at 19:18, Aurelien Jarno wrote:
>>>> 
>>>>> On Thu, Nov 26, 2009 at 02:23:13PM +0100, Alexander Graf wrote:
>>>>>> S390x was one of the first platforms that received support for KVM back in the
>>>>>> day. Unfortunately until now there hasn't been a qemu implementation that would
>>>>>> enable users to actually run guests.
>>>>>> 
>>>>>> So let's include support for KVM S390x in qemu!
>>>>> 
>>>>> Please find the comments below.
>>>>> 
>>>>>> Signed-off-by: Alexander Graf <agraf@suse.de>
>>>>>> ---
>>>>>> configure          |    4 +-
>>>>>> target-s390x/kvm.c |  463 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>>> 2 files changed, 466 insertions(+), 1 deletions(-)
>>>>>> create mode 100644 target-s390x/kvm.c
>>>>>> 
>>>>>> diff --git a/configure b/configure
>>>>>> index b616e1a..cf466ec 100755
>>>>>> --- a/configure
>>>>>> +++ b/configure
>>>>>> @@ -1348,6 +1348,8 @@ EOF
>>>>>>           kvm_cflags="$kvm_cflags -I$kerneldir/arch/x86/include"
>>>>>> 	elif test "$cpu" = "ppc" -a -d "$kerneldir/arch/powerpc/include" ; then
>>>>>> 	    kvm_cflags="$kvm_cflags -I$kerneldir/arch/powerpc/include"
>>>>>> +	elif test "$cpu" = "s390x" -a -d "$kerneldir/arch/s390/include" ; then
>>>>>> +	    kvm_cflags="$kvm_cflags -I$kerneldir/arch/s390/include"
>>>>>>       elif test -d "$kerneldir/arch/$cpu/include" ; then
>>>>>>           kvm_cflags="$kvm_cflags -I$kerneldir/arch/$cpu/include"
>>>>>>     fi
>>>>>> @@ -2360,7 +2362,7 @@ case "$target_arch2" in
>>>>>>   fi
>>>>>> esac
>>>>>> case "$target_arch2" in
>>>>>> -  i386|x86_64|ppcemb|ppc|ppc64)
>>>>>> +  i386|x86_64|ppcemb|ppc|ppc64|s390x)
>>>>>>   # Make sure the target and host cpus are compatible
>>>>>>   if test "$kvm" = "yes" -a "$target_softmmu" = "yes" -a \
>>>>>>     \( "$target_arch2" = "$cpu" -o \
>>>>>> diff --git a/target-s390x/kvm.c b/target-s390x/kvm.c
>>>>>> new file mode 100644
>>>>>> index 0000000..d477664
>>>>>> --- /dev/null
>>>>>> +++ b/target-s390x/kvm.c
>>>>>> @@ -0,0 +1,463 @@
>>>>>> +/*
>>>>>> + * QEMU S390x KVM implementation
>>>>>> + *
>>>>>> + * Copyright (c) 2009 Alexander Graf <agraf@suse.de>
>>>>>> + *
>>>>>> + * This library is free software; you can redistribute it and/or
>>>>>> + * modify it under the terms of the GNU Lesser General Public
>>>>>> + * License as published by the Free Software Foundation; either
>>>>>> + * version 2 of the License, or (at your option) any later version.
>>>>>> + *
>>>>>> + * This library is distributed in the hope that it will be useful,
>>>>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>>>>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
>>>>>> + * Lesser General Public License for more details.
>>>>>> + *
>>>>>> + * You should have received a copy of the GNU Lesser General Public
>>>>>> + * License along with this library; if not, see <http://www.gnu.org/licenses/>.
>>>>>> + */
>>>>>> +
>>>>>> +#include <sys/types.h>
>>>>>> +#include <sys/ioctl.h>
>>>>>> +#include <sys/mman.h>
>>>>>> +
>>>>>> +#include <linux/kvm.h>
>>>>>> +#include <asm/ptrace.h>
>>>>>> +
>>>>>> +#include "qemu-common.h"
>>>>>> +#include "qemu-timer.h"
>>>>>> +#include "sysemu.h"
>>>>>> +#include "kvm.h"
>>>>>> +#include "cpu.h"
>>>>>> +#include "device_tree.h"
>>>>>> +
>>>>>> +/* #define DEBUG_KVM */
>>>>>> +
>>>>>> +#ifdef DEBUG_KVM
>>>>>> +#define dprintf(fmt, ...) \
>>>>>> +    do { fprintf(stderr, fmt, ## __VA_ARGS__); } while (0)
>>>>>> +#else
>>>>>> +#define dprintf(fmt, ...) \
>>>>>> +    do { } while (0)
>>>>>> +#endif
>>>>>> +
>>>>>> +#define IPA0_DIAG               0x8300
>>>>>> +#define IPA0_SIGP               0xae00
>>>>>> +#define IPA0_PRIV               0xb200
>>>>>> +
>>>>>> +#define PRIV_SCLP_CALL          0x20
>>>>>> +#define DIAG_KVM_HYPERCALL      0x500
>>>>>> +#define DIAG_KVM_BREAKPOINT     0x501
>>>>>> +
>>>>>> +#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
>>>>>> +
>>>>>> +#define ICPT_INSTRUCTION        0x04
>>>>>> +#define ICPT_WAITPSW            0x1c
>>>>>> +#define ICPT_SOFT_INTERCEPT     0x24
>>>>>> +#define ICPT_CPU_STOP           0x28
>>>>>> +#define ICPT_IO                 0x40
>>>>>> +
>>>>>> +#define SIGP_RESTART            0x06
>>>>>> +#define SIGP_INITIAL_CPU_RESET  0x0b
>>>>>> +#define SIGP_STORE_STATUS_ADDR  0x0e
>>>>>> +#define SIGP_SET_ARCH           0x12
>>>>>> +
>>>>>> +
>>>>>> +int kvm_arch_init(KVMState *s, int smp_cpus)
>>>>>> +{
>>>>>> +    return 0;
>>>>>> +}
>>>>>> +
>>>>>> +int kvm_arch_init_vcpu(CPUState *env)
>>>>>> +{
>>>>>> +    int ret = 0;
>>>>>> +
>>>>>> +    if (kvm_vcpu_ioctl(env, KVM_S390_INITIAL_RESET, NULL) < 0)
>>>>>> +        perror("cannot init reset vcpu");
>>>>> 
>>>>> coding style.
>>>>> 
>>>>>> +
>>>>>> +    return ret;
>>>>>> +}
>>>>>> +
>>>>>> +void kvm_arch_reset_vcpu(CPUState *env)
>>>>>> +{
>>>>>> +}
>>>>> 
>>>>> Is there really nothing to do? If some code has to be added later, it
>>>>> may be worth adding a comment.
>>>> 
>>>> As you have probably realized by now, all reset code is missing. In fact, we don't even ever call the qemu reset functions to actually do a reset.
>>> 
>>> Can you make it clean with an abort?
>>> 
>>>>>> +
>>>>>> +static void _kvm_s390_interrupt(CPUState *env, int type, uint32_t parm, uint64_t parm64, int vm)
>>>>>> +{
>>>>> 
>>>>> Why such a name starting with an underscore?
>>>> 
>>>> Because that's the internal function that gets used by the exported, properly named ones. Are there any conventions on how to declare private functions?
>>> 
>>> I don't think there is any convention, but I know malc always complains
>>> about not introducing names starting with an underscore.
> 
> Yeah he does.
> 
>> 
>> Hm - I just wanted to clearly show that this is an internal API, nobody 
>> should really have to call directly. But I'm open for other naming 
>> suggestions.
> 
> Thing is, in 7.1.3#1 standard says (after explicitly reserving __ _[A-Z]
> for any use):
>         -- All  identifiers  that  begin  with  an  underscore are
>            always reserved for use as identifiers with file  scope
>            in both the ordinary and tag name spaces.
> 
> And i could never really understand (or recall/comprehend when asked
> and being given an answer) what this entails. (Anyone?)

I don't get the second part, but the first one clearly says "If you use a function beginning underscore, only use it within the file you're declaring it at".

> So i would go with something imaginative like internal_do_not_use_kvm*,
> but that's just me. You can go wild here, leading underscore doesn't look
> attractive though.

Well I could have gone with kvm_s390_interrupt_generic or kvm_s390_interrupt_internal. But I wanted to have a function name that doesn't exceed the 80 character limit all by itself ;-).


Alex
malc - Dec. 2, 2009, 8:59 a.m.
On Wed, 2 Dec 2009, Alexander Graf wrote:

> 
> On 02.12.2009, at 09:42, malc wrote:
> 
> > On Wed, 2 Dec 2009, Alexander Graf wrote:

[..snip..]

> >>>>> 
> >>>>> Why such a name starting with an underscore?
> >>>> 
> >>>> Because that's the internal function that gets used by the exported, properly named ones. Are there any conventions on how to declare private functions?
> >>> 
> >>> I don't think there is any convention, but I know malc always complains
> >>> about not introducing names starting with an underscore.
> > 
> > Yeah he does.
> > 
> >> 
> >> Hm - I just wanted to clearly show that this is an internal API, nobody 
> >> should really have to call directly. But I'm open for other naming 
> >> suggestions.
> > 
> > Thing is, in 7.1.3#1 standard says (after explicitly reserving __ _[A-Z]
> > for any use):
> >         -- All  identifiers  that  begin  with  an  underscore are
> >            always reserved for use as identifiers with file  scope
> >            in both the ordinary and tag name spaces.
> > 
> > And i could never really understand (or recall/comprehend when asked
> > and being given an answer) what this entails. (Anyone?)
> 
> I don't get the second part, but the first one clearly says "If you
> use a function beginning underscore, only use it within the file
> you're declaring it at".

6.2.3 describes name spaces.

FWIW here's my interpretation: those names are reserved by implementation
for use in oridnary and tag name spaces, so keep out, feel free to use them
in other name spaces.

Perhaps Mans Rullgard, if he still reads this that is, can enlighten
us.  Unfortuantelly i do not have write access to Usenet anymore so
can not ask this on comp.std.c on my own.

> 
> > So i would go with something imaginative like internal_do_not_use_kvm*,
> > but that's just me. You can go wild here, leading underscore doesn't look
> > attractive though.
> 
> Well I could have gone with kvm_s390_interrupt_generic or
> kvm_s390_interrupt_internal. But I wanted to have a function name
> that doesn't exceed the 80 character limit all by itself ;-).

Oh c'mon those are less than 30 chars :)
Markus Armbruster - Dec. 2, 2009, 9:36 a.m.
malc <av1474@comtv.ru> writes:

> On Wed, 2 Dec 2009, Alexander Graf wrote:
>
>> 
>> On 02.12.2009, at 09:12, Aurelien Jarno wrote:
>> 
>> > On Mon, Nov 30, 2009 at 11:25:03PM +0100, Alexander Graf wrote:
>> >> 
>> >> On 30.11.2009, at 19:18, Aurelien Jarno wrote:
>> >> 
>> >>> On Thu, Nov 26, 2009 at 02:23:13PM +0100, Alexander Graf wrote:
[...]
>> >>>> +
>> >>>> +static void _kvm_s390_interrupt(CPUState *env, int type, uint32_t parm, uint64_t parm64, int vm)
>> >>>> +{
>> >>> 
>> >>> Why such a name starting with an underscore?
>> >> 
>> >> Because that's the internal function that gets used by the exported, properly named ones. Are there any conventions on how to declare private functions?
>> > 
>> > I don't think there is any convention, but I know malc always complains
>> > about not introducing names starting with an underscore.
>
> Yeah he does.
>
>> 
>> Hm - I just wanted to clearly show that this is an internal API, nobody 
>> should really have to call directly. But I'm open for other naming 
>> suggestions.
>
> Thing is, in 7.1.3#1 standard says (after explicitly reserving __ _[A-Z]
> for any use):
>          -- All  identifiers  that  begin  with  an  underscore are
>             always reserved for use as identifiers with file  scope
>             in both the ordinary and tag name spaces.
>
> And i could never really understand (or recall/comprehend when asked
> and being given an answer) what this entails. (Anyone?)

Later in 7.1.3:

    If the program declares or defines an identifier in a context in
    which it is reserved (other than as allowed by 7.1.4), or defines a
    reserved identifier as a macro name, the behavior is undefined.

This gives implementations of the standard (compiler + libc) license to
use reserved identifiers for their own purposes.  If they clash with the
user's identifiers, and things break, the user gets to keep the pieces.

Now, it's quite unlikely that _kvm_s390_interrupt() clashes with
anything in practice.  It does, however, set a bad example.

> So i would go with something imaginative like internal_do_not_use_kvm*,
> but that's just me. You can go wild here, leading underscore doesn't look
> attractive though.

Why not kvm_s390_interrupt_internal(), or even kvm_s390_interrupt_()?
malc - Dec. 2, 2009, 9:48 a.m.
On Wed, 2 Dec 2009, Markus Armbruster wrote:

> malc <av1474@comtv.ru> writes:
> 
> > On Wed, 2 Dec 2009, Alexander Graf wrote:
> >
> >> 
> >> On 02.12.2009, at 09:12, Aurelien Jarno wrote:
> >> 
> >> > On Mon, Nov 30, 2009 at 11:25:03PM +0100, Alexander Graf wrote:
> >> >> 
> >> >> On 30.11.2009, at 19:18, Aurelien Jarno wrote:
> >> >> 
> >> >>> On Thu, Nov 26, 2009 at 02:23:13PM +0100, Alexander Graf wrote:
> [...]
> >> >>>> +
> >> >>>> +static void _kvm_s390_interrupt(CPUState *env, int type, uint32_t parm, uint64_t parm64, int vm)
> >> >>>> +{
> >> >>> 
> >> >>> Why such a name starting with an underscore?
> >> >> 
> >> >> Because that's the internal function that gets used by the exported, properly named ones. Are there any conventions on how to declare private functions?
> >> > 
> >> > I don't think there is any convention, but I know malc always complains
> >> > about not introducing names starting with an underscore.
> >
> > Yeah he does.
> >
> >> 
> >> Hm - I just wanted to clearly show that this is an internal API, nobody 
> >> should really have to call directly. But I'm open for other naming 
> >> suggestions.
> >
> > Thing is, in 7.1.3#1 standard says (after explicitly reserving __ _[A-Z]
> > for any use):
> >          -- All  identifiers  that  begin  with  an  underscore are
> >             always reserved for use as identifiers with file  scope
> >             in both the ordinary and tag name spaces.
> >
> > And i could never really understand (or recall/comprehend when asked
> > and being given an answer) what this entails. (Anyone?)
> 
> Later in 7.1.3:
> 
>     If the program declares or defines an identifier in a context in
>     which it is reserved (other than as allowed by 7.1.4), or defines a
>     reserved identifier as a macro name, the behavior is undefined.
> 
> This gives implementations of the standard (compiler + libc) license to
> use reserved identifiers for their own purposes.  If they clash with the
> user's identifiers, and things break, the user gets to keep the pieces.

Yes, that's how i would interpret it too (as stated in another message)

> 
> Now, it's quite unlikely that _kvm_s390_interrupt() clashes with
> anything in practice.  It does, however, set a bad example.

Indeed.

> 
> > So i would go with something imaginative like internal_do_not_use_kvm*,
> > but that's just me. You can go wild here, leading underscore doesn't look
> > attractive though.
> 
> Why not kvm_s390_interrupt_internal(), or even kvm_s390_interrupt_()?
>

Patch

diff --git a/configure b/configure
index b616e1a..cf466ec 100755
--- a/configure
+++ b/configure
@@ -1348,6 +1348,8 @@  EOF
             kvm_cflags="$kvm_cflags -I$kerneldir/arch/x86/include"
 	elif test "$cpu" = "ppc" -a -d "$kerneldir/arch/powerpc/include" ; then
 	    kvm_cflags="$kvm_cflags -I$kerneldir/arch/powerpc/include"
+	elif test "$cpu" = "s390x" -a -d "$kerneldir/arch/s390/include" ; then
+	    kvm_cflags="$kvm_cflags -I$kerneldir/arch/s390/include"
         elif test -d "$kerneldir/arch/$cpu/include" ; then
             kvm_cflags="$kvm_cflags -I$kerneldir/arch/$cpu/include"
       fi
@@ -2360,7 +2362,7 @@  case "$target_arch2" in
     fi
 esac
 case "$target_arch2" in
-  i386|x86_64|ppcemb|ppc|ppc64)
+  i386|x86_64|ppcemb|ppc|ppc64|s390x)
     # Make sure the target and host cpus are compatible
     if test "$kvm" = "yes" -a "$target_softmmu" = "yes" -a \
       \( "$target_arch2" = "$cpu" -o \
diff --git a/target-s390x/kvm.c b/target-s390x/kvm.c
new file mode 100644
index 0000000..d477664
--- /dev/null
+++ b/target-s390x/kvm.c
@@ -0,0 +1,463 @@ 
+/*
+ * QEMU S390x KVM implementation
+ *
+ * Copyright (c) 2009 Alexander Graf <agraf@suse.de>
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <sys/types.h>
+#include <sys/ioctl.h>
+#include <sys/mman.h>
+
+#include <linux/kvm.h>
+#include <asm/ptrace.h>
+
+#include "qemu-common.h"
+#include "qemu-timer.h"
+#include "sysemu.h"
+#include "kvm.h"
+#include "cpu.h"
+#include "device_tree.h"
+
+/* #define DEBUG_KVM */
+
+#ifdef DEBUG_KVM
+#define dprintf(fmt, ...) \
+    do { fprintf(stderr, fmt, ## __VA_ARGS__); } while (0)
+#else
+#define dprintf(fmt, ...) \
+    do { } while (0)
+#endif
+
+#define IPA0_DIAG               0x8300
+#define IPA0_SIGP               0xae00
+#define IPA0_PRIV               0xb200
+
+#define PRIV_SCLP_CALL          0x20
+#define DIAG_KVM_HYPERCALL      0x500
+#define DIAG_KVM_BREAKPOINT     0x501
+
+#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
+
+#define ICPT_INSTRUCTION        0x04
+#define ICPT_WAITPSW            0x1c
+#define ICPT_SOFT_INTERCEPT     0x24
+#define ICPT_CPU_STOP           0x28
+#define ICPT_IO                 0x40
+
+#define SIGP_RESTART            0x06
+#define SIGP_INITIAL_CPU_RESET  0x0b
+#define SIGP_STORE_STATUS_ADDR  0x0e
+#define SIGP_SET_ARCH           0x12
+
+
+int kvm_arch_init(KVMState *s, int smp_cpus)
+{
+    return 0;
+}
+
+int kvm_arch_init_vcpu(CPUState *env)
+{
+    int ret = 0;
+
+    if (kvm_vcpu_ioctl(env, KVM_S390_INITIAL_RESET, NULL) < 0)
+        perror("cannot init reset vcpu");
+
+    return ret;
+}
+
+void kvm_arch_reset_vcpu(CPUState *env)
+{
+}
+
+int kvm_arch_put_registers(CPUState *env)
+{
+    struct kvm_regs regs;
+    int ret;
+    int i;
+
+    ret = kvm_vcpu_ioctl(env, KVM_GET_REGS, &regs);
+    if (ret < 0)
+        return ret;
+
+    for (i = 0; i < 16; i++)
+        regs.gprs[i] = env->regs[i];
+
+    ret = kvm_vcpu_ioctl(env, KVM_SET_REGS, &regs);
+    if (ret < 0)
+        return ret;
+
+    env->kvm_run->psw_addr = env->psw.addr;
+    env->kvm_run->psw_mask = env->psw.mask;
+
+    return ret;
+}
+
+int kvm_arch_get_registers(CPUState *env)
+{
+    uint32_t ret;
+    struct kvm_regs regs;
+    int i;
+
+    ret = kvm_vcpu_ioctl(env, KVM_GET_REGS, &regs);
+    if (ret < 0)
+        return ret;
+
+    for (i = 0; i < 16; i++)
+        env->regs[i] = regs.gprs[i];
+
+    env->psw.addr = env->kvm_run->psw_addr;
+    env->psw.mask = env->kvm_run->psw_mask;
+
+    return 0;
+}
+
+int kvm_arch_insert_sw_breakpoint(CPUState *env, struct kvm_sw_breakpoint *bp)
+{
+    static const uint8_t diag_501[] = {0x83, 0x24, 0x05, 0x01};
+
+    if (cpu_memory_rw_debug(env, bp->pc, (uint8_t *)&bp->saved_insn, 4, 0) ||
+        cpu_memory_rw_debug(env, bp->pc, (uint8_t *)diag_501, 4, 1))
+        return -EINVAL;
+    return 0;
+}
+
+int kvm_arch_remove_sw_breakpoint(CPUState *env, struct kvm_sw_breakpoint *bp)
+{
+    uint8_t t[4];
+    static const uint8_t diag_501[] = {0x83, 0x24, 0x05, 0x01};
+
+    if (cpu_memory_rw_debug(env, bp->pc, t, 4, 0))
+        return -EINVAL;
+    if (memcmp(t, diag_501, 4))
+        return -EINVAL;
+    if (cpu_memory_rw_debug(env, bp->pc, (uint8_t *)&bp->saved_insn, 1, 1))
+        return -EINVAL;
+
+    return 0;
+}
+
+int kvm_arch_pre_run(CPUState *env, struct kvm_run *run)
+{
+    return 0;
+}
+
+int kvm_arch_post_run(CPUState *env, struct kvm_run *run)
+{
+    return 0;
+}
+
+static void _kvm_s390_interrupt(CPUState *env, int type, uint32_t parm, uint64_t parm64, int vm)
+{
+    struct kvm_s390_interrupt kvmint;
+    int r;
+
+    if (!env->kvm_state)
+        return;
+
+    env->halted = 0;
+    env->exception_index = 0;
+
+    kvmint.type = type;
+    kvmint.parm = parm;
+    kvmint.parm64 = parm64;
+
+    if (vm)
+        r = kvm_vm_ioctl(env->kvm_state, KVM_S390_INTERRUPT, &kvmint);
+    else 
+        r = kvm_vcpu_ioctl(env, KVM_S390_INTERRUPT, &kvmint);
+
+    if (r < 0) {
+        fprintf(stderr, "KVM failed to inject interrupt\n");
+        exit(1);
+    }
+}
+
+void kvm_s390_virtio_irq(CPUState *env, int config_change, uint64_t token)
+{
+    _kvm_s390_interrupt(env, KVM_S390_INT_VIRTIO, config_change, token, 1);
+}
+
+static void kvm_s390_interrupt(CPUState *env, int type, uint32_t code)
+{
+    _kvm_s390_interrupt(env, type, code, 0, 0);
+}
+
+static void enter_pgmcheck(CPUState *env, uint16_t code)
+{
+    kvm_s390_interrupt(env, KVM_S390_PROGRAM_INT, code);
+}
+
+static void setcc(CPUState *env, uint64_t cc)
+{
+    env->kvm_run->psw_mask &= ~(3ul << 44);
+    env->kvm_run->psw_mask |= (cc & 3) << 44;
+
+    env->psw.mask &= ~(3ul << 44);
+    env->psw.mask |= (cc & 3) << 44;
+}
+
+static int sclp_service_call(CPUState *env, struct kvm_run *run, uint16_t ipbh0)
+{
+    uint32_t sccb;
+    uint64_t code;
+    int r = 0;
+
+    cpu_synchronize_state(env);
+    sccb = env->regs[ipbh0 & 0xf];
+    code = env->regs[(ipbh0 & 0xf0) >> 4];
+
+    dprintf("sclp(0x%x, 0x%lx)\n", sccb, code);
+
+    if (sccb & ~0x7ffffff8ul) {
+        fprintf(stderr, "KVM: invalid sccb address 0x%x\n", sccb);
+        r = -1;
+        goto out;
+    }
+
+    switch(code) {
+        case 0x00020001:
+        case 0x00120001:
+            stw_phys(sccb + SCP_MEM_CODE, ram_size >> 20);
+            stb_phys(sccb + SCP_INCREMENT, 1);
+            stw_phys(sccb + SCP_RESPONSE_CODE, 0x10);
+            setcc(env, 0);
+
+            _kvm_s390_interrupt(env, KVM_S390_INT_SERVICE, sccb & ~3, 0, 1);
+            break;
+        default:
+            dprintf("KVM: invalid sclp call 0x%x / 0x%lx\n", sccb, code);
+            r = -1;
+            break;
+    }
+
+out:
+    if (r < 0)
+        setcc(env, 3);
+    return 0;
+}
+
+static int handle_priv(CPUState *env, struct kvm_run *run, uint8_t ipa1)
+{
+    int r = 0;
+    uint16_t ipbh0 = (run->s390_sieic.ipb & 0xffff0000) >> 16;
+
+    dprintf("KVM: PRIV: %d\n", ipa1);
+    switch (ipa1) {
+        case PRIV_SCLP_CALL:
+            r = sclp_service_call(env, run, ipbh0);
+            break;
+        default:
+            dprintf("KVM: unknown PRIV: 0x%x\n", ipa1);
+            r = -1;
+            break;
+    }
+
+    return r;
+}
+
+static int handle_hypercall(CPUState *env, struct kvm_run *run)
+{
+    int r;
+
+    cpu_synchronize_state(env);
+    r = s390_virtio_hypercall(env);
+    kvm_arch_put_registers(env);
+
+    return r;
+}
+
+static int handle_diag(CPUState *env, struct kvm_run *run, int ipb_code)
+{
+    int r = 0;
+
+    switch (ipb_code) {
+        case DIAG_KVM_HYPERCALL:
+            r = handle_hypercall(env, run);
+            break;
+        case DIAG_KVM_BREAKPOINT:
+            sleep(10);
+            break;
+        default:
+            dprintf("KVM: unknown DIAG: 0x%x\n", ipb_code);
+            r = -1;
+            break;
+    }
+
+    return r;
+}
+
+static int s390_cpu_restart(CPUState *env)
+{
+    kvm_s390_interrupt(env, KVM_S390_RESTART, 0);
+    env->halted = 0;
+    env->exception_index = 0;
+    qemu_cpu_kick(env);
+    fprintf(stderr, "DONE: SIGP cpu restart: %p\n", env);
+    return 0;
+}
+
+static int s390_store_status(CPUState *env, uint32_t parameter)
+{
+    /* XXX */
+    fprintf(stderr, "XXX SIGP store status\n");
+    return -1;
+}
+
+static int s390_cpu_initial_reset(CPUState *env)
+{
+    /* XXX */
+    fprintf(stderr, "XXX SIGP init\n");
+    return -1;
+}
+
+static int handle_sigp(CPUState *env, struct kvm_run *run, uint8_t ipa1)
+{
+    uint8_t order_code;
+    uint32_t parameter;
+    uint16_t cpu_addr;
+    uint8_t t;
+    int r = -1;
+    CPUState *target_env;
+
+    cpu_synchronize_state(env);
+
+    /* get order code */
+    order_code = run->s390_sieic.ipb >> 28;
+    if (order_code > 0)
+        order_code = env->regs[order_code];
+    order_code += (run->s390_sieic.ipb & 0x0fff0000) >> 16;
+
+    /* get parameters */
+    t = (ipa1 & 0xf0) >> 4;
+    if (!(t % 2))
+        t++;
+
+    parameter = env->regs[t] & 0x7ffffe00;
+    cpu_addr = env->regs[ipa1 & 0x0f];
+
+    target_env = s390_cpu_addr2state(cpu_addr);
+    if (!target_env)
+        goto out;
+
+    switch (order_code) {
+        case SIGP_RESTART:
+            r = s390_cpu_restart(target_env);
+            break;
+        case SIGP_STORE_STATUS_ADDR:
+            r = s390_store_status(target_env, parameter);
+            break;
+        case SIGP_SET_ARCH:
+            /* make the caller panic */
+            return -1;
+        case SIGP_INITIAL_CPU_RESET:
+            r = s390_cpu_initial_reset(target_env);
+            break;
+        default:
+            fprintf(stderr, "KVM: unknown SIGP: 0x%x\n", ipa1);
+            break;
+    }
+
+out:
+    setcc(env, r ? 3 : 0);
+    return 0;
+}
+
+static int handle_instruction(CPUState *env, struct kvm_run *run)
+{
+    unsigned int ipa0 = (run->s390_sieic.ipa & 0xff00);
+    uint8_t ipa1 = run->s390_sieic.ipa & 0x00ff;
+    int ipb_code = (run->s390_sieic.ipb & 0x0fff0000) >> 16;
+    int r = 0;
+
+    dprintf("handle_instruction 0x%x 0x%x\n", run->s390_sieic.ipa, run->s390_sieic.ipb);
+    switch (ipa0) {
+        case IPA0_PRIV:
+            r = handle_priv(env, run, ipa1);
+            break;
+        case IPA0_DIAG:
+            r = handle_diag(env, run, ipb_code);
+            break;
+        case IPA0_SIGP:
+            r = handle_sigp(env, run, ipa1);
+            break;
+    }
+
+    if (r < 0) {
+        enter_pgmcheck(env, 0x0001);
+    }
+    return r;
+}
+
+static int handle_intercept(CPUState *env)
+{
+    struct kvm_run *run = env->kvm_run;
+    int icpt_code = run->s390_sieic.icptcode;
+    int r = 0;
+
+    dprintf("intercept: 0x%x (at 0x%lx)\n", icpt_code, env->kvm_run->psw_addr);
+    switch (icpt_code) {
+        case ICPT_INSTRUCTION:
+            r = handle_instruction(env, run);
+            break;
+        case ICPT_WAITPSW:
+            /* XXX What to do on system shutdown? */
+            env->halted = 1;
+            env->exception_index = EXCP_HLT;
+            break;
+        case ICPT_SOFT_INTERCEPT:
+            fprintf(stderr, "KVM unimplemented icpt SOFT\n");
+            exit(1);
+            break;
+        case ICPT_CPU_STOP:
+            qemu_system_shutdown_request();
+            break;
+        case ICPT_IO:
+            fprintf(stderr, "KVM unimplemented icpt IO\n");
+            exit(1);
+            break;
+        default:
+            fprintf(stderr, "Unknown intercept code: %d\n", icpt_code);
+            exit(1);
+            break;
+    }
+
+    return r;
+}
+
+int kvm_arch_handle_exit(CPUState *env, struct kvm_run *run)
+{
+    int ret = 0;
+
+    switch (run->exit_reason) {
+        case KVM_EXIT_S390_SIEIC:
+            ret = handle_intercept(env);
+            break;
+        case KVM_EXIT_S390_RESET:
+            fprintf(stderr, "RESET not implemented\n");
+            exit(1);
+            break;
+        default:
+            fprintf(stderr, "Unknown KVM exit: %d\n", run->exit_reason);
+            break;
+    }
+
+    return ret;
+}