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

[Qemu-devel] [PATCH 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.

Paolo Bonzini (3):
  cpu: make CPU_INTERRUPT_RESET available on all targets
  pc: port 92 reset requires a low->high transition
  x86: 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 ++++---
 9 files changed, 52 insertions(+), 23 deletions(-)

-- 
1.8.1.4

[Qemu-devel] [PATCH 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.

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 ++++---
 5 files changed, 33 insertions(+), 16 deletions(-)

diff --git a/cpu-exec.c b/cpu-exec.c
index 9092145..e48bb6c 100644
--- a/cpu-exec.c
+++ b/cpu-exec.c
@@ -300,19 +300,26 @@ int cpu_exec(CPUArchState *env)
                     }
 #endif
 #if defined(TARGET_I386)
-#if !defined(CONFIG_USER_ONLY)
-                    if (interrupt_request & CPU_INTERRUPT_POLL) {
-                        env->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) {
+                        env->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) &&
@@ -365,9 +372,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 c4b021d..665175d 100644
--- a/cpus.c
+++ b/cpus.c
@@ -405,6 +405,15 @@ void hw_error(const char *fmt, ...)
     abort();
 }
 
+void cpu_soft_reset(void)
+{
+    CPUArchState *env;
+
+    for (env = first_cpu; env; env = env->next_cpu) {
+        cpu_interrupt(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 249e046..1361d22 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_soft_reset(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 493dda8..73dacdd 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 */
-- 
1.8.1.4

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

[Anthony Liguori]

Paolo Bonzini <pbonzini@redhat.com> writes:

> 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.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

This is a very nice approach.

It would still be nice to remove the explicit reset registrations for
the various cpus and instead just call cpu_soft_reset().

I'm not sure "soft" is the best word.  This is just a normal CPU reset
but I won't bikeshed over that.

Reviewed-by: Anthony Liguori <aliguori@us.ibm.com>

Regards,

Anthony Liguori

> ---
>  cpu-exec.c             | 24 ++++++++++++++----------
>  cpus.c                 |  9 +++++++++
>  include/exec/cpu-all.h |  8 +++++---
>  include/sysemu/cpus.h  |  1 +
>  target-i386/cpu.h      |  7 ++++---
>  5 files changed, 33 insertions(+), 16 deletions(-)
>
> diff --git a/cpu-exec.c b/cpu-exec.c
> index 9092145..e48bb6c 100644
> --- a/cpu-exec.c
> +++ b/cpu-exec.c
> @@ -300,19 +300,26 @@ int cpu_exec(CPUArchState *env)
>                      }
>  #endif
>  #if defined(TARGET_I386)
> -#if !defined(CONFIG_USER_ONLY)
> -                    if (interrupt_request & CPU_INTERRUPT_POLL) {
> -                        env->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) {
> +                        env->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) &&
> @@ -365,9 +372,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 c4b021d..665175d 100644
> --- a/cpus.c
> +++ b/cpus.c
> @@ -405,6 +405,15 @@ void hw_error(const char *fmt, ...)
>      abort();
>  }
>  
> +void cpu_soft_reset(void)
> +{
> +    CPUArchState *env;
> +
> +    for (env = first_cpu; env; env = env->next_cpu) {
> +        cpu_interrupt(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 249e046..1361d22 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_soft_reset(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 493dda8..73dacdd 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 */
> -- 
> 1.8.1.4

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

[Andreas Färber]

Am 05.03.2013 16:38, schrieb Anthony Liguori:
> Paolo Bonzini <pbonzini@redhat.com> writes:
> 
>> 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.
>>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> 
> This is a very nice approach.
> 
> It would still be nice to remove the explicit reset registrations for
> the various cpus and instead just call cpu_soft_reset().
> 
> I'm not sure "soft" is the best word.  This is just a normal CPU reset
> but I won't bikeshed over that.
> 
> Reviewed-by: Anthony Liguori <aliguori@us.ibm.com>

I would like to take this through qom-cpu please since I already changed
cpu_interrupt() to CPUState with my last patch series, for which the
prereqs got an Rb now.

cpu_soft_reset() still needs to live outside qom/cpu.c due to the CPU
iteration, so this would be purely mechanical.

Regards,
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 1/3] cpu: make CPU_INTERRUPT_RESET available on all targets

[Paolo Bonzini]

Il 05/03/2013 17:10, Andreas Färber ha scritto:
> I would like to take this through qom-cpu please since I already changed
> cpu_interrupt() to CPUState with my last patch series, for which the
> prereqs got an Rb now.
> 
> cpu_soft_reset() still needs to live outside qom/cpu.c due to the CPU
> iteration, so this would be purely mechanical.

Fine by me.

Paolo

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

[Laszlo Ersek]

Comments in-line.

(In advance, can you please change your format-series alias so that
format-patch gets "-O/abs/path/to/order-file", where "order-file" puts
*.h before *.c? Thanks.)

On 03/05/13 16:04, Paolo Bonzini 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.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>


> diff --git a/include/exec/cpu-all.h b/include/exec/cpu-all.h
> index 249e046..1361d22 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.  */
>

I think this is the basis of the patch. CPUArchState.interrupt_request
is a bitmask of pending interrupts, apparently. According to commit
9c76219e, CPU_INTERRUPT_TGT_INT_* should never be used directly (hence
the poisoning); targets should #define their own internal interrupts in
terms of these (so that they fit in the bits reserved for this purpose.
Here you create a new "global" (= interpreted for all targets) interrupt
bit, but since there are probably no more free bits, you have to shrink
the CPU_INTERRUPT_TGT_INT_* pool. (Why do you choose to shrink that
pool?)

Most probably targets will use up this pool from INT_1 upwards, so you
remove INT_3. (This could answer my previous question: nobody used to
use INT_3 except target-i386... git grep agrees. OTOH an "external"
interrupt might be cleaner, no?, since at least according to commit
9c76219e, INT_* originates from within the CPU, and 0xCF9 etc would
qualify as "external hardware interrupt" I guess.)

What I don't understand is why you don't just kill off
CPU_INTERRUPT_TGT_INT_3 from the list, and #define CPU_INTERRUPT_RESET
with value 0x2000? It looks as if the value 0x0400 was special and you
insisted on that. I don't see the reason. Plus this "sliding up" of
values changes the meaning for INT_1 and INT_2.

(I can see the second PPC reference in the commit message but I don't
understand it.)

(BTW CPU_INTERRUPT_TGT_INT_3 had not been poisoned like INT_2 and
below.)


> diff --git a/target-i386/cpu.h b/target-i386/cpu.h
> index 493dda8..73dacdd 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 */
>

OK, we want to express CPU_INTERRUPT_INIT in terms of
CPU_INTERRUPT_RESET. We also want to keep its value unchanged (for
migration purposes), so that's the answer to my previous question. Then,
since you hoisted CPU_INTERRUPT_TGT_INT_1 from the pool (slid up the
rest in order to keep the macro names contiguous), you have to adjust
the references here, again for migration's sake. There are no other
references to INT_[123] in the tree so we don't have to patch up those.


> 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_soft_reset(void);
>  void cpu_synchronize_all_states(void);
>  void cpu_synchronize_all_post_reset(void);
>  void cpu_synchronize_all_post_init(void);

> diff --git a/cpus.c b/cpus.c
> index c4b021d..665175d 100644
> --- a/cpus.c
> +++ b/cpus.c
> @@ -405,6 +405,15 @@ void hw_error(const char *fmt, ...)
>      abort();
>  }
>
> +void cpu_soft_reset(void)
> +{
> +    CPUArchState *env;
> +
> +    for (env = first_cpu; env; env = env->next_cpu) {
> +        cpu_interrupt(env, CPU_INTERRUPT_RESET);
> +    }
> +}
> +
>  void cpu_synchronize_all_states(void)
>  {
>      CPUArchState *cpu;

This adds a generic function that sets the new CPU_INTERRUPT_RESET bit
in the mask. For target-i386 this makes CPU_INTERRUPT_INIT pending.


> diff --git a/cpu-exec.c b/cpu-exec.c
> index 9092145..e48bb6c 100644
> --- a/cpu-exec.c
> +++ b/cpu-exec.c
> @@ -300,19 +300,26 @@ int cpu_exec(CPUArchState *env)
>                      }
>  #endif
>  #if defined(TARGET_I386)
> -#if !defined(CONFIG_USER_ONLY)
> -                    if (interrupt_request & CPU_INTERRUPT_POLL) {
> -                        env->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) {
> +                        env->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) &&
> @@ -365,9 +372,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)

Before:
(b1) on i386 and not user-only: check for CPU_INTERRUPT_POLL
(b2) on i386: check for CPU_INTERRUPT_INIT
(b3) on i386: if CPU_INTERRUPT_INIT was clear, check for
     CPU_INTERRUPT_SIPI
(b4) on ppc: check for CPU_INTERRUPT_RESET

After:
(a1) on i386, check for CPU_INTERRUPT_INIT
(a2) on non-i386, including PPC, check for CPU_INTERRUPT_RESET
(a3) on i386 and not user-only: check for CPU_INTERRUPT_POLL
(a4) on i386, check for CPU_INTERRUPT_SIPI independently of
     CPU_INTERRUPT_INIT

b1/b2/b3 is reordered to a3/a1/a4, inverting the relative order of
INIT/POLL, and SIPI can co-incide (on the surface) with INIT now. I
trust this doesn't matter.

I now understand the first PPC reference in the commit message.


... Since for the PPC target the macro CPU_INTERRUPT_RESET has already
existed:

target-ppc/cpu.h:#define CPU_INTERRUPT_RESET       CPU_INTERRUPT_TGT_INT_0

won't you now get a macro definition conflict between "target-ppc/cpu.h"
and "include/exec/cpu-all.h", when building for target PPC? I think you
should just drop the #define in "target-ppc/cpu.h": if, as you say, PPC
doesn't support migration, its CPU_INTERRUPT_RESET is allowed to change
from CPU_INTERRUPT_TGT_INT_0 (0x0100) to 0x0400.

The patch juggles an impressive number of bits.

Thanks,
Laszlo

[Qemu-devel] [PATCH 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 07caba7..523db1f 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -435,11 +435,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.4

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

[Anthony Liguori]

Paolo Bonzini <pbonzini@redhat.com> writes:

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

Reviewed-by: Anthony Liguori <aliguori@us.ibm.com>

Regards,

Anthony Liguori

> ---
>  hw/pc.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/hw/pc.c b/hw/pc.c
> index 07caba7..523db1f 100644
> --- a/hw/pc.c
> +++ b/hw/pc.c
> @@ -435,11 +435,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.4

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

[Laszlo Ersek]

On 03/05/13 16:04, Paolo Bonzini wrote:
> 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 07caba7..523db1f 100644
> --- a/hw/pc.c
> +++ b/hw/pc.c
> @@ -435,11 +435,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();
>      }
>  }
> 

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

[Qemu-devel] [PATCH 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.

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 eceb052..fae31df 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_soft_reset();
+        }
         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 523db1f..6080d62 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"
@@ -441,7 +442,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_soft_reset();
     }
 }
 
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_soft_reset();
     }
 }
 
