From d817cd525589765aa5f6798734e422c867685a58 Mon Sep 17 00:00:00 2001 From: Jeff Mahoney Date: Fri, 15 Jan 2010 17:01:26 -0800 Subject: virtio: fix section mismatch warnings Fix fixes the following warnings by renaming the driver structures to be suffixed with _driver. WARNING: drivers/virtio/virtio_balloon.o(.data+0x88): Section mismatch in reference from the variable virtio_balloon to the function .devexit.text:virtballoon_remove() WARNING: drivers/char/hw_random/virtio-rng.o(.data+0x88): Section mismatch in reference from the variable virtio_rng to the function .devexit.text:virtrng_remove() Signed-off-by: Jeff Mahoney Acked-by: Rusty Russell Signed-off-by: Andrew Morton Signed-off-by: Linus Torvalds --- drivers/virtio/virtio_balloon.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) (limited to 'drivers/virtio') diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c index 9dd588042880..505be88c82ae 100644 --- a/drivers/virtio/virtio_balloon.c +++ b/drivers/virtio/virtio_balloon.c @@ -266,7 +266,7 @@ static void __devexit virtballoon_remove(struct virtio_device *vdev) static unsigned int features[] = { VIRTIO_BALLOON_F_MUST_TELL_HOST }; -static struct virtio_driver virtio_balloon = { +static struct virtio_driver virtio_balloon_driver = { .feature_table = features, .feature_table_size = ARRAY_SIZE(features), .driver.name = KBUILD_MODNAME, @@ -279,12 +279,12 @@ static struct virtio_driver virtio_balloon = { static int __init init(void) { - return register_virtio_driver(&virtio_balloon); + return register_virtio_driver(&virtio_balloon_driver); } static void __exit fini(void) { - unregister_virtio_driver(&virtio_balloon); + unregister_virtio_driver(&virtio_balloon_driver); } module_init(init); module_exit(fini); -- cgit v1.2.3 From 1f08b833ddbdb1c8e81c4b1053c2ebb7b89cb437 Mon Sep 17 00:00:00 2001 From: Jamie Lokier Date: Fri, 8 Jan 2010 22:01:43 +0000 Subject: Add __devexit_p around reference to virtio_pci_remove This is needed to compile with CONFIG_VIRTIO_PCI=y, because virtio_pci_remove is marked __devexit. Signed-off-by: Jamie Lokier Signed-off-by: Rusty Russell --- drivers/virtio/virtio_pci.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'drivers/virtio') diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c index 28d9cf7cf72f..1d5191fab62e 100644 --- a/drivers/virtio/virtio_pci.c +++ b/drivers/virtio/virtio_pci.c @@ -702,7 +702,7 @@ static struct pci_driver virtio_pci_driver = { .name = "virtio-pci", .id_table = virtio_pci_id_table, .probe = virtio_pci_probe, - .remove = virtio_pci_remove, + .remove = __devexit_p(virtio_pci_remove), #ifdef CONFIG_PM .suspend = virtio_pci_suspend, .resume = virtio_pci_resume, -- cgit v1.2.3 From 9564e138b1f6eb137f7149772438d3f3fb3277dd Mon Sep 17 00:00:00 2001 From: Adam Litke Date: Mon, 30 Nov 2009 10:14:15 -0600 Subject: virtio: Add memory statistics reporting to the balloon driver (V4) Changes since V3: - Do not do endian conversions as they will be done in the host - Report stats that reference a quantity of memory in bytes - Minor coding style updates Changes since V2: - Increase stat field size to 64 bits - Report all sizes in kb (not pages) - Drop anon_pages stat and fix endianness conversion Changes since V1: - Use a virtqueue instead of the device config space When using ballooning to manage overcommitted memory on a host, a system for guests to communicate their memory usage to the host can provide information that will minimize the impact of ballooning on the guests. The current method employs a daemon running in each guest that communicates memory statistics to a host daemon at a specified time interval. The host daemon aggregates this information and inflates and/or deflates balloons according to the level of host memory pressure. This approach is effective but overly complex since a daemon must be installed inside each guest and coordinated to communicate with the host. A simpler approach is to collect memory statistics in the virtio balloon driver and communicate them directly to the hypervisor. This patch enables the guest-side support by adding stats collection and reporting to the virtio balloon driver. Signed-off-by: Adam Litke Cc: Anthony Liguori Cc: virtualization@lists.linux-foundation.org Signed-off-by: Rusty Russell (minor fixes) --- drivers/virtio/virtio_balloon.c | 94 +++++++++++++++++++++++++++++++++++++---- 1 file changed, 86 insertions(+), 8 deletions(-) (limited to 'drivers/virtio') diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c index 505be88c82ae..cd778b1752b5 100644 --- a/drivers/virtio/virtio_balloon.c +++ b/drivers/virtio/virtio_balloon.c @@ -28,7 +28,7 @@ struct virtio_balloon { struct virtio_device *vdev; - struct virtqueue *inflate_vq, *deflate_vq; + struct virtqueue *inflate_vq, *deflate_vq, *stats_vq; /* Where the ballooning thread waits for config to change. */ wait_queue_head_t config_change; @@ -49,6 +49,9 @@ struct virtio_balloon /* The array of pfns we tell the Host about. */ unsigned int num_pfns; u32 pfns[256]; + + /* Memory statistics */ + struct virtio_balloon_stat stats[VIRTIO_BALLOON_S_NR]; }; static struct virtio_device_id id_table[] = { @@ -154,6 +157,62 @@ static void leak_balloon(struct virtio_balloon *vb, size_t num) } } +static inline void update_stat(struct virtio_balloon *vb, int idx, + u16 tag, u64 val) +{ + BUG_ON(idx >= VIRTIO_BALLOON_S_NR); + vb->stats[idx].tag = tag; + vb->stats[idx].val = val; +} + +#define pages_to_bytes(x) ((u64)(x) << PAGE_SHIFT) + +static void update_balloon_stats(struct virtio_balloon *vb) +{ + unsigned long events[NR_VM_EVENT_ITEMS]; + struct sysinfo i; + int idx = 0; + + all_vm_events(events); + si_meminfo(&i); + + update_stat(vb, idx++, VIRTIO_BALLOON_S_SWAP_IN, + pages_to_bytes(events[PSWPIN])); + update_stat(vb, idx++, VIRTIO_BALLOON_S_SWAP_OUT, + pages_to_bytes(events[PSWPOUT])); + update_stat(vb, idx++, VIRTIO_BALLOON_S_MAJFLT, events[PGMAJFAULT]); + update_stat(vb, idx++, VIRTIO_BALLOON_S_MINFLT, events[PGFAULT]); + update_stat(vb, idx++, VIRTIO_BALLOON_S_MEMFREE, + pages_to_bytes(i.freeram)); + update_stat(vb, idx++, VIRTIO_BALLOON_S_MEMTOT, + pages_to_bytes(i.totalram)); +} + +/* + * While most virtqueues communicate guest-initiated requests to the hypervisor, + * the stats queue operates in reverse. The driver initializes the virtqueue + * with a single buffer. From that point forward, all conversations consist of + * a hypervisor request (a call to this function) which directs us to refill + * the virtqueue with a fresh stats buffer. + */ +static void stats_ack(struct virtqueue *vq) +{ + struct virtio_balloon *vb; + unsigned int len; + struct scatterlist sg; + + vb = vq->vq_ops->get_buf(vq, &len); + if (!vb) + return; + + update_balloon_stats(vb); + + sg_init_one(&sg, vb->stats, sizeof(vb->stats)); + if (vq->vq_ops->add_buf(vq, &sg, 1, 0, vb) < 0) + BUG(); + vq->vq_ops->kick(vq); +} + static void virtballoon_changed(struct virtio_device *vdev) { struct virtio_balloon *vb = vdev->priv; @@ -204,10 +263,10 @@ static int balloon(void *_vballoon) static int virtballoon_probe(struct virtio_device *vdev) { struct virtio_balloon *vb; - struct virtqueue *vqs[2]; - vq_callback_t *callbacks[] = { balloon_ack, balloon_ack }; - const char *names[] = { "inflate", "deflate" }; - int err; + struct virtqueue *vqs[3]; + vq_callback_t *callbacks[] = { balloon_ack, balloon_ack, stats_ack }; + const char *names[] = { "inflate", "deflate", "stats" }; + int err, nvqs; vdev->priv = vb = kmalloc(sizeof(*vb), GFP_KERNEL); if (!vb) { @@ -220,13 +279,29 @@ static int virtballoon_probe(struct virtio_device *vdev) init_waitqueue_head(&vb->config_change); vb->vdev = vdev; - /* We expect two virtqueues. */ - err = vdev->config->find_vqs(vdev, 2, vqs, callbacks, names); + /* We expect two virtqueues: inflate and deflate, + * and optionally stat. */ + nvqs = virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_STATS_VQ) ? 3 : 2; + err = vdev->config->find_vqs(vdev, nvqs, vqs, callbacks, names); if (err) goto out_free_vb; vb->inflate_vq = vqs[0]; vb->deflate_vq = vqs[1]; + if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_STATS_VQ)) { + struct scatterlist sg; + vb->stats_vq = vqs[2]; + + /* + * Prime this virtqueue with one buffer so the hypervisor can + * use it to signal us later. + */ + sg_init_one(&sg, vb->stats, sizeof vb->stats); + if (vb->stats_vq->vq_ops->add_buf(vb->stats_vq, + &sg, 1, 0, vb) < 0) + BUG(); + vb->stats_vq->vq_ops->kick(vb->stats_vq); + } vb->thread = kthread_run(balloon, vb, "vballoon"); if (IS_ERR(vb->thread)) { @@ -264,7 +339,10 @@ static void __devexit virtballoon_remove(struct virtio_device *vdev) kfree(vb); } -static unsigned int features[] = { VIRTIO_BALLOON_F_MUST_TELL_HOST }; +static unsigned int features[] = { + VIRTIO_BALLOON_F_MUST_TELL_HOST, + VIRTIO_BALLOON_F_STATS_VQ, +}; static struct virtio_driver virtio_balloon_driver = { .feature_table = features, -- cgit v1.2.3 From 1f34c71afe5115e77a49c4e67720a66e27053e54 Mon Sep 17 00:00:00 2001 From: Adam Litke Date: Thu, 10 Dec 2009 16:35:15 -0600 Subject: virtio: Fix scheduling while atomic in virtio_balloon stats This is a fix for my earlier patch: "virtio: Add memory statistics reporting to the balloon driver (V4)". I discovered that all_vm_events() can sleep and therefore stats collection cannot be done in interrupt context. One solution is to handle the interrupt by noting that stats need to be collected and waking the existing vballoon kthread which will complete the work via stats_handle_request(). Rusty, is this a saner way of doing business? There is one issue that I would like a broader opinion on. In stats_request, I update vb->need_stats_update and then wake up the kthread. The kthread uses vb->need_stats_update as a condition variable. Do I need a memory barrier between the update and wake_up to ensure that my kthread sees the correct value? My testing suggests that it is not needed but I would like some confirmation from the experts. Signed-off-by: Adam Litke To: Rusty Russell Cc: Anthony Liguori Cc: linux-kernel@vger.kernel.org Signed-off-by: Rusty Russell --- drivers/virtio/virtio_balloon.c | 22 ++++++++++++++++++---- 1 file changed, 18 insertions(+), 4 deletions(-) (limited to 'drivers/virtio') diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c index cd778b1752b5..3db3d242c3ee 100644 --- a/drivers/virtio/virtio_balloon.c +++ b/drivers/virtio/virtio_balloon.c @@ -51,6 +51,7 @@ struct virtio_balloon u32 pfns[256]; /* Memory statistics */ + int need_stats_update; struct virtio_balloon_stat stats[VIRTIO_BALLOON_S_NR]; }; @@ -193,20 +194,30 @@ static void update_balloon_stats(struct virtio_balloon *vb) * the stats queue operates in reverse. The driver initializes the virtqueue * with a single buffer. From that point forward, all conversations consist of * a hypervisor request (a call to this function) which directs us to refill - * the virtqueue with a fresh stats buffer. + * the virtqueue with a fresh stats buffer. Since stats collection can sleep, + * we notify our kthread which does the actual work via stats_handle_request(). */ -static void stats_ack(struct virtqueue *vq) +static void stats_request(struct virtqueue *vq) { struct virtio_balloon *vb; unsigned int len; - struct scatterlist sg; vb = vq->vq_ops->get_buf(vq, &len); if (!vb) return; + vb->need_stats_update = 1; + wake_up(&vb->config_change); +} + +static void stats_handle_request(struct virtio_balloon *vb) +{ + struct virtqueue *vq; + struct scatterlist sg; + vb->need_stats_update = 0; update_balloon_stats(vb); + vq = vb->stats_vq; sg_init_one(&sg, vb->stats, sizeof(vb->stats)); if (vq->vq_ops->add_buf(vq, &sg, 1, 0, vb) < 0) BUG(); @@ -249,8 +260,11 @@ static int balloon(void *_vballoon) try_to_freeze(); wait_event_interruptible(vb->config_change, (diff = towards_target(vb)) != 0 + || vb->need_stats_update || kthread_should_stop() || freezing(current)); + if (vb->need_stats_update) + stats_handle_request(vb); if (diff > 0) fill_balloon(vb, diff); else if (diff < 0) @@ -264,7 +278,7 @@ static int virtballoon_probe(struct virtio_device *vdev) { struct virtio_balloon *vb; struct virtqueue *vqs[3]; - vq_callback_t *callbacks[] = { balloon_ack, balloon_ack, stats_ack }; + vq_callback_t *callbacks[] = { balloon_ack, balloon_ack, stats_request }; const char *names[] = { "inflate", "deflate", "stats" }; int err, nvqs; -- cgit v1.2.3 From 169c246a30808588436794e96a97c61a01af9bed Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Wed, 24 Feb 2010 14:22:14 -0600 Subject: virtio: fix balloon without VIRTIO_BALLOON_F_STATS_VQ When running under qemu-kvm-0.11.0: BUG: unable to handle kernel paging request at 56e58955 ... Process vballoon (pid: 1297, ti=c7976000 task=c70a6ca0 task.ti=c7 ... Call Trace: [] ? balloon+0x1b3/0x440 [virtio_balloon] [] ? schedule+0x327/0x9d0 [] ? balloon+0x0/0x440 [virtio_balloon] [] ? kthread+0x74/0x80 [] ? kthread+0x0/0x80 [] ? kernel_thread_helper+0x6/0x30 need_stats_update should be zero-initialized. Signed-off-by: Rusty Russell Acked-by: Adam Litke --- drivers/virtio/virtio_balloon.c | 1 + 1 file changed, 1 insertion(+) (limited to 'drivers/virtio') diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c index 3db3d242c3ee..369f2eebbad1 100644 --- a/drivers/virtio/virtio_balloon.c +++ b/drivers/virtio/virtio_balloon.c @@ -292,6 +292,7 @@ static int virtballoon_probe(struct virtio_device *vdev) vb->num_pages = 0; init_waitqueue_head(&vb->config_change); vb->vdev = vdev; + vb->need_stats_update = 0; /* We expect two virtqueues: inflate and deflate, * and optionally stat. */ -- cgit v1.2.3 From 97a545ab6ce922a0f868d192718a48a0091ebc5e Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Wed, 24 Feb 2010 14:22:22 -0600 Subject: virtio: remove bogus barriers from DEBUG version of virtio_ring.c With DEBUG defined, we add an ->in_use flag to detect if the caller invokes two virtio methods in parallel. The barriers attempt to ensure timely update of the ->in_use flag. But they're voodoo: if we need these barriers it implies that the calling code doesn't have sufficient synchronization to ensure the code paths aren't invoked at the same time anyway, and we want to detect it. Also, adding barriers changes timing, so turning on debug has more chance of hiding real problems. Thanks to MST for drawing my attention to this code... CC: Michael S. Tsirkin Signed-off-by: Rusty Russell --- drivers/virtio/virtio_ring.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) (limited to 'drivers/virtio') diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c index fbd2ecde93e4..1ee97d402a48 100644 --- a/drivers/virtio/virtio_ring.c +++ b/drivers/virtio/virtio_ring.c @@ -36,10 +36,9 @@ panic("%s:in_use = %i\n", \ (_vq)->vq.name, (_vq)->in_use); \ (_vq)->in_use = __LINE__; \ - mb(); \ } while (0) #define END_USE(_vq) \ - do { BUG_ON(!(_vq)->in_use); (_vq)->in_use = 0; mb(); } while(0) + do { BUG_ON(!(_vq)->in_use); (_vq)->in_use = 0; } while(0) #else #define BAD_RING(_vq, fmt, args...) \ do { \ -- cgit v1.2.3 From d57ed95da483418e8b0433da693c9168dd0a2df6 Mon Sep 17 00:00:00 2001 From: "Michael S. Tsirkin" Date: Thu, 28 Jan 2010 00:42:23 +0200 Subject: virtio: use smp_XX barriers on SMP virtio is communicating with a virtual "device" that actually runs on another host processor. Thus SMP barriers can be used to control memory access ordering. Where possible, we should use SMP barriers which are more lightweight than mandatory barriers, because mandatory barriers also control MMIO effects on accesses through relaxed memory I/O windows (which virtio does not use) (compare specifically smp_rmb and rmb on x86_64). We can't just use smp_mb and friends though, because we must force memory ordering even if guest is UP since host could be running on another CPU, but SMP barriers are defined to barrier() in that configuration. So, for UP fall back to mandatory barriers instead. Signed-off-by: Michael S. Tsirkin Signed-off-by: Rusty Russell --- drivers/virtio/virtio_ring.c | 26 ++++++++++++++++++++++---- 1 file changed, 22 insertions(+), 4 deletions(-) (limited to 'drivers/virtio') diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c index 1ee97d402a48..827f7e042610 100644 --- a/drivers/virtio/virtio_ring.c +++ b/drivers/virtio/virtio_ring.c @@ -21,6 +21,24 @@ #include #include +/* virtio guest is communicating with a virtual "device" that actually runs on + * a host processor. Memory barriers are used to control SMP effects. */ +#ifdef CONFIG_SMP +/* Where possible, use SMP barriers which are more lightweight than mandatory + * barriers, because mandatory barriers control MMIO effects on accesses + * through relaxed memory I/O windows (which virtio does not use). */ +#define virtio_mb() smp_mb() +#define virtio_rmb() smp_rmb() +#define virtio_wmb() smp_wmb() +#else +/* We must force memory ordering even if guest is UP since host could be + * running on another CPU, but SMP barriers are defined to barrier() in that + * configuration. So fall back to mandatory barriers instead. */ +#define virtio_mb() mb() +#define virtio_rmb() rmb() +#define virtio_wmb() wmb() +#endif + #ifdef DEBUG /* For development, we want to crash whenever the ring is screwed. */ #define BAD_RING(_vq, fmt, args...) \ @@ -220,13 +238,13 @@ static void vring_kick(struct virtqueue *_vq) START_USE(vq); /* Descriptors and available array need to be set before we expose the * new available array entries. */ - wmb(); + virtio_wmb(); vq->vring.avail->idx += vq->num_added; vq->num_added = 0; /* Need to update avail index before checking if we should notify */ - mb(); + virtio_mb(); if (!(vq->vring.used->flags & VRING_USED_F_NO_NOTIFY)) /* Prod other side to tell it about changes. */ @@ -285,7 +303,7 @@ static void *vring_get_buf(struct virtqueue *_vq, unsigned int *len) } /* Only get used array entries after they have been exposed by host. */ - rmb(); + virtio_rmb(); i = vq->vring.used->ring[vq->last_used_idx%vq->vring.num].id; *len = vq->vring.used->ring[vq->last_used_idx%vq->vring.num].len; @@ -323,7 +341,7 @@ static bool vring_enable_cb(struct virtqueue *_vq) /* We optimistically turn back on interrupts, then check if there was * more to do. */ vq->vring.avail->flags &= ~VRING_AVAIL_F_NO_INTERRUPT; - mb(); + virtio_mb(); if (unlikely(more_used(vq))) { END_USE(vq); return false; -- cgit v1.2.3 From c021eac4148c16bf53baa0dd14e8ebee6f39dab5 Mon Sep 17 00:00:00 2001 From: Shirley Ma Date: Mon, 18 Jan 2010 19:15:23 +0530 Subject: virtio: Add ability to detach unused buffers from vrings There's currently no way for a virtio driver to ask for unused buffers, so it has to keep a list itself to reclaim them at shutdown. This is redundant, since virtio_ring stores that information. So add a new hook to do this. Signed-off-by: Shirley Ma Signed-off-by: Amit Shah Signed-off-by: Rusty Russell --- drivers/virtio/virtio_ring.c | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) (limited to 'drivers/virtio') diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c index 827f7e042610..782b7292a3d8 100644 --- a/drivers/virtio/virtio_ring.c +++ b/drivers/virtio/virtio_ring.c @@ -351,6 +351,30 @@ static bool vring_enable_cb(struct virtqueue *_vq) return true; } +static void *vring_detach_unused_buf(struct virtqueue *_vq) +{ + struct vring_virtqueue *vq = to_vvq(_vq); + unsigned int i; + void *buf; + + START_USE(vq); + + for (i = 0; i < vq->vring.num; i++) { + if (!vq->data[i]) + continue; + /* detach_buf clears data, so grab it now. */ + buf = vq->data[i]; + detach_buf(vq, i); + END_USE(vq); + return buf; + } + /* That should have freed everything. */ + BUG_ON(vq->num_free != vq->vring.num); + + END_USE(vq); + return NULL; +} + irqreturn_t vring_interrupt(int irq, void *_vq) { struct vring_virtqueue *vq = to_vvq(_vq); @@ -377,6 +401,7 @@ static struct virtqueue_ops vring_vq_ops = { .kick = vring_kick, .disable_cb = vring_disable_cb, .enable_cb = vring_enable_cb, + .detach_unused_buf = vring_detach_unused_buf, }; struct virtqueue *vring_new_virtqueue(unsigned int num, -- cgit v1.2.3 From 3b8706240ee6084ccb46e53cd3a554356b7eeec8 Mon Sep 17 00:00:00 2001 From: Amit Shah Date: Fri, 12 Feb 2010 10:32:14 +0530 Subject: virtio: Initialize vq->data entries to NULL vq operations depend on vq->data[i] being NULL to figure out if the vq entry is in use (since the previous patch). We have to initialize them to NULL to ensure we don't work with junk data and trigger false BUG_ONs. Signed-off-by: Amit Shah Signed-off-by: Rusty Russell Cc: Shirley Ma --- drivers/virtio/virtio_ring.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) (limited to 'drivers/virtio') diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c index 782b7292a3d8..0db906b3c95d 100644 --- a/drivers/virtio/virtio_ring.c +++ b/drivers/virtio/virtio_ring.c @@ -448,8 +448,11 @@ struct virtqueue *vring_new_virtqueue(unsigned int num, /* Put everything in free lists. */ vq->num_free = num; vq->free_head = 0; - for (i = 0; i < num-1; i++) + for (i = 0; i < num-1; i++) { vq->vring.desc[i].next = i+1; + vq->data[i] = NULL; + } + vq->data[i] = NULL; return &vq->vq; } -- cgit v1.2.3 From 3119815912a220bdac943dfbdfee640414c0c611 Mon Sep 17 00:00:00 2001 From: "Michael S. Tsirkin" Date: Thu, 25 Feb 2010 19:08:55 +0200 Subject: virtio: fix out of range array access I have observed the following error on virtio-net module unload: ------------[ cut here ]------------ WARNING: at kernel/irq/manage.c:858 __free_irq+0xa0/0x14c() Hardware name: Bochs Trying to free already-free IRQ 0 Modules linked in: virtio_net(-) virtio_blk virtio_pci virtio_ring virtio af_packet e1000 shpchp aacraid uhci_hcd ohci_hcd ehci_hcd [last unloaded: scsi_wait_scan] Pid: 1957, comm: rmmod Not tainted 2.6.33-rc8-vhost #24 Call Trace: [] warn_slowpath_common+0x7c/0x94 [] warn_slowpath_fmt+0x41/0x43 [] ? __free_pages+0x5a/0x70 [] __free_irq+0xa0/0x14c [] free_irq+0x3f/0x65 [] vp_del_vqs+0x81/0xb1 [virtio_pci] [] virtnet_remove+0xda/0x10b [virtio_net] [] virtio_dev_remove+0x22/0x4a [virtio] [] __device_release_driver+0x66/0xac [] driver_detach+0x83/0xa9 [] bus_remove_driver+0x91/0xb4 [] driver_unregister+0x6c/0x74 [] unregister_virtio_driver+0xe/0x10 [virtio] [] fini+0x15/0x17 [virtio_net] [] sys_delete_module+0x1c3/0x230 [] ? old_ich_force_enable_hpet+0x117/0x164 [] ? do_page_fault+0x29c/0x2cc [] sysenter_dispatch+0x7/0x27 ---[ end trace 15e88e4c576cc62b ]--- The bug is in virtio-pci: we use msix_vector as array index to get irq entry, but some vqs do not have a dedicated vector so this causes an out of bounds access. By chance, we seem to often get 0 value, which results in this error. Fix by verifying that vector is legal before using it as index. Signed-off-by: Michael S. Tsirkin Acked-by: Anthony Liguori Acked-by: Shirley Ma Acked-by: Amit Shah --- drivers/virtio/virtio_pci.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) (limited to 'drivers/virtio') diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c index 1d5191fab62e..1b6573216998 100644 --- a/drivers/virtio/virtio_pci.c +++ b/drivers/virtio/virtio_pci.c @@ -473,7 +473,8 @@ static void vp_del_vqs(struct virtio_device *vdev) list_for_each_entry_safe(vq, n, &vdev->vqs, list) { info = vq->priv; - if (vp_dev->per_vq_vectors) + if (vp_dev->per_vq_vectors && + info->msix_vector != VIRTIO_MSI_NO_VECTOR) free_irq(vp_dev->msix_entries[info->msix_vector].vector, vq); vp_del_vq(vq); -- cgit v1.2.3 From bc505f373979692d51a86d40925f77a8b09d17b9 Mon Sep 17 00:00:00 2001 From: "Michael S. Tsirkin" Date: Sun, 29 Nov 2009 17:52:00 +0200 Subject: virtio: set pci bus master enable bit As all virtio devices perform DMA, we must enable bus mastering for them to be spec compliant. This patch fixes hotplug of virtio devices with Linux guests and qemu 0.11-0.12. Tested-by: Alexander Graf Signed-off-by: Michael S. Tsirkin --- drivers/virtio/virtio_pci.c | 1 + 1 file changed, 1 insertion(+) (limited to 'drivers/virtio') diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c index 1b6573216998..625447f645d9 100644 --- a/drivers/virtio/virtio_pci.c +++ b/drivers/virtio/virtio_pci.c @@ -649,6 +649,7 @@ static int __devinit virtio_pci_probe(struct pci_dev *pci_dev, goto out_req_regions; pci_set_drvdata(pci_dev, vp_dev); + pci_set_master(pci_dev); /* we use the subsystem vendor/device id as the virtio vendor/device * id. this allows us to use the same PCI vendor/device id for all -- cgit v1.2.3