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

[Oleksii Kurochko <oleksii.kurochko@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 3995 bytes --]


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@gmail.com>
>> ---
>>   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

[-- Attachment #2: Type: text/html, Size: 5420 bytes --]