Re: [Qemu-devel] [PATCHv3] s390: Fix cpu shutdown for KVM

[Alexander Graf <agraf@suse.de>]

On 10/04/2011 05:20 PM, Christian Borntraeger wrote:
> On s390 a shutdown is the state of all CPUs being either stopped
> or disabled (for interrupts) waiting. We have to track the overall
> number of running CPUs to call the shutdown sequence accordingly.
> This patch implements the counting and shutdown handling for the
> kvm path in qemu.
> Lets also wrap changes to env->halted and env->exception_index.
>
> Signed-off-by: Christian Borntraeger<borntraeger@de.ibm.com>
> ---
>   hw/s390-virtio.c   |   32 ++++++++++++++++++++++++++++++--
>   target-s390x/cpu.h |    2 ++
>   target-s390x/kvm.c |   19 +++++++------------
>   3 files changed, 39 insertions(+), 14 deletions(-)
>
> Index: b/hw/s390-virtio.c
> ===================================================================
> --- a/hw/s390-virtio.c
> +++ b/hw/s390-virtio.c
> @@ -130,6 +130,34 @@ int s390_virtio_hypercall(CPUState *env,
>       return r;
>   }
>
> +/*
> + * The number of running CPUs. On s390 a shutdown is the state of all CPUs
> + * being either stopped or disabled (for interrupts) waiting. We have to
> + * track this number to call the shutdown sequence accordingly. This
> + * number is modified either on startup or while holding the big qemu lock.
> + */
> +static unsigned s390_running_cpus;
> +
> +void s390_add_running_cpu(CPUState *env)
> +{
> +    if (env->halted) {
> +        s390_running_cpus++;
> +        env->halted = 0;
> +        env->exception_index = -1;
> +    }
> +}
> +
> +unsigned s390_del_running_cpu(CPUState *env)
> +{
> +    if (env->halted == 0) {
> +        assert(s390_running_cpus>= 1);
> +        s390_running_cpus--;
> +        env->halted = 1;
> +        env->exception_index = EXCP_HLT;
> +    }
> +    return s390_running_cpus;
> +}
> +
>   /* PC hardware initialisation */
>   static void s390_init(ram_addr_t my_ram_size,
>                         const char *boot_device,
> @@ -187,8 +215,8 @@ static void s390_init(ram_addr_t my_ram_
>           tmp_env->storage_keys = storage_keys;
>       }
>
> -    env->halted = 0;
> -    env->exception_index = 0;
> +    /* One CPU has to run */
> +    s390_add_running_cpu(env);
>
>       if (kernel_filename) {
>           kernel_size = load_image(kernel_filename, qemu_get_ram_ptr(0));
> Index: b/target-s390x/cpu.h
> ===================================================================
> --- a/target-s390x/cpu.h
> +++ b/target-s390x/cpu.h
> @@ -309,6 +309,8 @@ static inline void kvm_s390_interrupt_in
>   }
>   #endif
>   CPUState *s390_cpu_addr2state(uint16_t cpu_addr);
> +void s390_add_running_cpu(CPUState *env);
> +unsigned s390_del_running_cpu(CPUState *env);
>
>   /* from s390-virtio-bus */
>   extern const target_phys_addr_t virtio_size;
> Index: b/target-s390x/kvm.c
> ===================================================================
> --- a/target-s390x/kvm.c
> +++ b/target-s390x/kvm.c
> @@ -185,8 +185,7 @@ void kvm_s390_interrupt_internal(CPUStat
>           return;
>       }
>
> -    env->halted = 0;
> -    env->exception_index = -1;
> +    s390_add_running_cpu(env);
>       qemu_cpu_kick(env);
>
>       kvmint.type = type;
> @@ -299,8 +298,7 @@ static int handle_diag(CPUState *env, st
>   static int s390_cpu_restart(CPUState *env)
>   {
>       kvm_s390_interrupt(env, KVM_S390_RESTART, 0);
> -    env->halted = 0;
> -    env->exception_index = -1;
> +    s390_add_running_cpu(env);
>       qemu_cpu_kick(env);
>       dprintf("DONE: SIGP cpu restart: %p\n", env);
>       return 0;
> @@ -425,17 +423,16 @@ static int handle_intercept(CPUState *en
>               r = handle_instruction(env, run);
>               break;
>           case ICPT_WAITPSW:
> -            /* XXX What to do on system shutdown? */
> -            env->halted = 1;
> -            env->exception_index = EXCP_HLT;
> +        case ICPT_CPU_STOP:
> +            if (s390_del_running_cpu(env) == 0) {
> +                qemu_system_shutdown_request();
> +            }
> +            r = EXCP_HALTED;
>               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);
> @@ -468,8 +465,6 @@ int kvm_arch_handle_exit(CPUState *env,
>
>       if (ret == 0) {
>           ret = EXCP_INTERRUPT;
> -    } else if (ret>  0) {
> -        ret = 0;

Hrm. Are you sure that this doesn't break anything? The rest looks good.


Alex