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

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 8/10/26 3:32 PM, Baptiste Le Duc 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.
> >
> > Signed-off-by: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>
> >
> > diff --git a/xen/arch/riscv/aia.c b/xen/arch/riscv/aia.c
> > index e31c9c2d24..4f7f46f58f 100644
> > --- 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>
> Add a #include <xen/percpu.h> here instead of in aia.h.
Sorry, but I’m a little confused here. <asm/aia.h> doesn’t include 
<xen/percpu.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;
> > +};
> > +
> > +/*
> > + * VGEIN control structure for each physical CPU to track which VS (guest)
> > + * interrupt file IDs are in use.
> > + */
> > +static DEFINE_PER_CPU(struct vgein_ctrl, vgein);
> >   
> >   static bool __ro_after_init _aia_usable;
> >   
> > @@ -14,10 +36,133 @@ bool aia_usable(void)
> >       return _aia_usable;
> >   }
> >   
> > +static int vgein_init(unsigned int cpu)
> Could we call this function with a different cpu arg than the current
> one running? If yes, we would read hgeie of not the cpu we wanted.
Considering that it touches the CSR_HGIEI register, it can only be 
called on the currently running CPU.
That’s why I suggested in one of my replies to Jan B. that I would drop 
the argument altogether for this function.
> > +{
> > +    struct vgein_ctrl *vgein = &per_cpu(vgein, cpu);
> > +
> > +    csr_write(CSR_HGEIE, -1UL);
> > +    vgein->geilen = flsl(csr_read(CSR_HGEIE) >> 1);
> > +    csr_write(CSR_HGEIE, 0);
> > +
> > +    printk("cpu%u.geilen=%u\n", cpu, vgein->geilen);
> > +
> > +    if ( !vgein->geilen )
> > +        return -EOPNOTSUPP;
> > +
> > +    vgein->owners = xvzalloc_array(struct vcpu *, vgein->geilen);
> > +    if ( !vgein->owners )
> > +        return -ENOMEM;
> > +
> > +    spin_lock_init(&vgein->lock);
> > +
> > +    return 0;
> > +}
> > +
> > +static int cf_check cpu_callback(struct notifier_block *nfb, unsigned long 
> > action,
> > +                        void *hcpu)
> > +{
> > +    unsigned int cpu = (unsigned long)hcpu;
> > +    int rc = 0;
> > +
> > +    switch ( action )
> > +    {
> > +    case CPU_STARTING:
> > +        rc = vgein_init(cpu);
> > +        if ( rc )
> > +            printk("AIA: failed to init vgein for CPU%u\n", cpu);
> > +        break;
> > +    }
> > +
> > +    return notifier_from_errno(rc);
> > +}
> > +
> > +static struct notifier_block cpu_nfb = {
> > +    .notifier_call = cpu_callback,
> > +};
> > +
> >   void __init aia_init(void)
> >   {
> > +    int rc;
> > +
> >       if ( !riscv_isa_extension_available(NULL, RISCV_ISA_EXT_ssaia) )
> > +    {
> > +        dprintk(XENLOG_WARNING, "SSAIA isn't present in riscv,isa\n");
> >           return;
> > +    }
> > +
> > +    if ( (rc = vgein_init(0)) )
> Why `0` rather than smp_processor_id()? As described above vgein_init() reads 
> CSR_HGEIE
> of the current hart but stores the result into per_cpu(vgein, cpu), so the two
> must agree.
aia_init() is executed on boot cpu only so it uses 0 as Xen boot cpu is 
always 0. But it won't be an issue anymore as I mentioned above an 
argument of vgein_init() will be dropped anyway so it will be guaranteed 
that a correct CPU is used.
> > +    {
> > +        dprintk(XENLOG_ERR, "vgein_init() failed: %d\n", rc);
> > +        return;
> > +    }
> >   
> >       _aia_usable = true;
> > +
> > +    register_cpu_notifier(&cpu_nfb);
> > +}
> > +
> > +unsigned int vgein_assign(struct vcpu *v)
> > +{
> > +    unsigned int vgein_id;
> > +    struct vgein_ctrl *vgein = &per_cpu(vgein, v->processor);
> What happens if v->processor change between vgein_assign() and
> vgein_release? Because it seems in such case the release will hit a
> different pCPU's bitmap: the original bit will leak and an unrelated
> CPU's bit will be cleared under another vCPU's feet.
So, if v->processor changes between the calls to vgein_assign() and 
vgein_release(), it means that migration has happened. If migration has 
happened, then it is the responsibility of the migration code to 
properly assign the new vgein and release the previous one.
All other cases where vgein_release() is called are when the vCPU is 
dying, so everything is okay there as migration cannot happen.

> > +    unsigned long *bmp = &vgein->bmp;
> > +    unsigned long flags;
> > +
> > +    if ( !vgein->geilen )
> > +        return 0;
> > +
> > +    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 privileged spec:
> > +     *   Hypervisor Status Register (hstatus) in RISC-V privileged 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
> > +    {
> Potential index error, because above you did:
>
>      vgein->owners = xvzalloc_array(struct vcpu*, vgein->geilen)
>
> so valid index are 0...(vgein->geilen-1). Adopt either
> one of those two options:
>      1. vgein->owners[vgein_id-1] = v
>      2. vgein->owners = xvzalloc_array(struct vcpu *, vgein->geilen+1) in
>         vgein_init()
>
> I think `2` could be better to have vgein->owners replicated hgeie CSR but
> it would left the first entry read-only.
I've found that too during prepare a reply to Jan B. so fixed it already 
in v2. I've decided to go with what you suggested in 2.
> > +        __set_bit(vgein_id, bmp);
> > +        vgein->owners[vgein_id] = v;
> > +    }
> > +
> > +    spin_unlock_irqrestore(&vgein->lock, flags);
> > +
> > +#ifdef VGEIN_DEBUG
> VGEIN_DEBUG is not defined anywhere in the patch, please use
> gdprintk(XENLOG_DEBUG, ...) directly, or drop this branch.
It is intentionally not defined. If a user needs additional VGEIN debug 
information, they should define it themselves, as it can produce a 
pretty large amount of logs due to, for example, the migration process, 
where vgein_assign() and vgein_release() are used quite actively.
> > +    gprintk(XENLOG_DEBUG, "%s: %pv: vgein_id(%u), xen_cpu%u_bmp=%#lx\n",
> > +           __func__, v, vgein_id, v->processor, *bmp);
> > +#endif
> > +
> > +    return vgein_id;
> > +}
> > +
> > +void vgein_release(struct vcpu *v, unsigned int vgein_id)
> > +{
> > +    unsigned long flags;
> > +    struct vgein_ctrl *vgein = &per_cpu(vgein, v->processor);
> > +
> > +    if ( !vgein_id )
> > +        return;
> > +
> > +    spin_lock_irqsave(&vgein->lock, flags);
> > +    __clear_bit(vgein_id, &vgein->bmp);
> > +    vgein->owners[vgein_id] = NULL;
> > +    spin_unlock_irqrestore(&vgein->lock, flags);
> > +
> > +#ifdef VGEIN_DEBUG
> > +    gprintk(XENLOG_DEBUG, "%s: vgein_id(%u), xen_cpu%u_bmp=%#lx\n",
> > +           __func__, vgein_id, v->processor, vgein->bmp);
> > +#endif
> >   }
> > diff --git a/xen/arch/riscv/include/asm/aia.h b/xen/arch/riscv/include/asm/aia.h
> > index aaa4bf91fc..c67be0069a 100644
> > --- a/xen/arch/riscv/include/asm/aia.h
> > +++ b/xen/arch/riscv/include/asm/aia.h
> > @@ -3,8 +3,16 @@
> >   #ifndef RISCV_AIA_H
> >   #define RISCV_AIA_H
> >   
> > +#include <xen/percpu.h>
> asm/aia.h needs neither <xen/percpu.h> nor <xen/spinlock.h> as struct
> vgein_ctrl and the per-CPU variable both live in aia.c. Please drop them
> and add <xen/percpu.h> in aia.c
Yes, it is redundant code that I missed removing. I’ve already noticed 
it and removed it in v2.
Thanks.

~ Oleksii