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

[Julien Grall <julien@xxxxxxx>]

Hi Jan,

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 ...
> 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.
Cheers,

--
Julien Grall