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

[Julien Grall <julien@xxxxxxx>]

On 27/11/2020 14:39, Jürgen Groß wrote:
> On 27.11.20 15:23, Julien Grall wrote:
> > On 25/11/2020 10:51, Juergen Gross wrote:
> > > evtchn_fifo_set_pending() can be simplified a little bit.
> > The commit message is quite light... For posterity, it would be good 
> > to explain why the simplication can be done. In particular, there is a 
> > chance in behavior after this patch.
> > > Suggested-by: Jan Beulich <jbeulich@xxxxxxxx>
> > > Signed-off-by: Juergen Gross <jgross@xxxxxxxx>
> > > ---
> > > V8:
> > > - new patch
> > > ---
> > >   xen/common/event_fifo.c | 34 +++++++++++++++-------------------
> > >   1 file changed, 15 insertions(+), 19 deletions(-)
> > >
> > > diff --git a/xen/common/event_fifo.c b/xen/common/event_fifo.c
> > > index 443593c3b3..77609539b1 100644
> > > --- 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.
Yes it should. Sorry I should have been clearer.

I am happy to also wrote the patch if you feel you had enough with event 
channel :).
Cheers,

--
Julien Grall