Re: [PATCH v2 36/39] xen/riscv: wake up a descheduled vCPU on a guest external interrupt

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 9/18/26 2:52 PM, Jan Beulich wrote:
> On 27.08.2026 17:21, Oleksii Kurochko wrote:
> > @@ -62,23 +69,25 @@ static int cf_check cpu_callback(struct notifier_block *nfb,
> >                                    unsigned long action, void *hcpu)
> >   {
> >       unsigned int cpu = (unsigned long)hcpu;
> > -    int rc = 0;
> >   
> >       switch ( action )
> >       {
> >       case CPU_STARTING:
> > -        rc = vgein_init();
> > +    {
> > +        int rc = vgein_init();
> > +
> >           if ( rc )
> >               printk(XENLOG_ERR "AIA: failed to init vgein for CPU%u: %d\n",
> >                      cpu, rc);
> >           break;
> > +    }
> >   
> >       case CPU_DYING:
> >           vgein_deinit();
> >           break;
> >       }
> >   
> > -    return notifier_from_errno(rc);
> > +    return NOTIFY_DONE;
> >   }
> What is this hunk doing in this patch? Was this meant to be merged into the
> prior one?
Yes, it was meant to be a part of prev. patch.

> But then - why?
I had a case with what rc returns but looking at my downstream branches 
I don't face this case anymore so this hunk should be just dropped.
> > @@ -168,3 +181,36 @@ void vgein_release(struct vcpu *v, unsigned int vgein_id, 
> > unsigned int cpu)
> >               __func__, v, vgein_id, cpu, vgein->bmp);
> >   #endif
> >   }
> > +
> > +void hgei_interrupt(void)
> > +{
> > +    unsigned long hgei_mask, flags;
> > +    struct vgein_ctrl *vgein = &this_cpu(vgein);
> > +
> > +    hgei_mask = csr_read(CSR_HGEIP) & csr_read(CSR_HGEIE);
> Misra, aiui, isn't going to like this. You may want to split it up.
I will write in the following way then:
    hgei_mask = csr_read(CSR_HGEIP);
    hgei_mask &= csr_read(CSR_HGEIE);

> > +    csr_clear(CSR_HGEIE, hgei_mask);
> > +
> > +    spin_lock_irqsave(&vgein->lock, flags);
> > +
> > +    for_each_set_bit ( guest_file_id, hgei_mask )
> > +    {
> > +        /*
> > +         * guest_file_id shouldn't be zero, as it will indicate that no
> > +         * guest external interrupt source is selected for VS-level external
> > +         * interrupts.
> > +         */
> > +        ASSERT(guest_file_id);
> While it only affects debug builds, this check still needlessly is
> done on every loop iteration, when doing it once ahead of the loop
> would suffice.
Good point. I will do in the following way then before the loop:

    /*
     * Bit 0 of HGEIP/HGEIE is read-only zero: guest interrupt file ID 0
     * means that no guest external interrupt source is selected.
     */
    ASSERT(!(hgei_mask & 1));


> > +        if ( vgein->owners[guest_file_id] )
> > +        {
> > +#ifdef VGEIN_DEBUG
> > +            gprintk(XENLOG_DEBUG, "%s: kick ->%pv, hgei_mask(%#lx)\n",
> > +                    __func__, vgein->owners[guest_file_id], hgei_mask);
> > +#endif
> This can ocur very frequently (when VGEIN_DEBUG is defined). A
> trace record may be a better alternative.
Agree, it would be nice but tracing isn't ready for RISC-V.

Considering that we haven't had any issue with hgei interrupt for a long 
time I will just drop gprintk() for now and use `trace record` when 
functionality will be ready.
> > --- a/xen/arch/riscv/domain.c
> > +++ b/xen/arch/riscv/domain.c
> > @@ -136,6 +136,8 @@ static void vcpu_csr_init(struct vcpu *v)
> >           v->arch.hstateen0 = (hstateen0 & csr_masks.hstateen0) |
> >                               csr_masks.ro_one.hstateen0;
> >       }
> > +
> > +    v->arch.hie = MIP_SGEIP;
> Neither part of the rhs identifier has anything to do with the CSR
> having its default value set here. That's perhaps again a piece of
> RISC-V I'm missing, but I can't make sense of this.
All interrupt pending/enable CSRs share one bit layout (bit N 
corresponds to interrupt cause N), which is why MIP_SGEIP happened to be 
numerically right. I'll use BIT(IRQ_S_GEXT, UL) instead and add a 
comment: hie.SGEIE is what allows the SGEI raised through hgeie to be 
taken by Xen at all, while hie's VS-level bits alias the guest's vsie 
and are left cleared. I will apply the following change:
-    v->arch.hie = MIP_SGEIP;
+    /*
+     * Enable SGEIs, so that a guest interrupt file marked in HGEIE while
+     * the vCPU is descheduled can raise an interrupt to Xen.
+     *
+     * The VS-level bits of hie alias the guest's vsie, which is saved and
+     * restored separately, so they are left clear here.
+     */
+    v->arch.hie = BIT(IRQ_S_GEXT, UL);


> > --- a/xen/arch/riscv/imsic.c
> > +++ b/xen/arch/riscv/imsic.c
> > @@ -510,12 +510,31 @@ void cf_check imsic_ctxt_switch_from(struct vcpu *v)
> >   
> >       write_lock_irqsave(&imsic_state->vsfile_lock, flags);
> >       imsic_state->vsfile_cpu = v->processor;
> > +    /*
> > +     * Start to observe the VS-file from HS-mode: while the vCPU isn't
> > +     * running an interrupt pending in its VS-file is reported through HGEIP
> > +     * instead of being delivered to VS-mode, which lets Xen wake the vCPU up.
> > +     */
> > +    csr_set(CSR_HGEIE, BIT(imsic_state->guest_file_id, UL));
> >       write_unlock_irqrestore(&imsic_state->vsfile_lock, flags);
> >   }
> It is suspicious for the HGEIE write to be the last step. How's this free
> of a window where an interrupt is lost. (Sorry, likely another blind spot
> of mine wrt RISC-V.)
There's no window: a guest interrupt file's hgeip bit is 
level-sensitive, i.e. it reflects whether the file currently has a 
pending-and-enabled interrupt (the MSI itself stays latched in the 
file's eip[] until the guest claims it), and hip.SGEIP is simply (hgeip 
& hgeie) != 0. So an MSI arriving after the vCPU stopped running but 
before hgeie is set raises an SGEI as soon as the bit is set, taken once 
interrupts are re-enabled. I'll extend the comment to say so:
    /*
     * Start to observe the VS-file from HS-mode: while the vCPU isn't
     * running an interrupt pending in its VS-file is reported through 
HGEIP
     * instead of being delivered to VS-mode, which lets Xen wake the 
vCPU up.
     *
     * HGEIP is level-sensitive, reflecting the VS-file's current state, so
     * an interrupt that became pending before this point raises an SGEI as
     * soon as the bit is set in HGEIE; nothing is lost in between.
     */

Does it make sense?

> >   void cf_check imsic_ctxt_switch_to(struct vcpu *v)
> >   {
> > -    /* Nothing to do */
> > +    struct vimsic_state *imsic_state = v->arch.vimsic_state;
> > +    unsigned long flags;
> > +
> > +    /* A s/w VS-file is never observed through HGEIP. */
> > +    if ( !vcpu_guest_file_id(v) )
> > +        return;
> > +
> > +    /*
> > +     * The vCPU is about to run, so hstatus.VGEIN delivers the VS-file's
> > +     * interrupts to it directly and there is nothing left for Xen to observe.
> > +     */
> > +    read_lock_irqsave(&imsic_state->vsfile_lock, flags);
> > +    csr_clear(CSR_HGEIE, BIT(imsic_state->guest_file_id, UL));
> > +    read_unlock_irqrestore(&imsic_state->vsfile_lock, flags);
> >   }
> How can this be a read-lock when you write a CSR?
But here is a protection of ->guest_file_id not of write to a CSR and we 
want to not have a change of ->guest_file_id during an update of CSR_HGEIE.
> Or else - why is locking
> here necessary in the firt place?
Strictly speaking no locking is needed with the current implementation: 
HGEIE is local to this pCPU, and guest_file_id can't change 
concurrently. It's updated either by this very pCPU ahead of switching 
the vCPU in (imsic_migrate_vcpu() called from schedule(), 
imsic_vsfile_attach() right after), or while the vCPU can't be 
scheduled: sched_unit_migrate_finish() defers to unit_context_saved()
while the unit is running, and sched_move_domain() pauses the domain.

(and the similar are true for imsic_ctxt_switch_from())

But IMO we should keep around ->vsfile_lock to not miss the case where 
some case will update ->guest_file_id in parallel with 
imsic_ctxt_switch_to().
Does it make to continue to have read_lock_irqsave(...->vsfile_lock, 
...) here just for potential future cases?
Thanks.

~ Oleksii