diff mbox

Assign the correct pci id range to virtio_pci

Message ID 1295ed070904280742m3171729al2b0e671c66aa7134@mail.gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

Pantelis Koukousoulas April 28, 2009, 2:42 p.m. UTC 
On Mon, Apr 27, 2009 at 2:56 PM, Avi Kivity <avi@redhat.com> wrote:
> Pantelis Koukousoulas wrote:
>>>
>>> Or maybe
>>>
>>>  modprobe virtio-pci claim=0x10f2 claim=0x10f7
>>>
>>
>> How about claim=0x10f2,0x10f7 instead so that it can be implemented as
>> a standard module array parameter?
>>
>
> Even better.

Ok, since a day has passed with no further comments, I 'll dare to
assume everyone
is happy with this solution. So, here is an implementation. I 've
tested locally with
my driver that uses 0x10f5 and it seems to work.

I am both attaching and inlining the patch because I 'm sure gmail
will mess it up
but I have no access to any other mailer at this time.

Feel free to rewrite any part that is too ugly.

Pantelis

From: Pantelis Koukousoulas <pktoss@gmail.com>
Date: Mon, 27 Apr 2009 20:49:20 +0300
Subject: [PATCH] Fix virtio_pci handling of PCI IDs

Currently virtio_pci does not claim the PCI IDs in the
"experimental range" 0x10f0-0x10ff. This means that
developers wanting to to the "right thing" and use
one of these IDs find that their drivers don't load.
In the end, this encourages developers to just hijack
an ID from the low end of the managed range (0x1000-0x103f).

Also, the choice of only claiming part of the available
managed range (0x1000-0x10ef) might seem arbitrary or
a typo to someone reading the code, since there is
no comment to explain/justify it.

After discussion of the problem in kvm-devel, this patch
attempts to fix these problems.

For the managed range we just add a comment to explain that
the reason for only claiming part of the range was
future-proofing against potential ABI breakage.

For the experimental range we implement a module parameter
to allow developers to claim the IDs they want individually.

E.g., to develop 2 virtio devices with IDs 0x10f3 and 0x10f5
you just add:

options virtio_pci claim=0x10f3,0x10f5

to e.g., /etc/modprobe.d/virtio.conf and you are set. This
way should also  be ABI breakage-proof while still allowing
private development of up to 16 devices by the same organization
simultaneously (i.e., the full experimental range).

Gerd Hoffmann suggested a module parameter and Avi Kivity
suggested the claim= syntax.

