[PATCH V2 0/5] rework on the IRQ hardening of virtio

[PATCH V2 0/5] rework on the IRQ hardening of virtio

[Jason Wang]

Hi All:

This is a rework on the IRQ hardening for virtio which is done
previously by the following commits are reverted:

9e35276a5344 ("virtio_pci: harden MSI-X interrupts")
080cd7c3ac87 ("virtio-pci: harden INTX interrupts")

The reason is that it depends on the IRQF_NO_AUTOEN which may conflict
with the assumption of the affinity managed IRQ that is used by some
virtio drivers. And what's more, it is only done for virtio-pci but
not other transports.

In this rework, I try to implement a general virtio solution which
borrows the idea of the INTX hardening by introducing a boolean for
virtqueue callback enabling and toggle it in virtio_device_ready()
and virtio_reset_device(). Then vring_interrupt() can simply check and
return early if the driver is not ready.

Please review.

Changes since v1:

- Use transport specific irq synchronization method when possible
- Drop the module parameter and enable the hardening unconditonally
- Tweak the barrier/ordering facilities used in the code
- Reanme irq_soft_enabled to driver_ready
- Avoid unnecssary IRQ synchornization (e.g during boot)

Jason Wang (4):
  virtio: use virtio_reset_device() when possible
  virtio: introduce config op to synchronize vring callbacks
  virtio-pci: implement synchronize_vqs()
  virtio: harden vring IRQ

Stefano Garzarella (1):
  virtio: use virtio_device_ready() in virtio_device_restore()

 drivers/virtio/virtio.c            | 20 ++++++++++++++++----
 drivers/virtio/virtio_pci_common.c | 14 ++++++++++++++
 drivers/virtio/virtio_pci_common.h |  2 ++
 drivers/virtio/virtio_pci_legacy.c |  1 +
 drivers/virtio/virtio_pci_modern.c |  2 ++
 drivers/virtio/virtio_ring.c       |  9 ++++++++-
 include/linux/virtio.h             |  2 ++
 include/linux/virtio_config.h      | 24 ++++++++++++++++++++++++
 8 files changed, 69 insertions(+), 5 deletions(-)

-- 
2.25.1

[PATCH V2 1/5] virtio: use virtio_device_ready() in virtio_device_restore()

[Jason Wang]

From: Stefano Garzarella <sgarzare@redhat.com>

It will allows us to do extension on virtio_device_ready() without
duplicating codes.

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: Stefano Garzarella <sgarzare@redhat.com>
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 drivers/virtio/virtio.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
index 22f15f444f75..75c8d560bbd3 100644
--- a/drivers/virtio/virtio.c
+++ b/drivers/virtio/virtio.c
@@ -526,8 +526,9 @@ int virtio_device_restore(struct virtio_device *dev)
 			goto err;
 	}
 
-	/* Finally, tell the device we're all set */
-	virtio_add_status(dev, VIRTIO_CONFIG_S_DRIVER_OK);
+	/* If restore didn't do it, mark device DRIVER_OK ourselves. */
+	if (!(dev->config->get_status(dev) & VIRTIO_CONFIG_S_DRIVER_OK))
+		virtio_device_ready(dev);
 
 	virtio_config_enable(dev);
 
-- 
2.25.1

Re: [PATCH V2 1/5] virtio: use virtio_device_ready() in virtio_device_restore()

[Michael S. Tsirkin]

patch had wrong mime type. I managed to extra it but pls fix.

> 
> 
> From: Stefano Garzarella <sgarzare@redhat.com>
> 
> It will allows us

will allow us

> to do extension on virtio_device_ready() without
> duplicating codes.

code

> 
> 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: Stefano Garzarella <sgarzare@redhat.com>
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
>  drivers/virtio/virtio.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
> index 22f15f444f75..75c8d560bbd3 100644
> --- a/drivers/virtio/virtio.c
> +++ b/drivers/virtio/virtio.c
> @@ -526,8 +526,9 @@ int virtio_device_restore(struct virtio_device *dev)
>  			goto err;
>  	}
>  
> -	/* Finally, tell the device we're all set */
> -	virtio_add_status(dev, VIRTIO_CONFIG_S_DRIVER_OK);
> +	/* If restore didn't do it, mark device DRIVER_OK ourselves. */
> +	if (!(dev->config->get_status(dev) & VIRTIO_CONFIG_S_DRIVER_OK))
> +		virtio_device_ready(dev);
>  
>  	virtio_config_enable(dev);

it's unfortunate that this adds an extra vmexit since virtio_device_ready
calls get_status too.

We now have:

static inline
void virtio_device_ready(struct virtio_device *dev)
{
        unsigned status = dev->config->get_status(dev);
                
        BUG_ON(status & VIRTIO_CONFIG_S_DRIVER_OK);
        dev->config->set_status(dev, status | VIRTIO_CONFIG_S_DRIVER_OK);
}


I propose adding a helper and putting common code there.

>  
> -- 
> 2.25.1

Re: [PATCH V2 1/5] virtio: use virtio_device_ready() in virtio_device_restore()

[Jason Wang]

在 2022/4/6 下午7:44, Michael S. Tsirkin 写道:
> patch had wrong mime type. I managed to extra it but pls fix.
>
>>
>> From: Stefano Garzarella <sgarzare@redhat.com>
>>
>> It will allows us
> will allow us
>
>> to do extension on virtio_device_ready() without
>> duplicating codes.
> code
>
>> 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: Stefano Garzarella <sgarzare@redhat.com>
>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>> ---
>>   drivers/virtio/virtio.c | 5 +++--
>>   1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
>> index 22f15f444f75..75c8d560bbd3 100644
>> --- a/drivers/virtio/virtio.c
>> +++ b/drivers/virtio/virtio.c
>> @@ -526,8 +526,9 @@ int virtio_device_restore(struct virtio_device *dev)
>>   			goto err;
>>   	}
>>   
>> -	/* Finally, tell the device we're all set */
>> -	virtio_add_status(dev, VIRTIO_CONFIG_S_DRIVER_OK);
>> +	/* If restore didn't do it, mark device DRIVER_OK ourselves. */
>> +	if (!(dev->config->get_status(dev) & VIRTIO_CONFIG_S_DRIVER_OK))
>> +		virtio_device_ready(dev);
>>   
>>   	virtio_config_enable(dev);
> it's unfortunate that this adds an extra vmexit since virtio_device_ready
> calls get_status too.
>
> We now have:
>
> static inline
> void virtio_device_ready(struct virtio_device *dev)
> {
>          unsigned status = dev->config->get_status(dev);
>                  
>          BUG_ON(status & VIRTIO_CONFIG_S_DRIVER_OK);
>          dev->config->set_status(dev, status | VIRTIO_CONFIG_S_DRIVER_OK);
> }
>
>
> I propose adding a helper and putting common code there.


Ok, let me fix it.

Thanks


>
>>   
>> -- 
>> 2.25.1

[PATCH V2 2/5] virtio: use virtio_reset_device() when possible

[Jason Wang]

This allows us to do common extension without duplicating codes.

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 | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
index 75c8d560bbd3..8dde44ea044a 100644
--- a/drivers/virtio/virtio.c
+++ b/drivers/virtio/virtio.c
@@ -430,7 +430,7 @@ int register_virtio_device(struct virtio_device *dev)
 
 	/* We always start by resetting the device, in case a previous
 	 * driver messed it up.  This also tests that code path a little. */
-	dev->config->reset(dev);
+	virtio_reset_device(dev);
 
 	/* Acknowledge that we've seen the device. */
 	virtio_add_status(dev, VIRTIO_CONFIG_S_ACKNOWLEDGE);
@@ -496,7 +496,7 @@ int virtio_device_restore(struct virtio_device *dev)
 
 	/* We always start by resetting the device, in case a previous
 	 * driver messed it up. */
-	dev->config->reset(dev);
+	virtio_reset_device(dev);
 
 	/* Acknowledge that we've seen the device. */
 	virtio_add_status(dev, VIRTIO_CONFIG_S_ACKNOWLEDGE);
-- 
2.25.1

Re: [PATCH V2 2/5] virtio: use virtio_reset_device() when possible

[Michael S. Tsirkin]

On Wed, Apr 06, 2022 at 04:35:35PM +0800, Jason Wang wrote:
> This allows us to do common extension without duplicating codes.

codes -> code

> 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 | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
> index 75c8d560bbd3..8dde44ea044a 100644
> --- a/drivers/virtio/virtio.c
> +++ b/drivers/virtio/virtio.c
> @@ -430,7 +430,7 @@ int register_virtio_device(struct virtio_device *dev)
>  
>  	/* We always start by resetting the device, in case a previous
>  	 * driver messed it up.  This also tests that code path a little. */
> -	dev->config->reset(dev);
> +	virtio_reset_device(dev);
>  
>  	/* Acknowledge that we've seen the device. */
>  	virtio_add_status(dev, VIRTIO_CONFIG_S_ACKNOWLEDGE);
> @@ -496,7 +496,7 @@ int virtio_device_restore(struct virtio_device *dev)
>  
>  	/* We always start by resetting the device, in case a previous
>  	 * driver messed it up. */
> -	dev->config->reset(dev);
> +	virtio_reset_device(dev);
>  
>  	/* Acknowledge that we've seen the device. */
>  	virtio_add_status(dev, VIRTIO_CONFIG_S_ACKNOWLEDGE);
> -- 
> 2.25.1

[PATCH V2 3/5] virtio: introduce config op to synchronize vring callbacks

[Jason Wang]

This patch introduce a new virtio config ops to vring
callbacks. Transport specific method is required to call
synchornize_irq() on the IRQs. For the transport that doesn't provide
synchronize_vqs(), use synchornize_rcu() as a fallback.

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>
---
 include/linux/virtio_config.h | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/include/linux/virtio_config.h b/include/linux/virtio_config.h
index b341dd62aa4d..08b73d9bbff2 100644
--- a/include/linux/virtio_config.h
+++ b/include/linux/virtio_config.h
@@ -57,6 +57,8 @@ struct virtio_shm_region {
  *		include a NULL entry for vqs unused by driver
  *	Returns 0 on success or error status
  * @del_vqs: free virtqueues found by find_vqs().
+ * @synchronize_vqs: synchronize with the virtqueue callbacks.
+ *	vdev: the virtio_device
  * @get_features: get the array of feature bits for this device.
  *	vdev: the virtio_device
  *	Returns the first 64 feature bits (all we currently need).
@@ -89,6 +91,7 @@ struct virtio_config_ops {
 			const char * const names[], const bool *ctx,
 			struct irq_affinity *desc);
 	void (*del_vqs)(struct virtio_device *);
+	void (*synchronize_vqs)(struct virtio_device *);
 	u64 (*get_features)(struct virtio_device *vdev);
 	int (*finalize_features)(struct virtio_device *vdev);
 	const char *(*bus_name)(struct virtio_device *vdev);
@@ -217,6 +220,19 @@ int virtio_find_vqs_ctx(struct virtio_device *vdev, unsigned nvqs,
 				      desc);
 }
 
