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

[Oleksii Kurochko <oleksii.kurochko@gmail.com>]

On 4/2/26 12:03 PM, Jan Beulich wrote:
> 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?

At the moment, we don't support only APLIC for guest interrupts as it 
will require trap-and-emulation approach, so just from the start it was 
decided to go with APLIC+IMSIC (IMSIC here as it only one interrupt 
controller which exist and support VGEIN stuff at the momemnt) approach 
and then when it will be needed back to only the case when APLIC is 
supported.

Maybe, it was better to introduce in patch series where a lauching of 
domain actually happens.

Considering that you've already made a review, I prefer then to have 
this patch part of this patch series.

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

It could be, it couldn't be before when I have vgein/hgei interrupt 
handler in traps but after I decided to move it to aia.c, it looks like 
it is fine to make it static.

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

Yes, it is going to be called inside the secondary CPU bring-up function.

__init sections are removed much later, after all CPUs are brought up, 
so it looks like that at the moment when secondary CPUs are being 
brought up, __init still exists and can be called.

> 
> Also - static?

It isn't static because it will be called inside the secondary CPU 
bring-up function.

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

But they explicitly wrote that: The least-significant bits are 
implemented first, apart from bit 0. So bit 0 is explicitly excluded.

> 
> Anyway, may I suggest
> 
>      vgein->geilen = flsl(csr_read(CSR_HGEIE) >> 1);
> 
> instead?

It would be really better.

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

Hm, I can't remember why I did so. You are right there is no any sense 
to allocate a single unsinged long in runtime...

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

Agree, it make sense it will be much less memory if to allocate like:
     vgein->owners = xvzalloc_array(struct vcpu *, vgein->geilen);
     if ( !vgein->owners )
         return -ENOMEM;

as maximum value of vgein->geilen is 63.

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

I relised that it is worse to mention that only when saw your reply in 
the earlier patch, I will update the commit message for convience.

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

Oh, it is really wrong. find_first_zero_bit() should be instead or
vgein_id = find_next_zero_bit(bmp, vgein_bmp->geilen + 1, 1);

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

It'll be nice. But we can't know how many vCPUs will be ran on pCPU when 
vgein_init() is executed.

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

It is just a temporary ASSERT() (as we don't support software guest 
interrupt files) because in general it is fine if there is no bit 
available, it will just mean that that no physical hardware guest 
interrupt file is assigned to the virtual hart, and software-based 
emulation (a "software file") must be used to handle guest external 
interrupts.

Will it be better to return 0 now here and just don't create a vCPU
on ...

  Yet there is no caller of this function,
> so how can one verify whether this assertion is appropriate?

... the caller side when an assignment is expected to be happen?


> 
>> +    bitmap_set(bmp, vgein_id, 1);
> 
> __set_bit()?

I thought that it will be fine to use for bmp, bitmap_* functions(). 
__set_bit is what is called inside bitmap_set().


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

vgein_assign() function is going to be invoked during the call of 
arch_vcpu_create().

I also thought to make vgein_assign() just work with vgein_id and just 
return vgein_id and fill v->hstatus on the caller side. It looks a 
little bit cleaner from some point of view.
It is still need to return vgein_id as it is needed for IMSIC's guest 
interrupt file address calculation.

> 
>> +    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()?
> 

The same as with bitmap_set() as ->bmp is bitmap I expect that 
bitmap_*() functions should be used. But just to avoid extra if() inside 
bitmap_clear(), I will use __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?

By function name which is the first argument (__func__).

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

Agree, there is no any sense for _bmp. It would be better to use _ctrl.

Also, I will move this struct to aia.c. Then it also make sense to 
rename vgein_vmp variable just to vgein.

Thanks.

~ Oleksii