@@ -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_soft_reset();
         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_soft_reset();
+        }
     }
     d->rcr = val & 2; /* keep System Reset type only */
 }
-- 
1.8.1.4

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

[Anthony Liguori]

Paolo Bonzini <pbonzini@redhat.com> writes:

> Do not do a hard reset for port 92h, keyboard controller, or cf9h soft reset.
> These only reset the CPU.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

I'm quite confident these devices should trigger a soft reset but not
confident this is exhaustive.

Reviewed-by: Anthony Liguori <aliguori@us.ibm.com>

Regards,

Anthony Liguori

> ---
>  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 eceb052..fae31df 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_soft_reset();
> +        }
>          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 523db1f..6080d62 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"
> @@ -441,7 +442,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_soft_reset();
>      }
>  }
>  
> 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_soft_reset();
>      }
>  }
>  
> @@ -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_soft_reset();
>          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_soft_reset();
> +        }
>      }
>      d->rcr = val & 2; /* keep System Reset type only */
>  }
> -- 
> 1.8.1.4

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

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 241 bytes --]

On Tue, 2013-03-05 at 16:04 +0100, Paolo Bonzini wrote:
> Do not do a hard reset for port 92h, keyboard controller, or cf9h soft
> reset. These only reset the CPU.

Is a keyboard reset also supposed to reset the A20 gate?

