Re: [PATCH v1 08/17] xen/riscv: add IMSIC state save/restore

On 8/13/26 11:51 AM, Jan Beulich wrote:
> On 13.08.2026 11:34, Oleksii Kurochko wrote:
>> On 8/13/26 11:30 AM, Baptiste Le Duc wrote:
>>>> --- a/xen/arch/riscv/imsic.c
>>>> +++ b/xen/arch/riscv/imsic.c
>>>> @@ -20,6 +20,7 @@
>>>>    #include <xen/init.h>
>>>>    #include <xen/libfdt/libfdt.h>
>>>>    #include <xen/macros.h>
>>>> +#include <xen/rwlock.h>
>>>>    #include <xen/sched.h>
>>>>    #include <xen/smp.h>
>>>>    #include <xen/spinlock.h>
>>>> @@ -418,6 +419,28 @@ int imsic_map_guest_file(struct vcpu *v, unsigned int vsfile_id)
>>>>        return res;
>>>>    }
>>>>    
>>>> +void imsic_state_save(struct vcpu *v)
>>>> +{
>>>> +    struct vimsic_state *imsic_state = v->arch.vimsic_state;
>>>> +    unsigned long flags;
>>>> +
>>>> +    /*
>>>> +     * SW interrupt file always has ->vsfile_pcpu = NR_CPUS so nothing specific
>>>> +     * should be done in this case.
>>>> +     */
>>>> +    if ( !vcpu_guest_file_id(v) )
>>>> +        return;
>>>
>>>
>>>> +
>>>> +    write_lock_irqsave(&imsic_state->vsfile_lock, flags);
>>>> +    imsic_state->vsfile_pcpu = cpuid_to_hartid(v->processor);
>>>
>>> How will you detect a migration is needed? Don't you need to first know
>>> if ->vsfile_pcpu is different to cpuid_to_hartid(v->processor)? (I
>>> didn't take a look to other patchs for the moment, so the
>>> explanations might be later.)
>>
>> Migration (if you are speaking about migration of vCPU from one pCPU to
>> another) is completely different path. Look at sched_move_irqs().
> 
> See how terminology is important. As said elsewhere, "save state" and
> "restore state" don't make clear at all in which situation they're to be
> used.

I totally agree that it is important.

Just to clarify it now (before I started to re-shuffle and/or adding 
extra patches to have better context how this functions will be called) 
I will add some information here. So imsic_state_save() and 
imsic_state_restore() is going to be called from context_switch() 
function when one vCPU is de-scheduled and new vCPU is scheduled (so no 
migration here at all, yes it could happen but it is still a separate 
path and so separate question). Considering that my understanding that 
during context_switch() I have to save state of IMSIC which corresponds 
to vCPU which is going to be de-scheduled and restore a state of IMSIC 
of vCPU which is going to be scheduled.

With the current context is imsic_state_save() and imsic_state_restore() 
are correct names?

> 
> Also, can both of you please adjust Roger's email address when replying?

Could you please clarify what is wrong with it? For example, in this 
patch series:
   [PATCH v2 0/2] vpci: allow unaligned accesses by the hardware domain

This one is used: Roger Pau Monne <[email protected]>

~ Oleksii