From mboxrd@z Thu Jan 1 00:00:00 1970 From: Konrad Rzeszutek Wilk Subject: Re: [PATCHv1 1/2] passthrough: use per-interrupt lock when injecting an interrupt Date: Fri, 30 Oct 2015 10:45:33 -0400 Message-ID: <20151030144533.GB22037@l.oracle.com> References: <1445598322-22154-1-git-send-email-david.vrabel@citrix.com> <1445598322-22154-2-git-send-email-david.vrabel@citrix.com> <562F747202000078000AF16A@prv-mh.provo.novell.com> <20151028201853.GA21384@l.oracle.com> <5631F0DB02000078000AFBE9@prv-mh.provo.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta3.messagelabs.com ([195.245.230.39]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1ZsAw0-0001IO-Vu for xen-devel@lists.xenproject.org; Fri, 30 Oct 2015 14:45:41 +0000 Content-Disposition: inline In-Reply-To: <5631F0DB02000078000AFBE9@prv-mh.provo.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@lists.xenproject.org, David Vrabel , Ian Campbell List-Id: xen-devel@lists.xenproject.org On Thu, Oct 29, 2015 at 03:11:39AM -0600, Jan Beulich wrote: > >>> On 28.10.15 at 21:18, wrote: > >> > @@ -481,6 +489,8 @@ int pt_irq_destroy_bind( > >> > pirq = pirq_info(d, machine_gsi); > >> > pirq_dpci = pirq_dpci(pirq); > >> > > >> > + spin_lock(&pirq_dpci->lock); > >> > >> Considering that code further down in this function checks > >> pirq_dpci to be non-NULL, this doesn't look safe (or else those > >> checks should go away, as after this addition they would be > >> likely to trigger e.g. Coverity warnings). > > > > ? The checks are for pirq_dpci->dom. > > What about > > /* clear the mirq info */ > if ( pirq_dpci && (pirq_dpci->flags & HVM_IRQ_DPCI_MAPPED) ) > > and > > if ( pirq_dpci && (pirq_dpci->flags & HVM_IRQ_DPCI_MAPPED) && > list_empty(&pirq_dpci->digl_list) ) Yes. I was looking at the function code (pt_irq_create_bind) not the pt_irq_destroy_bind! > > ? In fact I can't spot any access to pirq_dpci->dom in this function. > > >> > @@ -675,7 +687,7 @@ static void hvm_dirq_assist(struct domain *d, struct hvm_pirq_dpci *pirq_dpci) > >> > { > >> > ASSERT(d->arch.hvm_domain.irq.dpci); > >> > > >> > - spin_lock(&d->event_lock); > >> > + spin_lock(&pirq_dpci->lock); > >> > if ( test_and_clear_bool(pirq_dpci->masked) ) > >> > { > >> > struct pirq *pirq = dpci_pirq(pirq_dpci); > >> > >> Along the same lines - it's not obvious that the uses of pirq here are > >> safe with event_lock no longer held. In particular I wonder about > >> send_guest_pirq() - despite the other use in __do_IRQ_guest() not > >> doing any locking either I'm not convinced this is correct. > > > > > > It seems that the event channel mechanism only uses the event channel > > lock when expanding and initializing (FIFO). For the old mechanism > > it was for binding, closing (uhuh), status, reset, and set_priority. > > Well, the event lock is also used for some pIRQ management iirc. Correct - pirq_guest_bind, pirq_guest_unbind, map_domain_pirq, map_domain_emuirq_pirq, unmap_domain_pirq_emuirq, and so on. > > Jan >