Re: [PATCH 02/16] virtio: unify config_changed handling
On Sun, 5 Oct 2014 19:06:52 +0300 Michael S. Tsirkin m...@redhat.com wrote: Replace duplicated code in all transports with a single wrapper in virtio.c. The only functional change is in virtio_mmio.c: if a buggy device sends us an interrupt before driver is set, we previously returned IRQ_NONE, now we return IRQ_HANDLED. As this must not happen in practice, this does not look like a big deal. See also commit 3fff0179e33cd7d0a688dab65700c46ad089e934 virtio-pci: do not oops on config change if driver not loaded. for the original motivation behind the driver check. Signed-off-by: Michael S. Tsirkin m...@redhat.com --- include/linux/virtio.h | 2 ++ drivers/misc/mic/card/mic_virtio.c | 6 +- drivers/s390/kvm/kvm_virtio.c | 9 + drivers/s390/kvm/virtio_ccw.c | 6 +- drivers/virtio/virtio.c| 10 ++ drivers/virtio/virtio_mmio.c | 7 ++- drivers/virtio/virtio_pci.c| 6 +- 7 files changed, 18 insertions(+), 28 deletions(-) Reviewed-by: Cornelia Huck cornelia.h...@de.ibm.com ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v5 2/3] virtio_pci: Use the DMA API for virtqueues when possible
Am 29.09.2014 20:55, schrieb Andy Lutomirski: On Wed, Sep 17, 2014 at 7:16 AM, Michael S. Tsirkin m...@redhat.com wrote: On Wed, Sep 17, 2014 at 08:02:31AM -0400, Benjamin Herrenschmidt wrote: On Tue, 2014-09-16 at 22:22 -0700, Andy Lutomirski wrote: On non-PPC systems, virtio_pci should use the DMA API. This fixes virtio_pci on Xen. On PPC, using the DMA API would break things, so we need to preserve the old behavior. The big comment in this patch explains the considerations in more detail. I still disagree with using CONFIG_PPC as a trigger here. Fundamentally, the qemu implementation today bypasses IOMMUs on all platforms as far as I can tell. If that changes, we'll have a backward compatibility problem. The virtio device should advertise whether it's using that bypass mode of operation and virtio_pci should react accordingly. Well if there's a way to detect that - that's outside the device, then we probably shouldn't use device-specific interfaces for this capability. There is a demand for being able to operate on top of an IOMMU on powerpc as well for some embedded stuff using PCI as a transport so your patch precludes that. Cheers, Ben. As far as I can see, nothing changes on PPC so this patch does not preclude anything that was working? Rusty and Michael, what's the status of this? I think that (aside from the trivial DMI/DMA typo) the only real issue here is that the situation on PPC is ugly. We're failing to enable physical virtio hardware on PPC with these patches, but that never worked anyway. I don't think that there are any regressions other than ugliness. My preference would be to apply the patches as is (or with DMA spelled correctly), and then to: - Make sure that all virtio-mmio systems have working DMA ops so that virtio-mmio can the DMA API - Fix the DMA API on s390 (probably easy) and on PPC (not necessarily so easy) Just as a comment: On s390 we always considered the memory access as access to real memory (not device memory) for virtio accesses. I prefer to not touch the DMA API on s390 as it is quite s390-PCI specific but it is somewhat strange for CCW devices. We would basically have two kinds of DMA mappings on s390 then, one iommu like for the PCI devices and one that maps DMA memory 1:1 to real memory. We would also need some buy-in from the s390 arch maintainers then. - Remove the non-DMA-API code, which would be a very small change on top of these patches. --Andy -- To unsubscribe from this list: send the line unsubscribe linux-s390 in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 03/16] virtio: refactor to use drv_to_virtio
On Sun, 5 Oct 2014 19:06:59 +0300 Michael S. Tsirkin m...@redhat.com wrote: Use drv_to_virtio instead of open-coding it. Fix whitespace damage introduced by virtio: unify config_changed handling Would it make sense to merge this into the previous patch? Signed-off-by: Michael S. Tsirkin m...@redhat.com --- drivers/virtio/virtio.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v5 2/3] virtio_pci: Use the DMA API for virtqueues when possible
On Mon, 2014-10-06 at 11:59 +0200, Christian Borntraeger wrote: Just as a comment: On s390 we always considered the memory access as access to real memory (not device memory) for virtio accesses. I prefer to not touch the DMA API on s390 as it is quite s390-PCI specific but it is somewhat strange for CCW devices. We would basically have two kinds of DMA mappings on s390 then, one iommu like for the PCI devices and one that maps DMA memory 1:1 to real memory. We would also need some buy-in from the s390 arch maintainers then. It's pretty standard to have the dma API go via dma-ops hanging off struct device, you can then easily make them to the right thing for the type of device... Provided we have a way to identify bypass everyting virtio vs. virtio going through normal PCI translation. Cheers, Ben. ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 04/16] virtio-pci: move freeze/restore to virtio core
On Sun, 5 Oct 2014 19:07:02 +0300 Michael S. Tsirkin m...@redhat.com wrote: This is in preparation to extending config changed event handling in core. Wrapping these in an API also seems to make for a cleaner code. Signed-off-by: Michael S. Tsirkin m...@redhat.com --- include/linux/virtio.h | 6 + drivers/virtio/virtio.c | 53 +++ drivers/virtio/virtio_pci.c | 55 ++--- 3 files changed, 61 insertions(+), 53 deletions(-) diff --git a/include/linux/virtio.h b/include/linux/virtio.h index 3c19bd3..8df7ba8 100644 --- a/include/linux/virtio.h +++ b/include/linux/virtio.h @@ -78,6 +78,7 @@ bool virtqueue_is_broken(struct virtqueue *vq); /** * virtio_device - representation of a device using virtio * @index: unique position on the virtio bus + * @failed: saved value for CONFIG_S_FAILED bit (for restore) * @dev: underlying device. * @id: the device type identification (used to match it with a driver). * @config: the configuration ops for this device. The only minor gripe I have is with the naming of this field: To a cursory reader, it might indicate the current 'failed' status of the device. What about 'saved_failed'? Otherwise, moving this into common virtio code makes sense. Reviewed-by: Cornelia Huck cornelia.h...@de.ibm.com ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 05/16] virtio: defer config changed notifications
On Sun, 5 Oct 2014 19:07:05 +0300
Michael S. Tsirkin m...@redhat.com wrote:
Defer config changed notifications that arrive during
probe/scan/freeze/restore.
This will allow drivers to set DRIVER_OK earlier, without worrying about
racing with config change interrupts.
This change will also benefit old hypervisors (before 2009)
that send interrupts without checking DRIVER_OK: previously,
the callback could race with driver-specific initialization.
This will also help simplify drivers.
Signed-off-by: Michael S. Tsirkin m...@redhat.com
---
include/linux/virtio.h | 6 ++
drivers/virtio/virtio.c | 55
+
2 files changed, 52 insertions(+), 9 deletions(-)
diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
index 657f817..578e02d 100644
--- a/drivers/virtio/virtio.c
+++ b/drivers/virtio/virtio.c
@@ -117,6 +117,40 @@ void virtio_check_driver_offered_feature(const struct
virtio_device *vdev,
}
EXPORT_SYMBOL_GPL(virtio_check_driver_offered_feature);
+static void __virtio_config_changed(struct virtio_device *dev)
+{
+ struct virtio_driver *drv = drv_to_virtio(dev-dev.driver);
+
+ if (!dev-config_enabled)
+ dev-config_changed = true;
+ else if (drv drv-config_changed)
+ drv-config_changed(dev);
+}
+
+void virtio_config_changed(struct virtio_device *dev)
+{
+ spin_lock_irq(dev-config_lock);
+ __virtio_config_changed(dev);
+ spin_unlock_irq(dev-config_lock);
Hm, isn't this function called from the interrupt handler?
+}
+EXPORT_SYMBOL_GPL(virtio_config_changed);
+
+static void virtio_config_disable(struct virtio_device *dev)
+{
+ spin_lock_irq(dev-config_lock);
+ dev-config_enabled = false;
+ spin_unlock_irq(dev-config_lock);
+}
+
+static void virtio_config_enable(struct virtio_device *dev)
+{
+ spin_lock_irq(dev-config_lock);
+ dev-config_enabled = true;
+ __virtio_config_changed(dev);
+ dev-config_changed = false;
+ spin_unlock_irq(dev-config_lock);
+}
+
Maybe call these virtio_config_change_{dis,en}able()?
static int virtio_dev_probe(struct device *_d)
{
int err, i;
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 06/16] virtio_blk: drop config_enable
On Sun, 5 Oct 2014 19:07:07 +0300 Michael S. Tsirkin m...@redhat.com wrote: Now that virtio core ensures config changes don't arrive during probing, drop config_enable flag in virtio blk. On removal, flush is now sufficient to guarantee that no change work is queued. This help simplify the driver, and will allow setting DRIVER_OK earlier without losing config change notifications. Signed-off-by: Michael S. Tsirkin m...@redhat.com --- drivers/block/virtio_blk.c | 19 ++- 1 file changed, 2 insertions(+), 17 deletions(-) diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c index 0a58140..c8cf6a1 100644 --- a/drivers/block/virtio_blk.c +++ b/drivers/block/virtio_blk.c @@ -772,9 +766,7 @@ static void virtblk_remove(struct virtio_device *vdev) int refc; /* Prevent config work handler from accessing the device. */ /* Common code ensures no further work will be queued. */ instead? - mutex_lock(vblk-config_lock); - vblk-config_enable = false; - mutex_unlock(vblk-config_lock); + flush_work(vblk-config_work); del_gendisk(vblk-disk); blk_cleanup_queue(vblk-disk-queue); @@ -806,10 +796,6 @@ static int virtblk_freeze(struct virtio_device *vdev) vdev-config-reset(vdev); /* Prevent config work handler from accessing the device. */ dito on the comment - mutex_lock(vblk-config_lock); - vblk-config_enable = false; - mutex_unlock(vblk-config_lock); - flush_work(vblk-config_work); blk_mq_stop_hw_queues(vblk-disk-queue); ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 09/16] virtio-net: drop config_mutex
Hello. On 10/5/2014 8:07 PM, Michael S. Tsirkin wrote: config_mutex served two purposes: prevent multiple concurrent config change handlers, and synchronize access to config_enable flag. Since commit dbf2576e37da0fcc7aacbfbb9fd5d3de7888a3c1 workqueue: make all workqueues non-reentrant all workqueues are non-reentrant, and config_enable is now gone. Get rid of the unnecessary lock. Signed-off-by: Michael S. Tsirkin m...@redhat.com --- drivers/net/virtio_net.c | 7 +-- 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c index fa17afa..d80fef4 100644 --- a/drivers/net/virtio_net.c +++ b/drivers/net/virtio_net.c [...] @@ -1430,7 +1426,7 @@ static void virtnet_config_changed_work(struct work_struct *work) netif_tx_stop_all_queues(vi-dev); } done: - mutex_unlock(vi-config_lock); + return; There's no need for this *return*. } static void virtnet_config_changed(struct virtio_device *vdev) WBR, Sergei ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 07/16] virtio-blk: drop config_mutex
On Sun, 5 Oct 2014 19:07:10 +0300 Michael S. Tsirkin m...@redhat.com wrote: config_mutex served two purposes: prevent multiple concurrent config change handlers, and synchronize access to config_enable flag. Since commit dbf2576e37da0fcc7aacbfbb9fd5d3de7888a3c1 workqueue: make all workqueues non-reentrant all workqueues are non-reentrant, and config_enable is now gone. Get rid of the unnecessary lock. Signed-off-by: Michael S. Tsirkin m...@redhat.com --- drivers/block/virtio_blk.c | 8 1 file changed, 8 deletions(-) Reviewed-by: Cornelia Huck cornelia.h...@de.ibm.com ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 09/16] virtio-net: drop config_mutex
On Mon, Oct 06, 2014 at 03:46:15PM +0400, Sergei Shtylyov wrote: Hello. On 10/5/2014 8:07 PM, Michael S. Tsirkin wrote: config_mutex served two purposes: prevent multiple concurrent config change handlers, and synchronize access to config_enable flag. Since commit dbf2576e37da0fcc7aacbfbb9fd5d3de7888a3c1 workqueue: make all workqueues non-reentrant all workqueues are non-reentrant, and config_enable is now gone. Get rid of the unnecessary lock. Signed-off-by: Michael S. Tsirkin m...@redhat.com --- drivers/net/virtio_net.c | 7 +-- 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c index fa17afa..d80fef4 100644 --- a/drivers/net/virtio_net.c +++ b/drivers/net/virtio_net.c [...] @@ -1430,7 +1426,7 @@ static void virtnet_config_changed_work(struct work_struct *work) netif_tx_stop_all_queues(vi-dev); } done: -mutex_unlock(vi-config_lock); +return; There's no need for this *return*. I know - it's removed by the follow-up patch. It's formatted like this to make diff smaller and make review easier. } static void virtnet_config_changed(struct virtio_device *vdev) WBR, Sergei ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 09/16] virtio-net: drop config_mutex
On Sun, 5 Oct 2014 19:07:16 +0300 Michael S. Tsirkin m...@redhat.com wrote: config_mutex served two purposes: prevent multiple concurrent config change handlers, and synchronize access to config_enable flag. Since commit dbf2576e37da0fcc7aacbfbb9fd5d3de7888a3c1 workqueue: make all workqueues non-reentrant all workqueues are non-reentrant, and config_enable is now gone. Get rid of the unnecessary lock. Signed-off-by: Michael S. Tsirkin m...@redhat.com --- drivers/net/virtio_net.c | 7 +-- 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c index fa17afa..d80fef4 100644 --- a/drivers/net/virtio_net.c +++ b/drivers/net/virtio_net.c @@ -1430,7 +1426,7 @@ static void virtnet_config_changed_work(struct work_struct *work) netif_tx_stop_all_queues(vi-dev); } done: - mutex_unlock(vi-config_lock); + return; } static void virtnet_config_changed(struct virtio_device *vdev) I'd probably return directly in the remaining 'goto done;' cases, but still Reviewed-by: Cornelia Huck cornelia.h...@de.ibm.com ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 05/16] virtio: defer config changed notifications
On Mon, Oct 06, 2014 at 01:33:38PM +0200, Cornelia Huck wrote:
On Sun, 5 Oct 2014 19:07:05 +0300
Michael S. Tsirkin m...@redhat.com wrote:
Defer config changed notifications that arrive during
probe/scan/freeze/restore.
This will allow drivers to set DRIVER_OK earlier, without worrying about
racing with config change interrupts.
This change will also benefit old hypervisors (before 2009)
that send interrupts without checking DRIVER_OK: previously,
the callback could race with driver-specific initialization.
This will also help simplify drivers.
Signed-off-by: Michael S. Tsirkin m...@redhat.com
---
include/linux/virtio.h | 6 ++
drivers/virtio/virtio.c | 55
+
2 files changed, 52 insertions(+), 9 deletions(-)
diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
index 657f817..578e02d 100644
--- a/drivers/virtio/virtio.c
+++ b/drivers/virtio/virtio.c
@@ -117,6 +117,40 @@ void virtio_check_driver_offered_feature(const struct
virtio_device *vdev,
}
EXPORT_SYMBOL_GPL(virtio_check_driver_offered_feature);
+static void __virtio_config_changed(struct virtio_device *dev)
+{
+ struct virtio_driver *drv = drv_to_virtio(dev-dev.driver);
+
+ if (!dev-config_enabled)
+ dev-config_changed = true;
+ else if (drv drv-config_changed)
+ drv-config_changed(dev);
+}
+
+void virtio_config_changed(struct virtio_device *dev)
+{
+ spin_lock_irq(dev-config_lock);
+ __virtio_config_changed(dev);
+ spin_unlock_irq(dev-config_lock);
Hm, isn't this function called from the interrupt handler?
Good catch, will fix up, thanks!
+}
+EXPORT_SYMBOL_GPL(virtio_config_changed);
+
+static void virtio_config_disable(struct virtio_device *dev)
+{
+ spin_lock_irq(dev-config_lock);
+ dev-config_enabled = false;
+ spin_unlock_irq(dev-config_lock);
+}
+
+static void virtio_config_enable(struct virtio_device *dev)
+{
+ spin_lock_irq(dev-config_lock);
+ dev-config_enabled = true;
+ __virtio_config_changed(dev);
+ dev-config_changed = false;
+ spin_unlock_irq(dev-config_lock);
+}
+
Maybe call these virtio_config_change_{dis,en}able()?
static int virtio_dev_probe(struct device *_d)
{
int err, i;
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 09/16] virtio-net: drop config_mutex
On Mon, Oct 06, 2014 at 01:56:10PM +0200, Cornelia Huck wrote: On Sun, 5 Oct 2014 19:07:16 +0300 Michael S. Tsirkin m...@redhat.com wrote: config_mutex served two purposes: prevent multiple concurrent config change handlers, and synchronize access to config_enable flag. Since commit dbf2576e37da0fcc7aacbfbb9fd5d3de7888a3c1 workqueue: make all workqueues non-reentrant all workqueues are non-reentrant, and config_enable is now gone. Get rid of the unnecessary lock. Signed-off-by: Michael S. Tsirkin m...@redhat.com --- drivers/net/virtio_net.c | 7 +-- 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c index fa17afa..d80fef4 100644 --- a/drivers/net/virtio_net.c +++ b/drivers/net/virtio_net.c @@ -1430,7 +1426,7 @@ static void virtnet_config_changed_work(struct work_struct *work) netif_tx_stop_all_queues(vi-dev); } done: - mutex_unlock(vi-config_lock); + return; } static void virtnet_config_changed(struct virtio_device *vdev) I'd probably return directly in the remaining 'goto done;' cases, but still Reviewed-by: Cornelia Huck cornelia.h...@de.ibm.com Yes: this is exactly what [PATCH 11/16] virtio_net: minor cleanup does -- MST ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 08/16] virtio_net: drop config_enable
On Mon, Oct 06, 2014 at 01:50:36PM +0200, Cornelia Huck wrote: On Sun, 5 Oct 2014 19:07:13 +0300 Michael S. Tsirkin m...@redhat.com wrote: Now that virtio core ensures config changes don't arrive during probing, drop config_enable flag in virtio net. On removal, flush is now sufficient to guarantee that no change work is queued. This help simplify the driver, and will allow setting DRIVER_OK earlier without losing config change notifications. Signed-off-by: Michael S. Tsirkin m...@redhat.com --- drivers/net/virtio_net.c | 23 ++- 1 file changed, 2 insertions(+), 21 deletions(-) diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c index 59caa06..fa17afa 100644 --- a/drivers/net/virtio_net.c +++ b/drivers/net/virtio_net.c @@ -1876,16 +1869,12 @@ static void virtnet_remove(struct virtio_device *vdev) unregister_hotcpu_notifier(vi-nb); /* Prevent config work handler from accessing the device. */ Same comment as for the equivalent comment in the virtio-blk code. Same answer :) - mutex_lock(vi-config_lock); - vi-config_enable = false; - mutex_unlock(vi-config_lock); + flush_work(vi-config_work); unregister_netdev(vi-dev); remove_vq_common(vi); - flush_work(vi-config_work); - free_percpu(vi-stats); free_netdev(vi-dev); } @@ -1899,9 +1888,7 @@ static int virtnet_freeze(struct virtio_device *vdev) unregister_hotcpu_notifier(vi-nb); /* Prevent config work handler from accessing the device */ dito Same here. - mutex_lock(vi-config_lock); - vi-config_enable = false; - mutex_unlock(vi-config_lock); + flush_work(vi-config_work); netif_device_detach(vi-dev); cancel_delayed_work_sync(vi-refill); ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 11/16] virtio_net: minor cleanup
On Sun, 5 Oct 2014 19:07:21 +0300 Michael S. Tsirkin m...@redhat.com wrote: goto done; done: return; is ugly, it was put there to make diff review easier. replace by open-coded return. Signed-off-by: Michael S. Tsirkin m...@redhat.com --- drivers/net/virtio_net.c | 6 ++ 1 file changed, 2 insertions(+), 4 deletions(-) If you don't want to merge it into the mutex removal patch, maybe move it one up in the series? Acked-by: Cornelia Huck cornelia.h...@de.ibm.com ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 09/16] virtio-net: drop config_mutex
On 10/6/2014 3:56 PM, Michael S. Tsirkin wrote: config_mutex served two purposes: prevent multiple concurrent config change handlers, and synchronize access to config_enable flag. Since commit dbf2576e37da0fcc7aacbfbb9fd5d3de7888a3c1 workqueue: make all workqueues non-reentrant all workqueues are non-reentrant, and config_enable is now gone. Get rid of the unnecessary lock. Signed-off-by: Michael S. Tsirkin m...@redhat.com --- drivers/net/virtio_net.c | 7 +-- 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c index fa17afa..d80fef4 100644 --- a/drivers/net/virtio_net.c +++ b/drivers/net/virtio_net.c [...] @@ -1430,7 +1426,7 @@ static void virtnet_config_changed_work(struct work_struct *work) netif_tx_stop_all_queues(vi-dev); } done: - mutex_unlock(vi-config_lock); + return; There's no need for this *return*. I know - it's removed by the follow-up patch. Yeah, I saw. It's formatted like this to make diff smaller and make review easier. Don't understand how adding this line makes diff smaller though... You first need to add it and then to delete it, where's the save? WBR, Sergei ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 09/16] virtio-net: drop config_mutex
On Mon, Oct 06, 2014 at 04:07:32PM +0400, Sergei Shtylyov wrote: On 10/6/2014 3:56 PM, Michael S. Tsirkin wrote: config_mutex served two purposes: prevent multiple concurrent config change handlers, and synchronize access to config_enable flag. Since commit dbf2576e37da0fcc7aacbfbb9fd5d3de7888a3c1 workqueue: make all workqueues non-reentrant all workqueues are non-reentrant, and config_enable is now gone. Get rid of the unnecessary lock. Signed-off-by: Michael S. Tsirkin m...@redhat.com --- drivers/net/virtio_net.c | 7 +-- 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c index fa17afa..d80fef4 100644 --- a/drivers/net/virtio_net.c +++ b/drivers/net/virtio_net.c [...] @@ -1430,7 +1426,7 @@ static void virtnet_config_changed_work(struct work_struct *work) netif_tx_stop_all_queues(vi-dev); } done: - mutex_unlock(vi-config_lock); + return; There's no need for this *return*. I know - it's removed by the follow-up patch. Yeah, I saw. It's formatted like this to make diff smaller and make review easier. Don't understand how adding this line makes diff smaller though... You first need to add it and then to delete it, where's the save? WBR, Sergei If I don't add it, gcc generates a compiler warning: it does not like labels at the end of functions. So I don't want to just drop the return function: I must drop the label too. ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 06/16] virtio_blk: drop config_enable
On Mon, 6 Oct 2014 15:09:53 +0300 Michael S. Tsirkin m...@redhat.com wrote: On Mon, Oct 06, 2014 at 01:42:29PM +0200, Cornelia Huck wrote: On Sun, 5 Oct 2014 19:07:07 +0300 Michael S. Tsirkin m...@redhat.com wrote: Now that virtio core ensures config changes don't arrive during probing, drop config_enable flag in virtio blk. On removal, flush is now sufficient to guarantee that no change work is queued. This help simplify the driver, and will allow setting DRIVER_OK earlier without losing config change notifications. Signed-off-by: Michael S. Tsirkin m...@redhat.com --- drivers/block/virtio_blk.c | 19 ++- 1 file changed, 2 insertions(+), 17 deletions(-) diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c index 0a58140..c8cf6a1 100644 --- a/drivers/block/virtio_blk.c +++ b/drivers/block/virtio_blk.c @@ -772,9 +766,7 @@ static void virtblk_remove(struct virtio_device *vdev) int refc; /* Prevent config work handler from accessing the device. */ /* Common code ensures no further work will be queued. */ instead? No, I think you missed the point: this comment now refers to the flush below: flush is required to ensure work handler is not running. Agree? I think we both mean the same thing. Preventing the handler from access sounds to me more like when the handler starts running, it is prevented from accessing the device (like with setting config_enable, as the code did before). What I meant was common code has already ensured that our work-queueing function will not be called, therefore flushing the workqueue is enough. (same for net) ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 06/16] virtio_blk: drop config_enable
On Mon, Oct 06, 2014 at 02:20:38PM +0200, Cornelia Huck wrote: On Mon, 6 Oct 2014 15:09:53 +0300 Michael S. Tsirkin m...@redhat.com wrote: On Mon, Oct 06, 2014 at 01:42:29PM +0200, Cornelia Huck wrote: On Sun, 5 Oct 2014 19:07:07 +0300 Michael S. Tsirkin m...@redhat.com wrote: Now that virtio core ensures config changes don't arrive during probing, drop config_enable flag in virtio blk. On removal, flush is now sufficient to guarantee that no change work is queued. This help simplify the driver, and will allow setting DRIVER_OK earlier without losing config change notifications. Signed-off-by: Michael S. Tsirkin m...@redhat.com --- drivers/block/virtio_blk.c | 19 ++- 1 file changed, 2 insertions(+), 17 deletions(-) diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c index 0a58140..c8cf6a1 100644 --- a/drivers/block/virtio_blk.c +++ b/drivers/block/virtio_blk.c @@ -772,9 +766,7 @@ static void virtblk_remove(struct virtio_device *vdev) int refc; /* Prevent config work handler from accessing the device. */ /* Common code ensures no further work will be queued. */ instead? No, I think you missed the point: this comment now refers to the flush below: flush is required to ensure work handler is not running. Agree? I think we both mean the same thing. Preventing the handler from access sounds to me more like when the handler starts running, it is prevented from accessing the device (like with setting config_enable, as the code did before). What I meant was common code has already ensured that our work-queueing function will not be called, therefore flushing the workqueue is enough. (same for net) OK so I'll rewrite this to /* Make sure no work handler is accessing the device. */ ? I prefer not duplicating core guarantees in all devices. -- MST ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 09/16] virtio-net: drop config_mutex
On 10/6/2014 4:22 PM, Michael S. Tsirkin wrote: config_mutex served two purposes: prevent multiple concurrent config change handlers, and synchronize access to config_enable flag. Since commit dbf2576e37da0fcc7aacbfbb9fd5d3de7888a3c1 workqueue: make all workqueues non-reentrant all workqueues are non-reentrant, and config_enable is now gone. Get rid of the unnecessary lock. Signed-off-by: Michael S. Tsirkin m...@redhat.com --- drivers/net/virtio_net.c | 7 +-- 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c index fa17afa..d80fef4 100644 --- a/drivers/net/virtio_net.c +++ b/drivers/net/virtio_net.c [...] @@ -1430,7 +1426,7 @@ static void virtnet_config_changed_work(struct work_struct *work) netif_tx_stop_all_queues(vi-dev); } done: - mutex_unlock(vi-config_lock); + return; There's no need for this *return*. I know - it's removed by the follow-up patch. Yeah, I saw. It's formatted like this to make diff smaller and make review easier. Don't understand how adding this line makes diff smaller though... You first need to add it and then to delete it, where's the save? WBR, Sergei If I don't add it, gcc generates a compiler warning: it does not like labels at the end of functions. Ahh... nevermind then. WBR, Sergei ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 06/16] virtio_blk: drop config_enable
On Mon, 6 Oct 2014 15:31:10 +0300 Michael S. Tsirkin m...@redhat.com wrote: On Mon, Oct 06, 2014 at 02:20:38PM +0200, Cornelia Huck wrote: On Mon, 6 Oct 2014 15:09:53 +0300 Michael S. Tsirkin m...@redhat.com wrote: On Mon, Oct 06, 2014 at 01:42:29PM +0200, Cornelia Huck wrote: On Sun, 5 Oct 2014 19:07:07 +0300 Michael S. Tsirkin m...@redhat.com wrote: Now that virtio core ensures config changes don't arrive during probing, drop config_enable flag in virtio blk. On removal, flush is now sufficient to guarantee that no change work is queued. This help simplify the driver, and will allow setting DRIVER_OK earlier without losing config change notifications. Signed-off-by: Michael S. Tsirkin m...@redhat.com --- drivers/block/virtio_blk.c | 19 ++- 1 file changed, 2 insertions(+), 17 deletions(-) diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c index 0a58140..c8cf6a1 100644 --- a/drivers/block/virtio_blk.c +++ b/drivers/block/virtio_blk.c @@ -772,9 +766,7 @@ static void virtblk_remove(struct virtio_device *vdev) int refc; /* Prevent config work handler from accessing the device. */ /* Common code ensures no further work will be queued. */ instead? No, I think you missed the point: this comment now refers to the flush below: flush is required to ensure work handler is not running. Agree? I think we both mean the same thing. Preventing the handler from access sounds to me more like when the handler starts running, it is prevented from accessing the device (like with setting config_enable, as the code did before). What I meant was common code has already ensured that our work-queueing function will not be called, therefore flushing the workqueue is enough. (same for net) OK so I'll rewrite this to /* Make sure no work handler is accessing the device. */ ? I prefer not duplicating core guarantees in all devices. Sounds good to me. ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 10/16] virtio: add API to enable VQs early
On Sun, 5 Oct 2014 19:07:19 +0300
Michael S. Tsirkin m...@redhat.com wrote:
virtio spec requires DRIVER_OK to be set before
VQs are used, but some drivers use VQs before probe
function returns.
Since DRIVER_OK is set after probe, this violates the spec.
Even though transitional devices support this behaviour, we want to
make it possible for those early callers to become spec compliant.
?
Add API for drivers to call before using VQs.
Sets DRIVER_OK internally.
Signed-off-by: Michael S. Tsirkin m...@redhat.com
---
include/linux/virtio_config.h | 17 +
1 file changed, 17 insertions(+)
diff --git a/include/linux/virtio_config.h b/include/linux/virtio_config.h
index e8f8f71..6127fc8 100644
--- a/include/linux/virtio_config.h
+++ b/include/linux/virtio_config.h
@@ -109,6 +109,23 @@ struct virtqueue *virtio_find_single_vq(struct
virtio_device *vdev,
return vq;
}
+/**
+ * virtio_early_enable_vqs - enable vq use in probe function
+ * @vdev: the device
+ *
+ * Driver must call this to use vqs in the probe function.
+ *
+ * Note: vqs are enabled automatically after probe returns.
+ */
+static inline
+void virtio_early_enable_vqs(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);
+}
+
Maybe virtio_enable_vqs_early() instead?
static inline
const char *virtio_bus_name(struct virtio_device *vdev)
{
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 16/16] virtio_net: fix use after free on allocation failure
On Sun, 5 Oct 2014 19:07:38 +0300 Michael S. Tsirkin m...@redhat.com wrote: In the extremely unlikely event that driver initialization fails after RX buffers are added, virtio net frees RX buffers while VQs are still active, potentially causing device to use a freed buffer. To fix, reset device first - same as we do on device removal. Signed-off-by: Michael S. Tsirkin m...@redhat.com --- drivers/net/virtio_net.c | 2 ++ 1 file changed, 2 insertions(+) Reviewed-by: Cornelia Huck cornelia.h...@de.ibm.com ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 10/16] virtio: add API to enable VQs early
On Mon, Oct 06, 2014 at 04:08:16PM +0200, Cornelia Huck wrote:
On Sun, 5 Oct 2014 19:07:19 +0300
Michael S. Tsirkin m...@redhat.com wrote:
virtio spec requires DRIVER_OK to be set before
VQs are used, but some drivers use VQs before probe
function returns.
Since DRIVER_OK is set after probe, this violates the spec.
Even though transitional devices support this behaviour, we want to
make it possible for those early callers to become spec compliant.
?
Exactly.
Add API for drivers to call before using VQs.
Sets DRIVER_OK internally.
Signed-off-by: Michael S. Tsirkin m...@redhat.com
---
include/linux/virtio_config.h | 17 +
1 file changed, 17 insertions(+)
diff --git a/include/linux/virtio_config.h b/include/linux/virtio_config.h
index e8f8f71..6127fc8 100644
--- a/include/linux/virtio_config.h
+++ b/include/linux/virtio_config.h
@@ -109,6 +109,23 @@ struct virtqueue *virtio_find_single_vq(struct
virtio_device *vdev,
return vq;
}
+/**
+ * virtio_early_enable_vqs - enable vq use in probe function
+ * @vdev: the device
+ *
+ * Driver must call this to use vqs in the probe function.
+ *
+ * Note: vqs are enabled automatically after probe returns.
+ */
+static inline
+void virtio_early_enable_vqs(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);
+}
+
Maybe virtio_enable_vqs_early() instead?
OK.
static inline
const char *virtio_bus_name(struct virtio_device *vdev)
{
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH v2 00/15] virtio: fix spec compliance issues
Rusty, I have a mind to include this patchset for this merge window. Any input on this? This fixes the following virtio spec compliance issues: 1. on restore, drivers use device before setting ACKNOWLEDGE and DRIVER bits 2. on probe, drivers aren't prepared to handle config interrupts arriving before probe returns 3. on probe, drivers use device before DRIVER_OK it set Note that 1 is a clear violation of virtio spec 0.9 and 1.0, so I am proposing the fix for stable. OTOH 2 is against 1.0 rules but is a known documented bug in many drivers, so let's fix it going forward, but it does not seem to be worth it to backport the changes. An error handling bugfix for virtio-net is also included. 2 is merely a theoretical race condition, but it seems important to address to make sure that changes to address 3 do not introduce instability. Michael S. Tsirkin (15): virtio_pci: fix virtio spec compliance on restore virtio: unify config_changed handling virtio-pci: move freeze/restore to virtio core virtio: defer config changed notifications virtio_blk: drop config_enable virtio-blk: drop config_mutex virtio_net: drop config_enable virtio-net: drop config_mutex virtio_net: minor cleanup virtio: add API to enable VQs early virtio_net: enable VQs early virtio_blk: enable VQs early virtio_console: enable VQs early 9p/trans_virtio: enable VQs early virtio_net: fix use after free on allocation failure include/linux/virtio.h | 14 + include/linux/virtio_config.h | 17 +++ drivers/block/virtio_blk.c | 31 ++-- drivers/char/virtio_console.c | 2 + drivers/misc/mic/card/mic_virtio.c | 6 +-- drivers/net/virtio_net.c | 42 --- drivers/s390/kvm/kvm_virtio.c | 9 +--- drivers/s390/kvm/virtio_ccw.c | 6 +-- drivers/virtio/virtio.c| 101 + drivers/virtio/virtio_mmio.c | 7 +-- drivers/virtio/virtio_pci.c| 33 ++-- net/9p/trans_virtio.c | 2 + 12 files changed, 159 insertions(+), 111 deletions(-) -- MST ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH v2 01/15] virtio_pci: fix virtio spec compliance on restore
On restore, virtio pci does the following:
+ set features
+ init vqs etc - device can be used at this point!
+ set ACKNOWLEDGE,DRIVER and DRIVER_OK status bits
This is in violation of the virtio spec, which
requires the following order:
- ACKNOWLEDGE
- DRIVER
- init vqs
- DRIVER_OK
This behaviour will break with hypervisors that assume spec compliant
behaviour. It seems like a good idea to have this patch applied to
stable branches to reduce the support butden for the hypervisors.
Cc: sta...@vger.kernel.org
Cc: Amit Shah amit.s...@redhat.com
Signed-off-by: Michael S. Tsirkin m...@redhat.com
---
drivers/virtio/virtio_pci.c | 36
1 file changed, 32 insertions(+), 4 deletions(-)
diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c
index 3d1463c..0f2db51 100644
--- a/drivers/virtio/virtio_pci.c
+++ b/drivers/virtio/virtio_pci.c
@@ -789,6 +789,7 @@ static int virtio_pci_restore(struct device *dev)
struct pci_dev *pci_dev = to_pci_dev(dev);
struct virtio_pci_device *vp_dev = pci_get_drvdata(pci_dev);
struct virtio_driver *drv;
+ unsigned status = 0;
int ret;
drv = container_of(vp_dev-vdev.dev.driver,
@@ -799,14 +800,41 @@ static int virtio_pci_restore(struct device *dev)
return ret;
pci_set_master(pci_dev);
+ /* We always start by resetting the device, in case a previous
+* driver messed it up. */
+ vp_reset(vp_dev-vdev);
+
+ /* Acknowledge that we've seen the device. */
+ status |= VIRTIO_CONFIG_S_ACKNOWLEDGE;
+ vp_set_status(vp_dev-vdev, status);
+
+ /* Maybe driver failed before freeze.
+* Restore the failed status, for debugging. */
+ status |= vp_dev-saved_status VIRTIO_CONFIG_S_FAILED;
+ vp_set_status(vp_dev-vdev, status);
+
+ if (!drv)
+ return 0;
+
+ /* We have a driver! */
+ status |= VIRTIO_CONFIG_S_DRIVER;
+ vp_set_status(vp_dev-vdev, status);
+
vp_finalize_features(vp_dev-vdev);
- if (drv drv-restore)
- ret = drv-restore(vp_dev-vdev);
+ if (!drv-restore)
+ return 0;
+
+ ret = drv-restore(vp_dev-vdev);
+ if (ret) {
+ status |= VIRTIO_CONFIG_S_FAILED;
+ vp_set_status(vp_dev-vdev, status);
+ return ret;
+ }
/* Finally, tell the device we're all set */
- if (!ret)
- vp_set_status(vp_dev-vdev, vp_dev-saved_status);
+ status |= VIRTIO_CONFIG_S_DRIVER_OK;
+ vp_set_status(vp_dev-vdev, status);
return ret;
}
--
MST
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH v2 03/15] virtio-pci: move freeze/restore to virtio core
This is in preparation to extending config changed event handling
in core.
Wrapping these in an API also seems to make for a cleaner code.
Signed-off-by: Michael S. Tsirkin m...@redhat.com
---
include/linux/virtio.h | 6 +
drivers/virtio/virtio.c | 53 +++
drivers/virtio/virtio_pci.c | 55 ++---
3 files changed, 61 insertions(+), 53 deletions(-)
diff --git a/include/linux/virtio.h b/include/linux/virtio.h
index 3c19bd3..8df7ba8 100644
--- a/include/linux/virtio.h
+++ b/include/linux/virtio.h
@@ -78,6 +78,7 @@ bool virtqueue_is_broken(struct virtqueue *vq);
/**
* virtio_device - representation of a device using virtio
* @index: unique position on the virtio bus
+ * @failed: saved value for CONFIG_S_FAILED bit (for restore)
* @dev: underlying device.
* @id: the device type identification (used to match it with a driver).
* @config: the configuration ops for this device.
@@ -88,6 +89,7 @@ bool virtqueue_is_broken(struct virtqueue *vq);
*/
struct virtio_device {
int index;
+ bool failed;
struct device dev;
struct virtio_device_id id;
const struct virtio_config_ops *config;
@@ -109,6 +111,10 @@ void unregister_virtio_device(struct virtio_device *dev);
void virtio_break_device(struct virtio_device *dev);
void virtio_config_changed(struct virtio_device *dev);
+#ifdef CONFIG_PM_SLEEP
+int virtio_device_freeze(struct virtio_device *dev);
+int virtio_device_restore(struct virtio_device *dev);
+#endif
/**
* virtio_driver - operations for a virtio I/O driver
diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
index 3980687..657f817 100644
--- a/drivers/virtio/virtio.c
+++ b/drivers/virtio/virtio.c
@@ -248,6 +248,59 @@ void virtio_config_changed(struct virtio_device *dev)
}
EXPORT_SYMBOL_GPL(virtio_config_changed);
+#ifdef CONFIG_PM_SLEEP
+int virtio_device_freeze(struct virtio_device *dev)
+{
+ struct virtio_driver *drv = drv_to_virtio(dev-dev.driver);
+
+ dev-failed = dev-config-get_status(dev) VIRTIO_CONFIG_S_FAILED;
+
+ if (drv drv-freeze)
+ return drv-freeze(dev);
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(virtio_device_freeze);
+
+int virtio_device_restore(struct virtio_device *dev)
+{
+ struct virtio_driver *drv = drv_to_virtio(dev-dev.driver);
+
+ /* We always start by resetting the device, in case a previous
+* driver messed it up. */
+ dev-config-reset(dev);
+
+ /* Acknowledge that we've seen the device. */
+ add_status(dev, VIRTIO_CONFIG_S_ACKNOWLEDGE);
+
+ /* Maybe driver failed before freeze.
+* Restore the failed status, for debugging. */
+ if (dev-failed)
+ add_status(dev, VIRTIO_CONFIG_S_FAILED);
+
+ if (!drv)
+ return 0;
+
+ /* We have a driver! */
+ add_status(dev, VIRTIO_CONFIG_S_DRIVER);
+
+ dev-config-finalize_features(dev);
+
+ if (drv-restore) {
+ int ret = drv-restore(dev);
+ if (ret) {
+ add_status(dev, VIRTIO_CONFIG_S_FAILED);
+ return ret;
+ }
+ /* Finally, tell the device we're all set */
+ add_status(dev, VIRTIO_CONFIG_S_DRIVER_OK);
+ }
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(virtio_device_restore);
+#endif
+
static int virtio_init(void)
{
if (bus_register(virtio_bus) != 0)
diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c
index 58cbf6e..d34ebfa 100644
--- a/drivers/virtio/virtio_pci.c
+++ b/drivers/virtio/virtio_pci.c
@@ -57,9 +57,6 @@ struct virtio_pci_device
/* Vectors allocated, excluding per-vq vectors if any */
unsigned msix_used_vectors;
- /* Status saved during hibernate/restore */
- u8 saved_status;
-
/* Whether we have vector per vq */
bool per_vq_vectors;
};
@@ -764,16 +761,9 @@ static int virtio_pci_freeze(struct device *dev)
{
struct pci_dev *pci_dev = to_pci_dev(dev);
struct virtio_pci_device *vp_dev = pci_get_drvdata(pci_dev);
- struct virtio_driver *drv;
int ret;
- drv = container_of(vp_dev-vdev.dev.driver,
- struct virtio_driver, driver);
-
- ret = 0;
- vp_dev-saved_status = vp_get_status(vp_dev-vdev);
- if (drv drv-freeze)
- ret = drv-freeze(vp_dev-vdev);
+ ret = virtio_device_freeze(vp_dev-vdev);
if (!ret)
pci_disable_device(pci_dev);
@@ -784,55 +774,14 @@ static int virtio_pci_restore(struct device *dev)
{
struct pci_dev *pci_dev = to_pci_dev(dev);
struct virtio_pci_device *vp_dev = pci_get_drvdata(pci_dev);
- struct virtio_driver *drv;
- unsigned status = 0;
int ret;
- drv = container_of(vp_dev-vdev.dev.driver,
- struct virtio_driver, driver);
-
ret =
[PATCH v2 04/15] virtio: defer config changed notifications
Defer config changed notifications that arrive during
probe/scan/freeze/restore.
This will allow drivers to set DRIVER_OK earlier, without worrying about
racing with config change interrupts.
This change will also benefit old hypervisors (before 2009)
that send interrupts without checking DRIVER_OK: previously,
the callback could race with driver-specific initialization.
This will also help simplify drivers.
Signed-off-by: Michael S. Tsirkin m...@redhat.com
---
include/linux/virtio.h | 6 ++
drivers/virtio/virtio.c | 57 +
2 files changed, 54 insertions(+), 9 deletions(-)
diff --git a/include/linux/virtio.h b/include/linux/virtio.h
index 8df7ba8..5636b11 100644
--- a/include/linux/virtio.h
+++ b/include/linux/virtio.h
@@ -79,6 +79,9 @@ bool virtqueue_is_broken(struct virtqueue *vq);
* virtio_device - representation of a device using virtio
* @index: unique position on the virtio bus
* @failed: saved value for CONFIG_S_FAILED bit (for restore)
+ * @config_enabled: configuration change reporting enabled
+ * @config_changed: configuration change reported while disabled
+ * @config_lock: protects configuration change reporting
* @dev: underlying device.
* @id: the device type identification (used to match it with a driver).
* @config: the configuration ops for this device.
@@ -90,6 +93,9 @@ bool virtqueue_is_broken(struct virtqueue *vq);
struct virtio_device {
int index;
bool failed;
+ bool config_enabled;
+ bool config_changed;
+ spinlock_t config_lock;
struct device dev;
struct virtio_device_id id;
const struct virtio_config_ops *config;
diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
index 657f817..6214744 100644
--- a/drivers/virtio/virtio.c
+++ b/drivers/virtio/virtio.c
@@ -117,6 +117,42 @@ void virtio_check_driver_offered_feature(const struct
virtio_device *vdev,
}
EXPORT_SYMBOL_GPL(virtio_check_driver_offered_feature);
+static void __virtio_config_changed(struct virtio_device *dev)
+{
+ struct virtio_driver *drv = drv_to_virtio(dev-dev.driver);
+
+ if (!dev-config_enabled)
+ dev-config_changed = true;
+ else if (drv drv-config_changed)
+ drv-config_changed(dev);
+}
+
+void virtio_config_changed(struct virtio_device *dev)
+{
+ unsigned long flags;
+
+ spin_lock_irqsave(dev-config_lock, flags);
+ __virtio_config_changed(dev);
+ spin_unlock_irqrestore(dev-config_lock, flags);
+}
+EXPORT_SYMBOL_GPL(virtio_config_changed);
+
+static void virtio_config_disable(struct virtio_device *dev)
+{
+ spin_lock_irq(dev-config_lock);
+ dev-config_enabled = false;
+ spin_unlock_irq(dev-config_lock);
+}
+
+static void virtio_config_enable(struct virtio_device *dev)
+{
+ spin_lock_irq(dev-config_lock);
+ dev-config_enabled = true;
+ __virtio_config_changed(dev);
+ dev-config_changed = false;
+ spin_unlock_irq(dev-config_lock);
+}
+
static int virtio_dev_probe(struct device *_d)
{
int err, i;
@@ -153,6 +189,8 @@ static int virtio_dev_probe(struct device *_d)
add_status(dev, VIRTIO_CONFIG_S_DRIVER_OK);
if (drv-scan)
drv-scan(dev);
+
+ virtio_config_enable(dev);
}
return err;
@@ -163,6 +201,8 @@ static int virtio_dev_remove(struct device *_d)
struct virtio_device *dev = dev_to_virtio(_d);
struct virtio_driver *drv = drv_to_virtio(dev-dev.driver);
+ virtio_config_disable(dev);
+
drv-remove(dev);
/* Driver should have reset device. */
@@ -211,6 +251,10 @@ int register_virtio_device(struct virtio_device *dev)
dev-index = err;
dev_set_name(dev-dev, virtio%u, dev-index);
+ spin_lock_init(dev-config_lock);
+ dev-config_enabled = false;
+ dev-config_changed = false;
+
/* 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);
@@ -239,20 +283,13 @@ void unregister_virtio_device(struct virtio_device *dev)
}
EXPORT_SYMBOL_GPL(unregister_virtio_device);
-void virtio_config_changed(struct virtio_device *dev)
-{
- struct virtio_driver *drv = drv_to_virtio(dev-dev.driver);
-
- if (drv drv-config_changed)
- drv-config_changed(dev);
-}
-EXPORT_SYMBOL_GPL(virtio_config_changed);
-
#ifdef CONFIG_PM_SLEEP
int virtio_device_freeze(struct virtio_device *dev)
{
struct virtio_driver *drv = drv_to_virtio(dev-dev.driver);
+ virtio_config_disable(dev);
+
dev-failed = dev-config-get_status(dev) VIRTIO_CONFIG_S_FAILED;
if (drv drv-freeze)
@@ -296,6 +333,8 @@ int virtio_device_restore(struct virtio_device *dev)
add_status(dev, VIRTIO_CONFIG_S_DRIVER_OK);
}
+ virtio_config_enable(dev);
+
[PATCH v2 06/15] virtio-blk: drop config_mutex
config_mutex served two purposes: prevent multiple concurrent config
change handlers, and synchronize access to config_enable flag.
Since commit dbf2576e37da0fcc7aacbfbb9fd5d3de7888a3c1
workqueue: make all workqueues non-reentrant
all workqueues are non-reentrant, and config_enable
is now gone.
Get rid of the unnecessary lock.
Signed-off-by: Michael S. Tsirkin m...@redhat.com
Reviewed-by: Cornelia Huck cornelia.h...@de.ibm.com
---
drivers/block/virtio_blk.c | 8
1 file changed, 8 deletions(-)
diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
index 91272f1a..89ba8d6 100644
--- a/drivers/block/virtio_blk.c
+++ b/drivers/block/virtio_blk.c
@@ -41,9 +41,6 @@ struct virtio_blk
/* Process context for config space updates */
struct work_struct config_work;
- /* Lock for config space updates */
- struct mutex config_lock;
-
/* What host tells us, plus 2 for header tailer. */
unsigned int sg_elems;
@@ -344,8 +341,6 @@ static void virtblk_config_changed_work(struct work_struct
*work)
char *envp[] = { RESIZE=1, NULL };
u64 capacity, size;
- mutex_lock(vblk-config_lock);
-
/* Host must always specify the capacity. */
virtio_cread(vdev, struct virtio_blk_config, capacity, capacity);
@@ -369,8 +364,6 @@ static void virtblk_config_changed_work(struct work_struct
*work)
set_capacity(vblk-disk, capacity);
revalidate_disk(vblk-disk);
kobject_uevent_env(disk_to_dev(vblk-disk)-kobj, KOBJ_CHANGE, envp);
-
- mutex_unlock(vblk-config_lock);
}
static void virtblk_config_changed(struct virtio_device *vdev)
@@ -601,7 +594,6 @@ static int virtblk_probe(struct virtio_device *vdev)
vblk-vdev = vdev;
vblk-sg_elems = sg_elems;
- mutex_init(vblk-config_lock);
INIT_WORK(vblk-config_work, virtblk_config_changed_work);
--
MST
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH v2 05/15] virtio_blk: drop config_enable
Now that virtio core ensures config changes don't arrive during probing, drop config_enable flag in virtio blk. On removal, flush is now sufficient to guarantee that no change work is queued. This help simplify the driver, and will allow setting DRIVER_OK earlier without losing config change notifications. Signed-off-by: Michael S. Tsirkin m...@redhat.com --- drivers/block/virtio_blk.c | 23 --- 1 file changed, 4 insertions(+), 19 deletions(-) diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c index 0a58140..91272f1a 100644 --- a/drivers/block/virtio_blk.c +++ b/drivers/block/virtio_blk.c @@ -44,9 +44,6 @@ struct virtio_blk /* Lock for config space updates */ struct mutex config_lock; - /* enable config space updates */ - bool config_enable; - /* What host tells us, plus 2 for header tailer. */ unsigned int sg_elems; @@ -348,8 +345,6 @@ static void virtblk_config_changed_work(struct work_struct *work) u64 capacity, size; mutex_lock(vblk-config_lock); - if (!vblk-config_enable) - goto done; /* Host must always specify the capacity. */ virtio_cread(vdev, struct virtio_blk_config, capacity, capacity); @@ -374,7 +369,7 @@ static void virtblk_config_changed_work(struct work_struct *work) set_capacity(vblk-disk, capacity); revalidate_disk(vblk-disk); kobject_uevent_env(disk_to_dev(vblk-disk)-kobj, KOBJ_CHANGE, envp); -done: + mutex_unlock(vblk-config_lock); } @@ -609,7 +604,6 @@ static int virtblk_probe(struct virtio_device *vdev) mutex_init(vblk-config_lock); INIT_WORK(vblk-config_work, virtblk_config_changed_work); - vblk-config_enable = true; err = init_vq(vblk); if (err) @@ -771,10 +765,8 @@ static void virtblk_remove(struct virtio_device *vdev) int index = vblk-index; int refc; - /* Prevent config work handler from accessing the device. */ - mutex_lock(vblk-config_lock); - vblk-config_enable = false; - mutex_unlock(vblk-config_lock); + /* Make sure no work handler is accessing the device. */ + flush_work(vblk-config_work); del_gendisk(vblk-disk); blk_cleanup_queue(vblk-disk-queue); @@ -784,8 +776,6 @@ static void virtblk_remove(struct virtio_device *vdev) /* Stop all the virtqueues. */ vdev-config-reset(vdev); - flush_work(vblk-config_work); - refc = atomic_read(disk_to_dev(vblk-disk)-kobj.kref.refcount); put_disk(vblk-disk); vdev-config-del_vqs(vdev); @@ -805,11 +795,7 @@ static int virtblk_freeze(struct virtio_device *vdev) /* Ensure we don't receive any more interrupts */ vdev-config-reset(vdev); - /* Prevent config work handler from accessing the device. */ - mutex_lock(vblk-config_lock); - vblk-config_enable = false; - mutex_unlock(vblk-config_lock); - + /* Make sure no work handler is accessing the device. */ flush_work(vblk-config_work); blk_mq_stop_hw_queues(vblk-disk-queue); @@ -823,7 +809,6 @@ static int virtblk_restore(struct virtio_device *vdev) struct virtio_blk *vblk = vdev-priv; int ret; - vblk-config_enable = true; ret = init_vq(vdev-priv); if (!ret) blk_mq_start_stopped_hw_queues(vblk-disk-queue, true); -- MST ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH v2 07/15] virtio_net: drop config_enable
Now that virtio core ensures config changes don't arrive during probing,
drop config_enable flag in virtio net.
On removal, flush is now sufficient to guarantee that no change work is
queued.
This help simplify the driver, and will allow setting DRIVER_OK earlier
without losing config change notifications.
Signed-off-by: Michael S. Tsirkin m...@redhat.com
---
drivers/net/virtio_net.c | 27 ---
1 file changed, 4 insertions(+), 23 deletions(-)
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 59caa06..743fb04 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -123,9 +123,6 @@ struct virtnet_info {
/* Host can handle any s/g split between our header and packet data */
bool any_header_sg;
- /* enable config space updates */
- bool config_enable;
-
/* Active statistics */
struct virtnet_stats __percpu *stats;
@@ -1408,9 +1405,6 @@ static void virtnet_config_changed_work(struct
work_struct *work)
u16 v;
mutex_lock(vi-config_lock);
- if (!vi-config_enable)
- goto done;
-
if (virtio_cread_feature(vi-vdev, VIRTIO_NET_F_STATUS,
struct virtio_net_config, status, v) 0)
goto done;
@@ -1758,7 +1752,6 @@ static int virtnet_probe(struct virtio_device *vdev)
}
mutex_init(vi-config_lock);
- vi-config_enable = true;
INIT_WORK(vi-config_work, virtnet_config_changed_work);
/* If we can receive ANY GSO packets, we must allocate large ones. */
@@ -1875,17 +1868,13 @@ static void virtnet_remove(struct virtio_device *vdev)
unregister_hotcpu_notifier(vi-nb);
- /* Prevent config work handler from accessing the device. */
- mutex_lock(vi-config_lock);
- vi-config_enable = false;
- mutex_unlock(vi-config_lock);
+ /* Make sure no work handler is accessing the device. */
+ flush_work(vi-config_work);
unregister_netdev(vi-dev);
remove_vq_common(vi);
- flush_work(vi-config_work);
-
free_percpu(vi-stats);
free_netdev(vi-dev);
}
@@ -1898,10 +1887,8 @@ static int virtnet_freeze(struct virtio_device *vdev)
unregister_hotcpu_notifier(vi-nb);
- /* Prevent config work handler from accessing the device */
- mutex_lock(vi-config_lock);
- vi-config_enable = false;
- mutex_unlock(vi-config_lock);
+ /* Make sure no work handler is accessing the device */
+ flush_work(vi-config_work);
netif_device_detach(vi-dev);
cancel_delayed_work_sync(vi-refill);
@@ -1916,8 +1903,6 @@ static int virtnet_freeze(struct virtio_device *vdev)
remove_vq_common(vi);
- flush_work(vi-config_work);
-
return 0;
}
@@ -1941,10 +1926,6 @@ static int virtnet_restore(struct virtio_device *vdev)
netif_device_attach(vi-dev);
- mutex_lock(vi-config_lock);
- vi-config_enable = true;
- mutex_unlock(vi-config_lock);
-
rtnl_lock();
virtnet_set_queues(vi, vi-curr_queue_pairs);
rtnl_unlock();
--
MST
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH v2 02/15] virtio: unify config_changed handling
Replace duplicated code in all transports with a single wrapper in
virtio.c.
The only functional change is in virtio_mmio.c: if a buggy device sends
us an interrupt before driver is set, we previously returned IRQ_NONE,
now we return IRQ_HANDLED.
As this must not happen in practice, this does not look like a big deal.
See also commit 3fff0179e33cd7d0a688dab65700c46ad089e934
virtio-pci: do not oops on config change if driver not loaded.
for the original motivation behind the driver check.
Signed-off-by: Michael S. Tsirkin m...@redhat.com
---
include/linux/virtio.h | 2 ++
drivers/misc/mic/card/mic_virtio.c | 6 +-
drivers/s390/kvm/kvm_virtio.c | 9 +
drivers/s390/kvm/virtio_ccw.c | 6 +-
drivers/virtio/virtio.c| 9 +
drivers/virtio/virtio_mmio.c | 7 ++-
drivers/virtio/virtio_pci.c| 6 +-
7 files changed, 17 insertions(+), 28 deletions(-)
diff --git a/include/linux/virtio.h b/include/linux/virtio.h
index b46671e..3c19bd3 100644
--- a/include/linux/virtio.h
+++ b/include/linux/virtio.h
@@ -108,6 +108,8 @@ void unregister_virtio_device(struct virtio_device *dev);
void virtio_break_device(struct virtio_device *dev);
+void virtio_config_changed(struct virtio_device *dev);
+
/**
* virtio_driver - operations for a virtio I/O driver
* @driver: underlying device driver (populate name and owner).
diff --git a/drivers/misc/mic/card/mic_virtio.c
b/drivers/misc/mic/card/mic_virtio.c
index f14b600..e647947 100644
--- a/drivers/misc/mic/card/mic_virtio.c
+++ b/drivers/misc/mic/card/mic_virtio.c
@@ -462,16 +462,12 @@ static void mic_handle_config_change(struct
mic_device_desc __iomem *d,
struct mic_device_ctrl __iomem *dc
= (void __iomem *)d + mic_aligned_desc_size(d);
struct mic_vdev *mvdev = (struct mic_vdev *)ioread64(dc-vdev);
- struct virtio_driver *drv;
if (ioread8(dc-config_change) != MIC_VIRTIO_PARAM_CONFIG_CHANGED)
return;
dev_dbg(mdrv-dev, %s %d\n, __func__, __LINE__);
- drv = container_of(mvdev-vdev.dev.driver,
- struct virtio_driver, driver);
- if (drv-config_changed)
- drv-config_changed(mvdev-vdev);
+ virtio_config_changed(mvdev-vdev);
iowrite8(1, dc-guest_ack);
}
diff --git a/drivers/s390/kvm/kvm_virtio.c b/drivers/s390/kvm/kvm_virtio.c
index a134965..6431290 100644
--- a/drivers/s390/kvm/kvm_virtio.c
+++ b/drivers/s390/kvm/kvm_virtio.c
@@ -406,15 +406,8 @@ static void kvm_extint_handler(struct ext_code ext_code,
switch (param) {
case VIRTIO_PARAM_CONFIG_CHANGED:
- {
- struct virtio_driver *drv;
- drv = container_of(vq-vdev-dev.driver,
- struct virtio_driver, driver);
- if (drv-config_changed)
- drv-config_changed(vq-vdev);
-
+ virtio_config_changed(vq-vdev);
break;
- }
case VIRTIO_PARAM_DEV_ADD:
schedule_work(hotplug_work);
break;
diff --git a/drivers/s390/kvm/virtio_ccw.c b/drivers/s390/kvm/virtio_ccw.c
index d2c0b44..6cbe6ef 100644
--- a/drivers/s390/kvm/virtio_ccw.c
+++ b/drivers/s390/kvm/virtio_ccw.c
@@ -940,11 +940,7 @@ static void virtio_ccw_int_handler(struct ccw_device *cdev,
vring_interrupt(0, vq);
}
if (test_bit(0, vcdev-indicators2)) {
- drv = container_of(vcdev-vdev.dev.driver,
- struct virtio_driver, driver);
-
- if (drv drv-config_changed)
- drv-config_changed(vcdev-vdev);
+ virtio_config_changed(vcdev-vdev);
clear_bit(0, vcdev-indicators2);
}
}
diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
index fed0ce1..3980687 100644
--- a/drivers/virtio/virtio.c
+++ b/drivers/virtio/virtio.c
@@ -239,6 +239,15 @@ void unregister_virtio_device(struct virtio_device *dev)
}
EXPORT_SYMBOL_GPL(unregister_virtio_device);
+void virtio_config_changed(struct virtio_device *dev)
+{
+ struct virtio_driver *drv = drv_to_virtio(dev-dev.driver);
+
+ if (drv drv-config_changed)
+ drv-config_changed(dev);
+}
+EXPORT_SYMBOL_GPL(virtio_config_changed);
+
static int virtio_init(void)
{
if (bus_register(virtio_bus) != 0)
diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c
index c600ccf..ef9a165 100644
--- a/drivers/virtio/virtio_mmio.c
+++ b/drivers/virtio/virtio_mmio.c
@@ -234,8 +234,6 @@ static irqreturn_t vm_interrupt(int irq, void *opaque)
{
struct virtio_mmio_device *vm_dev = opaque;
struct virtio_mmio_vq_info *info;
- struct virtio_driver *vdrv = container_of(vm_dev-vdev.dev.driver,
- struct virtio_driver, driver);
unsigned long status;
unsigned long flags;
irqreturn_t ret =
[PATCH v2 09/15] virtio_net: minor cleanup
goto done;
done:
return;
is ugly, it was put there to make diff review easier.
replace by open-coded return.
Signed-off-by: Michael S. Tsirkin m...@redhat.com
Acked-by: Cornelia Huck cornelia.h...@de.ibm.com
---
drivers/net/virtio_net.c | 6 ++
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 23e4a69..ef04d23 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -1403,7 +1403,7 @@ static void virtnet_config_changed_work(struct
work_struct *work)
if (virtio_cread_feature(vi-vdev, VIRTIO_NET_F_STATUS,
struct virtio_net_config, status, v) 0)
- goto done;
+ return;
if (v VIRTIO_NET_S_ANNOUNCE) {
netdev_notify_peers(vi-dev);
@@ -1414,7 +1414,7 @@ static void virtnet_config_changed_work(struct
work_struct *work)
v = VIRTIO_NET_S_LINK_UP;
if (vi-status == v)
- goto done;
+ return;
vi-status = v;
@@ -1425,8 +1425,6 @@ static void virtnet_config_changed_work(struct
work_struct *work)
netif_carrier_off(vi-dev);
netif_tx_stop_all_queues(vi-dev);
}
-done:
- return;
}
static void virtnet_config_changed(struct virtio_device *vdev)
--
MST
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH v2 11/15] virtio_net: enable VQs early
virtio spec requires drivers to set DRIVER_OK before using VQs.
This is set automatically after probe returns, virtio net violated this
rule by using receive VQs within probe.
To fix, call virtio_enable_vqs_early before using VQs.
Signed-off-by: Michael S. Tsirkin m...@redhat.com
---
drivers/net/virtio_net.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index ef04d23..430f3ae 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -1792,6 +1792,8 @@ static int virtnet_probe(struct virtio_device *vdev)
goto free_vqs;
}
+ virtio_enable_vqs_early(vdev);
+
/* Last of all, set up some receive buffers. */
for (i = 0; i vi-curr_queue_pairs; i++) {
try_fill_recv(vi-rq[i], GFP_KERNEL);
--
MST
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH v2 12/15] virtio_blk: enable VQs early
virtio spec requires drivers to set DRIVER_OK before using VQs. This is set automatically after probe returns, virtio block violated this rule by calling add_disk, which causes the VQ to be used directly within probe. To fix, call virtio_enable_vqs_early before using VQs. Signed-off-by: Michael S. Tsirkin m...@redhat.com --- drivers/block/virtio_blk.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c index 89ba8d6..46b04bf 100644 --- a/drivers/block/virtio_blk.c +++ b/drivers/block/virtio_blk.c @@ -719,6 +719,8 @@ static int virtblk_probe(struct virtio_device *vdev) if (!err opt_io_size) blk_queue_io_opt(q, blk_size * opt_io_size); + virtio_enable_vqs_early(vdev); + add_disk(vblk-disk); err = device_create_file(disk_to_dev(vblk-disk), dev_attr_serial); if (err) -- MST ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH v2 10/15] virtio: add API to enable VQs early
virtio spec 0.9.X requires DRIVER_OK to be set before
VQs are used, but some drivers use VQs before probe
function returns.
Since DRIVER_OK is set after probe, this violates the spec.
Even though under virtio 1.0 transitional devices support this
behaviour, we want to make it possible for those early callers to become
spec compliant and eventually support non-transitional devices.
Add API for drivers to call before using VQs.
Sets DRIVER_OK internally.
Signed-off-by: Michael S. Tsirkin m...@redhat.com
---
include/linux/virtio_config.h | 17 +
1 file changed, 17 insertions(+)
diff --git a/include/linux/virtio_config.h b/include/linux/virtio_config.h
index e8f8f71..e36403b 100644
--- a/include/linux/virtio_config.h
+++ b/include/linux/virtio_config.h
@@ -109,6 +109,23 @@ struct virtqueue *virtio_find_single_vq(struct
virtio_device *vdev,
return vq;
}
+/**
+ * virtio_enable_vqs_early - enable vq use in probe function
+ * @vdev: the device
+ *
+ * Driver must call this to use vqs in the probe function.
+ *
+ * Note: vqs are enabled automatically after probe returns.
+ */
+static inline
+void virtio_enable_vqs_early(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);
+}
+
static inline
const char *virtio_bus_name(struct virtio_device *vdev)
{
--
MST
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH v2 15/15] virtio_net: fix use after free on allocation failure
In the extremely unlikely event that driver initialization fails after RX buffers are added, virtio net frees RX buffers while VQs are still active, potentially causing device to use a freed buffer. To fix, reset device first - same as we do on device removal. Signed-off-by: Michael S. Tsirkin m...@redhat.com Reviewed-by: Cornelia Huck cornelia.h...@de.ibm.com --- drivers/net/virtio_net.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c index 430f3ae..3551417 100644 --- a/drivers/net/virtio_net.c +++ b/drivers/net/virtio_net.c @@ -1830,6 +1830,8 @@ static int virtnet_probe(struct virtio_device *vdev) return 0; free_recv_bufs: + vi-vdev-config-reset(vdev); + free_receive_bufs(vi); unregister_netdev(dev); free_vqs: -- MST ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH v2 13/15] virtio_console: enable VQs early
virtio spec requires drivers to set DRIVER_OK before using VQs.
This is set automatically after probe returns, virtio console violated this
rule by adding inbufs, which causes the VQ to be used directly within
probe.
To fix, call virtio_enable_vqs_early before using VQs.
Signed-off-by: Michael S. Tsirkin m...@redhat.com
---
drivers/char/virtio_console.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index b585b47..6ebe8f6 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -1449,6 +1449,8 @@ static int add_port(struct ports_device *portdev, u32 id)
spin_lock_init(port-outvq_lock);
init_waitqueue_head(port-waitqueue);
+ virtio_enable_vqs_early(portdev-vdev);
+
/* Fill the in_vq with buffers so the host can send us data. */
nr_added_bufs = fill_queue(port-in_vq, port-inbuf_lock);
if (!nr_added_bufs) {
--
MST
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH v2 08/15] virtio-net: drop config_mutex
config_mutex served two purposes: prevent multiple concurrent config
change handlers, and synchronize access to config_enable flag.
Since commit dbf2576e37da0fcc7aacbfbb9fd5d3de7888a3c1
workqueue: make all workqueues non-reentrant
all workqueues are non-reentrant, and config_enable
is now gone.
Get rid of the unnecessary lock.
Signed-off-by: Michael S. Tsirkin m...@redhat.com
Reviewed-by: Cornelia Huck cornelia.h...@de.ibm.com
---
drivers/net/virtio_net.c | 7 +--
1 file changed, 1 insertion(+), 6 deletions(-)
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 743fb04..23e4a69 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -132,9 +132,6 @@ struct virtnet_info {
/* Work struct for config space updates */
struct work_struct config_work;
- /* Lock for config space updates */
- struct mutex config_lock;
-
/* Does the affinity hint is set for virtqueues? */
bool affinity_hint_set;
@@ -1404,7 +1401,6 @@ static void virtnet_config_changed_work(struct
work_struct *work)
container_of(work, struct virtnet_info, config_work);
u16 v;
- mutex_lock(vi-config_lock);
if (virtio_cread_feature(vi-vdev, VIRTIO_NET_F_STATUS,
struct virtio_net_config, status, v) 0)
goto done;
@@ -1430,7 +1426,7 @@ static void virtnet_config_changed_work(struct
work_struct *work)
netif_tx_stop_all_queues(vi-dev);
}
done:
- mutex_unlock(vi-config_lock);
+ return;
}
static void virtnet_config_changed(struct virtio_device *vdev)
@@ -1751,7 +1747,6 @@ static int virtnet_probe(struct virtio_device *vdev)
u64_stats_init(virtnet_stats-rx_syncp);
}
- mutex_init(vi-config_lock);
INIT_WORK(vi-config_work, virtnet_config_changed_work);
/* If we can receive ANY GSO packets, we must allocate large ones. */
--
MST
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2 02/15] virtio: unify config_changed handling
On Mon, 6 Oct 2014 18:10:44 +0300 Michael S. Tsirkin m...@redhat.com wrote: Replace duplicated code in all transports with a single wrapper in virtio.c. The only functional change is in virtio_mmio.c: if a buggy device sends us an interrupt before driver is set, we previously returned IRQ_NONE, now we return IRQ_HANDLED. As this must not happen in practice, this does not look like a big deal. See also commit 3fff0179e33cd7d0a688dab65700c46ad089e934 virtio-pci: do not oops on config change if driver not loaded. for the original motivation behind the driver check. Signed-off-by: Michael S. Tsirkin m...@redhat.com --- include/linux/virtio.h | 2 ++ drivers/misc/mic/card/mic_virtio.c | 6 +- drivers/s390/kvm/kvm_virtio.c | 9 + drivers/s390/kvm/virtio_ccw.c | 6 +- drivers/virtio/virtio.c| 9 + drivers/virtio/virtio_mmio.c | 7 ++- drivers/virtio/virtio_pci.c| 6 +- 7 files changed, 17 insertions(+), 28 deletions(-) Reviewed-by: Cornelia Huck cornelia.h...@de.ibm.com ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2 03/15] virtio-pci: move freeze/restore to virtio core
On Mon, 6 Oct 2014 18:10:51 +0300 Michael S. Tsirkin m...@redhat.com wrote: This is in preparation to extending config changed event handling in core. Wrapping these in an API also seems to make for a cleaner code. Signed-off-by: Michael S. Tsirkin m...@redhat.com --- include/linux/virtio.h | 6 + drivers/virtio/virtio.c | 53 +++ drivers/virtio/virtio_pci.c | 55 ++--- 3 files changed, 61 insertions(+), 53 deletions(-) diff --git a/include/linux/virtio.h b/include/linux/virtio.h index 3c19bd3..8df7ba8 100644 --- a/include/linux/virtio.h +++ b/include/linux/virtio.h @@ -78,6 +78,7 @@ bool virtqueue_is_broken(struct virtqueue *vq); /** * virtio_device - representation of a device using virtio * @index: unique position on the virtio bus + * @failed: saved value for CONFIG_S_FAILED bit (for restore) Have you considered s/failed/saved_failed/ ? * @dev: underlying device. * @id: the device type identification (used to match it with a driver). * @config: the configuration ops for this device. Otherwise, my Reviewed-by: Cornelia Huck cornelia.h...@de.ibm.com still stands. ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH v2 14/15] 9p/trans_virtio: enable VQs early
virtio spec requires drivers to set DRIVER_OK before using VQs. This is set automatically after probe returns, but virtio 9p device adds self to channel list within probe, at which point VQ can be used in violation of the spec. To fix, call virtio_enable_vqs_early before using VQs. Signed-off-by: Michael S. Tsirkin m...@redhat.com --- net/9p/trans_virtio.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/net/9p/trans_virtio.c b/net/9p/trans_virtio.c index 6940d8f..766ba48 100644 --- a/net/9p/trans_virtio.c +++ b/net/9p/trans_virtio.c @@ -575,6 +575,8 @@ static int p9_virtio_probe(struct virtio_device *vdev) /* Ceiling limit to avoid denial of service attacks */ chan-p9_max_pages = nr_free_buffer_pages()/4; + virtio_enable_vqs_early(vdev); + mutex_lock(virtio_9p_lock); list_add_tail(chan-chan_list, virtio_chan_list); mutex_unlock(virtio_9p_lock); -- MST ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2 03/15] virtio-pci: move freeze/restore to virtio core
On Mon, Oct 06, 2014 at 05:20:55PM +0200, Cornelia Huck wrote: On Mon, 6 Oct 2014 18:10:51 +0300 Michael S. Tsirkin m...@redhat.com wrote: This is in preparation to extending config changed event handling in core. Wrapping these in an API also seems to make for a cleaner code. Signed-off-by: Michael S. Tsirkin m...@redhat.com --- include/linux/virtio.h | 6 + drivers/virtio/virtio.c | 53 +++ drivers/virtio/virtio_pci.c | 55 ++--- 3 files changed, 61 insertions(+), 53 deletions(-) diff --git a/include/linux/virtio.h b/include/linux/virtio.h index 3c19bd3..8df7ba8 100644 --- a/include/linux/virtio.h +++ b/include/linux/virtio.h @@ -78,6 +78,7 @@ bool virtqueue_is_broken(struct virtqueue *vq); /** * virtio_device - representation of a device using virtio * @index: unique position on the virtio bus + * @failed: saved value for CONFIG_S_FAILED bit (for restore) Have you considered s/failed/saved_failed/ ? I kind of prefer the shorter name, your reviewed tag made me think you don't cosider this too important? * @dev: underlying device. * @id: the device type identification (used to match it with a driver). * @config: the configuration ops for this device. Otherwise, my Reviewed-by: Cornelia Huck cornelia.h...@de.ibm.com still stands. ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2 04/15] virtio: defer config changed notifications
On Mon, 6 Oct 2014 18:10:55 +0300 Michael S. Tsirkin m...@redhat.com wrote: Defer config changed notifications that arrive during probe/scan/freeze/restore. This will allow drivers to set DRIVER_OK earlier, without worrying about racing with config change interrupts. This change will also benefit old hypervisors (before 2009) that send interrupts without checking DRIVER_OK: previously, the callback could race with driver-specific initialization. This will also help simplify drivers. Signed-off-by: Michael S. Tsirkin m...@redhat.com --- include/linux/virtio.h | 6 ++ drivers/virtio/virtio.c | 57 + 2 files changed, 54 insertions(+), 9 deletions(-) Reviewed-by: Cornelia Huck cornelia.h...@de.ibm.com ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2 05/15] virtio_blk: drop config_enable
On Mon, 6 Oct 2014 18:10:58 +0300 Michael S. Tsirkin m...@redhat.com wrote: Now that virtio core ensures config changes don't arrive during probing, drop config_enable flag in virtio blk. On removal, flush is now sufficient to guarantee that no change work is queued. This help simplify the driver, and will allow setting DRIVER_OK earlier without losing config change notifications. Signed-off-by: Michael S. Tsirkin m...@redhat.com --- drivers/block/virtio_blk.c | 23 --- 1 file changed, 4 insertions(+), 19 deletions(-) Reviewed-by: Cornelia Huck cornelia.h...@de.ibm.com ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2 07/15] virtio_net: drop config_enable
On Mon, 6 Oct 2014 18:11:05 +0300 Michael S. Tsirkin m...@redhat.com wrote: Now that virtio core ensures config changes don't arrive during probing, drop config_enable flag in virtio net. On removal, flush is now sufficient to guarantee that no change work is queued. This help simplify the driver, and will allow setting DRIVER_OK earlier without losing config change notifications. Signed-off-by: Michael S. Tsirkin m...@redhat.com --- drivers/net/virtio_net.c | 27 --- 1 file changed, 4 insertions(+), 23 deletions(-) Reviewed-by: Cornelia Huck cornelia.h...@de.ibm.com ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2 10/15] virtio: add API to enable VQs early
On Mon, 6 Oct 2014 18:11:16 +0300 Michael S. Tsirkin m...@redhat.com wrote: virtio spec 0.9.X requires DRIVER_OK to be set before VQs are used, but some drivers use VQs before probe function returns. Since DRIVER_OK is set after probe, this violates the spec. Even though under virtio 1.0 transitional devices support this behaviour, we want to make it possible for those early callers to become spec compliant and eventually support non-transitional devices. Add API for drivers to call before using VQs. Sets DRIVER_OK internally. Signed-off-by: Michael S. Tsirkin m...@redhat.com --- include/linux/virtio_config.h | 17 + 1 file changed, 17 insertions(+) Reviewed-by: Cornelia Huck cornelia.h...@de.ibm.com ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2 03/15] virtio-pci: move freeze/restore to virtio core
On Mon, 6 Oct 2014 18:26:55 +0300 Michael S. Tsirkin m...@redhat.com wrote: On Mon, Oct 06, 2014 at 05:20:55PM +0200, Cornelia Huck wrote: On Mon, 6 Oct 2014 18:10:51 +0300 Michael S. Tsirkin m...@redhat.com wrote: This is in preparation to extending config changed event handling in core. Wrapping these in an API also seems to make for a cleaner code. Signed-off-by: Michael S. Tsirkin m...@redhat.com --- include/linux/virtio.h | 6 + drivers/virtio/virtio.c | 53 +++ drivers/virtio/virtio_pci.c | 55 ++--- 3 files changed, 61 insertions(+), 53 deletions(-) diff --git a/include/linux/virtio.h b/include/linux/virtio.h index 3c19bd3..8df7ba8 100644 --- a/include/linux/virtio.h +++ b/include/linux/virtio.h @@ -78,6 +78,7 @@ bool virtqueue_is_broken(struct virtqueue *vq); /** * virtio_device - representation of a device using virtio * @index: unique position on the virtio bus + * @failed: saved value for CONFIG_S_FAILED bit (for restore) Have you considered s/failed/saved_failed/ ? I kind of prefer the shorter name, your reviewed tag made me think you don't cosider this too important? Indeed; if you prefer the short name, just keep it. * @dev: underlying device. * @id: the device type identification (used to match it with a driver). * @config: the configuration ops for this device. Otherwise, my Reviewed-by: Cornelia Huck cornelia.h...@de.ibm.com still stands. ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2 11/15] virtio_net: enable VQs early
On Mon, 6 Oct 2014 18:11:19 +0300 Michael S. Tsirkin m...@redhat.com wrote: virtio spec requires drivers to set DRIVER_OK before using VQs. This is set automatically after probe returns, virtio net violated this rule by using receive VQs within probe. To fix, call virtio_enable_vqs_early before using VQs. Signed-off-by: Michael S. Tsirkin m...@redhat.com --- drivers/net/virtio_net.c | 2 ++ 1 file changed, 2 insertions(+) Reviewed-by: Cornelia Huck cornelia.h...@de.ibm.com ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2 12/15] virtio_blk: enable VQs early
On Mon, 6 Oct 2014 18:11:22 +0300 Michael S. Tsirkin m...@redhat.com wrote: virtio spec requires drivers to set DRIVER_OK before using VQs. This is set automatically after probe returns, virtio block violated this rule by calling add_disk, which causes the VQ to be used directly within probe. To fix, call virtio_enable_vqs_early before using VQs. Signed-off-by: Michael S. Tsirkin m...@redhat.com --- drivers/block/virtio_blk.c | 2 ++ 1 file changed, 2 insertions(+) Reviewed-by: Cornelia Huck cornelia.h...@de.ibm.com ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 08/16] virtio_net: drop config_enable
From: Michael S. Tsirkin m...@redhat.com Date: Sun, 5 Oct 2014 19:07:13 +0300 Now that virtio core ensures config changes don't arrive during probing, drop config_enable flag in virtio net. On removal, flush is now sufficient to guarantee that no change work is queued. This help simplify the driver, and will allow setting DRIVER_OK earlier without losing config change notifications. Signed-off-by: Michael S. Tsirkin m...@redhat.com It's hard for people on the networking side to review these changes since you haven't CC:'d them on any of the postings necessary to understand the context of the net/ and drivers/net/ changes. Please at a minimum CC: everyone on your header [PATCH 0/N] posting so we know at least at a high level what is going on, and why. Thanks. ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
