Re: [PATCH v2 33/39] xen/riscv: dump old interrupt file to memory

[Jan Beulich <jbeulich@xxxxxxxx>]

On 27.08.2026 17:21, Oleksii Kurochko wrote:
> At the old interrupt file, dump to memory all the eip and eie arrays).
> After this step is done, the old interrupt file is no longer in use so
> old intrrupt file VGEIN could be released.
> 
> Restoring of old interrupt file state will be done in follow-up
> patch.
> 
> There are cases where it is needed to specify on which cpu it is
> necessary to VGEIN should be released so update vgein_release() to
> deal with that.

Beside this being difficult to parse, it looks like it is inapplicable? As
said ...

> Keep 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_release() is stub for now and will be introduced later.

... also here?

> --- a/xen/arch/riscv/imsic.c
> +++ b/xen/arch/riscv/imsic.c
> @@ -56,6 +56,24 @@ static unsigned int __ro_after_init guest_num_msis;
>   */
>  #define GUEST_IMSIC_MAX_MSIS 255U
>  
> +/*
> + * The interrupt identities an IMSIC interrupt file provides are 0 (which is
> + * never valid, but still occupies a bit) up to IMSIC_MAX_ID inclusive, so
> + * IMSIC_MAX_ID + 1 bits have to be covered.
> + */
> +#define IMSIC_MAX_EIX DIV_ROUND_UP(IMSIC_MAX_ID + 1, BITS_PER_TYPE(uint64_t))

As before - plain 64 please, or it needs to become clear where the uint64_t
is actually coming from.

> +struct imsic_mrif_eix {
> +    unsigned long eip[BITS_PER_TYPE(uint64_t) / BITS_PER_LONG];
> +    unsigned long eie[BITS_PER_TYPE(uint64_t) / BITS_PER_LONG];

Same here. Yet better may be to use DECLARE_BITMAP()?

> @@ -85,6 +103,15 @@ do {                            \
>      csr_clear(CSR_SIREG, v);    \
>  } while (0)
>  
> +#define imsic_vs_csr_swap(c, v)     \
> +({                                  \
> +    unsigned long r_;               \
> +                                    \
> +    csr_write(CSR_VSISELECT, (c));  \
> +    r_ = csr_swap(CSR_VSIREG, (v)); \
> +    r_;                             \
> +})

Excess parentheses again. Plus - what use is r_ here?

> @@ -130,6 +157,21 @@ do {                                \
>      imsic_switchcase_32(F, ireg + 0, ##__VA_ARGS__) \
>      imsic_switchcase_32(F, ireg + 32, ##__VA_ARGS__)
>  
> +static unsigned long imsic_eix_swap(unsigned int ireg, unsigned long val)
> +{
> +    switch ( ireg )
> +    {
> +    imsic_switchcase_64(imsic_switchcase_ret, IMSIC_EIP0,
> +                        imsic_vs_csr_swap, val)
> +    imsic_switchcase_64(imsic_switchcase_ret, IMSIC_EIE0,
> +                        imsic_vs_csr_swap, val)
> +    default:
> +        ASSERT_UNREACHABLE();
> +    }

There still wants to "break" in the default case.

> @@ -973,5 +1071,11 @@ void imsic_migrate_vcpu(struct vcpu *v)
>       * to the new IMSIC VS-file.
>       */
>  
> +    /* Read and clear register state from old IMSIC VS-file */
> +    imsic_vsfile_read_clear(old_vsfile_id, old_vsfile_cpu, nr_hw_eix, 
> &tmrif);

Why is &tmrif being passed into the function, when it's not otherwise used
here? The function could itself have a suitable local var.

Jan