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

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

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)
> > +#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))
>
> ?
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

Am I missing something?

> Apart from this, no matter that it was Andrew to suggest this, I'd like to
> (once again) point out that identifiers starting with two underscores are
> reserved. I don't see why a single underscore wouldn't do here. Or
> alternatively csr__write32h().
I will apply your suggestion.

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

Thanks.


~ Oleksii