+/**
+ * virtio_synchronize_vqs - synchronize with virtqueue callbacks
+ * @vdev: the device
+ */
+static inline
+void virtio_synchronize_vqs(struct virtio_device *dev)
+{
+	if (dev->config->synchronize_vqs)
+		dev->config->synchronize_vqs(dev);
+	else
+		synchronize_rcu();
+}
+
 /**
  * virtio_device_ready - enable vq use in probe function
  * @vdev: the device
-- 
2.25.1

Re: [PATCH V2 3/5] virtio: introduce config op to synchronize vring callbacks

[Michael S. Tsirkin]

On Wed, Apr 06, 2022 at 04:35:36PM +0800, Jason Wang wrote:
> This patch introduce

introduces

> a new

new

> virtio config ops to vring
> callbacks. Transport specific method is required to call
> synchornize_irq() on the IRQs. For the transport that doesn't provide
> synchronize_vqs(), use synchornize_rcu() as a fallback.
> 
> 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>
> ---
>  include/linux/virtio_config.h | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/include/linux/virtio_config.h b/include/linux/virtio_config.h
> index b341dd62aa4d..08b73d9bbff2 100644
> --- a/include/linux/virtio_config.h
> +++ b/include/linux/virtio_config.h
> @@ -57,6 +57,8 @@ struct virtio_shm_region {
>   *		include a NULL entry for vqs unused by driver
>   *	Returns 0 on success or error status
>   * @del_vqs: free virtqueues found by find_vqs().
> + * @synchronize_vqs: synchronize with the virtqueue callbacks.
> + *	vdev: the virtio_device

I think I prefer synchronize_callbacks

>   * @get_features: get the array of feature bits for this device.
>   *	vdev: the virtio_device
>   *	Returns the first 64 feature bits (all we currently need).
> @@ -89,6 +91,7 @@ struct virtio_config_ops {
>  			const char * const names[], const bool *ctx,
>  			struct irq_affinity *desc);
>  	void (*del_vqs)(struct virtio_device *);
> +	void (*synchronize_vqs)(struct virtio_device *);
>  	u64 (*get_features)(struct virtio_device *vdev);
>  	int (*finalize_features)(struct virtio_device *vdev);
>  	const char *(*bus_name)(struct virtio_device *vdev);
> @@ -217,6 +220,19 @@ int virtio_find_vqs_ctx(struct virtio_device *vdev, unsigned nvqs,
>  				      desc);
>  }
>  
> +/**
> + * virtio_synchronize_vqs - synchronize with virtqueue callbacks
> + * @vdev: the device
> + */
> +static inline
> +void virtio_synchronize_vqs(struct virtio_device *dev)
> +{
> +	if (dev->config->synchronize_vqs)
> +		dev->config->synchronize_vqs(dev);
> +	else
> +		synchronize_rcu();

I am not sure about this fallback and the latency impact.
Maybe synchronize_rcu_expedited is better here.

> +}
> +
>  /**
>   * virtio_device_ready - enable vq use in probe function
>   * @vdev: the device
> -- 
> 2.25.1

Re: [PATCH V2 3/5] virtio: introduce config op to synchronize vring callbacks

[Jason Wang]

在 2022/4/6 下午7:59, Michael S. Tsirkin 写道:
> On Wed, Apr 06, 2022 at 04:35:36PM +0800, Jason Wang wrote:
>> This patch introduce
> introduces
>
>> a new
> new
>
>> virtio config ops to vring
>> callbacks. Transport specific method is required to call
>> synchornize_irq() on the IRQs. For the transport that doesn't provide
>> synchronize_vqs(), use synchornize_rcu() as a fallback.
>>
>> 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>
>> ---
>>   include/linux/virtio_config.h | 16 ++++++++++++++++
>>   1 file changed, 16 insertions(+)
>>
>> diff --git a/include/linux/virtio_config.h b/include/linux/virtio_config.h
>> index b341dd62aa4d..08b73d9bbff2 100644
>> --- a/include/linux/virtio_config.h
>> +++ b/include/linux/virtio_config.h
>> @@ -57,6 +57,8 @@ struct virtio_shm_region {
>>    *		include a NULL entry for vqs unused by driver
>>    *	Returns 0 on success or error status
>>    * @del_vqs: free virtqueues found by find_vqs().
>> + * @synchronize_vqs: synchronize with the virtqueue callbacks.
>> + *	vdev: the virtio_device
> I think I prefer synchronize_callbacks


Ok, I will rename it.


>
>>    * @get_features: get the array of feature bits for this device.
>>    *	vdev: the virtio_device
>>    *	Returns the first 64 feature bits (all we currently need).
>> @@ -89,6 +91,7 @@ struct virtio_config_ops {
>>   			const char * const names[], const bool *ctx,
>>   			struct irq_affinity *desc);
>>   	void (*del_vqs)(struct virtio_device *);
>> +	void (*synchronize_vqs)(struct virtio_device *);
>>   	u64 (*get_features)(struct virtio_device *vdev);
>>   	int (*finalize_features)(struct virtio_device *vdev);
>>   	const char *(*bus_name)(struct virtio_device *vdev);
>> @@ -217,6 +220,19 @@ int virtio_find_vqs_ctx(struct virtio_device *vdev, unsigned nvqs,
>>   				      desc);
>>   }
>>   
>> +/**
>> + * virtio_synchronize_vqs - synchronize with virtqueue callbacks
>> + * @vdev: the device
>> + */
>> +static inline
>> +void virtio_synchronize_vqs(struct virtio_device *dev)
>> +{
>> +	if (dev->config->synchronize_vqs)
>> +		dev->config->synchronize_vqs(dev);
>> +	else
>> +		synchronize_rcu();
> I am not sure about this fallback and the latency impact.


Unless each transport can implement their own synchronization routine, 
we need something that can do best effort as fallback here.


> Maybe synchronize_rcu_expedited is better here.


Not sure, it might lead IPIs and according to the 
Documentation/RCU/checklist.rst:


"""

         The expedited forms of these primitives have the same semantics
         as the non-expedited forms, but expediting is both expensive and
         (with the exception of synchronize_srcu_expedited()) unfriendly
         to real-time workloads.  Use of the expedited primitives should
         be restricted to rare configuration-change operations that would
         not normally be undertaken while a real-time workload is running.
         However, real-time workloads can use rcupdate.rcu_normal kernel
         boot parameter to completely disable expedited grace periods,
         though this might have performance implications.

"""

It will be expensive for real time workloads.

Thanks


>
>> +}
>> +
>>   /**
>>    * virtio_device_ready - enable vq use in probe function
>>    * @vdev: the device
>> -- 
>> 2.25.1

[PATCH V2 4/5] virtio-pci: implement synchronize_vqs()

[Jason Wang]

This patch implements PCI version of synchronize_vqs().

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_pci_common.c | 14 ++++++++++++++
 drivers/virtio/virtio_pci_common.h |  2 ++
 drivers/virtio/virtio_pci_legacy.c |  1 +
 drivers/virtio/virtio_pci_modern.c |  2 ++
 4 files changed, 19 insertions(+)

diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c
index d724f676608b..b78c8bc93a97 100644
--- a/drivers/virtio/virtio_pci_common.c
+++ b/drivers/virtio/virtio_pci_common.c
@@ -37,6 +37,20 @@ void vp_synchronize_vectors(struct virtio_device *vdev)
 		synchronize_irq(pci_irq_vector(vp_dev->pci_dev, i));
 }
 
+void vp_synchronize_vqs(struct virtio_device *vdev)
+{
+	struct virtio_pci_device *vp_dev = to_vp_device(vdev);
+	int i;
+
+	if (vp_dev->intx_enabled) {
+		synchronize_irq(vp_dev->pci_dev->irq);
+		return;
+	}
+
+	for (i = 0; i < vp_dev->msix_vectors; ++i)
+		synchronize_irq(pci_irq_vector(vp_dev->pci_dev, i));
+}
+
 /* the notify function used when creating a virt queue */
 bool vp_notify(struct virtqueue *vq)
 {
diff --git a/drivers/virtio/virtio_pci_common.h b/drivers/virtio/virtio_pci_common.h
index eb17a29fc7ef..2b84d5c1b5bc 100644
--- a/drivers/virtio/virtio_pci_common.h
+++ b/drivers/virtio/virtio_pci_common.h
@@ -105,6 +105,8 @@ static struct virtio_pci_device *to_vp_device(struct virtio_device *vdev)
 void vp_synchronize_vectors(struct virtio_device *vdev);
 /* the notify function used when creating a virt queue */
 bool vp_notify(struct virtqueue *vq);
+/* synchronize with callbacks */
+void vp_synchronize_vqs(struct virtio_device *vdev);
 /* the config->del_vqs() implementation */
 void vp_del_vqs(struct virtio_device *vdev);
 /* the config->find_vqs() implementation */
diff --git a/drivers/virtio/virtio_pci_legacy.c b/drivers/virtio/virtio_pci_legacy.c
index 6f4e34ce96b8..5a9e62320edc 100644
--- a/drivers/virtio/virtio_pci_legacy.c
+++ b/drivers/virtio/virtio_pci_legacy.c
@@ -192,6 +192,7 @@ static const struct virtio_config_ops virtio_pci_config_ops = {
 	.reset		= vp_reset,
 	.find_vqs	= vp_find_vqs,
 	.del_vqs	= vp_del_vqs,
+	.synchronize_vqs = vp_synchronize_vqs,
 	.get_features	= vp_get_features,
 	.finalize_features = vp_finalize_features,
 	.bus_name	= vp_bus_name,
diff --git a/drivers/virtio/virtio_pci_modern.c b/drivers/virtio/virtio_pci_modern.c
index a2671a20ef77..584850389855 100644
--- a/drivers/virtio/virtio_pci_modern.c
+++ b/drivers/virtio/virtio_pci_modern.c
@@ -394,6 +394,7 @@ static const struct virtio_config_ops virtio_pci_config_nodev_ops = {
 	.reset		= vp_reset,
 	.find_vqs	= vp_modern_find_vqs,
 	.del_vqs	= vp_del_vqs,
+	.synchronize_vqs = vp_synchronize_vqs,
 	.get_features	= vp_get_features,
 	.finalize_features = vp_finalize_features,
 	.bus_name	= vp_bus_name,
@@ -411,6 +412,7 @@ static const struct virtio_config_ops virtio_pci_config_ops = {
 	.reset		= vp_reset,
 	.find_vqs	= vp_modern_find_vqs,
 	.del_vqs	= vp_del_vqs,
+	.synchronize_vqs = vp_synchronize_vqs,
 	.get_features	= vp_get_features,
 	.finalize_features = vp_finalize_features,
 	.bus_name	= vp_bus_name,
-- 
2.25.1

Re: [PATCH V2 4/5] virtio-pci: implement synchronize_vqs()

[Michael S. Tsirkin]

On Wed, Apr 06, 2022 at 04:35:37PM +0800, Jason Wang wrote:
> This patch implements PCI version of synchronize_vqs().
> 
> 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>

Please add implementations at least for ccw and mmio.

> ---
>  drivers/virtio/virtio_pci_common.c | 14 ++++++++++++++
>  drivers/virtio/virtio_pci_common.h |  2 ++
>  drivers/virtio/virtio_pci_legacy.c |  1 +
>  drivers/virtio/virtio_pci_modern.c |  2 ++
>  4 files changed, 19 insertions(+)
> 
> diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c
> index d724f676608b..b78c8bc93a97 100644
> --- a/drivers/virtio/virtio_pci_common.c
> +++ b/drivers/virtio/virtio_pci_common.c
> @@ -37,6 +37,20 @@ void vp_synchronize_vectors(struct virtio_device *vdev)
>  		synchronize_irq(pci_irq_vector(vp_dev->pci_dev, i));
>  }
>  
> +void vp_synchronize_vqs(struct virtio_device *vdev)
> +{
> +	struct virtio_pci_device *vp_dev = to_vp_device(vdev);
> +	int i;
> +
> +	if (vp_dev->intx_enabled) {
> +		synchronize_irq(vp_dev->pci_dev->irq);
> +		return;
> +	}
> +
> +	for (i = 0; i < vp_dev->msix_vectors; ++i)
> +		synchronize_irq(pci_irq_vector(vp_dev->pci_dev, i));
> +}
> +
>  /* the notify function used when creating a virt queue */
>  bool vp_notify(struct virtqueue *vq)
>  {
> diff --git a/drivers/virtio/virtio_pci_common.h b/drivers/virtio/virtio_pci_common.h
> index eb17a29fc7ef..2b84d5c1b5bc 100644
> --- a/drivers/virtio/virtio_pci_common.h
> +++ b/drivers/virtio/virtio_pci_common.h
> @@ -105,6 +105,8 @@ static struct virtio_pci_device *to_vp_device(struct virtio_device *vdev)
>  void vp_synchronize_vectors(struct virtio_device *vdev);
>  /* the notify function used when creating a virt queue */
>  bool vp_notify(struct virtqueue *vq);
> +/* synchronize with callbacks */
> +void vp_synchronize_vqs(struct virtio_device *vdev);
>  /* the config->del_vqs() implementation */
>  void vp_del_vqs(struct virtio_device *vdev);
>  /* the config->find_vqs() implementation */
> diff --git a/drivers/virtio/virtio_pci_legacy.c b/drivers/virtio/virtio_pci_legacy.c
> index 6f4e34ce96b8..5a9e62320edc 100644
> --- a/drivers/virtio/virtio_pci_legacy.c
> +++ b/drivers/virtio/virtio_pci_legacy.c
> @@ -192,6 +192,7 @@ static const struct virtio_config_ops virtio_pci_config_ops = {
>  	.reset		= vp_reset,
>  	.find_vqs	= vp_find_vqs,
>  	.del_vqs	= vp_del_vqs,
> +	.synchronize_vqs = vp_synchronize_vqs,
>  	.get_features	= vp_get_features,
>  	.finalize_features = vp_finalize_features,
>  	.bus_name	= vp_bus_name,
> diff --git a/drivers/virtio/virtio_pci_modern.c b/drivers/virtio/virtio_pci_modern.c
> index a2671a20ef77..584850389855 100644
> --- a/drivers/virtio/virtio_pci_modern.c
> +++ b/drivers/virtio/virtio_pci_modern.c
> @@ -394,6 +394,7 @@ static const struct virtio_config_ops virtio_pci_config_nodev_ops = {
>  	.reset		= vp_reset,
>  	.find_vqs	= vp_modern_find_vqs,
>  	.del_vqs	= vp_del_vqs,
> +	.synchronize_vqs = vp_synchronize_vqs,
>  	.get_features	= vp_get_features,
>  	.finalize_features = vp_finalize_features,
>  	.bus_name	= vp_bus_name,
> @@ -411,6 +412,7 @@ static const struct virtio_config_ops virtio_pci_config_ops = {
>  	.reset		= vp_reset,
>  	.find_vqs	= vp_modern_find_vqs,
>  	.del_vqs	= vp_del_vqs,
> +	.synchronize_vqs = vp_synchronize_vqs,
>  	.get_features	= vp_get_features,
>  	.finalize_features = vp_finalize_features,
>  	.bus_name	= vp_bus_name,
> -- 
> 2.25.1

Re: [PATCH V2 4/5] virtio-pci: implement synchronize_vqs()

[Cornelia Huck]

On Wed, Apr 06 2022, "Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Wed, Apr 06, 2022 at 04:35:37PM +0800, Jason Wang wrote:
>> This patch implements PCI version of synchronize_vqs().
>> 
>> 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>
>
> Please add implementations at least for ccw and mmio.

I'm not sure what (if anything) can/should be done for ccw...

>
>> ---
>>  drivers/virtio/virtio_pci_common.c | 14 ++++++++++++++
>>  drivers/virtio/virtio_pci_common.h |  2 ++
>>  drivers/virtio/virtio_pci_legacy.c |  1 +
>>  drivers/virtio/virtio_pci_modern.c |  2 ++
>>  4 files changed, 19 insertions(+)
>> 
>> diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c
>> index d724f676608b..b78c8bc93a97 100644
>> --- a/drivers/virtio/virtio_pci_common.c
>> +++ b/drivers/virtio/virtio_pci_common.c
>> @@ -37,6 +37,20 @@ void vp_synchronize_vectors(struct virtio_device *vdev)
>>  		synchronize_irq(pci_irq_vector(vp_dev->pci_dev, i));
>>  }
>>  
>> +void vp_synchronize_vqs(struct virtio_device *vdev)
>> +{
>> +	struct virtio_pci_device *vp_dev = to_vp_device(vdev);
>> +	int i;
>> +
>> +	if (vp_dev->intx_enabled) {
>> +		synchronize_irq(vp_dev->pci_dev->irq);
>> +		return;
>> +	}
>> +
>> +	for (i = 0; i < vp_dev->msix_vectors; ++i)
>> +		synchronize_irq(pci_irq_vector(vp_dev->pci_dev, i));
>> +}
>> +

...given that this seems to synchronize threaded interrupt handlers?
Halil, do you think ccw needs to do anything? (AFAICS, we only have one
'irq' for channel devices anyway, and the handler just calls the
relevant callbacks directly.)

>>  /* the notify function used when creating a virt queue */
>>  bool vp_notify(struct virtqueue *vq)
>>  {

Re: [PATCH V2 4/5] virtio-pci: implement synchronize_vqs()

[Michael S. Tsirkin]

On Wed, Apr 06, 2022 at 03:04:32PM +0200, Cornelia Huck wrote:
> On Wed, Apr 06 2022, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Wed, Apr 06, 2022 at 04:35:37PM +0800, Jason Wang wrote:
> >> This patch implements PCI version of synchronize_vqs().
> >> 
> >> 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>
> >
> > Please add implementations at least for ccw and mmio.
> 
> I'm not sure what (if anything) can/should be done for ccw...
> 
> >
> >> ---
> >>  drivers/virtio/virtio_pci_common.c | 14 ++++++++++++++
> >>  drivers/virtio/virtio_pci_common.h |  2 ++
> >>  drivers/virtio/virtio_pci_legacy.c |  1 +
> >>  drivers/virtio/virtio_pci_modern.c |  2 ++
> >>  4 files changed, 19 insertions(+)
> >> 
> >> diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c
> >> index d724f676608b..b78c8bc93a97 100644
> >> --- a/drivers/virtio/virtio_pci_common.c
> >> +++ b/drivers/virtio/virtio_pci_common.c
> >> @@ -37,6 +37,20 @@ void vp_synchronize_vectors(struct virtio_device *vdev)
> >>  		synchronize_irq(pci_irq_vector(vp_dev->pci_dev, i));
> >>  }
> >>  
> >> +void vp_synchronize_vqs(struct virtio_device *vdev)
> >> +{
> >> +	struct virtio_pci_device *vp_dev = to_vp_device(vdev);
> >> +	int i;
> >> +
> >> +	if (vp_dev->intx_enabled) {
> >> +		synchronize_irq(vp_dev->pci_dev->irq);
> >> +		return;
> >> +	}
> >> +
> >> +	for (i = 0; i < vp_dev->msix_vectors; ++i)
> >> +		synchronize_irq(pci_irq_vector(vp_dev->pci_dev, i));
> >> +}
> >> +
> 
> ...given that this seems to synchronize threaded interrupt handlers?

No, any handlers at all. The point is to make sure any memory changes
made prior to this op are visible to callbacks.

Jason, maybe add that to the documentation?

> Halil, do you think ccw needs to do anything? (AFAICS, we only have one
> 'irq' for channel devices anyway, and the handler just calls the
> relevant callbacks directly.)

Then you need to synchronize with that.

> >>  /* the notify function used when creating a virt queue */
> >>  bool vp_notify(struct virtqueue *vq)
> >>  {

Re: [PATCH V2 4/5] virtio-pci: implement synchronize_vqs()

[Jason Wang]

在 2022/4/6 下午11:31, Michael S. Tsirkin 写道:
> On Wed, Apr 06, 2022 at 03:04:32PM +0200, Cornelia Huck wrote:
>> On Wed, Apr 06 2022, "Michael S. Tsirkin" <mst@redhat.com> wrote:
>>
>>> On Wed, Apr 06, 2022 at 04:35:37PM +0800, Jason Wang wrote:
>>>> This patch implements PCI version of synchronize_vqs().
>>>>
>>>> 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>
>>> Please add implementations at least for ccw and mmio.
>> I'm not sure what (if anything) can/should be done for ccw...
>>
>>>> ---
>>>>   drivers/virtio/virtio_pci_common.c | 14 ++++++++++++++
>>>>   drivers/virtio/virtio_pci_common.h |  2 ++
>>>>   drivers/virtio/virtio_pci_legacy.c |  1 +
>>>>   drivers/virtio/virtio_pci_modern.c |  2 ++
>>>>   4 files changed, 19 insertions(+)
>>>>
>>>> diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c
>>>> index d724f676608b..b78c8bc93a97 100644
>>>> --- a/drivers/virtio/virtio_pci_common.c
>>>> +++ b/drivers/virtio/virtio_pci_common.c
>>>> @@ -37,6 +37,20 @@ void vp_synchronize_vectors(struct virtio_device *vdev)
>>>>   		synchronize_irq(pci_irq_vector(vp_dev->pci_dev, i));
>>>>   }
>>>>   
>>>> +void vp_synchronize_vqs(struct virtio_device *vdev)
>>>> +{
>>>> +	struct virtio_pci_device *vp_dev = to_vp_device(vdev);
>>>> +	int i;
>>>> +
>>>> +	if (vp_dev->intx_enabled) {
>>>> +		synchronize_irq(vp_dev->pci_dev->irq);
>>>> +		return;
>>>> +	}
>>>> +
>>>> +	for (i = 0; i < vp_dev->msix_vectors; ++i)
>>>> +		synchronize_irq(pci_irq_vector(vp_dev->pci_dev, i));
>>>> +}
>>>> +
>> ...given that this seems to synchronize threaded interrupt handlers?
> No, any handlers at all. The point is to make sure any memory changes
> made prior to this op are visible to callbacks.
>
> Jason, maybe add that to the documentation?


Sure.


>
>> Halil, do you think ccw needs to do anything? (AFAICS, we only have one
>> 'irq' for channel devices anyway, and the handler just calls the
>> relevant callbacks directly.)
> Then you need to synchronize with that.


Have a quick glance at the codes, it looks to me we can synchronize with 
the IO_INTERRUPT. (Assuming all callbacks are triggered via 
ccw_device_irq()).

Thanks


>
>>>>   /* the notify function used when creating a virt queue */
>>>>   bool vp_notify(struct virtqueue *vq)
>>>>   {

Re: [PATCH V2 4/5] virtio-pci: implement synchronize_vqs()

[Cornelia Huck]

On Thu, Apr 07 2022, Jason Wang <jasowang@redhat.com> wrote:

> 在 2022/4/6 下午11:31, Michael S. Tsirkin 写道:
>> On Wed, Apr 06, 2022 at 03:04:32PM +0200, Cornelia Huck wrote:
>>> On Wed, Apr 06 2022, "Michael S. Tsirkin" <mst@redhat.com> wrote:
>>>
>>>> On Wed, Apr 06, 2022 at 04:35:37PM +0800, Jason Wang wrote:
>>>>> This patch implements PCI version of synchronize_vqs().
>>>>>
>>>>> 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>
>>>> Please add implementations at least for ccw and mmio.
>>> I'm not sure what (if anything) can/should be done for ccw...
>>>
>>>>> ---
>>>>>   drivers/virtio/virtio_pci_common.c | 14 ++++++++++++++
>>>>>   drivers/virtio/virtio_pci_common.h |  2 ++
>>>>>   drivers/virtio/virtio_pci_legacy.c |  1 +
>>>>>   drivers/virtio/virtio_pci_modern.c |  2 ++
>>>>>   4 files changed, 19 insertions(+)
>>>>>
>>>>> diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c
>>>>> index d724f676608b..b78c8bc93a97 100644
>>>>> --- a/drivers/virtio/virtio_pci_common.c
>>>>> +++ b/drivers/virtio/virtio_pci_common.c
>>>>> @@ -37,6 +37,20 @@ void vp_synchronize_vectors(struct virtio_device *vdev)
>>>>>   		synchronize_irq(pci_irq_vector(vp_dev->pci_dev, i));
>>>>>   }
>>>>>   
>>>>> +void vp_synchronize_vqs(struct virtio_device *vdev)
>>>>> +{
>>>>> +	struct virtio_pci_device *vp_dev = to_vp_device(vdev);
>>>>> +	int i;
>>>>> +
>>>>> +	if (vp_dev->intx_enabled) {
>>>>> +		synchronize_irq(vp_dev->pci_dev->irq);
>>>>> +		return;
>>>>> +	}
>>>>> +
>>>>> +	for (i = 0; i < vp_dev->msix_vectors; ++i)
>>>>> +		synchronize_irq(pci_irq_vector(vp_dev->pci_dev, i));
>>>>> +}
>>>>> +
>>> ...given that this seems to synchronize threaded interrupt handlers?
>> No, any handlers at all. The point is to make sure any memory changes
>> made prior to this op are visible to callbacks.
>>
>> Jason, maybe add that to the documentation?
>
>
> Sure.
>
>
>>
>>> Halil, do you think ccw needs to do anything? (AFAICS, we only have one
>>> 'irq' for channel devices anyway, and the handler just calls the
>>> relevant callbacks directly.)
>> Then you need to synchronize with that.
>
>
> Have a quick glance at the codes, it looks to me we can synchronize with 
> the IO_INTERRUPT. (Assuming all callbacks are triggered via 
> ccw_device_irq()).

Not quite, adapter interrupts are device-independent, but they are
relevant for vring interrupts.

That would mean that we would need to synchronize _all_ channel I/O
interrupts, which looks like a huge hammer. But do we really need that
at all?

The last patch in this series seems to be concerned with the "no vring
interrupts if the device is not ready" case, so it needs to synchronize
vring interrupts with device reset and setting the device status to
ready. For virtio-ccw, both reset and setting the status are channel I/O
operations, i.e. starting a program and waiting for the final device
interrupt for it, so synchronization (on a device level) is already
happening in a way. So I'm not sure if any extra synchronization is
actually needed in this case, but maybe I'm misunderstanding.

Do you have further use cases in mind?

Re: [PATCH V2 4/5] virtio-pci: implement synchronize_vqs()

[Jason Wang]

On Thu, Apr 7, 2022 at 3:53 PM Cornelia Huck <cohuck@redhat.com> wrote:
>
> On Thu, Apr 07 2022, Jason Wang <jasowang@redhat.com> wrote:
>
> > 在 2022/4/6 下午11:31, Michael S. Tsirkin 写道:
> >> On Wed, Apr 06, 2022 at 03:04:32PM +0200, Cornelia Huck wrote:
> >>> On Wed, Apr 06 2022, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> >>>
> >>>> On Wed, Apr 06, 2022 at 04:35:37PM +0800, Jason Wang wrote:
> >>>>> This patch implements PCI version of synchronize_vqs().
> >>>>>
> >>>>> 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>
> >>>> Please add implementations at least for ccw and mmio.
> >>> I'm not sure what (if anything) can/should be done for ccw...
> >>>
> >>>>> ---
> >>>>>   drivers/virtio/virtio_pci_common.c | 14 ++++++++++++++
> >>>>>   drivers/virtio/virtio_pci_common.h |  2 ++
> >>>>>   drivers/virtio/virtio_pci_legacy.c |  1 +
> >>>>>   drivers/virtio/virtio_pci_modern.c |  2 ++
> >>>>>   4 files changed, 19 insertions(+)
> >>>>>
> >>>>> diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c
> >>>>> index d724f676608b..b78c8bc93a97 100644
> >>>>> --- a/drivers/virtio/virtio_pci_common.c
> >>>>> +++ b/drivers/virtio/virtio_pci_common.c
> >>>>> @@ -37,6 +37,20 @@ void vp_synchronize_vectors(struct virtio_device *vdev)
> >>>>>                   synchronize_irq(pci_irq_vector(vp_dev->pci_dev, i));
> >>>>>   }
> >>>>>
> >>>>> +void vp_synchronize_vqs(struct virtio_device *vdev)
> >>>>> +{
> >>>>> + struct virtio_pci_device *vp_dev = to_vp_device(vdev);
> >>>>> + int i;
> >>>>> +
> >>>>> + if (vp_dev->intx_enabled) {
> >>>>> +         synchronize_irq(vp_dev->pci_dev->irq);
> >>>>> +         return;
> >>>>> + }
> >>>>> +
> >>>>> + for (i = 0; i < vp_dev->msix_vectors; ++i)
> >>>>> +         synchronize_irq(pci_irq_vector(vp_dev->pci_dev, i));
> >>>>> +}
> >>>>> +
> >>> ...given that this seems to synchronize threaded interrupt handlers?
> >> No, any handlers at all. The point is to make sure any memory changes
> >> made prior to this op are visible to callbacks.
> >>
> >> Jason, maybe add that to the documentation?
> >
> >
> > Sure.
> >
> >
> >>
> >>> Halil, do you think ccw needs to do anything? (AFAICS, we only have one
> >>> 'irq' for channel devices anyway, and the handler just calls the
> >>> relevant callbacks directly.)
> >> Then you need to synchronize with that.
> >
> >
> > Have a quick glance at the codes, it looks to me we can synchronize with
> > the IO_INTERRUPT. (Assuming all callbacks are triggered via
> > ccw_device_irq()).
>
> Not quite, adapter interrupts are device-independent, but they are
> relevant for vring interrupts.
>
> That would mean that we would need to synchronize _all_ channel I/O
> interrupts, which looks like a huge hammer. But do we really need that
> at all?

We don't, we only need to synchronize with the vring callbacks, to make sure:

1) the memory changes that is done before this op is visible to the
callbacks that came after this op
2) the memory changes that is done after this op is not visible to
callbacks that came before this op

>
> The last patch in this series seems to be concerned with the "no vring
> interrupts if the device is not ready" case, so it needs to synchronize
> vring interrupts with device reset and setting the device status to
> ready. For virtio-ccw, both reset and setting the status are channel I/O
> operations, i.e. starting a program and waiting for the final device
> interrupt for it, so synchronization (on a device level) is already
> happening in a way. So I'm not sure if any extra synchronization is
> actually needed in this case, but maybe I'm misunderstanding.
>
> Do you have further use cases in mind?

Its goal is to prevent the buggy or malicus device/host from attacking
the driver/guest. One use case is the confidential computing where
guest memory is encrypted and the guest doesn't trust the hypervisor.

In that case, the device can try to raise the interrupt after
request_irq but before DRIVER_OK, which tries to trigger the vq
callbacks when the device is not fully initialized:

request_irq()
virtio_specific_setup() // vq callbacks to be triggered in the middle
virtio_device_ready()

Thanks

>

Re: [PATCH V2 4/5] virtio-pci: implement synchronize_vqs()

[Halil Pasic]

On Wed, 06 Apr 2022 15:04:32 +0200
Cornelia Huck <cohuck@redhat.com> wrote:

> On Wed, Apr 06 2022, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Wed, Apr 06, 2022 at 04:35:37PM +0800, Jason Wang wrote:  
> >> This patch implements PCI version of synchronize_vqs().
> >> 
> >> 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>  
> >
> > Please add implementations at least for ccw and mmio.  
> 
> I'm not sure what (if anything) can/should be done for ccw...

If nothing needs to be done I would like to have at least a comment in
the code that explains why. So that somebody who reads the code
doesn't wonder: why is virtio-ccw not implementing that callback.

> 
> >  
> >> ---
> >>  drivers/virtio/virtio_pci_common.c | 14 ++++++++++++++
> >>  drivers/virtio/virtio_pci_common.h |  2 ++
> >>  drivers/virtio/virtio_pci_legacy.c |  1 +
> >>  drivers/virtio/virtio_pci_modern.c |  2 ++
> >>  4 files changed, 19 insertions(+)
> >> 
> >> diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c
> >> index d724f676608b..b78c8bc93a97 100644
> >> --- a/drivers/virtio/virtio_pci_common.c
> >> +++ b/drivers/virtio/virtio_pci_common.c
> >> @@ -37,6 +37,20 @@ void vp_synchronize_vectors(struct virtio_device *vdev)
> >>  		synchronize_irq(pci_irq_vector(vp_dev->pci_dev, i));
> >>  }
> >>  
> >> +void vp_synchronize_vqs(struct virtio_device *vdev)
> >> +{
> >> +	struct virtio_pci_device *vp_dev = to_vp_device(vdev);
> >> +	int i;
> >> +
> >> +	if (vp_dev->intx_enabled) {
> >> +		synchronize_irq(vp_dev->pci_dev->irq);
> >> +		return;
> >> +	}
> >> +
> >> +	for (i = 0; i < vp_dev->msix_vectors; ++i)
> >> +		synchronize_irq(pci_irq_vector(vp_dev->pci_dev, i));
> >> +}
> >> +  
> 
> ...given that this seems to synchronize threaded interrupt handlers?
> Halil, do you think ccw needs to do anything? (AFAICS, we only have one
> 'irq' for channel devices anyway, and the handler just calls the
> relevant callbacks directly.)

Sorry I don't understand enough yet. A more verbose documentation on
"virtio_synchronize_vqs - synchronize with virtqueue callbacks" would
surely benefit me. It may be more than enough for a back-belt but it
ain't enough for me to tell what is the callback supposed to accomplish.

I will have to study this discussion and the code more thoroughly.
Tentatively I side with Jason and Michael in a sense, that I don't
believe virtio-ccw is safe against rough interrupts.

Sorry for the late response. I intend to revisit this on Monday. If
I don't please feel encouraged to ping.

Regards,
Halil

Re: [PATCH V2 4/5] virtio-pci: implement synchronize_vqs()

[Michael S. Tsirkin]

On Fri, Apr 08, 2022 at 03:03:07PM +0200, Halil Pasic wrote:
> On Wed, 06 Apr 2022 15:04:32 +0200
> Cornelia Huck <cohuck@redhat.com> wrote:
> 
> > On Wed, Apr 06 2022, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > 
> > > On Wed, Apr 06, 2022 at 04:35:37PM +0800, Jason Wang wrote:  
> > >> This patch implements PCI version of synchronize_vqs().
> > >> 
> > >> 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>  
> > >
> > > Please add implementations at least for ccw and mmio.  
> > 
> > I'm not sure what (if anything) can/should be done for ccw...
> 
> If nothing needs to be done I would like to have at least a comment in
> the code that explains why. So that somebody who reads the code
> doesn't wonder: why is virtio-ccw not implementing that callback.

Right.

I am currently thinking instead of making this optional in the
core we should make it mandatory, and have transports which do not
need to sync have an empty stub with documentation explaining why.

Also, do we want to document this sync is explicitly for irq enable/disable?
synchronize_irq_enable_disable?


> > 
> > >  
> > >> ---
> > >>  drivers/virtio/virtio_pci_common.c | 14 ++++++++++++++
> > >>  drivers/virtio/virtio_pci_common.h |  2 ++
> > >>  drivers/virtio/virtio_pci_legacy.c |  1 +
> > >>  drivers/virtio/virtio_pci_modern.c |  2 ++
> > >>  4 files changed, 19 insertions(+)
> > >> 
> > >> diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c
> > >> index d724f676608b..b78c8bc93a97 100644
> > >> --- a/drivers/virtio/virtio_pci_common.c
> > >> +++ b/drivers/virtio/virtio_pci_common.c
> > >> @@ -37,6 +37,20 @@ void vp_synchronize_vectors(struct virtio_device *vdev)
> > >>  		synchronize_irq(pci_irq_vector(vp_dev->pci_dev, i));
> > >>  }
> > >>  
> > >> +void vp_synchronize_vqs(struct virtio_device *vdev)
> > >> +{
> > >> +	struct virtio_pci_device *vp_dev = to_vp_device(vdev);
> > >> +	int i;
> > >> +
> > >> +	if (vp_dev->intx_enabled) {
> > >> +		synchronize_irq(vp_dev->pci_dev->irq);
> > >> +		return;
> > >> +	}
> > >> +
> > >> +	for (i = 0; i < vp_dev->msix_vectors; ++i)
> > >> +		synchronize_irq(pci_irq_vector(vp_dev->pci_dev, i));
> > >> +}
> > >> +  
> > 
> > ...given that this seems to synchronize threaded interrupt handlers?
> > Halil, do you think ccw needs to do anything? (AFAICS, we only have one
> > 'irq' for channel devices anyway, and the handler just calls the
> > relevant callbacks directly.)
> 
> Sorry I don't understand enough yet. A more verbose documentation on
> "virtio_synchronize_vqs - synchronize with virtqueue callbacks" would
> surely benefit me. It may be more than enough for a back-belt but it
> ain't enough for me to tell what is the callback supposed to accomplish.
> 
> I will have to study this discussion and the code more thoroughly.
> Tentatively I side with Jason and Michael in a sense, that I don't
> believe virtio-ccw is safe against rough interrupts.
> 
> Sorry for the late response. I intend to revisit this on Monday. If
> I don't please feel encouraged to ping.
> 
> Regards,
> Halil

Re: [PATCH V2 4/5] virtio-pci: implement synchronize_vqs()

[Jason Wang]

On Sun, Apr 10, 2022 at 3:51 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Fri, Apr 08, 2022 at 03:03:07PM +0200, Halil Pasic wrote:
> > On Wed, 06 Apr 2022 15:04:32 +0200
> > Cornelia Huck <cohuck@redhat.com> wrote:
> >
> > > On Wed, Apr 06 2022, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > >
> > > > On Wed, Apr 06, 2022 at 04:35:37PM +0800, Jason Wang wrote:
> > > >> This patch implements PCI version of synchronize_vqs().
> > > >>
> > > >> 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>
> > > >
> > > > Please add implementations at least for ccw and mmio.
> > >
> > > I'm not sure what (if anything) can/should be done for ccw...
> >
> > If nothing needs to be done I would like to have at least a comment in
> > the code that explains why. So that somebody who reads the code
> > doesn't wonder: why is virtio-ccw not implementing that callback.
>
> Right.
>
> I am currently thinking instead of making this optional in the
> core we should make it mandatory, and have transports which do not
> need to sync have an empty stub with documentation explaining why.
>
> Also, do we want to document this sync is explicitly for irq enable/disable?
> synchronize_irq_enable_disable?

I would not since the transport is not guaranteed to use an interrupt
for callbacks.

>
>
> > >
> > > >
> > > >> ---
> > > >>  drivers/virtio/virtio_pci_common.c | 14 ++++++++++++++
> > > >>  drivers/virtio/virtio_pci_common.h |  2 ++
> > > >>  drivers/virtio/virtio_pci_legacy.c |  1 +
> > > >>  drivers/virtio/virtio_pci_modern.c |  2 ++
> > > >>  4 files changed, 19 insertions(+)
> > > >>
> > > >> diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c
> > > >> index d724f676608b..b78c8bc93a97 100644
> > > >> --- a/drivers/virtio/virtio_pci_common.c
> > > >> +++ b/drivers/virtio/virtio_pci_common.c
> > > >> @@ -37,6 +37,20 @@ void vp_synchronize_vectors(struct virtio_device *vdev)
> > > >>                  synchronize_irq(pci_irq_vector(vp_dev->pci_dev, i));
> > > >>  }
> > > >>
> > > >> +void vp_synchronize_vqs(struct virtio_device *vdev)
> > > >> +{
> > > >> +        struct virtio_pci_device *vp_dev = to_vp_device(vdev);
> > > >> +        int i;
> > > >> +
> > > >> +        if (vp_dev->intx_enabled) {
> > > >> +                synchronize_irq(vp_dev->pci_dev->irq);
> > > >> +                return;
> > > >> +        }
> > > >> +
> > > >> +        for (i = 0; i < vp_dev->msix_vectors; ++i)
> > > >> +                synchronize_irq(pci_irq_vector(vp_dev->pci_dev, i));
> > > >> +}
> > > >> +
> > >
> > > ...given that this seems to synchronize threaded interrupt handlers?
> > > Halil, do you think ccw needs to do anything? (AFAICS, we only have one
> > > 'irq' for channel devices anyway, and the handler just calls the
> > > relevant callbacks directly.)
> >
> > Sorry I don't understand enough yet. A more verbose documentation on
> > "virtio_synchronize_vqs - synchronize with virtqueue callbacks" would
> > surely benefit me. It may be more than enough for a back-belt but it
> > ain't enough for me to tell what is the callback supposed to accomplish.
> >
> > I will have to study this discussion and the code more thoroughly.
> > Tentatively I side with Jason and Michael in a sense, that I don't
> > believe virtio-ccw is safe against rough interrupts.

