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

On 8/12/26 3:57 PM, Jan Beulich wrote:
> On 20.07.2026 18:02, Oleksii Kurochko wrote:
>> @@ -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;
> 
> How does the ->vsfile_pcpu sentinel value matter here, when you're checking
> ->guest_file_id?

Comment is incorrect. I will fix it.

> 
> And anyway, there being dependencies like this one on the other big series
> makes it rather hard to review things.
> 
>> +    write_lock_irqsave(&imsic_state->vsfile_lock, flags);
>> +    imsic_state->vsfile_pcpu = cpuid_to_hartid(v->processor);
> 
> As discussed for another patch in this series, this will need to change then
> as well.

I will update that properly.

> 
>> +    write_unlock_irqrestore(&imsic_state->vsfile_lock, flags);
>> +}
>> +
>> +void imsic_state_restore(struct vcpu *v)
>> +{
>> +    /* Nothing to do */
>> +}
> 
> "save" and "restore" have meaning other than what you intend here, aiui. Once
> again without call sites it remains unclear when exactly these functions would
> be called. Which makes it close to impossible to suggest better names.

I will add some extra context and/or re-shuffle patches to make it more 
clearer. Anyway as you explained me in another thread a name is really 
incorrect. I will use imsic_ctxt_switch_{to,from}() instead.

Thanks.

~ Oleksii