Re: [PATCH v3 28/39] xen/riscv: introduce vaplic_reconfigure_target()

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 10/22/26 3:38 PM, Baptiste Le Duc wrote:
> > When a vCPU is migrated to a different pCPU, its IMSIC guest interrupt
> > file changes. Any APLIC interrupt previously configured to deliver an
> > MSI to the old interrupt file must be retargeted to the new one.
> Jan's first question on v2 was how this avoids losing interrupts that
> are raised mid-retarget. You answered it on the list (step 3 of the AIA
> VS-level migration sequence; in-flight MSIs still land in the old file's
> eip and get merged into the new file later), but neither the description
> nor a comment says so. Please put it in the description, since this
> function is only correct as one step of that sequence.
I will add the following:

This is correct only as part of step 3 of the sequence in the AIA spec's
"Migrating a virtual hart to a different guest interrupt file". Before
it is called, the new file must be cleared, with eidelivery set to 0,
and recorded in the vCPU's IMSIC state, which aplic_msi_target_gen()
takes the new target from. Once it returns, Xen must synchronize with
the APLIC.

That is also why an interrupt raised mid-retarget isn't lost. An MSI
sent before its source's TARGET register is rewritten is latched in the
old file's eip, the synchronization ensures it has arrived there, and
steps 4 and 5 merge that eip into the new file. One sent after the
rewrite lands in the new file and stays pending there until eidelivery
is restored (step 6).


