Re: [PATCH v1 02/14] xen/riscv: introduce smp_clear_cpu_maps()

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 4/10/25 3:10 PM, Jan Beulich wrote:

> On 08.04.2025 17:57, Oleksii Kurochko wrote:
> > Initialize cpu_{possible, online, present}_map by using smp_clear_cpu_maps().
> >
> > Drop DEFINE_PER_CPU(unsigned int, cpu_id) from stubs.c as this variable isn't
> > expected to be used in RISC-V at all.
> >
> > Move declaration of cpu_{possible,online,present}_map from stubs.c to smpboot.c
> > as now smpboot.c is now introduced.
> > Other defintions keep in stubs.c as they are not initialized and not needed, at
> > the moment.
> >
> > Signed-off-by: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>
> > ---
> >  xen/arch/riscv/Makefile          |  1 +
> >  xen/arch/riscv/include/asm/smp.h |  2 ++
> >  xen/arch/riscv/setup.c           |  2 ++
> >  xen/arch/riscv/smpboot.c         | 15 +++++++++++++++
> >  xen/arch/riscv/stubs.c           |  6 ------
> >  5 files changed, 20 insertions(+), 6 deletions(-)
> >  create mode 100644 xen/arch/riscv/smpboot.c
> >
> > diff --git a/xen/arch/riscv/Makefile b/xen/arch/riscv/Makefile
> > index 0c6c4a38a3..f551bf32a2 100644
> > --- a/xen/arch/riscv/Makefile
> > +++ b/xen/arch/riscv/Makefile
> > @@ -10,6 +10,7 @@ obj-y += sbi.o
> >  obj-y += setup.o
> >  obj-y += shutdown.o
> >  obj-y += smp.o
> > +obj-y += smpboot.o
> >  obj-y += stubs.o
> >  obj-y += time.o
> >  obj-y += traps.o
> > diff --git a/xen/arch/riscv/include/asm/smp.h b/xen/arch/riscv/include/asm/smp.h
> > index 5e170b57b3..188c033718 100644
> > --- a/xen/arch/riscv/include/asm/smp.h
> > +++ b/xen/arch/riscv/include/asm/smp.h
> > @@ -26,6 +26,8 @@ static inline void set_cpuid_to_hartid(unsigned long cpuid,
> >  
> >  void setup_tp(unsigned int cpuid);
> >  
> > +void smp_clear_cpu_maps(void);
> > +
> >  #endif
> >  
> >  /*
> > diff --git a/xen/arch/riscv/setup.c b/xen/arch/riscv/setup.c
> > index 4e416f6e44..7f68f3f5b7 100644
> > --- a/xen/arch/riscv/setup.c
> > +++ b/xen/arch/riscv/setup.c
> > @@ -72,6 +72,8 @@ void __init noreturn start_xen(unsigned long bootcpu_id,
> >  
> >      remove_identity_mapping();
> >  
> > +    smp_clear_cpu_maps();
> > +
> >      set_processor_id(0);
> >  
> >      set_cpuid_to_hartid(0, bootcpu_id);
> > diff --git a/xen/arch/riscv/smpboot.c b/xen/arch/riscv/smpboot.c
> > new file mode 100644
> > index 0000000000..0f4dcc28e1
> > --- /dev/null
> > +++ b/xen/arch/riscv/smpboot.c
> > @@ -0,0 +1,15 @@
> > +#include <xen/cpumask.h>
> > +#include <xen/init.h>
> > +
> > +cpumask_t cpu_online_map;
> > +cpumask_t cpu_present_map;
> > +cpumask_t cpu_possible_map;
> >       
> __read_mostly for all of them, perhaps (if CPU hotplug isn't expected to
> be supported) even __ro_after_init for the latter two?
>     
We have been living without CPU hotplug support for a long time in the downstream
branch, but I can't say whether it is expected to be supported in the future or not.
To ensure we can add such an option later without changing the attributes of
cpu_online_map variable, I prefer to use __read_mostly here and __ro_after_init for
cpu_possible_map.

> As to cpu_possible_map - do you predict that you'll actually use it? Arm
> does (and instead has only a fake cpu_present_map), but on x86 we get away
> without.
>     
I checked how it is used now in downstream latest branch and it isn't really used
only during initialization smp_clear_cpu_maps() and smp_prepare_cpus() so we can
skip it for RISC-V too.
> > +void __init smp_clear_cpu_maps(void)
> > +{
> > +    cpumask_clear(&cpu_possible_map);
> > +    cpumask_clear(&cpu_online_map);
> >       
> What's the point of these? All three maps start out fully zeroed.
>     
It could be really dropped. I saw your patch for Arm, I'll align the current
patch with that changes.

> > +    cpumask_set_cpu(0, &cpu_possible_map);
> > +    cpumask_set_cpu(0, &cpu_online_map);
> >       
> These are contradicting the name of the function. The somewhat equivalent
> function we have on x86 is smp_prepare_boot_cpu().
>
> > +    cpumask_copy(&cpu_present_map, &cpu_possible_map);
> >       
> Another cpumask_set_cpu() is probably cheaper here then.
>     
What do you mean by cheaper here?
~ Oleksii