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 <address@hidden>


> 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