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

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

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