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 Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 2509DC433EF for ; Wed, 9 Mar 2022 12:14:24 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S232606AbiCIMPV (ORCPT ); Wed, 9 Mar 2022 07:15:21 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:49342 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S230022AbiCIMPT (ORCPT ); Wed, 9 Mar 2022 07:15:19 -0500 Received: from ams.source.kernel.org (ams.source.kernel.org [145.40.68.75]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 4B573BD2FE for ; Wed, 9 Mar 2022 04:14:20 -0800 (PST) Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ams.source.kernel.org (Postfix) with ESMTPS id DBBF5B8212B for ; Wed, 9 Mar 2022 12:14:18 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 84755C340E8; Wed, 9 Mar 2022 12:14:17 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1646828057; bh=DBzUzrZXVxY0V/vwcWBhWaoNtaT2PMO3+fnyHLXqVkU=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=oGRjwHnQDaCnEZmeLKCaAIl+6HiZ2T+QXbOAMF4B3AyzSJd0NGscCGkrBwjI/x60o Ncl15MmO2Xke5Ln5leRaQRPtYcX3/7Je1wFEhussSGVkC2l56URqUu4PlIMzHkJqiZ Mk/hZiR8DOgZDp7TUmbtstuxukzemnSiyo+LnuTFKOOE/wSeo1fkrHxGxRIRvO5lfK SWhxY67k1TMJaGV89LIsM91z37Iw+lmHHoBC0U8kJ5ZxXuCZC1Bb2wsk5Gwk/+5V0F T9sXLiFvZISun6f3iegF6bulxSo0Uh6QHIj2kpK8wxyuT2SFnYxiyI68oTNbMtTdxd G2LYOAJsLY6Gg== Received: from sofa.misterjones.org ([185.219.108.64] helo=why.misterjones.org) by disco-boy.misterjones.org with esmtpsa (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.94.2) (envelope-from ) id 1nRvCp-00DJRG-5a; Wed, 09 Mar 2022 12:14:15 +0000 Date: Wed, 09 Mar 2022 12:14:14 +0000 Message-ID: <87tuc7z5k9.wl-maz@kernel.org> From: Marc Zyngier To: "Michael S. Tsirkin" Cc: Jason Wang , Will Deacon , virtualization@lists.linux-foundation.org, linux-kernel@vger.kernel.org, f.hetzelt@tu-berlin.de, david.kaplan@amd.com, konrad.wilk@oracle.com, Boqun Feng , Thomas Gleixner , Peter Zijlstra , "Paul E . McKenney" , keirf@google.com Subject: Re: [PATCH V3 05/10] virtio-pci: harden INTX interrupts In-Reply-To: <20220309060703-mutt-send-email-mst@kernel.org> References: <20211019070152.8236-1-jasowang@redhat.com> <20211019070152.8236-6-jasowang@redhat.com> <87wnh3z9nm.wl-maz@kernel.org> <20220309060703-mutt-send-email-mst@kernel.org> User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI-EPG/1.14.7 (Harue) FLIM-LB/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL-LB/10.8 EasyPG/1.0.0 Emacs/27.1 (x86_64-pc-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: 185.219.108.64 X-SA-Exim-Rcpt-To: mst@redhat.com, jasowang@redhat.com, will@kernel.org, virtualization@lists.linux-foundation.org, linux-kernel@vger.kernel.org, f.hetzelt@tu-berlin.de, david.kaplan@amd.com, konrad.wilk@oracle.com, boqun.feng@gmail.com, tglx@linutronix.de, peterz@infradead.org, paulmck@kernel.org, keirf@google.com X-SA-Exim-Mail-From: maz@kernel.org X-SA-Exim-Scanned: No (on disco-boy.misterjones.org); SAEximRunCond expanded to false Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, 09 Mar 2022 11:27:42 +0000, "Michael S. Tsirkin" wrote: > > On Wed, Mar 09, 2022 at 10:45:49AM +0000, Marc Zyngier wrote: > > [Adding Will to check on my understanding of the interactions between > > spinlocks and WRITE_ONCE()] > > > > On Tue, 19 Oct 2021 08:01:47 +0100, > > Jason Wang wrote: > > > > > > This patch tries to make sure the virtio interrupt handler for INTX > > > won't be called after a reset and before virtio_device_ready(). We > > > can't use IRQF_NO_AUTOEN since we're using shared interrupt > > > (IRQF_SHARED). So this patch tracks the INTX enabling status in a new > > > intx_soft_enabled variable and toggle it during in > > > vp_disable/enable_vectors(). The INTX interrupt handler will check > > > intx_soft_enabled before processing the actual interrupt. > > > > > > Cc: Boqun Feng > > > Cc: Thomas Gleixner > > > Cc: Peter Zijlstra > > > Cc: Paul E. McKenney > > > Signed-off-by: Jason Wang > > > --- > > > drivers/virtio/virtio_pci_common.c | 23 +++++++++++++++++++++-- > > > drivers/virtio/virtio_pci_common.h | 1 + > > > 2 files changed, 22 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c > > > index 8d8f83aca721..1bce254a462a 100644 > > > --- a/drivers/virtio/virtio_pci_common.c > > > +++ b/drivers/virtio/virtio_pci_common.c > > > @@ -30,8 +30,16 @@ void vp_disable_cbs(struct virtio_device *vdev) > > > struct virtio_pci_device *vp_dev = to_vp_device(vdev); > > > int i; > > > > > > - if (vp_dev->intx_enabled) > > > + if (vp_dev->intx_enabled) { > > > + /* > > > + * The below synchronize() guarantees that any > > > + * interrupt for this line arriving after > > > + * synchronize_irq() has completed is guaranteed to see > > > + * intx_soft_enabled == false. > > > + */ > > > + WRITE_ONCE(vp_dev->intx_soft_enabled, false); > > > synchronize_irq(vp_dev->pci_dev->irq); > > > + } > > > > > > for (i = 0; i < vp_dev->msix_vectors; ++i) > > > disable_irq(pci_irq_vector(vp_dev->pci_dev, i)); > > > @@ -43,8 +51,16 @@ void vp_enable_cbs(struct virtio_device *vdev) > > > struct virtio_pci_device *vp_dev = to_vp_device(vdev); > > > int i; > > > > > > - if (vp_dev->intx_enabled) > > > + if (vp_dev->intx_enabled) { > > > + disable_irq(vp_dev->pci_dev->irq); > > > + /* > > > + * The above disable_irq() provides TSO ordering and > > > + * as such promotes the below store to store-release. > > > + */ > > > + WRITE_ONCE(vp_dev->intx_soft_enabled, true); > > > > What do you mean by TSO here? AFAICT, the CPU is allowed hoist this > > write up into the lock used by disable_irq(), as the unlock only has > > release semantics. Is that what you are relying on? I don't see how > > this upgrades WRITE_ONCE() to have release semantics. > > > > > + enable_irq(vp_dev->pci_dev->irq); > > > > Same thing does here: my understanding is that the write can be pushed > > down into the lock, which has acquire semantics only. > > > > Thanks, > > > > M. > > Overall I feel what we are doing here is very standard and should be > pretty common for a driver that wants to be protected against a > malicious device: > > > 1- get IRQ > 2- initialize device with IRQ > 3- enable IRQ > > Doing it in the core kernel helps make sure interrupts are > not lost if they trigger during 2. But this isn't the core kernel. You're doing that in some random driver (and even more, only for the PCI version of that driver). > > Without core kernel support one has to refactor the driver along the lines of: > > a- initialize driver > b- get IRQ > c- initialize device with IRQ > > and this is often tricky especially if one wants to do things like > discover device configuration and reconfigure the driver accordingly. But this isn't what this patch is about, is it? You are just tracking whether interrupts are enabled or not. To which my reply to you on the previous patch still applies (this is the wrong place to track such state). You also haven't answered my question: what are your ordering expectations wrt the WRITE_ONCE() above? The comment says 'TSO', and I don't really understand how this is enforced. Thanks, M. -- Without deviation from the norm, progress is not possible.