That's my feeling as well.

Thanks

> >
> > Sorry for the late response. I intend to revisit this on Monday. If
> > I don't please feel encouraged to ping.
> >
> > Regards,
> > Halil
>

Re: [PATCH V2 4/5] virtio-pci: implement synchronize_vqs()

[Michael S. Tsirkin]

On Mon, Apr 11, 2022 at 04:22:19PM +0800, Jason Wang wrote:
> On Sun, Apr 10, 2022 at 3:51 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Fri, Apr 08, 2022 at 03:03:07PM +0200, Halil Pasic wrote:
> > > On Wed, 06 Apr 2022 15:04:32 +0200
> > > Cornelia Huck <cohuck@redhat.com> wrote:
> > >
> > > > On Wed, Apr 06 2022, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > >
> > > > > On Wed, Apr 06, 2022 at 04:35:37PM +0800, Jason Wang wrote:
> > > > >> This patch implements PCI version of synchronize_vqs().
> > > > >>
> > > > >> 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>
> > > > >
> > > > > Please add implementations at least for ccw and mmio.
> > > >
> > > > I'm not sure what (if anything) can/should be done for ccw...
> > >
> > > If nothing needs to be done I would like to have at least a comment in
> > > the code that explains why. So that somebody who reads the code
> > > doesn't wonder: why is virtio-ccw not implementing that callback.
> >
> > Right.
> >
> > I am currently thinking instead of making this optional in the
> > core we should make it mandatory, and have transports which do not
> > need to sync have an empty stub with documentation explaining why.
> >
> > Also, do we want to document this sync is explicitly for irq enable/disable?
> > synchronize_irq_enable_disable?
> 
> I would not since the transport is not guaranteed to use an interrupt
> for callbacks.

