Re: [PATCH v2 6/8] evtchn: convert vIRQ lock to an r/w one

[Julien Grall <julien@xxxxxxx>]

On 30/10/2020 12:00, Jan Beulich wrote:
> On 30.10.2020 11:57, Julien Grall wrote:
> > On 20/10/2020 15:10, Jan Beulich wrote:
> > > --- a/xen/common/event_channel.c
> > > +++ b/xen/common/event_channel.c
> > > @@ -449,6 +449,13 @@ int evtchn_bind_virq(evtchn_bind_virq_t
> > >    
> > >        spin_unlock_irqrestore(&chn->lock, flags);
> > >    
> > > +    /*
> > > +     * If by any, the update of virq_to_evtchn[] would need guarding by
> > > +     * virq_lock, but since this is the last action here, there's no strict
> > > +     * need to acquire the lock. Hnece holding event_lock isn't helpful
> > s/Hnece/Hence/
> >
> > > +     * anymore at this point, but utilize that its unlocking acts as the
> > > +     * otherwise necessary smp_wmb() here.
> > > +     */
> > >        v->virq_to_evtchn[virq] = bind->port = port;
> > I think all access to v->virq_to_evtchn[virq] should use ACCESS_ONCE()
> > or {read, write}_atomic() to avoid any store/load tearing.
> IOW you're suggesting this to be the subject of a separate patch?
> I don't think such a conversion belongs here (nor even in this
> series, seeing the much wider applicability of such a change
> throughout the code base).
> Or are you seeing anything here which
> would require such a conversion to be done as a prereq?
Yes, your comment implies that it is fine to write to virq_to_evtchn[] 
without the lock taken. However, this is *only* valid if the compiler 
doesn't tear down load/store.
So this is a pre-req to get your comment valid.

Cheers,

--
Julien Grall