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

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 9/14/26 3:13 PM, Jan Beulich wrote:
> On 27.08.2026 17:21, Oleksii Kurochko wrote:
> > @@ -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));     \
> As patch context also tells: Excess parentheses.
I will drop them then.

> > +} 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__.
> Just that there's no F below.
Oh, right. I will move this comment a little bit down.

> > + * 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__)
> Ah, there is an F here.
>
> This (recurring below) shows another problem: The two F invocations
> look syntacticlly incorrect, due to the missing semicolon. Semicolon
> use wants redoing everywhere here.
I will drop then ';' from imsic_switchcase_{break,ret}.

> Further (and again throughout) I think we'd be better off using either
> standard C constructs (e.g. __VA_ARGS__) or the gcc extension
> permitting use of ## after a comma. A mix of both always looks odd
> (to me at least).
I will follow C standard here.

> Finally, unlike further up, here (and below) ireg wants parenthesizing.
I will add some.


> > @@ -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;
> I can't spot any use of this field (and hence I also can't judge
> whether const wants adding).
It should be introduced later in the another patch.

> > +};
> > +
> > +/*
> > + * 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 this is supposed to be passed struct imsic_vsfile_data *, why not say
> so here? Be as type-safe as possible. Of course the callback function
> has to use void *.
At the moment, I don't see where I am using void * so I will use struct 
imsic_vsfile_data * instead.
> > +static void cf_check imsic_vsfile_local_clear(void *data)
> > +{
> > +    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;
> Wouldn't it make sense to avoid the call here altogether then?
I think it is better to have this if () here instead of the caller side 
as this function one day could be just directly (w/ imsic_call_on_cpu) 
and even the way how it is called now and in the case of 
imsic_call_on_cpu() is executed on local cpu then it will be basically 
just direct call of imsic_vsfile_local_clear(). So in the case I am not 
missing something I prefer to have a check here.
> > +    old_vsiselect = csr_read(CSR_VSISELECT);
> Likely obvious to you, but I can't spot why vsiselect would need saving
> here. If you want me to ack such code, please add at least brief comments.
I think then it will be better to put the comment once above struct 
imsic_vsfile_data and then just point here to that comment as basically 
it will be needed for all imsic_vsfile_local_* helpers.
So basically I am suggesting:

--- a/xen/arch/riscv/imsic.c
+++ b/xen/arch/riscv/imsic.c
@@ ... @@
 /*
  * Arguments of the imsic_vsfile_local_*() helpers, which are executed 
by the
  * pCPU owning the interrupt file, thereby through imsic_call_on_cpu().
+ *
+ * The helpers interrupt whatever vCPU context is loaded on that pCPU, 
which
+ * generally isn't the vCPU the interrupt file belongs to. To reach the 
file
+ * they retarget hstatus.VGEIN and select the file's registers through
+ * vsiselect. Both CSRs are live state of the interrupted vCPU 
(vsiselect is
+ * saved to struct arch_vcpu only on context switch, but the 
interrupted vCPU
+ * may resume guest execution without one), hence the helpers have to 
restore
+ * them before returning.
  */
 struct imsic_vsfile_data {
     unsigned int hgei;
     unsigned int nr_eix;
     struct imsic_mrif *mrif;
 };
@@ ... @@ static void cf_check imsic_vsfile_local_clear(void *data)
     /* We can only zero-out if we have a IMSIC VS-file */
     if ( !idata->hgei )
         return;

+    /* See the comment ahead of struct imsic_vsfile_data. */
     old_vsiselect = csr_read(CSR_VSISELECT);
     old_hstatus = csr_read(CSR_HSTATUS);
@@ ... @@ static void cf_check imsic_vsfile_local_read_clear(void *data)
     csr_clear(CSR_HGEIE, BIT(idata->hgei, UL));

+    /* See the comment ahead of struct imsic_vsfile_data. */
     old_vsiselect = csr_read(CSR_VSISELECT);
     old_hstatus = csr_read(CSR_HSTATUS);
@@ ... @@ static void cf_check imsic_vsfile_local_update(void *data)
      * stack.
      */

+    /* See the comment ahead of struct imsic_vsfile_data. */
     old_vsiselect = csr_read(CSR_VSISELECT);
     old_hstatus = csr_read(CSR_HSTATUS);


Does it look clear now?

