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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 21.04.2026 11:01, Oleksii Kurochko wrote:
> On 4/20/26 9:56 AM, Jan Beulich wrote:
>> On 17.04.2026 09:24, Oleksii Kurochko wrote:
>>> --- a/xen/arch/riscv/include/asm/csr.h
>>> +++ b/xen/arch/riscv/include/asm/csr.h
>>> @@ -32,6 +32,20 @@
>>>                              : "memory" );                        \
>>>   })
>>>   
>>> +#ifdef CONFIG_RISCV_32
>>> +# define __csr_write32h(csr, val) csr_write(csr ## H, (val) >> 32)

In my reply I followed this. If this compiled, then ...

>>> +#else
>>> +# define __csr_write32h(csr, val) ((void)(csr), (void)(val))
>>
>> In order to be able to spot issues in 64-bit builds, how about
>>
>> # define __csr_write32h(csr, val) ((void)csr ## H, (void)(val))
>>
>> ?

... aiui this would compile as well. Looks like the RV32 case then is in
need of adjustment as well.

> But this will cause a build issue in 64-bit builds.
> 
> csr_write64(CSR_STIMECMP, ...)
>    └─ __csr_write32h(csr, _v)   ← csr is NOT ##-adjacent here
>                                    so preprocessor expands it FIRST
>                                    CSR_STIMECMP → 0x14D
>         └─ (void)csr ## H       ← csr is already 0x14D here
>                                    0x14D ## H → 0x14DH  ERROR
> 
> Probably, it would be better to do in the following way:
> 
> #ifdef CONFIG_RISCV_32
> #define csr_write64(csr, val)       \
> ({                                  \
>      uint64_t v_ = (val);            \
>      csr_write(csr, v_);             \
>      csr_write(csr ## H, v_ >> 32);  \
> })
> #else
> #define csr_write64(csr, val)       \
> ({                                  \
>      uint64_t v_ = (val);            \
>      csr_write(csr, v_);             \
> })
> #endif

E.g. like this, albeit in the RV64 case the local v_ isn't needed. Instead,
again to be able to spot issues in RV64 builds, (void)csr ## H may want
adding.

A clear downside to all of this is that this helper can only be used with
CSR_* constants, not with runtime-calculated CSR numbers.

>>> @@ -279,8 +299,6 @@ static int cf_check sbi_set_timer_v01(uint64_t 
>>> stime_value)
>>>       return sbi_err_map_xen_errno(ret.error);
>>>   }
>>>   
>>> -int (* __ro_after_init sbi_set_timer)(uint64_t stime_value) = 
>>> sbi_set_timer_v01;
>>> -
>>>   int sbi_remote_sfence_vma(const cpumask_t *cpu_mask, vaddr_t start,
>>>                             size_t size)
>>>   {
>>> @@ -360,10 +378,9 @@ int __init sbi_init(void)
>>>           }
>>>   
>>>           if ( sbi_probe_extension(SBI_EXT_TIME) > 0 )
>>> -        {
>>> -            sbi_set_timer = sbi_set_timer_v02;
>>> -            dprintk(XENLOG_INFO, "SBI v0.2 TIME extension detected\n");
>>> -        }
>>> +            set_xen_timer = sbi_set_timer_v02;
>>> +        else
>>> +            set_xen_timer = sbi_set_timer_v01;
>>>       }
>>
>> Sadly this isn't quite equivalent to sbi_set_timer having had an initializer.
>> I would have wanted to suggest to use a constructor function, but we call
>> init_constructors() even later than do_initcalls() on both Arm and x86 (we
>> don't call the latter at all on RISC-V so far). Might it be necessary to
>> introduce sbi_early_init(), called very early during boot? Else how do you
>> guarantee no accidental use of the variable before it is first set?
> 
> I thought about an introduction of sbi_early_init() but then decided 
> that set_xen_timer() won't be used earlier than at lest timer_init() + 
> local_irq_enable().
> Also, sbi_init() is executed pretty early.

Many more additions to setup.c are to be expected. Are you sure hardly any will
go ahead of the call to sbi_init()?

Jan