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 <pbonz...@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
