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

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 9/14/26 5:02 PM, Jan Beulich wrote:
> On 27.08.2026 17:21, Oleksii Kurochko wrote:
> > @@ -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));   \
> Nit: Excess parentheses again.
I will drop them.

> > @@ -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);
> Both can be unsigned int, can't they?
Yes, agreed. Both isel and bit fit within unsigned int. I will update 
them in the next version.
> > +    return !!(imsic_csr_read(isel) & bit);
> > +}
> No need for !! here.
>
> What about endianness, btw? Does the IMSIC always match the CPU (and
> its setting)?
No, the IMSIC does not dynamically adapt its register interfaces based 
on the CPU's runtime endianness configuration (e.g. mstatus.SBE/MBE):
- CSR Accesses (imsic_csr_read): Indirect CSR accesses (siselect/sireg 
or miselect/mireg) operate using standard RISC-V CSR instructions at 
current XLEN width. Values are read and written directly into 
architectural GPRs without byte-swapping.
- MMIO Ports: For incoming device MSIs, IMSIC uses fixed memory-mapped 
offsets: offset 0x000 (seteipnum_le) always expects Little-Endian, while 
offset 0x004 (seteipnum_be) expects Big-Endian.
- Memory-Resident Interrupt Files (MRIF): For virtualized environments, 
MRIF structures in memory are strictly defined in Little-Endian byte 
order regardless of whether the CPU harts operate in Big-Endian or 
Little-Endian mode.
> Overall, what does "local" in the function name signify? (For a static
> function, the "imsic" prefix may also be unnecessary.)
It signifies that local (on which code is executed now) hart's IMSIC 
CSRs (isel and ireg in the case of imsic_csr_read()) are touched.
> > @@ -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)
> If the parameter isn't used, maybe best to name it "unused"?
Makes sense to me. I will do that in the next version + I will update 
the type to 'struct imsic_vsfile_data *' as it was suggested for 
imsic_call_on_cpu() in "Re: [PATCH v2 30/39] xen/riscv: prepare new 
IMSIC VS-file".
> > @@ -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.
> >        */
> Isn't the new part of the comment premature? Moving doesn't start until ...
>
> > @@ -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);
> > +
> > +    /*
> > +     * TODO: 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.
> > +     */
> > +    if ( iommu_enabled )
> > +        printk_once("IMSIC: IOMMU MSI retargeting is not implemented\n");
> > +
> > +    /*
> > +     * 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.
> > +     */
> > +    aplic_reconfigure_target(v, old_vsfile_id, old_vsfile_cpu);
> ... here, as it looks.
At some point I agree but the steps before are preparation of moving and 
is a part of moving process.
Then probably it make sense to reword the comment to:

   /*
     * At this point, all interrupt producers are still using the old IMSIC
     * VS-file.  Allocate and clear the new one before redirecting anything
     * to it.
     */

Would it be better?

~ Oleksii