On 30.09.2020 09:31, Paul Durrant wrote: >> -----Original Message----- >> From: Jan Beulich <[email protected]> >> Sent: 30 September 2020 07:42 >> To: [email protected] >> Cc: [email protected]; 'Andrew Cooper' >> <[email protected]>; 'George Dunlap' >> <[email protected]>; 'Ian Jackson' <[email protected]>; 'Julien >> Grall' <[email protected]>; >> 'Wei Liu' <[email protected]>; 'Stefano Stabellini' <[email protected]> >> Subject: Re: [PATCH 04/12] evtchn: evtchn_set_priority() needs to acquire >> the per-channel lock >> >> On 29.09.2020 18:31, Paul Durrant wrote: >>>> -----Original Message----- >>>> From: Xen-devel <[email protected]> On Behalf Of Jan >>>> Beulich >>>> Sent: 28 September 2020 11:58 >>>> To: [email protected] >>>> Cc: Andrew Cooper <[email protected]>; George Dunlap >>>> <[email protected]>; Ian >>>> Jackson <[email protected]>; Julien Grall <[email protected]>; Wei Liu >>>> <[email protected]>; Stefano >> Stabellini >>>> <[email protected]> >>>> Subject: [PATCH 04/12] evtchn: evtchn_set_priority() needs to acquire the >>>> per-channel lock >>>> >>>> evtchn_fifo_set_pending() (invoked with the per-channel lock held) has >>>> two uses of the channel's priority field. >>> >>> AFAICT it is invoked with only the sending end's lock held... >>> >>>> The field gets updated by >>>> evtchn_fifo_set_priority() with only the per-domain event_lock held, >>>> i.e. the two reads may observe two different values. While the 2nd use >>>> could - afaict - in principle be replaced by q->priority, I think >>>> evtchn_set_priority() should acquire the per-channel lock in any event. >>>> >>> >>> ... so how is this going to help? >> >> I guess the reasoning needs to change here - it should focus solely >> on using the finer grained lock here (as holding the per-domain one >> doesn't help anyway). It would then be patch 10 which addresses the >> (FIFO-specific) concern of possibly reading inconsistent values. >> > > Yes, it looks like patch #10 should ensure consistency. > > Prior to ad34d0656fc at least the first layer of calls done in evtchn_send() > didn't take the evtchn itself as an arg. Of course, evtchn_set_pending() then > looked up the evtchn and passed it to evtchn_port_set_pending() without any > locking in the interdomain case. I wonder whether, to make reasoning easier, > there ought to be a rule that ABI entry points are always called with the > evtchn lock held?
What do you mean by "ABI entry points" here? To me this would sound like what's directly accessible to guests, but that's hardly what you mean. Do you perhaps mean the hooks in struct evtchn_port_ops? As per the comment that got added there recently, the locking unfortunately is less consistent there. Jan
