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

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

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