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

[Jan Beulich <jbeulich@xxxxxxxx>]

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?

> @@ -14,10 +36,132 @@ bool aia_usable(void)
>      return is_aia_usable;
>  }
>  
> +static int vgein_init(unsigned int cpu)
> +{
> +    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%d.geilen=%u\n", cpu, vgein->geilen);

%u please with unsigned int.

> +    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%un", cpu);

Looks like there's \ missing ahead of the trailing 'n'.

> +        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)) )
> +    {
> +        dprintk(XENLOG_ERR, "vgein_init() failed with rc(%d)\n", rc);

Messages like this one, provided they're really needed, should be yet more
terse imo: "vgein_init() failed: %d\n".

>          return;
> +    }
>  
>      is_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);
> +    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?

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

Jan