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

[Jan Beulich <jbeulich@xxxxxxxx>]

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 question though whether something like this needs logging.

> +        /*
> +         * 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.

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

Jan