[PATCH kvm-unit-tests 0/6] arm/arm64: some cleanups

[PATCH kvm-unit-tests 0/6] arm/arm64: some cleanups

[Andrew Jones]

Andrew Jones (6):
  lib/arm/gic: gic_version cleanup
  lib/arm/psci: make psci less ugly
  lib/arm/smp: introduce smp_run
  arm/arm64: selftest: apply smp_run
  arm/arm64: spinlock-test: apply smp_run
  arm/arm64: gic: apply smp_run

 arm/cstart.S         |  3 ++-
 arm/cstart64.S       |  3 ++-
 arm/gic.c            | 18 +++++++++---------
 arm/selftest.c       | 19 +------------------
 arm/spinlock-test.c  | 20 +-------------------
 lib/arm/asm/psci.h   |  6 ++----
 lib/arm/asm/smp.h    |  2 ++
 lib/arm/gic.c        | 27 ++++++++++++---------------
 lib/arm/psci.c       | 10 +++++++---
 lib/arm/smp.c        | 26 ++++++++++++++++++++++++++
 lib/arm64/asm/psci.h | 16 +---------------
 11 files changed, 65 insertions(+), 85 deletions(-)

-- 
2.9.4

[PATCH kvm-unit-tests 1/6] lib/arm/gic: gic_version cleanup

[Andrew Jones]

Remove version from ops as it's not an op, nor necessary.

Signed-off-by: Andrew Jones <drjones@redhat.com>
---
 lib/arm/gic.c | 27 ++++++++++++---------------
 1 file changed, 12 insertions(+), 15 deletions(-)

