All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] Assign the correct pci id range to virtio_pci
@ 2009-04-24 11:17 Pantelis Koukousoulas
  2009-04-24 13:19 ` Anthony Liguori
  2009-04-26 10:36 ` Avi Kivity
  0 siblings, 2 replies; 20+ messages in thread
From: Pantelis Koukousoulas @ 2009-04-24 11:17 UTC (permalink / raw)
  To: kvm; +Cc: Pantelis Koukousoulas

According to the file pci-ids.txt in qemu sources, the range of PCI
device IDs assigned to virtio_pci is 0x1000 to 0x10ff, with a few
subranges that have different rules regarding who can get an ID
there and how.

Nevertheless, the full range should be assigned to the generic
virtio_pci driver, so that all corresponding devices, including
the experimental/unreleased ones "just work".
---
 drivers/virtio/virtio_pci.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c
index 330aacb..db3f3b5 100644
--- a/drivers/virtio/virtio_pci.c
+++ b/drivers/virtio/virtio_pci.c
@@ -325,8 +325,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)
+	/* We only own devices >= 0x1000 and <= 0x10ff: leave the rest. */
+	if (pci_dev->device < 0x1000 || pci_dev->device > 0x10ff)
 		return -ENODEV;
 
 	if (pci_dev->revision != VIRTIO_PCI_ABI_VERSION) {
-- 
1.5.6.3


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* Re: [PATCH] Assign the correct pci id range to virtio_pci
  2009-04-24 11:17 [PATCH] Assign the correct pci id range to virtio_pci Pantelis Koukousoulas
@ 2009-04-24 13:19 ` Anthony Liguori
       [not found]   ` <49F55DD1.8020506@redhat.com>
  2009-04-26 10:36 ` Avi Kivity
  1 sibling, 1 reply; 20+ messages in thread
From: Anthony Liguori @ 2009-04-24 13:19 UTC (permalink / raw)
  To: Pantelis Koukousoulas; +Cc: kvm, Gerd Hoffmann

Pantelis Koukousoulas wrote:
> According to the file pci-ids.txt in qemu sources, the range of PCI
> device IDs assigned to virtio_pci is 0x1000 to 0x10ff, with a few
> subranges that have different rules regarding who can get an ID
> there and how.
>
> Nevertheless, the full range should be assigned to the generic
> virtio_pci driver, so that all corresponding devices, including
> the experimental/unreleased ones "just work".
>   

Gerd, since you appear to be managing the PCI ID range, what do you think?

Regards,

Anthony Liguori


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH] Assign the correct pci id range to virtio_pci
  2009-04-24 11:17 [PATCH] Assign the correct pci id range to virtio_pci Pantelis Koukousoulas
  2009-04-24 13:19 ` Anthony Liguori
@ 2009-04-26 10:36 ` Avi Kivity
  2009-04-26 12:44   ` Pantelis Koukousoulas
  1 sibling, 1 reply; 20+ messages in thread
From: Avi Kivity @ 2009-04-26 10:36 UTC (permalink / raw)
  To: Pantelis Koukousoulas; +Cc: kvm

Pantelis Koukousoulas wrote:
> According to the file pci-ids.txt in qemu sources, the range of PCI
> device IDs assigned to virtio_pci is 0x1000 to 0x10ff, with a few
> subranges that have different rules regarding who can get an ID
> there and how.
>
> Nevertheless, the full range should be assigned to the generic
> virtio_pci driver, so that all corresponding devices, including
> the experimental/unreleased ones "just work".
>   

Please copy the virtio maintainer (Rusty Russell 
<rusty@rustcorp.com.au>) on virtio guest patches.

-- 
error compiling committee.c: too many arguments to function


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH] Assign the correct pci id range to virtio_pci
  2009-04-26 10:36 ` Avi Kivity
@ 2009-04-26 12:44   ` Pantelis Koukousoulas
  2009-04-26 12:49     ` Avi Kivity
  0 siblings, 1 reply; 20+ messages in thread
From: Pantelis Koukousoulas @ 2009-04-26 12:44 UTC (permalink / raw)
  To: Avi Kivity; +Cc: kvm

> Please copy the virtio maintainer (Rusty Russell <rusty@rustcorp.com.au>) on
> virtio guest patches.

Well, for now the issue is whether my understanding of qemu/pci-ids.txt and the
comment in virtio_pci.c that both say that the full 0x1000 - 0x10ff range of PCI
device IDs is donated for virtio_pci devices is correct.

If that is true, virtio_pci only claiming 0x1000 - 0x103f doesn't make
much sense to me
and looks more like a typo, because there is no explicit justification
(perhaps in a comment) either.
(3f does not even show up in pci-ids.txt).

The ranges mentioned there are:

1000 -> 10ef (one needs to contact Gerd to reserve an unallocated ID
in that range)
and
10f0 -> 10ff  (available for experimental devices, a random ID in that
range can be
                     used during private development without asking
anyone as long as
                     you are not shipping anything using it)

the range ef -> f0 (exclusive) is reserved.

>From the above, my understanding is that virtio_pci should definitely
claim at least
00 -> ef and most likely it should claim f0->ff too. The only reason
not to claim some
IDs is to allow someone to have virtio PCI devices that do *not* use
the virtio_pci
infrastructure but why would we want that?

The reason I asked here (I guess qemu-devel would be just as relevant or more,
but it has more traffic) is because Anthony is the author of
virtio_pci.c (at least it looks like it)
so hopefully he knows if that 3f was a typo or not and Gerd is responsible for
the PCI ID namespace management so he knows if pci-ids.txt is correct or not.

Once this issue is clarified I 'm happy to resend the same or an
improved version
of the patch as appropriate.

Thanks,
Pantelis

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH] Assign the correct pci id range to virtio_pci
  2009-04-26 12:44   ` Pantelis Koukousoulas
