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

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 1/15/26 10:52 AM, Jan Beulich wrote:
> On 15.01.2026 10:14, Oleksii Kurochko wrote:
> > On 1/14/26 4:56 PM, Jan Beulich wrote:
> > > On 14.01.2026 16:39, Oleksii Kurochko wrote:
> > > > 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:
> > > > > > 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);
> > > > +        }
> > > > +    }
> > > I fear I don't understand this at all. Why would the guest having set a
> > > pending bit not result in the IRQ to be marked pending?
> > Maybe it is wrong assumption but based on the spec:
> >     Bits sip.SSIP and sie.SSIE are the interrupt-pending and interrupt-enable
> >     bits  for supervisor-level software interrupts. If implemented, SSIP is
> >     writable in sip and may also be set to 1 by a platform-specific interrupt
> >     controller.
> > and:
> >     Interprocessor interrupts are sent to other harts by implementation-specific
> >     means, which will ultimately cause the SSIP bit to be set in the recipient
> >     hart’s sip register.
> >
> > Meaning that sending an IPI to self by writing 1 to sip.SSIP is
> > well-defined. The same should be true of vsip.SSIP while in VS mode.
> I can't read that out of the text above. To the contrary, "will ultimately cause
> the SSIP bit to be set" suggests to me that the bit is not to be set by writing
> the CSR. Things still may work like this for self-IPI, but that wouldn't follow
> from the quotation above.
Why not that wouldn't follow from the quotation above?

The first quotation tells that we can do self-IPI so VSSIP.SSIP will set to 1
what we could miss SSIP bit if won't explicitly try to read h/w HVIP (or VSSIP,
or whatever other alias of the SSIP bit) and sync with what we have cached
in hypervisor.

The second quotation tells that if another CPU send IPI to CPUx then CPUx.SIP 
will
have SSIP bit set to 1 and again hypervisor won't know that without explicit
reading of HVIP (or VSSIP, or whatever other alias of the SSIP bit).


> > >    You can't know
> > > whether that guest write happened before or after you last touched
> > > .irqs_pending{,mask}[]?
> > Yes, I think you are right.
> >
> > On the other hand, if we are in hypervisor when vcpu_sync_interrupts() is
> > called it means that pCPU on which vCPU is ran and for which
> > vcpu_sync_interrupts() is called now executes some hypervisor things, so
> > guest won't able to update VSIP.SSIP for this pCPU. So nothing else will
> > change VSIP.SSIP and so h/w HVIP won't be changed by something and it is
> > okay to sync .irqs_pending{,mask} with what h/w in its HVIP.
> That is, vcpu_sync_interrupts() is called on every entry to the hypervisor?
> Not just during context switch?
It is called each time before exit from the hypervisor to a guest.

~ Oleksii