Re: [PATCH v1 08/15] xen/riscv: introduce vtimer_set_timer() and vtimer_expired()

[Jan Beulich <jbeulich@xxxxxxxx>]

On 13.01.2026 15:44, Oleksii Kurochko wrote:
> On 1/8/26 11:28 AM, Jan Beulich wrote:
>> On 24.12.2025 18:03, Oleksii Kurochko wrote:
>>> @@ -15,7 +17,9 @@ int domain_vtimer_init(struct domain *d, struct 
>>> xen_arch_domainconfig *config)
>>>   
>>>   static void vtimer_expired(void *data)
>>>   {
>>> -    panic("%s: TBD\n", __func__);
>>> +    struct vtimer *t = data;
>> Pointer-to-const please.
>>
>>> @@ -37,3 +41,27 @@ void vcpu_timer_destroy(struct vcpu *v)
>>>   
>>>       kill_timer(&v->arch.vtimer.timer);
>>>   }
>>> +
>>> +void vtimer_set_timer(struct vtimer *t, const uint64_t ticks)
>>> +{
>>> +    s_time_t expires = ticks_to_ns(ticks - boot_clock_cycles);
>> boot_clock_cycles is known to just Xen. If the guest provided input is an
>> absolute value, how would that work across migration? Doesn't there need
>> to be a guest-specific bias instead?
> 
> I think that I don't understand fully your questions, but it sounds like it 
> is a job
> for htimedelta register.

Ah yes. As said, still learning RISC-V while reviewing your work.

>>> +    vcpu_unset_interrupt(t->v, IRQ_VS_TIMER);
>>> +
>>> +    /*
>>> +     * According to the RISC-V sbi spec:
>>> +     *   If the supervisor wishes to clear the timer interrupt without
>>> +     *   scheduling the next timer event, it can either request a timer
>>> +     *   interrupt infinitely far into the future (i.e., (uint64_t)-1),
>>> +     *   or it can instead mask the timer interrupt by clearing sie.STIE 
>>> CSR
>>> +     *   bit.
>>> +     */
>> And SBI is the only way to set the expiry value? No CSR access? (Question
>> also concerns the unconditional vcpu_unset_interrupt() above.)
> 
> If we don't have SSTC extension support then I suppose yes, as CSR_MI{E,P} 
> could
> be accessed only from M-mode:

How do M-mode CSRs come into play here? My question was rather towards ...

>   (code from OpenSBI)
> void sbi_timer_event_start(u64 next_event)
> {
>       sbi_pmu_ctr_incr_fw(SBI_PMU_FW_SET_TIMER);
> 
>       /**
>        * Update the stimecmp directly if available. This allows
>        * the older software to leverage sstc extension on newer hardware.
>        */
>       if (sbi_hart_has_extension(sbi_scratch_thishart_ptr(), 
> SBI_HART_EXT_SSTC)) {
> #if __riscv_xlen == 32
>               csr_write(CSR_STIMECMP, next_event & 0xFFFFFFFF);
>               csr_write(CSR_STIMECMPH, next_event >> 32);
> #else
>               csr_write(CSR_STIMECMP, next_event);
> #endif

... what if a guest did these CSR writes directly. Besides intercepting
access to them, you'd also need to synchronize both paths, I suppose.

>>> +    {
>>> +        stop_timer(&t->timer);
>>> +
>>> +        return;
>>> +    }
>>> +
>>> +    set_timer(&t->timer, expires);
>> See the handling of VCPUOP_set_singleshot_timer for what you may want to
>> do if the expiry asked for is (perhaps just very slightly) into the past.
> 
> I got an idea why we want to check if "expires" already expired, but ...
> 
>> There you'll also find a use of migrate_timer(), which you will want to
>> at least consider using here as well.
> 
> ... I don't get why we want to migrate timer before set_timer() here.
> Could you please explain that?

Didn't I see you use migrate_timer() in other patches (making me assume
you understand)? Having the timer tied to the pCPU where the vCPU runs
means the signalling to that vCPU will (commonly) be cheaper. Whether
that actually matters depends on what vtimer_expired() will eventually
contain. Hence why I said "consider using".

Jan