| Message ID | 03a703996ae7300a9eda54283711b19c42a7d116.1724256027.git.oleksii.kurochko@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | RISCV device tree mapping | expand |
On 21.08.2024 18:06, Oleksii Kurochko wrote: > Introduce struct pcpu_info to store pCPU-related information. > Initially, it includes only processor_id and hart id, but it > will be extended to include guest CPU information and > temporary variables for saving/restoring vCPU registers. > > Add set_processor_id() and get_processor_id() functions to set > and retrieve the processor_id stored in pcpu_info. > > Introduce cpuid_to_hartid_map() to convert Xen logical CPUs to > hart IDs (physical CPU IDs). There's no function of that name anymore. > --- > Changes in V5: > - add hart_id to pcpu_info; > - add comments to pcpu_info members. > - define INVALID_HARTID as ULONG_MAX as mhart_id register has MXLEN which is > equal to 32 for RV-32 and 64 for RV-64. > - add hart_id to pcpu_info structure. > - drop cpuid_to_hartid_map[] and use pcpu_info[] for the same purpose. > - introduce new function setup_tp(cpuid). > - add the FIXME commit on top of pcpu_info[]. Once again "comment" here? And that despite ... > - setup TP register before start_xen() being called. > - update the commit message. > - change "commit message" to "comment" in "Changes in V4" in "update the comment > above the code of TP..." ... this? > --- a/xen/arch/riscv/include/asm/processor.h > +++ b/xen/arch/riscv/include/asm/processor.h > @@ -12,8 +12,33 @@ > > #ifndef __ASSEMBLY__ > > -/* TODO: need to be implemeted */ > -#define smp_processor_id() 0 > +#include <xen/bug.h> > + > +register struct pcpu_info *tp asm ("tp"); Nit: Strictly speaking there need to be blanks inside the parentheses. But maybe an exception for a register variable name declaration is okay. > +struct pcpu_info { > + unsigned int processor_id; /* Xen CPU id */ > + unsigned long hart_id; /* physical CPU 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(); Nit: This can easily be the initializer of the variable. > + BUG_ON(id > NR_CPUS); I'm pretty sure I pointed out before that this is off by 1: NR_CPUS itself is invalid, too. > --- a/xen/arch/riscv/include/asm/smp.h > +++ b/xen/arch/riscv/include/asm/smp.h > @@ -5,6 +5,10 @@ > #include <xen/cpumask.h> > #include <xen/percpu.h> > > +#include <asm/processor.h> > + > +#define INVALID_HARTID ULONG_MAX So what if the firmware report this value for one of the harts? > @@ -14,6 +18,13 @@ DECLARE_PER_CPU(cpumask_var_t, cpu_core_mask); > */ > #define park_offline_cpus false > > +void smp_set_bootcpu_id(unsigned long boot_cpu_hartid); > + > +/* > + * Mapping between linux logical cpu index and hartid. > + */ > +#define cpuid_to_hartid(cpu) pcpu_info[cpu].hart_id If I'm not mistaken Misra demands parentheses around the expression even in cases like this one (where at the use sites you can't really do anything [that makes at least some sense] to break what the macro is supposed to do). > --- a/xen/arch/riscv/riscv64/head.S > +++ b/xen/arch/riscv/riscv64/head.S > @@ -55,6 +55,10 @@ FUNC(start) > */ > jal reset_stack > > + /* Xen's boot cpu id is equal to 0 so setup TP register for it */ > + mv a0, x0 > + jal setup_tp I'm not going to insist, but for the casual reader "li a0, 0" may be more obvious as to what it does, and if I'm not mistaken that actually expands to the same underlying insn as the "mv" you use. > --- a/xen/arch/riscv/setup.c > +++ b/xen/arch/riscv/setup.c > @@ -8,6 +8,7 @@ > #include <public/version.h> > > #include <asm/early_printk.h> > +#include <asm/smp.h> > #include <asm/traps.h> > > void arch_get_xen_caps(xen_capabilities_info_t *info) > @@ -40,6 +41,10 @@ void __init noreturn start_xen(unsigned long bootcpu_id, > { > remove_identity_mapping(); > > + set_processor_id(0); This isn't really needed, is it? The pcpu_info[] initializer already installs the necessary 0. Another thing would be if the initializer set the field to, say, NR_CPUS. > --- /dev/null > +++ b/xen/arch/riscv/smp.c > @@ -0,0 +1,21 @@ > +#include <xen/smp.h> > + > +/* > + * FIXME: make pcpu_info[] dynamically allocated when necessary > + * functionality will be ready > + */ > +/* tp points to one of these per cpu */ > +struct pcpu_info pcpu_info[NR_CPUS] = { { 0, INVALID_HARTID } }; As to the initializer - what about CPUs other than CPU0? Would they better all have hart_id set to invalid? Also, as a pretty strong suggestion to avoid excessive churn going forward: Please consider using dedicated initializers here. IOW perhaps struct pcpu_info pcpu_info[NR_CPUS] = { [0 ... NR_CPUS - 1] = { .hart_id = INVALID_HARTID, }}; Yet as said earlier - in addition you likely want to make sure no two CPUs have (part of) their struct instance in the same cache line. That won't matter right now, as you have no fields you alter at runtime, but I expect such fields will appear. > +void setup_tp(unsigned int cpuid) > +{ > + /* > + * tp register contains an address of physical cpu information. > + * So write physical CPU info of cpuid to tp register. > + * It will be used later by get_processor_id() ( look at > + * <asm/processor.h> ): > + * #define get_processor_id() (tp->processor_id) > + */ > + asm volatile ( "mv tp, %0" > + :: "r" ((unsigned long)&pcpu_info[cpuid]) : "memory" ); > +} So you've opted to still do this in C. Which means there's still a residual risk of the compiler assuming it can already to tp. What's the problem with doing this properly in assembly? As to the memory clobber - in an isolated, non-inline function its significance is reduced mostly to the case of LTO (which I'm not sure you even target). Nevertheless probably worth keeping, even if mainly for documentation purposes. Provided of course this C function is to remain. > --- /dev/null > +++ b/xen/arch/riscv/smpboot.c > @@ -0,0 +1,8 @@ > +#include <xen/init.h> > +#include <xen/sections.h> > +#include <xen/smp.h> > + > +void __init smp_set_bootcpu_id(unsigned long boot_cpu_hartid) > +{ > + cpuid_to_hartid(0) = boot_cpu_hartid; > +} Does this really need its own function? Jan
On Tue, 2024-08-27 at 15:44 +0200, Jan Beulich wrote: > On 21.08.2024 18:06, Oleksii Kurochko wrote: > > > --- a/xen/arch/riscv/include/asm/smp.h > > +++ b/xen/arch/riscv/include/asm/smp.h > > @@ -5,6 +5,10 @@ > > #include <xen/cpumask.h> > > #include <xen/percpu.h> > > > > +#include <asm/processor.h> > > + > > +#define INVALID_HARTID ULONG_MAX > > So what if the firmware report this value for one of the harts? It could be an issue, but in my opinion, there is a small chance that the firmware will use such a high number. I can add a BUG_ON() in start_xen() to check that bootcpu_id is not equal to INVALID_HARTID to ensure that the firmware does not report this value. Otherwise, we would need to add a 'bool valid;' to struct pcpu_info and use it instead of INVALID_HARTID. > > --- a/xen/arch/riscv/setup.c > > +++ b/xen/arch/riscv/setup.c > > @@ -8,6 +8,7 @@ > > #include <public/version.h> > > > > #include <asm/early_printk.h> > > +#include <asm/smp.h> > > #include <asm/traps.h> > > > > void arch_get_xen_caps(xen_capabilities_info_t *info) > > @@ -40,6 +41,10 @@ void __init noreturn start_xen(unsigned long > > bootcpu_id, > > { > > remove_identity_mapping(); > > > > + set_processor_id(0); > > This isn't really needed, is it? The pcpu_info[] initializer already > installs the necessary 0. Another thing would be if the initializer > set the field to, say, NR_CPUS. > > > --- /dev/null > > +++ b/xen/arch/riscv/smp.c > > @@ -0,0 +1,21 @@ > > +#include <xen/smp.h> > > + > > +/* > > + * FIXME: make pcpu_info[] dynamically allocated when necessary > > + * functionality will be ready > > + */ > > +/* tp points to one of these per cpu */ > > +struct pcpu_info pcpu_info[NR_CPUS] = { { 0, INVALID_HARTID } }; > > As to the initializer - what about CPUs other than CPU0? Would they > better all have hart_id set to invalid? I thought about that, but I decided that if we have INVALID_HARTID as hart_id and the hart_id is checked in the appropriate places, then it doesn't really matter what the processor_id member of struct pcpu_info is. For clarity, it might be better to set it to an invalid value, but it doesn't clear which value we should choose as invalid. I assume that NR_CPUS is a good candidate for that? > > Also, as a pretty strong suggestion to avoid excessive churn going > forward: Please consider using dedicated initializers here. IOW > perhaps > > struct pcpu_info pcpu_info[NR_CPUS] = { [0 ... NR_CPUS - 1] = { > .hart_id = INVALID_HARTID, > }}; > > Yet as said earlier - in addition you likely want to make sure no > two CPUs have (part of) their struct instance in the same cache line. > That won't matter right now, as you have no fields you alter at > runtime, but I expect such fields will appear. Is my understanding correct that adding __cacheline_aligned will be sufficient: struct pcpu_info { ... } __cacheline_aligned; > > > +void setup_tp(unsigned int cpuid) > > +{ > > + /* > > + * tp register contains an address of physical cpu > > information. > > + * So write physical CPU info of cpuid to tp register. > > + * It will be used later by get_processor_id() ( look at > > + * <asm/processor.h> ): > > + * #define get_processor_id() (tp->processor_id) > > + */ > > + asm volatile ( "mv tp, %0" > > + :: "r" ((unsigned long)&pcpu_info[cpuid]) : > > "memory" ); > > +} > > So you've opted to still do this in C. Which means there's still a > residual risk of the compiler assuming it can already to tp. What's > the problem with doing this properly in assembly? There is no problem and to be on the safe side I will re-write it to assembly. > > As to the memory clobber - in an isolated, non-inline function its > significance is reduced mostly to the case of LTO (which I'm not > sure you even target). Nevertheless probably worth keeping, even if > mainly for documentation purposes. Provided of course this C function > is to remain. > > > --- /dev/null > > +++ b/xen/arch/riscv/smpboot.c > > @@ -0,0 +1,8 @@ > > +#include <xen/init.h> > > +#include <xen/sections.h> > > +#include <xen/smp.h> > > + > > +void __init smp_set_bootcpu_id(unsigned long boot_cpu_hartid) > > +{ > > + cpuid_to_hartid(0) = boot_cpu_hartid; > > +} > > Does this really need its own function? No, there is no such need. I will drop it. Thanks. ~ Oleksii
On 28.08.2024 12:56, oleksii.kurochko@gmail.com wrote: > On Tue, 2024-08-27 at 15:44 +0200, Jan Beulich wrote: >> On 21.08.2024 18:06, Oleksii Kurochko wrote: >>> --- a/xen/arch/riscv/include/asm/smp.h >>> +++ b/xen/arch/riscv/include/asm/smp.h >>> @@ -5,6 +5,10 @@ >>> #include <xen/cpumask.h> >>> #include <xen/percpu.h> >>> >>> +#include <asm/processor.h> >>> + >>> +#define INVALID_HARTID ULONG_MAX >> >> So what if the firmware report this value for one of the harts? > It could be an issue, but in my opinion, there is a small chance that > the firmware will use such a high number. I can add a BUG_ON() in > start_xen() to check that bootcpu_id is not equal to INVALID_HARTID to > ensure that the firmware does not report this value. Otherwise, we > would need to add a 'bool valid;' to struct pcpu_info and use it > instead of INVALID_HARTID. Which route to go largely depends on expectations to actual hardware we're intending Xen to be usable on. >>> --- a/xen/arch/riscv/setup.c >>> +++ b/xen/arch/riscv/setup.c >>> @@ -8,6 +8,7 @@ >>> #include <public/version.h> >>> >>> #include <asm/early_printk.h> >>> +#include <asm/smp.h> >>> #include <asm/traps.h> >>> >>> void arch_get_xen_caps(xen_capabilities_info_t *info) >>> @@ -40,6 +41,10 @@ void __init noreturn start_xen(unsigned long >>> bootcpu_id, >>> { >>> remove_identity_mapping(); >>> >>> + set_processor_id(0); >> >> This isn't really needed, is it? The pcpu_info[] initializer already >> installs the necessary 0. Another thing would be if the initializer >> set the field to, say, NR_CPUS. As suggested here, ... >>> --- /dev/null >>> +++ b/xen/arch/riscv/smp.c >>> @@ -0,0 +1,21 @@ >>> +#include <xen/smp.h> >>> + >>> +/* >>> + * FIXME: make pcpu_info[] dynamically allocated when necessary >>> + * functionality will be ready >>> + */ >>> +/* tp points to one of these per cpu */ >>> +struct pcpu_info pcpu_info[NR_CPUS] = { { 0, INVALID_HARTID } }; >> >> As to the initializer - what about CPUs other than CPU0? Would they >> better all have hart_id set to invalid? > I thought about that, but I decided that if we have INVALID_HARTID as > hart_id and the hart_id is checked in the appropriate places, then it > doesn't really matter what the processor_id member of struct pcpu_info > is. For clarity, it might be better to set it to an invalid value, but > it doesn't clear which value we should choose as invalid. I assume that > NR_CPUS is a good candidate for that? ... yes. With that you'd also avoid the need for a "valid" flag: An entry's hart ID would be valid (no matter which value) if its processor_id field is valid (less than NR_CPUS). >> Also, as a pretty strong suggestion to avoid excessive churn going >> forward: Please consider using dedicated initializers here. IOW >> perhaps >> >> struct pcpu_info pcpu_info[NR_CPUS] = { [0 ... NR_CPUS - 1] = { >> .hart_id = INVALID_HARTID, >> }}; >> >> Yet as said earlier - in addition you likely want to make sure no >> two CPUs have (part of) their struct instance in the same cache line. >> That won't matter right now, as you have no fields you alter at >> runtime, but I expect such fields will appear. > Is my understanding correct that adding __cacheline_aligned will be > sufficient: > struct pcpu_info { > ... > } __cacheline_aligned; Yes, that's what we do elsewhere. 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..98c45afb6c 100644 --- a/xen/arch/riscv/include/asm/processor.h +++ b/xen/arch/riscv/include/asm/processor.h @@ -12,8 +12,33 @@ #ifndef __ASSEMBLY__ -/* TODO: need to be implemeted */ -#define smp_processor_id() 0 +#include <xen/bug.h> + +register struct pcpu_info *tp asm ("tp"); + +struct pcpu_info { + unsigned int processor_id; /* Xen CPU id */ + unsigned long hart_id; /* physical CPU 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(); + + BUG_ON(id > NR_CPUS); + + 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..2b719616ee 100644 --- a/xen/arch/riscv/include/asm/smp.h +++ b/xen/arch/riscv/include/asm/smp.h @@ -5,6 +5,10 @@ #include <xen/cpumask.h> #include <xen/percpu.h> +#include <asm/processor.h> + +#define INVALID_HARTID ULONG_MAX + DECLARE_PER_CPU(cpumask_var_t, cpu_sibling_mask); DECLARE_PER_CPU(cpumask_var_t, cpu_core_mask); @@ -14,6 +18,13 @@ DECLARE_PER_CPU(cpumask_var_t, cpu_core_mask); */ #define park_offline_cpus false +void smp_set_bootcpu_id(unsigned long boot_cpu_hartid); + +/* + * Mapping between linux logical cpu index and hartid. + */ +#define cpuid_to_hartid(cpu) pcpu_info[cpu].hart_id + #endif /* diff --git a/xen/arch/riscv/riscv64/head.S b/xen/arch/riscv/riscv64/head.S index 3261e9fce8..9e5b9a0708 100644 --- a/xen/arch/riscv/riscv64/head.S +++ b/xen/arch/riscv/riscv64/head.S @@ -55,6 +55,10 @@ FUNC(start) */ jal reset_stack + /* Xen's boot cpu id is equal to 0 so setup TP register for it */ + mv a0, x0 + jal setup_tp + /* restore hart_id ( bootcpu_id ) and dtb address */ mv a0, s0 mv a1, s1 diff --git a/xen/arch/riscv/setup.c b/xen/arch/riscv/setup.c index 13f0e8c77d..e15f34509c 100644 --- a/xen/arch/riscv/setup.c +++ b/xen/arch/riscv/setup.c @@ -8,6 +8,7 @@ #include <public/version.h> #include <asm/early_printk.h> +#include <asm/smp.h> #include <asm/traps.h> void arch_get_xen_caps(xen_capabilities_info_t *info) @@ -40,6 +41,10 @@ void __init noreturn start_xen(unsigned long bootcpu_id, { remove_identity_mapping(); + set_processor_id(0); + + smp_set_bootcpu_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..478ea5aeab --- /dev/null +++ b/xen/arch/riscv/smp.c @@ -0,0 +1,21 @@ +#include <xen/smp.h> + +/* + * FIXME: make pcpu_info[] dynamically allocated when necessary + * functionality will be ready + */ +/* tp points to one of these per cpu */ +struct pcpu_info pcpu_info[NR_CPUS] = { { 0, INVALID_HARTID } }; + +void setup_tp(unsigned int cpuid) +{ + /* + * tp register contains an address of physical cpu information. + * So write physical CPU info of cpuid to tp register. + * It will be used later by get_processor_id() ( look at + * <asm/processor.h> ): + * #define get_processor_id() (tp->processor_id) + */ + asm volatile ( "mv tp, %0" + :: "r" ((unsigned long)&pcpu_info[cpuid]) : "memory" ); +} diff --git a/xen/arch/riscv/smpboot.c b/xen/arch/riscv/smpboot.c new file mode 100644 index 0000000000..34319f8875 --- /dev/null +++ b/xen/arch/riscv/smpboot.c @@ -0,0 +1,8 @@ +#include <xen/init.h> +#include <xen/sections.h> +#include <xen/smp.h> + +void __init smp_set_bootcpu_id(unsigned long boot_cpu_hartid) +{ + cpuid_to_hartid(0) = boot_cpu_hartid; +}
Introduce struct pcpu_info to store pCPU-related information. Initially, it includes only processor_id and hart id, but it will be extended to include guest CPU information and temporary variables for saving/restoring vCPU registers. Add set_processor_id() and get_processor_id() functions to set and retrieve the processor_id stored in pcpu_info. Introduce cpuid_to_hartid_map() to convert Xen logical CPUs to hart IDs (physical CPU IDs). Define smp_processor_id() to provide accurate information, replacing the previous "dummy" value of 0. Initialize tp registers to point to pcpu_info[0]. Set processor_id to 0 for logical CPU 0 and store the physical CPU ID in pcpu_info[0]. Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com> --- Changes in V5: - add hart_id to pcpu_info; - add comments to pcpu_info members. - define INVALID_HARTID as ULONG_MAX as mhart_id register has MXLEN which is equal to 32 for RV-32 and 64 for RV-64. - add hart_id to pcpu_info structure. - drop cpuid_to_hartid_map[] and use pcpu_info[] for the same purpose. - introduce new function setup_tp(cpuid). - add the FIXME commit on top of pcpu_info[]. - setup TP register before start_xen() being called. - update the commit message. - change "commit message" to "comment" in "Changes in V4" in "update the comment above the code of TP..." --- Changes in V4: - wrap id with () inside set_processor_id(). - code style fixes - update BUG_ON(id > NR_CPUS) in smp_processor_id() and drop the comment above BUG_ON(). - s/__cpuid_to_hartid_map/cpuid_to_hartid_map - s/cpuid_to_hartid_map/cpuid_to_harti ( here cpuid_to_hartid_map is the name of the macros ). - update the comment above the code of TP register initialization in start_xen(). - s/smp_setup_processor_id/smp_setup_bootcpu_id - update the commit message. - cleanup headers which are included in <asm/processor.h> --- Changes in V3: - new patch. --- xen/arch/riscv/Makefile | 2 ++ xen/arch/riscv/include/asm/processor.h | 29 ++++++++++++++++++++++++-- xen/arch/riscv/include/asm/smp.h | 11 ++++++++++ xen/arch/riscv/riscv64/head.S | 4 ++++ xen/arch/riscv/setup.c | 5 +++++ xen/arch/riscv/smp.c | 21 +++++++++++++++++++ xen/arch/riscv/smpboot.c | 8 +++++++ 7 files changed, 78 insertions(+), 2 deletions(-) create mode 100644 xen/arch/riscv/smp.c create mode 100644 xen/arch/riscv/smpboot.c