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

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

On 2026-09-23 18:08 +0200, Oleksii Kurochko wrote:
> 
> 
> 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.".
Oh okay, I think I misunderstood the comment at the first place then. I
thought you wanted to say the pointer itself is on the stack, not MRIF.
Now it's more clear.

> 
> 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...
Yes sorry, no needs for more investigations here.
> 
> ~ Oleksii
> 
> 
> 
>