-- 
dwmw2

[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 6171 bytes --]

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

[Paolo Bonzini]

Il 05/03/2013 18:22, David Woodhouse ha scritto:
> On Tue, 2013-03-05 at 16:04 +0100, Paolo Bonzini wrote:
>> > Do not do a hard reset for port 92h, keyboard controller, or cf9h soft
>> > reset. These only reset the CPU.
> Is a keyboard reset also supposed to reset the A20 gate?

Resetting with 0xFE is not supposed to reset the A20 gate.

Resetting with 0xD1 will set the A20 gate to whatever value you place in
the data byte.  (Also, resetting with 0xD1 for what I could find does
not have the same edge-triggered behavior as port 92).

Paolo

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

[Laszlo Ersek]

comments in-line

On 03/05/13 16:04, Paolo Bonzini wrote:
> Do not do a hard reset for port 92h, keyboard controller, or cf9h soft reset.
> These only reset the CPU.
> 
> 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 eceb052..fae31df 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_soft_reset();
> +        }
>          return;
>      }
>      lpc->rst_cnt = val & 0xA; /* keep FULL_RST (bit 3) and SYS_RST (bit 1) */

So any of FULL_RST and SYS_RST render it hard.


> diff --git a/hw/pc.c b/hw/pc.c
> index 523db1f..6080d62 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"
> @@ -441,7 +442,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_soft_reset();
>      }
>  }

