Re: [PATCH v3 29/39] xen/riscv: prepare new IMSIC VS-file

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 10/23/26 2:55 PM, Baptiste Le Duc wrote:
> > [ ... 64 lines skipped ... ]
> > diff --git a/xen/arch/riscv/aia.c b/xen/arch/riscv/aia.c
> > index e31c9c2d24..229ab4b867 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>
> Nit: Why xen/sched.h? The stub doesn't dereference v, and asm/aia.h already
> forward-declares struct vcpu. If it stays: sched < sections, so it goes one
> line up.
Indeed, nothing needs it here. I'll include <asm/aia.h> instead, which
aia.c doesn't include yet: without that the stub's struct vcpu would 
only be declared inside the parameter list. <xen/sched.h> then moves to 
the patch where v gets dereferenced first.
> > [ ... 30 lines skipped ... ]
> > +#define imsic_vs_csr_write(c, v)    \
> > +do {                                \
> > +    csr_write(CSR_VSISELECT, c);    \
> > +    csr_write(CSR_VSIREG, v);       \
> > +} while (0)
> > +
> > +/*
> > + * Leaf macros for the switchcase expansion pyramid below:
> > + * imsic_switchcase_break(ireg, op, v) - emit "case ireg: op(ireg, v); break"
> > + */
> > +#define imsic_switchcase_break(ireg, op, v) \
> > +    case ireg:                              \
> > +        op(ireg, v);                        \
> > +        break
> > +
> > +/*
> > + * Generic switchcase expansion pyramid.
> > + * F is the per-operation leaf macro, ireg is the base register index.
> > + * Extra args (e.g. an operation and/or a value) are forwarded to F via
> > + * __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();
> > +        break;
> > +    }
> > +}
> > +
> What does the switch buy? ireg only ends up as the value written to
> vsiselect, not as a CSR number, so it doesn't need to be a compile-time
> constant. imsic_vs_csr_write(ireg, val) does the same, without the 128 case
> labels and the macro pyramid. If there's a reason, it wants stating in a
> comment; otherwise please drop the pyramid (an ASSERT() on the range can
> stay if you want one).
Good point.

I will drop it and use the following instead:

/*
 * 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. ireg names the first, even-numbered
 * register of such a group, among eip0..eip63 or eie0..eie63.
 */
static void imsic_eix_check(unsigned int ireg)
{
    ASSERT(!(ireg & 1));
    ASSERT((ireg >= IMSIC_EIP0 && ireg < IMSIC_EIP0 + 64) ||
           (ireg >= IMSIC_EIE0 && ireg < IMSIC_EIE0 + 64));
}

/* Write a whole 64-bit EIx group, laid out as described above. */
static void imsic_eix_write64(unsigned int ireg, uint64_t val)
{
    imsic_eix_check(ireg);
    imsic_vs_csr_write(ireg, val);
    if ( IS_ENABLED(CONFIG_RISCV_32) )
        imsic_vs_csr_write(ireg + 1, val >> 32);
}

I will applt the similar patter to other patches where the pyramid was used.


> Typo (only if the macros stay): "Leaf macros" - there's only one here. And
> `case ireg:` / `op(ireg, v)` as well as c and v in imsic_vs_csr_write()
> want parenthesizing (MISRA 20.7).
As to c and v in imsic_vs_csr_write(): they're passed as whole arguments
to csr_write() and the other csr_*() macros, which is covered by the
R20.7 deviation for macro parameters used as macro arguments. That's 
also why the parentheses were dropped in v3.
> > [ ... 67 lines skipped ... ]
> > +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 */
> Typo: s/a IMSIC/an IMSIC/
> > [ ... 31 lines skipped ... ]
> >   void imsic_migrate_vcpu(struct vcpu *v)
> >   {
> > +    struct imsic_vsfile_data vsfile_data = {
> > +        .nr_eix = imsic_cfg.nr_eix,
> > +    };
> > +
> >       /*
> >        * The scheduler can mark a freshly created vCPU's unit as migrated and
> >        * invoke this before the vCPU has ever run (see the migrated branch in
> > @@ -703,5 +849,18 @@ void imsic_migrate_vcpu(struct vcpu *v)
> >       if ( v->arch.last_cpu == CPU_NONE )
> >           return;
> >   
> > +    /*
> > +     * At this point, all interrupt producers are still using the old IMSIC
> > +     * VS-file.
> > +     */
> > +
> > +    vsfile_data.hgei = vgein_assign(v);
> > +
> > +    /* We don't support SW interrupt files at the moment. */
> > +    BUG_ON(!vsfile_data.hgei);
> > +
> > +    /* Zero-out new IMSIC VS-file */
> > +    imsic_call_on_cpu(v->processor, imsic_vsfile_local_clear, &vsfile_data);
> Nit: The comment restates the call; drop it.
Agreed, I will drop.

Thanks!

~ Oleksii