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

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

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