OK but let's then document this in more detail.
More readers will wonder about what is the callback
trying to accomplish, and Halil requested that as well.

For example, let's document why is sync required on enable.

> >
> >
> > > >
> > > > >
> > > > >> ---
> > > > >>  drivers/virtio/virtio_pci_common.c | 14 ++++++++++++++
> > > > >>  drivers/virtio/virtio_pci_common.h |  2 ++
> > > > >>  drivers/virtio/virtio_pci_legacy.c |  1 +
> > > > >>  drivers/virtio/virtio_pci_modern.c |  2 ++
> > > > >>  4 files changed, 19 insertions(+)
> > > > >>
> > > > >> diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c
> > > > >> index d724f676608b..b78c8bc93a97 100644
> > > > >> --- a/drivers/virtio/virtio_pci_common.c
> > > > >> +++ b/drivers/virtio/virtio_pci_common.c
> > > > >> @@ -37,6 +37,20 @@ void vp_synchronize_vectors(struct virtio_device *vdev)
> > > > >>                  synchronize_irq(pci_irq_vector(vp_dev->pci_dev, i));
> > > > >>  }
> > > > >>
> > > > >> +void vp_synchronize_vqs(struct virtio_device *vdev)
> > > > >> +{
> > > > >> +        struct virtio_pci_device *vp_dev = to_vp_device(vdev);
> > > > >> +        int i;
> > > > >> +
> > > > >> +        if (vp_dev->intx_enabled) {
> > > > >> +                synchronize_irq(vp_dev->pci_dev->irq);
> > > > >> +                return;
> > > > >> +        }
> > > > >> +
> > > > >> +        for (i = 0; i < vp_dev->msix_vectors; ++i)
> > > > >> +                synchronize_irq(pci_irq_vector(vp_dev->pci_dev, i));
> > > > >> +}
> > > > >> +
> > > >
> > > > ...given that this seems to synchronize threaded interrupt handlers?
> > > > Halil, do you think ccw needs to do anything? (AFAICS, we only have one
> > > > 'irq' for channel devices anyway, and the handler just calls the
> > > > relevant callbacks directly.)
> > >
> > > Sorry I don't understand enough yet. A more verbose documentation on
> > > "virtio_synchronize_vqs - synchronize with virtqueue callbacks" would
> > > surely benefit me. It may be more than enough for a back-belt but it
> > > ain't enough for me to tell what is the callback supposed to accomplish.
> > >
> > > I will have to study this discussion and the code more thoroughly.
> > > Tentatively I side with Jason and Michael in a sense, that I don't
> > > believe virtio-ccw is safe against rough interrupts.
> 
> That's my feeling as well.
> 
> Thanks
> 
> > >
> > > Sorry for the late response. I intend to revisit this on Monday. If
> > > I don't please feel encouraged to ping.
> > >
> > > Regards,
> > > Halil
> >

