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

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 9/14/26 2:25 PM, Jan Beulich wrote:
> 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.
The RISC-V AIA specification relies on this separation for the 6-step 
vCPU migration sequence:
Step 1. Setting eidelivery = 0 at the old interrupt file stops new traps 
on the host CPU.
Step 3: Reconfiguring APLIC/IOMMU and flushing the interconnect forces 
all in-flight "straggler" MSIs to reach the old interrupt file.
Step 4: Because eidelivery = 0 did not block incoming MSIs from setting 
bits in eip, those straggler MSIs safely landed in the old eip array.
Step 5: The hypervisor reads/dumps the old eip array and bitwise ORs it 
into the new interrupt file, guaranteeing that no in-flight MSIs are 
lost during the transition.
(Note that I re-word some steps and skipped some for simplicity. Here 
you can find full text: 
https://github.com/riscv/riscv-aia/blob/main/src/VSLevel.adoc?plain=1#L134)
Does it make sense now?

> > --- 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?
Because aplic_hart_field() construct target's part of hart field which 
isn't contains only pure hart value (apparently, thanks to the way how 
spec is written and things are done).
Note that based on other reviews from this patch series it is renamed to:
  unsigned long old_hart_index = aplic_hart_index(old_cpu);
(and here _index for the same reason it is basically how AIA spec calls 
this part of the target register)

> > +    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.
I agree.

Then per vcpu's target register lock (or per-irq lock) + APLIC's global 
lock mention here only for a short period when APLIC register would be 
needed.
I've done such change for support of IMSIC software interrupt file but 
it seems like it is started to need earlier.
> > +    {
> > +        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.
I agree with guest_id suggestion but hart_index should be left as 
according to the spec what is stored in hart index field of target 
register isn't pure hart cpu id but it is a combination of hart cpu id + 
group index (check the comment above aplic_hart_field() for better context).
Thanks.

~ Oleksii