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

[Baptiste Le Duc <baptiste.le-duc@xxxxxxxxxx>]

> 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.


> [ ... 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.
> [ ... 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.
> [ ... 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.

> [ ... 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.

-- 
Baptiste Le Duc <baptiste.le-duc@xxxxxxxxxx>