Re: [PATCH V2 4/5] virtio-pci: implement synchronize_vqs()

[Jason Wang]

On Mon, Apr 11, 2022 at 4:56 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Mon, Apr 11, 2022 at 04:22:19PM +0800, Jason Wang wrote:
> > On Sun, Apr 10, 2022 at 3:51 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > >
> > > On Fri, Apr 08, 2022 at 03:03:07PM +0200, Halil Pasic wrote:
> > > > On Wed, 06 Apr 2022 15:04:32 +0200
> > > > Cornelia Huck <cohuck@redhat.com> wrote:
> > > >
> > > > > On Wed, Apr 06 2022, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > > >
> > > > > > On Wed, Apr 06, 2022 at 04:35:37PM +0800, Jason Wang wrote:
> > > > > >> This patch implements PCI version of synchronize_vqs().
> > > > > >>
> > > > > >> 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>
> > > > > >
> > > > > > Please add implementations at least for ccw and mmio.
> > > > >
> > > > > I'm not sure what (if anything) can/should be done for ccw...
> > > >
> > > > If nothing needs to be done I would like to have at least a comment in
> > > > the code that explains why. So that somebody who reads the code
> > > > doesn't wonder: why is virtio-ccw not implementing that callback.
> > >
> > > Right.
> > >
> > > I am currently thinking instead of making this optional in the
> > > core we should make it mandatory, and have transports which do not
> > > need to sync have an empty stub with documentation explaining why.
> > >
> > > Also, do we want to document this sync is explicitly for irq enable/disable?
> > > synchronize_irq_enable_disable?
> >
> > I would not since the transport is not guaranteed to use an interrupt
> > for callbacks.
>
> OK but let's then document this in more detail.
> More readers will wonder about what is the callback
> trying to accomplish, and Halil requested that as well.
>
> For example, let's document why is sync required on enable.

