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

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 1/13/26 4:12 PM, Jan Beulich wrote:
> 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:
> > > > +    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 ...
Without SSTC (Supervisor Timer Extension) the current Privileged arch 
specification
only defines a hardware mechanism for generating machine-mode timer interrupts 
(based
on the mtime and mtimecmp registers). With the resultant requirement that timer
services for S-mode/HS-mode (and for VS-mode) have to all be provided by M-mode 
- via
SBI calls from S/HS-mode up to M-mode (or VS-mode calls to HS-mode and then to 
M-mode).

> >    (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,
These registers are available only when the SSTC extension is present.
When SSTC is available and a guest accesses CSR_STIMECMP{H}, it actually
accesses the corresponding VS aliases, VSTIMECMP{H}. The hardware continuously
compares the value in VSTIMECMP against the guest’s view of time
(time + htimedelta). When the condition is met, the hardware asserts the
virtual supervisor timer interrupt pending bit (VSTIP) in the hypervisor’s
HIP register and guest automatically receives timer interrupt.

Therefore, there is no real need to intercept accesses to these registers.

It is possible that VS-mode software may continue to use the SBI timer call
instead of directly accessing the SSTC CSRs. In that case, VSTIMECMP would
need to be updated manually by the hypervisor when such an SBI call occurs.
However, this is not the case at the moment, as the SSTC extension is not
currently supported.

Technically, the hypervisor could also clear henvcfg.STCE when SSTC is
vailable. In that case, the hypervisor would receive an illegal
instruction trap in HS-mode when the guest attempts to access SSTC-related
registers.
However, I do not see a reason to prevent delegation of SSTC register access
to the guest, since SSTC provides VS-* aliases for these registers, so I don't
consider that as a real case.


> you'd also need to synchronize both paths, I suppose.
I didn't get you what is needed to be synchronized. Could you please explain?


> > > > +    {
> > > > +        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.
I thought that migrate_timer() is needed only when a vCPU changes the pCPU
it is running on to ensure that it is running on correct pCPU after migrations,
hotplug events, or scheduling changes. That is why I placed it in
vtimer_restore(), as there is no guarantee that the vCPU will run on the
same pCPU it was running on previously.

So that is why ...

> Whether
> that actually matters depends on what vtimer_expired() will eventually
> contain. Hence why I said "consider using".
... I didn't get why I might need vtimer_expired() in vtimer_set_timer()
before set_timer().

vtimer_expired() will only notify the vCPU that a timer interrupt has
occurred by setting bit in irqs_pending bitmap which then will be synced
with vcpu->hvip, but I still do not understand whether migrate_timer()
is needed before calling set_timer() here.

Considering that vtimer_set_timer() is called from the vCPU while it is
running on the current pCPU, and assuming no pCPU rescheduling has
occurred for this vCPU, we are already on the correct pCPU.
If pCPU rescheduling for the vCPU did occur, then migrate_timer() would
have been called in context_switch(), and at the point where
vtimer_set_timer() is invoked, we would already be running on the
correct pCPU.

~ Oleksii