| Message ID | 1362510056-3316-2-git-send-email-pbonzini@redhat.com |
|---|---|
| State | New |
| Headers | show |
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
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 > > >
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
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
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
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
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 > >
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)