Re: [PATCH v6 03/14] xen/riscv: introduce tracking of pending vCPU interrupts, part 1

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 3/3/26 2:35 PM, Jan Beulich wrote:
> On 26.02.2026 12:51, Oleksii Kurochko wrote:
> > Based on Linux kernel v6.16.0.
> > Note that smp_wmb() is used instead of smp_mb__before_atomic() as what
> > we want to guarantee that if a bit in irqs_pending_mask is obversable
> > that the correspondent bit in irqs_pending is observable too.
> >
> > Add lockless tracking of pending vCPU interrupts using atomic bitops.
> > Two bitmaps are introduced:
> >   - irqs_pending — interrupts currently pending for the vCPU
> >   - irqs_pending_mask — bits that have changed in irqs_pending
> >
> > The design follows a multi-producer, single-consumer model, where the
> > consumer is the vCPU itself. Producers may set bits in
> > irqs_pending_mask without a lock. Clearing bits in irqs_pending_mask is
> > performed only by the consumer via xchg(). The consumer must not write
> > to irqs_pending and must not act on bits that are not set in the mask.
> > Otherwise, extra synchronization should be provided.
> >
> > On RISC-V interrupts are not injected via guest registers, so pending
> > interrupts must be recorded in irqs_pending (using the new
> > vcpu_{un}set_interrupt() helpers) and flushed to the guest by updating
> > HVIP before returning control to the guest. The consumer side is
> > implemented in a follow-up patch.
> >
> > A barrier between updating irqs_pending and setting the corresponding
> > mask bit in vcpu_set_interrupt()/vcpu_unset_interrupt() guarantees
> > that if the consumer observes a mask bit set, the corresponding pending
> > bit is also visible. This prevents missed interrupts during the flush.
> >
> > It is possible that a guest could have pending bit in the hardware
> > register without being marked pending in irq_pending bitmap as:
> >    According to the RISC-V ISA specification:
> >      Bits hip.VSSIP and hie.VSSIE are the interrupt-pending and
> >      interrupt-enable  bits for VS-level software interrupts. VSSIP in hip
> >      is an alias (writable) of the same bit in hvip.
> >    Additionally:
> >      When bit 2 of hideleg is zero, vsip.SSIP and vsie.SSIE are read-only
> >      zeros. Else, vsip.SSIP and vsie.SSIE are aliases of hip.VSSIP and
> >      hie.VSSIE.
> > This means the guest may modify vsip.SSIP, which implicitly updates
> > hip.VSSIP and the bit being written with 1 would also trigger an interrupt
> > as according to the RISC-V spec:
> >    These conditions for an interrupt trap to occur must be evaluated in a
> >    bounded   amount of time from when an interrupt becomes, or ceases to be,
> >    pending in sip,  and must also be evaluated immediately following the
> >    execution of an SRET  instruction or an explicit write to a CSR on which
> >    these interrupt trap conditions expressly depend (including sip, sie and
> >    sstatus).
> > What means that IRQ_VS_SOFT must be synchronized separately, what is done
> > in vcpu_sync_interrupts(). Note, also, that IRQ_PMU_OVF would want to be
> > synced for the similar reason as IRQ_VS_SOFT, but isn't sync-ed now as
> > PMU isn't supported now.
> >
> > For the remaining VS-level interrupt types (IRQ_VS_TIMER and
> > IRQ_VS_EXT), the specification states they cannot be modified by the guest
> > and are read-only because of:
> >    Bits hip.VSEIP and hie.VSEIE are the interrupt-pending and interrupt-enable
> >    bits for VS-level external interrupts. VSEIP is read-only in hip, and is
> >    the logical-OR of these interrupt sources:
> >      • bit VSEIP of hvip;
> >      • the bit of hgeip selected by hstatus.VGEIN; and
> >      • any other platform-specific external interrupt signal directed to
> >        VS-level.
> >    Bits hip.VSTIP and hie.VSTIE are the interrupt-pending and interrupt-enable
> >    bits for VS-level timer interrupts. VSTIP is read-only in hip, and is the
> >    logical-OR of hvip.VSTIP and any other platform-specific timer interrupt
> >    signal directed to VS-level.
> > and
> >    When bit 10 of hideleg is zero, vsip.SEIP and vsie.SEIE are read-only zeros.
> >    Else, vsip.SEIP and vsie.SEIE are aliases of hip.VSEIP and hie.VSEIE.
> >
> >    When bit 6 of hideleg is zero, vsip.STIP and vsie.STIE are read-only zeros.
> >    Else, vsip.STIP and vsie.STIE are aliases of hip.VSTIP and hie.VSTIE.
> > and also,
> >    Bits sip.SEIP and sie.SEIE are the interrupt-pending and interrupt-enable
> >    bits for supervisor-level external interrupts. If implemented, SEIP is
> >    read-only in sip, and is set and cleared by the execution environment,
> >    typically through a platform-specific interrupt controller.
> >
> >    Bits sip.STIP and sie.STIE are the interrupt-pending and interrupt-enable
> >    bits for supervisor-level timer interrupts. If implemented, STIP is
> >    read-only in sip, and is set and cleared by the execution environment
> > Thus, for these interrupt types, it is sufficient to use vcpu_set_interrupt()
> > and vcpu_unset_interrupt(), and flush them during the call of
> > vcpu_flush_interrupts() (which is introduced in follow up patch).
> >
> > vcpu_sync_interrupts(), which is called just before entering the VM,
> > slightly bends the rule that the irqs_pending bit must be written
> > first, followed by updating the corresponding bit in irqs_pending_mask.
> > However, it still respects the core guarantee that the producer never
> > clears the mask and only writes to irqs_pending if it is the one that
> > flipped the corresponding mask bit from 0 to 1.
> > Moreover, since the consumer won't run concurrently because
> > vcpu_sync_interrupts() and the consumer path are going to be invoked
> > sequentially immediately before VM entry, it is safe to slightly relax
> > this ordering rule in vcpu_sync_interrupts().
> >
> > Signed-off-by: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>
> > ---
> > Changes in v6:
> >   - Drop for the moment:
> >         /* Read current HVIP and VSIE CSRs */
> >         v->arch.vsie = csr_read(CSR_VSIE);
> >     from vcpu_sync_interrupts() as it isn't used at the moment and will
> >     be introduced when a usage will be more clear.
> With this, shouldn't the RV32 related #ifdef in vcpu_sync_interrupts() also
> go away?
It could be done in this way. It's just a hint for a person who will add RV32
not to miss to update vcpu_sync_interrupts() properly. Lets drop that for now
too and deal with that during review if a person will miss to make correspondent
update of vcpu_sync_interrupts() for RV32.

> > +void vcpu_sync_interrupts(struct vcpu *v)
> The sole caller passes "current". Are other uses of this function planned?
> If not either "current" wants directly using here, or minimally the parameter
> wants renaming to "curr". In fact ...
>
> > +{
> > +    unsigned long hvip = csr_read(CSR_HVIP);
> ... this suggests it's unlikely for the function to be valid to call with
> other than "current".
Agree, I will drop v argument and use current inside vcpu_sync_interrupts().

Thanks.

~ Oleksii