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

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 4/16/26 2:31 PM, Jan Beulich wrote:
> On 14.04.2026 11:22, Oleksii Kurochko wrote:
> > On 4/2/26 1:31 PM, Jan Beulich wrote:
> > > On 10.03.2026 18:08, Oleksii Kurochko wrote:
> > > > --- 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.
> None of this addresses my pointing out that the returned value will be
> stale by the point the caller gets to look at it.
Yes, I agree that lock in vcpu_guest_file_id() is useless and it should 
be on the caller side and used for the whole IMSIC state access. But ...
> Which in turn raises
> said question about the use of a lock. If you read
> imsic_state->guest_file_id atomically (i.e. excluding tearing of reads),
> the value seen / used will be stale as with the lock in use. Unless of
> course there's yet another aspect hidden somewhere in what is not being
> explained.
... I am not sure that I get this part.

If I am somewhere in migration code where I took write lock to update 
imsic state (and of course ->guest_file_id as part of it) then if 
someone else in parallel calls imsic_has_interrupt() then it won't enter 
critical section where ->guest_file_id is trying to be read so no stale 
->guest_file_id will be read.
Then does it make sense to use ACCESS_ONCE() during read and write of
->guest_file_id in such use cases?

~ Oleksii