| Message ID | 4ea9005d4209e24df9b30a7b3c282276084a3cf1.1721834549.git.oleksii.kurochko@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | RISCV device tree mapping | expand |
On 24.07.2024 17:31, Oleksii Kurochko wrote: > --- a/xen/arch/riscv/include/asm/processor.h > +++ b/xen/arch/riscv/include/asm/processor.h > @@ -12,8 +12,39 @@ > > #ifndef __ASSEMBLY__ > > -/* TODO: need to be implemeted */ > -#define smp_processor_id() 0 > +#include <xen/bug.h> > +#include <xen/types.h> > + > +register struct pcpu_info *tp asm ("tp"); > + > +struct pcpu_info { > + unsigned int processor_id; > +}; > + > +/* tp points to one of these */ > +extern struct pcpu_info pcpu_info[NR_CPUS]; > + > +#define get_processor_id() (tp->processor_id) > +#define set_processor_id(id) do { \ > + tp->processor_id = id; \ Nit: Parentheses around id please. > +} while(0) While often we omit the blanks inside the parentheses for such constructs, the one ahead of the opening paren should still be there. > +static inline unsigned int smp_processor_id(void) > +{ > + unsigned int id; > + > + id = get_processor_id(); > + > + /* > + * Technically the hartid can be greater than what a uint can hold. > + * If such a system were to exist, we will need to change > + * the smp_processor_id() API to be unsigned long instead of > + * unsigned int. > + */ > + BUG_ON(id > UINT_MAX); Compilers may complaing about this condition being always false. But: Why do you check against UINT_MAX, not against NR_CPUS? It's not the hart ID your retrieving get_processor_id(), but Xen's, isn't it? Even if I'm missing something here, ... > --- a/xen/arch/riscv/include/asm/smp.h > +++ b/xen/arch/riscv/include/asm/smp.h > @@ -5,6 +5,8 @@ > #include <xen/cpumask.h> > #include <xen/percpu.h> > > +#define INVALID_HARTID UINT_MAX ... this implies that the check above would need to use >=. > @@ -14,6 +16,14 @@ DECLARE_PER_CPU(cpumask_var_t, cpu_core_mask); > */ > #define park_offline_cpus false > > +void smp_setup_processor_id(unsigned long boot_cpu_hartid); > + > +/* > + * Mapping between linux logical cpu index and hartid. > + */ > +extern unsigned long __cpuid_to_hartid_map[NR_CPUS]; > +#define cpuid_to_hartid_map(cpu) __cpuid_to_hartid_map[cpu] May I please ask that there be no new variables with double underscores at the front or any other namespacing violations? > --- a/xen/arch/riscv/setup.c > +++ b/xen/arch/riscv/setup.c > @@ -40,6 +40,19 @@ void __init noreturn start_xen(unsigned long bootcpu_id, > { > remove_identity_mapping(); > > + /* > + * tp register contains an address of physical cpu information. > + * So write physical CPU info of boot cpu to tp register > + * It will be used later by get_processor_id() to get process_id ( look at process_id? > + * <asm/processor.h> ): > + * #define get_processor_id() (tp->processor_id) > + */ > + asm volatile ("mv tp, %0" : : "r"((unsigned long)&pcpu_info[0])); Nit: Blanks missing. > --- /dev/null > +++ b/xen/arch/riscv/smp.c > @@ -0,0 +1,4 @@ > +#include <xen/smp.h> > + > +/* tp points to one of these per cpu */ > +struct pcpu_info pcpu_info[NR_CPUS]; > \ No newline at end of file Please correct this. > --- /dev/null > +++ b/xen/arch/riscv/smpboot.c > @@ -0,0 +1,12 @@ > +#include <xen/init.h> > +#include <xen/sections.h> > +#include <xen/smp.h> > + > +unsigned long __cpuid_to_hartid_map[NR_CPUS] __ro_after_init = { > + [0 ... NR_CPUS-1] = INVALID_HARTID Nit: Blanks around - please. > +}; > + > +void __init smp_setup_processor_id(unsigned long boot_cpu_hartid) > +{ > + cpuid_to_hartid_map(0) = boot_cpu_hartid; > +} The function name suggests this can be used for all CPUs, but the code is pretty clear abut this being for the boot CPU only.
On Mon, 2024-07-29 at 17:28 +0200, Jan Beulich wrote: > On 24.07.2024 17:31, Oleksii Kurochko wrote: > > --- a/xen/arch/riscv/include/asm/processor.h > > +++ b/xen/arch/riscv/include/asm/processor.h > > @@ -12,8 +12,39 @@ > > > > #ifndef __ASSEMBLY__ > > > > -/* TODO: need to be implemeted */ > > -#define smp_processor_id() 0 > > +#include <xen/bug.h> > > +#include <xen/types.h> > > + > > +register struct pcpu_info *tp asm ("tp"); > > + > > +struct pcpu_info { > > + unsigned int processor_id; > > +}; > > + > > +/* tp points to one of these */ > > +extern struct pcpu_info pcpu_info[NR_CPUS]; > > + > > +#define get_processor_id() (tp->processor_id) > > +#define set_processor_id(id) do { \ > > + tp->processor_id = id; \ > > Nit: Parentheses around id please. > > > +} while(0) > > While often we omit the blanks inside the parentheses for such > constructs, the one ahead of the opening paren should still be there. > > > +static inline unsigned int smp_processor_id(void) > > +{ > > + unsigned int id; > > + > > + id = get_processor_id(); > > + > > + /* > > + * Technically the hartid can be greater than what a uint can > > hold. > > + * If such a system were to exist, we will need to change > > + * the smp_processor_id() API to be unsigned long instead of > > + * unsigned int. > > + */ > > + BUG_ON(id > UINT_MAX); > > Compilers may complaing about this condition being always false. But: > Why > do you check against UINT_MAX, not against NR_CPUS? Because HART id theoretically could be greater then what unsigned int can provide thereby NR_CPUS could be also greater then unsigned int ( or it can't ? ). Generally I agree it would be better to compare it with NR_CPUS. > It's not the hart ID > your retrieving get_processor_id(), but Xen's, isn't it? You are right it is Xen's CPU id. > Even if I'm > missing something here, ... > > > --- a/xen/arch/riscv/include/asm/smp.h > > +++ b/xen/arch/riscv/include/asm/smp.h > > @@ -5,6 +5,8 @@ > > #include <xen/cpumask.h> > > #include <xen/percpu.h> > > > > +#define INVALID_HARTID UINT_MAX > > ... this implies that the check above would need to use >=. > > > @@ -14,6 +16,14 @@ DECLARE_PER_CPU(cpumask_var_t, cpu_core_mask); > > */ > > #define park_offline_cpus false > > > > +void smp_setup_processor_id(unsigned long boot_cpu_hartid); > > + > > +/* > > + * Mapping between linux logical cpu index and hartid. > > + */ > > +extern unsigned long __cpuid_to_hartid_map[NR_CPUS]; > > +#define cpuid_to_hartid_map(cpu) __cpuid_to_hartid_map[cpu] > > May I please ask that there be no new variables with double > underscores > at the front or any other namespacing violations? I just couldn't come up with better naming for __cpuid_to_hartid_map[] to show that it is private array. I will update the namings here. > > --- a/xen/arch/riscv/setup.c > > +++ b/xen/arch/riscv/setup.c > > @@ -40,6 +40,19 @@ void __init noreturn start_xen(unsigned long > > bootcpu_id, > > { > > remove_identity_mapping(); > > > > + /* > > + * tp register contains an address of physical cpu > > information. > > + * So write physical CPU info of boot cpu to tp register > > + * It will be used later by get_processor_id() to get > > process_id ( look at > > process_id? I meant processor_id here. > > > > +}; > > + > > +void __init smp_setup_processor_id(unsigned long boot_cpu_hartid) > > +{ > > + cpuid_to_hartid_map(0) = boot_cpu_hartid; > > +} > > The function name suggests this can be used for all CPUs, but the > code is pretty clear abut this being for the boot CPU only. Then I will rename it to setup_bootcpu_id(...). Thanks. ~ Oleksii
On 29.07.2024 18:32, oleksii.kurochko@gmail.com wrote: > On Mon, 2024-07-29 at 17:28 +0200, Jan Beulich wrote: >> On 24.07.2024 17:31, Oleksii Kurochko wrote: >>> +static inline unsigned int smp_processor_id(void) >>> +{ >>> + unsigned int id; >>> + >>> + id = get_processor_id(); >>> + >>> + /* >>> + * Technically the hartid can be greater than what a uint can >>> hold. >>> + * If such a system were to exist, we will need to change >>> + * the smp_processor_id() API to be unsigned long instead of >>> + * unsigned int. >>> + */ >>> + BUG_ON(id > UINT_MAX); >> >> Compilers may complaing about this condition being always false. But: >> Why >> do you check against UINT_MAX, not against NR_CPUS? > Because HART id theoretically could be greater then what unsigned int > can provide thereby NR_CPUS could be also greater then unsigned int ( > or it can't ? ). Well, there are two aspects here. On a system with billions of harts, we wouldn't be able to bring up all of them anyway. NR_CPUS is presently limited at about 16k. Yet than I have no idea whether hart IDs need to be all consecutive. On other hardware (x86 for example), the analog APIC IDs don't need to be. Hence I could see there being large hart IDs (unless excluded somewhere in the spec), which then you map to consecutive Xen CPU IDs, all having relatively small numbers (less than NR_CPUS). If hart IDs can be sparse and wider than 32 bits, then I'd suggest to use an appropriately typed array right away for the Xen -> hart translation. For the hart -> Xen translation, if also needed, an array may then not be appropriate to use. Jan
diff --git a/xen/arch/riscv/Makefile b/xen/arch/riscv/Makefile index 81b77b13d6..334fd24547 100644 --- a/xen/arch/riscv/Makefile +++ b/xen/arch/riscv/Makefile @@ -4,6 +4,8 @@ obj-y += mm.o obj-$(CONFIG_RISCV_64) += riscv64/ obj-y += sbi.o obj-y += setup.o +obj-y += smp.o +obj-y += smpboot.o obj-y += stubs.o obj-y += traps.o obj-y += vm_event.o diff --git a/xen/arch/riscv/include/asm/processor.h b/xen/arch/riscv/include/asm/processor.h index 3ae164c265..4c3e45fd17 100644 --- a/xen/arch/riscv/include/asm/processor.h +++ b/xen/arch/riscv/include/asm/processor.h @@ -12,8 +12,39 @@ #ifndef __ASSEMBLY__ -/* TODO: need to be implemeted */ -#define smp_processor_id() 0 +#include <xen/bug.h> +#include <xen/types.h> + +register struct pcpu_info *tp asm ("tp"); + +struct pcpu_info { + unsigned int processor_id; +}; + +/* tp points to one of these */ +extern struct pcpu_info pcpu_info[NR_CPUS]; + +#define get_processor_id() (tp->processor_id) +#define set_processor_id(id) do { \ + tp->processor_id = id; \ +} while(0) + +static inline unsigned int smp_processor_id(void) +{ + unsigned int id; + + id = get_processor_id(); + + /* + * Technically the hartid can be greater than what a uint can hold. + * If such a system were to exist, we will need to change + * the smp_processor_id() API to be unsigned long instead of + * unsigned int. + */ + BUG_ON(id > UINT_MAX); + + return id; +} /* On stack VCPU state */ struct cpu_user_regs diff --git a/xen/arch/riscv/include/asm/smp.h b/xen/arch/riscv/include/asm/smp.h index b1ea91b1eb..3fff27a8a5 100644 --- a/xen/arch/riscv/include/asm/smp.h +++ b/xen/arch/riscv/include/asm/smp.h @@ -5,6 +5,8 @@ #include <xen/cpumask.h> #include <xen/percpu.h> +#define INVALID_HARTID UINT_MAX + DECLARE_PER_CPU(cpumask_var_t, cpu_sibling_mask); DECLARE_PER_CPU(cpumask_var_t, cpu_core_mask); @@ -14,6 +16,14 @@ DECLARE_PER_CPU(cpumask_var_t, cpu_core_mask); */ #define park_offline_cpus false +void smp_setup_processor_id(unsigned long boot_cpu_hartid); + +/* + * Mapping between linux logical cpu index and hartid. + */ +extern unsigned long __cpuid_to_hartid_map[NR_CPUS]; +#define cpuid_to_hartid_map(cpu) __cpuid_to_hartid_map[cpu] + #endif /* diff --git a/xen/arch/riscv/setup.c b/xen/arch/riscv/setup.c index 13f0e8c77d..37b234360c 100644 --- a/xen/arch/riscv/setup.c +++ b/xen/arch/riscv/setup.c @@ -40,6 +40,19 @@ void __init noreturn start_xen(unsigned long bootcpu_id, { remove_identity_mapping(); + /* + * tp register contains an address of physical cpu information. + * So write physical CPU info of boot cpu to tp register + * It will be used later by get_processor_id() to get process_id ( look at + * <asm/processor.h> ): + * #define get_processor_id() (tp->processor_id) + */ + asm volatile ("mv tp, %0" : : "r"((unsigned long)&pcpu_info[0])); + + set_processor_id(0); + + smp_setup_processor_id(bootcpu_id); + trap_init(); #ifdef CONFIG_SELF_TESTS diff --git a/xen/arch/riscv/smp.c b/xen/arch/riscv/smp.c new file mode 100644 index 0000000000..006a062ad7 --- /dev/null +++ b/xen/arch/riscv/smp.c @@ -0,0 +1,4 @@ +#include <xen/smp.h> + +/* tp points to one of these per cpu */ +struct pcpu_info pcpu_info[NR_CPUS]; \ No newline at end of file diff --git a/xen/arch/riscv/smpboot.c b/xen/arch/riscv/smpboot.c new file mode 100644 index 0000000000..a90401ffd4 --- /dev/null +++ b/xen/arch/riscv/smpboot.c @@ -0,0 +1,12 @@ +#include <xen/init.h> +#include <xen/sections.h> +#include <xen/smp.h> + +unsigned long __cpuid_to_hartid_map[NR_CPUS] __ro_after_init = { + [0 ... NR_CPUS-1] = INVALID_HARTID +}; + +void __init smp_setup_processor_id(unsigned long boot_cpu_hartid) +{ + cpuid_to_hartid_map(0) = boot_cpu_hartid; +}
struct pcpu_info is introduced to store pcpu related information. At the moment, it is only processor_id but in the fututre it will be guest cpu information and some temporary variables which will be used during save/restore of vcpu registers. set_processor_id() and get_processor_id() are introduced to set and get processor id which is stored in pcpu_info. __cpuid_to_hartid_map[NR_CPUS] is introduced to store the mapping between Xen logical CPU and hartid ( physical CPU id ) and auxiliary macros cpuid_to_hartid_map() for convience access to __cpuid_to_hartid_map[]. smp_processor_id() is defined properly as it is enough to information to define it now instead of seting it to "dummy" 0. Also, tp registers is initialized to point to pcpu_info[0]; set_processor_id is set to 0 as Xen is running on logical cpu 0 and save physical CPU id for current logical CPU id in __cpuid_to_hartid_map[]. Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com> --- Changes in V3: - new patch. --- xen/arch/riscv/Makefile | 2 ++ xen/arch/riscv/include/asm/processor.h | 35 ++++++++++++++++++++++++-- xen/arch/riscv/include/asm/smp.h | 10 ++++++++ xen/arch/riscv/setup.c | 13 ++++++++++ xen/arch/riscv/smp.c | 4 +++ xen/arch/riscv/smpboot.c | 12 +++++++++ 6 files changed, 74 insertions(+), 2 deletions(-) create mode 100644 xen/arch/riscv/smp.c create mode 100644 xen/arch/riscv/smpboot.c