Re: [PATCH v5 4/7] xen/riscv: introduce functionality to work with CPU info

[Jan Beulich <jbeulich@xxxxxxxx>]

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