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

[oleksii.kurochko@xxxxxxxxx]

On Tue, 2024-08-27 at 15:44 +0200, Jan Beulich wrote:
> On 21.08.2024 18:06, Oleksii Kurochko wrote:

> 
> > --- 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?
It could be an issue, but in my opinion, there is a small chance that
the firmware will use such a high number. I can add a BUG_ON() in
start_xen() to check that bootcpu_id is not equal to INVALID_HARTID to
ensure that the firmware does not report this value. Otherwise, we
would need to add a 'bool valid;' to struct pcpu_info and use it
instead of INVALID_HARTID.

> > --- 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?
I thought about that, but I decided that if we have INVALID_HARTID as
hart_id and the hart_id is checked in the appropriate places, then it
doesn't really matter what the processor_id member of struct pcpu_info
is. For clarity, it might be better to set it to an invalid value, but
it doesn't clear which value we should choose as invalid. I assume that
NR_CPUS is a good candidate for that?

> 
> 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.
Is my understanding correct that adding __cacheline_aligned will be
sufficient:
   struct pcpu_info {
   ...
   } __cacheline_aligned;


> 
> > +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?
There is no problem and to be on the safe side I will re-write it to
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?
No, there is no such need. I will drop it.

Thanks.

~ Oleksii