On Mon, Sep 13, 2021 at 02:45:38PM +0800, Jason Wang wrote:

On Mon, Sep 13, 2021 at 2:41 PM Michael S. Tsirkin [off-list ref] wrote:

quoted

On Mon, Sep 13, 2021 at 02:36:54PM +0800, Jason Wang wrote:

quoted

On Mon, Sep 13, 2021 at 2:33 PM Michael S. Tsirkin [off-list ref] wrote:

quoted

On Mon, Sep 13, 2021 at 01:53:51PM +0800, Jason Wang wrote:

quoted

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.

Signed-off-by: Jason Wang <jasowang@redhat.com>

Not all that excited about all the memory barriers for something
that should be an extremely rare event (for most kernels -
literally once per boot). Can't we do better?

I'm not sure, but do we need to care about the slow path (INTX)?

Otherwise we won't try to support this, right?

Sorry, what I meant is "do we need to care about the performance of
the slow path".

quoted

quoted

(Or do you have a better approach?)

Thanks

Don't know really, maybe rcu or whatever?

I am sure it's worth it to bother since it's the slow path.

quoted

But let's try to be much more specific - is there anything
specific we are trying to protect against here?

The unexpected calling of the vring or config interrupt handler. (The
same as MSI-X, e.g the untrusted device can send irq at any time).

Thanks

And so, does this do more than crash the guest?  Hypervisors
already can do that ...

quoted

quoted

quoted

quoted

---
 drivers/virtio/virtio_pci_common.c | 18 ++++++++++++++++--
 drivers/virtio/virtio_pci_common.h |  1 +
 2 files changed, 17 insertions(+), 2 deletions(-)

diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c
index 0b9523e6dd39..835197151dc1 100644
--- a/drivers/virtio/virtio_pci_common.c
+++ b/drivers/virtio/virtio_pci_common.c

@@ -30,8 +30,12 @@ void vp_disable_vectors(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) {
+             vp_dev->intx_soft_enabled = false;
+             /* ensure the vp_interrupt see this intx_soft_enabled value */
+             smp_wmb();
              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 +47,12 @@ void vp_enable_vectors(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) {
+             vp_dev->intx_soft_enabled = true;
+             /* ensure the vp_interrupt see this intx_soft_enabled value */
+             smp_wmb();
              return;
+     }

      for (i = 0; i < vp_dev->msix_vectors; ++i)
              enable_irq(pci_irq_vector(vp_dev->pci_dev, i));

@@ -97,6 +105,12 @@ static irqreturn_t vp_interrupt(int irq, void *opaque)
      struct virtio_pci_device *vp_dev = opaque;
      u8 isr;

+     if (!vp_dev->intx_soft_enabled)
+             return IRQ_NONE;
+
+     /* read intx_soft_enabled before read others */
+     smp_rmb();
+
      /* reading the ISR has the effect of also clearing it so it's very
       * important to save off the value. */
      isr = ioread8(vp_dev->isr);

diff --git a/drivers/virtio/virtio_pci_common.h b/drivers/virtio/virtio_pci_common.h
index a235ce9ff6a5..3c06e0f92ee4 100644
--- a/drivers/virtio/virtio_pci_common.h
+++ b/drivers/virtio/virtio_pci_common.h

@@ -64,6 +64,7 @@ struct virtio_pci_device {
      /* MSI-X support */
      int msix_enabled;
      int intx_enabled;
+     bool intx_soft_enabled;
      cpumask_var_t *msix_affinity_masks;
      /* Name strings for interrupts. This size should be enough,
       * and I'm too lazy to allocate each name separately. */
--

2.25.1

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization