[Qemu-devel] [PATCH v2 0/3] Implement x86 soft reset

[Qemu-devel] [PATCH v2 0/3] Implement x86 soft reset

[Paolo Bonzini]

This series makes various devices (port 92h, pckbd and the
PIIX4/ICH9 southbridges) implement x86 soft reset correctly.

v1->v2: rebased onto Andreas's branch, fixed target-ppc/cpu.h,
        renamed function

Paolo Bonzini (3):
  cpu: make CPU_INTERRUPT_RESET available on all targets
  pc: port 92 reset requires a low->high transition
  hw: correctly implement soft reset

 cpu-exec.c             | 24 ++++++++++++++----------
 cpus.c                 |  9 +++++++++
 hw/lpc_ich9.c          |  7 ++++++-
 hw/pc.c                |  6 ++++--
 hw/pckbd.c             |  5 +++--
 hw/piix_pci.c          |  8 ++++++--
 include/exec/cpu-all.h |  8 +++++---
 include/sysemu/cpus.h  |  1 +
 target-i386/cpu.h      |  7 ++++---
 target-ppc/cpu.h       |  3 ---
 10 files changed, 52 insertions(+), 26 deletions(-)

-- 
1.8.1.2

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

[Paolo Bonzini]

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

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

[Peter Maydell]

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

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

[Paolo Bonzini]

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

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 <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
>
>
>

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

[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?

Paolo

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

[Andreas Färber]

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

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

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

[Peter Maydell]

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

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

[Paolo Bonzini]

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
> 
> 

[Qemu-devel] [PATCH v2 2/3] pc: port 92 reset requires a low->high transition

[Paolo Bonzini]

The PIIX datasheet says that "before another INIT pulse can be
generated via [port 92h], [bit 0] must be written back to a
zero.

This bug is masked right now because a full reset will clear the
value of port 92h.  But once we implement soft reset correctly,
the next attempt to enable the A20 line by setting bit 1 (and
leaving the others untouched) will cause another reset.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/pc.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/pc.c b/hw/pc.c
index 37f6b52..3e1cf2e 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -437,11 +437,12 @@ static void port92_write(void *opaque, hwaddr addr, uint64_t val,
                          unsigned size)
 {
     Port92State *s = opaque;
+    int oldval = s->outport;
 
     DPRINTF("port92: write 0x%02x\n", val);
     s->outport = val;
     qemu_set_irq(*s->a20_out, (val >> 1) & 1);
-    if (val & 1) {
+    if ((val & 1) && !(oldval & 1)) {
         qemu_system_reset_request();
     }
 }
-- 
1.8.1.2

[Qemu-devel] [PATCH v2 3/3] hw: correctly implement soft reset

[Paolo Bonzini]

Do not do a hard reset for port 92h, keyboard controller, or cf9h soft reset.
These only reset the CPU.

Reviewed-by: Anthony Liguori <aliguori@us.ibm.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/lpc_ich9.c | 7 ++++++-
 hw/pc.c       | 3 ++-
 hw/pckbd.c    | 5 +++--
 hw/piix_pci.c | 8 ++++++--
 4 files changed, 17 insertions(+), 6 deletions(-)

diff --git a/hw/lpc_ich9.c b/hw/lpc_ich9.c
index e473758..5540f61 100644
--- a/hw/lpc_ich9.c
+++ b/hw/lpc_ich9.c
@@ -45,6 +45,7 @@
 #include "pci/pci_bus.h"
 #include "exec/address-spaces.h"
 #include "sysemu/sysemu.h"
+#include "sysemu/cpus.h"
 
 static int ich9_lpc_sci_irq(ICH9LPCState *lpc);
 
@@ -506,7 +507,11 @@ static void ich9_rst_cnt_write(void *opaque, hwaddr addr, uint64_t val,
     ICH9LPCState *lpc = opaque;
 
     if (val & 4) {
-        qemu_system_reset_request();
+        if (val & 0xA) {
+            qemu_system_reset_request();
+        } else {
+            cpu_reset_all_async();
+        }
         return;
     }
     lpc->rst_cnt = val & 0xA; /* keep FULL_RST (bit 3) and SYS_RST (bit 1) */
diff --git a/hw/pc.c b/hw/pc.c
index 3e1cf2e..54f5b72 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -45,6 +45,7 @@
 #include "kvm_i386.h"
 #include "xen.h"
 #include "sysemu/blockdev.h"
+#include "sysemu/cpus.h"
 #include "hw/block-common.h"
 #include "ui/qemu-spice.h"
 #include "exec/memory.h"
@@ -443,7 +444,7 @@ static void port92_write(void *opaque, hwaddr addr, uint64_t val,
     s->outport = val;
     qemu_set_irq(*s->a20_out, (val >> 1) & 1);
     if ((val & 1) && !(oldval & 1)) {
-        qemu_system_reset_request();
+        cpu_reset_all_async();
     }
 }
 
diff --git a/hw/pckbd.c b/hw/pckbd.c
index 3bad09b..fd66788 100644
--- a/hw/pckbd.c
+++ b/hw/pckbd.c
@@ -26,6 +26,7 @@
 #include "pc.h"
 #include "ps2.h"
 #include "sysemu/sysemu.h"
+#include "sysemu/cpus.h"
 
 /* debug PC keyboard */
 //#define DEBUG_KBD
@@ -220,7 +221,7 @@ static void outport_write(KBDState *s, uint32_t val)
         qemu_set_irq(*s->a20_out, (val >> 1) & 1);
     }
     if (!(val & 1)) {
-        qemu_system_reset_request();
+        cpu_reset_all_async();
     }
 }
 
@@ -299,7 +300,7 @@ static void kbd_write_command(void *opaque, hwaddr addr,
         s->outport &= ~KBD_OUT_A20;
         break;
     case KBD_CCMD_RESET:
-        qemu_system_reset_request();
+        cpu_reset_all_async();
         break;
     case KBD_CCMD_NO_OP:
         /* ignore that */
diff --git a/hw/piix_pci.c b/hw/piix_pci.c
index 6c77e49..785e0a7 100644
--- a/hw/piix_pci.c
+++ b/hw/piix_pci.c
@@ -32,6 +32,7 @@
 #include "xen.h"
 #include "pam.h"
 #include "sysemu/sysemu.h"
+#include "sysemu/cpus.h"
 
 /*
  * I440FX chipset data sheet.
@@ -521,8 +522,11 @@ static void rcr_write(void *opaque, hwaddr addr, uint64_t val, unsigned len)
     PIIX3State *d = opaque;
 
     if (val & 4) {
-        qemu_system_reset_request();
-        return;
+        if (val & 2) {
+            qemu_system_reset_request();
+        } else {
+            cpu_reset_all_async();
+        }
     }
     d->rcr = val & 2; /* keep System Reset type only */
 }
-- 
1.8.1.2

Re: [Qemu-devel] [PATCH v2 3/3] hw: correctly implement soft reset

[li guang]

在 2013-03-05二的 20:00 +0100，Paolo Bonzini写道：
> Do not do a hard reset for port 92h, keyboard controller, or cf9h soft reset.
> These only reset the CPU.
> 
> Reviewed-by: Anthony Liguori <aliguori@us.ibm.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  hw/lpc_ich9.c | 7 ++++++-
>  hw/pc.c       | 3 ++-
>  hw/pckbd.c    | 5 +++--
>  hw/piix_pci.c | 8 ++++++--
>  4 files changed, 17 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/lpc_ich9.c b/hw/lpc_ich9.c
> index e473758..5540f61 100644
> --- a/hw/lpc_ich9.c
> +++ b/hw/lpc_ich9.c
> @@ -45,6 +45,7 @@
>  #include "pci/pci_bus.h"
>  #include "exec/address-spaces.h"
>  #include "sysemu/sysemu.h"
> +#include "sysemu/cpus.h"
>  
>  static int ich9_lpc_sci_irq(ICH9LPCState *lpc);
>  
> @@ -506,7 +507,11 @@ static void ich9_rst_cnt_write(void *opaque, hwaddr addr, uint64_t val,
>      ICH9LPCState *lpc = opaque;
>  
>      if (val & 4) {
> -        qemu_system_reset_request();
> +        if (val & 0xA) {
> +            qemu_system_reset_request();
> +        } else {
> +            cpu_reset_all_async();
> +        }
>          return;

in fact, soft reset is cpu reset, hard reset is platform reset,
it is too harsh to require both bit 3 & 1 to do a system reset,
they are independent, either of them can trigger that.

>      }
>      lpc->rst_cnt = val & 0xA; /* keep FULL_RST (bit 3) and SYS_RST (bit 1) */
> diff --git a/hw/pc.c b/hw/pc.c
> index 3e1cf2e..54f5b72 100644
> --- a/hw/pc.c
> +++ b/hw/pc.c
> @@ -45,6 +45,7 @@
>  #include "kvm_i386.h"
>  #include "xen.h"
>  #include "sysemu/blockdev.h"
> +#include "sysemu/cpus.h"
>  #include "hw/block-common.h"
>  #include "ui/qemu-spice.h"
>  #include "exec/memory.h"
> @@ -443,7 +444,7 @@ static void port92_write(void *opaque, hwaddr addr, uint64_t val,
>      s->outport = val;
>      qemu_set_irq(*s->a20_out, (val >> 1) & 1);
>      if ((val & 1) && !(oldval & 1)) {
> -        qemu_system_reset_request();
> +        cpu_reset_all_async();
>      }
>  }
>  
> diff --git a/hw/pckbd.c b/hw/pckbd.c
> index 3bad09b..fd66788 100644
> --- a/hw/pckbd.c
> +++ b/hw/pckbd.c
> @@ -26,6 +26,7 @@
>  #include "pc.h"
>  #include "ps2.h"
>  #include "sysemu/sysemu.h"
> +#include "sysemu/cpus.h"
>  
>  /* debug PC keyboard */
>  //#define DEBUG_KBD
> @@ -220,7 +221,7 @@ static void outport_write(KBDState *s, uint32_t val)
>          qemu_set_irq(*s->a20_out, (val >> 1) & 1);
>      }
>      if (!(val & 1)) {
> -        qemu_system_reset_request();
> +        cpu_reset_all_async();
>      }
>  }
>  
> @@ -299,7 +300,7 @@ static void kbd_write_command(void *opaque, hwaddr addr,
>          s->outport &= ~KBD_OUT_A20;
>          break;
>      case KBD_CCMD_RESET:
> -        qemu_system_reset_request();
> +        cpu_reset_all_async();

Oh, no, system reset is correct.
in the real world, system and cpu reset are quite different,
cpu reset only reset processor power, while system reset
will reset platform power.

>          break;
>      case KBD_CCMD_NO_OP:
>          /* ignore that */
> diff --git a/hw/piix_pci.c b/hw/piix_pci.c
> index 6c77e49..785e0a7 100644
> --- a/hw/piix_pci.c
> +++ b/hw/piix_pci.c
> @@ -32,6 +32,7 @@
>  #include "xen.h"
>  #include "pam.h"
>  #include "sysemu/sysemu.h"
> +#include "sysemu/cpus.h"
>  
>  /*
>   * I440FX chipset data sheet.
> @@ -521,8 +522,11 @@ static void rcr_write(void *opaque, hwaddr addr, uint64_t val, unsigned len)
>      PIIX3State *d = opaque;
>  
>      if (val & 4) {
> -        qemu_system_reset_request();
> -        return;
> +        if (val & 2) {
> +            qemu_system_reset_request();
> +        } else {
> +            cpu_reset_all_async();
> +        }
>      }
>      d->rcr = val & 2; /* keep System Reset type only */
>  }

Re: [Qemu-devel] [PATCH v2 3/3] hw: correctly implement soft reset

[Paolo Bonzini]

Il 06/03/2013 03:02, li guang ha scritto:
> 在 2013-03-05二的 20:00 +0100，Paolo Bonzini写道：
>> Do not do a hard reset for port 92h, keyboard controller, or cf9h soft reset.
>> These only reset the CPU.
>>
>> Reviewed-by: Anthony Liguori <aliguori@us.ibm.com>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> ---
>>  hw/lpc_ich9.c | 7 ++++++-
>>  hw/pc.c       | 3 ++-
>>  hw/pckbd.c    | 5 +++--
>>  hw/piix_pci.c | 8 ++++++--
>>  4 files changed, 17 insertions(+), 6 deletions(-)
>>
>> diff --git a/hw/lpc_ich9.c b/hw/lpc_ich9.c
>> index e473758..5540f61 100644
>> --- a/hw/lpc_ich9.c
>> +++ b/hw/lpc_ich9.c
>> @@ -45,6 +45,7 @@
>>  #include "pci/pci_bus.h"
>>  #include "exec/address-spaces.h"
>>  #include "sysemu/sysemu.h"
>> +#include "sysemu/cpus.h"
>>  
>>  static int ich9_lpc_sci_irq(ICH9LPCState *lpc);
>>  
>> @@ -506,7 +507,11 @@ static void ich9_rst_cnt_write(void *opaque, hwaddr addr, uint64_t val,
>>      ICH9LPCState *lpc = opaque;
>>  
>>      if (val & 4) {
>> -        qemu_system_reset_request();
>> +        if (val & 0xA) {
>> +            qemu_system_reset_request();
>> +        } else {
>> +            cpu_reset_all_async();
>> +        }
>>          return;
> 
> in fact, soft reset is cpu reset, hard reset is platform reset,
> it is too harsh to require both bit 3 & 1 to do a system reset,
> they are independent, either of them can trigger that.

This is "full reset if (val & 0xA) != 0".  You interpreted it as (val &
0xA) = 0xA, I think.

>>      }
>>      lpc->rst_cnt = val & 0xA; /* keep FULL_RST (bit 3) and SYS_RST (bit 1) */
>> diff --git a/hw/pc.c b/hw/pc.c
>> index 3e1cf2e..54f5b72 100644
>> --- a/hw/pc.c
>> +++ b/hw/pc.c
>> @@ -45,6 +45,7 @@
>>  #include "kvm_i386.h"
>>  #include "xen.h"
>>  #include "sysemu/blockdev.h"
>> +#include "sysemu/cpus.h"
>>  #include "hw/block-common.h"
>>  #include "ui/qemu-spice.h"
>>  #include "exec/memory.h"
>> @@ -443,7 +444,7 @@ static void port92_write(void *opaque, hwaddr addr, uint64_t val,
>>      s->outport = val;
>>      qemu_set_irq(*s->a20_out, (val >> 1) & 1);
>>      if ((val & 1) && !(oldval & 1)) {
>> -        qemu_system_reset_request();
>> +        cpu_reset_all_async();
>>      }
>>  }
>>  
>> diff --git a/hw/pckbd.c b/hw/pckbd.c
>> index 3bad09b..fd66788 100644
>> --- a/hw/pckbd.c
>> +++ b/hw/pckbd.c
>> @@ -26,6 +26,7 @@
>>  #include "pc.h"
>>  #include "ps2.h"
>>  #include "sysemu/sysemu.h"
>> +#include "sysemu/cpus.h"
>>  
>>  /* debug PC keyboard */
>>  //#define DEBUG_KBD
>> @@ -220,7 +221,7 @@ static void outport_write(KBDState *s, uint32_t val)
>>          qemu_set_irq(*s->a20_out, (val >> 1) & 1);
>>      }
>>      if (!(val & 1)) {
>> -        qemu_system_reset_request();
>> +        cpu_reset_all_async();
>>      }
>>  }
>>  
>> @@ -299,7 +300,7 @@ static void kbd_write_command(void *opaque, hwaddr addr,
>>          s->outport &= ~KBD_OUT_A20;
>>          break;
>>      case KBD_CCMD_RESET:
>> -        qemu_system_reset_request();
>> +        cpu_reset_all_async();
> 
> Oh, no, system reset is correct.

No, the keyboard controller is connected to the CPU reset signal.

> in the real world, system and cpu reset are quite different,
> cpu reset only reset processor power, while system reset
> will reset platform power.

Separating the two is exactly the purpose of this patch. :)

