Re: [PATCH v2 29/39] xen/riscv: introduce aplic_reconfigure_target()

[Jan Beulich <jbeulich@xxxxxxxx>]

On 27.08.2026 17:21, Oleksii Kurochko 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.
> 
> Implement aplic_reconfigure_target() to scan all interrupts allocated
> to the domain and update their APLIC TARGET registers accordingly.
> 
> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>

First of all I'd like to understand how this "reconfigure" works without
losing interrupts and at the same time without other possible races. An
interrupt can be raised at any time, after all.

> --- a/xen/arch/riscv/aplic.c
> +++ b/xen/arch/riscv/aplic.c
> @@ -138,6 +138,48 @@ uint32_t aplic_msi_target_gen(const struct vcpu 
> *target_vcpu,
>      return base_val;
>  }
>  
> +void aplic_reconfigure_target(const struct vcpu *v,
> +                              unsigned int old_guest_file_id,
> +                              unsigned int old_cpu)
> +{
> +    const struct vintc *vintc = v->domain->arch.vintc;
> +    const unsigned long *auth_irq_bmp = vintc->used_irqs;
> +    unsigned long old_hart_field = aplic_hart_field(old_cpu);

Once again a question you may already recognize: What extra value does
"field" in the variable name add?

> +    unsigned long flags;
> +    unsigned int irqn;
> +
> +    /* Support only MSI mode at the moment */
> +    BUG_ON(!aplic_msi_mode());
> +
> +    spin_lock_irqsave(&aplic.lock, flags);

Taking a global lock for a per-vCPU operation isn't going to scale
very well. Even more so when then ...

> +    bitmap_for_each ( irqn, auth_irq_bmp, vintc->nr_virqs )

... you run a loop with perhaps many (hundreds? thousands?)
iterations.

> +    {
> +        volatile uint32_t __iomem *ptarget;
> +        uint32_t target_val;
> +        unsigned int guest_index, hart_index;
> +
> +        if ( !irqn )
> +            continue;
> +
> +        ptarget = &aplic.regs->target[irqn - 1];
> +        target_val = readl(ptarget);
> +
> +        guest_index = MASK_EXTR(target_val, APLIC_TARGET_GUEST_IDX);
> +        hart_index = MASK_EXTR(target_val, APLIC_TARGET_HART_IDX);
> +
> +        if ( (guest_index != old_guest_file_id) ||
> +             (hart_index != old_hart_field) )
> +            continue;

Along the lines of the naming comment above: This would be more
logical to follow if it was

        if ( (guest_id != old_guest_id) ||
             (hart != old_hart) )
            continue;

i.e. names on each side of the != suitably matching up.

Jan