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@xxxxxxxxxx>
> >>> ---
> >>> 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);
> >>> +}
> >>>