diff --git a/lib/arm/gic.c b/lib/arm/gic.c
index 3ed539727f8c..59273b1716d6 100644
--- a/lib/arm/gic.c
+++ b/lib/arm/gic.c
@@ -11,7 +11,6 @@ struct gicv2_data gicv2_data;
 struct gicv3_data gicv3_data;
 
 struct gic_common_ops {
-	int gic_version;
 	void (*enable_defaults)(void);
 	u32 (*read_iar)(void);
 	u32 (*iar_irqnr)(u32 iar);
@@ -23,7 +22,6 @@ struct gic_common_ops {
 static const struct gic_common_ops *gic_common_ops;
 
 static const struct gic_common_ops gicv2_common_ops = {
-	.gic_version = 2,
 	.enable_defaults = gicv2_enable_defaults,
 	.read_iar = gicv2_read_iar,
 	.iar_irqnr = gicv2_iar_irqnr,
@@ -33,7 +31,6 @@ static const struct gic_common_ops gicv2_common_ops = {
 };
 
 static const struct gic_common_ops gicv3_common_ops = {
-	.gic_version = 3,
 	.enable_defaults = gicv3_enable_defaults,
 	.read_iar = gicv3_read_iar,
 	.iar_irqnr = gicv3_iar_irqnr,
@@ -88,18 +85,24 @@ int gicv3_init(void)
 			&gicv3_data.redist_base[0]);
 }
 
-int gic_init(void)
+int gic_version(void)
 {
-	if (gicv2_init()) {
-		gic_common_ops = &gicv2_common_ops;
+	if (gic_common_ops == &gicv2_common_ops)
 		return 2;
-	} else if (gicv3_init()) {
-		gic_common_ops = &gicv3_common_ops;
+	else if (gic_common_ops == &gicv3_common_ops)
 		return 3;
-	}
 	return 0;
 }
 
+int gic_init(void)
+{
+	if (gicv2_init())
+		gic_common_ops = &gicv2_common_ops;
+	else if (gicv3_init())
+		gic_common_ops = &gicv3_common_ops;
+	return gic_version();
+}
+
 void gic_enable_defaults(void)
 {
 	if (!gic_common_ops) {
@@ -110,12 +113,6 @@ void gic_enable_defaults(void)
 	gic_common_ops->enable_defaults();
 }
 
-int gic_version(void)
-{
-	assert(gic_common_ops);
-	return gic_common_ops->gic_version;
-}
-
 u32 gic_read_iar(void)
 {
 	assert(gic_common_ops && gic_common_ops->read_iar);
-- 
2.9.4

Re: [PATCH kvm-unit-tests 1/6] lib/arm/gic: gic_version cleanup

[Paolo Bonzini]

On 25/05/2017 12:28, Andrew Jones wrote:
> +int gic_init(void)
> +{
> +	if (gicv2_init())
> +		gic_common_ops = &gicv2_common_ops;
> +	else if (gicv3_init())
> +		gic_common_ops = &gicv3_common_ops;
> +	return gic_version();
> +}
> +

Of the other uses of gic_version(), this one is the ugliest:

                if (gic_version() == 2)
                        base = gicv2_dist_base();
                else
                        base = gicv3_redist_base();

                writel(val, base + GICD_ICACTIVER);

Could you change this writel to a new op gicv[23]_eoi or something like
that?

Thanks,

Paolo

Re: [PATCH kvm-unit-tests 1/6] lib/arm/gic: gic_version cleanup

[Andrew Jones]

On Thu, May 25, 2017 at 03:53:10PM +0200, Paolo Bonzini wrote:
> 
> 
> On 25/05/2017 12:28, Andrew Jones wrote:
> > +int gic_init(void)
> > +{
> > +	if (gicv2_init())
> > +		gic_common_ops = &gicv2_common_ops;
> > +	else if (gicv3_init())
> > +		gic_common_ops = &gicv3_common_ops;
> > +	return gic_version();
> > +}
> > +
> 
> Of the other uses of gic_version(), this one is the ugliest:
> 
>                 if (gic_version() == 2)
>                         base = gicv2_dist_base();
>                 else
>                         base = gicv3_redist_base();
> 
>                 writel(val, base + GICD_ICACTIVER);
> 
> Could you change this writel to a new op gicv[23]_eoi or something like
> that?

That use is just for an irq handler specific to a single unit test,
and at this time I don't think we need an op for that writel, as
that's the only place we currently write that register.

I do have some plans for irq handler refactoring in arm/gic.c, in
order to prepare for more tests. I'll clean this up a bit when I
do that.

Thanks,
drew

[PATCH kvm-unit-tests 2/6] lib/arm/psci: make psci less ugly

[Andrew Jones]

Signed-off-by: Andrew Jones <drjones@redhat.com>
---
 lib/arm/asm/psci.h   |  6 ++----
 lib/arm/psci.c       | 10 +++++++---
 lib/arm64/asm/psci.h | 16 +---------------
 3 files changed, 10 insertions(+), 22 deletions(-)

diff --git a/lib/arm/asm/psci.h b/lib/arm/asm/psci.h
index 11ac45028d78..eeb55cf299d1 100644
--- a/lib/arm/asm/psci.h
+++ b/lib/arm/asm/psci.h
@@ -3,10 +3,8 @@
 #include <libcflat.h>
 #include <linux/psci.h>
 
-#define PSCI_INVOKE_ARG_TYPE	u32
-#define PSCI_FN_CPU_ON		PSCI_0_2_FN_CPU_ON
-
-extern int psci_invoke(u32 function_id, u32 arg0, u32 arg1, u32 arg2);
+extern int psci_invoke(unsigned long function_id, unsigned long arg0,
+		       unsigned long arg1, unsigned long arg2);
 extern int psci_cpu_on(unsigned long cpuid, unsigned long entry_point);
 extern void psci_sys_reset(void);
 extern int cpu_psci_cpu_boot(unsigned int cpu);
diff --git a/lib/arm/psci.c b/lib/arm/psci.c
index aca88851171f..a14acddeefd3 100644
--- a/lib/arm/psci.c
+++ b/lib/arm/psci.c
@@ -10,9 +10,9 @@
 #include <asm/setup.h>
 #include <asm/page.h>
 
-#define T PSCI_INVOKE_ARG_TYPE
 __attribute__((noinline))
-int psci_invoke(T function_id, T arg0, T arg1, T arg2)
+int psci_invoke(unsigned long function_id, unsigned long arg0,
+		unsigned long arg1, unsigned long arg2)
 {
 	asm volatile(
 		"hvc #0"
@@ -23,7 +23,11 @@ int psci_invoke(T function_id, T arg0, T arg1, T arg2)
 
 int psci_cpu_on(unsigned long cpuid, unsigned long entry_point)
 {
-	return psci_invoke(PSCI_FN_CPU_ON, cpuid, entry_point, 0);
+#ifdef __arm__
+	return psci_invoke(PSCI_0_2_FN_CPU_ON, cpuid, entry_point, 0);
+#else
+	return psci_invoke(PSCI_0_2_FN64_CPU_ON, cpuid, entry_point, 0);
+#endif
 }
 
 extern void secondary_entry(void);
diff --git a/lib/arm64/asm/psci.h b/lib/arm64/asm/psci.h
index 0a7d7c854e2b..783b36ef579e 100644
--- a/lib/arm64/asm/psci.h
+++ b/lib/arm64/asm/psci.h
@@ -1,15 +1 @@
-#ifndef _ASMARM64_PSCI_H_
-#define _ASMARM64_PSCI_H_
-#include <libcflat.h>
-#include <linux/psci.h>
-
-#define PSCI_INVOKE_ARG_TYPE	u64
-#define PSCI_FN_CPU_ON		PSCI_0_2_FN64_CPU_ON
-
-extern int psci_invoke(u64 function_id, u64 arg0, u64 arg1, u64 arg2);
-extern int psci_cpu_on(unsigned long cpuid, unsigned long entry_point);
-extern void psci_sys_reset(void);
-extern int cpu_psci_cpu_boot(unsigned int cpu);
-extern void cpu_psci_cpu_die(unsigned int cpu);
-
-#endif /* _ASMARM64_PSCI_H_ */
+#include "../../arm/asm/psci.h"
-- 
2.9.4

[PATCH kvm-unit-tests 3/6] lib/arm/smp: introduce smp_run

[Andrew Jones]

A common pattern is
 - run a function on all cpus
 - signal each cpu's completion with a cpumask
 - halt the secondaries when they're complete
 - have the primary wait on the cpumask for all to complete

smp_run is a wrapper for that pattern. Also, we were allowing
secondaries to go off in the weeds if they returned from their
secondary entry function, which can be difficult to debug. A
nice side-effect of adding this wrapper is we don't do that
anymore, and can even know when a secondary has halted with the
new cpu_halted_mask.

Signed-off-by: Andrew Jones <drjones@redhat.com>
---
 arm/cstart.S      |  3 ++-
 arm/cstart64.S    |  3 ++-
 lib/arm/asm/smp.h |  2 ++
 lib/arm/smp.c     | 26 ++++++++++++++++++++++++++
 4 files changed, 32 insertions(+), 2 deletions(-)

diff --git a/arm/cstart.S b/arm/cstart.S
index 12461d104dad..b3176b274933 100644
--- a/arm/cstart.S
+++ b/arm/cstart.S
@@ -118,7 +118,8 @@ secondary_entry:
 	bl	secondary_cinit
 
 	/* r0 is now the entry function, run it */
-	mov	pc, r0
+	blx	r0
+	b	secondary_halt
 
 .globl halt
 halt:
diff --git a/arm/cstart64.S b/arm/cstart64.S
index 7738babc4109..30f732f1e99b 100644
--- a/arm/cstart64.S
+++ b/arm/cstart64.S
@@ -79,7 +79,8 @@ secondary_entry:
 	bl	secondary_cinit
 
 	/* x0 is now the entry function, run it */
-	br	x0
+	blr	x0
+	b	secondary_halt
 
 .globl halt
 halt:
diff --git a/lib/arm/asm/smp.h b/lib/arm/asm/smp.h
index 4cb86b6ce342..1d1cd7a29991 100644
--- a/lib/arm/asm/smp.h
+++ b/lib/arm/asm/smp.h
@@ -14,6 +14,7 @@ extern void halt(void);
 
 extern cpumask_t cpu_present_mask;
 extern cpumask_t cpu_online_mask;
+extern cpumask_t cpu_halted_mask;
 #define cpu_present(cpu)		cpumask_test_cpu(cpu, &cpu_present_mask)
 #define cpu_online(cpu)			cpumask_test_cpu(cpu, &cpu_online_mask)
 #define for_each_present_cpu(cpu)	for_each_cpu(cpu, &cpu_present_mask)
@@ -45,5 +46,6 @@ struct secondary_data {
 extern struct secondary_data secondary_data;
 
 extern void smp_boot_secondary(int cpu, secondary_entry_fn entry);
+extern void smp_run(void (*func)(void));
 
 #endif /* _ASMARM_SMP_H_ */
diff --git a/lib/arm/smp.c b/lib/arm/smp.c
index bbaf9e60e950..8528aa454108 100644
--- a/lib/arm/smp.c
+++ b/lib/arm/smp.c
@@ -15,6 +15,7 @@
 
 cpumask_t cpu_present_mask;
 cpumask_t cpu_online_mask;
+cpumask_t cpu_halted_mask;
 struct secondary_data secondary_data;
 
 secondary_entry_fn secondary_cinit(void)
@@ -53,3 +54,28 @@ void smp_boot_secondary(int cpu, secondary_entry_fn entry)
 	while (!cpu_online(cpu))
 		wfe();
 }
+
+void secondary_halt(void)
+{
+	struct thread_info *ti = current_thread_info();
+
+	cpumask_set_cpu(ti->cpu, &cpu_halted_mask);
+	halt();
+}
+
+void smp_run(void (*func)(void))
+{
+	int cpu;
+
+	for_each_present_cpu(cpu) {
+		if (cpu == 0)
+			continue;
+		smp_boot_secondary(cpu, func);
+	}
+	func();
+
+	cpumask_set_cpu(0, &cpu_halted_mask);
+	while (!cpumask_full(&cpu_halted_mask))
+		cpu_relax();
+	cpumask_clear_cpu(0, &cpu_halted_mask);
+}
-- 
2.9.4

Re: [PATCH kvm-unit-tests 3/6] lib/arm/smp: introduce smp_run

[Paolo Bonzini]

On 25/05/2017 12:28, Andrew Jones wrote:
> A common pattern is
>  - run a function on all cpus
>  - signal each cpu's completion with a cpumask
>  - halt the secondaries when they're complete
>  - have the primary wait on the cpumask for all to complete
> 
> smp_run is a wrapper for that pattern. Also, we were allowing
> secondaries to go off in the weeds if they returned from their
> secondary entry function, which can be difficult to debug. A
> nice side-effect of adding this wrapper is we don't do that
> anymore, and can even know when a secondary has halted with the
> new cpu_halted_mask.

Would it make sense to just call main() this way?

Paolo

> Signed-off-by: Andrew Jones <drjones@redhat.com>
> ---
>  arm/cstart.S      |  3 ++-
>  arm/cstart64.S    |  3 ++-
>  lib/arm/asm/smp.h |  2 ++
>  lib/arm/smp.c     | 26 ++++++++++++++++++++++++++
>  4 files changed, 32 insertions(+), 2 deletions(-)
> 
> diff --git a/arm/cstart.S b/arm/cstart.S
> index 12461d104dad..b3176b274933 100644
> --- a/arm/cstart.S
> +++ b/arm/cstart.S
> @@ -118,7 +118,8 @@ secondary_entry:
>  	bl	secondary_cinit
>  
>  	/* r0 is now the entry function, run it */
> -	mov	pc, r0
> +	blx	r0
> +	b	secondary_halt
>  
>  .globl halt
>  halt:
> diff --git a/arm/cstart64.S b/arm/cstart64.S
> index 7738babc4109..30f732f1e99b 100644
> --- a/arm/cstart64.S
> +++ b/arm/cstart64.S
> @@ -79,7 +79,8 @@ secondary_entry:
>  	bl	secondary_cinit
>  
>  	/* x0 is now the entry function, run it */
> -	br	x0
> +	blr	x0
> +	b	secondary_halt
>  
>  .globl halt
>  halt:
> diff --git a/lib/arm/asm/smp.h b/lib/arm/asm/smp.h
> index 4cb86b6ce342..1d1cd7a29991 100644
> --- a/lib/arm/asm/smp.h
> +++ b/lib/arm/asm/smp.h
> @@ -14,6 +14,7 @@ extern void halt(void);
>  
>  extern cpumask_t cpu_present_mask;
>  extern cpumask_t cpu_online_mask;
> +extern cpumask_t cpu_halted_mask;
>  #define cpu_present(cpu)		cpumask_test_cpu(cpu, &cpu_present_mask)
>  #define cpu_online(cpu)			cpumask_test_cpu(cpu, &cpu_online_mask)
>  #define for_each_present_cpu(cpu)	for_each_cpu(cpu, &cpu_present_mask)
> @@ -45,5 +46,6 @@ struct secondary_data {
>  extern struct secondary_data secondary_data;
>  
>  extern void smp_boot_secondary(int cpu, secondary_entry_fn entry);
> +extern void smp_run(void (*func)(void));
>  
>  #endif /* _ASMARM_SMP_H_ */
> diff --git a/lib/arm/smp.c b/lib/arm/smp.c
> index bbaf9e60e950..8528aa454108 100644
> --- a/lib/arm/smp.c
> +++ b/lib/arm/smp.c
> @@ -15,6 +15,7 @@
>  
>  cpumask_t cpu_present_mask;
>  cpumask_t cpu_online_mask;
> +cpumask_t cpu_halted_mask;
>  struct secondary_data secondary_data;
>  
>  secondary_entry_fn secondary_cinit(void)
> @@ -53,3 +54,28 @@ void smp_boot_secondary(int cpu, secondary_entry_fn entry)
>  	while (!cpu_online(cpu))
>  		wfe();
>  }
> +
> +void secondary_halt(void)
> +{
> +	struct thread_info *ti = current_thread_info();
> +
> +	cpumask_set_cpu(ti->cpu, &cpu_halted_mask);
> +	halt();
> +}
> +
> +void smp_run(void (*func)(void))
> +{
> +	int cpu;
> +
> +	for_each_present_cpu(cpu) {
> +		if (cpu == 0)
> +			continue;
> +		smp_boot_secondary(cpu, func);
> +	}
> +	func();
> +
> +	cpumask_set_cpu(0, &cpu_halted_mask);
> +	while (!cpumask_full(&cpu_halted_mask))
> +		cpu_relax();
> +	cpumask_clear_cpu(0, &cpu_halted_mask);
> +}
> 

Re: [PATCH kvm-unit-tests 3/6] lib/arm/smp: introduce smp_run

[Andrew Jones]

On Thu, May 25, 2017 at 03:56:00PM +0200, Paolo Bonzini wrote:
> 
> 
> On 25/05/2017 12:28, Andrew Jones wrote:
> > A common pattern is
> >  - run a function on all cpus
> >  - signal each cpu's completion with a cpumask
> >  - halt the secondaries when they're complete
> >  - have the primary wait on the cpumask for all to complete
> > 
> > smp_run is a wrapper for that pattern. Also, we were allowing
> > secondaries to go off in the weeds if they returned from their
> > secondary entry function, which can be difficult to debug. A
> > nice side-effect of adding this wrapper is we don't do that
> > anymore, and can even know when a secondary has halted with the
> > new cpu_halted_mask.
> 
> Would it make sense to just call main() this way?

Do you mean to start main() on all cpus all the time and then
leave it to the test writer to ensure things that should only
be done by the primary are guarded with 'if (cpu == 0)' ? If
so, then I think we might over complicate simple tests. I
suppose some tests could do something like

 void main(void)
 {
     if (smp_processor_id() == 0)
         smp_run(main);
     do_test1();
     do_test2();
     if (smp_processor_id() == 0)
         return report_summary();
 }

but I think that looks worse than

 void do_tests(void)
 {
     do_test1();
     do_test2();
 }

 void main(void)
 {
     smp_run(do_tests);
     return report_summary();
 }

One thing I did think of later is that cpu0 is allowed to
halt without setting its bit in cpu_halted_mask. But that
can only happen when chr_testdev_exit() fails (and who
would look at it?), so it doesn't really matter. I'd fix
it up if we want it for completeness though.

Thanks,
drew


> 
> Paolo
> 
> > Signed-off-by: Andrew Jones <drjones@redhat.com>
> > ---
> >  arm/cstart.S      |  3 ++-
> >  arm/cstart64.S    |  3 ++-
> >  lib/arm/asm/smp.h |  2 ++
> >  lib/arm/smp.c     | 26 ++++++++++++++++++++++++++
> >  4 files changed, 32 insertions(+), 2 deletions(-)
> > 
> > diff --git a/arm/cstart.S b/arm/cstart.S
> > index 12461d104dad..b3176b274933 100644
> > --- a/arm/cstart.S
> > +++ b/arm/cstart.S
> > @@ -118,7 +118,8 @@ secondary_entry:
> >  	bl	secondary_cinit
> >  
> >  	/* r0 is now the entry function, run it */
> > -	mov	pc, r0
> > +	blx	r0
> > +	b	secondary_halt
> >  
> >  .globl halt
> >  halt:
> > diff --git a/arm/cstart64.S b/arm/cstart64.S
> > index 7738babc4109..30f732f1e99b 100644
> > --- a/arm/cstart64.S
> > +++ b/arm/cstart64.S
> > @@ -79,7 +79,8 @@ secondary_entry:
> >  	bl	secondary_cinit
> >  
> >  	/* x0 is now the entry function, run it */
> > -	br	x0
> > +	blr	x0
> > +	b	secondary_halt
> >  
> >  .globl halt
> >  halt:
> > diff --git a/lib/arm/asm/smp.h b/lib/arm/asm/smp.h
> > index 4cb86b6ce342..1d1cd7a29991 100644
> > --- a/lib/arm/asm/smp.h
> > +++ b/lib/arm/asm/smp.h
> > @@ -14,6 +14,7 @@ extern void halt(void);
> >  
> >  extern cpumask_t cpu_present_mask;
> >  extern cpumask_t cpu_online_mask;
> > +extern cpumask_t cpu_halted_mask;
> >  #define cpu_present(cpu)		cpumask_test_cpu(cpu, &cpu_present_mask)
> >  #define cpu_online(cpu)			cpumask_test_cpu(cpu, &cpu_online_mask)
> >  #define for_each_present_cpu(cpu)	for_each_cpu(cpu, &cpu_present_mask)
> > @@ -45,5 +46,6 @@ struct secondary_data {
> >  extern struct secondary_data secondary_data;
> >  
> >  extern void smp_boot_secondary(int cpu, secondary_entry_fn entry);
> > +extern void smp_run(void (*func)(void));
> >  
> >  #endif /* _ASMARM_SMP_H_ */
> > diff --git a/lib/arm/smp.c b/lib/arm/smp.c
> > index bbaf9e60e950..8528aa454108 100644
> > --- a/lib/arm/smp.c
> > +++ b/lib/arm/smp.c
> > @@ -15,6 +15,7 @@
> >  
> >  cpumask_t cpu_present_mask;
> >  cpumask_t cpu_online_mask;
> > +cpumask_t cpu_halted_mask;
> >  struct secondary_data secondary_data;
> >  
> >  secondary_entry_fn secondary_cinit(void)
> > @@ -53,3 +54,28 @@ void smp_boot_secondary(int cpu, secondary_entry_fn entry)
> >  	while (!cpu_online(cpu))
> >  		wfe();
> >  }
> > +
> > +void secondary_halt(void)
> > +{
> > +	struct thread_info *ti = current_thread_info();
> > +
> > +	cpumask_set_cpu(ti->cpu, &cpu_halted_mask);
> > +	halt();
> > +}
> > +
> > +void smp_run(void (*func)(void))
> > +{
> > +	int cpu;
> > +
> > +	for_each_present_cpu(cpu) {
> > +		if (cpu == 0)
> > +			continue;
> > +		smp_boot_secondary(cpu, func);
> > +	}
> > +	func();
> > +
> > +	cpumask_set_cpu(0, &cpu_halted_mask);
> > +	while (!cpumask_full(&cpu_halted_mask))
> > +		cpu_relax();
> > +	cpumask_clear_cpu(0, &cpu_halted_mask);
> > +}
> > 

Re: [PATCH kvm-unit-tests 3/6] lib/arm/smp: introduce smp_run

[Paolo Bonzini]

On 25/05/2017 16:41, Andrew Jones wrote:
> On Thu, May 25, 2017 at 03:56:00PM +0200, Paolo Bonzini wrote:
>>
>>
>> On 25/05/2017 12:28, Andrew Jones wrote:
>>> A common pattern is
>>>  - run a function on all cpus
>>>  - signal each cpu's completion with a cpumask
>>>  - halt the secondaries when they're complete
>>>  - have the primary wait on the cpumask for all to complete
>>>
>>> smp_run is a wrapper for that pattern. Also, we were allowing
>>> secondaries to go off in the weeds if they returned from their
>>> secondary entry function, which can be difficult to debug. A
>>> nice side-effect of adding this wrapper is we don't do that
>>> anymore, and can even know when a secondary has halted with the
>>> new cpu_halted_mask.
>>
>> Would it make sense to just call main() this way?
> 
> Do you mean to start main() on all cpus all the time and then
> leave it to the test writer to ensure things that should only
> be done by the primary are guarded with 'if (cpu == 0)' ? If
> so, then I think we might over complicate simple tests. I
> suppose some tests could do something like

Start main on cpu 0 and put other CPUs in a WFI loop where they can take
IPIs.  This is similar to how x86 works, and homogeneity would be useful
I think.

Paolo

>  void main(void)
>  {
>      if (smp_processor_id() == 0)
>          smp_run(main);
>      do_test1();
>      do_test2();
>      if (smp_processor_id() == 0)
>          return report_summary();
>  }
> 
> but I think that looks worse than
> 
>  void do_tests(void)
>  {
>      do_test1();
>      do_test2();
>  }
> 
>  void main(void)
>  {
>      smp_run(do_tests);
>      return report_summary();
>  }
> 
> One thing I did think of later is that cpu0 is allowed to
> halt without setting its bit in cpu_halted_mask. But that
> can only happen when chr_testdev_exit() fails (and who
> would look at it?), so it doesn't really matter. I'd fix
> it up if we want it for completeness though.
> 
> Thanks,
> drew
> 
> 
>>
>> Paolo
>>
>>> Signed-off-by: Andrew Jones <drjones@redhat.com>
>>> ---
>>>  arm/cstart.S      |  3 ++-
>>>  arm/cstart64.S    |  3 ++-
>>>  lib/arm/asm/smp.h |  2 ++
>>>  lib/arm/smp.c     | 26 ++++++++++++++++++++++++++
>>>  4 files changed, 32 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/arm/cstart.S b/arm/cstart.S
>>> index 12461d104dad..b3176b274933 100644
>>> --- a/arm/cstart.S
>>> +++ b/arm/cstart.S
>>> @@ -118,7 +118,8 @@ secondary_entry:
>>>  	bl	secondary_cinit
>>>  
>>>  	/* r0 is now the entry function, run it */
>>> -	mov	pc, r0
>>> +	blx	r0
>>> +	b	secondary_halt
>>>  
>>>  .globl halt
>>>  halt:
>>> diff --git a/arm/cstart64.S b/arm/cstart64.S
>>> index 7738babc4109..30f732f1e99b 100644
>>> --- a/arm/cstart64.S
>>> +++ b/arm/cstart64.S
>>> @@ -79,7 +79,8 @@ secondary_entry:
>>>  	bl	secondary_cinit
>>>  
>>>  	/* x0 is now the entry function, run it */
>>> -	br	x0
>>> +	blr	x0
>>> +	b	secondary_halt
>>>  
>>>  .globl halt
>>>  halt:
>>> diff --git a/lib/arm/asm/smp.h b/lib/arm/asm/smp.h
>>> index 4cb86b6ce342..1d1cd7a29991 100644
>>> --- a/lib/arm/asm/smp.h
>>> +++ b/lib/arm/asm/smp.h
>>> @@ -14,6 +14,7 @@ extern void halt(void);
>>>  
>>>  extern cpumask_t cpu_present_mask;
>>>  extern cpumask_t cpu_online_mask;
>>> +extern cpumask_t cpu_halted_mask;
>>>  #define cpu_present(cpu)		cpumask_test_cpu(cpu, &cpu_present_mask)
>>>  #define cpu_online(cpu)			cpumask_test_cpu(cpu, &cpu_online_mask)
>>>  #define for_each_present_cpu(cpu)	for_each_cpu(cpu, &cpu_present_mask)
>>> @@ -45,5 +46,6 @@ struct secondary_data {
>>>  extern struct secondary_data secondary_data;
>>>  
>>>  extern void smp_boot_secondary(int cpu, secondary_entry_fn entry);
>>> +extern void smp_run(void (*func)(void));
>>>  
>>>  #endif /* _ASMARM_SMP_H_ */
>>> diff --git a/lib/arm/smp.c b/lib/arm/smp.c
>>> index bbaf9e60e950..8528aa454108 100644
>>> --- a/lib/arm/smp.c
>>> +++ b/lib/arm/smp.c
>>> @@ -15,6 +15,7 @@
>>>  
>>>  cpumask_t cpu_present_mask;
>>>  cpumask_t cpu_online_mask;
>>> +cpumask_t cpu_halted_mask;
>>>  struct secondary_data secondary_data;
>>>  
>>>  secondary_entry_fn secondary_cinit(void)
>>> @@ -53,3 +54,28 @@ void smp_boot_secondary(int cpu, secondary_entry_fn entry)
>>>  	while (!cpu_online(cpu))
>>>  		wfe();
>>>  }
>>> +
>>> +void secondary_halt(void)
>>> +{
>>> +	struct thread_info *ti = current_thread_info();
>>> +
>>> +	cpumask_set_cpu(ti->cpu, &cpu_halted_mask);
>>> +	halt();
>>> +}
>>> +
>>> +void smp_run(void (*func)(void))
>>> +{
>>> +	int cpu;
>>> +
>>> +	for_each_present_cpu(cpu) {
>>> +		if (cpu == 0)
>>> +			continue;
>>> +		smp_boot_secondary(cpu, func);
>>> +	}
>>> +	func();
>>> +
>>> +	cpumask_set_cpu(0, &cpu_halted_mask);
>>> +	while (!cpumask_full(&cpu_halted_mask))
>>> +		cpu_relax();
>>> +	cpumask_clear_cpu(0, &cpu_halted_mask);
>>> +}
>>>

Re: [PATCH kvm-unit-tests 3/6] lib/arm/smp: introduce smp_run

[Andrew Jones]

On Thu, May 25, 2017 at 05:07:25PM +0200, Paolo Bonzini wrote:
> 
> 
> On 25/05/2017 16:41, Andrew Jones wrote:
> > On Thu, May 25, 2017 at 03:56:00PM +0200, Paolo Bonzini wrote:
> >>
> >>
> >> On 25/05/2017 12:28, Andrew Jones wrote:
> >>> A common pattern is
> >>>  - run a function on all cpus
> >>>  - signal each cpu's completion with a cpumask
> >>>  - halt the secondaries when they're complete
> >>>  - have the primary wait on the cpumask for all to complete
> >>>
> >>> smp_run is a wrapper for that pattern. Also, we were allowing
> >>> secondaries to go off in the weeds if they returned from their
> >>> secondary entry function, which can be difficult to debug. A
> >>> nice side-effect of adding this wrapper is we don't do that
> >>> anymore, and can even know when a secondary has halted with the
> >>> new cpu_halted_mask.
> >>
> >> Would it make sense to just call main() this way?
> > 
> > Do you mean to start main() on all cpus all the time and then
> > leave it to the test writer to ensure things that should only
> > be done by the primary are guarded with 'if (cpu == 0)' ? If
> > so, then I think we might over complicate simple tests. I
> > suppose some tests could do something like
> 
> Start main on cpu 0 and put other CPUs in a WFI loop where they can take
> IPIs.  This is similar to how x86 works, and homogeneity would be useful
> I think.

Thinking about this some more, I don't think we need to call main
from smp_run() to be like x86, but rather we just need the two main
API calls on_cpu() and on_cpu_async(). x86 doesn't have something like
smp_run() yet (which would be on_cpus()) and smp_run() currently has the
limitations that it can only be run once and only when secondaries haven't
already been boot. I think I should change that. I propose the following

1) introduce on_cpu() and on_cpu_async() to arm, using PSCI and sev/wfe
   for the underling mechanisms.
2) change smp_run() to be built on on_cpu() and rename it to on_cpus()

How's that sound?

x86 might want an on_cpus() too. I see potentially 16 places it could
be used

 $ git grep -B1 on_cpu | grep -c for
 16

Thanks,
drew

> 
> Paolo
> 
> >  void main(void)
> >  {
> >      if (smp_processor_id() == 0)
> >          smp_run(main);
> >      do_test1();
> >      do_test2();
> >      if (smp_processor_id() == 0)
> >          return report_summary();
> >  }
> > 
> > but I think that looks worse than
> > 
> >  void do_tests(void)
> >  {
> >      do_test1();
> >      do_test2();
> >  }
> > 
> >  void main(void)
> >  {
> >      smp_run(do_tests);
> >      return report_summary();
> >  }
> > 
> > One thing I did think of later is that cpu0 is allowed to
> > halt without setting its bit in cpu_halted_mask. But that
> > can only happen when chr_testdev_exit() fails (and who
> > would look at it?), so it doesn't really matter. I'd fix
> > it up if we want it for completeness though.
> > 
> > Thanks,
> > drew
> > 
> > 
> >>
> >> Paolo
> >>
> >>> Signed-off-by: Andrew Jones <drjones@redhat.com>
> >>> ---
> >>>  arm/cstart.S      |  3 ++-
> >>>  arm/cstart64.S    |  3 ++-
> >>>  lib/arm/asm/smp.h |  2 ++
> >>>  lib/arm/smp.c     | 26 ++++++++++++++++++++++++++
> >>>  4 files changed, 32 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/arm/cstart.S b/arm/cstart.S
> >>> index 12461d104dad..b3176b274933 100644
> >>> --- a/arm/cstart.S
> >>> +++ b/arm/cstart.S
> >>> @@ -118,7 +118,8 @@ secondary_entry:
> >>>  	bl	secondary_cinit
> >>>  
> >>>  	/* r0 is now the entry function, run it */
> >>> -	mov	pc, r0
> >>> +	blx	r0
> >>> +	b	secondary_halt
> >>>  
> >>>  .globl halt
> >>>  halt:
> >>> diff --git a/arm/cstart64.S b/arm/cstart64.S
> >>> index 7738babc4109..30f732f1e99b 100644
> >>> --- a/arm/cstart64.S
> >>> +++ b/arm/cstart64.S
> >>> @@ -79,7 +79,8 @@ secondary_entry:
> >>>  	bl	secondary_cinit
> >>>  
> >>>  	/* x0 is now the entry function, run it */
> >>> -	br	x0
> >>> +	blr	x0
> >>> +	b	secondary_halt
> >>>  
> >>>  .globl halt
> >>>  halt:
> >>> diff --git a/lib/arm/asm/smp.h b/lib/arm/asm/smp.h
> >>> index 4cb86b6ce342..1d1cd7a29991 100644
> >>> --- a/lib/arm/asm/smp.h
> >>> +++ b/lib/arm/asm/smp.h
> >>> @@ -14,6 +14,7 @@ extern void halt(void);
> >>>  
> >>>  extern cpumask_t cpu_present_mask;
> >>>  extern cpumask_t cpu_online_mask;
> >>> +extern cpumask_t cpu_halted_mask;
> >>>  #define cpu_present(cpu)		cpumask_test_cpu(cpu, &cpu_present_mask)
> >>>  #define cpu_online(cpu)			cpumask_test_cpu(cpu, &cpu_online_mask)
> >>>  #define for_each_present_cpu(cpu)	for_each_cpu(cpu, &cpu_present_mask)
> >>> @@ -45,5 +46,6 @@ struct secondary_data {
> >>>  extern struct secondary_data secondary_data;
> >>>  
> >>>  extern void smp_boot_secondary(int cpu, secondary_entry_fn entry);
> >>> +extern void smp_run(void (*func)(void));
> >>>  
> >>>  #endif /* _ASMARM_SMP_H_ */
> >>> diff --git a/lib/arm/smp.c b/lib/arm/smp.c
> >>> index bbaf9e60e950..8528aa454108 100644
> >>> --- a/lib/arm/smp.c
> >>> +++ b/lib/arm/smp.c
> >>> @@ -15,6 +15,7 @@
> >>>  
> >>>  cpumask_t cpu_present_mask;
> >>>  cpumask_t cpu_online_mask;
> >>> +cpumask_t cpu_halted_mask;
> >>>  struct secondary_data secondary_data;
> >>>  
> >>>  secondary_entry_fn secondary_cinit(void)
> >>> @@ -53,3 +54,28 @@ void smp_boot_secondary(int cpu, secondary_entry_fn entry)
> >>>  	while (!cpu_online(cpu))
> >>>  		wfe();
> >>>  }
> >>> +
> >>> +void secondary_halt(void)
> >>> +{
> >>> +	struct thread_info *ti = current_thread_info();
> >>> +
> >>> +	cpumask_set_cpu(ti->cpu, &cpu_halted_mask);
> >>> +	halt();
> >>> +}
> >>> +
> >>> +void smp_run(void (*func)(void))
> >>> +{
> >>> +	int cpu;
> >>> +
> >>> +	for_each_present_cpu(cpu) {
> >>> +		if (cpu == 0)
> >>> +			continue;
> >>> +		smp_boot_secondary(cpu, func);
> >>> +	}
> >>> +	func();
> >>> +
> >>> +	cpumask_set_cpu(0, &cpu_halted_mask);
> >>> +	while (!cpumask_full(&cpu_halted_mask))
> >>> +		cpu_relax();
> >>> +	cpumask_clear_cpu(0, &cpu_halted_mask);
> >>> +}
> >>>

Re: [PATCH kvm-unit-tests 3/6] lib/arm/smp: introduce smp_run

[Paolo Bonzini]

On 26/05/2017 13:47, Andrew Jones wrote:
> Thinking about this some more, I don't think we need to call main
> from smp_run() to be like x86, but rather we just need the two main
> API calls on_cpu() and on_cpu_async(). x86 doesn't have something like
> smp_run() yet (which would be on_cpus()) and smp_run() currently has the
> limitations that it can only be run once and only when secondaries haven't
> already been boot. I think I should change that. I propose the following
> 
> 1) introduce on_cpu() and on_cpu_async() to arm, using PSCI and sev/wfe
>    for the underling mechanisms.
> 2) change smp_run() to be built on on_cpu() and rename it to on_cpus()
> 
> How's that sound?
> 
> x86 might want an on_cpus() too. I see potentially 16 places it could
> be used
> 
>  $ git grep -B1 on_cpu | grep -c for
>  16

Both sound good, thanks!

Paolo

[PATCH kvm-unit-tests 4/6] arm/arm64: selftest: apply smp_run

[Andrew Jones]

Signed-off-by: Andrew Jones <drjones@redhat.com>
---
 arm/selftest.c | 19 +------------------
 1 file changed, 1 insertion(+), 18 deletions(-)

diff --git a/arm/selftest.c b/arm/selftest.c
index f305f4e3be19..dd750c418644 100644
--- a/arm/selftest.c
+++ b/arm/selftest.c
@@ -15,7 +15,6 @@
 #include <asm/thread_info.h>
 #include <asm/psci.h>
 #include <asm/smp.h>
-#include <asm/cpumask.h>
 #include <asm/barrier.h>
 
 static void check_setup(int argc, char **argv)
@@ -309,7 +308,6 @@ static bool psci_check(void)
 	return true;
 }
 
-static cpumask_t smp_reported;
 static void cpu_report(void)
 {
 	uint64_t mpidr = get_mpidr();
@@ -317,8 +315,6 @@ static void cpu_report(void)
 
 	report("CPU(%3d) mpidr=%010" PRIx64,
 		mpidr_to_cpu(mpidr) == cpu, cpu, mpidr);
-	cpumask_set_cpu(cpu, &smp_reported);
-	halt();
 }
 
 int main(int argc, char **argv)
@@ -345,22 +341,9 @@ int main(int argc, char **argv)
 
 	} else if (strcmp(argv[1], "smp") == 0) {
 
-		uint64_t mpidr = get_mpidr();
-		int cpu;
-
 		report("PSCI version", psci_check());
+		smp_run(cpu_report);
 
-		for_each_present_cpu(cpu) {
-			if (cpu == 0)
-				continue;
-			smp_boot_secondary(cpu, cpu_report);
-		}
-
-		report("CPU(%3d) mpidr=%010" PRIx64,
-			mpidr_to_cpu(mpidr) == 0, 0, mpidr);
-		cpumask_set_cpu(0, &smp_reported);
-		while (!cpumask_full(&smp_reported))
-			cpu_relax();
 	} else {
 		printf("Unknown subtest\n");
 		abort();
-- 
2.9.4

[PATCH kvm-unit-tests 5/6] arm/arm64: spinlock-test: apply smp_run

[Andrew Jones]

Signed-off-by: Andrew Jones <drjones@redhat.com>
---
 arm/spinlock-test.c | 20 +-------------------
 1 file changed, 1 insertion(+), 19 deletions(-)

diff --git a/arm/spinlock-test.c b/arm/spinlock-test.c
index b2fdbab03f83..d1e45d2d9b05 100644
--- a/arm/spinlock-test.c
+++ b/arm/spinlock-test.c
@@ -12,7 +12,6 @@
 
 #include <libcflat.h>
 #include <asm/smp.h>
-#include <asm/cpumask.h>
 #include <asm/barrier.h>
 
 #define LOOP_SIZE 10000000
@@ -44,8 +43,6 @@ static void none_unlock(int *lock_var)
 static int global_a, global_b;
 static int global_lock;
 
-static cpumask_t smp_test_complete;
-
 static void test_spinlock(void)
 {
 	int i, errors = 0;
@@ -71,16 +68,10 @@ static void test_spinlock(void)
 		lock_ops.unlock(&global_lock);
 	}
 	report("CPU%d: Done - Errors: %d", errors == 0, cpu, errors);
-
-	cpumask_set_cpu(cpu, &smp_test_complete);
-	if (cpu != 0)
-		halt();
 }
 
 int main(int argc, char **argv)
 {
-	int cpu;
-
 	if (argc > 1 && strcmp(argv[1], "bad") != 0) {
 		lock_ops.lock = gcc_builtin_lock;
 		lock_ops.unlock = gcc_builtin_unlock;
@@ -89,16 +80,7 @@ int main(int argc, char **argv)
 		lock_ops.unlock = none_unlock;
 	}
 
-	for_each_present_cpu(cpu) {
-		if (cpu == 0)
-			continue;
-		smp_boot_secondary(cpu, test_spinlock);
-	}
-
-	test_spinlock();
-
-	while (!cpumask_full(&smp_test_complete))
-		cpu_relax();
+	smp_run(test_spinlock);
 
 	return report_summary();
 }