I checked this against the data-sheet, OK.

>  
> 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_soft_reset();
>      }
>  }
>  
> @@ -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_soft_reset();
>          break;
>      case KBD_CCMD_NO_OP:
>          /* ignore that */

I couldn't find the datasheet for this, but
<http://wiki.osdev.org/%228042%22_PS/2_Controller> seems to confirm that
both of these should trigger a reset. Not sure about hard vs. soft.

> 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_soft_reset();
> +        }
>      }
>      d->rcr = val & 2; /* keep System Reset type only */
>  }
> 

This is slightly different from your ICH9 change at the top: you remove
the "return" statement only here. For a hard reset it doesn't matter,
but in case of a soft reset, PIIX3State.rcr will updated in a
guest-visible way, while ICH9LPCState.rst_cnt won't be updated at all.

I guess we should unify these by dropping the "return" from
ich9_rst_cnt_write() -- the other bits changed simultaneously when
kicking the Reset CPU bit should be visible after a soft reset, I think.

Laszlo

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

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 473 bytes --]

On Tue, 2013-03-05 at 16:04 +0100, Paolo Bonzini wrote:
> This series makes various devices (port 92h, pckbd and the
> PIIX4/ICH9 southbridges) implement x86 soft reset correctly.

How about suspend/resume?

If you apply my patch which "fixes" the hard reset for i440FX to
actually reset the PAM configuration, will it no longer "break"
suspend/resume?

(I think even my three cleanup patches for i440fx still haven't been
pulled, have they? :( )

-- 
dwmw2

[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 6171 bytes --]

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

[Paolo Bonzini]

Il 05/03/2013 16:51, David Woodhouse ha scritto:
> On Tue, 2013-03-05 at 16:04 +0100, Paolo Bonzini wrote:
>> This series makes various devices (port 92h, pckbd and the
>> PIIX4/ICH9 southbridges) implement x86 soft reset correctly.
> 
> How about suspend/resume?
> 
> If you apply my patch which "fixes" the hard reset for i440FX to
> actually reset the PAM configuration, will it no longer "break"
> suspend/resume?

Hmm, I forgot about that issue.  It needs a trivial follow-up patch,
I'll send it as patch 4/3.

> (I think even my three cleanup patches for i440fx still haven't been
> pulled, have they? :( )

No.  Michael, can you look at David's "[PATCH 0/3] piix_pci cleanups"?
I just sent a ping.

Paolo

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

[Andreas Färber]

Am 05.03.2013 17:00, schrieb Paolo Bonzini:
> Il 05/03/2013 16:51, David Woodhouse ha scritto:
>> (I think even my three cleanup patches for i440fx still haven't been
>> pulled, have they? :( )
> 
> No.  Michael, can you look at David's "[PATCH 0/3] piix_pci cleanups"?
> I just sent a ping.

I asked Anthony on IRC yesterday, he said he wanted to apply them
directly. May the fastest win. ;-)

Andreas

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

[Qemu-devel] [PATCH 4/3] wakeup: only reset the CPU

[Paolo Bonzini]

Resuming from suspend-to-RAM should not reset all devices.  Only the CPU
should get a reset signal.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 vl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/vl.c b/vl.c
index c03edf1..2c93478 100644
--- a/vl.c
+++ b/vl.c
@@ -1973,8 +1973,8 @@ static bool main_loop_should_exit(void)
     }
     if (qemu_wakeup_requested()) {
         pause_all_vcpus();
+        cpu_soft_reset();
         cpu_synchronize_all_states();
-        qemu_system_reset(VMRESET_SILENT);
         resume_all_vcpus();
         monitor_protocol_event(QEVENT_WAKEUP, NULL);
     }
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH 4/3] wakeup: only reset the CPU

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 782 bytes --]

