| Message ID | 77a00222008f8b41d2454e74d1c9247252d7ccd9.1725295716.git.oleksii.kurochko@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | RISCV device tree mapping | expand |
On 02.09.2024 19:01, Oleksii Kurochko wrote: > --- a/xen/arch/riscv/include/asm/processor.h > +++ b/xen/arch/riscv/include/asm/processor.h > @@ -12,8 +12,31 @@ > > #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 */ > +} __cacheline_aligned; Shouldn't you include xen/cache.h for this, to be sure the header can be included on its own? I'm also unconvinced of this placement: Both Arm and x86 have similar structures (afaict), living in current.h. > +/* tp points to one of these */ > +extern struct pcpu_info pcpu_info[NR_CPUS]; > + > +#define get_processor_id() (tp->processor_id) Iirc it was in response to one of your earlier patches that we removed get_processor_id() from the other architectures, as being fully redundant with smp_processor_id(). Is there a particular reason you re-introduce that now for RISC-V? > +#define set_processor_id(id) do { \ > + tp->processor_id = (id); \ > +} while (0) > + > +static inline unsigned int smp_processor_id(void) > +{ > + unsigned int id = get_processor_id(); > + > + BUG_ON(id > (NR_CPUS - 1)); The more conventional way of expressing this is >= NR_CPUS. > @@ -14,6 +16,13 @@ DECLARE_PER_CPU(cpumask_var_t, cpu_core_mask); > */ > #define park_offline_cpus false > > +/* > + * Mapping between linux logical cpu index and hartid. > + */ > +#define cpuid_to_hartid(cpu) (pcpu_info[cpu].hart_id) Does this need to be a macro (rather than an inline function)? > @@ -72,6 +77,16 @@ FUNC(reset_stack) > ret > END(reset_stack) > > +/* void setup_tp(unsigned int xen_cpuid); */ > +FUNC(setup_tp) > + la tp, pcpu_info > + li t0, PCPU_INFO_SIZE > + mul t1, a0, t0 > + add tp, tp, t1 > + > + ret > +END(setup_tp) I take it this is going to run (i.e. also for secondary CPUs) ahead of Xen being able to handle any kind of exception (on the given CPU)? If so, all is fine here. If not, transiently pointing tp at CPU0's space is a possible problem. Jan
On Tue, 2024-09-10 at 12:33 +0200, Jan Beulich wrote: > On 02.09.2024 19:01, Oleksii Kurochko wrote: > > --- a/xen/arch/riscv/include/asm/processor.h > > +++ b/xen/arch/riscv/include/asm/processor.h > > @@ -12,8 +12,31 @@ > > > > #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 */ > > +} __cacheline_aligned; > > Shouldn't you include xen/cache.h for this, to be sure the header can > be included on its own? Agree, it would be better to include xen/cache.h header. > > I'm also unconvinced of this placement: Both Arm and x86 have similar > structures (afaict), living in current.h. Then for consistency it would be better to move this structure to current.h for RISC-V. > > > +/* tp points to one of these */ > > +extern struct pcpu_info pcpu_info[NR_CPUS]; > > + > > +#define get_processor_id() (tp->processor_id) > > Iirc it was in response to one of your earlier patches that we > removed > get_processor_id() from the other architectures, as being fully > redundant with smp_processor_id(). Is there a particular reason you > re-introduce that now for RISC-V? No reason, just forgot that we agreed to use only smp_processor_id() and made a bad rebase of my 'latest' branch on top of the current staging which doesn't tell me about merge conflict in that place. I will drop get_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 = get_processor_id(); > > + > > + BUG_ON(id > (NR_CPUS - 1)); > > The more conventional way of expressing this is >= NR_CPUS. > > > @@ -14,6 +16,13 @@ DECLARE_PER_CPU(cpumask_var_t, cpu_core_mask); > > */ > > #define park_offline_cpus false > > > > +/* > > + * Mapping between linux logical cpu index and hartid. > > + */ > > +#define cpuid_to_hartid(cpu) (pcpu_info[cpu].hart_id) > > Does this need to be a macro (rather than an inline function)? No, there is no such need. I will use inline function instead. > > > @@ -72,6 +77,16 @@ FUNC(reset_stack) > > ret > > END(reset_stack) > > > > +/* void setup_tp(unsigned int xen_cpuid); */ > > +FUNC(setup_tp) > > + la tp, pcpu_info > > + li t0, PCPU_INFO_SIZE > > + mul t1, a0, t0 > > + add tp, tp, t1 > > + > > + ret > > +END(setup_tp) > > I take it this is going to run (i.e. also for secondary CPUs) ahead > of > Xen being able to handle any kind of exception (on the given CPU)? Yes, I am using it for secondary CPUs and Xen are handling exceptions ( on the given CPU ) fine. > If > so, all is fine here. If not, transiently pointing tp at CPU0's space > is a possible problem. I haven't had any problem with that at the moment. Do you think that it will be better to use DECLARE_PER_CPU() with updating of setup_tp() instead of pcpu_info[] when SMP will be introduced? What kind of problems should I take into account? ~ Oleksii
On 11.09.2024 14:05, oleksii.kurochko@gmail.com wrote: > On Tue, 2024-09-10 at 12:33 +0200, Jan Beulich wrote: >> On 02.09.2024 19:01, Oleksii Kurochko wrote: >>> @@ -72,6 +77,16 @@ FUNC(reset_stack) >>> ret >>> END(reset_stack) >>> >>> +/* void setup_tp(unsigned int xen_cpuid); */ >>> +FUNC(setup_tp) >>> + la tp, pcpu_info >>> + li t0, PCPU_INFO_SIZE >>> + mul t1, a0, t0 >>> + add tp, tp, t1 >>> + >>> + ret >>> +END(setup_tp) >> >> I take it this is going to run (i.e. also for secondary CPUs) ahead >> of >> Xen being able to handle any kind of exception (on the given CPU)? > Yes, I am using it for secondary CPUs and Xen are handling exceptions ( > on the given CPU ) fine. Yet that wasn't my question. Note in particular the use of "ahead of". >> If >> so, all is fine here. If not, transiently pointing tp at CPU0's space >> is a possible problem. > I haven't had any problem with that at the moment. > > Do you think that it will be better to use DECLARE_PER_CPU() with > updating of setup_tp() instead of pcpu_info[] when SMP will be > introduced? > What kind of problems should I take into account? If exceptions can be handled by Xen already when entering this function, then the exception handler would need to be setting up tp for itself. If not, it would use whatever the interrupted context used (or what is brought into context by hardware while delivering the exception). If I assumed that tp in principle doesn't need setting up when handling exceptions (sorry, haven't read up enough yet about how guest -> host switches work for RISC-V), and if further exceptions can already be handled upon entering setup_tp(), then keeping tp properly invalid until it can be set to its correct value will make it easier to diagnose problems than when - like you do - transiently setting tp to CPU0's value (and hence risking corruption of its state). Jan
On Wed, 2024-09-11 at 14:14 +0200, Jan Beulich wrote: > On 11.09.2024 14:05, oleksii.kurochko@gmail.com wrote: > > On Tue, 2024-09-10 at 12:33 +0200, Jan Beulich wrote: > > > On 02.09.2024 19:01, Oleksii Kurochko wrote: > > > > @@ -72,6 +77,16 @@ FUNC(reset_stack) > > > > ret > > > > END(reset_stack) > > > > > > > > +/* void setup_tp(unsigned int xen_cpuid); */ > > > > +FUNC(setup_tp) > > > > + la tp, pcpu_info > > > > + li t0, PCPU_INFO_SIZE > > > > + mul t1, a0, t0 > > > > + add tp, tp, t1 > > > > + > > > > + ret > > > > +END(setup_tp) > > > > > > I take it this is going to run (i.e. also for secondary CPUs) > > > ahead > > > of > > > Xen being able to handle any kind of exception (on the given > > > CPU)? > > Yes, I am using it for secondary CPUs and Xen are handling > > exceptions ( > > on the given CPU ) fine. > > Yet that wasn't my question. Note in particular the use of "ahead > of". The first executed function for secondary CPU will be ( https://gitlab.com/xen-project/people/olkur/xen/-/blob/latest/xen/arch/riscv/riscv64/head.S?ref_type=heads#L100 ) where the first instruction mask all interrupts: /* * a0 -> started hart id * a1 -> private data passed by boot cpu */ ENTRY(secondary_start_sbi) /* Mask all interrupts */ csrw CSR_SIE, zero ... tail smp_callin Then at the start of smp_callin ( https://gitlab.com/xen-project/people/olkur/xen/-/blob/latest/xen/arch/riscv/smpboot.c?ref_type=heads#L258 ) tp register is setup ( in the old way for now using inline assembly I will switch to setup_tp() later a little bit and call it before 'tail smp_callin' ) and only after that local irqs are enabled: void __init smp_callin(unsigned int cpuid) { unsigned int hcpu = 1; for ( ; (hcpu < NR_CPUS) && (cpuid_to_hartid_map(hcpu) != cpuid); hcpu++) {} asm volatile ("mv tp, %0" : : "r"((unsigned long)&pcpu_info[hcpu])); ... trap_init(); /* write handle_trap() address to CSR_STVEC */ ... local_irq_enable(); ... > > > > If > > > so, all is fine here. If not, transiently pointing tp at CPU0's > > > space > > > is a possible problem. > > I haven't had any problem with that at the moment. > > > > Do you think that it will be better to use DECLARE_PER_CPU() with > > updating of setup_tp() instead of pcpu_info[] when SMP will be > > introduced? > > What kind of problems should I take into account? > > If exceptions can be handled by Xen already when entering this > function, > then the exception handler would need to be setting up tp for itself. > If > not, it would use whatever the interrupted context used (or what is > brought into context by hardware while delivering the exception). If > I > assumed that tp in principle doesn't need setting up when handling > exceptions (sorry, haven't read up enough yet about how guest -> host > switches work for RISC-V), and if further exceptions can already be > handled upon entering setup_tp(), then keeping tp properly invalid > until > it can be set to its correct value will make it easier to diagnose > problems than when - like you do - transiently setting tp to CPU0's > value (and hence risking corruption of its state). Regarding tp in exception handler if it is an exception from Xen it will be set to 0 ( it is done by switch CSR_SSCRATCH and tp, and CSR_SSCRATCH is always 0 for Xen and for guest it will be set to pcpu_info[cpuid] before returning to new vcpu:https://gitlab.com/xen-project/people/olkur/xen/-/blob/latest/xen/arch/riscv/entry.S?ref_type=heads#L165 ) at the start of the handler; otherwise if an exception from Guest it will set to &pcpu_info[cpuid] which was stored in CSR_SSCRATCH: https://gitlab.com/xen-project/people/olkur/xen/-/blob/latest/xen/arch/riscv/entry.S?ref_type=heads#L15 As I mentioned above, interrupts will be disabled until tp is set. Even if they aren’t disabled, tp will be set to 0 because, at the moment the secondary CPU boots, CSR_SSCRATCH will be 0, which indicates that the interrupt is from Xen. > - like you do - transiently setting tp to CPU0's value (and hence > risking corruption of its state). I think I’m missing something—why would the secondary CPU have the same value as CPU0? If we don’t set up the tp register when the secondary CPU boots, it will contain a default value, which is expected upon boot. It will retain this value until setup_tp() is called, which will then set tp to pcpu_info[SECONDARY_CPU_ID]. ~ Oleksii
On 12.09.2024 11:27, oleksii.kurochko@gmail.com wrote: > As I mentioned above, interrupts will be disabled until tp is set. Okay, so all good then > Even > if they aren’t disabled, tp will be set to 0 because, at the moment the > secondary CPU boots, CSR_SSCRATCH will be 0, which indicates that the > interrupt is from Xen. > >> - like you do - transiently setting tp to CPU0's value (and hence > > risking corruption of its state). > I think I’m missing something—why would the secondary CPU have the same > value as CPU0? If we don’t set up the tp register when the secondary > CPU boots, it will contain a default value, which is expected upon > boot. It will retain this value until setup_tp() is called, which will > then set tp to pcpu_info[SECONDARY_CPU_ID]. Just to clarify (shouldn't matter in practice according to what you said above) - in FUNC(setup_tp) la tp, pcpu_info li t0, PCPU_INFO_SIZE mul t1, a0, t0 add tp, tp, t1 ret END(setup_tp) you start with setting tp to the CPU0 value. You only then adjust tp (3 insns later) to the designated value. If you wanted to play safe, you'd do it e.g. like this FUNC(setup_tp) la t0, pcpu_info li t1, PCPU_INFO_SIZE mul t1, a0, t1 add tp, t0, t1 ret END(setup_tp) Jan
On Tue, 2024-09-10 at 12:33 +0200, Jan Beulich wrote: > > +/* > > + * Mapping between linux logical cpu index and hartid. > > + */ > > +#define cpuid_to_hartid(cpu) (pcpu_info[cpu].hart_id) > > Does this need to be a macro (rather than an inline function)? I started to rework that and I am using this macros for both read and write. So it will be needed to introduce set and get inline functions instead of just one macros. I think I will stick to one macros instead of 2 inline functions. ~ Oleksii
On 12.09.2024 18:02, oleksii.kurochko@gmail.com wrote: > On Tue, 2024-09-10 at 12:33 +0200, Jan Beulich wrote: >>> +/* >>> + * Mapping between linux logical cpu index and hartid. >>> + */ >>> +#define cpuid_to_hartid(cpu) (pcpu_info[cpu].hart_id) >> >> Does this need to be a macro (rather than an inline function)? > I started to rework that and I am using this macros for both read > and write. So it will be needed to introduce set and get inline > functions instead of just one macros. I think I will stick to one > macros instead of 2 inline functions. You may want to consult with Andrew as to use of such a macro on the lhs of an assignment. I expect he'll ask to avoid such, and instead indeed go with both a get and a set accessor (unless it would make sense to simply open-code the few sets that there are going to be). Jan
diff --git a/xen/arch/riscv/Makefile b/xen/arch/riscv/Makefile index 81b77b13d6..2f2d6647a2 100644 --- a/xen/arch/riscv/Makefile +++ b/xen/arch/riscv/Makefile @@ -4,6 +4,7 @@ obj-y += mm.o obj-$(CONFIG_RISCV_64) += riscv64/ obj-y += sbi.o obj-y += setup.o +obj-y += smp.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..4799243863 100644 --- a/xen/arch/riscv/include/asm/processor.h +++ b/xen/arch/riscv/include/asm/processor.h @@ -12,8 +12,31 @@ #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 */ +} __cacheline_aligned; + +/* 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 = get_processor_id(); + + BUG_ON(id > (NR_CPUS - 1)); + + 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..11eee67d62 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> +#include <asm/processor.h> + DECLARE_PER_CPU(cpumask_var_t, cpu_sibling_mask); DECLARE_PER_CPU(cpumask_var_t, cpu_core_mask); @@ -14,6 +16,13 @@ DECLARE_PER_CPU(cpumask_var_t, cpu_core_mask); */ #define park_offline_cpus false +/* + * Mapping between linux logical cpu index and hartid. + */ +#define cpuid_to_hartid(cpu) (pcpu_info[cpu].hart_id) + +void setup_tp(unsigned int cpuid); + #endif /* diff --git a/xen/arch/riscv/riscv64/asm-offsets.c b/xen/arch/riscv/riscv64/asm-offsets.c index 9f663b9510..11400c4697 100644 --- a/xen/arch/riscv/riscv64/asm-offsets.c +++ b/xen/arch/riscv/riscv64/asm-offsets.c @@ -50,4 +50,6 @@ void asm_offsets(void) OFFSET(CPU_USER_REGS_SSTATUS, struct cpu_user_regs, sstatus); OFFSET(CPU_USER_REGS_PREGS, struct cpu_user_regs, pregs); BLANK(); + DEFINE(PCPU_INFO_SIZE, sizeof(struct pcpu_info)); + BLANK(); } diff --git a/xen/arch/riscv/riscv64/head.S b/xen/arch/riscv/riscv64/head.S index 3261e9fce8..c7d8bf18c5 100644 --- a/xen/arch/riscv/riscv64/head.S +++ b/xen/arch/riscv/riscv64/head.S @@ -1,4 +1,5 @@ #include <asm/asm.h> +#include <asm/asm-offsets.h> #include <asm/riscv_encoding.h> .section .text.header, "ax", %progbits @@ -55,6 +56,10 @@ FUNC(start) */ jal reset_stack + /* Xen's boot cpu id is equal to 0 so setup TP register for it */ + li a0, 0 + jal setup_tp + /* restore hart_id ( bootcpu_id ) and dtb address */ mv a0, s0 mv a1, s1 @@ -72,6 +77,16 @@ FUNC(reset_stack) ret END(reset_stack) +/* void setup_tp(unsigned int xen_cpuid); */ +FUNC(setup_tp) + la tp, pcpu_info + li t0, PCPU_INFO_SIZE + mul t1, a0, t0 + add tp, tp, t1 + + ret +END(setup_tp) + .section .text.ident, "ax", %progbits FUNC(turn_on_mmu) diff --git a/xen/arch/riscv/setup.c b/xen/arch/riscv/setup.c index 13f0e8c77d..540a3a608e 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); + + cpuid_to_hartid(0) = 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..4ca6a4e892 --- /dev/null +++ b/xen/arch/riscv/smp.c @@ -0,0 +1,15 @@ +#include <xen/smp.h> + +/* + * FIXME: make pcpu_info[] dynamically allocated when necessary + * functionality will be ready + */ +/* + * tp points to one of these per cpu. + * + * hart_id would be valid (no matter which value) if its + * processor_id field is valid (less than NR_CPUS). + */ +struct pcpu_info pcpu_info[NR_CPUS] = { [0 ... NR_CPUS - 1] = { + .processor_id = NR_CPUS, +}};
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. 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 V6: - update the commit message ( drop outdated information ). - s/FIXME commit/FIXME comment in "changes in V5". - code style fixes. - refactoring of smp_processor_id() and fix BUG_ON() condition inside it. - change "mv a0,x0" to "li a0, 0". - add __cacheline_aligned to the struct pcpu_info. - drop smp_set_bootcpu_id() and smpboot.c as it has only smp_set_bootcpu_id() defined at the moment. - re-write setup_tp() to assembler. --- 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 comment 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 | 1 + xen/arch/riscv/include/asm/processor.h | 27 ++++++++++++++++++++++++-- xen/arch/riscv/include/asm/smp.h | 9 +++++++++ xen/arch/riscv/riscv64/asm-offsets.c | 2 ++ xen/arch/riscv/riscv64/head.S | 15 ++++++++++++++ xen/arch/riscv/setup.c | 5 +++++ xen/arch/riscv/smp.c | 15 ++++++++++++++ 7 files changed, 72 insertions(+), 2 deletions(-) create mode 100644 xen/arch/riscv/smp.c