-- 
2.9.4

[PATCH kvm-unit-tests 6/6] arm/arm64: gic: apply smp_run

[Andrew Jones]

Signed-off-by: Andrew Jones <drjones@redhat.com>
---
 arm/gic.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/arm/gic.c b/arm/gic.c
index 82f663218b0b..629d6b51f68f 100644
--- a/arm/gic.c
+++ b/arm/gic.c
@@ -240,6 +240,14 @@ static void ipi_recv(void)
 		wfi();
 }
 
+static void ipi_test(void)
+{
+	if (smp_processor_id() == IPI_SENDER)
+		ipi_send();
+	else
+		ipi_recv();
+}
+
 static struct gic gicv2 = {
 	.ipi = {
 		.send_self = gicv2_ipi_send_self,
@@ -298,7 +306,6 @@ static void run_active_clear_test(void)
 int main(int argc, char **argv)
 {
 	char pfx[8];
-	int cpu;
 
 	if (!gic_init()) {
 		printf("No supported gic present, skipping tests...\n");
@@ -323,14 +330,7 @@ int main(int argc, char **argv)
 	if (strcmp(argv[1], "ipi") == 0) {
 		report_prefix_push(argv[1]);
 		nr_cpu_check(2);
-
-		for_each_present_cpu(cpu) {
-			if (cpu == 0)
-				continue;
-			smp_boot_secondary(cpu,
-				cpu == IPI_SENDER ? ipi_send : ipi_recv);
-		}
-		ipi_recv();
+		smp_run(ipi_test);
 	} else if (strcmp(argv[1], "active") == 0) {
 		run_active_clear_test();
 	} else {
-- 
2.9.4

Re: [PATCH kvm-unit-tests 0/6] arm/arm64: some cleanups

[Paolo Bonzini]

On 25/05/2017 12:28, Andrew Jones wrote:
> Andrew Jones (6):
>   lib/arm/gic: gic_version cleanup
>   lib/arm/psci: make psci less ugly
>   lib/arm/smp: introduce smp_run
>   arm/arm64: selftest: apply smp_run
>   arm/arm64: spinlock-test: apply smp_run
>   arm/arm64: gic: apply smp_run
> 
>  arm/cstart.S         |  3 ++-
>  arm/cstart64.S       |  3 ++-
>  arm/gic.c            | 18 +++++++++---------
>  arm/selftest.c       | 19 +------------------
>  arm/spinlock-test.c  | 20 +-------------------
>  lib/arm/asm/psci.h   |  6 ++----
>  lib/arm/asm/smp.h    |  2 ++
>  lib/arm/gic.c        | 27 ++++++++++++---------------
>  lib/arm/psci.c       | 10 +++++++---
>  lib/arm/smp.c        | 26 ++++++++++++++++++++++++++
>  lib/arm64/asm/psci.h | 16 +---------------
>  11 files changed, 65 insertions(+), 85 deletions(-)

Nice, I made a couple more proposals in reply to single messages.
Queued, thanks.

Paolo