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

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

On 9/21/26 5:08 PM, Jan Beulich wrote:
> On 21.09.2026 16:01, Oleksii Kurochko wrote:
>> On 9/18/26 2:52 PM, Jan Beulich wrote:
>>> On 27.08.2026 17:21, Oleksii Kurochko wrote:
>>>> --- 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?
> 
> I think I get what you're trying to explain, but the term "level-sensitive"
> here doesn't really help. I'm unconvinced you actually mean that, as it
> requires pins / physical signals, which don't exist with MSI. It feels like
> you may mean "sticky" instead.

Fair point regarding the terminology: 'level-sensitive' usually implies 
physical signal lines, whereas here MSIs are memory writes latched in 
the file's eip[] array. I meant that HGEIP dynamically reflects whether 
the VS-file currently has any pending-and-enabled interrupt latched.

I'll update the comment to clarify this without using 'level-sensitive':

/*
  * 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.
  *
  * An MSI is recorded in the VS-file's eip[] array until the guest claims
  * it, and HGEIP isn't latched but reflects whether the file currently
  * has a pending-and-enabled interrupt. Hence an interrupt which arrived
  * before this point raises an SGEI as soon as the bit is set in HGEIE;
  * nothing is lost in between.
  */

> 
>>>>    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?
> 
> You get to judge. If the lock typically is uncontended, keeping things
> as-is may indeed be fine. Introducing a bottleneck "just for potential
> future cases" otoh wouldn't look overly nice to me.
> 
For the moment, I prefer to have a lock here as it is expected to be 
cheap. If one day it won't be true anymore it will be easier to spot all 
the places and remove it where it will be necessary.

~ Oleksii