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

[oleksii.kurochko@gmail.com]

On Wed, 2024-09-11 at 14:14 +0200, Jan Beulich wrote:
> On 11.09.2024 14:05, oleksii.kurochko@gmail.com wrote:
> > On Tue, 2024-09-10 at 12:33 +0200, Jan Beulich wrote:
> > > On 02.09.2024 19:01, Oleksii Kurochko wrote:
> > > > @@ -72,6 +77,16 @@ FUNC(reset_stack)
> > > >          ret
> > > >  END(reset_stack)
> > > >  
> > > > +/* void setup_tp(unsigned int xen_cpuid); */
> > > > +FUNC(setup_tp)
> > > > +        la      tp, pcpu_info
> > > > +        li      t0, PCPU_INFO_SIZE
> > > > +        mul     t1, a0, t0
> > > > +        add     tp, tp, t1
> > > > +
> > > > +        ret
> > > > +END(setup_tp)
> > > 
> > > I take it this is going to run (i.e. also for secondary CPUs)
> > > ahead
> > > of
> > > Xen being able to handle any kind of exception (on the given
> > > CPU)?
> > Yes, I am using it for secondary CPUs and Xen are handling
> > exceptions (
> > on the given CPU ) fine.
> 
> Yet that wasn't my question. Note in particular the use of "ahead
> of".
The first executed function for secondary CPU will be
( https://gitlab.com/xen-project/people/olkur/xen/-/blob/latest/xen/arch/riscv/riscv64/head.S?ref_type=heads#L100
) where the first instruction mask all interrupts:
           /*
            * a0 -> started hart id
            * a1 -> private data passed by boot cpu
            */
   ENTRY(secondary_start_sbi)
           /* Mask all interrupts */
           csrw    CSR_SIE, zero
           ...
   	tail    smp_callin
   
Then at the start of smp_callin
( https://gitlab.com/xen-project/people/olkur/xen/-/blob/latest/xen/arch/riscv/smpboot.c?ref_type=heads#L258
) tp register is setup ( in the old way for now using inline assembly I
will switch to setup_tp() later a little bit and call it before 'tail
smp_callin' ) and only after that local irqs are enabled:
   void __init smp_callin(unsigned int cpuid)
   {
       unsigned int hcpu = 1;
   
       for ( ; (hcpu < NR_CPUS) && (cpuid_to_hartid_map(hcpu) != cpuid);
   hcpu++)
       {}
   
       asm volatile ("mv tp, %0" : : "r"((unsigned
   long)&pcpu_info[hcpu]));
   ...
      trap_init(); /* write handle_trap() address to CSR_STVEC */
   ...
      local_irq_enable();
   ...
   
> 
> > >  If
> > > so, all is fine here. If not, transiently pointing tp at CPU0's
> > > space
> > > is a possible problem.
> > I haven't had any problem with that at the moment.
> > 
> > Do you think that it will be better to use DECLARE_PER_CPU() with
> > updating of setup_tp() instead of pcpu_info[] when SMP will be
> > introduced?
> > What kind of problems should I take into account?
> 
> If exceptions can be handled by Xen already when entering this
> function,
> then the exception handler would need to be setting up tp for itself.
> If
> not, it would use whatever the interrupted context used (or what is
> brought into context by hardware while delivering the exception). If
> I
> assumed that tp in principle doesn't need setting up when handling
> exceptions (sorry, haven't read up enough yet about how guest -> host
> switches work for RISC-V), and if further exceptions can already be
> handled upon entering setup_tp(), then keeping tp properly invalid
> until
> it can be set to its correct value will make it easier to diagnose
> problems than when - like you do - transiently setting tp to CPU0's
> value (and hence risking corruption of its state).
Regarding tp in exception handler if it is an exception from Xen it
will be set to 0 ( it is done by switch CSR_SSCRATCH and tp, and
CSR_SSCRATCH is always 0 for Xen and for guest it will be set to
pcpu_info[cpuid] before returning to new
vcpu:https://gitlab.com/xen-project/people/olkur/xen/-/blob/latest/xen/arch/riscv/entry.S?ref_type=heads#L165
) at the start of the handler; otherwise if an exception from Guest it
will set to &pcpu_info[cpuid] which was stored in CSR_SSCRATCH:
https://gitlab.com/xen-project/people/olkur/xen/-/blob/latest/xen/arch/riscv/entry.S?ref_type=heads#L15

As I mentioned above, interrupts will be disabled until tp is set. Even
if they aren’t disabled, tp will be set to 0 because, at the moment the
secondary CPU boots, CSR_SSCRATCH will be 0, which indicates that the
interrupt is from Xen.

> - like you do - transiently setting tp to CPU0's value (and hence >
risking corruption of its state).
I think I’m missing something—why would the secondary CPU have the same
value as CPU0? If we don’t set up the tp register when the secondary
CPU boots, it will contain a default value, which is expected upon
boot. It will retain this value until setup_tp() is called, which will
then set tp to pcpu_info[SECONDARY_CPU_ID].

~ Oleksii