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

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 11/4/26 11:37 AM, Baptiste Le Duc wrote:
> > While a vCPU is running, MSIs written to its h/w IMSIC guest interrupt
> > file are delivered straight to VS-mode. Once the vCPU is descheduled
> > nobody observes that file anymore, so a guest blocked on such an
> > interrupt would stay blocked until some unrelated event happens to
> > schedule it again.
> >
> > Let Xen observe the file in that window: on deschedule set the vCPU's
> > bit in HGEIE, which turns an interrupt pending in its VS-file into an
> > HS-level SGEI, and clear the bit again on schedule-in. HGEIP only
> > reports an interrupt file number, so to find the vCPU to kick, keep a
> > per-pCPU owners[] array indexed by file number and updated in
> > vgein_assign()/vgein_release() together with the VGEIN bitmap. The SGEI
> > handler clears the HGEIE bits it has seen before kicking their owners,
> > so that the interrupt doesn't keep firing until the vCPU is switched in.
> Please explain why this is enough for a descheduled
> vCPU to never miss a wake-up: an interrupt pending in a VS-file always
> shows up in HGEIP, but it reaches VS-mode only if hstatus.VGEIN selects
> that file, and traps to Xen as an SGEI only if the file's HGEIE bit is
> set. While the vCPU is descheduled, HGEIE is therefore Xen's only way to
> notice it. And as HGEIP reflects the file's current state rather than
> being latched, an interrupt which arrived before the bit got set still
> raises the SGEI once it is.
Will add, thanks.

> > [ ... 49 lines skipped ... ]
> > --- a/xen/arch/riscv/aia.c
> > +++ b/xen/arch/riscv/aia.c
> > @@ -18,6 +18,13 @@ struct vgein_ctrl {
> >       /* The least-significant bits are implemented first, apart from bit 0 */
> >       unsigned long bmp;
> >       spinlock_t lock;
> > +    /*
> > +     * Guest interrupt file IDs run from 1 to geilen inclusive (0 means that
> > +     * no guest external interrupt source is selected), and geilen can never
> > +     * exceed BITS_PER_LONG - 1, so indexing this array by the ID directly
> > +     * always fits.
> > +     */
> > +    struct vcpu *owners[BITS_PER_LONG];
> It looks like owners[] already carries the information bmp provides.
> Couldn't we drop bmp and use owners[] alone, with owners[i] == NULL
> meaning that file i is free? That would avoid having to keep the two in
> sync, and vgein_release() could then check ASSERT(owners[id] == v)
> rather than just that the bit was set. A linear scan over 1..geilen in
> vgein_assign() should be fine given geilen < BITS_PER_LONG.
>
> If you agree, it would probably be cleaner to introduce owners[] in
> place of bmp in the earlier "add basic VGEIN management" patch, rather
> than adding bmp there and replacing it here.
It would work, but I'd rather keep bmp: it's updated together with
owners[], under the same lock, in vgein_assign() and vgein_release()
only, so there isn't much to keep in sync. It also keeps the state of
all the files in a single word, so a free VGEIN ID is found with a
single find_next_zero_bit() rather than by iterating over owners[] (but 
I could agree that likely not a big win), and the debug output can print 
it as is.
The stronger check in vgein_release() is a good idea regardless, so I'll
add ASSERT(vgein->owners[vgein_id] == v) here.

> > [ ... 26 lines skipped ... ]
> > +
> > +void hgei_interrupt(void)
> > +{
> > +    unsigned long hgei_mask, flags;
> > +    struct vgein_ctrl *vgein = &this_cpu(vgein);
> > +
> > +    hgei_mask = csr_read(CSR_HGEIP);
> > +    hgei_mask &= csr_read(CSR_HGEIE);
> > +
> > +    csr_clear(CSR_HGEIE, hgei_mask);
> > +
> > +    spin_lock_irqsave(&vgein->lock, flags);
> > +
> > +    /*
> > +     * 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));
> Is this ASSERT() really needed? Bit 0 of HGEIP/HGEIE is read-only zero
> per the spec, and vgein_init() already relies on that when computing
> geilen. Even if it were ever reported, owners[0] is never assigned, so
> the loop below would simply skip it.
Agreed, lets drop it.

> > [ ... 35 lines skipped ... ]
> > --- a/xen/arch/riscv/imsic.c
> > +++ b/xen/arch/riscv/imsic.c
> > @@ -555,12 +555,37 @@ void cf_check imsic_ctxt_switch_from(struct vcpu *p)
> >   
> >       write_lock_irqsave(&imsic_state->vsfile_lock, flags);
> >       imsic_state->vsfile_cpu = p->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.
> "instead of" reads as if HGEIP only reports it while the vCPU isn't
> running. It always does; what changes is that VGEIN no longer selects
> the file and the HGEIE bit set below turns it into an SGEI.
Right, I'll reword it accordingly.

> > [ ... 29 lines skipped ... ]
> >   int imsic_map_guest_file(struct vcpu *v, unsigned int vsfile_id)
> > @@ -695,6 +720,12 @@ static void cf_check imsic_vsfile_local_read_clear(void 
> > *data)
> >       struct imsic_mrif *mrif = idata->mrif;
> >       unsigned long new_hstatus, old_hstatus, old_vsiselect;
> >   
> > +    /*
> > +     * The HGEIE bit imsic_ctxt_switch_from() armed belongs to the old owner
> > +     * only.
> > +     */
> "Old owner" is ambiguous: it reads like another vCPU, but it's the same
> vCPU on the file it is leaving. To mirror imsic_local_hgeie_set(), I'd
> rather have an imsic_local_hgeie_clear() called from here, documenting
> the pairing the same way, e.g.:
>
> /*
>   * Stop observing an interrupt file which a vCPU leaves when it moves to
>   * another pCPU by clearing the bit imsic_ctxt_switch_from() set.
>   *
>   * Counterpart of imsic_local_hgeie_set() for the new file.
>   */
>
> In that case ...
> > +    csr_clear(CSR_HGEIE, BIT(idata->hgei, UL));
> > +
> >       /*
> >        * The file is reached by retargeting hstatus.VGEIN, and
> >        * imsic_vs_csr_swap() alters vsiselect. Both belong to the vCPU running
> > @@ -1044,6 +1075,21 @@ int __init vimsic_make_domu_dt_node(struct kernel_info 
> > *kinfo,
> >       return fdt_end_node(fdt);
> >   }
> >   
> > +/*
> > + * Start to observe the interrupt file from HS-mode, the same way
> > + * imsic_ctxt_switch_from() does it for a vCPU which is switched out.
> > + *
> > + * The counterpart, clearing the bit of the interrupt file which is left
> > + * behind, is done by imsic_vsfile_local_read_clear(), which already runs on
> > + * the pCPU owning that file.
>   ... this wouldn't be needed at all.
Agreed, I'll introduce imsic_local_hgeie_clear() next to
imsic_local_hgeie_set(), with your comment. It'll take the file number
rather than void *, though, as it's called directly from
imsic_vsfile_local_read_clear() and not through imsic_call_on_cpu().

~ Oleksii