Re: [PATCH v2 12/26] xen/riscv: add basic VGEIN management for AIA guests

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 5/21/26 5:11 PM, Jan Beulich wrote:
> On 08.05.2026 16:43, Oleksii Kurochko wrote:
> > --- a/xen/arch/riscv/aia.c
> > +++ b/xen/arch/riscv/aia.c
> > @@ -1,11 +1,33 @@
> >   /* SPDX-License-Identifier: GPL-2.0-only */
> >   
> > +#include <xen/bitmap.h>
> > +#include <xen/cpu.h>
> >   #include <xen/errno.h>
> >   #include <xen/init.h>
> >   #include <xen/sections.h>
> > +#include <xen/sched.h>
> > +#include <xen/spinlock.h>
> >   #include <xen/types.h>
> > +#include <xen/xvmalloc.h>
> >   
> > +#include <asm/aia.h>
> >   #include <asm/cpufeature.h>
> > +#include <asm/csr.h>
> > +#include <asm/current.h>
> > +
> > +struct vgein_ctrl {
> > +    unsigned long bmp;
> > +    spinlock_t lock;
> > +    struct vcpu **owners;
> > +    /* The least-significant bits are implemented first, apart from bit 0 */
> > +    unsigned int geilen;
> > +};
> > +
> > +/*
> > + * Bitmap for each physical cpus to detect which VS (guest)
> > + * interrupt file id was used.
> > + */
> > +static DEFINE_PER_CPU(struct vgein_ctrl, vgein);
> Why "Bitmap" in the comment?
Hmm, good question. Looks like rudiment from initial implementation.

I will rephrase it to:
/*
 * VGEIN control structure for each physical CPU to track which VS (guest)
 * interrupt file IDs are in use.
 */

> > +
> > +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;
> > +
> > +    spin_lock_irqsave(&vgein->lock, flags);
> > +    /*
> > +     * The vgein_id shouldn't be zero, as it will indicate that no guest
> > +     * external interrupt source is selected for VS-level external interrupts
> > +     * according to RISC-V priviliged spec:
> > +     *   Hypervisor Status Register (hstatus) in RISC-V priviliged spec:
> > +     *
> > +     *   The VGEIN (Virtual Guest External Interrupt Number) field selects
> > +     *   a guest external interrupt source for VS-level external interrupts.
> > +     *   VGEIN is a WLRL field that must be able to hold values between zero
> > +     *   and the maximum guest external interrupt number (known as GEILEN),
> > +     *   inclusive.
> > +     *   When VGEIN=0, no guest external interrupt source is selected for
> > +     *   VS-level external interrupts.
> > +     *
> > +     * So start to search from bit number 1.
> > +     */
> > +    vgein_id = find_next_zero_bit(bmp, vgein->geilen + 1, 1);
> > +
> > +    if ( vgein_id > vgein->geilen )
> > +        vgein_id = 0;
> > +    else
> > +        __set_bit(vgein_id, bmp);
> > +
> > +    spin_unlock_irqrestore(&vgein->lock, flags);
> > +
> > +#ifdef VGEIN_DEBUG
> > +    gprintk(XENLOG_DEBUG, "%s: %pv: vgein_id(%u), xen_cpu%d_bmp=%#lx\n",
> > +           __func__, v, vgein_id, v->processor, *bmp);
> %d vs unsigned int again (and then yet again further down).
>
> > +#endif
> > +
> > +    vcpu_guest_cpu_user_regs(v)->hstatus &= ~HSTATUS_VGEIN;
> Is this needed when vgein_release() also does it?
Considering how ->hstatus is initialized I agree that we don't need that 
here.
> > +    vcpu_guest_cpu_user_regs(v)->hstatus |=
> > +        MASK_INSR(vgein_id, HSTATUS_VGEIN);
> > +
> > +    return vgein_id;
> > +}
> > +
> > +void vgein_release(struct vcpu *v, unsigned int vgen_id)
> > +{
> > +    unsigned long flags;
> > +    struct vgein_ctrl *vgein = &per_cpu(vgein, v->processor);
> > +
> > +    if ( !vgen_id )
> > +        return;
> > +
> > +    spin_lock_irqsave(&vgein->lock, flags);
> > +     __clear_bit(vgen_id, &vgein->bmp);
> > +    spin_unlock_irqrestore(&vgein->lock, flags);
> > +
> > +#ifdef VGEIN_DEBUG
> > +    gprintk(XENLOG_DEBUG, "%s: vgein_id(%u), xen_cpu%d_bmp=%#lx\n",
> > +           __func__, vgen_id, v->processor, vgein->bmp);
> > +#endif
> > +
> > +    vcpu_guest_cpu_user_regs(v)->hstatus &= ~HSTATUS_VGEIN;
> >   }
> Overall: How is one to review these two functions, when it's entirely
> unclear where they're going to be called from? Among other aspects it
> doesn't become clear what the behavior is going to be when
> vgein_assign() doesn't find an available ID. I've therefore only
> commented on mechanical aspects I noticed.
It is used in "[PATCH v2 14/26] xen/riscv: add very early virtual APLIC 
(vAPLIC) initialization support" from this patch series. I realize that 
was not obvious, sorry for the omission.
That said, I am planning to move all AIA-related logic out of 
vcpu_vaplic_init() and into continue_new_vcpu(), because some 
IMSIC-related operations require knowing which physical CPU the vCPU 
will be scheduled on, and moving the work there avoids redundant 
recalculation. As a result, vgein_assign() would no longer be called 
from vcpu_vaplic_init(), so I could drop this patch from the series 
entirely and reintroduce it when it is actually needed.
Alternatively, I can keep the patch and extend the commit message to 
explain the intended call site and how the return value will be handled.
Thanks.

~ Oleksii