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

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

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

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

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