Messages in this thread | | | Date | Thu, 7 Apr 2022 14:39:44 +0800 | Subject | Re: [PATCH V2 5/5] virtio: harden vring IRQ | From | Jason Wang <> |
| |
在 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@linutronix.de> >> Cc: Peter Zijlstra <peterz@infradead.org> >> Cc: "Paul E. McKenney" <paulmck@kernel.org> >> Cc: Marc Zyngier <maz@kernel.org> >> Signed-off-by: Jason Wang <jasowang@redhat.com> >> --- >> 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
| |