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

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 4/8/26 1:25 PM, Jan Beulich wrote:
> On 08.04.2026 12:58, Oleksii Kurochko wrote:
> > On 4/2/26 8:41 AM, Jan Beulich wrote:
> > > On 31.03.2026 21:04, Oleksii Kurochko wrote:
> > > > @@ -61,20 +73,7 @@ int reprogram_timer(s_time_t timeout)
> > > >        if ( deadline <= now )
> > > >            return 0;
> > > >    
> > > > -    /*
> > > > -     * TODO: When the SSTC extension is supported, it would be preferable to
> > > > -     *       use the supervisor timer registers directly here for better
> > > > -     *       performance, since an SBI call and mode switch would no longer
> > > > -     *       be required.
> > > > -     *
> > > > -     *       This would also reduce reliance on a specific SBI implementation.
> > > > -     *       For example, it is not ideal to panic() if sbi_set_timer() returns
> > > > -     *       a non-zero value. Currently it can return 0 or -ENOSUPP, and
> > > > -     *       without SSTC we still need an implementation because only the
> > > > -     *       M-mode timer is available, and it can only be programmed in
> > > > -     *       M-mode.
> > > > -     */
> > > > -    if ( (rc = sbi_set_timer(deadline)) )
> > > > +    if ( (rc = set_xen_timer(deadline)) )
> > > >            panic("%s: timer wasn't set because: %d\n", __func__, rc);
> > > >    
> > > >        /* Enable timer interrupt */
> > > > @@ -85,10 +84,17 @@ int reprogram_timer(s_time_t timeout)
> > > >    
> > > >    void __init preinit_xen_time(void)
> > > >    {
> > > > +    unsigned long tmp;
> > > > +
> > > >        if ( acpi_disabled )
> > > >            preinit_dt_xen_time();
> > > >        else
> > > >            panic("%s: ACPI isn't supported\n", __func__);
> > > >    
> > > >        boot_clock_cycles = get_cycles();
> > > > +
> > > > +    if ( csr_read_safe(CSR_STIMECMP, &tmp) )
> > > > +        set_xen_timer = sstc_set_xen_timer;
> > > > +    else
> > > > +        set_xen_timer = sbi_set_timer;
> > > >    }
> > > Doesn't all of this together eliminate the need for sbi_set_timer as a
> > > separate global variable?
> > There's still a need for that SBI-level dispatch. However, sbi_set_timer
> > doesn't need to be a global variable (exported from sbi.h). Since the
> > only external user after this patch is the time.c, sbi_set_timer could
> > be refactored into a plain static internal pointer with a non-static
> > wrapper function:
> >
> > // sbi.c — keep dispatch internal
> > static int (* __ro_after_init sbi_set_timer_fn)(uint64_t) =
> > sbi_set_timer_v01;
> >
> > int cf_check sbi_set_timer(uint64_t stime_value)
> > {
> >       return sbi_set_timer_fn(stime_value);
> > }
> >
> > Do you mean this?
> No. Why is it that we'd still need both set_xen_timer and sbi_set_timer
> as distinct variables?
We don't need, it just a question of design I think.

extern int (* __ro_after_init sbi_set_timer)(uint64_t stime_value) could 
be renamed to set_xen_timer.
And then re-init set_xen_timer here in preinit_xen_time() if SSTC is 
available:
  if ( csr_read_safe(CSR_STIMECMP, &tmp) )
     set_xen_timer = sstc_set_xen_timer;

~ Oleksii