On Tue, 2013-03-05 at 17:03 +0100, Paolo Bonzini wrote:
> Resuming from suspend-to-RAM should not reset all devices.  Only the
> CPU should get a reset signal.

Hm... on reflection, I don't actually know if this is true.

Perhaps we *should* reset all devices. After all, in a real machine
they'll all have been turned off and the RAM will have been in
self-refresh. Surely they have to be reset?

So maybe we should *let* the i440FX PAM registers get reset to point to
ROM. And fix the firmware to *cope* with that, check to see if the
shadow RAM already holds an image of a started-up firmware with the
correct checksum, and jump back to it.

That is: perhaps it's a *SeaBIOS* bug that suspend/resume doesn't work
if the PAM configuration is reset?

-- 
dwmw2

[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 6171 bytes --]

Re: [Qemu-devel] [PATCH 4/3] wakeup: only reset the CPU

[Paolo Bonzini]

Il 05/03/2013 17:59, David Woodhouse ha scritto:
> On Tue, 2013-03-05 at 17:03 +0100, Paolo Bonzini wrote:
>> > Resuming from suspend-to-RAM should not reset all devices.  Only the
>> > CPU should get a reset signal.
> Hm... on reflection, I don't actually know if this is true.
> 
> Perhaps we *should* reset all devices. After all, in a real machine
> they'll all have been turned off and the RAM will have been in
> self-refresh. Surely they have to be reset?
> 
> So maybe we should *let* the i440FX PAM registers get reset to point to
> ROM. And fix the firmware to *cope* with that, check to see if the
> shadow RAM already holds an image of a started-up firmware with the
> correct checksum, and jump back to it.
> 
> That is: perhaps it's a *SeaBIOS* bug that suspend/resume doesn't work
> if the PAM configuration is reset?

Yeah, it sounded a bit weird when I wrote that commit message.  This
could be the case.  How does it work on Coreboot?

Paolo

Re: [Qemu-devel] [PATCH 4/3] wakeup: only reset the CPU

[Peter Stuge]

Paolo Bonzini wrote:
> > That is: perhaps it's a *SeaBIOS* bug that suspend/resume doesn't
> > work if the PAM configuration is reset?
> 
> Yeah, it sounded a bit weird when I wrote that commit message. 
> This could be the case.  How does it work on Coreboot?

Yes all hardware except RAM in self-refresh and registers explicitly
hooked to battery sleep power rail has reset state on resume from S3.
Those registers seem to grow over time, the i440 probably doesn't
have very many. The memory controller configuration would be stored
there.

Power on and resume are identical except for memory controller init
and what should happen once init is finished.

On power on jump to payload.

On resume jump to S3 resume vector set by OSPM.

This is another case where coreboot has infrastructure since a long
time, which can be used by hypervisors. Please don't replicate that
into SeaBIOS. In the end you will have what is essentially a fork of
coreboot, which seems like a tremendous waste of effort.

See if there is a 440 board in coreboot where suspend works. If yes,
then making that work for the QEMU board should take only moderate
effort.


//Peter

Re: [Qemu-devel] [PATCH 4/3] wakeup: only reset the CPU

[Paolo Bonzini]