> > +    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
> In asm/imsic.h I see
>
> #define IMSIC_EIPx_BITS         32
>
> Why is the number of CSR writes different here for RV32 vs RV64? 
IMSIC_EIPx_BITS is the unit the AIA spec numbers the eip<k>/eie<k> 
registers by. On RV32 all of eip0..eip63 exist and are 32 bits wide. On 
RV64 only the even-numbered ones exist, each being 64 bits wide and 
covering what eip<k> and eip<k+1> cover on RV32; accessing an 
odd-numbered one is an illegal instruction. Hence one 64-bit group of 
interrupt identities takes one register on RV64, but two on RV32.
I will add some small comments:

    for ( i = 0; i < idata->nr_eix; i++ )
    {
        /* On RV64 a 64-bit EIx group is the even-numbered register 
alone. */
        imsic_eix_write(IMSIC_EIP0 + i * 2, 0);
        imsic_eix_write(IMSIC_EIE0 + i * 2, 0);
#ifdef CONFIG_RISCV_32
        /*
         * On RV32 it is split into the even-numbered (low half) and the
         * following odd-numbered (high half) register.
         */
        imsic_eix_write(IMSIC_EIP0 + i * 2 + 1, 0);
        imsic_eix_write(IMSIC_EIE0 + i * 2 + 1, 0);
#endif
    }

> And
> if so, why would you not use the 64-bit write function, allowing the
> #ifdef to be omitted?
I can introduce something like:

/*
 * On RV64 a 64-bit EIx group is the even-numbered register alone, whereas
 * on RV32 it is split into the even-numbered (low half) and the following
 * odd-numbered (high half) register.
 */
static void imsic_eix_write64(unsigned int ireg, uint64_t val)
{
    imsic_eix_write(ireg, val);
    if ( IS_ENABLED(CONFIG_RISCV_32) )
        imsic_eix_write(ireg + 1, val >> 32);
}

And then:

    for ( i = 0; i < idata->nr_eix; i++ )
    {
        imsic_eix_write64(IMSIC_EIP0 + i * 2, 0);
        imsic_eix_write64(IMSIC_EIE0 + i * 2, 0);
    }

Would it be better?

Or instead of imsic_eix_write64() I could just have a combination of 
imisc_vsfile_local_clear():
    imsic_eix_write(ireg, 0);
    if ( IS_ENABLED(CONFIG_RISCV_32) )
        imsic_eix_write(ireg + 1, 0);

> > @@ -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));
> As written, this could also be 64. uint64_t is a fixed-width type after
> all. The question here is: Which variable's type do you really mean
> here?
The minimum number of hardware EIx groups is 1 (for the minimum 63 
supported interrupt identities, i.e. DIV_ROUND_UP(63 + 1, 64) = 1), and 
the maximum is 32 (for 2047 identities), on both RV32 and RV64.You are 
right regarding BITS_PER_TYPE(uint64_t). Since an architectural EIx 
group always covers 64 interrupt identities regardless of XLEN, using 
BITS_PER_TYPE(uint64_t) is unnecessarily indirect when we mean a 
constant 64-bit group size.I will simplify this in v3 to use 64.
> > +    struct imsic_vsfile_data vsfile_data = {
> > +        .nr_eix = nr_hw_eix,
> The local variable looks to be used only here. Is there really a need
> for such a local variable?
No, I will drop nr_hw_eix.

> > @@ -699,5 +836,27 @@ void imsic_migrate_vcpu(struct vcpu *v)
> >       if ( v->arch.last_cpu == NR_CPUS )
> >           return;
> >   
> > +    /*
> > +     * At this point, all interrupt producers are still using the old IMSIC
> > +     * VS-file.
> > +     */
> > +
> > +    /*
> > +     * Latch the pCPU the new interrupt file is taken from: vgein_assign()
> > +     * allocates it from v->processor's pool of guest interrupt files, and
> > +     * only that hart can access the file afterwards.
> > +     */
> > +    new_vsfile_cpu = v->processor;
> Same here: Is this variable really needed? 
Technically no, it could be used v->processor everywhere but just for 
readability (to how spec is wording migration process) I think I will 
prefer to have new_vsfile_cpu here. But if it doesn't make sense I can 
agree to drop it.
> And what exactly is the comment
> telling me?
I am re-reading it now and it looks just useless.

I think that initially I thought that for some reason v->processor could 
change during the end of migration and so by that I wanted to fix new 
vsfile cpu so all the interrupts will go there before migration functon 
for that vcpu will be called again and reschedule all the interrupt to 
new v->processor.
I think it isn't real case so the comment could be dropped.

> > +    new_vsfile_hgei = vgein_assign(v);
> > +
> > +    /* We don't support SW interrupt files at the moment. */
> > +    BUG_ON(!new_vsfile_hgei);
> > +
> > +    vsfile_data.hgei = new_vsfile_hgei;
> And again - any real need for the separate local variable?
Here I agree, we could have only vsfile_data.hgei.

Thanks.

~ Oleksii