Re: [PATCH v3 6/9] xen/riscv: introduce functionality to work with cpu info

[Jan Beulich <jbeulich@xxxxxxxx>]

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.