[PATCH v2 15/25] x86: mp: Add iterators for CPUs

[Bin Meng]

Hi Simon,

On Mon, Jun 15, 2020 at 1:00 AM Simon Glass <sjg at chromium.org> wrote:
>
> It is convenient to iterate through the CPUs performing work on each one
> and processing the result. Add a few iterator functions which handle this.
> These can be used by any client code. It can call mp_run_on_cpus() on
> each CPU that is returned, handling them one at a time.
>
> Signed-off-by: Simon Glass <sjg at chromium.org>
> Reviewed-by: Wolfgang Wallner <wolfgang.wallner at br-automation.com>
> ---
>
> (no changes since v1)
>
>  arch/x86/cpu/mp_init.c    | 62 +++++++++++++++++++++++++++++++++++++++
>  arch/x86/include/asm/mp.h | 40 +++++++++++++++++++++++++
>  2 files changed, 102 insertions(+)
>
> diff --git a/arch/x86/cpu/mp_init.c b/arch/x86/cpu/mp_init.c
> index 9970b51c8d..c708c3e3c0 100644
> --- a/arch/x86/cpu/mp_init.c
> +++ b/arch/x86/cpu/mp_init.c
> @@ -675,6 +675,68 @@ int mp_park_aps(void)
>         return get_timer(start);
>  }
>
> +int mp_first_cpu(int cpu_select)
> +{
> +       struct udevice *dev;
> +       int num_cpus;
> +       int ret;
> +
> +       /*
> +        * This assumes that CPUs are numbered from 0. This function tries to
> +        * avoid assuming the CPU 0 is the boot CPU

So CPU 0 is not BSP ..

> +        */
> +       if (cpu_select == MP_SELECT_ALL)
> +               return 0;   /* start with the first one */
> +
> +       ret = get_bsp(&dev, &num_cpus);
> +       if (ret < 0)
> +               return log_msg_ret("bsp", ret);
> +
> +       /* Return boot CPU if requested */
> +       if (cpu_select == MP_SELECT_BSP)
> +               return ret;
> +
> +       /* Return something other than the boot CPU, if APs requested */
> +       if (cpu_select == MP_SELECT_APS && num_cpus > 1)
> +               return ret == 0 ? 1 : 0;
> +
> +       /* Try to check for an invalid value */
> +       if (cpu_select < 0 || cpu_select >= num_cpus)

The logic (cpu_select >= num_cpus) assumes that cpu number is consecutive.

> +               return -EINVAL;
> +
> +       return cpu_select;  /* return the only selected one */
> +}
> +
> +int mp_next_cpu(int cpu_select, int prev_cpu)
> +{
> +       struct udevice *dev;
> +       int num_cpus;
> +       int ret;
> +       int bsp;
> +
> +       /* If we selected the BSP or a particular single CPU, we are done */
> +       if (cpu_select == MP_SELECT_BSP || cpu_select >= 0)

Why stops on MP_SELECT_BSP?

So if I call the 2 APIs with the following sequence, is this allowed?

int cpu = mp_first_cpu(MP_SELECT_ALL); // this will return zero
cpu = mp_next_cpu(MP_SELECT_BSP, cpu);  // then I got -EFBIG

> +               return -EFBIG;
> +
> +       /* Must be doing MP_SELECT_ALL or MP_SELECT_APS; return the next CPU */
> +       ret = get_bsp(&dev, &num_cpus);
> +       if (ret < 0)
> +               return log_msg_ret("bsp", ret);
> +       bsp = ret;
> +
> +       /* Move to the next CPU */
> +       assert(prev_cpu >= 0);
> +       ret = prev_cpu + 1;
> +
> +       /* Skip the BSP if needed */
> +       if (cpu_select == MP_SELECT_APS && ret == bsp)
> +               ret++;
> +       if (ret >= num_cpus)
> +               return -EFBIG;
> +
> +       return ret;
> +}
> +
>  int mp_init(void)
>  {
>         int num_aps, num_cpus;
> diff --git a/arch/x86/include/asm/mp.h b/arch/x86/include/asm/mp.h
> index 38961ca44b..9f4223ae8c 100644
> --- a/arch/x86/include/asm/mp.h
> +++ b/arch/x86/include/asm/mp.h
> @@ -115,6 +115,31 @@ int mp_run_on_cpus(int cpu_select, mp_run_func func, void *arg);
>   * @return 0 on success, -ve on error
>   */
>  int mp_park_aps(void);
> +
> +/**
> + * mp_first_cpu() - Get the first CPU to process, from a selection
> + *
> + * This is used to iterate through selected CPUs. Call this function first, then
> + * call mp_next_cpu() repeatedly until it returns -EFBIG.

So how does specify the cpu_select of these 2 APIs? Do they have to be the same?

For example, how about the following code sequence:

int cpu = mp_first_cpu(MP_SELECT_APS);
cpu = mp_next_cpu(MP_SELECT_BSP, cpu);
cpu = mp_next_cpu(MP_SELECT_APS, cpu);

It's quite ambiguous API design.

> + *
> + * @cpu_select: Selected CPUs (either a CPU number or MP_SELECT_...)
> + * @return next CPU number to run on (e.g. 0)
> + */
> +int mp_first_cpu(int cpu_select);
> +
> +/**
> + * mp_next_cpu() - Get the next CPU to process, from a selection
> + *
> + * This is used to iterate through selected CPUs. After first calling
> + * mp_first_cpu() once, call this function repeatedly until it returns -EFBIG.
> + *
> + * The value of @cpu_select must be the same for all calls.
> + *
> + * @cpu_select: Selected CPUs (either a CPU number or MP_SELECT_...)

Per the codes, if cpu_select >=0, -EFBIG will be returned. So this
suggests that cpu_selct can only be MP_SELECT_APS?

I have to say that these 2 APIs are hard to understand ..

> + * @prev_cpu: Previous value returned by mp_first_cpu()/mp_next_cpu()
> + * @return next CPU number to run on (e.g. 0)
> + */
> +int mp_next_cpu(int cpu_select, int prev_cpu);
>  #else
>  static inline int mp_run_on_cpus(int cpu_select, mp_run_func func, void *arg)
>  {
> @@ -131,6 +156,21 @@ static inline int mp_park_aps(void)
>         return 0;
>  }
>
> +static inline int mp_first_cpu(int cpu_select)
> +{
> +       /* We cannot run on any APs, nor a selected CPU */
> +       return cpu_select == MP_SELECT_APS ? -EFBIG : MP_SELECT_BSP;
> +}
> +
> +static inline int mp_next_cpu(int cpu_select, int prev_cpu)
> +{
> +       /*
> +        * When MP is not enabled, there is only one CPU and we did it in
> +        * mp_first_cpu()
> +        */
> +       return -EFBIG;
> +}
> +
>  #endif
>
>  #endif /* _X86_MP_H_ */

Regards,
Bin