> > [ ... 43 lines skipped ... ]
> > --- a/xen/arch/riscv/aplic.c
> > +++ b/xen/arch/riscv/aplic.c
> > @@ -128,7 +128,10 @@ static unsigned long aplic_hart_index(unsigned int cpu)
> >   /*
> >    * v->processor can't be used here: during a migration it names the new pCPU
> >    * before the interrupt file is moved there. The caller has to hold
> > - * vsfile_lock until the result is written to the h/w.
> > + * vsfile_lock, so that the guest file id and the pCPU owning it are read as
> > + * a consistent pair. A value built from the old file just before a migration
> > + * publishes the new one isn't lost: vaplic_reconfigure_target() runs after
> > + * the publication and rewrites every source targeting the vCPU.
> At this point in the series nothing calls vaplic_reconfigure_target(),
> so "runs after the publication" describes a caller that doesn't exist
> yet. Either move this sentence to the patch that adds the call, or word
> it as a requirement on that caller.
Agreed, I'll word it as a requirement on that caller:

  A value may still be built from the old file just before a migration
  publishes the new one, so whoever publishes a new file has to call
  vaplic_reconfigure_target() afterwards, to rewrite every source
  targeting the vCPU.



> > [ ... 43 lines skipped ... ]
> >
> > +/* Decode the target vCPU from a TARGET snapshot (NULL if none). */
> > +static struct vcpu *vaplic_target_decode_vcpu(const struct domain *d,
> > +                                              uint32_t target)
> > +{
> > +    unsigned int hart_idx = MASK_EXTR(target, APLIC_TARGET_HART_IDX);
> > +
> > +    /*
> > +     * Skip a source whose EIID is 0, e.g. one still holding its reset
> > +     * value: interrupt identity 0 isn't valid at any IMSIC interrupt file,
> > +     * so such a source delivers nothing and needs no retargeting.
> > +     * The hart index range check is only a guard: the write path refuses to
> > +     * record values with an out-of-range hart index.
> > +     */
> "Reset value" reads as if the AIA spec defined one, but it doesn't: APLIC
> state other than domaincfg, the MSI address config registers and the
> genmsi Busy bit is UNSPECIFIED after reset. The 0 here only comes from
> xvzalloc_array() in domain_vaplic_init(). Could this be made explicit?
> It may also be worth saying that this check keeps unprogrammed sources
> from decoding as targeting vCPU0 (hart index 0), which would otherwise
> cause pointless physical TARGET rewrites on every vCPU0 migration.
>
> Something like:
>
>        /*
>         * Skip a source whose EIID is 0 (not yet programmed, vAPLIC state
>         * is zero-initialised): identity 0 is reserved at every IMSIC, so
>         * it can't fire. Otherwise it would also decode as targeting
>         * vCPU0 and get rewritten on every vCPU0 migration.
>         * The hart index range check is only a guard: the write path
>         * refuses to record values with an out-of-range hart index.
>         */
Agreed on both points. I'd word it slightly differently though: the
write path doesn't reject EIID 0, so a guest can program it itself and
"not yet programmed" is only one of the cases; the spec calls identity 0
never valid rather than reserved; and such a source can still become
pending and have its MSI sent, it's the IMSIC that ignores it. So:

    Skip a source whose EIID is 0: interrupt identity 0 is never valid
    at an IMSIC, so such a source delivers nothing and needs no
    retargeting. The AIA spec leaves target[] UNSPECIFIED after reset,
    but the vAPLIC state is zero-initialized, so this also covers every
    source the guest hasn't programmed yet, which would otherwise decode
    as targeting vCPU0 (hart index 0) and have its h/w TARGET register
    needlessly rewritten on every vCPU0 migration.


> > [ ... 6 lines skipped ... ]
> > +void vaplic_reconfigure_target(const struct vcpu *v)
> > +{
> > +    struct domain *d = v->domain;
> > +    struct vaplic *vaplic = to_vaplic(d);
> > +    struct vimsic_state *vimsic_state = v->arch.vimsic_state;
> > +    unsigned int irqn;
> > +
> > +    bitmap_for_each ( irqn, d->arch.vintc->used_irqs, d->arch.vintc->nr_virqs )
> > +    {
> > +        struct vaplic_source *src = &vaplic->regs.sources[irqn];
> > +        unsigned long flags;
> > +        uint32_t target;
> > +
> > +        /* APLIC interrupt sources start from 1, there is no source 0. */
> > +        if ( !irqn )
> > +            continue;
> > +
> > +        spin_lock_irqsave(&src->lock, flags);
> > +
> > +        target = read_atomic(&src->target);
> > +
> > +        if ( vaplic_target_decode_vcpu(d, target) == v )
> > +        {
> > +            /*
> > +             * IRQs are already off under src->lock so no need for *_irqsave
> > +             * here.
> > +             */
> > +            read_lock(&vimsic_state->vsfile_lock);
> > +            aplic_hw_write_reg(offsetof(struct aplic_regs, target[irqn - 1]),
> > +                               aplic_msi_target_gen(v, target));
> Doesn't this need the same vsfile_cpu != CPU_NONE check as
> vaplic_emulate_store()? 
The case with the similar place in vaplic_emulate_store() is slightly 
different, as a guest could write to the target register using a vCPU 
for which the h/w interrupt file hasn't been assigned yet. Since we 
support only h/w interrupt files, we should simply ignore such writes, 
as the vCPU isn't ready for some reason.
> Because it seems that, for a vCPU on a s/w
> VS-file, aplic_msi_target_gen() calls aplic_hart_index(CPU_NONE) and
> indexes imsic->msi[] out of bounds; the ASSERT() in
> aplic_msi_target_gen() doesn't help in release builds.
I think ASSERT() should be sufficient to catch this situation. Since 
software interrupt file support is not included in this patch series, 
vaplic_reconfigure_target() cannot be called for a vCPU with a software 
interrupt file. The ASSERT() in aplic_hw_write_reg() will catch such 
cases. Therefore, even in a release build, we will never encounter a 
vCPU with a software interrupt file so we shouldn't care that ASSERT() 
is do nothing in release built.
When it comes to adding software interrupt file support, debug builds 
will be used during development, so the need to update 
aplic_msi_target_gen() to handle this case properly won't be overlooked.
> Since both
> callers now do the same lock/check/generate/write sequence, a helper for
> it would make sense.
Looking at my current implementation of the software interrupt file 
support, aplic_msi_target_gen() has been updated so that there is no 
longer a need to check vsfile_cpu != CPU_NONE before calling it. 
Therefore, this check will be removed from vaplic_emulate_store() and 
both the places will look the same.
~ Oleksii