Re: [PATCH v1 02/17] xen/riscv: add basic VGEIN management for AIA guests

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 7/30/26 6:03 PM, Jan Beulich wrote:
> On 30.07.2026 17:46, Oleksii Kurochko wrote:
> > On 7/30/26 9:42 AM, Jan Beulich wrote:
> > > On 29.07.2026 16:55, Oleksii Kurochko wrote:
> > > > On 7/27/26 5:41 PM, Jan Beulich wrote:
> > > > > On 20.07.2026 18:02, Oleksii Kurochko wrote:
> > > > > > It was decided to add support for IMSIC from the start instead of having APLIC
> > > > > > operate in direct delivery mode, as it requires a trap-and-emulation approach,
> > > > > > which is not optimal from a performance standpoint.
> > > > > >
> > > > > > AIA provides a hardware-accelerated mechanism for delivering external
> > > > > > interrupts to domains via "guest interrupt files" located in IMSIC.
> > > > > > A single physical hart can implement multiple such files (up to GEILEN),
> > > > > > allowing several virtual harts to receive interrupts directly from hardware.
> > > > > >
> > > > > > Introduce per-CPU tracking of guest interrupt file identifiers (VGEIN)
> > > > > > for systems implementing AIA specification. Each CPU maintains
> > > > > > a bitmap describing which guest interrupt files are currently in use.
> > > > > >
> > > > > > Add helpers to initialize the bitmap based on the number of available
> > > > > > guest interrupt files (GEILEN), assign a VGEIN to a vCPU, and release it
> > > > > > when no longer needed. When assigning a VGEIN, the corresponding value
> > > > > > is written to the VGEIN field of the guest hstatus register so that
> > > > > > VS-level external interrupts are delivered from the selected interrupt
> > > > > > file.
> > > > > And when exactly is this "assignment" intended to occur? vgein_assign() and
> > > > > vgein_release() have no callers here, so this remains entirely unclear.
> > > > [A] Agreed, I should have added that information to the commit message:
> > > >
> > > > VGEIN is assigned (via vgein_assign()) before jumping to the new vCPU
> > > > execution context (in continue_new_vcpu()) and is re-assigned during
> > > > vCPU migration from one pCPU to another.
> > > >
> > > > VGEIN is released (via vgein_release()) on the old pCPU during migration.
> > > That is, state of that vCPU is held in hardware for perhaps an extended
> > > period of time after the vCPU was last de-scheduled. That's a fair
> > > optimization (we do something similar on x86, albeit that has been
> > > increasingly under question lately). However, doesn't this then require
> > > sync_local_execstate() to become non-empty?
> > IIUC, sync_local_execstate() is needed for the lazy context switch case
> > when switching from vCPUA to the idle vCPU.
> Or when full state is to be obtained for a vCPU, for example.
I assume you're referring to XEN_DOMCTL_getvcpucontext, right?

In general, it seems that sync_local_execstate() is primarily an 
optimization. If lazy switching isn't supported, then every time a vCPU 
is de-scheduled, its state must be fully saved to memory. My 
understanding is that everything will still work correctly, just less 
efficiently.
I'm curious how much this optimization actually helps. How often does it 
happen that a vCPU is de-scheduled from a pCPU and then immediately 
scheduled back onto the same pCPU without any other vCPU being scheduled 
in between?
I will add to my TODO list that it is nice to use sync_local_execstate() 
in future.
> > > > > > +unsigned int vgein_assign(struct vcpu *v)
> > > > > > +{
> > > > > > +    unsigned int vgein_id;
> > > > > > +    struct vgein_ctrl *vgein = &per_cpu(vgein, v->processor);
> > > > > > +    unsigned long *bmp = &vgein->bmp;
> > > > > > +    unsigned long flags;
> > > > > > +
> > > > > > +    if ( !vgein->geilen )
> > > > > > +        return 0;
> > > > > > +
> > > > > > +    spin_lock_irqsave(&vgein->lock, flags);
> > > > > Because it's unclear where this is to be called from, it's also unclear whether
> > > > > a lock is needed here (and if so whether a plain spin lock is appropriate).
> > > > Based on what I wrote in [A] above a lock is defintely needed as it
> > > > could be that vgein_release() is called for old pCPU during migration
> > > > and at the same time old pCPU could call vgein_assign() so we want to
> > > > keep vgein bitmap consistent.
> > > Can this really happen? It almost sounds as if you were suspecting
> > > context-switch-in could race with context-switch-out. Yet again - none of
> > > this can sensibly be discussed without seeing how / where the functions are
> > > to be used.
> > Maybe I didn't explain it clearly, but during migration (which,
> > according to my understanding of vcpu_move_irqs(), is executed on
> > pCPU1), when vCPU0 is migrated from pCPU0 to pCPU1, its old VGEIN on
> > pCPU0 needs to be released. I don't see any reason why, at the same
> > time, pCPU0 could not try to assign that VGEIN to another vCPU. Without
> > proper protection, this could lead to race conditions.
> Doesn't migration of vCPU-s between pCPU-s happen under suitable scheduler
> locks?
If I am not mistaken every path that reaches arch_move_irqs() drops the 
scheduler lock first. The only thing still held at that point is 
sched_res_rculock , and that is an RCU read-side critical section, not 
mutual exclusion: it merely keeps struct sched_resource alive across 
get_sched_res() dereferences, since cpupool/hotplug frees those via 
call_rcu(&sr->rcu, sched_res_free). Any number of pCPUs can be inside it 
concurrently, and it does not disable interrupts, so it serialises 
neither the source pCPU against the destination one nor hgei_interrupt() 
mentioned above against either.
~ Oleksii