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

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 9/14/26 5:15 PM, Jan Beulich wrote:
> 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?
Agree that paragraph should be dropped, it ins't applicable anymore.

> > --- 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.
I will use plain 64.

> > +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()?
It will be better. I'll use:

    DECLARE_BITMAP(eip, 64);
    DECLARE_BITMAP(eie, 64);

> > @@ -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?
r_ stands for `return value` but it should be dropped as it could be 
done so imsic_vs_csr_swap() just returns a value directly:
#define imsic_vs_csr_swap(c, v)     \
({                                  \
    csr_write(CSR_VSISELECT, c);    \
    csr_swap(CSR_VSIREG, v);        \
})


> > @@ -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.
I'll add.

> > @@ -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.
tmrif isn't a scratch buffer local to the read: it carries the register 
state of the old interrupt file over to the new one. In this patch it's 
only filled in, but the next patch ("restore register state in the new 
IMSIC VS-file") passes the same buffer, via vsfile_data.mrif, to 
imsic_vsfile_local_update() on the new pCPU. By then the old file has 
been cleared and released, so the state has to live in the caller. I'll 
add a sentence to this patch's description saying the dumped state is 
consumed by the following patch:
The state is dumped into a buffer provided by the caller rather than one
local to imsic_vsfile_read_clear(), as it has to outlive the old 
interrupt file: once that file is released, the state is still needed to 
be restored into the new interrupt file.
Just to emphasize that in code I will use vsfile_data.mrif instead of 
&tmrif:
  imsic_vsfile_read_clear(old_vsfile_id, old_vsfile_cpu, 
vsfile_data.nr_eix, vsfile_data.mrif);
Thanks.

~ Oleksii