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

[Simon Glass]

Hi Bin,

On Sun, 28 Jun 2020 at 01:35, Bin Meng <bmeng.cn at gmail.com> wrote:
>
> 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 ..

It does seem to be, but I worry that we might hit an architecture where it
is not?

>
> > +        */
> > +       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.

Yes, 0...n-1 as set up by mp_init() using values from the device tree.

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

No, you must provide the same value for cpu_select to both calls.

It does say that in the header file but I will add another note there.

>
> > +               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.

I will beef up the comments. cpu_select must be the same for all calls for
the iteration to work.

>
> > + *
> > + * @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?

mp_first_cpu() will return that CPU
mp_next_cpu() will return -EFBIG of course, since the CPU has already run,
so there is no 'next' CPU

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

Think of the two functions as part of the same iterator.

Regards,
SImon