@ 2009-04-26 12:49     ` Avi Kivity
  2009-04-26 23:49       ` Rusty Russell
  0 siblings, 1 reply; 20+ messages in thread
From: Avi Kivity @ 2009-04-26 12:49 UTC (permalink / raw)
  To: Pantelis Koukousoulas; +Cc: kvm, Rusty Russell, Anthony Liguori

Pantelis Koukousoulas wrote:
>> Please copy the virtio maintainer (Rusty Russell <rusty@rustcorp.com.au>) on
>> virtio guest patches.
>>     
>
> Well, for now the issue is whether my understanding of qemu/pci-ids.txt and the
> comment in virtio_pci.c that both say that the full 0x1000 - 0x10ff range of PCI
> device IDs is donated for virtio_pci devices is correct.
>   

0x1000-0x10ff is correct.  I don't know where the 0x103f came from.  Rusty?

> If that is true, virtio_pci only claiming 0x1000 - 0x103f doesn't make
> much sense to me
> and looks more like a typo, because there is no explicit justification
> (perhaps in a comment) either.
> (3f does not even show up in pci-ids.txt).
>
> The ranges mentioned there are:
>
> 1000 -> 10ef (one needs to contact Gerd to reserve an unallocated ID
> in that range)
> and
> 10f0 -> 10ff  (available for experimental devices, a random ID in that
> range can be
>                      used during private development without asking
> anyone as long as
>                      you are not shipping anything using it)
>
> the range ef -> f0 (exclusive) is reserved.
>
> From the above, my understanding is that virtio_pci should definitely
> claim at least
> 00 -> ef and most likely it should claim f0->ff too. The only reason
> not to claim some
> IDs is to allow someone to have virtio PCI devices that do *not* use
> the virtio_pci
> infrastructure but why would we want that?
>   

We wouldn't.  If it isn't a virtio-pci device, it should leech an ID 
from someone else.

> The reason I asked here (I guess qemu-devel would be just as relevant or more,
> but it has more traffic) is because Anthony is the author of
> virtio_pci.c (at least it looks like it)
>   

Added cc.

> so hopefully he knows if that 3f was a typo or not and Gerd is responsible for
> the PCI ID namespace management so he knows if pci-ids.txt is correct or not.
>
> Once this issue is clarified I 'm happy to resend the same or an
> improved version
> of the patch as appropriate.
>   


