Re: [PATCH v2 30/39] xen/riscv: prepare new IMSIC VS-file

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 9/21/26 6:15 PM, Baptiste Le Duc wrote:
> On 8/27/26 5:24 PM, Oleksii Kurochko wrote:
> > Implement first steps of migration a vCPU to a different guest 
> > interrupt file
> > procedure:
> > - At the old interrupt file, save to memory the values of registers
> >    eidelivery and eithreshold, and set eidelivery = 0.
> > - At the new interrupt file, set eidelivery = 0, and zero all
> >    implemented interrupt-pending bits (the eip array).
> >
> > The following steps will be introduced in follow-up patches.
> >
> > Add a 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.
> >
> > vgein_assign() will be introduced later in a separate patch, for not 
> > it is
> > only stub.
> >
> > Signed-off-by: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>
> > ---
> > Changes in v2:
> >   - New patch.
> > ---
> > ---
> >   xen/arch/riscv/aia.c             |   9 ++
> >   xen/arch/riscv/imsic.c           | 159 +++++++++++++++++++++++++++++++
> >   xen/arch/riscv/include/asm/aia.h |   4 +
> >   xen/include/xen/config.h         |   1 +
> >   4 files changed, 173 insertions(+)
> >
> > diff --git a/xen/arch/riscv/aia.c b/xen/arch/riscv/aia.c
> > index e31c9c2d24b6..75c82bcfa1b3 100644
> > --- a/xen/arch/riscv/aia.c
> > +++ b/xen/arch/riscv/aia.c
> > @@ -1,8 +1,10 @@
> >   /* SPDX-License-Identifier: GPL-2.0-only */
> > +#include <xen/bug.h>
> >   #include <xen/errno.h>
> >   #include <xen/init.h>
> >   #include <xen/sections.h>
> > +#include <xen/sched.h>
> >   #include <xen/types.h>
> >   #include <asm/cpufeature.h>
> > @@ -21,3 +23,10 @@ void __init aia_init(void)
> >       _aia_usable = true;
> >   }
> > +
> > +unsigned int vgein_assign(struct vcpu *v)
> > +{
> > +    BUG_ON("unimplemented\n");
> > +
> > +    return 0;
> > +}
> > diff --git a/xen/arch/riscv/imsic.c b/xen/arch/riscv/imsic.c
> > index ad7fbe708bfd..516f0105352a 100644
> > --- a/xen/arch/riscv/imsic.c
> > +++ b/xen/arch/riscv/imsic.c
> > @@ -26,6 +26,7 @@
> >   #include <xen/spinlock.h>
> >   #include <xen/xvmalloc.h>
> > +#include <asm/aia.h>
> >   #include <asm/imsic.h>
> >   #define IMSIC_HART_SIZE(guest_bits) (BIT(guest_bits, U) * 
> > IMSIC_MMIO_PAGE_SZ)
> > @@ -77,6 +78,64 @@ do {                            \
> >       csr_clear(CSR_SIREG, v);    \
> >   } while (0)
> > +#define imsic_vs_csr_write(c, v)    \
> > +do {                                \
> > +    csr_write(CSR_VSISELECT, (c));  \
> > +    csr_write(CSR_VSIREG, (v));     \
> > +} while ( 0 )
> > +
> > +/*
> > + * Generic switchcase expansion pyramid.
> > + * F is the per-operation leaf macro, ireg is the base register index.
> > + * Optional extra args (e.g. an operation and/or a value) are 
> > forwarded to F
> > + * via __VA_ARGS__.
> > + *
> > + * imsic_switchcase_break(ireg, op, v) - emit "case ireg: op(ireg,v); 
> > break;"
> > + * imsic_switchcase_ret(ireg, op, ...) - emit "case ireg: return 
> > op(ireg[,v]);"
> > + *   The variadic tail is optional so the same leaf works for both 
> > read (no v)
> > + *   and swap (with v).
> > + */
> > +#define imsic_switchcase_break(ireg, op, v) \
> > +    case ireg:                              \
> > +        op(ireg, v);                        \
> > +        break;
> > +
> > +#define imsic_switchcase_ret(ireg, op, ...) \
> > +    case ireg:                              \
> > +        return op(ireg, ##__VA_ARGS__);
> > +
> > +#define imsic_switchcase_2(F, ireg, ...)    \
> > +    F(ireg + 0, ##__VA_ARGS__)              \
> > +    F(ireg + 1, ##__VA_ARGS__)
> > +#define imsic_switchcase_4(F, ireg, ...)    \
> > +    imsic_switchcase_2(F, ireg + 0, ##__VA_ARGS__)  \
> > +    imsic_switchcase_2(F, ireg + 2, ##__VA_ARGS__)
> > +#define imsic_switchcase_8(F, ireg, ...)    \
> > +    imsic_switchcase_4(F, ireg + 0, ##__VA_ARGS__)  \
> > +    imsic_switchcase_4(F, ireg + 4, ##__VA_ARGS__)
> > +#define imsic_switchcase_16(F, ireg, ...)   \
> > +    imsic_switchcase_8(F, ireg + 0, ##__VA_ARGS__)  \
> > +    imsic_switchcase_8(F, ireg + 8, ##__VA_ARGS__)
> > +#define imsic_switchcase_32(F, ireg, ...)   \
> > +    imsic_switchcase_16(F, ireg + 0, ##__VA_ARGS__) \
> > +    imsic_switchcase_16(F, ireg + 16, ##__VA_ARGS__)
> > +#define imsic_switchcase_64(F, ireg, ...)   \
> > +    imsic_switchcase_32(F, ireg + 0, ##__VA_ARGS__) \
> > +    imsic_switchcase_32(F, ireg + 32, ##__VA_ARGS__)
> > +
> > +static void imsic_eix_write(unsigned int ireg, unsigned long val)
> > +{
> > +    switch ( ireg )
> > +    {
> > +    imsic_switchcase_64(imsic_switchcase_break, IMSIC_EIP0,
> > +                        imsic_vs_csr_write, val)
> > +    imsic_switchcase_64(imsic_switchcase_break, IMSIC_EIE0,
> > +                        imsic_vs_csr_write, val)
> > +    default:
> > +        ASSERT_UNREACHABLE();
> > +    }
> > +}
> > +
> >   unsigned int vcpu_guest_file_id(const struct vcpu *v)
> >   {
> >       return ACCESS_ONCE(v->arch.vimsic_state->guest_file_id);
> > @@ -389,6 +448,76 @@ int cf_check vcpu_imsic_init(struct vcpu *v)
> >       return 0;
> >   }
> > +/*
> > + * Arguments of the imsic_vsfile_local_*() helpers, which are 
> > executed by the
> > + * pCPU owning the interrupt file, thereby through imsic_call_on_cpu().
> > + */
> > +struct imsic_vsfile_data {
> > +    unsigned int hgei;
> > +    unsigned int nr_eix;
> > +    struct imsic_mrif *mrif;
> > +};
> > +
> > +/*
> > + * Execute func() on the pCPU which owns the IMSIC interrupt file 
> > func() is
> > + * going to work with.
> > + *
> > + * An IMSIC VS-file is reachable only through hstatus.VGEIN of the 
> > hart the
> > + * file belongs to, and a guest interrupt file index is meaningless 
> > on any
> > + * other hart, so such work always has to be done by that very hart.
> > + *
> > + * The local case runs with IRQs disabled to provide func() with the 
> > same
> > + * environment it is given when it is called from the function call IPI
> > + * handler.
> > + */
> > +static void imsic_call_on_cpu(unsigned int cpu, void (*func)(void *),
> > +                              void *data)
> > +{
> > +    if ( cpu == smp_processor_id() )
> > +    {
> > +        unsigned long flags;
> > +
> > +        local_irq_save(flags);
> > +        func(data);
> > +        local_irq_restore(flags);
> > +    }
> > +    else
> > +        on_selected_cpus(cpumask_of(cpu), func, data, 1);
> > +}
> > +
> > +static void cf_check imsic_vsfile_local_clear(void *data)
> I think the remark from Jan to direclty pass the type instead of void
> could be applied here.
I think it can't because of how function pointer is passed to 
on_selected_cpus() through imsic_call_on_cpu().
> > +{
> > +    unsigned int i;
> > +    const struct imsic_vsfile_data *idata = data;
> > +    unsigned long new_hstatus, old_hstatus, old_vsiselect;
> > +
> > +    /* We can only zero-out if we have a IMSIC VS-file */
> > +    if ( !idata->hgei )
> > +        return;
> > +
> > +    old_vsiselect = csr_read(CSR_VSISELECT);
> > +    old_hstatus = csr_read(CSR_HSTATUS);
> > +    new_hstatus = old_hstatus & ~HSTATUS_VGEIN;
> > +    new_hstatus |= MASK_INSR(idata->hgei, HSTATUS_VGEIN);
> > +    csr_write(CSR_HSTATUS, new_hstatus);
> > +
> > +    imsic_vs_csr_write(IMSIC_EIDELIVERY, 0);
> > +    imsic_vs_csr_write(IMSIC_EITHRESHOLD, 0);
> > +
> > +    for ( i = 0; i < idata->nr_eix; i++ )
> > +    {
> > +        imsic_eix_write(IMSIC_EIP0 + i * 2, 0);
> > +        imsic_eix_write(IMSIC_EIE0 + i * 2, 0);
> > +#ifdef CONFIG_RISCV_32
> > +        imsic_eix_write(IMSIC_EIP0 + i * 2 + 1, 0);
> > +        imsic_eix_write(IMSIC_EIE0 + i * 2 + 1, 0);
> > +#endif
> > +    }
> > +
> > +    csr_write(CSR_HSTATUS, old_hstatus);
> > +    csr_write(CSR_VSISELECT, old_vsiselect);
> > +}
> > +
> >   void cf_check vcpu_imsic_deinit(struct vcpu *v)
> >   {
> >       XVFREE(v->arch.vimsic_state);
> > @@ -689,6 +818,14 @@ int __init vimsic_make_domu_dt_node(struct 
> > kernel_info *kinfo,
> >   void imsic_migrate_vcpu(struct vcpu *v)
> >   {
> > +    unsigned int new_vsfile_hgei;
> > +    unsigned int new_vsfile_cpu;
> > +    unsigned int nr_hw_eix = DIV_ROUND_UP(imsic_cfg.nr_ids + 1,
> > +                                          BITS_PER_TYPE(uint64_t));
> This value appears to remain constant after initialization, since it 
> depends directly on the hw,
> so it is not necessary to calculate it each time.
I will add then:

--- a/xen/arch/riscv/include/asm/imsic.h
+++ b/xen/arch/riscv/include/asm/imsic.h
@@ struct imsic_config {
     /* Number off interrupt identities */
     unsigned int nr_ids;

+    /*
+     * Number of 64-bit EIx groups needed to cover all the interrupt
+     * identities, which are 0 (never valid, but it still occupies a 
bit) up
+     * to and including nr_ids.
+     */
+    unsigned int nr_eix;
+

and init it once in imsic_parse_node().

Thanks.

~ Oleksii