Re: [PATCH v1 14/27] xen/riscv: introduce per-vCPU IMSIC state

[Jan Beulich <jbeulich@xxxxxxxx>]

On 17.04.2026 15:47, Oleksii Kurochko wrote:
> On 4/16/26 2:31 PM, Jan Beulich wrote:
>> On 14.04.2026 11:22, Oleksii Kurochko wrote:
>>> On 4/2/26 1:31 PM, Jan Beulich wrote:
>>>> On 10.03.2026 18:08, Oleksii Kurochko wrote:
>>>>> --- a/xen/arch/riscv/imsic.c
>>>>> +++ b/xen/arch/riscv/imsic.c
>>>>> @@ -59,6 +59,29 @@ do {                            \
>>>>>        csr_clear(CSR_SIREG, v);    \
>>>>>    } while (0)
>>>>>    
>>>>> +unsigned int vcpu_guest_file_id(const struct vcpu *v)
>>>>> +{
>>>>> +    struct imsic_state *imsic_state = v->arch.imsic_state;
>>>>> +    unsigned long flags;
>>>>> +    unsigned int vsfile_id;
>>>>> +
>>>>> +    read_lock_irqsave(&imsic_state->vsfile_lock, flags);
>>>>> +    vsfile_id = imsic_state->guest_file_id;
>>>>> +    read_unlock_irqrestore(&imsic_state->vsfile_lock, flags);
>>>>
>>>> What purpose does this locking have? Already ...
>>>>
>>>>> +    return vsfile_id;
>>>>
>>>> ... here the value can be stale, if indeed there is a chance of races.
>>>> Did you perhaps mean to use ACCESS_ONCE() here and where the value is
>>>> set?
>>>
>>> ACCESS_ONCE() isn't guarantee only compiler re-ordering (as basically it
>>> is just volatile-related stuff inisde the macros)?
>>>
>>> Generally, I think that that guest_file_id is needed to be updated only
>>> during migration of vCPU from one pCPU to another and I expect that
>>> during this migration vCPU isn't active, so no one will want to read
>>> imsic_state->guest_file_id. But on the other hand, there is:
>>>     bool imsic_has_interrupt(const struct vcpu *vcpu)
>>>     {
>>>     ...
>>>       /*
>>>        * The IMSIC SW-file directly injects interrupt via hvip so
>>>        * only check for interrupt when IMSIC VS-file is being used.
>>>        */
>>>
>>>       read_lock_irqsave(&imsic_state->vsfile_lock, flags);
>>>       if ( imsic_state->vsfile_pcpu != NR_CPUS )
>>>           ret = !!(csr_read(CSR_HGEIP) & BIT(imsic_state->guest_file_id,
>>> UL));
>>>       read_unlock_irqrestore(&imsic_state->vsfile_lock, flags);
>>>     ...
>>>     }
>>> which I think could be called in parallel with with migration, so then
>>> still lock are needed.
>>
>> None of this addresses my pointing out that the returned value will be
>> stale by the point the caller gets to look at it.
> 
> Yes, I agree that lock in vcpu_guest_file_id() is useless and it should 
> be on the caller side and used for the whole IMSIC state access. But ...
> 
>> Which in turn raises
>> said question about the use of a lock. If you read
>> imsic_state->guest_file_id atomically (i.e. excluding tearing of reads),
>> the value seen / used will be stale as with the lock in use. Unless of
>> course there's yet another aspect hidden somewhere in what is not being
>> explained.
> 
> ... I am not sure that I get this part.
> 
> If I am somewhere in migration code where I took write lock to update 
> imsic state (and of course ->guest_file_id as part of it) then if 
> someone else in parallel calls imsic_has_interrupt() then it won't enter 
> critical section where ->guest_file_id is trying to be read so no stale 
> ->guest_file_id will be read.

Well, hence why I said "Unless of course there's yet another aspect hidden
somewhere in what is not being explained." If I don't know the full
picture, I can't very well judge whether a lock is needed, or whether ...

> Then does it make sense to use ACCESS_ONCE() during read and write of
> ->guest_file_id in such use cases?

... ACCESS_ONCE() would be enough.

Jan