Ok.

Thanks

>
> > >
> > >
> > > > >
> > > > > >
> > > > > >> ---
> > > > > >>  drivers/virtio/virtio_pci_common.c | 14 ++++++++++++++
> > > > > >>  drivers/virtio/virtio_pci_common.h |  2 ++
> > > > > >>  drivers/virtio/virtio_pci_legacy.c |  1 +
> > > > > >>  drivers/virtio/virtio_pci_modern.c |  2 ++
> > > > > >>  4 files changed, 19 insertions(+)
> > > > > >>
> > > > > >> diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c
> > > > > >> index d724f676608b..b78c8bc93a97 100644
> > > > > >> --- a/drivers/virtio/virtio_pci_common.c
> > > > > >> +++ b/drivers/virtio/virtio_pci_common.c
> > > > > >> @@ -37,6 +37,20 @@ void vp_synchronize_vectors(struct virtio_device *vdev)
> > > > > >>                  synchronize_irq(pci_irq_vector(vp_dev->pci_dev, i));
> > > > > >>  }
> > > > > >>
> > > > > >> +void vp_synchronize_vqs(struct virtio_device *vdev)
> > > > > >> +{
> > > > > >> +        struct virtio_pci_device *vp_dev = to_vp_device(vdev);
> > > > > >> +        int i;
> > > > > >> +
> > > > > >> +        if (vp_dev->intx_enabled) {
> > > > > >> +                synchronize_irq(vp_dev->pci_dev->irq);
> > > > > >> +                return;
> > > > > >> +        }
> > > > > >> +
> > > > > >> +        for (i = 0; i < vp_dev->msix_vectors; ++i)
> > > > > >> +                synchronize_irq(pci_irq_vector(vp_dev->pci_dev, i));
> > > > > >> +}
> > > > > >> +
> > > > >
> > > > > ...given that this seems to synchronize threaded interrupt handlers?
> > > > > Halil, do you think ccw needs to do anything? (AFAICS, we only have one
> > > > > 'irq' for channel devices anyway, and the handler just calls the
> > > > > relevant callbacks directly.)
> > > >
> > > > Sorry I don't understand enough yet. A more verbose documentation on
> > > > "virtio_synchronize_vqs - synchronize with virtqueue callbacks" would
> > > > surely benefit me. It may be more than enough for a back-belt but it
> > > > ain't enough for me to tell what is the callback supposed to accomplish.
> > > >
> > > > I will have to study this discussion and the code more thoroughly.
> > > > Tentatively I side with Jason and Michael in a sense, that I don't
> > > > believe virtio-ccw is safe against rough interrupts.
> >
> > That's my feeling as well.
> >
> > Thanks
> >
> > > >
> > > > Sorry for the late response. I intend to revisit this on Monday. If
> > > > I don't please feel encouraged to ping.
> > > >
> > > > Regards,
> > > > Halil
> > >
>

Re: [PATCH V2 4/5] virtio-pci: implement synchronize_vqs()

[Cornelia Huck]

On Mon, Apr 11 2022, Jason Wang <jasowang@redhat.com> wrote:

> On Sun, Apr 10, 2022 at 3:51 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>>
>> On Fri, Apr 08, 2022 at 03:03:07PM +0200, Halil Pasic wrote:
>> > On Wed, 06 Apr 2022 15:04:32 +0200
>> > Cornelia Huck <cohuck@redhat.com> wrote:
>> >
>> > > On Wed, Apr 06 2022, "Michael S. Tsirkin" <mst@redhat.com> wrote:
>> > >
>> > > > On Wed, Apr 06, 2022 at 04:35:37PM +0800, Jason Wang wrote:
>> > > >> This patch implements PCI version of synchronize_vqs().
>> > > >>
>> > > >> 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>
>> > > >
>> > > > Please add implementations at least for ccw and mmio.
>> > >
>> > > I'm not sure what (if anything) can/should be done for ccw...
>> >
>> > If nothing needs to be done I would like to have at least a comment in
>> > the code that explains why. So that somebody who reads the code
>> > doesn't wonder: why is virtio-ccw not implementing that callback.
>>
>> Right.
>>
>> I am currently thinking instead of making this optional in the
>> core we should make it mandatory, and have transports which do not
>> need to sync have an empty stub with documentation explaining why.

Yes, that makes sense to me. If we can explain why we don't need to do
anything, we should keep that explanation easily accessible.

>>
>> Also, do we want to document this sync is explicitly for irq enable/disable?
>> synchronize_irq_enable_disable?
>
> I would not since the transport is not guaranteed to use an interrupt
> for callbacks.
>
>>
>>
>> > >
>> > > >
>> > > >> ---
>> > > >>  drivers/virtio/virtio_pci_common.c | 14 ++++++++++++++
>> > > >>  drivers/virtio/virtio_pci_common.h |  2 ++
>> > > >>  drivers/virtio/virtio_pci_legacy.c |  1 +
>> > > >>  drivers/virtio/virtio_pci_modern.c |  2 ++
>> > > >>  4 files changed, 19 insertions(+)
>> > > >>
>> > > >> diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c
>> > > >> index d724f676608b..b78c8bc93a97 100644
>> > > >> --- a/drivers/virtio/virtio_pci_common.c
>> > > >> +++ b/drivers/virtio/virtio_pci_common.c
>> > > >> @@ -37,6 +37,20 @@ void vp_synchronize_vectors(struct virtio_device *vdev)
>> > > >>                  synchronize_irq(pci_irq_vector(vp_dev->pci_dev, i));
>> > > >>  }
>> > > >>
>> > > >> +void vp_synchronize_vqs(struct virtio_device *vdev)
>> > > >> +{
>> > > >> +        struct virtio_pci_device *vp_dev = to_vp_device(vdev);
>> > > >> +        int i;
>> > > >> +
>> > > >> +        if (vp_dev->intx_enabled) {
>> > > >> +                synchronize_irq(vp_dev->pci_dev->irq);
>> > > >> +                return;
>> > > >> +        }
>> > > >> +
>> > > >> +        for (i = 0; i < vp_dev->msix_vectors; ++i)
>> > > >> +                synchronize_irq(pci_irq_vector(vp_dev->pci_dev, i));
>> > > >> +}
>> > > >> +
>> > >
>> > > ...given that this seems to synchronize threaded interrupt handlers?
>> > > Halil, do you think ccw needs to do anything? (AFAICS, we only have one
>> > > 'irq' for channel devices anyway, and the handler just calls the
>> > > relevant callbacks directly.)
>> >
>> > Sorry I don't understand enough yet. A more verbose documentation on
>> > "virtio_synchronize_vqs - synchronize with virtqueue callbacks" would
>> > surely benefit me. It may be more than enough for a back-belt but it
>> > ain't enough for me to tell what is the callback supposed to accomplish.

+1 for more explanations.

>> >
>> > I will have to study this discussion and the code more thoroughly.
>> > Tentatively I side with Jason and Michael in a sense, that I don't
>> > believe virtio-ccw is safe against rough interrupts.
>
> That's my feeling as well.

I'd say ccw is safe against "notification interrupts before indicators
have been registered". For the reverse case, maybe we should always
invalidate the indicators in the reset case? More information regarding
the attack vector would help here :)

My main concern is that we would need to synchronize against a single
interrupt that covers all kinds of I/O interrupts, not just a single
device...

Re: [PATCH V2 4/5] virtio-pci: implement synchronize_vqs()

[Halil Pasic]

On Mon, 11 Apr 2022 16:27:41 +0200
Cornelia Huck <cohuck@redhat.com> wrote:

> On Mon, Apr 11 2022, Jason Wang <jasowang@redhat.com> wrote:
> 
> > On Sun, Apr 10, 2022 at 3:51 PM Michael S. Tsirkin <mst@redhat.com> wrote:  
> >>
> >> On Fri, Apr 08, 2022 at 03:03:07PM +0200, Halil Pasic wrote:  
> >> > On Wed, 06 Apr 2022 15:04:32 +0200
> >> > Cornelia Huck <cohuck@redhat.com> wrote:
> >> >  
> >> > > On Wed, Apr 06 2022, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> >> > >  
> >> > > > On Wed, Apr 06, 2022 at 04:35:37PM +0800, Jason Wang wrote:  
> >> > > >> This patch implements PCI version of synchronize_vqs().
> >> > > >>
> >> > > >> 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>  
> >> > > >
> >> > > > Please add implementations at least for ccw and mmio.  
> >> > >
> >> > > I'm not sure what (if anything) can/should be done for ccw...  
> >> >
> >> > If nothing needs to be done I would like to have at least a comment in
> >> > the code that explains why. So that somebody who reads the code
> >> > doesn't wonder: why is virtio-ccw not implementing that callback.  
> >>
> >> Right.
> >>
> >> I am currently thinking instead of making this optional in the
> >> core we should make it mandatory, and have transports which do not
> >> need to sync have an empty stub with documentation explaining why.  
> 
> Yes, that makes sense to me. If we can explain why we don't need to do
> anything, we should keep that explanation easily accessible.
> 
> >>
> >> Also, do we want to document this sync is explicitly for irq enable/disable?
> >> synchronize_irq_enable_disable?  
> >
> > I would not since the transport is not guaranteed to use an interrupt
> > for callbacks.
> >  
> >>
> >>  
> >> > >  
> >> > > >  
> >> > > >> ---
> >> > > >>  drivers/virtio/virtio_pci_common.c | 14 ++++++++++++++
> >> > > >>  drivers/virtio/virtio_pci_common.h |  2 ++
> >> > > >>  drivers/virtio/virtio_pci_legacy.c |  1 +
> >> > > >>  drivers/virtio/virtio_pci_modern.c |  2 ++
> >> > > >>  4 files changed, 19 insertions(+)
> >> > > >>
> >> > > >> diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c
> >> > > >> index d724f676608b..b78c8bc93a97 100644
> >> > > >> --- a/drivers/virtio/virtio_pci_common.c
> >> > > >> +++ b/drivers/virtio/virtio_pci_common.c
> >> > > >> @@ -37,6 +37,20 @@ void vp_synchronize_vectors(struct virtio_device *vdev)
> >> > > >>                  synchronize_irq(pci_irq_vector(vp_dev->pci_dev, i));
> >> > > >>  }
> >> > > >>
> >> > > >> +void vp_synchronize_vqs(struct virtio_device *vdev)
> >> > > >> +{
> >> > > >> +        struct virtio_pci_device *vp_dev = to_vp_device(vdev);
> >> > > >> +        int i;
> >> > > >> +
> >> > > >> +        if (vp_dev->intx_enabled) {
> >> > > >> +                synchronize_irq(vp_dev->pci_dev->irq);
> >> > > >> +                return;
> >> > > >> +        }
> >> > > >> +
> >> > > >> +        for (i = 0; i < vp_dev->msix_vectors; ++i)
> >> > > >> +                synchronize_irq(pci_irq_vector(vp_dev->pci_dev, i));
> >> > > >> +}
> >> > > >> +  
> >> > >
> >> > > ...given that this seems to synchronize threaded interrupt handlers?
> >> > > Halil, do you think ccw needs to do anything? (AFAICS, we only have one
> >> > > 'irq' for channel devices anyway, and the handler just calls the
> >> > > relevant callbacks directly.)  
> >> >
> >> > Sorry I don't understand enough yet. A more verbose documentation on
> >> > "virtio_synchronize_vqs - synchronize with virtqueue callbacks" would
> >> > surely benefit me. It may be more than enough for a back-belt but it
> >> > ain't enough for me to tell what is the callback supposed to accomplish.  
> 
> +1 for more explanations.
> 
> >> >
> >> > I will have to study this discussion and the code more thoroughly.
> >> > Tentatively I side with Jason and Michael in a sense, that I don't
> >> > believe virtio-ccw is safe against rough interrupts.  
> >
> > That's my feeling as well.  
> 
> I'd say ccw is safe against "notification interrupts before indicators
> have been registered".

