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

On 29.07.2026 16:55, Oleksii Kurochko wrote:
> 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.

That is, state of that vCPU is held in hardware for perhaps an extended
period of time after the vCPU was last de-scheduled. That's a fair
optimization (we do something similar on x86, albeit that has been
increasingly under question lately). However, doesn't this then require
sync_local_execstate() to become non-empty?

Furthermore, rather than having vgein_assign() fail when
find_next_zero_bit() fails to find an available ID, shouldn't you release
some other vCPU's ID, making it available for re-use?

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

Does this hold in all cases? What migrates vCPU-s off of a pCPU going down?
IOW aren't you introducing an ordering problem between your notifier handler
and the scheduler's?

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

But there's an upper bound, isn't there? Use that for preliminary allocation,
and re-alloc (best effort) from CPU_ONLINE?

Yet then I continue to question the presence of this array in the first place.
Something similar isn't needed elsewhere (afaik), and its intended use (as
said) doesn't become obvious here.

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

Can this really happen? It almost sounds as if you were suspecting
context-switch-in could race with context-switch-out. Yet again - none of
this can sensibly be discussed without seeing how / where the functions are
to be used.

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

Why the mention of _irqsave? My use of "plain spinlock" was meant to contrast
to the possible use of an r/w lock.

Jan