Re: [PATCH v2 13/26] xen/riscv: introduce per-vCPU IMSIC state

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

On 5/21/26 5:24 PM, Jan Beulich wrote:
> On 08.05.2026 16:43, Oleksii Kurochko wrote:
>> --- a/xen/arch/riscv/imsic.c
>> +++ b/xen/arch/riscv/imsic.c
>> @@ -16,6 +16,7 @@
>>   #include <xen/errno.h>
>>   #include <xen/init.h>
>>   #include <xen/macros.h>
>> +#include <xen/sched.h>
>>   #include <xen/smp.h>
>>   #include <xen/spinlock.h>
>>   #include <xen/xvmalloc.h>
>> @@ -56,6 +57,16 @@ do {                            \
>>       csr_clear(CSR_SIREG, v);    \
>>   } while (0)
>>   
>> +unsigned int vcpu_guest_file_id(const struct vcpu *v)
>> +{
>> +    return ACCESS_ONCE(v->arch.vimsic_state->guest_file_id);
>> +}
>> +
>> +void imsic_set_guest_file_id(const struct vcpu *v, unsigned int guest_file_id)
> 
> Some people will demand that "const" be omitted in cases like this one, as
> it only works ...
> 
>> +{
>> +    ACCESS_ONCE(v->arch.vimsic_state->guest_file_id) = guest_file_id;
> 
> ... as long as vimsic_state is a pointer (and not a sub-structure).

Agree, it works only because of a pointer is used. Lets ommit const then 
here.

> 
>> @@ -312,6 +323,30 @@ static int imsic_parse_node(const struct dt_device_node *node,
>>       return 0;
>>   }
>>   
>> +int vcpu_imsic_init(struct vcpu *v)
>> +{
>> +    struct vimsic_state *imsic_state;
>> +
>> +    /* Allocate IMSIC context */
>> +    imsic_state = xvzalloc(struct vimsic_state);
>> +    if ( !imsic_state )
>> +        return -ENOMEM;
>> +
>> +    v->arch.vimsic_state = imsic_state;
>> +
>> +    /* Setup IMSIC context  */
>> +    rwlock_init(&imsic_state->vsfile_lock);
>> +
>> +    imsic_state->vsfile_pcpu = NR_CPUS;
>> +
>> +    return 0;
>> +}
>> +
>> +void vcpu_imsic_deinit(const struct vcpu *v)
>> +{
>> +    xvfree(v->arch.vimsic_state);
> 
> Better XVFREE(), for the function to be idempotent.

Agree it would be better.

> 
>> --- a/xen/arch/riscv/include/asm/imsic.h
>> +++ b/xen/arch/riscv/include/asm/imsic.h
>> @@ -11,6 +11,7 @@
>>   #ifndef ASM_RISCV_IMSIC_H
>>   #define ASM_RISCV_IMSIC_H
>>   
>> +#include <xen/rwlock.h>
>>   #include <xen/spinlock.h>
>>   #include <xen/stdbool.h>
>>   #include <xen/types.h>
>> @@ -61,7 +62,24 @@ struct imsic_config {
>>       spinlock_t lock;
>>   };
>>   
>> +struct vimsic_state {
>> +    /* IMSIC VS-file */
>> +    rwlock_t vsfile_lock;
>> +    /*
>> +     * (guest_file_id == 0) -> s/w IMSIC SW-file
>> +     * (guest_file_id > 0) -> h/w IMSIC VS-file
>> +     */
>> +    unsigned int guest_file_id;
>> +    /*
>> +     * (vsfile_pcpu >= 0) => h/w IMSIC VS-file
>> +     * (vsfile_pcpu == NR_CPUS) => s/w IMSIC SW-file
>> +     */
>> +    unsigned int vsfile_pcpu;
>> +};
> 
> In the comments, what does SW stand for? Not "software" I assume, as
> that's already expressed by s/w.

It is a typo and it should be VS-file.

Thanks.

~ Oleksii