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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 10.03.2026 18:08, Oleksii Kurochko wrote:
> 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

Isn't use of such an optimization coming prematurely? Shouldn't this series
focus on getting basic functionality in place?

> --- a/xen/arch/riscv/aia.c
> +++ b/xen/arch/riscv/aia.c
> @@ -1,11 +1,24 @@
>  /* SPDX-License-Identifier: GPL-2.0-only */
>  
> +#include <xen/bitmap.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>
> +
> +/*
> + * Bitmap for each physical cpus to detect which VS (guest)
> + * interrupt file id was used.
> + */
> +DEFINE_PER_CPU(struct vgein_bmp, vgein_bmp);

Why can this not be static? All management looks to be in this same file.

> @@ -14,12 +27,109 @@ bool aia_available(void)
>      return is_aia_available;
>  }
>  
> +int __init vgein_init(unsigned int cpu)

If this needs invoking once per CPU being brought up, it can't be __init.

Also - static?

> +{
> +    struct vgein_bmp *vgein = &per_cpu(vgein_bmp, cpu);
> +
> +    csr_write(CSR_HGEIE, -1UL);
> +    vgein->geilen = flsl(csr_read(CSR_HGEIE));
> +    csr_write(CSR_HGEIE, 0);
> +    if ( vgein->geilen )
> +        vgein->geilen--;

I don't understand this. The "len" in "geilen" stands for "length", I suppose,
i.e. the number of bits. Hmm, the spec itself is inconsistent: "The number of
bits implemented in hgeip and hgeie for guest external interrupts is UNSPECIFIED
and may be zero. This number is known as GEILEN." This may or may not include
bit 0 (which is implemented, but r/o zero). Then saying "Hence, if GEILEN is
nonzero, bits GEILEN:1 shall be writable in ..." suggests 0 isn't included, but
that's not unambiguous.

Anyway, may I suggest

    vgein->geilen = flsl(csr_read(CSR_HGEIE) >> 1);

instead?

> +    BUG_ON(!vgein->geilen);

You can return (an error, but see the respective remark on the earlier patch),
no need to crash the system. That return may want to come after the printk()
below, though.

> +    printk("cpu%d.geilen=%d\n", cpu, vgein->geilen);

As before - %u please with unsigned int.

> +    if ( !vgein->bmp )

Why would this check be needed?

> +    {
> +        vgein->bmp = xvzalloc_array(unsigned long, 
> BITS_TO_LONGS(vgein->geilen));

With the determination above, isn't BITS_TO_LONGS(vgein->geilen) ==
BITS_PER_LONG in all cases? Surely you don't mean to runtime-allocate
space for a single unsigned long? So I wonder is the dimension used
is wrong.

If it isn't, dynamically allocating the owners array may be more
useful, as (on RV64) occupies a fixed 512 bytes right now.

> +        if ( !vgein->bmp )
> +            return -ENOMEM;
> +    }
> +
> +    spin_lock_init(&vgein->lock);
> +
> +    return 0;
> +}
> +
>  int __init aia_init(void)
>  {
> +    int rc = 0;
> +
>      if ( !riscv_isa_extension_available(NULL, RISCV_ISA_EXT_ssaia) )
>          return -ENODEV;
>  
> +    if ( (rc = vgein_init(0)) )
> +        return rc;
> +
>      is_aia_available = true;

Ah, this answers a question of mine on the earlier patch: This boolean
indicates more than just the extension being available. But why does
the description there not simply say so? How am I as a reviewer supposed
to know?

> -    return 0;
> +    return rc;
> +}
> +
> +unsigned int vgein_assign(struct vcpu *v)
> +{
> +    unsigned int vgein_id;
> +

Seemingly undue blank line.

> +    struct vgein_bmp *vgein_bmp = &per_cpu(vgein_bmp, v->processor);
> +    unsigned long *bmp = vgein_bmp->bmp;
> +    unsigned long flags;
> +
> +    spin_lock_irqsave(&vgein_bmp->lock, flags);
> +    vgein_id = bitmap_weight(bmp, vgein_bmp->geilen);

How can the ID to use be the number of bits which are set? This only works
if all set bits are contiguous at the bottom.

> +    /*
> +     * All vCPU guest interrupt files are used and we don't support a case
> +     * when number of vCPU on 1 pCPU is bigger then geilen.
> +     */

This wants checking in vgein_init() then. CPUs (beyond the boot one)
violating this should not be brought online.

> +    ASSERT(vgein_id < vgein_bmp->geilen);

What if not bit is available? By asserting, you assume the caller will not
call here when no ID is available. Yet there is no caller of this function,
so how can one verify whether this assertion is appropriate?

> +    bitmap_set(bmp, vgein_id, 1);

__set_bit()?

> +    spin_unlock_irqrestore(&vgein_bmp->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:
> +     *   8.2.1 Hypervisor Status Register (hstatus) in RISC-V priviliged 
> spec:

Please avoid section numbers in such references. The section of this name
in the version I'm looking at is 21.2.1.

> +     *   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.
> +     */
> +    vgein_id++;

Related to my comment regarding GEILEN, this shouldn't be necessary. Keep
bits in their natural positions, and simply avoid using bit 0 (either by
setting it during init and then never clearing it, or by starting the
scan for clear bits at bit 1).

> +#ifdef VGEIN_DEBUG
> +    printk("%s: %pv: vgein_id(%u), xen_cpu%d_bmp=%#lx\n",
> +           __func__, v, vgein_id, v->processor, *bmp);
> +#endif
> +
> +    vcpu_guest_cpu_user_regs(v)->hstatus &= ~HSTATUS_VGEIN;
> +    vcpu_guest_cpu_user_regs(v)->hstatus |=
> +        MASK_INSR(vgein_id, HSTATUS_VGEIN);

When is this function going to be invoked? (As before, not knowing this is
one of the problems with introducing functions with no callers.)

> +    return vgein_id;
> +}
> +
> +void vgein_release(struct vcpu *v, unsigned int vgen_id)
> +{
> +    unsigned long flags;
> +

Another seemingly stray blank line.

> +    struct vgein_bmp *vgein_bmp = &per_cpu(vgein_bmp, v->processor);
> +
> +    spin_lock_irqsave(&vgein_bmp->lock, flags);
> +    bitmap_clear(vgein_bmp->bmp, vgen_id - 1, 1);

__clear_bit()?

> +    spin_unlock_irqrestore(&vgein_bmp->lock, flags);
> +
> +#ifdef VGEIN_DEBUG
> +    printk("%s: vgein_id(%u), xen_cpu%d_bmp=%#lx\n",
> +           __func__, vgen_id, v->processor, *vgein_bmp->bmp);

I can't spot a difference from the message in vgein_assign(). How is one
to distinguish the two in a log?

> --- a/xen/arch/riscv/include/asm/aia.h
> +++ b/xen/arch/riscv/include/asm/aia.h
> @@ -3,8 +3,26 @@
>  #ifndef ASM__RISCV__AIA_H
>  #define ASM__RISCV__AIA_H
>  
> +#include <xen/percpu.h>
> +#include <xen/spinlock.h>
> +
> +struct vcpu;
> +
> +struct vgein_bmp {

What does the _bmp suffix indicate here? There's ...

> +    unsigned long *bmp;

... a bitmap field, yes, but ...

> +    spinlock_t lock;
> +    struct vcpu *owners[BITS_PER_LONG];
> +    unsigned int geilen;
> +};

... the structure as a whole has quite a bit more.

Jan