On 30.10.2020 13:08, Julien Grall wrote:
> 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.
That's an interesting way to view it. I'm only adding the comment,
without changing the code here. Iirc it was you who asked me to
add the comment. And now this is leading to you wanting me to do
entirely unrelated changes, when the code base is full of similar
assumptions towards no torn accesses? (Yes, I certainly can add
such a patch, but no, I don't think this should be a requirement
here.)
Jan