-- 
error compiling committee.c: too many arguments to function


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH] Assign the correct pci id range to virtio_pci
  2009-04-26 12:49     ` Avi Kivity
@ 2009-04-26 23:49       ` Rusty Russell
  2009-04-27  0:44         ` Anthony Liguori
  0 siblings, 1 reply; 20+ messages in thread
From: Rusty Russell @ 2009-04-26 23:49 UTC (permalink / raw)
  To: Avi Kivity; +Cc: Pantelis Koukousoulas, kvm, Anthony Liguori

On Sun, 26 Apr 2009 10:19:16 pm Avi Kivity wrote:
> Pantelis Koukousoulas wrote:
> >> Please copy the virtio maintainer (Rusty Russell <rusty@rustcorp.com.au>) on
> >> virtio guest patches.
> >>     
> >
> > Well, for now the issue is whether my understanding of qemu/pci-ids.txt and the
> > comment in virtio_pci.c that both say that the full 0x1000 - 0x10ff range of PCI
> > device IDs is donated for virtio_pci devices is correct.
> >   
> 
> 0x1000-0x10ff is correct.  I don't know where the 0x103f came from.  Rusty?

We decided to hedge our bets in case we broke the ABI.

AFAICT there's no reason to claim the full range until we need it.  Wake me
when device #32 is used :)

Rusty.

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH] Assign the correct pci id range to virtio_pci
  2009-04-26 23:49       ` Rusty Russell
@ 2009-04-27  0:44         ` Anthony Liguori
  2009-04-27  3:23           ` Pantelis Koukousoulas
  0 siblings, 1 reply; 20+ messages in thread
From: Anthony Liguori @ 2009-04-27  0:44 UTC (permalink / raw)
  To: Rusty Russell; +Cc: Avi Kivity, Pantelis Koukousoulas, kvm

Rusty Russell wrote:
> On Sun, 26 Apr 2009 10:19:16 pm Avi Kivity wrote:
>   
>> Pantelis Koukousoulas wrote:
>>     
>>>> Please copy the virtio maintainer (Rusty Russell <rusty@rustcorp.com.au>) on
>>>> virtio guest patches.
>>>>     
>>>>         
>>> Well, for now the issue is whether my understanding of qemu/pci-ids.txt and the
>>> comment in virtio_pci.c that both say that the full 0x1000 - 0x10ff range of PCI
>>> device IDs is donated for virtio_pci devices is correct.
>>>   
>>>       
>> 0x1000-0x10ff is correct.  I don't know where the 0x103f came from.  Rusty?
>>     
>
> We decided to hedge our bets in case we broke the ABI.
>
> AFAICT there's no reason to claim the full range until we need it.  Wake me
> when device #32 is used :)
>   
Would be good to at least include the "experiment range" in case people 
are making third-party virtio modules and want to play around without 
replacing virtio-{pci,*}.

Regards,

Anthony Liguori

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


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH] Assign the correct pci id range to virtio_pci
  2009-04-27  0:44         ` Anthony Liguori
@ 2009-04-27  3:23           ` Pantelis Koukousoulas
  2009-05-04  1:53             ` Rusty Russell
  0 siblings, 1 reply; 20+ messages in thread
From: Pantelis Koukousoulas @ 2009-04-27  3:23 UTC (permalink / raw)
  To: Anthony Liguori; +Cc: Rusty Russell, Avi Kivity, kvm

On Mon, Apr 27, 2009 at 3:44 AM, Anthony Liguori <anthony@codemonkey.ws> wrote:
> Rusty Russell wrote:
>>
>> On Sun, 26 Apr 2009 10:19:16 pm Avi Kivity wrote:
>>>
>>> 0x1000-0x10ff is correct.  I don't know where the 0x103f came from.
>>>  Rusty?
>>>
>>
>> We decided to hedge our bets in case we broke the ABI.
>>
>> AFAICT there's no reason to claim the full range until we need it.  Wake
>> me
>> when device #32 is used :)
>>
>
> Would be good to at least include the "experiment range" in case people are
> making third-party virtio modules and want to play around without replacing
> virtio-{pci,*}.

I 'd be happy with a simple comment explaining the 0x103f (e.g.,
/* Not yet using the full 0x1000 - 0x10ef to hedge our bets in case we
broke the ABI.*/
as explained above)

plus including the experimental range as Anthony proposed.

The reason I came across this was I was playing with such a simple "third party"
module and after reading pci-ids.txt I decided to choose 0x10f5 for myself
only to find out that virtio_pci (and therefore my driver too) would
not load any more.

Thanks,
Pantelis

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH] Assign the correct pci id range to virtio_pci
       [not found]   ` <49F55DD1.8020506@redhat.com>
@ 2009-04-27  8:39     ` Pantelis Koukousoulas
  2009-04-27  9:01       ` Gerd Hoffmann
  0 siblings, 1 reply; 20+ messages in thread
From: Pantelis Koukousoulas @ 2009-04-27  8:39 UTC (permalink / raw)
  To: Gerd Hoffmann; +Cc: Anthony Liguori, kvm

