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

[Julien Grall <julien@xen.org>]

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