[v1,09/27] target/s390x: factor out handling of WAIT PSW into handle_wait()

This will now also detect crashes under TCG. We can directly use
cpu->env.psw.addr instead of kvm_run, as we do a
cpu_synchronize_state().

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 target/s390x/helper.c   | 28 ++++++++++++++++++++++------
 target/s390x/internal.h |  1 +
 target/s390x/kvm.c      | 15 +--------------
 3 files changed, 24 insertions(+), 20 deletions(-)

On 18.09.2017 17:59, David Hildenbrand wrote:
> This will now also detect crashes under TCG. We can directly use
> cpu->env.psw.addr instead of kvm_run, as we do a
> cpu_synchronize_state().
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  target/s390x/helper.c   | 28 ++++++++++++++++++++++------
>  target/s390x/internal.h |  1 +
>  target/s390x/kvm.c      | 15 +--------------
>  3 files changed, 24 insertions(+), 20 deletions(-)
> 
> diff --git a/target/s390x/helper.c b/target/s390x/helper.c
> index e22b93258b..75ceb0bf2b 100644
> --- a/target/s390x/helper.c
> +++ b/target/s390x/helper.c
> @@ -26,6 +26,7 @@
>  #include "qemu/timer.h"
>  #include "exec/exec-all.h"
>  #include "hw/s390x/ioinst.h"
> +#include "sysemu/hw_accel.h"
>  #ifndef CONFIG_USER_ONLY
>  #include "sysemu/sysemu.h"
>  #endif
> @@ -113,6 +114,26 @@ hwaddr s390_cpu_get_phys_addr_debug(CPUState *cs, vaddr vaddr)
>      return phys_addr;
>  }
>  
> +static inline bool is_special_wait_psw(uint64_t psw_addr)
> +{
> +    /* signal quiesce */
> +    return psw_addr == 0xfffUL;
> +}
> +
> +void handle_wait(S390CPU *cpu)

"handle_wait" is a very generic name ... could you maybe at least name
this "s390x_handle_wait" or so?

> +{
> +    cpu_synchronize_state(CPU(cpu));
> +    if (s390_cpu_halt(cpu) == 0) {
> +#ifndef CONFIG_USER_ONLY
> +        if (is_special_wait_psw(cpu->env.psw.addr)) {
> +            qemu_system_shutdown_request(SHUTDOWN_CAUSE_GUEST_SHUTDOWN);
> +        } else {
> +            qemu_system_guest_panicked(NULL);
> +        }
> +#endif
> +    }
> +}
> +
>  void load_psw(CPUS390XState *env, uint64_t mask, uint64_t addr)
>  {
>      uint64_t old_mask = env->psw.mask;
> @@ -128,12 +149,7 @@ void load_psw(CPUS390XState *env, uint64_t mask, uint64_t addr)
>      }
>  
>      if (mask & PSW_MASK_WAIT) {
> -        S390CPU *cpu = s390_env_get_cpu(env);
> -        if (s390_cpu_halt(cpu) == 0) {
> -#ifndef CONFIG_USER_ONLY
> -            qemu_system_shutdown_request(SHUTDOWN_CAUSE_GUEST_SHUTDOWN);
> -#endif
> -        }
> +        handle_wait(s390_env_get_cpu(env));
>      }
>  }
>  
> diff --git a/target/s390x/internal.h b/target/s390x/internal.h
> index 15743ec40f..cb331f35ea 100644
> --- a/target/s390x/internal.h
> +++ b/target/s390x/internal.h
> @@ -280,6 +280,7 @@ const char *cc_name(enum cc_op cc_op);
>  void load_psw(CPUS390XState *env, uint64_t mask, uint64_t addr);
>  uint32_t calc_cc(CPUS390XState *env, uint32_t cc_op, uint64_t src, uint64_t dst,
>                   uint64_t vr);
> +void handle_wait(S390CPU *cpu);
>  
>  
>  /* cpu.c */
> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
> index 3f9983154f..14f864697d 100644
> --- a/target/s390x/kvm.c
> +++ b/target/s390x/kvm.c
> @@ -1936,12 +1936,6 @@ static int handle_instruction(S390CPU *cpu, struct kvm_run *run)
>      return r;
>  }
>  
> -static bool is_special_wait_psw(CPUState *cs)
> -{
> -    /* signal quiesce */
> -    return cs->kvm_run->psw_addr == 0xfffUL;
> -}
> -
>  static void unmanageable_intercept(S390CPU *cpu, const char *str, int pswoffset)
>  {
>      CPUState *cs = CPU(cpu);
> @@ -2012,14 +2006,7 @@ static int handle_intercept(S390CPU *cpu)
>              break;
>          case ICPT_WAITPSW:
>              /* disabled wait, since enabled wait is handled in kernel */
> -            cpu_synchronize_state(cs);

Maybe we should rather keep the cpu_synchronize_state() here instead of
putting it in helper.c? No other function in helper.c seems to call that
on its own yet...

> -            if (s390_cpu_halt(cpu) == 0) {
> -                if (is_special_wait_psw(cs)) {
> -                    qemu_system_shutdown_request(SHUTDOWN_CAUSE_GUEST_SHUTDOWN);
> -                } else {
> -                    qemu_system_guest_panicked(NULL);
> -                }
> -            }
> +            handle_wait(cpu);
>              r = EXCP_HALTED;
>              break;
>          case ICPT_CPU_STOP:
> 

 Thomas

On 09/18/2017 08:59 AM, David Hildenbrand wrote:
> This will now also detect crashes under TCG. We can directly use
> cpu->env.psw.addr instead of kvm_run, as we do a
> cpu_synchronize_state().
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  target/s390x/helper.c   | 28 ++++++++++++++++++++++------
>  target/s390x/internal.h |  1 +
>  target/s390x/kvm.c      | 15 +--------------
>  3 files changed, 24 insertions(+), 20 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

>> +
>> +void handle_wait(S390CPU *cpu)
> 
> "handle_wait" is a very generic name ... could you maybe at least name
> this "s390x_handle_wait" or so?

sure, will do, thanks!

> 
>> +{
>> +    cpu_synchronize_state(CPU(cpu));
>> +    if (s390_cpu_halt(cpu) == 0) {
>> +#ifndef CONFIG_USER_ONLY
>> +        if (is_special_wait_psw(cpu->env.psw.addr)) {
>> +            qemu_system_shutdown_request(SHUTDOWN_CAUSE_GUEST_SHUTDOWN);
>> +        } else {
>> +            qemu_system_guest_panicked(NULL);
>> +        }
>> +#endif
>> +    }
>> +}
[...]

>> -            cpu_synchronize_state(cs);
> 
> Maybe we should rather keep the cpu_synchronize_state() here instead of
> putting it in helper.c? No other function in helper.c seems to call that
> on its own yet...

Don't have a strong opinion on this, as long as it works :)

> 
>> -            if (s390_cpu_halt(cpu) == 0) {
>> -                if (is_special_wait_psw(cs)) {
>> -                    qemu_system_shutdown_request(SHUTDOWN_CAUSE_GUEST_SHUTDOWN);
>> -                } else {
>> -                    qemu_system_guest_panicked(NULL);
>> -                }
>> -            }
>> +            handle_wait(cpu);
>>              r = EXCP_HALTED;
>>              break;
>>          case ICPT_CPU_STOP:
>>
> 
>  Thomas
>