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

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

On 7/27/26 5:41 PM, Jan Beulich wrote:
> On 20.07.2026 18:02, Oleksii Kurochko 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.
> 
> And when exactly is this "assignment" intended to occur? vgein_assign() and
> vgein_release() have no callers here, so this remains entirely unclear.

[A] Agreed, I should have added that information to the commit message:

VGEIN is assigned (via vgein_assign()) before jumping to the new vCPU 
execution context (in continue_new_vcpu()) and is re-assigned during 
vCPU migration from one pCPU to another.

VGEIN is released (via vgein_release()) on the old pCPU during migration.


> 
>> @@ -14,10 +36,133 @@ bool aia_usable(void)
>>       return _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%u.geilen=%u\n", cpu, vgein->geilen);
> 
> At most dprintk(), I'd say. Better drop altogether.

I will drop it.

> 
>> +    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,
> 
> Nit: Line length.
> 
>> +                        void *hcpu)
> 
> Nit: Indentation.
> 
>> +{
>> +    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);
>> +}
> 
> Where's the freeing of the allocation vgein_init(), when CPU bringup fails
> or a CPU was brought down?
I'll add the following:

case CPU_UP_CANCELED:
case CPU_DEAD:
     vgein_free(cpu);
     break;

and:

static void vgein_free(unsigned int cpu)
{
     struct vgein_ctrl *vgein = &per_cpu(vgein, cpu);

     ASSERT(!vgein->bmp);

     vgein->geilen = 0;
     XVFREE(vgein->owners);
}

I'm also wondering whether vgein_init() should be moved to 
CPU_UP_PREPARE. If vgein_init() fails in CPU_STARTING, the hypervisor 
will stop instead of simply ignoring the CPU.

However, in CPU_UP_PREPARE we don't yet know the value of GEILEN, which 
is needed to allocate vgein->owners. As I understand it, CPU_UP_PREPARE 
is not executed on the CPU that is being brought up.


> 
>> +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: %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);
>> +    unsigned long *bmp = &vgein->bmp;
>> +    unsigned long flags;
>> +
>> +    if ( !vgein->geilen )
>> +        return 0;
>> +
>> +    spin_lock_irqsave(&vgein->lock, flags);
> 
> Because it's unclear where this is to be called from, it's also unclear whether
> a lock is needed here (and if so whether a plain spin lock is appropriate).

Based on what I wrote in [A] above a lock is defintely needed as it 
could be that vgein_release() is called for old pCPU during migration 
and at the same time old pCPU could call vgein_assign() so we want to 
keep vgein bitmap consistent.

Regarding why _irqsave() it is mostly connected to ...

> 
>> +    /*
>> +     * 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
>> +    {
>> +        __set_bit(vgein_id, bmp);
>> +        vgein->owners[vgein_id] = v;
> 
> Again somewhat related to is being unclear how the function is going to be used,
> it also remains unclear what ->owners[] is going to be needed for. Right now the
> array is only ever written to.

->owners[] is used in IRQ context to wake up a vCPU. For example, if a 
vCPU has been descheduled, we need to set the corresponding CSR_HGEIE[] 
bit so that when an interrupt associated with that vCPU occurs, it traps 
into the hgei_interrupt() handler, which then wakes the vCPU. (all of 
that isn't introduced now but I thought it would be useful to track 
->owners[] just from the start).

Since ->owners[] is accessed from both IRQ-safe (hgei_interrupt()) and 
IRQ-unsafe (vCPU migration) contexts, we specifically need the 
_irqsave() variant of the lock.

To make this clearer, I'll add the following to the commit message (if 
that helps):
```
Along with the bitmap, track which vCPU owns each guest interrupt file
id. Nothing consumes this yet, but it is filled in from the start as
the owner is what a guest external interrupt handler needs: a guest
interrupt file stays enabled in hgeie while its vCPU is descheduled, so
an interrupt targeting that file traps to Xen, which then has to find
the vCPU it belongs to in order to wake it up.

While the tracking is per-CPU data, it isn't accessed only locally: a
guest interrupt file belongs to the pCPU a vCPU is going to run on, so
it is allocated and released by whichever CPU is handling the vCPU at
the time: the release side is even passed the target CPU explicitly.
Hence a lock is needed. It has to be the IRQ-safe variant, as the
tracking is also going to be read from interrupt context on the CPU
owning it.
```
> 
>> +    }
>> +
>> +    spin_unlock_irqrestore(&vgein->lock, flags);
>> +
>> +#ifdef VGEIN_DEBUG
>> +    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;
> 
> If already you track the vCPU, also assert that prior to clearing the array
> slot it has the expected value? For the bit being cleared, maybe also
> 
>      if ( !__test_and_clear_bit(vgein_id, &vgein->bmp) )
>          ASSERT_UNREACHABLE();
> 
> ? Yet as said - much remains unclear without knowing how all of this is
> meant to be used.

It makes sense. I will add that.

Thanks.

~ Oleksii