Re: [PATCH v8 3/3] xen/events: do some cleanups in evtchn_fifo_set_pending()

[Julien Grall <julien@xxxxxxx>]

On 27/11/2020 14:48, Jan Beulich wrote:
> On 27.11.2020 15:39, Jürgen Groß wrote:
> > On 27.11.20 15:23, Julien Grall wrote:
> > > On 25/11/2020 10:51, Juergen Gross wrote:
> > > > --- a/xen/common/event_fifo.c
> > > > +++ b/xen/common/event_fifo.c
> > > > @@ -175,6 +175,18 @@ static void evtchn_fifo_set_pending(struct vcpu
> > > > *v, struct evtchn *evtchn)
> > > >            return;
> > > >        }
> > > > +    /*
> > > > +     * Control block not mapped.  The guest must not unmask an
> > > > +     * event until the control block is initialized, so we can
> > > > +     * just drop the event.
> > > > +     */
> > > > +    if ( unlikely(!v->evtchn_fifo->control_block) )
> > > Sort of unrelated, AFAICT, v->evtchn_fifo->control_block can be set
> > > concurrently to this access.
> > >
> > > Thankfully, once the control block is mapped, it can't be unmapped.
> > > However, there is still a possibility that you may see half of the update.
> > >
> > > Shouldn't the field access with ACCESS_ONCE()?
> > Shouldn't this be another patch? Especially as the writing side needs
> > the same treatment.
> Indeed. As said on several different occasions - our code base is
> full of places where we chance torn accesses, if there really was
> a compiler to let us down on this.
I am quite amazed you that you managed to test all the version of 
GCC/Clang that were built and confirm this is unlikely to happen :).
> This recurring pattern
> shouldn't lead to unrelated patches getting bloated, unless _all_
> affected sites get touched anyway.
You probably missed the point where I say "sort of unrelated". This 
wasn't not a suggestion to fix it here (I should have been clearer 
though) but instead point out issue as I see them.
Cheers,

--
Julien Grall