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

[oleksii.kurochko@xxxxxxxxx]

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