Il 05/03/2013 18:26, Peter Stuge ha scritto:
>>> > > That is: perhaps it's a *SeaBIOS* bug that suspend/resume doesn't
>>> > > work if the PAM configuration is reset?
>> > 
>> > Yeah, it sounded a bit weird when I wrote that commit message. 
>> > This could be the case.  How does it work on Coreboot?
> Yes all hardware except RAM in self-refresh and registers explicitly
> hooked to battery sleep power rail has reset state on resume from S3.
> Those registers seem to grow over time, the i440 probably doesn't
> have very many. The memory controller configuration would be stored
> there.

Ok, so it's not correct either with this patch or without. :)

But without this patch it is more correct; things such as USB queue
heads must be reset, or you'll corrupt random memory when the VM is
restarted.

Paolo

Re: [Qemu-devel] [PATCH 4/3] wakeup: only reset the CPU

[Laszlo Ersek]

On 03/05/13 17:59, David Woodhouse wrote:> On Tue, 2013-03-05 at 17:03 +0100, Paolo Bonzini wrote:
>> Resuming from suspend-to-RAM should not reset all devices.  Only the
>> CPU should get a reset signal.
>
> Hm... on reflection, I don't actually know if this is true.
>
> Perhaps we *should* reset all devices. After all, in a real machine
> they'll all have been turned off and the RAM will have been in
> self-refresh. Surely they have to be reset?
>
> So maybe we should *let* the i440FX PAM registers get reset to point to
> ROM. And fix the firmware to *cope* with that, check to see if the
> shadow RAM already holds an image of a started-up firmware with the
> correct checksum, and jump back to it.
>
> That is: perhaps it's a *SeaBIOS* bug that suspend/resume doesn't work
> if the PAM configuration is reset?

I think it is indeed a problem with SeaBIOS. (git.seabios.org
(80.81.252.135) seems to be down ATM, so I can only check at f465e1ec.)
Open romlayout.S:

   622  reset_vector:
   623          ljmpw $SEG_BIOS, $entry_post

   537  entry_post:
   538          cmpl $0, %cs:HaveRunPost                // Check for resume/reboot
   539          jnz entry_resume
   540          ENTRY_INTO32 _cfunc32flat_handle_post   // Normal entry point

   239  entry_resume:
...
   248          // Call handler.
   249          jmp handle_resume

handle_resume() [src/resume.c]
  inb_cmos(CMOS_RESET_CODE)

It checks the CMOS only after looking at HaveRunPost. The value of
HaveRunPost depends on the PAM settings. It's always 0 in ROM, in which
case we continue at handle_post() [src/post.c].

Laszlo

Re: [Qemu-devel] [PATCH 4/3] wakeup: only reset the CPU

[Paolo Bonzini]

Il 05/03/2013 20:12, Laszlo Ersek ha scritto:
> On 03/05/13 17:59, David Woodhouse wrote:> On Tue, 2013-03-05 at 17:03 +0100, Paolo Bonzini wrote:
>>> Resuming from suspend-to-RAM should not reset all devices.  Only the
>>> CPU should get a reset signal.
>>
>> Hm... on reflection, I don't actually know if this is true.
>>
>> Perhaps we *should* reset all devices. After all, in a real machine
>> they'll all have been turned off and the RAM will have been in
>> self-refresh. Surely they have to be reset?
>>
>> So maybe we should *let* the i440FX PAM registers get reset to point to
>> ROM. And fix the firmware to *cope* with that, check to see if the
>> shadow RAM already holds an image of a started-up firmware with the
>> correct checksum, and jump back to it.
>>
>> That is: perhaps it's a *SeaBIOS* bug that suspend/resume doesn't work
>> if the PAM configuration is reset?
> 
> I think it is indeed a problem with SeaBIOS. Open romlayout.S: [...]
>
> It checks the CMOS only after looking at HaveRunPost. The value of
> HaveRunPost depends on the PAM settings. It's always 0 in ROM, in which
> case we continue at handle_post() [src/post.c].

Actually, Peter explained that it is okay.  S3 doesn't clear the PAM
configuration, but S4 does.  The PAM registers are attached to the same
power line as the RAM.

Paolo

Re: [Qemu-devel] [PATCH 4/3] wakeup: only reset the CPU

[Laszlo Ersek]

