[PATCH V2 0/2] xen/arm: implement smp_call_function

[PATCH V2 0/2] xen/arm: implement smp_call_function

[Julien Grall]

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

[PATCH V2 1/2] xen/arm: gic_disable_cpu must be called with interrupts disabled

[Julien Grall]

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

[PATCH V2 2/2] xen/arm: implement smp_call_function

[Julien Grall]

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

Re: [PATCH V2 1/2] xen/arm: gic_disable_cpu must be called with interrupts disabled

[Ian Campbell]

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>

Re: [PATCH V2 2/2] xen/arm: implement smp_call_function

[Ian Campbell]

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

Re: [PATCH V2 0/2] xen/arm: implement smp_call_function

[Ian Campbell]

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?

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
> 

Re: [PATCH V2 0/2] xen/arm: implement smp_call_function

[Ian Campbell]

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

Re: [PATCH V2 1/2] xen/arm: gic_disable_cpu must be called with interrupts disabled

[Ian Campbell]

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)

Re: [PATCH V2 1/2] xen/arm: gic_disable_cpu must be called with interrupts disabled

[Julien Grall]

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

Re: [PATCH V2 2/2] xen/arm: implement smp_call_function

[Keir Fraser]

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

Re: [PATCH V2 1/2] xen/arm: gic_disable_cpu must be called with interrupts disabled

[Ian Campbell]

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.