Re: [PATCH v2 5/8] evtchn: drop acquiring of per-channel lock from send_guest_{global,vcpu}_virq()

[Julien Grall <julien@xxxxxxx>]

Hi Jan,

On 22/10/2020 17:17, Jan Beulich wrote:
> On 22.10.2020 18:00, Roger Pau Monné wrote:
> > On Tue, Oct 20, 2020 at 04:10:09PM +0200, Jan Beulich wrote:
> > > The per-vCPU virq_lock, which is being held anyway, together with there
> > > not being any call to evtchn_port_set_pending() when v->virq_to_evtchn[]
> > > is zero, provide sufficient guarantees.
> > This is also fine because closing the event channel will be done with
> > the VIRQ hold held also AFAICT.
> Right, I'm not going into these details (or else binding would also
> need mentioning) here, as the code comment update should sufficiently
> cover it. Hence just saying "sufficient guarantees".
>
> > > --- a/xen/include/xen/event.h
> > > +++ b/xen/include/xen/event.h
> > > @@ -177,9 +177,16 @@ int evtchn_reset(struct domain *d, bool
> > >    * Low-level event channel port ops.
> > >    *
> > >    * All hooks have to be called with a lock held which prevents the channel
> > > - * from changing state. This may be the domain event lock, the per-channel
> > > - * lock, or in the case of sending interdomain events also the other side's
> > > - * per-channel lock. Exceptions apply in certain cases for the PV shim.
> > > + * from changing state. This may be
> > > + * - the domain event lock,
> > > + * - the per-channel lock,
> > > + * - in the case of sending interdomain events the other side's per-channel
> > > + *   lock,
> > > + * - in the case of sending non-global vIRQ-s the per-vCPU virq_lock (in
> > > + *   combination with the ordering enforced through how the vCPU's
> > > + *   virq_to_evtchn[] gets updated),
> > > + * - in the case of sending global vIRQ-s vCPU 0's virq_lock.
> > > + * Exceptions apply in certain cases for the PV shim.
> > Having such a wide locking discipline looks dangerous to me, it's easy
> > to get things wrong without notice IMO.
> It is effectively only describing how things are (or were before
> XSA-343, getting restored here).
I agree with Roger here, the new/old locking discipline is dangerous and 
it is only a matter of time before it will bite us again.
I think we should consider Juergen's series because the locking for the 
event channel is easier to understand.
With his series in place, this patch will become unecessary.

Cheers,

--
Julien Grall