On Thu, Dec 9, 2021 at 2:55 PM Michael S. Tsirkin [off-list ref] wrote:

On Thu, Dec 09, 2021 at 10:06:34AM +0800, Jason Wang wrote:

quoted

On Thu, Dec 9, 2021 at 4:27 AM Michael S. Tsirkin [off-list ref] wrote:

quoted

On Fri, Nov 26, 2021 at 12:41:02PM +0800, Jason Wang wrote:

quoted

This patch tries to make sure the virtio interrupt handler for MMIO
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 interrupt enabling status in a
new intr_soft_enabled variable and toggle it during in
vm_disable/enable_interrupts(). The MMIO interrupt handler will check
intr_soft_enabled before processing the actual interrupt.

Signed-off-by: Jason Wang <jasowang@redhat.com>
---
Changes since V1:
- Silent compling warnings
 drivers/virtio/virtio_mmio.c | 37 ++++++++++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)

diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c
index 56128b9c46eb..c517afdd2cc5 100644
--- a/drivers/virtio/virtio_mmio.c
+++ b/drivers/virtio/virtio_mmio.c

@@ -90,6 +90,7 @@ struct virtio_mmio_device {
      /* a list of queues so we can dispatch IRQs */
      spinlock_t lock;
      struct list_head virtqueues;
+     bool intr_soft_enabled;
 };

 struct virtio_mmio_vq_info {

@@ -100,7 +101,37 @@ struct virtio_mmio_vq_info {
      struct list_head node;
 };

+/* disable irq handlers */
+static void vm_disable_cbs(struct virtio_device *vdev)
+{
+     struct virtio_mmio_device *vm_dev = to_virtio_mmio_device(vdev);
+     int irq = platform_get_irq(vm_dev->pdev, 0);

+     /*
+      * 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(vm_dev->intr_soft_enabled, false);
+     synchronize_irq(irq);
+}
+
+/* enable irq handlers */
+static void vm_enable_cbs(struct virtio_device *vdev)
+{
+     struct virtio_mmio_device *vm_dev = to_virtio_mmio_device(vdev);
+     int irq = platform_get_irq(vm_dev->pdev, 0);
+
+     disable_irq(irq);
+     /*
+      * The above disable_irq() provides TSO ordering and
+      * as such promotes the below store to store-release.
+      */
+     WRITE_ONCE(vm_dev->intr_soft_enabled, true);
+     enable_irq(irq);
+     return;
+}

 /* Configuration interface */

@@ -262,6 +293,8 @@ static void vm_reset(struct virtio_device *vdev)

      /* 0 status means a reset. */
      writel(0, vm_dev->base + VIRTIO_MMIO_STATUS);

There was a discussion about reading status to make sure it is clear.
The spec says we should, this can't hurt as a further hardening measure.
In fact, let's do it in the core maybe? Spec says it applies to all
devices ...

We can do that, but I'm not sure if we break some existing device.

Hmm. Have anything specific in mind?

No, I can send a patch to do that.

quoted

quoted

quoted

+     /* Disable VQ/configuration callbacks. */
+     vm_disable_cbs(vdev);
 }

@@ -288,6 +321,9 @@ static irqreturn_t vm_interrupt(int irq, void *opaque)
      unsigned long flags;
      irqreturn_t ret = IRQ_NONE;

+     if (!READ_ONCE(vm_dev->intr_soft_enabled))
+             return IRQ_NONE;
+

So if the write is seen before reset happened (should not happen, but we
are talking a buggy device) then it won't be acknowledged and device
will keep pulling the interrupt. I think as long as we are hardening
this, let's go the full mile and try to avoid DoS if we can, do the
check before invoking the callback, but do not skip the read.
Whether to still return IRQ_NONE is a good question.

Did you mean something like this:

        /* Read and acknowledge interrupts */
        status = readl(vm_dev->base + VIRTIO_MMIO_INTERRUPT_STATUS);
        writel(status, vm_dev->base + VIRTIO_MMIO_INTERRUPT_ACK);

        if (status)
                ret = IRQ_HANDLED;

       if (!READ_ONCE(vm_dev->intr_soft_enabled))
               return ret;

Thanks

Maybe. Or is

        if (!READ_ONCE(vm_dev->intr_soft_enabled))
                return IRQ_NONE;

better here?

Yes, I just paste part of the code, the ret is initialized to IRQ_NONE.

Thanks

quoted

quoted

quoted

      /* Read and acknowledge interrupts */
      status = readl(vm_dev->base + VIRTIO_MMIO_INTERRUPT_STATUS);
      writel(status, vm_dev->base + VIRTIO_MMIO_INTERRUPT_ACK);

@@ -529,6 +565,7 @@ static bool vm_get_shm_region(struct virtio_device *vdev,
 }

 static const struct virtio_config_ops virtio_mmio_config_ops = {
+     .enable_cbs     = vm_enable_cbs,
      .get            = vm_get,
      .set            = vm_set,
      .generation     = vm_generation,
--

2.25.1

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