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

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?

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

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

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

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

Jan