Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <726caf4c-3dde-8a47-e665-6b2b963304b6@redhat.com>
Date:   Thu, 7 Apr 2022 14:39:44 +0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:91.0)
 Gecko/20100101 Thunderbird/91.7.0
Subject: Re: [PATCH V2 5/5] virtio: harden vring IRQ
Content-Language: en-US
To:     "Michael S. Tsirkin" <mst@redhat.com>
Cc:     virtualization@lists.linux-foundation.org,
        linux-kernel@vger.kernel.org, maz@kernel.org, tglx@linutronix.de,
        peterz@infradead.org, sgarzare@redhat.com,
        "Paul E. McKenney" <paulmck@kernel.org>
References: <20220406083538.16274-1-jasowang@redhat.com>
 <20220406083538.16274-6-jasowang@redhat.com>
 <20220406080135-mutt-send-email-mst@kernel.org>
From:   Jason Wang <jasowang@redhat.com>
In-Reply-To: <20220406080135-mutt-send-email-mst@kernel.org>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk


在 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