From mboxrd@z Thu Jan 1 00:00:00 1970 From: David Vrabel Subject: Re: [PATCH 3/3] evtchn/fifo: don't spin indefinitely when setting LINK Date: Mon, 4 Nov 2013 16:30:33 +0000 Message-ID: <5277CBA9.1060006@citrix.com> References: <1383231791-4604-1-git-send-email-david.vrabel@citrix.com> <1383231791-4604-4-git-send-email-david.vrabel@citrix.com> <5277BF9F02000078000FF1F2@nat28.tlf.novell.com> <5277B4C2.5000508@citrix.com> <5277C3EC02000078000FF21F@nat28.tlf.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta14.messagelabs.com ([193.109.254.103]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1VdN32-0007ZQ-Cp for xen-devel@lists.xenproject.org; Mon, 04 Nov 2013 16:30:40 +0000 In-Reply-To: <5277C3EC02000078000FF21F@nat28.tlf.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich Cc: xen-devel , Keir Fraser List-Id: xen-devel@lists.xenproject.org On 04/11/13 14:57, Jan Beulich wrote: >>>> On 04.11.13 at 15:52, David Vrabel wrote: >> On 04/11/13 14:39, Jan Beulich wrote: >>>>>> On 31.10.13 at 16:03, David Vrabel wrote: >>>> If the buggy or malicious guest does cause the loop to exit early, the >>>> newly pending event will be unreachable by the guest and it and >>>> subsequent events may be lost. >>> >>> ... yet here it is not really clear which guest the last "guest" refers >>> to (i.e. it's fine if the malicious guest harms itself, but the change >>> would be pointless if the malicious guest could still harm the other >>> one). >> >> The malicious guest loses the event. > > So then "that guest" would perhaps be more precise than "the > guest"? Yes. I'll improve the commit message to be clearer. >>>> @@ -90,6 +92,8 @@ static void evtchn_fifo_set_pending(struct vcpu *v, struct evtchn *evtchn) >>>> event_word_t *tail_word; >>>> bool_t linked = 0; >>>> >>>> + evtchn->q = q; >>>> + >>>> spin_lock_irqsave(&q->lock, flags); >>>> >>>> /* >>> >>> I fail to see how this change is related to the rest of the patch. >> >> This is needed so the correct queue lock is used in evtchn_fifo_unmask(). > > I guessed that, but I wasn't able to immediately see why that would > not have been a requirement before. An event now has two queues associated with it. 1. The queue it is supposed to be on. This is found with evtchn->notify_vcpu_id and evtchn->priority when an event is being linked. 2. The queue the event is on right now (the new evtchn->q), until this patch we didn't care which queue an event was on, only that it was on some queue. These are not necessarily the same since an event may be moved between VCPUs or have its priority changed while it is already on a queue. Some additional notes on the locking: The locking in the FIFO-based ABI is designed on the assumption that every hypervisor operation on an event channel (set pending, unmask) is done while holding a per-event channel spin lock. Currently, the per-event locking is done with the coarser per-domain event_lock. The queue lock serializes adding events and clearing MASKED on tail events. The event lock also protects evtchn->q and the evtchn->q->tail == evtchn->port test. David