Re: [PATCH v1 07/17] xen/riscv: introduce vCPU AIA initialization

On 8/6/26 4:56 PM, Jan Beulich wrote:
> On 20.07.2026 18:02, Oleksii Kurochko wrote:
>> Introduce vcpu_aia_init() to initialize the AIA-related state needed
>> for a vCPU to have a working guest interrupt file.
>>
>> A guest (VS) interrupt file must be mapped to one of a pCPU's
>> hardware interrupt files (if they exist), so the pCPU a vCPU will actually
>> run on needs to be known first. arch_vcpu_create() is therefore not a
>> suitable place to call vcpu_aia_init(), since the pCPU assigned to a
>> vCPU can still change before it is first scheduled. To avoid
>> reassigning the VS interrupt file id and remapping it to a different
>> pCPU's hardware interrupt file, vcpu_aia_init() will instead be
>> called from a later point in the scheduling path (e.g.
>> continue_to_new_vcpu()), to be introduced in a follow-up patch. Since
>> it will end up being called from a non-__init context, it is not
>> itself marked __init.
> 
> If it's called during scheduling, perhaps vcpu_aia_init() simply isn't
> an appropriate name, and that issue is then also reflected in a
> misleading patch subject?z
I also thought about that while working on the IMSIC interrupt file 
support, but I was thinking of moving it to imsic.c.

Regarding the function name, a better name would be 
vcpu_imsic_hw_vsfile_attach(). Alternatively, we could use a slightly 
more architectural term, such as HGEI/VGEIN, and call it 
vcpu_imsic_hgei_attach(). I think I prefer vcpu_imsic_hw_vsfile_attach().

Considering your observation and question, it could also be placed where 
it will actually be called from continue_new() in the future, so 
riscv/domain.c might be the right place for it but at the moment I think 
it will be better to put it in imsic.c closer to other IMSIC functionality.

> 
>> @@ -36,6 +37,35 @@ bool aia_usable(void)
>>       return _aia_usable;
>>   }
>>   
>> +void vcpu_aia_init(struct vcpu *v)
>> +{
>> +    unsigned int new_vsfile_id;
>> +    int rc;
>> +
>> +    if ( !aia_usable() )
>> +        return;
>> +
>> +    new_vsfile_id = vgein_assign(v);

I will add here also the check that if new_vsfile_id = 0 then we don't 
need to map guest file.

>> +
>> +    /*
>> +     * vgein_assign() returns 0 when no free h/w guest interrupt file is
>> +     * available (including GEILEN == 0); imsic_map_guest_file() maps nothing
>> +     * in that case.
>> +     */
>> +    rc = imsic_map_guest_file(v, new_vsfile_id);
>> +    if ( rc )
>> +    {

I missed here vgein_release().

>> +        /* Can't continue w/o correctly mapped IMSIC interrupt file */
>> +        domain_crash(v->domain);
>> +        return;
>> +    }
>> +
>> +    vcpu_guest_cpu_user_regs(v)->hstatus |=
>> +        MASK_INSR(new_vsfile_id, HSTATUS_VGEIN);
> 
> Looks like you're assuming that no other ID was previously stored in that
> field. That can't be quite right when the function is called after the
> vCPU moved to a different pCPU.

I don't use it during the migration process as during migration it is a 
little bit different sequence of how all of that inside the function is 
called; I use it only jumping to new vCPU (continue_new_cpu()), where I 
expect ->hstatus.vgein to be zero because of how the area for the vCPU 
registers is allocated, via vzalloc().

Probably I should consider to rework that and make it re-usable for both 
creating/jumping_to_new_vcpu and migration process.

> 
>> --- a/xen/arch/riscv/imsic.c
>> +++ b/xen/arch/riscv/imsic.c
>> @@ -83,6 +83,19 @@ unsigned int vcpu_guest_file_id(const struct vcpu *v)
>>       return ACCESS_ONCE(v->arch.vimsic_state->guest_file_id);
>>   }
>>   
>> +void imsic_update_state(struct vcpu *v, unsigned int guest_file_id)
>> +{
>> +    unsigned long flags;
>> +    struct vimsic_state *vimsic_state = v->arch.vimsic_state;
>> +    unsigned long pcpu = ( !guest_file_id ) ?
>> +                         NR_CPUS : cpuid_to_hartid(v->processor);
> 
> "pcpu" as a name is misleading when what you store is a hart ID. NR_CPUS
> then also isn't a suitable sentinel.
> 

Agree. I will store here v->processor and NR_CPUS if s/w interrupt file 
is used and then use cpuid_to_hartid() when it will be necessary.

> Also, style nit: The parentheses aren't really needed around the conditional.
> But what's definitely wrong are the blanks immediately inside them.

I will deal with that.

> 
>> +    write_lock_irqsave(&vimsic_state->vsfile_lock, flags);
>> +    vimsic_state->guest_file_id = guest_file_id;
>> +    vimsic_state->vsfile_pcpu = pcpu;
> 
> By implication from the remark above, the field name stored into then also
> is misnamed.

I think with the suggested changed above here everything will be fine.

Thanks.

~ Oleksii