Re: [PATCH v2 39/39] xen/riscv: introduce IMSIC h/w interrupt file attaching to vcpu

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 9/25/26 3:11 PM, Baptiste Le Duc wrote:
> > Introduce imsic_vsfile_attach() 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, imsic_vsfile_attach() is called from a
> > later point in the scheduling path (e.g. continue_new_vcpu()). 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.
> I have difficulties to clearly understand the aim of this function. In
> fact, if we want to know if a vCPU has a pending interrupts, wouldn't be
> enough to check hgeip[guest_file_id]?
Lets consider the function which checks pending interrupts:

bool imsic_has_interrupt(const struct vcpu *vcpu)
{
    struct vimsic_state *imsic_state = vcpu->arch.vimsic_state;
    unsigned long flags;
    bool ret = false;

    /*
     * The IMSIC SW-file directly injects interrupt via hvip so
     * only check for interrupt when IMSIC VS-file is being used.
     */

    read_lock_irqsave(&imsic_state->vsfile_lock, flags);
    if ( imsic_state->vsfile_cpu != NR_CPUS )
        ret = !!(csr_read(CSR_HGEIP) & BIT(imsic_state->guest_file_id, 
UL));
    read_unlock_irqrestore(&imsic_state->vsfile_lock, flags);

    return ret;
}

You are right that it is enough hgeip[guest_file_id] but at the same 
time we won't to be sure that guest_file_id wasn't changed during this 
check, right? Otherwise we could accidentally check guest_file_id of 
ohter vCPU.
And so we need imsic_update_state() to have abitility atomically update 
guest_file_id (and also vsfile_pcpu).
I will re-write this part with dropping part of pending interrupts it 
will be just clear from the code of imsic_has_interrupt() when it will 
be needed to be introduced:
```
Implement imsic_update_state(), which records the vCPU's guest
interrupt file id together with the pCPU owning the file. The two are
read as a pair, e.g. by imsic_migrate_vcpu() and the IMSIC context
switch hooks, so they are updated under vsfile_lock as a single unit.
```


