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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 13.08.2026 12:22, Oleksii Kurochko wrote:
> 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?

No. "save" and "restore" would best be limited to migration paths (migration
of guests between hosts, that is). I can only once again suggest that you
look at existing naming in the code base. You'll find e.g.
svm_ctxt_switch_from() or vmx_ctxt_switch_to() under x86/hvm/.

>> 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 <roger@xxxxxxxxxxxxxx>

Whereas in your mail it was still Roger Pau Monné <roger.pau@xxxxxxxxxx>.
When you originally posted the series, that was still correct. But in the
meantime it has changed (and I expect sending mail to the old address
wouldn't reach him anymore).

Jan