Re: [PATCH] virtio: decrement dev_index when device is unregistered
- Original Message -
On Tue, Apr 5, 2011 at 5:49 AM, Takuma Umeya tum...@redhat.com
wrote:
When virtio device is removed, dev_index does not get decremented.
The next device hotplug event results in consuming the next pci to
the one that is suppose to be available.
Signed-off-by: Takuma Umeya tum...@redhat.com
diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
index efb35aa..67fe71d 100644
--- a/drivers/virtio/virtio.c
+++ b/drivers/virtio/virtio.c
@@ -216,6 +216,7 @@ EXPORT_SYMBOL_GPL(register_virtio_device);
void unregister_virtio_device(struct virtio_device *dev)
{
device_unregister(dev-dev);
+ dev_index--;
I don't think there is any guarantee that virtio devices are
added/removed in first-in-last-out order.
That means I could add a virtio-net device (index 0) followed by a
virtio-blk device (index 1). Now I remove the virtio-net device
(index 0) which causes me to decrement dev_index and hand index 1 out
again to the next device. This leaves us with virtio-blk (index 1)
and the new device with index 1, which is not unique.
Perhaps I missed a constraint which prevents this from occurring?
I believe the address is assigned up to 1f so using u32 value
to track use/free. This should make the code immune to the scenario.
Would this be adequate?
diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
index efb35aa..0c73507 100644
--- a/drivers/virtio/virtio.c
+++ b/drivers/virtio/virtio.c
@@ -3,7 +3,7 @@
#include linux/virtio_config.h
/* Unique numbering for virtio devices. */
-static unsigned int dev_index;
+static u32 dev_index;
static ssize_t device_show(struct device *_d,
struct device_attribute *attr, char *buf)
@@ -187,12 +187,23 @@ EXPORT_SYMBOL_GPL(unregister_virtio_driver);
int register_virtio_device(struct virtio_device *dev)
{
- int err;
+ int err, count;
+ u32 testbit;
+
+ count = 0;
dev-dev.bus = virtio_bus;
/* Assign a unique device index and hence name. */
- dev-index = dev_index++;
+ while (count 32){
+testbit = 1UL count;
+if(!(dev_index testbit)){
+dev-index = count;
+dev_index |= testbit;
+break;
+}
+count++;
+}
dev_set_name(dev-dev, virtio%u, dev-index);
/* We always start by resetting the device, in case a previous
@@ -215,7 +226,13 @@ EXPORT_SYMBOL_GPL(register_virtio_device);
void unregister_virtio_device(struct virtio_device *dev)
{
+ u32 removebit;
+
+removebit = 1UL (dev-index);
+
device_unregister(dev-dev);
+
+ dev_index ^= removebit;
}
EXPORT_SYMBOL_GPL(unregister_virtio_device);
Stefan
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization
Re: [RFC][PATCH] virtio balloon: kill tell-host-first logic
On Thu, Apr 07, 2011 at 10:43:25AM -0700, Dave Hansen wrote:
The virtio balloon driver has a VIRTIO_BALLOON_F_MUST_TELL_HOST
feature bit. Whenever the bit is set, we must always tell the
host before we free pages back to the allocator. Without this
we might free a page (and have another user touch it) while the
hypervisor is unprepared for it.
But, if the bit is _not_ set, we are under no obligation to
reverse the order. Furthermore, all modern qemus set this bit.
Which qemus do this, specifically? Amit Shah just pointed out to me
that upstream qemu.git and qemu-kvm.git don't seem to do this.
Which qemu did you test this with?
So, the tell second code is completely unused and untestable.
Quoting Anthony: untested code is broken code.
This _also_ means that we don't have to preserve a pfn list
after the pages are freed, which should let us get rid of some
temporary storage (vb-pfns) eventually.
Signed-off-by: Dave Hansen d...@linux.vnet.ibm.com
---
linux-2.6.git-dave/drivers/virtio/virtio_balloon.c | 21
-
1 file changed, 8 insertions(+), 13 deletions(-)
diff -puN drivers/virtio/virtio_balloon.c~kill-tell-host-first-logic
drivers/virtio/virtio_balloon.c
--- linux-2.6.git/drivers/virtio/virtio_balloon.c~kill-tell-host-first-logic
2011-04-07 10:23:12.016343374 -0700
+++ linux-2.6.git-dave/drivers/virtio/virtio_balloon.c2011-04-07
10:23:12.024343370 -0700
@@ -40,9 +40,6 @@ struct virtio_balloon
/* Waiting for host to ack the pages we released. */
struct completion acked;
- /* Do we have to tell Host *before* we reuse pages? */
- bool tell_host_first;
-
/* The pages we've told the Host we're not using. */
unsigned int num_pages;
struct list_head pages;
@@ -151,13 +148,14 @@ static void leak_balloon(struct virtio_b
vb-num_pages--;
}
- if (vb-tell_host_first) {
- tell_host(vb, vb-deflate_vq);
- release_pages_by_pfn(vb-pfns, vb-num_pfns);
- } else {
- release_pages_by_pfn(vb-pfns, vb-num_pfns);
- tell_host(vb, vb-deflate_vq);
- }
+
+ /*
+ * Note that if
+ * virtio_has_feature(vdev, VIRTIO_BALLOON_F_MUST_TELL_HOST);
+ * is true, we *have* to do it in this order
+ */
+ tell_host(vb, vb-deflate_vq);
+ release_pages_by_pfn(vb-pfns, vb-num_pfns);
}
static inline void update_stat(struct virtio_balloon *vb, int idx,
@@ -325,9 +323,6 @@ static int virtballoon_probe(struct virt
goto out_del_vqs;
}
- vb-tell_host_first
- = virtio_has_feature(vdev, VIRTIO_BALLOON_F_MUST_TELL_HOST);
-
return 0;
out_del_vqs:
_
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization
[RFC][PATCH] virtio: 64 bit features
Extend features to 64 bit so we can use more
transport bits.
Future patches add two new feature bits which would
exhaust the supply of transport feature bits,
so let's add bit 31 to tell the guest that
there are now 64 worth of features.
For PCI this also changes the config layout.
Signed-off-by: Michael S. Tsirkin m...@redhat.com
---
drivers/lguest/lguest_device.c |8
drivers/s390/kvm/kvm_virtio.c |6 +++---
drivers/virtio/virtio.c|8
drivers/virtio/virtio_pci.c| 33 +++--
include/linux/virtio.h |2 +-
include/linux/virtio_config.h | 15 +--
include/linux/virtio_pci.h |9 -
7 files changed, 56 insertions(+), 25 deletions(-)
diff --git a/drivers/lguest/lguest_device.c b/drivers/lguest/lguest_device.c
index 69c84a1..d2d6953 100644
--- a/drivers/lguest/lguest_device.c
+++ b/drivers/lguest/lguest_device.c
@@ -93,17 +93,17 @@ static unsigned desc_size(const struct lguest_device_desc
*desc)
}
/* This gets the device's feature bits. */
-static u32 lg_get_features(struct virtio_device *vdev)
+static u64 lg_get_features(struct virtio_device *vdev)
{
unsigned int i;
- u32 features = 0;
+ u64 features = 0;
struct lguest_device_desc *desc = to_lgdev(vdev)-desc;
u8 *in_features = lg_features(desc);
/* We do this the slow but generic way. */
- for (i = 0; i min(desc-feature_len * 8, 32); i++)
+ for (i = 0; i min(desc-feature_len * 8, 64); i++)
if (in_features[i / 8] (1 (i % 8)))
- features |= (1 i);
+ features |= (1ull i);
return features;
}
diff --git a/drivers/s390/kvm/kvm_virtio.c b/drivers/s390/kvm/kvm_virtio.c
index 414427d..8c976d0 100644
--- a/drivers/s390/kvm/kvm_virtio.c
+++ b/drivers/s390/kvm/kvm_virtio.c
@@ -82,13 +82,13 @@ static unsigned desc_size(const struct kvm_device_desc
*desc)
static u32 kvm_get_features(struct virtio_device *vdev)
{
unsigned int i;
- u32 features = 0;
+ u64 features = 0;
struct kvm_device_desc *desc = to_kvmdev(vdev)-desc;
u8 *in_features = kvm_vq_features(desc);
- for (i = 0; i min(desc-feature_len * 8, 32); i++)
+ for (i = 0; i min(desc-feature_len * 8, 64); i++)
if (in_features[i / 8] (1 (i % 8)))
- features |= (1 i);
+ features |= (1ull i);
return features;
}
diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
index efb35aa..52b24d7 100644
--- a/drivers/virtio/virtio.c
+++ b/drivers/virtio/virtio.c
@@ -112,7 +112,7 @@ static int virtio_dev_probe(struct device *_d)
struct virtio_device *dev = container_of(_d,struct virtio_device,dev);
struct virtio_driver *drv = container_of(dev-dev.driver,
struct virtio_driver, driver);
- u32 device_features;
+ u64 device_features;
/* We have a driver! */
add_status(dev, VIRTIO_CONFIG_S_DRIVER);
@@ -124,14 +124,14 @@ static int virtio_dev_probe(struct device *_d)
memset(dev-features, 0, sizeof(dev-features));
for (i = 0; i drv-feature_table_size; i++) {
unsigned int f = drv-feature_table[i];
- BUG_ON(f = 32);
- if (device_features (1 f))
+ BUG_ON(f = 64);
+ if (device_features (1ull f))
set_bit(f, dev-features);
}
/* Transport features always preserved to pass to finalize_features. */
for (i = VIRTIO_TRANSPORT_F_START; i VIRTIO_TRANSPORT_F_END; i++)
- if (device_features (1 i))
+ if (device_features (1ull i))
set_bit(i, dev-features);
dev-config-finalize_features(dev);
diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c
index 4fb5b2b..2a2ee72 100644
--- a/drivers/virtio/virtio_pci.c
+++ b/drivers/virtio/virtio_pci.c
@@ -44,6 +44,8 @@ struct virtio_pci_device
spinlock_t lock;
struct list_head virtqueues;
+ /* 64 bit features */
+ int features_hi;
/* MSI-X support */
int msix_enabled;
int intx_enabled;
@@ -103,26 +105,45 @@ static struct virtio_pci_device *to_vp_device(struct
virtio_device *vdev)
}
/* virtio config-get_features() implementation */
-static u32 vp_get_features(struct virtio_device *vdev)
+static u64 vp_get_features(struct virtio_device *vdev)
{
struct virtio_pci_device *vp_dev = to_vp_device(vdev);
+ u32 flo, fhi;
- /* When someone needs more than 32 feature bits, we'll need to
+ /* When someone needs more than 32 feature bits, we need to
* steal a bit to indicate that the rest are somewhere else. */
- return ioread32(vp_dev-ioaddr + VIRTIO_PCI_HOST_FEATURES);
+ flo = ioread32(vp_dev-ioaddr +
Hyper-V vmbus driver
Greg, Recently, you applied a patch-set from me that cleaned a bunch of architectural issues in the vmbus driver. With that patch-set, I think I have addressed all architectural issues that I am aware of. I was wondering if you would have the time to let me know what else would have to be addressed in the vmbus driver, before it could be considered ready for exiting staging. As always your help is greatly appreciated. Regards, K. Y ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
Re: Hyper-V vmbus driver
On Mon, Apr 11, 2011 at 06:46:24PM +, KY Srinivasan wrote: Greg, Recently, you applied a patch-set from me that cleaned a bunch of architectural issues in the vmbus driver. With that patch-set, I think I have addressed all architectural issues that I am aware of. I was wondering if you would have the time to let me know what else would have to be addressed in the vmbus driver, before it could be considered ready for exiting staging. As always your help is greatly appreciated. Hm, interesting word wrapping there, might I consider a real email client one of these days? :) Anyway, yes, I discussed this with Hank last week at the LF Collab summit. I'll look at the vmbus code later this week when I catch up on all of my other work (stable, usb, tty, staging, etc.) that has piled up during my 2 week absence, and get back to you with what I feel is still needed to be done, if anything. thanks, greg k-h ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
Re: [RFC][PATCH] virtio balloon: kill tell-host-first logic
On Mon, 2011-04-11 at 14:01 +0300, Michael S. Tsirkin wrote: On Thu, Apr 07, 2011 at 10:43:25AM -0700, Dave Hansen wrote: The virtio balloon driver has a VIRTIO_BALLOON_F_MUST_TELL_HOST feature bit. Whenever the bit is set, we must always tell the host before we free pages back to the allocator. Without this we might free a page (and have another user touch it) while the hypervisor is unprepared for it. But, if the bit is _not_ set, we are under no obligation to reverse the order. Furthermore, all modern qemus set this bit. Which qemus do this, specifically? Amit Shah just pointed out to me that upstream qemu.git and qemu-kvm.git don't seem to do this. I had a conversation with Anthony about it, and I think I managed to confuse myself somewhere. Just to be clear, all that I see in the qemu-kvm git tree right now (df85c051d780bca0ee2462cfeb8ef6d9552a19b0) is this: hw/virtio-balloon.h:#define VIRTIO_BALLOON_F_MUST_TELL_HOST 0 /* Tell before reclaiming pages */ Which qemu did you test this with? Probably a week or two old qemu-kvm. My changelog could probably use some work, but the patch still stands. The only requirement we have is that when VIRTIO_BALLOON_F_MUST_TELL_HOST is set we *MUST* tell the host, first. But, when it's not set, we can do whatever we want. So, we might as well always have the ...F_MUST_TELL_HOST behavior all the time. -- Dave ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
Re: [RFC][PATCH] virtio: 64 bit features
On Mon, 11 Apr 2011 19:55:25 +0300, Michael S. Tsirkin m...@redhat.com wrote: Extend features to 64 bit so we can use more transport bits. Future patches add two new feature bits which would exhaust the supply of transport feature bits, so let's add bit 31 to tell the guest that there are now 64 worth of features. For PCI this also changes the config layout. Signed-off-by: Michael S. Tsirkin m...@redhat.com This was pretty much the way I imagined it would work. We need a patch to the spec and a lot of cross testing... Thanks! Rusty. ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
Re: [RFC][PATCH] virtio balloon: kill tell-host-first logic
On (Mon) 11 Apr 2011 [15:11:11], Dave Hansen wrote: On Mon, 2011-04-11 at 14:01 +0300, Michael S. Tsirkin wrote: On Thu, Apr 07, 2011 at 10:43:25AM -0700, Dave Hansen wrote: The virtio balloon driver has a VIRTIO_BALLOON_F_MUST_TELL_HOST feature bit. Whenever the bit is set, we must always tell the host before we free pages back to the allocator. Without this we might free a page (and have another user touch it) while the hypervisor is unprepared for it. But, if the bit is _not_ set, we are under no obligation to reverse the order. Furthermore, all modern qemus set this bit. Which qemus do this, specifically? Amit Shah just pointed out to me that upstream qemu.git and qemu-kvm.git don't seem to do this. I had a conversation with Anthony about it, and I think I managed to confuse myself somewhere. Just to be clear, all that I see in the qemu-kvm git tree right now (df85c051d780bca0ee2462cfeb8ef6d9552a19b0) is this: hw/virtio-balloon.h:#define VIRTIO_BALLOON_F_MUST_TELL_HOST 0 /* Tell before reclaiming pages */ Which qemu did you test this with? Probably a week or two old qemu-kvm. My changelog could probably use some work, but the patch still stands. The only requirement we have is that when VIRTIO_BALLOON_F_MUST_TELL_HOST is set we *MUST* tell the host, first. But, when it's not set, we can do whatever we want. So, we might as well always have the ...F_MUST_TELL_HOST behavior all the time. Sure, the only contention was on the commit message, where you stated modern qemus set this... qemu doesn't, and it should. Care to do a patch for that? Thanks, Amit ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