> I'd suggest to exclude the experimental range by default (enable via module
> parameter) to make clear it isn't for regular use.

Module parameter on what? The module parameter parsing code is afaict
provided by the end-driver (e.g., virtio-net) which only speaks virtio and has
no idea there is an actual PCI device in the backend.

Isn't it easier to just make it clear that a PCI id within the 0x1000-10ef range
is a prerequisite for inclusion in mainline linux / qemu and leave it at that?

Btw, including just a subset of the experimental range like e.g.,
0x10f0-0x10f3 would
be fine with me, if there is a desire to be compatible with the "allow for
breaking the ABI" rationale for the 0x1000-0x103f range.

Thanks,
Pantelis

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH] Assign the correct pci id range to virtio_pci
  2009-04-27  8:39     ` Pantelis Koukousoulas
@ 2009-04-27  9:01       ` Gerd Hoffmann
  2009-04-27  9:11         ` Pantelis Koukousoulas
  0 siblings, 1 reply; 20+ messages in thread
From: Gerd Hoffmann @ 2009-04-27  9:01 UTC (permalink / raw)
  To: Pantelis Koukousoulas; +Cc: Anthony Liguori, kvm

On 04/27/09 10:39, Pantelis Koukousoulas wrote:
>> I'd suggest to exclude the experimental range by default (enable via module
>> parameter) to make clear it isn't for regular use.
>
> Module parameter on what?

virtio_pci.ko

Then you'll do something like this ...

echo "options virtio_pci experimental=1" >> /etc/modprobe.d/virtio.conf

... to enable the experimental IDs.

> The module parameter parsing code is afaict
> provided by the end-driver (e.g., virtio-net) which only speaks virtio and has
> no idea there is an actual PCI device in the backend.

virtio_pci is a separate module and can have parameters too.

cheers,
   Gerd


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH] Assign the correct pci id range to virtio_pci
  2009-04-27  9:01       ` Gerd Hoffmann
@ 2009-04-27  9:11         ` Pantelis Koukousoulas
  2009-04-27  9:16           ` Gerd Hoffmann
  0 siblings, 1 reply; 20+ messages in thread
From: Pantelis Koukousoulas @ 2009-04-27  9:11 UTC (permalink / raw)
  To: Gerd Hoffmann; +Cc: Anthony Liguori, kvm

>> Module parameter on what?
>
> virtio_pci.ko
>
> Then you'll do something like this ...
>
> echo "options virtio_pci experimental=1" >> /etc/modprobe.d/virtio.conf
>
> ... to enable the experimental IDs.

Ok, I can sure live with such an arrangement :)
Should the entire 0x10f0-0x10ff range be enabled by this option or
just a subset?
(and if a subset, how large, 4 IDs, more ?)

Thanks,
Pantelis

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH] Assign the correct pci id range to virtio_pci
  2009-04-27  9:11         ` Pantelis Koukousoulas
@ 2009-04-27  9:16           ` Gerd Hoffmann
  2009-04-27  9:24             ` Avi Kivity
  0 siblings, 1 reply; 20+ messages in thread
From: Gerd Hoffmann @ 2009-04-27  9:16 UTC (permalink / raw)
  To: Pantelis Koukousoulas; +Cc: Anthony Liguori, kvm

On 04/27/09 11:11, Pantelis Koukousoulas wrote:
>>> Module parameter on what?
>> virtio_pci.ko
>>
>> Then you'll do something like this ...
>>
>> echo "options virtio_pci experimental=1">>  /etc/modprobe.d/virtio.conf
>>
>> ... to enable the experimental IDs.
>
> Ok, I can sure live with such an arrangement :)
> Should the entire 0x10f0-0x10ff range be enabled by this option or
> just a subset?
> (and if a subset, how large, 4 IDs, more ?)

That roughly matches the number of non-experimental IDs (1/4 of the 
complete range), so 4 IDs looks good to me.

cheers,
   Gerd


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH] Assign the correct pci id range to virtio_pci
  2009-04-27  9:16           ` Gerd Hoffmann
@ 2009-04-27  9:24             ` Avi Kivity
  2009-04-27 11:45               ` Pantelis Koukousoulas
  0 siblings, 1 reply; 20+ messages in thread
From: Avi Kivity @ 2009-04-27  9:24 UTC (permalink / raw)
  To: Gerd Hoffmann; +Cc: Pantelis Koukousoulas, Anthony Liguori, kvm

