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

[Baptiste Le Duc <baptiste.le-duc@xxxxxxxxxx>]

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

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

> [ ... 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.
       */

> [ ... 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()? 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. Since both
callers now do the same lock/check/generate/write sequence, a helper for
it would make sense.

-- 
Baptiste Le Duc <baptiste.le-duc@xxxxxxxxxx>