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

[Oleksii Kurochko <oleksii.kurochko@gmail.com>]

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