On 03/05/13 20:25, Paolo Bonzini wrote:
> Il 05/03/2013 20:12, Laszlo Ersek ha scritto:
>> On 03/05/13 17:59, David Woodhouse wrote:> On Tue, 2013-03-05 at 17:03 +0100, Paolo Bonzini wrote:
>>>> Resuming from suspend-to-RAM should not reset all devices.  Only the
>>>> CPU should get a reset signal.
>>>
>>> Hm... on reflection, I don't actually know if this is true.
>>>
>>> Perhaps we *should* reset all devices. After all, in a real machine
>>> they'll all have been turned off and the RAM will have been in
>>> self-refresh. Surely they have to be reset?
>>>
>>> So maybe we should *let* the i440FX PAM registers get reset to point to
>>> ROM. And fix the firmware to *cope* with that, check to see if the
>>> shadow RAM already holds an image of a started-up firmware with the
>>> correct checksum, and jump back to it.
>>>
>>> That is: perhaps it's a *SeaBIOS* bug that suspend/resume doesn't work
>>> if the PAM configuration is reset?
>>
>> I think it is indeed a problem with SeaBIOS. Open romlayout.S: [...]
>>
>> It checks the CMOS only after looking at HaveRunPost. The value of
>> HaveRunPost depends on the PAM settings. It's always 0 in ROM, in which
>> case we continue at handle_post() [src/post.c].
> 
> Actually, Peter explained that it is okay.  S3 doesn't clear the PAM
> configuration, but S4 does.  The PAM registers are attached to the same
> power line as the RAM.

Ah, you expect me to understand that "memory controller configuration"
is a superset of the PAM registers :)

Laszlo

Re: [Qemu-devel] [PATCH 4/3] wakeup: only reset the CPU

[Kevin O'Connor]

On Tue, Mar 05, 2013 at 04:59:51PM +0000, David Woodhouse wrote:
> On Tue, 2013-03-05 at 17:03 +0100, Paolo Bonzini wrote:
> > Resuming from suspend-to-RAM should not reset all devices.  Only the
> > CPU should get a reset signal.
> 
> Hm... on reflection, I don't actually know if this is true.
> 
> Perhaps we *should* reset all devices. After all, in a real machine
> they'll all have been turned off and the RAM will have been in
> self-refresh. Surely they have to be reset?
> 
> So maybe we should *let* the i440FX PAM registers get reset to point to
> ROM. And fix the firmware to *cope* with that, check to see if the
> shadow RAM already holds an image of a started-up firmware with the
> correct checksum, and jump back to it.
> 
> That is: perhaps it's a *SeaBIOS* bug that suspend/resume doesn't work
> if the PAM configuration is reset?

On a real machine the firmware would query the available memory and
its settings via the smbus and then program the memory controller.
Programming the memory controller without accessing any memory is a
non-trivial task.  QEMU doesn't emulate this low-level hardware - nor
does it even fully emulate the PAM registers.

So, I think the question isn't what does real-hardware do (there's no
gain to be had in emulating all of this).  Instead, I think the
question is - what makes the most sense.  Given that the objective is
to restore the memory image and reset everything else, I think it
would be best for QEMU's S3resume to completely restore the memory
image - including the PAM settings.

-Kevin

Re: [Qemu-devel] [PATCH 4/3] wakeup: only reset the CPU

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 909 bytes --]

On Tue, 2013-03-05 at 20:45 -0500, Kevin O'Connor wrote:
> 
> So, I think the question isn't what does real-hardware do (there's no
> gain to be had in emulating all of this).  Instead, I think the
> question is - what makes the most sense.  

Well,... what we implement for the 'native SeaBIOS on qemu' case doesn't
need to work on real hardware, it's true.

But a bunch of other combinations *do* need to work on real hardware, so
it's worth bearing it in mind to a certain extent.

We wouldn't want Coreboot and OVMF to have to have workarounds for qemu
behaving differently, just because we've done something simpler for
SeaBIOS. But it looks like the PAM configuration is considered part of
the memory setup and isn't lost in S3, so for the case I was wondering
about it's fine. Qemu *is* behaving like real hardware, which *is* the
simple option that makes most sense.

-- 
dwmw2


[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 6171 bytes --]