I believe Jason's scope is broader than that. Let me try to explain. A
quote form the standard:
"""
3.1.1 Driver Requirements: Device Initialization
The driver MUST follow this sequence to initialize a device: 
    1. Reset the device. 
    2. Set the ACKNOWLEDGE status bit: the guest OS has noticed the device. 
    3. Set the DRIVER status bit: the guest OS knows how to drive the device. 
    4. Read device feature bits, and write the subset of feature bits understood by the OS and driver to the device. During this step the driver MAY read (but MUST NOT write) the device-specific configuration fields to check that it can support the device before accepting it. 
    5. Set the FEATURES_OK status bit. The driver MUST NOT accept new feature bits after this step. 
    6. Re-read device status to ensure the FEATURES_OK bit is still set: otherwise, the device does not support our subset of features and the device is unusable. 
    7. Perform device-specific setup, including discovery of virtqueues for the device, optional per-bus setup, reading and possibly writing the device’s virtio configuration space, and population of virtqueues. 
    8. Set the DRIVER_OK status bit. At this point the device is “live”.
"""

That means stuff may happen between "discovery of virtqueues" and "DRIVER_OK". So it
is not sufficient to be "safe against notifications before indicators
have been registered", but we want to be also safe between "indicators have
been registered" and "DRIVER_OK status has been set". 

@Jason: can you confirm?

Regarding the question "are we safe against notifications before
indicators have been registered" I think we really need to think about
something like Secure Execution. We don't have, and we are unlikely
to have in hardware virtio-ccw implementations, and for a malicious hypervisor
that has full access to the guest memory hardening makes no sense.

But if we assume that an attacker can inject adapter interrupts for an arbitrary
ISC, and can poke any shared memory (notifier bits are shared)... Things might
become critical already when register_adapter_interrupt() does it's magic.


> For the reverse case, maybe we should always
> invalidate the indicators in the reset case? More information regarding
> the attack vector would help here :)
> 
> My main concern is that we would need to synchronize against a single
> interrupt that covers all kinds of I/O interrupts, not just a single
> device...
> 

Could we synchronize on struct airq_info's lock member? If we were
to grab all of these that might be involved...

AFAIU for the synchronize implementation we need a lock or a set of locks
that contain all the possible vring_interrupt() calls with the queuues
that belong to the given device as a critical section. That way, one
has the acquire's and release's in place so that the vrign_interrupt()
either guaranteed to finish before the change of driver_ready is
guaranteed to be complete, or it is guaranteed to see the change.

In any case, I guess we should first get clear on the first part. I.e.
when do we want to allow host->guest notifications.

Regards,
Halil

Re: [PATCH V2 4/5] virtio-pci: implement synchronize_vqs()

[Jason Wang]

On Tue, Apr 12, 2022 at 8:02 AM Halil Pasic <pasic@linux.ibm.com> wrote:
>
> On Mon, 11 Apr 2022 16:27:41 +0200
> Cornelia Huck <cohuck@redhat.com> wrote:
>
> > On Mon, Apr 11 2022, Jason Wang <jasowang@redhat.com> wrote:
> >
> > > On Sun, Apr 10, 2022 at 3:51 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > >>
> > >> On Fri, Apr 08, 2022 at 03:03:07PM +0200, Halil Pasic wrote:
> > >> > On Wed, 06 Apr 2022 15:04:32 +0200
> > >> > Cornelia Huck <cohuck@redhat.com> wrote:
> > >> >
> > >> > > On Wed, Apr 06 2022, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > >> > >
> > >> > > > On Wed, Apr 06, 2022 at 04:35:37PM +0800, Jason Wang wrote:
> > >> > > >> This patch implements PCI version of synchronize_vqs().
> > >> > > >>
> > >> > > >> 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>
> > >> > > >
> > >> > > > Please add implementations at least for ccw and mmio.
> > >> > >
> > >> > > I'm not sure what (if anything) can/should be done for ccw...
> > >> >
> > >> > If nothing needs to be done I would like to have at least a comment in
> > >> > the code that explains why. So that somebody who reads the code
> > >> > doesn't wonder: why is virtio-ccw not implementing that callback.
> > >>
> > >> Right.
> > >>
> > >> I am currently thinking instead of making this optional in the
> > >> core we should make it mandatory, and have transports which do not
> > >> need to sync have an empty stub with documentation explaining why.
> >
> > Yes, that makes sense to me. If we can explain why we don't need to do
> > anything, we should keep that explanation easily accessible.
> >
> > >>
> > >> Also, do we want to document this sync is explicitly for irq enable/disable?
> > >> synchronize_irq_enable_disable?
> > >
> > > I would not since the transport is not guaranteed to use an interrupt
> > > for callbacks.
> > >
> > >>
> > >>
> > >> > >
> > >> > > >
> > >> > > >> ---
> > >> > > >>  drivers/virtio/virtio_pci_common.c | 14 ++++++++++++++
> > >> > > >>  drivers/virtio/virtio_pci_common.h |  2 ++
> > >> > > >>  drivers/virtio/virtio_pci_legacy.c |  1 +
> > >> > > >>  drivers/virtio/virtio_pci_modern.c |  2 ++
> > >> > > >>  4 files changed, 19 insertions(+)
> > >> > > >>
> > >> > > >> diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c
> > >> > > >> index d724f676608b..b78c8bc93a97 100644
> > >> > > >> --- a/drivers/virtio/virtio_pci_common.c
> > >> > > >> +++ b/drivers/virtio/virtio_pci_common.c
> > >> > > >> @@ -37,6 +37,20 @@ void vp_synchronize_vectors(struct virtio_device *vdev)
> > >> > > >>                  synchronize_irq(pci_irq_vector(vp_dev->pci_dev, i));
> > >> > > >>  }
> > >> > > >>
> > >> > > >> +void vp_synchronize_vqs(struct virtio_device *vdev)
> > >> > > >> +{
> > >> > > >> +        struct virtio_pci_device *vp_dev = to_vp_device(vdev);
> > >> > > >> +        int i;
> > >> > > >> +
> > >> > > >> +        if (vp_dev->intx_enabled) {
> > >> > > >> +                synchronize_irq(vp_dev->pci_dev->irq);
> > >> > > >> +                return;
> > >> > > >> +        }
> > >> > > >> +
> > >> > > >> +        for (i = 0; i < vp_dev->msix_vectors; ++i)
> > >> > > >> +                synchronize_irq(pci_irq_vector(vp_dev->pci_dev, i));
> > >> > > >> +}
> > >> > > >> +
> > >> > >
> > >> > > ...given that this seems to synchronize threaded interrupt handlers?
> > >> > > Halil, do you think ccw needs to do anything? (AFAICS, we only have one
> > >> > > 'irq' for channel devices anyway, and the handler just calls the
> > >> > > relevant callbacks directly.)
> > >> >
> > >> > Sorry I don't understand enough yet. A more verbose documentation on
> > >> > "virtio_synchronize_vqs - synchronize with virtqueue callbacks" would
> > >> > surely benefit me. It may be more than enough for a back-belt but it
> > >> > ain't enough for me to tell what is the callback supposed to accomplish.
> >
> > +1 for more explanations.
> >
> > >> >
> > >> > I will have to study this discussion and the code more thoroughly.
> > >> > Tentatively I side with Jason and Michael in a sense, that I don't
> > >> > believe virtio-ccw is safe against rough interrupts.
> > >
> > > That's my feeling as well.
> >
> > I'd say ccw is safe against "notification interrupts before indicators
> > have been registered".
>
> I believe Jason's scope is broader than that. Let me try to explain. A
> quote form the standard:
> """
> 3.1.1 Driver Requirements: Device Initialization
> The driver MUST follow this sequence to initialize a device:
>     1. Reset the device.
>     2. Set the ACKNOWLEDGE status bit: the guest OS has noticed the device.
>     3. Set the DRIVER status bit: the guest OS knows how to drive the device.
>     4. Read device feature bits, and write the subset of feature bits understood by the OS and driver to the device. During this step the driver MAY read (but MUST NOT write) the device-specific configuration fields to check that it can support the device before accepting it.
>     5. Set the FEATURES_OK status bit. The driver MUST NOT accept new feature bits after this step.
>     6. Re-read device status to ensure the FEATURES_OK bit is still set: otherwise, the device does not support our subset of features and the device is unusable.
>     7. Perform device-specific setup, including discovery of virtqueues for the device, optional per-bus setup, reading and possibly writing the device’s virtio configuration space, and population of virtqueues.
>     8. Set the DRIVER_OK status bit. At this point the device is “live”.
> """
>
> That means stuff may happen between "discovery of virtqueues" and "DRIVER_OK". So it
> is not sufficient to be "safe against notifications before indicators
> have been registered", but we want to be also safe between "indicators have
> been registered" and "DRIVER_OK status has been set".
>
> @Jason: can you confirm?

Exactly.

>
> Regarding the question "are we safe against notifications before
> indicators have been registered" I think we really need to think about
> something like Secure Execution. We don't have, and we are unlikely
> to have in hardware virtio-ccw implementations, and for a malicious hypervisor
> that has full access to the guest memory hardening makes no sense.

Does s390 have something like memory encryption? (I guess yes). In the
case of x86 VM encryption, the I/O buffers were now done via software
IOTLB, that's why hardening of the virtio driver is needed to prevent
the hypervisor to poke the swiotlb etc.

Thanks

>
> But if we assume that an attacker can inject adapter interrupts for an arbitrary
> ISC, and can poke any shared memory (notifier bits are shared)... Things might
> become critical already when register_adapter_interrupt() does it's magic.
>
>
> > For the reverse case, maybe we should always
> > invalidate the indicators in the reset case? More information regarding
> > the attack vector would help here :)
> >
> > My main concern is that we would need to synchronize against a single
> > interrupt that covers all kinds of I/O interrupts, not just a single
> > device...
> >
>
> Could we synchronize on struct airq_info's lock member? If we were
> to grab all of these that might be involved...
>
> AFAIU for the synchronize implementation we need a lock or a set of locks
> that contain all the possible vring_interrupt() calls with the queuues
> that belong to the given device as a critical section. That way, one
> has the acquire's and release's in place so that the vrign_interrupt()
> either guaranteed to finish before the change of driver_ready is
> guaranteed to be complete, or it is guaranteed to see the change.
>
> In any case, I guess we should first get clear on the first part. I.e.
> when do we want to allow host->guest notifications.
>
> Regards,
> Halil
>

