Re: [PATCH v2 32/39] xen/riscv: remap interrupts to new IMSIC VS-file

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

On 2026-09-23 17:45 +0200, Oleksii Kurochko wrote:
> 
> 
> On 9/23/26 3:34 PM, Baptiste Le Duc wrote:
> >> After new IMSIC VS-file is zeroed-out it is necessary to do G-stage 
> >> remaping
> >> fo new IMSIC VS-file. Also, if any interrupts at an APLIC are forwarded by
> >> MSIs to the old interrupt file, reconfigure the APLIC to send them to the
> >> new interrupt file.
> >>
> >> Generally it is needed also to modify the relevant translation tables at
> >> all IOMMUs so that MSIs for this virtual interrupt file are now sent to
> >> the new physical interrupt file but it is skipped for now there is no IOMMU
> >> support for RISC-V.
> >>
> >> Synchronize with APLIC to ensure that no straggler MSIs will arrive at
> >> the old interrupt file by using of aplic_genmsi_barrier().
> >>
> > 
> > 
> >> Technically there is no need for read_lock_irqsave() and
> >> read_unlock_irqrestore() around reading of ->guest_file_id, as a write
> >> cannot happen in parallel: any update to ->guest_file_id for a vCPU
> >> will happen either in imsic_migrate_vcpu() itself or before the vCPU
> > 
> > 
> >> first gains control (in continue_new_vcpu()), so there is no concurrent
> >> access to it in imsic_migrate_vcpu(). The lock is added here for
> >> potential future cases.
> >>
> >> Keep BUG_ON("unimplemented") placeholder in imsic_migrate_vcpu() to guard
> >> against silent incorrect behaviour or unexpected panics in guest VMs until
> >> the function is fully implemented.
> >>
> >> imsic_map_guest_file() and imsic_update_state() are stubs for now and will
> >> be introduced later in a separate patch.
> > "will be introduced later" would be stale in the future.
> 
> I will drop that part from the commit message.
> 
> >>
> >> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>
> > 
> > 
> >>
> >> diff --git a/xen/arch/riscv/imsic.c b/xen/arch/riscv/imsic.c
> >> index 5de4594961..5e9f6995e4 100644
> >> --- a/xen/arch/riscv/imsic.c
> >> +++ b/xen/arch/riscv/imsic.c
> >> @@ -27,6 +27,7 @@
> >>   #include <xen/xvmalloc.h>
> >>   
> >>   #include <asm/aia.h>
> >> +#include <asm/aplic.h>
> >>   #include <asm/imsic.h>
> >>   
> >>   #define IMSIC_HART_SIZE(guest_bits) (BIT(guest_bits, U) * 
> >> IMSIC_MMIO_PAGE_SZ)
> >> @@ -60,6 +61,12 @@ static unsigned int __ro_after_init guest_num_msis;
> >>   #define IMSIC_DISABLE_EITHRESHOLD   1
> >>   #define IMSIC_ENABLE_EITHRESHOLD    0
> >>   
> >> +#define imsic_csr_read(c)           \
> >> +({                                  \
> >> +    csr_write(CSR_SISELECT, (c));   \
> >> +    csr_read(CSR_SIREG);            \
> >> +})
> >> +
> >>   #define imsic_csr_write(c, v)   \
> >>   do {                            \
> >>       csr_write(CSR_SISELECT, c); \
> >> @@ -141,6 +148,11 @@ unsigned int vcpu_guest_file_id(const struct vcpu *v)
> >>       return ACCESS_ONCE(v->arch.vimsic_state->guest_file_id);
> >>   }
> >>   
> >> +void imsic_update_state(struct vcpu *v, unsigned int guest_file_id)
> >> +{
> >> +    BUG_ON("unimplemented\n");
> >> +}
> >> +
> >>   void __init imsic_ids_local_delivery(bool enable)
> >>   {
> >>       if ( enable )
> >> @@ -242,6 +254,15 @@ void imsic_irq_disable(unsigned int irq)
> >>       spin_unlock(&imsic_cfg.lock);
> >>   }
> >>   
> >> +static bool imsic_local_is_pending(unsigned int id)
> >> +{
> >> +    unsigned long isel =
> >> +        (id / BITS_PER_LONG) * (BITS_PER_LONG / IMSIC_EIPx_BITS) + 
> >> IMSIC_EIP0;
> >> +    unsigned long bit = BIT(id % BITS_PER_LONG, UL);
> >> +
> >> +    return !!(imsic_csr_read(isel) & bit);
> >> +}
> >> +
> >>   /* Callers aren't intended to changed imsic_cfg so return const. */
> >>   const struct imsic_config *imsic_get_config(void)
> >>   {
> >> @@ -436,6 +457,11 @@ void cf_check imsic_ctxt_switch_to(struct vcpu *v)
> >>       /* Nothing to do */
> >>   }
> >>   
> >> +int imsic_map_guest_file(struct vcpu *v, unsigned int vsfile_id)
> >> +{
> >> +    return -EOPNOTSUPP;
> >> +}
> >> +
> >>   int cf_check vcpu_imsic_init(struct vcpu *v)
> >>   {
> >>       struct vimsic_state *imsic_state;
> >> @@ -497,6 +523,27 @@ static void imsic_call_on_cpu(unsigned int cpu, void 
> >> (*func)(void *),
> >>           on_selected_cpus(cpumask_of(cpu), func, data, 1);
> >>   }
> >>   
> >> +/*
> >> + * Ensure that all the MSIs the APLIC has already generated for the hart 
> >> this
> >> + * runs on have really reached the hart's IMSIC.
> >> + *
> >> + * The barrier is the one described by the AIA specification in
> >> + * "Synchronizing interactions between a hart and the APLIC": ask the 
> >> APLIC to
> >> + * send an MSI to the hart itself and wait until it shows up as pending 
> >> in the
> >> + * hart's own interrupt file. As it says nothing about MSIs on their way 
> >> to
> >> + * any other hart, it has to be executed by the pCPU owning the interrupt 
> >> file
> >> + * the MSIs were being sent to.
> >> + */
> >> +static void cf_check imsic_aplic_sync(void *data)
> > It seems data arg is not used here
> 
> It will be renamed to `unused` in the v3 but we still need to have it 
> becuase how this function is called through imsic_call_on_cpu().
> 
> >> +{
> >> +    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();
> >> +}
> >> +
> >>   static void cf_check imsic_vsfile_local_clear(void *data)
> >>   {
> >>       unsigned int i;
> >> @@ -837,6 +884,10 @@ void imsic_migrate_vcpu(struct vcpu *v)
> >>       struct imsic_vsfile_data vsfile_data = {
> >>           .nr_eix = nr_hw_eix,
> >>       };
> >> +    struct vimsic_state *imsic_state = v->arch.vimsic_state;
> >> +    unsigned long flags;
> >> +    unsigned int old_vsfile_id;
> >> +    unsigned int old_vsfile_cpu;
> >>   
> >>       /*
> >>        * The scheduler can mark a freshly created vCPU's unit as migrated 
> >> and
> >> @@ -848,8 +899,22 @@ void imsic_migrate_vcpu(struct vcpu *v)
> >>       if ( v->arch.last_cpu == NR_CPUS )
> >>           return;
> >>   
> >> +    read_lock_irqsave(&imsic_state->vsfile_lock, flags);
> >> +    old_vsfile_id = imsic_state->guest_file_id;
> >> +    old_vsfile_cpu = imsic_state->vsfile_cpu;
> >> +    read_unlock_irqrestore(&imsic_state->vsfile_lock, flags);
> >> +
> >> +    /*
> >> +     * We don't support SW interrupt files at the moment. Bail out before
> >> +     * anything is touched, as the old file has no owning pCPU in that 
> >> case
> >> +     * and there is nothing to retarget the producers away from.
> >> +     */
> >> +    if ( old_vsfile_cpu == NR_CPUS )
> >> +        panic("IMSIC SW-file isn't supported\n");
> >> +
> >>       /*
> >>        * At this point, all interrupt producers are still using the old 
> >> IMSIC
> >> +     * VS-file so we first move all interrupt producers to the new IMSIC
> >>        * VS-file.
> >>        */
> >>   
> >> @@ -870,5 +935,43 @@ void imsic_migrate_vcpu(struct vcpu *v)
> >>       /* Zero-out new IMSIC VS-file */
> >>       imsic_call_on_cpu(new_vsfile_cpu, imsic_vsfile_local_clear, 
> >> &vsfile_data);
> >> +    /* Update G-stage mapping for the new IMSIC VS-file */
> >> +    if ( imsic_map_guest_file(v, new_vsfile_hgei) )
> >> +    {
> >> +        domain_crash(v->domain, "Migration to hw interrupt file 
> >> failed\n");
> >> +
> >> +        return;
> >> +    }
> >> +
> >> +    imsic_update_state(v, new_vsfile_hgei);
> >> +
> > This gets rewritten later in the series by "xen/riscv: introduce IMSIC h/w
> > interrupt file attaching to vcpu", which moves the clear/map/update sequence
> > into imsic_vsfile_acquire() (and adds vgein_release() on the error path).
> > 
> > Could imsic_vsfile_acquire() be introduced here (or in a prep patch) 
> > instead,
> > so that the later patch only adds imsic_vsfile_attach()? That would avoid 
> > the
> > churn.
> > 
> 
> It could, I just thought that it will be easier to justify necessity of 
> it by introduction in "xen/riscv: introduce IMSIC h/w interrupt file 
> attaching to vcpu". But I am okay to move it to this patch.
Hum, finally I don't know if it's the good solution. First time I'm
saw this, so I was suprised, but if it's common practice, let's keep
it here for clarity.
> Thanks.
> 
> ~ Oleksii
> 
> 
>