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

[Jan Beulich <jbeulich@xxxxxxxx>]

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/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);

It's somewhat better this way, yes. And as said - in not looking great to
me (doc-wise) is likely an issue of mine, not of yours.

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

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

Jan