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

[Jan Beulich <jbeulich@xxxxxxxx>]

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?

Jan