Re: [PATCH v2 34/39] xen/riscv: restore register state in the new IMSIC VS-file

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 9/23/26 5:42 PM, Baptiste Le Duc wrote:
> > At this point, all interrupt producers have been moved to the new
> > IMSIC VS-file so we move register state from the old IMSIC VS/SW-file
> > to the new IMSIC VS-file.
> >
> > As new IMSIC VS-file is ready to be used update vCPU's hstatus with
> > new VGEIN.
> >
> > As the whole migration procedure is finished add some extra explanatory
> > comments.
> >
> > Signed-off-by: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>
> >
> > diff --git a/xen/arch/riscv/imsic.c b/xen/arch/riscv/imsic.c
> > index 3cba58e0c1..d7b137a1f5 100644
> > --- a/xen/arch/riscv/imsic.c
> > +++ b/xen/arch/riscv/imsic.c
> > @@ -112,6 +112,12 @@ do {                            \
> >       r_;                             \
> >   })
> >   
> > +#define imsic_vs_csr_set(c, v)      \
> > +do {                                \
> > +    csr_write(CSR_VSISELECT, (c));  \
> > +    csr_set(CSR_VSIREG, (v));       \
> > +} while ( 0 )
> > +
> >   #define imsic_vs_csr_write(c, v)    \
> >   do {                                \
> >       csr_write(CSR_VSISELECT, (c));  \
> > @@ -185,6 +191,19 @@ static void imsic_eix_write(unsigned int ireg, unsigned 
> > long val)
> >       }
> >   }
> >   
> > +static void imsic_eix_set(unsigned int ireg, unsigned long val)
> > +{
> > +    switch ( ireg )
> > +    {
> > +    imsic_switchcase_64(imsic_switchcase_break, IMSIC_EIP0,
> > +                        imsic_vs_csr_set, val)
> > +    imsic_switchcase_64(imsic_switchcase_break, IMSIC_EIE0,
> > +                        imsic_vs_csr_set, val)
> > +    default:
> > +        ASSERT_UNREACHABLE();
> > +    }
> > +}
> > +
> >   unsigned int vcpu_guest_file_id(const struct vcpu *v)
> >   {
> >       return ACCESS_ONCE(v->arch.vimsic_state->guest_file_id);
> > @@ -630,13 +649,13 @@ static void cf_check imsic_vsfile_local_read_clear(void 
> > *data)
> >       old_vsiselect = csr_read(CSR_VSISELECT);
> >       old_hstatus = csr_read(CSR_HSTATUS);
> >       new_hstatus = old_hstatus & ~HSTATUS_VGEIN;
> > -    new_hstatus |= ((unsigned long)idata->hgei) << HSTATUS_VGEIN_SHIFT;
> > +    new_hstatus |= MASK_INSR(idata->hgei, HSTATUS_VGEIN);
>
> >       csr_write(CSR_HSTATUS, new_hstatus);
> >   
> >       /*
> > -     * There is no need to use atomic functions version to store
> > -     * values in MRIF because imsic_vsfile_read_clear() is always called
> > -     * with pointer to temporary MRIF on stack.
> > +     * No atomic accessors are needed to store the values into the MRIF here,
> > +     * as imsic_vsfile_read_clear() is always called with a pointer to a
> > +     * temporary MRIF on the stack.
> >        */
>
> >   
> >       mrif->eidelivery = imsic_vs_csr_swap(IMSIC_EIDELIVERY, 0);
> > @@ -972,6 +991,49 @@ int __init vimsic_make_domu_dt_node(struct kernel_info 
> > *kinfo,
> >       return fdt_end_node(fdt);
> >   }
> >   
> > +static void cf_check imsic_vsfile_local_update(void *data)
> > +{
> > +    unsigned int i;
> > +    struct imsic_mrif_eix *eix;
> > +    const struct imsic_vsfile_data *idata = data;
> > +    struct imsic_mrif *mrif = idata->mrif;
> > +    unsigned long new_hstatus, old_hstatus, old_vsiselect;
> > +
> > +    /* We can only update if we have a HW IMSIC context */
> > +    if ( !idata->hgei )
> > +        return;
> > +
> > +    /*
> > +     * No atomic accessors are needed to read the values out of the MRIF here,
> > +     * as this is always called with a pointer to a temporary MRIF on the
> > +     * stack.
> > +     */
> idata->mrif may point anywhere, so what the comment really states is a
> requirement on callers. It also only makes full sense next to KVM, where
> a shared SW-file MRIF is accessed atomically; Xen has neither of those
> (yet). Either explicitely explain that callers must declare mrif on
> their stack or move the comment directly in call sites.
It is mentioned in the comment "is always called with a pointer to a 
temporary MRIF on the stack.".
With SW-file MRIF I expect that atomic operations should be used so this 
functions shouldn't just use for them and I assume KVM has something 
different function to work with SW-file MRIF. Anyway just mentioning KVM 
code isn't always useful as it forces me to go and investigate what is 
going on there what I am not fully convinced that it is okay...
~ Oleksii