> In any case, why does updating vCPU's guest IMSIC state would be
> required?
If it is migrating from one pCPU to another we will need to update 
guest_file_id as on new pCPU guest_file_id could be different from what 
we had for this vCPU on old pCPU + we have to update vsfile_pcpu as a 
new pCPU will be an owner of guest file id of vCPU.
> > Signed-off-by: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>
> >
> > diff --git a/xen/arch/riscv/domain.c b/xen/arch/riscv/domain.c
> > index 0782148b72..15b6bfffa9 100644
> > --- a/xen/arch/riscv/domain.c
> > +++ b/xen/arch/riscv/domain.c
> > @@ -155,6 +155,8 @@ static void continue_new_vcpu(struct vcpu *prev)
> >           reset_stack_and_jump(idle_loop);
> >       else
> >       {
> > +        imsic_vsfile_attach(current);
> > +
> >           /*
> >            * During a context switch to a new vCPU, interrupts must be disabled
> >            * to guarantee that the vCPU's CSR state can be safely restored into
> > diff --git a/xen/arch/riscv/imsic.c b/xen/arch/riscv/imsic.c
> > index 374a21ace1..ad638d7485 100644
> > --- a/xen/arch/riscv/imsic.c
> > +++ b/xen/arch/riscv/imsic.c
> > @@ -212,7 +212,14 @@ unsigned int vcpu_guest_file_id(const struct vcpu *v)
> >   
> >   void imsic_update_state(struct vcpu *v, unsigned int guest_file_id)
> >   {
> > -    BUG_ON("unimplemented\n");
> > +    unsigned long flags;
> > +    struct vimsic_state *vimsic_state = v->arch.vimsic_state;
> > +    unsigned int cpu = guest_file_id ? v->processor : NR_CPUS;
> > +
> > +    write_lock_irqsave(&vimsic_state->vsfile_lock, flags);
> > +    vimsic_state->guest_file_id = guest_file_id;
> > +    vimsic_state->vsfile_cpu = cpu;
> > +    write_unlock_irqrestore(&vimsic_state->vsfile_lock, flags);
> >   }
> >   
> >   void __init imsic_ids_local_delivery(bool enable)
> > @@ -640,6 +647,16 @@ struct imsic_vsfile_data {
> >       struct imsic_mrif *mrif;
> >   };
> >   
> > +/*
> > + * Number of 64-bit EIx groups needed to cover all the interrupt identities an
> > + * IMSIC interrupt file provides, which are 0 (never valid, but it still
> > + * occupies a bit) up to and including imsic_cfg.nr_ids.
> > + */
> > +static unsigned int imsic_nr_eix(void)
> > +{
> > +    return DIV_ROUND_UP(imsic_cfg.nr_ids + 1, BITS_PER_TYPE(uint64_t));
> > +}
> > +
> >   /*
> >    * Execute func() on the pCPU which owns the IMSIC interrupt file func() is
> >    * going to work with.
> > @@ -1138,20 +1155,90 @@ static void cf_check imsic_vsfile_local_update(void 
> > *data)
> >       csr_write(CSR_VSISELECT, old_vsiselect);
> >   }
> >   
> > +/*
> > + * Point the vCPU's HSTATUS.VGEIN at the guest interrupt file it has been
> > + * given. It is applied to the hart when the vCPU's context is restored.
> > + */
> > +static void vcpu_set_vgein(struct vcpu *v, unsigned int vsfile_id)
> > +{
> > +    unsigned long hstatus = vcpu_guest_cpu_user_regs(v)->hstatus;
> > +
> > +    hstatus &= ~HSTATUS_VGEIN;
> > +    hstatus |= MASK_INSR(vsfile_id, HSTATUS_VGEIN);
> > +
> > +    vcpu_guest_cpu_user_regs(v)->hstatus = hstatus;
> > +}
> > +
> > +/*
> > + * Take a h/w guest interrupt file of 'cpu' for the vCPU: zero the file out,
> > + * map it into the domain's G-stage at the vCPU's virtual IMSIC page and
> > + * record the new location in the per-vCPU IMSIC state.
> > + *
> > + * HSTATUS.VGEIN is deliberately left alone: the vCPU may be pointed at the
> > + * file only when the file already holds the vCPU's interrupt state, which in
> > + * the case of imsic_migrate_vcpu() happens only after the old file has been
> > + * moved to the new one. Thereby it is up to the caller to call
> > + * vcpu_set_vgein() at the right moment.
> Nit: would clarify the paragraph here:
>    HSTATUS.VGEIN is deliberately left alone: it must only point at a file
>    which already holds the vCPU's interrupt state. The file taken here is
>    zeroed, so for imsic_migrate_vcpu() VGEIN can only be updated once the
>    state of the old file has been copied into it. Thereby it is up to the
>    caller to call vcpu_set_vgein() at the right moment.
I will use your suggesttion.

> > + *
> > + * Returns the id of the taken interrupt file, or 0 if none could be taken, in
> > + * which case the domain is crashed.
> > + */
> > +static unsigned int imsic_vsfile_acquire(struct vcpu *v, unsigned int cpu)
> > +{
> > +    struct imsic_vsfile_data vsfile_data = { .nr_eix = imsic_nr_eix() };
> > +    unsigned int vsfile_id;
> > +    int rc;
> > +
> > +    vsfile_id = vgein_assign(v);
> > +    if ( !vsfile_id )
> > +    {
> > +        /*
> > +         * vgein_assign() returns 0 when no free h/w guest interrupt file is
> > +         * available. s/w guest interrupt files aren't supported yet, so such
> > +         * a vCPU can't be run.
> > +         */
> > +        domain_crash(v->domain,
> > +                     "%pv: no free h/w guest interrupt file on CPU%u\n",
> > +                     v, cpu);
> > +        return 0;
> > +    }
> > +
> > +    vsfile_data.hgei = vsfile_id;
> > +
> > +    /* The file could still hold the state of its previous owner */
> > +    imsic_call_on_cpu(cpu, imsic_vsfile_local_clear, &vsfile_data);
> > +
> > +    rc = imsic_map_guest_file(v, vsfile_id);
> > +    if ( rc )
> > +    {
> > +        vgein_release(v, vsfile_id, cpu);
> > +
> > +        /* Can't continue w/o correctly mapped IMSIC interrupt file */
> > +        domain_crash(v->domain,
> > +                     "%pv: failed to map h/w guest interrupt file %u: %d\n",
> > +                     v, vsfile_id, rc);
> > +        return 0;
> > +    }
> > +
> > +    imsic_update_state(v, vsfile_id);
> > +
> > +    return vsfile_id;
> > +}
> > +
> >   void imsic_migrate_vcpu(struct vcpu *v)
> >   {
> > -    unsigned int new_vsfile_hgei;
> > +    unsigned int new_vsfile_id;
> >       unsigned int new_vsfile_cpu;
> > -    unsigned int nr_hw_eix = DIV_ROUND_UP(imsic_cfg.nr_ids + 1,
> > -                                          BITS_PER_TYPE(uint64_t));
> > -    struct imsic_vsfile_data vsfile_data = {
> > -        .nr_eix = nr_hw_eix,
> > -    };
> > +    unsigned int nr_hw_eix = imsic_nr_eix();
> >       struct vimsic_state *imsic_state = v->arch.vimsic_state;
> >       unsigned long flags;
> >       unsigned int old_vsfile_id;
> >       unsigned int old_vsfile_cpu;
> >       struct imsic_mrif tmrif = { };
> > +    struct imsic_vsfile_data vsfile_data = {
> > +        .nr_eix = nr_hw_eix,
> > +        .mrif = &tmrif,
> > +    };
> >   
> >       /*
> >        * The scheduler can mark a freshly created vCPU's unit as migrated and
> > @@ -1189,25 +1276,10 @@ void imsic_migrate_vcpu(struct vcpu *v)
> >        */
> >       new_vsfile_cpu = v->processor;
> >   
> > -    new_vsfile_hgei = vgein_assign(v);
> > -
> > -    /* We don't support SW interrupt files at the moment. */
> > -    BUG_ON(!new_vsfile_hgei);
> > -
> > -    vsfile_data.hgei = new_vsfile_hgei;
> > -
> > -    /* Zero-out new IMSIC VS-file */
> > -    imsic_call_on_cpu(new_vsfile_cpu, imsic_vsfile_local_clear, &vsfile_data);
> > -
> > -    /* Update G-stage mapping for the new IMSIC VS-file */
> > -    if ( imsic_map_guest_file(v, new_vsfile_hgei) )
> > -    {
> > -        domain_crash(v->domain, "Migration to hw interrupt file failed\n");
> > -
> > +    /* Zero-out, map and start to use the new IMSIC VS-file */
> > +    new_vsfile_id = imsic_vsfile_acquire(v, new_vsfile_cpu);
> > +    if ( !new_vsfile_id )
> >           return;
> > -    }
> > -
> > -    imsic_update_state(v, new_vsfile_hgei);
> >   
> >       /*
> >        * TODO: Modify the relevant translation tables at all IOMMUs so that MSIs
> > @@ -1245,7 +1317,7 @@ void imsic_migrate_vcpu(struct vcpu *v)
> >       vgein_release(v, old_vsfile_id, old_vsfile_cpu);
> >   
> >       /* Restore register state in the new IMSIC VS-file */
> > -    vsfile_data.mrif = &tmrif;
> > +    vsfile_data.hgei = new_vsfile_id;
> Isn't this already done by imsic_vsfile_acquire()?
It isn't done by imsic_vsfile_acquire() as this function returns new 
vsfile id.
> Couldn't we pass a
> &vsfile_data in their arg so it could fill it instead of allocating one
> in the stack?
We could done in that way but then we will need a stack variable for 
vsfile_data structure in imsic_vsfile_attach() which will consume more 
stack memory then just one local variable for new_vsfile_id currently 
declared in imsic_vsfile_attach(). But I will double-check if it won't 
be really better just refactor something and pass &vsfile_data as you 
suggested.
Thanks.

~ Oleksii