Paolo

>>          break;
>>      case KBD_CCMD_NO_OP:
>>          /* ignore that */
>> diff --git a/hw/piix_pci.c b/hw/piix_pci.c
>> index 6c77e49..785e0a7 100644
>> --- a/hw/piix_pci.c
>> +++ b/hw/piix_pci.c
>> @@ -32,6 +32,7 @@
>>  #include "xen.h"
>>  #include "pam.h"
>>  #include "sysemu/sysemu.h"
>> +#include "sysemu/cpus.h"
>>  
>>  /*
>>   * I440FX chipset data sheet.
>> @@ -521,8 +522,11 @@ static void rcr_write(void *opaque, hwaddr addr, uint64_t val, unsigned len)
>>      PIIX3State *d = opaque;
>>  
>>      if (val & 4) {
>> -        qemu_system_reset_request();
>> -        return;
>> +        if (val & 2) {
>> +            qemu_system_reset_request();
>> +        } else {
>> +            cpu_reset_all_async();
>> +        }
>>      }
>>      d->rcr = val & 2; /* keep System Reset type only */
>>  }
> 
> 
> 
> 

Re: [Qemu-devel] [PATCH v2 3/3] hw: correctly implement soft reset

[li guang]

在 2013-03-06三的 09:36 +0100，Paolo Bonzini写道：
> Il 06/03/2013 03:02, li guang ha scritto:
> > 在 2013-03-05二的 20:00 +0100，Paolo Bonzini写道：
> >> Do not do a hard reset for port 92h, keyboard controller, or cf9h soft reset.
> >> These only reset the CPU.
> >>
> >> Reviewed-by: Anthony Liguori <aliguori@us.ibm.com>
> >> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> >> ---
> >>  hw/lpc_ich9.c | 7 ++++++-
> >>  hw/pc.c       | 3 ++-
> >>  hw/pckbd.c    | 5 +++--
> >>  hw/piix_pci.c | 8 ++++++--
> >>  4 files changed, 17 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/hw/lpc_ich9.c b/hw/lpc_ich9.c
> >> index e473758..5540f61 100644
> >> --- a/hw/lpc_ich9.c
> >> +++ b/hw/lpc_ich9.c
> >> @@ -45,6 +45,7 @@
> >>  #include "pci/pci_bus.h"
> >>  #include "exec/address-spaces.h"
> >>  #include "sysemu/sysemu.h"
> >> +#include "sysemu/cpus.h"
> >>  
> >>  static int ich9_lpc_sci_irq(ICH9LPCState *lpc);
> >>  
> >> @@ -506,7 +507,11 @@ static void ich9_rst_cnt_write(void *opaque, hwaddr addr, uint64_t val,
> >>      ICH9LPCState *lpc = opaque;
> >>  
> >>      if (val & 4) {
> >> -        qemu_system_reset_request();
> >> +        if (val & 0xA) {
> >> +            qemu_system_reset_request();
> >> +        } else {
> >> +            cpu_reset_all_async();
> >> +        }
> >>          return;
> > 
> > in fact, soft reset is cpu reset, hard reset is platform reset,
> > it is too harsh to require both bit 3 & 1 to do a system reset,
> > they are independent, either of them can trigger that.
> 
> This is "full reset if (val & 0xA) != 0".  You interpreted it as (val &
> 0xA) = 0xA, I think.

No, IMHO, full reset should be done only by bit 4.
that is bit 1 & 2 are combined to determined reset cpu/system.
and system reset does not always be the same with full reset(
mostly be different in hardware signal of power cycle),
it's up to the board hardware designer.

> 
> >>      }
> >>      lpc->rst_cnt = val & 0xA; /* keep FULL_RST (bit 3) and SYS_RST (bit 1) */
> >> diff --git a/hw/pc.c b/hw/pc.c
> >> index 3e1cf2e..54f5b72 100644
> >> --- a/hw/pc.c
> >> +++ b/hw/pc.c
> >> @@ -45,6 +45,7 @@
> >>  #include "kvm_i386.h"
> >>  #include "xen.h"
> >>  #include "sysemu/blockdev.h"
> >> +#include "sysemu/cpus.h"
> >>  #include "hw/block-common.h"
> >>  #include "ui/qemu-spice.h"
> >>  #include "exec/memory.h"
> >> @@ -443,7 +444,7 @@ static void port92_write(void *opaque, hwaddr addr, uint64_t val,
> >>      s->outport = val;
> >>      qemu_set_irq(*s->a20_out, (val >> 1) & 1);
> >>      if ((val & 1) && !(oldval & 1)) {
> >> -        qemu_system_reset_request();
> >> +        cpu_reset_all_async();
> >>      }
> >>  }
> >>  
> >> diff --git a/hw/pckbd.c b/hw/pckbd.c
> >> index 3bad09b..fd66788 100644
> >> --- a/hw/pckbd.c
> >> +++ b/hw/pckbd.c
> >> @@ -26,6 +26,7 @@
> >>  #include "pc.h"
> >>  #include "ps2.h"
> >>  #include "sysemu/sysemu.h"
> >> +#include "sysemu/cpus.h"
> >>  
> >>  /* debug PC keyboard */
> >>  //#define DEBUG_KBD
> >> @@ -220,7 +221,7 @@ static void outport_write(KBDState *s, uint32_t val)
> >>          qemu_set_irq(*s->a20_out, (val >> 1) & 1);
> >>      }
> >>      if (!(val & 1)) {
> >> -        qemu_system_reset_request();
> >> +        cpu_reset_all_async();
> >>      }
> >>  }
> >>  
> >> @@ -299,7 +300,7 @@ static void kbd_write_command(void *opaque, hwaddr addr,
> >>          s->outport &= ~KBD_OUT_A20;
> >>          break;
> >>      case KBD_CCMD_RESET:
> >> -        qemu_system_reset_request();
> >> +        cpu_reset_all_async();
> > 
> > Oh, no, system reset is correct.
> 
> No, the keyboard controller is connected to the CPU reset signal.

Not always, AFAIK,
traditionally, this should do system reset,
though system reset may be triggered by INIT# which 
I guess is you said "CPU reset signal".

> 
> > in the real world, system and cpu reset are quite different,
> > cpu reset only reset processor power, while system reset
> > will reset platform power.
> 
> Separating the two is exactly the purpose of this patch. :)
> 
> Paolo
> 
> >>          break;
> >>      case KBD_CCMD_NO_OP:
> >>          /* ignore that */
> >> diff --git a/hw/piix_pci.c b/hw/piix_pci.c
> >> index 6c77e49..785e0a7 100644
> >> --- a/hw/piix_pci.c
> >> +++ b/hw/piix_pci.c
> >> @@ -32,6 +32,7 @@
> >>  #include "xen.h"
> >>  #include "pam.h"
> >>  #include "sysemu/sysemu.h"
> >> +#include "sysemu/cpus.h"
> >>  
> >>  /*
> >>   * I440FX chipset data sheet.
> >> @@ -521,8 +522,11 @@ static void rcr_write(void *opaque, hwaddr addr, uint64_t val, unsigned len)
> >>      PIIX3State *d = opaque;
> >>  
> >>      if (val & 4) {
> >> -        qemu_system_reset_request();
> >> -        return;
> >> +        if (val & 2) {
> >> +            qemu_system_reset_request();
> >> +        } else {
> >> +            cpu_reset_all_async();
> >> +        }
> >>      }
> >>      d->rcr = val & 2; /* keep System Reset type only */
> >>  }
> > 
> > 
> > 
> > 
> 

