Re: [Qemu-devel] [PATCH v2 1/3] cpu: make CPU_INTERRUPT_RESET available on all targets

[Peter Crosthwaite]

Hi Paolo,

On Wed, Mar 6, 2013 at 5:00 AM, Paolo Bonzini <address@hidden> wrote:
> On the x86, some devices need access to the CPU reset pin (INIT#).
> Provide a generic service to do this, using one of the internal
> cpu_interrupt targets.  Generalize the PPC-specific code for
> CPU_INTERRUPT_RESET to other targets, and provide a function that
> will raise the interrupt on all CPUs.
>
> Since PPC does not support migration, I picked the value that is
> used on x86, CPU_INTERRUPT_TGT_INT_1.  No other arch used to use
> CPU_INTERRUPT_TGT_INT_1.
>
> Reviewed-by: Anthony Liguori <address@hidden>
> Signed-off-by: Paolo Bonzini <address@hidden>
> ---
>  cpu-exec.c             | 24 ++++++++++++++----------
>  cpus.c                 |  9 +++++++++
>  include/exec/cpu-all.h |  8 +++++---
>  include/sysemu/cpus.h  |  1 +
>  target-i386/cpu.h      |  7 ++++---
>  target-ppc/cpu.h       |  3 ---
>  6 files changed, 33 insertions(+), 19 deletions(-)
>
> diff --git a/cpu-exec.c b/cpu-exec.c
> index 94fedc5..f400676 100644
> --- a/cpu-exec.c
> +++ b/cpu-exec.c
> @@ -304,19 +304,26 @@ int cpu_exec(CPUArchState *env)
>                      }
>  #endif
>  #if defined(TARGET_I386)
> -#if !defined(CONFIG_USER_ONLY)
> -                    if (interrupt_request & CPU_INTERRUPT_POLL) {
> -                        cpu->interrupt_request &= ~CPU_INTERRUPT_POLL;
> -                        apic_poll_irq(env->apic_state);
> -                    }
> -#endif
>                      if (interrupt_request & CPU_INTERRUPT_INIT) {
>                              cpu_svm_check_intercept_param(env, SVM_EXIT_INIT,
>                                                            0);
>                              do_cpu_init(x86_env_get_cpu(env));
>                              env->exception_index = EXCP_HALTED;
>                              cpu_loop_exit(env);
> -                    } else if (interrupt_request & CPU_INTERRUPT_SIPI) {
> +                    }
> +#else
> +                    if ((interrupt_request & CPU_INTERRUPT_RESET)) {
> +                        cpu_reset(cpu);
> +                    }
> +#endif
> +#if defined(TARGET_I386)
> +#if !defined(CONFIG_USER_ONLY)
> +                    if (interrupt_request & CPU_INTERRUPT_POLL) {
> +                        cpu->interrupt_request &= ~CPU_INTERRUPT_POLL;
> +                        apic_poll_irq(env->apic_state);
> +                    }
> +#endif
> +                    if (interrupt_request & CPU_INTERRUPT_SIPI) {
>                              do_cpu_sipi(x86_env_get_cpu(env));
>                      } else if (env->hflags2 & HF2_GIF_MASK) {
>                          if ((interrupt_request & CPU_INTERRUPT_SMI) &&
> @@ -370,9 +377,6 @@ int cpu_exec(CPUArchState *env)
>                          }
>                      }
>  #elif defined(TARGET_PPC)
> -                    if ((interrupt_request & CPU_INTERRUPT_RESET)) {
> -                        cpu_reset(cpu);
> -                    }
>                      if (interrupt_request & CPU_INTERRUPT_HARD) {
>                          ppc_hw_interrupt(env);
>                          if (env->pending_interrupts == 0) {
> diff --git a/cpus.c b/cpus.c
> index e919dd7..87d471a 100644
> --- a/cpus.c
> +++ b/cpus.c
> @@ -405,6 +405,15 @@ void hw_error(const char *fmt, ...)
>      abort();
>  }
>
> +void cpu_reset_all_async(void)
> +{
> +    CPUArchState *env;
> +
> +    for (env = first_cpu; env; env = env->next_cpu) {
> +        cpu_interrupt(ENV_GET_CPU(env), CPU_INTERRUPT_RESET);
> +    }
> +}
> +

If you truly have connectivity from device land to the CPU cluster
should that be reflected by some sort of QOM linkage? Then the device
explicitly resets the device its connected to. This strikes me as a
step backwards - I'm not sure why CPU resets are special and are
allowed to rely on global state (first_cpu).

I currently have a series on list for my power controller to implement
its reset. Comments in relation to whats going on here are very
welcome. But if this series is acceptable ill remake my series to do
it this way - my device will have to cheat by using first_cpu to get a
handle on the CPUs.

http://www.mail-archive.com/address@hidden/msg158571.html

Regards,
Peter

>  void cpu_synchronize_all_states(void)
>  {
>      CPUArchState *cpu;
> diff --git a/include/exec/cpu-all.h b/include/exec/cpu-all.h
> index e9c3717..62d2654 100644
> --- a/include/exec/cpu-all.h
> +++ b/include/exec/cpu-all.h
> @@ -392,6 +392,9 @@ DECLARE_TLS(CPUArchState *,cpu_single_env);
>  /* Debug event pending.  */
>  #define CPU_INTERRUPT_DEBUG       0x0080
>
> +/* Reset signal.  */
> +#define CPU_INTERRUPT_RESET       0x0400
> +
>  /* Several target-specific external hardware interrupts.  Each target/cpu.h
>     should define proper names based on these defines.  */
>  #define CPU_INTERRUPT_TGT_EXT_0   0x0008
> @@ -406,9 +409,8 @@ DECLARE_TLS(CPUArchState *,cpu_single_env);
>     instruction being executed.  These, therefore, are not masked while
>     single-stepping within the debugger.  */
>  #define CPU_INTERRUPT_TGT_INT_0   0x0100
> -#define CPU_INTERRUPT_TGT_INT_1   0x0400
> -#define CPU_INTERRUPT_TGT_INT_2   0x0800
> -#define CPU_INTERRUPT_TGT_INT_3   0x2000
> +#define CPU_INTERRUPT_TGT_INT_1   0x0800
> +#define CPU_INTERRUPT_TGT_INT_2   0x2000
>
>  /* First unused bit: 0x4000.  */
>
> diff --git a/include/sysemu/cpus.h b/include/sysemu/cpus.h
> index 6502488..87b9829 100644
> --- a/include/sysemu/cpus.h
> +++ b/include/sysemu/cpus.h
> @@ -7,6 +7,7 @@ void resume_all_vcpus(void);
>  void pause_all_vcpus(void);
>  void cpu_stop_current(void);
>
> +void cpu_reset_all_async(void);
>  void cpu_synchronize_all_states(void);
>  void cpu_synchronize_all_post_reset(void);
>  void cpu_synchronize_all_post_init(void);
> diff --git a/target-i386/cpu.h b/target-i386/cpu.h
> index 48f41ca..c7b8176 100644
> --- a/target-i386/cpu.h
> +++ b/target-i386/cpu.h
> @@ -577,10 +577,11 @@ typedef uint32_t FeatureWordArray[FEATURE_WORDS];
>  #define CPU_INTERRUPT_NMI       CPU_INTERRUPT_TGT_EXT_3
>  #define CPU_INTERRUPT_MCE       CPU_INTERRUPT_TGT_EXT_4
>  #define CPU_INTERRUPT_VIRQ      CPU_INTERRUPT_TGT_INT_0
> -#define CPU_INTERRUPT_INIT      CPU_INTERRUPT_TGT_INT_1
> -#define CPU_INTERRUPT_SIPI      CPU_INTERRUPT_TGT_INT_2
> -#define CPU_INTERRUPT_TPR       CPU_INTERRUPT_TGT_INT_3
> +#define CPU_INTERRUPT_SIPI      CPU_INTERRUPT_TGT_INT_1
> +#define CPU_INTERRUPT_TPR       CPU_INTERRUPT_TGT_INT_2
>
> +/* CPU_INTERRUPT_RESET acts as the INIT# pin.  */
> +#define CPU_INTERRUPT_INIT      CPU_INTERRUPT_RESET
>
>  typedef enum {
>      CC_OP_DYNAMIC, /* must use dynamic code to get cc_op */
> diff --git a/target-ppc/cpu.h b/target-ppc/cpu.h
> index 4604a28..ceb0a12 100644
> --- a/target-ppc/cpu.h
> +++ b/target-ppc/cpu.h
> @@ -2084,9 +2084,6 @@ enum {
>      PPC_INTERRUPT_PERFM,          /* Performance monitor interrupt        */
>  };
>
> -/* CPU should be reset next, restart from scratch afterwards */
> -#define CPU_INTERRUPT_RESET       CPU_INTERRUPT_TGT_INT_0
> -
>  
> /*****************************************************************************/
>
>  static inline target_ulong cpu_read_xer(CPUPPCState *env)
> --
> 1.8.1.2
>
>
>