Gerd Hoffmann wrote:
> On 04/27/09 11:11, Pantelis Koukousoulas wrote:
>>>> Module parameter on what?
>>> virtio_pci.ko
>>>
>>> Then you'll do something like this ...
>>>
>>> echo "options virtio_pci experimental=1">>  /etc/modprobe.d/virtio.conf
>>>
>>> ... to enable the experimental IDs.
>>
>> Ok, I can sure live with such an arrangement :)
>> Should the entire 0x10f0-0x10ff range be enabled by this option or
>> just a subset?
>> (and if a subset, how large, 4 IDs, more ?)
>
> That roughly matches the number of non-experimental IDs (1/4 of the 
> complete range), so 4 IDs looks good to me.

Or maybe

   modprobe virtio-pci claim=0x10f2 claim=0x10f7

-- 
error compiling committee.c: too many arguments to function


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH] Assign the correct pci id range to virtio_pci
  2009-04-27  9:24             ` Avi Kivity
@ 2009-04-27 11:45               ` Pantelis Koukousoulas
  2009-04-27 11:56                 ` Avi Kivity
  0 siblings, 1 reply; 20+ messages in thread
From: Pantelis Koukousoulas @ 2009-04-27 11:45 UTC (permalink / raw)
  To: Avi Kivity; +Cc: Gerd Hoffmann, Anthony Liguori, kvm

> 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?

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH] Assign the correct pci id range to virtio_pci
  2009-04-27 11:45               ` Pantelis Koukousoulas
@ 2009-04-27 11:56                 ` Avi Kivity
  2009-04-28 14:42                   ` Pantelis Koukousoulas
  0 siblings, 1 reply; 20+ messages in thread
From: Avi Kivity @ 2009-04-27 11:56 UTC (permalink / raw)
  To: Pantelis Koukousoulas; +Cc: Gerd Hoffmann, Anthony Liguori, kvm

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.

-- 
error compiling committee.c: too many arguments to function


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH] Assign the correct pci id range to virtio_pci
  2009-04-27 11:56                 ` Avi Kivity
@ 2009-04-28 14:42                   ` Pantelis Koukousoulas
  2009-04-28 15:19                     ` Gerd Hoffmann
  0 siblings, 1 reply; 20+ messages in thread
From: Pantelis Koukousoulas @ 2009-04-28 14:42 UTC (permalink / raw)
  To: kvm; +Cc: Gerd Hoffmann, Anthony Liguori, Avi Kivity, Rusty Russell

[-- Attachment #1: Type: text/plain, Size: 4328 bytes --]

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(-)

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

[-- Attachment #2: 0001-Fix-virtio_pci-handling-of-PCI-IDs.patch --]
[-- Type: text/x-diff, Size: 3543 bytes --]

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


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* Re: [PATCH] Assign the correct pci id range to virtio_pci
  2009-04-28 14:42                   ` Pantelis Koukousoulas
@ 2009-04-28 15:19                     ` Gerd Hoffmann
  2009-04-29  7:47                       ` Pantelis Koukousoulas
  0 siblings, 1 reply; 20+ messages in thread
From: Gerd Hoffmann @ 2009-04-28 15:19 UTC (permalink / raw)
  To: Pantelis Koukousoulas; +Cc: kvm, Anthony Liguori, Avi Kivity, Rusty Russell

   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


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH] Assign the correct pci id range to virtio_pci
  2009-04-28 15:19                     ` Gerd Hoffmann
@ 2009-04-29  7:47                       ` Pantelis Koukousoulas
  0 siblings, 0 replies; 20+ messages in thread
