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

[Oleksii Kurochko <oleksii.kurochko@gmail.com>]

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.


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

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

> 
>> +{
>> +    struct imsic_state *imsic_state;
>> +
>> +    /* Allocate IMSIC context */
>> +    imsic_state = xvzalloc(struct imsic_state);
>> +    if ( !imsic_state )
>> +        return -ENOMEM;
>> +
>> +    v->arch.imsic_state = imsic_state;
>> +
>> +    /* Setup IMSIC context  */
>> +    rwlock_init(&imsic_state->vsfile_lock);
>> +
>> +    imsic_state->guest_file_id = imsic_state->vsfile_pcpu = NR_CPUS;
> 
> Iirc Misra dislikes such double assignments, so better avoid them right away.
> (As per a comment at the bottom this may need splitting anyway.)
> 
>> --- a/xen/arch/riscv/include/asm/domain.h
>> +++ b/xen/arch/riscv/include/asm/domain.h
>> @@ -52,6 +52,8 @@ struct arch_vcpu {
>>   
>>       struct vtimer vtimer;
>>   
>> +    struct imsic_state *imsic_state;
> 
> Just like it's "vtimer", perhaps also "vimsic_state" for both the field
> and the struct tag?

Makes sense. Lets rename this field and tag.

> 
>> @@ -64,8 +65,20 @@ struct imsic_config {
>>       spinlock_t lock;
>>   };
>>   
>> +struct imsic_state {
>> +    /* IMSIC VS-file */
>> +    rwlock_t vsfile_lock;
>> +    unsigned int guest_file_id;
>> +    /*
>> +     * (vsfile_pcpu >= 0) => h/w IMSIC VS-file
>> +     * (vsfile_pcpu == NR_CPUS) => s/w IMSIC SW-file
>> +     */
>> +    unsigned long vsfile_pcpu;
> 
> And why unsigned long, when unsigned int will do (as about everywhere else
> for CPU numbers)? That'll also shrink the structure size by 8 bytes.

Originally, IIRC mhartid register is unsigned long, so potentially we 
could have amount of pCPU up to what fit into unsigned long.

But know I see that AIA limits amount of pCPUs to 16,384 harts. So we 
could really use unsigned int here.

> 
> As to the comment - as per vcpu_imsic_init() NR_CPUS also has some special
> meaning for guest_file_id, yet there's no comment there. How do file ID and
> NR_CPUS fit together anyway?

Agree, it looks incorrect. I tried to avoid introduction of "#define 
GIELEN_MAX 64".
As an option I can use UINT_MAX as init value for guest_file_id or even 
better just use 0 as it basically means that SW-file should be used.

The idea is that if s/w IMSIC SW-file is used that it doesn't sit on any 
pCPU, so we should have some marker to show that and NR_CPUS should be fine.

As a result guest_file_id should be init-ed with 0 (and it will be by 
default as xvzalloc() is used for allocation) and ->vsfile_pcpu inits 
with NR_CPUS. Also, I will add a comment above guest_file_id.

Thanks.

~ Oleksii