This patch series can be applied on top of Ian''s patch series "arm: host SMP support" The first patch fixes an issue on SMP board when smp_call_function is implemented. The second patch implements the function. Changes since V1: - Move smp_call_function and on_selected_cpus in the common part - Use spin_lock instead of spin_lock_irqsave in patch 1 Julien Grall (2): xen/arm: gic_disable_cpu must be called with interrupts disabled xen/arm: implement smp_call_function xen/arch/arm/gic.c | 9 +++- xen/arch/arm/smp.c | 11 ++--- xen/arch/x86/smp.c | 88 +++----------------------------------- xen/common/Makefile | 1 + xen/common/smp.c | 94 +++++++++++++++++++++++++++++++++++++++++ xen/include/asm-arm/gic.h | 1 + xen/include/asm-x86/hardirq.h | 1 + xen/include/xen/smp.h | 7 +++ 8 files changed, 120 insertions(+), 92 deletions(-) create mode 100644 xen/common/smp.c -- 1.7.10.4
Julien Grall
2013-Apr-16 13:38 UTC
[PATCH V2 1/2] xen/arm: gic_disable_cpu must be called with interrupts disabled
gic_disable_cpu is only called with interrupt disabled. Use spin_lock instead of spin_lock_irq and check the function is called with interrupts disabled. Signed-off-by: Julien Grall <julien.grall@citrix.com> --- xen/arch/arm/gic.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c index aa179a9..0ef994e 100644 --- a/xen/arch/arm/gic.c +++ b/xen/arch/arm/gic.c @@ -419,10 +419,12 @@ void __cpuinit gic_init_secondary_cpu(void) /* Shut down the per-CPU GIC interface */ void gic_disable_cpu(void) { - spin_lock_irq(&gic.lock); + ASSERT(!local_irq_is_enabled()); + + spin_lock(&gic.lock); gic_cpu_disable(); gic_hyp_disable(); - spin_unlock_irq(&gic.lock); + spin_unlock(&gic.lock); } void gic_route_ppis(void) -- 1.7.10.4
Move smp_call_function and on_selected_cpus to common code. Signed-off-by: Julien Grall <julien.grall@citrix.com> --- xen/arch/arm/gic.c | 3 ++ xen/arch/arm/smp.c | 11 ++--- xen/arch/x86/smp.c | 88 +++----------------------------------- xen/common/Makefile | 1 + xen/common/smp.c | 94 +++++++++++++++++++++++++++++++++++++++++ xen/include/asm-arm/gic.h | 1 + xen/include/asm-x86/hardirq.h | 1 + xen/include/xen/smp.h | 7 +++ 8 files changed, 116 insertions(+), 90 deletions(-) create mode 100644 xen/common/smp.c diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c index 0ef994e..82aa6fe 100644 --- a/xen/arch/arm/gic.c +++ b/xen/arch/arm/gic.c @@ -658,6 +658,9 @@ static void do_sgi(struct cpu_user_regs *regs, int othercpu, enum gic_sgi sgi) case GIC_SGI_DUMP_STATE: dump_execstate(regs); break; + case GIC_SGI_CALL_FUNCTION: + smp_call_function_interrupt(); + break; default: panic("Unhandled SGI %d on CPU%d\n", sgi, smp_processor_id()); break; diff --git a/xen/arch/arm/smp.c b/xen/arch/arm/smp.c index a902d84..4042db5 100644 --- a/xen/arch/arm/smp.c +++ b/xen/arch/arm/smp.c @@ -11,17 +11,14 @@ void flush_tlb_mask(const cpumask_t *mask) flush_xen_data_tlb(); } -void smp_call_function( - void (*func) (void *info), - void *info, - int wait) +void smp_send_event_check_mask(const cpumask_t *mask) { - panic("%s not implmented\n", __func__); + send_SGI_mask(mask, GIC_SGI_EVENT_CHECK); } -void smp_send_event_check_mask(const cpumask_t *mask) +void smp_send_call_function_mask(const cpumask_t *mask) { - send_SGI_mask(mask, GIC_SGI_EVENT_CHECK); + send_SGI_mask(mask, GIC_SGI_CALL_FUNCTION); } /* diff --git a/xen/arch/x86/smp.c b/xen/arch/x86/smp.c index a607531..0433f30 100644 --- a/xen/arch/x86/smp.c +++ b/xen/arch/x86/smp.c @@ -269,67 +269,16 @@ void smp_send_event_check_mask(const cpumask_t *mask) send_IPI_mask(mask, EVENT_CHECK_VECTOR); } -/* - * Structure and data for smp_call_function()/on_selected_cpus(). - */ - -static void __smp_call_function_interrupt(void); -static DEFINE_SPINLOCK(call_lock); -static struct call_data_struct { - void (*func) (void *info); - void *info; - int wait; - cpumask_t selected; -} call_data; - -void smp_call_function( - void (*func) (void *info), - void *info, - int wait) +void smp_send_call_function_mask(const cpumask_t *mask) { - cpumask_t allbutself; - - cpumask_andnot(&allbutself, &cpu_online_map, - cpumask_of(smp_processor_id())); - on_selected_cpus(&allbutself, func, info, wait); -} - -void on_selected_cpus( - const cpumask_t *selected, - void (*func) (void *info), - void *info, - int wait) -{ - unsigned int nr_cpus; - - ASSERT(local_irq_is_enabled()); - - spin_lock(&call_lock); - - cpumask_copy(&call_data.selected, selected); - - nr_cpus = cpumask_weight(&call_data.selected); - if ( nr_cpus == 0 ) - goto out; - - call_data.func = func; - call_data.info = info; - call_data.wait = wait; - - send_IPI_mask(&call_data.selected, CALL_FUNCTION_VECTOR); + send_IPI_mask(mask, CALL_FUNCTION_VECTOR); - if ( cpumask_test_cpu(smp_processor_id(), &call_data.selected) ) + if ( cpumask_test_cpu(smp_processor_id(), mask) ) { local_irq_disable(); - __smp_call_function_interrupt(); + smp_call_function_interrupt(); local_irq_enable(); } - - while ( !cpumask_empty(&call_data.selected) ) - cpu_relax(); - - out: - spin_unlock(&call_lock); } void __stop_this_cpu(void) @@ -390,36 +339,9 @@ void event_check_interrupt(struct cpu_user_regs *regs) this_cpu(irq_count)++; } -static void __smp_call_function_interrupt(void) -{ - void (*func)(void *info) = call_data.func; - void *info = call_data.info; - unsigned int cpu = smp_processor_id(); - - if ( !cpumask_test_cpu(cpu, &call_data.selected) ) - return; - - irq_enter(); - - if ( call_data.wait ) - { - (*func)(info); - mb(); - cpumask_clear_cpu(cpu, &call_data.selected); - } - else - { - mb(); - cpumask_clear_cpu(cpu, &call_data.selected); - (*func)(info); - } - - irq_exit(); -} - void call_function_interrupt(struct cpu_user_regs *regs) { ack_APIC_irq(); perfc_incr(ipis); - __smp_call_function_interrupt(); + smp_call_function_interrupt(); } diff --git a/xen/common/Makefile b/xen/common/Makefile index 8a0c506..0dc2050 100644 --- a/xen/common/Makefile +++ b/xen/common/Makefile @@ -26,6 +26,7 @@ obj-y += schedule.o obj-y += shutdown.o obj-y += softirq.o obj-y += sort.o +obj-y += smp.o obj-y += spinlock.o obj-y += stop_machine.o obj-y += string.o diff --git a/xen/common/smp.c b/xen/common/smp.c new file mode 100644 index 0000000..dcd93ad --- /dev/null +++ b/xen/common/smp.c @@ -0,0 +1,94 @@ +#include <asm/hardirq.h> +#include <asm/processor.h> +#include <xen/spinlock.h> +#include <xen/smp.h> + +/* + * Structure and data for smp_call_function()/on_selected_cpus(). + */ +static DEFINE_SPINLOCK(call_lock); +static struct call_data_struct { + void (*func) (void *info); + void *info; + int wait; + cpumask_t selected; +} call_data; + +void smp_call_function( + void (*func) (void *info), + void *info, + int wait) +{ + cpumask_t allbutself; + + cpumask_andnot(&allbutself, &cpu_online_map, + cpumask_of(smp_processor_id())); + on_selected_cpus(&allbutself, func, info, wait); +} + +void on_selected_cpus( + const cpumask_t *selected, + void (*func) (void *info), + void *info, + int wait) +{ + unsigned int nr_cpus; + + ASSERT(local_irq_is_enabled()); + + spin_lock(&call_lock); + + cpumask_copy(&call_data.selected, selected); + + nr_cpus = cpumask_weight(&call_data.selected); + if ( nr_cpus == 0 ) + goto out; + + call_data.func = func; + call_data.info = info; + call_data.wait = wait; + + smp_send_call_function_mask(&call_data.selected); + + while ( !cpumask_empty(&call_data.selected) ) + cpu_relax(); + +out: + spin_unlock(&call_lock); +} + +void smp_call_function_interrupt(void) +{ + void (*func)(void *info) = call_data.func; + void *info = call_data.info; + unsigned int cpu = smp_processor_id(); + + if ( !cpumask_test_cpu(cpu, &call_data.selected) ) + return; + + irq_enter(); + + if ( call_data.wait ) + { + (*func)(info); + mb(); + cpumask_clear_cpu(cpu, &call_data.selected); + } + else + { + mb(); + cpumask_clear_cpu(cpu, &call_data.selected); + (*func)(info); + } + + irq_exit(); +} + +/* + * Local variables: + * mode: C + * c-file-style: "BSD" + * c-basic-offset: 4 + * indent-tabs-mode: nil + * End: + */ diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h index 4f2c8b8..47354dd 100644 --- a/xen/include/asm-arm/gic.h +++ b/xen/include/asm-arm/gic.h @@ -173,6 +173,7 @@ extern void gic_restore_state(struct vcpu *v); enum gic_sgi { GIC_SGI_EVENT_CHECK = 0, GIC_SGI_DUMP_STATE = 1, + GIC_SGI_CALL_FUNCTION = 2, }; extern void send_SGI_mask(const cpumask_t *cpumask, enum gic_sgi sgi); extern void send_SGI_one(unsigned int cpu, enum gic_sgi sgi); diff --git a/xen/include/asm-x86/hardirq.h b/xen/include/asm-x86/hardirq.h index e573007..01a41b9 100644 --- a/xen/include/asm-x86/hardirq.h +++ b/xen/include/asm-x86/hardirq.h @@ -3,6 +3,7 @@ #include <xen/config.h> #include <xen/cache.h> +#include <xen/types.h> typedef struct { unsigned int __softirq_pending; diff --git a/xen/include/xen/smp.h b/xen/include/xen/smp.h index e05f390..6febb56 100644 --- a/xen/include/xen/smp.h +++ b/xen/include/xen/smp.h @@ -58,6 +58,13 @@ static inline void on_each_cpu( on_selected_cpus(&cpu_online_map, func, info, wait); } +/* + * Call a function on the current CPU + */ +void smp_call_function_interrupt(void); + +void smp_send_call_function_mask(const cpumask_t *mask); + #define smp_processor_id() raw_smp_processor_id() int alloc_cpu_id(void); -- 1.7.10.4
Ian Campbell
2013-Apr-16 14:16 UTC
Re: [PATCH V2 1/2] xen/arm: gic_disable_cpu must be called with interrupts disabled
On Tue, 2013-04-16 at 14:38 +0100, Julien Grall wrote:> gic_disable_cpu is only called with interrupt disabled. > Use spin_lock instead of spin_lock_irq and check the function is > called with interrupts disabled. > > Signed-off-by: Julien Grall <julien.grall@citrix.com>Acked-by: Ian Campbell <ian.campbell@citrix.com>
Ian Campbell
2013-Apr-16 14:20 UTC
Re: [PATCH V2 2/2] xen/arm: implement smp_call_function
Needs Keirs ack I think, CC added. On Tue, 2013-04-16 at 14:38 +0100, Julien Grall wrote:> Move smp_call_function and on_selected_cpus to common code. > > Signed-off-by: Julien Grall <julien.grall@citrix.com> > --- > xen/arch/arm/gic.c | 3 ++ > xen/arch/arm/smp.c | 11 ++--- > xen/arch/x86/smp.c | 88 +++----------------------------------- > xen/common/Makefile | 1 + > xen/common/smp.c | 94 +++++++++++++++++++++++++++++++++++++++++ > xen/include/asm-arm/gic.h | 1 + > xen/include/asm-x86/hardirq.h | 1 + > xen/include/xen/smp.h | 7 +++ > 8 files changed, 116 insertions(+), 90 deletions(-) > create mode 100644 xen/common/smp.c > > diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c > index 0ef994e..82aa6fe 100644 > --- a/xen/arch/arm/gic.c > +++ b/xen/arch/arm/gic.c > @@ -658,6 +658,9 @@ static void do_sgi(struct cpu_user_regs *regs, int othercpu, enum gic_sgi sgi) > case GIC_SGI_DUMP_STATE: > dump_execstate(regs); > break; > + case GIC_SGI_CALL_FUNCTION: > + smp_call_function_interrupt(); > + break; > default: > panic("Unhandled SGI %d on CPU%d\n", sgi, smp_processor_id()); > break; > diff --git a/xen/arch/arm/smp.c b/xen/arch/arm/smp.c > index a902d84..4042db5 100644 > --- a/xen/arch/arm/smp.c > +++ b/xen/arch/arm/smp.c > @@ -11,17 +11,14 @@ void flush_tlb_mask(const cpumask_t *mask) > flush_xen_data_tlb(); > } > > -void smp_call_function( > - void (*func) (void *info), > - void *info, > - int wait) > +void smp_send_event_check_mask(const cpumask_t *mask) > { > - panic("%s not implmented\n", __func__); > + send_SGI_mask(mask, GIC_SGI_EVENT_CHECK); > } > > -void smp_send_event_check_mask(const cpumask_t *mask) > +void smp_send_call_function_mask(const cpumask_t *mask) > { > - send_SGI_mask(mask, GIC_SGI_EVENT_CHECK); > + send_SGI_mask(mask, GIC_SGI_CALL_FUNCTION); > }Wow, diff has chosen a really unhelpful way to represent this change ;-) However it does look correct and assuming the code motion from x86->common is just that plus the switch to calling smp_send_call_function_mask then it is: Acked-by: Ian Campbell <ian.campbell@citrix.com>> > /* > diff --git a/xen/arch/x86/smp.c b/xen/arch/x86/smp.c > index a607531..0433f30 100644 > --- a/xen/arch/x86/smp.c > +++ b/xen/arch/x86/smp.c > @@ -269,67 +269,16 @@ void smp_send_event_check_mask(const cpumask_t *mask) > send_IPI_mask(mask, EVENT_CHECK_VECTOR); > } > > -/* > - * Structure and data for smp_call_function()/on_selected_cpus(). > - */ > - > -static void __smp_call_function_interrupt(void); > -static DEFINE_SPINLOCK(call_lock); > -static struct call_data_struct { > - void (*func) (void *info); > - void *info; > - int wait; > - cpumask_t selected; > -} call_data; > - > -void smp_call_function( > - void (*func) (void *info), > - void *info, > - int wait) > +void smp_send_call_function_mask(const cpumask_t *mask) > { > - cpumask_t allbutself; > - > - cpumask_andnot(&allbutself, &cpu_online_map, > - cpumask_of(smp_processor_id())); > - on_selected_cpus(&allbutself, func, info, wait); > -} > - > -void on_selected_cpus( > - const cpumask_t *selected, > - void (*func) (void *info), > - void *info, > - int wait) > -{ > - unsigned int nr_cpus; > - > - ASSERT(local_irq_is_enabled()); > - > - spin_lock(&call_lock); > - > - cpumask_copy(&call_data.selected, selected); > - > - nr_cpus = cpumask_weight(&call_data.selected); > - if ( nr_cpus == 0 ) > - goto out; > - > - call_data.func = func; > - call_data.info = info; > - call_data.wait = wait; > - > - send_IPI_mask(&call_data.selected, CALL_FUNCTION_VECTOR); > + send_IPI_mask(mask, CALL_FUNCTION_VECTOR); > > - if ( cpumask_test_cpu(smp_processor_id(), &call_data.selected) ) > + if ( cpumask_test_cpu(smp_processor_id(), mask) ) > { > local_irq_disable(); > - __smp_call_function_interrupt(); > + smp_call_function_interrupt(); > local_irq_enable(); > } > - > - while ( !cpumask_empty(&call_data.selected) ) > - cpu_relax(); > - > - out: > - spin_unlock(&call_lock); > } > > void __stop_this_cpu(void) > @@ -390,36 +339,9 @@ void event_check_interrupt(struct cpu_user_regs *regs) > this_cpu(irq_count)++; > } > > -static void __smp_call_function_interrupt(void) > -{ > - void (*func)(void *info) = call_data.func; > - void *info = call_data.info; > - unsigned int cpu = smp_processor_id(); > - > - if ( !cpumask_test_cpu(cpu, &call_data.selected) ) > - return; > - > - irq_enter(); > - > - if ( call_data.wait ) > - { > - (*func)(info); > - mb(); > - cpumask_clear_cpu(cpu, &call_data.selected); > - } > - else > - { > - mb(); > - cpumask_clear_cpu(cpu, &call_data.selected); > - (*func)(info); > - } > - > - irq_exit(); > -} > - > void call_function_interrupt(struct cpu_user_regs *regs) > { > ack_APIC_irq(); > perfc_incr(ipis); > - __smp_call_function_interrupt(); > + smp_call_function_interrupt(); > } > diff --git a/xen/common/Makefile b/xen/common/Makefile > index 8a0c506..0dc2050 100644 > --- a/xen/common/Makefile > +++ b/xen/common/Makefile > @@ -26,6 +26,7 @@ obj-y += schedule.o > obj-y += shutdown.o > obj-y += softirq.o > obj-y += sort.o > +obj-y += smp.o > obj-y += spinlock.o > obj-y += stop_machine.o > obj-y += string.o > diff --git a/xen/common/smp.c b/xen/common/smp.c > new file mode 100644 > index 0000000..dcd93ad > --- /dev/null > +++ b/xen/common/smp.c > @@ -0,0 +1,94 @@ > +#include <asm/hardirq.h> > +#include <asm/processor.h> > +#include <xen/spinlock.h> > +#include <xen/smp.h> > + > +/* > + * Structure and data for smp_call_function()/on_selected_cpus(). > + */ > +static DEFINE_SPINLOCK(call_lock); > +static struct call_data_struct { > + void (*func) (void *info); > + void *info; > + int wait; > + cpumask_t selected; > +} call_data; > + > +void smp_call_function( > + void (*func) (void *info), > + void *info, > + int wait) > +{ > + cpumask_t allbutself; > + > + cpumask_andnot(&allbutself, &cpu_online_map, > + cpumask_of(smp_processor_id())); > + on_selected_cpus(&allbutself, func, info, wait); > +} > + > +void on_selected_cpus( > + const cpumask_t *selected, > + void (*func) (void *info), > + void *info, > + int wait) > +{ > + unsigned int nr_cpus; > + > + ASSERT(local_irq_is_enabled()); > + > + spin_lock(&call_lock); > + > + cpumask_copy(&call_data.selected, selected); > + > + nr_cpus = cpumask_weight(&call_data.selected); > + if ( nr_cpus == 0 ) > + goto out; > + > + call_data.func = func; > + call_data.info = info; > + call_data.wait = wait; > + > + smp_send_call_function_mask(&call_data.selected); > + > + while ( !cpumask_empty(&call_data.selected) ) > + cpu_relax(); > + > +out: > + spin_unlock(&call_lock); > +} > + > +void smp_call_function_interrupt(void) > +{ > + void (*func)(void *info) = call_data.func; > + void *info = call_data.info; > + unsigned int cpu = smp_processor_id(); > + > + if ( !cpumask_test_cpu(cpu, &call_data.selected) ) > + return; > + > + irq_enter(); > + > + if ( call_data.wait ) > + { > + (*func)(info); > + mb(); > + cpumask_clear_cpu(cpu, &call_data.selected); > + } > + else > + { > + mb(); > + cpumask_clear_cpu(cpu, &call_data.selected); > + (*func)(info); > + } > + > + irq_exit(); > +} > + > +/* > + * Local variables: > + * mode: C > + * c-file-style: "BSD" > + * c-basic-offset: 4 > + * indent-tabs-mode: nil > + * End: > + */ > diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h > index 4f2c8b8..47354dd 100644 > --- a/xen/include/asm-arm/gic.h > +++ b/xen/include/asm-arm/gic.h > @@ -173,6 +173,7 @@ extern void gic_restore_state(struct vcpu *v); > enum gic_sgi { > GIC_SGI_EVENT_CHECK = 0, > GIC_SGI_DUMP_STATE = 1, > + GIC_SGI_CALL_FUNCTION = 2, > }; > extern void send_SGI_mask(const cpumask_t *cpumask, enum gic_sgi sgi); > extern void send_SGI_one(unsigned int cpu, enum gic_sgi sgi); > diff --git a/xen/include/asm-x86/hardirq.h b/xen/include/asm-x86/hardirq.h > index e573007..01a41b9 100644 > --- a/xen/include/asm-x86/hardirq.h > +++ b/xen/include/asm-x86/hardirq.h > @@ -3,6 +3,7 @@ > > #include <xen/config.h> > #include <xen/cache.h> > +#include <xen/types.h> > > typedef struct { > unsigned int __softirq_pending; > diff --git a/xen/include/xen/smp.h b/xen/include/xen/smp.h > index e05f390..6febb56 100644 > --- a/xen/include/xen/smp.h > +++ b/xen/include/xen/smp.h > @@ -58,6 +58,13 @@ static inline void on_each_cpu( > on_selected_cpus(&cpu_online_map, func, info, wait); > } > > +/* > + * Call a function on the current CPU > + */ > +void smp_call_function_interrupt(void); > + > +void smp_send_call_function_mask(const cpumask_t *mask); > + > #define smp_processor_id() raw_smp_processor_id() > > int alloc_cpu_id(void);
Ian Campbell
2013-Apr-22 11:44 UTC
Re: [PATCH V2 0/2] xen/arm: implement smp_call_function
On Tue, 2013-04-16 at 14:38 +0100, Julien Grall wrote:> This patch series can be applied on top of Ian''s patch series "arm: host SMP support" > > The first patch fixes an issue on SMP board when smp_call_function is > implemented. > The second patch implements the function. > > Changes since V1: > - Move smp_call_function and on_selected_cpus in the common part > - Use spin_lock instead of spin_lock_irqsave in patch 1 > > Julien Grall (2): > xen/arm: gic_disable_cpu must be called with interrupts disabled > xen/arm: implement smp_call_functionKeir/Jan do you have any Ack/Nack WRT the code motion from x86 to common code? WRT the freeze George said in <516EA78A.9090800@eu.citrix.com>: If Keir or Jan think that if there is a regression we are highly likely to catch it before the release, I''m OK with Julien''s series. I think smp_call_function is used much more on x86 than on ARM (which currently only needs it for reboot). On x86 it (or rather the inner on_selected_cpus function) is used in at least the IRQ, time calibration and EPT code which ought to give it a reasonable workout under normal use I think...> > xen/arch/arm/gic.c | 9 +++- > xen/arch/arm/smp.c | 11 ++--- > xen/arch/x86/smp.c | 88 +++----------------------------------- > xen/common/Makefile | 1 + > xen/common/smp.c | 94 +++++++++++++++++++++++++++++++++++++++++ > xen/include/asm-arm/gic.h | 1 + > xen/include/asm-x86/hardirq.h | 1 + > xen/include/xen/smp.h | 7 +++ > 8 files changed, 120 insertions(+), 92 deletions(-) > create mode 100644 xen/common/smp.c >
Ian Campbell
2013-Apr-30 15:42 UTC
Re: [PATCH V2 0/2] xen/arm: implement smp_call_function
On Mon, 2013-04-22 at 12:44 +0100, Ian Campbell wrote:> On Tue, 2013-04-16 at 14:38 +0100, Julien Grall wrote: > > This patch series can be applied on top of Ian''s patch series "arm: host SMP support" > > > > The first patch fixes an issue on SMP board when smp_call_function is > > implemented. > > The second patch implements the function. > > > > Changes since V1: > > - Move smp_call_function and on_selected_cpus in the common part > > - Use spin_lock instead of spin_lock_irqsave in patch 1 > > > > Julien Grall (2): > > xen/arm: gic_disable_cpu must be called with interrupts disabled > > xen/arm: implement smp_call_function > > Keir/Jan do you have any Ack/Nack WRT the code motion from x86 to common > code?Ping?> WRT the freeze George said in <516EA78A.9090800@eu.citrix.com>: > > If Keir or Jan think that if there is a regression we are highly > likely to catch it before the release, I''m OK with Julien''s > series. > > I think smp_call_function is used much more on x86 than on ARM (which > currently only needs it for reboot). On x86 it (or rather the inner > on_selected_cpus function) is used in at least the IRQ, time calibration > and EPT code which ought to give it a reasonable workout under normal > use I think... > > > > > xen/arch/arm/gic.c | 9 +++- > > xen/arch/arm/smp.c | 11 ++--- > > xen/arch/x86/smp.c | 88 +++----------------------------------- > > xen/common/Makefile | 1 + > > xen/common/smp.c | 94 +++++++++++++++++++++++++++++++++++++++++ > > xen/include/asm-arm/gic.h | 1 + > > xen/include/asm-x86/hardirq.h | 1 + > > xen/include/xen/smp.h | 7 +++ > > 8 files changed, 120 insertions(+), 92 deletions(-) > > create mode 100644 xen/common/smp.c > > > > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel
Ian Campbell
2013-May-01 11:55 UTC
Re: [PATCH V2 1/2] xen/arm: gic_disable_cpu must be called with interrupts disabled
On Tue, 2013-04-16 at 14:38 +0100, Julien Grall wrote:> gic_disable_cpu is only called with interrupt disabled. > Use spin_lock instead of spin_lock_irq and check the function is > called with interrupts disabled.Is this patch still relevant after this change which is now in master? commit b4aae03ce525e8e2364814d0fed8f982aefc4958 Author: Stefano Stabellini <stefano.stabellini@eu.citrix.com> Date: Mon Apr 29 18:04:25 2013 +0100 xen/arm: do not call __cpu_disable on machine_halt __cpu_disable shouldn''t be called on machine_halt, in fact it cannot succeed: cpu_disable_scheduler won''t be able to migrate away vcpus to others pcpus. Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com> Acked-by: Ian Campbell <ian.campbell@citrix.com>> > Signed-off-by: Julien Grall <julien.grall@citrix.com> > --- > xen/arch/arm/gic.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c > index aa179a9..0ef994e 100644 > --- a/xen/arch/arm/gic.c > +++ b/xen/arch/arm/gic.c > @@ -419,10 +419,12 @@ void __cpuinit gic_init_secondary_cpu(void) > /* Shut down the per-CPU GIC interface */ > void gic_disable_cpu(void) > { > - spin_lock_irq(&gic.lock); > + ASSERT(!local_irq_is_enabled()); > + > + spin_lock(&gic.lock); > gic_cpu_disable(); > gic_hyp_disable(); > - spin_unlock_irq(&gic.lock); > + spin_unlock(&gic.lock); > } > > void gic_route_ppis(void)
Julien Grall
2013-May-01 12:25 UTC
Re: [PATCH V2 1/2] xen/arm: gic_disable_cpu must be called with interrupts disabled
On 05/01/2013 12:55 PM, Ian Campbell wrote:> On Tue, 2013-04-16 at 14:38 +0100, Julien Grall wrote: >> gic_disable_cpu is only called with interrupt disabled. >> Use spin_lock instead of spin_lock_irq and check the function is >> called with interrupts disabled. > > Is this patch still relevant after this change which is now in master?Yes. gic_disable is only called by __cpu_disable, and the irq is disabled. local_irq_disable(); gic_disable_cpu();> > commit b4aae03ce525e8e2364814d0fed8f982aefc4958 > Author: Stefano Stabellini <stefano.stabellini@eu.citrix.com> > Date: Mon Apr 29 18:04:25 2013 +0100 > > xen/arm: do not call __cpu_disable on machine_halt > > __cpu_disable shouldn''t be called on machine_halt, in fact it cannot > succeed: cpu_disable_scheduler won''t be able to migrate away vcpus to > others pcpus. > > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com> > Acked-by: Ian Campbell <ian.campbell@citrix.com> > >> >> Signed-off-by: Julien Grall <julien.grall@citrix.com> >> --- >> xen/arch/arm/gic.c | 6 ++++-- >> 1 file changed, 4 insertions(+), 2 deletions(-) >> >> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c >> index aa179a9..0ef994e 100644 >> --- a/xen/arch/arm/gic.c >> +++ b/xen/arch/arm/gic.c >> @@ -419,10 +419,12 @@ void __cpuinit gic_init_secondary_cpu(void) >> /* Shut down the per-CPU GIC interface */ >> void gic_disable_cpu(void) >> { >> - spin_lock_irq(&gic.lock); >> + ASSERT(!local_irq_is_enabled()); >> + >> + spin_lock(&gic.lock); >> gic_cpu_disable(); >> gic_hyp_disable(); >> - spin_unlock_irq(&gic.lock); >> + spin_unlock(&gic.lock); >> } >> >> void gic_route_ppis(void) > >
On 16/04/2013 14:38, "Julien Grall" <julien.grall@citrix.com> wrote:> Move smp_call_function and on_selected_cpus to common code. > > Signed-off-by: Julien Grall <julien.grall@citrix.com>Please put a Xen-style heading at the top of new file common/smp.c -- see common/page_alloc.c (among many others) for an example. Apart from that very minor consideration, the code is totally fine and I will Ack this patch in advance of your next iteration of it: Acked-by: Keir Fraser <keir@xen.org> -- Keir> --- > xen/arch/arm/gic.c | 3 ++ > xen/arch/arm/smp.c | 11 ++--- > xen/arch/x86/smp.c | 88 +++----------------------------------- > xen/common/Makefile | 1 + > xen/common/smp.c | 94 > +++++++++++++++++++++++++++++++++++++++++ > xen/include/asm-arm/gic.h | 1 + > xen/include/asm-x86/hardirq.h | 1 + > xen/include/xen/smp.h | 7 +++ > 8 files changed, 116 insertions(+), 90 deletions(-) > create mode 100644 xen/common/smp.c > > diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c > index 0ef994e..82aa6fe 100644 > --- a/xen/arch/arm/gic.c > +++ b/xen/arch/arm/gic.c > @@ -658,6 +658,9 @@ static void do_sgi(struct cpu_user_regs *regs, int > othercpu, enum gic_sgi sgi) > case GIC_SGI_DUMP_STATE: > dump_execstate(regs); > break; > + case GIC_SGI_CALL_FUNCTION: > + smp_call_function_interrupt(); > + break; > default: > panic("Unhandled SGI %d on CPU%d\n", sgi, smp_processor_id()); > break; > diff --git a/xen/arch/arm/smp.c b/xen/arch/arm/smp.c > index a902d84..4042db5 100644 > --- a/xen/arch/arm/smp.c > +++ b/xen/arch/arm/smp.c > @@ -11,17 +11,14 @@ void flush_tlb_mask(const cpumask_t *mask) > flush_xen_data_tlb(); > } > > -void smp_call_function( > - void (*func) (void *info), > - void *info, > - int wait) > +void smp_send_event_check_mask(const cpumask_t *mask) > { > - panic("%s not implmented\n", __func__); > + send_SGI_mask(mask, GIC_SGI_EVENT_CHECK); > } > > -void smp_send_event_check_mask(const cpumask_t *mask) > +void smp_send_call_function_mask(const cpumask_t *mask) > { > - send_SGI_mask(mask, GIC_SGI_EVENT_CHECK); > + send_SGI_mask(mask, GIC_SGI_CALL_FUNCTION); > } > > /* > diff --git a/xen/arch/x86/smp.c b/xen/arch/x86/smp.c > index a607531..0433f30 100644 > --- a/xen/arch/x86/smp.c > +++ b/xen/arch/x86/smp.c > @@ -269,67 +269,16 @@ void smp_send_event_check_mask(const cpumask_t *mask) > send_IPI_mask(mask, EVENT_CHECK_VECTOR); > } > > -/* > - * Structure and data for smp_call_function()/on_selected_cpus(). > - */ > - > -static void __smp_call_function_interrupt(void); > -static DEFINE_SPINLOCK(call_lock); > -static struct call_data_struct { > - void (*func) (void *info); > - void *info; > - int wait; > - cpumask_t selected; > -} call_data; > - > -void smp_call_function( > - void (*func) (void *info), > - void *info, > - int wait) > +void smp_send_call_function_mask(const cpumask_t *mask) > { > - cpumask_t allbutself; > - > - cpumask_andnot(&allbutself, &cpu_online_map, > - cpumask_of(smp_processor_id())); > - on_selected_cpus(&allbutself, func, info, wait); > -} > - > -void on_selected_cpus( > - const cpumask_t *selected, > - void (*func) (void *info), > - void *info, > - int wait) > -{ > - unsigned int nr_cpus; > - > - ASSERT(local_irq_is_enabled()); > - > - spin_lock(&call_lock); > - > - cpumask_copy(&call_data.selected, selected); > - > - nr_cpus = cpumask_weight(&call_data.selected); > - if ( nr_cpus == 0 ) > - goto out; > - > - call_data.func = func; > - call_data.info = info; > - call_data.wait = wait; > - > - send_IPI_mask(&call_data.selected, CALL_FUNCTION_VECTOR); > + send_IPI_mask(mask, CALL_FUNCTION_VECTOR); > > - if ( cpumask_test_cpu(smp_processor_id(), &call_data.selected) ) > + if ( cpumask_test_cpu(smp_processor_id(), mask) ) > { > local_irq_disable(); > - __smp_call_function_interrupt(); > + smp_call_function_interrupt(); > local_irq_enable(); > } > - > - while ( !cpumask_empty(&call_data.selected) ) > - cpu_relax(); > - > - out: > - spin_unlock(&call_lock); > } > > void __stop_this_cpu(void) > @@ -390,36 +339,9 @@ void event_check_interrupt(struct cpu_user_regs *regs) > this_cpu(irq_count)++; > } > > -static void __smp_call_function_interrupt(void) > -{ > - void (*func)(void *info) = call_data.func; > - void *info = call_data.info; > - unsigned int cpu = smp_processor_id(); > - > - if ( !cpumask_test_cpu(cpu, &call_data.selected) ) > - return; > - > - irq_enter(); > - > - if ( call_data.wait ) > - { > - (*func)(info); > - mb(); > - cpumask_clear_cpu(cpu, &call_data.selected); > - } > - else > - { > - mb(); > - cpumask_clear_cpu(cpu, &call_data.selected); > - (*func)(info); > - } > - > - irq_exit(); > -} > - > void call_function_interrupt(struct cpu_user_regs *regs) > { > ack_APIC_irq(); > perfc_incr(ipis); > - __smp_call_function_interrupt(); > + smp_call_function_interrupt(); > } > diff --git a/xen/common/Makefile b/xen/common/Makefile > index 8a0c506..0dc2050 100644 > --- a/xen/common/Makefile > +++ b/xen/common/Makefile > @@ -26,6 +26,7 @@ obj-y += schedule.o > obj-y += shutdown.o > obj-y += softirq.o > obj-y += sort.o > +obj-y += smp.o > obj-y += spinlock.o > obj-y += stop_machine.o > obj-y += string.o > diff --git a/xen/common/smp.c b/xen/common/smp.c > new file mode 100644 > index 0000000..dcd93ad > --- /dev/null > +++ b/xen/common/smp.c > @@ -0,0 +1,94 @@ > +#include <asm/hardirq.h> > +#include <asm/processor.h> > +#include <xen/spinlock.h> > +#include <xen/smp.h> > + > +/* > + * Structure and data for smp_call_function()/on_selected_cpus(). > + */ > +static DEFINE_SPINLOCK(call_lock); > +static struct call_data_struct { > + void (*func) (void *info); > + void *info; > + int wait; > + cpumask_t selected; > +} call_data; > + > +void smp_call_function( > + void (*func) (void *info), > + void *info, > + int wait) > +{ > + cpumask_t allbutself; > + > + cpumask_andnot(&allbutself, &cpu_online_map, > + cpumask_of(smp_processor_id())); > + on_selected_cpus(&allbutself, func, info, wait); > +} > + > +void on_selected_cpus( > + const cpumask_t *selected, > + void (*func) (void *info), > + void *info, > + int wait) > +{ > + unsigned int nr_cpus; > + > + ASSERT(local_irq_is_enabled()); > + > + spin_lock(&call_lock); > + > + cpumask_copy(&call_data.selected, selected); > + > + nr_cpus = cpumask_weight(&call_data.selected); > + if ( nr_cpus == 0 ) > + goto out; > + > + call_data.func = func; > + call_data.info = info; > + call_data.wait = wait; > + > + smp_send_call_function_mask(&call_data.selected); > + > + while ( !cpumask_empty(&call_data.selected) ) > + cpu_relax(); > + > +out: > + spin_unlock(&call_lock); > +} > + > +void smp_call_function_interrupt(void) > +{ > + void (*func)(void *info) = call_data.func; > + void *info = call_data.info; > + unsigned int cpu = smp_processor_id(); > + > + if ( !cpumask_test_cpu(cpu, &call_data.selected) ) > + return; > + > + irq_enter(); > + > + if ( call_data.wait ) > + { > + (*func)(info); > + mb(); > + cpumask_clear_cpu(cpu, &call_data.selected); > + } > + else > + { > + mb(); > + cpumask_clear_cpu(cpu, &call_data.selected); > + (*func)(info); > + } > + > + irq_exit(); > +} > + > +/* > + * Local variables: > + * mode: C > + * c-file-style: "BSD" > + * c-basic-offset: 4 > + * indent-tabs-mode: nil > + * End: > + */ > diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h > index 4f2c8b8..47354dd 100644 > --- a/xen/include/asm-arm/gic.h > +++ b/xen/include/asm-arm/gic.h > @@ -173,6 +173,7 @@ extern void gic_restore_state(struct vcpu *v); > enum gic_sgi { > GIC_SGI_EVENT_CHECK = 0, > GIC_SGI_DUMP_STATE = 1, > + GIC_SGI_CALL_FUNCTION = 2, > }; > extern void send_SGI_mask(const cpumask_t *cpumask, enum gic_sgi sgi); > extern void send_SGI_one(unsigned int cpu, enum gic_sgi sgi); > diff --git a/xen/include/asm-x86/hardirq.h b/xen/include/asm-x86/hardirq.h > index e573007..01a41b9 100644 > --- a/xen/include/asm-x86/hardirq.h > +++ b/xen/include/asm-x86/hardirq.h > @@ -3,6 +3,7 @@ > > #include <xen/config.h> > #include <xen/cache.h> > +#include <xen/types.h> > > typedef struct { > unsigned int __softirq_pending; > diff --git a/xen/include/xen/smp.h b/xen/include/xen/smp.h > index e05f390..6febb56 100644 > --- a/xen/include/xen/smp.h > +++ b/xen/include/xen/smp.h > @@ -58,6 +58,13 @@ static inline void on_each_cpu( > on_selected_cpus(&cpu_online_map, func, info, wait); > } > > +/* > + * Call a function on the current CPU > + */ > +void smp_call_function_interrupt(void); > + > +void smp_send_call_function_mask(const cpumask_t *mask); > + > #define smp_processor_id() raw_smp_processor_id() > > int alloc_cpu_id(void);
Ian Campbell
2013-May-10 14:22 UTC
Re: [PATCH V2 1/2] xen/arm: gic_disable_cpu must be called with interrupts disabled
On Wed, 2013-05-01 at 13:25 +0100, Julien Grall wrote:> On 05/01/2013 12:55 PM, Ian Campbell wrote: > > > On Tue, 2013-04-16 at 14:38 +0100, Julien Grall wrote: > >> gic_disable_cpu is only called with interrupt disabled. > >> Use spin_lock instead of spin_lock_irq and check the function is > >> called with interrupts disabled. > > > > Is this patch still relevant after this change which is now in master? > > > Yes. gic_disable is only called by __cpu_disable, and the irq is disabled. > > local_irq_disable(); > gic_disable_cpu();Thanks, I somehow dropped this from my queue without applying, thanks to your kick IRL I have now applied.