Michael S. Tsirkin
2022-Mar-09 12:30 UTC
[PATCH V3 05/10] virtio-pci: harden INTX interrupts
On Wed, Mar 09, 2022 at 12:14:14PM +0000, Marc Zyngier wrote:> On Wed, 09 Mar 2022 11:27:42 +0000, > "Michael S. Tsirkin" <mst at redhat.com> 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 <jasowang at redhat.com> 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 <boqun.feng at gmail.com> > > > > Cc: Thomas Gleixner <tglx at linutronix.de> > > > > Cc: Peter Zijlstra <peterz at infradead.org> > > > > Cc: Paul E. McKenney <paulmck at kernel.org> > > > > Signed-off-by: Jason Wang <jasowang at redhat.com> > > > > --- > > > > 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).Oh, I'm answering on the INTX patch. I was referring to the MSIX path actually. Yes INTX can not currently start in disabled state because it's shared.> > > > 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).It's not that we care. What we need is to make sure that interrupts sent before driver is ready to accept them do not cause a callback. If there was a way to tell kernel "I'm ready to accept callbacks now" problem would be solved.> 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.It's based on Peter's suggestion: https://lore.kernel.org/r/YUCBZjjk77q8JS4f%40hirez.programming.kicks-ass.net> -- > Without deviation from the norm, progress is not possible.