From: Pantelis Koukousoulas @ 2009-04-29  7:47 UTC (permalink / raw)
  To: Gerd Hoffmann; +Cc: kvm, Anthony Liguori, Avi Kivity, Rusty Russell

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

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH] Assign the correct pci id range to virtio_pci
  2009-04-27  3:23           ` Pantelis Koukousoulas
@ 2009-05-04  1:53             ` Rusty Russell
  2009-05-04  2:32               ` Pantelis Koukousoulas
  0 siblings, 1 reply; 20+ messages in thread
From: Rusty Russell @ 2009-05-04  1:53 UTC (permalink / raw)
  To: Pantelis Koukousoulas; +Cc: Anthony Liguori, Avi Kivity, kvm

On Mon, 27 Apr 2009 12:53:25 pm Pantelis Koukousoulas wrote:
> On Mon, Apr 27, 2009 at 3:44 AM, Anthony Liguori <anthony@codemonkey.ws> wrote:
> > Would be good to at least include the "experiment range" in case people are
> > making third-party virtio modules and want to play around without replacing
> > virtio-{pci,*}.
> 
> I 'd be happy with a simple comment explaining the 0x103f (e.g.,
> /* Not yet using the full 0x1000 - 0x10ef to hedge our bets in case we
> broke the ABI.*/
> as explained above)

Thanks, I like your patch.

Where did this idea of "experimental" range come from, BTW?  I prefer your
module cmdline approach, as it discourages deployment with such numbers.

Thanks,
Rusty.

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH] Assign the correct pci id range to virtio_pci
  2009-05-04  1:53             ` Rusty Russell
@ 2009-05-04  2:32               ` Pantelis Koukousoulas
  0 siblings, 0 replies; 20+ messages in thread
From: Pantelis Koukousoulas @ 2009-05-04  2:32 UTC (permalink / raw)
  To: Rusty Russell; +Cc: Anthony Liguori, Avi Kivity, kvm

[-- Attachment #1: Type: text/plain, Size: 663 bytes --]

>> I 'd be happy with a simple comment explaining the 0x103f (e.g.,
>> /* Not yet using the full 0x1000 - 0x10ef to hedge our bets in case we
>> broke the ABI.*/
>> as explained above)
>
> Thanks, I like your patch.
>
> Where did this idea of "experimental" range come from, BTW?

In the qemu sources, there is a file pci-ids.txt that documents the
PCI ID rules.
I 'm attaching it for your convenience.

> I prefer your module cmdline approach, as it discourages
> deployment with such numbers.

Great, I like this way better too, because it allows using the full experimental
range (16 IDs) while also allowing for breaking the virtio_pci ABI.

Thanks,
Pantelis

[-- Attachment #2: pci-ids.txt --]
[-- Type: text/plain, Size: 883 bytes --]


PCI IDs for qemu
================

Red Hat, Inc. donates a part of its device ID range to qemu, to be used for
virtual devices.  The vendor ID is 1af4 (formerly Qumranet ID).

The 1000 -> 10ff device ID range is used for VirtIO devices.

The 1100 device ID is used as PCI Subsystem ID for existing hardware
devices emulated by qemu.

All other device IDs are reserved.


VirtIO Device IDs
-----------------

1af4:1000  network device
1af4:1001  block device
1af4:1002  balloon device
1af4:1003  console device

1af4:1004  Reserved.
   to      Contact Gerd Hoffmann <kraxel@redhat.com> to get a
1af4:10ef  device ID assigned for your new virtio device.

1af4:10f0  Available for experimental usage without registration.  Must get
   to      official ID when the code leaves the test lab (i.e. when seeking
1af4:10ff  upstream merge or shipping a distro/product) to avoid conflicts.


^ permalink raw reply	[flat|nested] 20+ messages in thread

end of thread, other threads:[~2009-05-04  2:32 UTC | newest]

Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2009-04-24 11:17 [PATCH] Assign the correct pci id range to virtio_pci Pantelis Koukousoulas
2009-04-24 13:19 ` Anthony Liguori
     [not found]   ` <49F55DD1.8020506@redhat.com>
2009-04-27  8:39     ` Pantelis Koukousoulas
2009-04-27  9:01       ` Gerd Hoffmann
2009-04-27  9:11         ` Pantelis Koukousoulas
2009-04-27  9:16           ` Gerd Hoffmann
2009-04-27  9:24             ` Avi Kivity
2009-04-27 11:45               ` Pantelis Koukousoulas
2009-04-27 11:56                 ` Avi Kivity
2009-04-28 14:42                   ` Pantelis Koukousoulas
2009-04-28 15:19                     ` Gerd Hoffmann
2009-04-29  7:47                       ` Pantelis Koukousoulas
2009-04-26 10:36 ` Avi Kivity
2009-04-26 12:44   ` Pantelis Koukousoulas
2009-04-26 12:49     ` Avi Kivity
2009-04-26 23:49       ` Rusty Russell
2009-04-27  0:44         ` Anthony Liguori
2009-04-27  3:23           ` Pantelis Koukousoulas
2009-05-04  1:53             ` Rusty Russell
2009-05-04  2:32               ` Pantelis Koukousoulas

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.