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

[Jan Beulich <jbeulich@xxxxxxxx>]

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:
>>> Each vCPU interacting with the IMSIC requires state to track the
>>> associated guest interrupt file and its backing context.
>>>
>>> Introduce a per-vCPU structure to hold IMSIC-related state, including
>>> the guest interrupt file identifier and the CPU providing the backing
>>> VS-file. Access to the guest file identifier is protected by a lock.
>>>
>>> Initialize this structure during vCPU setup and store it in arch_vcpu.
>>> The initial state marks the VS-file as software-backed until it becomes
>>> associated with a physical CPU.
>>>
>>> Add helpers to retrieve and update the guest interrupt file identifier.
>>
>> Yet again a functions with no callers.
> 
> They will be called in follow-up patches.

In which case please provide some minimal information on the intended use.

>>> --- 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. 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.

>>> @@ -315,6 +338,25 @@ static int imsic_parse_node(const struct 
>>> dt_device_node *node,
>>>       return 0;
>>>   }
>>>   
>>> +int __init vcpu_imsic_init(struct vcpu *v)
>>
>> __init for a function involved in setting up a vCPU?
> 
> Yes, it will be used during creationg of a vCPU.

And vCPU-s can be created post-boot, can't they? (Outside of dom0less
of course, but imo you really don't want to tie each and every function
to dom0less being the primary goal right now.)

Jan