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

On 8/13/26 11:24 AM, Baptiste Le Duc 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.
> 
> 
>>
>> Introduce imsic_update_state() to update a vCPU's guest IMSIC state
>> (the guest interrupt file id and the pCPU whose hardware interrupt
>> file it is mapped to) as a single consistent unit. This state can be
>> read concurrently, e.g. by a future helper that checks whether a
>> vCPU has a pending IMSIC interrupt, though no such consumer exists
>> yet at this stage - so it is protected by a lock.
>>
>> Signed-off-by: Oleksii Kurochko <[email protected]>
>>
>> diff --git a/xen/arch/riscv/aia.c b/xen/arch/riscv/aia.c
>> index 4f7f46f58f..ed19600d46 100644
>> --- a/xen/arch/riscv/aia.c
>> +++ b/xen/arch/riscv/aia.c
>> @@ -14,6 +14,7 @@
>>   #include <asm/cpufeature.h>
>>   #include <asm/csr.h>
>>   #include <asm/current.h>
>> +#include <asm/imsic.h>
>>   
>>   struct vgein_ctrl {
>>       unsigned long bmp;
>> @@ -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);
>> +
>> +    /*
>> +     * 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 )
>> +    {
>> +        /* Can't continue w/o correctly mapped IMSIC interrupt file */
>> +        domain_crash(v->domain);
> 
> The vgein id assigned a few lines up is not released here. The domain is dying
> anyway, but the guest interrupt file stays marked in use on that pCPU forever,
> since nothing else ever calls vgein_release(). A vgein_release(v,
> new_vsfile_id) before the domain_crash() would fix it.
> 

Yes, agree with that vgein_release() is missed. I've mentioned that 
before in reply to Jan B.

Thanks!

~ Oleksii