On 29.05.2024 11:01, Roger Pau Monne wrote:
> The current callers of stop_machine_run() outside of init code already have
> the
> CPU maps locked, and hence there's no reason for stop_machine_run() to attempt
> to lock again.
While purely from a description perspective this is okay, ...
> --- a/xen/common/stop_machine.c
> +++ b/xen/common/stop_machine.c
> @@ -82,9 +82,15 @@ int stop_machine_run(int (*fn)(void *data), void *data,
> unsigned int cpu)
> BUG_ON(!local_irq_is_enabled());
> BUG_ON(!is_idle_vcpu(current));
>
> - /* cpu_online_map must not change. */
> - if ( !get_cpu_maps() )
> + /*
> + * cpu_online_map must not change. The only two callers of
> + * stop_machine_run() outside of init code already have the CPU map
> locked.
> + */
... the "two" here is not unlikely to quickly go stale; who knows what PPC
and RISC-V will have as their code becomes more complete?
I'm also unconvinced that requiring ...
> + if ( system_state >= SYS_STATE_active && !cpu_map_locked() )
... this for all future (post-init) uses of stop_machine_run() is a good
idea. It is quite a bit more natural, to me at least, for the function to
effect this itself, as is the case prior to your change.
In fact I'm puzzled by Arm's (init-time) use: They use it twice, for errata
workaround and feature enabling. They do that after bringing up secondary
CPUs, but still from start_xen(). How's that going to work for CPUs brought
offline and then back online later on? __cpu_up() isn't __init, so there's
no obvious sign that soft-{off,on}lining isn't possible on Arm. Cc-ing Arm
maintainers.
Jan
> + {
> + ASSERT_UNREACHABLE();
> return -EBUSY;
> + }
>
> nr_cpus = num_online_cpus();
> if ( cpu_online(this) )
> @@ -92,10 +98,7 @@ int stop_machine_run(int (*fn)(void *data), void *data,
> unsigned int cpu)
>
> /* Must not spin here as the holder will expect us to be descheduled. */
> if ( !spin_trylock(&stopmachine_lock) )
> - {
> - put_cpu_maps();
> return -EBUSY;
> - }
>
> stopmachine_data.fn = fn;
> stopmachine_data.fn_data = data;
> @@ -136,8 +139,6 @@ int stop_machine_run(int (*fn)(void *data), void *data,
> unsigned int cpu)
>
> spin_unlock(&stopmachine_lock);
>
> - put_cpu_maps();
> -
> return ret;
> }
>
> diff --git a/xen/include/xen/cpu.h b/xen/include/xen/cpu.h
> index e1d4eb59675c..d8c8264c58b0 100644
> --- a/xen/include/xen/cpu.h
> +++ b/xen/include/xen/cpu.h
> @@ -13,6 +13,8 @@ void put_cpu_maps(void);
> void cpu_hotplug_begin(void);
> void cpu_hotplug_done(void);
>
> +bool cpu_map_locked(void);
> +
> /* Receive notification of CPU hotplug events. */
> void register_cpu_notifier(struct notifier_block *nb);
>
