Patchwork [v2,1/3] cpu: make CPU_INTERRUPT_RESET available on all targets

login
register
mail settings
Submitter Paolo Bonzini
Date March 5, 2013, 7 p.m.
Message ID <1362510056-3316-2-git-send-email-pbonzini@redhat.com>
Download mbox | patch
Permalink /patch/225166/
State New
Headers show

Comments

Paolo Bonzini - March 5, 2013, 7 p.m.
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 <aliguori@us.ibm.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 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(-)
Peter Maydell - March 5, 2013, 11:23 p.m.
On 6 March 2013 03:00, Paolo Bonzini <pbonzini@redhat.com> 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.

Not sure this makes sense -- reset isn't an interrupt...

-- PMM
Peter Crosthwaite - March 6, 2013, 2:02 a.m.
Hi Paolo,

On Wed, Mar 6, 2013 at 5:00 AM, Paolo Bonzini <pbonzini@redhat.com> 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 <aliguori@us.ibm.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  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/qemu-devel@nongnu.org/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
>
>
>
Paolo Bonzini - March 6, 2013, 8:49 a.m.
Il 06/03/2013 00:23, Peter Maydell ha scritto:
> On 6 March 2013 03:00, Paolo Bonzini <pbonzini@redhat.com> 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.
> 
> Not sure this makes sense -- reset isn't an interrupt...

cpu_interrupt is not just for interrupts, CPU_INTERRUPT_TGT_INT_* is a
generic mechanism for adding events to the CPU that have to exit the
translation block (they do not even have to be input pins, though
CPU_INTERRUPT_RESET is).  This patch just takes one particular
CPU_INTERRUPT_TGT_INT_* value and makes it available to all targets.

It is important for the reset to exit the translation block, or the CPU
goes into the weeds.  The problem I was seeing is that the code looked like:

    mov $0xfe, %al
    out %al, $0x60
    jmp foo           // this is a relative jump
...
foo:
    cli
    hlt

Now, if the reset were synchronous (i.e. cpu_reset), it would modify the
stored PC to 0xfffffff0 without leaving the translation block.
Because the jump is relative, it would go to 0xfffffff0 + the offset
instead of jumping to foo.

This could also be implemented by something like this:

    run_on_cpu(cpu, cpu_reset);
    cpu_interrupt(cpu, CPU_INTERRUPT_EXITTB);

But I preferred to reuse the existing logic (there would be some
additional complication because the x86 INIT signal does _not_ reset a
couple of things that are reset at power up).

Paolo
Paolo Bonzini - March 6, 2013, 9:13 a.m.
Il 06/03/2013 03:02, Peter Crosthwaite ha scritto:
> If you truly have connectivity from device land to the CPU cluster
> should that be reflected by some sort of QOM linkage?

I think in real hardware what happens is that a single "wire" is
distributed to all CPUs.  Devices do not have direct links to all the
CPUs, they are agnostic of how many CPUs they control (at least on x86).
 In this sense, using first_cpu is the right modelling in my opinion.

Having qemu_irqs for all the reset requests would definitely be a good
thing to do.  In the meanwhile, however, having half of the reset
signals as qemu_irqs, and the other half as function calls would be
confusing.

Alternatively we could add some kind of "meta-device" that distributes
stuff to all CPUs, and hide the usage of first_cpu there.  Andreas, what
do you think?

Paolo
Andreas Färber - March 6, 2013, 11:54 a.m.
Am 06.03.2013 10:13, schrieb Paolo Bonzini:
> Il 06/03/2013 03:02, Peter Crosthwaite ha scritto:
>> If you truly have connectivity from device land to the CPU cluster
>> should that be reflected by some sort of QOM linkage?
> 
> I think in real hardware what happens is that a single "wire" is
> distributed to all CPUs.  Devices do not have direct links to all the
> CPUs, they are agnostic of how many CPUs they control (at least on x86).
>  In this sense, using first_cpu is the right modelling in my opinion.
> 
> Having qemu_irqs for all the reset requests would definitely be a good
> thing to do.  In the meanwhile, however, having half of the reset
> signals as qemu_irqs, and the other half as function calls would be
> confusing.
> 
> Alternatively we could add some kind of "meta-device" that distributes
> stuff to all CPUs, and hide the usage of first_cpu there.  Andreas, what
> do you think?

In short I think that first_cpu is ugly but we do not have a
CPU_FOREACH() macro/function replacement yet.

That's why I like the approach of defining a qemu_reset_cpus() function
(or whatever we name it) that hides this rather than everyone
open-coding it, which has recently led to some conversion mistakes on my
part.

I don't see a working way to model 1:n link<CPUState> ATM. Having a
"cpu-resetter" object with a QOM reset method distributing
cpu_reset/cpu_interrupt seems unnecessarily complicated to me.

Seeing that qemu_irq was going to be replaced with a QOM Pin object, I
don't see an advantage in trying to introduce new qemu_irqs where a
simple function call works just as well. You could decide this on a
case-by-case basis depending on whether the reset is level-triggered.
Or dig out Anthony's Pin series and do something based on that. ;-)

But mostly my concerns are not introducing new per-target global
functions and keeping/making CPU reset code sane in terms of not getting
duplicated into multiple places, i.e. code sharing and consolidation, if
we start differentiating between hard and soft and whatever reset types.

Andreas
Peter Maydell - March 6, 2013, 12:12 p.m.
On 6 March 2013 17:13, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 06/03/2013 03:02, Peter Crosthwaite ha scritto:
>> If you truly have connectivity from device land to the CPU cluster
>> should that be reflected by some sort of QOM linkage?
>
> I think in real hardware what happens is that a single "wire" is
> distributed to all CPUs.  Devices do not have direct links to all the
> CPUs, they are agnostic of how many CPUs they control (at least on x86).

This is definitely x86 specific. On ARM, typically each core
has its own reset line and the only way to reset all cores is
to assert all the lines.

http://infocenter.arm.com/help/topic/com.arm.doc.ddi0407i/BABGCECJ.html
lists all the reset inputs for an A9MP, for example.

-- PMM
Paolo Bonzini - March 6, 2013, 12:19 p.m.
Il 06/03/2013 13:12, Peter Maydell ha scritto:
> On 6 March 2013 17:13, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> Il 06/03/2013 03:02, Peter Crosthwaite ha scritto:
>>> If you truly have connectivity from device land to the CPU cluster
>>> should that be reflected by some sort of QOM linkage?
>>
>> I think in real hardware what happens is that a single "wire" is
>> distributed to all CPUs.  Devices do not have direct links to all the
>> CPUs, they are agnostic of how many CPUs they control (at least on x86).
> 
> This is definitely x86 specific. On ARM, typically each core
> has its own reset line and the only way to reset all cores is
> to assert all the lines.

Each core has indeed its own reset line, but there is only one output
pin for reset in the southbridge (which is where the reset port lies).

Paolo

> http://infocenter.arm.com/help/topic/com.arm.doc.ddi0407i/BABGCECJ.html
> lists all the reset inputs for an A9MP, for example.
> 
> -- PMM
> 
>

Patch

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);
+    }
+}
+
 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)