Re: [PATCH V2 5/5] virtio: harden vring IRQ
From: Jason Wang
Date: Thu Apr 07 2022 - 02:40:21 EST
在 2022/4/6 下午8:04, Michael S. Tsirkin 写道:
On Wed, Apr 06, 2022 at 04:35:38PM +0800, Jason Wang wrote:
This is a rework on the previous IRQ hardening that is done for
virtio-pci where several drawbacks were found and were reverted:
1) try to use IRQF_NO_AUTOEN which is not friendly to affinity managed IRQ
that is used by some device such as virtio-blk
2) done only for PCI transport
In this patch, we tries to borrow the idea from the INTX IRQ hardening
in the reverted commit 080cd7c3ac87 ("virtio-pci: harden INTX interrupts")
by introducing a global device_ready variable for each
virtio_device. Then we can to toggle it during
virtio_reset_device()/virtio_device_ready(). A
virtio_synchornize_vqs() is used in both virtio_device_ready() and
virtio_reset_device() to synchronize with the vring callbacks. With
this, vring_interrupt() can return check and early if driver_ready is
false.
Note that the hardening is only done for vring interrupt since the
config interrupt hardening is already done in commit 22b7050a024d7
("virtio: defer config changed notifications"). But the method that is
used by config interrupt can't be reused by the vring interrupt
handler because it uses spinlock to do the synchronization which is
expensive.
Cc: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
Cc: Peter Zijlstra <peterz@xxxxxxxxxxxxx>
Cc: "Paul E. McKenney" <paulmck@xxxxxxxxxx>
Cc: Marc Zyngier <maz@xxxxxxxxxx>
Signed-off-by: Jason Wang <jasowang@xxxxxxxxxx>
---
drivers/virtio/virtio.c | 11 +++++++++++
drivers/virtio/virtio_ring.c | 9 ++++++++-
include/linux/virtio.h | 2 ++
include/linux/virtio_config.h | 8 ++++++++
4 files changed, 29 insertions(+), 1 deletion(-)
diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
index 8dde44ea044a..2f3a6f8e3d9c 100644
--- a/drivers/virtio/virtio.c
+++ b/drivers/virtio/virtio.c
@@ -220,6 +220,17 @@ static int virtio_features_ok(struct virtio_device *dev)
* */
void virtio_reset_device(struct virtio_device *dev)
{
+ if (READ_ONCE(dev->driver_ready)) {
+ /*
+ * The below virtio_synchronize_vqs() guarantees that any
+ * interrupt for this line arriving after
+ * virtio_synchronize_vqs() has completed is guaranteed to see
+ * driver_ready == false.
+ */
+ WRITE_ONCE(dev->driver_ready, false);
+ virtio_synchronize_vqs(dev);
+ }
+
dev->config->reset(dev);
}
EXPORT_SYMBOL_GPL(virtio_reset_device);
diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index cfb028ca238e..a4592e55c9f8 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -2127,10 +2127,17 @@ static inline bool more_used(const struct vring_virtqueue *vq)
return vq->packed_ring ? more_used_packed(vq) : more_used_split(vq);
}
-irqreturn_t vring_interrupt(int irq, void *_vq)
+irqreturn_t vring_interrupt(int irq, void *v)
{
+ struct virtqueue *_vq = v;
+ struct virtio_device *vdev = _vq->vdev;
struct vring_virtqueue *vq = to_vvq(_vq);
+ if (!READ_ONCE(vdev->driver_ready)) {
I am not sure why we need READ_ONCE here, it's done under lock.
I may miss something but which lock did you mean here? (Note the irq
handler doesn't hold the irq descriptor lock).
Thanks
Accrdingly, same thing above for READ_ONCE and WRITE_ONCE.
+ dev_warn_once(&vdev->dev, "virtio vring IRQ raised before DRIVER_OK");
+ return IRQ_NONE;
+ }
+
if (!more_used(vq)) {
pr_debug("virtqueue interrupt with no work for %p\n", vq);
return IRQ_NONE;
diff --git a/include/linux/virtio.h b/include/linux/virtio.h
index 5464f398912a..dfa2638a293e 100644
--- a/include/linux/virtio.h
+++ b/include/linux/virtio.h
@@ -95,6 +95,7 @@ dma_addr_t virtqueue_get_used_addr(struct virtqueue *vq);
* @failed: saved value for VIRTIO_CONFIG_S_FAILED bit (for restore)
* @config_enabled: configuration change reporting enabled
* @config_change_pending: configuration change reported while disabled
+ * @driver_ready: whehter the driver is ready (e.g for vring callbacks)
* @config_lock: protects configuration change reporting
* @dev: underlying device.
* @id: the device type identification (used to match it with a driver).
@@ -109,6 +110,7 @@ struct virtio_device {
bool failed;
bool config_enabled;
bool config_change_pending;
+ bool driver_ready;
spinlock_t config_lock;
spinlock_t vqs_list_lock; /* Protects VQs list access */
struct device dev;
diff --git a/include/linux/virtio_config.h b/include/linux/virtio_config.h
index 08b73d9bbff2..c9e207bf2c9c 100644
--- a/include/linux/virtio_config.h
+++ b/include/linux/virtio_config.h
@@ -246,6 +246,14 @@ void virtio_device_ready(struct virtio_device *dev)
{
unsigned status = dev->config->get_status(dev);
+ virtio_synchronize_vqs(dev);
+ /*
+ * The above virtio_synchronize_vqs() make sure
makes sure
+ * vring_interrupt() will see the driver specific setup if it
+ * see driver_ready as true.
sees
+ */
+ WRITE_ONCE(dev->driver_ready, true);
+
BUG_ON(status & VIRTIO_CONFIG_S_DRIVER_OK);
dev->config->set_status(dev, status | VIRTIO_CONFIG_S_DRIVER_OK);
}
--
2.25.1