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

[Jan Beulich <jbeulich@xxxxxxxx>]

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. This recurring pattern
shouldn't lead to unrelated patches getting bloated, unless _all_
affected sites get touched anyway.

Jan