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

[Alexander Graf <agraf@suse.de>]

On 10/04/2011 03:47 PM, Christian Borntraeger wrote:
> Something like the following?
>
> s390: Fix cpu shutdown for KVM
>
> 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 patch implements
> the counting and shutdown handling for the kvm path in qemu.
>
> Signed-off-by: Christian Borntraeger<borntraeger@de.ibm.com>
> ---
>   hw/s390-virtio.c   |   21 +++++++++++++++++++++
>   target-s390x/cpu.h |   12 ++++++++++++
>   target-s390x/kvm.c |   21 +++++++++++++++++----
>   3 files changed, 50 insertions(+), 4 deletions(-)
>
> Index: b/hw/s390-virtio.c
> ===================================================================
> --- a/hw/s390-virtio.c
> +++ b/hw/s390-virtio.c
> @@ -130,6 +130,26 @@ 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 kvm_s390_add_running_cpu(CPUState *env)
> +{
> +    assert(env->halted == 0);
> +    s390_running_cpus++;
> +}
> +
> +unsigned kvm_s390_del_running_cpu(CPUState *env)
> +{
> +    assert(s390_running_cpus>= 1);
> +    return --s390_running_cpus;
> +}

Almost. It has nothing to do with KVM though. Please keep the 
abstraction intact!

> +
>   /* PC hardware initialisation */
>   static void s390_init(ram_addr_t my_ram_size,
>                         const char *boot_device,
> @@ -189,6 +209,7 @@ static void s390_init(ram_addr_t my_ram_
>
>       env->halted = 0;
>       env->exception_index = 0;
> +    kvm_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
> @@ -292,6 +292,8 @@ void kvm_s390_interrupt(CPUState *env, i
>   void kvm_s390_virtio_irq(CPUState *env, int config_change, uint64_t token);
>   void kvm_s390_interrupt_internal(CPUState *env, int type, uint32_t parm,
>                                    uint64_t parm64, int vm);
> +void kvm_s390_add_running_cpu(CPUState *env);
> +unsigned kvm_s390_del_running_cpu(CPUState *env);
>   #else
>   static inline void kvm_s390_interrupt(CPUState *env, int type, uint32_t code)
>   {
> @@ -307,6 +309,16 @@ static inline void kvm_s390_interrupt_in
>                                                  int vm)
>   {
>   }
> +
> +static inline void kvm_s390_add_running_cpu(CPUState *env)
> +{
> +}
> +
> +static inline unsigned kvm_s390_del_running_cpu(CPUState *env)
> +{
> +    return 0;
> +}

The functions are defined in s390-virtio.c and thus are available 
anywhere now. No need to special-case KVM anywhere.

> +
>   #endif
>   CPUState *s390_cpu_addr2state(uint16_t cpu_addr);
>
> Index: b/target-s390x/kvm.c
> ===================================================================
> --- a/target-s390x/kvm.c
> +++ b/target-s390x/kvm.c
> @@ -185,6 +185,12 @@ void kvm_s390_interrupt_internal(CPUStat
>           return;
>       }
>
> +    /*
> +     * We can only deliver interrupts to (interrupt) enabled CPUs.
> +     * We dont call kvm_s390_add_running_cpu here, since CPUs in enabled wait
> +     * will wait inside the kernel (no exit). Therefore, the targeted
> +     * CPUs was neither disabled waiting or stopped for qemu.
> +     */
>       env->halted = 0;
>       env->exception_index = -1;
>       qemu_cpu_kick(env);
> @@ -301,6 +307,7 @@ static int s390_cpu_restart(CPUState *en
>       kvm_s390_interrupt(env, KVM_S390_RESTART, 0);
>       env->halted = 0;
>       env->exception_index = -1;
> +    kvm_s390_add_running_cpu(env);

Hrm. Is add_running correct here? Maybe we should add a wrapper around 
the halted = 0 parts in s390-virtio.c and do the increase of the counter 
there? That way we can make sure that transitioning from 0 -> 0 doesn't 
increase the counter.


Alex