Re: [PATCH v1 07/15] xen/riscv: introduce tracking of pending vCPU interrupts, part 1

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 1/13/26 2:54 PM, Jan Beulich wrote:
> On 13.01.2026 13:51, Oleksii Kurochko wrote:
> > On 1/7/26 5:28 PM, Jan Beulich wrote:
> > > On 24.12.2025 18:03, Oleksii Kurochko wrote:
> > > > --- a/xen/arch/riscv/include/asm/domain.h
> > > > +++ b/xen/arch/riscv/include/asm/domain.h
> > > > @@ -85,6 +85,22 @@ struct arch_vcpu
> > > >        register_t vstval;
> > > >        register_t vsatp;
> > > >        register_t vsepc;
> > > > +
> > > > +    /*
> > > > +     * VCPU interrupts
> > > > +     *
> > > > +     * We have a lockless approach for tracking pending VCPU interrupts
> > > > +     * implemented using atomic bitops. The irqs_pending bitmap represent
> > > > +     * pending interrupts whereas irqs_pending_mask represent bits changed
> > > > +     * in irqs_pending.
> > > And hence a set immediately followed by an unset is then indistinguishable
> > > from just an unset (or the other way around).
> > I think it is distinguishable with the combination of irqs_pending_mask.
> No. The set mask bit tells you that there was a change. But irqs_pending[]
> records only the most recent set / clear.
>
> > >    This may not be a problem, but
> > > if it isn't, I think this needs explaining. Much like it is unclear why the
> > > "changed" state needs tracking in the first place.
> > It is needed to track which bits are changed, irqs_pending only represents
> > the current state of pending interrupts.CPU might want to react to changes
> > rather than the absolute state.
> >
> > Example:
> >    - If CPU 0 sets an interrupt, CPU 1 needs to notice “something changed”
> >      to inject it into the VCPU.
> >    - If CPU 0 sets and then clears the bit before CPU 1 reads it,
> >      irqs_pending alone shows 0, the transition is lost.
> The fact there was any number of transitions is recorded in _mask[], yes,
> but "the transition" was still lost if we consider the "set" in your
> example in isolation. And it's not quite clear to me what's interesting
> about a 0 -> 0 transition. (On x86, such a lost 0 -> 1 transition, i.e.
> one followed directly by a 1 -> 0 one, would result in a "spurious
> interrupt": There would be an indication that there was a lost interrupt
> without there being a way to know which one it was.)
IIUC, in this reply you are talking about when the contents written to the
irq_pending and irqs_pending_mask bitmaps are flushed to the hardware
registers.

Originally, I understood your question to be about the case where
vcpu_set_interrupt() is called and then vcpu_unset_interrupt() is called.

I am trying to understand whether such a scenario is possible.

Let’s take the vtimer as an example. vcpu_set_interrupt(t->v, IRQ_VS_TIMER)
is not called again until vcpu_unset_interrupt(t->v, IRQ_VS_TIMER) and
set_timer() are called in vtimer_set_timer().

The opposite situation is not possible: it cannot happen that
vcpu_set_interrupt(t->v, IRQ_VS_TIMER) is called and then immediately
vcpu_unset_interrupt(t->v, IRQ_VS_TIMER) is called, because
vcpu_unset_interrupt() and set_timer() are only invoked when the guest
has handled the timer interrupt and requested a new one.

So if no interrupt flush is happening, the vcpu_set_interrupt() →
vcpu_unset_interrupt() sequence will only update the irq_pending and
irqs_pending_mask bitmaps, without touching the hardware registers,
so no spurious interrupt will occur. And if an interrupt flush does
happen, it is not possible to have a 1 -> 0 transition due to the call
sequence I mentioned in the last two paragraphs above.

> > By maintaining irqs_pending_mask, you can detect “this bit changed
> > recently,” even if the final state is 0.
> >
> > Also, having irqs_pending_mask allows to flush interrupts without lock:
> > if ( ACCESS_ONCE(v->arch.irqs_pending_mask[0]) )
> > {
> > mask = xchg(&v->arch.irqs_pending_mask[0], 0UL);
> > val = ACCESS_ONCE(v->arch.irqs_pending[0]) & mask;
> >
> > *hvip &= ~mask;
> > *hvip |= val;
> > }
> > Without it I assume that we should have spinlcok around access to irqs_pending.
> Ah yes, this would indeed be a benefit. Just that it's not quite clear to
> me:
>
>      *hvip |= xchg(&v->arch.irqs_pending[0], 0UL);
>
> wouldn't require a lock either
Because vCPU's hvip (which is stored on the stack) can't be changed concurrently
and it's almost the one place in the code where vCPU->hvip is changed. Another
place it is save_csrs() during context switch but it can't be called in parallel
with the vcpu_sync_interrupts() (look below).

> . What may be confusing me is that you put
> things as if it was normal to see 1 -> 0 transitions from (virtual)
> hardware, when I (with my x86 background) would expect 1 -> 0 transitions
> to only occur due to software actions (End Of Interrupt), unless - see
> above - something malfunctioned and an interrupt was lost. That (the 1 ->
> 0 transitions) could be (guest) writes to SVIP, for example.
>
> Talking of which - do you really mean HVIP in the code you provided, not
> VSVIP? So far I my understanding was that HVIP would be recording the
> interrupts the hypervisor itself has pending (and needs to service).
HVIP is correct to use here, HVIP is used to indicate virtual interrupts
intended for VS-mode. And I think you confused HVIP with the HIP register
which supplements the standard supervisor-level SIP register to indicate
pending virtual supervisor (VS-level) interrupts and hypervisor-specific
interrupts.

If a guest will do "That (the 1 -> 0 transitions) could be (guest) writes
to SVIP, for example." then the correspondent HVIP (and HIP as usually
they are aliasis of HVIP) bits will be updated. And that is why we need
vcpu_sync_interrupts() I've mentioned in one of replies and sync VSSIP:
+void vcpu_sync_interrupts(struct vcpu *v)
+{
+    unsigned long hvip;
+
+    /* Read current HVIP and VSIE CSRs */
+    v->arch.vsie = csr_read(CSR_VSIE);
+
+    /* Sync-up HVIP.VSSIP bit changes does by Guest */
+    hvip = csr_read(CSR_HVIP);
+    if ( (v->arch.hvip ^ hvip) & BIT(IRQ_VS_SOFT, UL) )
+    {
+        if ( hvip & BIT(IRQ_VS_SOFT, UL) )
+        {
+            if ( !test_and_set_bit(IRQ_VS_SOFT,
+                                   &v->arch.irqs_pending_mask) )
+                set_bit(IRQ_VS_SOFT, &v->arch.irqs_pending);
+        }
+        else
+        {
+            if ( !test_and_set_bit(IRQ_VS_SOFT,
+                                   &v->arch.irqs_pending_mask) )
+                clear_bit(IRQ_VS_SOFT, &v->arch.irqs_pending);
+        }
+    }
+
+    /* Sync-up AIA high interrupts */
+    vcpu_aia_sync_interrupts(v);
+
+    /* Sync-up timer CSRs */
+    vtimer_sync(v);
+}

