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.
