Re: [PATCH 11/12] evtchn: convert vIRQ lock to an r/w one

[Jan Beulich <jbeulich@xxxxxxxx>]

On 01.10.2020 18:21, Julien Grall wrote:
> On 30/09/2020 11:16, Jan Beulich wrote:
>> On 30.09.2020 10:52, Paul Durrant wrote:
>>> Looking again, given that both send_guest_vcpu_virq() and
>>> send_guest_global_virq() (rightly) hold the evtchn lock before
>>> calling evtchn_port_set_pending() I think you could do away with
>>> the virq lock by adding checks in those functions to verify
>>> evtchn->state == ECS_VIRQ and u.virq == virq after having
>>> acquired the channel lock but before calling
>>> evtchn_port_set_pending().
>>
>> I don't think so: The adjustment of v->virq_to_evtchn[] in
>> evtchn_close() would then happen with just the domain's event
>> lock held, which the sending paths don't use at all. The per-
>> channel lock gets acquired in evtchn_close() a bit later only
>> (and this lock can't possibly protect per-vCPU state).
>>
>> In fact I'm now getting puzzled by evtchn_bind_virq() updating
>> this array with (just) the per-domain lock held. Since it's
>> the last thing in the function, there's technically no strict
>> need for acquiring the vIRQ lock,
> 
> Well, we at least need to prevent the compiler to tear the store/load. 
> If we don't use a lock, then we would should use ACCESS_ONCE() or 
> {read,write}_atomic() for all the usage.
> 
>> but holding the event lock
>> definitely doesn't help.
> 
> It helps because spin_unlock() and write_unlock() use the same barrier 
> (arch_lock_release_barrier()). So ...

I'm having trouble making this part of your reply fit ...

>> All that looks to be needed is the
>> barrier implied from write_unlock().
> 
> No barrier should be necessary. Although, I would suggest to add a 
> comment explaining it.

... this. If we moved the update of v->virq_to_evtchn[] out of the
locked region (as the lock doesn't protect anything anymore at that
point), I think a barrier would need adding, such that the sending
paths will observe the update by the time evtchn_bind_virq()
returns (and hence sending of a respective vIRQ event can
legitimately be expected to actually work). Or did you possibly
just misunderstand what I wrote before? By putting in question the
utility of holding the event lock, I implied the write could be
moved out of the locked region ...

Jan