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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 13.03.2026 17:44, Oleksii Kurochko wrote:
> OpenSBI currently does not advertise the SSTC extension via the device
> tree. Additionally, SSTC can no longer be reliably disabled by removing
> the "sstc" string from riscv,isa, as OpenSBI probes support by attempting
> to access CSR_STIMECMP.

Still don't yopu need to remove that string from what guests get to see, ...

> Introduce a runtime probe in Xen to determine whether SSTC is available.
> The probe attempts to read CSR_STIMECMP using csr_allowed_read(). If the
> access succeeds, SSTC is considered available; if a trap occurs, it is
> treated as unsupported.
> 
> When SSTC is detected, Xen may use it internally to program timers.
> However, the extension is not exposed to guests because the required
> context switch handling for the SSTC CSRs is not yet implemented.
> 
> To prevent guests from using SSTC, RISCV_ISA_EXT_sstc is cleared from the
> riscv_isa bitmap. As a result, the corresponding HENVCFG bit is not set
> and guests fall back to the SBI timer interface. Timer requests are then
> handled by Xen via the usual SBI interception path.

... alongside the riscv_isa adjustment?

> --- a/xen/arch/riscv/cpufeature.c
> +++ b/xen/arch/riscv/cpufeature.c
> @@ -17,6 +17,7 @@
>  #include <xen/sections.h>
>  
>  #include <asm/cpufeature.h>
> +#include <asm/csr.h>
>  
>  #ifdef CONFIG_ACPI
>  # error "cpufeature.c functions should be updated to support ACPI"
> @@ -139,6 +140,7 @@ const struct riscv_isa_ext_data __initconst 
> riscv_isa_ext[] = {
>      RISCV_ISA_EXT_DATA(smaia),
>      RISCV_ISA_EXT_DATA(smstateen),
>      RISCV_ISA_EXT_DATA(ssaia),
> +    RISCV_ISA_EXT_DATA(sstc),
>      RISCV_ISA_EXT_DATA(svade),
>      RISCV_ISA_EXT_DATA(svpbmt),
>  };
> @@ -483,6 +485,7 @@ void __init riscv_fill_hwcap(void)
>      unsigned int i;
>      const size_t req_extns_amount = ARRAY_SIZE(required_extensions);
>      bool all_extns_available = true;
> +    unsigned long tmp;
>  
>      riscv_fill_hwcap_from_isa_string();
>  
> @@ -495,6 +498,36 @@ void __init riscv_fill_hwcap(void)
>          panic("HW capabilities parsing failed: %s\n", failure_msg);
>      }
>  
> +    if ( csr_allowed_read(CSR_STIMECMP, &tmp) )
> +    {
> +        printk("SSTC is detected but is supported only for Xen usage not for 
> "
> +               "a guest.\n");

No full stops please in log messages.

> +        /*
> +         * As SSTC for guest isn't supported it is needed temprorary to:
> +         *
> +         * 1. Clear bit RISCV_ISA_EXT_sstc in riscv_isa as theoretuically it
> +         *    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

Nit: Stray double blanks.

> +         *    willn't allow guest to use SSTC extension as vtimer context

Nit: won't

> +         *    switch and restore isn't ready for that.
> +         */
> +        __clear_bit(RISCV_ISA_EXT_sstc, riscv_isa);
> +
> +        /*
> +         * 2. A VS-timer interrupt becomes pending whenever the value of
> +         *    (time + htimedelta) is greater than or equal to vstimecmp CSR.
> +         *    Thereby to avoid spurious VS-timer irqs set vstimecmp CSR to
> +         *    -1.

-1 is misleading here, as any unsigned value is greater than -1. You mean
UINT64_MAX or e.g. ~0U here.

> +         * It should be dropped when SSTC for guests will be supported.
> +         */
> +        csr_write(CSR_VSTIMECMP, ULONG_MAX);
> +#ifdef CONFIG_RISCV_32
> +        csr_write(CSR_VSTIMECMPH, ULONG_MAX);
> +#endif
> +    }
> +
>      for ( i = 0; i < req_extns_amount; i++ )
>      {
>          const struct riscv_isa_ext_data ext = required_extensions[i];
> --- a/xen/arch/riscv/domain.c
> +++ b/xen/arch/riscv/domain.c
> @@ -99,6 +99,9 @@ static void vcpu_csr_init(struct vcpu *v)
>      if ( riscv_isa_extension_available(NULL, RISCV_ISA_EXT_svpbmt) )
>          v->arch.henvcfg = ENVCFG_PBMTE & csr_masks.henvcfg;
>  
> +    if ( riscv_isa_extension_available(NULL, RISCV_ISA_EXT_sstc) )
> +        v->arch.henvcfg |= ENVCFG_STCE & csr_masks.henvcfg;

Wouldn't this better be part of the (future) patch enabling SSTC for guests?

> --- a/xen/arch/riscv/include/asm/riscv_encoding.h
> +++ b/xen/arch/riscv/include/asm/riscv_encoding.h
> @@ -396,6 +396,8 @@
>  #define CSR_VSTVAL                   0x243
>  #define CSR_VSIP                     0x244
>  #define CSR_VSATP                    0x280
> +#define CSR_VSTIMECMP                0x24D
> +#define CSR_VSTIMECMPH               0x25D

I think it would be nice if throughout the CSR definitions you settled on
using upper case hex digits uniformly, or all lower case ones (personally
I'd prefer the latter).

> --- a/xen/arch/riscv/time.c
> +++ b/xen/arch/riscv/time.c
> @@ -13,6 +13,20 @@
>  unsigned long __ro_after_init cpu_khz; /* CPU clock frequency in kHz. */
>  uint64_t __ro_after_init boot_clock_cycles;
>  
> +static int cf_check sstc_set_xen_timer(uint64_t deadline)
> +{
> +#ifdef CONFIG_RISCV_32
> +    csr_write(CSR_STIMECMP, deadline & 0xFFFFFFFF);

The "& 0x..." isn't needed here, is it? I.e. the whole function could be ...

> +    csr_write(CSR_STIMECMPH, deadline >> 32);
> +#else
> +    csr_write(CSR_STIMECMP, deadline);
> +#endif
> +
> +    return 0;
> +}

static int cf_check sstc_set_xen_timer(uint64_t deadline)
{
    csr_write(CSR_STIMECMP, deadline);
#ifdef CONFIG_RISCV_32
    csr_write(CSR_STIMECMPH, deadline >> 32);
#endif

    return 0;
}

> +int (* __ro_after_init set_xen_timer)(uint64_t deadline);

static?

Jan