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

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

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