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

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

On 2026-09-22 15:01:02+02:00, Oleksii Kurochko wrote:
> 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;
> +
Thanks! Maybe use `registers` instead of `groups` wording.
> and init it once in imsic_parse_node().
> 
> Thanks.
> 
> ~ Oleksii