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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

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

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

> +{
> +    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?

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

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?

Jan