Re: [PATCH V2 4/5] virtio-pci: implement synchronize_vqs()

[Halil Pasic]

On Tue, 12 Apr 2022 10:24:35 +0800
Jason Wang <jasowang@redhat.com> wrote:

> > Regarding the question "are we safe against notifications before
> > indicators have been registered" I think we really need to think about
> > something like Secure Execution. We don't have, and we are unlikely
> > to have in hardware virtio-ccw implementations, and for a malicious hypervisor
> > that has full access to the guest memory hardening makes no sense.  
> 
> Does s390 have something like memory encryption? (I guess yes). In the
> case of x86 VM encryption, the I/O buffers were now done via software
> IOTLB, that's why hardening of the virtio driver is needed to prevent
> the hypervisor to poke the swiotlb etc.

Yep! Secure Execution is a confidential computing solution which is much
like encrypted guest memory, except for one gets exceptions when trying
to access private memory instead of ending up with garbage  because of
the encryption. These improvements are IMHO relevant to us!

Regards,
Halil

Re: [PATCH V2 4/5] virtio-pci: implement synchronize_vqs()

[Cornelia Huck]

On Tue, Apr 12 2022, Halil Pasic <pasic@linux.ibm.com> wrote:

> On Mon, 11 Apr 2022 16:27:41 +0200
> Cornelia Huck <cohuck@redhat.com> wrote:

>> My main concern is that we would need to synchronize against a single
>> interrupt that covers all kinds of I/O interrupts, not just a single
>> device...
>> 
>
> Could we synchronize on struct airq_info's lock member? If we were
> to grab all of these that might be involved...

Hm, that could possibly narrow the sync down to a subset, which seems
better. For devices still using classic interrupts, per-device sync
would be easy.

>
> AFAIU for the synchronize implementation we need a lock or a set of locks
> that contain all the possible vring_interrupt() calls with the queuues
> that belong to the given device as a critical section. That way, one
> has the acquire's and release's in place so that the vrign_interrupt()
> either guaranteed to finish before the change of driver_ready is
> guaranteed to be complete, or it is guaranteed to see the change.
>
> In any case, I guess we should first get clear on the first part. I.e.
> when do we want to allow host->guest notifications.

Also, whether we just care about vring interrupts, or general device
interrupts (not sure if a config change interrupt may also trigger
things we do not want to trigger?)

Re: [PATCH V2 4/5] virtio-pci: implement synchronize_vqs()

[Jason Wang]

On Wed, Apr 13, 2022 at 12:49 AM Cornelia Huck <cohuck@redhat.com> wrote:
>
> On Tue, Apr 12 2022, Halil Pasic <pasic@linux.ibm.com> wrote:
>
> > On Mon, 11 Apr 2022 16:27:41 +0200
> > Cornelia Huck <cohuck@redhat.com> wrote:
>
> >> My main concern is that we would need to synchronize against a single
> >> interrupt that covers all kinds of I/O interrupts, not just a single
> >> device...
> >>
> >
> > Could we synchronize on struct airq_info's lock member? If we were
> > to grab all of these that might be involved...
>
> Hm, that could possibly narrow the sync down to a subset, which seems
> better. For devices still using classic interrupts, per-device sync
> would be easy.
>
> >
> > AFAIU for the synchronize implementation we need a lock or a set of locks
> > that contain all the possible vring_interrupt() calls with the queuues
> > that belong to the given device as a critical section. That way, one
> > has the acquire's and release's in place so that the vrign_interrupt()
> > either guaranteed to finish before the change of driver_ready is
> > guaranteed to be complete, or it is guaranteed to see the change.
> >
> > In any case, I guess we should first get clear on the first part. I.e.
> > when do we want to allow host->guest notifications.
>
> Also, whether we just care about vring interrupts, or general device
> interrupts (not sure if a config change interrupt may also trigger
> things we do not want to trigger?)

I think only vring interrupts, since the config interrupt hardening is
done via 22b7050a024d7 ("virtio: defer config changed notifications")

Thanks

>

Re: [PATCH V2 4/5] virtio-pci: implement synchronize_vqs()

[Cornelia Huck]

On Wed, Apr 13 2022, Jason Wang <jasowang@redhat.com> wrote:

> On Wed, Apr 13, 2022 at 12:49 AM Cornelia Huck <cohuck@redhat.com> wrote:
>>
>> On Tue, Apr 12 2022, Halil Pasic <pasic@linux.ibm.com> wrote:
>>
>> > On Mon, 11 Apr 2022 16:27:41 +0200
>> > Cornelia Huck <cohuck@redhat.com> wrote:
>>
>> >> My main concern is that we would need to synchronize against a single
>> >> interrupt that covers all kinds of I/O interrupts, not just a single
>> >> device...
>> >>
>> >
>> > Could we synchronize on struct airq_info's lock member? If we were
>> > to grab all of these that might be involved...
>>
>> Hm, that could possibly narrow the sync down to a subset, which seems
>> better. For devices still using classic interrupts, per-device sync
>> would be easy.
>>
>> >
>> > AFAIU for the synchronize implementation we need a lock or a set of locks
>> > that contain all the possible vring_interrupt() calls with the queuues
>> > that belong to the given device as a critical section. That way, one
>> > has the acquire's and release's in place so that the vrign_interrupt()
>> > either guaranteed to finish before the change of driver_ready is
>> > guaranteed to be complete, or it is guaranteed to see the change.
>> >
>> > In any case, I guess we should first get clear on the first part. I.e.
>> > when do we want to allow host->guest notifications.
>>
>> Also, whether we just care about vring interrupts, or general device
>> interrupts (not sure if a config change interrupt may also trigger
>> things we do not want to trigger?)
>
> I think only vring interrupts, since the config interrupt hardening is
> done via 22b7050a024d7 ("virtio: defer config changed notifications")

Ah thanks, I even reviewed that one back then :)

[PATCH V2 5/5] virtio: harden vring IRQ

[Jason Wang]

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)) {
+		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
+         * vring_interrupt() will see the driver specific setup if it
+         * see driver_ready as true.
+         */
+	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

Re: [PATCH V2 5/5] virtio: harden vring IRQ

[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.


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

Re: [PATCH V2 5/5] virtio: harden vring IRQ

[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

Re: [PATCH V2 0/5] rework on the IRQ hardening of virtio

[Michael S. Tsirkin]

On Wed, Apr 06, 2022 at 04:35:33PM +0800, Jason Wang wrote:
> Hi All:
> 
> This is a rework on the IRQ hardening for virtio which is done
> previously by the following commits are reverted:
> 
> 9e35276a5344 ("virtio_pci: harden MSI-X interrupts")
> 080cd7c3ac87 ("virtio-pci: harden INTX interrupts")
> 
> The reason is that it depends on the IRQF_NO_AUTOEN which may conflict
> with the assumption of the affinity managed IRQ that is used by some
> virtio drivers. And what's more, it is only done for virtio-pci but
> not other transports.
> 
> In this rework, I try to implement a general virtio solution which
> borrows the idea of the INTX hardening by introducing a boolean for
> virtqueue callback enabling and toggle it in virtio_device_ready()
> and virtio_reset_device(). Then vring_interrupt() can simply check and
> return early if the driver is not ready.


All of a sudden all patches are having a wrong mime type.

It is application/octet-stream; should be text/plain

Pls fix and repost, thanks!

> Please review.
> 
> Changes since v1:
> 
> - Use transport specific irq synchronization method when possible
> - Drop the module parameter and enable the hardening unconditonally
> - Tweak the barrier/ordering facilities used in the code
> - Reanme irq_soft_enabled to driver_ready
> - Avoid unnecssary IRQ synchornization (e.g during boot)
> 
> Jason Wang (4):
>   virtio: use virtio_reset_device() when possible
>   virtio: introduce config op to synchronize vring callbacks
>   virtio-pci: implement synchronize_vqs()
>   virtio: harden vring IRQ
> 
> Stefano Garzarella (1):
>   virtio: use virtio_device_ready() in virtio_device_restore()
> 
>  drivers/virtio/virtio.c            | 20 ++++++++++++++++----
>  drivers/virtio/virtio_pci_common.c | 14 ++++++++++++++
>  drivers/virtio/virtio_pci_common.h |  2 ++
>  drivers/virtio/virtio_pci_legacy.c |  1 +
>  drivers/virtio/virtio_pci_modern.c |  2 ++
>  drivers/virtio/virtio_ring.c       |  9 ++++++++-
>  include/linux/virtio.h             |  2 ++
>  include/linux/virtio_config.h      | 24 ++++++++++++++++++++++++
>  8 files changed, 69 insertions(+), 5 deletions(-)
> 
> -- 
> 2.25.1

Re: [PATCH V2 0/5] rework on the IRQ hardening of virtio

[Jason Wang]

在 2022/4/6 下午7:36, Michael S. Tsirkin 写道:
> On Wed, Apr 06, 2022 at 04:35:33PM +0800, Jason Wang wrote:
>> Hi All:
>>
>> This is a rework on the IRQ hardening for virtio which is done
>> previously by the following commits are reverted:
>>
>> 9e35276a5344 ("virtio_pci: harden MSI-X interrupts")
>> 080cd7c3ac87 ("virtio-pci: harden INTX interrupts")
>>
>> The reason is that it depends on the IRQF_NO_AUTOEN which may conflict
>> with the assumption of the affinity managed IRQ that is used by some
>> virtio drivers. And what's more, it is only done for virtio-pci but
>> not other transports.
>>
>> In this rework, I try to implement a general virtio solution which
>> borrows the idea of the INTX hardening by introducing a boolean for
>> virtqueue callback enabling and toggle it in virtio_device_ready()
>> and virtio_reset_device(). Then vring_interrupt() can simply check and
>> return early if the driver is not ready.
>
> All of a sudden all patches are having a wrong mime type.
>
> It is application/octet-stream; should be text/plain
>
> Pls fix and repost, thanks!


So the patches are generated via git-format-patch and git-send-email in 
one run.

I can see many upstream patches were converted to 
application/octet-stream if From: tag is different from the sender.

Maxime told me they've also noticed the issue and it looks like a issue 
of mimecast.

Thanks


>
>> Please review.
>>
>> Changes since v1:
>>
>> - Use transport specific irq synchronization method when possible
>> - Drop the module parameter and enable the hardening unconditonally
>> - Tweak the barrier/ordering facilities used in the code
>> - Reanme irq_soft_enabled to driver_ready
>> - Avoid unnecssary IRQ synchornization (e.g during boot)
>>
>> Jason Wang (4):
>>    virtio: use virtio_reset_device() when possible
>>    virtio: introduce config op to synchronize vring callbacks
>>    virtio-pci: implement synchronize_vqs()
>>    virtio: harden vring IRQ
>>
>> Stefano Garzarella (1):
>>    virtio: use virtio_device_ready() in virtio_device_restore()
>>
>>   drivers/virtio/virtio.c            | 20 ++++++++++++++++----
>>   drivers/virtio/virtio_pci_common.c | 14 ++++++++++++++
>>   drivers/virtio/virtio_pci_common.h |  2 ++
>>   drivers/virtio/virtio_pci_legacy.c |  1 +
>>   drivers/virtio/virtio_pci_modern.c |  2 ++
>>   drivers/virtio/virtio_ring.c       |  9 ++++++++-
>>   include/linux/virtio.h             |  2 ++
>>   include/linux/virtio_config.h      | 24 ++++++++++++++++++++++++
>>   8 files changed, 69 insertions(+), 5 deletions(-)
>>
>> -- 
>> 2.25.1