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

[Oleksii Kurochko <oleksii.kurochko@gmail.com>]

On 4/2/26 8:41 AM, Jan Beulich wrote:
> On 31.03.2026 21:04, Oleksii Kurochko wrote:
>> @@ -495,6 +498,36 @@ void __init riscv_fill_hwcap(void)
>>           panic("HW capabilities parsing failed: %s\n", failure_msg);
>>       }
>>   
>> +    if ( csr_read_safe(CSR_STIMECMP, &tmp) )
>> +    {
>> +        printk("SSTC is detected but is supported only for Xen usage not for "
>> +               "a guest\n");
> 
> Please don't wrap format strings. Them going slightly beyond 80 columns is okay.
> Them going much beyond that is a good indication that you want to consider re-
> wording. (Here e.g. "SSTC detected; supported for Xen use, but not for guests".)

I will reword log message in the form you suggested.

> 
> I question though whether something like this needs logging.

It is just a debug reminder that something should be additionally done. 
I will do it dprintk() so it won't appear in release builds.

> 
>> +        /*
>> +         * As SSTC for guest isn't supported it is needed temprorary to:
> 
> Nit: temporary
> 
>> +         *
>> +         * 1. Clear bit RISCV_ISA_EXT_sstc in riscv_isa as theoretuically it
> 
> Nit: theoretically
> 
>> +         *    could be that OpenSBI (it doesn't pass it now) or whatever ran
>> +         *    before Xen will add SSTC to riscv,isa string. This bit clear
>> +         *    won't allow guest to use SSTC extension as vtimer context
>> +         *    switch and restore isn't ready for that.
>> +         */
>> +        __clear_bit(RISCV_ISA_EXT_sstc, riscv_isa);
> 
> Seeing your other series, shouldn't this instead be done without affecting
> riscv_isa? The BUG_ON()s in vtimer.x therefore also look inappropriate.

It is incorrect to use __clear_bit(). What should be used instead is 
__set_bit(), because in the current boot process OpenSBI does not add 
"sstc" to the riscv,isa string. Therefore, we need to set the 
RISCV_ISA_EXT_sstc bit manually.

This change will affect BUG_ON() checks in vtimer.c, which I plan to 
remove. If SSTC is not supported for the guest, there is nothing we can 
do anyway. It might make sense to reintroduce these BUG_ON() checks once 
an unsupported bitmap is implemented [1]. At that point, we could have 
something like the following in the vtimer save and restore context 
functions in vtimer.c:

BUG_ON(!riscv_isa_extension_available(guest_unsupp_bmp, 
RISCV_ISA_EXT_sstc));

On the other hand, using BUG_ON() in the vtimer save and restore 
functions is of limited value. If SSTC support for guests is added in 
the future, these functions will need to be updated anyway, so such 
checks may become redundant.

[1] 
https://lore.kernel.org/xen-devel/007c0a0243ac7ff1d1ab3faa4ebcdd6fcd14e485.1773157782.git.oleksii.kurochko@gmail.com/


> 
>> @@ -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?

Thanks.

~ Oleksii