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

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

On 3/24/26 3:32 PM, Jan Beulich wrote:
> 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?

Right, I have to mentioned that in the commit message. It will be 
skipped for riscv_isa property here:
  
https://lore.kernel.org/xen-devel/cover.1773157782.git.oleksii.kurochko@gmail.com/T/#m6e45279d24258fe78c6aebf29379fa9135ec9f1c

(what will require to add SSTC extension to guest_unsupp_exts.)

I will add to commit message that except HEVFCFG bit shouldn't be set to 
not let a guest try to access VSTIMECMP register, it should be droppped 
(or not added) from riscv,isa property.

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

Probably, it will be better. Lets drop these changes and re-introduce 
later when guests will support SSTC.

> 
>> --- 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 ...

I think you are right, it "& 0x..." could be dropped as it anyway will 
be truncated by (unsigned long) cast inside csr_write().

Thanks.

~ Oleksii