Re: [Qemu-devel] [PATCH v2 3/3] hw: correctly implement soft reset

[Paolo Bonzini]

Il 06/03/2013 10:06, li guang ha scritto:
> 在 2013-03-06三的 09:36 +0100，Paolo Bonzini写道：
>> > Il 06/03/2013 03:02, li guang ha scritto:
>>> > > 在 2013-03-05二的 20:00 +0100，Paolo Bonzini写道：
>>>> > >> Do not do a hard reset for port 92h, keyboard controller, or cf9h soft reset.
>>>> > >> These only reset the CPU.
>>>> > >>
>>>> > >> Reviewed-by: Anthony Liguori <aliguori@us.ibm.com>
>>>> > >> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>>>> > >> ---
>>>> > >>  hw/lpc_ich9.c | 7 ++++++-
>>>> > >>  hw/pc.c       | 3 ++-
>>>> > >>  hw/pckbd.c    | 5 +++--
>>>> > >>  hw/piix_pci.c | 8 ++++++--
>>>> > >>  4 files changed, 17 insertions(+), 6 deletions(-)
>>>> > >>
>>>> > >> diff --git a/hw/lpc_ich9.c b/hw/lpc_ich9.c
>>>> > >> index e473758..5540f61 100644
>>>> > >> --- a/hw/lpc_ich9.c
>>>> > >> +++ b/hw/lpc_ich9.c
>>>> > >> @@ -45,6 +45,7 @@
>>>> > >>  #include "pci/pci_bus.h"
>>>> > >>  #include "exec/address-spaces.h"
>>>> > >>  #include "sysemu/sysemu.h"
>>>> > >> +#include "sysemu/cpus.h"
>>>> > >>  
>>>> > >>  static int ich9_lpc_sci_irq(ICH9LPCState *lpc);
>>>> > >>  
>>>> > >> @@ -506,7 +507,11 @@ static void ich9_rst_cnt_write(void *opaque, hwaddr addr, uint64_t val,
>>>> > >>      ICH9LPCState *lpc = opaque;
>>>> > >>  
>>>> > >>      if (val & 4) {
>>>> > >> -        qemu_system_reset_request();
>>>> > >> +        if (val & 0xA) {
>>>> > >> +            qemu_system_reset_request();
>>>> > >> +        } else {
>>>> > >> +            cpu_reset_all_async();
>>>> > >> +        }
>>>> > >>          return;
>>> > > 
>>> > > in fact, soft reset is cpu reset, hard reset is platform reset,
>>> > > it is too harsh to require both bit 3 & 1 to do a system reset,
>>> > > they are independent, either of them can trigger that.
>> > 
>> > This is "full reset if (val & 0xA) != 0".  You interpreted it as (val &
>> > 0xA) = 0xA, I think.
>
> No, IMHO, full reset should be done only by bit 4.

Ah, you mean by bit 1, as in bit 3 only being consulted if bit 1 is 1.
Hence, writing 0xC should do an INIT# cycle, not a full reset.  It would
matter if you want a watchdog to do a full power cycle (asserting
SLP_S3/4/5#), but you want to use cf9h to do a CPU reset.  That could be
the case.

> that is bit 1 & 2 are combined to determined reset cpu/system.
> and system reset does not always be the same with full reset(
> mostly be different in hardware signal of power cycle),

The ICH9 datasheet says exactly how the two differ.  For a full reset,
ICH9 will drive SLP_S3/4/5# low for 3-5 seconds.  We do not emulate this
in QEMU.

> it's up to the board hardware designer.
> 
>> > 
>>>> > >>      }
>>>> > >>      lpc->rst_cnt = val & 0xA; /* keep FULL_RST (bit 3) and SYS_RST (bit 1) */
>>>> > >> diff --git a/hw/pc.c b/hw/pc.c
>>>> > >> index 3e1cf2e..54f5b72 100644
>>>> > >> --- a/hw/pc.c
>>>> > >> +++ b/hw/pc.c
>>>> > >> @@ -45,6 +45,7 @@
>>>> > >>  #include "kvm_i386.h"
>>>> > >>  #include "xen.h"
>>>> > >>  #include "sysemu/blockdev.h"
>>>> > >> +#include "sysemu/cpus.h"
>>>> > >>  #include "hw/block-common.h"
>>>> > >>  #include "ui/qemu-spice.h"
>>>> > >>  #include "exec/memory.h"
>>>> > >> @@ -443,7 +444,7 @@ static void port92_write(void *opaque, hwaddr addr, uint64_t val,
>>>> > >>      s->outport = val;
>>>> > >>      qemu_set_irq(*s->a20_out, (val >> 1) & 1);
>>>> > >>      if ((val & 1) && !(oldval & 1)) {
>>>> > >> -        qemu_system_reset_request();
>>>> > >> +        cpu_reset_all_async();
>>>> > >>      }
>>>> > >>  }
>>>> > >>  
>>>> > >> diff --git a/hw/pckbd.c b/hw/pckbd.c
>>>> > >> index 3bad09b..fd66788 100644
>>>> > >> --- a/hw/pckbd.c
>>>> > >> +++ b/hw/pckbd.c
>>>> > >> @@ -26,6 +26,7 @@
>>>> > >>  #include "pc.h"
>>>> > >>  #include "ps2.h"
>>>> > >>  #include "sysemu/sysemu.h"
>>>> > >> +#include "sysemu/cpus.h"
>>>> > >>  
>>>> > >>  /* debug PC keyboard */
>>>> > >>  //#define DEBUG_KBD
>>>> > >> @@ -220,7 +221,7 @@ static void outport_write(KBDState *s, uint32_t val)
>>>> > >>          qemu_set_irq(*s->a20_out, (val >> 1) & 1);
>>>> > >>      }
>>>> > >>      if (!(val & 1)) {
>>>> > >> -        qemu_system_reset_request();
>>>> > >> +        cpu_reset_all_async();
>>>> > >>      }
>>>> > >>  }
>>>> > >>  
>>>> > >> @@ -299,7 +300,7 @@ static void kbd_write_command(void *opaque, hwaddr addr,
>>>> > >>          s->outport &= ~KBD_OUT_A20;
>>>> > >>          break;
>>>> > >>      case KBD_CCMD_RESET:
>>>> > >> -        qemu_system_reset_request();
>>>> > >> +        cpu_reset_all_async();
>>> > > 
>>> > > Oh, no, system reset is correct.
>> > 
>> > No, the keyboard controller is connected to the CPU reset signal.
> Not always, AFAIK,
> traditionally, this should do system reset,
> though system reset may be triggered by INIT# which 
> I guess is you said "CPU reset signal".

Some websites say it is a system reset, but I don't think it's ever been
correct.  But on PIIX and ICH9 the keyboard controller reset is
connected to INIT#, which is definitely not a system reset.

Paolo

Re: [Qemu-devel] [PATCH v2 0/3] Implement x86 soft reset

[Laszlo Ersek]

On 03/05/13 20:00, Paolo Bonzini wrote:
> This series makes various devices (port 92h, pckbd and the
> PIIX4/ICH9 southbridges) implement x86 soft reset correctly.
> 
> v1->v2: rebased onto Andreas's branch, fixed target-ppc/cpu.h,
>         renamed function
> 
> Paolo Bonzini (3):
>   cpu: make CPU_INTERRUPT_RESET available on all targets
>   pc: port 92 reset requires a low->high transition
>   hw: correctly implement soft reset
> 
>  cpu-exec.c             | 24 ++++++++++++++----------
>  cpus.c                 |  9 +++++++++
>  hw/lpc_ich9.c          |  7 ++++++-
>  hw/pc.c                |  6 ++++--
>  hw/pckbd.c             |  5 +++--
>  hw/piix_pci.c          |  8 ++++++--
>  include/exec/cpu-all.h |  8 +++++---
>  include/sysemu/cpus.h  |  1 +
>  target-i386/cpu.h      |  7 ++++---
>  target-ppc/cpu.h       |  3 ---
>  10 files changed, 52 insertions(+), 26 deletions(-)
> 

You haven't addressed the difference on soft reset between
ich9_rst_cnt_write() and rcr_write() that I pointed out in v1 3/3, but I
can live with that.

Series
Reviewed-by: Laszlo Ersek <lersek@redhat.com>