Re: [PATCH v2 12/16] xen/riscv: introduce sbi_set_timer()

[Jan Beulich <jbeulich@xxxxxxxx>]

On 04.02.2026 10:29, Oleksii Kurochko wrote:
> On 2/3/26 6:02 PM, Jan Beulich wrote:
>> On 22.01.2026 17:47, Oleksii Kurochko wrote:
>>> --- a/xen/arch/riscv/sbi.c
>>> +++ b/xen/arch/riscv/sbi.c
>>> @@ -249,6 +249,38 @@ static int (* __ro_after_init sbi_rfence)(unsigned 
>>> long fid,
>>>                                             unsigned long arg4,
>>>                                             unsigned long arg5);
>>>   
>>> +static int cf_check sbi_set_timer_v02(uint64_t stime_value)
>>> +{
>>> +    struct sbiret ret;
>>> +
>>> +    ret = sbi_ecall(SBI_EXT_TIME, SBI_EXT_TIME_SET_TIMER, stime_value,
>>> +#ifdef CONFIG_RISCV_32
>>> +                    stime_value >> 32,
>>> +#else
>>> +                    0,
>>> +#endif
>>> +                    0, 0, 0, 0);
>>> +
>>> +    return sbi_err_map_xen_errno(ret.error);
>>> +}
>>> +
>>> +static int cf_check sbi_set_timer_v01(uint64_t stime_value)
>>> +{
>>> +    struct sbiret ret;
>>> +
>>> +    ret = sbi_ecall(SBI_EXT_0_1_SET_TIMER, 0, stime_value,
>>  From this name I'm judging version 0.1 is meant. How does this fit with ...
>>
>>> @@ -326,6 +358,14 @@ int __init sbi_init(void)
>>>               sbi_rfence = sbi_rfence_v02;
>>>               printk("SBI v0.2 RFENCE extension detected\n");
>>>           }
>>> +
>>> +        if ( sbi_probe_extension(SBI_EXT_TIME) > 0 )
>>> +        {
>>> +            sbi_set_timer = sbi_set_timer_v02;
>>> +            printk("SBI v0.2 TIME extension detected\n");
>>> +        }
>>> +        else
>>> +            sbi_set_timer = sbi_set_timer_v01;
>>>       }
>>>       else
>>>           panic("Ooops. SBI spec version 0.1 detected. Need to add 
>>> support");
>> ... the panic() here?
> 
> panic() is still here as then we will want to add support for rfence v01 SBI 
> call
> too what hasn't been done yet.
> 
> Could it be an option to change panic to:
>     sbi_set_timer = sbi_set_timer_v01;
>     dprintk("SBI v0.1 isn't fully supported\n");
> and then add sbi_rfence = sbi_rfence_v01 when i will be first used?

I don't mind keeping the panic(), but what you add here wants to be structured
such that things won't need moving around once the panic() goes away. I.e. you
want to avoid dealing with v0.1 in both the if() and the else. To accommodate
that, perhaps sbi_set_timer_v01 could simply be the initializer of the new
function pointer variable?

Beyond that, once again you want to clarify things in the commit message.
Adding support for a case which elsewhere you panic() on is, well, confusing
without some explanation.

Jan