From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-7.1 required=3.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY, SPF_HELO_NONE,SPF_PASS autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 86052C432C0 for ; Wed, 27 Nov 2019 18:20:26 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 5FCD92070B for ; Wed, 27 Nov 2019 18:20:26 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1574878826; bh=RWq548uZwXZHvZHEpFkh8vtHWlNDXK/JIQn/WYACwko=; h=Date:From:To:Cc:Subject:In-Reply-To:References:List-ID:From; b=w+q38aWvC4lATA9R+gcoYw7O1FaCHVdhMDkosi2UTECwaEwo5vJNBoRu3S8wawaCM uuvT+VtZMdfqAzjks3cx8+z3vErckCdwJjpYTsE9CCHjPYoIPScWFF39ww0NdZ0PiC y5Ztbyaq7HF7+sxMxvf8IlrJsP2N14Zc3RhWraDU= Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1727071AbfK0SUZ (ORCPT ); Wed, 27 Nov 2019 13:20:25 -0500 Received: from inca-roads.misterjones.org ([213.251.177.50]:58592 "EHLO inca-roads.misterjones.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726633AbfK0SUZ (ORCPT ); Wed, 27 Nov 2019 13:20:25 -0500 Received: from 29-193-142-46.pool.kielnet.net ([46.142.193.29] helo=big-swifty.misterjones.org) by cheepnis.misterjones.org with esmtpsa (TLSv1.2:AES256-GCM-SHA384:256) (Exim 4.80) (envelope-from ) id 1ia1vL-000491-Q4; Wed, 27 Nov 2019 19:20:23 +0100 Date: Wed, 27 Nov 2019 18:20:14 +0000 Message-ID: <86sgm9yye9.wl-maz@kernel.org> From: Marc Zyngier To: Jiang Yi Cc: kvm@vger.kernel.org, adulea@amazon.de, jschoenh@amazon.de, alex.williamson@redhat.com, cohuck@redhat.com Subject: Re: [PATCH] vfio: call irq_bypass_unregister_producer() before freeing irq In-Reply-To: <20191127164910.15888-1-giangyi@amazon.com> References: <20191127164910.15888-1-giangyi@amazon.com> User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI-EPG/1.14.7 (Harue) FLIM/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL/10.8 EasyPG/1.0.0 Emacs/26 (aarch64-unknown-linux-gnu) MULE/6.0 (HANACHIRUSATO) MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue") Content-Type: text/plain; charset=US-ASCII X-SA-Exim-Connect-IP: 46.142.193.29 X-SA-Exim-Rcpt-To: giangyi@amazon.com, kvm@vger.kernel.org, adulea@amazon.de, jschoenh@amazon.de, alex.williamson@redhat.com, cohuck@redhat.com X-SA-Exim-Mail-From: maz@kernel.org X-SA-Exim-Scanned: No (on cheepnis.misterjones.org); SAEximRunCond expanded to false Sender: kvm-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: kvm@vger.kernel.org On Wed, 27 Nov 2019 16:49:10 +0000, Jiang Yi wrote: Hi Jiang, Thanks for spotting this! > Since irq_bypass_register_producer() is called after request_irq(), we > should do tear-down in reverse order: irq_bypass_unregister_producer() > then free_irq(). More importantly, free_irq() is going to releases resources that can still be required by the del_producer callback. Notably, for arm64 and GICv4: free_irq(irq) __free_irq(irq) irq_domain_deactivate_irq(irq) its_irq_domain_deactivate() [unmap the VLPI from the ITS] kvm_arch_irq_bypass_del_producer(cons, prod) kvm_vgic_v4_unset_forwarding(kvm, irq, ...) its_unmap_vlpi(irq) [Unmap the VLPI from the ITS (again), remap the original LPI] which isn't great, and has the potential to wedge the HW. Reversing the two makes more sense: Unmap the VLPI, remap the LPI, and finally unmap the LPI. I haven't checked what it does with VT-D. > Signed-off-by: Jiang Yi > --- > drivers/vfio/pci/vfio_pci_intrs.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/vfio/pci/vfio_pci_intrs.c b/drivers/vfio/pci/vfio_pci_intrs.c > index 3fa3f728fb39..2056f3f85f59 100644 > --- a/drivers/vfio/pci/vfio_pci_intrs.c > +++ b/drivers/vfio/pci/vfio_pci_intrs.c > @@ -289,18 +289,18 @@ static int vfio_msi_set_vector_signal(struct vfio_pci_device *vdev, > int irq, ret; > > if (vector < 0 || vector >= vdev->num_ctx) > return -EINVAL; > > irq = pci_irq_vector(pdev, vector); > > if (vdev->ctx[vector].trigger) { > - free_irq(irq, vdev->ctx[vector].trigger); > irq_bypass_unregister_producer(&vdev->ctx[vector].producer); > + free_irq(irq, vdev->ctx[vector].trigger); > kfree(vdev->ctx[vector].name); > eventfd_ctx_put(vdev->ctx[vector].trigger); > vdev->ctx[vector].trigger = NULL; > } > > if (fd < 0) > return 0; > > -- > 2.17.1 > > FWIW: Cc: stable@vger.kernel.org # v4.4+ Fixes: 6d7425f109d26 ("vfio: Register/unregister irq_bypass_producer") Reviewed-by: Marc Zyngier Thanks again, M. -- Jazz is not dead, it just smells funny.