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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

> +} 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.

> + * 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.

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).

Finally, unlike further up, here (and below) ireg wants parenthesizing.

> @@ -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).

> +};
> +
> +/*
> + * 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 *.

> +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?

> +    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.

> +    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? And
if so, why would you not use the 64-bit write function, allowing the
#ifdef to be omitted?

> @@ -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?

> +    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?

> @@ -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? And what exactly is the comment
telling me?

> +    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?

Jan