Signed-off-by: Pantelis Koukousoulas <pktoss@gmail.com>
---
 drivers/virtio/virtio_pci.c |   33 ++++++++++++++++++++++++++++++---
 1 files changed, 30 insertions(+), 3 deletions(-)

 		printk(KERN_ERR "virtio_pci: expected ABI version %d, got %d\n",

Comments

Gerd Hoffmann April 28, 2009, 3:19 p.m. UTC | #1 
Hi,

> Ok, since a day has passed with no further comments, I 'll dare to
> assume everyone is happy with this solution. So, here is an
> implementation. I 've tested locally with my driver that uses 0x10f5
> and it seems to work.

Patch looks fine to me.

cheers,
   Gerd

--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Pantelis Koukousoulas April 29, 2009, 7:47 a.m. UTC | #2 
On Tue, Apr 28, 2009 at 6:19 PM, Gerd Hoffmann <kraxel@redhat.com> wrote:
>  Hi,
>
>> Ok, since a day has passed with no further comments, I 'll dare to
>> assume everyone is happy with this solution. So, here is an
>> implementation. I 've tested locally with my driver that uses 0x10f5
>> and it seems to work.
>
> Patch looks fine to me.
>
> cheers,
>  Gerd

Cool, Avi, Rusty, what do you think ?

Pantelis
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

From daa4ba5078dd05f9e58d3f9a2327e5d60f345150 Mon Sep 17 00:00:00 2001
From: Pantelis Koukousoulas <pktoss@gmail.com>
Date: Mon, 27 Apr 2009 20:49:20 +0300
Subject: [PATCH] Fix virtio_pci handling of PCI IDs

Currently virtio_pci does not claim the PCI IDs in the
"experimental range" 0x10f0-0x10ff. This means that
developers wanting to to the "right thing" and use
one of these IDs find that their drivers don't load.
In the end, this encourages developers to just hijack
an ID from the low end of the managed range (0x1000-0x103f).

Also, the choice of only claiming part of the available
managed range (0x1000-0x10ef) might seem arbitrary or
a typo to someone reading the code, since there is
no comment to explain/justify it.

After discussion of the problem in kvm-devel, this patch
attempts to fix these problems.

For the managed range we just add a comment to explain that
the reason for only claiming part of the range was
future-proofing against potential ABI breakage.

For the experimental range we implement a module parameter
to allow developers to claim the IDs they want individually.

E.g., to develop 2 virtio devices with IDs 0x10f3 and 0x10f5
you just add:

options virtio_pci claim=0x10f3,0x10f5

to e.g., /etc/modprobe.d/virtio.conf and you are set. This
way should also  be ABI breakage-proof while still allowing
private development of up to 16 devices by the same organization
simultaneously (i.e., the full experimental range).

Gerd Hoffmann suggested a module parameter and Avi Kivity
suggested the claim= syntax.

Signed-off-by: Pantelis Koukousoulas <pktoss@gmail.com>
---
 drivers/virtio/virtio_pci.c |   33 ++++++++++++++++++++++++++++++---
 1 files changed, 30 insertions(+), 3 deletions(-)

diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c
index 330aacb..9337a1d 100644
--- a/drivers/virtio/virtio_pci.c
+++ b/drivers/virtio/virtio_pci.c
@@ -30,6 +30,10 @@  MODULE_DESCRIPTION("virtio-pci");
 MODULE_LICENSE("GPL");
 MODULE_VERSION("1");
 
+static int claim[16];
+module_param_array(claim, int, NULL, 0444);
+MODULE_PARM_DESC(claim, "Claimed PCI IDs in the 0x10f0-0x10ff range");
+
 /* Our device structure */
 struct virtio_pci_device
 {
@@ -318,6 +322,30 @@  static void virtio_pci_release_dev(struct device *_d)
 	kfree(vp_dev);
 }
 
+/*
+ * We only claim devices >= 0x1000 and <= 0x103f from the managed
+ * range: leave the rest. This allows for potential breaking of the
+ * ABI in the future. We also allow explicit selective claiming of
+ * IDs in the experimental range 0x10f0 - 0x10ff via a module param.
+ */
+static inline int is_valid_virtio_pci_id(short id)
+{
+	int i;
+
+	if (id < 0x1000 || (id > 0x103f && id < 0x10f0) || id > 0x10ff)
+		return 0;
+
+	if (id > 0x10f0) { /* 0x10f0 - 0x10ff case: experimental range id */
+		for (i = 0; i < ARRAY_SIZE(claim); i++) {
+			if (id == claim[i])
+				return 1;
+		}
+		return 0;
+	}
+
+	return 1;
+}
+
 /* the PCI probing function */
 static int __devinit virtio_pci_probe(struct pci_dev *pci_dev,
 				      const struct pci_device_id *id)
@@ -325,9 +353,8 @@  static int __devinit virtio_pci_probe(struct pci_dev *pci_dev,
 	struct virtio_pci_device *vp_dev;
 	int err;
 
-	/* We only own devices >= 0x1000 and <= 0x103f: leave the rest. */
-	if (pci_dev->device < 0x1000 || pci_dev->device > 0x103f)
-		return -ENODEV;
+	if (!is_valid_virtio_pci_id(pci_dev->device))
+	    return -ENODEV;
 
 	if (pci_dev->revision != VIRTIO_PCI_ABI_VERSION) {
 		printk(KERN_ERR "virtio_pci: expected ABI version %d, got %d\n",
-- 
1.5.6.3