On Tue, 2024-08-13 at 10:54 +0200, Jan Beulich wrote:
> On 09.08.2024 18:19, Oleksii Kurochko wrote:
> > Introduce struct pcpu_info to store pCPU-related information.
> > Initially, it includes only processor_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[NR_CPUS] to map Xen logical CPUs to
> > hart IDs (physical CPU IDs). Add auxiliary macros cpuid_to_hartid()
> > for convenient access to this mapping.
> >
> > 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
> > for the current logical CPU in cpuid_to_hartid_map[].
> >
> > Signed-off-by: Oleksii Kurochko <oleksii.kuroc...@gmail.com>
> > ---
> > 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 commit message above the code of TP register
> > initialization in
> > start_xen().
>
> I guess that's once again "comment", not "commit message"?
Yes, sorry for confusion. It should be comment.
>
> > --- a/xen/arch/riscv/include/asm/processor.h
> > +++ b/xen/arch/riscv/include/asm/processor.h
> > @@ -12,8 +12,32 @@
> >
> > #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;
> > +};
> > +
> > +/* 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();
> > +
> > + BUG_ON(id > NR_CPUS);
>
> > = ?
>
> > --- 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
> > +
> > DECLARE_PER_CPU(cpumask_var_t, cpu_sibling_mask);
> > DECLARE_PER_CPU(cpumask_var_t, cpu_core_mask);
> >
> > @@ -14,6 +16,14 @@ 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.
> > + */
> > +extern unsigned long cpuid_to_hartid_map[NR_CPUS];
> > +#define cpuid_to_hartid(cpu) cpuid_to_hartid_map[cpu]
>
> How wide can hart IDs be? Wider than 32 bits? If not, why unsigned
> long?
According to the spec:
```
The mhartid CSR is an MXLEN-bit read-only register containing the
integer ID of the hardware thread running the code
```
where MXLEN can bit 32 and 64.
> If so, what about RV32 (which generally you at least try cover where
> easily possible)?
On RV32 MXLEN will be 32 and unsigned long will be 32-bit too.
>
> Is there a reason this needs to be a separate array, rather than
> being
> part of struct pcpu_info?
>
> > @@ -40,6 +41,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() ( look at
> > + * <asm/processor.h> ):
> > + * #define get_processor_id() (tp->processor_id)
> > + */
> > + asm volatile ( "mv tp, %0" : : "r"((unsigned
> > long)&pcpu_info[0]) );
>
> Perhaps be on the safe side and also add a memory barrier?
Do you mean compiler barrier? ( asm volatile ( "..." :: ... : "memory"
)? )
>
> Perhaps be on the safe side and do this absolutely first in the
> function,
> or even in assembly (such that initializers of future variables
> declared
> at the top of the function end up safe, too)?
I am not sure that I am following your part related to put this code in
assembler ( instructions in assembly code still code be re-ordered what
can affect a time when tp will be set with correct value ) and what do
you mean by "initializers of future variables declared at the top of
the function end up safe"
>
> Also nit: Blank please between closing quote and opening parenthesis.
> (Otoh you could omit the blank between the two colons.)
>
> > --- /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];
>
> And they all need setting up statically? Is there a plan to make this
> dynamic (which could be recorded in a "fixme" in the comment)?
I didn't plan to make allocation of this array dynamic. I don't expect
that NR_CPUS will be big. I can add "fixme" but I am not really
understand what will be advantages if pcpu_info[] will be allocated
dynamically.
~ Oleksii