> > > > Our approach is modeled around multiple producer
> > > > +     * and single consumer problem where the consumer is the VCPU itself.
> > > > +     *
> > > > +     * DECLARE_BITMAP() is needed here to support 64 vCPU local interrupts
> > > > +     * on RV32 host.
> > > > +     */
> > > > +#define RISCV_VCPU_NR_IRQS 64
> > > > +    DECLARE_BITMAP(irqs_pending, RISCV_VCPU_NR_IRQS);
> > > > +    DECLARE_BITMAP(irqs_pending_mask, RISCV_VCPU_NR_IRQS);
> > > >    }  __cacheline_aligned;
> > > >    
> > > >    struct paging_domain {
> > > > @@ -123,6 +139,9 @@ static inline void update_guest_memory_policy(struct vcpu 
> > > > *v,
> > > >    
> > > >    static inline void arch_vcpu_block(struct vcpu *v) {}
> > > >    
> > > > +int vcpu_set_interrupt(struct vcpu *v, const unsigned int irq);
> > > > +int vcpu_unset_interrupt(struct vcpu *v, const unsigned int irq);
> > > Why the const-s?
> > As irq number isn't going to be changed inside these functions.
> You realize though that we don't normally use const like this? This
> use of qualifiers is meaningless to callers, and of limited meaning to
> the function definition itself. There can be exceptions of course, when
> it is important to clarify that a parameter must not change throughout
> the function.
>
> > > > --- a/xen/arch/riscv/include/asm/riscv_encoding.h
> > > > +++ b/xen/arch/riscv/include/asm/riscv_encoding.h
> > > > @@ -91,6 +91,7 @@
> > > >    #define IRQ_M_EXT                    11
> > > >    #define IRQ_S_GEXT                   12
> > > >    #define IRQ_PMU_OVF                  13
> > > > +#define IRQ_LOCAL_MAX          (IRQ_PMU_OVF + 1)
> > > MAX together with "+ 1" looks wrong. What is 14 (which, when MAX is 14,
> > > must be a valid interrupt)? Or if 14 isn't a valid interrupt, please use
> > > NR or NUM.
> > I didn’t fully understand your idea. Are you suggesting having|IRQ_LOCAL_NR|?
> > That sounds unclear, as it’s not obvious what it would represent.
> > Using|MAX_HART| seems better, since it represents the maximum number allowed
> > for a local interrupt. Any IRQ below that value is considered local, while
> > values above it are implementation-specific interrupts.
> Not quite. If you say "max", anything below _or equal_ that value is
> valid / covered. When you say "num", anything below that value is
> valid / covered. That is, "max" is inclusive for the upper bound of
> the range, while "num" is exclusive. Hence my question whether 14 is
> a valid local interrupt.
14 is architecturally classified as a local interrupt, but its specific
function is currently reserved.

Intention was to cover standard portion (bits 15:0) of sip for which bits
15 and 14 are 0 as they are reserved, so it seems like NUM could be used here.

~ Oleksii