Re: [PATCH v2 31/39] xen/riscv: implement APLIC-hart sync barrier for vCPU migration

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

On 2026-09-23 12:57 +0200, Oleksii Kurochko wrote:
> 
> 
> On 9/22/26 8:48 PM, Oleksii Kurochko wrote:
> > 
> > 
> > On 9/22/26 7:00 PM, Baptiste Le Duc wrote:
> >>> During migration of a virtual hart to a different guest interrupt file,
> >>> straggler MSIs from the APLIC could arrive at the old interrupt file
> >>> after the switch.
> >>>
> >>> genmsi is used despite not supporting guest interrupt files because the
> >>> AIA spec guarantees that all MSIs previously sent from the APLIC to the
> >>> same hart are visible at the hart's IMSIC before the extempore MSI from
> >>> genmsi becomes visible.
> >>
> >>
> >>>
> >>> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>
> >>>
> >>> diff --git a/xen/arch/riscv/aplic.c b/xen/arch/riscv/aplic.c
> >>> index 0af13f28e4..cb11d6aeaa 100644
> >>> --- a/xen/arch/riscv/aplic.c
> >>> +++ b/xen/arch/riscv/aplic.c
> >>> @@ -27,7 +27,9 @@
> >>>   #include <asm/imsic.h>
> >>>   #include <asm/intc.h>
> >>>   #include <asm/io.h>
> >>> +#include <asm/processor.h>
> >>>   #include <asm/riscv_encoding.h>
> >>> +#include <asm/smp.h>
> >>>   static struct aplic_priv aplic = {
> >>>       .lock = SPIN_LOCK_UNLOCKED,
> >>> @@ -205,6 +207,30 @@ void aplic_hw_write_reg(unsigned int offset, 
> >>> uint32_t value)
> >>>       spin_unlock_irqrestore(&aplic.lock, flags);
> >>>   }
> >>> +/*
> >>> + * As needed, synchronize with all IOMMUs and APLICs to ensure that no
> >>> + * straggler MSIs will arrive at the old interrupt file after this 
> >>> step.
> >>> + */
> >>> +void aplic_genmsi_barrier(void)
> >>> +{
> >>> +    const struct imsic_config *imsic = imsic_get_config();
> >>> +    unsigned int cpu = smp_processor_id();
> >>> +    unsigned long flags;
> >>> +    uint32_t val;
> >>> +
> >>> +    val = MASK_INSR(aplic_hart_field(cpu), APLIC_TARGET_HART_IDX) |
> >>> +          (imsic->sync_id & APLIC_TARGET_EIID);
> >>
> >>
> >>> +
> >>> +    spin_lock_irqsave(&aplic.lock, flags);
> >>> +
> >>> +    writel(val, &aplic.regs->genmsi);
> >>> +
> >>> +    while ( readl(&aplic.regs->genmsi) & APLIC_GENMSI_BUSY )
> >>> +        cpu_relax();
> >>> +
> >>> +    spin_unlock_irqrestore(&aplic.lock, flags);
> >>> +}
> >>> +
> >> According to AIA spec §4.9.3 (Synchronizing interactions between a 
> >> hart and the APLIC), the sequence needs 6 steps; this implements only 
> >> steps 2-5:
> >>
> >> - Step 1: clear the pending bit for sync_id at the hart's IMSIC before 
> >> writing genmsi.
> >> - Step 6: after releasing the lock, poll the pending bit for sync_id 
> >> at the hart's IMSIC until it's set.
> >>
> >> Step 4 (Busy clear) only means the APLIC has accepted/sent the MSI, 
> >> not that it has arrived at the hart (the spec notes an unspecified 
> >> travel delay).
> >> Without step 6, aplic_genmsi_barrier() returns before the MSI (and 
> >> thus prior MSIs) actually reach the hart, so it doesn't achieve the 
> >> barrier it's meant to
> >> provide.
> >>
> > 
> > It is really missed but it exists in riscv-next-upstream branch 
> > (https://gitlab.com/xen-project/people/olkur/xen/-/blob/riscv-next- 
> > upstreaming/xen/arch/riscv/imsic.c#L723).
> > 
> > I will re-check why it is missed here.
> 
> Step 1 and step 6 are not missing, they are just not part of
> aplic_genmsi_barrier() itself. They are done by its caller, which is 
> added in "xen/riscv: remap interrupts to new IMSIC VS-file":
> 
>      static void cf_check imsic_aplic_sync(void *unused)
>      {
>          imsic_local_eix_update(imsic_cfg.sync_id, 1, true, false);
> 
>          aplic_genmsi_barrier();
> 
>          while ( !imsic_local_is_pending(imsic_cfg.sync_id) )
>              cpu_relax();
>      }
> 
> I did consider doing all six steps inside aplic_genmsi_barrier(), but 
> that would make aplic.c reach into IMSIC internals (the local EIx CSR 
> accessors, which are private to imsic.c), and I would rather not add 
> that dependency. Keeping the APLIC half (write genmsi, wait for Busy to 
> clear) in aplic.c and the IMSIC half (clear the pending bit of sync_id 
> before, poll it until set afterwards) in imsic.c keeps the layering clean.
> 
> You are right, though, that nothing in this patch says so, and that the
> helper on its own is not the full barrier its name suggests. I will 
> spell the split out in the commit message and in the comment above the 
> function.
> 
> Probably it will be better to introduce function imsic_aplic_sync() as a 
> part of this patch.
Yes it would make sense as function imsic_aplic_sync() is introduced in
next patch so reviewer couldn't know the existence of it when reading
this patch. Thanks for that!
> 
> ~ Oleksii
> 
> 
>