Hi Bin,
On Sun, 28 Jun 2020 at 01:35, Bin Meng <bmeng...@gmail.com> wrote:
>
> Hi Simon,
>
> On Mon, Jun 15, 2020 at 1:00 AM Simon Glass <s...@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 <s...@chromium.org>
> > Reviewed-by: Wolfgang Wallner <wolfgang.wall...@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
