Re: [PATCH v4] xen/riscv: allow Xen to use SSTC while hiding it from guests

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 4/21/26 11:10 AM, Jan Beulich wrote:
> On 21.04.2026 11:01, Oleksii Kurochko wrote:
> > On 4/20/26 9:56 AM, Jan Beulich wrote:
> > > On 17.04.2026 09:24, Oleksii Kurochko wrote:
> > > > --- a/xen/arch/riscv/include/asm/csr.h
> > > > +++ b/xen/arch/riscv/include/asm/csr.h
> > > > @@ -32,6 +32,20 @@
> > > >                               : "memory" );                        \
> > > >    })
> > > >    
> > > > +#ifdef CONFIG_RISCV_32
> > > > +# define __csr_write32h(csr, val) csr_write(csr ## H, (val) >> 32)
> In my reply I followed this. If this compiled, then ...
>
> > > > +#else
> > > > +# define __csr_write32h(csr, val) ((void)(csr), (void)(val))
> > > In order to be able to spot issues in 64-bit builds, how about
> > >
> > > # define __csr_write32h(csr, val) ((void)csr ## H, (void)(val))
> > >
> > > ?
> ... aiui this would compile as well. Looks like the RV32 case then is in
> need of adjustment as well.
>
> > But this will cause a build issue in 64-bit builds.
> >
> > csr_write64(CSR_STIMECMP, ...)
> >     └─ __csr_write32h(csr, _v)   ← csr is NOT ##-adjacent here
> >                                     so preprocessor expands it FIRST
> >                                     CSR_STIMECMP → 0x14D
> >          └─ (void)csr ## H       ← csr is already 0x14D here
> >                                     0x14D ## H → 0x14DH  ERROR
> >
> > Probably, it would be better to do in the following way:
> >
> > #ifdef CONFIG_RISCV_32
> > #define csr_write64(csr, val)       \
> > ({                                  \
> >       uint64_t v_ = (val);            \
> >       csr_write(csr, v_);             \
> >       csr_write(csr ## H, v_ >> 32);  \
> > })
> > #else
> > #define csr_write64(csr, val)       \
> > ({                                  \
> >       uint64_t v_ = (val);            \
> >       csr_write(csr, v_);             \
> > })
> > #endif
> E.g. like this, albeit in the RV64 case the local v_ isn't needed. Instead,
> again to be able to spot issues in RV64 builds, (void)csr ## H may want
> adding.
>
> A clear downside to all of this is that this helper can only be used with
> CSR_* constants, not with runtime-calculated CSR numbers.
Yes, but it isn't critical downside as I don't see cases where it will 
be useful to have runtime-calculated CSR numbers.
> > > > @@ -279,8 +299,6 @@ static int cf_check sbi_set_timer_v01(uint64_t stime_value)
> > > >        return sbi_err_map_xen_errno(ret.error);
> > > >    }
> > > >    
> > > > -int (* __ro_after_init sbi_set_timer)(uint64_t stime_value) = sbi_set_timer_v01;
> > > > -
> > > >    int sbi_remote_sfence_vma(const cpumask_t *cpu_mask, vaddr_t start,
> > > >                              size_t size)
> > > >    {
> > > > @@ -360,10 +378,9 @@ int __init sbi_init(void)
> > > >            }
> > > >    
> > > >            if ( sbi_probe_extension(SBI_EXT_TIME) > 0 )
> > > > -        {
> > > > -            sbi_set_timer = sbi_set_timer_v02;
> > > > -            dprintk(XENLOG_INFO, "SBI v0.2 TIME extension detected\n");
> > > > -        }
> > > > +            set_xen_timer = sbi_set_timer_v02;
> > > > +        else
> > > > +            set_xen_timer = sbi_set_timer_v01;
> > > >        }
> > > Sadly this isn't quite equivalent to sbi_set_timer having had an initializer.
> > > I would have wanted to suggest to use a constructor function, but we call
> > > init_constructors() even later than do_initcalls() on both Arm and x86 (we
> > > don't call the latter at all on RISC-V so far). Might it be necessary to
> > > introduce sbi_early_init(), called very early during boot? Else how do you
> > > guarantee no accidental use of the variable before it is first set?
> > I thought about an introduction of sbi_early_init() but then decided
> > that set_xen_timer() won't be used earlier than at lest timer_init() +
> > local_irq_enable().
> > Also, sbi_init() is executed pretty early.
> Many more additions to setup.c are to be expected. Are you sure hardly any will
> go ahead of the call to sbi_init()?
Looking at the current state, I don't see something new what will added 
before sbi_init() except percpu_init_areas().
I am okay to introduce sbi_early_init() if it will be really better:

--- a/xen/arch/riscv/sbi.c
+++ b/xen/arch/riscv/sbi.c
@@ -387,3 +387,8 @@ int __init sbi_init(void)

     return 0;
 }
+
+void __init sbi_early_init(void)
+{
+    set_xen_timer = sbi_set_timer_v01;
+}
diff --git a/xen/arch/riscv/setup.c b/xen/arch/riscv/setup.c
index 56a0907a855f..b187a84cd28d 100644
--- a/xen/arch/riscv/setup.c
+++ b/xen/arch/riscv/setup.c
@@ -78,6 +78,8 @@ void __init noreturn start_xen(unsigned long bootcpu_id,
     const char *cmdline;
     size_t fdt_size;

+    sbi_early_init();

But it looks to me that is fine to have what we have now as even someone 
will try to use set_xen_timer earlier a trap will occur and thereby it 
will be need to put the code which start to use set_xen_timer after 
sbi_init().
Best regards,
 Oleksii