[PATCH] Use PCI revision field to indicate virtio PCI ABI version

[PATCH] Use PCI revision field to indicate virtio PCI ABI version

[Anthony Liguori]

As Avi pointed out, as we continue to massage the virtio PCI ABI, we can make
things a little more friendly to users by utilizing the PCI revision field to
indicate which version of the ABI we're using.  This is a hard ABI version
and incrementing it will cause the guest driver to break.

This is the necessary changes to virtio_pci to support this.

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>

diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c
index 192687e..26f787d 100644
--- a/drivers/virtio/virtio_pci.c
+++ b/drivers/virtio/virtio_pci.c
@@ -311,6 +311,12 @@ static int __devinit virtio_pci_probe(struct pci_dev *pci_dev,
 	if (pci_dev->device < 0x1000 || pci_dev->device > 0x103f)
 		return -ENODEV;
 
+	if (pci_dev->revision != VIRTIO_PCI_ABI_VERSION) {
+		printk(KERN_ERR "virtio_pci: expected ABI version %d, got %d\n",
+		       VIRTIO_PCI_ABI_VERSION, pci_dev->revision);
+		return -ENODEV;
+	}
+
 	/* allocate our structure and fill it out */
 	vp_dev = kzalloc(sizeof(struct virtio_pci_device), GFP_KERNEL);
 	if (vp_dev == NULL)
diff --git a/include/linux/virtio_pci.h b/include/linux/virtio_pci.h
index 860eb37..b315165 100644
--- a/include/linux/virtio_pci.h
+++ b/include/linux/virtio_pci.h
@@ -52,4 +52,6 @@
  * configuration space */
 #define VIRTIO_PCI_CONFIG		20
 
+/* Virtio ABI version, this must match exactly */
+#define VIRTIO_PCI_ABI_VERSION		0
 #endif

Re: [PATCH] Use PCI revision field to indicate virtio PCI ABI version

[Rusty Russell]

On Tuesday 29 January 2008 02:59:59 Anthony Liguori wrote:
> As Avi pointed out, as we continue to massage the virtio PCI ABI, we can
> make things a little more friendly to users by utilizing the PCI revision
> field to indicate which version of the ABI we're using.  This is a hard ABI
> version and incrementing it will cause the guest driver to break.
>
> This is the necessary changes to virtio_pci to support this.
>
> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>

Applied, thanks.

Cheers,
Rusty.

Re: [PATCH] Use PCI revision field to indicate virtio PCI ABI version

[Avi Kivity]

Rusty Russell wrote:
> On Tuesday 29 January 2008 02:59:59 Anthony Liguori wrote:
>   
>> As Avi pointed out, as we continue to massage the virtio PCI ABI, we can
>> make things a little more friendly to users by utilizing the PCI revision
>> field to indicate which version of the ABI we're using.  This is a hard ABI
>> version and incrementing it will cause the guest driver to break.
>>
>> This is the necessary changes to virtio_pci to support this.
>>
>> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
>>     
>
> Applied, thanks.
>
>   

But that's done at the wrong level.  Anthony agreed the revision ID 
should indicate the device ABI, not just the virtio ABI.  If we move to 
that level, bumping just one device rev id on the host will break all 
devices on the guest.

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

Re: [PATCH] Use PCI revision field to indicate virtio PCI ABI version

[Rusty Russell]

On Tuesday 29 January 2008 19:03:26 Avi Kivity wrote:
> Rusty Russell wrote:
> > On Tuesday 29 January 2008 02:59:59 Anthony Liguori wrote:
> >> As Avi pointed out, as we continue to massage the virtio PCI ABI, we can
> >> make things a little more friendly to users by utilizing the PCI
> >> revision field to indicate which version of the ABI we're using.  This
> >> is a hard ABI version and incrementing it will cause the guest driver to
> >> break.
> >>
> >> This is the necessary changes to virtio_pci to support this.
> >>
> >> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> >
> > Applied, thanks.
>
> But that's done at the wrong level.  Anthony agreed the revision ID
> should indicate the device ABI, not just the virtio ABI.  If we move to
> that level, bumping just one device rev id on the host will break all
> devices on the guest.

OK, I'll drop it.

It's dumb anyway, since we can just change the ID if we want to break drivers.  
Having multiple ways of doing something we don't want to do seems silly.

Cheers,
Rusty.

Re: [PATCH] Use PCI revision field to indicate virtio PCI ABI version

[Anthony Liguori]

Rusty Russell wrote:
> On Tuesday 29 January 2008 19:03:26 Avi Kivity wrote:
>   
>> Rusty Russell wrote:
>>     
>>> On Tuesday 29 January 2008 02:59:59 Anthony Liguori wrote:
>>>       
>>>> As Avi pointed out, as we continue to massage the virtio PCI ABI, we can
>>>> make things a little more friendly to users by utilizing the PCI
>>>> revision field to indicate which version of the ABI we're using.  This
>>>> is a hard ABI version and incrementing it will cause the guest driver to
>>>> break.
>>>>
>>>> This is the necessary changes to virtio_pci to support this.
>>>>
>>>> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
>>>>         
>>> Applied, thanks.
>>>       
>> But that's done at the wrong level.  Anthony agreed the revision ID
>> should indicate the device ABI, not just the virtio ABI.  If we move to
>> that level, bumping just one device rev id on the host will break all
>> devices on the guest.
>>     
>
> OK, I'll drop it.
>   

Please pull in the patch again.

> It's dumb anyway, since we can just change the ID if we want to break drivers.  
> Having multiple ways of doing something we don't want to do seems silly.
>   

I considered that too but there's no point in burning through the device 
IDs.  This is what PCI revision IDs are meant for so we might as well 
use them.

Regards,

Anthony Liguori

> Cheers,
> Rusty.
>   

Re: [PATCH] Use PCI revision field to indicate virtio PCI ABI version

[Rusty Russell]

On Wednesday 30 January 2008 03:03:42 Anthony Liguori wrote:
> Rusty Russell wrote:
> > On Tuesday 29 January 2008 19:03:26 Avi Kivity wrote:
> >> But that's done at the wrong level.  Anthony agreed the revision ID
> >> should indicate the device ABI, not just the virtio ABI.  If we move to
> >> that level, bumping just one device rev id on the host will break all
> >> devices on the guest.
> >
> > OK, I'll drop it.
>
> Please pull in the patch again.

That's OK: I lied, I never dropped it :)

Cheers,
Rusty.

Re: [PATCH] Use PCI revision field to indicate virtio PCI ABI version

[Anthony Liguori]

Avi Kivity wrote:
> Rusty Russell wrote:
>> On Tuesday 29 January 2008 02:59:59 Anthony Liguori wrote:
>>  
>>> As Avi pointed out, as we continue to massage the virtio PCI ABI, we 
>>> can
>>> make things a little more friendly to users by utilizing the PCI 
>>> revision
>>> field to indicate which version of the ABI we're using.  This is a 
>>> hard ABI
>>> version and incrementing it will cause the guest driver to break.
>>>
>>> This is the necessary changes to virtio_pci to support this.
>>>
>>> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
>>>     
>>
>> Applied, thanks.
>>
>>   
>
> But that's done at the wrong level.  Anthony agreed the revision ID 
> should indicate the device ABI, not just the virtio ABI.  If we move 
> to that level, bumping just one device rev id on the host will break 
> all devices on the guest.

That's not what I was agreeing too.  I don't want to plumb an ABI 
interface through virtio for each device.  This is what I didn't like 
about having an ABI field in the first place.  I'm thinking we should 
just drop both of these and instead just rely on feature bits.

Regards,

Anthony Liguori

Re: [PATCH] Use PCI revision field to indicate virtio PCI ABI version

[Christian Borntraeger]

Am Dienstag, 29. Januar 2008 schrieb Anthony Liguori:
> That's not what I was agreeing too.  I don't want to plumb an ABI 
> interface through virtio for each device.  This is what I didn't like 
> about having an ABI field in the first place.  I'm thinking we should 
> just drop both of these and instead just rely on feature bits.

Me also updating the our prototype code to the latest levels...

And I agree with Anthony. Feature bits seems to be a much better solution 
than ABI versions.


Christian

Re: [PATCH] Use PCI revision field to indicate virtio PCI ABI version

[Avi Kivity]

Christian Borntraeger wrote:
> Am Dienstag, 29. Januar 2008 schrieb Anthony Liguori:
>   
>> That's not what I was agreeing too.  I don't want to plumb an ABI 
>> interface through virtio for each device.  This is what I didn't like 
>> about having an ABI field in the first place.  I'm thinking we should 
>> just drop both of these and instead just rely on feature bits.
>>     
>
> Me also updating the our prototype code to the latest levels...
>
> And I agree with Anthony. Feature bits seems to be a much better solution 
> than ABI versions.
>   

I agree that feature bits are the long term solution; but we need a 
short term solution before the ABI is stabilized.  We don't want to add 
feature bits now, since that will encode virtio development history into 
those bits (likely consuming most of them).


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

Re: [PATCH] Use PCI revision field to indicate virtio PCI ABI version

[Anthony Liguori]

Avi Kivity wrote:
> Christian Borntraeger wrote:
>> Am Dienstag, 29. Januar 2008 schrieb Anthony Liguori:
>>  
>>> That's not what I was agreeing too.  I don't want to plumb an ABI 
>>> interface through virtio for each device.  This is what I didn't 
>>> like about having an ABI field in the first place.  I'm thinking we 
>>> should just drop both of these and instead just rely on feature bits.
>>>     
>>
>> Me also updating the our prototype code to the latest levels...
>>
>> And I agree with Anthony. Feature bits seems to be a much better 
>> solution than ABI versions.
>>   
>
> I agree that feature bits are the long term solution; but we need a 
> short term solution before the ABI is stabilized.  We don't want to 
> add feature bits now, since that will encode virtio development 
> history into those bits (likely consuming most of them).

Well then let's stick with the current patches I put out.  It gives us a 
safe guard where we can gracefully break things.  I'm inclined to think 
that we should not bother just "breaking" a particular device but break 
all of them at once.  I don't want to overcomplicate something that we 
expect to never use.

Regards,

Anthony Liguori

Re: [PATCH] Use PCI revision field to indicate virtio PCI ABI version

[Avi Kivity]

Anthony Liguori wrote:
> Avi Kivity wrote:
>> Christian Borntraeger wrote:
>>> Am Dienstag, 29. Januar 2008 schrieb Anthony Liguori:
>>>  
>>>> That's not what I was agreeing too.  I don't want to plumb an ABI 
>>>> interface through virtio for each device.  This is what I didn't 
>>>> like about having an ABI field in the first place.  I'm thinking we 
>>>> should just drop both of these and instead just rely on feature bits.
>>>>     
>>>
>>> Me also updating the our prototype code to the latest levels...
>>>
>>> And I agree with Anthony. Feature bits seems to be a much better 
>>> solution than ABI versions.
>>>   
>>
>> I agree that feature bits are the long term solution; but we need a 
>> short term solution before the ABI is stabilized.  We don't want to 
>> add feature bits now, since that will encode virtio development 
>> history into those bits (likely consuming most of them).
>
> Well then let's stick with the current patches I put out.  It gives us 
> a safe guard where we can gracefully break things.  I'm inclined to 
> think that we should not bother just "breaking" a particular device 
> but break all of them at once.  I don't want to overcomplicate 
> something that we expect to never use.
>

Yeah, okay.  We'll bump it one last time when 2.6.25 is released.

I'll merge the userspace side.

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

Re: [PATCH] Use PCI revision field to indicate virtio PCI ABI version

[Christian Borntraeger]

Am Dienstag, 29. Januar 2008 schrieb Avi Kivity:
> Yeah, okay.  We'll bump it one last time when 2.6.25 is released.
> 
> I'll merge the userspace side.

Do we really want to freeze the interface for virtio already with 2.6.25? 
Maybe every possible issue is found then, but I would prefer to have a more
flexible schedule on finalizing virtio. Dont know if I am just paranoid...

Christian

Re: [PATCH] Use PCI revision field to indicate virtio PCI ABI version

[Avi Kivity]

Christian Borntraeger wrote:
> Am Dienstag, 29. Januar 2008 schrieb Avi Kivity:
>   
>> Yeah, okay.  We'll bump it one last time when 2.6.25 is released.
>>
>> I'll merge the userspace side.
>>     
>
> Do we really want to freeze the interface for virtio already with 2.6.25? 
> Maybe every possible issue is found then, but I would prefer to have a more
> flexible schedule on finalizing virtio. Dont know if I am just paranoid...
>   

We can always bump it again if we find out we suck.  But I hope three 
months ought to do it.

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