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

Oleksii Kurochko posted 27 patches 4 weeks ago
[PATCH v1 14/27] xen/riscv: introduce per-vCPU IMSIC state
Posted by Oleksii Kurochko 4 weeks ago
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.

Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
 xen/arch/riscv/imsic.c              | 42 +++++++++++++++++++++++++++++
 xen/arch/riscv/include/asm/domain.h |  2 ++
 xen/arch/riscv/include/asm/imsic.h  | 17 ++++++++++++
 3 files changed, 61 insertions(+)

diff --git a/xen/arch/riscv/imsic.c b/xen/arch/riscv/imsic.c
index 0956b187705f..bbadbdf352a1 100644
--- 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);
+
+    return vsfile_id;
+}
+
+void imsic_set_guest_file_id(const struct vcpu *v, unsigned int guest_file_id)
+{
+    struct imsic_state *imsic_state = v->arch.imsic_state;
+    unsigned long flags;
+
+    write_lock_irqsave(&imsic_state->vsfile_lock, flags);
+    imsic_state->guest_file_id = guest_file_id;
+    write_unlock_irqrestore(&imsic_state->vsfile_lock, flags);
+}
+
 void __init imsic_ids_local_delivery(bool enable)
 {
     if ( enable )
@@ -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)
+{
+    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;
+
+    return 0;
+}
+
 /*
  * Initialize the imsic_cfg structure based on the IMSIC DT node.
  *
diff --git a/xen/arch/riscv/include/asm/domain.h b/xen/arch/riscv/include/asm/domain.h
index 506365f199c7..bdb1ffd748c9 100644
--- 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;
+
     register_t hcounteren;
     register_t hedeleg;
     register_t hideleg;
diff --git a/xen/arch/riscv/include/asm/imsic.h b/xen/arch/riscv/include/asm/imsic.h
index a63d56fbd5d9..13a563dce066 100644
--- 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>
@@ -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;
+};
+
 struct dt_device_node;
 struct kernel_info;
+struct vcpu;
 
 int imsic_init(const struct dt_device_node *node);
 
@@ -78,4 +91,8 @@ void imsic_ids_local_delivery(bool enable);
 
 int imsic_make_dt_node(const struct kernel_info *kinfo);
 
+int vcpu_imsic_init(struct vcpu *v);
+unsigned int vcpu_guest_file_id(const struct vcpu *v);
+void imsic_set_guest_file_id(const struct vcpu *v, unsigned int guest_file_id);
+
 #endif /* ASM_RISCV_IMSIC_H */
-- 
2.53.0
Re: [PATCH v1 14/27] xen/riscv: introduce per-vCPU IMSIC state
Posted by Jan Beulich 5 days, 15 hours ago
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